gpio: Handle counting of Freescale chipselects

Message ID 20191127094031.140736-1-linus.walleij@linaro.org
State Superseded
Headers show
Series
  • gpio: Handle counting of Freescale chipselects
Related show

Commit Message

Linus Walleij Nov. 27, 2019, 9:40 a.m.
We have a special quirk to handle the Freescale
nonstandard SPI chipselect GPIOs in the gpiolib-of.c
file, but it currently only handles the case where
the GPIOs are actually requested (gpiod_*get()).

We also need to handle that the SPI core attempts
to count the GPIOs before use, and that needs a
similar quirk in the OF part of the library.

Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>
Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Mark: I will merge this through the GPIO tree if
it fixes the problem.
---
 drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

-- 
2.23.0

Comments

Christophe Leroy Nov. 27, 2019, 10:07 a.m. | #1
Le 27/11/2019 à 10:40, Linus Walleij a écrit :
> We have a special quirk to handle the Freescale

> nonstandard SPI chipselect GPIOs in the gpiolib-of.c

> file, but it currently only handles the case where

> the GPIOs are actually requested (gpiod_*get()).

> 

> We also need to handle that the SPI core attempts

> to count the GPIOs before use, and that needs a

> similar quirk in the OF part of the library.

> 

> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

> Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>

> Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Still getting:

[    3.374867] fsl_spi: probe of ff000a80.spi failed with error -22

Christophe


> ---

> Mark: I will merge this through the GPIO tree if

> it fixes the problem.

> ---

>   drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++

>   1 file changed, 27 insertions(+)

> 

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

> index 80ea49f570f4..33e16fa17bd8 100644

> --- a/drivers/gpio/gpiolib-of.c

> +++ b/drivers/gpio/gpiolib-of.c

> @@ -23,6 +23,29 @@

>   #include "gpiolib.h"

>   #include "gpiolib-of.h"

>   

> +/**

> + * of_gpio_spi_cs_get_count() - special GPIO counting for SPI

> + * Some elder GPIO controllers need special quirks. Currently we handle

> + * the Freescale GPIO controller with bindings that doesn't use the

> + * established "cs-gpios" for chip selects but instead rely on

> + * "gpios" for the chip select lines. If we detect this, we redirect

> + * the counting of "cs-gpios" to count "gpios" transparent to the

> + * driver.

> + */

> +int of_gpio_spi_cs_get_count(struct device *dev, const char *con_id)

> +{

> +	struct device_node *np = dev->of_node;

> +

> +	if (!IS_ENABLED(CONFIG_SPI_MASTER))

> +		return 0;

> +	if (strcmp(con_id, "cs"))

> +		return 0;

> +	if (!of_device_is_compatible(np, "fsl,spi") &&

> +	    !of_device_is_compatible(np, "aeroflexgaisler,spictrl"))

> +		return 0;

> +	return of_gpio_get_count(dev, NULL);

> +}

> +

