Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into spi-dw-mchp

[Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>]

Serge Semin writes:

> On Tue, May 19, 2020 at 02:05:19PM +0200, Lars Povlsen wrote:
>> On 13/05/20 16:18, Mark Brown wrote:
>> > Date: Wed, 13 May 2020 16:18:11 +0100
>> > From: Mark Brown <broonie@xxxxxxxxxx>
>> > To: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
>> > Cc: SoC Team <soc@xxxxxxxxxx>, Microchip Linux Driver Support
>> >  <UNGLinuxDriver@xxxxxxxxxxxxx>, linux-spi@xxxxxxxxxxxxxxx,
>> >  devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx,
>> >  linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, Alexandre Belloni
>> >  <alexandre.belloni@xxxxxxxxxxx>
>> > Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into
>> >  spi-dw-mchp
>> > User-Agent: Mutt/1.10.1 (2018-07-13)
>> >
>> > On Wed, May 13, 2020 at 04:00:26PM +0200, Lars Povlsen wrote:
>> >
>> > > +config SPI_DW_MCHP
>> > > + tristate "Memory-mapped io interface driver using DW SPI core of MSCC SoCs"
>> > > + default y if ARCH_SPARX5
>> > > + default y if SOC_VCOREIII
>> >
>> > Why the default ys?
>>
>> The SoC will typically boot from SPI... But its not a requirement per
>> se. I will remove it.
>>
>> >
>> > > +++ b/drivers/spi/Makefile
>> > > @@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_DAVINCI)               += spi-davinci.o
>> > >  obj-$(CONFIG_SPI_DLN2)                   += spi-dln2.o
>> > >  obj-$(CONFIG_SPI_DESIGNWARE)             += spi-dw.o
>> > >  obj-$(CONFIG_SPI_DW_MMIO)                += spi-dw-mmio.o
>> > > +obj-$(CONFIG_SPI_DW_MCHP)                += spi-dw-mchp.o
>> > >  obj-$(CONFIG_SPI_DW_PCI)         += spi-dw-midpci.o
>> > >  spi-dw-midpci-objs                       := spi-dw-pci.o spi-dw-mid.o
>> > >  obj-$(CONFIG_SPI_EFM32)                  += spi-efm32.o
>> >
>> > Please keep the file alphabetically sorted.
>> >
>>
>> Noted.
>>
>> > > +++ b/drivers/spi/spi-dw-mchp.c
>> > > @@ -0,0 +1,232 @@
>> > > +// SPDX-License-Identifier: GPL-2.0-only
>> > > +/*
>> > > + * Memory-mapped interface driver for MSCC SoCs
>> > > + *
>> >
>> > Please make the entire comment a C++ one so things look more
>> > intentional.
>>
>> Sure, I can do that. The presented form matches that of the other
>> spi-dw-* drivers, but I can see other using // blocks. Ack.
>>
>> >
>> > > +#define MAX_CS           4
>> >
>> > This should be namespaced.
>>
>> Ack.
>>
>
>> >
>> > > + rx_sample_dly = 0;
>> > > + device_property_read_u32(&pdev->dev, "spi-rx-delay-us", &rx_sample_dly);
>> > > + dws->rx_sample_dly = DIV_ROUND_UP(rx_sample_dly,
>> > > +                                   (dws->max_freq / 1000000));
>
> Perhaps 100000 is better to be replace with macro USEC_PER_SEC...
>
> Moreover are you sure the formulae is correct?
> dws->rx_sample_dly - a number of ssi_clk periods/cycles to delay the Rx-data sample,
> dws->max_freq - ssi_clk frequency (not period).
>
> In real math the formulae would look like:
> S = d * P [s], where d - number of delay cycles, P - ssi_clk period in seconds,
> S - requested delay in seconds.
> In the driver notation: d = dws->rx_sample_dly, P = 1 / dws->max_freq,
> S = rx_sample_dly ("spi-rx-delay-us" property value).
>
> dws->rx_sample_dly * (1 / dws->max_freq) = rx_sample_dly <=>
> dws->rx_sample_dly = rx_sample_dly * dws->max_freq.
>
> Though that's represented in seconds, so if rx_sample_dly is specified in usec,
> then you'd need to scale it down dividing by USEC_PER_SEC.
>
> For example, imagine we need a delay of 1 usec with ssi_clk of 50MHz.
> By your formulae we'd have: 1 / (50000000 / 1000000) = 0 cycles (actually 1 due
> to DIV_ROUND_UP, but incorrect anyway),
> By mine: 1 * (500000000 / 1000000) = 50 cycles. Seems closer to reality.
>
> Am I missing something?

No, you are perfectly right, the calculation was wrong - and I concur
the unit should be NS.

(your example threw me off, you are using 500Mhz, typo I guess)

I believe the calculation should be:

  device_property_read_u32(&pdev->dev, "snps,rx-sample-delay-ns", &rx_sample_dly);
  dws->rx_sample_dly = DIV_ROUND_CLOSEST(rx_sample_dly, NSEC_PER_SEC / dws->max_freq);
        
So for your example of 1us = 1000ns, we have a cycle time of 20 ns => 50 cycles.

And I assume DIV_ROUND_CLOSEST() is the better instead of explicit
rounding up/down. And I assume its fair to assume that the cycle time is
not a fraction.

Ok?

>
>> >
>> > If this is a standard feature of the DesignWare IP why parse it here and
>> > not in the generic code?
>>
>> This is a standard feature of the DesignWare IP, so good suggestion. I
>> will arrange with Serge.
>
> Regarding "spi-rx-delay-us" and the sampling delay the IP supports. Here is what
> documentation says regarding the register, which is then initialized with this
> parameter "This register controls the number of ssi_clk cycles that are
> delayed from the default sample time before the actual sample of the rxd input
> signal occurs." While the "spi-rx-delay-us" property is described as: "Delay, in
> microseconds, after a read transfer." I may misunderstand something, but IMO
> these descriptions don't refer to the same values. The only real use of the
> "spi-rx-delay-us" property I've found in "./drivers/input/rmi4/rmi_spi.c".
> That driver gets the value of the property and just sets the delay_usecs
> of some transfers, which isn't even close to the functionality the RX_SAMPLE_DLY
> register provides.
>
> To be clear the RX_SAMPLE_DLY register can be used to delay the RX-bits sample
> with respect to the normal Rx sampling timing. The delay is measured in the
> numbers of the ssi_clk periods. (Note also that the maximum delay is limited
> with a constant parameter pre-initialized at the IP-core synthesis stage. It can
> be defined within a range [4, 255]. In our IP it's limited with just 4 periods.)
>

Yes - I was not aware of the instantiation incurred limit before. Turned
our IP has up to 100ns worth of fifo depth - 25 cycles.

> As I see it, a better way would be to either define a new vendor-specific
> property like "snps,rx-sample-delay-ns" (note NS here, since normally the
> ssi_clk is much higher than 1MHz), or define a new generic SPI property.
> Mark, Andy?

I'll assume "snps,rx-sample-delay-ns" for now, its easy to rename if you
decide so.

Thanks again!

---Lars

>
> -Sergey
>
>>
>> Thank you for your comments!
>>
>> ---Lars
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lars Povlsen,
Microchip