[U-Boot] [PATCH] mx6: Add IOMUX_CONFIG_SION flag to all GPIO pins

[Benoît Thébaudeau]

Hi Otavio,

On Tuesday, October 1, 2013 10:01:57 PM, Otavio Salvador wrote:
> Subject: Re: [U-Boot] [PATCH] mx6: Add IOMUX_CONFIG_SION flag to all GPIO pins
> 
> On Tue, Oct 1, 2013 at 4:50 PM, Benoît Thébaudeau
> <benoit.thebaudeau at advansee.com> wrote:
> > Hi Eric,
> >
> > On Tuesday, October 1, 2013 6:17:06 PM, Eric Nelson wrote:
> >> Hi Stefano,
> >>
> >> On 10/01/2013 09:10 AM, Stefano Babic wrote:
> >> > Hi Eric,
> >> >
> >> > On 01/10/2013 17:56, Eric Nelson wrote:
> >> >> Hi Stefano,
> >> >>
> >> >> On 10/01/2013 07:49 AM, Stefano Babic wrote:
> >> >>> Hi Eric,
> >> >>>
> >> >>> On 01/10/2013 16:26, Eric Nelson wrote:
> >> >>>
> >> >>>> I'm not sure where you're seeing this in the RM, but in
> >> >>>> order to read the pad state when not configured as an input,
> >> >>>> the SION bit needs to be set in the pad mux register on i.MX51/53.
> >> >>>
> >> >>> I have checked inside the "37.3.2.2 GPIO Write" for i.MX53 and "
> >> >>> 35.4.2.1 Read Value from Pad" for i:MX51, I have not read anything
> >> >>> about
> >> >>> SION. If someone has found where it is described, please mail !
> >> >>>
> >> >>
> >> >> That documentation seems to imply that there's no dependency
> >> >> (i.e. there's no reference to SION), but I think that's an omission.
> >> >
> >> > Ok - this is surely not the first time we find errors in the
> >> > documentation.
> >> >
> >> >> I've tested this many times, since it's a really handy way of
> >> >> debugging hardware setups.
> >> >>
> >> >> That said, I'm not sure that there's a huge difference between
> >> >> a single patch or multiple patches for each arch unless there's
> >> >> some functionality dependent on being able to read the actual
> >> >> value of a pin configured as a GPIO output.
> >> >
> >> > Ok - then I tend to apply Otavio's patch, and we will fix for the other
> >> > i.MXes if we really find the same issues.
> >> >
> >> >>
> >> >> Did I miss something in this thread that does actually require
> >> >> that ability? It seems a pretty obscure thing in the normal case
> >> >> to drive an output without confidence that it will succeed.
> >> >
> >> > Yes, it seems quite strange, but it helps to debug the hardware. It is
> >> > not the first time we see that, even driving the output, the signal does
> >> > not go to the expected value, due for example to a conflict (another
> >> > peripheral driving the signal) or to a wrong pull up resistor. As U-Boot
> >> > is a great tool for hardware debugging, reading the signal back let
> >> > check that the output is set to the desired value.
> >> >
> >>
> >> I agree with all of that, though this only covers the case of a
> >> pin set up as a GPIO output, and that same debugging approach
> >> is often used for other functions (display data pins, clock inputs
> >> and outputs, et cetera).
> >>
> >> You probably wouldn't just set SION on all pins, right? I suspect
> >> that there'd be some ramification in terms of power consumption if
> >> nothing else.
> >
> > Right. Well, instead of adding SION to the pin definition header files,
> > then we
> > could just add SION where needed on a per-pin basis, e.g.:
> > ---
> > imx_iomux_v3_setup_pad(MX6_PAD_NANDF_D1__GPIO_2_1 |
> >                         IOMUX_CONFIG_SION << MUX_MODE_SHIFT);
> > ---
> >
> > A helper macro could be defined in
> > arch/arm/include/asm/imx-common/iomux-v3.h in
> > order to simplify the writing, e.g.:
> > ---
> > #define MUX_MODE_SION   (IOMUX_CONFIG_SION << MUX_MODE_SHIFT)
> > ---
> > imx_iomux_v3_setup_pad(MX6_PAD_NANDF_D1__GPIO_2_1 | MUX_MODE_SION);
> > ---
> >
> > On the Linux side of things, the CONFIG field in the pinctrl DT bindings
> > already
> > provides bit 30 for SION. Hence, using MUX_MODE_SION like above would be
> > close
> > to Linux's pin config.
> 
> Thus dropping my patch, right?

Yes, if everybody agrees. You could replace it with a patch adding MUX_MODE_SION
to iomux-v3.h, and another patch adding MUX_MODE_SION to the configuration of
your status LED pin mux on your board (if it is in mainline).

> Or do you think for GPIO we ought to have
> them?

SION is not GPIO-specific, so no. Eric is right: SION may slightly increase the
current consumption if it is set for too many pins, so it does not make sense to
set it everywhere, and it may be needed for any pin, not just for GPIOs, so
setting it either for all GPIOs or for all pins would not make sense. SION
should be set only where strictly required. Since SION is a hardware
configuration option, let's keep it a software configuration as well, just like
in Linux.

Best regards,
Benoît