Patchwork [RFC] plat-mxc: iomux-v3.h: implicitly enable pull-up/down when that's desired

login
register
mail settings
Submitter Paul Fertser
Date Oct. 10, 2011, 7:19 a.m.
Message ID <201110100758.p9A7wCYT011235@home.pavel.comp>
Download mbox | patch
Permalink /patch/118652/
State New
Headers show

Comments

Paul Fertser - Oct. 10, 2011, 7:19 a.m.
To configure pads during the initialisation a set of special constants
is used, e.g.
#define MX25_PAD_FEC_MDIO__FEC_MDIO IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)

The problem is that no pull-up/down is getting activated unless both
PAD_CTL_PUE (pull-up enable) and PAD_CTL_PKE (pull/keeper module
enable) set. This is clearly stated in the i.MX25 datasheet and is
confirmed by the measurements on hardware. This leads to some rather
hard to understand bugs such as misdetecting an absent ethernet PHY (a
real bug i had), unstable data transfer etc. This might affect mx25,
mx35, mx50, mx51 and mx53 SoCs.

It's reasonable to expect that if the pullup value is specified, the
intention was to have it actually active, so we implicitly add the
needed bits.

Cc: stable@kernel.org
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---

I'm not sure if that's really suitable for -stable so please excuse me
if it's not. The issue looks real though and if you prefer fixing it
any other way, please let me know. Sascha, if you think this's appropriate,
i'll be happy to send the same fix for barebox.

 arch/arm/plat-mxc/include/mach/iomux-v3.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Baruch Siach - Oct. 10, 2011, 8:14 a.m.
Hi Paul,

On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> To configure pads during the initialisation a set of special constants
> is used, e.g.
> #define MX25_PAD_FEC_MDIO__FEC_MDIO IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)
> 
> The problem is that no pull-up/down is getting activated unless both
> PAD_CTL_PUE (pull-up enable) and PAD_CTL_PKE (pull/keeper module
> enable) set. This is clearly stated in the i.MX25 datasheet and is
> confirmed by the measurements on hardware. This leads to some rather
> hard to understand bugs such as misdetecting an absent ethernet PHY (a
> real bug i had), unstable data transfer etc. This might affect mx25,
> mx35, mx50, mx51 and mx53 SoCs.
> 
> It's reasonable to expect that if the pullup value is specified, the
> intention was to have it actually active, so we implicitly add the
> needed bits.

IMO, this patch should include the removal of the now redundant PAD_CTL_PKE 
from arch/arm/plat-mxc/include/mach/iomux-mx25.h, and other v3 iomux headers.

baruch

> Cc: stable@kernel.org
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
> 
> I'm not sure if that's really suitable for -stable so please excuse me
> if it's not. The issue looks real though and if you prefer fixing it
> any other way, please let me know. Sascha, if you think this's appropriate,
> i'll be happy to send the same fix for barebox.
> 
>  arch/arm/plat-mxc/include/mach/iomux-v3.h |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/include/mach/iomux-v3.h b/arch/arm/plat-mxc/include/mach/iomux-v3.h
> index ebbce33..4509956 100644
> --- a/arch/arm/plat-mxc/include/mach/iomux-v3.h
> +++ b/arch/arm/plat-mxc/include/mach/iomux-v3.h
> @@ -89,11 +89,11 @@ typedef u64 iomux_v3_cfg_t;
>  #define PAD_CTL_HYS			(1 << 8)
>  
>  #define PAD_CTL_PKE			(1 << 7)
> -#define PAD_CTL_PUE			(1 << 6)
> -#define PAD_CTL_PUS_100K_DOWN		(0 << 4)
> -#define PAD_CTL_PUS_47K_UP		(1 << 4)
> -#define PAD_CTL_PUS_100K_UP		(2 << 4)
> -#define PAD_CTL_PUS_22K_UP		(3 << 4)
> +#define PAD_CTL_PUE			(1 << 6 | PAD_CTL_PKE)
> +#define PAD_CTL_PUS_100K_DOWN		(0 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_47K_UP		(1 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_100K_UP		(2 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_22K_UP		(3 << 4 | PAD_CTL_PUE)
>  
>  #define PAD_CTL_ODE			(1 << 3)
Lothar Waßmann - Oct. 10, 2011, 9:54 a.m.
Hi,

Baruch Siach writes:
> Hi Paul,
> 
> On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> > To configure pads during the initialisation a set of special constants
> > is used, e.g.
> > #define MX25_PAD_FEC_MDIO__FEC_MDIO IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)
> > 
> > The problem is that no pull-up/down is getting activated unless both
> > PAD_CTL_PUE (pull-up enable) and PAD_CTL_PKE (pull/keeper module
> > enable) set. This is clearly stated in the i.MX25 datasheet and is
> > confirmed by the measurements on hardware. This leads to some rather
> > hard to understand bugs such as misdetecting an absent ethernet PHY (a
> > real bug i had), unstable data transfer etc. This might affect mx25,
> > mx35, mx50, mx51 and mx53 SoCs.
> > 
> > It's reasonable to expect that if the pullup value is specified, the
> > intention was to have it actually active, so we implicitly add the
> > needed bits.
> 
> IMO, this patch should include the removal of the now redundant PAD_CTL_PKE 
> from arch/arm/plat-mxc/include/mach/iomux-mx25.h, and other v3 iomux headers.
> 
Why should PAD_CTL_PKE be redundant? You still need it to configure a
pin for the 'Keeper' functionality.
Even if there is no default pin configuration that uses this, it
should be possible to enable it when desired.


Lothar Waßmann
Paul Fertser - Oct. 10, 2011, 10:01 a.m.
On Mon, Oct 10, 2011 at 11:54:26AM +0200, Lothar Waßmann wrote:
> Baruch Siach writes:
> > IMO, this patch should include the removal of the now redundant PAD_CTL_PKE 
> > from arch/arm/plat-mxc/include/mach/iomux-mx25.h, and other v3 iomux headers.
> > 
> Why should PAD_CTL_PKE be redundant? You still need it to configure a
> pin for the 'Keeper' functionality.
> Even if there is no default pin configuration that uses this, it
> should be possible to enable it when desired.

He meant not removing the definition itself but removing its usage
whereever it becomes redudant if my patch is accepted. I can do that
with a follow-up patch if desired.
Sascha Hauer - Oct. 12, 2011, 8:27 a.m.
On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> To configure pads during the initialisation a set of special constants
> is used, e.g.
> #define MX25_PAD_FEC_MDIO__FEC_MDIO IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)
> 
> The problem is that no pull-up/down is getting activated unless both
> PAD_CTL_PUE (pull-up enable) and PAD_CTL_PKE (pull/keeper module
> enable) set. This is clearly stated in the i.MX25 datasheet and is
> confirmed by the measurements on hardware. This leads to some rather
> hard to understand bugs such as misdetecting an absent ethernet PHY (a
> real bug i had), unstable data transfer etc. This might affect mx25,
> mx35, mx50, mx51 and mx53 SoCs.
> 
> It's reasonable to expect that if the pullup value is specified, the
> intention was to have it actually active, so we implicitly add the
> needed bits.
> 
> Cc: stable@kernel.org
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
> 
> I'm not sure if that's really suitable for -stable so please excuse me
> if it's not.

I think it's not suitable for stable unless there is a real bug, that
is PUE or PKE are forgotten somewhere.

> The issue looks real though and if you prefer fixing it
> any other way, please let me know. Sascha, if you think this's appropriate,
> i'll be happy to send the same fix for barebox.

Yes please, once we agree what to do about this issue.

> 
>  arch/arm/plat-mxc/include/mach/iomux-v3.h |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/include/mach/iomux-v3.h b/arch/arm/plat-mxc/include/mach/iomux-v3.h
> index ebbce33..4509956 100644
> --- a/arch/arm/plat-mxc/include/mach/iomux-v3.h
> +++ b/arch/arm/plat-mxc/include/mach/iomux-v3.h
> @@ -89,11 +89,11 @@ typedef u64 iomux_v3_cfg_t;
>  #define PAD_CTL_HYS			(1 << 8)
>  
>  #define PAD_CTL_PKE			(1 << 7)
> -#define PAD_CTL_PUE			(1 << 6)
> -#define PAD_CTL_PUS_100K_DOWN		(0 << 4)
> -#define PAD_CTL_PUS_47K_UP		(1 << 4)
> -#define PAD_CTL_PUS_100K_UP		(2 << 4)
> -#define PAD_CTL_PUS_22K_UP		(3 << 4)
> +#define PAD_CTL_PUE			(1 << 6 | PAD_CTL_PKE)
> +#define PAD_CTL_PUS_100K_DOWN		(0 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_47K_UP		(1 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_100K_UP		(2 << 4 | PAD_CTL_PUE)
> +#define PAD_CTL_PUS_22K_UP		(3 << 4 | PAD_CTL_PUE)

