RE: [PATCH v2 1/5] dt-bindings: phy: cadence-torrent: Add optional input reference clock for PLL1

[Swapnil Kashinath Jakhade]

> -----Original Message-----
> From: Roger Quadros <rogerq@xxxxxxxxxx>
> Sent: Tuesday, December 19, 2023 2:39 AM
> To: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>; Swapnil Kashinath
> Jakhade <sjakhade@xxxxxxxxxxx>; vkoul@xxxxxxxxxx; kishon@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> linux-phy@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: Milind Parab <mparab@xxxxxxxxxxx>; s-vadapalli@xxxxxx
> Subject: Re: [PATCH v2 1/5] dt-bindings: phy: cadence-torrent: Add optional
> input reference clock for PLL1
> 
> EXTERNAL MAIL
> 
> 
> 
> On 14/12/2023 09:22, Krzysztof Kozlowski wrote:
> > On 14/12/2023 08:02, Swapnil Kashinath Jakhade wrote:
> >> Hi Krzysztof,
> >>
> >>> -----Original Message-----
> >>> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >>> Sent: Wednesday, December 13, 2023 12:19 PM
> >>> To: Swapnil Kashinath Jakhade <sjakhade@xxxxxxxxxxx>;
> vkoul@xxxxxxxxxx;
> >>> kishon@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx;
> >>> conor+dt@xxxxxxxxxx; linux-phy@xxxxxxxxxxxxxxxxxxx; linux-
> >>> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> >>> Cc: Milind Parab <mparab@xxxxxxxxxxx>; rogerq@xxxxxxxxxx; s-
> >>> vadapalli@xxxxxx
> >>> Subject: Re: [PATCH v2 1/5] dt-bindings: phy: cadence-torrent: Add optional
> >>> input reference clock for PLL1
> >>>
> >>> EXTERNAL MAIL
> >>>
> >>>
> >>> On 12/12/2023 12:48, Swapnil Jakhade wrote:
> >>>> Torrent PHY can have two input reference clocks. Update bindings
> >>>
> >>> It already supports two.
> >>>
> >>
> >> Thanks for your comments.
> >> refclk and pll1_refclk are the two input reference clocks for the PLLs.
> >> phy_en_refclk is used to enable output reference clock in some cases.
> >
> > Why input clock is used to enable output reference clock?
> 
> Looking at the driver code, "phy_en_refclk" is used at 2 places only to
> set the parent of 2 clocks apparently called "received reference clock"
> and "derived reference clock", either of which can be used to enable an
> optional reference clock output feature.
> 
> My understanding is that it is a separate refclk input than 'refclk' (pll0)
> or 'pll1_refclk' which can be used to enable a reference clock output feature.
> 
> So this PHY can support a total of 3 input reference clocks:
> "refclk" is PLL0 reference clock and is required on all platforms
> "pll1_refclk" is PLL1 reference clock and is required on some platforms
> "phy_en_refclk" is reference for output reference clock generator and is
> optional on all platforms
> 
> Swapnil, can you please confirm my understanding? Thanks!
> 

Yes. This is correct. The two input reference clocks mentioned in commit
message refers to refclk and pll1_refclk.

Thanks & regards,
Swapnil
> >
> >>
> >>>> to support dual reference clock multilink configurations.
> >>>>
> >>>> Signed-off-by: Swapnil Jakhade <sjakhade@xxxxxxxxxxx>
> >>>> ---
> >>>>  .../devicetree/bindings/phy/phy-cadence-torrent.yaml        | 6 +++---
> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-
> >>> torrent.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-
> >>> torrent.yaml
> >>>> index dfb31314face..98946f549895 100644
> >>>> --- a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> >>>> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> >>>> @@ -35,14 +35,14 @@ properties:
> >>>>      minItems: 1
> >>>>      maxItems: 2
> >>>>      description:
> >>>> -      PHY reference clock for 1 item. Must contain an entry in clock-names.
> >>>> -      Optional Parent to enable output reference clock.
> >>>> +      PHY input reference clocks - refclk & pll1_refclk (optional).
> >>>> +      Optional Parent to enable output reference clock (phy_en_refclk).
> >>>
> >>> So third clock? But you allow only two? Confusing.
> >>>
> >>
> >> Yes, if both refclk and pll1_refclk are present, phy_en_refclk can't be used.
> >>
> >>>>
> >>>>    clock-names:
> >>>>      minItems: 1
> >>>>      items:
> >>>>        - const: refclk
> >>>> -      - const: phy_en_refclk
> >>>> +      - enum: [ pll1_refclk, phy_en_refclk ]
> >>>
> >>> This does not match your commit msg. You already had two clocks there.
> >>>
> >> Yes, but refclk was the single input reference clock used for PLLs earlier.
> >> As stated in commit message, a new input reference clock (pll1_refclk) is
> added here.
> >
> > existing phy_en_refclk is also input reference clock, isn't it?
> >
> >>
> >
> > Best regards,
> > Krzysztof
> >
> 
> --
> cheers,
> -roger