Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Anurag Kumar Vulisha <anuragku@xilinx.com>
To:     Rob Herring <robh@kernel.org>
CC:     "kishon@ti.com" <kishon@ti.com>, Michal Simek <michals@xilinx.com>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "vivek.gautam@codeaurora.org" <vivek.gautam@codeaurora.org>,
        "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: RE: [PATCH v3 2/2] phy: zynqmp: Add dt bindings for ZynqMP phy
Thread-Topic: [PATCH v3 2/2] phy: zynqmp: Add dt bindings for ZynqMP phy
Thread-Index: AQHURTd6gQs0hQ3Au06kiXaejJde5qTp/+6AgACiMUA=
Date:   Tue, 11 Sep 2018 08:07:09 +0000
Message-ID: <BN3PR0201MB09934721CEEE6E9F3316644CA7040@BN3PR0201MB0993.namprd02.prod.outlook.com>
References: <1536165747-6405-1-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <1536165747-6405-3-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <20180910203059.GA7055@bogus>
In-Reply-To: <20180910203059.GA7055@bogus>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e99e1772-18b7-410b-4f43-08d617bd928e
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Sep 2018 08:07:09.2610
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR0201MB0884
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Rob,

Thanks a lot for reviewing this code, please find my comments inline

>> +This binding describes a ZynqMP PHY device that is used to control
>> +ZynqMP High Speed Gigabit Transceiver(GT). ZynqMP PS GTR provides
>> +four lanes and are used by USB, SATA, PCIE, Display port and Ethernet S=
GMMI
>controllers.
>> +
>> +Phy provider node
>> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> +
>> +Required properties:
>> +- compatible	: Can be "xlnx,zynqmp-psgtr-v1.1" or "xlnx,zynqmp-psgtr"
>> +
>> +- reg           : Address and length of register sets for each device i=
n
>> +                  "reg-names"
>> +
>> +- reg-names     : The names of the register addresses corresponding to =
the
>> +		  registers filled in "reg":
>> +			- serdes: SERDES block register set
>> +			- siou: SIOU block register set
>> +
>> +Optional properties:
>> +- xlnx,tx_termination_fix : Include this for fixing functional issue
>> +with the
>
>s/_/-/
>

Will fix this in next series of the patch

>> +		  TX termination resistance in GT, which can be out of spec for
>> +		  the XCZU9EG silicon version. This property is not required for
>> +		  "xlnx,zynqmp-psgtr-v1.1"
>> +
>> +Required nodes	:  A sub-node is required for each lane the controller
>> +		   provides.
>> +
>> +Phy sub-nodes
>> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> +
>> +Required properties:
>> +lane0:
>
>lane0 or lane@0? If you need or care about numbering then add a reg proper=
ty.
>

Thanks for correcting,  Will change this to lane@0 and resend the patches

>> +- #phy-cells	: Should be 4
>> +
>> +lane1:
>> +- #phy-cells	: Should be 4
>> +
>> +lane2:
>> +- #phy-cells	: Should be 4
>> +
>> +lane3:
>> +- #phy-cells	: Should be 4
>> +
>> +Example:
>> +	zynqmp_phy: phy@fd400000 {
>> +		compatible =3D "xlnx,zynqmp-psgtr-v1.1";
>> +		status =3D "okay";
>> +		reg =3D <0x0 0xfd400000 0x0 0x40000>, <0x0 0xfd3d0000 0x0 0x1000>;
>> +		reg-names =3D "serdes", "siou";
>> +
>> +		lane0: lane@0 {
>> +			#phy-cells =3D <4>;
>> +		};
>> +		lane1: lane@1 {
>> +			#phy-cells =3D <4>;
>> +		};
>> +		lane2: lane@2 {
>> +			#phy-cells =3D <4>;
>> +		};
>> +		lane3: lane@3 {
>> +			#phy-cells =3D <4>;
>> +		};
>> +	};
>> +
>> +Specifying phy control of devices
>> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> +
>> +Device nodes should specify the configuration required in their "phys"
>> +property, containing a phandle to the phy port node and a device type.
>> +
>> +phys =3D <PHANDLE CONTROLLER_TYPE CONTROLLER_INSTANCE LANE_NUM
>> +LANE_FREQ>;
>> +
>> +PHANDLE                 =3D &lane0 or &lane1 or &lane2 or &lane3
>> +CONTROLLER_TYPE         =3D PHY_TYPE_PCIE or PHY_TYPE_SATA or PHY_TYPE_=
USB
>> +			  or PHY_TYPE_DP or PHY_TYPE_SGMII
>> +CONTROLLER_INSTANCE     =3D Depends on controller type used, can be any=
 of
>> +				PHY_TYPE_PCIE : 0 or 1 or 2 or 3
>> +				PHY_TYPE_SATA : 0 or 1
>> +				PHY_TYPE_USB  : 0 or 1
>> +				PHY_TYPE_DP   : 0 or 1
>> +				PHY_TYPE_SGMII: 0 or 1 or 2 or 3
>> +LANE_NUM                =3D Depends on which lane clock is used as ref =
clk, can be
>> +			  0 or 1 or 2 or 3
>
>I'm not really clear about what this is.
>

ZynqMP phy has a feature that each individual lane (among the 4 lanes) can =
have its own PLL
reference  clock or share the clock generated from other lanes. For example=
=20

Lane 0 can use its own ref clock for PLL: in this case LANE_NUM =3D 0
Lane0 can use Lane 3's ref clock for PLL: in this case  LANE_NUM =3D 3

Maybe I should change the name LANE_NUM to LANE_REF_CLK to give more readab=
ility. Please
correct me if wrong. =20

>> +LANE_FREQ               =3D Frequency that controller can operate, can =
be any of
>> +
>19.2Mhz,20Mhz,24Mhz,26Mhz,27Mhz,28.4Mhz,40Mhz,52Mhz,
>> +			  100Mhz,108Mhz,125Mhz,135Mhz,150Mhz
>
>This can't be implied by the mode/type? I wouldn't expect any frequency ca=
n be used
>for each mode.
>

Yes you are correct, not all frequencies are supported by all protocols, bu=
t each protocol supports
a limited set of multiple reference clock frequencies. For example USB3.0 p=
rotocol can operate with
26, 52 & 100 MHZ. Providing any other frequency would make the phy fail to =
lock the PLL and error
would be generated by the driver. As you suggested, I will add more details=
 regarding the frequencies
supported by different protocols in the next series of the patch.=20
 =20
>> +
>> +Example:
>> +
>> +#include <dt-bindings/phy/phy.h>
>> +
>> +	usb@fe200000 {
>> +		...
>> +		phys	  =3D <&lane2 PHY_TYPE_USB3 0 2 2600000>;
>> +		...
>> +	};
>> +
>> +	ahci@fd0c0000 {
>> +		...
>> +		phys	  =3D <&lane3 PHY_TYPE_SATA 1 1 125000000>;
>> +		...
>> +	};
>
>What if you are doing multiple lanes for 1 device? That would be more usef=
ul 2nd
>example if that's supported.
>

Will add the multi lane example in the next series of the patch.

Thanks,
Anurag Kumar Vulisha

>> --
>> 2.1.1
>>