| Message ID | 20220818101839.28860-1-billy_tsai@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: aspeed: Force to disable the function's signal | expand |
Hi Billy, On Thu, 18 Aug 2022, at 19:48, Billy Tsai wrote: > When the driver want to disable the signal of the function, it doesn't > need to query the state of the mux function's signal on a pin. The > condition below will miss the disable of the signal: > Ball | Default | P0 Signal | P0 Expression | Other > -----+---------+-----------+-----------------------------+---------- > E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 > -----+---------+-----------+-----------------------------+---------- > B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 > -----+---------+-----------+-----------------------------+---------- > Assume the register status like below: > SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 > After the driver set the Ball E21 to the GPIOG0: > SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 > When the driver want to set the Ball B22 to the GPIOG1, the condition of > the SD2CMD will be false causing SCU4B4[17] not to be cleared. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c > b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > index 83d47ff1cea8..a30912a92f05 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > @@ -92,19 +92,10 @@ static int aspeed_sig_expr_enable(struct > aspeed_pinmux_data *ctx, > static int aspeed_sig_expr_disable(struct aspeed_pinmux_data *ctx, > const struct aspeed_sig_expr *expr) > { > - int ret; > - > pr_debug("Disabling signal %s for %s\n", expr->signal, > expr->function); > > - ret = aspeed_sig_expr_eval(ctx, expr, true); > - if (ret < 0) > - return ret; > - > - if (ret) > - return aspeed_sig_expr_set(ctx, expr, false); > - > - return 0; > + return aspeed_sig_expr_set(ctx, expr, false); Okay, maybe I was short-circuiting things in a way that wasn't quite right. However, I'm a little nervous that we'll end up whacking state that we can't restore and give ourselves mux-request ordering problems. The Aspeed pin controllers are such a complex sea of state. Hopefully we get away without needing to fix the theory behind the driver's implementation. This code is common to the 2400, 2500 and 2600, have you tested the patch on platforms for each to get coverage for the various pin state expressions we have? I also wonder if we can write kunit tests to build some confidence with the expected SCU bit state patterns for a given set of desired mux states. Is this something you've looked at (it would be handy if kunit can intercept regmap accesses)? Andrew
Hi Andrew, On 2022/8/19, 8:40 AM, "Andrew Jeffery" <andrew@aj.id.au> wrote: > Hi Billy, On Thu, 18 Aug 2022, at 19:48, Billy Tsai wrote: > > When the driver want to disable the signal of the function, it doesn't > > need to query the state of the mux function's signal on a pin. The > > condition below will miss the disable of the signal: > > Ball | Default | P0 Signal | P0 Expression | Other > > -----+---------+-----------+-----------------------------+---------- > > E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 > > -----+---------+-----------+-----------------------------+---------- > > B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 > > -----+---------+-----------+-----------------------------+---------- > > Assume the register status like below: > > SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 > > After the driver set the Ball E21 to the GPIOG0: > > SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 > > When the driver want to set the Ball B22 to the GPIOG1, the condition of > > the SD2CMD will be false causing SCU4B4[17] not to be cleared. > > > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > > --- > > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 11 +---------- > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c > > b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > > index 83d47ff1cea8..a30912a92f05 100644 > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > > @@ -92,19 +92,10 @@ static int aspeed_sig_expr_enable(struct > > aspeed_pinmux_data *ctx, > > static int aspeed_sig_expr_disable(struct aspeed_pinmux_data *ctx, > > const struct aspeed_sig_expr *expr) > > { > > - int ret; > > - > > pr_debug("Disabling signal %s for %s\n", expr->signal, > > expr->function); > > > > - ret = aspeed_sig_expr_eval(ctx, expr, true); > > - if (ret < 0) > > - return ret; > > - > > - if (ret) > > - return aspeed_sig_expr_set(ctx, expr, false); > > - > > - return 0; > > + return aspeed_sig_expr_set(ctx, expr, false); > Okay, maybe I was short-circuiting things in a way that wasn't quite > right. However, I'm a little nervous that we'll end up whacking state > that we can't restore and give ourselves mux-request ordering problems. > The Aspeed pin controllers are such a complex sea of state. Hopefully > we get away without needing to fix the theory behind the driver's > implementation. > This code is common to the 2400, 2500 and 2600, have you tested the > patch on platforms for each to get coverage for the various pin state > expressions we have? I think that we just need to make sure that the logic of the driver is the same as the Aspeed Datasheet table 5.1 => Revert all settings of the higher priority function and apply the the setting of the current function, then the pinmux will belong to that function. I have confirmed the design logic with our designer and it can adapt to 2400 and 2500. This concept also covers the original driver logic which invalidates the condition of the higher priority function. > I also wonder if we can write kunit tests to build some confidence with > the expected SCU bit state patterns for a given set of desired mux > states. Is this something you've looked at (it would be handy if kunit > can intercept regmap accesses)? I didn't look at it. Billy > Andrew
On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: > When the driver want to disable the signal of the function, it doesn't > need to query the state of the mux function's signal on a pin. The > condition below will miss the disable of the signal: > Ball | Default | P0 Signal | P0 Expression | Other > -----+---------+-----------+-----------------------------+---------- > E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 > -----+---------+-----------+-----------------------------+---------- > B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 > -----+---------+-----------+-----------------------------+---------- > Assume the register status like below: > SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 > After the driver set the Ball E21 to the GPIOG0: > SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 > When the driver want to set the Ball B22 to the GPIOG1, the condition of > the SD2CMD will be false causing SCU4B4[17] not to be cleared. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> I can't see the verdict for this patch? Will there be a new version, or are we in the middle of a discussion? I'd really like Andrew's ACK on the result before merging. Yours, Linus Walleij
On Sat, 27 Aug 2022, at 07:26, Linus Walleij wrote: > On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: > >> When the driver want to disable the signal of the function, it doesn't >> need to query the state of the mux function's signal on a pin. The >> condition below will miss the disable of the signal: >> Ball | Default | P0 Signal | P0 Expression | Other >> -----+---------+-----------+-----------------------------+---------- >> E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 >> -----+---------+-----------+-----------------------------+---------- >> B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 >> -----+---------+-----------+-----------------------------+---------- >> Assume the register status like below: >> SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 >> After the driver set the Ball E21 to the GPIOG0: >> SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 >> When the driver want to set the Ball B22 to the GPIOG1, the condition of >> the SD2CMD will be false causing SCU4B4[17] not to be cleared. >> >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > > I can't see the verdict for this patch? Will there be a new > version, or are we in the middle of a discussion? > I'd really like Andrew's ACK on the result before merging. Apologies, it's been a bit of A Week :) Given the approach has been discussed with the IP designer and solves a bug I'm okay for it to be merged. If we run into issues it is easy enough to back it out. Acked-by: Andrew Jeffery <andrew@aj.id.au> > > Yours, > Linus Walleij
On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: > When the driver want to disable the signal of the function, it doesn't > need to query the state of the mux function's signal on a pin. The > condition below will miss the disable of the signal: > Ball | Default | P0 Signal | P0 Expression | Other > -----+---------+-----------+-----------------------------+---------- > E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 > -----+---------+-----------+-----------------------------+---------- > B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 > -----+---------+-----------+-----------------------------+---------- > Assume the register status like below: > SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 > After the driver set the Ball E21 to the GPIOG0: > SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 > When the driver want to set the Ball B22 to the GPIOG1, the condition of > the SD2CMD will be false causing SCU4B4[17] not to be cleared. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> Patch applied! Yours, Linus Walleij
On Fri, 26 Aug 2022 at 22:48, Andrew Jeffery <andrew@aj.id.au> wrote: > On Sat, 27 Aug 2022, at 07:26, Linus Walleij wrote: > > On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: > > > >> When the driver want to disable the signal of the function, it doesn't > >> need to query the state of the mux function's signal on a pin. The > >> condition below will miss the disable of the signal: > > I can't see the verdict for this patch? Will there be a new > > version, or are we in the middle of a discussion? > > I'd really like Andrew's ACK on the result before merging. > > Apologies, it's been a bit of A Week :) > > Given the approach has been discussed with the IP designer and solves a bug I'm okay for it to be merged. If we run into issues it is easy enough to back it out. As foreseen by Andrew, this caused a regression. On the Romulus machine the device tree contains a gpio hog for GPIO S7. With the patch applied: [ 0.384796] aspeed-g5-pinctrl 1e6e2080.pinctrl: request pin 151 (AA20) for 1e780000.gpio:943 [ 0.385009] Muxing pin 151 for GPIO [ 0.385081] Disabling signal VPOB9 for VPO [ 0.402291] aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 [ 0.402521] aspeed-g5-pinctrl 1e6e2080.pinctrl: request() failed for pin 151 The code path is aspeed-gpio -> pinmux-g5 -> regmap -> clk, and the of_clock code returns an error as it doesn't have a valid struct clk_hw pointer. The regmap call happens because pinmux wants to check the GFX node (IP block 1) to query bits there. For reference, reverting the patch gives us this trace: [ 0.393160] Muxing pin 151 for GPIO [ 0.393267] Disabling signal VPOB9 for VPO [ 0.393383] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 [ 0.393552] Disabling signal VPOB9 for VPOOFF1 [ 0.393681] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 [ 0.393835] Disabling signal VPOB9 for VPOOFF2 [ 0.393965] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 [ 0.394097] Enabling signal GPIOS7 for GPIOS7 [ 0.394217] Muxed pin 151 as GPIOS7 [ 0.394411] gpio-943 (seq_cont): hogged as output/low This can be reproduced in qemu without userspace: qemu-system-arm -M romulus-bmc -nographic -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb -no-reboot Billy, do you have any suggestions? Cheers, Joel
[TLDR: I'm adding this report to the list of tracked Linux kernel regressions; the text you find below is based on a few templates paragraphs you might have encountered already in similar form. See link in footer if these mails annoy you.] [CCing the regression list, as it should be in the loop for regressions: https://docs.kernel.org/admin-guide/reporting-regressions.html] On 19.01.23 02:54, Joel Stanley wrote: > On Fri, 26 Aug 2022 at 22:48, Andrew Jeffery <andrew@aj.id.au> wrote: >> On Sat, 27 Aug 2022, at 07:26, Linus Walleij wrote: >>> On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: >>> >>>> When the driver want to disable the signal of the function, it doesn't >>>> need to query the state of the mux function's signal on a pin. The >>>> condition below will miss the disable of the signal: > >>> I can't see the verdict for this patch? Will there be a new >>> version, or are we in the middle of a discussion? >>> I'd really like Andrew's ACK on the result before merging. >> >> Apologies, it's been a bit of A Week :) >> >> Given the approach has been discussed with the IP designer and solves a bug I'm okay for it to be merged. If we run into issues it is easy enough to back it out. > > As foreseen by Andrew, this caused a regression. On the Romulus > machine the device tree contains a gpio hog for GPIO S7. With the > patch applied: > > [ 0.384796] aspeed-g5-pinctrl 1e6e2080.pinctrl: request pin 151 > (AA20) for 1e780000.gpio:943 > [ 0.385009] Muxing pin 151 for GPIO > [ 0.385081] Disabling signal VPOB9 for VPO > [ 0.402291] aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire > regmap for IP block 1 > [ 0.402521] aspeed-g5-pinctrl 1e6e2080.pinctrl: request() failed for pin 151 > > The code path is aspeed-gpio -> pinmux-g5 -> regmap -> clk, and the > of_clock code returns an error as it doesn't have a valid struct > clk_hw pointer. The regmap call happens because pinmux wants to check > the GFX node (IP block 1) to query bits there. > > For reference, reverting the patch gives us this trace: > > [ 0.393160] Muxing pin 151 for GPIO > [ 0.393267] Disabling signal VPOB9 for VPO > [ 0.393383] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > [ 0.393552] Disabling signal VPOB9 for VPOOFF1 > [ 0.393681] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > [ 0.393835] Disabling signal VPOB9 for VPOOFF2 > [ 0.393965] Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > [ 0.394097] Enabling signal GPIOS7 for GPIOS7 > [ 0.394217] Muxed pin 151 as GPIOS7 > [ 0.394411] gpio-943 (seq_cont): hogged as output/low > > This can be reproduced in qemu without userspace: > > qemu-system-arm -M romulus-bmc -nographic -kernel arch/arm/boot/zImage > -dtb arch/arm/boot/dts/aspeed-bmc-opp-romulus.dtb -no-reboot > > Billy, do you have any suggestions? Thanks for the report. To be sure the issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression tracking bot: #regzbot ^introduced cf517fef601b #regzbot title pinctrl: aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 #regzbot ignore-activity This isn't a regression? This issue or a fix for it are already discussed somewhere else? It was fixed already? You want to clarify when the regression started to happen? Or point out I got the title or something else totally wrong? Then just reply and tell me -- ideally while also telling regzbot about it, as explained by the page listed in the footer of this mail. Developers: When fixing the issue, remember to add 'Link:' tags pointing to the report (the parent of this mail). See page linked in footer for details. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
On Thu, Jan 19, 2023 at 2:54 AM Joel Stanley <joel@jms.id.au> wrote: > As foreseen by Andrew, this caused a regression. On the Romulus > machine the device tree contains a gpio hog for GPIO S7. With the > patch applied: OK shall I just revert the patch? Yours, Linus Walleij
On Fri, 27 Jan 2023 at 12:39, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Jan 19, 2023 at 2:54 AM Joel Stanley <joel@jms.id.au> wrote: > > > As foreseen by Andrew, this caused a regression. On the Romulus > > machine the device tree contains a gpio hog for GPIO S7. With the > > patch applied: > > OK shall I just revert the patch? Yep! I was going to send a revert but I thought I should write up a commit message. If you're happy just putting a revert in with a note that it caused a regression that's enough for me.
On Mon, 30 Jan 2023, at 13:36, Joel Stanley wrote: > On Fri, 27 Jan 2023 at 12:39, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> On Thu, Jan 19, 2023 at 2:54 AM Joel Stanley <joel@jms.id.au> wrote: >> >> > As foreseen by Andrew, this caused a regression. On the Romulus >> > machine the device tree contains a gpio hog for GPIO S7. With the >> > patch applied: >> >> OK shall I just revert the patch? > > Yep! I was going to send a revert but I thought I should write up a > commit message. If you're happy just putting a revert in with a note > that it caused a regression that's enough for me. Agree, let's revert this one for now. Andrew
[TLDR: This mail in primarily relevant for Linux regression tracking. A change or fix related to the regression discussed in this thread was posted or applied, but it did not use a Link: tag to point to the report, as Linus and the documentation call for. Things happen, no worries -- but now the regression tracking bot needs to be told manually about the fix. See link in footer if these mails annoy you.] On 21.01.23 13:32, Linux kernel regression tracking (#adding) wrote: > On 19.01.23 02:54, Joel Stanley wrote: >> On Fri, 26 Aug 2022 at 22:48, Andrew Jeffery <andrew@aj.id.au> wrote: >>> On Sat, 27 Aug 2022, at 07:26, Linus Walleij wrote: >>>> On Thu, Aug 18, 2022 at 12:18 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote: >>>> >>>>> When the driver want to disable the signal of the function, it doesn't >>>>> need to query the state of the mux function's signal on a pin. The >>>>> condition below will miss the disable of the signal: >> >>>> I can't see the verdict for this patch? Will there be a new >>>> version, or are we in the middle of a discussion? >>>> I'd really like Andrew's ACK on the result before merging. >>> >>> Apologies, it's been a bit of A Week :) >>> >>> Given the approach has been discussed with the IP designer and solves a bug I'm okay for it to be merged. If we run into issues it is easy enough to back it out. >> >> As foreseen by Andrew, this caused a regression. On the Romulus >> machine the device tree contains a gpio hog for GPIO S7. With the >> patch applied: > > #regzbot ^introduced cf517fef601b > #regzbot title pinctrl: aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to > acquire regmap for IP block 1 > #regzbot ignore-activity #regzbot fix: 606d4ef4922662 Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you. #regzbot ignore-activity
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c index 83d47ff1cea8..a30912a92f05 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c @@ -92,19 +92,10 @@ static int aspeed_sig_expr_enable(struct aspeed_pinmux_data *ctx, static int aspeed_sig_expr_disable(struct aspeed_pinmux_data *ctx, const struct aspeed_sig_expr *expr) { - int ret; - pr_debug("Disabling signal %s for %s\n", expr->signal, expr->function); - ret = aspeed_sig_expr_eval(ctx, expr, true); - if (ret < 0) - return ret; - - if (ret) - return aspeed_sig_expr_set(ctx, expr, false); - - return 0; + return aspeed_sig_expr_set(ctx, expr, false); } /**
When the driver want to disable the signal of the function, it doesn't need to query the state of the mux function's signal on a pin. The condition below will miss the disable of the signal: Ball | Default | P0 Signal | P0 Expression | Other -----+---------+-----------+-----------------------------+---------- E21 GPIOG0 SD2CLK SCU4B4[16]=1 & SCU450[1]=1 GPIOG0 -----+---------+-----------+-----------------------------+---------- B22 GPIOG1 SD2CMD SCU4B4[17]=1 & SCU450[1]=1 GPIOG1 -----+---------+-----------+-----------------------------+---------- Assume the register status like below: SCU4B4[16] == 1 & SCU4B4[17] == 1 & SCU450[1]==1 After the driver set the Ball E21 to the GPIOG0: SCU4B4[16] == 0 & SCU4B4[17] == 1 & SCU450[1]==0 When the driver want to set the Ball B22 to the GPIOG1, the condition of the SD2CMD will be false causing SCU4B4[17] not to be cleared. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- drivers/pinctrl/aspeed/pinctrl-aspeed.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)