Patchwork [U-Boot,v2,2/8] mxs: Fix PULLUP/NOPULL definitions

login
register
mail settings
Submitter Fabio Estevam
Date May 2, 2013, 2:04 p.m.
Message ID <1367503462-24742-3-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/241009/
State Superseded
Headers show

Comments

Fabio Estevam - May 2, 2013, 2:04 p.m.
On mx23 and mx28 the pullup bits are defined as:

- 0: Enable the internal pad keeper 
- 1: Disable the internal pad keeper

Fix the definitions as they are currently the opposite.

Cc: Lauri Hintsala <lauri.hintsala@bluegiga.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
This affects mx28 as well, so also adding Lauri.

I also tested on mx28evk and behavior is fine after this change.

Changes since v1:
- Newly introduced

 arch/arm/include/asm/arch-mxs/iomux.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Michael Heimpold - May 2, 2013, 6:44 p.m.
Hi Fabio,

> On mx23 and mx28 the pullup bits are defined as:
> 
> - 0: Enable the internal pad keeper 
> - 1: Disable the internal pad keeper
> 
> Fix the definitions as they are currently the opposite.

Sorry, but I think this is not correct. Please have a look
at the reference manual for i.MX28 page 786:
It seems that some pins have an internal pullup, but other
pins "only" have the internal keeper you mentioned.

Quote from :
bank 0 pin 24: "Set this bit to one to _enable_ the internal pullup..."
bank 0 pin 25: "Set this bit to one to _disable_ the internal keeper..."

So I think the current implementation is correct.

However, if you are trying to enable a pullup for a pin which does not
have this function (e.g. because the hardware guys trimmed the BOM
and did not spend an external pullup) you are actually disabling the
keeper which makes the situation even worse in some situations.

I'm still wondering what an ideal solution could be...

Best regards,
Michael
Fabio Estevam - May 2, 2013, 7:35 p.m.
Hi Michael,

On Thu, May 2, 2013 at 3:44 PM, Michael Heimpold <mhei@heimpold.de> wrote:

> Sorry, but I think this is not correct. Please have a look
> at the reference manual for i.MX28 page 786:
> It seems that some pins have an internal pullup, but other
> pins "only" have the internal keeper you mentioned.
>
> Quote from :
> bank 0 pin 24: "Set this bit to one to _enable_ the internal pullup..."
> bank 0 pin 25: "Set this bit to one to _disable_ the internal keeper..."
>
> So I think the current implementation is correct.

Ok, I see.

mx23 does not have the "Set this bit to one to _enable_ the internal
pullup" option.

>
> However, if you are trying to enable a pullup for a pin which does not
> have this function (e.g. because the hardware guys trimmed the BOM
> and did not spend an external pullup) you are actually disabling the
> keeper which makes the situation even worse in some situations.
>
> I'm still wondering what an ideal solution could be...

So I plan to keep mx28 bits untouched and do the reverse definition
only for mx23:

#if defined CONFIG_MX28
#define PAD_PULLUP		1
#define PAD_NOPULL		0
#else
#define PAD_PULLUP		0
#define PAD_NOPULL		1
#endif
Marek Vasut - May 2, 2013, 7:44 p.m.
Dear Fabio Estevam,

> Hi Michael,
> 
> On Thu, May 2, 2013 at 3:44 PM, Michael Heimpold <mhei@heimpold.de> wrote:
> > Sorry, but I think this is not correct. Please have a look
> > at the reference manual for i.MX28 page 786:
> > It seems that some pins have an internal pullup, but other
> > pins "only" have the internal keeper you mentioned.
> > 
> > Quote from :
> > bank 0 pin 24: "Set this bit to one to _enable_ the internal pullup..."
> > bank 0 pin 25: "Set this bit to one to _disable_ the internal keeper..."
> > 
> > So I think the current implementation is correct.
> 
> Ok, I see.
> 
> mx23 does not have the "Set this bit to one to _enable_ the internal
> pullup" option.
> 
> > However, if you are trying to enable a pullup for a pin which does not
> > have this function (e.g. because the hardware guys trimmed the BOM
> > and did not spend an external pullup) you are actually disabling the
> > keeper which makes the situation even worse in some situations.
> > 
> > I'm still wondering what an ideal solution could be...
> 
> So I plan to keep mx28 bits untouched and do the reverse definition
> only for mx23:
> 
> #if defined CONFIG_MX28
> #define PAD_PULLUP		1
> #define PAD_NOPULL		0
> #else
> #define PAD_PULLUP		0
> #define PAD_NOPULL		1
> #endif

Wait, this code is pulled from Linux kernel. How does Linux solve that? Is it 
also broken in Linux?

Best regards,
Marek Vasut
Michael Heimpold - May 2, 2013, 9:45 p.m.
Hi Fabio,

> mx23 does not have the "Set this bit to one to _enable_ the internal
> pullup" option.

Hm, in the RM for i.MX233, page 1480 and above
(http://cache.freescale.com/files/dsp/doc/ref_manual/IMX23RM.pdf)
I can only find definitions like
"Set this bit to one to _disable_ the internal pad keeper and _enable_
the internal pull up resistor..." or for EMI pads at bank 3
"Set this bit to one to _disable_ the internal keeper"
(without a note about pull ups).

Could you please link me to your reference where the reverse
definition comes from?

Best regards,
Michael
Fabio Estevam - May 2, 2013, 10:34 p.m.
Hi Michael,

On Thu, May 2, 2013 at 6:45 PM, Michael Heimpold <mhei@heimpold.de> wrote:

> Could you please link me to your reference where the reverse
> definition comes from?

Sorry for the confusion, current code is correct for both mx23 and mx28.

I discarded this patch and will send v3 shortly.

Thanks,

Fabio Estevam

Patch

diff --git a/arch/arm/include/asm/arch-mxs/iomux.h b/arch/arm/include/asm/arch-mxs/iomux.h
index 66dcd36..70300d5 100644
--- a/arch/arm/include/asm/arch-mxs/iomux.h
+++ b/arch/arm/include/asm/arch-mxs/iomux.h
@@ -78,8 +78,8 @@  typedef u32 iomux_cfg_t;
 #define PAD_3V3			0
 #endif
 
-#define PAD_NOPULL		0
-#define PAD_PULLUP		1
+#define PAD_PULLUP		0
+#define PAD_NOPULL		1
 
 #define MXS_PAD_4MA	((PAD_4MA << MXS_PAD_MA_SHIFT) | \
 					MXS_PAD_MA_VALID_MASK)