| Message ID | 20240429091314.761900-2-quic_varada@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect driver for IPQ9574 SoC | expand |
On 29/04/2024 10:13, Varadarajan Narayanan wrote: > for (i = 0, j = 0; i < num_clocks; i++) { > qp->clocks[i].clk = data[i].clk; > > - node = icc_node_create(first_id + j); > + node = icc_node_create(first_id + data[i].master_id); You have a few conditionals in the way down the end of the existing for() loop but then you hit this onecell->nodes[j++] = node; } which means that this node = icc_node_create(first_id + data[i].master_id); is not analogous to this node = icc_node_create(first_id + j); So for any loop of this for() where j was incremented previously you would not _not_ have the same node ids after your change. In other words dropping the j index will result in different node numbering. Is that a) intended b) correct Your commit log says "allow the drive rto provide the preferred master ids and slave ids" which it does but it _also_ changes the autogenerated ids. So could you either a) fix that or b) justify it, in your commit log. Also I think the 8996 specific change should be in its own patch. TBH I'm not sure the autogen change is on-purpose or warranted and for certain the commit log is not elucidating on which is the intended case. I think you should rewrite this patch in two ways 1. Fix the autogen case or 1. Justify the change for the autogen case. 2. Separate drivers/clk/qcom/clk-cbf-8996.c into its own patch that applies directly after changing the core Perhaps you've already gone through this debate with other reviewers but then you haven't captured that in your cover letter or commit log so at a minimum, please do that. --- bod
On 29.04.24 12:13, Varadarajan Narayanan wrote: > Presently, icc-clk driver autogenerates the master and slave ids. > However, devices with multiple nodes on the interconnect could > have other constraints and may not match with the auto generated > node ids. Hence, allow the driver to provide the preferred master > and slave ids. > > Also, update clk-cbf-8996 accordingly. > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> Acked-by: Georgi Djakov <djakov@kernel.org> > --- > v9: squash cbf-msm8996 change into this > v8: Per review feedback, set master/slave ids explicitly. Dont autogenerate > https://lore.kernel.org/linux-arm-msm/f1b0d280-6986-4055-a611-2caceb15867d@linaro.org/ > --- > drivers/clk/qcom/clk-cbf-8996.c | 7 ++++++- > drivers/interconnect/icc-clk.c | 6 +++--- > include/linux/interconnect-clk.h | 2 ++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c > index fe24b4abeab4..a077d4403967 100644 > --- a/drivers/clk/qcom/clk-cbf-8996.c > +++ b/drivers/clk/qcom/clk-cbf-8996.c > @@ -237,7 +237,12 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct cl > struct device *dev = &pdev->dev; > struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf"); > const struct icc_clk_data data[] = { > - { .clk = clk, .name = "cbf", }, > + { > + .clk = clk, > + .name = "cbf", > + .master_id = MASTER_CBF_M4M, > + .slave_id = SLAVE_CBF_M4M, > + }, > }; > struct icc_provider *provider; > > diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c > index d787f2ea36d9..2be193fd7d8f 100644 > --- a/drivers/interconnect/icc-clk.c > +++ b/drivers/interconnect/icc-clk.c > @@ -108,7 +108,7 @@ struct icc_provider *icc_clk_register(struct device *dev, > for (i = 0, j = 0; i < num_clocks; i++) { > qp->clocks[i].clk = data[i].clk; > > - node = icc_node_create(first_id + j); > + node = icc_node_create(first_id + data[i].master_id); > if (IS_ERR(node)) { > ret = PTR_ERR(node); > goto err; > @@ -118,10 +118,10 @@ struct icc_provider *icc_clk_register(struct device *dev, > node->data = &qp->clocks[i]; > icc_node_add(node, provider); > /* link to the next node, slave */ > - icc_link_create(node, first_id + j + 1); > + icc_link_create(node, first_id + data[i].slave_id); > onecell->nodes[j++] = node; > > - node = icc_node_create(first_id + j); > + node = icc_node_create(first_id + data[i].slave_id); > if (IS_ERR(node)) { > ret = PTR_ERR(node); > goto err; > diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h > index 0cd80112bea5..170898faaacb 100644 > --- a/include/linux/interconnect-clk.h > +++ b/include/linux/interconnect-clk.h > @@ -11,6 +11,8 @@ struct device; > struct icc_clk_data { > struct clk *clk; > const char *name; > + unsigned int master_id; > + unsigned int slave_id; > }; > > struct icc_provider *icc_clk_register(struct device *dev,
On Mon, Apr 29, 2024 at 12:08:40PM +0100, Bryan O'Donoghue wrote: > On 29/04/2024 10:13, Varadarajan Narayanan wrote: > > for (i = 0, j = 0; i < num_clocks; i++) { > > qp->clocks[i].clk = data[i].clk; > > - node = icc_node_create(first_id + j); > > + node = icc_node_create(first_id + data[i].master_id); > > You have a few conditionals in the way down the end of the existing for() > loop but then you hit this > > onecell->nodes[j++] = node; > } > > which means that this > > node = icc_node_create(first_id + data[i].master_id); > > is not analogous to this > > node = icc_node_create(first_id + j); > > So for any loop of this for() where j was incremented previously you would > not _not_ have the same node ids after your change. > > In other words dropping the j index will result in different node numbering. > > Is that > > a) intended Yes. > b) correct Currently, drivers/clk/qcom/clk-cbf-8996.c is the only user of icc-clk. And, it had exactly one master and one slave node. For this the auto generated master (= 1) and slave (= 0) was enough. However, when drivers/clk/qcom/gcc-ipq9574.c wanted to make use of the icc-clk framework, it had more number of master and slave nodes and the auto generated ids did not suit the usage. Hence wanted to move away from the auto generated method. And instead use the ids specified by the caller. > and slave ids" which it does but it _also_ changes the autogenerated ids. > > So could you either a) fix that or b) justify it, in your commit log. Will change the commit log and post a new version. > Also I think the 8996 specific change should be in its own patch. Earlier it was separate. Was squashed into this based on community feedback. Please refer to https://lore.kernel.org/linux-arm-msm/CAA8EJpqaXU=H6Nhz2_WTYHS1A0bi1QrMdp7Y+s6HUELioCzbeg@mail.gmail.com/ > TBH I'm not sure the autogen change is on-purpose or warranted and for > certain the commit log is not elucidating on which is the intended case. > > I think you should rewrite this patch in two ways > > 1. Fix the autogen case or > 1. Justify the change for the autogen case. > 2. Separate drivers/clk/qcom/clk-cbf-8996.c into its own patch that > applies directly after changing the core > > Perhaps you've already gone through this debate with other reviewers but > then you haven't captured that in your cover letter or commit log so at a > minimum, please do that. Will update the commit log and post a new version. Thanks for the inputs. -Varada
diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c index fe24b4abeab4..a077d4403967 100644 --- a/drivers/clk/qcom/clk-cbf-8996.c +++ b/drivers/clk/qcom/clk-cbf-8996.c @@ -237,7 +237,12 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct cl struct device *dev = &pdev->dev; struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf"); const struct icc_clk_data data[] = { - { .clk = clk, .name = "cbf", }, + { + .clk = clk, + .name = "cbf", + .master_id = MASTER_CBF_M4M, + .slave_id = SLAVE_CBF_M4M, + }, }; struct icc_provider *provider; diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c index d787f2ea36d9..2be193fd7d8f 100644 --- a/drivers/interconnect/icc-clk.c +++ b/drivers/interconnect/icc-clk.c @@ -108,7 +108,7 @@ struct icc_provider *icc_clk_register(struct device *dev, for (i = 0, j = 0; i < num_clocks; i++) { qp->clocks[i].clk = data[i].clk; - node = icc_node_create(first_id + j); + node = icc_node_create(first_id + data[i].master_id); if (IS_ERR(node)) { ret = PTR_ERR(node); goto err; @@ -118,10 +118,10 @@ struct icc_provider *icc_clk_register(struct device *dev, node->data = &qp->clocks[i]; icc_node_add(node, provider); /* link to the next node, slave */ - icc_link_create(node, first_id + j + 1); + icc_link_create(node, first_id + data[i].slave_id); onecell->nodes[j++] = node; - node = icc_node_create(first_id + j); + node = icc_node_create(first_id + data[i].slave_id); if (IS_ERR(node)) { ret = PTR_ERR(node); goto err; diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h index 0cd80112bea5..170898faaacb 100644 --- a/include/linux/interconnect-clk.h +++ b/include/linux/interconnect-clk.h @@ -11,6 +11,8 @@ struct device; struct icc_clk_data { struct clk *clk; const char *name; + unsigned int master_id; + unsigned int slave_id; }; struct icc_provider *icc_clk_register(struct device *dev,
Presently, icc-clk driver autogenerates the master and slave ids. However, devices with multiple nodes on the interconnect could have other constraints and may not match with the auto generated node ids. Hence, allow the driver to provide the preferred master and slave ids. Also, update clk-cbf-8996 accordingly. Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> --- v9: squash cbf-msm8996 change into this v8: Per review feedback, set master/slave ids explicitly. Dont autogenerate https://lore.kernel.org/linux-arm-msm/f1b0d280-6986-4055-a611-2caceb15867d@linaro.org/ --- drivers/clk/qcom/clk-cbf-8996.c | 7 ++++++- drivers/interconnect/icc-clk.c | 6 +++--- include/linux/interconnect-clk.h | 2 ++ 3 files changed, 11 insertions(+), 4 deletions(-)