use csr::virtual_leds for SFP0..3 LED indication #244

Merged
sb10q merged 10 commits from morgan/artiq-zynq:feature into master 2023-08-28 16:08:11 +08:00
4 changed files with 174 additions and 60 deletions

View File

@ -10,6 +10,7 @@ from migen.genlib.cdc import MultiReg
from migen_axi.integration.soc_core import SoCCore from migen_axi.integration.soc_core import SoCCore
from migen_axi.platforms import kasli_soc from migen_axi.platforms import kasli_soc
from misoc.interconnect.csr import * from misoc.interconnect.csr import *
from misoc.cores import virtual_leds
from misoc.integration import cpu_interface from misoc.integration import cpu_interface
from artiq.coredevice import jsondesc from artiq.coredevice import jsondesc
@ -86,6 +87,8 @@ class GenericStandalone(SoCCore):
self.acpki = acpki self.acpki = acpki
self.rustc_cfg = dict() self.rustc_cfg = dict()
self.rustc_cfg["hw_rev"] = description["hw_rev"]
platform = kasli_soc.Platform() platform = kasli_soc.Platform()
platform.toolchain.bitstream_commands.extend([ platform.toolchain.bitstream_commands.extend([
"set_property BITSTREAM.GENERAL.COMPRESS True [current_design]", "set_property BITSTREAM.GENERAL.COMPRESS True [current_design]",
@ -178,6 +181,8 @@ class GenericMaster(SoCCore):
self.acpki = acpki self.acpki = acpki
self.rustc_cfg = dict() self.rustc_cfg = dict()
self.rustc_cfg["hw_rev"] = description["hw_rev"]
platform = kasli_soc.Platform() platform = kasli_soc.Platform()
platform.toolchain.bitstream_commands.extend([ platform.toolchain.bitstream_commands.extend([
"set_property BITSTREAM.GENERAL.COMPRESS True [current_design]", "set_property BITSTREAM.GENERAL.COMPRESS True [current_design]",
@ -305,6 +310,14 @@ class GenericMaster(SoCCore):
self.add_csr_group("grabber", self.grabber_csr_group) self.add_csr_group("grabber", self.grabber_csr_group)
self.submodules.virtual_leds = virtual_leds.VirtualLeds()
self.csr_devices.append("virtual_leds")
self.comb += [self.virtual_leds.get(i).eq(channel.rx_ready)
for i, channel in enumerate(self.gt_drtio.channels)]
class GenericSatellite(SoCCore): class GenericSatellite(SoCCore):
def __init__(self, description, acpki=False): def __init__(self, description, acpki=False):
clk_freq = description["rtio_frequency"] clk_freq = description["rtio_frequency"]
@ -312,6 +325,8 @@ class GenericSatellite(SoCCore):
self.acpki = acpki self.acpki = acpki
self.rustc_cfg = dict() self.rustc_cfg = dict()
self.rustc_cfg["hw_rev"] = description["hw_rev"]
platform = kasli_soc.Platform() platform = kasli_soc.Platform()
platform.toolchain.bitstream_commands.extend([ platform.toolchain.bitstream_commands.extend([
"set_property BITSTREAM.GENERAL.COMPRESS True [current_design]", "set_property BITSTREAM.GENERAL.COMPRESS True [current_design]",
@ -467,6 +482,12 @@ class GenericSatellite(SoCCore):
self.add_csr_group("grabber", self.grabber_csr_group) self.add_csr_group("grabber", self.grabber_csr_group)
# no RTIO CRG here # no RTIO CRG here
self.submodules.virtual_leds = virtual_leds.VirtualLeds()
self.csr_devices.append("virtual_leds")
self.comb += [self.virtual_leds.get(i).eq(channel.rx_ready)
for i, channel in enumerate(self.gt_drtio.channels)]
Outdated
Review

needs update

needs update
def write_mem_file(soc, filename): def write_mem_file(soc, filename):
with open(filename, "w") as f: with open(filename, "w") as f:

View File

@ -1,6 +1,8 @@
use libboard_zynq::i2c; use libboard_zynq::i2c;
use log::info; use log::info;
use crate::pl::csr;
// Only the bare minimum registers. Bits/IO connections equivalent between IC types. // Only the bare minimum registers. Bits/IO connections equivalent between IC types.
struct Registers { struct Registers {
// PCA9539 equivalent register names in comments // PCA9539 equivalent register names in comments
@ -10,23 +12,49 @@ struct Registers {
gpiob: u8, // Output Port 1 gpiob: u8, // Output Port 1
} }
pub struct IoExpander<'a> { //IO expanders pins
i2c: &'a mut i2c::I2c, const IODIR_OUT_SFP_TX_DISABLE: u8 = 0x02;
morgan marked this conversation as resolved Outdated
Outdated
Review

Doesn't seem like this constant adds anything to the code.

Doesn't seem like this constant adds anything to the code.
const IODIR_OUT_SFP_LED: u8 = 0x40;
morgan marked this conversation as resolved Outdated
Outdated
Review

Move the inversions into the iodir definition.

Move the inversions into the iodir definition.
#[cfg(hw_rev = "v1.0")]
const IODIR_OUT_SFP0_LED: u8 = 0x40;
#[cfg(hw_rev = "v1.1")]
const IODIR_OUT_SFP0_LED: u8 = 0x80;
//IO expander port direction
const IODIR0: [u8; 2] = [
0xFF & !IODIR_OUT_SFP_TX_DISABLE & !IODIR_OUT_SFP0_LED,
morgan marked this conversation as resolved Outdated
Outdated
Review

It's just called "iodir", not "mapping"

It's just called "iodir", not "mapping"
0xFF & !IODIR_OUT_SFP_TX_DISABLE & !IODIR_OUT_SFP_LED,
];
const IODIR1: [u8; 2] = [
0xFF & !IODIR_OUT_SFP_TX_DISABLE & !IODIR_OUT_SFP_LED,
0xFF & !IODIR_OUT_SFP_TX_DISABLE & !IODIR_OUT_SFP_LED,
];
pub struct IoExpander {
address: u8, address: u8,
virtual_led_mapping: &'static [(u8, u8, u8)],
iodir: [u8; 2], iodir: [u8; 2],
out_current: [u8; 2], out_current: [u8; 2],
out_target: [u8; 2], out_target: [u8; 2],
registers: Registers, registers: Registers,
} }
impl<'a> IoExpander<'a> { impl IoExpander {
morgan marked this conversation as resolved Outdated
Outdated
Review

What is the purpose of that channel field?

What is the purpose of that channel field?
pub fn new(i2c: &'a mut i2c::I2c, index: u8) -> Result<Self, &'static str> { pub fn new(i2c: &mut i2c::I2c, index: u8) -> Result<Self, &'static str> {
#[cfg(hw_rev = "v1.0")]
const VIRTUAL_LED_MAPPING0: [(u8, u8, u8); 2] = [(0, 0, 6), (1, 1, 6)];
#[cfg(hw_rev = "v1.1")]
const VIRTUAL_LED_MAPPING0: [(u8, u8, u8); 2] = [(0, 0, 7), (1, 1, 6)];
const VIRTUAL_LED_MAPPING1: [(u8, u8, u8); 2] = [(2, 0, 6), (3, 1, 6)];
// Both expanders on SHARED I2C bus // Both expanders on SHARED I2C bus
let mut io_expander = match index { let mut io_expander = match index {
0 => IoExpander { 0 => IoExpander {
i2c,
address: 0x40, address: 0x40,
iodir: [0xff; 2], virtual_led_mapping: &VIRTUAL_LED_MAPPING0,
iodir: IODIR0,
out_current: [0; 2], out_current: [0; 2],
out_target: [0; 2], out_target: [0; 2],
registers: Registers { registers: Registers {
@ -37,9 +65,9 @@ impl<'a> IoExpander<'a> {
}, },
}, },
1 => IoExpander { 1 => IoExpander {
i2c,
address: 0x42, address: 0x42,
iodir: [0xff; 2], virtual_led_mapping: &VIRTUAL_LED_MAPPING1,
iodir: IODIR1,
out_current: [0; 2], out_current: [0; 2],
out_target: [0; 2], out_target: [0; 2],
registers: Registers { registers: Registers {
@ -51,7 +79,7 @@ impl<'a> IoExpander<'a> {
}, },
_ => return Err("incorrect I/O expander index"), _ => return Err("incorrect I/O expander index"),
}; };
if !io_expander.check_ack()? { if !io_expander.check_ack(i2c)? {
info!("MCP23017 io expander {} not found. Checking for PCA9539.", index); info!("MCP23017 io expander {} not found. Checking for PCA9539.", index);
io_expander.address += 0xa8; // translate to PCA9539 addresses (see schematic) io_expander.address += 0xa8; // translate to PCA9539 addresses (see schematic)
io_expander.registers = Registers { io_expander.registers = Registers {
@ -60,57 +88,58 @@ impl<'a> IoExpander<'a> {
gpioa: 0x02, gpioa: 0x02,
gpiob: 0x03, gpiob: 0x03,
}; };
if !io_expander.check_ack()? { if !io_expander.check_ack(i2c)? {
return Err("Neither MCP23017 nor PCA9539 io expander found."); return Err("Neither MCP23017 nor PCA9539 io expander found.");
}; };
} }
Ok(io_expander) Ok(io_expander)
} }
fn select(&mut self) -> Result<(), &'static str> { fn select(&self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
self.i2c.pca954x_select(0x70, None)?; i2c.pca954x_select(0x70, None)?;
self.i2c.pca954x_select(0x71, Some(3))?; i2c.pca954x_select(0x71, Some(3))?;
Ok(()) Ok(())
} }
fn write(&mut self, addr: u8, value: u8) -> Result<(), &'static str> { fn write(&self, i2c: &mut i2c::I2c, addr: u8, value: u8) -> Result<(), &'static str> {
self.i2c.start()?; i2c.start()?;
self.i2c.write(self.address)?; i2c.write(self.address)?;
self.i2c.write(addr)?; i2c.write(addr)?;
self.i2c.write(value)?; i2c.write(value)?;
self.i2c.stop()?; i2c.stop()?;
Ok(()) Ok(())
} }
fn check_ack(&mut self) -> Result<bool, &'static str> { fn check_ack(&self, i2c: &mut i2c::I2c) -> Result<bool, &'static str> {
// Check for ack from io expander // Check for ack from io expander
self.select()?; self.select(i2c)?;
self.i2c.start()?; i2c.start()?;
let ack = self.i2c.write(self.address)?; let ack = i2c.write(self.address)?;
self.i2c.stop()?; i2c.stop()?;
Ok(ack) Ok(ack)
} }
fn update_iodir(&mut self) -> Result<(), &'static str> { fn update_iodir(&self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
self.write(self.registers.iodira, self.iodir[0])?; self.write(i2c, self.registers.iodira, self.iodir[0])?;
self.write(self.registers.iodirb, self.iodir[1])?; self.write(i2c, self.registers.iodirb, self.iodir[1])?;
Ok(()) Ok(())
} }
pub fn init(&mut self) -> Result<(), &'static str> { pub fn init(&mut self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
self.select()?; self.select(i2c)?;
self.update_iodir()?;
self.update_iodir(i2c)?;
self.out_current[0] = 0x00; self.out_current[0] = 0x00;
self.write(self.registers.gpioa, 0x00)?; self.write(i2c, self.registers.gpioa, 0x00)?;
self.out_current[1] = 0x00; self.out_current[1] = 0x00;
self.write(self.registers.gpiob, 0x00)?; self.write(i2c, self.registers.gpiob, 0x00)?;
Ok(()) Ok(())
} }
pub fn set_oe(&mut self, port: u8, outputs: u8) -> Result<(), &'static str> { pub fn set_oe(&mut self, i2c: &mut i2c::I2c, port: u8, outputs: u8) -> Result<(), &'static str> {
self.iodir[port as usize] &= !outputs; self.iodir[port as usize] &= !outputs;
self.update_iodir()?; self.update_iodir(i2c)?;
morgan marked this conversation as resolved Outdated
Outdated
Review

Aren't you doing that already at object creation above?

Aren't you doing that already at object creation above?
Ok(()) Ok(())
} }
@ -122,15 +151,20 @@ impl<'a> IoExpander<'a> {
} }
} }
pub fn service(&mut self) -> Result<(), &'static str> { pub fn service(&mut self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
for (led, port, bit) in self.virtual_led_mapping.iter() {
let level = unsafe { csr::virtual_leds::status_read() >> led & 1 };
self.set(*port, *bit, level != 0);
}
if self.out_target != self.out_current { if self.out_target != self.out_current {
self.select()?; self.select(i2c)?;
if self.out_target[0] != self.out_current[0] { if self.out_target[0] != self.out_current[0] {
self.write(self.registers.gpioa, self.out_target[0])?; self.write(i2c, self.registers.gpioa, self.out_target[0])?;
self.out_current[0] = self.out_target[0]; self.out_current[0] = self.out_target[0];
} }
if self.out_target[1] != self.out_current[1] { if self.out_target[1] != self.out_current[1] {
self.write(self.registers.gpiob, self.out_target[1])?; self.write(i2c, self.registers.gpiob, self.out_target[1])?;
self.out_current[1] = self.out_target[1]; self.out_current[1] = self.out_target[1];
} }
} }

View File

@ -12,6 +12,8 @@
#[macro_use] #[macro_use]
extern crate alloc; extern crate alloc;
use core::cell::RefCell;
use libasync::{block_async, task}; use libasync::{block_async, task};
#[cfg(feature = "target_kasli_soc")] #[cfg(feature = "target_kasli_soc")]
use libboard_artiq::io_expander; use libboard_artiq::io_expander;
@ -102,6 +104,25 @@ async fn report_async_rtio_errors() {
} }
} }
#[cfg(feature = "target_kasli_soc")]
async fn io_expanders_service(
i2c_bus: RefCell<&mut libboard_zynq::i2c::I2c>,
io_expander0: RefCell<io_expander::IoExpander>,
morgan marked this conversation as resolved Outdated
Outdated
Review

The only unsafe line is the next one, no?

The only unsafe line is the next one, no?

LAST_VIRTUAL_LED_STATUS is a static mut. So it need an unsafe too

```LAST_VIRTUAL_LED_STATUS``` is a static mut. So it need an unsafe too
Outdated
Review

I don't think this should be gated on has_drtio.

I don't think this should be gated on has_drtio.
io_expander1: RefCell<io_expander::IoExpander>,
) {
morgan marked this conversation as resolved Outdated

Could save a CSR call by getting the value before the if

Could save a CSR call by getting the value before the ``if``
loop {
task::r#yield().await;
io_expander0
.borrow_mut()
.service(&mut i2c_bus.borrow_mut())
Outdated
Review

I don't think this entire block_async is any useful. It also duplicates logic and breaks abstraction layers. Anyway just remove it.

I don't think this entire block_async is any useful. It also duplicates logic and breaks abstraction layers. Anyway just remove it.

The block_async prevent "io_expanders_service" from locking up the whole cpu. As it blocks the loop from running over and over again.

The block_async prevent "io_expanders_service" from locking up the whole cpu. As it blocks the loop from running over and over again.
.expect("I2C I/O expander #0 service failed");
io_expander1
.borrow_mut()
.service(&mut i2c_bus.borrow_mut())
morgan marked this conversation as resolved Outdated
Outdated
Review

async_ in the name sounds redundant since this is declared ``async fn

``async_`` in the name sounds redundant since this is declared ``async fn
.expect("I2C I/O expander #1 service failed");
}
Outdated
Review

It's confusing to create IO expander objects, which also should be singletons and do more than controlling LEDs, into a function whose name indicates it is about LEDs.

It's confusing to create IO expander objects, which also should be singletons and do more than controlling LEDs, into a function whose name indicates it is about LEDs.
Outdated
Review

As I told you before, those I/O expander objects should be created only once in the entire firmware.

Additionally, you create them once with i2c::I2C_BUS and once with &mut *i2c_ptr, which is inconsistent.

As I told you before, those I/O expander objects should be created only once in the entire firmware. Additionally, you create them once with ``i2c::I2C_BUS`` and once with ``&mut *i2c_ptr``, which is inconsistent.
}
morgan marked this conversation as resolved Outdated
Outdated
Review

Just inline it.

 block_async!(|| {
   let current = unsafe { pl::csr::virtual_leds::status_read() };
   ...
Just inline it. ``` block_async!(|| { let current = unsafe { pl::csr::virtual_leds::status_read() }; ... ```
static mut LOG_BUFFER: [u8; 1 << 17] = [0; 1 << 17]; static mut LOG_BUFFER: [u8; 1 << 17] = [0; 1 << 17];
morgan marked this conversation as resolved Outdated

Debug leftover? Remove pls

Debug leftover? Remove pls
#[no_mangle] #[no_mangle]
@ -122,20 +143,26 @@ pub fn main_core0() {
info!("gateware ident: {}", identifier_read(&mut [0; 64])); info!("gateware ident: {}", identifier_read(&mut [0; 64]));
i2c::init(); i2c::init();
let i2c_bus = unsafe { (i2c::I2C_BUS).as_mut().unwrap() };
let (mut io_expander0, mut io_expander1);
#[cfg(feature = "target_kasli_soc")] #[cfg(feature = "target_kasli_soc")]
{ {
let i2c = unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }; io_expander0 = io_expander::IoExpander::new(i2c_bus, 0).unwrap();
for expander_i in 0..=1 { io_expander1 = io_expander::IoExpander::new(i2c_bus, 1).unwrap();
let mut io_expander = io_expander::IoExpander::new(i2c, expander_i).unwrap(); io_expander0
io_expander.init().expect("I2C I/O expander #0 initialization failed"); .init(i2c_bus)
// Actively drive TX_DISABLE to false on SFP0..3 .expect("I2C I/O expander #0 initialization failed");
io_expander.set_oe(0, 1 << 1).unwrap(); io_expander1
io_expander.set_oe(1, 1 << 1).unwrap(); .init(i2c_bus)
io_expander.set(0, 1, false); .expect("I2C I/O expander #1 initialization failed");
io_expander.set(1, 1, false); // Actively drive TX_DISABLE to false on SFP0..3
io_expander.service().unwrap(); io_expander0.set(0, 1, false);
} io_expander1.set(0, 1, false);
io_expander0.set(1, 1, false);
io_expander1.set(1, 1, false);
io_expander0.service(i2c_bus).unwrap();
io_expander1.service(i2c_bus).unwrap();
} }
let cfg = match Config::new() { let cfg = match Config::new() {
@ -150,5 +177,11 @@ pub fn main_core0() {
task::spawn(report_async_rtio_errors()); task::spawn(report_async_rtio_errors());
#[cfg(feature = "target_kasli_soc")]
task::spawn(io_expanders_service(
RefCell::new(i2c_bus),
RefCell::new(io_expander0),
RefCell::new(io_expander1),
));
comms::main(timer, cfg); comms::main(timer, cfg);
} }

View File

@ -611,18 +611,25 @@ pub extern "C" fn main_core0() -> i32 {
let mut i2c = I2c::i2c0(); let mut i2c = I2c::i2c0();
i2c.init().expect("I2C initialization failed"); i2c.init().expect("I2C initialization failed");
let (mut io_expander0, mut io_expander1);
#[cfg(feature = "target_kasli_soc")] #[cfg(feature = "target_kasli_soc")]
{ {
for expander_i in 0..=1 { io_expander0 = io_expander::IoExpander::new(&mut i2c, 0).unwrap();
let mut io_expander = io_expander::IoExpander::new(&mut i2c, expander_i).unwrap(); io_expander1 = io_expander::IoExpander::new(&mut i2c, 1).unwrap();
io_expander.init().expect("I2C I/O expander #0 initialization failed"); io_expander0
// Actively drive TX_DISABLE to false on SFP0..3 .init(&mut i2c)
io_expander.set_oe(0, 1 << 1).unwrap(); .expect("I2C I/O expander #0 initialization failed");
io_expander.set_oe(1, 1 << 1).unwrap(); io_expander1
io_expander.set(0, 1, false); .init(&mut i2c)
io_expander.set(1, 1, false); .expect("I2C I/O expander #1 initialization failed");
io_expander.service().unwrap(); // Actively drive TX_DISABLE to false on SFP0..3
} io_expander0.set(0, 1, false);
io_expander1.set(0, 1, false);
io_expander0.set(1, 1, false);
io_expander1.set(1, 1, false);
io_expander0.service(&mut i2c).unwrap();
io_expander1.service(&mut i2c).unwrap();
} }
#[cfg(has_si5324)] #[cfg(has_si5324)]
@ -664,6 +671,16 @@ pub extern "C" fn main_core0() -> i32 {
for mut rep in repeaters.iter_mut() { for mut rep in repeaters.iter_mut() {
rep.service(&routing_table, rank, &mut timer); rep.service(&routing_table, rank, &mut timer);
} }
#[cfg(feature = "target_kasli_soc")]
{
io_expander0
.service(&mut i2c)
.expect("I2C I/O expander #0 service failed");
io_expander1
.service(&mut i2c)
.expect("I2C I/O expander #1 service failed");
}
hardware_tick(&mut hardware_tick_ts, &mut timer); hardware_tick(&mut hardware_tick_ts, &mut timer);
} }
@ -700,6 +717,15 @@ pub extern "C" fn main_core0() -> i32 {
for mut rep in repeaters.iter_mut() { for mut rep in repeaters.iter_mut() {
rep.service(&routing_table, rank, &mut timer); rep.service(&routing_table, rank, &mut timer);
} }
#[cfg(feature = "target_kasli_soc")]
{
io_expander0
.service(&mut i2c)
.expect("I2C I/O expander #0 service failed");
io_expander1
.service(&mut i2c)
.expect("I2C I/O expander #1 service failed");
}
hardware_tick(&mut hardware_tick_ts, &mut timer); hardware_tick(&mut hardware_tick_ts, &mut timer);
if drtiosat_tsc_loaded() { if drtiosat_tsc_loaded() {
info!("TSC loaded from uplink"); info!("TSC loaded from uplink");