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

From: Yang Xiwen
Date: Tue Feb 20 2024 - 12:29:59 EST

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@xxxxxxxxxxx>

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@xxxxxxxxxxx>
---
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