search

efabless / caravel_board

32 stars 41 forks source link

Updated housekeeping scripts for chipignite firmware #98

Closed glingy closed 2 months ago

glingy commented 9 months ago

These are updates to the housekeeping SPI scripts to make them a little more modular and easier to understand.

Reference https://open-source-silicon.slack.com/archives/C016UL7AQ73/p1706031837367009

algofoogle commented 4 months ago

Thanks for this! This is a good structural improvement and natural abstraction. I've discussed this with @RTimothyEdwards and I'm in the process of a code review now. This is mostly good to go as-is, and will be a good basis for other planned improvements.

glingy commented 4 months ago

We've actually gone long past where this pr is in the scripts we're using internally (we've got some custom hkspi implementations using the abstraction here, etc), I'm not sure if I'm going to have time to go back and understand what I was doing and update the pr. If someone has the time to edit it to get it merged in, go for it.

algofoogle commented 4 months ago

No worries @glingy . I appreciate you going to the trouble in the first place. I will make the respective changes myself. Hopefully you could answer a couple of quick questions, though:

  1. Do you agree that the extra byte on the end of write_reg is probably unintentional?
  2. To your knowledge, could setting cs_count=1 instead of =2 have any unintended side-effect on the state of the pins, generally? I don't know FTDI chips and PyFTDI well enough.
RTimothyEdwards commented 4 months ago

@algofoogle : I assume that cs_count is the number of individual chip selects; multiple SPI interfaces can be tied together as long as every chip select is independent and only one chip select is active at a time. The FTDI supports multiple chip select pins, I think on specific upper bits of the same port that the normal FTDI SPI is defined on. You'll note that a few lines down there's the line spi.get_port(cs=0). There was some version of the Caravel board (an early one) in which we used two chip select lines, and the housekeeping SPI would only work with spi.get_port(cs=1) (it might even be earlier than the Caravel boards---I did a similar setup with a housekeeping SPI as far back as the Raven/Ravenna/Raptor chips and boards.)

algofoogle commented 4 months ago

There was some version of the Caravel board (an early one) in which we used two chip select lines

Understood. Thanks @RTimothyEdwards! I'll keep the cs_count=1 change, then, as this seems sensible.

algofoogle commented 4 months ago

@RTimothyEdwards @glingy I've made the discussed changes, to restore consistency with the UART-enable behaviour of the original scripts, if either of you would care to review. Should be about ready to merge.