Re: [PATCH v2 01/11] clk: Always use the supplied struct clk

Hi Sean
From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Sean Anderson Sent: Thursday, January 16, 2020 6:48 AM To: U-Boot Mailing List Subject: [PATCH v2 01/11] clk: Always use the supplied struct clk
CCF clocks should always use the struct clock passed to their methods for extracting the driver-specific clock information struct. Previously, many functions would use the clk->dev->priv if the device was bound. This could cause problems with composite clocks. The individual clocks in a composite clock did not have the ->dev field filled in. This was fine, because the device-specific clock information would be used. However, since there was no ->dev, there was no way to get the parent clock. This caused the recalc_rate method of the CCF divider clock to fail. One option would be to use the clk->priv field to get the composite clock and from there get the appropriate parent device. However, this would tie the implementation to the composite clock. In general, different devices should not rely on the contents of ->priv from another device.
The simple solution to this problem is to just always use the supplied struct clock. The composite clock now fills in the ->dev pointer of its child clocks. This allows child clocks to make calls like clk_get_parent() without issue.
imx avoided the above problem by using a custom get_rate function with composite clocks.
Signed-off-by: Sean Anderson seanga2@gmail.com
drivers/clk/clk-composite.c | 8 ++++++++ drivers/clk/clk-divider.c | 6 ++---- drivers/clk/clk-fixed-factor.c | 3 +-- drivers/clk/clk-gate.c | 6 ++---- drivers/clk/clk-mux.c | 12 ++++-------- drivers/clk/imx/clk-gate2.c | 4 ++-- 6 files changed, 19 insertions(+), 20 deletions(-)
This patch is clock relative patch, if it is not a necessary patch for Sipeed Maix support. Please remove patch 1/11 and 2/11, and send them in another patch-sets.
Rick, Thanks
diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c index a5626c33d1..d0f273d47f 100644 --- a/drivers/clk/clk-composite.c +++ b/drivers/clk/clk-composite.c @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device *dev, const char *name, goto err; }
if (composite->mux)
composite->mux->dev = clk->dev;
if (composite->rate)
composite->rate->dev = clk->dev;
if (composite->gate)
composite->gate->dev = clk->dev;
return clk;
err: diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 822e09b084..bfa05f24a3 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw, unsigned long parent_rate,
static ulong clk_divider_recalc_rate(struct clk *clk) {
struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_divider *divider = to_clk_divider(clk); unsigned long parent_rate = clk_get_parent_rate(clk); unsigned int val;
@@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
static ulong clk_divider_set_rate(struct clk *clk, unsigned long rate) {
struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_divider *divider = to_clk_divider(clk); unsigned long parent_rate = clk_get_parent_rate(clk); int value; u32 val;
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 711b0588bc..d2401cf440 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -18,8 +18,7 @@
static ulong clk_factor_recalc_rate(struct clk *clk) {
struct clk_fixed_factor *fix =
to_clk_fixed_factor(dev_get_clk_ptr(clk->dev));
struct clk_fixed_factor *fix = to_clk_fixed_factor(clk); unsigned long parent_rate = clk_get_parent_rate(clk); unsigned long long int rate;
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index 70b8794554..b2933bc24a 100644 --- a/drivers/clk/clk-gate.c +++ b/drivers/clk/clk-gate.c @@ -43,8 +43,7 @@ */ static void clk_gate_endisable(struct clk *clk, int enable) {
struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_gate *gate = to_clk_gate(clk); int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0; u32 reg;
@@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk)
int clk_gate_is_enabled(struct clk *clk) {
struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_gate *gate = to_clk_gate(clk); u32 reg;
#if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF) diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index 5acc0b8cbd..67b4afef28 100644 --- a/drivers/clk/clk-mux.c +++ b/drivers/clk/clk-mux.c @@ -35,8 +35,7 @@ int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int flags, unsigned int val) {
struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_mux *mux = to_clk_mux(clk); int num_parents = mux->num_parents; if (table) {
@@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index)
u8 clk_mux_get_parent(struct clk *clk) {
struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_mux *mux = to_clk_mux(clk); u32 val;
#if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF) @@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk) static int clk_fetch_parent_index(struct clk *clk, struct clk *parent) {
struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_mux *mux = to_clk_mux(clk); int i;
@@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk,
static int clk_mux_set_parent(struct clk *clk, struct clk *parent) {
struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
dev_get_clk_ptr(clk->dev) : clk);
struct clk_mux *mux = to_clk_mux(clk); int index; u32 val; u32 reg;
diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c index 1b9db6e791..e32c0cb53e 100644 --- a/drivers/clk/imx/clk-gate2.c +++ b/drivers/clk/imx/clk-gate2.c @@ -37,7 +37,7 @@ struct clk_gate2 {
static int clk_gate2_enable(struct clk *clk) {
struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
struct clk_gate2 *gate = to_clk_gate2(clk); u32 reg; reg = readl(gate->reg);
@@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk)
static int clk_gate2_disable(struct clk *clk) {
struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
struct clk_gate2 *gate = to_clk_gate2(clk); u32 reg; reg = readl(gate->reg);
-- 2.24.1

This patch is clock relative patch, if it is not a necessary patch for Sipeed Maix support. Please remove patch 1/11 and 2/11, and send them in another patch-sets.
Patches 10 and 11 rely on these first two patches. Should I just reference the split off patches in the cover letter?

Hi Sean,
This patch is clock relative patch, if it is not a necessary patch for Sipeed Maix support. Please remove patch 1/11 and 2/11, and send them in another patch-sets.
Patches 10 and 11 rely on these first two patches. Should I just reference the split off patches in the cover letter?
I am not sure the modifications of clk are ok for other people. It will be better to send pure riscv relative patch-sets.
Thanks, Rick

I am not sure the modifications of clk are ok for other people. It will be better to send pure riscv relative patch-sets.
Hm, well at the moment removing the dependency on these two patches would probably require a substantial rewrite of patch 11, which would primarily consist of duplicating functionality currently found in the clk_composite driver. I actually think the way clk_composite works at the moment is very confusing, since the imx code which uses it uses some custom functions to smooth out clk_composite's behaviour. These two patches are probably the ones I would like to see merged most of all, since it would make implementing complex clock drivers like on this board much easier. I originally submitted these patches around a month ago [1, 2], so I was hoping that I'd have recieved feedback one way or the other by now.
[1] https://patchwork.ozlabs.org/patch/1215327/ [2] https://patchwork.ozlabs.org/patch/1215328/

[1] https://patchwork.ozlabs.org/patch/1215327/ [2] https://patchwork.ozlabs.org/patch/1215328/
err, these references should be
[1] https://patchwork.ozlabs.org/patch/1215335/ [2] https://patchwork.ozlabs.org/patch/1215337/

Hi Sean,
I am not sure the modifications of clk are ok for other people. It will be better to send pure riscv relative patch-sets.
Hm, well at the moment removing the dependency on these two patches would probably require a substantial rewrite of patch 11, which would primarily consist of duplicating functionality currently found in the clk_composite driver. I actually think the way clk_composite works at the moment is very confusing, since the imx code which uses it uses some custom functions to smooth out clk_composite's behaviour. These two patches are probably the ones I would like to see merged most of all, since it would make implementing complex clock drivers like on this board much easier. I originally submitted these patches around a month ago [1, 2], so I was hoping that I'd have recieved feedback one way or the other by now.
[1] https://patchwork.ozlabs.org/patch/1215327/ [2] https://patchwork.ozlabs.org/patch/1215328/
I saw your patches. Unfortunately, there was the Christmas/New year's break and afterwards I had some more urgent tasks to do. Apologize for that...
What I would like to see here is to reuse (or better - make the code less confusing) the code.
Rationale - the CCF was ported from iMX6Q Linux code from the outset.
Then Peng (CC'ed) wanted to adjust it to support composite clocks from i.MX8.
As a result the CCF drifted to be an iMX aligned, but the goal is to have it usable for other archs as well (and reuse from Linux as much as possible).
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

On 1/24/20 9:27 AM, Lukasz Majewski wrote:
I saw your patches. Unfortunately, there was the Christmas/New year's break and afterwards I had some more urgent tasks to do. Apologize for that...
What I would like to see here is to reuse (or better - make the code less confusing) the code.
Rationale - the CCF was ported from iMX6Q Linux code from the outset.
Then Peng (CC'ed) wanted to adjust it to support composite clocks from i.MX8.
As a result the CCF drifted to be an iMX aligned, but the goal is to have it usable for other archs as well (and reuse from Linux as much as possible).
Ok, so I believe that these patches are a step in that direction. Is there any specific feedback you would like to give regarding them? I've tried to infer what good default behaviour should be. However, I'm sure there are places where someone more familiar with the CCF would be able to comment on.
--Sean

Hi Sean,
On 1/24/20 9:27 AM, Lukasz Majewski wrote:
I saw your patches. Unfortunately, there was the Christmas/New year's break and afterwards I had some more urgent tasks to do. Apologize for that...
What I would like to see here is to reuse (or better - make the code less confusing) the code.
Rationale - the CCF was ported from iMX6Q Linux code from the outset.
Then Peng (CC'ed) wanted to adjust it to support composite clocks from i.MX8.
As a result the CCF drifted to be an iMX aligned, but the goal is to have it usable for other archs as well (and reuse from Linux as much as possible).
Ok, so I believe that these patches are a step in that direction. Is there any specific feedback you would like to give regarding them? I've tried to infer what good default behaviour should be. However, I'm sure there are places where someone more familiar with the CCF would be able to comment on.
I will do the review by the end of the weekend.
--Sean
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
participants (3)
-
Lukasz Majewski
-
Rick Chen
-
Sean Anderson