gpio-omap: add support gpiolib bias (pull-up/down) flags?

gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

Hello Grygorii, Santosh and Kevin,

You are listed as gpio-omap maintainers so I wanted to reach out and
get your feedback on this.

The gpiolib userspace API added support for bias flags
(pull-up/pull-down) in Linux 5.5:
[GIT PULL] GPIO changes for v5.5 [1]

The merged code is from the patch series:
[PATCH v6 0/7] gpio: expose line bias flags to userspace [2]

The gpiochip character device now supports theses flags [3]:
GPIOHANDLE_REQUEST_BIAS_PULL_UP
GPIOHANDLE_REQUEST_BIAS_PULL_DOWN
GPIOHANDLE_REQUEST_BIAS_DISABLE

The pinctrl-bcm2835 driver used on Raspberry Pi already supports the
pull-up/down bias flags [4].  pinctrl-bcm2835 is also a gpio driver
[5].  libgpiod v1.5 supports these bias flags [6], so the command line
gpioset utility can set the bias flags for a line on the Raspberry Pi
[7].

I would like the BeagleBone, which has the TI AM3358 SoC, to be able
to use the bias flags as well.   The AM3358 uses the gpio-omap GPIO
driver.  However, gpio-omap does not support these flags [8].

Do you have any feedback on whether this is possible to implement?

If so, do you have any guidance about the correct way for me to add
support for those bias flags in gpio-omap.c?

Thank you,
Drew

[1] https://www.spinics.net/lists/linux-gpio/msg43719.html
[2] https://lore.kernel.org/linux-gpio/CACRpkdbJxcfj6pK=1qjXxffFn0RUH9VD0HRFXX0RoZJDi=hfRw@mail.gmail.com/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c#n1061
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/bcm/pinctrl-bcm2835.c#n958
[5] https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt
[6] https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/commit/?h=v1.5.x&id=26d8d8f0b7bc0e300aaab05c75d5af1b0686af08
[7] https://microhobby.com.br/blog/2020/02/02/new-linux-kernel-5-5-new-interfaces-in-gpiolib/
[8] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-omap.c

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Linus Walleij]

Hi Drew,

On Sun, Mar 8, 2020 at 2:07 PM Drew Fustini <drew@pdp7.com> wrote:

> I would like the BeagleBone, which has the TI AM3358 SoC, to be able
> to use the bias flags as well.   The AM3358 uses the gpio-omap GPIO
> driver.  However, gpio-omap does not support these flags [8].
>
> Do you have any feedback on whether this is possible to implement?

Do we have a datasheet for this GPIO block somewhere? Should
be the datasheet for the ASIC.

We already have the required .set_config() callback on the OMAP
driver, it's just that it only uses it for debounce.

The driver is a bit convoluted with register offsets in a struct
omap_gpio_reg_offs depending on variant, but if they have
a register for this I'd say just get hacking.

If the GPIO driver is using pin control as back-end you are
looking at something more complex similar to what Intel is
doing inside drivers/pinctrl/intel/pinctrl-intel.c: this driver
is just calling up to gpiochip_generic_config() which will
try to configure the lines behind the GPIO using pin config,
which works if the proper ranges are defined so the
framework can map a GPIO line to a pin control pin.

Yours,
Linus Walleij

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Thu, Mar 12, 2020 at 1:43 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> Do we have a datasheet for this GPIO block somewhere? Should
> be the datasheet for the ASIC.

I am looking at the AM335x reference manual [0] but I can not actually
find any references to pull-up/down or bias for GPIO pins.  I guess I
was making of the mistake of assuming this would be something the gpio
pins support.

> We already have the required .set_config() callback on the OMAP
> driver, it's just that it only uses it for debounce.
>
> The driver is a bit convoluted with register offsets in a struct
> omap_gpio_reg_offs depending on variant, but if they have
> a register for this I'd say just get hacking.
>
> If the GPIO driver is using pin control as back-end you are
> looking at something more complex similar to what Intel is
> doing inside drivers/pinctrl/intel/pinctrl-intel.c: this driver
> is just calling up to gpiochip_generic_config() which will
> try to configure the lines behind the GPIO using pin config,
> which works if the proper ranges are defined so the
> framework can map a GPIO line to a pin control pin.

Thank you for the feedback, Linus.

Upon further review of drivers/pinctrl/pinctrl-single.c, I am not
certain it actually supports pull-up/down.

I see there is pcs_pinconf_clear_bias() and pcs_pinconf_bias_disable()
but I don't see a place where the PIN_CONFIG_BIAS_PULL_DOWN or
PIN_CONFIG_BIAS_PULL_UP get set.

I'll have to look at that some more before I go back to thinking about
how to integrate into gpio-omap.

Thanks,
Drew

[0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Haojian Zhuang]

On Fri, 13 Mar 2020 at 08:38, Drew Fustini <pdp7pdp7@gmail.com> wrote:
>
> On Thu, Mar 12, 2020 at 1:43 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > Do we have a datasheet for this GPIO block somewhere? Should
> > be the datasheet for the ASIC.
>
> I am looking at the AM335x reference manual [0] but I can not actually
> find any references to pull-up/down or bias for GPIO pins.  I guess I
> was making of the mistake of assuming this would be something the gpio
> pins support.
>
> > We already have the required .set_config() callback on the OMAP
> > driver, it's just that it only uses it for debounce.
> >
> > The driver is a bit convoluted with register offsets in a struct
> > omap_gpio_reg_offs depending on variant, but if they have
> > a register for this I'd say just get hacking.
> >
> > If the GPIO driver is using pin control as back-end you are
> > looking at something more complex similar to what Intel is
> > doing inside drivers/pinctrl/intel/pinctrl-intel.c: this driver
> > is just calling up to gpiochip_generic_config() which will
> > try to configure the lines behind the GPIO using pin config,
> > which works if the proper ranges are defined so the
> > framework can map a GPIO line to a pin control pin.
>
> Thank you for the feedback, Linus.
>
> Upon further review of drivers/pinctrl/pinctrl-single.c, I am not
> certain it actually supports pull-up/down.
>
> I see there is pcs_pinconf_clear_bias() and pcs_pinconf_bias_disable()
> but I don't see a place where the PIN_CONFIG_BIAS_PULL_DOWN or
> PIN_CONFIG_BIAS_PULL_UP get set.
>

                        /* 4 parameters */
                        case PIN_CONFIG_BIAS_DISABLE:
                                pcs_pinconf_clear_bias(pctldev, pin);
                                break;
                        case PIN_CONFIG_BIAS_PULL_DOWN:
                        case PIN_CONFIG_BIAS_PULL_UP:
                                if (arg)
                                        pcs_pinconf_clear_bias(pctldev, pin);
                                /* fall through */
                        case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
                                data &= ~func->conf[i].mask;
                                if (arg)
                                        data |= func->conf[i].enable;
                                else
                                        data |= func->conf[i].disable;
                                break;

Because it does fall through, pullup/pulldown is set in the snippet of
"PIN_CONFIG_INPUT_SCHMITT_ENABLE".

