[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 at kernel.org>
> Sent: Tuesday, December 19, 2023 2:39 AM
> To: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>; Swapnil Kashinath
> Jakhade <sjakhade at cadence.com>; vkoul at kernel.org; kishon at kernel.org;
> robh+dt at kernel.org; krzysztof.kozlowski+dt at linaro.org; conor+dt at kernel.org;
> linux-phy at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org
> Cc: Milind Parab <mparab at cadence.com>; s-vadapalli at ti.com
> 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 at linaro.org>
> >>> Sent: Wednesday, December 13, 2023 12:19 PM
> >>> To: Swapnil Kashinath Jakhade <sjakhade at cadence.com>;
> vkoul at kernel.org;
> >>> kishon at kernel.org; robh+dt at kernel.org;
> krzysztof.kozlowski+dt at linaro.org;
> >>> conor+dt at kernel.org; linux-phy at lists.infradead.org; linux-
> >>> kernel at vger.kernel.org; devicetree at vger.kernel.org
> >>> Cc: Milind Parab <mparab at cadence.com>; rogerq at kernel.org; s-
> >>> vadapalli at ti.com
> >>> 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 at cadence.com>
> >>>> ---
> >>>>  .../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