[v10,0/6] Add interconnect driver for IPQ9574 SoC

MSM platforms manage NoC related clocks and scaling from RPM.
However, in IPQ SoCs, RPM is not involved in managing NoC
related clocks and there is no NoC scaling.

However, there is a requirement to enable some NoC interface
clocks for the accessing the peripherals present in the
system. Hence add a minimalistic interconnect driver that
establishes a path from the processor/memory to those peripherals
and vice versa.

---
v10:	Set gcc-ipq9574 driver's sync_state to icc_sync_state
v9:	Squash icc-clk driver change and cbf-msm8996 change
	Remove HWS_DATA macro
v8:	Change icc-clk driver to take master and slave ids instead
	of auto generating
	Remove ICC_xxx defines from dt-bindings header
	Define MASTER/SLAVE_xxx macros from 0 .. n

v7:	Fix macro names in dt-bindings header
	Do clock get in icc driver

v6:	Removed 'Reviewed-by: Krzysztof' from dt-bindings patch
	Remove clock get from ICC driver as suggested by Stephen Boyd
	so that the actual peripheral can do the clock get
	first_id -> icc_first_node_id
	Remove tristate from INTERCONNECT_CLK
v5:
	Split gcc-ipq9574.c and common.c changes into separate patches
	Introduce devm_icc_clk_register
	Fix error handling
v4:
gcc-ipq9574.c
	Use clk_hw instead of indices
common.c
	Do icc register in qcom_cc_probe() call stream
common.h
	Add icc clock info to qcom_cc_desc structure

v3:
qcom,ipq9574.h
	Move 'first id' define to clock driver
gcc-ipq9574.c:
	Use indexed identifiers here to avoid confusion
	Fix error messages and move code to common.c as it can be
	shared with future SoCs

v2:
qcom,ipq9574.h
	Fix license identifier
	Rename macros
qcom,ipq9574-gcc.yaml
	Include interconnect-cells
gcc-ipq9574.c
	Update commit log
	Remove IS_ENABLED(CONFIG_INTERCONNECT) and auto select it from Kconfig
ipq9574.dtsi
	Moved to separate patch
	Include interconnect-cells to clock controller node
drivers/clk/qcom/Kconfig:
	Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK

Varadarajan Narayanan (6):
  interconnect: icc-clk: Allow user to specify master/slave ids
  dt-bindings: interconnect: Add Qualcomm IPQ9574 support
  interconnect: icc-clk: Add devm_icc_clk_register
  clk: qcom: common: Add interconnect clocks support
  clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks
  arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

 .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         |  2 +
 drivers/clk/qcom/Kconfig                      |  2 +
 drivers/clk/qcom/clk-cbf-8996.c               |  7 ++-
 drivers/clk/qcom/common.c                     | 35 ++++++++++-
 drivers/clk/qcom/common.h                     |  9 +++
 drivers/clk/qcom/gcc-ipq9574.c                | 33 +++++++++++
 drivers/interconnect/icc-clk.c                | 24 +++++++-
 .../dt-bindings/interconnect/qcom,ipq9574.h   | 59 +++++++++++++++++++
 include/linux/interconnect-clk.h              |  4 ++
 10 files changed, 173 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h

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 29.04.24 12:13, Varadarajan Narayanan wrote:
> Wrap icc_clk_register to create devm_icc_clk_register to be
> able to release the resources properly.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>

Acked-by: Georgi Djakov <djakov@kernel.org>

> ---
> v8: Added Reviewed-by: Dmitry Baryshkov
> v7: Simplify devm_icc_clk_register implementation as suggested in review
> v5: Introduced devm_icc_clk_register
> ---
>   drivers/interconnect/icc-clk.c   | 18 ++++++++++++++++++
>   include/linux/interconnect-clk.h |  2 ++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
> index 2be193fd7d8f..f788db15cd76 100644
> --- a/drivers/interconnect/icc-clk.c
> +++ b/drivers/interconnect/icc-clk.c
> @@ -148,6 +148,24 @@ struct icc_provider *icc_clk_register(struct device *dev,
>   }
>   EXPORT_SYMBOL_GPL(icc_clk_register);
>   
> +static void devm_icc_release(void *res)
> +{
> +	icc_clk_unregister(res);
> +}
> +
> +int devm_icc_clk_register(struct device *dev, unsigned int first_id,
> +			  unsigned int num_clocks, const struct icc_clk_data *data)
> +{
> +	struct icc_provider *prov;
> +
> +	prov = icc_clk_register(dev, first_id, num_clocks, data);
> +	if (IS_ERR(prov))
> +		return PTR_ERR(prov);
> +
> +	return devm_add_action_or_reset(dev, devm_icc_release, prov);
> +}
> +EXPORT_SYMBOL_GPL(devm_icc_clk_register);
> +
>   /**
>    * icc_clk_unregister() - unregister a previously registered clk interconnect provider
>    * @provider: provider returned by icc_clk_register()
> diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
> index 170898faaacb..9bcee3e9c56c 100644
> --- a/include/linux/interconnect-clk.h
> +++ b/include/linux/interconnect-clk.h
> @@ -19,6 +19,8 @@ struct icc_provider *icc_clk_register(struct device *dev,
>   				      unsigned int first_id,
>   				      unsigned int num_clocks,
>   				      const struct icc_clk_data *data);
> +int devm_icc_clk_register(struct device *dev, unsigned int first_id,
> +			  unsigned int num_clocks, const struct icc_clk_data *data);
>   void icc_clk_unregister(struct icc_provider *provider);
>   
>   #endif

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