Best Regards
Haojian

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Fri, Mar 13, 2020 at 01:23:15PM +0800, Haojian Zhuang wrote:
> On Fri, 13 Mar 2020 at 08:38, Drew Fustini <pdp7pdp7@gmail.com> wrote:
> >
> > On Thu, Mar 12, 2020 at 1:43 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > Do we have a datasheet for this GPIO block somewhere? Should
> > > be the datasheet for the ASIC.
> >
> > I am looking at the AM335x reference manual [0] but I can not actually
> > find any references to pull-up/down or bias for GPIO pins.  I guess I
> > was making of the mistake of assuming this would be something the gpio
> > pins support.
> >
> > > We already have the required .set_config() callback on the OMAP
> > > driver, it's just that it only uses it for debounce.
> > >
> > > The driver is a bit convoluted with register offsets in a struct
> > > omap_gpio_reg_offs depending on variant, but if they have
> > > a register for this I'd say just get hacking.
> > >
> > > If the GPIO driver is using pin control as back-end you are
> > > looking at something more complex similar to what Intel is
> > > doing inside drivers/pinctrl/intel/pinctrl-intel.c: this driver
> > > is just calling up to gpiochip_generic_config() which will
> > > try to configure the lines behind the GPIO using pin config,
> > > which works if the proper ranges are defined so the
> > > framework can map a GPIO line to a pin control pin.
> >
> > Thank you for the feedback, Linus.
> >
> > Upon further review of drivers/pinctrl/pinctrl-single.c, I am not
> > certain it actually supports pull-up/down.
> >
> > I see there is pcs_pinconf_clear_bias() and pcs_pinconf_bias_disable()
> > but I don't see a place where the PIN_CONFIG_BIAS_PULL_DOWN or
> > PIN_CONFIG_BIAS_PULL_UP get set.
> >
> 
>                         /* 4 parameters */
>                         case PIN_CONFIG_BIAS_DISABLE:
>                                 pcs_pinconf_clear_bias(pctldev, pin);
>                                 break;
>                         case PIN_CONFIG_BIAS_PULL_DOWN:
>                         case PIN_CONFIG_BIAS_PULL_UP:
>                                 if (arg)
>                                         pcs_pinconf_clear_bias(pctldev, pin);
>                                 /* fall through */
>                         case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
>                                 data &= ~func->conf[i].mask;
>                                 if (arg)
>                                         data |= func->conf[i].enable;
>                                 else
>                                         data |= func->conf[i].disable;
>                                 break;
> 
> Because it does fall through, pullup/pulldown is set in the snippet of
> "PIN_CONFIG_INPUT_SCHMITT_ENABLE".
> 
> Best Regards
> Haojian

Thank you for the insights, Haojian and Linus.

I've added debug print statements and it seems that pcs_pinconf_set()
is never called on the BeagleBone (TI AM3358) either during boot or
when gpiomon runs with bias switch that invokes GPIO_GET_LINEEVENT_IOCTL
with GPIOHANDLE_REQUEST_BIAS_PULL_UP flag.

The pinctrl-single driver and gpio-omap driver bind as a result of these
device tree nodes in arch/arm/boot/dts/am33xx-l4.dtsi:

    am33xx_pinmux: pinmux@800 {
        compatible = "pinctrl-single";
        reg = <0x800 0x238>;
        #pinctrl-cells = <1>;
        pinctrl-single,register-width = <32>;
        pinctrl-single,function-mask = <0x7f>;
    };

    gpio0: gpio@0 {
        compatible = "ti,omap4-gpio";
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        reg = <0x0 0x1000>;
        interrupts = <96>;
        gpio-line-names =
        "MDIO_DATA",    // 0
        <snip>

I see in Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

    "pinctrl-single" means that pinconf isn't supported.

I believe this is why pcs_pinconf_set() never gets called.

Any suggestions as to how I could proceed?

Is it reasonable to change the compatible to "pinconf-single"?


Thank You,
Drew

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Grygorii Strashko]

On 13/04/2020 15:39, Drew Fustini wrote:
> On Fri, Mar 13, 2020 at 01:23:15PM +0800, Haojian Zhuang wrote:
>> On Fri, 13 Mar 2020 at 08:38, Drew Fustini <pdp7pdp7@gmail.com> wrote:
>>>
>>> On Thu, Mar 12, 2020 at 1:43 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> Do we have a datasheet for this GPIO block somewhere? Should
>>>> be the datasheet for the ASIC.
>>>
>>> I am looking at the AM335x reference manual [0] but I can not actually
>>> find any references to pull-up/down or bias for GPIO pins.  I guess I
>>> was making of the mistake of assuming this would be something the gpio
>>> pins support.
>>>
>>>> We already have the required .set_config() callback on the OMAP
>>>> driver, it's just that it only uses it for debounce.
>>>>
>>>> The driver is a bit convoluted with register offsets in a struct
>>>> omap_gpio_reg_offs depending on variant, but if they have
>>>> a register for this I'd say just get hacking.
>>>>
>>>> If the GPIO driver is using pin control as back-end you are
>>>> looking at something more complex similar to what Intel is
>>>> doing inside drivers/pinctrl/intel/pinctrl-intel.c: this driver
>>>> is just calling up to gpiochip_generic_config() which will
>>>> try to configure the lines behind the GPIO using pin config,
>>>> which works if the proper ranges are defined so the
>>>> framework can map a GPIO line to a pin control pin.
>>>
>>> Thank you for the feedback, Linus.
>>>
>>> Upon further review of drivers/pinctrl/pinctrl-single.c, I am not
>>> certain it actually supports pull-up/down.
>>>
>>> I see there is pcs_pinconf_clear_bias() and pcs_pinconf_bias_disable()
>>> but I don't see a place where the PIN_CONFIG_BIAS_PULL_DOWN or
>>> PIN_CONFIG_BIAS_PULL_UP get set.
>>>
>>
>>                          /* 4 parameters */
>>                          case PIN_CONFIG_BIAS_DISABLE:
>>                                  pcs_pinconf_clear_bias(pctldev, pin);
>>                                  break;
>>                          case PIN_CONFIG_BIAS_PULL_DOWN:
>>                          case PIN_CONFIG_BIAS_PULL_UP:
>>                                  if (arg)
>>                                          pcs_pinconf_clear_bias(pctldev, pin);
>>                                  /* fall through */
>>                          case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
>>                                  data &= ~func->conf[i].mask;
>>                                  if (arg)
>>                                          data |= func->conf[i].enable;
>>                                  else
>>                                          data |= func->conf[i].disable;
>>                                  break;
>>
>> Because it does fall through, pullup/pulldown is set in the snippet of
>> "PIN_CONFIG_INPUT_SCHMITT_ENABLE".
>>
>> Best Regards
>> Haojian
> 
> Thank you for the insights, Haojian and Linus.
> 
> I've added debug print statements and it seems that pcs_pinconf_set()
> is never called on the BeagleBone (TI AM3358) either during boot or
> when gpiomon runs with bias switch that invokes GPIO_GET_LINEEVENT_IOCTL
> with GPIOHANDLE_REQUEST_BIAS_PULL_UP flag.
> 
> The pinctrl-single driver and gpio-omap driver bind as a result of these
> device tree nodes in arch/arm/boot/dts/am33xx-l4.dtsi:
> 
>      am33xx_pinmux: pinmux@800 {
>          compatible = "pinctrl-single";
>          reg = <0x800 0x238>;
>          #pinctrl-cells = <1>;
>          pinctrl-single,register-width = <32>;
>          pinctrl-single,function-mask = <0x7f>;
>      };
> 
>      gpio0: gpio@0 {
>          compatible = "ti,omap4-gpio";
>          gpio-controller;
>          #gpio-cells = <2>;
>          interrupt-controller;
>          #interrupt-cells = <2>;
>          reg = <0x0 0x1000>;
>          interrupts = <96>;
>          gpio-line-names =
>          "MDIO_DATA",    // 0
>          <snip>
> 
> I see in Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> 
>      "pinctrl-single" means that pinconf isn't supported.
> 
> I believe this is why pcs_pinconf_set() never gets called.
> 
> Any suggestions as to how I could proceed?
> 
> Is it reasonable to change the compatible to "pinconf-single"?

For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
does not provide such functions as pullup/pulldown.

Before use the pin has to be configured like;
&am33xx_pinmux {

	user_leds_s0: user_leds_s0 {
		pinctrl-single,pins = <
			AM33XX_PADCONF(AM335X_PIN_GPMC_A5, PIN_OUTPUT_PULLDOWN, MUX_MODE7)	/* gpmc_a5.gpio1_21 */
			AM33XX_PADCONF(AM335X_PIN_GPMC_A6, PIN_OUTPUT_PULLUP, MUX_MODE7)	/* gpmc_a6.gpio1_22 */
			AM33XX_PADCONF(AM335X_PIN_GPMC_A7, PIN_OUTPUT_PULLDOWN, MUX_MODE7)	/* gpmc_a7.gpio1_23 */
			AM33XX_PADCONF(AM335X_PIN_GPMC_A8, PIN_OUTPUT_PULLUP, MUX_MODE7)	/* gpmc_a8.gpio1_24 */
		>;
	};

	mmc1_pins: pinmux_mmc1_pins {
		pinctrl-single,pins = <
			AM33XX_PADCONF(AM335X_PIN_SPI0_CS1, PIN_INPUT, MUX_MODE7)		/* spio0_cs1.gpio0_6 */


-- 
Best regards,
grygorii

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Robert Nelson]

Hi Grygorii,

On Wed, Apr 15, 2020 at 8:15 AM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
> For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
> does not provide such functions as pullup/pulldown.

Correct, that's the state today, while Drew is investing time into
trying to figure out how to properly extend this feature into our
platform.

Regards,

-- 
Robert Nelson
https://rcn-ee.com/

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Grygorii Strashko]

On 15/04/2020 16:20, Robert Nelson wrote:
> Hi Grygorii,
> 
> On Wed, Apr 15, 2020 at 8:15 AM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>>
>> For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
>> does not provide such functions as pullup/pulldown.
> 
> Correct, that's the state today, while Drew is investing time into
> trying to figure out how to properly extend this feature into our
> platform.

Sry, but it's not clear what's the final target (at least from public part of this thread).


-- 
Best regards,
grygorii

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Robert Nelson]

On Wed, Apr 15, 2020 at 8:47 AM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
>
>
> On 15/04/2020 16:20, Robert Nelson wrote:
> > Hi Grygorii,
> >
> > On Wed, Apr 15, 2020 at 8:15 AM Grygorii Strashko
> > <grygorii.strashko@ti.com> wrote:
> >>
> >> For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
> >> does not provide such functions as pullup/pulldown.
> >
> > Correct, that's the state today, while Drew is investing time into
> > trying to figure out how to properly extend this feature into our
> > platform.
>
> Sry, but it's not clear what's the final target (at least from public part of this thread).

We are mainly targeting am335x based devices.  Today (well last few
years) we've utilized a "hack-ish" kernel module (bone-pinmux-helper)
to allow users to overide/change the pinmux-ing directly from
user-space...  (This evil module allows us to specify a list of
options for each pin, thus users can easily configure specifies of the
pin, aka gpio_pd/gpio_pu/etc from user-space...).  Since that time,
mainline has now grown a generic gpio pull-up/pull-down functionality,
with the ability to re-control these values directly from a generic
gpio library (libgpiod).

Regards,

-- 
Robert Nelson
https://rcn-ee.com/

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Wed, Apr 15, 2020 at 08:59:09AM -0500, Robert Nelson wrote:
> On Wed, Apr 15, 2020 at 8:47 AM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> > On 15/04/2020 16:20, Robert Nelson wrote:
> > > Hi Grygorii,
> > >
> > > On Wed, Apr 15, 2020 at 8:15 AM Grygorii Strashko
> > > <grygorii.strashko@ti.com> wrote:
> > >>
> > >> For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
> > >> does not provide such functions as pullup/pulldown.
> > >
> > > Correct, that's the state today, while Drew is investing time into
> > > trying to figure out how to properly extend this feature into our
> > > platform.
> >
> > Sry, but it's not clear what's the final target (at least from public part of this thread).
> 
> We are mainly targeting am335x based devices.  Today (well last few
> years) we've utilized a "hack-ish" kernel module (bone-pinmux-helper)
> to allow users to overide/change the pinmux-ing directly from
> user-space...  (This evil module allows us to specify a list of
> options for each pin, thus users can easily configure specifies of the
> pin, aka gpio_pd/gpio_pu/etc from user-space...).  Since that time,
> mainline has now grown a generic gpio pull-up/pull-down functionality,
> with the ability to re-control these values directly from a generic
> gpio library (libgpiod).

Hello Grygorii -

As Robert described, I wanted to make us of the new support for bias
flags in the gpiolib uapi which allows userspace libraries like libgpiod
set pull-up or pull-down on lines [0].

Is there no way for gpio-omap to call into the pinctrl-single backend to
set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?

Thank you,
Drew

[0] https://lore.kernel.org/linux-gpio/20191105020429.18942-1-warthog618@gmail.com/

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Linus Walleij]

On Thu, Apr 16, 2020 at 1:36 AM Drew Fustini <drew@pdp7.com> wrote:

> Is there no way for gpio-omap to call into the pinctrl-single backend to
> set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?

Mostly a Tony question I think, but the single pinconf_ops call
pcs_pinconf_set() which is pretty straight-forward.

Have you tried modifying omap_gpio_set_config() so that
it accepts these configs and just calls down to
gpiochip_generic_config() for anything that is not debounce?

Yours,
Linus Walleij

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Grygorii Strashko]

On 16/04/2020 02:37, Drew Fustini wrote:
> On Wed, Apr 15, 2020 at 08:59:09AM -0500, Robert Nelson wrote:
>> On Wed, Apr 15, 2020 at 8:47 AM Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>> On 15/04/2020 16:20, Robert Nelson wrote:
>>>> Hi Grygorii,
>>>>
>>>> On Wed, Apr 15, 2020 at 8:15 AM Grygorii Strashko
>>>> <grygorii.strashko@ti.com> wrote:
>>>>>
>>>>> For this platforms the dynamic GPIO muxing/configuration is not supported, and GPIO block by itself
>>>>> does not provide such functions as pullup/pulldown.
>>>>
>>>> Correct, that's the state today, while Drew is investing time into
>>>> trying to figure out how to properly extend this feature into our
>>>> platform.
>>>
>>> Sry, but it's not clear what's the final target (at least from public part of this thread).
>>
>> We are mainly targeting am335x based devices.  Today (well last few
>> years) we've utilized a "hack-ish" kernel module (bone-pinmux-helper)
>> to allow users to overide/change the pinmux-ing directly from
>> user-space...  (This evil module allows us to specify a list of
>> options for each pin, thus users can easily configure specifies of the
>> pin, aka gpio_pd/gpio_pu/etc from user-space...).  Since that time,
>> mainline has now grown a generic gpio pull-up/pull-down functionality,
>> with the ability to re-control these values directly from a generic
>> gpio library (libgpiod).
> 
> Hello Grygorii -
> 
> As Robert described, I wanted to make us of the new support for bias
> flags in the gpiolib uapi which allows userspace libraries like libgpiod
> set pull-up or pull-down on lines [0].
> 
> Is there no way for gpio-omap to call into the pinctrl-single backend to
> set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?
> 

I'm not sure how this could help ;( If there were dedicated GPIO pins - then yes.
But on all Sitara SoCs the pins are multi-functional and GPIO is not a primary function.
So to use some PIN as GPIO the MUX_MODE has to be *carefully* changed - and for this DT updated.
Which, in turn, means proper pull-up/pull-down can also be configured at the same time.
And all above is static configuration - even if it involves connecting different modules to BB.

(Changing MUX_MODE from user space sounds very unsafe for me.)

Next, how it can be ensured that User space will not corrupt PIN which is not a GPIO?
Usually only set of GPIOs per bank  are really muxed out as GPIO and, right now, there is no
way to say which ones, because pinmuxing and GPIOs configurations are going through different frameworks.

