linux-next regression caused by "gpiolib: request the gpio before querying its direction"

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

Commit "gpiolib: request the gpio before querying its
direction" (108d23e322a247d9f89ba2e2742520ead0944cc9) is causing a
regression on Marvell platforms, and potentially other platforms as
well. With this commit applied, we lose the serial console. Reverting
this commit makes the serial console functional again.

On the Marvell Armada 8K, the UART pins are pin-muxed, with the choice
of GPIO or UART as functionality. Currently, the Armada 8K Device Tree
do not have the pin-mux information for the UART, because the pinctrl
driver didn't exist when we first added support for the platform, so we
continue to rely on the bootloader having configured the UART muxing.

Therefore, with Timur's commit applied, when the system boots, we get
serial output, up to the point where gpiochip_add_data() is called, and
requests all GPIOs. Since our UART pins are not requested at the
pinctrl level, the gpio_request succeeds and re-muxes those pins as
GPIOs: we lose the UART.

So, this is a clear regression, as with the current Device Tree for the
Armada 8K platforms, linux-next has the serial console broken.

If we add the relevant pinctrl muxing details in the DT, things are
working again of course. *But* it is still not good, because there is a
window of time during which we don't get serial port messages: between
the moment the pins are muxed as GPIOs by gpiochip_add_data(), and the
moment the UART driver probes, requests the pins, and they get remuxed
as UART. With earlycon enabled, it means we loose kernel messages.

I believe the UART pins are quite critical, and therefore they
shouldn't be remuxed as GPIOs automatically that early at boot time.

What do you suggest to fix this problem ?

Some more details about the platform:

 - DT is arch/arm64/boot/dts/marvell/armada-8040-db.dts

 - Affected GPIO/pinctrl is described in
   arch/arm64/boot/dts/marvell/armada-ap806.dtsi, nodes ap_pinctrl and
   ap_gpio.

 - The drivers/gpio/gpio-mvebu.c probe() function calls
   devm_gpiochip_add_data(), which calls gpiochip_add_data(), which
   iterates through all pins and requests them as GPIOs.

 - The UART pins are described in the pinctrl driver
   drivers/pinctrl/mvebu/pinctrl-armada-ap806.c. The most problematic
   one is pin 11, which is UART0 TXD, and gets remuxed as GPIO, causing
   the bug.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

Commit "gpiolib: request the gpio before querying its
direction" (108d23e322a247d9f89ba2e2742520ead0944cc9) is causing a
regression on Marvell platforms, and potentially other platforms as
well. With this commit applied, we lose the serial console. Reverting
this commit makes the serial console functional again.

On the Marvell Armada 8K, the UART pins are pin-muxed, with the choice
of GPIO or UART as functionality. Currently, the Armada 8K Device Tree
do not have the pin-mux information for the UART, because the pinctrl
driver didn't exist when we first added support for the platform, so we
continue to rely on the bootloader having configured the UART muxing.

Therefore, with Timur's commit applied, when the system boots, we get
serial output, up to the point where gpiochip_add_data() is called, and
requests all GPIOs. Since our UART pins are not requested at the
pinctrl level, the gpio_request succeeds and re-muxes those pins as
GPIOs: we lose the UART.

So, this is a clear regression, as with the current Device Tree for the
Armada 8K platforms, linux-next has the serial console broken.

If we add the relevant pinctrl muxing details in the DT, things are
working again of course. *But* it is still not good, because there is a
window of time during which we don't get serial port messages: between
the moment the pins are muxed as GPIOs by gpiochip_add_data(), and the
moment the UART driver probes, requests the pins, and they get remuxed
as UART. With earlycon enabled, it means we loose kernel messages.

I believe the UART pins are quite critical, and therefore they
shouldn't be remuxed as GPIOs automatically that early at boot time.

What do you suggest to fix this problem ?

Some more details about the platform:

 - DT is arch/arm64/boot/dts/marvell/armada-8040-db.dts

 - Affected GPIO/pinctrl is described in
   arch/arm64/boot/dts/marvell/armada-ap806.dtsi, nodes ap_pinctrl and
   ap_gpio.

 - The drivers/gpio/gpio-mvebu.c probe() function calls
   devm_gpiochip_add_data(), which calls gpiochip_add_data(), which
   iterates through all pins and requests them as GPIOs.

 - The UART pins are described in the pinctrl driver
   drivers/pinctrl/mvebu/pinctrl-armada-ap806.c. The most problematic
   one is pin 11, which is UART0 TXD, and gets remuxed as GPIO, causing
   the bug.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 8/30/17 4:24 AM, Thomas Petazzoni wrote:
> Therefore, with Timur's commit applied, when the system boots, we get
> serial output, up to the point where gpiochip_add_data() is called, and
> requests all GPIOs. Since our UART pins are not requested at the
> pinctrl level, the gpio_request succeeds and re-muxes those pins as
> GPIOs: we lose the UART.

This part I don't understand.  My patch just only impacts the code that 
queries the direction of the GPIO.  It does not set the direction.

When gpiochip_add_data() calls chip->request, what function is that calling?

The only thing I can think of is that the ->request function is not just 
returning status, but is also muxing the GPIO.  If so, then I think 
that's a bug.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 8/30/17 4:24 AM, Thomas Petazzoni wrote:
> Therefore, with Timur's commit applied, when the system boots, we get
> serial output, up to the point where gpiochip_add_data() is called, and
> requests all GPIOs. Since our UART pins are not requested at the
> pinctrl level, the gpio_request succeeds and re-muxes those pins as
> GPIOs: we lose the UART.

This part I don't understand.  My patch just only impacts the code that 
queries the direction of the GPIO.  It does not set the direction.

When gpiochip_add_data() calls chip->request, what function is that calling?

The only thing I can think of is that the ->request function is not just 
returning status, but is also muxing the GPIO.  If so, then I think 
that's a bug.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Gregory CLEMENT]

Hi Timur,
 
 On mer., août 30 2017, Timur Tabi <timur@codeaurora.org> wrote:

> On 8/30/17 4:24 AM, Thomas Petazzoni wrote:
>> Therefore, with Timur's commit applied, when the system boots, we get
>> serial output, up to the point where gpiochip_add_data() is called, and
>> requests all GPIOs. Since our UART pins are not requested at the
>> pinctrl level, the gpio_request succeeds and re-muxes those pins as
>> GPIOs: we lose the UART.
>
> This part I don't understand.  My patch just only impacts the code
> that queries the direction of the GPIO.  It does not set the
> direction.
>
> When gpiochip_add_data() calls chip->request, what function is that
> calling?

the request callback is gpiochip_generic_request so I would be surprised
that there was a bug in it.

>
> The only thing I can think of is that the ->request function is not
> just returning status, but is also muxing the GPIO.  If so, then I
> think that's a bug.

Gregory

>
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, hosted by The Linux Foundation.
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Gregory CLEMENT]

Hi Timur,
 
 On mer., ao?t 30 2017, Timur Tabi <timur@codeaurora.org> wrote:

> On 8/30/17 4:24 AM, Thomas Petazzoni wrote:
>> Therefore, with Timur's commit applied, when the system boots, we get
>> serial output, up to the point where gpiochip_add_data() is called, and
>> requests all GPIOs. Since our UART pins are not requested at the
>> pinctrl level, the gpio_request succeeds and re-muxes those pins as
>> GPIOs: we lose the UART.
>
> This part I don't understand.  My patch just only impacts the code
> that queries the direction of the GPIO.  It does not set the
> direction.
>
> When gpiochip_add_data() calls chip->request, what function is that
> calling?

the request callback is gpiochip_generic_request so I would be surprised
that there was a bug in it.

>
> The only thing I can think of is that the ->request function is not
> just returning status, but is also muxing the GPIO.  If so, then I
> think that's a bug.

Gregory

>
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, hosted by The Linux Foundation.
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Wed, 30 Aug 2017 15:59:00 +0200, Gregory CLEMENT wrote:

> > When gpiochip_add_data() calls chip->request, what function is that
> > calling?  
> 
> the request callback is gpiochip_generic_request so I would be surprised
> that there was a bug in it.

The call chain leading to the problem is:

 gpiochip_add_data()
  chip->request() == gpiochip_generic_request()
   pinctrl_request_gpio()
    pinmux_request_gpio()
     pin_request()
      ops->gpio_request_enable() == mvebu_pinmux_gpio_request_enable()
       mvebu_pinconf_group_set()
        grp->ctrl->mpp_set() == mvebu_regmap_mpp_ctrl_set()

So what Timur is saying perhaps is that
mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().

However, even the "reference" pinctrl-single.c implementation does it,
in pcs_request_gpio().

Am I missing something ?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Wed, 30 Aug 2017 15:59:00 +0200, Gregory CLEMENT wrote:

> > When gpiochip_add_data() calls chip->request, what function is that
> > calling?  
> 
> the request callback is gpiochip_generic_request so I would be surprised
> that there was a bug in it.

The call chain leading to the problem is:

 gpiochip_add_data()
  chip->request() == gpiochip_generic_request()
   pinctrl_request_gpio()
    pinmux_request_gpio()
     pin_request()
      ops->gpio_request_enable() == mvebu_pinmux_gpio_request_enable()
       mvebu_pinconf_group_set()
        grp->ctrl->mpp_set() == mvebu_regmap_mpp_ctrl_set()

So what Timur is saying perhaps is that
mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().

However, even the "reference" pinctrl-single.c implementation does it,
in pcs_request_gpio().

Am I missing something ?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 8/30/17 9:17 AM, Thomas Petazzoni wrote:
> So what Timur is saying perhaps is that
> mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
> muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().
> 
> However, even the "reference" pinctrl-single.c implementation does it,
> in pcs_request_gpio().
> 
> Am I missing something ?

No, that's it.  The question is, what exactly should the 'request' 
function do?  Should it be modifying the hardware to satisfy the 
request?  When I wrote my patch, I assumed that it wouldn't.  I thought 
that request simply answered the question, "can I touch this GPIO"?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 8/30/17 9:17 AM, Thomas Petazzoni wrote:
> So what Timur is saying perhaps is that
> mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
> muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().
> 
> However, even the "reference" pinctrl-single.c implementation does it,
> in pcs_request_gpio().
> 
> Am I missing something ?

No, that's it.  The question is, what exactly should the 'request' 
function do?  Should it be modifying the hardware to satisfy the 
request?  When I wrote my patch, I assumed that it wouldn't.  I thought 
that request simply answered the question, "can I touch this GPIO"?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:

> No, that's it.  The question is, what exactly should the 'request' 
> function do?  Should it be modifying the hardware to satisfy the 
> request?  When I wrote my patch, I assumed that it wouldn't.  I thought 
> that request simply answered the question, "can I touch this GPIO"?

No, it also muxes the pin in GPIO, and you can see the "reference"
implementation pinctrl-single also does it.

Let's see what Linus W. has to say about the semantic of the "request"
operation.

But if we change the semantic of "request" to no longer mux the hardware
as GPIO, then you will also have regressions, because there are plenty
of GPIOs that are requested, but not explicitly muxed as GPIOs in the
DT, precisely because today requesting a GPIO is sufficient to have it
re-muxed as GPIO at the pinctrl level.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:

> No, that's it.  The question is, what exactly should the 'request' 
> function do?  Should it be modifying the hardware to satisfy the 
> request?  When I wrote my patch, I assumed that it wouldn't.  I thought 
> that request simply answered the question, "can I touch this GPIO"?

No, it also muxes the pin in GPIO, and you can see the "reference"
implementation pinctrl-single also does it.

Let's see what Linus W. has to say about the semantic of the "request"
operation.

But if we change the semantic of "request" to no longer mux the hardware
as GPIO, then you will also have regressions, because there are plenty
of GPIOs that are requested, but not explicitly muxed as GPIOs in the
DT, precisely because today requesting a GPIO is sufficient to have it
re-muxed as GPIO at the pinctrl level.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[jmondi]

Hello,

On Wed, Aug 30, 2017 at 04:32:55PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:
>
> > No, that's it.  The question is, what exactly should the 'request'
> > function do?  Should it be modifying the hardware to satisfy the
> > request?  When I wrote my patch, I assumed that it wouldn't.  I thought
> > that request simply answered the question, "can I touch this GPIO"?
>
> No, it also muxes the pin in GPIO, and you can see the "reference"
> implementation pinctrl-single also does it.
>
> Let's see what Linus W. has to say about the semantic of the "request"
> operation.
>
> But if we change the semantic of "request" to no longer mux the hardware
> as GPIO, then you will also have regressions, because there are plenty
> of GPIOs that are requested, but not explicitly muxed as GPIOs in the
> DT, precisely because today requesting a GPIO is sufficient to have it
> re-muxed as GPIO at the pinctrl level.

Just to point out one of Renesas' pin controller devices seems to
suffer from the same problem, introduced by Timur's commit

https://www.spinics.net/lists/linux-renesas-soc/msg17647.html

This is indeed caused by the "request" introduced by the above said
commit, that in rza1 pincontroller, actually muxes the requested
pin as GPIO.

Reverting that commit solves all the issues in our case too.

Thanks
   j
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[jmondi]

Hello,

On Wed, Aug 30, 2017 at 04:32:55PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:
>
> > No, that's it.  The question is, what exactly should the 'request'
> > function do?  Should it be modifying the hardware to satisfy the
> > request?  When I wrote my patch, I assumed that it wouldn't.  I thought
> > that request simply answered the question, "can I touch this GPIO"?
>
> No, it also muxes the pin in GPIO, and you can see the "reference"
> implementation pinctrl-single also does it.
>
> Let's see what Linus W. has to say about the semantic of the "request"
> operation.
>
> But if we change the semantic of "request" to no longer mux the hardware
> as GPIO, then you will also have regressions, because there are plenty
> of GPIOs that are requested, but not explicitly muxed as GPIOs in the
> DT, precisely because today requesting a GPIO is sufficient to have it
> re-muxed as GPIO at the pinctrl level.

