Re: [PATCH v4 01/20] dt-bindings: phy: Add starfive,jh7110-inno-hdmi-phy

[Icenowy Zheng]

在 2026-10-04日的 00:35 +0200，Michal Wilczynski写道：
> 
> 
> On 10/3/26 22:45, Krzysztof Kozlowski wrote:
> > On 03/10/2026 17:36, Michal Wilczynski wrote:
> > > 
> > > 
> > > On 9/30/26 13:01, Krzysztof Kozlowski wrote:
> > > > On 25/09/2026 23:05, Michal Wilczynski wrote:
> > > > > > > +  clocks:
> > > > > > > +    maxItems: 1
> > > > > > > +    description: Reference oscillator.
> > > > > > 
> > > > > > This barely counts as a resource, so usual question: no
> > > > > > resources here?
> > > > > > no MMIO? Even the user of this phy is the block itself.
> > > > > > 
> > > > > > This makes me wonder if this should be a device node in the
> > > > > > first place
> > > > > > (instead folded into the parent).
> > > > > 
> > > > > The PHY has no reg because the reg is shared with the
> > > > > controller and
> > > > > owned by the parent - patch 9 lets the bridge take its regmap
> > > > > from
> > > > > there.
> > > > > 
> > > > > The user of the PHY is not only the block itself. It is the
> > > > > pixel clock
> > > > > provider for the whole display subsystem, voutcrg takes
> > > > > hdmitx0_pixelclk
> > > > > as the parent of its DC8200 pixel MUXes, and while HDMI
> > > > > output is active
> > > > > it is the only intended source for that clock. The parent has
> > > > > to be
> > > > > assigned explicitly so the general PLL does not end up
> > > > > driving the pixel
> > > > > clock, and so a DSI user does not reach the HDMI PHY clock
> > > > > generator.
> > > > > 
> > > > > So it has to be its own node. The HDMI block has two
> > > > > independent
> > > > 
> > > > I do not see the logic which lead to this conclusion. Pixel
> > > > clock
> > > > provider, so a clock controller, cannot be a user of a phy.
> > > > Clock
> > > > controller does not have a physical layer.
> > > 
> > > Sorry I think the wording was not perfect. I meant the PHY node
> > > is a
> > > pixel clock provider. voutcrg consumes a clock not a PHY.
> > > 
> > > voutcrg: clock-controller@295c0000 {
> > >          clocks = <&syscrg ...>, <&hdmi_phy>;
> > >          clock-names = ...,"hdmitx0_pixelclk";
> > > };
> > > 
> > > The consumer of the PHY is only the hdmi controller.
> > > 
> > > What matters for the node layout is where that clock goes.
> > > voutcrg is the
> > > SoC display clock controller - it is not part of the HDMI block:
> > > 
> > > hdmi_phy -- pixel clock -> voutcrg
> > >                               |
> > >                               - pclk/mclk/bclk -> hdmi_controller
> > >                               - pix0/pix1 -> dc8200
> > > 
> > > hdmi_phy and hdmi_controller are the same register block with one
> > > reg
> > > owned by the parent. So if that block is described as a single
> > > node:
> > > 
> > > hdmi_node - pixel clock -> voutcrg
> > >      ^                          |
> > >      --- pclk/mclk/bclk --------
> > > 
> > > the node provides a clock to voutcrg and consumes three clocks
> > > from
> > > voutcrg. That is a cycle in the device tree description.
> > 
> > There is no cycle. Internal signals to the block are not
> > represented in DT.
> > 
> > What you have is a driver problem and you create some sort of DT
> > structure to solve that. DT purpose is NOT to solve your driver
> > dependencies or circular connections.
> 
> The structure is not something I added. The voutcrg binding has
> required
> an input clock named hdmitx0_pixelclk since it was merged in 2023
> a097a5ec14df ("dt-bindings: clock: Add StarFive JH7110 Video-Output
> clock and reset generator") and this series does not touch that file:
> 
> Documentation/devicetree/bindings/clock/starfive,jh7110-voutcrg.yaml
>        - const: hdmitx0_pixelclk
> 
> Mainline satisfies it with a placeholder, because so far nothing
> provided the
> real clock:
> 
> hdmitx0_pixelclk: hdmitx0-pixel-clock {
>             compatible = "fixed-
> clock";                                                              
>             
>             clock-output-names = "hdmitx0_pixelclk";
>             #clock-cells = <0>;
> };
> 
> All this series does is point that existing input at the device that
> actually
> generates it.
> 
> TRM [1] table 5-3  on page 529 lists the display CRG's input clocks.
> Most come from
> dom_vout_top, but three come from IP blocks inside the subsystem:
> 
> clk_hdmitx0_pixelclk        297 MHz   u0_hdmi_tx.clk_pix
> clk_mipitx_dphy_rxesc        10 MHz   u0_mipitx_dphy.clk_rxesc
> clk_mipitx_dphy_txbytehs    297 MHz   u0_mipitx_dphy.clk_txbytehs
> 
> and on page 530 the CRG offers clk_hdmitx0_pixelclk as Source1 for:
> 
> u0_dc8200.clk_pix0      Source0 clk_dc8200_pix0  Source1
> clk_hdmitx0_pixelclk
> u0_dc8200.clk_pix1      Source0 clk_dc8200_pix0  Source1
> clk_hdmitx0_pixelclk
> u0_cdns_dsiTx.clk_dpi   Source0 clk_dc8200_pix0  Source1
> clk_hdmitx0_pixelclk
> 
> The DC8200 and the DSI TX are separate devices, so the pixel clock
> crosses two block boundaries to reach them.
> 
> That is also why the DT has to be able to name it. Those MUXes have
> two
> parents and the selection is made in DT:
> 
> &dc8200 {
>           assigned-clocks = <&voutcrg JH7110_VOUTCLK_DC8200_PIX0>,
> ...;
>           assigned-clock-parents = <&hdmi_phy>, <&hdmi_phy>;
> };
> 
> and voutcrg takes it as an input clock because that is what the
> hardware
> does. If the PHY is folded into the HDMI node, that input phandle
> points at
> a node which also consumes pclk/mclk/bclk from voutcrg. That is where
> the
> cycle comes from - the hardware topology not the driver.

I think the common clock framework has a "orphaned clock" feature to
solve this kind of cyclic clock dependency, although this could be a
pandora's box if not properly used.

Thanks,
Icenowy

> 
> [1] -
> https://doc-en.rvspace.org/JH7110/PDF/JH7110_TRM_StarFive_Preliminary_V2.pdf
> 
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> Best regards,