[PATCHv3 0/2] spi: imx: native chip selects and devicetree

[PATCHv3 0/2] spi: imx: native chip selects and devicetree

[Greg Ungerer]

Selecting native chip selects for iMX SPI devices in a devicetree
configuration does not work. That is the case for imx25 based SoC
parts, and I think for imx31 SoC as well. There is no problem with
configuring SPI ports to use a GPIO as chip select.

Selecting native chip selects via the old platform setup code will
work, it is only devicetree configurations that are broken.

With platform configuration you specify a native chip select by
setting the cs_gpio for the SPI device to be "32 - chipselect",
This will be a negative number - and thus not a valid GPIO number.
And that "chipselect" is the actual hardware native chip select
number.

You cannot specify the cs_gpio in dvicetree as a negative number,
so this whole scheme does not work. The common method in devicetree
is to set the cs_gpio entry for your device to be "<0>". If you do
this to configure your SPI device to use a native chip select it is
valid, but the SPI device cannot be accessed (no valid read data
returned from it).

The problem lies in the way the spi-imx.c driver sets up the
controlling registers of the iMX SPI block. It doesn't have the
correct logic for using native chip selects in the devicetree case.

v1 of this patch set contained the fixes for some platform setups to
make sure that the correct "chip_select" is used that is associated with
the required native chip select. This was dropped in v2, since Shawn 
picked that change up for arch/arm/imx and that is now in mainline.

This v3 series is just an up to date rebase of the v2 patches.

The following patches fix use of native chip selects in the spi-imx
driver for the devicetree case, and also document the use of the "<0>"
notation in devicetree.

Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt |    4 +++-
 drivers/spi/spi-imx.c                                  |    8 ++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Greg Ungerer]

Document the "<0>" notation for specifying use of a native chip-select
in the "cs-gpios" tag of an SPI device in devicetree. This isn't unique
to the spi-imx driver, but this clearly spells out that you can use it
with this driver.

Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
index 31b5b21..cf616ea 100644
--- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
+++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
@@ -12,6 +12,7 @@ Required properties:
 - reg : Offset and length of the register set for the device
 - interrupts : Should contain CSPI/eCSPI interrupt
 - cs-gpios : Specifies the gpio pins to be used for chipselects.
+	Specify use of native chipselects with "<0>" in place of a gpio.
 - clocks : Clock specifiers for both ipg and per clocks.
 - clock-names : Clock names should include both "ipg" and "per"
 See the clock consumer binding,
@@ -38,7 +39,8 @@ ecspi@70010000 {
 	reg = <0x70010000 0x4000>;
 	interrupts = <36>;
 	cs-gpios = <&gpio3 24 0>, /* GPIO3_24 */
-		   <&gpio3 25 0>; /* GPIO3_25 */
+		   <&gpio3 25 0>, /* GPIO3_25 */
+		   <0>;           /* Native chip select */
 	dmas = <&sdma 3 7 1>, <&sdma 4 7 2>;
 	dma-names = "rx", "tx";
 	fsl,spi-rdy-drctl = <1>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Vladimir Zapolskiy]

Hello Greg,

On 07/11/2017 07:22 AM, Greg Ungerer wrote:
> Document the "<0>" notation for specifying use of a native chip-select
> in the "cs-gpios" tag of an SPI device in devicetree. This isn't unique
> to the spi-imx driver, but this clearly spells out that you can use it
> with this driver.
> 
> Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> index 31b5b21..cf616ea 100644
> --- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> +++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
> @@ -12,6 +12,7 @@ Required properties:
>  - reg : Offset and length of the register set for the device
>  - interrupts : Should contain CSPI/eCSPI interrupt
>  - cs-gpios : Specifies the gpio pins to be used for chipselects.
> +	Specify use of native chipselects with "<0>" in place of a gpio.
>  - clocks : Clock specifiers for both ipg and per clocks.
>  - clock-names : Clock names should include both "ipg" and "per"
>  See the clock consumer binding,
> @@ -38,7 +39,8 @@ ecspi@70010000 {
>  	reg = <0x70010000 0x4000>;
>  	interrupts = <36>;
>  	cs-gpios = <&gpio3 24 0>, /* GPIO3_24 */
> -		   <&gpio3 25 0>; /* GPIO3_25 */
> +		   <&gpio3 25 0>, /* GPIO3_25 */
> +		   <0>;           /* Native chip select */
>  	dmas = <&sdma 3 7 1>, <&sdma 4 7 2>;
>  	dma-names = "rx", "tx";
>  	fsl,spi-rdy-drctl = <1>;
> 

Adding Rob to Cc.

Reviewed-by: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>

Rob, if you find the change acceptable, can you ack it?

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Fabio Estevam]

