| Message ID | 20231001-scmi-clock-v2-v3-2-898bd92d8939@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | firmware: arm_scmi: clock: support parents commands | expand |
Hi Stephen, On Sun, Oct 01, 2023 at 12:38:44PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > to support them. > The SCMI changes look good. If you are happy with this driver change, please Ack so that I can take it along with the SCMI changes. There are other patches clk driver patches that you have already acked in my branch, hence the need to route it via SCMI tree.
On Sun, Oct 01, 2023 at 12:38:44PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > to support them. > Hi, a few notes down below. > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/clk-scmi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2e1337b511eb..5aaca674830f 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -24,6 +24,7 @@ struct scmi_clk { > struct clk_hw hw; > const struct scmi_clock_info *info; > const struct scmi_protocol_handle *ph; > + struct clk_parent_data *parent_data; > }; > > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) > @@ -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > } > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > +} > + > +static u8 scmi_clk_get_parent(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + u32 parent_id; > + int ret; > + > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > + if (ret) > + return 0; > + > + return parent_id; > +} > + While testing using CLK Debugfs with CLOCK_ALLOW_WRITE_DEBUGFS 1 I noticed that I can correctly change the clk_parent and then read back the clk_possible_parents, BUT if I read clk_parent right after boot (OR after loading the clk-scmi module) I cannot get back any value from debugfs even though I can see the correct SCMI messages being exchanged from the traces. My guess was that, while scmi_clk_set_parent is invoked by the CLK core with a parent_index that has been remapped by the core to the SCMI clock domain ID, this is not done by scmi_clk_get_parent() so you are returning to the clock framework as parent_id the raw SCMI clock domain id as returned by the platform instead of the clk parent id used by the core. This does not happen after you issue at first a reparent because in that case on the following read of clk_parent the CLK framework returns the last value you have set that it had cached previously. This fixes for me the issue: ---8<---- diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 5aaca674830f..fd47232d4021 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -89,14 +89,21 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) static u8 scmi_clk_get_parent(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - u32 parent_id; + u32 parent_id, p_idx; int ret; ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); if (ret) return 0; - return parent_id; + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++) + if (clk->parent_data[p_idx].index == parent_id) + break; + + if (p_idx == clk->info->num_parents) + return 0; + + return p_idx; } ----8<----- Not sure if there is a clever way to do it. Aside from this, another inherent issue is that you cannot really return an error from .get_parent() so if the SCMI get_parent ops should fail (ex. timeout) you return 0... (and me too in the above fix) but this is due to the CLK framework callback definition itself. Thanks, Cristian
Hi Cristian, > Subject: Re: [PATCH v3 2/2] clk: scmi: add set/get_parent support > > On Sun, Oct 01, 2023 at 12:38:44PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > > to support them. > > > > Hi, > > a few notes down below. > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/clk/clk-scmi.c | 50 > > +++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index > > 2e1337b511eb..5aaca674830f 100644 > > --- a/drivers/clk/clk-scmi.c > > +++ b/drivers/clk/clk-scmi.c > > @@ -24,6 +24,7 @@ struct scmi_clk { > > struct clk_hw hw; > > const struct scmi_clock_info *info; > > const struct scmi_protocol_handle *ph; > > + struct clk_parent_data *parent_data; > > }; > > > > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) @@ > > -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned > long rate, > > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); } > > > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) { > > + struct scmi_clk *clk = to_scmi_clk(hw); > > + > > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, > > +parent_index); } > > + > > +static u8 scmi_clk_get_parent(struct clk_hw *hw) { > > + struct scmi_clk *clk = to_scmi_clk(hw); > > + u32 parent_id; > > + int ret; > > + > > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > > + if (ret) > > + return 0; > > + > > + return parent_id; > > +} > > + > > While testing using CLK Debugfs with CLOCK_ALLOW_WRITE_DEBUGFS 1 I > noticed that I can correctly change the clk_parent and then read back the > clk_possible_parents, BUT if I read clk_parent right after boot (OR after > loading the clk-scmi module) I cannot get back any value from debugfs even > though I can see the correct SCMI messages being exchanged from the traces. > > My guess was that, while scmi_clk_set_parent is invoked by the CLK core with > a parent_index that has been remapped by the core to the SCMI clock > domain ID, this is not done by scmi_clk_get_parent() so you are returning to > the clock framework as parent_id the raw SCMI clock domain id as returned > by the platform instead of the clk parent id used by the core. > > This does not happen after you issue at first a reparent because in that case > on the following read of clk_parent the CLK framework returns the last value > you have set that it had cached previously. > > This fixes for me the issue: > > ---8<---- > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index > 5aaca674830f..fd47232d4021 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -89,14 +89,21 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 > parent_index) static u8 scmi_clk_get_parent(struct clk_hw *hw) { > struct scmi_clk *clk = to_scmi_clk(hw); > - u32 parent_id; > + u32 parent_id, p_idx; > int ret; > > ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > if (ret) > return 0; > > - return parent_id; > + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++) > + if (clk->parent_data[p_idx].index == parent_id) > + break; > + > + if (p_idx == clk->info->num_parents) > + return 0; > + > + return p_idx; > } > You are right. Thanks for doing the fix. > ----8<----- > > Not sure if there is a clever way to do it. > > Aside from this, another inherent issue is that you cannot really return an > error from .get_parent() so if the SCMI get_parent ops should fail (ex. timeout) > you return 0... (and me too in the above fix) but this is due to the CLK > framework callback definition itself. Yes. Right. I will include your fix and do a test, then out v4, should be soon. Thanks, Peng. > > Thanks, > Cristian
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 2e1337b511eb..5aaca674830f 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -24,6 +24,7 @@ struct scmi_clk { struct clk_hw hw; const struct scmi_clock_info *info; const struct scmi_protocol_handle *ph; + struct clk_parent_data *parent_data; }; #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) @@ -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); } +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) +{ + struct scmi_clk *clk = to_scmi_clk(hw); + + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); +} + +static u8 scmi_clk_get_parent(struct clk_hw *hw) +{ + struct scmi_clk *clk = to_scmi_clk(hw); + u32 parent_id; + int ret; + + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); + if (ret) + return 0; + + return parent_id; +} + +static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) +{ + /* + * Suppose all the requested rates are supported, and let firmware + * to handle the left work. + */ + return 0; +} + static int scmi_clk_enable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); @@ -139,6 +169,9 @@ static const struct clk_ops scmi_clk_ops = { .set_rate = scmi_clk_set_rate, .prepare = scmi_clk_enable, .unprepare = scmi_clk_disable, + .set_parent = scmi_clk_set_parent, + .get_parent = scmi_clk_get_parent, + .determine_rate = scmi_clk_determine_rate, }; static const struct clk_ops scmi_atomic_clk_ops = { @@ -148,6 +181,9 @@ static const struct clk_ops scmi_atomic_clk_ops = { .enable = scmi_clk_atomic_enable, .disable = scmi_clk_atomic_disable, .is_enabled = scmi_clk_atomic_is_enabled, + .set_parent = scmi_clk_set_parent, + .get_parent = scmi_clk_get_parent, + .determine_rate = scmi_clk_determine_rate, }; static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, @@ -158,9 +194,10 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, struct clk_init_data init = { .flags = CLK_GET_RATE_NOCACHE, - .num_parents = 0, + .num_parents = sclk->info->num_parents, .ops = scmi_ops, .name = sclk->info->name, + .parent_data = sclk->parent_data, }; sclk->hw.init = &init; @@ -250,6 +287,17 @@ static int scmi_clocks_probe(struct scmi_device *sdev) else scmi_ops = &scmi_clk_ops; + /* Initialize clock parent data. */ + if (sclk->info->num_parents > 0) { + sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents, + sizeof(*sclk->parent_data), GFP_KERNEL); + + for (int i = 0; i < sclk->info->num_parents; i++) { + sclk->parent_data[i].index = sclk->info->parents[i]; + sclk->parent_data[i].hw = hws[sclk->info->parents[i]]; + } + } + err = scmi_clk_ops_init(dev, sclk, scmi_ops); if (err) { dev_err(dev, "failed to register clock %d\n", idx);