Add support for AST2700 clk driver

PATCH Superseded Ryan Chen 4 participants 18 messages Feb 24, 2025 – Mar 6, 2025

Subsystems (4)

Device tree bindings Common clk Arm/Aspeed machine support Reset controller framework

lore

Share & tools

lore

Ryan Chen

author reviewer

2025-02-24 09:55 UTC #0 lore raw

v9 cover Add support for AST2700 clk driver

This patch series is add clk driver for AST2700.

AST2700 is the 8th generation of Integrated Remote Management Processor introduced by ASPEED Technology Inc. Which is Board Management controller (BMC) SoC family. AST2700 have two SoC connected, one is SoC0, another is SoC1, it has it's own scu, this driver inlcude SCU0 and SCU1 driver.

v9:
-aspeed,ast2700-scu.h: no change.
add more clear commit description.
-clk-ast2700.c:
add inlcude bitfield.h
remove redundant clk_parent_data soc0_mpll_div8/soc0_ahb/uart13clk/ uart14clk/uart15clk/uart16clk/soc1_ahb/d_clk_sels

v8:
-aspeed,ast2700-scu.h: remove no use soc0 clock, add new clock -clk-ast2700.c: remove include <linux/auxiliary_bus.h>, include <linux/clk-provider.h>, include <linux/of_address.h> -clk-ast2700.c: add include <linux/mod_devicetable.h> -clk-ast2700.c: modify include <soc/aspeed/reset-aspeed.h> order before dt-bindings
-clk-ast2700.c: modify define to be tabbed out space -clk-ast2700.c: add union struct for each clk type

	union {
		struct ast2700_clk_fixed_factor_data factor;
		struct ast2700_clk_fixed_rate_data rate;
		struct ast2700_clk_gate_data gate;
		struct ast2700_clk_div_data div;
		struct ast2700_clk_pll_data pll;
		struct ast2700_clk_mux_data mux;
	} data;

-clk-ast2700.c: modify clk_data = device_get_match_data(dev); -clk-ast2700.c: modify builtin_platform_driver_probe to arch_initcall(clk_ast2700_init)
-clk-ast2700.c: ast2700_clk_hw_register_hpll explain: scu010[4:2], scu010[4:2] = 010, hpll force 1.8Ghz
scu010[4:2] = 011, hpll force 1.7Ghz
scu010[4:2] = 110, hpll force 1.2Ghz
scu010[4:2] = 111, hpll force 800Mhz
others depend on hpll parameter register setting.

v7:
-reset-aspeed.h: fix declare static inline aspeed_reset_controller_register if the function is not used.

v6:
-patch-2: add reset-aspeed.h
-reset-aspeed: add include cleanup.h for guard() -reset-aspeed: change ids name clk_aspeed to reset_aspeed -reset-aspeed: move aspeed_reset_controller_register, aspeed_reset_adev_release, aspeed_reset_unregister_adev from clk-ast2700.c -reset-aspeed: drop base check, since it check in clk-ast2700.c -clk-ast2700: sync each gate name from *clk to *clk-gate name. -clk-ast2700: add CLK_GATE_ASPEED to diff clk_hw_register_gate and ast2700_clk_hw_register_gate.

v5:
-patch-2 Kconfig: add select AUXILIARY_BUS
-reset-aspeed: #define to_aspeed_reset(p) turn into static inline function. -reset-aspeed: modify spin_lock_irqsave to guard(spinlock_irqsave) -reset-aspeed: remove unnecessary parentheses.
-clk-ast2700: use <linux/units.h> and refrain from define clk

v4:
-yaml: keep size-cells=<1>.
-merge clk,reset dt binding header with yaml the same patch. -rename clk,reset dt binding header to aspeed,ast2700-scu.h -reset-aspeed: update tables tabs sapces to consistent spaces. -reset-aspeed: remove no use dev_set_drvdata.
-clk-ast2700: modify reset_name to const int scu in struct clk_data. -clk-ast2700: use scu number in clk_data generate reset_name for reset
driver register.
-clk-ast2700: fix pll number mix up scu0,scu1. -clk-ast2700: update dt-binding clock include file.

v3:
-yaml: v2 missing send yaml patch, v3 add. -yaml: drop 64bits address example.
-yaml: add discription about soc0 and soc1 -dt-bindings: remove (), *_NUMS, reserved. -dt-bindings: remove dulipated define number. -dt-bindings: merge clk and reset to be one patch. -reset-aspeed: add auxiliary device for reset driver. -clk-ast2700: modify reset to be auxiliary add. -clk-ast2700: modify to be platform driver. -clk-ast2700: modify each clk to const clk array.

v2:
-yaml: drop 64bits address example.
-yaml: add discription about soc0 and soc1
-dt-bindings: remove (), *_NUMS, reserved.
-dt-bindings: remove dulipated define number
-clk-ast2700: drop WARN_ON, weird comment.

Ryan Chen (3):
dt-binding: clock: ast2700: modify soc0/1 clock define
reset: aspeed: register AST2700 reset auxiliary bus device
clk: aspeed: add AST2700 clock driver

 drivers/clk/Kconfig                           |    8 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/clk-ast2700.c                     | 1120 +++++++++++++++++
 drivers/reset/Kconfig                         |    7 +
 drivers/reset/Makefile                        |    1 +
 drivers/reset/reset-aspeed.c                  |  302 +++++
 .../dt-bindings/clock/aspeed,ast2700-scu.h    |    7 +-
 include/soc/aspeed/reset-aspeed.h             |   21 +
 8 files changed, 1464 insertions(+), 3 deletions(-)
 create mode 100644 drivers/clk/clk-ast2700.c
 create mode 100644 drivers/reset/reset-aspeed.c
 create mode 100644 include/soc/aspeed/reset-aspeed.h

Signature · 1 line

2.34.1

Ryan Chen

author reviewer

2025-02-24 09:55 UTC #1 lore raw

PATCH v9 1/3 dt-binding: clock: ast2700: modify soc0/1 clock define

-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.

-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb

                  -> clk_ahb

hpll->hpll_div_ahb

new use SOC0_CLK_AHBMUX for more understand clock source divide tree. mpll->

      ahb_mux -> div_table -> clk_ahb

hpll->

-new add clock:
SOC0_CLK_MPHYSRC: UFS MPHY clock source.
SOC0_CLK_U2PHY_REFCLKSRC: USB2.0 phy clock reference source.
SOC1_CLK_I3C: I3C clock source.

Signed-off-by Ryan Chen <[email protected]>

---
 include/dt-bindings/clock/aspeed,ast2700-scu.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

include/dt-bindings/clock/aspeed,ast2700-scu.h +4 -3

index 63021af3caf5..c7389530629d 100644
@@ -13,18 +13,17 @@
 #define SCU0_CLK_24M		1
 #define SCU0_CLK_192M		2
 #define SCU0_CLK_UART		3
-#define SCU0_CLK_UART_DIV13	3
 #define SCU0_CLK_PSP		4
 #define SCU0_CLK_HPLL		5
 #define SCU0_CLK_HPLL_DIV2	6
 #define SCU0_CLK_HPLL_DIV4	7
-#define SCU0_CLK_HPLL_DIV_AHB	8
+#define SCU0_CLK_AHBMUX		8
 #define SCU0_CLK_DPLL		9
 #define SCU0_CLK_MPLL		10
 #define SCU0_CLK_MPLL_DIV2	11
 #define SCU0_CLK_MPLL_DIV4	12
 #define SCU0_CLK_MPLL_DIV8	13
-#define SCU0_CLK_MPLL_DIV_AHB	14
+#define SCU0_CLK_MPHYSRC	14
 #define SCU0_CLK_D0		15
 #define SCU0_CLK_D1		16
 #define SCU0_CLK_CRT0		17

@@ -68,6 +67,7 @@
 #define SCU0_CLK_GATE_UFSCLK	53
 #define SCU0_CLK_GATE_EMMCCLK	54
 #define SCU0_CLK_GATE_RVAS1CLK	55
+#define SCU0_CLK_U2PHY_REFCLKSRC 56
 
 /* SOC1 clk */
 #define SCU1_CLKIN		0

@@ -160,4 +160,5 @@
 #define SCU1_CLK_GATE_PORTDUSB2CLK	85
 #define SCU1_CLK_GATE_LTPI1TXCLK	86
 
+#define SCU1_CLK_I3C	87
 #endif

Signature · 1 line

2.34.1

Krzysztof Kozlowski

2025-02-24 10:10 UTC #2 lore raw

Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 10 lines · Ryan Chen

-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb

I can barely understand it and from the pieces I got, it does not explain need for ABI break.

Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-25 00:41 UTC #3 lore raw

RE: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 8 lines

Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 4 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.

Quoted 11 lines

Quoted 6 lines
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.

#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX
Older implement where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for AHB. In **the new approach (v8)**, I refactored the clock tree to clock tree.
It should be ABI-safe change

Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add SOC0_CLK_AHBMUX. To be 1st patch, then 2n patch remove redundant SOC0_CLK_HPLL_DIV_AHB?

Quoted 3 lines

Best regards,
Krzysztof

Krzysztof Kozlowski

2025-02-25 07:54 UTC #4 lore raw

Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 20 lines · Ryan Chen

Quoted 8 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 4 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
Quoted 11 lines
Quoted 6 lines
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.

Quoted 1 line

#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break

You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI break.

Quoted 3 lines

#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX
Older implement where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for AHB. In **the new approach (v8)**, I refactored the clock tree to clock tree.

I still cannot parse sentences like "refactoring A to A". It's meaningless to me.

Quoted 1 line

It should be ABI-safe change

No, you do not understand the ABI. You removed a clock ID, that's the ABI change.

Otherwise explain how this is not changing ABI.

Quoted 3 lines

Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add SOC0_CLK_AHBMUX. To be 1st patch, then 2n patch remove redundant SOC0_CLK_HPLL_DIV_AHB?

If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.

Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-25 09:49 UTC #5 lore raw

RE: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 20 lines

Quoted 20 lines
Quoted 19 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 10 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.

Quoted 5 lines

Quoted 1 line
#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI break.

Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any upstream device trees. Since there is no in-tree usage of SCU0_CLK_UART_DIV13, its removal does not cause an ABI break.

Quoted 17 lines

Quoted 1 line
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
implement
Quoted 1 line
where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for
AHB.
Quoted 1 line
In **the new approach (v8)**, I refactored the clock tree to clock tree.
I still cannot parse sentences like "refactoring A to A". It's meaningless to me.
Quoted 1 line
It should be ABI-safe change
No, you do not understand the ABI. You removed a clock ID, that's the ABI change.
Otherwise explain how this is not changing ABI.
Quoted 1 line

Quoted 8 lines

Quoted 1 line
Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add
SOC0_CLK_AHBMUX.
Quoted 1 line
To be 1st patch, then 2n patch remove redundant
SOC0_CLK_HPLL_DIV_AHB?
If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.

Sorry, my point will be following steps to avoid potential ABI issues, I can modify the patch series as follows:
1. **Patch 1:** Add SOC0_CLK_AHBMUX without removing SOC0_CLK_HPLL_DIV_AHB.
2. **Patch 2:** Finally remove SOC0_CLK_HPLL_DIV_AHB.

