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

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

[Paul Fertser]

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 at 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)
 
-- 
1.7.2.3

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

[Baruch Siach]

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 at 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)

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Lothar Waßmann]

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
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

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

[Paul Fertser]

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.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav at gmail.com

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

[Sascha Hauer]

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 at 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


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Shawn Guo]

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 at 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)

-- 
Regards,
Shawn

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

[Paul Fertser]

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.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav at gmail.com

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

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |