| Message ID | 1489378139-11707-1-git-send-email-gerg@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Greg, On Mon, Mar 13, 2017 at 5:08 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: > I have an iMX253 based system with 2 SPI buses, one with a flash hanging > of it, and one with a SLIC. On modern kernels (I am using 4.9 - but I > think current mainline 4.11-rc2 is the same) the SPI bus with the SLIC > device can't be configured to work with a devicetree setup. > > The problem is that the SLIC device hardware arrangement needs to use the > iMX SPI native chipselect to provide the neccessary hardware cycles. > Maybe I am missing something but I can't see how that can be made to work > with the current devicetree setup required on this platform. > > It would appear that using a cs_gpio field of "<0>" is meant to indicate > use of a native chipselect - though I couldn't find that documented > anywhere. In any case I couldn't see any other way to do it either. There's no need to use "cs-gpios = <0>". You should just leave out the "cs-gpios" property for SPI slaves using native chip select, and the driver should use the native chip select, based on the value of the "reg" property. > But with a cs_gpio field set to "<0>" the config code doesn't correctly > configure the iMX SPI registers to use a native chipselect. In fact it > actively looks wrong in the way it mangles the cs_gpio to calculate a > native chipselect when using a devicetree (it looks like it would be > correct for the platform setup case though). I cannot comment on the iMX hardware specifics. > The commonly used mechanism of specifying the native chipselect on an > SPI device in devicetree (that is "cs-gpios = <0> ...") does not result > in the native chipselect being configured for use. So external SPI > devices that require use of the native chipselect will not work. > > You can successfully specify native chipselects if using a platform > setup by specifying the cs-gpio as negative offset by 32. And that > looks to be working correctly. Just don't use "cs-gpios", cfr. above. > The logic in the spi-imx.c driver during probe uses core spi function > of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag. > For valid GPIO values that will be recorded for use, all other entries in > cs_gpios list will be set to -ENOENT. So entries like "<0>" will be set > to -ENOENT in the cs_gpios list. Yep, that's what the core SPI code does. > When the SPI device registers are setup the code will use the GPIO > listed in the cs_gpios list for the desired chipselect. If the cs_gpio > is less then 0 then it is intended to be for a native chipselect, and > its cs_gpio value is added to 32 to get the chipselect number to use. > Problem is that with devicetree this can only ever be -ENOENT (which > is -2), and that alone results in an invalid chipselect number. But also > doesn't allow selection of the native chipselect at all. Ugh, that's indeed wrong. > To fix I modified the setup logic so that if cs_gpio is less than 0, > and it is -ENOENT, then we use the chipselect number associated with > this spi device. This will allow platform setups to continue to be able > to specify the chipselect number they want, and on devicetree systems > the absence of a valid GPIO will use the native chipselect. Ah, the real issue is that spi_new_device() / spi_add_device() (called from spi_register_board_info()) don't seem to support mixing cs_gpio and native CS? > Acked-by: Greg Ungerer <gerg@linux-m68k.org> SoB? (Wrong preprogrammed key? ;-) > --- > drivers/spi/spi-imx.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index 9a7c62f..c6e13f7 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -516,10 +516,12 @@ static int mx31_config(struct spi_device *spi, struct spi_imx_config *config) > reg |= MX31_CSPICTRL_POL; > if (spi->mode & SPI_CS_HIGH) > reg |= MX31_CSPICTRL_SSPOL; > - if (spi->cs_gpio < 0) > - reg |= (spi->cs_gpio + 32) << > - (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : > - MX31_CSPICTRL_CS_SHIFT); > + if (spi->cs_gpio < 0) { if (!gpio_is_valid(spi->cs_gpio)) { > + int cs = (spi->cs_gpio == -ENOENT) ? spi->chip_select : > + spi->cs_gpio + 32; I don't think the check for -ENOENT is needed, and thus you can always just use spi->chip-select directly. Same comments for the mx21 code. 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 linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 13/03/17 18:29, Geert Uytterhoeven wrote: > On Mon, Mar 13, 2017 at 5:08 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: >> I have an iMX253 based system with 2 SPI buses, one with a flash hanging >> of it, and one with a SLIC. On modern kernels (I am using 4.9 - but I >> think current mainline 4.11-rc2 is the same) the SPI bus with the SLIC >> device can't be configured to work with a devicetree setup. >> >> The problem is that the SLIC device hardware arrangement needs to use the >> iMX SPI native chipselect to provide the neccessary hardware cycles. >> Maybe I am missing something but I can't see how that can be made to work >> with the current devicetree setup required on this platform. >> >> It would appear that using a cs_gpio field of "<0>" is meant to indicate >> use of a native chipselect - though I couldn't find that documented >> anywhere. In any case I couldn't see any other way to do it either. > > There's no need to use "cs-gpios = <0>". You should just leave out the > "cs-gpios" property for SPI slaves using native chip select, and the driver > should use the native chip select, based on the value of the "reg" property. I agree, and that was my first thought. But the spi-imx.c driver will explicitly fail in the probe with: spi.1: No CS GPIOs available if you don't have a "cs-gpios" tag in the devicetree entry. >> But with a cs_gpio field set to "<0>" the config code doesn't correctly >> configure the iMX SPI registers to use a native chipselect. In fact it >> actively looks wrong in the way it mangles the cs_gpio to calculate a >> native chipselect when using a devicetree (it looks like it would be >> correct for the platform setup case though). > > I cannot comment on the iMX hardware specifics. > >> The commonly used mechanism of specifying the native chipselect on an >> SPI device in devicetree (that is "cs-gpios = <0> ...") does not result >> in the native chipselect being configured for use. So external SPI >> devices that require use of the native chipselect will not work. >> >> You can successfully specify native chipselects if using a platform >> setup by specifying the cs-gpio as negative offset by 32. And that >> looks to be working correctly. > > Just don't use "cs-gpios", cfr. above. In addition to fixing the actual register setting code, I think the spi-imx.c driver needs to be fixed to allow this too. But that is for another patch. >> The logic in the spi-imx.c driver during probe uses core spi function >> of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag. >> For valid GPIO values that will be recorded for use, all other entries in >> cs_gpios list will be set to -ENOENT. So entries like "<0>" will be set >> to -ENOENT in the cs_gpios list. > > Yep, that's what the core SPI code does. > >> When the SPI device registers are setup the code will use the GPIO >> listed in the cs_gpios list for the desired chipselect. If the cs_gpio >> is less then 0 then it is intended to be for a native chipselect, and >> its cs_gpio value is added to 32 to get the chipselect number to use. >> Problem is that with devicetree this can only ever be -ENOENT (which >> is -2), and that alone results in an invalid chipselect number. But also >> doesn't allow selection of the native chipselect at all. > > Ugh, that's indeed wrong. > >> To fix I modified the setup logic so that if cs_gpio is less than 0, >> and it is -ENOENT, then we use the chipselect number associated with >> this spi device. This will allow platform setups to continue to be able >> to specify the chipselect number they want, and on devicetree systems >> the absence of a valid GPIO will use the native chipselect. > > Ah, the real issue is that spi_new_device() / spi_add_device() (called > from spi_register_board_info()) don't seem to support mixing cs_gpio > and native CS? > >> Acked-by: Greg Ungerer <gerg@linux-m68k.org> > > SoB? (Wrong preprogrammed key? ;-) Ah... brain not engaged after debugging this :-) >> --- >> drivers/spi/spi-imx.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >> index 9a7c62f..c6e13f7 100644 >> --- a/drivers/spi/spi-imx.c >> +++ b/drivers/spi/spi-imx.c >> @@ -516,10 +516,12 @@ static int mx31_config(struct spi_device *spi, struct spi_imx_config *config) >> reg |= MX31_CSPICTRL_POL; >> if (spi->mode & SPI_CS_HIGH) >> reg |= MX31_CSPICTRL_SSPOL; >> - if (spi->cs_gpio < 0) >> - reg |= (spi->cs_gpio + 32) << >> - (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : >> - MX31_CSPICTRL_CS_SHIFT); >> + if (spi->cs_gpio < 0) { > > if (!gpio_is_valid(spi->cs_gpio)) { > >> + int cs = (spi->cs_gpio == -ENOENT) ? spi->chip_select : >> + spi->cs_gpio + 32; > > I don't think the check for -ENOENT is needed, and thus you can always > just use spi->chip-select directly. > > Same comments for the mx21 code. I suspect that is the case. I didn't check back through all the platform setups that currently rely on this "+ 32" mapping logic. I would expect that they should all end up with spi->chip_select being correct (and not needing cs_gpio here at all). Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 13/03/17 22:26, Greg Ungerer wrote: > On 13/03/17 18:29, Geert Uytterhoeven wrote: >> On Mon, Mar 13, 2017 at 5:08 AM, Greg Ungerer <gerg@linux-m68k.org> >> wrote: [snip] >>> --- >>> drivers/spi/spi-imx.c | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>> index 9a7c62f..c6e13f7 100644 >>> --- a/drivers/spi/spi-imx.c >>> +++ b/drivers/spi/spi-imx.c >>> @@ -516,10 +516,12 @@ static int mx31_config(struct spi_device *spi, >>> struct spi_imx_config *config) >>> reg |= MX31_CSPICTRL_POL; >>> if (spi->mode & SPI_CS_HIGH) >>> reg |= MX31_CSPICTRL_SSPOL; >>> - if (spi->cs_gpio < 0) >>> - reg |= (spi->cs_gpio + 32) << >>> - (is_imx35_cspi(spi_imx) ? >>> MX35_CSPICTRL_CS_SHIFT : >>> - >>> MX31_CSPICTRL_CS_SHIFT); >>> + if (spi->cs_gpio < 0) { >> >> if (!gpio_is_valid(spi->cs_gpio)) { >> >>> + int cs = (spi->cs_gpio == -ENOENT) ? spi->chip_select : >>> + spi->cs_gpio + 32; >> >> I don't think the check for -ENOENT is needed, and thus you can always >> just use spi->chip-select directly. >> >> Same comments for the mx21 code. > > I suspect that is the case. I didn't check back through all the > platform setups that currently rely on this "+ 32" mapping logic. > I would expect that they should all end up with spi->chip_select > being correct (and not needing cs_gpio here at all). It looks as if some platform setups don't map the appropriate chipselect to entries in the cs-gpios array. For example in arch/arm/mach-imx/mach-mx31_3ds.c it sets chipselect=1 but the cs-gpio equivalent entry is actually for native chipselect 2. There are a few other examples of this too. Now, these could be fixed to. Harder to test properly though without access to the effected boards. Regards Greg -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 9a7c62f..c6e13f7 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -516,10 +516,12 @@ static int mx31_config(struct spi_device *spi, struct spi_imx_config *config) reg |= MX31_CSPICTRL_POL; if (spi->mode & SPI_CS_HIGH) reg |= MX31_CSPICTRL_SSPOL; - if (spi->cs_gpio < 0) - reg |= (spi->cs_gpio + 32) << - (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : - MX31_CSPICTRL_CS_SHIFT); + if (spi->cs_gpio < 0) { + int cs = (spi->cs_gpio == -ENOENT) ? spi->chip_select : + spi->cs_gpio + 32; + reg |= cs << (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : + MX31_CSPICTRL_CS_SHIFT); + } if (spi_imx->usedma) reg |= MX31_CSPICTRL_SMC; @@ -608,8 +610,11 @@ static int mx21_config(struct spi_device *spi, struct spi_imx_config *config) reg |= MX21_CSPICTRL_POL; if (spi->mode & SPI_CS_HIGH) reg |= MX21_CSPICTRL_SSPOL; - if (spi->cs_gpio < 0) - reg |= (spi->cs_gpio + 32) << MX21_CSPICTRL_CS_SHIFT; + if (spi->cs_gpio < 0) { + int cs = (spi->cs_gpio == -ENOENT) ? spi->chip_select : + spi->cs_gpio + 32; + reg |= cs << MX21_CSPICTRL_CS_SHIFT; + } writel(reg, spi_imx->base + MXC_CSPICTRL);
I have an iMX253 based system with 2 SPI buses, one with a flash hanging of it, and one with a SLIC. On modern kernels (I am using 4.9 - but I think current mainline 4.11-rc2 is the same) the SPI bus with the SLIC device can't be configured to work with a devicetree setup. The problem is that the SLIC device hardware arrangement needs to use the iMX SPI native chipselect to provide the neccessary hardware cycles. Maybe I am missing something but I can't see how that can be made to work with the current devicetree setup required on this platform. It would appear that using a cs_gpio field of "<0>" is meant to indicate use of a native chipselect - though I couldn't find that documented anywhere. In any case I couldn't see any other way to do it either. But with a cs_gpio field set to "<0>" the config code doesn't correctly configure the iMX SPI registers to use a native chipselect. In fact it actively looks wrong in the way it mangles the cs_gpio to calculate a native chipselect when using a devicetree (it looks like it would be correct for the platform setup case though). Below is the patch I came up with to fix it for my hardware. This results in the correct native chipselect being set for my SLIC hardware and it all works nicely. Is this on the right track or have I missed something comepletely? -------- The commonly used mechanism of specifying the native chipselect on an SPI device in devicetree (that is "cs-gpios = <0> ...") does not result in the native chipselect being configured for use. So external SPI devices that require use of the native chipselect will not work. You can successfully specify native chipselects if using a platform setup by specifying the cs-gpio as negative offset by 32. And that looks to be working correctly. The logic in the spi-imx.c driver during probe uses core spi function of_spi_register_master() in spi.c to parse the "cs-gpios" devicetree tag. For valid GPIO values that will be recorded for use, all other entries in cs_gpios list will be set to -ENOENT. So entries like "<0>" will be set to -ENOENT in the cs_gpios list. When the SPI device registers are setup the code will use the GPIO listed in the cs_gpios list for the desired chipselect. If the cs_gpio is less then 0 then it is intended to be for a native chipselect, and its cs_gpio value is added to 32 to get the chipselect number to use. Problem is that with devicetree this can only ever be -ENOENT (which is -2), and that alone results in an invalid chipselect number. But also doesn't allow selection of the native chipselect at all. To fix I modified the setup logic so that if cs_gpio is less than 0, and it is -ENOENT, then we use the chipselect number associated with this spi device. This will allow platform setups to continue to be able to specify the chipselect number they want, and on devicetree systems the absence of a valid GPIO will use the native chipselect. Acked-by: Greg Ungerer <gerg@linux-m68k.org> --- drivers/spi/spi-imx.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)