On 09/04/2024 09:41, Varadarajan Narayanan wrote: > On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote: >> 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@xxxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxx/ >> 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@xxxxxxxxxx/ >> Are these supposed to be in a dt-binding header? >> >> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@xxxxxxxxxx/ >> 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. > > Krzysztof, > > Is this ok? Can I post a new version addressing other review comments? I don't understand and you did not answered before, why you have to do it differently than all other Qualcomm interconnect providers. Maybe the code here needs it, maybe not, but I don't see any argument proving this. Best regards, Krzysztof
