| Message ID | 20240705071419.11521-1-michael@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi <michael@amarulasolutions.com> wrote: > > Gate and mux does not have .set_rate operation, but they could have > CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a > parent up the tree which is capable of setting the rate (div, pll, etc). > Add clk_generic_set_rate to allow them to trasverse the clock tree. > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > --- > drivers/clk/clk-gate.c | 1 + > drivers/clk/clk-mux.c | 2 +- > drivers/clk/clk-uclass.c | 20 ++++++++++++++++++++ > drivers/clk/clk.c | 9 +++++++++ > include/clk.h | 9 +++++++++ > include/linux/clk-provider.h | 1 + > 6 files changed, 41 insertions(+), 1 deletion(-) > --- > V1->V2: > Fix missing include > --- > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > index cfd90b717e7..c86083ac5a3 100644 > --- a/drivers/clk/clk-gate.c > +++ b/drivers/clk/clk-gate.c > @@ -116,6 +116,7 @@ const struct clk_ops clk_gate_ops = { > .enable = clk_gate_enable, > .disable = clk_gate_disable, > .get_rate = clk_generic_get_rate, > + .set_rate = clk_generic_set_rate, I can see that clk_generic_get_rate() already exists here, and adding clk_generic_set_rate() tries to mimic that existing design. But frankly, I'm not sure it was a very good design choice in the first place. I mean, to have get/set rate operations for the clock types that clearly don't have such ability. I think it would be better to model the CCF clocks after real world clocks, and handle the propagation on the clock logic level. In fact, that's how it's already implemented in Linux kernel [1]. That keeps the particular clock implementations simple and brings the propagation logic to the actual clk API (clk_set_rate()), which in turn is able to cover all possible cases: even if some new clock types are implemented later, they will be covered already. That also reduces some code duplication and makes it easier to control that behavior in a centralized manner. The patch with the discussed implementation was already submitted a while ago [2], and still pending on the review. Can I ask you if patch [2] doesn't cover some of your cases? Or maybe I'm missing something out and having .set_rate op in gate/mux would actually be better for some particular reason? Thanks! [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-gate.c#n120 [2] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html > }; > > struct clk *clk_register_gate(struct device *dev, const char *name, > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c > index e3481be95fa..f99a67ebd35 100644 > --- a/drivers/clk/clk-mux.c > +++ b/drivers/clk/clk-mux.c > @@ -151,13 +151,13 @@ static int clk_mux_set_parent(struct clk *clk, struct clk *parent) > #else > writel(reg, mux->reg); > #endif > - > return 0; > } > > const struct clk_ops clk_mux_ops = { > .get_rate = clk_generic_get_rate, > .set_parent = clk_mux_set_parent, > + .set_rate = clk_generic_set_rate, > }; > > struct clk *clk_hw_register_mux_table(struct device *dev, const char *name, > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index ed6e60bc484..638864e6774 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -517,6 +517,26 @@ ulong clk_get_parent_rate(struct clk *clk) > return pclk->rate; > } > > +ulong clk_set_parent_rate(struct clk *clk, ulong rate) > +{ > + const struct clk_ops *ops; > + struct clk *pclk; > + > + debug("%s(clk=%p)\n", __func__, clk); > + if (!clk_valid(clk)) > + return 0; > + > + pclk = clk_get_parent(clk); > + if (IS_ERR(pclk)) > + return -ENODEV; > + > + ops = clk_dev_ops(pclk->dev); > + if (!ops->set_rate) > + return -ENOSYS; > + > + return clk_set_rate(pclk, rate); > +} > + > ulong clk_round_rate(struct clk *clk, ulong rate) > { > const struct clk_ops *ops; > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 6ede1b4d4dc..febd5314df2 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -14,6 +14,7 @@ > #include <dm/uclass.h> > #include <dm/lists.h> > #include <dm/device-internal.h> > +#include <linux/clk-provider.h> > > int clk_register(struct clk *clk, const char *drv_name, > const char *name, const char *parent_name) > @@ -61,6 +62,14 @@ ulong clk_generic_get_rate(struct clk *clk) > return clk_get_parent_rate(clk); > } > > +ulong clk_generic_set_rate(struct clk *clk, ulong rate) > +{ > + if (clk->flags & CLK_SET_RATE_PARENT) > + return clk_set_parent_rate(clk, rate); > + > + return clk_get_parent_rate(clk); > +} > + > const char *clk_hw_get_name(const struct clk *hw) > { > assert(hw); > diff --git a/include/clk.h b/include/clk.h > index af23e4f3475..1900377eddb 100644 > --- a/include/clk.h > +++ b/include/clk.h > @@ -452,6 +452,15 @@ struct clk *clk_get_parent(struct clk *clk); > */ > ulong clk_get_parent_rate(struct clk *clk); > > +/** > + * clk_set_parent_rate() - Set parent of current clock rate. > + * @clk: A clock struct that was previously successfully requested by > + * clk_request/get_by_*(). > + * > + * Return: clock rate in Hz, or -ve error code. > + */ > +ulong clk_set_parent_rate(struct clk *clk, ulong rate); > + > /** > * clk_round_rate() - Adjust a rate to the exact rate a clock can provide > * @clk: A clock struct that was previously successfully requested by > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 59f9c241b84..459fa2d15ce 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -253,6 +253,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name, > > const char *clk_hw_get_name(const struct clk *hw); > ulong clk_generic_get_rate(struct clk *clk); > +ulong clk_generic_set_rate(struct clk *clk, ulong rate); > > struct clk *dev_get_clk_ptr(struct udevice *dev); > > -- > 2.43.0 > To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi Sam On Tue, Jul 23, 2024 at 10:37 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi > <michael@amarulasolutions.com> wrote: > > > > Gate and mux does not have .set_rate operation, but they could have > > CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a > > parent up the tree which is capable of setting the rate (div, pll, etc). > > Add clk_generic_set_rate to allow them to trasverse the clock tree. > > > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > --- > > drivers/clk/clk-gate.c | 1 + > > drivers/clk/clk-mux.c | 2 +- > > drivers/clk/clk-uclass.c | 20 ++++++++++++++++++++ > > drivers/clk/clk.c | 9 +++++++++ > > include/clk.h | 9 +++++++++ > > include/linux/clk-provider.h | 1 + > > 6 files changed, 41 insertions(+), 1 deletion(-) > > --- > > V1->V2: > > Fix missing include > > --- > > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > > index cfd90b717e7..c86083ac5a3 100644 > > --- a/drivers/clk/clk-gate.c > > +++ b/drivers/clk/clk-gate.c > > @@ -116,6 +116,7 @@ const struct clk_ops clk_gate_ops = { > > .enable = clk_gate_enable, > > .disable = clk_gate_disable, > > .get_rate = clk_generic_get_rate, > > + .set_rate = clk_generic_set_rate, > > I can see that clk_generic_get_rate() already exists here, and adding > clk_generic_set_rate() tries to mimic that existing design. But > frankly, I'm not sure it was a very good design choice in the first > place. I mean, to have get/set rate operations for the clock types > that clearly don't have such ability. I think it would be better to > model the CCF clocks after real world clocks, and handle the > propagation on the clock logic level. In fact, that's how it's already > implemented in Linux kernel [1]. That keeps the particular clock Ok , so you suggest to have something like in the core if (p->set_rate) { set_rate()) } else { set_generic_rate() } The same for get_rate() > implementations simple and brings the propagation logic to the actual > clk API (clk_set_rate()), which in turn is able to cover all possible > cases: even if some new clock types are implemented later, they will > be covered already. That also reduces some code duplication and makes > it easier to control that behavior in a centralized manner. The patch > with the discussed implementation was already submitted a while ago > [2], and still pending on the review. I have seen it and because I'm extending the clk framework a bit more [1] I was thinking about a more complete approach. Nothing against your patch, but just I have observed in my use case (display) that I need more from the clock framework we have Michael [1] https://patchwork.amarulasolutions.com/patch/3291/ > > Can I ask you if patch [2] doesn't cover some of your cases? Or maybe > I'm missing something out and having .set_rate op in gate/mux would > actually be better for some particular reason? > > Thanks! > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-gate.c#n120 > [2] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html > > > > }; > > > > struct clk *clk_register_gate(struct device *dev, const char *name, > > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c > > index e3481be95fa..f99a67ebd35 100644 > > --- a/drivers/clk/clk-mux.c > > +++ b/drivers/clk/clk-mux.c > > @@ -151,13 +151,13 @@ static int clk_mux_set_parent(struct clk *clk, struct clk *parent) > > #else > > writel(reg, mux->reg); > > #endif > > - > > return 0; > > } > > > > const struct clk_ops clk_mux_ops = { > > .get_rate = clk_generic_get_rate, > > .set_parent = clk_mux_set_parent, > > + .set_rate = clk_generic_set_rate, > > }; > > > > struct clk *clk_hw_register_mux_table(struct device *dev, const char *name, > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > index ed6e60bc484..638864e6774 100644 > > --- a/drivers/clk/clk-uclass.c > > +++ b/drivers/clk/clk-uclass.c > > @@ -517,6 +517,26 @@ ulong clk_get_parent_rate(struct clk *clk) > > return pclk->rate; > > } > > > > +ulong clk_set_parent_rate(struct clk *clk, ulong rate) > > +{ > > + const struct clk_ops *ops; > > + struct clk *pclk; > > + > > + debug("%s(clk=%p)\n", __func__, clk); > > + if (!clk_valid(clk)) > > + return 0; > > + > > + pclk = clk_get_parent(clk); > > + if (IS_ERR(pclk)) > > + return -ENODEV; > > + > > + ops = clk_dev_ops(pclk->dev); > > + if (!ops->set_rate) > > + return -ENOSYS; > > + > > + return clk_set_rate(pclk, rate); > > +} > > + > > ulong clk_round_rate(struct clk *clk, ulong rate) > > { > > const struct clk_ops *ops; > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 6ede1b4d4dc..febd5314df2 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -14,6 +14,7 @@ > > #include <dm/uclass.h> > > #include <dm/lists.h> > > #include <dm/device-internal.h> > > +#include <linux/clk-provider.h> > > > > int clk_register(struct clk *clk, const char *drv_name, > > const char *name, const char *parent_name) > > @@ -61,6 +62,14 @@ ulong clk_generic_get_rate(struct clk *clk) > > return clk_get_parent_rate(clk); > > } > > > > +ulong clk_generic_set_rate(struct clk *clk, ulong rate) > > +{ > > + if (clk->flags & CLK_SET_RATE_PARENT) > > + return clk_set_parent_rate(clk, rate); > > + > > + return clk_get_parent_rate(clk); > > +} > > + > > const char *clk_hw_get_name(const struct clk *hw) > > { > > assert(hw); > > diff --git a/include/clk.h b/include/clk.h > > index af23e4f3475..1900377eddb 100644 > > --- a/include/clk.h > > +++ b/include/clk.h > > @@ -452,6 +452,15 @@ struct clk *clk_get_parent(struct clk *clk); > > */ > > ulong clk_get_parent_rate(struct clk *clk); > > > > +/** > > + * clk_set_parent_rate() - Set parent of current clock rate. > > + * @clk: A clock struct that was previously successfully requested by > > + * clk_request/get_by_*(). > > + * > > + * Return: clock rate in Hz, or -ve error code. > > + */ > > +ulong clk_set_parent_rate(struct clk *clk, ulong rate); > > + > > /** > > * clk_round_rate() - Adjust a rate to the exact rate a clock can provide > > * @clk: A clock struct that was previously successfully requested by > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > index 59f9c241b84..459fa2d15ce 100644 > > --- a/include/linux/clk-provider.h > > +++ b/include/linux/clk-provider.h > > @@ -253,6 +253,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name, > > > > const char *clk_hw_get_name(const struct clk *hw); > > ulong clk_generic_get_rate(struct clk *clk); > > +ulong clk_generic_set_rate(struct clk *clk, ulong rate); > > > > struct clk *dev_get_clk_ptr(struct udevice *dev); > > > > -- > > 2.43.0 > >
Hi Sam On Wed, Jul 24, 2024 at 9:25 AM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi Sam > > On Tue, Jul 23, 2024 at 10:37 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > > > On Fri, Jul 5, 2024 at 2:14 AM Michael Trimarchi > > <michael@amarulasolutions.com> wrote: > > > > > > Gate and mux does not have .set_rate operation, but they could have > > > CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a > > > parent up the tree which is capable of setting the rate (div, pll, etc). > > > Add clk_generic_set_rate to allow them to trasverse the clock tree. > > > > > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> > > > --- > > > drivers/clk/clk-gate.c | 1 + > > > drivers/clk/clk-mux.c | 2 +- > > > drivers/clk/clk-uclass.c | 20 ++++++++++++++++++++ > > > drivers/clk/clk.c | 9 +++++++++ > > > include/clk.h | 9 +++++++++ > > > include/linux/clk-provider.h | 1 + > > > 6 files changed, 41 insertions(+), 1 deletion(-) > > > --- > > > V1->V2: > > > Fix missing include > > > --- > > > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > > > index cfd90b717e7..c86083ac5a3 100644 > > > --- a/drivers/clk/clk-gate.c > > > +++ b/drivers/clk/clk-gate.c > > > @@ -116,6 +116,7 @@ const struct clk_ops clk_gate_ops = { > > > .enable = clk_gate_enable, > > > .disable = clk_gate_disable, > > > .get_rate = clk_generic_get_rate, > > > + .set_rate = clk_generic_set_rate, > > > > I can see that clk_generic_get_rate() already exists here, and adding > > clk_generic_set_rate() tries to mimic that existing design. But > > frankly, I'm not sure it was a very good design choice in the first > > place. I mean, to have get/set rate operations for the clock types > > that clearly don't have such ability. I think it would be better to > > model the CCF clocks after real world clocks, and handle the > > propagation on the clock logic level. In fact, that's how it's already > > implemented in Linux kernel [1]. That keeps the particular clock > > Ok , so you suggest to have something like in the core > > if (p->set_rate) > { > set_rate()) > > } > else > { > set_generic_rate() > } > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 8faf5a56e1..a309108dd4 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -467,8 +467,14 @@ ulong clk_get_rate(struct clk *clk) return 0; ops = clk_dev_ops(clk->dev); - if (!ops->get_rate) - return -ENOSYS; + if (!ops->get_rate) { + parent = clk_get_parent(clk); + if (!clk_is_valid(parent)) + return -ENOSYS; + ops = clk_dev_ops(parent->dev); + if (!ops->get_rate) + return -ENOSYS; + } return ops->get_rate(clk); } You suggest to remove get_rate, set_rate function in mux and gate and use some propagation in the core, am I right? Michael > The same for get_rate() > > > implementations simple and brings the propagation logic to the actual > > clk API (clk_set_rate()), which in turn is able to cover all possible > > cases: even if some new clock types are implemented later, they will > > be covered already. That also reduces some code duplication and makes > > it easier to control that behavior in a centralized manner. The patch > > with the discussed implementation was already submitted a while ago > > [2], and still pending on the review. > > I have seen it and because I'm extending the clk framework a bit more [1] > I was thinking about a more complete approach. Nothing against your > patch, but just > I have observed in my use case (display) that I need more from the > clock framework we have > > Michael > > [1] https://patchwork.amarulasolutions.com/patch/3291/ > > > > > Can I ask you if patch [2] doesn't cover some of your cases? Or maybe > > I'm missing something out and having .set_rate op in gate/mux would > > actually be better for some particular reason? > > > > Thanks! > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-gate.c#n120 > > [2] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html > > > > > > > }; > > > > > > struct clk *clk_register_gate(struct device *dev, const char *name, > > > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c > > > index e3481be95fa..f99a67ebd35 100644 > > > --- a/drivers/clk/clk-mux.c > > > +++ b/drivers/clk/clk-mux.c > > > @@ -151,13 +151,13 @@ static int clk_mux_set_parent(struct clk *clk, struct clk *parent) > > > #else > > > writel(reg, mux->reg); > > > #endif > > > - > > > return 0; > > > } > > > > > > const struct clk_ops clk_mux_ops = { > > > .get_rate = clk_generic_get_rate, > > > .set_parent = clk_mux_set_parent, > > > + .set_rate = clk_generic_set_rate, > > > }; > > > > > > struct clk *clk_hw_register_mux_table(struct device *dev, const char *name, > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > > index ed6e60bc484..638864e6774 100644 > > > --- a/drivers/clk/clk-uclass.c > > > +++ b/drivers/clk/clk-uclass.c > > > @@ -517,6 +517,26 @@ ulong clk_get_parent_rate(struct clk *clk) > > > return pclk->rate; > > > } > > > > > > +ulong clk_set_parent_rate(struct clk *clk, ulong rate) > > > +{ > > > + const struct clk_ops *ops; > > > + struct clk *pclk; > > > + > > > + debug("%s(clk=%p)\n", __func__, clk); > > > + if (!clk_valid(clk)) > > > + return 0; > > > + > > > + pclk = clk_get_parent(clk); > > > + if (IS_ERR(pclk)) > > > + return -ENODEV; > > > + > > > + ops = clk_dev_ops(pclk->dev); > > > + if (!ops->set_rate) > > > + return -ENOSYS; > > > + > > > + return clk_set_rate(pclk, rate); > > > +} > > > + > > > ulong clk_round_rate(struct clk *clk, ulong rate) > > > { > > > const struct clk_ops *ops; > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 6ede1b4d4dc..febd5314df2 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -14,6 +14,7 @@ > > > #include <dm/uclass.h> > > > #include <dm/lists.h> > > > #include <dm/device-internal.h> > > > +#include <linux/clk-provider.h> > > > > > > int clk_register(struct clk *clk, const char *drv_name, > > > const char *name, const char *parent_name) > > > @@ -61,6 +62,14 @@ ulong clk_generic_get_rate(struct clk *clk) > > > return clk_get_parent_rate(clk); > > > } > > > > > > +ulong clk_generic_set_rate(struct clk *clk, ulong rate) > > > +{ > > > + if (clk->flags & CLK_SET_RATE_PARENT) > > > + return clk_set_parent_rate(clk, rate); > > > + > > > + return clk_get_parent_rate(clk); > > > +} > > > + > > > const char *clk_hw_get_name(const struct clk *hw) > > > { > > > assert(hw); > > > diff --git a/include/clk.h b/include/clk.h > > > index af23e4f3475..1900377eddb 100644 > > > --- a/include/clk.h > > > +++ b/include/clk.h > > > @@ -452,6 +452,15 @@ struct clk *clk_get_parent(struct clk *clk); > > > */ > > > ulong clk_get_parent_rate(struct clk *clk); > > > > > > +/** > > > + * clk_set_parent_rate() - Set parent of current clock rate. > > > + * @clk: A clock struct that was previously successfully requested by > > > + * clk_request/get_by_*(). > > > + * > > > + * Return: clock rate in Hz, or -ve error code. > > > + */ > > > +ulong clk_set_parent_rate(struct clk *clk, ulong rate); > > > + > > > /** > > > * clk_round_rate() - Adjust a rate to the exact rate a clock can provide > > > * @clk: A clock struct that was previously successfully requested by > > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > > > index 59f9c241b84..459fa2d15ce 100644 > > > --- a/include/linux/clk-provider.h > > > +++ b/include/linux/clk-provider.h > > > @@ -253,6 +253,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name, > > > > > > const char *clk_hw_get_name(const struct clk *hw); > > > ulong clk_generic_get_rate(struct clk *clk); > > > +ulong clk_generic_set_rate(struct clk *clk, ulong rate); > > > > > > struct clk *dev_get_clk_ptr(struct udevice *dev); > > > > > > -- > > > 2.43.0 > > > > > > > -- > Michael Nazzareno Trimarchi > Co-Founder & Chief Executive Officer > M. +39 347 913 2170 > michael@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > info@amarulasolutions.com > www.amarulasolutions.com
On Wed, Jul 24, 2024 at 4:44 AM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: [snip] > > Ok , so you suggest to have something like in the core > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index 8faf5a56e1..a309108dd4 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -467,8 +467,14 @@ ulong clk_get_rate(struct clk *clk) > return 0; > ops = clk_dev_ops(clk->dev); > > - if (!ops->get_rate) > - return -ENOSYS; > + if (!ops->get_rate) { > + parent = clk_get_parent(clk); > + if (!clk_is_valid(parent)) > + return -ENOSYS; > + ops = clk_dev_ops(parent->dev); > + if (!ops->get_rate) > + return -ENOSYS; > + } > > return ops->get_rate(clk); > } > > You suggest to remove get_rate, set_rate function in mux and gate and > use some propagation in the core, am I right? Something like that, yes. But maybe instead of if (!ops->get_rate) use something like: while (!ops->get_rate) like I did in my patch [1] for clk_set_rate(). That while loop is needed to handle cases when there are multiple clocks without .get_rate() operation connected together (like mux or gate clocks). So you'd have to iterate them all up the tree to find some DIV clock which actually has .get_rate() defined. But at the moment I'm more concerned about .set_rate() propagation of course, which is really needed for my E850-96 MMC enablement series [2], which has already been pending on the review for a while. [1] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html [2] https://lists.denx.de/pipermail/u-boot/2024-July/559602.html [snip] > > > implementations simple and brings the propagation logic to the actual > > > clk API (clk_set_rate()), which in turn is able to cover all possible > > > cases: even if some new clock types are implemented later, they will > > > be covered already. That also reduces some code duplication and makes > > > it easier to control that behavior in a centralized manner. The patch > > > with the discussed implementation was already submitted a while ago > > > [2], and still pending on the review. > > > > I have seen it and because I'm extending the clk framework a bit more [1] > > I was thinking about a more complete approach. Nothing against your > > patch, but just > > I have observed in my use case (display) that I need more from the > > clock framework we have > > I see. Do you think it'd possible for you to use my patch for clk_set_rate() propagation for your case, and base your patches (for CLK_OPS_PARENT_ENABLE, etc) on top of that? Will it cover your requirements? Would be also nice to hear what maintainers think about that :) Thanks! > > Michael > > > > [1] https://patchwork.amarulasolutions.com/patch/3291/ > > > > > [snip] To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Hi Sam On Thu, Jul 25, 2024 at 4:41 AM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, Jul 24, 2024 at 4:44 AM Michael Nazzareno Trimarchi > <michael@amarulasolutions.com> wrote: > > [snip] > > > > Ok , so you suggest to have something like in the core > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > index 8faf5a56e1..a309108dd4 100644 > > --- a/drivers/clk/clk-uclass.c > > +++ b/drivers/clk/clk-uclass.c > > @@ -467,8 +467,14 @@ ulong clk_get_rate(struct clk *clk) > > return 0; > > ops = clk_dev_ops(clk->dev); > > > > - if (!ops->get_rate) > > - return -ENOSYS; > > + if (!ops->get_rate) { > > + parent = clk_get_parent(clk); > > + if (!clk_is_valid(parent)) > > + return -ENOSYS; > > + ops = clk_dev_ops(parent->dev); > > + if (!ops->get_rate) > > + return -ENOSYS; > > + } > > > > return ops->get_rate(clk); > > } > > > > You suggest to remove get_rate, set_rate function in mux and gate and > > use some propagation in the core, am I right? > > Something like that, yes. But maybe instead of > > if (!ops->get_rate) > > use something like: > > while (!ops->get_rate) > > like I did in my patch [1] for clk_set_rate(). That while loop is > needed to handle cases when there are multiple clocks without > .get_rate() operation connected together (like mux or gate clocks). So > you'd have to iterate them all up the tree to find some DIV clock > which actually has .get_rate() defined. > > But at the moment I'm more concerned about .set_rate() propagation of > course, which is really needed for my E850-96 MMC enablement series > [2], which has already been pending on the review for a while. > > [1] https://lists.denx.de/pipermail/u-boot/2024-March/547719.html > [2] https://lists.denx.de/pipermail/u-boot/2024-July/559602.html > > [snip] > > > > > implementations simple and brings the propagation logic to the actual > > > > clk API (clk_set_rate()), which in turn is able to cover all possible > > > > cases: even if some new clock types are implemented later, they will > > > > be covered already. That also reduces some code duplication and makes > > > > it easier to control that behavior in a centralized manner. The patch > > > > with the discussed implementation was already submitted a while ago > > > > [2], and still pending on the review. > > > > > > I have seen it and because I'm extending the clk framework a bit more [1] > > > I was thinking about a more complete approach. Nothing against your > > > patch, but just > > > I have observed in my use case (display) that I need more from the > > > clock framework we have > > > > > I see. Do you think it'd possible for you to use my patch for > clk_set_rate() propagation for your case, and base your patches (for > CLK_OPS_PARENT_ENABLE, etc) on top of that? Will it cover your > requirements? > If I remember how it works now we don't need the loop. Each get_clk_rate or clk_set_rate should implement propagation in their implementation if the CLK PARENT SET is present Michael > Would be also nice to hear what maintainers think about that :) > > Thanks! > > > > Michael > > > > > > [1] https://patchwork.amarulasolutions.com/patch/3291/ > > > > > > > > > [snip]
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index cfd90b717e7..c86083ac5a3 100644 --- a/drivers/clk/clk-gate.c +++ b/drivers/clk/clk-gate.c @@ -116,6 +116,7 @@ const struct clk_ops clk_gate_ops = { .enable = clk_gate_enable, .disable = clk_gate_disable, .get_rate = clk_generic_get_rate, + .set_rate = clk_generic_set_rate, }; struct clk *clk_register_gate(struct device *dev, const char *name, diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index e3481be95fa..f99a67ebd35 100644 --- a/drivers/clk/clk-mux.c +++ b/drivers/clk/clk-mux.c @@ -151,13 +151,13 @@ static int clk_mux_set_parent(struct clk *clk, struct clk *parent) #else writel(reg, mux->reg); #endif - return 0; } const struct clk_ops clk_mux_ops = { .get_rate = clk_generic_get_rate, .set_parent = clk_mux_set_parent, + .set_rate = clk_generic_set_rate, }; struct clk *clk_hw_register_mux_table(struct device *dev, const char *name, diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index ed6e60bc484..638864e6774 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -517,6 +517,26 @@ ulong clk_get_parent_rate(struct clk *clk) return pclk->rate; } +ulong clk_set_parent_rate(struct clk *clk, ulong rate) +{ + const struct clk_ops *ops; + struct clk *pclk; + + debug("%s(clk=%p)\n", __func__, clk); + if (!clk_valid(clk)) + return 0; + + pclk = clk_get_parent(clk); + if (IS_ERR(pclk)) + return -ENODEV; + + ops = clk_dev_ops(pclk->dev); + if (!ops->set_rate) + return -ENOSYS; + + return clk_set_rate(pclk, rate); +} + ulong clk_round_rate(struct clk *clk, ulong rate) { const struct clk_ops *ops; diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 6ede1b4d4dc..febd5314df2 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -14,6 +14,7 @@ #include <dm/uclass.h> #include <dm/lists.h> #include <dm/device-internal.h> +#include <linux/clk-provider.h> int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) @@ -61,6 +62,14 @@ ulong clk_generic_get_rate(struct clk *clk) return clk_get_parent_rate(clk); } +ulong clk_generic_set_rate(struct clk *clk, ulong rate) +{ + if (clk->flags & CLK_SET_RATE_PARENT) + return clk_set_parent_rate(clk, rate); + + return clk_get_parent_rate(clk); +} + const char *clk_hw_get_name(const struct clk *hw) { assert(hw); diff --git a/include/clk.h b/include/clk.h index af23e4f3475..1900377eddb 100644 --- a/include/clk.h +++ b/include/clk.h @@ -452,6 +452,15 @@ struct clk *clk_get_parent(struct clk *clk); */ ulong clk_get_parent_rate(struct clk *clk); +/** + * clk_set_parent_rate() - Set parent of current clock rate. + * @clk: A clock struct that was previously successfully requested by + * clk_request/get_by_*(). + * + * Return: clock rate in Hz, or -ve error code. + */ +ulong clk_set_parent_rate(struct clk *clk, ulong rate); + /** * clk_round_rate() - Adjust a rate to the exact rate a clock can provide * @clk: A clock struct that was previously successfully requested by diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 59f9c241b84..459fa2d15ce 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -253,6 +253,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name, const char *clk_hw_get_name(const struct clk *hw); ulong clk_generic_get_rate(struct clk *clk); +ulong clk_generic_set_rate(struct clk *clk, ulong rate); struct clk *dev_get_clk_ptr(struct udevice *dev);
Gate and mux does not have .set_rate operation, but they could have CLK_SET_PARENT_RATE flag set. In that case it's usually possible to find a parent up the tree which is capable of setting the rate (div, pll, etc). Add clk_generic_set_rate to allow them to trasverse the clock tree. Cc: Sam Protsenko <semen.protsenko@linaro.org> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com> --- drivers/clk/clk-gate.c | 1 + drivers/clk/clk-mux.c | 2 +- drivers/clk/clk-uclass.c | 20 ++++++++++++++++++++ drivers/clk/clk.c | 9 +++++++++ include/clk.h | 9 +++++++++ include/linux/clk-provider.h | 1 + 6 files changed, 41 insertions(+), 1 deletion(-) --- V1->V2: Fix missing include ---