[PATCH 2/3] rockchip: rk3288: Use rk3288-cru.h from dts/upstream

[Quentin Schulz]

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 at 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 at 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 at 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.

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.

Cheers,
Quentin