* [PATCH 0/2] ARM: stm32: add clock for pwrcfg syscon
@ 2023-10-02 18:08 Ben Wolsieffer
2023-10-02 18:08 ` [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered Ben Wolsieffer
2023-10-02 18:08 ` [PATCH 2/2] ARM: dts: stm32: add pwrcfg clock for stm32f4/7 Ben Wolsieffer
0 siblings, 2 replies; 6+ messages in thread
From: Ben Wolsieffer @ 2023-10-02 18:08 UTC (permalink / raw)
To: linux-stm32, linux-arm-kernel, linux-kernel, devicetree,
linux-clk
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Michael Turquette, Stephen Boyd, Ben Wolsieffer
The STM32F4/7 pwrcfg syscon was missing its clock, making it impossible
to use after clk_disable_unused(). Simply adding the clock creates a
circular dependency, because the syscon is used by the clock driver.
This series resolves this dependency and then adds the clock.
I tested this on a STM32F746 and verified that syscon writes now
succeed even after clk_disable_unused().
Ben Wolsieffer (2):
clk: stm32: initialize syscon after clocks are registered
ARM: dts: stm32: add pwrcfg clock for stm32f4/7
arch/arm/boot/dts/st/stm32f429.dtsi | 1 +
arch/arm/boot/dts/st/stm32f746.dtsi | 1 +
drivers/clk/clk-stm32f4.c | 12 ++++++------
3 files changed, 8 insertions(+), 6 deletions(-)
--
2.42.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered
2023-10-02 18:08 [PATCH 0/2] ARM: stm32: add clock for pwrcfg syscon Ben Wolsieffer
@ 2023-10-02 18:08 ` Ben Wolsieffer
2023-12-17 23:05 ` Stephen Boyd
2023-10-02 18:08 ` [PATCH 2/2] ARM: dts: stm32: add pwrcfg clock for stm32f4/7 Ben Wolsieffer
1 sibling, 1 reply; 6+ messages in thread
From: Ben Wolsieffer @ 2023-10-02 18:08 UTC (permalink / raw)
To: linux-stm32, linux-arm-kernel, linux-kernel, devicetree,
linux-clk
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Michael Turquette, Stephen Boyd, Ben Wolsieffer
The stm32-power-config syscon (PWR peripheral) is used in this driver
and the STM32 RTC driver to enable write access to backup domain
registers. The syscon's clock has a gate controlled by this clock
driver, but this clock is currently not registered in the device tree.
This only happens to work currently because all relevant clock setup and
RTC initialization happens before clk_disabled_unused(). After this
point, all syscon register writes are ignored.
If we simply add the syscon clock in the device tree, we end up with a
circular dependency because the clock has not been registered at the
point this driver requests the syscon.
This patch avoids this circular dependency by moving the syscon lookup
after the clocks are registered. This does appear to create a possible
race condition where someone could attempt to perform an operation on a
backup domain clock before the syscon has been initialized. This would
result in the operation having no effect because backup domain writes
could not be enabled. I'm not sure if this is a problem or if there is
a way to avoid it.
Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com>
---
drivers/clk/clk-stm32f4.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
index 07c13ebe327d..a88e762d2b5e 100644
--- a/drivers/clk/clk-stm32f4.c
+++ b/drivers/clk/clk-stm32f4.c
@@ -1697,12 +1697,6 @@ static void __init stm32f4_rcc_init(struct device_node *np)
return;
}
- pdrm = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
- if (IS_ERR(pdrm)) {
- pdrm = NULL;
- pr_warn("%s: Unable to get syscfg\n", __func__);
- }
-
match = of_match_node(stm32f4_of_match, np);
if (WARN_ON(!match))
return;
@@ -1894,6 +1888,12 @@ static void __init stm32f4_rcc_init(struct device_node *np)
of_clk_add_hw_provider(np, stm32f4_rcc_lookup_clk, NULL);
+ pdrm = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+ if (IS_ERR(pdrm)) {
+ pdrm = NULL;
+ pr_warn("%s: Unable to get syscfg\n", __func__);
+ }
+
return;
fail:
kfree(clks);
--
2.42.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered
2023-10-02 18:08 ` [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered Ben Wolsieffer
@ 2023-12-17 23:05 ` Stephen Boyd
2024-01-12 22:00 ` Ben Wolsieffer
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2023-12-17 23:05 UTC (permalink / raw)
To: Ben Wolsieffer, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-stm32
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Michael Turquette, Ben Wolsieffer
Quoting Ben Wolsieffer (2023-10-02 11:08:53)
> The stm32-power-config syscon (PWR peripheral) is used in this driver
> and the STM32 RTC driver to enable write access to backup domain
> registers. The syscon's clock has a gate controlled by this clock
> driver, but this clock is currently not registered in the device tree.
> This only happens to work currently because all relevant clock setup and
> RTC initialization happens before clk_disabled_unused(). After this
> point, all syscon register writes are ignored.
Seems like we should mark those clks as CLK_IGNORE_UNUSED and add a
comment to that fact.
>
> If we simply add the syscon clock in the device tree, we end up with a
> circular dependency because the clock has not been registered at the
> point this driver requests the syscon.
>
> This patch avoids this circular dependency by moving the syscon lookup
> after the clocks are registered. This does appear to create a possible
> race condition where someone could attempt to perform an operation on a
> backup domain clock before the syscon has been initialized. This would
> result in the operation having no effect because backup domain writes
> could not be enabled. I'm not sure if this is a problem or if there is
> a way to avoid it.
There's no comment in the code that says the regmap must be set there
instead of earlier. What's to stop someone from tripping over this
problem later? At the least, please add a comment.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered
2023-12-17 23:05 ` Stephen Boyd
@ 2024-01-12 22:00 ` Ben Wolsieffer
2024-04-12 6:28 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Ben Wolsieffer @ 2024-01-12 22:00 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, linux-arm-kernel, linux-clk, linux-kernel,
linux-stm32, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maxime Coquelin, Alexandre Torgue, Michael Turquette
On Sun, Dec 17, 2023 at 03:05:01PM -0800, Stephen Boyd wrote:
> Quoting Ben Wolsieffer (2023-10-02 11:08:53)
> > The stm32-power-config syscon (PWR peripheral) is used in this driver
> > and the STM32 RTC driver to enable write access to backup domain
> > registers. The syscon's clock has a gate controlled by this clock
> > driver, but this clock is currently not registered in the device tree.
> > This only happens to work currently because all relevant clock setup and
> > RTC initialization happens before clk_disabled_unused(). After this
> > point, all syscon register writes are ignored.
>
> Seems like we should mark those clks as CLK_IGNORE_UNUSED and add a
> comment to that fact.
That seems like a worse solution than specifying the clock dependency in
the device tree.
>
> >
> > If we simply add the syscon clock in the device tree, we end up with a
> > circular dependency because the clock has not been registered at the
> > point this driver requests the syscon.
> >
> > This patch avoids this circular dependency by moving the syscon lookup
> > after the clocks are registered. This does appear to create a possible
> > race condition where someone could attempt to perform an operation on a
> > backup domain clock before the syscon has been initialized. This would
> > result in the operation having no effect because backup domain writes
> > could not be enabled. I'm not sure if this is a problem or if there is
> > a way to avoid it.
>
> There's no comment in the code that says the regmap must be set there
> instead of earlier. What's to stop someone from tripping over this
> problem later? At the least, please add a comment.
Yeah, I'll fix that. Do you have any thoughts on the race condition I
described? Should I add some kind of locking to block
enable/disable_power_domain_write_protection() until stm32f4_rcc_init()
attempts to initialize the syscon?
Thank you, Ben
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered
2024-01-12 22:00 ` Ben Wolsieffer
@ 2024-04-12 6:28 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2024-04-12 6:28 UTC (permalink / raw)
To: Ben Wolsieffer
Cc: devicetree, linux-arm-kernel, linux-clk, linux-kernel,
linux-stm32, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maxime Coquelin, Alexandre Torgue, Michael Turquette
Quoting Ben Wolsieffer (2024-01-12 14:00:04)
> On Sun, Dec 17, 2023 at 03:05:01PM -0800, Stephen Boyd wrote:
> > Quoting Ben Wolsieffer (2023-10-02 11:08:53)
> > > The stm32-power-config syscon (PWR peripheral) is used in this driver
> > > and the STM32 RTC driver to enable write access to backup domain
> > > registers. The syscon's clock has a gate controlled by this clock
> > > driver, but this clock is currently not registered in the device tree.
> > > This only happens to work currently because all relevant clock setup and
> > > RTC initialization happens before clk_disabled_unused(). After this
> > > point, all syscon register writes are ignored.
> >
> > Seems like we should mark those clks as CLK_IGNORE_UNUSED and add a
> > comment to that fact.
>
> That seems like a worse solution than specifying the clock dependency in
> the device tree.
>
> >
> > >
> > > If we simply add the syscon clock in the device tree, we end up with a
> > > circular dependency because the clock has not been registered at the
> > > point this driver requests the syscon.
> > >
> > > This patch avoids this circular dependency by moving the syscon lookup
> > > after the clocks are registered. This does appear to create a possible
> > > race condition where someone could attempt to perform an operation on a
> > > backup domain clock before the syscon has been initialized. This would
> > > result in the operation having no effect because backup domain writes
> > > could not be enabled. I'm not sure if this is a problem or if there is
> > > a way to avoid it.
> >
> > There's no comment in the code that says the regmap must be set there
> > instead of earlier. What's to stop someone from tripping over this
> > problem later? At the least, please add a comment.
>
> Yeah, I'll fix that. Do you have any thoughts on the race condition I
> described? Should I add some kind of locking to block
> enable/disable_power_domain_write_protection() until stm32f4_rcc_init()
> attempts to initialize the syscon?
Maybe. I don't really know and it's probably because I don't really
understand the problem. Maybe you can solve it by turning on the backup
domain clock manually when the driver probes via direct register writes
and then only publish the syscon once the backup domain clock is
registered?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ARM: dts: stm32: add pwrcfg clock for stm32f4/7
2023-10-02 18:08 [PATCH 0/2] ARM: stm32: add clock for pwrcfg syscon Ben Wolsieffer
2023-10-02 18:08 ` [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered Ben Wolsieffer
@ 2023-10-02 18:08 ` Ben Wolsieffer
1 sibling, 0 replies; 6+ messages in thread
From: Ben Wolsieffer @ 2023-10-02 18:08 UTC (permalink / raw)
To: linux-stm32, linux-arm-kernel, linux-kernel, devicetree,
linux-clk
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Michael Turquette, Stephen Boyd, Ben Wolsieffer
Now that the circular dependency between syscon and clock initialization
has been resolved, we can add the clock that drives the pwrcfg syscon to
the device tree.
Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com>
---
arch/arm/boot/dts/st/stm32f429.dtsi | 1 +
arch/arm/boot/dts/st/stm32f746.dtsi | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32f429.dtsi b/arch/arm/boot/dts/st/stm32f429.dtsi
index 8efcda9ef8ae..7c9a9133cc86 100644
--- a/arch/arm/boot/dts/st/stm32f429.dtsi
+++ b/arch/arm/boot/dts/st/stm32f429.dtsi
@@ -665,6 +665,7 @@ spi6: spi@40015400 {
pwrcfg: power-config@40007000 {
compatible = "st,stm32-power-config", "syscon";
reg = <0x40007000 0x400>;
+ clocks = <&rcc 0 STM32F4_APB1_CLOCK(PWR)>;
};
ltdc: display-controller@40016800 {
diff --git a/arch/arm/boot/dts/st/stm32f746.dtsi b/arch/arm/boot/dts/st/stm32f746.dtsi
index d1802efd067c..cc8177466a51 100644
--- a/arch/arm/boot/dts/st/stm32f746.dtsi
+++ b/arch/arm/boot/dts/st/stm32f746.dtsi
@@ -510,6 +510,7 @@ pwm {
pwrcfg: power-config@40007000 {
compatible = "st,stm32-power-config", "syscon";
reg = <0x40007000 0x400>;
+ clocks = <&rcc 0 STM32F7_APB1_CLOCK(PWR)>;
};
crc: crc@40023000 {
--
2.42.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-12 6:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-02 18:08 [PATCH 0/2] ARM: stm32: add clock for pwrcfg syscon Ben Wolsieffer
2023-10-02 18:08 ` [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered Ben Wolsieffer
2023-12-17 23:05 ` Stephen Boyd
2024-01-12 22:00 ` Ben Wolsieffer
2024-04-12 6:28 ` Stephen Boyd
2023-10-02 18:08 ` [PATCH 2/2] ARM: dts: stm32: add pwrcfg clock for stm32f4/7 Ben Wolsieffer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).