M-Labs/artiq-zynq
10
1
Fork 20
Code Issues 13 Pull Requests 1 Releases Wiki Activity

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
Conversation 0 Commits 10 Files Changed 4 +125 -104
3 changed files with 125 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f814a2006f - Show all commits

105
src/libboard_artiq/src/io_expander.rs
View File

@ -13,38 +13,35 @@ struct Registers {
}
//IO expanders pins
const IO_DIR_INPUT_ALL: u8 = 0xFF;
const IO_DIR_OUT_SFP_TX_DISABLE: u8 = !0x02;
const IO_DIR_OUT_SFP_LED: u8 = !0x40;
const IODIR_OUT_SFP_TX_DISABLE: u8 = 0x02;
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:45:26 +08:00
Outdated
Review
Copy Link

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
sb10q commented 2023-08-04 15:46:27 +08:00
Outdated
Review
Copy Link

Move the inversions into the iodir definition.

Move the inversions into the iodir definition.
#[cfg(hw_rev = "v1.0")]
const IO_DIR_OUT_SFP0_LED: u8 = !0x40;
const IODIR_OUT_SFP0_LED: u8 = 0x40;
#[cfg(hw_rev = "v1.1")]
const IO_DIR_OUT_SFP0_LED: u8 = !0x80;
const IODIR_OUT_SFP0_LED: u8 = 0x80;
//IO expander port direction
const IO_DIR_MAPPING0: [u8; 2] = [
IO_DIR_INPUT_ALL & IO_DIR_OUT_SFP_TX_DISABLE & (IO_DIR_OUT_SFP0_LED),
IO_DIR_INPUT_ALL & IO_DIR_OUT_SFP_TX_DISABLE & IO_DIR_OUT_SFP_LED,
const IODIR0: [u8; 2] = [
0xFF & !IODIR_OUT_SFP_TX_DISABLE & !IODIR_OUT_SFP0_LED,
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:46:02 +08:00
Outdated
Review
Copy Link

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 IO_DIR_MAPPING1: [u8; 2] = [
IO_DIR_INPUT_ALL & IO_DIR_OUT_SFP_TX_DISABLE & IO_DIR_OUT_SFP_LED,
IO_DIR_INPUT_ALL & IO_DIR_OUT_SFP_TX_DISABLE & IO_DIR_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<'a> {
i2c: &'a mut i2c::I2c,
pub struct IoExpander {
address: u8,
virtual_led_mapping: &'static [(u8, u8, u8)],
iodir: [u8; 2],
out_current: [u8; 2],
out_target: [u8; 2],
registers: Registers,
virtual_led_mapping: &'static [(u8, u8, u8)],
channel: usize,
}
impl<'a> IoExpander<'a> {
pub fn new(i2c: &'a mut i2c::I2c, index: u8) -> Result<Self, &'static str> {
impl IoExpander {
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:48:01 +08:00
Outdated
Review
Copy Link

What is the purpose of that channel field?

What is the purpose of that channel field?
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")]
@ -55,11 +52,9 @@ impl<'a> IoExpander<'a> {
// Both expanders on SHARED I2C bus
let mut io_expander = match index {
0 => IoExpander {
i2c,
channel: 0,
address: 0x40,
virtual_led_mapping: &VIRTUAL_LED_MAPPING0,
iodir: IO_DIR_MAPPING0,
iodir: IODIR0,
out_current: [0; 2],
out_target: [0; 2],
registers: Registers {
@ -70,11 +65,9 @@ impl<'a> IoExpander<'a> {
},
},
1 => IoExpander {
i2c,
channel: 1,
address: 0x42,
virtual_led_mapping: &VIRTUAL_LED_MAPPING1,
iodir: IO_DIR_MAPPING1,
iodir: IODIR1,
out_current: [0; 2],
out_target: [0; 2],
registers: Registers {
@ -86,7 +79,7 @@ impl<'a> IoExpander<'a> {
},
_ => 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);
io_expander.address += 0xa8; // translate to PCA9539 addresses (see schematic)
io_expander.registers = Registers {
@ -95,64 +88,58 @@ impl<'a> IoExpander<'a> {
gpioa: 0x02,
gpiob: 0x03,
};
if !io_expander.check_ack()? {
if !io_expander.check_ack(i2c)? {
return Err("Neither MCP23017 nor PCA9539 io expander found.");
};
}
Ok(io_expander)
}
fn select(&mut self) -> Result<(), &'static str> {
self.i2c.pca954x_select(0x70, None)?;
self.i2c.pca954x_select(0x71, Some(3))?;
fn select(&self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
i2c.pca954x_select(0x70, None)?;
i2c.pca954x_select(0x71, Some(3))?;
Ok(())
}
fn write(&mut self, addr: u8, value: u8) -> Result<(), &'static str> {
self.i2c.start()?;
self.i2c.write(self.address)?;
self.i2c.write(addr)?;
self.i2c.write(value)?;
self.i2c.stop()?;
fn write(&self, i2c: &mut i2c::I2c, addr: u8, value: u8) -> Result<(), &'static str> {
i2c.start()?;
i2c.write(self.address)?;
i2c.write(addr)?;
i2c.write(value)?;
i2c.stop()?;
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
self.select()?;
self.i2c.start()?;
let ack = self.i2c.write(self.address)?;
self.i2c.stop()?;
self.select(i2c)?;
i2c.start()?;
let ack = i2c.write(self.address)?;
i2c.stop()?;
Ok(ack)
}
fn update_iodir(&mut self) -> Result<(), &'static str> {
self.write(self.registers.iodira, self.iodir[0])?;
self.write(self.registers.iodirb, self.iodir[1])?;
fn update_iodir(&self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
self.write(i2c, self.registers.iodira, self.iodir[0])?;
self.write(i2c, self.registers.iodirb, self.iodir[1])?;
Ok(())
}
pub fn init(&mut self) -> Result<(), &'static str> {
self.select()?;
pub fn init(&mut self, i2c: &mut i2c::I2c) -> Result<(), &'static str> {
self.select(i2c)?;
self.iodir = match self.channel {
0 => IO_DIR_MAPPING0,
1 => IO_DIR_MAPPING1,
_ => [IO_DIR_INPUT_ALL; 2],
};
self.update_iodir()?;
self.update_iodir(i2c)?;
self.out_current[0] = 0x00;
self.write(self.registers.gpioa, 0x00)?;
self.write(i2c, self.registers.gpioa, 0x00)?;
self.out_current[1] = 0x00;
self.write(self.registers.gpiob, 0x00)?;
self.write(i2c, self.registers.gpiob, 0x00)?;
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.update_iodir()?;
self.update_iodir(i2c)?;
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:47:50 +08:00
Outdated
Review
Copy Link

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

Aren't you doing that already at object creation above?
Ok(())
}
@ -164,20 +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 {
self.select()?;
self.select(i2c)?;
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];
}
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];
}
}

79
src/runtime/src/main.rs
View File

@ -12,6 +12,8 @@
#[macro_use]
extern crate alloc;
use core::cell::RefCell;
use libasync::{block_async, task};
#[cfg(feature = "target_kasli_soc")]
use libboard_artiq::io_expander;
@ -106,25 +108,33 @@ async fn report_async_rtio_errors() {
static mut LAST_VIRTUAL_LED_STATUS: u8 = 0;
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:41:03 +08:00
Outdated
Review
Copy Link

The only unsafe line is the next one, no?

The only unsafe line is the next one, no?
morgan commented 2023-08-04 15:48:54 +08:00
Outdated
Review
Copy Link

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
sb10q commented 2023-08-13 14:48:09 +08:00
Outdated
Review
Copy Link

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

I don't think this should be gated on has_drtio.
fn wait_for_virtual_leds_change() -> nb::Result<(), Void> {
unsafe {
let current = pl::csr::virtual_leds::status_read();
if current != LAST_VIRTUAL_LED_STATUS {
LAST_VIRTUAL_LED_STATUS = current;
Ok(())
} else {
Err(nb::Error::WouldBlock)
}
}
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_io_expanders_service() {
let mut io_expander0 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 1).unwrap();
async fn io_expanders_service(
i2c_bus: RefCell<&mut libboard_zynq::i2c::I2c>,
morgan marked this conversation as resolved Outdated
mwojcik commented 2023-08-01 16:35:29 +08:00
Outdated
Review
Copy Link

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

Could save a CSR call by getting the value before the ``if``
io_expander0: RefCell<io_expander::IoExpander>,
io_expander1: RefCell<io_expander::IoExpander>,
) {
loop {
let _ = block_async!(wait_for_virtual_leds_change()).await;
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
let _ = block_async!((|| -> nb::Result<(), Void> {
sb10q commented 2023-08-13 14:50:38 +08:00
Outdated
Review
Copy Link

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.
morgan commented 2023-08-15 11:28:26 +08:00
Outdated
Review
Copy Link

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.
unsafe {
let current = pl::csr::virtual_leds::status_read();
if current != LAST_VIRTUAL_LED_STATUS {
LAST_VIRTUAL_LED_STATUS = current;
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:41:40 +08:00
Outdated
Review
Copy Link

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

``async_`` in the name sounds redundant since this is declared ``async fn
Ok(())
} else {
sb10q commented 2023-08-02 17:48:13 +08:00
Outdated
Review
Copy Link

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.
sb10q commented 2023-08-04 15:53:53 +08:00
Outdated
Review
Copy Link

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.
Err(nb::Error::WouldBlock)
}
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:42:28 +08:00
Outdated
Review
Copy Link

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() }; ... ```
}
})())
morgan marked this conversation as resolved Outdated
mwojcik commented 2023-08-01 16:32:15 +08:00
Outdated
Review
Copy Link

Debug leftover? Remove pls

Debug leftover? Remove pls
.await;
io_expander0
.borrow_mut()
.service(&mut i2c_bus.borrow_mut())
.expect("I2C I/O expander #0 service failed");
io_expander1
.borrow_mut()
.service(&mut i2c_bus.borrow_mut())
.expect("I2C I/O expander #1 service failed");
}
}
@ -148,18 +158,26 @@ pub fn main_core0() {
info!("gateware ident: {}", identifier_read(&mut [0; 64]));
i2c::init();
let i2c_bus = unsafe { (i2c::I2C_BUS).as_mut().unwrap() };
let (mut io_expander0, mut io_expander1);
#[cfg(feature = "target_kasli_soc")]
{
let i2c = unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() };
for expander_i in 0..=1 {
let mut io_expander = io_expander::IoExpander::new(i2c, expander_i).unwrap();
io_expander.init().expect("I2C I/O expander #0 initialization failed");
// Actively drive TX_DISABLE to false on SFP0..3
io_expander.set(0, 1, false);
io_expander.set(1, 1, false);
io_expander.service().unwrap();
}
io_expander0 = io_expander::IoExpander::new(i2c_bus, 0).unwrap();
io_expander1 = io_expander::IoExpander::new(i2c_bus, 1).unwrap();
io_expander0
.init(i2c_bus)
.expect("I2C I/O expander #0 initialization failed");
io_expander1
.init(i2c_bus)
.expect("I2C I/O expander #1 initialization failed");
// Actively drive TX_DISABLE to false on SFP0..3
io_expander0.set(0, 1, false);
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:50:33 +08:00
Outdated
Review
Copy Link

As you can see this is called report_async_rtio_errors, not async_report_async_rtio_errors.

Asynchronous RTIO errors have nothing to do with async/await.

As you can see this is called ``report_async_rtio_errors``, not ``async_report_async_rtio_errors``. Asynchronous RTIO errors have nothing to do with async/await.
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() {
@ -175,7 +193,10 @@ pub fn main_core0() {
task::spawn(report_async_rtio_errors());
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
task::spawn(async_io_expanders_service());
task::spawn(io_expanders_service(
RefCell::new(i2c_bus),
RefCell::new(io_expander0),
RefCell::new(io_expander1),
));
comms::main(timer, cfg);
}

45
src/satman/src/main.rs
View File

@ -610,18 +610,26 @@ pub extern "C" fn main_core0() -> i32 {
ram::init_alloc_core0();
let mut i2c = I2c::i2c0();
let i2c_ptr = &mut i2c as *mut I2c;
i2c.init().expect("I2C initialization failed");
let (mut io_expander0, mut io_expander1);
#[cfg(feature = "target_kasli_soc")]
{
for expander_i in 0..=1 {
let mut io_expander = io_expander::IoExpander::new(&mut i2c, expander_i).unwrap();
io_expander.init().expect("I2C I/O expander #0 initialization failed");
// Actively drive TX_DISABLE to false on SFP0..3
io_expander.set(0, 1, false);
io_expander.set(1, 1, false);
io_expander.service().unwrap();
}
io_expander0 = io_expander::IoExpander::new(&mut i2c, 0).unwrap();
io_expander1 = io_expander::IoExpander::new(&mut i2c, 1).unwrap();
io_expander0
.init(&mut i2c)
.expect("I2C I/O expander #0 initialization failed");
io_expander1
.init(&mut i2c)
.expect("I2C I/O expander #1 initialization failed");
// 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)]
@ -656,9 +664,6 @@ pub extern "C" fn main_core0() -> i32 {
let mut hardware_tick_ts = 0;
let mut io_expander0 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 1).unwrap();
loop {
while !drtiosat_link_rx_up() {
drtiosat_process_errors();
@ -668,8 +673,12 @@ pub extern "C" fn main_core0() -> i32 {
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
{
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
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);
@ -710,8 +719,12 @@ pub extern "C" fn main_core0() -> i32 {
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
{
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
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);
if drtiosat_tsc_loaded() {