RE: [PATCH][RFC] spi: sh-msiof: Configure MSIOF sync signal timing in device tree

[yoshihiro shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>]

Hi Geert-san,

> -----Original Message-----
> From: devicetree-owner@xxxxxxxxxxxxxxx [mailto:devicetree-owner@xxxxxxxxxxxxxxx] On Behalf Of Geert Uytterhoeven
> Sent: Monday, December 01, 2014 5:27 PM
> 
> Hi Shimoda-san, Laurent,
> 
> On Sun, Nov 30, 2014 at 9:23 PM, Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> > On Friday 28 November 2014 12:45:27 Yoshihiro Shimoda wrote:
> >> The MSIOF controller has DTDL and SYNCDL in SITMDR1 and SIRMDR1
> >> registers. So, this patch adds new properties like the following
> >> commit:
> >>   d0fb47a5237d8b9576113568bacfd27892308b62
> >>   (spi: fsl-espi: Configure FSL eSPI CSBEF and CSAFT)
> >>
> >> The DTDL is the chip select (SYNC) setup time.
> >>  b'000: No bit delay
> >>  b'001: 1-clock-cycle delay
> >>  b'010: 2-clock-cycle delay
> >>  b'101: 0.5-clock-cycle delay
> >>  b'110: 1.5-clock-cycle delay
> >>
> >> The SYNCDL is the chip select (SYNC) hold time.
> >>  b'000: No bit delay
> >>  b'001: 1-clock-cycle delay
> >>  b'010: 2-clock-cycle delay
> >>  b'011: 3-clock-cycle delay
> >>  b'101: 0.5-clock-cycle delay
> >>  b'110: 1.5-clock-cycle delay
> 
> You forgot to quote the last line from the DTDL and SYNCDL paragraphs:
> 
> | In case of SPI mode, only 000 is allowed to set to this field.
> 
> And the spi-sh-msiof driver is using the MSIOF in SPI mode???

After I asked HW team about this restriction, this controller can set
the DTDL/SYNCDL in transfer mode.
(In other words, the current manual has error about this specification.)

So, I would like to send patches for this configurations.

< snip >
> >> diff --git a/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> b/Documentation/devicetree/bindings/spi/sh-msiof.txt index d11c372..5fe8ffd
> >> 100644
> >> --- a/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> +++ b/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> @@ -30,6 +30,14 @@ Optional properties:
> >>                        specifiers, one for transmission, and one for
> >>                        reception.
> >>  - dma-names            : Must contain a list of two DMA names, "tx" and
> >> "rx".
> >> +- renesas,tdmr-dtdl    : delay sync signal (setup) in transmit mode
> >> +                      (default is 0, we can set it to 0, 1, 2, 5, or 6)
> >> +- renesas,tdmr-syncdl  : delay sync signal (hold) in transmit mode
> >> +                      (default is 0, we can set it to 0, 1, 2, 3, 5, or 6)
> >> +- renesas,rdmr-dtdl    : delay sync signal (setup) in receive mode
> >> +                      (default is 0, we can set it to 0, 1, 2, 5, or 6)
> >> +- renesas,rdmr-syncdl  : delay sync signal (hold) in receive mode
> >> +                      (default is 0, we can set it to 0, 1, 2, 3, 5, or 6)
> 
> In general, it's frowned upon the specify actual register values in DT.
> So I think it's better to specify the delay values instead of the
> bitfield values,
> and drop the "tdmr" and "rdmr" from the names.

Thank you very much for your comment.
I understood we can use specify the delay values in DT.

> As DT supports integers only, that should become a fixed point value.
> E.g. delay in deci-clocks (0, 5, 10, 15, 20)?
> Or as a percentage of the clock cycle (0, 50, 100, 150, 200)?

I will use a percentage of the clock cycle.

> Do you really need two sets of values, for both transmit and receive?
> I.e. do you need different values for transmit and receive for your use case?
> SPI does both transmit and receive using the same clock and chip select.
> I know some MSIOF implementations can handle both indepedently.

Since MSIOF can set the configurations in transmit actually,
I will set the values for transmit mode.

> And to answer your last question: if several drivers need this, it makes sense
> to make this general, and add support in the SPI core. Mark?

I checked the files in Documentation/devicetree/spi/, and then
I found 2 files about such configurations:
 - fsl,csbef and fsl,csaft in fsl-spi.txt:
  - I already mentions in this RFC patch.

 - samsung,spi-deedback-delay in spi-samsung.txt:
  - But it is only affect to miso line??
    I don't know about the detail of this parameter.

So, almost all drivers doesn't have such configuration at the moment, I think.

Best regards,
Yoshihiro Shimoda

> > Was it really the intent of this patch to add DT properties without providing
> > an implementation in the sh-msiof driver ?
> 
> Given the sentence "If this patch is reasonable, I will modify the sh-msiof
> driver." in the introduction, I think so.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> 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
��.n��������+%������w��{.n�����{����)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