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

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

[Yoshihiro Shimoda]

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

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---

 I would like to add new properties for sh-msiof driver to adjust
the SYNC siginal timing using DTDL and SYNCDL. In the current driver,
these parameters are hardcoded to 0. And then, I checked other spi
drivers, and I found the following commit:
  d0fb47a5237d8b9576113568bacfd27892308b62
  (spi: fsl-espi: Configure FSL eSPI CSBEF and CSAFT)

If this patch is reasonable, I will modify the sh-msiof driver.
Or, should we add a new function for this timing adjusting in the
spi framework?

 Documentation/devicetree/bindings/spi/sh-msiof.txt |    8 ++++++++
 drivers/spi/spi-sh-msiof.c                         |    2 ++
 2 files changed, 10 insertions(+)

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)

 Optional properties, deprecated for soctype-specific bindings:
 - renesas,tx-fifo-size : Overrides the default tx fifo size given in words
diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 3f36540..09e0c38 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -296,11 +296,13 @@ static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
 	tmp = MDR1_SYNCMD_SPI | 1 << MDR1_FLD_SHIFT | MDR1_XXSTP;
 	tmp |= !cs_high << MDR1_SYNCAC_SHIFT;
 	tmp |= lsb_first << MDR1_BITLSB_SHIFT;
+printk("%s: TMDR1 = %x\n", __func__, tmp | MDR1_TRMD | TMDR1_PCON);
 	sh_msiof_write(p, TMDR1, tmp | MDR1_TRMD | TMDR1_PCON);
 	if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) {
 		/* These bits are reserved if RX needs TX */
 		tmp &= ~0x0000ffff;
 	}
+printk("%s: RMDR1 = %x\n", __func__, tmp);
 	sh_msiof_write(p, RMDR1, tmp);

 	tmp = 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

Hi Shimoda-san,

Thank you for the patch.

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
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> 
>  I would like to add new properties for sh-msiof driver to adjust
> the SYNC siginal timing using DTDL and SYNCDL. In the current driver,
> these parameters are hardcoded to 0. And then, I checked other spi
> drivers, and I found the following commit:
>   d0fb47a5237d8b9576113568bacfd27892308b62
>   (spi: fsl-espi: Configure FSL eSPI CSBEF and CSAFT)
> 
> If this patch is reasonable, I will modify the sh-msiof driver.
> Or, should we add a new function for this timing adjusting in the
> spi framework?
> 
>  Documentation/devicetree/bindings/spi/sh-msiof.txt |    8 ++++++++
>  drivers/spi/spi-sh-msiof.c                         |    2 ++
>  2 files changed, 10 insertions(+)
> 
> 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)

Was it really the intent of this patch to add DT properties without providing 
an implementation in the sh-msiof driver ?

>  Optional properties, deprecated for soctype-specific bindings:
>  - renesas,tx-fifo-size : Overrides the default tx fifo size given in words
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 3f36540..09e0c38 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -296,11 +296,13 @@ static void sh_msiof_spi_set_pin_regs(struct
> sh_msiof_spi_priv *p, tmp = MDR1_SYNCMD_SPI | 1 << MDR1_FLD_SHIFT |
> MDR1_XXSTP;
>  	tmp |= !cs_high << MDR1_SYNCAC_SHIFT;
>  	tmp |= lsb_first << MDR1_BITLSB_SHIFT;
> +printk("%s: TMDR1 = %x\n", __func__, tmp | MDR1_TRMD | TMDR1_PCON);
>  	sh_msiof_write(p, TMDR1, tmp | MDR1_TRMD | TMDR1_PCON);
>  	if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) {
>  		/* These bits are reserved if RX needs TX */
>  		tmp &= ~0x0000ffff;
>  	}
> +printk("%s: RMDR1 = %x\n", __func__, tmp);

I suppose these printk are debugging leftovers.

>  	sh_msiof_write(p, RMDR1, tmp);
> 
>  	tmp = 0;

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Geert Uytterhoeven]

Hi Shimoda-san, Laurent,

On Sun, Nov 30, 2014 at 9:23 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> 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???

>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
>> ---
>>
>>  I would like to add new properties for sh-msiof driver to adjust
>> the SYNC siginal timing using DTDL and SYNCDL. In the current driver,
>> these parameters are hardcoded to 0. And then, I checked other spi
>> drivers, and I found the following commit:
>>   d0fb47a5237d8b9576113568bacfd27892308b62
>>   (spi: fsl-espi: Configure FSL eSPI CSBEF and CSAFT)
>>
>> If this patch is reasonable, I will modify the sh-msiof driver.
>> Or, should we add a new function for this timing adjusting in the
>> spi framework?
>>
>>  Documentation/devicetree/bindings/spi/sh-msiof.txt |    8 ++++++++
>>  drivers/spi/spi-sh-msiof.c                         |    2 ++
>>  2 files changed, 10 insertions(+)
>>
>> 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.
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)?

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.

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?

> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

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 linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[yoshihiro shimoda]

Hi Geert-san, Laurent-san,

Thank you very much for your comments!

> On Sun, Nov 30, 2014 at 9:23 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.

Thank you for the point!
I asked HW team about the paragraphs. And then, the paragraphs are correct.

> And the spi-sh-msiof driver is using the MSIOF in SPI mode???

Yes, the driver is using the MSIOF in SPI mode.
So, we have no chance to set the DTDL and SYNCDL to 0 in SPI mode.

I should have looked into the datasheet more before I sent this RFC.

< snip >
> > 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.

Yes, I meant that.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[yoshihiro shimoda]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5337 bytes --]

Hi Geert-san,

> -----Original Message-----
> From: devicetree-owner@vger.kernel.org [mailto:devicetree-owner@vger.kernel.org] 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@ideasonboard.com> 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@linux-m68k.org
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f