Hi Greg,

On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Document the "<0>" notation for specifying use of a native chip-select
> in the "cs-gpios" tag of an SPI device in devicetree. This isn't unique
> to the spi-imx driver, but this clearly spells out that you can use it
> with this driver.
>
> Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>

Whic SoC did you use to test the native chip-select?

I remember that some old SoCs (mx27/mx31) used to have issues with
native chip-select.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Vladimir Zapolskiy]

Hi Fabio,

On 07/19/2017 03:37 AM, Fabio Estevam wrote:
> Hi Greg,
> 
> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> Document the "<0>" notation for specifying use of a native chip-select
>> in the "cs-gpios" tag of an SPI device in devicetree. This isn't unique
>> to the spi-imx driver, but this clearly spells out that you can use it
>> with this driver.
>>
>> Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> 
> Whic SoC did you use to test the native chip-select?
> 
> I remember that some old SoCs (mx27/mx31) used to have issues with
> native chip-select.

FWIW I tested on DT-only i.MX31, the change works acceptable.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Fabio Estevam]

Hi Vladimir,

On Tue, Jul 18, 2017 at 9:39 PM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:

> FWIW I tested on DT-only i.MX31, the change works acceptable.

MX31 has an issue with native chip select as far as I remember. Please
check DSPhl22960 from the chip errata:
http://www.nxp.com/docs/en/errata/MCIMX31CE.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Vladimir Zapolskiy]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
Tested-by: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Fabio Estevam]

Adding Pengutronix folks on Cc.

On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Greg Ungerer]

Hi Fabio,

On 19/07/17 10:37, Fabio Estevam wrote:
> Hi Greg,
> 
> On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> Document the "<0>" notation for specifying use of a native chip-select
>> in the "cs-gpios" tag of an SPI device in devicetree. This isn't unique
>> to the spi-imx driver, but this clearly spells out that you can use it
>> with this driver.
>>
>> Signed-off-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> 
> Whic SoC did you use to test the native chip-select?
> 
> I remember that some old SoCs (mx27/mx31) used to have issues with
> native chip-select.

I tested on an iMX253. I have a board with a Silicon Labs 32260 SLIC
hooked up to an SPI interface on the 253 and I need the native chip
select timing for it.

Regards
Greg



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/2] spi: imx: document use of native chip-selects in devicetree

[Vladimir Zapolskiy]

On 07/19/2017 03:42 AM, Fabio Estevam wrote:
> Hi Vladimir,
> 
> On Tue, Jul 18, 2017 at 9:39 PM, Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org> wrote:
> 
>> FWIW I tested on DT-only i.MX31, the change works acceptable.
> 
> MX31 has an issue with native chip select as far as I remember. Please
> check DSPhl22960 from the chip errata:
> http://www.nxp.com/docs/en/errata/MCIMX31CE.pdf

Oh, it's unpleasant, I didn't know or experience the issue, thank you
for information.

I can definitely tell that

a) my hardware (Logitech i.MX31 Litekit) does not allow to use GPIOs
instead of native chip selects for two SPI devices on board:
  Atmel AT93C66A EEPROM on CSPI1 pads selected by CS2,
  Freescale MC13783 PMIC on CSPI2 pads selected by CS0.

There is no option to mux CSPI1_SS2 or CSPI2_SS0 pad to GPIOs, so some
kind of native chip select support is needed for legacy hardware, even
if it is exposed to the errata item,

b) MC13783 PMIC on CS0 works fine with Greg's change,

c) actually there is no AT93C66A driver for better testing, but I do
experience issues with the native CS2, however as I said in another
email it is not related to Greg's change.

By the way I'm going to unrestrict DMA support on i.MX31, due to my
loose tests (only to/from MC13783) DMA transfers seem to work properly.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
> Tested-by: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
> 
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Oleksij Rempel]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Oleksij Rempel]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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 :)

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Oleksij Rempel]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Vladimir Zapolskiy]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Oleksij Rempel]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Greg Ungerer]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html