linux-kernel - Re: [PATCH RFC v2 1/5] dt-bindings: clock: histb-clock: Add missing common clock and Hi3798MV200 specific clock definition

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, 21 Feb 2024 01:29:31 +0800
From: Yang Xiwen <forbidden405@...look.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: David Yang <mmyangfl@...il.com>, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/5] dt-bindings: clock: histb-clock: Add missing
 common clock and Hi3798MV200 specific clock definition

On 2/21/2024 1:06 AM, Krzysztof Kozlowski wrote:
> On 20/02/2024 17:31, Yang Xiwen wrote:
>> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote:
>>> On 20/02/2024 17:19, Yang Xiwen wrote:
>>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>>>>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>>>>> From: Yang Xiwen <forbidden405@...look.com>
>>>>>>>>
>>>>>>>> According to the datasheet, some clocks are missing, add their
>>>>>>>> definitions first.
>>>>>>>>
>>>>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Xiwen <forbidden405@...look.com>
>>>>>>>> ---
>>>>>>>>      include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>>>>>      1 file changed, 21 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>>>>> index e64e5770ada6..68a53053586a 100644
>>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>>>>> @@ -58,6 +58,27 @@
>>>>>>>>      #define HISTB_USB3_UTMI_CLK1		48
>>>>>>>>      #define HISTB_USB3_PIPE_CLK1		49
>>>>>>>>      #define HISTB_USB3_SUSPEND_CLK1		50
>>>>>>>> +#define HISTB_SDIO1_BIU_CLK		51
>>>>>>>> +#define HISTB_SDIO1_CIU_CLK		52
>>>>>>>> +#define HISTB_SDIO1_DRV_CLK		53
>>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK		54
>>>>>>>> +#define HISTB_ETH0_PHY_CLK		55
>>>>>>>> +#define HISTB_ETH1_PHY_CLK		56
>>>>>>>> +#define HISTB_WDG0_CLK			57
>>>>>>>> +#define HISTB_USB2_UTMI0_CLK		HISTB_USB2_UTMI_CLK
>>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>>>> So this is somewhat broken at the beginning. It named after
>>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>>>>
>>>>>>
>>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>>>>> after it and increment all the indexes after it? Then the diff would be
>>>>>> very ugly.
>>>>> I still don't understand what is the problem you are trying to solve
>>>>> here. Your commit msg says add missing ID, but that ID -
>>>>> HISTB_USB2_UTMI_CLK - is already there.
>>>>>
>>>>> I also do not get why there is a need to rename anything.
>>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
>>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
>>> My comment was under UTMI0. We do not talk about UTMI1...
>>>
>>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
>>>> renaming it to UTMI0. Just like all the other clocks. E.g.
>>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
>>> Then place it next to old name and explain why it is deprecated with
>>> comment.
>>
>> Do we need to keep the old name? I can fix all the users (only
>> hi3798cv200.dtsi) in next version and drop this name directly. Is that
> All users in all projects? That might be tricky. And even for Linux
> kernel, how can you do it in a bisectable way? Just keep old name.
>
>
>> okay? Should i insert UTMI1_CLK to the middle and re-index all the
>> macros after it? Or simply add it to the tail?
> Bindings and header constants are ABI, so you cannot change them.


This file should be renamed to hi3798cv200-clock.h, it shouldn't be 
called histb-clock.h from the beginning. Now I have to workaround this 
in a dirty way. What if another HiSTB SoC has 3 or more UTMI_CLKs? Do we 
need to add more definitions to the end of the file? The file is gonna 
to be more and more unreadable with scattered clock definitions.


Do you think it's acceptable to create a new header file instead? I 
think we don't need a generic histb-clock.h. Each SoC should maintain 
their own clock indexes header file. Maybe we can rename it to 
hi3798cv200-clock.h and include it from a new histb-clock.h (also mark 
this generic header file deprecated and only for hi3798cv200). Then I'll 
create hi3798mv200-clock.h header file instead. So we don't have to 
workaround this.


>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen