[PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Vinicius Maciel]

In order to work appropriately, the max11043 ADC chip and probably
others, needs SPI master samples the data at the correct edge. From
max11043 datasheet: "The data at DIN is latched on the rising edge
of SCLK". Same to DOUT.

This patch add Master Sample Data Mode bit in normal sample mode.
It will affect only A20.

Signed-off-by: Vinicius Maciel <viniciusfre-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Siarhei Siamashka <siarhei.siamashka-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-sun4i.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index c5cd635c28f3..6325be2ce8d9 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -44,6 +44,7 @@
 #define SUN4I_CTL_CS_MANUAL			BIT(16)
 #define SUN4I_CTL_CS_LEVEL			BIT(17)
 #define SUN4I_CTL_TP				BIT(18)
+#define SUN4I_CTL_SDM				BIT(20)
 
 #define SUN4I_INT_CTL_REG		0x0c
 #define SUN4I_INT_CTL_RF_F34			BIT(4)
@@ -407,7 +408,8 @@ static int sun4i_spi_runtime_resume(struct device *dev)
 	}
 
 	sun4i_spi_write(sspi, SUN4I_CTL_REG,
-			SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP);
+			SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP |
+			SUN4I_CTL_SDM);
 
 	return 0;
 
-- 
2.11.0

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Sat, Feb 11, 2017 at 12:54:47AM -0300, Vinicius Maciel wrote:
> In order to work appropriately, the max11043 ADC chip and probably
> others, needs SPI master samples the data at the correct edge. From
> max11043 datasheet: "The data at DIN is latched on the rising edge
> of SCLK". Same to DOUT.
> 
> This patch add Master Sample Data Mode bit in normal sample mode.
> It will affect only A20.
> 
> Signed-off-by: Vinicius Maciel <viniciusfre-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Siarhei Siamashka <siarhei.siamashka-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Sat, Feb 11, 2017 at 12:54:47AM -0300, Vinicius Maciel wrote:
> In order to work appropriately, the max11043 ADC chip and probably
> others, needs SPI master samples the data at the correct edge. From
> max11043 datasheet: "The data at DIN is latched on the rising edge
> of SCLK". Same to DOUT.
> 
> This patch add Master Sample Data Mode bit in normal sample mode.
> It will affect only A20.

This should be controlled by the SPI mode settings, different chips have
different requirements.  If the controller supports multiple modes then
it should expose that and let the drivers and system integrations
choose.

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Vinicius Maciel]

[-- Attachment #1.1: Type: text/plain, Size: 911 bytes --]



Em segunda-feira, 13 de fevereiro de 2017 09:52:53 UTC-3, Mark Brown 
escreveu:
>
> This should be controlled by the SPI mode settings, different chips have 
> different requirements.  If the controller supports multiple modes then 
> it should expose that and let the drivers and system integrations 
> choose. 
>
Do you mean SPI_CPHA/SPI_CPOL, right? Well, I don't know how this mode can 
to fit in existing modes. Since this bit is to control when the data will 
be sampled, in the same clock edge or at the edge that is half cycle 
delayed. That has nothing to do with SPI_CPHA/SPI_CPOL.

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #1.2: Type: text/html, Size: 2123 bytes --]

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Siarhei Siamashka]

On Sun, 26 Feb 2017 06:10:50 -0800 (PST)
Vinicius Maciel <viniciusfre-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Em segunda-feira, 13 de fevereiro de 2017 09:52:53 UTC-3, Mark Brown 
> escreveu:
> >
> > This should be controlled by the SPI mode settings, different chips have 
> > different requirements.  If the controller supports multiple modes then 
> > it should expose that and let the drivers and system integrations 
> > choose. 
> >  
> Do you mean SPI_CPHA/SPI_CPOL, right? Well, I don't know how this mode can 
> to fit in existing modes. Since this bit is to control when the data will 
> be sampled, in the same clock edge or at the edge that is half cycle 
> delayed. That has nothing to do with SPI_CPHA/SPI_CPOL.

I'm not sure if it is realistic to make any progress with this issue
any time soon, but I would suggest you to re-send the patch with an
updated commit message, which could better explain what's going on.

I still think that your bugfix is good, that's why you got my
Reviewed-by tag. And we had an older discussion about it (Mark
was not participating there, so probably he missed it):

   https://patchwork.kernel.org/patch/9567547/

Here is a short summary. Older Allwinner A10/A13 SoCs used to have
reserved RAZ/WI (Read-As-Zero, Writes Ignored) bits 20-31 in the
SPI_CTL register. Newer Allwinner A20 SoC introduced some sort of
a wacky half-cycle delay mode and used the previously reserved bit
20 from the SPI_CTL to enable/disable it. Now the tricky part is that
the meaning of this new bit is inverted for some unknown reason: the
wacky mode is activated when the bit is set to zero, while the standard
behaviour (same as A10/A13) is preserved when this bit is set to 1. The
reset default of this bit is 1 on A20 (normal SPI behaviour). The
problem of the current SPI driver is that it clears bits 20-31 of
SPI_CTL, which results in normal SPI behaviour on A10/A13, and a wacky
half-cycle delayed mode on A20. Your bugfix just makes A20 behave in
the same way as A10/A13.

