Re: [PATCH v5 4/5] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks

[Varadarajan Narayanan]

On Sat, Mar 30, 2024 at 11:28:09AM +0100, Krzysztof Kozlowski wrote:
> On 30/03/2024 10:30, Varadarajan Narayanan wrote:
> > On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote:
> >> On 29/03/2024 11:55, Varadarajan Narayanan wrote:
> >>>>> +
> >>>>> +enum {
> >>>>> +       ICC_ANOC_PCIE0,
> >>>>> +       ICC_SNOC_PCIE0,
> >>>>> +       ICC_ANOC_PCIE1,
> >>>>> +       ICC_SNOC_PCIE1,
> >>>>> +       ICC_ANOC_PCIE2,
> >>>>> +       ICC_SNOC_PCIE2,
> >>>>> +       ICC_ANOC_PCIE3,
> >>>>> +       ICC_SNOC_PCIE3,
> >>>>> +       ICC_SNOC_USB,
> >>>>> +       ICC_ANOC_USB_AXI,
> >>>>> +       ICC_NSSNOC_NSSCC,
> >>>>> +       ICC_NSSNOC_SNOC_0,
> >>>>> +       ICC_NSSNOC_SNOC_1,
> >>>>> +       ICC_NSSNOC_PCNOC_1,
> >>>>> +       ICC_NSSNOC_QOSGEN_REF,
> >>>>> +       ICC_NSSNOC_TIMEOUT_REF,
> >>>>> +       ICC_NSSNOC_XO_DCD,
> >>>>> +       ICC_NSSNOC_ATB,
> >>>>> +       ICC_MEM_NOC_NSSNOC,
> >>>>> +       ICC_NSSNOC_MEMNOC,
> >>>>> +       ICC_NSSNOC_MEM_NOC_1,
> >>>>> +};
> >>>>
> >>>> Are these supposed to be in a dt-binding header?
> >>>
> >>> Since these don't directly relate to the ids in the dt-bindings
> >>> not sure if they will be permitted there. Will move and post a
> >>> new version and get feedback.
> >>
> >> You can answer this by yourself by looking at your DTS. Do you use them
> >> as well in the DTS?
> >
> > I can use them in the DTS. The icc-clk framework automatically
> > creates master and slave nodes as 'n' and 'n+1'. Hence I can have
> > something like this in the dt-bindings include file
> >
> > 	#define ICC_ANOC_PCIE0		0
> > 	#define ICC_SNOC_PCIE0		1
> > 		.
> > 		.
> > 		.
> > 	#define ICC_NSSNOC_MEM_NOC_1	20
> >
> > 	#define MASTER(x)	((ICC_ ## x) * 2)
> > 	#define SLAVE(x)	(MASTER(x) + 1)
>
> I don't understand this or maybe I misunderstood the purpose of this
> define. It does not matter if you "can" use something in DT. The
> question is: do you use them.

Yes. It will be used fot the pcie nodes. These defines are for
specifying the endpoints. The icc driver identifies a path that
can connect these endpoints. The peripheral drivers' DT nodes
will make use of these defines.

> >> It's a pity we see here only parts of DTS, instead of full interconnect
> >> usage.
> >
> > Unfortunately cannot include the pcie dts changes with this
> > patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@xxxxxxxxxxx/
> >
> > The above macros will be used in the pcie node as follows
> >
> > pcie0: pci@28000000 {
> > 	compatible = "qcom,pcie-ipq9574";
> > 	. . .
> > 	interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>,
> > 			<&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>;
> > 	interconnect-names = "pcie-mem", "cpu-pcie";
>
> Then why did you add header which is not used?

Since they are a part of the interconnect driver. The peripherals
that use the interconnects will make use of them to specify the
endpoints. After changing per Boyd's comments, the header will
be used from gcc driver. Will post a new version. Kindly review
that.

Thanks
Varada

> I will respond there...
>
> Best regards,
> Krzysztof
>