From mboxrd@z Thu Jan 1 00:00:00 1970 From: "A.S. Dong" Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio Date: Tue, 23 May 2017 10:23:23 +0000 Message-ID: References: <1494830906-6442-1-git-send-email-aisheng.dong@nxp.com> <1494830906-6442-3-git-send-email-aisheng.dong@nxp.com> <6d47abc0527ae6f2214a9b37dfbeb154@agner.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-he1eur01on0047.outbound.protection.outlook.com ([104.47.0.47]:60078 "EHLO EUR01-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S967389AbdEWKX2 (ORCPT ); Tue, 23 May 2017 06:23:28 -0400 Content-Language: en-US Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: "A.S. Dong" , Stefan Agner Cc: "linux-gpio@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linus.walleij@linaro.org" , "shawnguo@kernel.org" , Jacky Bai , Andy Duan , "kernel@pengutronix.de" , Alexandre Courbot Hi Stefan, > -----Original Message----- > From: A.S. Dong > Sent: Thursday, May 18, 2017 3:00 PM > To: 'Stefan Agner' > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan; > kernel@pengutronix.de; Alexandre Courbot > Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpi= o >=20 > > -----Original Message----- > > From: Stefan Agner [mailto:stefan@agner.ch] > > Sent: Thursday, May 18, 2017 2:16 AM > > To: A.S. Dong > > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan; > > kernel@pengutronix.de; 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@agner.ch] > > >> Sent: Tuesday, May 16, 2017 1:27 AM > > >> To: A.S. Dong > > >> Cc: linux-gpio@vger.kernel.org; > > >> linux-arm-kernel@lists.infradead.org; > > >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy > > >> Duan; kernel@pengutronix.de; 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 > > >> > Cc: Alexandre Courbot > > >> > Cc: Shawn Guo > > >> > Cc: Stefan Agner > > >> > Cc: Fugang Duan > > >> > Cc: Bai Ping > > >> > Signed-off-by: Dong Aisheng > > >> > --- > > >> > 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 =3D 0; pin < grp->num_pins; pin++) { > > >> > imx_pin =3D &((struct imx_pin *)(grp->data))[pin]; > > >> > - if (imx_pin->pin =3D=3D offset && !imx_pin->mux_mode) > > >> > + if (imx_pin->pin =3D=3D 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 > > >=20 > IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support > dynamically change GPIO from output to input. >=20 > > > > > > 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. > > >=20 > 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 =3D < > 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 > >; > }; >=20 > > > > > >> > } > > >> > } > > >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct > > >> > pinctrl_dev *pctldev, > > >> > reg =3D readl(ipctl->base + pin_reg->mux_reg); > > >> > reg &=3D ~info->mux_mask; > > >> > reg |=3D imx_pin->config; > > >> > + reg |=3D 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? >=20 > No, to fully disable a output, you must disable OBE as well. >=20 > > > > 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? > > >=20 > OBE should also be disabled. Otherwise the input may not function well. >=20 > > 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). >=20 > Okay, Clearer now. >=20 > But I do see the users of GPIO pads in Vybrid dts. > Above is an example which make me confuse at first. >=20 > > > > 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... > > >=20 > 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. >=20 > > 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 >=20 > Yes, we do want that. >=20 > > - Do not use gpio_set_direction either >=20 > Not, ULP needs it to support GPIO direction switch. >=20 > > - Users using a GPIO should enable output driver in IOMUXC (just use a > > pin configuration where output driver is enabled) >=20 > Users still need configure OBE/IBE in devicetree for statically assignmen= t. >=20 > > - The GPIO driver only enables/disables the output driver using its > > GPIO_PDDR register depending on GPIO direction >=20 > No, same reason as the second question. >=20 >=20 > 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. >=20 > Or. >=20 > 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. >=20 > Which one would you prefer? >=20 Any answer about it? Regards Dong Aisheng From mboxrd@z Thu Jan 1 00:00:00 1970 From: aisheng.dong@nxp.com (A.S. Dong) Date: Tue, 23 May 2017 10:23:23 +0000 Subject: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio References: <1494830906-6442-1-git-send-email-aisheng.dong@nxp.com> <1494830906-6442-3-git-send-email-aisheng.dong@nxp.com> <6d47abc0527ae6f2214a9b37dfbeb154@agner.ch> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Stefan, > -----Original Message----- > From: A.S. Dong > Sent: Thursday, May 18, 2017 3:00 PM > To: 'Stefan Agner' > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan; > kernel at pengutronix.de; Alexandre Courbot > Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio > > > -----Original Message----- > > From: Stefan Agner [mailto:stefan at agner.ch] > > Sent: Thursday, May 18, 2017 2:16 AM > > To: A.S. Dong > > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; > > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan; > > kernel at pengutronix.de; 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 at agner.ch] > > >> Sent: Tuesday, May 16, 2017 1:27 AM > > >> To: A.S. Dong > > >> Cc: linux-gpio at vger.kernel.org; > > >> linux-arm-kernel at lists.infradead.org; > > >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy > > >> Duan; kernel at pengutronix.de; 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 > > >> > Cc: Alexandre Courbot > > >> > Cc: Shawn Guo > > >> > Cc: Stefan Agner > > >> > Cc: Fugang Duan > > >> > Cc: Bai Ping > > >> > Signed-off-by: Dong Aisheng > > >> > --- > > >> > 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? > Any answer about it? Regards Dong Aisheng