| Message ID | 20230119235501.53294-1-joel@jms.id.au |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pinctrl: aspeed-g5: Bypass clock check when fetching regmap | expand |
On Fri, 20 Jan 2023, at 10:25, Joel Stanley wrote: > A recent commit cf517fef601b ("pinctrl: aspeed: Force to disable the > function's signal") exposed a problem with fetching the regmap for > reading the GFX register. > > The Romulus machine the device tree contains a gpio hog for GPIO S7. > With the patch applied: > > Muxing pin 151 for GPIO > Disabling signal VPOB9 for VPO > aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 > 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, before the offending patch: > > Muxing pin 151 for GPIO > Disabling signal VPOB9 for VPO > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > Disabling signal VPOB9 for VPOOFF1 > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > Disabling signal VPOB9 for VPOOFF2 > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > Enabling signal GPIOS7 for GPIOS7 > Muxed pin 151 as GPIOS7 > gpio-943 (seq_cont): hogged as output/low > > As a workaround, skip the clock check to allow pinmux to proceed. We'd want the clock on and and the device out of reset before this makes sense though. We're just assuming the IP is in an operational state? Was this just accidentally working because reading the register in a bad state is producing 0 instead of other undefined garbage? Andrew
On Fri, Jan 20, 2023 at 3:35 AM Andrew Jeffery <andrew@aj.id.au> wrote: > On Fri, 20 Jan 2023, at 10:25, Joel Stanley wrote: > > A recent commit cf517fef601b ("pinctrl: aspeed: Force to disable the > > function's signal") exposed a problem with fetching the regmap for > > reading the GFX register. > > > > The Romulus machine the device tree contains a gpio hog for GPIO S7. > > With the patch applied: > > > > Muxing pin 151 for GPIO > > Disabling signal VPOB9 for VPO > > aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 > > 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, before the offending patch: > > > > Muxing pin 151 for GPIO > > Disabling signal VPOB9 for VPO > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > Disabling signal VPOB9 for VPOOFF1 > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > Disabling signal VPOB9 for VPOOFF2 > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > Enabling signal GPIOS7 for GPIOS7 > > Muxed pin 151 as GPIOS7 > > gpio-943 (seq_cont): hogged as output/low > > > > As a workaround, skip the clock check to allow pinmux to proceed. > > We'd want the clock on and and the device out of reset before this > makes sense though. We're just assuming the IP is in an operational > state? Was this just accidentally working because reading the register > in a bad state is producing 0 instead of other undefined garbage? This makes sense, can we come up with a resonable patch for this problem or should this one be merged as an intermediate remedy? Yours, Linus Walleij
On Fri, 27 Jan 2023 at 12:36, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Jan 20, 2023 at 3:35 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > On Fri, 20 Jan 2023, at 10:25, Joel Stanley wrote: > > > > A recent commit cf517fef601b ("pinctrl: aspeed: Force to disable the > > > function's signal") exposed a problem with fetching the regmap for > > > reading the GFX register. > > > > > > The Romulus machine the device tree contains a gpio hog for GPIO S7. > > > With the patch applied: > > > > > > Muxing pin 151 for GPIO > > > Disabling signal VPOB9 for VPO > > > aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 > > > 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, before the offending patch: > > > > > > Muxing pin 151 for GPIO > > > Disabling signal VPOB9 for VPO > > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > > Disabling signal VPOB9 for VPOOFF1 > > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > > Disabling signal VPOB9 for VPOOFF2 > > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 > > > Enabling signal GPIOS7 for GPIOS7 > > > Muxed pin 151 as GPIOS7 > > > gpio-943 (seq_cont): hogged as output/low > > > > > > As a workaround, skip the clock check to allow pinmux to proceed. > > > > We'd want the clock on and and the device out of reset before this > > makes sense though. We're just assuming the IP is in an operational > > state? Was this just accidentally working because reading the register > > in a bad state is producing 0 instead of other undefined garbage? > > This makes sense, can we come up with a resonable patch for this > problem or should this one be merged as an intermediate remedy? Andrew is correct, my testing showed that we do need to take the device out of reset in order to write a value that sticks. Qemu is insufficient for testing this as it doesn't model the reset lines. We really don't want to enable the clocks just to set a value that doesn't need to be set. There's a big nasty delay in the clock driver. I suggest we revert the problematic patch until we can come up with a better solution. Cheers, Joel
On Mon, 30 Jan 2023, at 13:29, Joel Stanley wrote: > On Fri, 27 Jan 2023 at 12:36, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> On Fri, Jan 20, 2023 at 3:35 AM Andrew Jeffery <andrew@aj.id.au> wrote: >> > On Fri, 20 Jan 2023, at 10:25, Joel Stanley wrote: >> >> > > A recent commit cf517fef601b ("pinctrl: aspeed: Force to disable the >> > > function's signal") exposed a problem with fetching the regmap for >> > > reading the GFX register. >> > > >> > > The Romulus machine the device tree contains a gpio hog for GPIO S7. >> > > With the patch applied: >> > > >> > > Muxing pin 151 for GPIO >> > > Disabling signal VPOB9 for VPO >> > > aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 >> > > 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, before the offending patch: >> > > >> > > Muxing pin 151 for GPIO >> > > Disabling signal VPOB9 for VPO >> > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 >> > > Disabling signal VPOB9 for VPOOFF1 >> > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 >> > > Disabling signal VPOB9 for VPOOFF2 >> > > Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 >> > > Enabling signal GPIOS7 for GPIOS7 >> > > Muxed pin 151 as GPIOS7 >> > > gpio-943 (seq_cont): hogged as output/low >> > > >> > > As a workaround, skip the clock check to allow pinmux to proceed. >> > >> > We'd want the clock on and and the device out of reset before this >> > makes sense though. We're just assuming the IP is in an operational >> > state? Was this just accidentally working because reading the register >> > in a bad state is producing 0 instead of other undefined garbage? >> >> This makes sense, can we come up with a resonable patch for this >> problem or should this one be merged as an intermediate remedy? > > Andrew is correct, my testing showed that we do need to take the > device out of reset in order to write a value that sticks. Qemu is > insufficient for testing this as it doesn't model the reset lines. > > We really don't want to enable the clocks just to set a value that > doesn't need to be set. There's a big nasty delay in the clock driver. > > I suggest we revert the problematic patch until we can come up with a > better solution. Yeah, based on the testing I've just done the GFX logic is always clocked (D1CLK might be an output clock?), it's the reset that matters. I applied Joel's patch and did some testing using the romulus-bmc machine in qemu and we still see some failures to request some GPIOs. Given that Joel's patch doesn't completely resolve the regression I agree we revert Billy's patch for now, and I'll reconsider my life choices^W^W^W^W think about how to deal with Billy's problem another way. Cheers, Andrew
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c index 7ee0c473ad70..f714fe40e400 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c @@ -2635,7 +2635,7 @@ static struct regmap *aspeed_g5_acquire_regmap(struct aspeed_pinmux_data *ctx, node = of_parse_phandle(ctx->dev->of_node, "aspeed,external-nodes", 0); if (node) { - map = syscon_node_to_regmap(node); + map = device_node_to_regmap(node); of_node_put(node); if (IS_ERR(map)) return map;
A recent commit cf517fef601b ("pinctrl: aspeed: Force to disable the function's signal") exposed a problem with fetching the regmap for reading the GFX register. The Romulus machine the device tree contains a gpio hog for GPIO S7. With the patch applied: Muxing pin 151 for GPIO Disabling signal VPOB9 for VPO aspeed-g5-pinctrl 1e6e2080.pinctrl: Failed to acquire regmap for IP block 1 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, before the offending patch: Muxing pin 151 for GPIO Disabling signal VPOB9 for VPO Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 Disabling signal VPOB9 for VPOOFF1 Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 Disabling signal VPOB9 for VPOOFF2 Want SCU8C[0x00000080]=0x1, got 0x0 from 0x00000000 Enabling signal GPIOS7 for GPIOS7 Muxed pin 151 as GPIOS7 gpio-943 (seq_cont): hogged as output/low As a workaround, skip the clock check to allow pinmux to proceed. Fixes: cf517fef601b ("pinctrl: aspeed: Force to disable the function's signal") Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)