[BISECTED] v4.5-rc1 mvebu/kirkwood-pinctrl regression

[BISECTED] v4.5-rc1 mvebu/kirkwood-pinctrl regression

[Aaro Koskinen]

Hi,

On kirkwood-openrd-client SDIO stopped working, and I get the following
pinctrl logs:

[    0.096502] pinctrl core: initialized pinctrl subsystem
[    1.765275] kirkwood-pinctrl f1010000.pin-controller: registered pinctrl driver
[    5.375416] kirkwood-pinctrl f1010000.pin-controller: unsupported function sdio on pin mpp12
[    5.383972] pinctrl core: failed to register map default (5): invalid type given

Bisected to:

commit 0581b16b184031e767f64d20a93f65433d678685
Author: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date:   Sat Nov 28 11:26:49 2015 +0100

    pinctrl: mvebu: complain about missing group after checking variant

Reverting this patch makes my rootfs (SD card) accessible again.

A.

[BISECTED] v4.5-rc1 mvebu/kirkwood-pinctrl regression

[Andrew Lunn]

On Tue, Jan 26, 2016 at 08:22:51PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On kirkwood-openrd-client SDIO stopped working, and I get the following
> pinctrl logs:
> 
> [    0.096502] pinctrl core: initialized pinctrl subsystem
> [    1.765275] kirkwood-pinctrl f1010000.pin-controller: registered pinctrl driver
> [    5.375416] kirkwood-pinctrl f1010000.pin-controller: unsupported function sdio on pin mpp12
> [    5.383972] pinctrl core: failed to register map default (5): invalid type given

Humm.. What does

/sys/kernel/debug/pinctrl/f1010000.pin-controller/pinmux-functions show?

On my 6282 i have:

function: sdio, groups = [ mpp12 mpp13 mpp14 mpp15 mpp16 mpp17 ]

What about

/sys/kernel/debug/pinctrl/f1010000.pin-controller/pinmux-pins 

Has something already claimed it?

Thanks
	Andrew

[BISECTED] v4.5-rc1 mvebu/kirkwood-pinctrl regression

[Sebastian Hesselbarth]

On 26.01.2016 21:20, Andrew Lunn wrote:
> On Tue, Jan 26, 2016 at 08:22:51PM +0200, Aaro Koskinen wrote:
>> Hi,
>>
>> On kirkwood-openrd-client SDIO stopped working, and I get the following
>> pinctrl logs:
>>
>> [    0.096502] pinctrl core: initialized pinctrl subsystem
>> [    1.765275] kirkwood-pinctrl f1010000.pin-controller: registered pinctrl driver
>> [    5.375416] kirkwood-pinctrl f1010000.pin-controller: unsupported function sdio on pin mpp12
>> [    5.383972] pinctrl core: failed to register map default (5): invalid type given
> 
> Humm.. What does
> 
> /sys/kernel/debug/pinctrl/f1010000.pin-controller/pinmux-functions show?
> 
> On my 6282 i have:
> 
> function: sdio, groups = [ mpp12 mpp13 mpp14 mpp15 mpp16 mpp17 ]
> 
> What about
> 
> /sys/kernel/debug/pinctrl/f1010000.pin-controller/pinmux-pins 
> 
> Has something already claimed it?

Nah, Aaro is right, pinctrl-mvebu is broken for mpp12 on 6281. We should
revisit static mpp mode to dynamic mpp group assignment one day.

I'll send a fix in a minute when compiling v4.5-rc1 is done.

Sebastian

[PATCH] pinctrl: mvebu: fix num_settings in mpp group assignment

[Sebastian Hesselbarth]

When assigning mpp settings from static mpp modes to mpp groups,
we do not want any groups that have no setting for a specific Kirkwood
variant. However, when there is at least a single setting, we need to
assign the number of all settings in this mode to grp->num_settings
as we are reusing the static modes table.

Fixes: 0581b16b1840 ("pinctrl: mvebu: complain about missing group after checking variant")
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Aaro,

thanks for reporting the issue. It really looks like we should rework
the mpp mode/setting/group assignment one day. Please apply this fix
and report back with a Tested-by if it works.

