| Message ID | 20231003-scmi-clock-v3-v4-2-358d7f916a05@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | firmware: arm_scmi: clock: support parents commands | expand |
On Tue, Oct 03, 2023 at 07:48:49PM +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, in general LGTM, BUT I have just spotted one more *bad* thing that have to be fixe down below which I have missed previously, my bad. > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/clk-scmi.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2e1337b511eb..e7a27fda561b 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,43 @@ 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, p_idx; > + int ret; > + > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > + if (ret) > + return 0; > + > + 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; > +} > + > +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 +177,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 +189,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 +202,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 +295,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); > + Here, you missed to check the return value of devm_kcalloc() before carrying on. > + 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]]; > + } > + } > + Other than this, FWIW: Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 2e1337b511eb..e7a27fda561b 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,43 @@ 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, p_idx; + int ret; + + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); + if (ret) + return 0; + + 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; +} + +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 +177,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 +189,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 +202,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 +295,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);