linux-kernel - Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Jan 2019 10:49:37 +0100
From:   Jonas Bonn <jonas@...rbonn.se>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     Mark Brown <broonie@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Lanqing Liu <lanqing.liu@...eadtrum.com>
Subject: Re: [PATCH v3 1/2] spi: support inter-word delay requirement for
 devices



On 29/01/2019 10:35, Baolin Wang wrote:
> On Tue, 29 Jan 2019 at 17:14, Jonas Bonn <jonas@...rbonn.se> wrote:
>>
>>
>>
>> On 29/01/2019 10:04, Baolin Wang wrote:
>>> Hi Jonas,
>>> On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@...rbonn.se> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 28/01/2019 19:10, Mark Brown wrote:
>>>>> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
>>>>>
>>>>>> @@ -164,6 +166,7 @@ struct spi_device {
>>>>>>        char                    modalias[SPI_NAME_SIZE];
>>>>>>        const char              *driver_override;
>>>>>>        int                     cs_gpio;        /* chip select gpio */
>>>>>> +    uint16_t                word_delay;     /* inter-word delay (us) */
>>>>>
>>>>> This needs some code in the core joining it up with the per-transfer
>>>>> word delay similar to what we have for speed_hz and bits_per_word in
>>>>> __spi_validate().  Then the controller drivers can just look at the
>>>>> per-transfer value and support both without having to duplicate logic.
>>>>>
>>>>
>>>> So spi_transfer already has a field word_delay and it's defined as
>>>> _clock cycles_ to delay between words.  I defined word_delay in
>>>> spi_device as _microseconds_ to delay along the lines of delay_usecs.
>>>>
>>>> Given that the inter-word delay is a function of the slave device speed
>>>> and not of the SPI bus speed, I'm inclined to say that a time-based
>>>> delay is what we want (to be independent of bus speed).  As such, I want
>>>> to know if I should add word_delay_usecs to _both_ spi_transfer and
>>>> spi_device?
>>>>
>>>> There's only one user of word_delay from spi_transfer.  Just looking at
>>>> it quickly, it looks like it wants the word_delay in
>>>> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
>>>> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
>>>> that to be microseconds and do the calculation in the driver?
>>>
>>> The Spreadtrum SPI controller's word delay unit is clock cycle of the
>>> SPI clock, since the SPI source clock can be changed, we can not force
>>> user to know the real microseconds. But can we change it to a union
>>> structure? not sure if this is a good way.
>>
>> OK, so it is the SPI clock.  That's good.  There's a comment in the
>> driver that makes it look like it should be the source clock.
> 
> Sorry for my unclear description, what I mean is that it is the SPI
> source clock cycles.
> 
>> The problem with a delay in clock cycles is that the faster the clock,
>> the shorter the delay.  The delay is a property of the slave and the
>> slave has a fixed internal clock.  This means that if we increase SCK we
>> also need to increase the word_delay (in cycles) in order to give the
>> slave the same amount of breathing room.
> 
> Sorry for my confusing description, our case requires source clock
> cycles for word delay.

OK.  So the user (perhaps in userspace using spidev) has to know the 
rate of the IO clock that the SPI controller sits behind and then has to 
match this to the required delay of the slave device...  Doesn't sound 
very portable.

> 
>>
>>>
>>> union {
>>>        int word_delay_us;
>>>        int word_delay_cycle;
>>> } w;
>>>
>>
>> I don't think that's a practical solution.
>>
>> The register setting in the spi-sprd driver is what... SCK cycles?  So
>> you'd want word_delay_us * max_speed_hz?
>>
>> The register setting on my Atmel board is in SPI-clock cycles
>> (effectively).  So I want word_delay_us*clk_get_rate(spi-clk).
> 
> Okay, so your case is different with Spreadtrum SPI.
> 

No, if your register is source clock cycles then it's the same.  On my 
board, the register setting is source clock cycles, too.  It's a 
straightforward conversion from delay to clock cycles... rough, unscaled 
formula above.

/Jonas