pinctrl: at91: allow disabled gpio controllers
Subsystems (4)
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
This patch allows to have gpio controller with status set to disabled.
gpio_banks represents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower than gpio_banks and that some pointers in the gpio_chips array are NULL. This patch deals with these specific cases.
---
Hi,
Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't support unused gpio bank.
drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
drivers/pinctrl/pinctrl-at91.c +18 -12
@@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,for_each_child_of_node (np, child) {if (of_device_is_compatible (child, gpio_compat)) { - info->nbanks++; +if (of_device_is_available (child)) + info->nbanks++; }else { info->nfunctions++; info->ngroups +=of_get_child_count (child);
@@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info, } size /=sizeof (*list); -if (!size || size % info->nbanks) { +if (!size || size % gpio_banks) {dev_err (info->dev,"wrong mux mask array should be by %d\n" , info->nbanks);return -EINVAL ; } - info->nmux = size / info->nbanks; + info->nmux = size / gpio_banks; info->mux_mask =devm_kzalloc (info->dev,sizeof (u32) * size,GFP_KERNEL );if (!info->mux_mask) {
@@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev) {struct at91_pinctrl *info;struct pinctrl_pin_desc *pdesc; -int ret, i, j, k; +int ret, i, j, k, ngpio_chips_enabled =0 ; info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info)
@@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)* to obtain references to the struct gpio_chip * for them, and we * need this to proceed. */ -for (i =0 ; i < info->nbanks; i++) { -if (!gpio_chips[i]) { -dev_warn (&pdev->dev,"GPIO chip %d not registered yet\n" , i); -devm_kfree (&pdev->dev, info); -return -EPROBE_DEFER ; - } +for (i =0 ; i < gpio_banks; i++) { +if (gpio_chips[i]) + ngpio_chips_enabled++; + } +if (ngpio_chips_enabled < info->nbanks) { +dev_warn (&pdev->dev,"All GPIO chips are not registered yet\n" ); +devm_kfree (&pdev->dev, info); +return -EPROBE_DEFER ; } at91_pinctrl_desc.name =dev_name (&pdev->dev);
@@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev) }/* We will handle a range of GPIO pins */ -for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);dev_info (&pdev->dev,"initialized AT91 pinctrl driver\n" );
@@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)for (i =0 ; i < gpio_banks; i++) { at91_gpio = gpio_chips[i]; +if (!at91_gpio) +continue ;/* * GPIO controller are grouped on some SoC:
Signature · 1 line
2.0.3
PATCH 2/2 ARM: at91/dt: sama5d4: add pioD controller
PioD controller was not described in the device tree since we don't use it. As pinctrl-at91 allows disabled gpio controllers in the device tree, we can add it to complete the device description.
--- arch/arm/boot/dts/sama5d4.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+)
arch/arm/boot/dts/sama5d4.dtsi +13 -0
@@ -61,6 +61,7 @@ gpio0 = &pioA; gpio1 = &pioB; gpio2 = &pioC; + gpio3 = &pioD; gpio4 = &pioE; tcb0 = &tcb0; tcb1 = &tcb1;
@@ -1042,6 +1043,18 @@ clocks = <&pioC_clk>; }; + pioD: gpio at fc068000 { + compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; + reg = <0xfc068000 0x100>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>; + #gpio-cells = <2>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + clocks = <&pioD_clk>; + status = "disabled"; + }; + pioE: gpio at fc06d000 { compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; reg = <0xfc06d000 0x100>;
Signature · 1 line
2.0.3
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
In reply to Ludovic DesrochesQuoted 9 lines · Ludovic Desroches
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.Signed-off-by Ludovic Desroches <[email protected]>
(...)
drivers/pinctrl/pinctrl-at91.c static int at91_pinctrl_probe(struct platform_device *pdev) · line 1242
Quoted 6 lines
/* We will handle a range of GPIO pins */drivers/pinctrl/pinctrl-at91.c +3
-2-for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);
I highly suspect the real solution to this problem is to get rid of the pinctrl_add_gpio_range() call from the driver.
Remobe these calls, and instead in at91_gpio_probe() in the same file, call gpiochip_add_pingroup_range() for each GPIO chip.
That way the GPIO ranges are inserted from the GPIO side instead of the pinctrl side, which is way better, since it is more relative, and make you only add ranges for the gpio chips actually there.
Yours,
Linus Walleij
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
Quoted 24 lines · Linus Walleij
Quoted 9 lines · Ludovic Desroches
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.Signed-off-by Ludovic Desroches <[email protected]>(...)
drivers/pinctrl/pinctrl-at91.c static int at91_pinctrl_probe(struct platform_device *pdev) · line 1242
Quoted 6 lines
/* We will handle a range of GPIO pins */drivers/pinctrl/pinctrl-at91.c +3
-2-for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);I highly suspect the real solution to this problem is to get rid of the
pinctrl_add_gpio_range()call from the driver.
Quoted 6 lines
Remobe these calls, and instead in
at91_gpio_probe()in the same file, callgpiochip_add_pingroup_range()for each GPIO chip.That way the GPIO ranges are inserted from the GPIO side instead of the pinctrl side, which is way better, since it is more relative, and make you only add ranges for the gpio chips actually there.
Thanks for your advices, I'll try to do it the way you recommend.
Ludovic
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
In reply to Linus WalleijQuoted 24 lines · Linus Walleij
Quoted 9 lines · Ludovic Desroches
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.Signed-off-by Ludovic Desroches <[email protected]>(...)
drivers/pinctrl/pinctrl-at91.c static int at91_pinctrl_probe(struct platform_device *pdev) · line 1242
Quoted 6 lines
/* We will handle a range of GPIO pins */drivers/pinctrl/pinctrl-at91.c +3
-2-for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);I highly suspect the real solution to this problem is to get rid of the
pinctrl_add_gpio_range()call from the driver.
Quoted 6 lines
Remobe these calls, and instead in
at91_gpio_probe()in the same file, callgpiochip_add_pingroup_range()for each GPIO chip.That way the GPIO ranges are inserted from the GPIO side instead of the pinctrl side, which is way better, since it is more relative, and make you only add ranges for the gpio chips actually there.
I had a quick look to Documentation about that stuff, I totally agree that it is a better approach. Before going further I was wondering if it would not cause backward compatibility issue with old dtb (it seems I'll have to add some properties to gpio controllers).
Regards
Ludovic
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
Quoted 5 lines · Ludovic Desroches
Quoted 3 lines · Linus Walleij
Remobe these calls, and instead in
at91_gpio_probe()in the same file, callgpiochip_add_pingroup_range()for each GPIO chip.
Quoted 8 lines
Quoted 3 lines
That way the GPIO ranges are inserted from the GPIO side instead of the pinctrl side, which is way better, since it is more relative, and make you only add ranges for the gpio chips actually there.
I had a quick look to Documentation about that stuff, I totally agree that it is a better approach. Before going further I was wondering if it would not cause backward compatibility issue with old dtb (it seems I'll have to add some properties to gpio controllers).
The gpiolib core code parses and add ranges when using device tree, and that is part of the core gpio bindings.
I think you should just try to add in the ranges in the dts nodes for the gpiochips, remove the pinctrl_add_gpio_range() calls and see what happens.
Yours,
Linus Walleij
RFC PATCH 1/1 pinctrl: at91
---
Hi Linus,
I have reworked my patch (of course it will be split for submission) trying to follow your advices.
I have replaced pinctrl_add_gpio_range() with of_gpiochip_add(). I'll do more tests but it seems to work. Maybe I've missed something but I still need to fix the case when there is a gpio controller not used.
A lot of things rely on the gpio controller id (taken from the alias):
index for gpio_chips array, pin muxing, naming, etc. I am not sure I can't get rid of this constraint.
Regards
Ludovic
arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++- drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 20 deletions(-)
arch/arm/boot/dts/sama5d4.dtsi +18 -1
@@ -62,6 +62,7 @@ gpio0 = &pioA; gpio1 = &pioB; gpio2 = &pioC; + gpio3 = &pioD; gpio4 = &pioE; tcb0 = &tcb0; tcb1 = &tcb1;
@@ -1063,7 +1064,7 @@ }; - pinctrl at fc06a000 { + pinctrl: pinctrl at fc06a000 { #address-cells = <1>; #size-cells = <1>; compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
@@ -1084,6 +1085,7 @@ interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 0 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioA_clk>;
@@ -1095,6 +1097,7 @@ interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 32 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioB_clk>;
@@ -1106,17 +1109,31 @@ interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 64 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioC_clk>; }; + pioD: gpio at fc068000 { + compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; + reg = <0xfc068000 0x100>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>; + #gpio-cells = <2>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + clocks = <&pioD_clk>; + status = "disabled"; + }; + pioE: gpio at fc06d000 { compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; reg = <0xfc06d000 0x100>; interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 128 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioE_clk>;
drivers/pinctrl/pinctrl-at91.c +23 -19
110 lines not on this page · raw message
Signature · 1 line
2.0.3
RFC PATCH 1/1 pinctrl: at91
Le 15/12/2014 10:57, Ludovic Desroches a ?crit :
Quoted 8 lines
Signed-off-by Ludovic Desroches <[email protected]>---
Hi Linus,
I have reworked my patch (of course it will be split for submission) trying to follow your advices.
Quoted 7 lines
I have replaced
pinctrl_add_gpio_range()withof_gpiochip_add(). I'll do more tests but it seems to work. Maybe I've missed something but I still need to fix the case when there is a gpio controller not used.A lot of things rely on the gpio controller id (taken from the alias):
index forgpio_chipsarray, pin muxing, naming, etc. I am not sure I can't get rid of this constraint.
Fair enough, I'm personally okay with those changes. When you rework this RFC into real patches and when you correct the little nitpicking hereafter, you can add my:
On your side Linus, does it sound good?
BTW, once split, you'll have to add a commit message with explanation to each patch ;-)
Otherwise, see below:
Quoted 26 lines
arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++- drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 20 deletions(-)arch/arm/boot/dts/sama5d4.dtsi +2
-1@@ -62,6 +62,7 @@ gpio0 = &pioA; gpio1 = &pioB; gpio2 = &pioC; + gpio3 = &pioD; gpio4 = &pioE; tcb0 = &tcb0; tcb1 = &tcb1;@@ -1063,7 +1064,7 @@ }; - pinctrl at fc06a000 { + pinctrl: pinctrl at fc06a000 { #address-cells = <1>; #size-cells = <1>; compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";
arch/arm/boot/dts/sama5d4.dtsi line 1088
Quoted 4 lines
arch/arm/boot/dts/sama5d4.dtsi +1
-0@@ -1084,6 +1085,7 @@ interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 0 32>;
You may need to modify our pinctrl binding documentation as well.
Quoted 72 lines
arch/arm/boot/dts/sama5d4.dtsi +15
-0interrupt-controller; #interrupt-cells = <2>; clocks = <&pioA_clk>;@@ -1095,6 +1097,7 @@ interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 32 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioB_clk>;@@ -1106,17 +1109,31 @@ interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 64 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioC_clk>; }; + pioD: gpio at fc068000 { + compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; + reg = <0xfc068000 0x100>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>; + #gpio-cells = <2>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + clocks = <&pioD_clk>; + status = "disabled"; + }; + pioE: gpio at fc06d000 { compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; reg = <0xfc06d000 0x100>; interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 128 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioE_clk>;drivers/pinctrl/pinctrl-at91.c +2
-1@@ -13,6 +13,7 @@#include <linux/of.h> #include <linux/of_device.h> #include <linux/of_address.h> +#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/slab.h> #include <linux/interrupt.h> @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;struct at91_gpio_chip {struct gpio_chip chip; -struct pinctrl_gpio_range range;struct at91_gpio_chip *next;/* Bank sharing same clock */ int pioc_hwirq;/* PIO bank interrupt identifier on AIC */ int pioc_virq;/* PIO bank Linux virtual interrupt */ @@ -178,6 +178,7 @@ struct at91_pinctrl {struct pinctrl_dev *pctl;int nbanks; +int nactive_banks;uint32_t *mux_mask;int nmux;
drivers/pinctrl/pinctrl-at91.c static void at91_pinctrl_child_count(struct at91_pinctrl *info, · line 987
Quoted 5 lines
drivers/pinctrl/pinctrl-at91.c +2
-0@@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,for_each_child_of_node (np, child) {if (of_device_is_compatible (child, gpio_compat)) { info->nbanks++; +if (of_device_is_available (child)) + info->nactive_banks;
What is the purpose of the line just above?
Quoted 82 lines
drivers/pinctrl/pinctrl-at91.c +19
-11}else { info->nfunctions++; info->ngroups +=of_get_child_count (child);@@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,dev_dbg (&pdev->dev,"mux-mask\n" ); tmp = info->mux_mask;for (i =0 ; i < info->nbanks; i++) { +if (!gpio_chips[i]) { + tmp += info->nmux; +continue ; + }for (j =0 ; j < info->nmux; j++, tmp++) { -dev_dbg (&pdev->dev,"%d:%d\t0x%x\n" , i, j, tmp[0 ]); +dev_dbg (&pdev->dev,"pio%c:periphal %c\t0x%x\n" ,'A' + i,'A' + j, tmp[0 ]); } }@@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev) {struct at91_pinctrl *info;struct pinctrl_pin_desc *pdesc; -int ret, i, j, k; +int ret, i, j, k, ngpio_chips_enabled =0 ; info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info)@@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)* need this to proceed. */ for (i =0 ; i < info->nbanks; i++) { -if (!gpio_chips[i]) { -dev_warn (&pdev->dev,"GPIO chip %d not registered yet\n" , i); -devm_kfree (&pdev->dev, info); -return -EPROBE_DEFER ; - } +if (gpio_chips[i]) + ngpio_chips_enabled++; + } +if (ngpio_chips_enabled < info->nactive_banks) { +dev_warn (&pdev->dev, +"All GPIO chips are not registered yet (%d/%d)\n" , + ngpio_chips_enabled, info->nactive_banks); +devm_kfree (&pdev->dev, info); +return -EPROBE_DEFER ; } at91_pinctrl_desc.name =dev_name (&pdev->dev);@@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev) goto err; } -/* We will handle a range of GPIO pins */ for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +if (gpio_chips[i]) +of_gpiochip_add (&gpio_chips[i]->chip);dev_info (&pdev->dev,"initialized AT91 pinctrl driver\n" );@@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)for (i =0 ; i < gpio_banks; i++) { at91_gpio = gpio_chips[i]; +if (!at91_gpio) +continue ;/* * GPIO controller are grouped on some SoC: @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)struct resource *res;struct at91_gpio_chip *at91_chip =NULL ;struct gpio_chip *chip; -struct pinctrl_gpio_range *range;int ret =0 ;int irq, i;int alias_idx =of_alias_get_id (np,"gpio" );@@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev) chip->names = (const char *const *)names; - range = &at91_chip->range;
drivers/pinctrl/pinctrl-at91.c static int at91_gpio_probe(struct platform_device *pdev) · line 1807
Quoted 10 lines
drivers/pinctrl/pinctrl-at91.c +0
-7- range->name = chip->label; - range->id = alias_idx; - range->pin_base = range->base = range->id *MAX_NB_GPIO_PER_BANK ; - - range->npins = chip->ngpio; - range->gc = chip; - ret =gpiochip_add (chip);if (ret) goto gpiochip_add_err;
Thanks Ludovic, bye,
Signature · 1 line
Nicolas Ferre
RFC PATCH 1/1 pinctrl: at91
Hi Nicolas, Linus,
Quoted 17 lines · Nicolas Ferre
Le 15/12/2014 10:57, Ludovic Desroches a ?crit :
Quoted 8 lines
Signed-off-by Ludovic Desroches <[email protected]>---
Hi Linus,
I have reworked my patch (of course it will be split for submission) trying to follow your advices.
Quoted 7 lines
I have replaced
pinctrl_add_gpio_range()withof_gpiochip_add(). I'll do more tests but it seems to work. Maybe I've missed something but I still need to fix the case when there is a gpio controller not used.A lot of things rely on the gpio controller id (taken from the alias):
index forgpio_chipsarray, pin muxing, naming, etc. I am not sure I can't get rid of this constraint.
Quoted 8 lines
Fair enough, I'm personally okay with those changes. When you rework this RFC into real patches and when you correct the little nitpicking hereafter, you can add my:
Acked-by Nicolas Ferre <[email protected]>On your side Linus, does it sound good?
After testing more these changes, it breaks GPIOs if gpio-ranges property is not added to all our SOCs (about 10 dtsi to update).
Usage of of_gpiochip_add() only solves my issue about gpio but not about pinctrl stuff, I still need a patch to manage the case when we have a gap if a gpio controller is not enabled to not break the pin naming, etc.
Maybe I am missing something, I am still discovering how pinctrl subsystem works in order to maintain our pinctrl driver. So I would be pleased to have some advices to find the proper way to fix this issue.
Regards
Ludovic
Quoted 218 lines
BTW, once split, you'll have to add a commit message with explanation to each patch ;-)
Otherwise, see below:
Quoted 26 lines
arch/arm/boot/dts/sama5d4.dtsi | 19 ++++++++++++++++++- drivers/pinctrl/pinctrl-at91.c | 42 +++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 20 deletions(-)arch/arm/boot/dts/sama5d4.dtsi +2
-1@@ -62,6 +62,7 @@ gpio0 = &pioA; gpio1 = &pioB; gpio2 = &pioC; + gpio3 = &pioD; gpio4 = &pioE; tcb0 = &tcb0; tcb1 = &tcb1;@@ -1063,7 +1064,7 @@ }; - pinctrl at fc06a000 { + pinctrl: pinctrl at fc06a000 { #address-cells = <1>; #size-cells = <1>; compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus";arch/arm/boot/dts/sama5d4.dtsi line 1088
Quoted 4 lines
arch/arm/boot/dts/sama5d4.dtsi +1
-0@@ -1084,6 +1085,7 @@ interrupts = <23 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 0 32>;You may need to modify our pinctrl binding documentation as well.
Quoted 72 lines
arch/arm/boot/dts/sama5d4.dtsi +15
-0interrupt-controller; #interrupt-cells = <2>; clocks = <&pioA_clk>;@@ -1095,6 +1097,7 @@ interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 32 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioB_clk>;@@ -1106,17 +1109,31 @@ interrupts = <25 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 64 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioC_clk>; }; + pioD: gpio at fc068000 { + compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; + reg = <0xfc068000 0x100>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 1>; + #gpio-cells = <2>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + clocks = <&pioD_clk>; + status = "disabled"; + }; + pioE: gpio at fc06d000 { compatible = "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio"; reg = <0xfc06d000 0x100>; interrupts = <26 IRQ_TYPE_LEVEL_HIGH 1>; #gpio-cells = <2>; gpio-controller; + gpio-ranges = <&pinctrl 0 128 32>; interrupt-controller; #interrupt-cells = <2>; clocks = <&pioE_clk>;drivers/pinctrl/pinctrl-at91.c +2
-1@@ -13,6 +13,7 @@#include <linux/of.h> #include <linux/of_device.h> #include <linux/of_address.h> +#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/slab.h> #include <linux/interrupt.h> @@ -35,7 +36,6 @@ struct at91_pinctrl_mux_ops;struct at91_gpio_chip {struct gpio_chip chip; -struct pinctrl_gpio_range range;struct at91_gpio_chip *next;/* Bank sharing same clock */ int pioc_hwirq;/* PIO bank interrupt identifier on AIC */ int pioc_virq;/* PIO bank Linux virtual interrupt */ @@ -178,6 +178,7 @@ struct at91_pinctrl {struct pinctrl_dev *pctl;int nbanks; +int nactive_banks;uint32_t *mux_mask;int nmux;drivers/pinctrl/pinctrl-at91.c static void at91_pinctrl_child_count(struct at91_pinctrl *info, · line 987
Quoted 5 lines
drivers/pinctrl/pinctrl-at91.c +2
-0@@ -982,6 +983,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,for_each_child_of_node (np, child) {if (of_device_is_compatible (child, gpio_compat)) { info->nbanks++; +if (of_device_is_available (child)) + info->nactive_banks;What is the purpose of the line just above?
Quoted 82 lines
drivers/pinctrl/pinctrl-at91.c +19
-11}else { info->nfunctions++; info->ngroups +=of_get_child_count (child);@@ -1145,8 +1148,12 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,dev_dbg (&pdev->dev,"mux-mask\n" ); tmp = info->mux_mask;for (i =0 ; i < info->nbanks; i++) { +if (!gpio_chips[i]) { + tmp += info->nmux; +continue ; + }for (j =0 ; j < info->nmux; j++, tmp++) { -dev_dbg (&pdev->dev,"%d:%d\t0x%x\n" , i, j, tmp[0 ]); +dev_dbg (&pdev->dev,"pio%c:periphal %c\t0x%x\n" ,'A' + i,'A' + j, tmp[0 ]); } }@@ -1185,7 +1192,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev) {struct at91_pinctrl *info;struct pinctrl_pin_desc *pdesc; -int ret, i, j, k; +int ret, i, j, k, ngpio_chips_enabled =0 ; info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info)@@ -1201,11 +1208,15 @@ static int at91_pinctrl_probe(struct platform_device *pdev)* need this to proceed. */ for (i =0 ; i < info->nbanks; i++) { -if (!gpio_chips[i]) { -dev_warn (&pdev->dev,"GPIO chip %d not registered yet\n" , i); -devm_kfree (&pdev->dev, info); -return -EPROBE_DEFER ; - } +if (gpio_chips[i]) + ngpio_chips_enabled++; + } +if (ngpio_chips_enabled < info->nactive_banks) { +dev_warn (&pdev->dev, +"All GPIO chips are not registered yet (%d/%d)\n" , + ngpio_chips_enabled, info->nactive_banks); +devm_kfree (&pdev->dev, info); +return -EPROBE_DEFER ; } at91_pinctrl_desc.name =dev_name (&pdev->dev);@@ -1233,9 +1244,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev) goto err; } -/* We will handle a range of GPIO pins */ for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +if (gpio_chips[i]) +of_gpiochip_add (&gpio_chips[i]->chip);dev_info (&pdev->dev,"initialized AT91 pinctrl driver\n" );@@ -1682,6 +1693,8 @@ static void at91_gpio_probe_fixup(void)for (i =0 ; i < gpio_banks; i++) { at91_gpio = gpio_chips[i]; +if (!at91_gpio) +continue ;/* * GPIO controller are grouped on some SoC: @@ -1705,7 +1718,6 @@ static int at91_gpio_probe(struct platform_device *pdev)struct resource *res;struct at91_gpio_chip *at91_chip =NULL ;struct gpio_chip *chip; -struct pinctrl_gpio_range *range;int ret =0 ;int irq, i;int alias_idx =of_alias_get_id (np,"gpio" );@@ -1790,14 +1802,6 @@ static int at91_gpio_probe(struct platform_device *pdev) chip->names = (const char *const *)names; - range = &at91_chip->range;drivers/pinctrl/pinctrl-at91.c static int at91_gpio_probe(struct platform_device *pdev) · line 1807
Quoted 10 lines
drivers/pinctrl/pinctrl-at91.c +0
-7- range->name = chip->label; - range->id = alias_idx; - range->pin_base = range->base = range->id *MAX_NB_GPIO_PER_BANK ; - - range->npins = chip->ngpio; - range->gc = chip; - ret =gpiochip_add (chip);if (ret) goto gpiochip_add_err;Thanks Ludovic, bye,
Signature · 1 line
Nicolas Ferre
RFC PATCH 1/1 pinctrl: at91
Quoted 3 lines · Ludovic Desroches
Usage of
of_gpiochip_add()only solves my issue about gpio but not about pinctrl stuff, I still need a patch to manage the case when we have a gap if a gpio controller is not enabled to not break the pin naming, etc.
This has been the topic of many threads today. I assume you are talking about keeping GPIO numbers consistent.
- My suggestion is to add alias handling of the GPIO chips to the core so they can be probed in the right order.
- For consistency in sysfs use the "names" array in struct gpio_chip so you can search for a symbolic name in sysfs and don't have to rely on fragile stuff like GPIO numbers.
- Partake in the development of a new GPIO ABI
that does not use the global GPIO numberspace.
Yours,
Linus Walleij
RFC PATCH 1/1 pinctrl: at91
Quoted 7 lines · Linus Walleij
Quoted 3 lines · Ludovic Desroches
Usage of
of_gpiochip_add()only solves my issue about gpio but not about pinctrl stuff, I still need a patch to manage the case when we have a gap if a gpio controller is not enabled to not break the pin naming, etc.
Quoted 6 lines
This has been the topic of many threads today. I assume you are talking about keeping GPIO numbers consistent.
- My suggestion is to add alias handling of the GPIO chips to the core so they can be probed in the right order.
We are already using aliases but it seems to not be the perfect solution. For example, at the probe time, we wait for all gpio controllers to be probed. We fill a gpio_chips table whose position in this table is the alias id of the gpio controller. The at91 pinctrl driver is waiting for 'maximum alias id' gpio controllers. What happens if don't want to use a gpio controller and don't declare it or set it as disabled?
Quoted 8 lines
- For consistency in sysfs use the "names" array in struct
gpio_chipso you can search for a symbolic name in sysfs and don't have to rely on fragile stuff like GPIO numbers.- Partake in the development of a new GPIO ABI
that does not use the global GPIO numberspace.
I will have a look and bring my humble contribution if I can but I think this topic is far away from the fix I am sending.
As you notice we can improve the at91 pinctrl driver, removing pinctrl_add_gpio_range() and the range computation in the driver but it won't solve my issue and it involves to add the gpio-range property to all our devices so it breaks backward compatibility with old dtb.
Quoted 1 line
From my point of view, it is two distinct topics. One is a very
important fix because our SAMA5D4 device is not booting without it. The other one is a proper way to manage gpio ranges but alone I don't think it can solve my issue.
Regards
Ludovic
RFC PATCH 1/1 pinctrl: at91
Quoted 4 lines · Ludovic Desroches
From my point of view, it is two distinct topics. One is a very important fix because our SAMA5D4 device is not booting without it. The other one is a proper way to manage gpio ranges but alone I don't think it can solve my issue.
Can you make me a minimal, sane patch that fixes the boot regression?
I hate regressions so these need to be fixed first...
The rest we can discuss I guess.
Or is it not possible to solve the boot regression without the larger fix?
Yours,
Linus Walleij
RFC PATCH 1/1 pinctrl: at91
Quoted 8 lines · Linus Walleij
Quoted 4 lines · Ludovic Desroches
From my point of view, it is two distinct topics. One is a very important fix because our SAMA5D4 device is not booting without it. The other one is a proper way to manage gpio ranges but alone I don't think it can solve my issue.
Quoted 8 lines
Can you make me a minimal, sane patch that fixes the boot regression?
I hate regressions so these need to be fixed first...
The rest we can discuss I guess.
Or is it not possible to solve the boot regression without the larger fix?
No I don't think we can have a smaller patch than this one to fix it. Next step is to decide if we go further as you suggested, knowing that we would have to modify the device tree. Moreover we will have to rework our pinctrl driver (or write a new one) since we will have new pio controller on future devices.
Regards
Ludovic
RFC PATCH 1/1 pinctrl: at91
Le 19/01/2015 11:30, Ludovic Desroches a ?crit :
Quoted 15 lines · Linus Walleij
Quoted 14 lines
Quoted 4 lines · Ludovic Desroches
From my point of view, it is two distinct topics. One is a very important fix because our SAMA5D4 device is not booting without it. The other one is a proper way to manage gpio ranges but alone I don't think it can solve my issue.
Can you make me a minimal, sane patch that fixes the boot regression?
I hate regressions so these need to be fixed first...
The rest we can discuss I guess.
Quoted 8 lines
Quoted 2 lines
Or is it not possible to solve the boot regression without the larger fix?
No I don't think we can have a smaller patch than this one to fix it. Next step is to decide if we go further as you suggested, knowing that we would have to modify the device tree. Moreover we will have to rework our pinctrl driver (or write a new one) since we will have new pio controller on future devices.
Yes, all the upcoming AT91 SoCs will embed a totally new pinctrl/pinmux IP. We will write a new driver for it and we will be able to use the up-to-date standards of the framework.
In the meantime, this fix allows us to use the SAMA5D4 without too much changes to the original version of our current driver and without modification to all the current SoCs device trees.
Bye,
Signature · 1 line
Nicolas Ferre
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
In reply to Ludovic DesrochesLe 28/11/2014 17:49, Ludovic Desroches a ?crit :
Quoted 9 lines
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.Signed-off-by Ludovic Desroches <[email protected]>
Yes:
Linus, is it too late for 3.18? Otherwise, we can add this tag:
Bye,
Quoted 91 lines
---
Hi,
Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't support unused gpio bank.
drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)drivers/pinctrl/pinctrl-at91.c +18
-12@@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,for_each_child_of_node (np, child) {if (of_device_is_compatible (child, gpio_compat)) { - info->nbanks++; +if (of_device_is_available (child)) + info->nbanks++; }else { info->nfunctions++; info->ngroups +=of_get_child_count (child);@@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info, } size /=sizeof (*list); -if (!size || size % info->nbanks) { +if (!size || size % gpio_banks) {dev_err (info->dev,"wrong mux mask array should be by %d\n" , info->nbanks);return -EINVAL ; } - info->nmux = size / info->nbanks; + info->nmux = size / gpio_banks; info->mux_mask =devm_kzalloc (info->dev,sizeof (u32) * size,GFP_KERNEL );if (!info->mux_mask) {@@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev) {struct at91_pinctrl *info;struct pinctrl_pin_desc *pdesc; -int ret, i, j, k; +int ret, i, j, k, ngpio_chips_enabled =0 ; info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info)@@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)* to obtain references to the struct gpio_chip * for them, and we * need this to proceed. */ -for (i =0 ; i < info->nbanks; i++) { -if (!gpio_chips[i]) { -dev_warn (&pdev->dev,"GPIO chip %d not registered yet\n" , i); -devm_kfree (&pdev->dev, info); -return -EPROBE_DEFER ; - } +for (i =0 ; i < gpio_banks; i++) { +if (gpio_chips[i]) + ngpio_chips_enabled++; + } +if (ngpio_chips_enabled < info->nbanks) { +dev_warn (&pdev->dev,"All GPIO chips are not registered yet\n" ); +devm_kfree (&pdev->dev, info); +return -EPROBE_DEFER ; } at91_pinctrl_desc.name =dev_name (&pdev->dev);@@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev) }/* We will handle a range of GPIO pins */ -for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);dev_info (&pdev->dev,"initialized AT91 pinctrl driver\n" );@@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)for (i =0 ; i < gpio_banks; i++) { at91_gpio = gpio_chips[i]; +if (!at91_gpio) +continue ;/* * GPIO controller are grouped on some SoC:
Signature · 1 line
Nicolas Ferre
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
Quoted 9 lines · Nicolas Ferre
Le 28/11/2014 17:49, Ludovic Desroches a ?crit :
Quoted 8 lines
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.
Quoted 7 lines
Quoted 1 line
Signed-off-by Ludovic Desroches <[email protected]>Yes:
Acked-by Nicolas Ferre <[email protected]>Linus, is it too late for 3.18?
I think it's too late for any kernel as I think it's the wrong fix ;)
Yours,
Linus Walleij
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
In reply to Ludovic DesrochesQuoted 8 lines · Ludovic Desroches
This patch allows to have gpio controller with status set to disabled.
gpio_banksrepresents all the gpio banks available on the device whereas nbanks represents the gpio banks used. Having a disabled gpio controller involves that nbanks value is lower thangpio_banksand that some pointers in thegpio_chipsarray are NULL. This patch deals with these specific cases.
Quoted 7 lines
Signed-off-by Ludovic Desroches <[email protected]>---
Hi,
Without this patch, pinctrl is broken on sama5d4 because pinctrl-at91 doesn't support unused gpio bank.
the problem is different
The current code does support this partially
but need to have few by using gpio_banks for the pinctrl part instead of info->banks
as we already known the last enabled bank
Quoted 76 lines
drivers/pinctrl/pinctrl-at91.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)drivers/pinctrl/pinctrl-at91.c +16
-12@@ -981,7 +981,8 @@ static void at91_pinctrl_child_count(struct at91_pinctrl *info,for_each_child_of_node (np, child) {if (of_device_is_compatible (child, gpio_compat)) { - info->nbanks++; +if (of_device_is_available (child)) + info->nbanks++; }else { info->nfunctions++; info->ngroups +=of_get_child_count (child);@@ -1003,11 +1004,11 @@ static int at91_pinctrl_mux_mask(struct at91_pinctrl *info, } size /=sizeof (*list); -if (!size || size % info->nbanks) { +if (!size || size % gpio_banks) {dev_err (info->dev,"wrong mux mask array should be by %d\n" , info->nbanks);return -EINVAL ; } - info->nmux = size / info->nbanks; + info->nmux = size / gpio_banks; info->mux_mask =devm_kzalloc (info->dev,sizeof (u32) * size,GFP_KERNEL );if (!info->mux_mask) {@@ -1185,7 +1186,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev) {struct at91_pinctrl *info;struct pinctrl_pin_desc *pdesc; -int ret, i, j, k; +int ret, i, j, k, ngpio_chips_enabled =0 ; info =devm_kzalloc (&pdev->dev,sizeof (*info),GFP_KERNEL );if (!info)@@ -1200,12 +1201,14 @@ static int at91_pinctrl_probe(struct platform_device *pdev)* to obtain references to the struct gpio_chip * for them, and we * need this to proceed. */ -for (i =0 ; i < info->nbanks; i++) { -if (!gpio_chips[i]) { -dev_warn (&pdev->dev,"GPIO chip %d not registered yet\n" , i); -devm_kfree (&pdev->dev, info); -return -EPROBE_DEFER ; - } +for (i =0 ; i < gpio_banks; i++) { +if (gpio_chips[i]) + ngpio_chips_enabled++; + } +if (ngpio_chips_enabled < info->nbanks) { +dev_warn (&pdev->dev,"All GPIO chips are not registered yet\n" ); +devm_kfree (&pdev->dev, info); +return -EPROBE_DEFER ; } at91_pinctrl_desc.name =dev_name (&pdev->dev);@@ -1234,8 +1237,9 @@ static int at91_pinctrl_probe(struct platform_device *pdev) }/* We will handle a range of GPIO pins */ -for (i =0 ; i < info->nbanks; i++) -pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range); +for (i =0 ; i < gpio_banks; i++) +if (gpio_chips[i]) +pinctrl_add_gpio_range (info->pctl, &gpio_chips[i]->range);dev_info (&pdev->dev,"initialized AT91 pinctrl driver\n" );
drivers/pinctrl/pinctrl-at91.c static void at91_gpio_probe_fixup(void) · line 1689
Quoted 5 lines
drivers/pinctrl/pinctrl-at91.c +2
-0@@ -1681,6 +1685,8 @@ static void at91_gpio_probe_fixup(void)for (i =0 ; i < gpio_banks; i++) { at91_gpio = gpio_chips[i]; +if (!at91_gpio) +continue ;
here Nack
as if you disable ANY of the bank that use pioc you break the IRQ support
attached a patch we the idea how to handle the current issue only compiled not tested (too late here)
Best Regards,
J.
Quoted 6 lines
drivers/pinctrl/pinctrl-at91.c +0
-0/* * GPIO controller are grouped on some SoC: Signature · 1 line
2.0.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pinctrl-at91-allow-to-have-disable-gpio-bank.patch
Type: text/x-diff
Size: 7649 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150114/b2a3e16e/attachment-0001.bin>
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
Quoted 2 lines · Jean-Christophe PLAGNIOL-VILLARD
attached a patch we the idea how to handle the current issue only compiled not tested (too late here)
Ludovic have you tested Jean-Christophe's patch?
Does it solve the boot issue?
Yours,
Linus Walleij
PATCH 1/2 pinctrl: at91: allow disabled gpio controllers
Quoted 9 lines · Linus Walleij
Quoted 2 lines · Jean-Christophe PLAGNIOL-VILLARD
attached a patch we the idea how to handle the current issue only compiled not tested (too late here)
Ludovic have you tested Jean-Christophe's patch?
Does it solve the boot issue?
Yes I have tested it. I have send a new version since it doesn't totally solved the boot issue. It corrects some potential problems I have missed but there is still an error in the pinctrl probe.
Regards
Ludovic