Strange message from Kirkwood pinctrl driver

Strange message from Kirkwood pinctrl driver

[Linus Walleij]

Hi Sebastian,

trying to use the Kirkwood pinctrl driver with compatible =
"marvell,88f6192-pinctrl";
on a Pogoplug series 4 yields the following message when instantiating
the driver:

kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 36
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 37
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 38
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 39
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 40
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 41
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 42
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 43
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 44
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 45
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 46
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 47
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 48
kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 49
kirkwood-pinctrl f1010000.pin-controller: registered pinctrl driver

It looks harmless but seems like a bug and make me uncertain.

The following naive patch fixes it:

diff --git a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
index 0f07dc554a1d..6c7c2c8819b8 100644
--- a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
+++ b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
@@ -411,7 +411,7 @@ static struct mvebu_pinctrl_soc_info mv88f6190_info = {
        .controls = mv88f619x_mpp_controls,
        .ncontrols = ARRAY_SIZE(mv88f619x_mpp_controls),
        .modes = mv88f6xxx_mpp_modes,
-       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes),
+       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes) - 14,
        .gpioranges = mv88f619x_gpio_ranges,
        .ngpioranges = ARRAY_SIZE(mv88f619x_gpio_ranges),
 };
@@ -421,7 +421,7 @@ static struct mvebu_pinctrl_soc_info mv88f6192_info = {
        .controls = mv88f619x_mpp_controls,
        .ncontrols = ARRAY_SIZE(mv88f619x_mpp_controls),
        .modes = mv88f6xxx_mpp_modes,
-       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes),
+       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes) - 14,
        .gpioranges = mv88f619x_gpio_ranges,
        .ngpioranges = ARRAY_SIZE(mv88f619x_gpio_ranges),
 };

What is the proper way to fix this?

Yours,
Linus Walleij

Re: Strange message from Kirkwood pinctrl driver

[Andrew Lunn]

On Wed, Nov 25, 2015 at 11:27:46AM +0100, Linus Walleij wrote:
> Hi Sebastian,
> 
> trying to use the Kirkwood pinctrl driver with compatible =
> "marvell,88f6192-pinctrl";
> on a Pogoplug series 4 yields the following message when instantiating
> the driver:
> 
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 36
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 37
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 38
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 39
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 40

Hi Linus

Humm, interesting. I don't remember it doing that before. I will look
into this in the next couple of days.

     Andrew

Re: Strange message from Kirkwood pinctrl driver

[Linus Walleij]

On Wed, Nov 25, 2015 at 3:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Nov 25, 2015 at 11:27:46AM +0100, Linus Walleij wrote:
>> Hi Sebastian,
>>
>> trying to use the Kirkwood pinctrl driver with compatible =
>> "marvell,88f6192-pinctrl";
>> on a Pogoplug series 4 yields the following message when instantiating
>> the driver:
>>
>> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 36
>> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 37
>> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 38
>> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 39
>> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 40
>
> Hi Linus
>
> Humm, interesting. I don't remember it doing that before. I will look
> into this in the next couple of days.

There are only few devices using it: it is declared in
kirkwood-6192.dtsi and that is just used by:

kirkwood-blackarmor-nas220.dts
kirkwood-laplug.dts
kirkwood-rd88f6192.dts
... and my new device tree

Interestingly, kirkwood-ns2[lite|mini].dts specifies it's compatible
with "marvell,kirkwood-88f6192" but instead includes
kirkwood-ns2-common.dtsi which includes
kirkwood-6281.dtsi. I haven't wrapped my head around that
one, I suspect it's a bug.

Yours,
Linus Walleij

Re: Strange message from Kirkwood pinctrl driver

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Wed, Nov 25, 2015 at 03:55:16PM +0100, Linus Walleij wrote:
> On Wed, Nov 25, 2015 at 3:46 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Nov 25, 2015 at 11:27:46AM +0100, Linus Walleij wrote:
> >> Hi Sebastian,
> >>
> >> trying to use the Kirkwood pinctrl driver with compatible =
> >> "marvell,88f6192-pinctrl";
> >> on a Pogoplug series 4 yields the following message when instantiating
> >> the driver:
> >>
> >> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 36
> >> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 37
> >> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 38
> >> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 39
> >> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 40
> >
> > Hi Linus
> >
> > Humm, interesting. I don't remember it doing that before. I will look
> > into this in the next couple of days.
> 
> There are only few devices using it: it is declared in
> kirkwood-6192.dtsi and that is just used by:
> 
> kirkwood-blackarmor-nas220.dts
> kirkwood-laplug.dts
> kirkwood-rd88f6192.dts
> ... and my new device tree
> 
> Interestingly, kirkwood-ns2[lite|mini].dts specifies it's compatible
> with "marvell,kirkwood-88f6192" but instead includes
> kirkwood-ns2-common.dtsi which includes
> kirkwood-6281.dtsi. I haven't wrapped my head around that
> one, I suspect it's a bug.

