| Message ID | 20190701091258.3870-5-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | arm64: g12a: add support for DVFS | expand |
+Cc Alexandre Mergnat On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Add a setup() callback in the eeclk structure, to call an optional > call() function at end of eeclk probe to setup clocks. > > It's used for the G12A clock controller to setup the CPU clock notifiers. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> this will probably work fine, but I want do double check first are we planning to get rid of meson-eeclk (mid-term)? Alex has some patches to get rid of all these IN_PREFIX logic. I'm asking because if we want to get rid of meson-eeclk it may be the time to do so now to have less logic to migrate later on Martin
On 03/07/2019 01:16, Martin Blumenstingl wrote: > +Cc Alexandre Mergnat > > On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> Add a setup() callback in the eeclk structure, to call an optional >> call() function at end of eeclk probe to setup clocks. >> >> It's used for the G12A clock controller to setup the CPU clock notifiers. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > this will probably work fine, but I want do double check first > > are we planning to get rid of meson-eeclk (mid-term)? AFAIK no, but maybe I'm not aware of it ! Neil > Alex has some patches to get rid of all these IN_PREFIX logic. > I'm asking because if we want to get rid of meson-eeclk it may be the > time to do so now to have less logic to migrate later on > > > Martin >
On Wed 03 Jul 2019 at 13:45, Neil Armstrong <narmstrong@baylibre.com> wrote: > On 03/07/2019 01:16, Martin Blumenstingl wrote: >> +Cc Alexandre Mergnat >> >> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote: >>> >>> Add a setup() callback in the eeclk structure, to call an optional >>> call() function at end of eeclk probe to setup clocks. >>> >>> It's used for the G12A clock controller to setup the CPU clock notifiers. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> this will probably work fine, but I want do double check first >> >> are we planning to get rid of meson-eeclk (mid-term)? > > AFAIK no, but maybe I'm not aware of it ! > > Neil > >> Alex has some patches to get rid of all these IN_PREFIX logic. The prefix logic will go away with Alex's rework, so are the input clock But meson-eeclk, which is just a common probe function do avoid repeating the same things over and over, will stay >> I'm asking because if we want to get rid of meson-eeclk it may be the May I ask why ? >> time to do so now to have less logic to migrate later on >> >> >> Martin >>
On Wed, Jul 3, 2019 at 2:40 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Wed 03 Jul 2019 at 13:45, Neil Armstrong <narmstrong@baylibre.com> wrote: > > > On 03/07/2019 01:16, Martin Blumenstingl wrote: > >> +Cc Alexandre Mergnat > >> > >> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > >>> > >>> Add a setup() callback in the eeclk structure, to call an optional > >>> call() function at end of eeclk probe to setup clocks. > >>> > >>> It's used for the G12A clock controller to setup the CPU clock notifiers. > >>> > >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> this will probably work fine, but I want do double check first > >> > >> are we planning to get rid of meson-eeclk (mid-term)? > > > > AFAIK no, but maybe I'm not aware of it ! > > > > Neil > > > >> Alex has some patches to get rid of all these IN_PREFIX logic. > > The prefix logic will go away with Alex's rework, so are the input clock > But meson-eeclk, which is just a common probe function do avoid > repeating the same things over and over, will stay OK, thank you for clarifying this > >> I'm asking because if we want to get rid of meson-eeclk it may be the > > May I ask why ? I only remember that Stephen asked us to get rid of something in our clock code I was under the impression that it was meson-eeclk, but I cannot find it anymore (that means I'm mixing it up with some other topic)
On Mon 01 Jul 2019 at 11:12, Neil Armstrong <narmstrong@baylibre.com> wrote: > Add a setup() callback in the eeclk structure, to call an optional > call() function at end of eeclk probe to setup clocks. > > It's used for the G12A clock controller to setup the CPU clock > notifiers. I'd prefer if you implement the probe function in the related controller have this probe function call meson_eeclkc_probe() for the common part In your case, I suppose it means implementing the g12a controller probe to deal with the notifiers > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/clk/meson/meson-eeclk.c | 6 ++++++ > drivers/clk/meson/meson-eeclk.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c > index 6ba2094be257..81fd2abcd173 100644 > --- a/drivers/clk/meson/meson-eeclk.c > +++ b/drivers/clk/meson/meson-eeclk.c > @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev) > } > } > > + if (data->setup) { > + ret = data->setup(pdev); > + if (ret) > + return ret; > + } > + > return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > data->hw_onecell_data); > } > diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h > index 9ab5d6fa7ccb..7fdf424f71a6 100644 > --- a/drivers/clk/meson/meson-eeclk.h > +++ b/drivers/clk/meson/meson-eeclk.h > @@ -20,6 +20,7 @@ struct meson_eeclkc_data { > const struct reg_sequence *init_regs; > unsigned int init_count; > struct clk_hw_onecell_data *hw_onecell_data; > + int (*setup)(struct platform_device *pdev); > }; > > int meson_eeclkc_probe(struct platform_device *pdev); > -- > 2.21.0
On 03/07/2019 16:17, Jerome Brunet wrote: > On Mon 01 Jul 2019 at 11:12, Neil Armstrong <narmstrong@baylibre.com> wrote: > >> Add a setup() callback in the eeclk structure, to call an optional >> call() function at end of eeclk probe to setup clocks. >> >> It's used for the G12A clock controller to setup the CPU clock >> notifiers. > > I'd prefer if you implement the probe function in the related controller > have this probe function call meson_eeclkc_probe() for the common part > > In your case, I suppose it means implementing the g12a controller probe > to deal with the notifiers Sure, but with this eeclk setup callback I can provide a different setup() callback for g12a and g12b (and later sm1), without this means adding a top data struct containing a setup() callback pointer and the soc meson_eeclkc_data struct to be able to call a setup() for each family like done actually, but this will broke eeclk since the match_data data won't be a meson_eeclkc_data() struct anymore. If you still prefer this, I can rework it like that. I'm rebasing all the stuff on v5.3-rc1 and plan to repost an updated version shortly, solving this would be easier. Neil > >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/clk/meson/meson-eeclk.c | 6 ++++++ >> drivers/clk/meson/meson-eeclk.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c >> index 6ba2094be257..81fd2abcd173 100644 >> --- a/drivers/clk/meson/meson-eeclk.c >> +++ b/drivers/clk/meson/meson-eeclk.c >> @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev) >> } >> } >> >> + if (data->setup) { >> + ret = data->setup(pdev); >> + if (ret) >> + return ret; >> + } >> + >> return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, >> data->hw_onecell_data); >> } >> diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h >> index 9ab5d6fa7ccb..7fdf424f71a6 100644 >> --- a/drivers/clk/meson/meson-eeclk.h >> +++ b/drivers/clk/meson/meson-eeclk.h >> @@ -20,6 +20,7 @@ struct meson_eeclkc_data { >> const struct reg_sequence *init_regs; >> unsigned int init_count; >> struct clk_hw_onecell_data *hw_onecell_data; >> + int (*setup)(struct platform_device *pdev); >> }; >> >> int meson_eeclkc_probe(struct platform_device *pdev); >> -- >> 2.21.0
On Fri 26 Jul 2019 at 16:50, Neil Armstrong <narmstrong@baylibre.com> wrote: > On 03/07/2019 16:17, Jerome Brunet wrote: >> On Mon 01 Jul 2019 at 11:12, Neil Armstrong <narmstrong@baylibre.com> wrote: >> >>> Add a setup() callback in the eeclk structure, to call an optional >>> call() function at end of eeclk probe to setup clocks. >>> >>> It's used for the G12A clock controller to setup the CPU clock >>> notifiers. >> >> I'd prefer if you implement the probe function in the related controller >> have this probe function call meson_eeclkc_probe() for the common part >> >> In your case, I suppose it means implementing the g12a controller probe >> to deal with the notifiers > > Sure, but with this eeclk setup callback I can provide a different setup() callback > for g12a and g12b (and later sm1), without this means adding a top data struct > containing a setup() callback pointer and the soc meson_eeclkc_data struct to be able > to call a setup() for each family like done actually, but this will broke eeclk since > the match_data data won't be a meson_eeclkc_data() struct anymore. meson_eeclkc_probe is an helper we added to factorize common code out of the clock controllers we had. I don't like the idea to now add callback in it deal with the specifics of each SoCs. It feels like we are going in circles. I think SoC/controller specific stuff should be dealt with in related probe function of each clock controller which would then call the 'common helper' if necessary. If the common part interface needs to be reworked, maybe changing the parameters, I'm ok with it. > > If you still prefer this, I can rework it like that. > > I'm rebasing all the stuff on v5.3-rc1 and plan to repost an updated version > shortly, solving this would be easier. > > Neil > >> >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/clk/meson/meson-eeclk.c | 6 ++++++ >>> drivers/clk/meson/meson-eeclk.h | 1 + >>> 2 files changed, 7 insertions(+) >>> >>> diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c >>> index 6ba2094be257..81fd2abcd173 100644 >>> --- a/drivers/clk/meson/meson-eeclk.c >>> +++ b/drivers/clk/meson/meson-eeclk.c >>> @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev) >>> } >>> } >>> >>> + if (data->setup) { >>> + ret = data->setup(pdev); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, >>> data->hw_onecell_data); >>> } >>> diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h >>> index 9ab5d6fa7ccb..7fdf424f71a6 100644 >>> --- a/drivers/clk/meson/meson-eeclk.h >>> +++ b/drivers/clk/meson/meson-eeclk.h >>> @@ -20,6 +20,7 @@ struct meson_eeclkc_data { >>> const struct reg_sequence *init_regs; >>> unsigned int init_count; >>> struct clk_hw_onecell_data *hw_onecell_data; >>> + int (*setup)(struct platform_device *pdev); >>> }; >>> >>> int meson_eeclkc_probe(struct platform_device *pdev); >>> -- >>> 2.21.0
diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c index 6ba2094be257..81fd2abcd173 100644 --- a/drivers/clk/meson/meson-eeclk.c +++ b/drivers/clk/meson/meson-eeclk.c @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev) } } + if (data->setup) { + ret = data->setup(pdev); + if (ret) + return ret; + } + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data->hw_onecell_data); } diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h index 9ab5d6fa7ccb..7fdf424f71a6 100644 --- a/drivers/clk/meson/meson-eeclk.h +++ b/drivers/clk/meson/meson-eeclk.h @@ -20,6 +20,7 @@ struct meson_eeclkc_data { const struct reg_sequence *init_regs; unsigned int init_count; struct clk_hw_onecell_data *hw_onecell_data; + int (*setup)(struct platform_device *pdev); }; int meson_eeclkc_probe(struct platform_device *pdev);
Add a setup() callback in the eeclk structure, to call an optional call() function at end of eeclk probe to setup clocks. It's used for the G12A clock controller to setup the CPU clock notifiers. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/meson/meson-eeclk.c | 6 ++++++ drivers/clk/meson/meson-eeclk.h | 1 + 2 files changed, 7 insertions(+)