On Fri, Apr 12, 2024 at 03:02:47PM +0530, Varadarajan Narayanan wrote: > 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. Have addressed these and other comments and posted v8. Please review. Thanks Varada