RE: [PATCH 1/3] phy: keystone: serdes driver for gbe 10gbe and pcie

[Kwok, WingMan]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@xxxxxxx]
> Sent: Tuesday, October 13, 2015 2:33 PM
> To: Kwok, WingMan
> Cc: robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx;
> ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx; KISHON VIJAY
> ABRAHAM; Quadros, Roger; Karicheri, Muralidharan; bhelgaas@xxxxxxxxxx;
> ssantosh@xxxxxxxxxx; linux@xxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/3] phy: keystone: serdes driver for gbe 10gbe
> and pcie
> 
> On Tue, Oct 13, 2015 at 02:04:22PM -0400, WingMan Kwok wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC.  The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > Signed-off-by: WingMan Kwok <w-kwok2@xxxxxx>
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  256 +++
> >  drivers/phy/Kconfig                              |    8 +
> >  drivers/phy/Makefile                             |    1 +
> >  drivers/phy/phy-keystone-serdes.c                | 2465
> ++++++++++++++++++++++
> >  4 files changed, 2730 insertions(+)
> >  create mode 100644 drivers/phy/phy-keystone-serdes.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt
> b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > index 9cf9446..231716e 100644
> > --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > @@ -115,4 +115,260 @@ sata_phy: phy@4A096000 {
> >  	clock-names = "sysclk", "refclk";
> >  	syscon-pllreset = <&scm_conf 0x3fc>;
> >  	#phy-cells = <0>;
> > +
> > +TI Keystone SerDes PHY
> > +======================
> > +
> > +Required properties:
> > + - compatible: should be one of
> > +	* "ti,keystone-serdes-gbe"
> > +	* "ti,keystone-serdes-xgbe"
> > +	* "ti,keystone-serdes-pcie"
> > + - reg:
> > +	* base address and length of the SerDes register set
> > + - reg-names:
> > +	* "reg_serdes"
> > +		- name of the reg SerDes register set
> 
> Just describe reg in terms of reg-names, and don't bother with the
> "reg_" prefix, we know this is a reg entry:
> 
> - reg: a list of address and length pairs, corresponding to entires in
>   reg-names
> 
> - reg-names: should contain:
>   * "serdes"
> 
> > + - #phy-cells:
> > +	* From the generic phy bindings, must be 0;
> > + - max-lanes:
> > +	* Number of lanes in SerDes.
> 
> Why is this not "num-lanes"? Why do you even need this?
> 

will change to "num-lanes".

serdes on different peripherals have different number of
lanes and each individual lane can be enabled or disabled.

> > + - phy-type: should be one of
> > +	* "sgmii"
> > +	* "xge"
> > +	* "pcie"
> > +
> > +Optional properties:
> > + - syscon-peripheral:
> > +	* Handle to the subsystem register region of the peripheral
> > +	  inside which the SerDes exists.
> > + - syscon-link:
> > +	* Handle to the Link register region of the peripheral inside
> > +	  which the SerDes exists.  Example: it is the PCSR register
> > +	  region in the case of 10gbe.
> > + - refclk-khz:
> > +	* Reference clock rate of SerDes in kHz.
> 
> Surely this should be an actual clock?
> 

ok, this is not actually needed and will remove

> > + - link-rate-kbps:
> > +	* SerDes link rate to be configured, in kbps.
> 
> Why does this need to be in the binding? How does one derive the
> correct
> value?
> 

a serdes can be configured at different link rates.  for example,
the 10gbe serdes can be configured at 1.25G link rate to support
1gbe on the 10gbe switch.

> > + - control-rate:
> > +	* Lane control rate
> > +		0: full rate
> > +		1: half rate
> > +		2: quarter rate
> 
> Likewise on both points.

the serdes is a generic hardware block that needs to be configured
with the proper link-rate and lane control rate.

> 
> > + - rx-start:
> > +	* Initial lane rx equalizer attenuation and boost configurations.
> > +	* Must be array of 2 integers.
> > + - rx-force:
> > +	* Forced lane rx equalizer attenuation and boost configurations.
> > +	* Must be array of 2 integers.
> > + - tx-coeff:
> > +	* Lane c1, c2, cm, attenuation and regulator outpust voltage
> > +	  configurations.
> > +	* Must be array of 5 integers.
> 
> s/outpust/output/
> 

ok.

> > + - debug:
> > +	* enable more debug messages.
> 
> NAK.
> 
> This is a driver option, and belongs on the kernel command line. It
> does
> not describe the hardware, and does not belong in the DT.
> 

will remove.

> > +Example for Keystone K2E GBE:
> > +-----------------------------
> > +
> > +gbe_serdes0: gbe_serdes@232a000 {
> > +	#phy-cells		= <0>;
> > +	compatible		= "ti,keystone-serdes-gbe";
> > +	reg			= <0x0232a000 0x2000>;
> > +	reg-names		= "reg_serdes";
> > +	refclk-khz		= <156250>;
> > +	link-rate-kbps		= <1250000>;
> > +	phy-type		= "sgmii";
> > +	max-lanes		= <4>;
> > +	lane0 {
> > +		control-rate	= <2>; /* quart */
> > +		rx-start	= <7 5>;
> > +		rx-force	= <1 1>;
> > +		tx-coeff	= <0 0 0 12 4>;
> > +		       /* c1 c2 cm att vreg */
> > +	};
> > +	lane1 {
> > +		control-rate	= <2>; /* quart */
> > +		rx-start	= <7 5>;
> > +		rx-force	= <1 1>;
> > +		tx-coeff	= <0 0 0 12 4>;
> > +		       /* c1 c2 cm att vreg */
> > +	};
> 
> The binding didn't describe the sub-nodes, and which properties belong
> to them.
> 

will add descriptions for lane nodes

> Don't use magic names. Define a new address space, and use reg to
> identify lanes.
> 

will update to the following

lanes {
	lane@0 {
		reg = <0>;
		...
	};

	lane@1 {
		reg = <1>;
		...	
	};
};


> > +	if (of_find_property(np, "disable", NULL))
> > +		lc->enable = 0;
> > +	else
> > +		lc->enable = 1;
> 
> This was not described in the binding, and uses the wrong accessor.
> What
> is this for?

after the above mentioned update, this will become: status = "disabled/ok".
> 
> > +	if (of_find_property(np, "loopback", NULL))
> > +		lc->loopback = 1;
> > +	else
> > +		lc->loopback = 0;
> 
> Likewise.

will add documentation

> 
> > +	if (of_property_read_bool(np, "syscon-peripheral")) {
> > +		sc->peripheral_regmap =
> > +			syscon_regmap_lookup_by_phandle(np,
> > +							"syscon-peripheral");
> 
> Can't you always call syscon_regmap_lookup_by_phandle, then check the
> return value to see if the property existed?
> 
> You clearly know that of_property_read_bool exists, why did you not
> use
> it to read properties which are boolean?
> 

ok.

> > +	if (of_property_read_bool(np, "syscon-link")) {
> > +		sc->pcsr_regmap =
> > +			syscon_regmap_lookup_by_phandle(np, "syscon-link");
> 
> Likewise.
> 

ok.

> > +	sc->debug = of_property_read_bool(np, "debug");
> 
> As stated above, NAK for this property.
> 
 
will remove.

> > +
> > +	if (of_find_property(np, "rx-force-enable", NULL))
> > +		sc->rx_force_enable = 1;
> > +	else	
> > +		sc->rx_force_enable = 0;
> 
> Not in the binding, and uses the wrong accessor.
> 

ok. will update the binding and change of_find_property to
of_property_read_bool.

> > +
> > +	for (i = 0; i < sc->lanes; i++) {
> > +		sprintf(&name[4], "%d", i);
> > +		lp = of_find_node_by_name(np, name);
> > +		if (lp) {
> > +			if (kserdes_get_lane_bindings(dev, lp, &sc->lane[i]))
> > +				return -EINVAL;
> > +		}
> > +	}
> 
> As above, use reg, not magic names.

ok.

Thanks,
WingMan