| Message ID | 20221114202943.2389489-1-bmasney@redhat.com |
|---|---|
| State | Accepted |
| Commit | 24c94060fc9b4e0f19e6e018869db46db21d6bc7 |
| Headers | show |
| Series | [RFC] gpiolib: ensure that fwnode is properly set | expand |
On 14. 11. 2022. 21:29, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. Hi, the following patch should fix it for you, I have hit the same issue on IPQ8074. https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/ Regards, Robert > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) {
On Mon, Nov 14, 2022 at 10:02:11PM +0100, Robert Marko wrote: > Hi, the following patch should fix it for you, I have hit the same issue on > IPQ8074. > > https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/ That fixed the issue. Thanks, Robert. Brian
On 11/15/2022 2:32 AM, Robert Marko wrote: > > On 14. 11. 2022. 21:29, Brian Masney wrote: >> Note that this is a RFC patch and not meant to be merged. I looked into >> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive >> board (sc8280xp) where the UFS host controller would fail to probe due >> to repeated probe deferrals when trying to get reset-gpios via >> devm_gpiod_get_optional(). >> >> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by >> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate >> function >> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The >> pinctrl driver doesn't define one, so of_gpiochip_add() should >> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't >> happen since the fwnode member on the struct gpiochip is set to null >> when of_gpiochip_add() is called. Let's work around this by ensuring >> that it's set if available. >> >> Note that this broke sometime within the last few weeks within >> linux-next and I haven't bisected this. I'm posting this in the hopes >> that someone may know offhand which patch(es) may have broken this. > > Hi, the following patch should fix it for you, I have hit the same issue on > IPQ8074. > > https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/ > This fixes the issue I was facing for sa8540p (sc8280xp). Thanks Robert. Shazad > Regards, > Robert > >> >> Signed-off-by: Brian Masney <bmasney@redhat.com> >> --- >> drivers/gpio/gpiolib.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index 11fb7ec883e9..8bec66008869 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip >> *gc, void *data, >> * Assign fwnode depending on the result of the previous calls, >> * if none of them succeed, assign it to the parent's one. >> */ >> - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; >> + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; >> gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); >> if (gdev->id < 0) {
On 2022-11-14 15:29:43, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. > > Signed-off-by: Brian Masney <bmasney@redhat.com> Thanks, this fixes the following abort I'm observing on multiple Qualcomm platforms (sdm630, Sony Nile, and msm8956, Sony Loire): [ 0.391439] Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP [ 0.391526] Modules linked in: [ 0.398678] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc5-next-20221114-07647-gb3ed2836a8f7 #38 [ 0.401642] Hardware name: Sony Xperia XA2 Ultra (DT) [ 0.410879] pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 0.415957] pc : msm_gpio_get_direction+0x40/0x80 [ 0.422680] lr : msm_gpio_get_direction+0x18/0x80 [ 0.427544] sp : ffff80000805b750 [ 0.432270] x29: ffff80000805b750 x28: ffff739180b48c00 x27: ffff739180b50180 [ 0.435537] x26: 0000000000000008 x25: 0000000000000002 x24: ffffdcf976192e98 [ 0.442695] x23: ffff739180b31890 x22: 0000000000000000 x21: ffff739180b48c08 [ 0.449814] x20: ffffdcf975ff4148 x19: 0000000000000008 x18: 0000000000000000 [ 0.456891] x17: 64656c62616e655f x16: 7469647561206465 x15: 0000000000000002 [ 0.464049] x14: 0000000000000001 x13: 006c7274636e6970 x12: 2e30303030303133 [ 0.471166] x11: ffffdcf9760c98d8 x10: 000000000042c70 x9 : 0000000000000004 [ 0.478243] x8 : 0101010101010101 x7 : 0073656d616e2d65 x6 : 0911040aadece9ee [ 0.485405] x5 : 6e696c2d0a041109 x4 : 0000000000000180 x3 : 0000000000008000 [ 0.492483] x2 : 0000000000000000 x1 : ffffdcf975628db0 x0 : ffff800008808000 [ 0.499642] Call trace: [ 0.506703] msm_gpio_get_direction+0x40/0x80 [ 0.509008] gpiochip_add_data_with_key+0x638/0xed0 [ 0.513481] msm_pinctrl_probe+0x430/0x580 [ 0.518168] sdm660_pinctrl_probe+0x18/0x30 [ 0.522376] platform_probe+0x68/0xc0 [ 0.526413] really_probe+0xc0/0x3dc [ 0.530234] __driver_probe_device+0x7c/0x190 [ 0.533922] driver_probe_device+0x3c/0x110 [ 0.538132] __device_attach_driver+0xbc/0x160 [ 0.542168] bus_for_each_drv+0x7c/0xd4 [ 0.546637] __device_attach+0x9c/0x1c0 [ 0.550370] device_initial_probe+0x14/0x20 [ 0.554231] bus_probe_device+0x9c/0xa4 [ 0.558358] device_add+0x3ac/0x8b0 [ 0.562175] of_device_add+0x54/0x64 [ 0.565689] of_platform_device_create_pdata+0x90/0x124 [ 0.569470] of_platform_bus_create+0x18c/0x510 [ 0.574416] of_platform_bus_create+0x1ec/0x510 [ 0.578971] of_platform_populate+0x60/0x150 [ 0.583445] of_platform_default_populate_init+0xe4/0x104 [ 0.588002] do_one_initcall+0x64/0x1dc [ 0.593253] kernel_init_freeable+0x1b8/0x220 [ 0.596900] kernel_init+0x24/0x130 [ 0.601453] ret_from_fork+0x10/0x20 [ 0.604715] Code: d37d0442 8b020000 f941b400 8b030000 (b9400000) [ 0.608583] ---[ end trace 0000000000000000 ]--- [ 0.619572] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b This wasn't an issue on -next 20221109. Tested-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) { > -- > 2.38.1 >
On 14/11/2022 21:29, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- Fixes boot on 8450 on next-20221115 Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) {
On 14/11/2022 20:29, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) { Fixes repeated failure to get GPIO despite the controller having successfully probed. [ 6.151075] ov9282 21-0060: failed to get reset gpio -517 [ 6.160599] ov9282 21-0060: HW configuration is not supported [ 6.160970] hub 1-1:1.0: USB hub found [ 6.168054] imx412 22-001a: failed to get reset gpio -517 [ 6.170710] hub 1-1:1.0: 4 ports detected [ 6.175904] imx412 22-001a: HW configuration is not supported
Hi,
Others in the thread pointed to Thierry's patch, but on the Lenovo
Thinkpad X13s, that patch did *not* fix the issue, and with it
applied, the X13s still immediately reboots as soon as exiting EFI
services. With this patch applied, the X13s does *not* reboot after
exiting EFI services, so thank you for this patch.
Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s
On 14/11/2022 21:29, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) { Fixes boot on 8550 on next-20221115 Tested-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks, Neil
On Tue, Nov 15, 2022 at 11:02 PM Steev Klimaszewski <steev@kali.org> wrote: > > Hi, > > Others in the thread pointed to Thierry's patch, but on the Lenovo > Thinkpad X13s, that patch did *not* fix the issue, and with it > applied, the X13s still immediately reboots as soon as exiting EFI > services. With this patch applied, the X13s does *not* reboot after > exiting EFI services, so thank you for this patch. > > Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s My bad, I was under the impression that Thierry's patch fixed this issue too. Now applied. Bart
On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote: > Note that this is a RFC patch and not meant to be merged. I looked into > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > board (sc8280xp) where the UFS host controller would fail to probe due > to repeated probe deferrals when trying to get reset-gpios via > devm_gpiod_get_optional(). > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > pinctrl driver doesn't define one, so of_gpiochip_add() should > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > happen since the fwnode member on the struct gpiochip is set to null > when of_gpiochip_add() is called. Let's work around this by ensuring > that it's set if available. > > Note that this broke sometime within the last few weeks within > linux-next and I haven't bisected this. I'm posting this in the hopes > that someone may know offhand which patch(es) may have broken this. > > Signed-off-by: Brian Masney <bmasney@redhat.com> > --- > drivers/gpio/gpiolib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..8bec66008869 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > * Assign fwnode depending on the result of the previous calls, > * if none of them succeed, assign it to the parent's one. > */ > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; This doesn't look right to me. Looking at the documentation of gc->fwnode and how it is used, the purpose of this is to allow explicitly overriding the fwnode that the GPIO chip will use. So really this should not be used beyond the initial registration in gpiochip_add_data_with_key(). If the above patch fixes anything, then I suspect somebody is using gc->fwnode outside of this registration. Looking at gpiolib, the only remaining place that seems to do this is the gpio-reserved-ranges handling code, in which case, the below on top of my initial patch might fix that. That might explain why MSM is still seeing issues. --- >8 --- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 11fb7ec883e9..d692ad5c5a27 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -447,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc) static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc) { + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); int size; /* Format is "start, count, ..." */ - size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges"); + size = fwnode_property_count_u32(fwnode, "gpio-reserved-ranges"); if (size > 0 && size % 2 == 0) return size; @@ -471,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc) { + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); unsigned int size; u32 *ranges; int ret; @@ -483,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc) if (!ranges) return -ENOMEM; - ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size); + ret = fwnode_property_read_u32_array(fwnode, "gpio-reserved-ranges", ranges, size); if (ret) { kfree(ranges); return ret; --- >8 --- I don't have a good idea about the Lenovo X13 issue, though, but I haven't looked at ACPI at all since I don't have any hardware to test on. Thierry
On Tue, Nov 15, 2022 at 04:02:38PM -0600, Steev Klimaszewski wrote: > Hi, > > Others in the thread pointed to Thierry's patch, but on the Lenovo > Thinkpad X13s, that patch did *not* fix the issue, and with it > applied, the X13s still immediately reboots as soon as exiting EFI > services. With this patch applied, the X13s does *not* reboot after > exiting EFI services, so thank you for this patch. > > Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s Do you happen to know what driver the X13s is using? As mentioned in another email, I don't think overriding gc->fwnode at this point is the right thing to do, so I suspect this field might be used unexpectedly in some other place, so I'd like to take a closer look at that. Thierry
On Wed, Nov 16, 2022 at 11:21:37AM +0100, Thierry Reding wrote: > On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote: > > Note that this is a RFC patch and not meant to be merged. I looked into > > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive > > board (sc8280xp) where the UFS host controller would fail to probe due > > to repeated probe deferrals when trying to get reset-gpios via > > devm_gpiod_get_optional(). > > > > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by > > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function > > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The > > pinctrl driver doesn't define one, so of_gpiochip_add() should > > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't > > happen since the fwnode member on the struct gpiochip is set to null > > when of_gpiochip_add() is called. Let's work around this by ensuring > > that it's set if available. > > > > Note that this broke sometime within the last few weeks within > > linux-next and I haven't bisected this. I'm posting this in the hopes > > that someone may know offhand which patch(es) may have broken this. > > > > Signed-off-by: Brian Masney <bmasney@redhat.com> > > --- > > drivers/gpio/gpiolib.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 11fb7ec883e9..8bec66008869 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > > * Assign fwnode depending on the result of the previous calls, > > * if none of them succeed, assign it to the parent's one. > > */ > > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > This doesn't look right to me. Looking at the documentation of > gc->fwnode and how it is used, the purpose of this is to allow > explicitly overriding the fwnode that the GPIO chip will use. > > So really this should not be used beyond the initial registration > in gpiochip_add_data_with_key(). If the above patch fixes anything, > then I suspect somebody is using gc->fwnode outside of this > registration. > > Looking at gpiolib, the only remaining place that seems to do this is > the gpio-reserved-ranges handling code, in which case, the below on top > of my initial patch might fix that. That might explain why MSM is still > seeing issues. > > --- >8 --- > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 11fb7ec883e9..d692ad5c5a27 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -447,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc) > > static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc) > { > + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); > int size; > > /* Format is "start, count, ..." */ > - size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges"); > + size = fwnode_property_count_u32(fwnode, "gpio-reserved-ranges"); > if (size > 0 && size % 2 == 0) > return size; > > @@ -471,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc) > > static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc) > { > + struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev); > unsigned int size; > u32 *ranges; > int ret; > @@ -483,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc) > if (!ranges) > return -ENOMEM; > > - ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size); > + ret = fwnode_property_read_u32_array(fwnode, "gpio-reserved-ranges", ranges, size); > if (ret) { > kfree(ranges); > return ret; > --- >8 --- > > I don't have a good idea about the Lenovo X13 issue, though, but I > haven't looked at ACPI at all since I don't have any hardware to test > on. Ah... looks like that device was actually a Thinkpad X13*s*, which is based on a Qualcomm chip, so maybe this patch fixes that one, too. It does use gpio-reserved-ranges, so seems at least likely. Thierry
On Wed, Nov 16, 2022 at 11:21:37AM +0100, Thierry Reding wrote: > On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote: > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 11fb7ec883e9..8bec66008869 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > > * Assign fwnode depending on the result of the previous calls, > > * if none of them succeed, assign it to the parent's one. > > */ > > - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > This doesn't look right to me. Looking at the documentation of > gc->fwnode and how it is used, the purpose of this is to allow > explicitly overriding the fwnode that the GPIO chip will use. > > So really this should not be used beyond the initial registration > in gpiochip_add_data_with_key(). If the above patch fixes anything, > then I suspect somebody is using gc->fwnode outside of this > registration. > > Looking at gpiolib, the only remaining place that seems to do this is > the gpio-reserved-ranges handling code, in which case, the below on top > of my initial patch might fix that. That might explain why MSM is still > seeing issues. That is correct. The Thinkpad x13s laptop uses the driver drivers/pinctrl/qcom/pinctrl-sc8280xp.c and arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts defines a gpio-reserved-ranges property. The SA8540p automotive board has the same SoC, however the DTS we are using doesn't use the gpio-reserved-ranges property, and why only your original patch fixed the issue for this board. I think my patch should be removed from the GPIO tree if Thierry's two patches work for everyone. Brian
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 11fb7ec883e9..8bec66008869 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, * Assign fwnode depending on the result of the previous calls, * if none of them succeed, assign it to the parent's one. */ - gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; + gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); if (gdev->id < 0) {
Note that this is a RFC patch and not meant to be merged. I looked into a problem with linux-next-20221110 on the Qualcomm SA8540P automotive board (sc8280xp) where the UFS host controller would fail to probe due to repeated probe deferrals when trying to get reset-gpios via devm_gpiod_get_optional(). of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The pinctrl driver doesn't define one, so of_gpiochip_add() should automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't happen since the fwnode member on the struct gpiochip is set to null when of_gpiochip_add() is called. Let's work around this by ensuring that it's set if available. Note that this broke sometime within the last few weeks within linux-next and I haven't bisected this. I'm posting this in the hopes that someone may know offhand which patch(es) may have broken this. Signed-off-by: Brian Masney <bmasney@redhat.com> --- drivers/gpio/gpiolib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)