[PATCH 1/5] dt-binding: clock: Document rockchip,rk3588-cru bindings

[Krzysztof Kozlowski]

On 27/06/2022 08:14, Heiko Stübner wrote:
> Hi Krzysztof,
> 
> Am Sonntag, 26. Juni 2022, 22:27:41 CEST schrieb Krzysztof Kozlowski:
>>> +#define PLL_V0PLL			4
>>> +#define PLL_AUPLL			5
>>> +#define PLL_CPLL			6
>>> +#define PLL_GPLL			7
>>> +#define PLL_NPLL			8
>>> +#define PLL_PPLL			9
>>> +#define ARMCLK_L			10
>>> +#define ARMCLK_B01			11
>>> +#define ARMCLK_B23			12
>>> +
>>> +/* cru clocks */
>>> +#define PCLK_BIGCORE0_ROOT		20
>>
>> These are abstract IDs, not register offsets, so no holes, incremented
>> by one.
> 
> I do believe Rockchip nowadays creates these automatically from soc design-
> documents. I've looked up the thread in [0] as this seems to have started
> with the rk3568.
> 
> So these are in fact not created as abstract IDs, but are part of the SoCs
> manual.
> 
> [0] https://lore.kernel.org/all/b663994d-853b-4474-bd77-a444317bfffb@rock-chips.com/

Nothing stops Rockchip to change the tools to generate incremental IDs
without holes, right?

> 
> 
>>> +#define PCLK_BIGCORE0_PVTM		21
>>> +#define PCLK_BIGCORE1_ROOT		22
>>> +#define PCLK_BIGCORE1_PVTM		23
> 
> [...]
> 
>>> +
>>> +#define CLK_NR_CLKS			(HCLK_SDIO_PRE + 1)
>>> +
>>> +/********Name=SOFTRST_CON01,Offset=0xA04********/
>>> +#define SRST_A_TOP_BIU			19
>>
>> What are all these? Bindings should not store register values or offsets.
>>
>> Also, resets go to separate header.
> 
> I think the comments are misleading, these are not register offsets.
> 
> Which in turn is a set of registers SOFTRST_CON0, etc containing the
> bits which to toggle to soft-reset individual blocks of the SoC.

So these are IDs used by the driver, not hardware? Then they can be as
IDs as well, don't they?

> The CRU (clock-and-reset-unit) always also contains the softreset block,
> so they have always been part of the cru dt binding as well.

Separate header under reset would still be part of the CRU DT binding -
I did not propose change that. However why storing reset IDs in a clock
subsystem binding header, instead of reset subsystem?

> 
> 
>>> +#define SRST_P_TOP_BIU			20
>>> +#define SRST_P_CSIPHY0			22
>>> +#define SRST_CSIPHY0			23
>>> +#define SRST_P_CSIPHY1			24
>>> +#define SRST_CSIPHY1			25
>>> +#define SRST_A_TOP_M500_BIU		31
>>
>> No holes, but abstract IDs incremented from 0 or 1.
> 
> The IDs are not abstract but instead do describe the location of
> the reset bit inside the soft-reset register block.
> 
> For reference see drivers/clk/rockchip/softrst.c and its
> 
> 	int bank = id / softrst->num_per_reg;
> 	int offset = id % softrst->num_per_reg;

So these are register offsets, as I mentioned before. Bindings are not
for this, this is not the purpose of binding headers. You do not store
as binding headers GPIO numbers, IRQ numbers, block unit addresses,
right? There is only one case such offsets make sense - firmware also
uses them and you cannot change it.

This is not the case here, so these values should not be in bindings.

To clarify - I don't ask you to change the driver, you can still code
offset like that. Just don't store this as binding.

> 
> And as we're doing this since 2013 this way, including these bindings,
> I guess it can't be too wrong :-)

I guess no one raised the question...

> 
> And they're probably also done via tooling.
> 
> 
> 
>>> +/********Name=PHPTOPSOFTRST_CON0,Offset=0x8A00********/
>>> +#define SRST_P_PHPTOP_CRU		131073
>>> +#define SRST_P_PCIE2_GRF0		131074
>>> +#define SRST_P_PCIE2_GRF1		131075
>>> +#define SRST_P_PCIE2_GRF2		131076
>>> +#define SRST_P_PCIE2_PHY0		131077
>>> +#define SRST_P_PCIE2_PHY1		131078
>>> +#define SRST_P_PCIE2_PHY2		131079
>>> +#define SRST_P_PCIE3_PHY		131080
>>> +#define SRST_P_APB2ASB_SLV_CHIP_TOP	131081
>>> +#define SRST_PCIE30_PHY			131082
>>> +
>>> +/********Name=PMU1SOFTRST_CON00,Offset=0x30A00********/
>>> +#define SRST_H_PMU1_BIU			786442
>>> +#define SRST_P_PMU1_BIU			786443
>>
>>
>> The numbering is getting quite unusual... As the value is not used by
>> the driver, it suggests it is some register offset or value, which are
>> not suitable for the bindings.
> 
> see above, it is used by the driver. Though it's still very much unusual.
> 
> Looking at the register offsets mentioned in the comments, the
> main block handling softreset-ids starts at 0xA04 in the clock controller.
> 
> And historically the soft-reset block has been a compact set of registers
> inside the device, though this time it seems someone tacked some more
> registers into the CRU far behind everything else (0x8A00 and 0x30a00).
> 
> So the IDs are in-line with the how we handle reset-ids currently, but
> I'm somewhat undecided if this counts as more of a hack.

These are not IDs but register offsets. You do not use them in a meaning
of ID. ID is a abstract number providing a mapping between this abstract
number and actual value. You do not have mapping here - you directly
decode this ID.

Best regards,
Krzysztof