[PATCH RFC v2 0/5] clk: hisilicon: add support for Hi3798MV200

[PATCH RFC v2 0/5] clk: hisilicon: add support for Hi3798MV200

[Yang Xiwen via B4 Relay]

This SoC is similar to Hi3798CV200 with a few more clocks in CRG module.

Note this driver is still ongoing, many clocks are not registered in the
driver now. Feedback is welcomed, especially from HiSilicon people.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v2:
- s/dt-binding/dt-bindings in commit logs: (Krzysztof Kozlowski)
- fix bot error by adding "hisilicon,hisi-sdmmc-dll" to syscon.yaml (Rob
  Herring)
- hi3798mv200-crg: assign fixed rate parents to some gates
- hi3798mv200-crg: s/ETH/FEMAC, add GMAC ctrl clock
- Link to v1: https://lore.kernel.org/r/20240216-clk-mv200-v1-0-a29ace29e636@outlook.com

---
Yang Xiwen (5):
      dt-bindings: clock: histb-clock: Add missing common clock and Hi3798MV200 specific clock definition
      clk: hisilicon: add CRG driver for Hi3798MV200 SoC
      dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator
      dt-bindings: mfd: syscon: Add hisilicon,sdmmc-sap-dll compatible
      dt-bindings: clock: hisilicon,clock-reset-controller: add Hi3798MV200 SoC support

 .../devicetree/bindings/clock/hi3660-clock.txt     |  47 ---
 .../devicetree/bindings/clock/hi3670-clock.txt     |  43 --
 .../devicetree/bindings/clock/hi6220-clock.txt     |  52 ---
 .../devicetree/bindings/clock/hisi-crg.txt         |  50 ---
 .../clock/hisilicon,clock-reset-generator.yaml     | 175 +++++++++
 .../clock/hisilicon,hi3559av100-clock.yaml         |  59 ---
 Documentation/devicetree/bindings/mfd/syscon.yaml  |   1 +
 drivers/clk/hisilicon/Kconfig                      |   8 +
 drivers/clk/hisilicon/Makefile                     |   1 +
 drivers/clk/hisilicon/crg-hi3798mv200.c            | 436 +++++++++++++++++++++
 include/dt-bindings/clock/histb-clock.h            |  21 +
 11 files changed, 642 insertions(+), 251 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-clk-mv200-cc8cae396ee0

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

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

[Yang Xiwen via B4 Relay]

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
+#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

-- 
2.43.0

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

[Krzysztof Kozlowski]

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

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

[Yang Xiwen]

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
>

-- 
Regards,
Yang Xiwen

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

[Krzysztof Kozlowski]

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

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

[Yang Xiwen]

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
>

-- 
Regards,
Yang Xiwen

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

[Krzysztof Kozlowski]

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

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

[Yang Xiwen]

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
>

-- 
Regards,
Yang Xiwen

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

[Krzysztof Kozlowski]

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

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

[Yang Xiwen]

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
>

-- 
Regards,
Yang Xiwen

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

[Krzysztof Kozlowski]

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

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

[Yang Xiwen]

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
>

-- 
Regards,
Yang Xiwen

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

[Krzysztof Kozlowski]

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

[PATCH RFC v2 2/5] clk: hisilicon: add CRG driver for Hi3798MV200 SoC

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Add CRG driver for Hi3798MV200 SoC. CRG(Clock and Reset
Generator) module generates clock and reset signals used
by other module blocks on SoC.

Only currently used clocks are added. Clocks without mainline
user are omitted.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/clk/hisilicon/Kconfig           |   8 +
 drivers/clk/hisilicon/Makefile          |   1 +
 drivers/clk/hisilicon/crg-hi3798mv200.c | 436 ++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+)

diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index c1ec75aa4ccd..fab8059240b7 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -45,6 +45,14 @@ config COMMON_CLK_HI3798CV200
 	help
 	  Build the clock driver for hi3798cv200.
 
+config COMMON_CLK_HI3798MV200
+	tristate "Hi3798MV200 Clock Driver"
+	depends on ARCH_HISI || COMPILE_TEST
+	select RESET_HISI
+	default ARCH_HISI
+	help
+	  Build the clock driver for hi3798mv200.
+
 config COMMON_CLK_HI6220
 	bool "Hi6220 Clock Driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 2978e56cb876..7acb63e909bd 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_COMMON_CLK_HI3559A)	+= clk-hi3559a.o
 obj-$(CONFIG_COMMON_CLK_HI3660) += clk-hi3660.o
 obj-$(CONFIG_COMMON_CLK_HI3670) += clk-hi3670.o
 obj-$(CONFIG_COMMON_CLK_HI3798CV200)	+= crg-hi3798cv200.o
+obj-$(CONFIG_COMMON_CLK_HI3798MV200)	+= crg-hi3798mv200.o
 obj-$(CONFIG_COMMON_CLK_HI6220)	+= clk-hi6220.o
 obj-$(CONFIG_RESET_HISI)	+= reset.o
 obj-$(CONFIG_STUB_CLK_HI6220)	+= clk-hi6220-stub.o