Just to point out one of Renesas' pin controller devices seems to
suffer from the same problem, introduced by Timur's commit

https://www.spinics.net/lists/linux-renesas-soc/msg17647.html

This is indeed caused by the "request" introduced by the above said
commit, that in rza1 pincontroller, actually muxes the requested
pin as GPIO.

Reverting that commit solves all the issues in our case too.

Thanks
   j
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

On Wed, Aug 30, 2017 at 6:24 PM, jmondi <jacopo@jmondi.org> wrote:
> On Wed, Aug 30, 2017 at 04:32:55PM +0200, Thomas Petazzoni wrote:
>> On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:
>> > No, that's it.  The question is, what exactly should the 'request'
>> > function do?  Should it be modifying the hardware to satisfy the
>> > request?  When I wrote my patch, I assumed that it wouldn't.  I thought
>> > that request simply answered the question, "can I touch this GPIO"?
>>
>> No, it also muxes the pin in GPIO, and you can see the "reference"
>> implementation pinctrl-single also does it.
>>
>> Let's see what Linus W. has to say about the semantic of the "request"
>> operation.
>>
>> But if we change the semantic of "request" to no longer mux the hardware
>> as GPIO, then you will also have regressions, because there are plenty
>> of GPIOs that are requested, but not explicitly muxed as GPIOs in the
>> DT, precisely because today requesting a GPIO is sufficient to have it
>> re-muxed as GPIO at the pinctrl level.
>
> Just to point out one of Renesas' pin controller devices seems to
> suffer from the same problem, introduced by Timur's commit
>
> https://www.spinics.net/lists/linux-renesas-soc/msg17647.html
>
> This is indeed caused by the "request" introduced by the above said
> commit, that in rza1 pincontroller, actually muxes the requested
> pin as GPIO.

To clarify: which causes havoc if that pin is used as e.g. an SDRAM
address line.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

On Wed, Aug 30, 2017 at 6:24 PM, jmondi <jacopo@jmondi.org> wrote:
> On Wed, Aug 30, 2017 at 04:32:55PM +0200, Thomas Petazzoni wrote:
>> On Wed, 30 Aug 2017 09:22:55 -0500, Timur Tabi wrote:
>> > No, that's it.  The question is, what exactly should the 'request'
>> > function do?  Should it be modifying the hardware to satisfy the
>> > request?  When I wrote my patch, I assumed that it wouldn't.  I thought
>> > that request simply answered the question, "can I touch this GPIO"?
>>
>> No, it also muxes the pin in GPIO, and you can see the "reference"
>> implementation pinctrl-single also does it.
>>
>> Let's see what Linus W. has to say about the semantic of the "request"
>> operation.
>>
>> But if we change the semantic of "request" to no longer mux the hardware
>> as GPIO, then you will also have regressions, because there are plenty
>> of GPIOs that are requested, but not explicitly muxed as GPIOs in the
>> DT, precisely because today requesting a GPIO is sufficient to have it
>> re-muxed as GPIO at the pinctrl level.
>
> Just to point out one of Renesas' pin controller devices seems to
> suffer from the same problem, introduced by Timur's commit
>
> https://www.spinics.net/lists/linux-renesas-soc/msg17647.html
>
> This is indeed caused by the "request" introduced by the above said
> commit, that in rza1 pincontroller, actually muxes the requested
> pin as GPIO.

To clarify: which causes havoc if that pin is used as e.g. an SDRAM
address line.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Linus Walleij]

On Wed, Aug 30, 2017 at 4:17 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> The call chain leading to the problem is:
>
>  gpiochip_add_data()
>   chip->request() == gpiochip_generic_request()
>    pinctrl_request_gpio()
>     pinmux_request_gpio()
>      pin_request()
>       ops->gpio_request_enable() == mvebu_pinmux_gpio_request_enable()
>        mvebu_pinconf_group_set()
>         grp->ctrl->mpp_set() == mvebu_regmap_mpp_ctrl_set()
>
> So what Timur is saying perhaps is that
> mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
> muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().
>
> However, even the "reference" pinctrl-single.c implementation does it,
> in pcs_request_gpio().

Yeah so we have unclear semantics on this and that is just a fact of
life. It's a bit of pain as maintainer because I sometimes don't know
what to do when something makes superficial sense and the only thing
I can do is to toss it into linux-next and see what happens.

Look what happened :D

If the semantics should be changed, all drivers must be changed consistently
in a larger patch series, so until then, we revert this and leave it as it is.

Now this is reverted anyways.

Yours,
Linus Walleij

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Linus Walleij]

