Re: [PATCH 2/3] clk: rockchip: Add dt-binding header for rk3576

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Heiko,

On Friday, 2 August 2024 10:34:07 EDT Heiko Stübner wrote:
> Hi Detlev,
> 
> Am Freitag, 2. August 2024, 16:12:49 CEST schrieb Detlev Casanova:
> > From: Elaine Zhang <zhangqing@rock-chips.com>
> > 
> > Add the dt-bindings header for the rk3576, that gets shared between
> > the clock controller and the clock references in the dts.
> > 
> > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  .../dt-bindings/clock/rockchip,rk3576-cru.h   | 1149 +++++++++++++++++
> >  1 file changed, 1149 insertions(+)
> >  create mode 100644 include/dt-bindings/clock/rockchip,rk3576-cru.h
> > 
> > diff --git a/include/dt-bindings/clock/rockchip,rk3576-cru.h
> > b/include/dt-bindings/clock/rockchip,rk3576-cru.h new file mode 100644
> > index 0000000000000..19d25f082dc57
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/rockchip,rk3576-cru.h
> > @@ -0,0 +1,1149 @@
> > 
> > +#define CLK_NR_CLKS			(ACLK_KLAD + 1)
> 
> this needs to go please. Take a look at how Sebastian got rid of needed
> that max-constant for rk3588.

Oh indeed, that looks better, I'll port it to using the same functions as in 
rk3588.

I'll also separate clocks and resets as done in rk3588 and improve the listing 
of those resets as in rst-rk3588.c

> [...]
> 
> > +#define SRST_H_VEPU1			1267
> > +#define SRST_A_VEPU1			1268
> > +#define SRST_VEPU1_CORE			1269
> > +
> > +/********Name=PHPPHYSOFTRST_CON00,Offset=0x8A00********/
> > +#define SRST_P_PHPPHY_CRU		131073
> > +#define SRST_P_APB2ASB_SLV_CHIP_TOP	131075
> > +#define SRST_P_PCIE2_COMBOPHY0		131077
> > +#define SRST_P_PCIE2_COMBOPHY0_GRF	131078
> > +#define SRST_P_PCIE2_COMBOPHY1		131079
> > +#define SRST_P_PCIE2_COMBOPHY1_GRF	131080
> 
> this seems to lump together different components and with that creates
> these gaps. I.e. I really don't think the phpphy in these registers is part
> of the core CRU.
> 
> That huge memory length of 0x5c000 in your dt-binding is also a good
> indicator that this needs to have more separation and not span multiple
> devices.

It is not really clear if those are different devices, but they can possibly be 
seen as different instances of the same device. I'll just remove those extra 
resets for now and add them with the correct device when support is 
implemented.

> > +/********Name=PHPPHYSOFTRST_CON01,Offset=0x8A04********/
> > +#define SRST_PCIE0_PIPE_PHY		131093
> > +#define SRST_PCIE1_PIPE_PHY		131096
> > +
> > +/********Name=SECURENSSOFTRST_CON00,Offset=0x10A00********/
> > +#define SRST_H_CRYPTO_NS		262147
> > +#define SRST_H_TRNG_NS			262148
> > +#define SRST_P_OTPC_NS			262152
> > +#define SRST_OTPC_NS			262153
> > +
> > +/********Name=PMU1SOFTRST_CON00,Offset=0x20A00********/
> > +#define SRST_P_HDPTX_GRF		524288
> 
> same here, that is also most likely not part of the CRU but a different
> block. Other socs already implement separate clock controllers for
> different parts, so please take a look there.

Let's add those resets when the device they are linked to is actually 
supported then.

> Thanks
> Heiko

Regards,
Detlev.