>   /*

>    * This is used by external users of of_gpio_count() from <linux/of_gpio.h>

>    *

> @@ -35,6 +58,10 @@ int of_gpio_get_count(struct device *dev, const char *con_id)

>   	char propname[32];

>   	unsigned int i;

>   

> +	ret = of_gpio_spi_cs_get_count(dev, con_id);

> +	if (ret > 0)

> +		return ret;

> +

>   	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {

>   		if (con_id)

>   			snprintf(propname, sizeof(propname), "%s-%s",

>
Christophe Leroy Nov. 27, 2019, 10:15 a.m. | #2
Le 27/11/2019 à 11:07, Christophe Leroy a écrit :
> 

> 

> Le 27/11/2019 à 10:40, Linus Walleij a écrit :

>> We have a special quirk to handle the Freescale

>> nonstandard SPI chipselect GPIOs in the gpiolib-of.c

>> file, but it currently only handles the case where

>> the GPIOs are actually requested (gpiod_*get()).

>>

>> We also need to handle that the SPI core attempts

>> to count the GPIOs before use, and that needs a

>> similar quirk in the OF part of the library.

>>

>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

>> Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>

>> Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> 

> Still getting:

> 

> [    3.374867] fsl_spi: probe of ff000a80.spi failed with error -22


Indeed,  of_spi_get_gpio_numbers() uses of_gpio_named_count(np, 
"cs-gpios") which still returns 0;

Christophe

> 

> Christophe

> 

> 

>> ---

>> Mark: I will merge this through the GPIO tree if

>> it fixes the problem.

>> ---

>>   drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++

>>   1 file changed, 27 insertions(+)

>>

>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

>> index 80ea49f570f4..33e16fa17bd8 100644

>> --- a/drivers/gpio/gpiolib-of.c

>> +++ b/drivers/gpio/gpiolib-of.c

>> @@ -23,6 +23,29 @@

>>   #include "gpiolib.h"

>>   #include "gpiolib-of.h"

>> +/**

>> + * of_gpio_spi_cs_get_count() - special GPIO counting for SPI

>> + * Some elder GPIO controllers need special quirks. Currently we handle

>> + * the Freescale GPIO controller with bindings that doesn't use the

>> + * established "cs-gpios" for chip selects but instead rely on

>> + * "gpios" for the chip select lines. If we detect this, we redirect

>> + * the counting of "cs-gpios" to count "gpios" transparent to the

>> + * driver.

>> + */

>> +int of_gpio_spi_cs_get_count(struct device *dev, const char *con_id)

>> +{

>> +    struct device_node *np = dev->of_node;

>> +

>> +    if (!IS_ENABLED(CONFIG_SPI_MASTER))

>> +        return 0;

>> +    if (strcmp(con_id, "cs"))

>> +        return 0;

>> +    if (!of_device_is_compatible(np, "fsl,spi") &&

>> +        !of_device_is_compatible(np, "aeroflexgaisler,spictrl"))

>> +        return 0;

>> +    return of_gpio_get_count(dev, NULL);

>> +}

>> +

>>   /*

>>    * This is used by external users of of_gpio_count() from 

>> <linux/of_gpio.h>

>>    *

>> @@ -35,6 +58,10 @@ int of_gpio_get_count(struct device *dev, const 

>> char *con_id)

>>       char propname[32];

>>       unsigned int i;

>> +    ret = of_gpio_spi_cs_get_count(dev, con_id);

>> +    if (ret > 0)

>> +        return ret;

>> +

>>       for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {

>>           if (con_id)

>>               snprintf(propname, sizeof(propname), "%s-%s",

>>
Christophe Leroy Nov. 27, 2019, 10:25 a.m. | #3
Le 27/11/2019 à 11:15, Christophe Leroy a écrit :
> 

> 

> Le 27/11/2019 à 11:07, Christophe Leroy a écrit :

>>

>>

>> Le 27/11/2019 à 10:40, Linus Walleij a écrit :

>>> We have a special quirk to handle the Freescale

>>> nonstandard SPI chipselect GPIOs in the gpiolib-of.c

>>> file, but it currently only handles the case where

>>> the GPIOs are actually requested (gpiod_*get()).

>>>

>>> We also need to handle that the SPI core attempts

>>> to count the GPIOs before use, and that needs a

>>> similar quirk in the OF part of the library.

>>>

>>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

>>> Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>

>>> Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")

>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>>

>> Still getting:

>>

>> [    3.374867] fsl_spi: probe of ff000a80.spi failed with error -22

> 

> Indeed,  of_spi_get_gpio_numbers() uses of_gpio_named_count(np, 

> "cs-gpios") which still returns 0;


Replacing by of_gpio_named_count(np, "gpios"); , I get further down to 
the same spi_fsl_setup() warning as when renaming the property in the DTS.

Christophe

> 

> Christophe

> 

>>

>> Christophe

>>

>>

>>> ---

>>> Mark: I will merge this through the GPIO tree if

>>> it fixes the problem.

>>> ---

>>>   drivers/gpio/gpiolib-of.c | 27 +++++++++++++++++++++++++++

>>>   1 file changed, 27 insertions(+)

>>>

>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

>>> index 80ea49f570f4..33e16fa17bd8 100644

>>> --- a/drivers/gpio/gpiolib-of.c

>>> +++ b/drivers/gpio/gpiolib-of.c

>>> @@ -23,6 +23,29 @@

>>>   #include "gpiolib.h"

>>>   #include "gpiolib-of.h"

>>> +/**

>>> + * of_gpio_spi_cs_get_count() - special GPIO counting for SPI

>>> + * Some elder GPIO controllers need special quirks. Currently we handle

>>> + * the Freescale GPIO controller with bindings that doesn't use the

>>> + * established "cs-gpios" for chip selects but instead rely on

>>> + * "gpios" for the chip select lines. If we detect this, we redirect

>>> + * the counting of "cs-gpios" to count "gpios" transparent to the

>>> + * driver.

>>> + */