On Wed, Aug 30, 2017 at 4:17 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> The call chain leading to the problem is:
>
>  gpiochip_add_data()
>   chip->request() == gpiochip_generic_request()
>    pinctrl_request_gpio()
>     pinmux_request_gpio()
>      pin_request()
>       ops->gpio_request_enable() == mvebu_pinmux_gpio_request_enable()
>        mvebu_pinconf_group_set()
>         grp->ctrl->mpp_set() == mvebu_regmap_mpp_ctrl_set()
>
> So what Timur is saying perhaps is that
> mvebu_pinmux_gpio_request_enable() shouldn't be changing the type of
> muxing, and therefore shouldn't be calling mvebu_pinconf_group_set().
>
> However, even the "reference" pinctrl-single.c implementation does it,
> in pcs_request_gpio().

Yeah so we have unclear semantics on this and that is just a fact of
life. It's a bit of pain as maintainer because I sometimes don't know
what to do when something makes superficial sense and the only thing
I can do is to toss it into linux-next and see what happens.

Look what happened :D

If the semantics should be changed, all drivers must be changed consistently
in a larger patch series, so until then, we revert this and leave it as it is.

Now this is reverted anyways.

Yours,
Linus Walleij

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Thu, 31 Aug 2017 09:08:45 +0200, Linus Walleij wrote:

> > However, even the "reference" pinctrl-single.c implementation does it,
> > in pcs_request_gpio().  
> 
> Yeah so we have unclear semantics on this and that is just a fact of
> life. It's a bit of pain as maintainer because I sometimes don't know
> what to do when something makes superficial sense and the only thing
> I can do is to toss it into linux-next and see what happens.
> 
> Look what happened :D
> 
> If the semantics should be changed, all drivers must be changed consistently
> in a larger patch series, so until then, we revert this and leave it as it is.
> 
> Now this is reverted anyways.

Thanks for taking action on this. Regarding the semantics, the
kerneldoc comment says:

 * @gpio_request_enable: requests and enables GPIO on a certain pin.
 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset(pin number) into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide.
 * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
 *      @gpio_request_enable

So the ->gpio_request_enable() comment is not super clear, but the
->gpio_disable_free() explicitly says "free up GPIO muxing", which
would mean the ->gpio_request_enable() hook has muxed the pin as GPIO.
Things could be clearer, but I believe it's quite clear the intent is
that the ->gpio_request_enable() should mux the pin as a GPIO at the HW
level.

Note that on my side, I've however not been convinced by this semantic:
I find it weird that when you request a GPIO, it gets automatically
muxed as such, without an explicit pinctrl configuration in the DT.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Thu, 31 Aug 2017 09:08:45 +0200, Linus Walleij wrote:

> > However, even the "reference" pinctrl-single.c implementation does it,
> > in pcs_request_gpio().  
> 
> Yeah so we have unclear semantics on this and that is just a fact of
> life. It's a bit of pain as maintainer because I sometimes don't know
> what to do when something makes superficial sense and the only thing
> I can do is to toss it into linux-next and see what happens.
> 
> Look what happened :D
> 
> If the semantics should be changed, all drivers must be changed consistently
> in a larger patch series, so until then, we revert this and leave it as it is.
> 
> Now this is reverted anyways.

Thanks for taking action on this. Regarding the semantics, the
kerneldoc comment says:

 * @gpio_request_enable: requests and enables GPIO on a certain pin.
 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset(pin number) into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide.
 * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
 *      @gpio_request_enable

So the ->gpio_request_enable() comment is not super clear, but the
->gpio_disable_free() explicitly says "free up GPIO muxing", which
would mean the ->gpio_request_enable() hook has muxed the pin as GPIO.
Things could be clearer, but I believe it's quite clear the intent is
that the ->gpio_request_enable() should mux the pin as a GPIO at the HW
level.

Note that on my side, I've however not been convinced by this semantic:
I find it weird that when you request a GPIO, it gets automatically
muxed as such, without an explicit pinctrl configuration in the DT.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Aug 31, 2017 at 9:18 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Thu, 31 Aug 2017 09:08:45 +0200, Linus Walleij wrote:
>> > However, even the "reference" pinctrl-single.c implementation does it,
>> > in pcs_request_gpio().
>>
>> Yeah so we have unclear semantics on this and that is just a fact of
>> life. It's a bit of pain as maintainer because I sometimes don't know
>> what to do when something makes superficial sense and the only thing
>> I can do is to toss it into linux-next and see what happens.
>>
>> Look what happened :D
>>
>> If the semantics should be changed, all drivers must be changed consistently
>> in a larger patch series, so until then, we revert this and leave it as it is.
>>
>> Now this is reverted anyways.
>
> Thanks for taking action on this. Regarding the semantics, the
> kerneldoc comment says:
>
>  * @gpio_request_enable: requests and enables GPIO on a certain pin.
>  *      Implement this only if you can mux every pin individually as GPIO. The
>  *      affected GPIO range is passed along with an offset(pin number) into that
>  *      specific GPIO range - function selectors and pin groups are orthogonal
>  *      to this, the core will however make sure the pins do not collide.
>  * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
>  *      @gpio_request_enable
>
> So the ->gpio_request_enable() comment is not super clear, but the
> ->gpio_disable_free() explicitly says "free up GPIO muxing", which
> would mean the ->gpio_request_enable() hook has muxed the pin as GPIO.
> Things could be clearer, but I believe it's quite clear the intent is
> that the ->gpio_request_enable() should mux the pin as a GPIO at the HW
> level.

