| Message ID | 20210528113403.5374-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: support regmap | expand |
Quoting Peng Fan (OSS) (2021-05-28 04:34:00) > From: Peng Fan <peng.fan@nxp.com> > > 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 6/2/21 10:18 AM, Stephen Boyd wrote: > Quoting Peng Fan (OSS) (2021-05-28 04:34:00) >> From: Peng Fan <peng.fan@nxp.com> >> >> 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
> Subject: Re: [PATCH 0/3] clk: support regmap > > Quoting Peng Fan (OSS) (2021-05-28 04:34:00) > > From: Peng Fan <peng.fan@nxp.com> > > > > 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
> 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 <peng.fan@nxp.com> > >> > >> 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 |
On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 6/2/21 10:18 AM, Stephen Boyd wrote: >> Quoting Peng Fan (OSS) (2021-05-28 04:34:00) >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> 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
On Wed 02 Jun 2021 at 11:32, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > >> On 6/2/21 10:18 AM, Stephen Boyd wrote: >>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00) >>>> From: Peng Fan <peng.fan@nxp.com> >>>> >>>> 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 <jbrunet@baylibre.com> wrote: > > > On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > >> On 6/2/21 10:18 AM, Stephen Boyd wrote: > >>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00) > >>>> From: Peng Fan <peng.fan@nxp.com> > >>>> > >>>> 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 :)
From: Peng Fan <peng.fan@nxp.com> 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(-)