Hi Quentin,
On 2024-11-06 15:35, Quentin Schulz wrote:
Hi Jonas,
On 11/4/24 9:58 PM, Jonas Karlman wrote:
clock/rk3288-cru.h in include/dt-bindings is almost identical to the version in dts/upstream, remove the copy from include/dt-bindings to only use the version from dts/upstream.
One clk, SCLK_MAC_PLL, is not part of the upstream bindings, this clk is not used by any upstream or in-tree DT. For now just ignore this unused clk and only workaround a build issue caused by this removal.
No functional change to board DTs is intended with this removal.
Signed-off-by: Jonas Karlman jonas@kwiboo.se
.../clock/rockchip,rk3288-cru.txt | 61 --- drivers/clk/rockchip/clk_rk3288.c | 2 + include/dt-bindings/clock/rk3288-cru.h | 381 ------------------ 3 files changed, 2 insertions(+), 442 deletions(-) delete mode 100644 doc/device-tree-bindings/clock/rockchip,rk3288-cru.txt delete mode 100644 include/dt-bindings/clock/rk3288-cru.h
diff --git a/doc/device-tree-bindings/clock/rockchip,rk3288-cru.txt b/doc/device-tree-bindings/clock/rockchip,rk3288-cru.txt deleted file mode 100644 index c9fbb76573e1..000000000000 --- a/doc/device-tree-bindings/clock/rockchip,rk3288-cru.txt +++ /dev/null @@ -1,61 +0,0 @@ -* Rockchip RK3288 Clock and Reset Unit
-The RK3288 clock controller generates and supplies clock to various -controllers within the SoC and also implements a reset controller for SoC -peripherals.
-Required Properties:
-- compatible: should be "rockchip,rk3288-cru" -- reg: physical base address of the controller and length of memory mapped
- region.
-- #clock-cells: should be 1. -- #reset-cells: should be 1.
-Optional Properties:
-- rockchip,grf: phandle to the syscon managing the "general register files"
- If missing pll rates are not changable, due to the missing pll lock status.
-Each clock is assigned an identifier and client nodes can use this identifier -to specify the clock which they consume. All available clocks are defined as -preprocessor macros in the dt-bindings/clock/rk3288-cru.h headers and can be -used in device tree sources. Similar macros exist for the reset sources in -these files.
-External clocks:
-There are several clocks that are generated outside the SoC. It is expected -that they are defined using standard clock bindings with following -clock-output-names:
- "xin24m" - crystal input - required,
- "xin32k" - rtc clock - optional,
- "ext_i2s" - external I2S clock - optional,
- "ext_hsadc" - external HSADC clock - optional,
- "ext_edp_24m" - external display port clock - optional,
- "ext_vip" - external VIP clock - optional,
- "ext_isp" - external ISP clock - optional,
- "ext_jtag" - external JTAG clock - optional
-Example: Clock controller node:
- cru: cru@20000000 {
compatible = "rockchip,rk3188-cru";reg = <0x20000000 0x1000>;rockchip,grf = <&grf>;#clock-cells = <1>;#reset-cells = <1>;- };
-Example: UART controller node that consumes the clock generated by the clock
- controller:
- uart0: serial@10124000 {
compatible = "snps,dw-apb-uart";reg = <0x10124000 0x400>;interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;reg-shift = <2>;reg-io-width = <1>;clocks = <&cru SCLK_UART0>;- };
diff --git a/drivers/clk/rockchip/clk_rk3288.c b/drivers/clk/rockchip/clk_rk3288.c index 43c44fadbe7a..69a2a9429ffd 100644 --- a/drivers/clk/rockchip/clk_rk3288.c +++ b/drivers/clk/rockchip/clk_rk3288.c @@ -902,6 +902,7 @@ static int __maybe_unused rk3288_gmac_set_parent(struct clk *clk, struct clk *pa const char *clock_output_name; int ret;
+#ifdef SCLK_MAC_PLL /* * If the requested parent is in the same clock-controller and * the id is SCLK_MAC_PLL ("mac_pll_src"), switch to the internal @@ -912,6 +913,7 @@ static int __maybe_unused rk3288_gmac_set_parent(struct clk *clk, struct clk *pa rk_clrsetreg(&cru->cru_clksel_con[21], RMII_EXTCLK_MASK, 0); return 0; } +#endif
I don't really like this.
Could we simply remove this part of the code in one commit explaining why we remove it (in preparation of syncing the device tree and because there's currently no upstream user of that clock). It would make it much more obvious from simply the commit title that we did something more than simply using the dt-binding upstream header AND also would make it easier to revert if anybody ever needs it.
Will split/add a commit to remove the code for SCLK_MAC_PLL in a v2.
Another option is to upstream that in the Linux kernel first and then we don't have to work around that issue. That may be a bit more difficult if there are currently no actual (known) users of that clock.
Did a grep for SCLK_MAC_PLL in vendor u-boot, linux 4.4/4.19/5.10/6.1 and cannot find any reference of SCLK_MAC_PLL, beside rockchip-dwmac.txt:
- assigned-clocks: main clock, should be <&cru SCLK_MAC>; - assigned-clock-parents = parent of main clock. can be <&ext_gmac> or <&cru SCLK_MAC_PLL>.
Think I prefer to just remove the code from U-Boot :-)
Regards, Jonas
Cheers, Quentin