diff mbox series

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

Message ID 20240217-clk-mv200-v2-1-b782e4eb66f7@outlook.com
State New
Headers
Series clk: hisilicon: add support for Hi3798MV200 |

Commit Message

Yang Xiwen via B4 Relay Feb. 17, 2024, 12:52 p.m. UTC 
  From: Yang Xiwen <forbidden405@outlook.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@outlook.com>
---
 include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Krzysztof Kozlowski Feb. 20, 2024, 10:10 a.m. UTC | #1 
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.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@outlook.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.

> +#define HISTB_USB2_UTMI1_CLK		58
> +#define HISTB_USB3_REF_CLK		59
> +#define HISTB_USB3_GM_CLK		60
> +#define HISTB_USB3_GS_CLK		61
> +
> +/* Hi3798MV200 specific clocks */
> +
> +// reuse clocks of histb

Don't mix comment styles.

> +#define HI3798MV200_GMAC_CLK		HISTB_ETH0_MAC_CLK
> +#define HI3798MV200_GMACIF_CLK		HISTB_ETH0_MACIF_CLK
> +#define HI3798MV200_FEMAC_CLK		HISTB_ETH1_MAC_CLK
> +#define HI3798MV200_FEMACIF_CLK		HISTB_ETH1_MACIF_CLK
> +#define HI3798MV200_FEPHY_CLK		HISTB_ETH1_PHY_CLK

I don't understand what do you want to achieve here. Clock IDs start
from 0 or 1.



Best regards,
Krzysztof
Yang Xiwen Feb. 20, 2024, 10:12 a.m. UTC | #2 
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@outlook.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@outlook.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.
>
>> +#define HISTB_USB2_UTMI1_CLK		58
>> +#define HISTB_USB3_REF_CLK		59
>> +#define HISTB_USB3_GM_CLK		60
>> +#define HISTB_USB3_GS_CLK		61
>> +
>> +/* Hi3798MV200 specific clocks */
>> +
>> +// reuse clocks of histb
> Don't mix comment styles.
>
>> +#define HI3798MV200_GMAC_CLK		HISTB_ETH0_MAC_CLK
>> +#define HI3798MV200_GMACIF_CLK		HISTB_ETH0_MACIF_CLK
>> +#define HI3798MV200_FEMAC_CLK		HISTB_ETH1_MAC_CLK
>> +#define HI3798MV200_FEMACIF_CLK		HISTB_ETH1_MACIF_CLK
>> +#define HI3798MV200_FEPHY_CLK		HISTB_ETH1_PHY_CLK
> I don't understand what do you want to achieve here. Clock IDs start
> from 0 or 1.
They are aliases. A friendlier name compared to ETH0/1.
>
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 20, 2024, 10:16 a.m. UTC | #3 
On 20/02/2024 11:12, 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@outlook.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@outlook.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.
>>
>>> +#define HISTB_USB2_UTMI1_CLK		58
>>> +#define HISTB_USB3_REF_CLK		59
>>> +#define HISTB_USB3_GM_CLK		60
>>> +#define HISTB_USB3_GS_CLK		61
>>> +
>>> +/* Hi3798MV200 specific clocks */
>>> +
>>> +// reuse clocks of histb
>> Don't mix comment styles.
>>
>>> +#define HI3798MV200_GMAC_CLK		HISTB_ETH0_MAC_CLK
>>> +#define HI3798MV200_GMACIF_CLK		HISTB_ETH0_MACIF_CLK
>>> +#define HI3798MV200_FEMAC_CLK		HISTB_ETH1_MAC_CLK
>>> +#define HI3798MV200_FEMACIF_CLK		HISTB_ETH1_MACIF_CLK
>>> +#define HI3798MV200_FEPHY_CLK		HISTB_ETH1_PHY_CLK
>> I don't understand what do you want to achieve here. Clock IDs start
>> from 0 or 1.
> They are aliases. A friendlier name compared to ETH0/1.

Fix your email client, so it will not remove line breaks before/after
quotes. Your email client makes it unreadable.

Aliases do not bind anything, so you can drop these.

Best regards,
Krzysztof
Yang Xiwen Feb. 20, 2024, 2:06 p.m. UTC | #4 
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@outlook.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@outlook.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.


>
>> +#define HISTB_USB2_UTMI1_CLK		58
>> +#define HISTB_USB3_REF_CLK		59
>> +#define HISTB_USB3_GM_CLK		60
>> +#define HISTB_USB3_GS_CLK		61
>> +
>> +/* Hi3798MV200 specific clocks */
>> +
>> +// reuse clocks of histb
> Don't mix comment styles.
>
>> +#define HI3798MV200_GMAC_CLK		HISTB_ETH0_MAC_CLK
>> +#define HI3798MV200_GMACIF_CLK		HISTB_ETH0_MACIF_CLK
>> +#define HI3798MV200_FEMAC_CLK		HISTB_ETH1_MAC_CLK
>> +#define HI3798MV200_FEMACIF_CLK		HISTB_ETH1_MACIF_CLK
>> +#define HI3798MV200_FEPHY_CLK		HISTB_ETH1_PHY_CLK
> I don't understand what do you want to achieve here. Clock IDs start
> from 0 or 1.
>
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 20, 2024, 4:13 p.m. UTC | #5 
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@outlook.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@outlook.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.



Best regards,
Krzysztof
Yang Xiwen Feb. 20, 2024, 4:19 p.m. UTC | #6 
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@outlook.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@outlook.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.


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.


>
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 20, 2024, 4:25 p.m. UTC | #7 
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@outlook.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@outlook.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.

Best regards,
Krzysztof
Yang Xiwen Feb. 20, 2024, 4:31 p.m. UTC | #8 
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@outlook.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@outlook.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 
okay? Should i insert UTMI1_CLK to the middle and re-index all the 
macros after it? Or simply add it to the tail?


>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 20, 2024, 5:06 p.m. UTC | #9 
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@outlook.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@outlook.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.

Best regards,
Krzysztof
Yang Xiwen Feb. 20, 2024, 5:29 p.m. UTC | #10 
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@outlook.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@outlook.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
>
Krzysztof Kozlowski Feb. 21, 2024, 7:27 a.m. UTC | #11 
On 20/02/2024 18:29, Yang Xiwen wrote:
> 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@outlook.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@outlook.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 

That's not a big problem, but indeed shows poor design of the driver and
bindings.

> to be more and more unreadable with scattered clock definitions.
> 
> 
> Do you think it's acceptable to create a new header file instead? I 

This depends on the purpose of it. In general every SoC follows that
concept - binding headers are per given clock controller, not even per SoC.

> 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
diff mbox series

Patch

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
+#define HISTB_USB2_UTMI1_CLK		58
+#define HISTB_USB3_REF_CLK		59
+#define HISTB_USB3_GM_CLK		60
+#define HISTB_USB3_GS_CLK		61
+
+/* Hi3798MV200 specific clocks */
+
+// reuse clocks of histb
+#define HI3798MV200_GMAC_CLK		HISTB_ETH0_MAC_CLK
+#define HI3798MV200_GMACIF_CLK		HISTB_ETH0_MACIF_CLK
+#define HI3798MV200_FEMAC_CLK		HISTB_ETH1_MAC_CLK
+#define HI3798MV200_FEMACIF_CLK		HISTB_ETH1_MACIF_CLK
+#define HI3798MV200_FEPHY_CLK		HISTB_ETH1_PHY_CLK
 
 /* clocks provided by mcu CRG */
 #define HISTB_MCE_CLK			1