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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/10/2026 00:35, Michal Wilczynski wrote:
> 
> 
> 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.

Which is perfectly "fine" - present in many other SoCs as well. This is
not different and your arguments are insufficient to give here
exceptions. Why? Because again you use as an argument a Linux driver
problem (cyclic dependency). Basically to solve Linux problem you
introduce new empty nodes.

Best regards,
Krzysztof