Re: Pinconf issues on AMxxx plattforms

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