[v2,2/3] mfd: syscon: Add optional clock support
diff mbox series

Message ID 1544604495-4082-3-git-send-email-fabrice.gasnier@st.com
State Superseded
Headers show
Series
  • mfd: syscon: Add optional clock support needed on stm32
Related show

Commit Message

Fabrice Gasnier Dec. 12, 2018, 8:48 a.m. UTC
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(+)

Comments

Arnd Bergmann Jan. 16, 2019, 12:14 p.m. UTC | #1
(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
Fabrice Gasnier Jan. 16, 2019, 2:10 p.m. UTC | #2
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
>
Arnd Bergmann Jan. 16, 2019, 3:11 p.m. UTC | #3
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
Fabrice Gasnier Jan. 28, 2019, 1:20 p.m. UTC | #4
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
>
Fabrice Gasnier Feb. 11, 2019, 4:32 p.m. UTC | #5
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
>>
Arnd Bergmann Feb. 18, 2019, 12:54 p.m. UTC | #6
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
Lee Jones March 20, 2019, 1:05 p.m. UTC | #7
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.

Patch
diff mbox series

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: