| Message ID | 1499746932-14850-2-git-send-email-gerg@linux-m68k.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hello Greg, On 07/11/2017 07:22 AM, Greg Ungerer wrote: > The commonly used mechanism of specifying the hardware or native > chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") > does not result in the native chip-select being configured for use. > So external SPI devices that require use of the native chip-select > will not work. > > You can successfully specify native chip-selects if using a platform > setup by specifying the cs-gpio as negative offset by 32. And that > works correctly. You cannot use the same method in devicetree. > > 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 > the 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 chip-select. If the cs_gpio > is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also > doesn't allow selection of the native chip-select at all. > > To fix, if the cs_gpio specified for this spi device is not a > valid GPIO then use the "chip_select" (that is the native chip-select > number) for hardware setup. > > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> > --- > drivers/spi/spi-imx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index b402530..f4fe66c 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -524,8 +524,8 @@ 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) << > + if (!gpio_is_valid(spi->cs_gpio)) > + reg |= (spi->chip_select) << > (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : > MX31_CSPICTRL_CS_SHIFT); > > @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) > + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; > > writel(reg, spi_imx->base + MXC_CSPICTRL); > > FWIW I still didn't manage to utilize the second and so on native chip selects on DT-only i.MX31 (master->num_chipselect value is preset to 1 if I remember correctly), but the experienced problem is not related to the change and it should be addressed separately. So, Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> Tested-by: Vladimir Zapolskiy <vz@mleia.com> Thank you for the change. -- With best wishes, Vladimir -- 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
Adding Pengutronix folks on Cc. On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: > The commonly used mechanism of specifying the hardware or native > chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") > does not result in the native chip-select being configured for use. > So external SPI devices that require use of the native chip-select > will not work. > > You can successfully specify native chip-selects if using a platform > setup by specifying the cs-gpio as negative offset by 32. And that > works correctly. You cannot use the same method in devicetree. > > 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 > the 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 chip-select. If the cs_gpio > is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also > doesn't allow selection of the native chip-select at all. > > To fix, if the cs_gpio specified for this spi device is not a > valid GPIO then use the "chip_select" (that is the native chip-select > number) for hardware setup. > > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> > --- > drivers/spi/spi-imx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index b402530..f4fe66c 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -524,8 +524,8 @@ 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) << > + if (!gpio_is_valid(spi->cs_gpio)) > + reg |= (spi->chip_select) << > (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : > MX31_CSPICTRL_CS_SHIFT); > > @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) > + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; > > writel(reg, spi_imx->base + MXC_CSPICTRL); > > -- > 1.9.1 > > -- > 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 -- 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 Vladimir, On 19/07/17 10:49, Vladimir Zapolskiy wrote: > Hello Greg, > > On 07/11/2017 07:22 AM, Greg Ungerer wrote: >> The commonly used mechanism of specifying the hardware or native >> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >> does not result in the native chip-select being configured for use. >> So external SPI devices that require use of the native chip-select >> will not work. >> >> You can successfully specify native chip-selects if using a platform >> setup by specifying the cs-gpio as negative offset by 32. And that >> works correctly. You cannot use the same method in devicetree. >> >> 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 >> the 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 chip-select. If the cs_gpio >> is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also >> doesn't allow selection of the native chip-select at all. >> >> To fix, if the cs_gpio specified for this spi device is not a >> valid GPIO then use the "chip_select" (that is the native chip-select >> number) for hardware setup. >> >> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >> --- >> drivers/spi/spi-imx.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >> index b402530..f4fe66c 100644 >> --- a/drivers/spi/spi-imx.c >> +++ b/drivers/spi/spi-imx.c >> @@ -524,8 +524,8 @@ 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) << >> + if (!gpio_is_valid(spi->cs_gpio)) >> + reg |= (spi->chip_select) << >> (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : >> MX31_CSPICTRL_CS_SHIFT); >> >> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >> >> writel(reg, spi_imx->base + MXC_CSPICTRL); >> >> > > FWIW I still didn't manage to utilize the second and so on native chip > selects on DT-only i.MX31 (master->num_chipselect value is preset to 1 > if I remember correctly), but the experienced problem is not related > to the change and it should be addressed separately. > > So, > > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > Tested-by: Vladimir Zapolskiy <vz@mleia.com> > > Thank you for the change. Thanks for the feedback. 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 Greg, On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: > Adding Pengutronix folks on Cc. > > On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: > > The commonly used mechanism of specifying the hardware or native > > chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") > > does not result in the native chip-select being configured for use. > > So external SPI devices that require use of the native chip-select > > will not work. > > > > You can successfully specify native chip-selects if using a platform > > setup by specifying the cs-gpio as negative offset by 32. And that > > works correctly. You cannot use the same method in devicetree. > > > > 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 > > the 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 chip-select. If the cs_gpio > > is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also > > doesn't allow selection of the native chip-select at all. > > > > To fix, if the cs_gpio specified for this spi device is not a > > valid GPIO then use the "chip_select" (that is the native chip-select > > number) for hardware setup. > > > > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> > > --- > > drivers/spi/spi-imx.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > > index b402530..f4fe66c 100644 > > --- a/drivers/spi/spi-imx.c > > +++ b/drivers/spi/spi-imx.c > > @@ -524,8 +524,8 @@ 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) << > > + if (!gpio_is_valid(spi->cs_gpio)) > > + reg |= (spi->chip_select) << > > (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : > > MX31_CSPICTRL_CS_SHIFT); > > > > @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) > > + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; > > > > writel(reg, spi_imx->base + MXC_CSPICTRL); > > hm... do I see this correctly, all native chip_selects should be registered before gpio based CS? For example like this? cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; Looks like we don't have any sanity checks for this kind of configuration: cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; We may shift some wired numbers here: reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; this should be reflected in the documentation too: Documentation/devicetree/bindings/spi/spi-bus.txt
Hi Oleksij, On 20/07/17 16:34, Oleksij Rempel wrote: > Hi Greg, > > On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: >> Adding Pengutronix folks on Cc. >> >> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: >>> The commonly used mechanism of specifying the hardware or native >>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >>> does not result in the native chip-select being configured for use. >>> So external SPI devices that require use of the native chip-select >>> will not work. >>> >>> You can successfully specify native chip-selects if using a platform >>> setup by specifying the cs-gpio as negative offset by 32. And that >>> works correctly. You cannot use the same method in devicetree. >>> >>> 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 >>> the 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 chip-select. If the cs_gpio >>> is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also >>> doesn't allow selection of the native chip-select at all. >>> >>> To fix, if the cs_gpio specified for this spi device is not a >>> valid GPIO then use the "chip_select" (that is the native chip-select >>> number) for hardware setup. >>> >>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >>> --- >>> drivers/spi/spi-imx.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>> index b402530..f4fe66c 100644 >>> --- a/drivers/spi/spi-imx.c >>> +++ b/drivers/spi/spi-imx.c >>> @@ -524,8 +524,8 @@ 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) << >>> + if (!gpio_is_valid(spi->cs_gpio)) >>> + reg |= (spi->chip_select) << >>> (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : >>> MX31_CSPICTRL_CS_SHIFT); >>> >>> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >>> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>> >>> writel(reg, spi_imx->base + MXC_CSPICTRL); >>> > > hm... do I see this correctly, all native chip_selects should > be registered before gpio based CS? I don't follow. The "<0>" must be in the position in the list where you want to use the native chip select. You can't arbitrarily change the order. > For example like this? > cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; > > Looks like we don't have any sanity checks for this kind of > configuration: > cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; The chip_select is sanity checked in spi_add_device(). > We may shift some wired numbers here: > reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; I am not sure I see how that can be the case? Regards Greg > this should be reflected in the documentation too: > Documentation/devicetree/bindings/spi/spi-bus.txt > -- 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
On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote: > Hi Oleksij, > > On 20/07/17 16:34, Oleksij Rempel wrote: > > Hi Greg, > > > > On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: > > > Adding Pengutronix folks on Cc. > > > > > > On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: > > > > The commonly used mechanism of specifying the hardware or native > > > > chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") > > > > does not result in the native chip-select being configured for use. > > > > So external SPI devices that require use of the native chip-select > > > > will not work. > > > > > > > > You can successfully specify native chip-selects if using a platform > > > > setup by specifying the cs-gpio as negative offset by 32. And that > > > > works correctly. You cannot use the same method in devicetree. > > > > > > > > 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 > > > > the 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 chip-select. If the cs_gpio > > > > is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also > > > > doesn't allow selection of the native chip-select at all. > > > > > > > > To fix, if the cs_gpio specified for this spi device is not a > > > > valid GPIO then use the "chip_select" (that is the native chip-select > > > > number) for hardware setup. > > > > > > > > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> > > > > --- > > > > drivers/spi/spi-imx.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > > > > index b402530..f4fe66c 100644 > > > > --- a/drivers/spi/spi-imx.c > > > > +++ b/drivers/spi/spi-imx.c > > > > @@ -524,8 +524,8 @@ 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) << > > > > + if (!gpio_is_valid(spi->cs_gpio)) > > > > + reg |= (spi->chip_select) << > > > > (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : > > > > MX31_CSPICTRL_CS_SHIFT); > > > > > > > > @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) > > > > + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; > > > > > > > > writel(reg, spi_imx->base + MXC_CSPICTRL); > > > > > > > > hm... do I see this correctly, all native chip_selects should > > be registered before gpio based CS? > > I don't follow. The "<0>" must be in the position in the list where > you want to use the native chip select. You can't arbitrarily change > the order. > > > > For example like this? > > cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; > > > > Looks like we don't have any sanity checks for this kind of > > configuration: > > cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; > > The chip_select is sanity checked in spi_add_device(). > > > > We may shift some wired numbers here: > > reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; > > I am not sure I see how that can be the case? old and new version of iMX have different amount of native CS. I can't find the code which is actually checking if we use right native CS-index. May be i'm blind :)
Hi Qleksij, On 24/07/17 16:21, Oleksij Rempel wrote: > On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote: >> Hi Oleksij, >> >> On 20/07/17 16:34, Oleksij Rempel wrote: >>> Hi Greg, >>> >>> On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: >>>> Adding Pengutronix folks on Cc. >>>> >>>> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> wrote: >>>>> The commonly used mechanism of specifying the hardware or native >>>>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >>>>> does not result in the native chip-select being configured for use. >>>>> So external SPI devices that require use of the native chip-select >>>>> will not work. >>>>> >>>>> You can successfully specify native chip-selects if using a platform >>>>> setup by specifying the cs-gpio as negative offset by 32. And that >>>>> works correctly. You cannot use the same method in devicetree. >>>>> >>>>> 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 >>>>> the 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 chip-select. If the cs_gpio >>>>> is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also >>>>> doesn't allow selection of the native chip-select at all. >>>>> >>>>> To fix, if the cs_gpio specified for this spi device is not a >>>>> valid GPIO then use the "chip_select" (that is the native chip-select >>>>> number) for hardware setup. >>>>> >>>>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >>>>> --- >>>>> drivers/spi/spi-imx.c | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>>>> index b402530..f4fe66c 100644 >>>>> --- a/drivers/spi/spi-imx.c >>>>> +++ b/drivers/spi/spi-imx.c >>>>> @@ -524,8 +524,8 @@ 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) << >>>>> + if (!gpio_is_valid(spi->cs_gpio)) >>>>> + reg |= (spi->chip_select) << >>>>> (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : >>>>> MX31_CSPICTRL_CS_SHIFT); >>>>> >>>>> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >>>>> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>>> >>>>> writel(reg, spi_imx->base + MXC_CSPICTRL); >>>>> >>> >>> hm... do I see this correctly, all native chip_selects should >>> be registered before gpio based CS? >> >> I don't follow. The "<0>" must be in the position in the list where >> you want to use the native chip select. You can't arbitrarily change >> the order. >> >> >>> For example like this? >>> cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; >>> >>> Looks like we don't have any sanity checks for this kind of >>> configuration: >>> cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; >> >> The chip_select is sanity checked in spi_add_device(). >> >> >>> We may shift some wired numbers here: >>> reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >> >> I am not sure I see how that can be the case? > > old and new version of iMX have different amount of native CS. > I can't find the code which is actually checking if we use right native > CS-index. > May be i'm blind :) I don't think I entirely understand what you are saying. The code at the top of spi_add_device() [drivers/spi/spi.c] looks like this: /* Chipselects are numbered 0..max; validate. */ if (spi->chip_select >= ctlr->num_chipselect) { dev_err(dev, "cs%d >= max %d\n", spi->chip_select, ctlr->num_chipselect); return -EINVAL; } So it will range check the spi device (spi->chip_select) to be within the range valid for this SPI controller. That is the very same spi->chip_select that is used in spi-imx.c to set the register bits when using a native chip select. 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 Greg, On 09.08.2017 15:00, Greg Ungerer wrote: > Hi Qleksij, > > On 24/07/17 16:21, Oleksij Rempel wrote: >> On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote: >>> Hi Oleksij, >>> >>> On 20/07/17 16:34, Oleksij Rempel wrote: >>>> Hi Greg, >>>> >>>> On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: >>>>> Adding Pengutronix folks on Cc. >>>>> >>>>> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> >>>>> wrote: >>>>>> The commonly used mechanism of specifying the hardware or native >>>>>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >>>>>> does not result in the native chip-select being configured for use. >>>>>> So external SPI devices that require use of the native chip-select >>>>>> will not work. >>>>>> >>>>>> You can successfully specify native chip-selects if using a platform >>>>>> setup by specifying the cs-gpio as negative offset by 32. And that >>>>>> works correctly. You cannot use the same method in devicetree. >>>>>> >>>>>> 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 >>>>>> the 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 chip-select. If the >>>>>> cs_gpio >>>>>> is less then 0 then it is intended to be for a native chip-select, >>>>>> 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 chip-select number. >>>>>> But also >>>>>> doesn't allow selection of the native chip-select at all. >>>>>> >>>>>> To fix, if the cs_gpio specified for this spi device is not a >>>>>> valid GPIO then use the "chip_select" (that is the native chip-select >>>>>> number) for hardware setup. >>>>>> >>>>>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >>>>>> --- >>>>>> drivers/spi/spi-imx.c | 8 ++++---- >>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>>>>> index b402530..f4fe66c 100644 >>>>>> --- a/drivers/spi/spi-imx.c >>>>>> +++ b/drivers/spi/spi-imx.c >>>>>> @@ -524,8 +524,8 @@ 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) << >>>>>> + if (!gpio_is_valid(spi->cs_gpio)) >>>>>> + reg |= (spi->chip_select) << >>>>>> (is_imx35_cspi(spi_imx) ? >>>>>> MX35_CSPICTRL_CS_SHIFT : >>>>>> >>>>>> MX31_CSPICTRL_CS_SHIFT); >>>>>> >>>>>> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >>>>>> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>>>> >>>>>> writel(reg, spi_imx->base + MXC_CSPICTRL); >>>>>> >>>> >>>> hm... do I see this correctly, all native chip_selects should >>>> be registered before gpio based CS? >>> >>> I don't follow. The "<0>" must be in the position in the list where >>> you want to use the native chip select. You can't arbitrarily change >>> the order. >>> >>> >>>> For example like this? >>>> cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; >>>> >>>> Looks like we don't have any sanity checks for this kind of >>>> configuration: >>>> cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; >>> >>> The chip_select is sanity checked in spi_add_device(). >>> >>> >>>> We may shift some wired numbers here: >>>> reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>> >>> I am not sure I see how that can be the case? >> >> old and new version of iMX have different amount of native CS. >> I can't find the code which is actually checking if we use right native >> CS-index. >> May be i'm blind :) > > I don't think I entirely understand what you are saying. The code at the > top of spi_add_device() [drivers/spi/spi.c] looks like this: > > /* Chipselects are numbered 0..max; validate. */ > if (spi->chip_select >= ctlr->num_chipselect) { > dev_err(dev, "cs%d >= max %d\n", spi->chip_select, > ctlr->num_chipselect); > return -EINVAL; > } > > So it will range check the spi device (spi->chip_select) to be within > the range valid for this SPI controller. That is the very same > spi->chip_select that is used in spi-imx.c to set the register bits > when using a native chip select. Correct. This is how ctlr->num_chipselect initialized: nb = of_gpio_named_count(np, "cs-gpios"); ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); it will take the count of cs-gpios. The i.MX233 has 3 native CSs (SSn) and i.MX6D/Q has 4 CSs - controlled by 2 bits. Lets assume in both cases we wish to use 5CSs, some of them are GPIOs. We will use same line for devicetree on i.MX233 and i.MX6D/Q: cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <0>, <&gpio1 4 0>; If i see it correctly, spi.c and imx-spi.c will just take it. But in case of i.MX6 it should work and on i.MX233 it should silently fail. And in this case: cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; we should produce a 3 bit value b100 which will be shifted left and "or"-ed with other ctrl bits. -- 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 Oleksij, On 08/10/2017 09:09 AM, Oleksij Rempel wrote: > Hi Greg, > > On 09.08.2017 15:00, Greg Ungerer wrote: >> Hi Qleksij, >> >> On 24/07/17 16:21, Oleksij Rempel wrote: >>> On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote: >>>> Hi Oleksij, >>>> >>>> On 20/07/17 16:34, Oleksij Rempel wrote: >>>>> Hi Greg, >>>>> >>>>> On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: >>>>>> Adding Pengutronix folks on Cc. >>>>>> >>>>>> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> >>>>>> wrote: >>>>>>> The commonly used mechanism of specifying the hardware or native >>>>>>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >>>>>>> does not result in the native chip-select being configured for use. >>>>>>> So external SPI devices that require use of the native chip-select >>>>>>> will not work. >>>>>>> >>>>>>> You can successfully specify native chip-selects if using a platform >>>>>>> setup by specifying the cs-gpio as negative offset by 32. And that >>>>>>> works correctly. You cannot use the same method in devicetree. >>>>>>> >>>>>>> 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 >>>>>>> the 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 chip-select. If the >>>>>>> cs_gpio >>>>>>> is less then 0 then it is intended to be for a native chip-select, >>>>>>> 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 chip-select number. >>>>>>> But also >>>>>>> doesn't allow selection of the native chip-select at all. >>>>>>> >>>>>>> To fix, if the cs_gpio specified for this spi device is not a >>>>>>> valid GPIO then use the "chip_select" (that is the native chip-select >>>>>>> number) for hardware setup. >>>>>>> >>>>>>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >>>>>>> --- >>>>>>> drivers/spi/spi-imx.c | 8 ++++---- >>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>>>>>> index b402530..f4fe66c 100644 >>>>>>> --- a/drivers/spi/spi-imx.c >>>>>>> +++ b/drivers/spi/spi-imx.c >>>>>>> @@ -524,8 +524,8 @@ 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) << >>>>>>> + if (!gpio_is_valid(spi->cs_gpio)) >>>>>>> + reg |= (spi->chip_select) << >>>>>>> (is_imx35_cspi(spi_imx) ? >>>>>>> MX35_CSPICTRL_CS_SHIFT : >>>>>>> >>>>>>> MX31_CSPICTRL_CS_SHIFT); >>>>>>> >>>>>>> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >>>>>>> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>>>>> >>>>>>> writel(reg, spi_imx->base + MXC_CSPICTRL); >>>>>>> >>>>> >>>>> hm... do I see this correctly, all native chip_selects should >>>>> be registered before gpio based CS? >>>> >>>> I don't follow. The "<0>" must be in the position in the list where >>>> you want to use the native chip select. You can't arbitrarily change >>>> the order. >>>> >>>> >>>>> For example like this? >>>>> cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; >>>>> >>>>> Looks like we don't have any sanity checks for this kind of >>>>> configuration: >>>>> cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; >>>> >>>> The chip_select is sanity checked in spi_add_device(). >>>> >>>> >>>>> We may shift some wired numbers here: >>>>> reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>> >>>> I am not sure I see how that can be the case? >>> >>> old and new version of iMX have different amount of native CS. >>> I can't find the code which is actually checking if we use right native >>> CS-index. >>> May be i'm blind :) >> >> I don't think I entirely understand what you are saying. The code at the >> top of spi_add_device() [drivers/spi/spi.c] looks like this: >> >> /* Chipselects are numbered 0..max; validate. */ >> if (spi->chip_select >= ctlr->num_chipselect) { >> dev_err(dev, "cs%d >= max %d\n", spi->chip_select, >> ctlr->num_chipselect); >> return -EINVAL; >> } >> >> So it will range check the spi device (spi->chip_select) to be within >> the range valid for this SPI controller. That is the very same >> spi->chip_select that is used in spi-imx.c to set the register bits >> when using a native chip select. > > Correct. This is how ctlr->num_chipselect initialized: > > nb = of_gpio_named_count(np, "cs-gpios"); > ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); > > it will take the count of cs-gpios. > > The i.MX233 has 3 native CSs (SSn) and i.MX6D/Q has 4 CSs - controlled > by 2 bits. Lets assume in both cases we wish to use 5CSs, some of them > are GPIOs. > > We will use same line for devicetree on i.MX233 and i.MX6D/Q: > cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <0>, <&gpio1 4 0>; > > If i see it correctly, spi.c and imx-spi.c will just take it. But in > case of i.MX6 it should work and on i.MX233 it should silently fail. Errors in DTB (or platform data) may confuse a driver and lead to runtime misbehaviour. You describe an error in a board DTB, which is definitely better to handle in the SPI driver, but I don't think it is strictly mandatory to do it, because DTB errors are supposed to be fixed in DTB. May be one day a formal check of DTBs against Documentation/devicetree descriptions will be added and such DTB errors could be captured on DTB compilation stage. > And in this case: > cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; > > we should produce a 3 bit value b100 which will be shifted left and > "or"-ed with other ctrl bits. Same is here. -- With best wishes, Vladimir -- 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
On 10/08/17 19:35, Vladimir Zapolskiy wrote: > Hi Oleksij, > > On 08/10/2017 09:09 AM, Oleksij Rempel wrote: >> Hi Greg, >> >> On 09.08.2017 15:00, Greg Ungerer wrote: >>> Hi Qleksij, >>> >>> On 24/07/17 16:21, Oleksij Rempel wrote: >>>> On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote: >>>>> Hi Oleksij, >>>>> >>>>> On 20/07/17 16:34, Oleksij Rempel wrote: >>>>>> Hi Greg, >>>>>> >>>>>> On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote: >>>>>>> Adding Pengutronix folks on Cc. >>>>>>> >>>>>>> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@linux-m68k.org> >>>>>>> wrote: >>>>>>>> The commonly used mechanism of specifying the hardware or native >>>>>>>> chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") >>>>>>>> does not result in the native chip-select being configured for use. >>>>>>>> So external SPI devices that require use of the native chip-select >>>>>>>> will not work. >>>>>>>> >>>>>>>> You can successfully specify native chip-selects if using a platform >>>>>>>> setup by specifying the cs-gpio as negative offset by 32. And that >>>>>>>> works correctly. You cannot use the same method in devicetree. >>>>>>>> >>>>>>>> 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 >>>>>>>> the 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 chip-select. If the >>>>>>>> cs_gpio >>>>>>>> is less then 0 then it is intended to be for a native chip-select, >>>>>>>> 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 chip-select number. >>>>>>>> But also >>>>>>>> doesn't allow selection of the native chip-select at all. >>>>>>>> >>>>>>>> To fix, if the cs_gpio specified for this spi device is not a >>>>>>>> valid GPIO then use the "chip_select" (that is the native chip-select >>>>>>>> number) for hardware setup. >>>>>>>> >>>>>>>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> >>>>>>>> --- >>>>>>>> drivers/spi/spi-imx.c | 8 ++++---- >>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c >>>>>>>> index b402530..f4fe66c 100644 >>>>>>>> --- a/drivers/spi/spi-imx.c >>>>>>>> +++ b/drivers/spi/spi-imx.c >>>>>>>> @@ -524,8 +524,8 @@ 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) << >>>>>>>> + if (!gpio_is_valid(spi->cs_gpio)) >>>>>>>> + reg |= (spi->chip_select) << >>>>>>>> (is_imx35_cspi(spi_imx) ? >>>>>>>> MX35_CSPICTRL_CS_SHIFT : >>>>>>>> >>>>>>>> MX31_CSPICTRL_CS_SHIFT); >>>>>>>> >>>>>>>> @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) >>>>>>>> + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>>>>>> >>>>>>>> writel(reg, spi_imx->base + MXC_CSPICTRL); >>>>>>>> >>>>>> >>>>>> hm... do I see this correctly, all native chip_selects should >>>>>> be registered before gpio based CS? >>>>> >>>>> I don't follow. The "<0>" must be in the position in the list where >>>>> you want to use the native chip select. You can't arbitrarily change >>>>> the order. >>>>> >>>>> >>>>>> For example like this? >>>>>> cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>; >>>>>> >>>>>> Looks like we don't have any sanity checks for this kind of >>>>>> configuration: >>>>>> cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>; >>>>> >>>>> The chip_select is sanity checked in spi_add_device(). >>>>> >>>>> >>>>>> We may shift some wired numbers here: >>>>>> reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; >>>>> >>>>> I am not sure I see how that can be the case? >>>> >>>> old and new version of iMX have different amount of native CS. >>>> I can't find the code which is actually checking if we use right native >>>> CS-index. >>>> May be i'm blind :) >>> >>> I don't think I entirely understand what you are saying. The code at the >>> top of spi_add_device() [drivers/spi/spi.c] looks like this: >>> >>> /* Chipselects are numbered 0..max; validate. */ >>> if (spi->chip_select >= ctlr->num_chipselect) { >>> dev_err(dev, "cs%d >= max %d\n", spi->chip_select, >>> ctlr->num_chipselect); >>> return -EINVAL; >>> } >>> >>> So it will range check the spi device (spi->chip_select) to be within >>> the range valid for this SPI controller. That is the very same >>> spi->chip_select that is used in spi-imx.c to set the register bits >>> when using a native chip select. >> >> Correct. This is how ctlr->num_chipselect initialized: >> >> nb = of_gpio_named_count(np, "cs-gpios"); >> ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect); >> >> it will take the count of cs-gpios. >> >> The i.MX233 has 3 native CSs (SSn) and i.MX6D/Q has 4 CSs - controlled >> by 2 bits. Lets assume in both cases we wish to use 5CSs, some of them >> are GPIOs. >> >> We will use same line for devicetree on i.MX233 and i.MX6D/Q: >> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <0>, <&gpio1 4 0>; >> >> If i see it correctly, spi.c and imx-spi.c will just take it. But in >> case of i.MX6 it should work and on i.MX233 it should silently fail. > > Errors in DTB (or platform data) may confuse a driver and lead to runtime > misbehaviour. You describe an error in a board DTB, which is definitely > better to handle in the SPI driver, but I don't think it is strictly > mandatory to do it, because DTB errors are supposed to be fixed in DTB. > > May be one day a formal check of DTBs against Documentation/devicetree > descriptions will be added and such DTB errors could be captured on DTB > compilation stage. I completely agree with Vladmir here. Since "cs-gpios" defines the number of chip selects, as per the code you point out, it is the range limit. So if a DTB defines it wrongly then you can expect some things not to work right. The spi code quite rightly relies on the DTB definitions to be correct for proper operation. >> And in this case: >> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; >> >> we should produce a 3 bit value b100 which will be shifted left and >> "or"-ed with other ctrl bits. So the register settings will be wrong and the device will not work. You can't really expect any other behavior from an incorrect DTB. 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
On 10.08.2017 13:35, Greg Ungerer wrote: > On 10/08/17 19:35, Vladimir Zapolskiy wrote: >> Hi Oleksij, >> ... >> >> Errors in DTB (or platform data) may confuse a driver and lead to runtime >> misbehaviour. You describe an error in a board DTB, which is definitely >> better to handle in the SPI driver, but I don't think it is strictly >> mandatory to do it, because DTB errors are supposed to be fixed in DTB. >> >> May be one day a formal check of DTBs against Documentation/devicetree >> descriptions will be added and such DTB errors could be captured on DTB >> compilation stage. > > I completely agree with Vladmir here. Since "cs-gpios" defines the > number of chip selects, as per the code you point out, it is the range > limit. So if a DTB defines it wrongly then you can expect some things > not to work right. The spi code quite rightly relies on the DTB > definitions to be correct for proper operation. > > >>> And in this case: >>> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; >>> >>> we should produce a 3 bit value b100 which will be shifted left and >>> "or"-ed with other ctrl bits. > > So the register settings will be wrong and the device will not work. > You can't really expect any other behavior from an incorrect DTB. Ok :) -- 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
On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote: > On 10/08/17 19:35, Vladimir Zapolskiy wrote: > > Errors in DTB (or platform data) may confuse a driver and lead to runtime > > misbehaviour. You describe an error in a board DTB, which is definitely > > better to handle in the SPI driver, but I don't think it is strictly > > mandatory to do it, because DTB errors are supposed to be fixed in DTB. > > > > May be one day a formal check of DTBs against Documentation/devicetree > > descriptions will be added and such DTB errors could be captured on DTB > > compilation stage. > > I completely agree with Vladmir here. Since "cs-gpios" defines the > number of chip selects, as per the code you point out, it is the range > limit. So if a DTB defines it wrongly then you can expect some things > not to work right. The spi code quite rightly relies on the DTB > definitions to be correct for proper operation. > > > > > And in this case: > > > cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; > > > > > > we should produce a 3 bit value b100 which will be shifted left and > > > "or"-ed with other ctrl bits. > > So the register settings will be wrong and the device will not work. > You can't really expect any other behavior from an incorrect DTB. I think nobody expects that a wrong dtb is good enough to make everything work. Maybe you can argue what should happen in a driver if it gets a wrong dtb. It can make the kernel oops, just silently not work or issue an error message; probably there are more options. The current state is that a broken dtb makes the spi-imx driver write to a unrelated register bit when asked to set a chip select signal. Oleksij tries to improve that and make the driver error out instead. It makes it easier for users to report problems or find the culprit themselves. I like that. I don't care much if you call it a driver bug or a dtb bug if the above happens. For sure the error checking that Oleksij introduces isn't mandatory, but still it improves the driver. Thanks Uwe
On 10/08/17 21:49, Uwe Kleine-König wrote: > On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote: >> On 10/08/17 19:35, Vladimir Zapolskiy wrote: >>> Errors in DTB (or platform data) may confuse a driver and lead to runtime >>> misbehaviour. You describe an error in a board DTB, which is definitely >>> better to handle in the SPI driver, but I don't think it is strictly >>> mandatory to do it, because DTB errors are supposed to be fixed in DTB. >>> >>> May be one day a formal check of DTBs against Documentation/devicetree >>> descriptions will be added and such DTB errors could be captured on DTB >>> compilation stage. >> >> I completely agree with Vladmir here. Since "cs-gpios" defines the >> number of chip selects, as per the code you point out, it is the range >> limit. So if a DTB defines it wrongly then you can expect some things >> not to work right. The spi code quite rightly relies on the DTB >> definitions to be correct for proper operation. >> >> >>>> And in this case: >>>> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; >>>> >>>> we should produce a 3 bit value b100 which will be shifted left and >>>> "or"-ed with other ctrl bits. >> >> So the register settings will be wrong and the device will not work. >> You can't really expect any other behavior from an incorrect DTB. > > I think nobody expects that a wrong dtb is good enough to make > everything work. Maybe you can argue what should happen in a driver if > it gets a wrong dtb. It can make the kernel oops, just silently not work > or issue an error message; probably there are more options. > > The current state is that a broken dtb makes the spi-imx driver write to > a unrelated register bit when asked to set a chip select signal. Oleksij > tries to improve that and make the driver error out instead. It makes it > easier for users to report problems or find the culprit themselves. I > like that. But the check if the chip select is valid is based on information that comes from the DTB. In the example of the wrong cs-gpios list the DTB is saying it has more chip selects that the actual hardware device does. How can you possibly protect against that? The current code seems to do its best to range check the chip select, based on what the DTB says this hardware has. > I don't care much if you call it a driver bug or a dtb bug if the above > happens. For sure the error checking that Oleksij introduces isn't > mandatory, but still it improves the driver. How can you implement an improved error check? You need the maximal range to check it, the hardware register definition in the iMX25 has 2bits - with reserved bits above it. You could reasonably guess that they have left space for up to 4 bits to select the chip select. In any case even if you somewhat arbitrarily limit the chip select to 2bits you still are not giving an error for the hardware with a 1bit selection. Of if some devices do have 4bits for chip select and you set the range limit to 16 chip selects then you won't produce an error for the case Oleksij presents above. 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
Hello Greg, On Thu, Aug 10, 2017 at 10:24:09PM +1000, Greg Ungerer wrote: > On 10/08/17 21:49, Uwe Kleine-König wrote: > > On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote: > > > On 10/08/17 19:35, Vladimir Zapolskiy wrote: > > > > Errors in DTB (or platform data) may confuse a driver and lead to runtime > > > > misbehaviour. You describe an error in a board DTB, which is definitely > > > > better to handle in the SPI driver, but I don't think it is strictly > > > > mandatory to do it, because DTB errors are supposed to be fixed in DTB. > > > > > > > > May be one day a formal check of DTBs against Documentation/devicetree > > > > descriptions will be added and such DTB errors could be captured on DTB > > > > compilation stage. > > > > > > I completely agree with Vladmir here. Since "cs-gpios" defines the > > > number of chip selects, as per the code you point out, it is the range > > > limit. So if a DTB defines it wrongly then you can expect some things > > > not to work right. The spi code quite rightly relies on the DTB > > > definitions to be correct for proper operation. > > > > > > > > > > > And in this case: > > > > > cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; > > > > > > > > > > we should produce a 3 bit value b100 which will be shifted left and > > > > > "or"-ed with other ctrl bits. > > > > > > So the register settings will be wrong and the device will not work. > > > You can't really expect any other behavior from an incorrect DTB. > > > > I think nobody expects that a wrong dtb is good enough to make > > everything work. Maybe you can argue what should happen in a driver if > > it gets a wrong dtb. It can make the kernel oops, just silently not work > > or issue an error message; probably there are more options. > > > > The current state is that a broken dtb makes the spi-imx driver write to > > a unrelated register bit when asked to set a chip select signal. Oleksij > > tries to improve that and make the driver error out instead. It makes it > > easier for users to report problems or find the culprit themselves. I > > like that. > > But the check if the chip select is valid is based on information > that comes from the DTB. In the example of the wrong cs-gpios list > the DTB is saying it has more chip selects that the actual hardware > device does. How can you possibly protect against that? > > The current code seems to do its best to range check the chip select, > based on what the DTB says this hardware has. If I understood correctly the current state is the following: The spi-imx driver supports GPIOs as CS and a "native" mode where the hardware drives the CS line. If you write: cs-gpios = <&gpio2 0 0>, <0>, <&gpio1 17 0> That means: Use gpio2.0 as CS 0, native CS as CS 1 and gpio1.17 as CS 2. So what the driver can check here: Does the dtb request a native CS at a position that is not supported by the hardware. Of course even if there are only 4 hardware CS the driver can still be used with 7 spi devices given that CS 4 .. 6 are using a GPIO as CS. Oleksij: please correct me if I'm wrong. Best regards Uwe
Hi Uwe, On 10/08/17 22:40, Uwe Kleine-König wrote: > Hello Greg, > > On Thu, Aug 10, 2017 at 10:24:09PM +1000, Greg Ungerer wrote: >> On 10/08/17 21:49, Uwe Kleine-König wrote: >>> On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote: >>>> On 10/08/17 19:35, Vladimir Zapolskiy wrote: >>>>> Errors in DTB (or platform data) may confuse a driver and lead to runtime >>>>> misbehaviour. You describe an error in a board DTB, which is definitely >>>>> better to handle in the SPI driver, but I don't think it is strictly >>>>> mandatory to do it, because DTB errors are supposed to be fixed in DTB. >>>>> >>>>> May be one day a formal check of DTBs against Documentation/devicetree >>>>> descriptions will be added and such DTB errors could be captured on DTB >>>>> compilation stage. >>>> >>>> I completely agree with Vladmir here. Since "cs-gpios" defines the >>>> number of chip selects, as per the code you point out, it is the range >>>> limit. So if a DTB defines it wrongly then you can expect some things >>>> not to work right. The spi code quite rightly relies on the DTB >>>> definitions to be correct for proper operation. >>>> >>>> >>>>>> And in this case: >>>>>> cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>; >>>>>> >>>>>> we should produce a 3 bit value b100 which will be shifted left and >>>>>> "or"-ed with other ctrl bits. >>>> >>>> So the register settings will be wrong and the device will not work. >>>> You can't really expect any other behavior from an incorrect DTB. >>> >>> I think nobody expects that a wrong dtb is good enough to make >>> everything work. Maybe you can argue what should happen in a driver if >>> it gets a wrong dtb. It can make the kernel oops, just silently not work >>> or issue an error message; probably there are more options. >>> >>> The current state is that a broken dtb makes the spi-imx driver write to >>> a unrelated register bit when asked to set a chip select signal. Oleksij >>> tries to improve that and make the driver error out instead. It makes it >>> easier for users to report problems or find the culprit themselves. I >>> like that. >> >> But the check if the chip select is valid is based on information >> that comes from the DTB. In the example of the wrong cs-gpios list >> the DTB is saying it has more chip selects that the actual hardware >> device does. How can you possibly protect against that? >> >> The current code seems to do its best to range check the chip select, >> based on what the DTB says this hardware has. > > If I understood correctly the current state is the following: > > The spi-imx driver supports GPIOs as CS and a "native" mode where the > hardware drives the CS line. If you write: > > cs-gpios = <&gpio2 0 0>, <0>, <&gpio1 17 0> > > That means: Use gpio2.0 as CS 0, native CS as CS 1 and gpio1.17 as CS 2. That is correct, yes. But the current driver is buggy and does not handle the "native" mode correctly - the intention of the patch at the start of this thread is to fix this. > So what the driver can check here: Does the dtb request a native CS at a > position that is not supported by the hardware. Of course even if there How does the driver know how many chip-selects are natively supported by the hardware? I am not familar with all iMX parts and their spi controllers, but Oleksij suggested in an earlier email that some have 3 native chip selects, some have 4, other variants may have some other number. Regards Greg > are only 4 hardware CS the driver can still be used with 7 spi devices > given that CS 4 .. 6 are using a GPIO as CS. > > Oleksij: please correct me if I'm wrong. > > Best regards > Uwe > -- 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
Hello, On Thu, Aug 10, 2017 at 11:21:14PM +1000, Greg Ungerer wrote: > > If I understood correctly the current state is the following: > > > > The spi-imx driver supports GPIOs as CS and a "native" mode where the > > hardware drives the CS line. If you write: > > > > cs-gpios = <&gpio2 0 0>, <0>, <&gpio1 17 0> > > > > That means: Use gpio2.0 as CS 0, native CS as CS 1 and gpio1.17 as CS 2. > > That is correct, yes. But the current driver is buggy and does not > handle the "native" mode correctly - the intention of the patch > at the start of this thread is to fix this. Right, and I had the impression, that this "fix" was discussed away with "if the dtb is broken, there is no hope and so nothing to fix". That's what I wanted to speak up against. > > So what the driver can check here: Does the dtb request a native CS at a > > position that is not supported by the hardware. Of course even if there > > How does the driver know how many chip-selects are natively supported > by the hardware? > > I am not familar with all iMX parts and their spi controllers, but > Oleksij suggested in an earlier email that some have 3 native chip > selects, some have 4, other variants may have some other number. Via the compatible is the obvious way to determine that. Best regards Uwe
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index b402530..f4fe66c 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -524,8 +524,8 @@ 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) << + if (!gpio_is_valid(spi->cs_gpio)) + reg |= (spi->chip_select) << (is_imx35_cspi(spi_imx) ? MX35_CSPICTRL_CS_SHIFT : MX31_CSPICTRL_CS_SHIFT); @@ -616,8 +616,8 @@ 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 (!gpio_is_valid(spi->cs_gpio)) + reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT; writel(reg, spi_imx->base + MXC_CSPICTRL);
The commonly used mechanism of specifying the hardware or native chip-select on an SPI device in devicetree (that is "cs-gpios = <0>") does not result in the native chip-select being configured for use. So external SPI devices that require use of the native chip-select will not work. You can successfully specify native chip-selects if using a platform setup by specifying the cs-gpio as negative offset by 32. And that works correctly. You cannot use the same method in devicetree. 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 the 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 chip-select. If the cs_gpio is less then 0 then it is intended to be for a native chip-select, 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 chip-select number. But also doesn't allow selection of the native chip-select at all. To fix, if the cs_gpio specified for this spi device is not a valid GPIO then use the "chip_select" (that is the native chip-select number) for hardware setup. Signed-off-by: Greg Ungerer <gerg@linux-m68k.org> --- drivers/spi/spi-imx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)