pinctrl: fsl: imx: Check for 0 config register

PATCH New Markus Pargmann 4 participants 9 messages Mar 24, 2015 – Apr 7, 2015

Subsystems (3)

Arm i.MX and Layerscape Pin control Pin controller - Freescale

lore

Share & tools

lore

Markus Pargmann

author

2015-03-24 15:26 UTC #0 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.

This patch checks for 0 config register and sets the internal value to -1. If the configuration value is not NO_PADCTL this leads to an error message that applying the configuration failed.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/pinctrl/freescale/pinctrl-imx.c +5 -2

index 448f10986c28..2cc400b0a26b 100644
@@ -542,10 +542,13 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
 		struct imx_pin_reg *pin_reg;
 		struct imx_pin *pin = &grp->pins[i];
 
-		if (info->flags & SHARE_MUX_CONF_REG)
+		if (info->flags & SHARE_MUX_CONF_REG) {
 			conf_reg = mux_reg;
-		else
+		} else {
 			conf_reg = be32_to_cpu(*list++);
+			if (!conf_reg)
+				conf_reg = -1;
+		}
 
 		pin_id = mux_reg ? mux_reg / 4 : conf_reg / 4;
 		pin_reg = &info->pin_regs[pin_id];

Signature · 1 line

2.1.4

Linus Walleij

2015-03-27 10:14 UTC #1 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

Quoted 7 lines · Markus Pargmann

0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.

Quoted 5 lines

This patch checks for 0 config register and sets the internal value to -1. If the configuration value is not NO_PADCTL this leads to an error message that applying the configuration failed.
Signed-off-by Markus Pargmann <[email protected]>

Stefan, Uwe: can you look at this patch.

Yours,
Linus Walleij

Stefan Agner

2015-03-27 10:32 UTC #2 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

In reply to Markus Pargmann

Quoted 6 lines · Markus Pargmann

0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.

0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.

What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid

However, I can't think of a easy solution which would solve this in a cleaner mannor... Maybe a comment in the function header how the 5 u32's are intepreted would appropriate?

--
Stefan

Quoted 32 lines

This patch checks for 0 config register and sets the internal value to -1. If the configuration value is not NO_PADCTL this leads to an error message that applying the configuration failed.
Signed-off-by Markus Pargmann <[email protected]>
---
 drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/pinctrl/freescale/pinctrl-imx.c +5 -2