Let me know if you prefer this approach.

Quoted 3 lines

Best regards,
Krzysztof

Krzysztof Kozlowski

2025-02-25 13:13 UTC #6 lore raw

Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 20 lines · Ryan Chen

Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 10 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI break.
Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any upstream device trees.

That's incomplete definition of ABI

Quoted 1 line

Since there is no in-tree usage of SCU0_CLK_UART_DIV13, its removal does not cause an ABI break.

You ignored out of tree users. Please read carefully ABI docs.

Quoted 23 lines

Quoted 23 lines
Quoted 2 lines
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
implement
Quoted 1 line
where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for
AHB.
Quoted 1 line
In **the new approach (v8)**, I refactored the clock tree to clock tree.
I still cannot parse sentences like "refactoring A to A". It's meaningless to me.
Quoted 1 line
It should be ABI-safe change
No, you do not understand the ABI. You removed a clock ID, that's the ABI change.
Otherwise explain how this is not changing ABI.
Quoted 2 lines
Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add
SOC0_CLK_AHBMUX.
Quoted 1 line
To be 1st patch, then 2n patch remove redundant
SOC0_CLK_HPLL_DIV_AHB?

Quoted 7 lines

Quoted 3 lines
If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.
Sorry, my point will be following steps to avoid potential ABI issues, I can modify the patch series as follows:
1. **Patch 1:** Add SOC0_CLK_AHBMUX without removing SOC0_CLK_HPLL_DIV_AHB.
2. **Patch 2:** Finally remove SOC0_CLK_HPLL_DIV_AHB.

I do not understand what changed here. You remove exported clock which is ABI, so how is this answering my question.

You keep dodging my questions. Here I asked "why". I do not see any answer why.

Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-26 05:10 UTC #7 lore raw

RE: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 76 lines

Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 20 lines · Ryan Chen
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 10 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI break.
Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any
upstream device trees.
That's incomplete definition of ABI
Quoted 1 line
Since there is no in-tree usage of SCU0_CLK_UART_DIV13, its removal does
not cause an ABI break.
You ignored out of tree users. Please read carefully ABI docs.
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
implement
Quoted 2 lines
where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for
AHB.
Quoted 1 line
In **the new approach (v8)**, I refactored the clock tree to clock tree.
I still cannot parse sentences like "refactoring A to A". It's meaningless to
me.
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
It should be ABI-safe change
No, you do not understand the ABI. You removed a clock ID, that's the ABI change.
Otherwise explain how this is not changing ABI.
Quoted 3 lines
Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add
SOC0_CLK_AHBMUX.
Quoted 1 line
To be 1st patch, then 2n patch remove redundant
Quoted 8 lines
Quoted 5 lines
SOC0_CLK_HPLL_DIV_AHB?
If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.
Sorry, my point will be following steps to avoid potential ABI issues, I can modify the patch series as follows:
1. **Patch 1:** Add SOC0_CLK_AHBMUX without removing
SOC0_CLK_HPLL_DIV_AHB.

Quoted 8 lines

Quoted 1 line
2. **Patch 2:** Finally remove SOC0_CLK_HPLL_DIV_AHB.
I do not understand what changed here. You remove exported clock which is ABI, so how is this answering my question.
You keep dodging my questions. Here I asked "why". I do not see any answer why.

Apology, I can't catch your point. But I still need your guideline.

**Regarding SCU0_CLK_UART_DIV13:**
- This clock ID was originally defined but was never used in any in-tree device trees. (i2c-ast2700.c v1 ~ v9)
- So there should not have ABI-break am I correct?

**Regarding SOC0_CLK_HPLL_DIV_AHB → SOC0_CLK_AHBMUX:**
- The previous implementation used dividers (mpll_div_ahb, hpll_div_ahb) for AHB clock selection. (i2c-ast2700.c v1 ~ v8)
mpll->mpll_div_ahb

                  -> clk_ahb
hpll->hpll_div_ahb
   - The new approach use `SOC0_CLK_AHBMUX`, which AHB clock sources via a mux. (i2c-ast2700.c v9)
mpll->
      ahb_mux -> div_table -> clk_ahb

hpll->
- And since i2c-ast2700.c (v8) is not applied, so there should also no one use this ABI. Am I correct?

If this is not acceptable, my next patch will keep SCU0_CLK_UART_DIV13/SCU0_CLK_HPLL_DIV_AHB define. But add new SCU0_CLK_AHBMUX define for avoid your point ABI break. Is this acceptable?

Any more clear guideline will help more clear your through. Thank your patience.

Quoted 3 lines

Best regards,
Krzysztof

Krzysztof Kozlowski

2025-02-26 09:52 UTC #8 lore raw

Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 82 lines · Ryan Chen

Quoted 82 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 20 lines · Ryan Chen
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 10 lines · Ryan Chen
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source. mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
I can barely understand it and from the pieces I got, it does not explain need for ABI break.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI break.
Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any
upstream device trees.
That's incomplete definition of ABI
Quoted 1 line
Since there is no in-tree usage of SCU0_CLK_UART_DIV13, its removal does
not cause an ABI break.
You ignored out of tree users. Please read carefully ABI docs.
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
implement
Quoted 2 lines
where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for
AHB.
Quoted 1 line
In **the new approach (v8)**, I refactored the clock tree to clock tree.
I still cannot parse sentences like "refactoring A to A". It's meaningless to
me.
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
It should be ABI-safe change
No, you do not understand the ABI. You removed a clock ID, that's the ABI change.
Otherwise explain how this is not changing ABI.
Quoted 3 lines
Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add
SOC0_CLK_AHBMUX.
Quoted 1 line
To be 1st patch, then 2n patch remove redundant
Quoted 8 lines
Quoted 5 lines
SOC0_CLK_HPLL_DIV_AHB?
If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.
Sorry, my point will be following steps to avoid potential ABI issues, I can modify the patch series as follows:
1. **Patch 1:** Add SOC0_CLK_AHBMUX without removing
SOC0_CLK_HPLL_DIV_AHB.
Quoted 1 line
2. **Patch 2:** Finally remove SOC0_CLK_HPLL_DIV_AHB.
I do not understand what changed here. You remove exported clock which is ABI, so how is this answering my question.