Sebastian
---
 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index e4d473811bb3..4a3c7fe533b5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -663,11 +663,11 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 	/* assign mpp modes to groups */
 	for (n = 0; n < soc->nmodes; n++) {
 		struct mvebu_mpp_mode *mode = &soc->modes[n];
-		struct mvebu_mpp_ctrl_setting *set = &mode->settings[0];
+		struct mvebu_mpp_ctrl_setting *set;
 		struct mvebu_pinctrl_group *grp;
 		unsigned num_settings;
 
-		for (num_settings = 0; ; set++) {
+		for (num_settings = 0, set = &mode->settings[0]; ; set++) {
 			if (!set->name)
 				break;
 
@@ -676,6 +676,21 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 				continue;
 
 			num_settings++;
+		}
+
+		/* skip modes with no settings for this variant */
+		if (!num_settings)
+			continue;
+
+		for (num_settings = 0, set = &mode->settings[0]; ; set++) {
+			if (!set->name)
+				break;
+
+			num_settings++;
+
+			/* skip unsupported settings for this variant */
+			if (pctl->variant && !(pctl->variant & set->variant))
+				continue;
 
 			/* find gpio/gpo/gpi settings */
 			if (strcmp(set->name, "gpio") == 0)
@@ -687,10 +702,6 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 				set->flags = MVEBU_SETTING_GPI;
 		}
 
-		/* skip modes with no settings for this variant */
-		if (!num_settings)
-			continue;
-
 		grp = mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
 		if (!grp) {
 			dev_warn(&pdev->dev, "unknown pinctrl group %d\n",
-- 
2.1.4

[PATCH] pinctrl: mvebu: fix num_settings in mpp group assignment

[Aaro Koskinen]

Hi,

On Tue, Jan 26, 2016 at 10:49:12PM +0100, Sebastian Hesselbarth wrote:
> When assigning mpp settings from static mpp modes to mpp groups,
> we do not want any groups that have no setting for a specific Kirkwood
> variant. However, when there is at least a single setting, we need to
> assign the number of all settings in this mode to grp->num_settings
> as we are reusing the static modes table.
> 
> Fixes: 0581b16b1840 ("pinctrl: mvebu: complain about missing group after checking variant")
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Aaro,
> 
> thanks for reporting the issue. It really looks like we should rework
> the mpp mode/setting/group assignment one day. Please apply this fix
> and report back with a Tested-by if it works.

This fixes the SDIO on kirkwood-openrd-client.

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

Thanks,

A.

> 
> Sebastian
> ---
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index e4d473811bb3..4a3c7fe533b5 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -663,11 +663,11 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
>  	/* assign mpp modes to groups */
>  	for (n = 0; n < soc->nmodes; n++) {
>  		struct mvebu_mpp_mode *mode = &soc->modes[n];
> -		struct mvebu_mpp_ctrl_setting *set = &mode->settings[0];
> +		struct mvebu_mpp_ctrl_setting *set;
>  		struct mvebu_pinctrl_group *grp;
>  		unsigned num_settings;
>  
> -		for (num_settings = 0; ; set++) {
> +		for (num_settings = 0, set = &mode->settings[0]; ; set++) {
>  			if (!set->name)
>  				break;
>  
> @@ -676,6 +676,21 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
>  				continue;
>  
>  			num_settings++;
> +		}
> +
> +		/* skip modes with no settings for this variant */
> +		if (!num_settings)
> +			continue;
> +
> +		for (num_settings = 0, set = &mode->settings[0]; ; set++) {
> +			if (!set->name)
> +				break;
> +
> +			num_settings++;
> +
> +			/* skip unsupported settings for this variant */
> +			if (pctl->variant && !(pctl->variant & set->variant))
> +				continue;
>  
>  			/* find gpio/gpo/gpi settings */
>  			if (strcmp(set->name, "gpio") == 0)
> @@ -687,10 +702,6 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
>  				set->flags = MVEBU_SETTING_GPI;
>  		}
>  
> -		/* skip modes with no settings for this variant */
> -		if (!num_settings)
> -			continue;
> -
>  		grp = mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
>  		if (!grp) {
>  			dev_warn(&pdev->dev, "unknown pinctrl group %d\n",
> -- 
> 2.1.4
> 

[PATCH v2] pinctrl: mvebu: fix num_settings in mpp group assignment

[Sebastian Hesselbarth]

When assigning mpp settings from static mpp modes to mpp groups,
we do not want any groups that have no supported setting for a
specific Kirkwood variant. However, when there is at least a
single supported setting, we need to assign the number of all
settings in this mode to grp->num_settings as we are reusing
the static modes table.

Fixes: 0581b16b1840 ("pinctrl: mvebu: complain about missing group after checking variant")
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
--
Changelog:
v1->v2:
- Chose a less intrusive fix by adding supp_settings variable
  instead of looping over the settings twice.

Aaro, Linus,

I prefer this less intrusive fix in favor of v1.

@Aaro: Can you please test again?

Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index e4d473811bb3..3ef798fac81b 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -666,16 +666,19 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 		struct mvebu_mpp_ctrl_setting *set = &mode->settings[0];
 		struct mvebu_pinctrl_group *grp;
 		unsigned num_settings;
+		unsigned supp_settings;
 
-		for (num_settings = 0; ; set++) {
+		for (num_settings = 0, supp_settings = 0; ; set++) {
 			if (!set->name)
 				break;
 
+			num_settings++;
+
 			/* skip unsupported settings for this variant */
 			if (pctl->variant && !(pctl->variant & set->variant))
 				continue;
 
-			num_settings++;
+			supp_settings++;
 
 			/* find gpio/gpo/gpi settings */
 			if (strcmp(set->name, "gpio") == 0)
@@ -688,7 +691,7 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
 		}
 
 		/* skip modes with no settings for this variant */
-		if (!num_settings)
+		if (!supp_settings)
 			continue;
 
 		grp = mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
-- 
2.1.4

[PATCH v2] pinctrl: mvebu: fix num_settings in mpp group assignment

[Aaro Koskinen]

Hi,

On Mon, Feb 01, 2016 at 11:48:23PM +0100, Sebastian Hesselbarth wrote:
> When assigning mpp settings from static mpp modes to mpp groups,
> we do not want any groups that have no supported setting for a
> specific Kirkwood variant. However, when there is at least a
> single supported setting, we need to assign the number of all
> settings in this mode to grp->num_settings as we are reusing
> the static modes table.
> 
> Fixes: 0581b16b1840 ("pinctrl: mvebu: complain about missing group after checking variant")
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> --
> Changelog:
> v1->v2:
> - Chose a less intrusive fix by adding supp_settings variable
>   instead of looping over the settings twice.
> 
> Aaro, Linus,
> 
> I prefer this less intrusive fix in favor of v1.
> 
> @Aaro: Can you please test again?

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

A.

> 
> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index e4d473811bb3..3ef798fac81b 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -666,16 +666,19 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
>  		struct mvebu_mpp_ctrl_setting *set = &mode->settings[0];
>  		struct mvebu_pinctrl_group *grp;
>  		unsigned num_settings;
> +		unsigned supp_settings;
>  
> -		for (num_settings = 0; ; set++) {
> +		for (num_settings = 0, supp_settings = 0; ; set++) {
>  			if (!set->name)
>  				break;
>  
> +			num_settings++;
> +
>  			/* skip unsupported settings for this variant */
>  			if (pctl->variant && !(pctl->variant & set->variant))
>  				continue;
>  
> -			num_settings++;
> +			supp_settings++;
>  
>  			/* find gpio/gpo/gpi settings */
>  			if (strcmp(set->name, "gpio") == 0)
> @@ -688,7 +691,7 @@ int mvebu_pinctrl_probe(struct platform_device *pdev)
>  		}
>  
>  		/* skip modes with no settings for this variant */
> -		if (!num_settings)
> +		if (!supp_settings)
>  			continue;
>  
>  		grp = mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
> -- 
> 2.1.4
> 

[PATCH v2] pinctrl: mvebu: fix num_settings in mpp group assignment

[Linus Walleij]

On Mon, Feb 1, 2016 at 11:48 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

> When assigning mpp settings from static mpp modes to mpp groups,
> we do not want any groups that have no supported setting for a
> specific Kirkwood variant. However, when there is at least a
> single supported setting, we need to assign the number of all
> settings in this mode to grp->num_settings as we are reusing
> the static modes table.
>
> Fixes: 0581b16b1840 ("pinctrl: mvebu: complain about missing group after checking variant")
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> --
> Changelog:
> v1->v2:
> - Chose a less intrusive fix by adding supp_settings variable
>   instead of looping over the settings twice.

Patch applied with Aaro's Test tag.

Yours,
Linus Walleij