Lets say request come to configure GPIO1.20 to PULLUDEN - there is no way to be sure this pin
is actually pinmuxed as GPIO and not rgmii2_td1. And everything depends on DT and User qualification.
Now this is safe - just User will not get what he wants if he misses with GPIO number.
But if GPIO driver will be allowed to go and change some pinmux - it can kill rgmii2.

Or do you have more global ideas for pinmux management?

-- 
Best regards,
grygorii

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Thu, Apr 16, 2020 at 02:03:43PM +0200, Linus Walleij wrote:
> On Thu, Apr 16, 2020 at 1:36 AM Drew Fustini <drew@pdp7.com> wrote:
> 
> > Is there no way for gpio-omap to call into the pinctrl-single backend to
> > set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?
> 
> Mostly a Tony question I think, but the single pinconf_ops call
> pcs_pinconf_set() which is pretty straight-forward.
> 
> Have you tried modifying omap_gpio_set_config() so that
> it accepts these configs and just calls down to
> gpiochip_generic_config() for anything that is not debounce?
> 
> Yours,
> Linus Walleij

I did add the plumbing to gpio-omap to call pinctrl_gpio_set_config()
when PIN_CONFIG_BIAS_* is set.  I added lots of debug printing to
pinctrl and determined that the problem was that
pinctrl_get_device_gpio_range() would fail.  This seems to be the result
of there being no gpio-ranges property define in the device tree.  This
makes sense as currently there is no interaction between gpio and
pinctrl subsystems when running on the TI AM3358.

Another issue seems to be that the AM3358 device tree is using
compatible of "pinctrl-single" [0] and the pinctrl-single binding
documentation states this means no pinconf is possible (as opposed to
compatible of "pinconf-single").

I did try switching to "pinconf-single" but it failed to boot
successfully.  I need to troubleshoot more to figure out why that is.
But I am uncertain if "pinconf-single" should in theory work for AM3358.

Thanks,
Drew

[0] https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/am33xx-l4.dtsi#L279

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Tony Lindgren]

Hi,

* Drew Fustini <drew@pdp7.com> [200415 23:37]:
> As Robert described, I wanted to make us of the new support for bias
> flags in the gpiolib uapi which allows userspace libraries like libgpiod
> set pull-up or pull-down on lines [0].
> 
> Is there no way for gpio-omap to call into the pinctrl-single backend to
> set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?

It sure would be nice to improve some of this :) You should be able to
do this using the gpio-ranges binding with the following steps:

1. Add gpio-ranges to dts files

This should be done for all the pins that need handling, here's
just one line version:

gpio-ranges = <&pmx_core 0 15 1>;
                         |  | |
			 |  | +-- number of pins
			 |  +---- pin start
			 +------- gpio start

Some mappings can use larger ranges, while some pins just need
to be added separately.

2. Implement parsing of gpio-ranges to pinctrl-single.c

The following test patch I did a while back should get you started.

From what I recall, the issue here the addressing. The addressing
ends up using an artiticial index of pin entries in the dts, while
it should use the read pinctrl device padconf offset.

Maybe Linus has some suggestion on how to deal with that?

3. Have gpio-omap.c call gpiod_direction_input(desc) and
   gpiod_to_irq(desc) for example for gpio interrupt pins

To do that, you need something like this in gpio-omap.c:

if (of_property_read_bool(dev->of_node, "gpio-ranges")) {
	chips->chip.request = gpiochip_generic_request;
	chips->chip.free = gpiochip_generic_free;
}

Regards,

Tony

8< -------------------
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/init.h>
+#include <linux/gpio.h>
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -149,6 +150,8 @@ struct pcs_soc_data {
  * @dev:	device entry
  * @np:		device tree node
  * @pctl:	pin controller device
+ * @gc:		optional gpio chip
+ * @nr_gpio:	optional number of gpio pins
  * @flags:	mask of PCS_FEAT_xxx values
  * @missing_nr_pinctrl_cells: for legacy binding, may go away
  * @socdata:	soc specific data
@@ -178,6 +181,8 @@ struct pcs_device {
 	struct device *dev;
 	struct device_node *np;
 	struct pinctrl_dev *pctl;
+	struct gpio_chip *gc;
+	int nr_gpio;
 	unsigned flags;
 #define PCS_CONTEXT_LOSS_OFF	(1 << 3)
 #define PCS_QUIRK_SHARED_IRQ	(1 << 2)
@@ -1340,6 +1345,8 @@ static int pcs_add_gpio_func(struct device_node *node, struct pcs_device *pcs)
 		mutex_lock(&pcs->mutex);
 		list_add_tail(&range->node, &pcs->gpiofuncs);
 		mutex_unlock(&pcs->mutex);
+
+		pcs->nr_gpio += range->npins;
 	}
 	return ret;
 }
@@ -1599,6 +1606,93 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs,
 	return 0;
 }
 
+static int pcs_gpio_find_by_offset(struct pcs_device *pcs, int offset)
+{
+
+}
+
+static int pcs_gpio_request(struct gpio_chip *gc, unsigned offset)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
+
+	return 0;
+}
+
+static void pcs_gpio_free(struct gpio_chip *gc, unsigned offset)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
+}
+
+static int pcs_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
+
+	return 0;
+}
+
+static int pcs_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
+
+	return -EINVAL;
+}
+
+static void pcs_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u value: %i\n",
+		 __func__, offset, value);
+}
+
+static int pcs_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	struct pcs_device *pcs = gpiochip_get_data(gc);
+
+	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
+
+	return 0;
+}
+
+static int pcs_init_gpiochip(struct device_node *np, struct pcs_device *pcs)
+{
+	int error;
+
+	if (!pcs->nr_gpio || !of_property_read_bool(np, "gpio-controller"))
+		return 0;
+
+	pcs->gc = devm_kzalloc(pcs->dev, sizeof(*pcs->gc), GFP_KERNEL);
+	if (!pcs->gc)
+		return -ENOMEM;
+
+	pcs->gc->request = pcs_gpio_request;
+	pcs->gc->free = pcs_gpio_free;
+	pcs->gc->direction_input = pcs_gpio_direction_input;
+	pcs->gc->get = pcs_gpio_get;
+	pcs->gc->set = pcs_gpio_set;
+	pcs->gc->to_irq = pcs_gpio_to_irq;
+
+	pcs->gc->label = pcs->desc.name;
+	pcs->gc->parent = pcs->dev;
+	pcs->gc->owner = THIS_MODULE;
+	pcs->gc->base = -1;
+	pcs->gc->ngpio = pcs->nr_gpio;
+
+	error = devm_gpiochip_add_data(pcs->dev, pcs->gc, pcs);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int pcs_save_context(struct pcs_device *pcs)
 {
@@ -1868,6 +1962,10 @@ static int pcs_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto free;
 
+	ret = pcs_init_gpiochip(np, pcs);
+	if (ret < 0)
+		goto free;
+
 	pcs->socdata.irq = irq_of_parse_and_map(np, 0);
 	if (pcs->socdata.irq)
 		pcs->flags |= PCS_FEAT_IRQ;
-- 
2.26.1

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Linus Walleij]

On Thu, Apr 16, 2020 at 4:17 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> (Changing MUX_MODE from user space sounds very unsafe for me.)

The way it works on a lot of platforms is to implement the
gpio_request_enable() callback such that it muxes the pin
when requested to use as GPIO.

How to handle that is up to the driver, but the simple ones
just assume that if (A) the pin is not muxed for something
else like SPI or MMC or whatever and (B) the pin can be
muxed into GPIO mode, then it goes ahead and does that.

But this policy is up to the driver maintainer.

Given that Beagles etc are pretty much for makers and
industrial control and such, a relaxed policy would be
beneficial for users that want to do some tinkerytink.
The reverse goes for users making airplane control
systems.

Yours,
Linus Walleij

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Thu, Apr 16, 2020 at 09:32:15AM -0700, Tony Lindgren wrote:
> Hi,
> 
> * Drew Fustini <drew@pdp7.com> [200415 23:37]:
> > As Robert described, I wanted to make us of the new support for bias
> > flags in the gpiolib uapi which allows userspace libraries like libgpiod
> > set pull-up or pull-down on lines [0].
> > 
> > Is there no way for gpio-omap to call into the pinctrl-single backend to
> > set the bias bits (PULLUDEN and PULLTYPESEL) in pad control registers?
> 
> It sure would be nice to improve some of this :) You should be able to
> do this using the gpio-ranges binding with the following steps:
> 
> 1. Add gpio-ranges to dts files
> 
> This should be done for all the pins that need handling, here's
> just one line version:
> 
> gpio-ranges = <&pmx_core 0 15 1>;
>                          |  | |
> 			 |  | +-- number of pins
> 			 |  +---- pin start
> 			 +------- gpio start
> 
> Some mappings can use larger ranges, while some pins just need
> to be added separately.
> 
> 2. Implement parsing of gpio-ranges to pinctrl-single.c
> 
> The following test patch I did a while back should get you started.
> 
> From what I recall, the issue here the addressing. The addressing
> ends up using an artiticial index of pin entries in the dts, while
> it should use the read pinctrl device padconf offset.

Thanks, Tony.  I was able to apply your patch cleanly to 5.5.9 kernel
and boot it ok on the PocketBeagle (AM3358) which is what I'm currently
testing with.  I can switch to 5.7.x but I just happened to be on 5.5.x
because that is when bias flags were added to gpiolib uapi.

I'm a somewhat confused about the difference between the "gpio-ranges"
property for the gpio[0-3] nodes and the "pinctrl-single,gpio-range"
property for the am33xx_pinmux node.

For a test, I tried adding "gpio-ranges" to arch/arm/boot/dts/am33xx-l4.dtsi:

                        gpio0: gpio@0 {
                                compatible = "ti,omap4-gpio";
                                gpio-controller;
                                #gpio-cells = <2>;
                                interrupt-controller;
                                #interrupt-cells = <2>;
                                reg = <0x0 0x1000>;
                                interrupts = <96>;
                                gpio-ranges = <&am33xx_pinmux 0 0 1>;
			}

and "pinctrl-single,gpio-range" like this:

                                am33xx_pinmux: pinmux@800 {
                                        compatible = "pinctrl-single";
                                        reg = <0x800 0x238>;
                                        #pinctrl-cells = <1>;
                                        pinctrl-single,register-width = <32>;
                                        pinctrl-single,function-mask = <0x7f>;

                                        pinctrl-single,gpio-range = <&range 0 1 0>;

                                        range: gpio-range {
                                                #pinctrl-single,gpio-range-cells = <3>;
                                        };
                                };

Do you think both of those properties would be needed?

> 
> Maybe Linus has some suggestion on how to deal with that?
> 
> 3. Have gpio-omap.c call gpiod_direction_input(desc) and
>    gpiod_to_irq(desc) for example for gpio interrupt pins
> 
> To do that, you need something like this in gpio-omap.c:
> 
> if (of_property_read_bool(dev->of_node, "gpio-ranges")) {
> 	chips->chip.request = gpiochip_generic_request;
> 	chips->chip.free = gpiochip_generic_free;
> }
> 
> Regards,
> 
> Tony
> 
> 8< -------------------
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/init.h>
> +#include <linux/gpio.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> @@ -149,6 +150,8 @@ struct pcs_soc_data {
>   * @dev:	device entry
>   * @np:		device tree node
>   * @pctl:	pin controller device
> + * @gc:		optional gpio chip
> + * @nr_gpio:	optional number of gpio pins
>   * @flags:	mask of PCS_FEAT_xxx values
>   * @missing_nr_pinctrl_cells: for legacy binding, may go away
>   * @socdata:	soc specific data
> @@ -178,6 +181,8 @@ struct pcs_device {
>  	struct device *dev;
>  	struct device_node *np;
>  	struct pinctrl_dev *pctl;
> +	struct gpio_chip *gc;
> +	int nr_gpio;
>  	unsigned flags;
>  #define PCS_CONTEXT_LOSS_OFF	(1 << 3)
>  #define PCS_QUIRK_SHARED_IRQ	(1 << 2)
> @@ -1340,6 +1345,8 @@ static int pcs_add_gpio_func(struct device_node *node, struct pcs_device *pcs)
>  		mutex_lock(&pcs->mutex);
>  		list_add_tail(&range->node, &pcs->gpiofuncs);
>  		mutex_unlock(&pcs->mutex);
> +
> +		pcs->nr_gpio += range->npins;
>  	}
>  	return ret;
>  }
> @@ -1599,6 +1606,93 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs,
>  	return 0;
>  }
>  
> +static int pcs_gpio_find_by_offset(struct pcs_device *pcs, int offset)
> +{
> +
> +}
> +
> +static int pcs_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
> +
> +	return 0;
> +}
> +
> +static void pcs_gpio_free(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
> +}
> +
> +static int pcs_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
> +
> +	return 0;
> +}
> +
> +static int pcs_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
> +
> +	return -EINVAL;
> +}
> +
> +static void pcs_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u value: %i\n",
> +		 __func__, offset, value);
> +}
> +
> +static int pcs_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +	struct pcs_device *pcs = gpiochip_get_data(gc);
> +
> +	dev_info(pcs->dev, "XXX %s offset: %u\n", __func__, offset);
> +
> +	return 0;
> +}
> +
> +static int pcs_init_gpiochip(struct device_node *np, struct pcs_device *pcs)
> +{
> +	int error;
> +
> +	if (!pcs->nr_gpio || !of_property_read_bool(np, "gpio-controller"))
> +		return 0;
> +
> +	pcs->gc = devm_kzalloc(pcs->dev, sizeof(*pcs->gc), GFP_KERNEL);
> +	if (!pcs->gc)
> +		return -ENOMEM;
> +
> +	pcs->gc->request = pcs_gpio_request;
> +	pcs->gc->free = pcs_gpio_free;
> +	pcs->gc->direction_input = pcs_gpio_direction_input;
> +	pcs->gc->get = pcs_gpio_get;
> +	pcs->gc->set = pcs_gpio_set;
> +	pcs->gc->to_irq = pcs_gpio_to_irq;
> +
> +	pcs->gc->label = pcs->desc.name;
> +	pcs->gc->parent = pcs->dev;
> +	pcs->gc->owner = THIS_MODULE;
> +	pcs->gc->base = -1;
> +	pcs->gc->ngpio = pcs->nr_gpio;
> +
> +	error = devm_gpiochip_add_data(pcs->dev, pcs->gc, pcs);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM
>  static int pcs_save_context(struct pcs_device *pcs)
>  {
> @@ -1868,6 +1962,10 @@ static int pcs_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto free;
>  
> +	ret = pcs_init_gpiochip(np, pcs);
> +	if (ret < 0)
> +		goto free;
> +
>  	pcs->socdata.irq = irq_of_parse_and_map(np, 0);
>  	if (pcs->socdata.irq)
>  		pcs->flags |= PCS_FEAT_IRQ;
> -- 
> 2.26.1

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Tony Lindgren]

* Drew Fustini <drew@pdp7.com> [200423 13:17]:
> Thanks, Tony.  I was able to apply your patch cleanly to 5.5.9 kernel
> and boot it ok on the PocketBeagle (AM3358) which is what I'm currently
> testing with.  I can switch to 5.7.x but I just happened to be on 5.5.x
> because that is when bias flags were added to gpiolib uapi.

OK. BTW, with PocketBeagle and mainline v5.6 kernel, I see the micro-USB
connection always get disconnected after few hours of use. Are you aware
ofthat?

This is with the micro-USB configured as acm and ecm gadget via configfs.

> I'm a somewhat confused about the difference between the "gpio-ranges"
> property for the gpio[0-3] nodes and the "pinctrl-single,gpio-range"
> property for the am33xx_pinmux node.
> 
> For a test, I tried adding "gpio-ranges" to arch/arm/boot/dts/am33xx-l4.dtsi:
> 
>                         gpio0: gpio@0 {
>                                 compatible = "ti,omap4-gpio";
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>                                 interrupt-controller;
>                                 #interrupt-cells = <2>;
>                                 reg = <0x0 0x1000>;
>                                 interrupts = <96>;
>                                 gpio-ranges = <&am33xx_pinmux 0 0 1>;
> 			}

So the gpio-ranges tells the gpio contorller what pinctrl device pin
to use for configuring things.

> and "pinctrl-single,gpio-range" like this:
> 
>                                 am33xx_pinmux: pinmux@800 {
>                                         compatible = "pinctrl-single";
>                                         reg = <0x800 0x238>;
>                                         #pinctrl-cells = <1>;
>                                         pinctrl-single,register-width = <32>;
>                                         pinctrl-single,function-mask = <0x7f>;
> 
>                                         pinctrl-single,gpio-range = <&range 0 1 0>;
> 
>                                         range: gpio-range {
>                                                 #pinctrl-single,gpio-range-cells = <3>;
>                                         };
>                                 };
> 
> Do you think both of those properties would be needed?

No I don't think so. The pinctrl-single could be additionally
configured for gpio functionality too. For omaps, that gpio
functionality would be mostly limited to output toggling using the
internal pulls. Would be still usable on some systems though.

Also, it's been a while so I don't remember where I started running
into addressing issues though.. My guess is that you will soon hit
them too and notice :)

