Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan <quic_varada@quicinc.com>]

On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> > Add interconnect-cells to clock provider so that it can be
> > used as icc provider.
> >
> > Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> > interfaces. This will be used by the gcc-ipq9574 driver
> > that will for providing interconnect services using the
> > icc-clk framework.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > v7:
> > Fix macro names to be consistent with other bindings
> > v6:
> > Removed Reviewed-by: Krzysztof Kozlowski
> > Redefine the bindings such that driver and DT can share them
> >
> > v3:
> > Squash Documentation/ and include/ changes into same patch
> >
> > qcom,ipq9574.h
> > 	Move 'first id' to clock driver
> >
> > ---
> >  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
> >  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
> >  2 files changed, 90 insertions(+)
> >  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > index 944a0ea79cd6..824781cbdf34 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > @@ -33,6 +33,9 @@ properties:
> >        - description: PCIE30 PHY3 pipe clock source
> >        - description: USB3 PHY pipe clock source
> >
> > +  '#interconnect-cells':
> > +    const: 1
> > +
> >  required:
> >    - compatible
> >    - clocks
> > diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > new file mode 100644
> > index 000000000000..0b076b0cf880
> > --- /dev/null
> > +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> > +#define INTERCONNECT_QCOM_IPQ9574_H
> > +
> > +#define ICC_ANOC_PCIE0		0
> > +#define ICC_SNOC_PCIE0		1
> > +#define ICC_ANOC_PCIE1		2
> > +#define ICC_SNOC_PCIE1		3
> > +#define ICC_ANOC_PCIE2		4
> > +#define ICC_SNOC_PCIE2		5
> > +#define ICC_ANOC_PCIE3		6
> > +#define ICC_SNOC_PCIE3		7
> > +#define ICC_SNOC_USB		8
> > +#define ICC_ANOC_USB_AXI	9
> > +#define ICC_NSSNOC_NSSCC	10
> > +#define ICC_NSSNOC_SNOC_0	11
> > +#define ICC_NSSNOC_SNOC_1	12
> > +#define ICC_NSSNOC_PCNOC_1	13
> > +#define ICC_NSSNOC_QOSGEN_REF	14
> > +#define ICC_NSSNOC_TIMEOUT_REF	15
> > +#define ICC_NSSNOC_XO_DCD	16
> > +#define ICC_NSSNOC_ATB		17
> > +#define ICC_MEM_NOC_NSSNOC	18
> > +#define ICC_NSSNOC_MEMNOC	19
> > +#define ICC_NSSNOC_MEM_NOC_1	20
> > +
> > +#define ICC_NSSNOC_PPE		0
> > +#define ICC_NSSNOC_PPE_CFG	1
> > +#define ICC_NSSNOC_NSS_CSR	2
> > +#define ICC_NSSNOC_IMEM_QSB	3
> > +#define ICC_NSSNOC_IMEM_AHB	4
> > +
> > +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> > +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
>
> Which existing Qualcomm platform has such code?

Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.

The icc_clk_register automatically creates master & slave nodes
for each clk entry provided as input with the node-ids 'n' and
'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
calculations.

However, ipq9574 gives an array of clock entries as input to
icc_clk_register. To tie the order/sequence of these clock
entries correctly with the node-ids, this calculation is needed.

> This is the third time I am asking for consistent headers. Open
> existing, recently added headers and look how it is done there. Why?
> Because I am against such calculations and see no reason for them.

Apologies. Regret that I have to trouble you.

In this ipq9574 case, have to reconcile between the following
feedbacks.

1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
   We could probably use indexed identifiers here to avoid confusion:
   [ICC_BINDING_NAME] = CLK_BINDING_NAME

2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
   Are these supposed to be in a dt-binding header?

3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
   Do you use them as well in the DTS?

Having the defines (with the calculations) seemed to to comply
with the above three feedbacks.

Please let me know if this can be handled in a different way that
would be consistent with other Qualcomm platforms.

Thanks
Varada