RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

["A.S. Dong" <aisheng.dong@xxxxxxx>]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@xxxxxxxx]
> Sent: Thursday, May 18, 2017 2:16 AM
> To: A.S. Dong
> Cc: linux-gpio@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linus.walleij@xxxxxxxxxx; shawnguo@xxxxxxxxxx; Jacky Bai; Andy Duan;
> kernel@xxxxxxxxxxxxxx; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-17 00:18, A.S. Dong wrote:
> >> -----Original Message-----
> >> From: Stefan Agner [mailto:stefan@xxxxxxxx]
> >> Sent: Tuesday, May 16, 2017 1:27 AM
> >> To: A.S. Dong
> >> Cc: linux-gpio@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> linus.walleij@xxxxxxxxxx; shawnguo@xxxxxxxxxx; Jacky Bai; Andy Duan;
> >> kernel@xxxxxxxxxxxxxx; Alexandre Courbot
> >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> >> gpio
> >>
> >> On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > Do not assume MUX 0 is GPIO function in core driver as a different
> >> > SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >> >
> >> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >> > Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> >> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> >> > Cc: Stefan Agner <stefan@xxxxxxxx>
> >> > Cc: Fugang Duan <fugang.duan@xxxxxxx>
> >> > Cc: Bai Ping <ping.bai@xxxxxxx>
> >> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> >> > ---
> >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > index 0d6aaca..ed8ea32 100644
> >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  			continue;
> >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> >> > +			if (imx_pin->pin == offset)
> >> >  				goto mux_pin;
> >>
> >> The reason I added that check was to make sure we pick a mux option
> >> which is GPIO... With this change, any pinmux might be picked...
> >>
> >
> > First of all, this seems to be wrong to me that GPIO mux mode is SoC
> > Dependant and should not be put in pinctrl-imx core driver.
> 
> Hm, agree, we should consider to move
> imx_pmx_gpio_request_enable/disable_free and imx_pmx_gpio_set_direction
> into pinctrl-vf610.c
> 

IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
dynamically change GPIO from output to input.

> >
> > Secondly, I think we may be over worried and it's not quite necessary
> > As we did not do the sanity check for both raw config and mux data
> > read From Device tree, why only do it for GPIO?
> >
> > We may trust the data in device tree.
> 
> In Vybrid, there is no need to explicitly assign a pinmux to use a pin as
> GPIO. So the pinmux could be anything... The implemented semantics for
> Vyrbid is really different than i.MX, see below.
> 

Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
e.g.
arch/arm/boot/dts/vf-colibri.dtsi
pinctrl_esdhc1: esdhc1grp {
        fsl,pins = <
                VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
                VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
                VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
                VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
                VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
                VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
                VF610_PAD_PTB20__GPIO_42        0x219d
        >;
};  

> >
> >> >  		}
> >> >  	}
> >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> >  	reg &= ~info->mux_mask;
> >> >  	reg |= imx_pin->config;
> >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> >>
> >> ... and muxed...
> >>
> >> Not sure if we want that.
> >>
> >> I had to control GPIO output/input through pinctrl since Vybrid does
> >> not allow to control that from the GPIO block.
> >>
> >> However, according to your GPIO patchset, the i.MX 7ULP has a new
> >> register GPIO_PDDR to control output from the GPIO block. Is
> >> controlling the output driver from IOMUXC still required?
> >
> > Yes, it's still required.
> >
> 
> That sounds weird, what is the GPIO_PDDR for then? Sure I  need to enable
> the output driver to drive the pin, but can I disable output just using
> GPIO_PDDR?

No, to fully disable a output, you must disable OBE as well.

> 
> Maybe we have a miss understanding here:
> 
> Lets assume we want to switch a GPIO between output and input:
> 
> echo "output" > /sys/class/gpio/gpioN/direction ..
> echo "input" > /sys/class/gpio/gpioN/direction
> 
> Do I need to disable the output driver in the IOMUXC or can we just
> disable GPIO_PDDR and use the pin as input?
> 

OBE should also be disabled. Otherwise the input may not function well.

> If we can disable the output driver just using GPIO_PDDR, we can avoid the
> gpio_set_direction cross call.
> 
> 
> >> If not, I really would just not use all that "find pinctrl config"
> >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
> >> that only for Vybrid.
> >>
> >> This would also align much better with the other i.MX devices, where
> >> pinmuxing and GPIO is completely orthogonal.
> >>
> >
> > Actually this patch came only because to make the exist code not break
> > MX7ULP.
> >
> > Actually I'm wondering why we need implement
> > imx_pmx_gpio_request_enable function?
> >
> > Per my understanding, IMX binding already set GPIO mux by Parsing MUX
> > mode from device tree, so why need gpio_request_enable which looks
> > like is duplicated.
> >
> > Can you help explain it?
> 
> I suggest to read this clarification email wrt to pinctrl/gpio from Linus
> Walleij:
> https://lkml.org/lkml/2016/10/10/87
> 
> To summarize: We have different semantics in Vybrid: The GPIO subsystem
> automatically mux the GPIO for you. So in Vybrid, you do not have to mux a
> GPIO (but a valid entry in your device tree is needed, but does not
> assigned to any node).

Okay, Clearer now.

But I do see the users of GPIO pads in Vybrid dts.
Above is an example which make me confuse at first. 

> 
> Is what the driver is doing for Vybrid wrong? It is different from i.MX,
> but afaik, it is not really wrong... Its a valid implementation according
> to the currently defined semantics...  Due to the *need* to touch pinctrl
> for direction, I had to implement cross calls anyway, so I thought I might
> as well go full mile and also mux the GPIO on request...
> 

It's not strickly wrong.
Just a bit confuse that gpio_request_enable seems not quite necessary
As we actually already and must define GPIO mux in device tree according
To standard IMX binding format.
e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
That means pinctrl already does the GPIO mux when enable sd function.

> So the question is, what semantic do we want for i.MX 7ULP? Since it is a
> i.MX device, we probably want the same semantics as i.MX 6/7 is already
> using for the sake of consistency. So in this case the
> gpio_request_enable/disable callbacks are not needed...
> 
> This is how I hope we can do the implementation for i.MX 7ULP:
> - Do not use gpio_request_enable/disable

Yes, we do want that.

> - Do not use gpio_set_direction either

Not, ULP needs it to support GPIO direction switch.

> - Users using a GPIO should enable output driver in IOMUXC (just use a pin
> configuration where output driver is enabled)

Users still need configure OBE/IBE in devicetree for statically assignment.

> - The GPIO driver only enables/disables the output driver using its
> GPIO_PDDR register depending on GPIO direction

No, same reason as the second question.


So, finnaly, I think the solution may be:
1. If Vybrid does not use gpio_request_enable/disable, we can simply
remove it. Both driver keeps using pinctrl gpio_set_direction.

Or.

2. Make gpio_request_enable/disable and gpio_set_direction
As pinctrl-imx core driver callbacks. And only assign gpio_set_direction
For IMX7ULP platform driver while assign both for Vybrid.

Which one would you prefer?

Regards
Dong Aisheng

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html