[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Andrew Lunn]

Hi Jason

I tested your patch to add SPI_CHPA and SPI_CPOL on a QNAP box which
has SPI NAND. No problems seen.

Tested-by: Andrew Lunn <andrew@lunn.ch>

   Andrew

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Grant Likely]

On Mon, 10 Dec 2012 14:38:00 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Jason
> 
> I tested your patch to add SPI_CHPA and SPI_CPOL on a QNAP box which
> has SPI NAND. No problems seen.
> 
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> 

Thanks Andrew.

g.

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Gunthorpe]

Support these transfer modes from the SPI layer by setting
the appropriate register bits before doing the transfer.

This was tested on the Marvell kirkwood SOC that uses this driver.

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Rolf Manderscheid <rvm@obsidianresearch.com>
---
 drivers/spi/spi-orion.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index b17c09c..011186d 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -32,8 +32,12 @@
 #define ORION_SPI_DATA_IN_REG		0x0c
 #define ORION_SPI_INT_CAUSE_REG		0x10
 
+#define ORION_SPI_MODE_CPOL		(1 << 11)
+#define ORION_SPI_MODE_CPHA		(1 << 12)
 #define ORION_SPI_IF_8_16_BIT_MODE	(1 << 5)
 #define ORION_SPI_CLK_PRESCALE_MASK	0x1F
+#define ORION_SPI_MODE_MASK		(ORION_SPI_MODE_CPOL | \
+					 ORION_SPI_MODE_CPHA)
 
 struct orion_spi {
 	struct spi_master	*master;
@@ -123,6 +127,23 @@ static int orion_spi_baudrate_set(struct spi_device *spi, unsigned int speed)
 	return 0;
 }
 
+static void
+orion_spi_mode_set(struct spi_device *spi)
+{
+	u32 reg;
+	struct orion_spi *orion_spi;
+
+	orion_spi = spi_master_get_devdata(spi->master);
+
+	reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
+	reg &= ~ORION_SPI_MODE_MASK;
+	if (spi->mode & SPI_CPOL)
+		reg |= ORION_SPI_MODE_CPOL;
+	if (spi->mode & SPI_CPHA)
+		reg |= ORION_SPI_MODE_CPHA;
+	writel(reg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
+}
+
 /*
  * called only when no transfer is active on the bus
  */
@@ -142,6 +163,8 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	if ((t != NULL) && t->bits_per_word)
 		bits_per_word = t->bits_per_word;
 
+	orion_spi_mode_set(spi);
+
 	rc = orion_spi_baudrate_set(spi, speed);
 	if (rc)
 		return rc;
@@ -399,7 +422,7 @@ static int __init orion_spi_probe(struct platform_device *pdev)
 	}
 
 	/* we support only mode 0, and no options */
-	master->mode_bits = 0;
+	master->mode_bits = SPI_CPHA | SPI_CPOL;
 
 	master->setup = orion_spi_setup;
 	master->transfer_one_message = orion_spi_transfer_one_message;
-- 
1.7.5.4

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Cooper]

On Wed, Nov 21, 2012 at 12:23:35PM -0700, Jason Gunthorpe wrote:
> Support these transfer modes from the SPI layer by setting
> the appropriate register bits before doing the transfer.
> 
> This was tested on the Marvell kirkwood SOC that uses this driver.
> 
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Rolf Manderscheid <rvm@obsidianresearch.com>
> ---
>  drivers/spi/spi-orion.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index b17c09c..011186d 100644
> --- a/drivers/spi/spi-orion.c
> +++ b/drivers/spi/spi-orion.c
> @@ -32,8 +32,12 @@
>  #define ORION_SPI_DATA_IN_REG		0x0c
>  #define ORION_SPI_INT_CAUSE_REG		0x10
>  
> +#define ORION_SPI_MODE_CPOL		(1 << 11)
> +#define ORION_SPI_MODE_CPHA		(1 << 12)
>  #define ORION_SPI_IF_8_16_BIT_MODE	(1 << 5)
>  #define ORION_SPI_CLK_PRESCALE_MASK	0x1F
> +#define ORION_SPI_MODE_MASK		(ORION_SPI_MODE_CPOL | \
> +					 ORION_SPI_MODE_CPHA)
>  
>  struct orion_spi {
>  	struct spi_master	*master;
> @@ -123,6 +127,23 @@ static int orion_spi_baudrate_set(struct spi_device *spi, unsigned int speed)
>  	return 0;
>  }
>  
> +static void
> +orion_spi_mode_set(struct spi_device *spi)
> +{
> +	u32 reg;
> +	struct orion_spi *orion_spi;
> +
> +	orion_spi = spi_master_get_devdata(spi->master);
> +
> +	reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> +	reg &= ~ORION_SPI_MODE_MASK;
> +	if (spi->mode & SPI_CPOL)
> +		reg |= ORION_SPI_MODE_CPOL;
> +	if (spi->mode & SPI_CPHA)
> +		reg |= ORION_SPI_MODE_CPHA;
> +	writel(reg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> +}
> +
>  /*
>   * called only when no transfer is active on the bus
>   */
> @@ -142,6 +163,8 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
>  	if ((t != NULL) && t->bits_per_word)
>  		bits_per_word = t->bits_per_word;
>  
> +	orion_spi_mode_set(spi);
> +
>  	rc = orion_spi_baudrate_set(spi, speed);
>  	if (rc)
>  		return rc;
> @@ -399,7 +422,7 @@ static int __init orion_spi_probe(struct platform_device *pdev)
>  	}
>  
>  	/* we support only mode 0, and no options */
> -	master->mode_bits = 0;
> +	master->mode_bits = SPI_CPHA | SPI_CPOL;

The comment no longer seems valid.  ;-)  Also, you are unconditionally
enabling these modes.  Do all users of this driver support this?

thx,

Jason.

>  
>  	master->setup = orion_spi_setup;
>  	master->transfer_one_message = orion_spi_transfer_one_message;
> -- 
> 1.7.5.4
> 

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Gunthorpe]

On Wed, Nov 21, 2012 at 02:35:52PM -0500, Jason Cooper wrote:

> >  	/* we support only mode 0, and no options */
> > -	master->mode_bits = 0;
> > +	master->mode_bits = SPI_CPHA | SPI_CPOL;
> 
> The comment no longer seems valid.  ;-)  Also, you are unconditionally
> enabling these modes.  Do all users of this driver support this?

Right on the comment.. Not sure about your second query?

 * @mode_bits: flags understood by this controller driver

The mode_bits need to be set so the mid layer will allow consumers to
pass them down through spi->mode when it calls into
orion_spi_setup_transfer. So that looks correct..

Now, it seems a fair question what mode the driver will run in by
default now.. Previously, the driver didn't change the CPOL and CPHA
bits, so whatever the boot firmware left them at will be the mode it
uses.

If SPI consumers are not properly requesting the proper CPOL/CPHA
operation and instead relying on the boot firmware to leave the proper
bit settings then things will break for those users (they will need to
update their board's DT file, or otherwise) - but I don't see any way
to address that and implement proper programmable support?

This could affect at least dove, dreamplug, ts219 and lsxl.

Jason

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Grant Likely]

On Wed, 21 Nov 2012 12:23:35 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> Support these transfer modes from the SPI layer by setting
> the appropriate register bits before doing the transfer.
> 
> This was tested on the Marvell kirkwood SOC that uses this driver.

Woo! a note about how it was tested. I can't (and often don't) see
enough of these.  :-)

Applied, thanks.

g.

> 
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Rolf Manderscheid <rvm@obsidianresearch.com>
> ---
>  drivers/spi/spi-orion.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index b17c09c..011186d 100644
> --- a/drivers/spi/spi-orion.c
> +++ b/drivers/spi/spi-orion.c
> @@ -32,8 +32,12 @@
>  #define ORION_SPI_DATA_IN_REG		0x0c
>  #define ORION_SPI_INT_CAUSE_REG		0x10
>  
> +#define ORION_SPI_MODE_CPOL		(1 << 11)
> +#define ORION_SPI_MODE_CPHA		(1 << 12)
>  #define ORION_SPI_IF_8_16_BIT_MODE	(1 << 5)
>  #define ORION_SPI_CLK_PRESCALE_MASK	0x1F
> +#define ORION_SPI_MODE_MASK		(ORION_SPI_MODE_CPOL | \
> +					 ORION_SPI_MODE_CPHA)
>  
>  struct orion_spi {
>  	struct spi_master	*master;
> @@ -123,6 +127,23 @@ static int orion_spi_baudrate_set(struct spi_device *spi, unsigned int speed)
>  	return 0;
>  }
>  
> +static void
> +orion_spi_mode_set(struct spi_device *spi)
> +{
> +	u32 reg;
> +	struct orion_spi *orion_spi;
> +
> +	orion_spi = spi_master_get_devdata(spi->master);
> +
> +	reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> +	reg &= ~ORION_SPI_MODE_MASK;
> +	if (spi->mode & SPI_CPOL)
> +		reg |= ORION_SPI_MODE_CPOL;
> +	if (spi->mode & SPI_CPHA)
> +		reg |= ORION_SPI_MODE_CPHA;
> +	writel(reg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> +}
> +
>  /*
>   * called only when no transfer is active on the bus
>   */
> @@ -142,6 +163,8 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
>  	if ((t != NULL) && t->bits_per_word)
>  		bits_per_word = t->bits_per_word;
>  
> +	orion_spi_mode_set(spi);
> +
>  	rc = orion_spi_baudrate_set(spi, speed);
>  	if (rc)
>  		return rc;
> @@ -399,7 +422,7 @@ static int __init orion_spi_probe(struct platform_device *pdev)
>  	}
>  
>  	/* we support only mode 0, and no options */
> -	master->mode_bits = 0;
> +	master->mode_bits = SPI_CPHA | SPI_CPOL;
>  
>  	master->setup = orion_spi_setup;
>  	master->transfer_one_message = orion_spi_transfer_one_message;
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Gunthorpe]

On Thu, Dec 06, 2012 at 02:25:21PM +0000, Grant Likely wrote:
> On Wed, 21 Nov 2012 12:23:35 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> > Support these transfer modes from the SPI layer by setting
> > the appropriate register bits before doing the transfer.
> > 
> > This was tested on the Marvell kirkwood SOC that uses this driver.
> 
> Woo! a note about how it was tested. I can't (and often don't) see
> enough of these.  :-)

Thanks for looking at this Grant,

But I think that Jason's comment (see
https://patchwork.kernel.org/patch/1782061/) is valid.

This will likely switch all current users from using 'whatever the
firmware left behind' to 'whatever the kernel default is' - which will
surely break something here and there??

I was thinking of a transitory patch that did:

reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
nreg = reg & ~ORION_SPI_MODE_MASK;
if (spi->mode & SPI_CPOL)
	nreg |= ORION_SPI_MODE_CPOL;
if (spi->mode & SPI_CPHA)
	nreg |= ORION_SPI_MODE_CPHA;
#ifdef TRANSITION
 if (nreg != reg)
        dev_error(..,"Not allowing a SPI mode change away from firmware
 defaults. Board support needs to be updated!");
#else
 writel(nreg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
#endif

At least that way there is a warning and the DTSs can be fixed up.

I haven't had a moment to look into that though..

But I'm not sure that is in line with kernel philosophy WRT to DT..

What do you think?

Regards,
Jason

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Grant Likely]

On Thu, 6 Dec 2012 10:25:04 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Dec 06, 2012 at 02:25:21PM +0000, Grant Likely wrote:
> > On Wed, 21 Nov 2012 12:23:35 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> > > Support these transfer modes from the SPI layer by setting
> > > the appropriate register bits before doing the transfer.
> > > 
> > > This was tested on the Marvell kirkwood SOC that uses this driver.
> > 
> > Woo! a note about how it was tested. I can't (and often don't) see
> > enough of these.  :-)
> 
> Thanks for looking at this Grant,
> 
> But I think that Jason's comment (see
> https://patchwork.kernel.org/patch/1782061/) is valid.
> 
> This will likely switch all current users from using 'whatever the
> firmware left behind' to 'whatever the kernel default is' - which will
> surely break something here and there??

Hmmm. Hard to say. Just the fact that existing users are depending on
little more than dumb luck that firmware touched the SPI is worrysome.