I don't like that the defines which are supposed to be defines for
the individual bits are changed. This may lead to trouble and confusion
once someone wants to read the values from the iomux registers and
tests for bits. How about Adding new defines like this instead:

/*
 * pullup/down only works with PKE/PUE set. Use these in board code
 */
#define PAD_CTL_100K_DOWN        (PAD_CTL_PUS_100K_DOWN | PAD_CTL_PUE | PAD_CTL_PKE)
#define PAD_CTL_47K_UP           (PAD_CTL_PUS_47K_UP | PAD_CTL_PUE | PAD_CTL_PKE)
#define PAD_CTL_100K_UP          (PAD_CTL_PUS_100K_UP | PAD_CTL_PUE | PAD_CTL_PKE)
#define PAD_CTL_22K_UP           (PAD_CTL_PUS_22K_UP | PAD_CTL_PUE | PAD_CTL_PKE)


Sascha
Shawn Guo - Oct. 13, 2011, 1:48 a.m.
On Wed, Oct 12, 2011 at 10:27:02AM +0200, Sascha Hauer wrote:
> On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> > To configure pads during the initialisation a set of special constants
> > is used, e.g.
> > #define MX25_PAD_FEC_MDIO__FEC_MDIO IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)
> > 
> > The problem is that no pull-up/down is getting activated unless both
> > PAD_CTL_PUE (pull-up enable) and PAD_CTL_PKE (pull/keeper module
> > enable) set. This is clearly stated in the i.MX25 datasheet and is
> > confirmed by the measurements on hardware. This leads to some rather
> > hard to understand bugs such as misdetecting an absent ethernet PHY (a
> > real bug i had), unstable data transfer etc. This might affect mx25,
> > mx35, mx50, mx51 and mx53 SoCs.
> > 
> > It's reasonable to expect that if the pullup value is specified, the
> > intention was to have it actually active, so we implicitly add the
> > needed bits.
> > 
> > Cc: stable@kernel.org
> > Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> > ---
> > 
> > I'm not sure if that's really suitable for -stable so please excuse me
> > if it's not.
> 
> I think it's not suitable for stable unless there is a real bug, that
> is PUE or PKE are forgotten somewhere.
> 
It seems that the example of MX25_PAD_FEC_MDIO__FEC_MDIO Paul put in
the commit message is a real bug.

$ git grep -n MX25_PAD_FEC_MDIO__FEC_MDIO arch/arm
arch/arm/mach-imx/mach-eukrea_cpuimx25.c:50:    MX25_PAD_FEC_MDIO__FEC_MDIO,
arch/arm/mach-imx/mach-mx25_3ds.c:52:   MX25_PAD_FEC_MDIO__FEC_MDIO,
arch/arm/plat-mxc/include/mach/iomux-mx25.h:419:#define MX25_PAD_FEC_MDIO__FEC_MDIO     IOMUX_PAD(0x3c4, 0x1cc, 0x10, 0, 0, PAD_CTL_HYS | PAD_CTL_PUS_22K_UP)
Paul Fertser - Oct. 13, 2011, 7:12 a.m.
On Wed, Oct 12, 2011 at 10:27:02AM +0200, Sascha Hauer wrote:
> On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> > This leads to some rather hard to understand bugs such as
> > misdetecting an absent ethernet PHY (a real bug i had)
...
> > This might affect mx25, mx35, mx50, mx51 and mx53 SoCs.
...
> > I'm not sure if that's really suitable for -stable so please excuse me
> > if it's not.
> 
> I think it's not suitable for stable unless there is a real bug, that
> is PUE or PKE are forgotten somewhere.