Quoted 8 lines

Quoted 2 lines
You keep dodging my questions. Here I asked "why". I do not see any answer why.
Apology, I can't catch your point. But I still need your guideline.
**Regarding SCU0_CLK_UART_DIV13:**
- This clock ID was originally defined but was never used in any in-tree device trees. (i2c-ast2700.c v1 ~ v9)
- So there should not have ABI-break am I correct?

No, because there are other users of bindings. All forks, out of tree DTS, other projects etc. You defined ABI for them.

Quoted 10 lines

**Regarding SOC0_CLK_HPLL_DIV_AHB → SOC0_CLK_AHBMUX:**
- The previous implementation used dividers (mpll_div_ahb, hpll_div_ahb) for AHB clock selection. (i2c-ast2700.c v1 ~ v8)
mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
   - The new approach use `SOC0_CLK_AHBMUX`, which AHB clock sources via a mux. (i2c-ast2700.c v9)
mpll->
      ahb_mux -> div_table -> clk_ahb
hpll->

Your formatting is one of the problems I have troubles understanding it. Above is not wrapped or wrapped oddly. You keep using bold * but double **, which is not a standard markup. Please switch to standard mailing list formatting - proper wrapping, only text mode and use client which actually can parse and create that.

What I don't understand is how clocks could change in hardware. You described implementation, but the clock IDS do not describe implementation but the the mapping to real hardware clocks. So how SOC0_CLK_HPLL_DIV_AHB clock disappeared from hardware?

Quoted 1 line

- And since i2c-ast2700.c (v8) is not applied, so there should also no one use this ABI. Am I correct?

If binding was not applied, then what are you changing here?

Does it mean header described clock which was in the hardware but its handling was not yet implemented in the driver?

Quoted 2 lines

If this is not acceptable, my next patch will keep SCU0_CLK_UART_DIV13/SCU0_CLK_HPLL_DIV_AHB define.

Maybe my last message was not clear, so: you need to explain why you are breaking ABI and/or explain the ABI impact in the commit msg.

Quoted 1 line

But add new SCU0_CLK_AHBMUX define for avoid your point ABI break. Is this acceptable?

Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-27 08:04 UTC #9 lore raw

RE: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

Quoted 125 lines

Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 7 lines · Ryan Chen
Quoted 7 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 3 lines · Ryan Chen
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Ryan Chen
Quoted 7 lines
-remove redundant SOC0_CLK_UART_DIV13:
SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source tree is uart clk src -> uart_div_table -> uart clk.
-Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
modify clock tree implement.
older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock
source.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
mpll->mpll_div_ahb
                  -> clk_ahb
hpll->hpll_div_ahb
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
I can barely understand it and from the pieces I got, it does not explain need for ABI break.
#1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
You did not explain how it does not impact. Clock was exported, there was a user and now there is no clock. User stops working. ABI
break.
Quoted 31 lines
Quoted 31 lines
Quoted 4 lines
Quoted 1 line
Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any
upstream device trees.
That's incomplete definition of ABI
Quoted 2 lines
Since there is no in-tree usage of SCU0_CLK_UART_DIV13, its removal does
not cause an ABI break.
You ignored out of tree users. Please read carefully ABI docs.
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
#2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
implement
Quoted 2 lines
where mpll_div_ahb and hpll_div_ahb were **hardcoded dividers** for
AHB.
Quoted 1 line
In **the new approach (v8)**, I refactored the clock tree to clock tree.
I still cannot parse sentences like "refactoring A to A". It's meaningless to
me.
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
It should be ABI-safe change
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
No, you do not understand the ABI. You removed a clock ID, that's the ABI change.
Otherwise explain how this is not changing ABI.
Quoted 2 lines
Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and
then
Quoted 18 lines
Quoted 18 lines
Quoted 11 lines
Quoted 8 lines
Quoted 1 line
add
SOC0_CLK_AHBMUX.
Quoted 1 line
To be 1st patch, then 2n patch remove redundant
SOC0_CLK_HPLL_DIV_AHB?
If you break the ABI you need to clearly explain why. We have long conversations and you still did not say why.
Sorry, my point will be following steps to avoid potential ABI issues, I can modify the patch series as follows:
1. **Patch 1:** Add SOC0_CLK_AHBMUX without removing
SOC0_CLK_HPLL_DIV_AHB.
Quoted 1 line
2. **Patch 2:** Finally remove SOC0_CLK_HPLL_DIV_AHB.
I do not understand what changed here. You remove exported clock which is ABI, so how is this answering my question.
Quoted 7 lines
Quoted 2 lines
You keep dodging my questions. Here I asked "why". I do not see any answer why.
Apology, I can't catch your point. But I still need your guideline.
**Regarding SCU0_CLK_UART_DIV13:**
- This clock ID was originally defined but was never used in any in-tree
device trees. (i2c-ast2700.c v1 ~ v9)
Quoted 1 line
- So there should not have ABI-break am I correct?
No, because there are other users of bindings. All forks, out of tree DTS, other projects etc. You defined ABI for them.
Quoted 5 lines
**Regarding SOC0_CLK_HPLL_DIV_AHB → SOC0_CLK_AHBMUX:**
- The previous implementation used dividers (mpll_div_ahb,
hpll_div_ahb) for AHB clock selection. (i2c-ast2700.c v1 ~ v8) mpll->mpll_div_ahb
Quoted 7 lines
                  -> clk_ahb
