| Message ID | 20121121192335.GA14868@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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.
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
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.
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
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.
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.
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;