From: Stephen Boyd <sboyd@kernel.org>
To: Ben Wolsieffer <ben.wolsieffer@hefring.com>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Maxime Coquelin <mcoquelin.stm32@gmail.com>,
Alexandre Torgue <alexandre.torgue@foss.st.com>,
Michael Turquette <mturquette@baylibre.com>
Subject: Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are registered
Date: Thu, 11 Apr 2024 23:28:07 -0700 [thread overview]
Message-ID: <3d6d34c5075559f3df506d14e38a9c0c.sboyd@kernel.org> (raw)
In-Reply-To: <ZaG2ZDCLP34jcI6Y@dell-precision-5540>
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?
next prev parent reply other threads:[~2024-04-12 6:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-10-02 18:08 ` [PATCH 2/2] ARM: dts: stm32: add pwrcfg clock for stm32f4/7 Ben Wolsieffer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3d6d34c5075559f3df506d14e38a9c0c.sboyd@kernel.org \
--to=sboyd@kernel.org \
--cc=alexandre.torgue@foss.st.com \
--cc=ben.wolsieffer@hefring.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).