It does say "if you can mux ... as GPIO".
And ->gpio_disable_free() is "the reverse of ->gpio_request_enable()".

> Note that on my side, I've however not been convinced by this semantic:
> I find it weird that when you request a GPIO, it gets automatically
> muxed as such, without an explicit pinctrl configuration in the DT.

On lots of hardware, you first have to select between GPIO and "other
function".
For "other function", you have to select the actual function later.
In that case, switching to a GPIO can be done by flipping a single bit.

Your hardware may vary, though.

In addition, you can claim GPIOs from anywhere, including from userspace.
While in-kernel drivers could be forced to specify a pinctrl config, that's not
so easy to require from userspace.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Aug 31, 2017 at 9:18 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> On Thu, 31 Aug 2017 09:08:45 +0200, Linus Walleij wrote:
>> > However, even the "reference" pinctrl-single.c implementation does it,
>> > in pcs_request_gpio().
>>
>> Yeah so we have unclear semantics on this and that is just a fact of
>> life. It's a bit of pain as maintainer because I sometimes don't know
>> what to do when something makes superficial sense and the only thing
>> I can do is to toss it into linux-next and see what happens.
>>
>> Look what happened :D
>>
>> If the semantics should be changed, all drivers must be changed consistently
>> in a larger patch series, so until then, we revert this and leave it as it is.
>>
>> Now this is reverted anyways.
>
> Thanks for taking action on this. Regarding the semantics, the
> kerneldoc comment says:
>
>  * @gpio_request_enable: requests and enables GPIO on a certain pin.
>  *      Implement this only if you can mux every pin individually as GPIO. The
>  *      affected GPIO range is passed along with an offset(pin number) into that
>  *      specific GPIO range - function selectors and pin groups are orthogonal
>  *      to this, the core will however make sure the pins do not collide.
>  * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
>  *      @gpio_request_enable
>
> So the ->gpio_request_enable() comment is not super clear, but the
> ->gpio_disable_free() explicitly says "free up GPIO muxing", which
> would mean the ->gpio_request_enable() hook has muxed the pin as GPIO.
> Things could be clearer, but I believe it's quite clear the intent is
> that the ->gpio_request_enable() should mux the pin as a GPIO at the HW
> level.

It does say "if you can mux ... as GPIO".
And ->gpio_disable_free() is "the reverse of ->gpio_request_enable()".

> Note that on my side, I've however not been convinced by this semantic:
> I find it weird that when you request a GPIO, it gets automatically
> muxed as such, without an explicit pinctrl configuration in the DT.

On lots of hardware, you first have to select between GPIO and "other
function".
For "other function", you have to select the actual function later.
In that case, switching to a GPIO can be done by flipping a single bit.

Your hardware may vary, though.

In addition, you can claim GPIOs from anywhere, including from userspace.
While in-kernel drivers could be forced to specify a pinctrl config, that's not
so easy to require from userspace.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Russell King - ARM Linux]

On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
> > Note that on my side, I've however not been convinced by this semantic:
> > I find it weird that when you request a GPIO, it gets automatically
> > muxed as such, without an explicit pinctrl configuration in the DT.
> 
> On lots of hardware, you first have to select between GPIO and "other
> function".
> For "other function", you have to select the actual function later.
> In that case, switching to a GPIO can be done by flipping a single bit.

What about hardware which you can configure for some alternate function
but still monitor the pin via GPIO, even though it's not mux'd as GPIO.

For instance, you may have a timer block which can capture on both
edges of an external event signal, which needs the pin to be muxed for
that function.  However, you need to read the state of the pin, and
that is only available through GPIO.  Muxing the pin to be a GPIO just
because someone requests the GPIO is, imho, ill thought-out and breaks
some use cases.

IMHO, the pinmux settings should always be specified in DT, and that's
what we should be using everywhere, not doing broken backdoor games like
"the gpio is being requested, it's obvious that we want this pin to be
a gpio" - that really doesn't follow.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Russell King - ARM Linux]