We have exactly zero users of the wacky half-cycle delayed mode.
Moreover, if it gets enabled accidentally (due to a bug in the
SPI_CTL register initialization in the SPI driver as explained
above), then it breaks your max11043 ADC chip. Also even if we
assume that there is some hypothetical equipment, which actually
needs this wacky mode to function preperly, then this equipment
is not compatible with older A10/A13 SoCs because the older SoCs
did not implement this mode at all.

You could rephrase the above two paragraphs and use them as the
commit message for the v3 patch. And maybe also add an extra
comment to the SPI driver code.

If Mark has any comments on this, then I would be very happy
to hear them. So far his suggestion looks like he prefers to
see a support for this wacky mode implemented in the SPI driver
(a *new feature*, which has no actual users and can't be
realistically tested). While your patch just prevents accidental
and undesired activation of the wacky mode (an *important bugfix*
for a real problem encountered in the wild).

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Vinicius Maciel]

[-- Attachment #1.1: Type: text/plain, Size: 1603 bytes --]



Em quinta-feira, 2 de março de 2017 17:44:16 UTC-3, Siarhei Siamashka 
escreveu:

> Here is a short summary. Older Allwinner A10/A13 SoCs used to have 
> reserved RAZ/WI (Read-As-Zero, Writes Ignored) bits 20-31 in the 
> SPI_CTL register. Newer Allwinner A20 SoC introduced some sort of 
> a wacky half-cycle delay mode and used the previously reserved bit 
> 20 from the SPI_CTL to enable/disable it. Now the tricky part is that 
> the meaning of this new bit is inverted for some unknown reason: the 
> wacky mode is activated when the bit is set to zero, while the standard 
> behaviour (same as A10/A13) is preserved when this bit is set to 1. The 
> reset default of this bit is 1 on A20 (normal SPI behaviour). The 
> problem of the current SPI driver is that it clears bits 20-31 of 
> SPI_CTL, which results in normal SPI behaviour on A10/A13, and a wacky 
> half-cycle delayed mode on A20. Your bugfix just makes A20 behave in 
> the same way as A10/A13. 

These are the exact words I was looking for, thanks Siarhei. 
And I don't really know which chip could to use this wacky half-cycle 
delay mode. Anyway, maybe a new dts property could be a solution to 
let user to enable or not normal mode and half-cycle mode?

Best regards,
Vinicius

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #1.2: Type: text/html, Size: 2026 bytes --]

Re: [PATCH v2] ARM: spi/sun7i: Add Master Sample Data Mode for SPI

[Icenowy Zheng]

ARM: is not needed.

2017年2月11日 11:54于 Vinicius Maciel <viniciusfre-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>写道：
>
> In order to work appropriately, the max11043 ADC chip and probably 
> others, needs SPI master samples the data at the correct edge. From 
> max11043 datasheet: "The data at DIN is latched on the rising edge 
> of SCLK". Same to DOUT. 
>
> This patch add Master Sample Data Mode bit in normal sample mode. 
> It will affect only A20. 
>
> Signed-off-by: Vinicius Maciel <viniciusfre-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
> Reviewed-by: Siarhei Siamashka <siarhei.siamashka-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
> --- 
> drivers/spi/spi-sun4i.c | 4 +++- 
> 1 file changed, 3 insertions(+), 1 deletion(-) 
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c 
> index c5cd635c28f3..6325be2ce8d9 100644 
> --- a/drivers/spi/spi-sun4i.c 
> +++ b/drivers/spi/spi-sun4i.c 
> @@ -44,6 +44,7 @@ 
> #define SUN4I_CTL_CS_MANUAL BIT(16) 
> #define SUN4I_CTL_CS_LEVEL BIT(17) 
> #define SUN4I_CTL_TP BIT(18) 
> +#define SUN4I_CTL_SDM BIT(20) 
>
> #define SUN4I_INT_CTL_REG 0x0c 
> #define SUN4I_INT_CTL_RF_F34 BIT(4) 
> @@ -407,7 +408,8 @@ static int sun4i_spi_runtime_resume(struct device *dev) 
> } 
>
> sun4i_spi_write(sspi, SUN4I_CTL_REG, 
> - SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP); 
> + SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP | 
> + SUN4I_CTL_SDM); 
>
> return 0; 
>
> -- 
> 2.11.0 
>
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group. 
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org 
> For more options, visit https://groups.google.com/d/optout. 

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.