| Message ID | 1427210778-28037-1-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 24, 2015 at 4:26 PM, Markus Pargmann <mpa@pengutronix.de> wrote: > 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 <mpa@pengutronix.de> Stefan, Uwe: can you look at this patch. Yours, Linus Walleij
On 2015-03-24 16:26, Markus Pargmann wrote: > 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 > > 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 <mpa@pengutronix.de> > --- > drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > b/drivers/pinctrl/freescale/pinctrl-imx.c > index 448f10986c28..2cc400b0a26b 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -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];
On Fri, Mar 27, 2015 at 11:32:31AM +0100, Stefan Agner wrote: > On 2015-03-24 16:26, Markus Pargmann wrote: > > 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? That means that on Vybrid you need to write to offset 0 a config value? Or not? Best regards Uwe
On 2015-03-27 11:41, Uwe Kleine-König wrote: > On Fri, Mar 27, 2015 at 11:32:31AM +0100, Stefan Agner wrote: >> On 2015-03-24 16:26, Markus Pargmann wrote: >> > 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? > 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
On Fri, Mar 27, 2015 at 11:32 AM, Stefan Agner <stefan@agner.ch> wrote: > On 2015-03-24 16:26, Markus Pargmann wrote: >> 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 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
On Mon, Mar 30, 2015 at 11:40:47AM +0200, Linus Walleij wrote: > On Fri, Mar 27, 2015 at 11:32 AM, Stefan Agner <stefan@agner.ch> wrote: > > On 2015-03-24 16:26, Markus Pargmann wrote: > > >> 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 > > 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. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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
On 2015-03-30 11:58, Uwe Kleine-König wrote: > On Mon, Mar 30, 2015 at 11:40:47AM +0200, Linus Walleij wrote: >> On Fri, Mar 27, 2015 at 11:32 AM, Stefan Agner <stefan@agner.ch> wrote: >> > On 2015-03-24 16:26, Markus Pargmann wrote: >> >> >> 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 >> >> 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. > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > 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 <stefan@agner.ch> Acked-by: Stefan Agner <stefan@agner.ch> -- Stefan > > Best regards > Uwe
On Tue, Mar 24, 2015 at 4:26 PM, Markus Pargmann <mpa@pengutronix.de> wrote: > 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 <mpa@pengutronix.de> Patch applied with the ACKs and all. Yours, Linus Walleij
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 448f10986c28..2cc400b0a26b 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -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];
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 <mpa@pengutronix.de> --- drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)