On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
> > Note that on my side, I've however not been convinced by this semantic:
> > I find it weird that when you request a GPIO, it gets automatically
> > muxed as such, without an explicit pinctrl configuration in the DT.
> 
> On lots of hardware, you first have to select between GPIO and "other
> function".
> For "other function", you have to select the actual function later.
> In that case, switching to a GPIO can be done by flipping a single bit.

What about hardware which you can configure for some alternate function
but still monitor the pin via GPIO, even though it's not mux'd as GPIO.

For instance, you may have a timer block which can capture on both
edges of an external event signal, which needs the pin to be muxed for
that function.  However, you need to read the state of the pin, and
that is only available through GPIO.  Muxing the pin to be a GPIO just
because someone requests the GPIO is, imho, ill thought-out and breaks
some use cases.

IMHO, the pinmux settings should always be specified in DT, and that's
what we should be using everywhere, not doing broken backdoor games like
"the gpio is being requested, it's obvious that we want this pin to be
a gpio" - that really doesn't follow.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Thu, 31 Aug 2017 10:22:12 +0100, Russell King - ARM Linux wrote:

> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
> 
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.
> 
> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

Agreed.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Thomas Petazzoni]

Hello,

On Thu, 31 Aug 2017 10:22:12 +0100, Russell King - ARM Linux wrote:

> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
> 
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.
> 
> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

Agreed.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 08/31/2017 04:39 AM, Thomas Petazzoni wrote:
>>
>> IMHO, the pinmux settings should always be specified in DT, and that's
>> what we should be using everywhere, not doing broken backdoor games like
>> "the gpio is being requested, it's obvious that we want this pin to be
>> a gpio" - that really doesn't follow.
> Agreed.

How many drivers need to be "fixed" if this is the plan?  I don't think 
we can make a blanket change to all drivers whose ->request functions 
touch the muxes without testing on all those platforms.  And it's not 
just the muxes.  The whole point behind my patch was to avoid 
gpiochip_add_data() touching the hardware without claiming the GPIO first.

The overall goal of my patch was to allow "sparse" GPIO maps.  The 
problem with gpiochip_add_data() is that it touches all GPIOs even 
before I call gpiochip_add_pin_range().

Maybe the for-loop that it's in gpiochip_add_data() should be moved to 
gpiochip_add_pin_range(), so that we only query the direction of a pin 
after it's been added, not before?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 08/31/2017 04:39 AM, Thomas Petazzoni wrote:
>>
>> IMHO, the pinmux settings should always be specified in DT, and that's
>> what we should be using everywhere, not doing broken backdoor games like
>> "the gpio is being requested, it's obvious that we want this pin to be
>> a gpio" - that really doesn't follow.
> Agreed.

How many drivers need to be "fixed" if this is the plan?  I don't think 
we can make a blanket change to all drivers whose ->request functions 
touch the muxes without testing on all those platforms.  And it's not 
just the muxes.  The whole point behind my patch was to avoid 
gpiochip_add_data() touching the hardware without claiming the GPIO first.

The overall goal of my patch was to allow "sparse" GPIO maps.  The 
problem with gpiochip_add_data() is that it touches all GPIOs even 
before I call gpiochip_add_pin_range().

Maybe the for-loop that it's in gpiochip_add_data() should be moved to 
gpiochip_add_pin_range(), so that we only query the direction of a pin 
after it's been added, not before?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

Hi Russell,

On Thu, Aug 31, 2017 at 11:22 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
>> > Note that on my side, I've however not been convinced by this semantic:
>> > I find it weird that when you request a GPIO, it gets automatically
>> > muxed as such, without an explicit pinctrl configuration in the DT.
>>
>> On lots of hardware, you first have to select between GPIO and "other
>> function".
>> For "other function", you have to select the actual function later.
>> In that case, switching to a GPIO can be done by flipping a single bit.
>
> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
>
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.

Yes, reading from the GPIO can work if the pin is muxed to another function.

> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

Do we need to specify pinmux setting for pins used by e.g. the SDRAM
controller, which doesn't have a Linux driver?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Geert Uytterhoeven]

Hi Russell,

On Thu, Aug 31, 2017 at 11:22 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
>> > Note that on my side, I've however not been convinced by this semantic:
>> > I find it weird that when you request a GPIO, it gets automatically
>> > muxed as such, without an explicit pinctrl configuration in the DT.
>>
>> On lots of hardware, you first have to select between GPIO and "other
>> function".
>> For "other function", you have to select the actual function later.
>> In that case, switching to a GPIO can be done by flipping a single bit.
>
> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
>
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.