>>> +int of_gpio_spi_cs_get_count(struct device *dev, const char *con_id)

>>> +{

>>> +    struct device_node *np = dev->of_node;

>>> +

>>> +    if (!IS_ENABLED(CONFIG_SPI_MASTER))

>>> +        return 0;

>>> +    if (strcmp(con_id, "cs"))

>>> +        return 0;

>>> +    if (!of_device_is_compatible(np, "fsl,spi") &&

>>> +        !of_device_is_compatible(np, "aeroflexgaisler,spictrl"))

>>> +        return 0;

>>> +    return of_gpio_get_count(dev, NULL);

>>> +}

>>> +

>>>   /*

>>>    * This is used by external users of of_gpio_count() from 

>>> <linux/of_gpio.h>

>>>    *

>>> @@ -35,6 +58,10 @@ int of_gpio_get_count(struct device *dev, const 

>>> char *con_id)

>>>       char propname[32];

>>>       unsigned int i;

>>> +    ret = of_gpio_spi_cs_get_count(dev, con_id);

>>> +    if (ret > 0)

>>> +        return ret;

>>> +

>>>       for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {

>>>           if (con_id)

>>>               snprintf(propname, sizeof(propname), "%s-%s",

>>>
Linus Walleij Nov. 27, 2019, 10:59 a.m. | #4
On Wed, Nov 27, 2019 at 11:25 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
> Le 27/11/2019 à 11:15, Christophe Leroy a écrit :

> > Le 27/11/2019 à 11:07, Christophe Leroy a écrit :

> >> Le 27/11/2019 à 10:40, Linus Walleij a écrit :

> >>> We have a special quirk to handle the Freescale

> >>> nonstandard SPI chipselect GPIOs in the gpiolib-of.c

> >>> file, but it currently only handles the case where

> >>> the GPIOs are actually requested (gpiod_*get()).

> >>>

> >>> We also need to handle that the SPI core attempts

> >>> to count the GPIOs before use, and that needs a

> >>> similar quirk in the OF part of the library.

> >>>

> >>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

> >>> Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>

> >>> Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")

> >>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> >>

> >> Still getting:

> >>

> >> [    3.374867] fsl_spi: probe of ff000a80.spi failed with error -22

> >

> > Indeed,  of_spi_get_gpio_numbers() uses of_gpio_named_count(np,

> > "cs-gpios") which still returns 0;

>

> Replacing by of_gpio_named_count(np, "gpios"); , I get further down to

> the same spi_fsl_setup() warning as when renaming the property in the DTS.


Ah, I got bitten by recursion, sorry.

OK I changed to to "gpios" in my patch too, it's the right way.

Now we need to find the final culprit that makes it not even work when
renaming to "cs-gpios"...

Yours,
Linus Walleij
Christophe Leroy Nov. 27, 2019, 12:05 p.m. | #5
Le 27/11/2019 à 11:59, Linus Walleij a écrit :
> On Wed, Nov 27, 2019 at 11:25 AM Christophe Leroy

> <christophe.leroy@c-s.fr> wrote:

>> Le 27/11/2019 à 11:15, Christophe Leroy a écrit :

>>> Le 27/11/2019 à 11:07, Christophe Leroy a écrit :

>>>> Le 27/11/2019 à 10:40, Linus Walleij a écrit :

>>>>> We have a special quirk to handle the Freescale

>>>>> nonstandard SPI chipselect GPIOs in the gpiolib-of.c

>>>>> file, but it currently only handles the case where

>>>>> the GPIOs are actually requested (gpiod_*get()).

>>>>>

>>>>> We also need to handle that the SPI core attempts

>>>>> to count the GPIOs before use, and that needs a

>>>>> similar quirk in the OF part of the library.

>>>>>

>>>>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>

>>>>> Reported-by: Christophe Leroy <christophe.leroy@c-s.fr>

>>>>> Fixes: 0f0581b24bd0 ("spi: fsl: Convert to use CS GPIO descriptors")

>>>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>>>>

>>>> Still getting:

>>>>

>>>> [    3.374867] fsl_spi: probe of ff000a80.spi failed with error -22

>>>

>>> Indeed,  of_spi_get_gpio_numbers() uses of_gpio_named_count(np,

>>> "cs-gpios") which still returns 0;

>>

>> Replacing by of_gpio_named_count(np, "gpios"); , I get further down to

>> the same spi_fsl_setup() warning as when renaming the property in the DTS.

> 

> Ah, I got bitten by recursion, sorry.

> 

> OK I changed to to "gpios" in my patch too, it's the right way.

> 

> Now we need to find the final culprit that makes it not even work when

> renaming to "cs-gpios"...

> 


Now that I have added master->use_gpio_descriptors = true; to the fsl 
driver, this patch crashes:

[    3.156848] BUG: Kernel NULL pointer dereference on read at 0x00000000
[    3.163062] Faulting instruction address: 0xc058aadc
[    3.167982] Oops: Kernel access of bad area, sig: 11 [#1]
[    3.173322] BE PAGE_SIZE=16K PREEMPT CMPC885
[    3.177559] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.4.0-s3k-dev-00899-g749f15aba2c9-dirty #2515
[    3.186306] NIP:  c058aadc LR: c028973c CTR: 00000000
[    3.191308] REGS: c60e1b70 TRAP: 0300   Not tainted 
(5.4.0-s3k-dev-00899-g749f15aba2c9-dirty)
[    3.199801] MSR:  00009032 <EE,ME,IR,DR,RI>  CR: 24000224  XER: 20000000
[    3.206433] DAR: 00000000 DSISR: c0000000
[    3.206433] GPR00: c028963c c60e1c28 c60d4000 ffffffff c06b512b 
00000000 00000020 c0facf33
[    3.206433] GPR08: c0609438 00000000 00000000 000affff 24000224 
00000000 c0003890 00000000
[    3.206433] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000000 c0800000 0000009e
[    3.206433] GPR24: c0856778 c06b512c c61f2210 c624fc00 c0857380 
00000000 00000000 c624fc00
[    3.243756] NIP [c058aadc] strcmp+0x10/0x40
[    3.247867] LR [c028973c] of_gpio_spi_cs_get_count+0x28/0x98
[    3.253416] Call Trace:
[    3.255881] [c60e1c28] [c034b920] 
__of_device_is_compatible+0xe4/0x14c (unreliable)
[    3.263437] [c60e1c38] [c028963c] of_gpio_get_count+0x24/0xfc
[    3.269116] [c60e1c88] [c028963c] of_gpio_get_count+0x24/0xfc
[    3.274831] [c60e1cd8] [c0286c1c] gpiod_count+0x34/0x100
[    3.280089] [c60e1cf8] [c030c5c0] spi_register_controller+0x14c/0xb50
[    3.286432] [c60e1d48] [c030d004] devm_spi_register_controller+0x40/0x98
[    3.293047] [c60e1d68] [c030ee60] of_fsl_spi_probe+0x2e8/0x3a8
[    3.298813] [c60e1db8] [c02c4f3c] platform_drv_probe+0x44/0xa4
[    3.304598] [c60e1dc8] [c02c30e0] really_probe+0x1ac/0x418
[    3.310012] [c60e1df8] [c02c3b60] device_driver_attach+0x88/0x90
[    3.315948] [c60e1e18] [c02c3c08] __driver_attach+0xa0/0x154
[    3.321540] [c60e1e38] [c02c1140] bus_for_each_dev+0x64/0xb4
[    3.327134] [c60e1e68] [c02c1b1c] bus_add_driver+0xe0/0x218
[    3.332646] [c60e1e88] [c02c43c0] driver_register+0x84/0x148
[    3.338239] [c60e1e98] [c06d8d30] do_one_initcall+0x8c/0x1cc
[    3.343824] [c60e1ef8] [c06d8fac] kernel_init_freeable+0x13c/0x1ec
[    3.349932] [c60e1f28] [c00038a4] kernel_init+0x14/0x110
[    3.355187] [c60e1f38] [c000e1cc] ret_from_kernel_thread+0x14/0x1c
[    3.361249] Instruction dump:
[    3.364183] 39200000 7d3fe9ae 7f43d378 80010024 bb410008 7c0803a6 
38210020 4e800020
[    3.371841] 3863ffff 3884ffff 48000008 41960024 <8d230001> 8d440001 
2e890000 7f895040
[    3.379710] ---[ end trace 795d948bc094d09f ]---

Christophe
Linus Walleij Nov. 27, 2019, 1:24 p.m. | #6
On Wed, Nov 27, 2019 at 1:05 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:

> Now that I have added master->use_gpio_descriptors = true; to the fsl

> driver, this patch crashes:


Oh strcpm() doesn't like NULL pointers, OK I toss in a check
for that too, thanks.

Yours,
Linus Walleij

Patch

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 80ea49f570f4..33e16fa17bd8 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -23,6 +23,29 @@ 
 #include "gpiolib.h"
 #include "gpiolib-of.h"
 
+/**
+ * of_gpio_spi_cs_get_count() - special GPIO counting for SPI
+ * Some elder GPIO controllers need special quirks. Currently we handle
+ * the Freescale GPIO controller with bindings that doesn't use the
+ * established "cs-gpios" for chip selects but instead rely on
+ * "gpios" for the chip select lines. If we detect this, we redirect
+ * the counting of "cs-gpios" to count "gpios" transparent to the
+ * driver.
+ */
+int of_gpio_spi_cs_get_count(struct device *dev, const char *con_id)
+{
+	struct device_node *np = dev->of_node;
+
+	if (!IS_ENABLED(CONFIG_SPI_MASTER))
+		return 0;
+	if (strcmp(con_id, "cs"))
+		return 0;
+	if (!of_device_is_compatible(np, "fsl,spi") &&
+	    !of_device_is_compatible(np, "aeroflexgaisler,spictrl"))
+		return 0;
+	return of_gpio_get_count(dev, NULL);
+}
+
 /*
  * This is used by external users of of_gpio_count() from <linux/of_gpio.h>
  *
@@ -35,6 +58,10 @@  int of_gpio_get_count(struct device *dev, const char *con_id)
 	char propname[32];
 	unsigned int i;
 
+	ret = of_gpio_spi_cs_get_count(dev, con_id);
+	if (ret > 0)
+		return ret;
+
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
 		if (con_id)
 			snprintf(propname, sizeof(propname), "%s-%s",