Re: [PATCH v7 3/3] riscv: dts: thead: Add device tree VO clock controller

[Drew Fustini <drew@pdp7.com>]

On Wed, May 07, 2025 at 12:04:01PM +0200, Michal Wilczynski wrote:
> 
> 
> On 5/6/25 23:30, Stephen Boyd wrote:
> > Quoting Michal Wilczynski (2025-04-30 00:52:29)
> >>
> >> In the v2 version of the patchset, there was no reset controller yet, so
> >> I thought your comment was made referring to that earlier version.
> >> This representation clearly describes the hardware correctly, which is
> >> the requirement for the Device Tree.
> >>
> >> The manual, in section 5.4.1.6 VO_SUBSYS, describes the reset registers
> >> starting at 0xFF_EF52_8000:
> >>
> >> GPU_RST_CFG             0x00
> >> DPU_RST_CFG             0x04
> >> MIPI_DSI0_RST_CFG       0x8
> >> MIPI_DSI1_RST_CFG       0xc
> >> HDMI_RST_CFG            0x14
> >> AXI4_VO_DW_AXI          0x18
> >> X2H_X4_VOSYS_DW_AXI_X2H 0x20
> >>
> >> And the clock registers for VO_SUBSYS, manual section 4.4.1.6 start at offset 0x50:
> >> VOSYS_CLK_GATE          0x50
> >> VOSYS_CLK_GATE1         0x54
> >> VOSYS_DPU_CCLK_CFG0     0x64
> >> TEST_CLK_FREQ_STAT      0xc4
> >> TEST_CLK_CFG            0xc8
> >>
> >> So I considered this back then and thought it was appropriate to divide
> >> it into two nodes, as the reset node wasn't being considered at that
> >> time.
> >>
> >> When looking for the reference [1], I didn't notice if you corrected
> >> yourself later, but I do remember considering the single-node approach
> >> at the time.
> >>
> > 
> > If the two register ranges don't overlap then this is probably OK. I
> > imagine this is one device shipped by the hardware engineer, VO_SUBSYS,
> > which happens to be a clock and reset controller. This is quite common,
> > and we usually have one node with both #clock-cells and #reset-cells in
> > it. Then we use the auxiliary bus to create the reset device from the
> > clk driver with the same node. This helps match the device in the
> > datasheet to the node and compatible in DT without making the compatible
> > provider specific (clk or reset postfix).
> > 
> > That's another reason why we usually have one node. DT doesn't describe
> > software, i.e. the split between clk and reset frameworks may not exist
> > in other operating systems. We don't want to put the software design
> > decisions into the DT.
> > 
> > It may also be that a device like this consumes shared power resources
> > like clks or regulators that need to be enabled to drive the device, or
> > an IOMMU is used to translate the register mappings. We wouldn't want to
> > split the device in DT in that case so we can easily manage the power
> > resources or memory mappings for the device.
> > 
> > TL;DR: This is probably OK, but I'd be careful to not make it a thing.
> 
> Thank you very much for the comprehensive explanation. Because the
> registers don’t overlap, it’s fine in this case. Since Drew also seem to
> agree, we can probably push these patches forward, while keeping in mind
> that for future SoCs it would be better to use a single node.

Yes, I think in this instance it makes sense to go ahead. I sent a pull
request [1] to Stephen for my thead clk for-next tree with this series
applied.

Thanks,
Drew

[1] https://lore.kernel.org/all/aBus+Yc7kf%2FH2HE5@x1/