Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

From: Joel Holdsworth
Date: Mon Nov 07 2016 - 13:49:56 EST

Hi Marek,

Thanks again for your comments.

On 07/11/16 11:01, Marek Vasut wrote:
On 11/07/2016 03:49 AM, Joel Holdsworth wrote:
The Lattice iCE40 is a family of FPGAs with a minimalistic architecture
and very regular structure, designed for low-cost, high-volume consumer
and system applications.


+static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
+ const char *buf, size_t count)
+ struct ice40_fpga_priv *priv = mgr->priv;
+ struct spi_device *dev = priv->dev;
+ struct spi_message message;
+ struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1,
+ .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY};

Should be this way for the sake of readability, fix globally:

struct spi_transfer assert_cs_then_reset_delay = {
.cs_change = 1,

Sure ok. Personally, I prefer it to be concise, but I'm happy to accept the norms.

Also I believe this could be const.

It doesn't work const - I tried it. spi_message_add_tail() expects it to be non-const. Looking at 'struct spi_transfer' it appears there is various bits of state used to perform the transfer, as well as the pointer to the next item in the single-linked list.

+ struct spi_transfer housekeeping_delay_then_release_cs = {

Don't we have some less hacky way of toggling the nCS ? Is this even nCS
or is this some control pin of the FPGA ? Maybe it should be treated
like a regular GPIO instead ?

I've been round this loop before also. drivers/spi/spi.c has a static function 'static void spi_set_cs(struct spi_device *dev, bool enable)'. It manipulates the CS line of devices where CS is built into the SPI master, and manipulates the GPIO on other devices.

I don't know why it's non-public - I tried to get an answer from the SPI folks, but didn't get one. I guess they don't want to encourage drivers to manually manipulate the CS line - because SPI transfers are usually meant to be interruptible by higher priority transfers to other devices at any time. The only reason it's legit for me to manually manipulate CS is because I first lock the bus.

Previously I had a copy of spi_set_cs copy-pasted into my driver, but in the end I decided to replace that with the zero-length transfers because there's a danger that if the original spi_set_cs() gets rewritten some time, my copy-paste code would leave around some nasty legacy.

On the whole, I don't think the zero-length transfers are too egregiously bad, and all the alternatives seem worse to me.

+ const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,};

The comma is not needed.

True. I'll make that change.

+ /* Check board setup data. */
+ if (spi->max_speed_hz > 25000000) {
+ dev_err(dev, "Speed is too high\n");
+ return -EINVAL;
+ }
+ if (spi->max_speed_hz < 1000000) {
+ dev_err(dev, "Speed is too low\n");
+ return -EINVAL;
+ }

Do you have some explanation for this limitation ?

Not really no.

The 'DS1040 - iCE40 LP/HX Family Data Sheet' page 3-18 claims f_max for Slave SPI Writing is >=1MHz && <=25MHz.

The exact reason I don't know.

Are they running a PLL off the clock? What if the clock is really jittery - it seems to work fine when I've tested it with bit-banged SPI on the RPi; just as well as with hardware SPI.

Or is it something to do with some pre-commit on-chip firmware storage? e.g. to check the CRC. Does the storage buffer have some time limitation before it gets committed to the FPGA core?

I'm not sure, so I decided to just reflect the datasheet instructions back to the user.