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

[Jonas Karlman]

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

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