> I was thinking of a transitory patch that did:
> 
> reg = readl(spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> nreg = reg & ~ORION_SPI_MODE_MASK;
> if (spi->mode & SPI_CPOL)
> 	nreg |= ORION_SPI_MODE_CPOL;
> if (spi->mode & SPI_CPHA)
> 	nreg |= ORION_SPI_MODE_CPHA;
> #ifdef TRANSITION
>  if (nreg != reg)
>         dev_error(..,"Not allowing a SPI mode change away from firmware
>  defaults. Board support needs to be updated!");
> #else
>  writel(nreg, spi_reg(orion_spi, ORION_SPI_IF_CONFIG_REG));
> #endif

Don't make this a #ifdef. Instead, you could add a property to the
device tree to enable the new behaviour... although DT support is brand
new, so you don't need to worry about the legacy scenario for DT users.
Just turn it on.

For platform_data configuration, you might want to enable it with a
flag. Whatever you decide send me a fixup patch against linux-next.

g.

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Gunthorpe]

On Thu, Dec 06, 2012 at 11:23:05PM +0000, Grant Likely wrote:

> > This will likely switch all current users from using 'whatever the
> > firmware left behind' to 'whatever the kernel default is' - which will
> > surely break something here and there??
> 
> Hmmm. Hard to say. Just the fact that existing users are depending on
> little more than dumb luck that firmware touched the SPI is worrysome.

It isn't entirely dumb luck, all the DT files I looked at were using
SPI to connect to FLASH, so the boot firmware will have to set the SPI
bus properly since it is executing out of the SPI boot flash.

At the very least, if it breaks it will be of the very obvious 'boot
from flash is completely broken' sort.

> For platform_data configuration, you might want to enable it with a
> flag. Whatever you decide send me a fixup patch against linux-next.

Okay, I think instead of the #ifdef a test for 'dev->ofdev == NULL'
would do the trick for now, then at least people can test the DT vs
the platform boot.

I am hoping to have time to revise patches on Friday, I will see.

Jason: Do you have any of these possibly affected boards with a SPI
flash to test linux-next? dove, dreamplug, ts219 and lsxl

Thanks,
Jason

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Cooper]

On Thu, Dec 06, 2012 at 04:49:17PM -0700, Jason Gunthorpe wrote:
> Jason: Do you have any of these possibly affected boards with a SPI
> flash to test linux-next? dove, dreamplug, ts219 and lsxl

Yep, dreamplug ought to do.

thx,

Jason.

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Gunthorpe]

On Thu, Dec 06, 2012 at 06:53:11PM -0500, Jason Cooper wrote:
> On Thu, Dec 06, 2012 at 04:49:17PM -0700, Jason Gunthorpe wrote:
> > Jason: Do you have any of these possibly affected boards with a SPI
> > flash to test linux-next? dove, dreamplug, ts219 and lsxl
> 
> Yep, dreamplug ought to do.

Hmm, I just reviewed all the FLASH P/Ns datasheets in the affected DT
files, and they are all mode 0/mode 3 compatible - that is the linux
default mode so I expect everything to work OK.

If your dreamplug is fine lets just leave it?

Jason

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Jason Cooper]

On Thu, Dec 06, 2012 at 05:14:33PM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 06, 2012 at 06:53:11PM -0500, Jason Cooper wrote:
> > On Thu, Dec 06, 2012 at 04:49:17PM -0700, Jason Gunthorpe wrote:
> > > Jason: Do you have any of these possibly affected boards with a SPI
> > > flash to test linux-next? dove, dreamplug, ts219 and lsxl
> > 
> > Yep, dreamplug ought to do.
> 
> Hmm, I just reviewed all the FLASH P/Ns datasheets in the affected DT
> files, and they are all mode 0/mode 3 compatible - that is the linux
> default mode so I expect everything to work OK.
> 
> If your dreamplug is fine lets just leave it?

Unfortunately, I'm unable to test until next week.  If Grant is
comfortable with it as is, I have no objection.  If he isn't, I'll try
to test it next week when I return home.

thx,

Jason.

[PATCH] ARM: Kirkwood: Add SPI_CHPA and SPI_CPOL support to spi-orion

[Grant Likely]

On Thu, 6 Dec 2012 19:49:33 -0500, Jason Cooper <jason@lakedaemon.net> wrote:
> On Thu, Dec 06, 2012 at 05:14:33PM -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 06, 2012 at 06:53:11PM -0500, Jason Cooper wrote:
> > > On Thu, Dec 06, 2012 at 04:49:17PM -0700, Jason Gunthorpe wrote:
> > > > Jason: Do you have any of these possibly affected boards with a SPI
> > > > flash to test linux-next? dove, dreamplug, ts219 and lsxl
> > > 
> > > Yep, dreamplug ought to do.
> > 
> > Hmm, I just reviewed all the FLASH P/Ns datasheets in the affected DT
> > files, and they are all mode 0/mode 3 compatible - that is the linux
> > default mode so I expect everything to work OK.
> > 
> > If your dreamplug is fine lets just leave it?
> 
> Unfortunately, I'm unable to test until next week.  If Grant is
> comfortable with it as is, I have no objection.  If he isn't, I'll try
> to test it next week when I return home.

I'm comfortable taking it. In the unlikely event it breaks then it can
be fixed up during stablization.

g.