hpll->hpll_div_ahb
- The new approach use SOC0_CLK_AHBMUX, which AHB clock sources
via a mux. (i2c-ast2700.c v9)
mpll->
      ahb_mux -> div_table -> clk_ahb
hpll->
Your formatting is one of the problems I have troubles understanding it. Above is not wrapped or wrapped oddly. You keep using bold * but double **, which is not a standard markup. Please switch to standard mailing list formatting - proper wrapping, only text mode and use client which actually can parse and create that.

Quoted 4 lines

What I don't understand is how clocks could change in hardware. You described implementation, but the clock IDS do not describe implementation but the the mapping to real hardware clocks. So how SOC0_CLK_HPLL_DIV_AHB clock disappeared from hardware?

Sorry, It's not disappeared, it is replaced by implement.

- The previous implementation hardcoded mpll_div_ahb and hpll_div_ahb as the only AHB clock sources:

     mpll -> mpll_div_ahb 
						-> clk_ahb
     hpll -> hpll_div_ahb>

- The new approach introduces SOC0_CLK_AHBMUX, clear clock tree selection of AHB clock sources via a mux:
Than through div_table to get ahb clk.

     mpll ->
       		-> ahb_mux -> div_table -> clk_ahb
     hpll ->

Quoted 5 lines

Quoted 2 lines
- And since i2c-ast2700.c (v8) is not applied, so there should also no one
use this ABI. Am I correct?
If binding was not applied, then what are you changing here?

My point is only i2c-ast2700.c (v8) used, which not be applied in Linux. And i2c-ast2700.c (v9) no need anymore.

Quoted 11 lines

Does it mean header described clock which was in the hardware but its handling was not yet implemented in the driver?
Quoted 2 lines
If this is not acceptable, my next patch will keep
SCU0_CLK_UART_DIV13/SCU0_CLK_HPLL_DIV_AHB define.
Maybe my last message was not clear, so: you need to explain why you are breaking ABI and/or explain the ABI impact in the commit msg.

Understood your concern about this.
I think add new SCU0_CLK_AHBMUX to avoid ABI impact will be the better way.

Thank your guideline.

Quoted 7 lines

Quoted 2 lines
But add new SCU0_CLK_AHBMUX define for avoid your point ABI break. Is
this acceptable?
Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-24 09:55 UTC #10 lore raw

PATCH v9 2/3 reset: aspeed: register AST2700 reset auxiliary bus device

In reply to Ryan Chen

The AST2700 reset driver is registered as an auxiliary device due to reset and clock controller share the same register region.

Signed-off-by Ryan Chen <[email protected]>

Reviewed-by Philipp Zabel <[email protected]>

---
 drivers/reset/Kconfig             |   7 +
 drivers/reset/Makefile            |   1 +
 drivers/reset/reset-aspeed.c      | 302 ++++++++++++++++++++++++++++++
 include/soc/aspeed/reset-aspeed.h |  21 +++
 4 files changed, 331 insertions(+)
 create mode 100644 drivers/reset/reset-aspeed.c
 create mode 100644 include/soc/aspeed/reset-aspeed.h

drivers/reset/Kconfig +7 -0

index 5b3abb6db248..7e1ba795eb23 100644
@@ -22,6 +22,13 @@ config RESET_A10SR
 	  This option enables support for the external reset functions for
 	  peripheral PHYs on the Altera Arria10 System Resource Chip.
 
+config RESET_ASPEED
+	tristate "ASPEED Reset Driver"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	select AUXILIARY_BUS
+	help
+	  This enables the reset controller driver for AST2700.
+
 config RESET_ATH79
 	bool "AR71xx Reset Driver" if COMPILE_TEST
 	default ATH79

drivers/reset/Makefile +1 -0

index 677c4d1e2632..25af31f673a4 100644
@@ -6,6 +6,7 @@ obj-y += starfive/
 obj-y += sti/
 obj-y += tegra/
 obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
+obj-$(CONFIG_RESET_ASPEED) += reset-aspeed.o
 obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
 obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o

drivers/reset/reset-aspeed.c new file +302 -0

302 lines not on this page · raw message

include/soc/aspeed/reset-aspeed.h new file +21 -0

new file mode 100644
index 000000000000..51a4d13fbe3d
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 ASPEED Technology Inc.
+ * Author: Ryan Chen <[email protected]>
+ */
+
+#ifndef __RESET_ASPEED_H__
+#define __RESET_ASPEED_H__
+
+#if IS_ENABLED(CONFIG_RESET_ASPEED)
+int aspeed_reset_controller_register(struct device *clk_dev, void __iomem *base,
+				     const char *adev_name);
+#else
+static inline int aspeed_reset_controller_register(struct device *clk_dev, void __iomem *base,
+				     const char *adev_name)
+{
+	return -ENODEV;
+}
+#endif /* if IS_ENABLED(CONFIG_RESET_ASPEED) */
+
+#endif /* __RESET_ASPEED_H__ */

Signature · 1 line

2.34.1

Krzysztof Kozlowski

2025-02-24 10:12 UTC #11 lore raw

Re: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device

Quoted 6 lines · Ryan Chen

drivers/reset/reset-aspeed.c new file +6 -0

+
+static void aspeed_reset_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);

drivers/reset/reset-aspeed.c line 242

Quoted 10 lines

drivers/reset/reset-aspeed.c new file +10 -0

+	auxiliary_device_uninit(adev);
+}
+
+static void aspeed_reset_adev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+

Every exported function *must* have kerneldoc.

Quoted 22 lines

drivers/reset/reset-aspeed.c new file +22 -0

+int aspeed_reset_controller_register(struct device *clk_dev, void __iomem *base,
+				     const char *adev_name)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = adev_name;
+	adev->dev.parent = clk_dev;
+	adev->dev.release = aspeed_reset_adev_release;
+	adev->id = 666u;
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(adev);

drivers/reset/reset-aspeed.c line 274

Quoted 10 lines

drivers/reset/reset-aspeed.c new file +10 -0

+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	adev->dev.platform_data = (__force void *)base;
+
+	return devm_add_action_or_reset(clk_dev, aspeed_reset_unregister_adev, adev);
+}
+EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);

No, you cannot export functions without users. There is no single user of this, so this is not justified at all.

Best regards,
Krzysztof

Krzysztof Kozlowski

2025-02-24 10:15 UTC #12 lore raw

Re: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device

Quoted 47 lines · Krzysztof Kozlowski

Quoted 6 lines · Ryan Chen

drivers/reset/reset-aspeed.c new file +6 -0

+
+static void aspeed_reset_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);

drivers/reset/reset-aspeed.c line 242

Quoted 10 lines

drivers/reset/reset-aspeed.c new file +10 -0

+	auxiliary_device_uninit(adev);
+}
+
+static void aspeed_reset_adev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+

Every exported function *must* have kerneldoc.

Quoted 27 lines

drivers/reset/reset-aspeed.c new file +27 -0

+int aspeed_reset_controller_register(struct device *clk_dev, void __iomem *base,
+				     const char *adev_name)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = adev_name;
+	adev->dev.parent = clk_dev;
+	adev->dev.release = aspeed_reset_adev_release;
+	adev->id = 666u;
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+

Quoted 8 lines

drivers/reset/reset-aspeed.c line 274
Quoted 5 lines
drivers/reset/reset-aspeed.c new file +5 -0
+	adev->dev.platform_data = (__force void *)base;
+
+	return devm_add_action_or_reset(clk_dev, aspeed_reset_unregister_adev, adev);
+}
+EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
No, you cannot export functions without users. There is no single user of this, so this is not justified at all.

My mistake, I missed patch #3 which uses it.

I don't get why do you need to export this in the first place, instead of putting it in the clock driver, as usually expected. Handling child creation is logically the task of the device having children, the parent. Not the child.

Best regards,
Krzysztof

Philipp Zabel

maintainer

2025-02-24 16:28 UTC #13 lore raw

Re: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device

Quoted 56 lines · Krzysztof Kozlowski

Quoted 47 lines · Krzysztof Kozlowski

Quoted 6 lines · Ryan Chen

drivers/reset/reset-aspeed.c new file +6 -0

+
+static void aspeed_reset_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);

drivers/reset/reset-aspeed.c line 242

Quoted 10 lines

drivers/reset/reset-aspeed.c new file +10 -0

+	auxiliary_device_uninit(adev);
+}
+
+static void aspeed_reset_adev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+

Every exported function *must* have kerneldoc.

Quoted 27 lines

drivers/reset/reset-aspeed.c new file +27 -0

+int aspeed_reset_controller_register(struct device *clk_dev, void __iomem *base,
+				     const char *adev_name)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = adev_name;
+	adev->dev.parent = clk_dev;
+	adev->dev.release = aspeed_reset_adev_release;
+	adev->id = 666u;
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+

Quoted 8 lines

drivers/reset/reset-aspeed.c line 274
Quoted 5 lines
drivers/reset/reset-aspeed.c new file +5 -0
+	adev->dev.platform_data = (__force void *)base;
+
+	return devm_add_action_or_reset(clk_dev, aspeed_reset_unregister_adev, adev);
+}
+EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
No, you cannot export functions without users. There is no single user of this, so this is not justified at all.

Quoted 6 lines

My mistake, I missed patch #3 which uses it.
I don't get why do you need to export this in the first place, instead of putting it in the clock driver, as usually expected. Handling child creation is logically the task of the device having children, the parent. Not the child.

Also, consider basing this on top of:

https://lore.kernel.org/all/[email protected]/

regards
Philipp

Ryan Chen

author reviewer

2025-02-25 02:20 UTC #14 lore raw

RE: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device

Quoted 66 lines

Subject: Re: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device
Quoted 19 lines · Krzysztof Kozlowski
Quoted 18 lines · Krzysztof Kozlowski
Quoted 4 lines · Ryan Chen
drivers/reset/reset-aspeed.c new file +4 -0
+
+static void aspeed_reset_unregister_adev(void *_adev) {
+	struct auxiliary_device *adev = _adev;
+
drivers/reset/reset-aspeed.c line 242
Quoted 10 lines
drivers/reset/reset-aspeed.c new file +10 -0
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+static void aspeed_reset_adev_release(struct device *dev) {
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+
Every exported function *must* have kerneldoc.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+int aspeed_reset_controller_register(struct device *clk_dev, void
__iomem *base,
Quoted 34 lines
Quoted 26 lines
Quoted 26 lines
drivers/reset/reset-aspeed.c new file +26 -0
+				     const char *adev_name)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = adev_name;
+	adev->dev.parent = clk_dev;
+	adev->dev.release = aspeed_reset_adev_release;
+	adev->id = 666u;
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
+	}
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
Quoted 8 lines
drivers/reset/reset-aspeed.c line 274
Quoted 5 lines
drivers/reset/reset-aspeed.c new file +5 -0
+	adev->dev.platform_data = (__force void *)base;
+
+	return devm_add_action_or_reset(clk_dev,
+aspeed_reset_unregister_adev, adev); }
+EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
No, you cannot export functions without users. There is no single user of this, so this is not justified at all.
Quoted 6 lines
My mistake, I missed patch #3 which uses it.
I don't get why do you need to export this in the first place, instead of putting it in the clock driver, as usually expected. Handling child creation is logically the task of the device having children, the parent. Not the child.

Quoted 4 lines

Also, consider basing this on top of:
https://lore.kernel.org/all/20250218-aux-device-create-helper-v4-0-c3d7dfdea
[email protected]/

Hello, Base on this series, I will use adev = devm_auxiliary_device_create instead addev_alloc, adev_releas. But it still have EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
Am I right?

Quoted 2 lines

regards
Philipp

Krzysztof Kozlowski

2025-02-25 07:51 UTC #15 lore raw

Re: [PATCH v9 2/3] reset: aspeed: register AST2700 reset auxiliary bus device

Quoted 14 lines · Ryan Chen

Quoted 14 lines
Quoted 13 lines
Quoted 7 lines
drivers/reset/reset-aspeed.c line 274
Quoted 4 lines
drivers/reset/reset-aspeed.c new file +4 -0
+
+	return devm_add_action_or_reset(clk_dev,
+aspeed_reset_unregister_adev, adev); }
+EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
No, you cannot export functions without users. There is no single user of this, so this is not justified at all.
My mistake, I missed patch #3 which uses it.
I don't get why do you need to export this in the first place, instead of putting it in the clock driver, as usually expected. Handling child creation is logically the task of the device having children, the parent. Not the child.

Quoted 8 lines

Quoted 4 lines
Also, consider basing this on top of:
https://lore.kernel.org/all/20250218-aux-device-create-helper-v4-0-c3d7dfdea
[email protected]/
Hello, Base on this series, I will use adev = devm_auxiliary_device_create instead addev_alloc, adev_releas. But it still have EXPORT_SYMBOL_GPL(aspeed_reset_controller_register);
Am I right?

Why? It looks like you skipped entirely my message. It is the same over and over - respond to people comments...

Best regards,
Krzysztof

Ryan Chen

author reviewer

2025-02-24 09:55 UTC #16 lore raw

PATCH v9 3/3 clk: aspeed: add AST2700 clock driver

In reply to Ryan Chen

Add AST2700 clock controller driver and also use axiliary device framework register the reset controller driver. Due to clock and reset using the same register region.

Signed-off-by Ryan Chen <[email protected]>

---
 drivers/clk/Kconfig       |    8 +
 drivers/clk/Makefile      |    1 +
 drivers/clk/clk-ast2700.c | 1120 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1129 insertions(+)
 create mode 100644 drivers/clk/clk-ast2700.c

drivers/clk/Kconfig +8 -0

index 713573b6c86c..285618c9b5ca 100644
@@ -288,6 +288,14 @@ config COMMON_CLK_ASPEED
 	  The G4 and G5 series, including the ast2400 and ast2500, are supported
 	  by this driver.
 
+config COMMON_CLK_AST2700
+	bool "Clock driver for AST2700 SoC"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	help
+	  This driver provides support for clock on AST2700 SoC.
+	  The driver is responsible for managing the various clocks required
+	  by the peripherals and cores within the AST2700.
+
 config COMMON_CLK_S2MPS11
 	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
 	depends on MFD_SEC_CORE || COMPILE_TEST

drivers/clk/Makefile +1 -0

index bf4bd45adc3a..c084e50bc662 100644
@@ -63,6 +63,7 @@ obj-$(CONFIG_COMMON_CLK_FSL_SAI)	+= clk-fsl-sai.o
 obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
 obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
 obj-$(CONFIG_MACH_ASPEED_G6)		+= clk-ast2600.o
+obj-$(CONFIG_COMMON_CLK_AST2700)	+= clk-ast2700.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
 obj-$(CONFIG_COMMON_CLK_K210)		+= clk-k210.o

drivers/clk/clk-ast2700.c new file +1120 -0

1,120 lines not on this page · raw message

Signature · 1 line

2.34.1

kernel test robot

build bot

2025-03-06 18:02 UTC #17 lore raw

Re: [PATCH v9 3/3] clk: aspeed: add AST2700 clock driver

kernel test robot noticed the following build warnings:

Hi Ryan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.14-rc5 next-20250306]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-binding-clock-ast2700-modify-soc0-1-clock-define/20250224-175830
base:   linus/master
patch link:    https://lore.kernel.org/r/20250224095506.2047064-4-ryan_chen%40aspeedtech.com
patch subject: [PATCH v9 3/3] clk: aspeed: add AST2700 clock driver
config: powerpc64-randconfig-r121-20250306 (https://download.01.org/0day-ci/archive/20250307/[email protected]/config)
compiler: powerpc64-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250307/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

>> drivers/clk/clk-ast2700.c:1066:92: sparse: sparse: Using plain integer as NULL pointer

vim +1066 drivers/clk/clk-ast2700.c

952
953 static int ast2700_soc_clk_probe(struct platform_device *pdev)
954 {
955 const struct ast2700_clk_data *clk_data;
956 struct ast2700_clk_ctrl *clk_ctrl;
957 struct clk_hw_onecell_data *clk_hw_data;
958 struct device *dev = &pdev->dev;
959 void __iomem *clk_base;
960 struct clk_hw **hws;
961 char *reset_name;
962 int ret;
963 int i;
964
965 clk_ctrl = devm_kzalloc(dev, sizeof(*clk_ctrl), GFP_KERNEL);
966 if (!clk_ctrl)
967 return -ENOMEM;
968 clk_ctrl->dev = dev;
969 dev_set_drvdata(&pdev->dev, clk_ctrl);
970
971 spin_lock_init(&clk_ctrl->lock);
972
973 clk_base = devm_platform_ioremap_resource(pdev, 0);
974 if (IS_ERR(clk_base))
975 return PTR_ERR(clk_base);
976
977 clk_ctrl->base = clk_base;
978
979 clk_data = device_get_match_data(dev);
980 if (!clk_data)
981 return -ENODEV;
982
983 clk_ctrl->clk_data = clk_data;
984 reset_name = devm_kasprintf(dev, GFP_KERNEL, "reset%d", clk_data->scu);
985
986 clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws, clk_data->nr_clks),
987 GFP_KERNEL);
988 if (!clk_hw_data)
989 return -ENOMEM;
990
991 clk_hw_data->num = clk_data->nr_clks;
992 hws = clk_hw_data->hws;
993
994 if (clk_data->scu)
995 ast2700_soc1_configure_i3c_clk(clk_ctrl);
996
997 for (i = 0; i < clk_data->nr_clks; i++) {
998 const struct ast2700_clk_info *clk = &clk_data->clk_info[i];
999 void __iomem *reg;
1000
1001 if (clk->type == CLK_FIXED) {
1002 const struct ast2700_clk_fixed_rate_data *fixed_rate = &clk->data.rate;
1003
1004 hws[i] = devm_clk_hw_register_fixed_rate(dev, clk->name, NULL, 0,
1005 fixed_rate->fixed_rate);
1006 } else if (clk->type == CLK_FIXED_FACTOR) {
1007 const struct ast2700_clk_fixed_factor_data *factor = &clk->data.factor;
1008
1009 hws[i] = devm_clk_hw_register_fixed_factor(dev, clk->name,
1010 factor->parent->name,
1011 0, factor->mult, factor->div);
1012 } else if (clk->type == DCLK_FIXED) {
1013 const struct ast2700_clk_pll_data *pll = &clk->data.pll;
1014
1015 reg = clk_ctrl->base + pll->reg;
1016 hws[i] = ast2700_clk_hw_register_dclk(reg, clk->name, clk_ctrl);
1017 } else if (clk->type == CLK_HPLL) {
1018 const struct ast2700_clk_pll_data *pll = &clk->data.pll;
1019
1020 reg = clk_ctrl->base + pll->reg;
1021 hws[i] = ast2700_clk_hw_register_hpll(reg, clk->name,
1022 pll->parent->name, clk_ctrl);
1023 } else if (clk->type == CLK_PLL) {
1024 const struct ast2700_clk_pll_data *pll = &clk->data.pll;
1025
1026 reg = clk_ctrl->base + pll->reg;
1027 hws[i] = ast2700_clk_hw_register_pll(i, reg, clk->name,
1028 pll->parent->name, clk_ctrl);
1029 } else if (clk->type == CLK_UART_PLL) {
1030 const struct ast2700_clk_pll_data *pll = &clk->data.pll;
1031
1032 reg = clk_ctrl->base + pll->reg;
1033 hws[i] = ast2700_clk_hw_register_uartpll(reg, clk->name,
1034 pll->parent->name, clk_ctrl);
1035 } else if (clk->type == CLK_MUX) {
1036 const struct ast2700_clk_mux_data *mux = &clk->data.mux;
1037
1038 reg = clk_ctrl->base + mux->reg;
1039 hws[i] = devm_clk_hw_register_mux_parent_data_table(dev, clk->name,

  1040										    mux->parents,
  1041										    mux->num_parents, 0,
  1042										    reg, mux->bit_shift,
  1043										    mux->bit_width, 0,
  1044										    NULL, &clk_ctrl->lock);
  1045			} else if (clk->type == CLK_MISC) {
  1046				const struct ast2700_clk_pll_data *misc = &clk->data.pll;
  1047	
  1048				reg = clk_ctrl->base + misc->reg;
  1049				hws[i] = ast2700_clk_hw_register_misc(i, reg, clk->name,
  1050								      misc->parent->name, clk_ctrl);
  1051			} else if (clk->type == CLK_DIVIDER) {
  1052				const struct ast2700_clk_div_data *div = &clk->data.div;
  1053	
  1054				reg = clk_ctrl->base + div->reg;
  1055				hws[i] = devm_clk_hw_register_divider_table(dev, clk->name,
  1056									    div->parent->name, 0,
  1057									    reg, div->bit_shift,
  1058									    div->bit_width, 0,
  1059									    div->div_table,
  1060									    &clk_ctrl->lock);
  1061			} else if (clk->type == CLK_GATE_ASPEED) {
  1062				const struct ast2700_clk_gate_data *gate = &clk->data.gate;
  1063	
  1064				reg = clk_ctrl->base + gate->reg;
  1065				hws[i] = ast2700_clk_hw_register_gate(dev, clk->name, gate->parent,
> 1066								      reg, gate->bit, gate->flags, 0);

1067
1068 } else {
1069 const struct ast2700_clk_gate_data *gate = &clk->data.gate;
1070
1071 reg = clk_ctrl->base + gate->reg;
1072 hws[i] = devm_clk_hw_register_gate_parent_data(dev, clk->name, gate->parent,
1073 0, reg, clk->clk_idx, 0,
1074 &clk_ctrl->lock);
1075 }
1076
1077 if (IS_ERR(hws[i]))
1078 return PTR_ERR(hws[i]);
1079 }
1080
1081 ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_hw_data);
1082 if (ret)
1083 return ret;
1084
1085 return aspeed_reset_controller_register(dev, clk_base, reset_name);
1086 }
1087

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki