linux-kernel - Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 10 Apr 2024 14:01:00 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
 Varadarajan Narayanan <quic_varada@...cinc.com>
Cc: andersson@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, djakov@...nel.org,
 dmitry.baryshkov@...aro.org, quic_anusha@...cinc.com,
 linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574
 support

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. The
indices you define in the binding must be used by DTS and by the driver.
Directly, otherwise it is error-prone and not really an ABI...

Best regards,
Krzysztof