STM32 syscfg registers are accessed using syscon. It needs syscfg clock
to be enabled while accessing registers.
This adds support for optional clock on syscon, and the relevant clock
in stm32mp157 device tree.
Changes in v2:
- move clocks to specific bindings using syscon as per Rob's comment
Fabrice Gasnier (3):
dt-bindings: stm32: syscon: add clock support
mfd: syscon: Add optional clock support
ARM: dts: stm32: Add clock on stm32mp157c syscfg
.../devicetree/bindings/arm/stm32/stm32-syscon.txt | 2 ++
arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
drivers/mfd/syscon.c | 19 +++++++++++++++++++
3 files changed, 22 insertions(+)
--
1.9.1
STM32 system configuration controller registers needs to be clocked.
Document clock support on stm32-syscon.
Signed-off-by: Fabrice Gasnier <[email protected]>
---
Changes in v2:
- move clocks to specific bindings using syscon as per Rob's comment
---
Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt b/Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt
index 99980ae..c92d411 100644
--- a/Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt
+++ b/Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt
@@ -5,10 +5,12 @@ Properties:
- " st,stm32mp157-syscfg " - for stm32mp157 based SoCs,
second value must be always "syscon".
- reg : offset and length of the register set.
+ - clocks: phandle to the syscfg clock
Example:
syscfg: syscon@50020000 {
compatible = "st,stm32mp157-syscfg", "syscon";
reg = <0x50020000 0x400>;
+ clocks = <&rcc SYSCFG>;
};
--
1.9.1
STM32 syscfg needs a clock to access registers.
Signed-off-by: Fabrice Gasnier <[email protected]>
---
arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 8bf1c17..61b2a70 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -820,6 +820,7 @@
syscfg: syscon@50020000 {
compatible = "st,stm32mp157-syscfg", "syscon";
reg = <0x50020000 0x400>;
+ clocks = <&rcc SYSCFG>;
};
lptimer2: timer@50021000 {
--
1.9.1
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 <[email protected]>
---
drivers/mfd/syscon.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
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:
--
1.9.1
On Wed, 12 Dec 2018 09:48:13 +0100, Fabrice Gasnier wrote:
> STM32 system configuration controller registers needs to be clocked.
> Document clock support on stm32-syscon.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> ---
> Changes in v2:
> - move clocks to specific bindings using syscon as per Rob's comment
> ---
> Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
(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 <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>
>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
Arnd
On Wed, 12 Dec 2018, Fabrice Gasnier wrote:
> STM32 system configuration controller registers needs to be clocked.
> Document clock support on stm32-syscon.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> ---
> Changes in v2:
> - move clocks to specific bindings using syscon as per Rob's comment
> ---
> Documentation/devicetree/bindings/arm/stm32/stm32-syscon.txt | 2 ++
> 1 file changed, 2 insertions(+)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
> ---
> drivers/mfd/syscon.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Fabrice
On 12/12/18 9:48 AM, Fabrice Gasnier wrote:
> STM32 syscfg needs a clock to access registers.
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
> ---
> arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 8bf1c17..61b2a70 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -820,6 +820,7 @@
> syscfg: syscon@50020000 {
> compatible = "st,stm32mp157-syscfg", "syscon";
> reg = <0x50020000 0x400>;
> + clocks = <&rcc SYSCFG>;
> };
>
> lptimer2: timer@50021000 {
>
For the DT patch:
Applied on stm32-next.
Thanks.
Alex