diff --git a/drivers/clk/hisilicon/crg-hi3798mv200.c b/drivers/clk/hisilicon/crg-hi3798mv200.c
new file mode 100644
index 000000000000..756deed9303f
--- /dev/null
+++ b/drivers/clk/hisilicon/crg-hi3798mv200.c
@@ -0,0 +1,436 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hi3798MV200 Clock and Reset Generator Driver
+ *
+ * Copyright (c) 2024 Yang Xiwen <forbidden405@outlook.com>
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ */
+
+#include <dt-bindings/clock/histb-clock.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include "clk.h"
+#include "crg.h"
+#include "reset.h"
+
+/* hi3798MV200 core CRG */
+#define HI3798MV200_INNER_CLK_OFFSET		64
+#define HI3798MV200_FIXED_3M			65
+#define HI3798MV200_FIXED_12M			66
+#define HI3798MV200_FIXED_24M			67
+#define HI3798MV200_FIXED_25M			68
+#define HI3798MV200_FIXED_27M			69
+#define HI3798MV200_FIXED_48M			70
+#define HI3798MV200_FIXED_50M			71
+#define HI3798MV200_FIXED_54M			72
+#define HI3798MV200_FIXED_60M			73
+#define HI3798MV200_FIXED_75M			74
+#define HI3798MV200_FIXED_100M			75
+#define HI3798MV200_FIXED_125M			76
+#define HI3798MV200_FIXED_150M			77
+#define HI3798MV200_FIXED_166P5M		78
+#define HI3798MV200_FIXED_200M			79
+#define HI3798MV200_MMC_MUX			80
+#define HI3798MV200_SDIO0_MUX			81
+#define HI3798MV200_SDIO1_MUX			82
+#define HI3798MV200_COMBPHY0_MUX		83
+#define HI3798MV200_FEMAC_MUX			84
+#define HI3798MV200_GMAC_MUX			85
+
+#define HI3798MV200_CRG_NR_CLKS			128
+
+static const struct hisi_fixed_rate_clock hi3798mv200_fixed_rate_clks[] = {
+	{ HISTB_OSC_CLK, "clk_osc", NULL, 0, 24000000, },
+	{ HISTB_APB_CLK, "clk_apb", NULL, 0, 100000000, },
+	{ HISTB_AHB_CLK, "clk_ahb", NULL, 0, 200000000, },
+	{ HI3798MV200_FIXED_3M, "3m", NULL, 0, 3000000, },
+	{ HI3798MV200_FIXED_12M, "12m", NULL, 0, 12000000, },
+	{ HI3798MV200_FIXED_24M, "24m", NULL, 0, 24000000, },
+	{ HI3798MV200_FIXED_25M, "25m", NULL, 0, 25000000, },
+	{ HI3798MV200_FIXED_27M, "27m", NULL, 0, 27000000, },
+	{ HI3798MV200_FIXED_48M, "48m", NULL, 0, 48000000, },
+	{ HI3798MV200_FIXED_50M, "50m", NULL, 0, 50000000, },
+	{ HI3798MV200_FIXED_54M, "54m", NULL, 0, 54000000, },
+	{ HI3798MV200_FIXED_60M, "60m", NULL, 0, 60000000, },
+	{ HI3798MV200_FIXED_75M, "75m", NULL, 0, 75000000, },
+	{ HI3798MV200_FIXED_100M, "100m", NULL, 0, 100000000, },
+	{ HI3798MV200_FIXED_125M, "125m", NULL, 0, 125000000, },
+	{ HI3798MV200_FIXED_150M, "150m", NULL, 0, 150000000, },
+	{ HI3798MV200_FIXED_166P5M, "166p5m", NULL, 0, 165000000, },
+	{ HI3798MV200_FIXED_200M, "200m", NULL, 0, 200000000, },
+};
+
+static const char *const mmc_mux_p[] = {
+		"100m", "50m", "25m", "200m", "150m" };
+static u32 mmc_mux_table[] = {0, 1, 2, 3, 6};
+
+static const char *const comphy_mux_p[] = {
+		"25m", "100m"};
+static u32 comphy_mux_table[] = {0, 1};
+
+static const char *const sdio_mux_p[] = {
+		"100m", "50m", "150m", "25m" };
+static u32 sdio_mux_table[] = {0, 1, 2, 3};
+
+static const char *const femac_mux_p[] = {
+		"54m", "27m" };
+static const char *const gmac_mux_p[] = {
+		"125m", "75m" };
+static u32 eth_mux_table[] = {0, 1};
+
+static struct hisi_mux_clock hi3798mv200_mux_clks[] = {
+	{ HI3798MV200_MMC_MUX, "mmc_mux", mmc_mux_p, ARRAY_SIZE(mmc_mux_p),
+		0, 0xa0, 8, 3, CLK_MUX_ROUND_CLOSEST, mmc_mux_table, },
+	{ HI3798MV200_COMBPHY0_MUX, "combphy0_mux", comphy_mux_p,
+		ARRAY_SIZE(comphy_mux_p), 0, 0x188, 3, 1, 0, comphy_mux_table, },
+	{ HI3798MV200_SDIO0_MUX, "sdio0_mux", sdio_mux_p, ARRAY_SIZE(sdio_mux_p),
+		0, 0x9c, 8, 2, CLK_MUX_ROUND_CLOSEST, sdio_mux_table, },
+	{ HI3798MV200_SDIO1_MUX, "sdio1_mux", sdio_mux_p, ARRAY_SIZE(sdio_mux_p),
+		0, 0x28c, 8, 2, CLK_MUX_ROUND_CLOSEST, sdio_mux_table, },
+	{ HI3798MV200_FEMAC_MUX, "femac_mux", femac_mux_p, ARRAY_SIZE(femac_mux_p),
+		0, 0xd0, 2, 1, 0, eth_mux_table, },
+	{ HI3798MV200_GMAC_MUX, "gmac_mux", gmac_mux_p, ARRAY_SIZE(gmac_mux_p),
+		0, 0xcc, 7, 1, 0, eth_mux_table, },
+};
+
+static u32 mmc_phase_regvals[] = {0, 1, 2, 3, 4, 5, 6, 7};
+static u32 mmc_phase_degrees[] = {0, 45, 90, 135, 180, 225, 270, 315};
+
+static struct hisi_phase_clock hi3798mv200_phase_clks[] = {
+	{ HISTB_SDIO0_SAMPLE_CLK, "sdio0_sample", "clk_sdio0_ciu",
+		0, 0x9c, 12, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+	{ HISTB_SDIO0_DRV_CLK, "sdio0_drive", "clk_sdio0_ciu",
+		0, 0x9c, 16, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+	{ HISTB_SDIO1_SAMPLE_CLK, "sdio1_sample", "clk_sdio1_ciu",
+		0, 0x28c, 12, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+	{ HISTB_SDIO1_DRV_CLK, "sdio1_drive", "clk_sdio1_ciu",
+		0, 0x28c, 16, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+	{ HISTB_MMC_SAMPLE_CLK, "mmc_sample", "clk_mmc_ciu",
+		0, 0xa0, 12, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+	{ HISTB_MMC_DRV_CLK, "mmc_drive", "clk_mmc_ciu",
+		0, 0xa0, 16, 3, mmc_phase_degrees,
+		mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+};
+
+static const struct hisi_gate_clock hi3798mv200_gate_clks[] = {
+	/* UART */
+	{ HISTB_UART2_CLK, "clk_uart2", "75m",
+		CLK_SET_RATE_PARENT, 0x68, 4, 0, },
+	{ HISTB_UART3_CLK, "clk_uart3", "75m",
+		CLK_SET_RATE_PARENT, 0x68, 6, 0, },
+	/* SPI */
+	{ HISTB_SPI0_CLK, "clk_spi0", "clk_apb",
+		CLK_SET_RATE_PARENT, 0x70, 0, 0, },
+	/* I2C */
+	{ HISTB_I2C0_CLK, "clk_i2c0", "clk_apb",
+		CLK_SET_RATE_PARENT, 0x6c, 4, 0, },
+	{ HISTB_I2C1_CLK, "clk_i2c1", "clk_apb",
+		CLK_SET_RATE_PARENT, 0x6c, 8, 0, },
+	{ HISTB_I2C2_CLK, "clk_i2c2", "clk_apb",
+		CLK_SET_RATE_PARENT, 0x6c, 12, 0, },
+	/* SDIO */
+	{ HISTB_SDIO0_BIU_CLK, "clk_sdio0_biu", "200m",
+		CLK_SET_RATE_PARENT, 0x9c, 0, 0, },
+	{ HISTB_SDIO0_CIU_CLK, "clk_sdio0_ciu", "sdio0_mux",
+		CLK_SET_RATE_PARENT, 0x9c, 1, 0, },
+	{ HISTB_SDIO1_BIU_CLK, "clk_sdio1_biu", "200m",
+		CLK_SET_RATE_PARENT, 0x28c, 0, 0, },
+	{ HISTB_SDIO1_CIU_CLK, "clk_sdio1_ciu", "sdio1_mux",
+		CLK_SET_RATE_PARENT, 0x28c, 1, 0, },
+	/* EMMC */
+	{ HISTB_MMC_BIU_CLK, "clk_mmc_biu", "200m",
+		CLK_SET_RATE_PARENT, 0xa0, 0, 0, },
+	{ HISTB_MMC_CIU_CLK, "clk_mmc_ciu", "mmc_mux",
+		CLK_SET_RATE_PARENT, 0xa0, 1, 0, },
+	/* Ethernet */
+	{ HI3798MV200_GMAC_CLK, "clk_gmac", "gmac_mux",
+		CLK_SET_RATE_PARENT, 0xcc, 2, 0, },
+	{ HI3798MV200_GMACIF_CLK, "clk_gmacif", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xcc, 0, 0, },
+	{ HI3798MV200_FEMAC_CLK, "clk_femac", "femac_mux",
+		CLK_SET_RATE_PARENT, 0xd0, 1, 0, },
+	{ HI3798MV200_FEMACIF_CLK, "clk_femacif", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xd0, 0, 0, },
+	{ HI3798MV200_FEPHY_CLK, "clk_fephy", "25m",
+		CLK_SET_RATE_PARENT, 0x388, 0, 0, },
+	/* COMBPHY0 */
+	{ HISTB_COMBPHY0_CLK, "clk_combphy0", "combphy0_mux",
+		CLK_SET_RATE_PARENT, 0x188, 0, 0, },
+	/* USB2 */
+	{ HISTB_USB2_BUS_CLK, "clk_u2_bus", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xb8, 0, 0, },
+	{ HISTB_USB2_PHY_CLK, "clk_u2_phy", "60m",
+		CLK_SET_RATE_PARENT, 0xb8, 4, 0, },
+	{ HISTB_USB2_12M_CLK, "clk_u2_12m", "12m",
+		CLK_SET_RATE_PARENT, 0xb8, 2, 0 },
+	{ HISTB_USB2_48M_CLK, "clk_u2_48m", "48m",
+		CLK_SET_RATE_PARENT, 0xb8, 1, 0 },
+	{ HISTB_USB2_UTMI0_CLK, "clk_u2_utmi0", "60m",
+		CLK_SET_RATE_PARENT, 0xb8, 5, 0 },
+	{ HISTB_USB2_UTMI1_CLK, "clk_u2_utmi1", "60m",
+		CLK_SET_RATE_PARENT, 0xb8, 6, 0 },
+	{ HISTB_USB2_OTG_UTMI_CLK, "clk_u2_otg_utmi", "60m",
+		CLK_SET_RATE_PARENT, 0xb8, 3, 0 },
+	{ HISTB_USB2_PHY1_REF_CLK, "clk_u2_phy1_ref", "24m",
+		CLK_SET_RATE_PARENT, 0xbc, 0, 0 },
+	{ HISTB_USB2_PHY2_REF_CLK, "clk_u2_phy2_ref", "24m",
+		CLK_SET_RATE_PARENT, 0xbc, 2, 0 },
+	/* USB3 bus */
+	{ HISTB_USB3_GM_CLK, "clk_u3_gm", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xb0, 6, 0 },
+	{ HISTB_USB3_GS_CLK, "clk_u3_gs", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xb0, 5, 0 },
+	{ HISTB_USB3_BUS_CLK, "clk_u3_bus", "clk_ahb",
+		CLK_SET_RATE_PARENT, 0xb0, 0, 0 },
+	/* USB3 ctrl */
+	{ HISTB_USB3_SUSPEND_CLK, "clk_u3_suspend", NULL,
+		CLK_SET_RATE_PARENT, 0xb0, 2, 0 },
+	{ HISTB_USB3_PIPE_CLK, "clk_u3_pipe", NULL,
+		CLK_SET_RATE_PARENT, 0xb0, 3, 0 },
+	{ HISTB_USB3_REF_CLK, "clk_u3_ref", "125m",
+		CLK_SET_RATE_PARENT, 0xb0, 1, 0 },
+	{ HISTB_USB3_UTMI_CLK, "clk_u3_utmi", "60m",
+		CLK_SET_RATE_PARENT, 0xb0, 4, 0 },
+	/* Watchdog */
+	{ HISTB_WDG0_CLK, "clk_wdg0", "24m",
+		CLK_SET_RATE_PARENT, 0x178, 0, 0 },
+};
+
+static struct hisi_clock_data *hi3798mv200_clk_register(
+				struct platform_device *pdev)
+{
+	struct hisi_clock_data *clk_data;
+	int ret;
+
+	clk_data = hisi_clk_alloc(pdev, HI3798MV200_CRG_NR_CLKS);
+	if (!clk_data)
+		return ERR_PTR(-ENOMEM);
+
+	/* hisi_phase_clock is resource managed */
+	ret = hisi_clk_register_phase(&pdev->dev,
+				hi3798mv200_phase_clks,
+				ARRAY_SIZE(hi3798mv200_phase_clks),
+				clk_data);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = hisi_clk_register_fixed_rate(hi3798mv200_fixed_rate_clks,
+				     ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+				     clk_data);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = hisi_clk_register_mux(hi3798mv200_mux_clks,
+				ARRAY_SIZE(hi3798mv200_mux_clks),
+				clk_data);
+	if (ret)
+		goto unregister_fixed_rate;
+
+	ret = hisi_clk_register_gate(hi3798mv200_gate_clks,
+				ARRAY_SIZE(hi3798mv200_gate_clks),
+				clk_data);
+	if (ret)
+		goto unregister_mux;
+
+	ret = of_clk_add_provider(pdev->dev.of_node,
+			of_clk_src_onecell_get, &clk_data->clk_data);
+	if (ret)
+		goto unregister_gate;
+
+	return clk_data;
+
+unregister_gate:
+	hisi_clk_unregister_gate(hi3798mv200_gate_clks,
+				ARRAY_SIZE(hi3798mv200_gate_clks),
+				clk_data);
+unregister_mux:
+	hisi_clk_unregister_mux(hi3798mv200_mux_clks,
+				ARRAY_SIZE(hi3798mv200_mux_clks),
+				clk_data);
+unregister_fixed_rate:
+	hisi_clk_unregister_fixed_rate(hi3798mv200_fixed_rate_clks,
+				ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+				clk_data);
+	return ERR_PTR(ret);
+}
+
+static void hi3798mv200_clk_unregister(struct platform_device *pdev)
+{
+	struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+	of_clk_del_provider(pdev->dev.of_node);
+
+	hisi_clk_unregister_gate(hi3798mv200_gate_clks,
+				ARRAY_SIZE(hi3798mv200_gate_clks),
+				crg->clk_data);
+	hisi_clk_unregister_mux(hi3798mv200_mux_clks,
+				ARRAY_SIZE(hi3798mv200_mux_clks),
+				crg->clk_data);
+	hisi_clk_unregister_fixed_rate(hi3798mv200_fixed_rate_clks,
+				ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+				crg->clk_data);
+}
+
+static const struct hisi_crg_funcs hi3798mv200_crg_funcs = {
+	.register_clks = hi3798mv200_clk_register,
+	.unregister_clks = hi3798mv200_clk_unregister,
+};
+
+/* hi3798MV200 sysctrl CRG */
+
+#define HI3798MV200_SYSCTRL_INNER_CLK_OFFSET	16
+#define HI3798MV200_UART0_MUX			17
+
+#define HI3798MV200_SYSCTRL_NR_CLKS		32
+
+static const char *const uart0_mux[] = {
+		"3m", "75m" };
+static u32 uart0_mux_table[] = {0, 1};
+
+static const struct hisi_mux_clock hi3798mv200_sysctrl_mux_clks[] = {
+	{ HI3798MV200_UART0_MUX, "uart0_mux", uart0_mux, ARRAY_SIZE(uart0_mux),
+		CLK_SET_RATE_PARENT, 0x48, 29, 1, 0, uart0_mux_table, },
+};
+
+static const struct hisi_gate_clock hi3798mv200_sysctrl_gate_clks[] = {
+	{ HISTB_IR_CLK, "clk_ir", "24m",
+		CLK_SET_RATE_PARENT, 0x48, 4, 0, },
+	{ HISTB_TIMER01_CLK, "clk_timer01", "24m",
+		CLK_SET_RATE_PARENT, 0x48, 6, 0, },
+	{ HISTB_UART0_CLK, "clk_uart0", "uart0_mux",
+		CLK_SET_RATE_PARENT, 0x48, 12, 0, },
+};
+
+static struct hisi_clock_data *hi3798mv200_sysctrl_clk_register(
+					struct platform_device *pdev)
+{
+	struct hisi_clock_data *clk_data;
+	int ret;
+
+	clk_data = hisi_clk_alloc(pdev, HI3798MV200_SYSCTRL_NR_CLKS);
+	if (!clk_data)
+		return ERR_PTR(-ENOMEM);
+
+	ret = hisi_clk_register_mux(hi3798mv200_sysctrl_mux_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+				clk_data);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = hisi_clk_register_gate(hi3798mv200_sysctrl_gate_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+				clk_data);
+	if (ret)
+		goto unregister_mux;
+
+	ret = of_clk_add_provider(pdev->dev.of_node,
+			of_clk_src_onecell_get, &clk_data->clk_data);
+	if (ret)
+		goto unregister_gate;
+
+	return clk_data;
+
+unregister_gate:
+	hisi_clk_unregister_gate(hi3798mv200_sysctrl_gate_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+				clk_data);
+unregister_mux:
+	hisi_clk_unregister_mux(hi3798mv200_sysctrl_mux_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+				clk_data);
+	return ERR_PTR(ret);
+}
+
+static void hi3798mv200_sysctrl_clk_unregister(struct platform_device *pdev)
+{
+	struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+	of_clk_del_provider(pdev->dev.of_node);
+
+	hisi_clk_unregister_gate(hi3798mv200_sysctrl_gate_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+				crg->clk_data);
+	hisi_clk_unregister_mux(hi3798mv200_sysctrl_mux_clks,
+				ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+				crg->clk_data);
+}
+
+static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
+	.register_clks = hi3798mv200_sysctrl_clk_register,
+	.unregister_clks = hi3798mv200_sysctrl_clk_unregister,
+};
+
+static const struct of_device_id hi3798mv200_crg_match_table[] = {
+	{ .compatible = "hisilicon,hi3798mv200-crg",
+		.data = &hi3798mv200_crg_funcs },
+	{ .compatible = "hisilicon,hi3798mv200-sysctrl",
+		.data = &hi3798mv200_sysctrl_funcs },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hi3798mv200_crg_match_table);
+
+static int hi3798mv200_crg_probe(struct platform_device *pdev)
+{
+	struct hisi_crg_dev *crg;
+
+	crg = devm_kmalloc(&pdev->dev, sizeof(*crg), GFP_KERNEL);
+	if (!crg)
+		return -ENOMEM;
+
+	crg->funcs = of_device_get_match_data(&pdev->dev);
+	if (!crg->funcs)
+		return -ENOENT;
+
+	crg->rstc = hisi_reset_init(pdev);
+	if (!crg->rstc)
+		return -ENOMEM;
+
+	crg->clk_data = crg->funcs->register_clks(pdev);
+	if (IS_ERR(crg->clk_data)) {
+		hisi_reset_exit(crg->rstc);
+		return PTR_ERR(crg->clk_data);
+	}
+
+	platform_set_drvdata(pdev, crg);
+	return 0;
+}
+
+static int hi3798mv200_crg_remove(struct platform_device *pdev)
+{
+	struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+	hisi_reset_exit(crg->rstc);
+	crg->funcs->unregister_clks(pdev);
+	return 0;
+}
+
+static struct platform_driver hi3798mv200_crg_driver = {
+	.probe          = hi3798mv200_crg_probe,
+	.remove		= hi3798mv200_crg_remove,
+	.driver         = {
+		.name   = "hi3798mv200-crg",
+		.of_match_table = hi3798mv200_crg_match_table,
+	},
+};
+
+static int __init hi3798mv200_crg_init(void)
+{
+	return platform_driver_register(&hi3798mv200_crg_driver);
+}
+core_initcall(hi3798mv200_crg_init);
+
+static void __exit hi3798mv200_crg_exit(void)
+{
+	platform_driver_unregister(&hi3798mv200_crg_driver);
+}
+module_exit(hi3798mv200_crg_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("HiSilicon Hi3798MV200 CRG Driver");

-- 
2.43.0

Re: [PATCH RFC v2 2/5] clk: hisilicon: add CRG driver for Hi3798MV200 SoC

[Krzysztof Kozlowski]

On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> +
> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
> +	.register_clks = hi3798mv200_sysctrl_clk_register,
> +	.unregister_clks = hi3798mv200_sysctrl_clk_unregister,
> +};
> +
> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
> +	{ .compatible = "hisilicon,hi3798mv200-crg",
> +		.data = &hi3798mv200_crg_funcs },
> +	{ .compatible = "hisilicon,hi3798mv200-sysctrl",
> +		.data = &hi3798mv200_sysctrl_funcs },

These are undocumented compatibles. Run checkpatch or properly order
your patchset.

Best regards,
Krzysztof

Re: [PATCH RFC v2 2/5] clk: hisilicon: add CRG driver for Hi3798MV200 SoC

[Yang Xiwen]

On 2/20/2024 6:11 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> +
>> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
>> +	.register_clks = hi3798mv200_sysctrl_clk_register,
>> +	.unregister_clks = hi3798mv200_sysctrl_clk_unregister,
>> +};
>> +
>> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
>> +	{ .compatible = "hisilicon,hi3798mv200-crg",
>> +		.data = &hi3798mv200_crg_funcs },
>> +	{ .compatible = "hisilicon,hi3798mv200-sysctrl",
>> +		.data = &hi3798mv200_sysctrl_funcs },
> These are undocumented compatibles. Run checkpatch or properly order
> your patchset.
It's in patch 5. You mean binding patch first and then driver?
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH RFC v2 2/5] clk: hisilicon: add CRG driver for Hi3798MV200 SoC

[Krzysztof Kozlowski]

On 20/02/2024 11:14, Yang Xiwen wrote:
> On 2/20/2024 6:11 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> +
>>> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
>>> +	.register_clks = hi3798mv200_sysctrl_clk_register,
>>> +	.unregister_clks = hi3798mv200_sysctrl_clk_unregister,
>>> +};
>>> +
>>> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
>>> +	{ .compatible = "hisilicon,hi3798mv200-crg",
>>> +		.data = &hi3798mv200_crg_funcs },
>>> +	{ .compatible = "hisilicon,hi3798mv200-sysctrl",
>>> +		.data = &hi3798mv200_sysctrl_funcs },
>> These are undocumented compatibles. Run checkpatch or properly order
>> your patchset.
> It's in patch 5. You mean binding patch first and then driver?

https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/submitting-patches.rst

Best regards,
Krzysztof

[PATCH RFC v2 3/5] dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

We don't need so many separated and duplicated dt-binding files. Merge
them all and convert them to YAML.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../devicetree/bindings/clock/hi3660-clock.txt     |  47 -------
 .../devicetree/bindings/clock/hi3670-clock.txt     |  43 -------
 .../devicetree/bindings/clock/hi6220-clock.txt     |  52 --------
 .../devicetree/bindings/clock/hisi-crg.txt         |  50 --------
 .../clock/hisilicon,clock-reset-generator.yaml     | 139 +++++++++++++++++++++
 .../clock/hisilicon,hi3559av100-clock.yaml         |  59 ---------
 6 files changed, 139 insertions(+), 251 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/hi3660-clock.txt b/Documentation/devicetree/bindings/clock/hi3660-clock.txt
deleted file mode 100644
index 946da7cee54f..000000000000
--- a/Documentation/devicetree/bindings/clock/hi3660-clock.txt
+++ /dev/null
@@ -1,47 +0,0 @@
-* Hisilicon Hi3660 Clock Controller
-
-The Hi3660 clock controller generates and supplies clock to various
-controllers within the Hi3660 SoC.
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
-	indicate the clock controller functionality.
-
-	- "hisilicon,hi3660-crgctrl"
-	- "hisilicon,hi3660-pctrl"
-	- "hisilicon,hi3660-pmuctrl"
-	- "hisilicon,hi3660-sctrl"
-	- "hisilicon,hi3660-iomcu"
-	- "hisilicon,hi3660-stub-clk"
-
-- reg: physical base address of the controller and length of memory mapped
-  region.
-
-- #clock-cells: should be 1.
-
-Optional Properties:
-
-- mboxes: Phandle to the mailbox for sending message to MCU.
-            (See: ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3660-clock.h>.
-
-Examples:
-	crg_ctrl: clock-controller@fff35000 {
-		compatible = "hisilicon,hi3660-crgctrl", "syscon";
-		reg = <0x0 0xfff35000 0x0 0x1000>;
-		#clock-cells = <1>;
-	};
-
-	uart0: serial@fdf02000 {
-		compatible = "arm,pl011", "arm,primecell";
-		reg = <0x0 0xfdf02000 0x0 0x1000>;
-		interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&crg_ctrl HI3660_CLK_MUX_UART0>,
-			 <&crg_ctrl HI3660_PCLK>;
-		clock-names = "uartclk", "apb_pclk";
-	};
diff --git a/Documentation/devicetree/bindings/clock/hi3670-clock.txt b/Documentation/devicetree/bindings/clock/hi3670-clock.txt
deleted file mode 100644
index 66f3697eca78..000000000000
--- a/Documentation/devicetree/bindings/clock/hi3670-clock.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-* Hisilicon Hi3670 Clock Controller
-
-The Hi3670 clock controller generates and supplies clock to various
-controllers within the Hi3670 SoC.
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
-	indicate the clock controller functionality.
-
-	- "hisilicon,hi3670-crgctrl"
-	- "hisilicon,hi3670-pctrl"
-	- "hisilicon,hi3670-pmuctrl"
-	- "hisilicon,hi3670-sctrl"
-	- "hisilicon,hi3670-iomcu"
-	- "hisilicon,hi3670-media1-crg"
-	- "hisilicon,hi3670-media2-crg"
-
-- reg: physical base address of the controller and length of memory mapped
-  region.
-
-- #clock-cells: should be 1.
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3670-clock.h>.
-
-Examples:
-	crg_ctrl: clock-controller@fff35000 {
-		compatible = "hisilicon,hi3670-crgctrl", "syscon";
-		reg = <0x0 0xfff35000 0x0 0x1000>;
-		#clock-cells = <1>;
-	};
-
-	uart0: serial@fdf02000 {
-		compatible = "arm,pl011", "arm,primecell";
-		reg = <0x0 0xfdf02000 0x0 0x1000>;
-		interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&crg_ctrl HI3670_CLK_GATE_UART0>,
-			 <&crg_ctrl HI3670_PCLK>;
-		clock-names = "uartclk", "apb_pclk";
-	};
diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
deleted file mode 100644
index 17ac4a3dd26a..000000000000
--- a/Documentation/devicetree/bindings/clock/hi6220-clock.txt
+++ /dev/null
@@ -1,52 +0,0 @@
-* Hisilicon Hi6220 Clock Controller
-
-Clock control registers reside in different Hi6220 system controllers,
-please refer the following document to know more about the binding rules
-for these system controllers:
-
-Documentation/devicetree/bindings/arm/hisilicon/hisilicon.yaml
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
-	indicate the clock controller functionality.
-
-	- "hisilicon,hi6220-acpu-sctrl"
-	- "hisilicon,hi6220-aoctrl"
-	- "hisilicon,hi6220-sysctrl"
-	- "hisilicon,hi6220-mediactrl"
-	- "hisilicon,hi6220-pmctrl"
-	- "hisilicon,hi6220-stub-clk"
-
-- reg: physical base address of the controller and length of memory mapped
-  region.
-
-- #clock-cells: should be 1.
-
-Optional Properties:
-
-- hisilicon,hi6220-clk-sram: phandle to the syscon managing the SoC internal sram;
-  the driver need use the sram to pass parameters for frequency change.
-
-- mboxes: use the label reference for the mailbox as the first parameter, the
-  second parameter is the channel number.
-
-Example 1:
-	sys_ctrl: sys_ctrl@f7030000 {
-		compatible = "hisilicon,hi6220-sysctrl", "syscon";
-		reg = <0x0 0xf7030000 0x0 0x2000>;
-		#clock-cells = <1>;
-	};
-
-Example 2:
-	stub_clock: stub_clock {
-		compatible = "hisilicon,hi6220-stub-clk";
-		hisilicon,hi6220-clk-sram = <&sram>;
-		#clock-cells = <1>;
-		mboxes = <&mailbox 1>;
-	};
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi6220-clock.h>.
diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
deleted file mode 100644
index cc60b3d423f3..000000000000
--- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-* HiSilicon Clock and Reset Generator(CRG)
-
-The CRG module provides clock and reset signals to various
-modules within the SoC.
-
-This binding uses the following bindings:
-    Documentation/devicetree/bindings/clock/clock-bindings.txt
-    Documentation/devicetree/bindings/reset/reset.txt
-
-Required Properties:
-
-- compatible: should be one of the following.
-  - "hisilicon,hi3516cv300-crg"
-  - "hisilicon,hi3516cv300-sysctrl"
-  - "hisilicon,hi3519-crg"
-  - "hisilicon,hi3798cv200-crg"
-  - "hisilicon,hi3798cv200-sysctrl"
-
-- reg: physical base address of the controller and length of memory mapped
-  region.
-
-- #clock-cells: should be 1.
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
-
-- #reset-cells: should be 2.
-
-A reset signal can be controlled by writing a bit register in the CRG module.
-The reset specifier consists of two cells. The first cell represents the
-register offset relative to the base address. The second cell represents the
-bit index in the register.
-
-Example: CRG nodes
-CRG: clock-reset-controller@12010000 {
-	compatible = "hisilicon,hi3519-crg";
-	reg = <0x12010000 0x10000>;
-	#clock-cells = <1>;
-	#reset-cells = <2>;
-};
-
-Example: consumer nodes
-i2c0: i2c@12110000 {
-	compatible = "hisilicon,hi3519-i2c";
-	reg = <0x12110000 0x1000>;
-	clocks = <&CRG HI3519_I2C0_RST>;
-	resets = <&CRG 0xe4 0>;
-};
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
new file mode 100644
index 000000000000..d37cd892473e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon SOC Clock and Reset Generator (CRG) module
+
+maintainers:
+  - Yang Xiwen <forbidden405@foxmail.com>
+
+description: |
+  Hisilicon SOC clock control module which supports the clocks, resets and
+  power domains on various SoCs.
+
+properties:
+  compatible:
+    minItems: 1
+    items:
+      - enum:
+          - hisilicon,hi3559av100-clock
+          - hisilicon,hi3559av100-shub-clock
+          - hisilicon,hi3660-crgctrl
+          - hisilicon,hi3660-pctrl
+          - hisilicon,hi3660-pmuctrl
+          - hisilicon,hi3660-sctrl
+          - hisilicon,hi3660-iomcu
+          - hisilicon,hi3660-stub-clk
+          - hisilicon,hi3670-crgctrl
+          - hisilicon,hi3670-pctrl
+          - hisilicon,hi3670-pmuctrl
+          - hisilicon,hi3670-sctrl
+          - hisilicon,hi3670-iomcu
+          - hisilicon,hi3670-media1-crg
+          - hisilicon,hi3670-media2-crg
+          - hisilicon,hi6220-acpu-sctrl
+          - hisilicon,hi6220-aoctrl
+          - hisilicon,hi6220-sysctrl
+          - hisilicon,hi6220-mediactrl
+          - hisilicon,hi6220-pmctrl
+          - hisilicon,hi6220-stub-clk
+          - hisilicon,hi3516cv300-crg
+          - hisilicon,hi3516cv300-sysctrl
+          - hisilicon,hi3519-crg
+          - hisilicon,hi3798cv200-crg
+          - hisilicon,hi3798cv200-sysctrl
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    enum: [1, 2]
+    description: |
+      First cell is reset request register offset.
+      Second cell is bit offset in reset request register.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
+  mboxes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      Phandle to the mailbox for sending msg to MCU
+      (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
+
+  mbox-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: |
+      Names of the mailboxes.
+
+  hisilicon,hi6220-clk-sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to the syscon managing the SoC internal sram
+      the driver needs using the sram to pass parameters for frequency change.
+
+  reset-controller:
+    type: object
+    description: |
+      Reset controller for Hi3798CV200 GMAC module
+
+required:
+  - compatible
+  - '#clock-cells'
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - hisilicon,hi3798cv200-crg
+    then:
+      properties:
+        reset-controller: false
+  - oneOf:
+      - required:
+          - hisilicon,hi6220-clk-sram
+      - required:
+          - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3559av100-clock.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@12010000 {
+            compatible = "hisilicon,hi3559av100-clock";
+            #clock-cells = <1>;
+            #reset-cells = <2>;
+            reg = <0x0 0x12010000 0x0 0x10000>;
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/hi3660-clock.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@fff35000 {
+            compatible = "hisilicon,hi3660-crgctrl", "syscon";
+            reg = <0x0 0xfff35000 0x0 0x1000>;
+            #clock-cells = <1>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
deleted file mode 100644
index 3ceb29cec704..000000000000
--- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
+++ /dev/null
@@ -1,59 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Hisilicon SOC Clock for HI3559AV100
-
-maintainers:
-  - Dongjiu Geng <gengdongjiu@huawei.com>
-
-description: |
-  Hisilicon SOC clock control module which supports the clocks, resets and
-  power domains on HI3559AV100.
-
-  See also:
-    dt-bindings/clock/hi3559av100-clock.h
-
-properties:
-  compatible:
-    enum:
-      - hisilicon,hi3559av100-clock
-      - hisilicon,hi3559av100-shub-clock
-
-  reg:
-    minItems: 1
-    maxItems: 2
-
-  '#clock-cells':
-    const: 1
-
-  '#reset-cells':
-    const: 2
-    description: |
-      First cell is reset request register offset.
-      Second cell is bit offset in reset request register.
-
-required:
-  - compatible
-  - reg
-  - '#clock-cells'
-  - '#reset-cells'
-
-additionalProperties: false
-
-examples:
-  - |
-    soc {
-        #address-cells = <2>;
-        #size-cells = <2>;
-
-        clock-controller@12010000 {
-            compatible = "hisilicon,hi3559av100-clock";
-            #clock-cells = <1>;
-            #reset-cells = <2>;
-            reg = <0x0 0x12010000 0x0 0x10000>;
-        };
-    };
-...

-- 
2.43.0

Re: [PATCH RFC v2 3/5] dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator

[Krzysztof Kozlowski]

On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> We don't need so many separated and duplicated dt-binding files. Merge
> them all and convert them to YAML.

What was exactly duplicated? You created unspecific, lose binding...

Why this is RFC? RFC means we should not review.

> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../devicetree/bindings/clock/hi3660-clock.txt     |  47 -------
>  .../devicetree/bindings/clock/hi3670-clock.txt     |  43 -------
>  .../devicetree/bindings/clock/hi6220-clock.txt     |  52 --------
>  .../devicetree/bindings/clock/hisi-crg.txt         |  50 --------
>  .../clock/hisilicon,clock-reset-generator.yaml     | 139 +++++++++++++++++++++
>  .../clock/hisilicon,hi3559av100-clock.yaml         |  59 ---------
>  6 files changed, 139 insertions(+), 251 deletions(-)
> 


> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> new file mode 100644
> index 000000000000..d37cd892473e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hisilicon SOC Clock and Reset Generator (CRG) module
> +
> +maintainers:
> +  - Yang Xiwen <forbidden405@foxmail.com>
> +
> +description: |
> +  Hisilicon SOC clock control module which supports the clocks, resets and
> +  power domains on various SoCs.
> +
> +properties:
> +  compatible:
> +    minItems: 1

No, it does not work like that. Compatibles are fixed, not fluid. It's
quite a hint that your merging is wrong approach.


> +    items:
> +      - enum:
> +          - hisilicon,hi3559av100-clock
> +          - hisilicon,hi3559av100-shub-clock
> +          - hisilicon,hi3660-crgctrl
> +          - hisilicon,hi3660-pctrl
> +          - hisilicon,hi3660-pmuctrl
> +          - hisilicon,hi3660-sctrl
> +          - hisilicon,hi3660-iomcu
> +          - hisilicon,hi3660-stub-clk
> +          - hisilicon,hi3670-crgctrl
> +          - hisilicon,hi3670-pctrl
> +          - hisilicon,hi3670-pmuctrl
> +          - hisilicon,hi3670-sctrl
> +          - hisilicon,hi3670-iomcu
> +          - hisilicon,hi3670-media1-crg
> +          - hisilicon,hi3670-media2-crg
> +          - hisilicon,hi6220-acpu-sctrl
> +          - hisilicon,hi6220-aoctrl
> +          - hisilicon,hi6220-sysctrl
> +          - hisilicon,hi6220-mediactrl
> +          - hisilicon,hi6220-pmctrl
> +          - hisilicon,hi6220-stub-clk
> +          - hisilicon,hi3516cv300-crg
> +          - hisilicon,hi3516cv300-sysctrl
> +          - hisilicon,hi3519-crg
> +          - hisilicon,hi3798cv200-crg
> +          - hisilicon,hi3798cv200-sysctrl
> +      - const: syscon
> +      - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  '#reset-cells':
> +    enum: [1, 2]
> +    description: |

Previous bindings has only 2. Your patch is difficult to review and
understand.

> +      First cell is reset request register offset.
> +      Second cell is bit offset in reset request register.

All of these are reset controllers? I doubt.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1

All of these have children? No, sorry, but this merging does not make
any sense.

> +
> +  mboxes:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      Phandle to the mailbox for sending msg to MCU
> +      (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
> +
> +  mbox-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: |
> +      Names of the mailboxes.
> +
> +  hisilicon,hi6220-clk-sram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to the syscon managing the SoC internal sram
> +      the driver needs using the sram to pass parameters for frequency change.
> +
> +  reset-controller:
> +    type: object
> +    description: |
> +      Reset controller for Hi3798CV200 GMAC module
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              enum:
> +                - hisilicon,hi3798cv200-crg
> +    then:
> +      properties:
> +        reset-controller: false
> +  - oneOf:
> +      - required:
> +          - hisilicon,hi6220-clk-sram
> +      - required:
> +          - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/hi3559av100-clock.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clock-controller@12010000 {
> +            compatible = "hisilicon,hi3559av100-clock";
> +            #clock-cells = <1>;
> +            #reset-cells = <2>;
> +            reg = <0x0 0x12010000 0x0 0x10000>;
> +        };
> +    };
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/hi3660-clock.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clock-controller@fff35000 {
> +            compatible = "hisilicon,hi3660-crgctrl", "syscon";
> +            reg = <0x0 0xfff35000 0x0 0x1000>;
> +            #clock-cells = <1>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
> deleted file mode 100644
> index 3ceb29cec704..000000000000
> --- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> -%YAML 1.2
> ----
> -$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
> -$schema: http://devicetree.org/meta-schemas/core.yaml#

NAK, not related patch.

Please split all your patches into logical chunks.

Please read submitting-patches *BEFORE SENDING* further submissions.

Best regards,
Krzysztof

Re: [PATCH RFC v2 3/5] dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator

[Yang Xiwen]

On 2/20/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> We don't need so many separated and duplicated dt-binding files. Merge
>> them all and convert them to YAML.
> What was exactly duplicated? You created unspecific, lose binding...

You can take at the drivers at drivers/clk/hisilicon. All of them use 
the same sets of APIs to register a few clocks and resets. That's why i 
think they should be merged.


>
> Why this is RFC? RFC means we should not review.
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../devicetree/bindings/clock/hi3660-clock.txt     |  47 -------
>>   .../devicetree/bindings/clock/hi3670-clock.txt     |  43 -------
>>   .../devicetree/bindings/clock/hi6220-clock.txt     |  52 --------
>>   .../devicetree/bindings/clock/hisi-crg.txt         |  50 --------
>>   .../clock/hisilicon,clock-reset-generator.yaml     | 139 +++++++++++++++++++++
>>   .../clock/hisilicon,hi3559av100-clock.yaml         |  59 ---------
>>   6 files changed, 139 insertions(+), 251 deletions(-)
>>
>
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> new file mode 100644
>> index 000000000000..d37cd892473e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hisilicon SOC Clock and Reset Generator (CRG) module
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@foxmail.com>
>> +
>> +description: |
>> +  Hisilicon SOC clock control module which supports the clocks, resets and
>> +  power domains on various SoCs.
>> +
>> +properties:
>> +  compatible:
>> +    minItems: 1
> No, it does not work like that. Compatibles are fixed, not fluid. It's
> quite a hint that your merging is wrong approach.
>
>
>> +    items:
>> +      - enum:
>> +          - hisilicon,hi3559av100-clock
>> +          - hisilicon,hi3559av100-shub-clock
>> +          - hisilicon,hi3660-crgctrl
>> +          - hisilicon,hi3660-pctrl
>> +          - hisilicon,hi3660-pmuctrl
>> +          - hisilicon,hi3660-sctrl
>> +          - hisilicon,hi3660-iomcu
>> +          - hisilicon,hi3660-stub-clk
>> +          - hisilicon,hi3670-crgctrl
>> +          - hisilicon,hi3670-pctrl
>> +          - hisilicon,hi3670-pmuctrl
>> +          - hisilicon,hi3670-sctrl
>> +          - hisilicon,hi3670-iomcu
>> +          - hisilicon,hi3670-media1-crg
>> +          - hisilicon,hi3670-media2-crg
>> +          - hisilicon,hi6220-acpu-sctrl
>> +          - hisilicon,hi6220-aoctrl
>> +          - hisilicon,hi6220-sysctrl
>> +          - hisilicon,hi6220-mediactrl
>> +          - hisilicon,hi6220-pmctrl
>> +          - hisilicon,hi6220-stub-clk
>> +          - hisilicon,hi3516cv300-crg
>> +          - hisilicon,hi3516cv300-sysctrl
>> +          - hisilicon,hi3519-crg
>> +          - hisilicon,hi3798cv200-crg
>> +          - hisilicon,hi3798cv200-sysctrl
>> +      - const: syscon
>> +      - const: simple-mfd
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  '#reset-cells':
>> +    enum: [1, 2]
>> +    description: |
> Previous bindings has only 2. Your patch is difficult to review and
> understand.
>
>> +      First cell is reset request register offset.
>> +      Second cell is bit offset in reset request register.
> All of these are reset controllers? I doubt.
>
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 1
> All of these have children? No, sorry, but this merging does not make
> any sense.
>
>> +
>> +  mboxes:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: |
>> +      Phandle to the mailbox for sending msg to MCU
>> +      (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
>> +
>> +  mbox-names:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description: |
>> +      Names of the mailboxes.
>> +
>> +  hisilicon,hi6220-clk-sram:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: |
>> +      Phandle to the syscon managing the SoC internal sram
>> +      the driver needs using the sram to pass parameters for frequency change.
>> +
>> +  reset-controller:
>> +    type: object
>> +    description: |
>> +      Reset controller for Hi3798CV200 GMAC module
>> +
>> +required:
>> +  - compatible
>> +  - '#clock-cells'
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          not:
>> +            contains:
>> +              enum:
>> +                - hisilicon,hi3798cv200-crg
>> +    then:
>> +      properties:
>> +        reset-controller: false
>> +  - oneOf:
>> +      - required:
>> +          - hisilicon,hi6220-clk-sram
>> +      - required:
>> +          - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/hi3559av100-clock.h>
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        clock-controller@12010000 {
>> +            compatible = "hisilicon,hi3559av100-clock";
>> +            #clock-cells = <1>;
>> +            #reset-cells = <2>;
>> +            reg = <0x0 0x12010000 0x0 0x10000>;
>> +        };
>> +    };
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/hi3660-clock.h>
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        clock-controller@fff35000 {
>> +            compatible = "hisilicon,hi3660-crgctrl", "syscon";
>> +            reg = <0x0 0xfff35000 0x0 0x1000>;
>> +            #clock-cells = <1>;
>> +        };
>> +    };
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
>> deleted file mode 100644
>> index 3ceb29cec704..000000000000
>> --- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
>> +++ /dev/null
>> @@ -1,59 +0,0 @@
>> -# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> -%YAML 1.2
>> ----
>> -$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
>> -$schema: http://devicetree.org/meta-schemas/core.yaml#
> NAK, not related patch.


Okay. I'll revert most of the changes here. Maybe i should only convert 
hisi-crg.txt to yaml. That's what i really cares.


>
> Please split all your patches into logical chunks.
>
> Please read submitting-patches *BEFORE SENDING* further submissions.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Re: [PATCH RFC v2 3/5] dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator

[Krzysztof Kozlowski]

On 20/02/2024 11:52, Yang Xiwen wrote:
> On 2/20/2024 6:14 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> We don't need so many separated and duplicated dt-binding files. Merge
>>> them all and convert them to YAML.
>> What was exactly duplicated? You created unspecific, lose binding...
> 
> You can take at the drivers at drivers/clk/hisilicon. All of them use 
> the same sets of APIs to register a few clocks and resets. That's why i 
> think they should be merged.

Drivers don't really matter for bindings. That's not a valid argument.

Creating invalid combinations and lose bindings is not the answer to
duplication of few parts.

Best regards,
Krzysztof

[PATCH RFC v2 4/5] dt-bindings: mfd: syscon: Add hisilicon,sdmmc-sap-dll compatible

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Add hisilicon,sdmmc-sap-dll compatible. This is a 8 bytes range with two
registers. Mainly used for precise sample phase selection during eMMC
tuning.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 084b5c2a2a3c..c685d4b36ea4 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -47,6 +47,7 @@ properties:
               - hisilicon,hi6220-sramctrl
               - hisilicon,pcie-sas-subctrl
               - hisilicon,peri-subctrl
+              - hisilicon,sdmmc-sap-dll
               - hpe,gxp-sysreg
               - intel,lgm-syscon
               - loongson,ls1b-syscon

-- 
2.43.0

[PATCH RFC v2 5/5] dt-bindings: clock: hisilicon,clock-reset-controller: add Hi3798MV200 SoC support

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

This SoC is similar to Hi3798CV200.

Also document the specific DLL regs and add an example for it.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../clock/hisilicon,clock-reset-generator.yaml     | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
index d37cd892473e..8ee844574eda 100644
--- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
+++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
@@ -44,12 +44,17 @@ properties:
           - hisilicon,hi3519-crg
           - hisilicon,hi3798cv200-crg
           - hisilicon,hi3798cv200-sysctrl
+          - hisilicon,hi3798mv200-crg
+          - hisilicon,hi3798mv200-sysctrl
       - const: syscon
       - const: simple-mfd
 
   reg:
     maxItems: 1
 
+  ranges:
+    maxItems: 1
+
   '#clock-cells':
     const: 1
 
@@ -87,6 +92,12 @@ properties:
     description: |
       Reset controller for Hi3798CV200 GMAC module
 
+patternProperties:
+  '.*-dll@[0-9a-f]+':
+    type: object
+    description: |
+      eMMC/SD delay-locked-loop (DLL) register subnode
+
 required:
   - compatible
   - '#clock-cells'
@@ -137,3 +148,28 @@ examples:
             #clock-cells = <1>;
         };
     };
+  - |
+    crg: clock-reset-controller@8a22000 {
+        compatible = "hisilicon,hi3798mv200-crg", "syscon", "simple-mfd";
+        reg = <0x8a22000 0x1000>;
+        ranges = <0x0 0x8a22000 0x1000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        #clock-cells = <1>;
+        #reset-cells = <2>;
+
+        emmc_sap_dll: sap-dll@39c {
+            compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+            reg = <0x39c 0x8>;
+        };
+
+        sdio0_sap_dll: sap-dll@3a4 {
+            compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+            reg = <0x3a4 0x8>;
+        };
+
+        sdio1_sap_dll: sap-dll@3ac {
+            compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+            reg = <0x3ac 0x8>;
+        };
+    };

-- 
2.43.0

Re: [PATCH RFC v2 5/5] dt-bindings: clock: hisilicon,clock-reset-controller: add Hi3798MV200 SoC support

[Krzysztof Kozlowski]

On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> This SoC is similar to Hi3798CV200.
> 
> Also document the specific DLL regs and add an example for it.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../clock/hisilicon,clock-reset-generator.yaml     | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> index d37cd892473e..8ee844574eda 100644
> --- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> @@ -44,12 +44,17 @@ properties:
>            - hisilicon,hi3519-crg
>            - hisilicon,hi3798cv200-crg
>            - hisilicon,hi3798cv200-sysctrl
> +          - hisilicon,hi3798mv200-crg
> +          - hisilicon,hi3798mv200-sysctrl
>        - const: syscon
>        - const: simple-mfd
>  
>    reg:
>      maxItems: 1
>  
> +  ranges:
> +    maxItems: 1
> +
>    '#clock-cells':
>      const: 1
>  
> @@ -87,6 +92,12 @@ properties:
>      description: |
>        Reset controller for Hi3798CV200 GMAC module
>  
> +patternProperties:
> +  '.*-dll@[0-9a-f]+':
> +    type: object
> +    description: |
> +      eMMC/SD delay-locked-loop (DLL) register subnode

NAK, now all of the syscons have the DLL node?

Best regards,
Krzysztof

Re: [PATCH RFC v2 5/5] dt-bindings: clock: hisilicon,clock-reset-controller: add Hi3798MV200 SoC support

[Yang Xiwen]

On 2/20/2024 6:15 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> This SoC is similar to Hi3798CV200.
>>
>> Also document the specific DLL regs and add an example for it.
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../clock/hisilicon,clock-reset-generator.yaml     | 36 ++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> index d37cd892473e..8ee844574eda 100644
>> --- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> @@ -44,12 +44,17 @@ properties:
>>             - hisilicon,hi3519-crg
>>             - hisilicon,hi3798cv200-crg
>>             - hisilicon,hi3798cv200-sysctrl
>> +          - hisilicon,hi3798mv200-crg
>> +          - hisilicon,hi3798mv200-sysctrl
>>         - const: syscon
>>         - const: simple-mfd
>>   
>>     reg:
>>       maxItems: 1
>>   
>> +  ranges:
>> +    maxItems: 1
>> +
>>     '#clock-cells':
>>       const: 1
>>   
>> @@ -87,6 +92,12 @@ properties:
>>       description: |
>>         Reset controller for Hi3798CV200 GMAC module
>>   
>> +patternProperties:
>> +  '.*-dll@[0-9a-f]+':
>> +    type: object
>> +    description: |
>> +      eMMC/SD delay-locked-loop (DLL) register subnode
> NAK, now all of the syscons have the DLL node?
Oops. Forgot to remove this. it should be removed since it's now not 
used anymore.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen