Pinconf issues on AMxxx plattforms

Pinconf issues on AMxxx plattforms

[Niedermayr, BENEDIKT]

Hello,

We encountered some issues when accessing the gpiochardev interface on an
AM65xx plaform.
 
The basic idea was to fully rely on all features the gpiochardev seems to
offer. 
I got all relevant information from the Linux Kernel Documentation
(Documentation/driver-api/pin-control.rst) and saw 
some presentations from Linus Walleij  regarding the gpiochardev
capabilities.

If I understand that correctly the gpiochardev interface should support the
following features:
* Requesting gpio pins from userspace
* Set input/output directions
* Set BIAS settings (pull-up, pull-down, etc.)
* Gpio function of that pin automatically gets muxed in if requested 

Requesting pins worked for me as expected after I added the required DTB
properties:
* pinctrl-single: Add each required pin to "pinctrl-single,gpio-range" in
the pincontroller node 
* gpio: Add each required pin to gpio-range property in the gpio node

I also added the required childnodes in the pinctrl node:

&main_pmx0 {
  ... 
   d6_gpio: d6-gpio {
pinctrl-single,pins = <
/* (AH16) GPIO0_38 */
AM65X_IOPAD(0x0098, PIN_INPUT, 7)
>;
pinctrl-single,bias-pullup   = <0x20000  0x20000  0x10000  0x30000>;
pinctrl-single,bias-pulldown = <0x00000  0x0      0x10000  0x30000>;
};
...
}

When I tried to set any BIAS settings nothing happened or some error occured
in the kernel logs (i'm not 100% sure anymore since almost 2 months have
past). 
The first thing I had to do was to assign the gpiochip_generic_config
callback to the gpiochiop for that (gpio-davinci.c). This callback in turn
will finally call pcs_pinconf_set(), which
is the pinctrl drivers related callback for setting the pinconf.
But still no success...

Then I went deeper into the rabbit whole and encountered that the error had
to do with pcs_get_function() (pinctrl-single.c).
I found out that this function requests the current function (or pinmux
state) from the pinctrl subsystem core. 
The pinctrl driver needs this information for accessing the correct pinctrl
childnode bits. 

So what is the Problem here?
The pinctrl offers 3 different options for muxing:

1.  Using the generic kernel APIs: 
     Call pinctrl_select_state() function as stated
in  Documentation/driver-api/pin-control.rst (section "Pin control requests
from drivers").
     This function will select a defined state which has been defined in DTB
with "pinctrl-0", "pinctrl-1", "pinctrl-x"
2.  Mux pins with debugfs:
     Write the desired pingroup and pinfunction into the "pinmux-select"
file of the related pin controller.
3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
drivers pcs_request_gpio() function.

The problem now is that only option 1. will store the current mux
information in the pinctrl subsystems core. 
The pinctrl-single driver highly depends on that information, which is not
available at all wenn muxing with options 2&3.

I was able to fix that for option 2 but not for option 3. The problem here
is that the pcs_request_gpio() function just does not provide enough
parameters with sufficient information for achieving that task.
 

I'm not sure if I miss something important here?
Are you aware of this issue? 


Cheers,
benedikt

Re: Pinconf issues on AMxxx plattforms

[Tony Lindgren]

Hi,

Adding linux-gpio and Linus W to Cc. Some questions and comments below
towards the end.

* Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [230503 08:38]:
> Hello,
> 
> We encountered some issues when accessing the gpiochardev interface on an
> AM65xx plaform.
>  
> The basic idea was to fully rely on all features the gpiochardev seems to
> offer. 
> I got all relevant information from the Linux Kernel Documentation
> (Documentation/driver-api/pin-control.rst) and saw 
> some presentations from Linus Walleij  regarding the gpiochardev
> capabilities.
> 
> If I understand that correctly the gpiochardev interface should support the
> following features:
> * Requesting gpio pins from userspace
> * Set input/output directions
> * Set BIAS settings (pull-up, pull-down, etc.)
> * Gpio function of that pin automatically gets muxed in if requested 
> 
> Requesting pins worked for me as expected after I added the required DTB
> properties:
> * pinctrl-single: Add each required pin to "pinctrl-single,gpio-range" in
> the pincontroller node 
> * gpio: Add each required pin to gpio-range property in the gpio node
> 
> I also added the required childnodes in the pinctrl node:
> 
> &main_pmx0 {
>   ... 
>    d6_gpio: d6-gpio {
> pinctrl-single,pins = <
> /* (AH16) GPIO0_38 */
> AM65X_IOPAD(0x0098, PIN_INPUT, 7)
> >;
> pinctrl-single,bias-pullup   = <0x20000  0x20000  0x10000  0x30000>;
> pinctrl-single,bias-pulldown = <0x00000  0x0      0x10000  0x30000>;
> };
> ...
> }
> 
> When I tried to set any BIAS settings nothing happened or some error occured
> in the kernel logs (i'm not 100% sure anymore since almost 2 months have
> past). 
> The first thing I had to do was to assign the gpiochip_generic_config
> callback to the gpiochiop for that (gpio-davinci.c). This callback in turn
> will finally call pcs_pinconf_set(), which
> is the pinctrl drivers related callback for setting the pinconf.
> But still no success...
> 
> Then I went deeper into the rabbit whole and encountered that the error had
> to do with pcs_get_function() (pinctrl-single.c).
> I found out that this function requests the current function (or pinmux
> state) from the pinctrl subsystem core. 
> The pinctrl driver needs this information for accessing the correct pinctrl
> childnode bits. 
> 
> So what is the Problem here?
> The pinctrl offers 3 different options for muxing:
> 
> 1.  Using the generic kernel APIs: 
>      Call pinctrl_select_state() function as stated
> in  Documentation/driver-api/pin-control.rst (section "Pin control requests
> from drivers").
>      This function will select a defined state which has been defined in DTB
> with "pinctrl-0", "pinctrl-1", "pinctrl-x"
> 2.  Mux pins with debugfs:
>      Write the desired pingroup and pinfunction into the "pinmux-select"
> file of the related pin controller.
> 3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
> drivers pcs_request_gpio() function.
> 
> The problem now is that only option 1. will store the current mux
> information in the pinctrl subsystems core. 
> The pinctrl-single driver highly depends on that information, which is not
> available at all wenn muxing with options 2&3.
> 
> I was able to fix that for option 2 but not for option 3. The problem here
> is that the pcs_request_gpio() function just does not provide enough
> parameters with sufficient information for achieving that task.

Care to post what your fix for #2 above looks like?

> I'm not sure if I miss something important here?
> Are you aware of this issue? 

Sounds like something needs to be implemented for pinctrl-single.c.

Regards,

Tony

Re: Pinconf issues on AMxxx plattforms

[Linus Walleij]

On Thu, May 4, 2023 at 7:35 AM Tony Lindgren <tony@atomide.com> wrote:
> * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [230503 08:38]:

> > We encountered some issues when accessing the gpiochardev interface on an
> > AM65xx plaform.

Thank you for using contemporary APIs!

> > The pinctrl offers 3 different options for muxing:
> >
> > 1.  Using the generic kernel APIs:
> >      Call pinctrl_select_state() function as stated
> > in  Documentation/driver-api/pin-control.rst (section "Pin control requests
> > from drivers").
> >      This function will select a defined state which has been defined in DTB
> > with "pinctrl-0", "pinctrl-1", "pinctrl-x"
> > 2.  Mux pins with debugfs:
> >      Write the desired pingroup and pinfunction into the "pinmux-select"
> > file of the related pin controller.
> > 3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
> > drivers pcs_request_gpio() function.
> >
> > The problem now is that only option 1. will store the current mux
> > information in the pinctrl subsystems core.
> > The pinctrl-single driver highly depends on that information, which is not
> > available at all wenn muxing with options 2&3.
> >
> > I was able to fix that for option 2 but not for option 3. The problem here
> > is that the pcs_request_gpio() function just does not provide enough
> > parameters with sufficient information for achieving that task.

The fact that 3) doesn't work has to do with how pinctrl-single has
been engineered I think, what the pinctrl_ops .gpio_request_enable/
.gpio_disable_free/.gpio_set_direction provide is a "shortcut"
for drivers that want to take it, in case they can provide the right
information.

The pinctrl-single driver only implements .gpio_request_enable()
but often that is enough.

Then it is the callbacks for generic config that you said you already
added to gpio-davinci.c, so that part should be fine, patches welcome!

Yours,
Linus Walleij

Re: Pinconf issues on AMxxx plattforms

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [230504 08:31]:
> On Thu, May 4, 2023 at 7:35 AM Tony Lindgren <tony@atomide.com> wrote:
> > * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [230503 08:38]:
> 
> > > We encountered some issues when accessing the gpiochardev interface on an
> > > AM65xx plaform.
> 
> Thank you for using contemporary APIs!
> 
> > > The pinctrl offers 3 different options for muxing:
> > >
> > > 1.  Using the generic kernel APIs:
> > >      Call pinctrl_select_state() function as stated
> > > in  Documentation/driver-api/pin-control.rst (section "Pin control requests
> > > from drivers").
> > >      This function will select a defined state which has been defined in DTB
> > > with "pinctrl-0", "pinctrl-1", "pinctrl-x"
> > > 2.  Mux pins with debugfs:
> > >      Write the desired pingroup and pinfunction into the "pinmux-select"
> > > file of the related pin controller.
> > > 3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
> > > drivers pcs_request_gpio() function.
> > >
> > > The problem now is that only option 1. will store the current mux
> > > information in the pinctrl subsystems core.
> > > The pinctrl-single driver highly depends on that information, which is not
> > > available at all wenn muxing with options 2&3.
> > >
> > > I was able to fix that for option 2 but not for option 3. The problem here
> > > is that the pcs_request_gpio() function just does not provide enough
> > > parameters with sufficient information for achieving that task.
> 
> The fact that 3) doesn't work has to do with how pinctrl-single has
> been engineered I think, what the pinctrl_ops .gpio_request_enable/
> .gpio_disable_free/.gpio_set_direction provide is a "shortcut"
> for drivers that want to take it, in case they can provide the right
> information.
> 
> The pinctrl-single driver only implements .gpio_request_enable()
> but often that is enough.
> 
> Then it is the callbacks for generic config that you said you already
> added to gpio-davinci.c, so that part should be fine, patches welcome!

Thanks for explaining. Sounds like pinctrl-single should possibly
implement the missing functions and update pinctrl core with the
changes when a gpio is requested.

Regards,

Tony

Re: Pinconf issues on AMxxx plattforms

[Niedermayr, BENEDIKT]

On Thu, 2023-05-04 at 08:35 +0300, Tony Lindgren wrote:
> Hi,
> 
> Adding linux-gpio and Linus W to Cc. Some questions and comments below
> towards the end.
> 
> * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [230503 08:38]:
> > Hello,
> > 
> > We encountered some issues when accessing the gpiochardev interface on an
> > AM65xx plaform.
> >  
> > The basic idea was to fully rely on all features the gpiochardev seems to
> > offer. 
> > I got all relevant information from the Linux Kernel Documentation
> > (Documentation/driver-api/pin-control.rst) and saw 
> > some presentations from Linus Walleij  regarding the gpiochardev
> > capabilities.
> > 
> > If I understand that correctly the gpiochardev interface should support the
> > following features:
> > * Requesting gpio pins from userspace
> > * Set input/output directions
> > * Set BIAS settings (pull-up, pull-down, etc.)
> > * Gpio function of that pin automatically gets muxed in if requested 
> > 
> > Requesting pins worked for me as expected after I added the required DTB
> > properties:
> > * pinctrl-single: Add each required pin to "pinctrl-single,gpio-range" in
> > the pincontroller node 
> > * gpio: Add each required pin to gpio-range property in the gpio node
> > 
> > I also added the required childnodes in the pinctrl node:
> > 
> > &main_pmx0 {
> >   ... 
> >    d6_gpio: d6-gpio {
> > pinctrl-single,pins = <
> > /* (AH16) GPIO0_38 */
> > AM65X_IOPAD(0x0098, PIN_INPUT, 7)
> > > ;
> > pinctrl-single,bias-pullup   = <0x20000  0x20000  0x10000  0x30000>;
> > pinctrl-single,bias-pulldown = <0x00000  0x0      0x10000  0x30000>;
> > };
> > ...
> > }
> > 
> > When I tried to set any BIAS settings nothing happened or some error occured
> > in the kernel logs (i'm not 100% sure anymore since almost 2 months have
> > past). 
> > The first thing I had to do was to assign the gpiochip_generic_config
> > callback to the gpiochiop for that (gpio-davinci.c). This callback in turn
> > will finally call pcs_pinconf_set(), which
> > is the pinctrl drivers related callback for setting the pinconf.
> > But still no success...
> > 
> > Then I went deeper into the rabbit whole and encountered that the error had
> > to do with pcs_get_function() (pinctrl-single.c).
> > I found out that this function requests the current function (or pinmux
> > state) from the pinctrl subsystem core. 
> > The pinctrl driver needs this information for accessing the correct pinctrl
> > childnode bits. 
> > 
> > So what is the Problem here?
> > The pinctrl offers 3 different options for muxing:
> > 
> > 1.  Using the generic kernel APIs: 
> >      Call pinctrl_select_state() function as stated
> > in  Documentation/driver-api/pin-control.rst (section "Pin control requests
> > from drivers").
> >      This function will select a defined state which has been defined in DTB
> > with "pinctrl-0", "pinctrl-1", "pinctrl-x"
> > 2.  Mux pins with debugfs:
> >      Write the desired pingroup and pinfunction into the "pinmux-select"
> > file of the related pin controller.
> > 3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
> > drivers pcs_request_gpio() function.
> > 
> > The problem now is that only option 1. will store the current mux
> > information in the pinctrl subsystems core. 
> > The pinctrl-single driver highly depends on that information, which is not
> > available at all wenn muxing with options 2&3.
> > 
> > I was able to fix that for option 2 but not for option 3. The problem here
> > is that the pcs_request_gpio() function just does not provide enough
> > parameters with sufficient information for achieving that task.
> 
> Care to post what your fix for #2 above looks like?

Thanks for the reply.

Sure, I can post it. But that is just an example and not a final proposal.

It seems pretty complex but accessing "pdesc->mux_setting" directly was not possible, since
it is defined as const pointer. 

So I created an internal shadow copy private data struct and stored the mux_settings there.
In pcs_set_mux() I can then update the mux_setting and query it in pcs_get_function().

Im sure that there are much better solutions but for now it's just a first shot.


diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index cd314a5305a2..fa47fbf1534c 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -38,6 +38,16 @@
 #define DRIVER_NAME			"pinctrl-single"
 #define PCS_OFF_DISABLED		~0U
 
+/**
+ * struct pcs_setting_mux - hold current mux settings per pin
+ * @func: 	selected pin function
+ * @group:	selected pin group
+ */
+struct pcs_setting_mux {
+	unsigned func;
+	unsigned group;
+};
+
 /**
  * struct pcs_func_vals - mux function register offset and value pair
  * @reg:	register virtual address
@@ -340,12 +350,11 @@ static int pcs_get_function(struct pinctrl_dev *pctldev, unsigned pin,
 {
 	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
 	struct pin_desc *pdesc = pin_desc_get(pctldev, pin);
-	const struct pinctrl_setting_mux *setting;
+	struct pcs_setting_mux *setting = pdesc->drv_data;
 	struct function_desc *function;
 	unsigned fselector;
 
 	/* If pin is not described in DTS & enabled, mux_setting is NULL. */
-	setting = pdesc->mux_setting;
 	if (!setting)
 		return -ENOTSUPP;
 	fselector = setting->func;
@@ -365,7 +374,9 @@ static int pcs_set_mux(struct pinctrl_dev *pctldev, unsigned fselector,
 	struct pcs_device *pcs;
 	struct function_desc *function;
 	struct pcs_function *func;
-	int i;
+	int i, ret;
+	const unsigned *pins = NULL;
+	unsigned num_pins = 0;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 	/* If function mask is null, needn't enable it. */
@@ -376,8 +387,32 @@ static int pcs_set_mux(struct pinctrl_dev *pctldev, unsigned fselector,
 	if (!func)
 		return -EINVAL;
 
-	dev_dbg(pcs->dev, "enabling %s function%i\n",
-		func->name, fselector);
+	/*
+	 * Last mux setting must be stored here as well.
+	 * Referring to proper pinconf requires the current pin function
+	 * to be known
+	 */
+	ret = pinctrl_generic_get_group_pins(pctldev, group, &pins, &num_pins);
+	if (ret && PCS_HAS_PINCONF) {
+		dev_err(pcs->dev, "Cannot store mux settings\n");
+		return ret;
+	}
+
+	for (i = 0; i < num_pins; i++) {
+		struct pcs_setting_mux *setting;
+		struct pin_desc *desc;
+
+		desc = pin_desc_get(pctldev, pins[i]);
+		if (!desc) {
+			dev_err(pcs->dev, "Unable to request pindesc for PIN%d\n",
+				pins[i]);
+			continue;
+		}
+
+		setting = desc->drv_data;
+		setting->func = fselector;
+		setting->group = group;
+	}
 
 	for (i = 0; i < func->nvals; i++) {
 		struct pcs_func_vals *vals;
@@ -682,6 +717,7 @@ static int pcs_add_pin(struct pcs_device *pcs, unsigned int offset)
 {
 	struct pcs_soc_data *pcs_soc = &pcs->socdata;
 	struct pinctrl_pin_desc *pin;
+	struct pcs_setting_mux *setting;
 	int i;
 
 	i = pcs->pins.cur;
@@ -703,7 +739,14 @@ static int pcs_add_pin(struct pcs_device *pcs, unsigned int offset)
 		}
 	}
 
+
+	setting = devm_kzalloc(pcs->dev, sizeof(*setting), GFP_KERNEL);
+	if (!setting)
+		return -ENOMEM;
+
+
 	pin = &pcs->pins.pa[i];
+	pin->drv_data = setting;
 	pin->number = i;
 	pcs->pins.cur++;

> pin_desc
> > I'm not sure if I miss something important here?
> > Are you aware of this issue? 
> 
> Sounds like something needs to be implemented for pinctrl-single.c.

Ok then I'm not completely wrong. 
I will try to sent out a more complete patchseries for discussions during next two weeks.
  

> 
> Regards,
> 
> Tony

Cheers,
Benedikt

Re: Pinconf issues on AMxxx plattforms

[Niedermayr, BENEDIKT]

On Thu, 2023-05-04 at 10:31 +0200, Linus Walleij wrote:
> On Thu, May 4, 2023 at 7:35 AM Tony Lindgren <tony@atomide.com> wrote:
> > * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [230503 08:38]:
> > > We encountered some issues when accessing the gpiochardev interface on an
> > > AM65xx plaform.
> 
> Thank you for using contemporary APIs!

And thank you for providing such!
> 
> > > The pinctrl offers 3 different options for muxing:
> > > 
> > > 1.  Using the generic kernel APIs:
> > >      Call pinctrl_select_state() function as stated
> > > in  Documentation/driver-api/pin-control.rst (section "Pin control requests
> > > from drivers").
> > >      This function will select a defined state which has been defined in DTB
> > > with "pinctrl-0", "pinctrl-1", "pinctrl-x"
> > > 2.  Mux pins with debugfs:
> > >      Write the desired pingroup and pinfunction into the "pinmux-select"
> > > file of the related pin controller.
> > > 3. Mux the GPIO function of a requested GPIO pin by calling the pinctrl
> > > drivers pcs_request_gpio() function.
> > > 
> > > The problem now is that only option 1. will store the current mux
> > > information in the pinctrl subsystems core.
> > > The pinctrl-single driver highly depends on that information, which is not
> > > available at all wenn muxing with options 2&3.
> > > 
> > > I was able to fix that for option 2 but not for option 3. The problem here
> > > is that the pcs_request_gpio() function just does not provide enough
> > > parameters with sufficient information for achieving that task.
> 
> The fact that 3) doesn't work has to do with how pinctrl-single has
> been engineered I think, what the pinctrl_ops .gpio_request_enable/
> .gpio_disable_free/.gpio_set_direction provide is a "shortcut"
> for drivers that want to take it, in case they can provide the right
> information.

Ok so I'm not completely wrong here. Now I feel more confident for further
investigation. 


> 
> The pinctrl-single driver only implements .gpio_request_enable()
> but often that is enough.
> 
> Then it is the callbacks for generic config that you said you already
> added to gpio-davinci.c, so that part should be fine, patches welcome!

I'll send out some patches the next two weeks...

> 
> Yours,
> Linus Walleij