RE: [PATCH v2 01/14] dt-bindings: phy: Sierra: Add bindings for Sierra in TI's J721E

[Anil Joy Varughese]

Hi Kishon/Rob

My comments are below.

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@xxxxxx>
> Sent: Wednesday, October 30, 2019 11:06 AM
> To: Rob Herring <robh@xxxxxxxxxx>; Anil Joy Varughese
> <aniljoy@xxxxxxxxxxx>
> Cc: Roger Quadros <rogerq@xxxxxx>; Jyri Sarha <jsarha@xxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 01/14] dt-bindings: phy: Sierra: Add bindings for Sierra
> in TI's J721E
> 
> EXTERNAL MAIL
> 
> 
> Hi Rob,
> 
> On 30/10/19 12:29 AM, Rob Herring wrote:
> > On Wed, Oct 23, 2019 at 06:27:22PM +0530, Kishon Vijay Abraham I wrote:
> >> Add DT binding documentation for Sierra PHY IP used in TI's J721E
> >> SoC.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
> >> ---
> >>  .../devicetree/bindings/phy/phy-cadence-sierra.txt  | 13
> >> ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt
> >> b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt
> >> index 6e1b47bfce43..bf90ef7e005e 100644
> >> --- a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt
> >> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt
> >> @@ -2,21 +2,24 @@ Cadence Sierra PHY
> >>  -----------------------
> >>
> >>  Required properties:
> >> -- compatible:	cdns,sierra-phy-t0
> >> -- clocks:	Must contain an entry in clock-names.
> >> -		See ../clocks/clock-bindings.txt for details.
> >> -- clock-names:	Must be "phy_clk"
> >> +- compatible:	Must be "cdns,sierra-phy-t0" for Sierra in Cadence
> platform
> >> +		Must be "ti,sierra-phy-t0" for Sierra in TI's J721E SoC.
> >>  - resets:	Must contain an entry for each in reset-names.
> >>  		See ../reset/reset.txt for details.
> >>  - reset-names:	Must include "sierra_reset" and "sierra_apb".
> >>  		"sierra_reset" must control the reset line to the PHY.
> >>  		"sierra_apb" must control the reset line to the APB PHY
> >> -		interface.
> >> +		interface ("sierra_apb" is optional).
> >>  - reg:		register range for the PHY.
> >>  - #address-cells: Must be 1
> >>  - #size-cells:	Must be 0
> >>
> >>  Optional properties:
> >> +- clocks:		Must contain an entry in clock-names.
> >> +			See ../clocks/clock-bindings.txt for details.
> >> +- clock-names:		Must be "phy_clk". Must contain "cmn_refclk"
> and
> >> +			"cmn_refclk1" for configuring the frequency of the
> >> +			clock to the lanes.
> >
> > I don't understand how the same block can have completely different
> > clocks. Did the original binding forget some?
> >
> > TI needs 0, 1 or 3 clocks? Reads like it could be any.
> 
> For TI, phy_clk is not needed. Anil, can you clarify what this clock actually
> corresponds to? Is it a functional clock of PHY?

When we had designed the DT binding for Sierra we thought of using phy_clk as a common interface for the clock inputs and there was no specific requirement for splitting it into multiple clocks then and also we had used a simulation environment for testing our IP. We can deprecate the phy_clk property. 

Thanks,
Anil

> Sierra SERDES actually has a number of clocks which can be configured. The
> initial dt-binding didn't model all these clocks. The "cmn_refclk" and
> "cmn_refclk1" are used to program the dividers withing the Sierra. The actual
> registers for programming the dividers are in the Sierra wrapper though. The
> original Sierra driver and dt-binding didn't try to change the default divider
> values.
> 
> Thanks
> Kishon
> >
> > Rob
> >