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

[Michal Wilczynski]

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.

Split in two there is no cycle: the PHY's only input is xin24m - the 24 MHz
oscillator which gives a linear order - hdmi_phy, voutcrg, hdmi_controller.

> 
> And really, I have no clue what hdmitx0_pixelclk and voutcrg are. I
> could probably study the patches a lot to figure that out, but my review
> queue has still 200 more, so I'll skip.
> 
> But nevertheless assigning clock parent of HDMI clock to PHY is
> standard, most of the platforms have it, thus it is not a justification
> for odd design.

Agreed and that is what this is - I followed RK3328, which uses the same
Innosilicon IP and also describes it as two nodes:

hdmi:    hdmi@ff3c0000   clocks = <&cru PCLK_HDMI>, ...
hdmiphy: phy@ff430000    clocks = <&cru PCLK_HDMIPHY>, <&xin24m>,
                                  <&cru DCLK_HDMIPHY>
                         clock-output-names = "hdmi_phy";
                         #clock-cells = <0>;
                         #phy-cells = <0>;

The only difference is reg: RK3328 has two register ranges, the JH7110 has
one, so the parent owns it and the children have none.

> 
>> functions with different clock inputs and one of them feeds back into
>> the SoC clock tree. The PHY generates hdmitx0_pixelclk which voutcrg
>> consumes the controller consumes pclk/mclk/bclk from voutcrg. Folded
>> into one node that node is both a provider to and a consumer of
>> voutcrg which is a cycle in the hardware description, not just in Linux.
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>