* Re: Pinconf issues on AMxxx plattforms
2023-05-04 5:35 ` Pinconf issues on AMxxx plattforms Tony Lindgren
@ 2023-05-04 8:31 ` Linus Walleij
2023-05-05 8:54 ` Tony Lindgren
2023-05-05 13:28 ` Niedermayr, BENEDIKT
2023-05-05 12:59 ` Niedermayr, BENEDIKT
1 sibling, 2 replies; 5+ messages in thread
From: Linus Walleij @ 2023-05-04 8:31 UTC (permalink / raw)
To: Tony Lindgren
Cc: Niedermayr, BENEDIKT, linux-omap@vger.kernel.org,
haojian.zhuang@linaro.org, linux-gpio
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Pinconf issues on AMxxx plattforms
2023-05-04 5:35 ` Pinconf issues on AMxxx plattforms Tony Lindgren
2023-05-04 8:31 ` Linus Walleij
@ 2023-05-05 12:59 ` Niedermayr, BENEDIKT
1 sibling, 0 replies; 5+ messages in thread
From: Niedermayr, BENEDIKT @ 2023-05-05 12:59 UTC (permalink / raw)
To: tony@atomide.com
Cc: haojian.zhuang@linaro.org, linux-gpio@vger.kernel.org,
linux-omap@vger.kernel.org, linus.walleij@linaro.org
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
^ permalink raw reply related [flat|nested] 5+ messages in thread