Received-SPF: pass (google.com: domain of linux-kernel+bounces-142417-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 12 Apr 2024 15:02:47 +0530
From: Varadarajan Narayanan <quic_varada@quicinc.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
CC: Konrad Dybcio <konrad.dybcio@linaro.org>, <andersson@kernel.org>,
        <mturquette@baylibre.com>, <sboyd@kernel.org>, <robh@kernel.org>,
        <krzk+dt@kernel.org>, <conor+dt@kernel.org>, <djakov@kernel.org>,
        <dmitry.baryshkov@linaro.org>, <quic_anusha@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574
 support
Message-ID: <Zhj/v+AfzHlUCwRg@hu-varada-blr.qualcomm.com>
References: <58c9b754-b9a7-444d-9545-9e6648010630@kernel.org>
 <Zg5q6mnWtK6hmPBT@hu-varada-blr.qualcomm.com>
 <ZhTxFVDH0xTSkw7r@hu-varada-blr.qualcomm.com>
 <1ec401be-11cb-416a-9eae-d72ea8acf06f@kernel.org>
 <ZhUghsa5Do5m7wrX@hu-varada-blr.qualcomm.com>
 <a0173a13-5f20-4e24-8417-afce5fdbda0e@kernel.org>
 <ZhZjuCkJrtPbwtS/@hu-varada-blr.qualcomm.com>
 <70d0afa7-4990-4180-8dfa-cdf267e4c7a2@kernel.org>
 <f1b0d280-6986-4055-a611-2caceb15867d@linaro.org>
 <82e5503c-0710-4b17-af79-8dece4794ec8@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <82e5503c-0710-4b17-af79-8dece4794ec8@kernel.org>

On Wed, Apr 10, 2024 at 02:01:00PM +0200, Krzysztof Kozlowski wrote:
> On 10/04/2024 13:48, Konrad Dybcio wrote:
> >
> >
> > On 4/10/24 13:15, Krzysztof Kozlowski wrote:
> >> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
> >>>> Okay, so what happens if icc-clk way of generating them changes a bit?
> >>>> It can change, why not, driver implementation is not an ABI.
> >>>>
> >>>>>
> >>>>> 	2. These auto-generated id-numbers have to be correctly
> >>>>> 	   tied to the DT nodes. Else, the relevant clocks may
> >>>>> 	   not get enabled.
> >>>>
> >>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
> >>>> What DT node?
> >>>
> >>> I meant the following usage for the 'interconnects' entry of the
> >>> consumer peripheral's node.
> >>>
> >>> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> >>> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> >>>
> >>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
> >>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
> >>>>> the below array would create holes.
> >>>>>
> >>>>> 	static int icc_ipq9574_hws[] = {
> >>>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> >>>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> >>>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> >>>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> >>>>> 		. . .
> >>>>> 	};
> >>>>>
> >>>>> Other Qualcomm drivers don't have this issue and they can
> >>>>> directly use the MASTER/SLAVE_xxx macros.
> >>>>
> >>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
> >>>> IDs which are not part of ABI.
> >>>>
> >>>>>
> >>>>> As the MASTER_xxx macros cannot be used, have to define a new set
> >>>>> of macros that can be used for indices in the above array. This
> >>>>> is the reason for the ICC_BINDING_NAME macros.
> >>>>
> >>>> Then maybe fix the driver, instead of adding something which is not an
> >>>> ABI to bindings and completely skipping the actual ABI.
> >>>
> >>> Will remove the ICC_xxx defines from the header. And in the
> >>> driver will change the declaration as follows. Will that be
> >>> acceptable?
> >>>
> >>> 	static int icc_ipq9574_hws[] = {
> >>> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> >>
> >> What is the binding in such case? What exactly do you bind between
> >> driver and DTS?
> >
> > I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
> > and the best solution would be to make it such that the interconnect indices
> > are set explicitly, instead of (master, slave), (master, slave) etc.
> >
> > Does that sound good, Krzysztof?
>
> Yes, I think earlier I expressed that icc-clk might needs fixes.

Ok

> The indices you define in the binding must be used by DTS and by the driver.

There are 3 drivers in play here.
	1. The icc-clk driver
	2. The gcc (i.e. the interconnect driver)
	3. The consumer peripheral's driver

By 'driver' I assume, you mean the icc-clk driver.

> Directly, otherwise it is error-prone and not really an ABI...

To address this, will modify the icc-clk driver as follows.

	==========================================
	diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
	index 5c611a8b0892..9bcee3e9c56c 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;
	 };


	diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
	index bce946592c98..f788db15cd76 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;
	==========================================

And update the inputs going from gcc-ipq9574.c accordingly
to use the MASTER_xxx and SLAVE_xxx defines. Will this be ok?

Konrad & Krzysztof kindly let me know.

Thanks
Varada