If there wasn't a real bug with MDIO, i wouldn't have found the
problem. I've also checked all other potentially affected SoCs and all
of them (except for the mx35, whose iomux file doesn't configure any
pads at all) had some suspicious entries.

> I don't like that the defines which are supposed to be defines for
> the individual bits are changed. This may lead to trouble and confusion
> once someone wants to read the values from the iomux registers and
> tests for bits. How about Adding new defines like this instead:

I perfectly understand the reasoning and fully agree it would be best
to do it that way right from the beginning. But given the current
situation when many affected iomux headers share the error that can
lead to subtle strange bugs, i thought it might be beneficial to fix
them all implicitly. And testing for those pull-up bits set without
testing for PUE/PKE doesn't seem to be of any use, and also before
sending a patch i did a git grep and found no examples of using those
constants in an unexpected way.

If you still prefer the cleaner way, i can also do that and correct
all the suspicious entries manually, just let me know.
Sascha Hauer - Oct. 13, 2011, 7:17 a.m.
On Thu, Oct 13, 2011 at 11:12:24AM +0400, Paul Fertser wrote:
> On Wed, Oct 12, 2011 at 10:27:02AM +0200, Sascha Hauer wrote:
> > On Mon, Oct 10, 2011 at 11:19:23AM +0400, Paul Fertser wrote:
> > > This leads to some rather hard to understand bugs such as
> > > misdetecting an absent ethernet PHY (a real bug i had)
> ...
> > > This might affect mx25, mx35, mx50, mx51 and mx53 SoCs.
> ...
> > > I'm not sure if that's really suitable for -stable so please excuse me
> > > if it's not.
> > 
> > I think it's not suitable for stable unless there is a real bug, that
> > is PUE or PKE are forgotten somewhere.
> 
> If there wasn't a real bug with MDIO, i wouldn't have found the
> problem. I've also checked all other potentially affected SoCs and all
> of them (except for the mx35, whose iomux file doesn't configure any
> pads at all) had some suspicious entries.
> 
> > I don't like that the defines which are supposed to be defines for
> > the individual bits are changed. This may lead to trouble and confusion
> > once someone wants to read the values from the iomux registers and
> > tests for bits. How about Adding new defines like this instead:
> 
> I perfectly understand the reasoning and fully agree it would be best
> to do it that way right from the beginning. But given the current
> situation when many affected iomux headers share the error that can
> lead to subtle strange bugs, i thought it might be beneficial to fix
> them all implicitly. And testing for those pull-up bits set without
> testing for PUE/PKE doesn't seem to be of any use, and also before
> sending a patch i did a git grep and found no examples of using those
> constants in an unexpected way.

Ok, convinced. Let's go with your patch for now as it's the least
intrusive way to deal with it. Still I think we should change this
as I suggested later.

Sascha

Patch

diff --git a/arch/arm/plat-mxc/include/mach/iomux-v3.h b/arch/arm/plat-mxc/include/mach/iomux-v3.h
index ebbce33..4509956 100644
--- a/arch/arm/plat-mxc/include/mach/iomux-v3.h
+++ b/arch/arm/plat-mxc/include/mach/iomux-v3.h
@@ -89,11 +89,11 @@  typedef u64 iomux_v3_cfg_t;
 #define PAD_CTL_HYS			(1 << 8)
 
 #define PAD_CTL_PKE			(1 << 7)
-#define PAD_CTL_PUE			(1 << 6)
-#define PAD_CTL_PUS_100K_DOWN		(0 << 4)
-#define PAD_CTL_PUS_47K_UP		(1 << 4)
-#define PAD_CTL_PUS_100K_UP		(2 << 4)
-#define PAD_CTL_PUS_22K_UP		(3 << 4)
+#define PAD_CTL_PUE			(1 << 6 | PAD_CTL_PKE)
+#define PAD_CTL_PUS_100K_DOWN		(0 << 4 | PAD_CTL_PUE)
+#define PAD_CTL_PUS_47K_UP		(1 << 4 | PAD_CTL_PUE)
+#define PAD_CTL_PUS_100K_UP		(2 << 4 | PAD_CTL_PUE)
+#define PAD_CTL_PUS_22K_UP		(3 << 4 | PAD_CTL_PUE)
 
 #define PAD_CTL_ODE			(1 << 3)