Yes, reading from the GPIO can work if the pin is muxed to another function.

> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

Do we need to specify pinmux setting for pins used by e.g. the SDRAM
controller, which doesn't have a Linux driver?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 08/31/2017 04:50 AM, Geert Uytterhoeven wrote:
>> For instance, you may have a timer block which can capture on both
>> edges of an external event signal, which needs the pin to be muxed for
>> that function.  However, you need to read the state of the pin, and
>> that is only available through GPIO.  Muxing the pin to be a GPIO just
>> because someone requests the GPIO is, imho, ill thought-out and breaks
>> some use cases.

> Yes, reading from the GPIO can work if the pin is muxed to another function.

Well that depends on the hardware.  On Qualcomm chips, the you can 
technically still read and write from/to the GPIO if it's muxed to some 
other function, but the results are meaningless.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Timur Tabi]

On 08/31/2017 04:50 AM, Geert Uytterhoeven wrote:
>> For instance, you may have a timer block which can capture on both
>> edges of an external event signal, which needs the pin to be muxed for
>> that function.  However, you need to read the state of the pin, and
>> that is only available through GPIO.  Muxing the pin to be a GPIO just
>> because someone requests the GPIO is, imho, ill thought-out and breaks
>> some use cases.

> Yes, reading from the GPIO can work if the pin is muxed to another function.

Well that depends on the hardware.  On Qualcomm chips, the you can 
technically still read and write from/to the GPIO if it's muxed to some 
other function, but the results are meaningless.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On Thu, Aug 31, 2017 at 10:22:12AM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
> > > Note that on my side, I've however not been convinced by this semantic:
> > > I find it weird that when you request a GPIO, it gets automatically
> > > muxed as such, without an explicit pinctrl configuration in the DT.
> > 
> > On lots of hardware, you first have to select between GPIO and "other
> > function".
> > For "other function", you have to select the actual function later.
> > In that case, switching to a GPIO can be done by flipping a single bit.
> 
> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
> 
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.
> 
> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

This doesn't really work anymore if you throw into the mix hardware
controllers that don't have muxing options for "a GPIO", but different
muxing options for input and output. For pins that could change
direction, you cannot just specify it in the DT anymore.

And that's leaving out the user-space interface issue.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Maxime Ripard]

On Thu, Aug 31, 2017 at 10:22:12AM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 31, 2017 at 09:30:54AM +0200, Geert Uytterhoeven wrote:
> > > Note that on my side, I've however not been convinced by this semantic:
> > > I find it weird that when you request a GPIO, it gets automatically
> > > muxed as such, without an explicit pinctrl configuration in the DT.
> > 
> > On lots of hardware, you first have to select between GPIO and "other
> > function".
> > For "other function", you have to select the actual function later.
> > In that case, switching to a GPIO can be done by flipping a single bit.
> 
> What about hardware which you can configure for some alternate function
> but still monitor the pin via GPIO, even though it's not mux'd as GPIO.
> 
> For instance, you may have a timer block which can capture on both
> edges of an external event signal, which needs the pin to be muxed for
> that function.  However, you need to read the state of the pin, and
> that is only available through GPIO.  Muxing the pin to be a GPIO just
> because someone requests the GPIO is, imho, ill thought-out and breaks
> some use cases.
> 
> IMHO, the pinmux settings should always be specified in DT, and that's
> what we should be using everywhere, not doing broken backdoor games like
> "the gpio is being requested, it's obvious that we want this pin to be
> a gpio" - that really doesn't follow.

This doesn't really work anymore if you throw into the mix hardware
controllers that don't have muxing options for "a GPIO", but different
muxing options for input and output. For pins that could change
direction, you cannot just specify it in the DT anymore.

And that's leaving out the user-space interface issue.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170831/6f3bfef8/attachment.sig>

Re: linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Linus Walleij]

On Wed, Aug 30, 2017 at 11:24 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> Commit "gpiolib: request the gpio before querying its
> direction" (108d23e322a247d9f89ba2e2742520ead0944cc9) is causing a
> regression on Marvell platforms, and potentially other platforms as
> well. With this commit applied, we lose the serial console. Reverting
> this commit makes the serial console functional again.

I have reverted this patch for now.

Yours,
Linus Walleij

linux-next regression caused by "gpiolib: request the gpio before querying its direction"

[Linus Walleij]

On Wed, Aug 30, 2017 at 11:24 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> Commit "gpiolib: request the gpio before querying its
> direction" (108d23e322a247d9f89ba2e2742520ead0944cc9) is causing a
> regression on Marvell platforms, and potentially other platforms as
> well. With this commit applied, we lose the serial console. Reverting
> this commit makes the serial console functional again.

I have reverted this patch for now.

Yours,
Linus Walleij