Re: [PATCH v4 net-next 1/2] dt-bindings: phy: cadence-torrent: Update property values to support 3 clocks

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/07/2026 09:35, Gokul Praveen wrote:
> Hi Krzystof,
> 
> On 02/07/26 11:53, Krzysztof Kozlowski wrote:
>> On Wed, Jul 01, 2026 at 07:54:56PM +0530, Gokul Praveen wrote:
>>> Update maxItems value of "clocks" property to 3 as description of
>>> this parameter already indicates 3 clocks(refclk,pll1_refclk(optional)
>>> and phy_en_refclk(optional)).
>> But what if description is wrong? You need to provide rationale why you
>> are doing it and you cannot use existing code alone as that rationale,
>> because as you pointed out - existing code is not fully correct.
> 
> The description is correct because not all device may have 2 input  

I do not see how you proved it in the commit msg.

> reference clocks , hence keeping the requirement of the 2nd reference 
> clock(pll1_refclk) optional.
> 
> Just as a note: phy_en_refclk is an output clock.

output clocks do not go to input clocks property.

> 
> In those cases the multilink serdes configurations requiring 2 different 
> input reference clocks will not work due to the limitation of having 
> only 1 clock.
> 
> However, when it comes to devices where 2 different input reference 
> clocks are supported and a multilink serdes configuration is 
> needed(where the links require separate reference clocks for each 
> protocol so as to cater to the  different clocking speed requirements of 
> these links).
> 
> Hence, in this case ,2 different input clocks are needed so as to cater 
> to 2 different clock speeds.
> 
> For eg: In the USXGMII+SGMII multilink serdes configuration which I had 
> tested, it failed because
> 
> USXGMII requires an input clock speed of 156.25 Mhz and SGMII protocol 
> requires an input clock speed of 100 Mhz.
> 
> But, since there was only one input clock(refclk) mentioned in the 
> clocks and clock-name parameter , this multilink serdes configuration 
> failed.
> 
> Hence, to make it work, the pll1_refclk had to be added which provided a 
> clock speed of 156.25 Mhz for USXGMI and the refclk provided
> 
> a clock speed of 100 Mhz  for SGMII.
> 
>>> Update the maxItems and items value of "clock-names" property with multiple
>>> combination of clock-names possible since pll1_refclk and phy_en_refclk are
>>> optional clocks.
>> Why? You need to describe why you are doing this, not what you are
>> doing.
> Sure , Krzysztof, I will be careful about that and prioritize that in 
> the commit message.
>>> Signed-off-by: Gokul Praveen <g-praveen@ti.com>
>>> ---
>>>   .../bindings/phy/phy-cadence-torrent.yaml        | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
>>> index 9af39b33646a..96c664d50629 100644
>>> --- a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
>>> @@ -34,7 +34,7 @@ properties:
>>>   
>>>     clocks:
>>>       minItems: 1
>>> -    maxItems: 2
>>> +    maxItems: 3
>>>       description:
>>>         PHY input reference clocks - refclk (for PLL0) & pll1_refclk (for PLL1).
>>>         pll1_refclk is optional and used for multi-protocol configurations requiring
>>> @@ -45,9 +45,17 @@ properties:
>>>   
>>>     clock-names:
>>>       minItems: 1
>>> -    items:
>>> -      - const: refclk
>>> -      - enum: [ pll1_refclk, phy_en_refclk ]
>>> +    maxItems: 3
>> Drop
> Sure, i will do that Krzysztof.
>>> +    oneOf:
>>> +      - items:
>>> +          - const: refclk
>>> +      - items:
>>> +          - const: refclk
>>> +          - enum: [ pll1_refclk, phy_en_refclk ]
>> Drop these, pointless. You were supposed to grow existing syntax.
>>
>>> +      - items:
>>> +          - const: refclk
>>> +          - const: pll1_refclk
>> So here is the enum.
>>
>>> +          - const: phy_en_refclk
>> And this stays.
>>
>> You make changes which do not make the binding better and are not
>> explained in commit msg. Focus on WHY you are doing things and also
>> explain WHY you did such complicated syntax (if you insist on rewriting
>> correct code into something odd we do not expect).
> 
> So, the reason I added the oneOf property is to support the following 
> combinations because pll1_refclk and phy_en_refclk are optional clocks. 
> With the earlier enum , only either of pll1_refclk or phy_en_refclk
> 
> can be used and both cannot be used at the same time.
> 
> Combination 1: refclk
> 
> Combination 2 : refclk, pll1_refclk
> 
> Combination 3: reclk, phy_en_refclk
> 
> Combination 4: refclk, pll1_refclk, phy_en_refclk
> 
> 
> Please feel free to suggest any alternative solution to support these 
> combinations .

I already did. Read the feedback carefully.

Best regards,
Krzysztof