Don't wrap your head too much about that :) It is indeed a bug and it
explains why I didn't see this messages before.

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Strange message from Kirkwood pinctrl driver

[Sebastian Hesselbarth]

On 25.11.2015 11:27, Linus Walleij wrote:
> trying to use the Kirkwood pinctrl driver with compatible =
> "marvell,88f6192-pinctrl";
> on a Pogoplug series 4 yields the following message when instantiating
> the driver:
>
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 36
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 37
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 38
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 39
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 40
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 41
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 42
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 43
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 44
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 45
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 46
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 47
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 48
> kirkwood-pinctrl f1010000.pin-controller: unknown pinctrl group 49
> kirkwood-pinctrl f1010000.pin-controller: registered pinctrl driver
>
> It looks harmless but seems like a bug and make me uncertain.
>
> The following naive patch fixes it:
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
> b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
> index 0f07dc554a1d..6c7c2c8819b8 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-kirkwood.c
> @@ -411,7 +411,7 @@ static struct mvebu_pinctrl_soc_info mv88f6190_info = {
>          .controls = mv88f619x_mpp_controls,
>          .ncontrols = ARRAY_SIZE(mv88f619x_mpp_controls),
>          .modes = mv88f6xxx_mpp_modes,
> -       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes),
> +       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes) - 14,
>          .gpioranges = mv88f619x_gpio_ranges,
>          .ngpioranges = ARRAY_SIZE(mv88f619x_gpio_ranges),
>   };
> @@ -421,7 +421,7 @@ static struct mvebu_pinctrl_soc_info mv88f6192_info = {
>          .controls = mv88f619x_mpp_controls,
>          .ncontrols = ARRAY_SIZE(mv88f619x_mpp_controls),
>          .modes = mv88f6xxx_mpp_modes,
> -       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes),
> +       .nmodes = ARRAY_SIZE(mv88f6xxx_mpp_modes) - 14,
>          .gpioranges = mv88f619x_gpio_ranges,
>          .ngpioranges = ARRAY_SIZE(mv88f619x_gpio_ranges),
>   };
>
> What is the proper way to fix this?

Linus,

I had a quick look at the pinctrl driver.

mv88f6xxx_mpp_modes contains mpp modes 0-49 plus corresponding
functions for all Kirkwood SoCs, some SoCs only have a subset
of that.

Looking at

static struct mvebu_mpp_ctrl mv88f619x_mpp_controls[] = {
	MPP_FUNC_CTRL(0, 35, NULL, kirkwood_mpp_ctrl),
};

Kirkwood 619x only provides mpp0-35.

Now in pinctrl-mvebu.c, we loop over the controls and
collect the number of available groups. For kirkwood
there are no groups with more than one single mpp pin
like Dove has.

/* count controls and create names for mvebu generic
    register controls; also does sanity checks */
pctl->num_groups = 0;
pctl->desc.npins = 0;
for (n = 0; n < soc->ncontrols; n++) {
	struct mvebu_mpp_ctrl *ctrl = &soc->controls[n];

	...

	/*
	 * We allow to pass controls with NULL name that we treat
	 * as a range of one-pin groups with generic mvebu register
	 * controls.
	 */
	if (!ctrl->name) {
		pctl->num_groups += ctrl->npins;
		...
	} else {
		pctl->num_groups += 1;
	}
}

After the loop pctl->num_groups is 36, i.e. mpp0 to mpp35.

A little later, we do:

/* assign mpp modes to groups */
for (n = 0; n < soc->nmodes; n++) {
	struct mvebu_mpp_mode *mode = &soc->modes[n];
	struct mvebu_pinctrl_group *grp =
		mvebu_pinctrl_find_group_by_pid(pctl, mode->pid);
	unsigned num_settings;

	if (!grp) {
		dev_warn(&pdev->dev, "unknown pinctrl group %d\n",
			mode->pid);
		continue;
	}

	...
}

Which is looping over all modes (0-49) passed to the pinctrl-mvebu
core driver. As said earlier, we pass one control with range from
0-35 that gets translated to 36 groups (pctl->num_groups).
mvebu_find_group_by_pid() will try to find the corresponding group
for a given pin number by checking pctl->num_groups.

That obviously fails for modes 36-49 and will issue that annoying
warning.

IMHO, the correct fix will be to make the last loop above run from
0 to min(pctl->num_groups, soc->nmodes) instead of soc->nmodes.

We could also limit pctl->num_groups to soc->nmodes earlier and let
the loop run from 0 to pctl->num_groups.

I am very short on time, but if nobody else jumps in earlier, I can
stich a patch within a week or so.

Thanks for reporting the issue,

Sebastian