| Message ID | 201110100758.p9A7wCYT011235@home.pavel.comp |
|---|---|
| State | New |
| Headers | show |
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)
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
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.
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
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)
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.
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
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)
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(-)