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@xxxxxxxxxxxxxx/ 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
