On 11/07/2016 07:49 PM, Joel Holdsworth wrote:
> Hi Marek,
Hi,
> 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.
I prefer it to be readable :)
>> 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.
Ah, right.
>>
>>> + 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.
So why not turn the CS line into GPIO and just toggle the GPIO?
>>> + 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.
OK
> 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.
Sounds good, thanks.
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
