From: Peng Fan <[email protected]>
To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
reset functionality, so we need make sure the access to the PCC register
be protected to avoid concurrent access from clk and reset subsystem.
So let's use regmap here.
The i.MX specific code will be posted if this patchset is ok for you.
Peng Fan (3):
clk: mux: support regmap
clk: fractional-divider: support regmap
clk: gate: support regmap
drivers/clk/clk-fractional-divider.c | 26 +++++++++++++++++++++++---
drivers/clk/clk-gate.c | 26 +++++++++++++++++++++++---
drivers/clk/clk-mux.c | 26 +++++++++++++++++++++++---
include/linux/clk-provider.h | 14 ++++++++++++++
4 files changed, 83 insertions(+), 9 deletions(-)
--
2.30.0
From: Peng Fan <[email protected]>
To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
reset functionality, so we need make sure the access to the PCC register
be protected to avoid concurrent access from clk and reset subsystem.
So let's use regmap here.
Signed-off-by: Peng Fan <[email protected]>
---
drivers/clk/clk-mux.c | 26 +++++++++++++++++++++++---
include/linux/clk-provider.h | 6 ++++++
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 20582aae7a35..90facb6eafe6 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -10,6 +10,7 @@
#include <linux/clk-provider.h>
#include <linux/device.h>
#include <linux/module.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/err.h>
@@ -26,18 +27,37 @@
static inline u32 clk_mux_readl(struct clk_mux *mux)
{
- if (mux->flags & CLK_MUX_BIG_ENDIAN)
+ int ret;
+ uint32_t val;
+
+ if (mux->flags & CLK_MUX_BIG_ENDIAN) {
return ioread32be(mux->reg);
+ } else if (mux->flags & CLK_MUX_REGMAP) {
+ ret = regmap_read(mux->regmap, mux->reg_off, &val);
+ if (ret < 0) {
+ pr_warn("%s: failed read %x, %d\n", __func__, mux->reg_off, ret);
+ return ret;
+ } else {
+ return val;
+ }
+ }
return readl(mux->reg);
}
static inline void clk_mux_writel(struct clk_mux *mux, u32 val)
{
- if (mux->flags & CLK_MUX_BIG_ENDIAN)
+ int ret;
+
+ if (mux->flags & CLK_MUX_BIG_ENDIAN) {
iowrite32be(val, mux->reg);
- else
+ } else if (mux->flags & CLK_MUX_REGMAP) {
+ ret = regmap_write(mux->regmap, mux->reg_off, val);
+ if (ret < 0)
+ pr_warn("%s: failed write %x, %d\n", __func__, mux->reg_off, ret);
+ } else {
writel(val, mux->reg);
+ }
}
int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 162a2e5546a3..6bd9288b953d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -837,6 +837,8 @@ void clk_hw_unregister_divider(struct clk_hw *hw);
* @mask: mask of mutliplexer bit field
* @flags: hardware-specific flags
* @lock: register lock
+ * @regmap: register controlling regmap
+ * @reg_off: register offset
*
* Clock with multiple selectable parents. Implements .get_parent, .set_parent
* and .recalc_rate
@@ -855,6 +857,7 @@ void clk_hw_unregister_divider(struct clk_hw *hw);
* CLK_MUX_BIG_ENDIAN - By default little endian register accesses are used for
* the mux register. Setting this flag makes the register accesses big
* endian.
+ * CLK_MUX_REGMAP - Indicate the accessing method is using regmap API.
*/
struct clk_mux {
struct clk_hw hw;
@@ -864,6 +867,8 @@ struct clk_mux {
u8 shift;
u8 flags;
spinlock_t *lock;
+ struct regmap *regmap;
+ u32 reg_off;
};
#define to_clk_mux(_hw) container_of(_hw, struct clk_mux, hw)
@@ -874,6 +879,7 @@ struct clk_mux {
#define CLK_MUX_READ_ONLY BIT(3) /* mux can't be changed */
#define CLK_MUX_ROUND_CLOSEST BIT(4)
#define CLK_MUX_BIG_ENDIAN BIT(5)
+#define CLK_MUX_REGMAP BIT(6)
extern const struct clk_ops clk_mux_ops;
extern const struct clk_ops clk_mux_ro_ops;
--
2.30.0
On 6/2/21 10:18 AM, Stephen Boyd wrote:
> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
>> From: Peng Fan <[email protected]>
>>
>> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
>> reset functionality, so we need make sure the access to the PCC register
>> be protected to avoid concurrent access from clk and reset subsystem.
>>
>> So let's use regmap here.
>>
>> The i.MX specific code will be posted if this patchset is ok for you.
>
> We have a couple regmap clk drivers in the tree. Either combine the
> different regmap clk drivers or duplicate it into the imx directory. I'd
> prefer we combine them but last time I couldn't agree on the approach
> when Jerome wanted to do it. Maybe now is the time to combine them all
> into one common piece of code.
IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
no sense to have them in an arch specific subdir, like meson.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> Subject: Re: [PATCH 0/3] clk: support regmap
>
> On 6/2/21 10:18 AM, Stephen Boyd wrote:
> > Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
> >> From: Peng Fan <[email protected]>
> >>
> >> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and
> >> peripheral reset functionality, so we need make sure the access to
> >> the PCC register be protected to avoid concurrent access from clk and
> reset subsystem.
> >>
> >> So let's use regmap here.
> >>
> >> The i.MX specific code will be posted if this patchset is ok for you.
> >
> > We have a couple regmap clk drivers in the tree. Either combine the
> > different regmap clk drivers or duplicate it into the imx directory.
> > I'd prefer we combine them but last time I couldn't agree on the
> > approach when Jerome wanted to do it. Maybe now is the time to combine
> > them all into one common piece of code.
>
> IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
> no sense to have them in an arch specific subdir, like meson.
Yeah. Let's make it as common piece code could be reused by others.
Thanks,
Peng.
>
> regards,
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> Subject: Re: [PATCH 0/3] clk: support regmap
>
> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
> > From: Peng Fan <[email protected]>
> >
> > To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and
> > peripheral reset functionality, so we need make sure the access to the
> > PCC register be protected to avoid concurrent access from clk and reset
> subsystem.
> >
> > So let's use regmap here.
> >
> > The i.MX specific code will be posted if this patchset is ok for you.
>
> We have a couple regmap clk drivers in the tree. Either combine the different
> regmap clk drivers or duplicate it into the imx directory. I'd prefer we combine
> them but last time I couldn't agree on the approach when Jerome wanted to
> do it.
You mean Jerome has some work to combine them all into one common?
Could you please share me the patch link?
Maybe now is the time to combine them all into one common piece of
> code.
ok, I see.
Thanks,
Peng.
>
> >
> > Peng Fan (3):
> > clk: mux: support regmap
> > clk: fractional-divider: support regmap
> > clk: gate: support regmap
On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <[email protected]> wrote:
> On 6/2/21 10:18 AM, Stephen Boyd wrote:
>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
>>> From: Peng Fan <[email protected]>
>>>
>>> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
>>> reset functionality, so we need make sure the access to the PCC register
>>> be protected to avoid concurrent access from clk and reset subsystem.
>>>
>>> So let's use regmap here.
>>>
>>> The i.MX specific code will be posted if this patchset is ok for you.
>>
>> We have a couple regmap clk drivers in the tree. Either combine the
>> different regmap clk drivers or duplicate it into the imx directory. I'd
>> prefer we combine them but last time I couldn't agree on the approach
>> when Jerome wanted to do it. Maybe now is the time to combine them all
>> into one common piece of code.
>
> IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
> no sense to have them in an arch specific subdir, like meson.
Indeed, those basic types were not meant to remain platform
specific. Some framework (ASoC for ex) make heavy use of regmap and
could welcome regmap based basic clock types.
At the time, Stephen (qcom) and I (meson) had slightly different
approaches. Before having those types spread through the kernel, I think
testing things out was a good thing ... this is why these are platform
specific ATM.
It's been 3 years now ... and it has not been a total disaster :)
In the end things are not so different. Let's compare:
a. Both have a generic "clk_regmap" type common to all regmap based
types. This is very useful to easily fix the regmap pointer in static
clocks (which are heavily used by both platform)
b. Meson uses a generic pointer to store the type specific info.
Qcom embeds the generic clk_regmap into the specific type one.
=> In the end, I don't see any advantage to the meson
approach. Switching to the qcom method would be quite a big bang
for meson but it is doable (nothing difficult, just a huge line count)
c. Qcom basic regmap type deviates a bit from the regular basic ones
when it comes to the actual data. The qcom "clk_regmap" also provides
the gate, mux have the qcom "parent_map". In meson, I tried to keep as
close as possible to regular basic types ... at least what they were 3
years ago. Only the register poking part should be different actually.
=> I'd be in favor of keeping close to meson here.
A possible plan could be:
1. Rework meson as explained in [b] above.
2. reword types in qcom where necessary to avoid name clashes (add
"_qcom" extension for ex)
3. Move the clk_regmap implementation out of meson to drivers/clk
4. Things are yours to play with ...
I can take care of 1. and 3. I would welcome help for 2. especially since
I won't be able to test it.
>
> regards,
> Marc
Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
> From: Peng Fan <[email protected]>
>
> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
> reset functionality, so we need make sure the access to the PCC register
> be protected to avoid concurrent access from clk and reset subsystem.
>
> So let's use regmap here.
>
> The i.MX specific code will be posted if this patchset is ok for you.
We have a couple regmap clk drivers in the tree. Either combine the
different regmap clk drivers or duplicate it into the imx directory. I'd
prefer we combine them but last time I couldn't agree on the approach
when Jerome wanted to do it. Maybe now is the time to combine them all
into one common piece of code.
>
> Peng Fan (3):
> clk: mux: support regmap
> clk: fractional-divider: support regmap
> clk: gate: support regmap
On Wed 02 Jun 2021 at 11:32, Jerome Brunet <[email protected]> wrote:
> On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <[email protected]> wrote:
>
>> On 6/2/21 10:18 AM, Stephen Boyd wrote:
>>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
>>>> From: Peng Fan <[email protected]>
>>>>
>>>> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
>>>> reset functionality, so we need make sure the access to the PCC register
>>>> be protected to avoid concurrent access from clk and reset subsystem.
>>>>
>>>> So let's use regmap here.
>>>>
>>>> The i.MX specific code will be posted if this patchset is ok for you.
>>>
>>> We have a couple regmap clk drivers in the tree. Either combine the
>>> different regmap clk drivers or duplicate it into the imx directory. I'd
>>> prefer we combine them but last time I couldn't agree on the approach
>>> when Jerome wanted to do it. Maybe now is the time to combine them all
>>> into one common piece of code.
>>
>> IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
>> no sense to have them in an arch specific subdir, like meson.
>
> Indeed, those basic types were not meant to remain platform
> specific. Some framework (ASoC for ex) make heavy use of regmap and
> could welcome regmap based basic clock types.
>
> At the time, Stephen (qcom) and I (meson) had slightly different
> approaches. Before having those types spread through the kernel, I think
> testing things out was a good thing ... this is why these are platform
> specific ATM.
>
> It's been 3 years now ... and it has not been a total disaster :)
>
> In the end things are not so different. Let's compare:
> a. Both have a generic "clk_regmap" type common to all regmap based
> types. This is very useful to easily fix the regmap pointer in static
> clocks (which are heavily used by both platform)
>
> b. Meson uses a generic pointer to store the type specific info.
> Qcom embeds the generic clk_regmap into the specific type one.
> => In the end, I don't see any advantage to the meson
> approach. Switching to the qcom method would be quite a big bang
> for meson but it is doable (nothing difficult, just a huge line count)
>
> c. Qcom basic regmap type deviates a bit from the regular basic ones
> when it comes to the actual data. The qcom "clk_regmap" also provides
> the gate, mux have the qcom "parent_map". In meson, I tried to keep as
> close as possible to regular basic types ... at least what they were 3
> years ago. Only the register poking part should be different actually.
> => I'd be in favor of keeping close to meson here.
>
> A possible plan could be:
> 1. Rework meson as explained in [b] above.
> 2. reword types in qcom where necessary to avoid name clashes (add
> "_qcom" extension for ex)
> 3. Move the clk_regmap implementation out of meson to drivers/clk
> 4. Things are yours to play with ...
>
> I can take care of 1. and 3. I would welcome help for 2. especially since
> I won't be able to test it.
>
Hi Stephen,
What do you think of the proposition above ?
There rework is going to take some time to do. I'll start only if this
OK with you.
Cheers
Jerome
>>
>> regards,
>> Marc
Quoting Jerome Brunet (2021-06-18 07:30:59)
>
> On Wed 02 Jun 2021 at 11:32, Jerome Brunet <[email protected]> wrote:
>
> > On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <[email protected]> wrote:
> >
> >> On 6/2/21 10:18 AM, Stephen Boyd wrote:
> >>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
> >>>> From: Peng Fan <[email protected]>
> >>>>
> >>>> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
> >>>> reset functionality, so we need make sure the access to the PCC register
> >>>> be protected to avoid concurrent access from clk and reset subsystem.
> >>>>
> >>>> So let's use regmap here.
> >>>>
> >>>> The i.MX specific code will be posted if this patchset is ok for you.
> >>>
> >>> We have a couple regmap clk drivers in the tree. Either combine the
> >>> different regmap clk drivers or duplicate it into the imx directory. I'd
> >>> prefer we combine them but last time I couldn't agree on the approach
> >>> when Jerome wanted to do it. Maybe now is the time to combine them all
> >>> into one common piece of code.
> >>
> >> IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
> >> no sense to have them in an arch specific subdir, like meson.
> >
> > Indeed, those basic types were not meant to remain platform
> > specific. Some framework (ASoC for ex) make heavy use of regmap and
> > could welcome regmap based basic clock types.
> >
> > At the time, Stephen (qcom) and I (meson) had slightly different
> > approaches. Before having those types spread through the kernel, I think
> > testing things out was a good thing ... this is why these are platform
> > specific ATM.
> >
> > It's been 3 years now ... and it has not been a total disaster :)
> >
> > In the end things are not so different. Let's compare:
> > a. Both have a generic "clk_regmap" type common to all regmap based
> > types. This is very useful to easily fix the regmap pointer in static
> > clocks (which are heavily used by both platform)
> >
> > b. Meson uses a generic pointer to store the type specific info.
> > Qcom embeds the generic clk_regmap into the specific type one.
> > => In the end, I don't see any advantage to the meson
> > approach. Switching to the qcom method would be quite a big bang
> > for meson but it is doable (nothing difficult, just a huge line count)
> >
> > c. Qcom basic regmap type deviates a bit from the regular basic ones
> > when it comes to the actual data. The qcom "clk_regmap" also provides
> > the gate, mux have the qcom "parent_map". In meson, I tried to keep as
> > close as possible to regular basic types ... at least what they were 3
> > years ago. Only the register poking part should be different actually.
> > => I'd be in favor of keeping close to meson here.
> >
> > A possible plan could be:
> > 1. Rework meson as explained in [b] above.
> > 2. reword types in qcom where necessary to avoid name clashes (add
> > "_qcom" extension for ex)
> > 3. Move the clk_regmap implementation out of meson to drivers/clk
> > 4. Things are yours to play with ...
> >
> > I can take care of 1. and 3. I would welcome help for 2. especially since
> > I won't be able to test it.
> >
>
> Hi Stephen,
>
> What do you think of the proposition above ?
> There rework is going to take some time to do. I'll start only if this
> OK with you.
Hey Jerome,
Sorry I wrote a long email and then it got lost when my machine
rebooted. :( That was weeks ago! :( :(
Anyway, we have struct device now associated with clks when they're
registered (yay!), so I wonder if perhaps we can make an API or two like
clk_hw_get_regmap(struct clk_hw *) that takes a clk_hw and finds the
device associated with the clk and then calls dev_get_regmap() with that
device pointer to get the regmap pointer. Then clk drivers can use the
regmap pointer wherever they want. The basic clk type code can be
modified to either have a flag like "I_AM_A_REGMAP_CLK" (please pick
better name), or to have an init clk_op that stores the regmap pointer
in the basic type structure if grabbing it all the time is too costly
(no idea here so it really needs a measurement).
We do have some clks that are registered with devices that have
more than one regmap. The clk_hw_get_regmap() API would need to probably
take a string argument too, similar to dev_get_regmap() that lets clk
providers figure out what regmap they're asking for if there's more than
one associated with it.
With that API (and probably one for clk_hw_get_dev(struct clk_hw *)) we
should be able to avoid having to consolidate on a 'struct clk_regmap'
and let the various drivers keep doing what they're doing with the
regmap pointer. I know this may not be what you're envisioning, but I
think it would be a good step to take so that we can simplify some of the
registration code and add regmap support to the basic types in a way
that isn't heavily invasive because we're changing structs or struct
members all over the place.
I for one don't look forward to reviewing a huge rewrite of any clk
driver :)