| Message ID | 1544604495-4082-3-git-send-email-fabrice.gasnier@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mfd: syscon: Add optional clock support needed on stm32 | expand |
(sorry for the late reply, I just realized that I had never sent out the mail after Lee asked me for a review last year and I had drafted my reply). On Wed, Dec 12, 2018 at 9:48 AM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > > Some system control registers need to be clocked, so the registers can > be accessed. Add an optional clock and attach it to regmap. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> This looks ok to me in principle, but I have one question: When we do a clk_get() and clk_prepare() as part of regmap_mmio_attach_clk(), does that change the behavior of syscon nodes that are otherwise unused? I think we have a bunch of devices that started out as a syscon but then we added a proper driver for them, which would handle the clocks explicitly. Is it guaranteed that this will keep working (including shutting down the clocks when they are unused) if we have two drivers that call clk_get() on the same device node? Arnd
On 1/16/19 1:14 PM, Arnd Bergmann wrote: > (sorry for the late reply, I just realized that I had never sent out the > mail after Lee asked me for a review last year and I had drafted > my reply). Hi Arnd, Many thanks for reviewing, no worries :-) > > On Wed, Dec 12, 2018 at 9:48 AM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >> >> Some system control registers need to be clocked, so the registers can >> be accessed. Add an optional clock and attach it to regmap. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > > This looks ok to me in principle, but I have one question: When we > do a clk_get() and clk_prepare() as part of regmap_mmio_attach_clk(), > does that change the behavior of syscon nodes that are otherwise > unused? I'm not sure I correctly understand this question. I don't think it will change the behavior for "unused" nodes. These should remain unused with this patch. > > I think we have a bunch of devices that started out as a syscon but > then we added a proper driver for them, which would handle the > clocks explicitly. Is it guaranteed that this will keep working (including > shutting down the clocks when they are unused) if we have two drivers > that call clk_get() on the same device node? I'd expect nothing wrong happens when two drivers call clk_get() for the same clock. Are there some case where two drivers are bind (e.g. syscon driver + another driver) for the same piece of hardware ? The clk_prepare() is part of regmap_mmio_attach_clk(). It's called once upon registration via of_syscon_register(). There is currently no mean to unregister, e.g. something like "of_syscon_unregister" and so do clk_unprepare via regmap_mmio_detach_clk(). Then point is clk_enable()/clk_disable() calls will be used in regmap_mmio_read() and regmap_mmio_write(). These calls are balanced. Then clock framework should correctly disable/gate the clock when unused, based on the enable count. Is this answering your question? Thanks again, Best regards, Fabrice > > Arnd >
On Wed, Jan 16, 2019 at 3:10 PM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > > On 1/16/19 1:14 PM, Arnd Bergmann wrote: > > (sorry for the late reply, I just realized that I had never sent out the > > mail after Lee asked me for a review last year and I had drafted > > my reply). > > Hi Arnd, > > Many thanks for reviewing, no worries :-) > > > > > On Wed, Dec 12, 2018 at 9:48 AM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > >> > >> Some system control registers need to be clocked, so the registers can > >> be accessed. Add an optional clock and attach it to regmap. > >> > >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > > > > This looks ok to me in principle, but I have one question: When we > > do a clk_get() and clk_prepare() as part of regmap_mmio_attach_clk(), > > does that change the behavior of syscon nodes that are otherwise > > unused? > > I'm not sure I correctly understand this question. I don't think it will > change the behavior for "unused" nodes. These should remain unused with > this patch. What I mean is that nodes that listed as 'compatible="syscon"' get probed by the syscon driver even when no other driver references them, and that in turn would acquire the clock, right? > > I think we have a bunch of devices that started out as a syscon but > > then we added a proper driver for them, which would handle the > > clocks explicitly. Is it guaranteed that this will keep working (including > > shutting down the clocks when they are unused) if we have two drivers > > that call clk_get() on the same device node? > > I'd expect nothing wrong happens when two drivers call clk_get() for the > same clock. > Are there some case where two drivers are bind (e.g. syscon driver + > another driver) for the same piece of hardware ? You won't actually have two drivers binding to the same device, but you could have a driver and a syscon user that does relies on the syscon_regmap_lookup_by_*() functions. I think we've had a couple of cases where we started out representing a device as syscon, and then later decided that a high-level abstraction would be needed because syscon didn't quite support all the needed features. Since each syscon node should also have a more specific compatible value, you can then have another driver that binds to that compatible string. Arnd
On 1/16/19 4:11 PM, Arnd Bergmann wrote: > On Wed, Jan 16, 2019 at 3:10 PM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >> >> On 1/16/19 1:14 PM, Arnd Bergmann wrote: >>> (sorry for the late reply, I just realized that I had never sent out the >>> mail after Lee asked me for a review last year and I had drafted >>> my reply). >> >> Hi Arnd, >> >> Many thanks for reviewing, no worries :-) >> >>> >>> On Wed, Dec 12, 2018 at 9:48 AM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >>>> >>>> Some system control registers need to be clocked, so the registers can >>>> be accessed. Add an optional clock and attach it to regmap. >>>> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >>> >>> This looks ok to me in principle, but I have one question: When we >>> do a clk_get() and clk_prepare() as part of regmap_mmio_attach_clk(), >>> does that change the behavior of syscon nodes that are otherwise >>> unused? >> >> I'm not sure I correctly understand this question. I don't think it will >> change the behavior for "unused" nodes. These should remain unused with >> this patch. > > What I mean is that nodes that listed as 'compatible="syscon"' get > probed by the syscon driver even when no other driver references > them, and that in turn would acquire the clock, right? Hi Arnd, Sorry for the late reply. When no other driver references them, nothing happens at probe time on the clock: no calls to get/prepare... the clock. => The clock will remain unrequested & unused until another driver calls one of "of_syscon_register()" variants: - syscon_node_to_regmap - syscon_regmap_lookup_by_compatible - syscon_regmap_lookup_by_phandle When another driver references them (e.g. one of the above calls), then it will acquire the optional clock and use it, e.g.: - clk_prepare() upon of_syscon_register() variants - clk_enable & clk_disable when accessing the registers I hope this clarifies. Please advise, Best Regards, Fabrice > >>> I think we have a bunch of devices that started out as a syscon but >>> then we added a proper driver for them, which would handle the >>> clocks explicitly. Is it guaranteed that this will keep working (including >>> shutting down the clocks when they are unused) if we have two drivers >>> that call clk_get() on the same device node? >> >> I'd expect nothing wrong happens when two drivers call clk_get() for the >> same clock. >> Are there some case where two drivers are bind (e.g. syscon driver + >> another driver) for the same piece of hardware ? > > You won't actually have two drivers binding to the same device, but you > could have a driver and a syscon user that does relies on the > syscon_regmap_lookup_by_*() functions. > > I think we've had a couple of cases where we started out representing > a device as syscon, and then later decided that a high-level abstraction > would be needed because syscon didn't quite support all the needed > features. > > Since each syscon node should also have a more specific > compatible value, you can then have another driver that binds > to that compatible string. > > Arnd >
On 1/28/19 2:20 PM, Fabrice Gasnier wrote: > On 1/16/19 4:11 PM, Arnd Bergmann wrote: >> On Wed, Jan 16, 2019 at 3:10 PM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >>> >>> On 1/16/19 1:14 PM, Arnd Bergmann wrote: >>>> (sorry for the late reply, I just realized that I had never sent out the >>>> mail after Lee asked me for a review last year and I had drafted >>>> my reply). >>> >>> Hi Arnd, >>> >>> Many thanks for reviewing, no worries :-) >>> >>>> >>>> On Wed, Dec 12, 2018 at 9:48 AM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >>>>> >>>>> Some system control registers need to be clocked, so the registers can >>>>> be accessed. Add an optional clock and attach it to regmap. >>>>> >>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >>>> >>>> This looks ok to me in principle, but I have one question: When we >>>> do a clk_get() and clk_prepare() as part of regmap_mmio_attach_clk(), >>>> does that change the behavior of syscon nodes that are otherwise >>>> unused? >>> >>> I'm not sure I correctly understand this question. I don't think it will >>> change the behavior for "unused" nodes. These should remain unused with >>> this patch. >> >> What I mean is that nodes that listed as 'compatible="syscon"' get >> probed by the syscon driver even when no other driver references >> them, and that in turn would acquire the clock, right? > > Hi Arnd, > > Sorry for the late reply. > > When no other driver references them, nothing happens at probe time on > the clock: no calls to get/prepare... the clock. > > => The clock will remain unrequested & unused until another driver calls > one of "of_syscon_register()" variants: > - syscon_node_to_regmap > - syscon_regmap_lookup_by_compatible > - syscon_regmap_lookup_by_phandle > > When another driver references them (e.g. one of the above calls), then > it will acquire the optional clock and use it, e.g.: > - clk_prepare() upon of_syscon_register() variants > - clk_enable & clk_disable when accessing the registers > > I hope this clarifies. > > Please advise, > Best Regards, > Fabrice Hi Arnd, Gentlemen reminder for this. I would appreciate to have your feedback. Many thanks, Fabrice > >> >>>> I think we have a bunch of devices that started out as a syscon but >>>> then we added a proper driver for them, which would handle the >>>> clocks explicitly. Is it guaranteed that this will keep working (including >>>> shutting down the clocks when they are unused) if we have two drivers >>>> that call clk_get() on the same device node? >>> >>> I'd expect nothing wrong happens when two drivers call clk_get() for the >>> same clock. >>> Are there some case where two drivers are bind (e.g. syscon driver + >>> another driver) for the same piece of hardware ? >> >> You won't actually have two drivers binding to the same device, but you >> could have a driver and a syscon user that does relies on the >> syscon_regmap_lookup_by_*() functions. >> >> I think we've had a couple of cases where we started out representing >> a device as syscon, and then later decided that a high-level abstraction >> would be needed because syscon didn't quite support all the needed >> features. >> >> Since each syscon node should also have a more specific >> compatible value, you can then have another driver that binds >> to that compatible string. >> >> Arnd >>
On Mon, Feb 11, 2019 at 5:32 PM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > > On 1/28/19 2:20 PM, Fabrice Gasnier wrote: > > On 1/16/19 4:11 PM, Arnd Bergmann wrote: > >> On Wed, Jan 16, 2019 at 3:10 PM Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > >> > >> What I mean is that nodes that listed as 'compatible="syscon"' get > >> probed by the syscon driver even when no other driver references > >> them, and that in turn would acquire the clock, right? > > > > When no other driver references them, nothing happens at probe time on > > the clock: no calls to get/prepare... the clock. > > > > => The clock will remain unrequested & unused until another driver calls > > one of "of_syscon_register()" variants: > > - syscon_node_to_regmap > > - syscon_regmap_lookup_by_compatible > > - syscon_regmap_lookup_by_phandle > > > > When another driver references them (e.g. one of the above calls), then > > it will acquire the optional clock and use it, e.g.: > > - clk_prepare() upon of_syscon_register() variants > > - clk_enable & clk_disable when accessing the registers > > > > I hope this clarifies. > > I would appreciate to have your feedback. Yes, I think that's all we need here, thanks for the clarification, and sorry for dropping the ball on this again. Acked-by: Arnd Bergmann <arnd@arndb.de> Arnd
On Wed, 12 Dec 2018, Fabrice Gasnier wrote: > Some system control registers need to be clocked, so the registers can > be accessed. Add an optional clock and attach it to regmap. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > --- > drivers/mfd/syscon.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) Applied, thanks.
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index b6d05cd..a0ba4ff 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -12,6 +12,7 @@ * (at your option) any later version. */ +#include <linux/clk.h> #include <linux/err.h> #include <linux/hwspinlock.h> #include <linux/io.h> @@ -45,6 +46,7 @@ struct syscon { static struct syscon *of_syscon_register(struct device_node *np) { + struct clk *clk; struct syscon *syscon; struct regmap *regmap; void __iomem *base; @@ -119,6 +121,18 @@ static struct syscon *of_syscon_register(struct device_node *np) goto err_regmap; } + clk = of_clk_get(np, 0); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); + /* clock is optional */ + if (ret != -ENOENT) + goto err_clk; + } else { + ret = regmap_mmio_attach_clk(regmap, clk); + if (ret) + goto err_attach; + } + syscon->regmap = regmap; syscon->np = np; @@ -128,6 +142,11 @@ static struct syscon *of_syscon_register(struct device_node *np) return syscon; +err_attach: + if (!IS_ERR(clk)) + clk_put(clk); +err_clk: + regmap_exit(regmap); err_regmap: iounmap(base); err_map:
Some system control registers need to be clocked, so the registers can be accessed. Add an optional clock and attach it to regmap. Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> --- drivers/mfd/syscon.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)