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,
.delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY
};
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 = {
+ .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY};
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.
Thanks
Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