index 448f10986c28..2cc400b0a26b 100644
@@ -542,10 +542,13 @@ static int imx_pinctrl_parse_groups(struct
 device_node *np,
 		struct imx_pin_reg *pin_reg;
 		struct imx_pin *pin = &grp->pins[i];
 
-		if (info->flags & SHARE_MUX_CONF_REG)
+		if (info->flags & SHARE_MUX_CONF_REG) {
 			conf_reg = mux_reg;
-		else
+		} else {
 			conf_reg = be32_to_cpu(*list++);
+			if (!conf_reg)
+				conf_reg = -1;
+		}
 
 		pin_id = mux_reg ? mux_reg / 4 : conf_reg / 4;
pin_reg = &info->pin_regs[pin_id];

Uwe Kleine-König

2015-03-27 10:41 UTC #3 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

Quoted 12 lines · Stefan Agner

Quoted 6 lines · Markus Pargmann
0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.
0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.

Quoted 8 lines

What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid
However, I can't think of a easy solution which would solve this in a cleaner mannor... Maybe a comment in the function header how the 5 u32's are intepreted would appropriate?

That means that on Vybrid you need to write to offset 0 a config value? Or not?

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Stefan Agner

2015-03-27 11:56 UTC #4 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

Quoted 18 lines · Uwe Kleine-K?nig

Quoted 17 lines · Stefan Agner
Quoted 6 lines · Markus Pargmann
0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.
0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.
What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid

Quoted 5 lines

Quoted 3 lines
However, I can't think of a easy solution which would solve this in a cleaner mannor... Maybe a comment in the function header how the 5 u32's are intepreted would appropriate?
That means that on Vybrid you need to write to offset 0 a config value? Or not?

Vybrid has a shared conf/mux_reg, hence the SHARE_MUX_CONF_REG is set. Vybrid need to write to offset 0, and is using the conf_reg as offset value in imx_pinconf_set:
...

	for (i = 0; i < num_configs; i++) {
		if (info->flags & SHARE_MUX_CONF_REG) {
			u32 reg;
			reg = readl(ipctl->base + pin_reg->conf_reg);
			reg &= ~0xffff;
			reg |= configs[i];
			writel(reg, ipctl->base + pin_reg->conf_reg);
		} else {

However, that conf_reg value is derived from mux_reg. This is what is done at that if in that patch. Original code:

		if (info->flags & SHARE_MUX_CONF_REG)
			conf_reg = mux_reg;
		else
			conf_reg = be32_to_cpu(*list++);

So, due to that, this patch does not affect the value computed for conf_reg on Vybrid devices (a value of 0 is still possible for Vybrid). Hence, everything should work fine... The different interpretation of the value "0" for the regs is somewhat confusing, but probably unavoidable without DT changes....

--
Stefan

Linus Walleij

2015-03-30 09:40 UTC #5 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

In reply to Stefan Agner

Quoted 9 lines · Stefan Agner

Quoted 6 lines · Markus Pargmann
0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.

Quoted 8 lines

0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.
What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid

What is generally scary about DTS files is that the DT formats are not ruled by a context-free grammar.

There were attempts to formalize some bindings using BNF but they seems to have been given up.

We need to combat this by trying to be strict and logic ... I can't really understand from the discussion here whether the patch should be applied or not, so I'm not applying it until Stefan | Uwe gives an explicit ACK.

Yours,
Linus Walleij

Uwe Kleine-König

2015-03-30 09:58 UTC #6 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

Quoted 22 lines · Linus Walleij

Quoted 9 lines · Stefan Agner
Quoted 6 lines · Markus Pargmann
0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.
Quoted 8 lines
0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.
What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid
What is generally scary about DTS files is that the DT formats are not ruled by a context-free grammar.

Quoted 7 lines

There were attempts to formalize some bindings using BNF but they seems to have been given up.
We need to combat this by trying to be strict and logic ... I can't really understand from the discussion here whether the patch should be applied or not, so I'm not applying it until Stefan | Uwe gives an explicit ACK.

Acked-by Uwe Kleine-K?nig <[email protected]>

I just have a small bad feeling because -1 and 0 both mean "no config register". But that is a problem already there without Markus' patch. And I see that this patch really fixes a problem. As Stefan sait it should be fine for Vybrid I think applying it is the right thing to do here.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Stefan Agner

2015-03-30 13:11 UTC #7 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

Quoted 30 lines · Uwe Kleine-K?nig

Quoted 22 lines · Linus Walleij
Quoted 9 lines · Stefan Agner
Quoted 6 lines · Markus Pargmann
0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.
Quoted 8 lines
0 is a valid offset for Vybrid's mux register, however, since Vybrid set the SHARE_MUX_CONF_REG, intepreting a 0 in conf_reg as "not set" actually works.
What is a bit a bummer is that we now have different meanings of 0, depending on the field:
- For the first field (mux), 0 is a valid offset
- For the second field (config), 0 means not valid
What is generally scary about DTS files is that the DT formats are not ruled by a context-free grammar.
Quoted 7 lines
There were attempts to formalize some bindings using BNF but they seems to have been given up.
We need to combat this by trying to be strict and logic ... I can't really understand from the discussion here whether the patch should be applied or not, so I'm not applying it until Stefan | Uwe gives an explicit ACK.

Quoted 7 lines

Acked-by Uwe Kleine-K?nig <[email protected]>
I just have a small bad feeling because -1 and 0 both mean "no config register". But that is a problem already there without Markus' patch. And I see that this patch really fixes a problem. As Stefan sait it should be fine for Vybrid I think applying it is the right thing to do here.

Yes, that is how I feel too. As far as I see the situation, it is too late to be strict and logic here. We already have device trees out there which use that different logic, it is only that currently, the code interprets that wrong. This probably worked once, and is actually a regression fix of 3dac1918a491 ("pinctrl: imx: detect uninitialized pins").

For a better future, we could add a define like IMX_NO_PAD_CTL, which uses the highest bit to define that a register does not exist. This would make sure that we don't have a collision with a valid offset (0) as we have today...

To be sure that this does not introduce another regression on Vybrid, I also quickly tested the patch on Vybrid. The pinmux worked smoothly, hence:

Tested-by Stefan Agner <[email protected]>

Acked-by Stefan Agner <[email protected]>

--
Stefan

Quoted 3 lines

Best regards
Uwe

Linus Walleij

2015-04-07 13:11 UTC #8 lore raw

PATCH 1/1 pinctrl: fsl: imx: Check for 0 config register

In reply to Markus Pargmann

Quoted 7 lines · Markus Pargmann

0 is used in all pinfunction definitions when a config register is not available, for example imx25-pinfunc.h. If a configuration value is used for such a pinfunction the driver will always write it to the configuration register if it is not -1. For a 0 configuration register the configuration value is written to offset 0x0. This can lead to a crashing/hanging system without any warning message.

Quoted 5 lines

This patch checks for 0 config register and sets the internal value to -1. If the configuration value is not NO_PADCTL this leads to an error message that applying the configuration failed.
Signed-off-by Markus Pargmann <[email protected]>

Patch applied with the ACKs and all.

Yours,
Linus Walleij