Re: [PATCH v8 3/7] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Wed, Apr 17, 2024 at 04:07:59PM +0200, Krzysztof Kozlowski wrote:
> On 17/04/2024 12:56, 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@xxxxxxxxxxx>
> > ---
> > v8:
> > Remove ICC_xxx macros
> > Fix macro defines to be consistent with other bindings
> > 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   | 59 +++++++++++++++++++
> >  2 files changed, 62 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..42019335c7dd
> > --- /dev/null
> > +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> > +#define INTERCONNECT_QCOM_IPQ9574_H
> > +
> > +#define MASTER_ANOC_PCIE0		0
> > +#define SLAVE_ANOC_PCIE0		1
>
> I still do not see any usage of it. At least symbol cannot be resolved.
> I assume you use the value, otherwise it would mean our entire feedback
> was ignored, but then why this cannot be searchable?
>
> Again, open existing drivers and look how it is there. Not being able to
> find the constant is not good.

It is used in the 6th patch in drivers/clk/qcom/gcc-ipq9574.c via the
HWS_DATA macro. Will remove the macro and use it explicitly (as suggested
by Dmitry also).

Thanks
Varada