But basically we want to reference the pinctrl pins based on their
physical offset from the padconf base register, and not based on an
invented number in the dts. Well maybe you can describe the problem
further for us to discuss when you see it :)

Regards,

Tony

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Thu, Apr 23, 2020 at 09:42:08AM -0700, Tony Lindgren wrote:
> * Drew Fustini <drew@pdp7.com> [200423 13:17]:
> > Thanks, Tony.  I was able to apply your patch cleanly to 5.5.9 kernel
> > and boot it ok on the PocketBeagle (AM3358) which is what I'm currently
> > testing with.  I can switch to 5.7.x but I just happened to be on 5.5.x
> > because that is when bias flags were added to gpiolib uapi.
> 
> OK. BTW, with PocketBeagle and mainline v5.6 kernel, I see the micro-USB
> connection always get disconnected after few hours of use. Are you aware
> ofthat?
> 
> This is with the micro-USB configured as acm and ecm gadget via configfs.

I've been rebooting often as I build new kernels with debug output but I
will do a test with 5.6 by leaving it running overnight and see if it gets
disrupted.  I use the PocketBeagle with the TechLab PocketCape [0] for
kernel development as there is no eMMC to confuse the boot sequence and
the TechLab has a seperate USB to serial converter so I can easily see
u-boot and kernel console.

I asked Robert Nelson if he'd heard of this issue and he had not.  But he
asked how you are powering the board as any blip would probably reset the
board instantly as not enough cap's on the PocketBeagle's USB power rail.  

> > I'm a somewhat confused about the difference between the "gpio-ranges"
> > property for the gpio[0-3] nodes and the "pinctrl-single,gpio-range"
> > property for the am33xx_pinmux node.
> > 
> > For a test, I tried adding "gpio-ranges" to arch/arm/boot/dts/am33xx-l4.dtsi:
> > 
> >                         gpio0: gpio@0 {
> >                                 compatible = "ti,omap4-gpio";
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> >                                 interrupt-controller;
> >                                 #interrupt-cells = <2>;
> >                                 reg = <0x0 0x1000>;
> >                                 interrupts = <96>;
> >                                 gpio-ranges = <&am33xx_pinmux 0 0 1>;
> > 			}
> 
> So the gpio-ranges tells the gpio contorller what pinctrl device pin
> to use for configuring things.

Thanks, that makes sense. 

But in this case, pinctrl-single also needs to parse "gpio-ranges"
property from the gpio nodes?

> > and "pinctrl-single,gpio-range" like this:
> > 
> >                                 am33xx_pinmux: pinmux@800 {
> >                                         compatible = "pinctrl-single";
> >                                         reg = <0x800 0x238>;
> >                                         #pinctrl-cells = <1>;
> >                                         pinctrl-single,register-width = <32>;
> >                                         pinctrl-single,function-mask = <0x7f>;
> > 
> >                                         pinctrl-single,gpio-range = <&range 0 1 0>;
> > 
> >                                         range: gpio-range {
> >                                                 #pinctrl-single,gpio-range-cells = <3>;
> >                                         };
> >                                 };
> > 
> > Do you think both of those properties would be needed?
> 
> No I don't think so. The pinctrl-single could be additionally
> configured for gpio functionality too. For omaps, that gpio
> functionality would be mostly limited to output toggling using the
> internal pulls. Would be still usable on some systems though.

What woud it mean for the pinctrl-single to be configured for gpio
functionality?

Would that be needed for pinctrl_gpio_request() to succeed?

> Also, it's been a while so I don't remember where I started running
> into addressing issues though.. My guess is that you will soon hit
> them too and notice :)
> 
> But basically we want to reference the pinctrl pins based on their
> physical offset from the padconf base register, and not based on an
> invented number in the dts. Well maybe you can describe the problem
> further for us to discuss when you see it :)

Wouldn't the pinctrl pin numbering be from 0 to pcs_device.size /
pcs_device.width ?  

That index number of the pin plus pcs_device would give the physical
address of the padconf register.


Thanks,
Drew

[0] https://beagleboard.org/techlab

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Tony Lindgren]

* Drew Fustini <drew@pdp7.com> [200424 17:32]:
> On Thu, Apr 23, 2020 at 09:42:08AM -0700, Tony Lindgren wrote:
> > * Drew Fustini <drew@pdp7.com> [200423 13:17]:
> > > Thanks, Tony.  I was able to apply your patch cleanly to 5.5.9 kernel
> > > and boot it ok on the PocketBeagle (AM3358) which is what I'm currently
> > > testing with.  I can switch to 5.7.x but I just happened to be on 5.5.x
> > > because that is when bias flags were added to gpiolib uapi.
> > 
> > OK. BTW, with PocketBeagle and mainline v5.6 kernel, I see the micro-USB
> > connection always get disconnected after few hours of use. Are you aware
> > ofthat?
> > 
> > This is with the micro-USB configured as acm and ecm gadget via configfs.
> 
> I've been rebooting often as I build new kernels with debug output but I
> will do a test with 5.6 by leaving it running overnight and see if it gets
> disrupted.  I use the PocketBeagle with the TechLab PocketCape [0] for
> kernel development as there is no eMMC to confuse the boot sequence and
> the TechLab has a seperate USB to serial converter so I can easily see
> u-boot and kernel console.

Oh OK that looks like a nice cape :)

> I asked Robert Nelson if he'd heard of this issue and he had not.  But he
> asked how you are powering the board as any blip would probably reset the
> board instantly as not enough cap's on the PocketBeagle's USB power rail.  

Hmm good point. I recall the pocketbeagle uptime being many days with
USB stopping enumerating. But yes, this is powered over USB.

> > > I'm a somewhat confused about the difference between the "gpio-ranges"
> > > property for the gpio[0-3] nodes and the "pinctrl-single,gpio-range"
> > > property for the am33xx_pinmux node.
> > > 
> > > For a test, I tried adding "gpio-ranges" to arch/arm/boot/dts/am33xx-l4.dtsi:
> > > 
> > >                         gpio0: gpio@0 {
> > >                                 compatible = "ti,omap4-gpio";
> > >                                 gpio-controller;
> > >                                 #gpio-cells = <2>;
> > >                                 interrupt-controller;
> > >                                 #interrupt-cells = <2>;
> > >                                 reg = <0x0 0x1000>;
> > >                                 interrupts = <96>;
> > >                                 gpio-ranges = <&am33xx_pinmux 0 0 1>;
> > > 			}
> > 
> > So the gpio-ranges tells the gpio contorller what pinctrl device pin
> > to use for configuring things.
> 
> Thanks, that makes sense. 
> 
> But in this case, pinctrl-single also needs to parse "gpio-ranges"
> property from the gpio nodes?

Hmm not sure but don't think so, I think that information comes from
the calls from gpio-omap.c to pinctrl-single.c.

> > > and "pinctrl-single,gpio-range" like this:
> > > 
> > >                                 am33xx_pinmux: pinmux@800 {
> > >                                         compatible = "pinctrl-single";
> > >                                         reg = <0x800 0x238>;
> > >                                         #pinctrl-cells = <1>;
> > >                                         pinctrl-single,register-width = <32>;
> > >                                         pinctrl-single,function-mask = <0x7f>;
> > > 
> > >                                         pinctrl-single,gpio-range = <&range 0 1 0>;
> > > 
> > >                                         range: gpio-range {
> > >                                                 #pinctrl-single,gpio-range-cells = <3>;
> > >                                         };
> > >                                 };
> > > 
> > > Do you think both of those properties would be needed?
> > 
> > No I don't think so. The pinctrl-single could be additionally
> > configured for gpio functionality too. For omaps, that gpio
> > functionality would be mostly limited to output toggling using the
> > internal pulls. Would be still usable on some systems though.
> 
> What woud it mean for the pinctrl-single to be configured for gpio
> functionality?

That would mean that you can also use pinctrl-single as a gp(i)o
controller in some cases with the pulls instead of gpio-omap.

> Would that be needed for pinctrl_gpio_request() to succeed?

No, that's a separate feature.

> > Also, it's been a while so I don't remember where I started running
> > into addressing issues though.. My guess is that you will soon hit
> > them too and notice :)
> > 
> > But basically we want to reference the pinctrl pins based on their
> > physical offset from the padconf base register, and not based on an
> > invented number in the dts. Well maybe you can describe the problem
> > further for us to discuss when you see it :)
> 
> Wouldn't the pinctrl pin numbering be from 0 to pcs_device.size /
> pcs_device.width ?  

Yes. And ideally we'd specify the real register offset rather
than the index number for the pin also in the dts.

If that does not work for some reason, then it's best to set up a
macro to avoid confusion with figuring out the pin index rather
than look up the register offset in the TRM. But we can figure
that out once you have something working.

> That index number of the pin plus pcs_device would give the physical
> address of the padconf register.

Right, I'm more concerned what gets put into the dts files
as that's an ABI.

Regards,

Tony

> [0] https://beagleboard.org/techlab

Re: gpio-omap: add support gpiolib bias (pull-up/down) flags?

[Drew Fustini]

On Fri, Apr 24, 2020 at 10:49:44AM -0700, Tony Lindgren wrote:
> * Drew Fustini <drew@pdp7.com> [200424 17:32]:
> > On Thu, Apr 23, 2020 at 09:42:08AM -0700, Tony Lindgren wrote:
> > > * Drew Fustini <drew@pdp7.com> [200423 13:17]:
> > > > Thanks, Tony.  I was able to apply your patch cleanly to 5.5.9 kernel
> > > > and boot it ok on the PocketBeagle (AM3358) which is what I'm currently
> > > > testing with.  I can switch to 5.7.x but I just happened to be on 5.5.x
> > > > because that is when bias flags were added to gpiolib uapi.
> > > 
> > > OK. BTW, with PocketBeagle and mainline v5.6 kernel, I see the micro-USB
> > > connection always get disconnected after few hours of use. Are you aware
> > > ofthat?
> > > 
> > > This is with the micro-USB configured as acm and ecm gadget via configfs.
> > 
> > I've been rebooting often as I build new kernels with debug output but I
> > will do a test with 5.6 by leaving it running overnight and see if it gets
> > disrupted.  I use the PocketBeagle with the TechLab PocketCape [0] for
> > kernel development as there is no eMMC to confuse the boot sequence and
> > the TechLab has a seperate USB to serial converter so I can easily see
> > u-boot and kernel console.
> 
> Oh OK that looks like a nice cape :)
> 
> > I asked Robert Nelson if he'd heard of this issue and he had not.  But he
> > asked how you are powering the board as any blip would probably reset the
> > board instantly as not enough cap's on the PocketBeagle's USB power rail.  
> 
> Hmm good point. I recall the pocketbeagle uptime being many days with
> USB stopping enumerating. But yes, this is powered over USB.
> 
> > > > I'm a somewhat confused about the difference between the "gpio-ranges"
> > > > property for the gpio[0-3] nodes and the "pinctrl-single,gpio-range"
> > > > property for the am33xx_pinmux node.
> > > > 
> > > > For a test, I tried adding "gpio-ranges" to arch/arm/boot/dts/am33xx-l4.dtsi:
> > > > 
> > > >                         gpio0: gpio@0 {
> > > >                                 compatible = "ti,omap4-gpio";
> > > >                                 gpio-controller;
> > > >                                 #gpio-cells = <2>;
> > > >                                 interrupt-controller;
> > > >                                 #interrupt-cells = <2>;
> > > >                                 reg = <0x0 0x1000>;
> > > >                                 interrupts = <96>;
> > > >                                 gpio-ranges = <&am33xx_pinmux 0 0 1>;
> > > > 			}
> > > 
> > > So the gpio-ranges tells the gpio contorller what pinctrl device pin
> > > to use for configuring things.
> > 
> > Thanks, that makes sense. 
> > 
> > But in this case, pinctrl-single also needs to parse "gpio-ranges"
> > property from the gpio nodes?
> 
> Hmm not sure but don't think so, I think that information comes from
> the calls from gpio-omap.c to pinctrl-single.c.
> 
> > > > and "pinctrl-single,gpio-range" like this:
> > > > 
> > > >                                 am33xx_pinmux: pinmux@800 {
> > > >                                         compatible = "pinctrl-single";
> > > >                                         reg = <0x800 0x238>;
> > > >                                         #pinctrl-cells = <1>;
> > > >                                         pinctrl-single,register-width = <32>;
> > > >                                         pinctrl-single,function-mask = <0x7f>;
> > > > 
> > > >                                         pinctrl-single,gpio-range = <&range 0 1 0>;
> > > > 
> > > >                                         range: gpio-range {
> > > >                                                 #pinctrl-single,gpio-range-cells = <3>;
> > > >                                         };
> > > >                                 };
> > > > 
> > > > Do you think both of those properties would be needed?
> > > 
> > > No I don't think so. The pinctrl-single could be additionally
> > > configured for gpio functionality too. For omaps, that gpio
> > > functionality would be mostly limited to output toggling using the
> > > internal pulls. Would be still usable on some systems though.
> > 
> > What woud it mean for the pinctrl-single to be configured for gpio
> > functionality?
> 
> That would mean that you can also use pinctrl-single as a gp(i)o
> controller in some cases with the pulls instead of gpio-omap.
> 
> > Would that be needed for pinctrl_gpio_request() to succeed?
> 
> No, that's a separate feature.
> 
> > > Also, it's been a while so I don't remember where I started running
> > > into addressing issues though.. My guess is that you will soon hit
> > > them too and notice :)
> > > 
> > > But basically we want to reference the pinctrl pins based on their
> > > physical offset from the padconf base register, and not based on an
> > > invented number in the dts. Well maybe you can describe the problem
> > > further for us to discuss when you see it :)
> > 
> > Wouldn't the pinctrl pin numbering be from 0 to pcs_device.size /
> > pcs_device.width ?  
> 
> Yes. And ideally we'd specify the real register offset rather
> than the index number for the pin also in the dts.
> 
> If that does not work for some reason, then it's best to set up a
> macro to avoid confusion with figuring out the pin index rather
> than look up the register offset in the TRM. But we can figure
> that out once you have something working.
> 
> > That index number of the pin plus pcs_device would give the physical
> > address of the padconf register.
> 
> Right, I'm more concerned what gets put into the dts files
> as that's an ABI.
> 
> Regards,
> 
> Tony

I have compiled the following csv list to generate the gpio-ranges
properties by referencing "Table 9-10. CONTROL_MODULE REGISTERS" in
the  "AM335xTechnical Reference Manual" [0] and "Table 4-2. Pin 
Attributes" in the "AM335x Sitara Processor datasheet" [1]:

gpiochip,gpio-line,pinctrl-PIN,pinctrl-address
0,0,82,44e10948
0,1,83,44e1094c
0,2,84,44e10950
0,3,85,44e10954
0,4,86,44e10958
0,5,87,44e1095c
0,6,88,44e10960
0,7,89,44e10964
0,8,52,44e108d0
0,9,53,44e108d4
0,10,54,44e108d8
0,11,55,44e108dc
0,12,94,44e10978
0,13,95,44e1097c
0,14,96,44e10980
0,15,97,44e10984
0,16,71,44e1091c
0,17,72,44e10920
0,18,135,44e10a1c
0,19,108,44e109b0
0,20,109,44e109b4
0,21,73,44e10924
0,22,8,44e10820
0,23,9,44e10824
0,26,10,44e10828
0,27,11,44e1082c
0,28,74,44e10928
0,29,81,44e10944
0,30,28,44e10870
0,31,29,44e10874
1,0,0,44e10800
1,1,1,44e10804
1,2,2,44e10808
1,3,3,44e1080c
1,4,4,44e10810
1,5,5,44e10814
1,6,6,44e10818
1,7,7,44e1081c
1,8,90,44e10968
1,9,91,44e1096c
1,10,92,44e10970
1,11,93,44e10974
1,12,12,44e10830
1,13,13,44e10834
1,14,14,44e10838
1,15,15,44e1083c
1,16,16,44e10840
1,17,17,44e10844
1,18,18,44e10848
1,19,19,44e1084c
1,20,20,44e10850
1,21,21,44e10854
1,22,22,44e10858
1,23,23,44e1085c
1,24,24,44e10860
1,25,25,44e10864
1,26,26,44e10868
1,27,27,44e1086c
1,28,30,44e10878
1,29,31,44e1087c
1,30,32,44e10880
1,31,33,44e10884
2,0,34,44e10888
2,1,35,44e1088c
2,2,36,44e10890
2,3,37,44e10894
2,4,38,44e10898
2,5,39,44e1089c
2,6,40,44e108a0
2,7,41,44e108a4
2,8,42,44e108a8
2,9,43,44e108ac
2,10,44,44e108b0
2,11,45,44e108b4
2,12,46,44e108b8
2,13,47,44e108bc
2,14,48,44e108c0
2,15,49,44e108c4
2,16,50,44e108c8
2,17,51,44e108cc
2,18,77,44e10934
2,19,78,44e10938
2,20,79,44e1093c
2,21,80,44e10940
2,22,56,44e108e0
2,23,57,44e108e4
2,24,58,44e108e8
2,25,59,44e108ec
2,26,60,44e108f0
2,27,61,44e108f4
2,28,62,44e108f8
2,29,63,44e108fc
2,30,64,44e10900
2,31,65,44e10904
3,0,66,44e10908
3,1,67,44e1090c
3,2,68,44e10910
3,3,69,44e10914
3,4,70,44e10918
3,5,98,44e10988
3,6,99,44e1098c
3,9,75,44e1092c
3,10,76,44e10930
3,13,141,44e10a34
3,14,100,44e10990
3,15,101,44e10994
3,16,102,44e10998
3,17,103,44e1099c
3,18,104,44e109a0
3,19,105,44e109a4
3,20,106,44e109a8
3,21,107,44e109ac


Here is the gpio-ranges property for gpiochip0 and I will post a patch
for gpiochip[0..3] if it seems like I am on the right track:
 
diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index 5ed7f3c58c0f..65d46777ffb3 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -151,6 +151,19 @@ SYSC_OMAP2_SOFTRESET |
 
                        gpio0: gpio@0 {
                                compatible = "ti,omap4-gpio";
+                               gpio-ranges =
+                               <&am33xx_pinmux  0  82 8>, // gpio0.0  ->  PIN82 (0x948)
+                               <&am33xx_pinmux  8  52 4>, // gpio0.8  ->  PIN52 (0x8D0)
+                               <&am33xx_pinmux 12  94 4>, // gpio0.12 ->  PIN94 (0x978)
+                               <&am33xx_pinmux 16  71 2>, // gpio0.16 ->  PIN71 (0x91C)
+                               <&am33xx_pinmux 18 135 1>, // gpio0.18 -> PIN135 (0xA1C)
+                               <&am33xx_pinmux 19 108 2>, // gpio0.19 -> PIN135 (0x9B0)
+                               <&am33xx_pinmux 21  73 1>, // gpio0.21 ->  PIN73 (0x9B0)
+                               <&am33xx_pinmux 22   8 2>, // gpio0.22 ->   PIN8 (0x820)
+                               <&am33xx_pinmux 26  10 2>, // gpio0.26 ->  PIN10 (0x828)
+                               <&am33xx_pinmux 28  74 1>, // gpio0.28 ->  PIN10 (0x928)
+                               <&am33xx_pinmux 29  81 1>, // gpio0.29 ->  PIN10 (0x944)
+                               <&am33xx_pinmux 30  28 2>; // gpio0.30 ->  PIN10 (0x870)
                                gpio-controller;
                                #gpio-cells = <2>;
                                interrupt-controller;

And here is the content of
/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/gpio-ranges:

GPIO ranges handled:
0: gpio-0-31 GPIOS [0 - 7] PINS [82 - 89]
8: gpio-0-31 GPIOS [8 - 11] PINS [52 - 55]
12: gpio-0-31 GPIOS [12 - 15] PINS [94 - 97]
16: gpio-0-31 GPIOS [16 - 17] PINS [71 - 72]
18: gpio-0-31 GPIOS [18 - 18] PINS [135 - 135]
19: gpio-0-31 GPIOS [19 - 20] PINS [108 - 109]
21: gpio-0-31 GPIOS [21 - 21] PINS [73 - 73]
22: gpio-0-31 GPIOS [22 - 23] PINS [8 - 9]
26: gpio-0-31 GPIOS [26 - 27] PINS [10 - 11]
28: gpio-0-31 GPIOS [28 - 28] PINS [74 - 74]
29: gpio-0-31 GPIOS [29 - 29] PINS [81 - 81]
30: gpio-0-31 GPIOS [30 - 31] PINS [28 - 29]

Therefore, I think it would be useful to populate "gpio-ranges" in 
am33xx-l4.dtsi.

Is there a problem that I am missing?

Thank you,
Drew