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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

> -----Original Message-----
> From: Rob Herring [mailto:robh+dt@xxxxxxxxxx]
> Sent: Thursday, October 15, 2015 12:15 PM
> To: Kwok, WingMan
> Cc: Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; KISHON VIJAY ABRAHAM;
> Quadros, Roger; Karicheri, Muralidharan; Bjorn Helgaas; Santosh Shilimkar;
> Russell King - ARM Linux; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
> 
> On Thu, Oct 15, 2015 at 9:25 AM, WingMan Kwok <w-kwok2@xxxxxx> 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.
> >
> > v1:
> >         - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok <w-kwok2@xxxxxx>
> > ---
> >  Documentation/devicetree/bindings/phy/ti-phy.txt |  278 +++
> >  drivers/phy/Kconfig                              |    8 +
> >  drivers/phy/Makefile                             |    1 +
> >  drivers/phy/phy-keystone-serdes.c                | 2373
> ++++++++++++++++++++++
> >  4 files changed, 2660 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..4dca271 100644
> > --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> > @@ -115,4 +115,282 @@ 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"
> 
> These are different blocks or different modes of the same block? It's
> fine if the former. If the latter, then you should have a single
> compatible and then have a mode property. Perhaps phy-connection-type
> from ePAPR ethernet binding can be extended.
> 

these are different hw blocks configured specifically
for the corresponding peripheral.

> 
> > + - reg:
> > +       * base address and length of the SerDes register set
> > + - reg-names:
> > +       * "serdes"
> > +               - name of the reg SerDes register set
> 
> reg-names is kind of pointless with only 1.
> 

will remove.

> > + - #phy-cells:
> > +       * From the generic phy bindings, must be 0;
> > + - num-lanes:
> > +       * Number of lanes in SerDes.
> > +
> > +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.
> > + - rx-force-enable:
> > +       * Include this property if receiver attenuation and boost are
> > +         to be configured with specific values defined in rx-force.
> > + - link-rate-kbps:
> > +       * SerDes link rate to be configured, in kbps.
> > +
> > +
> > +For gbe and 10gbe SerDes, it is optional to represent each lane as
> > +a sub-node, which can be enabled or disabled individually using
> > +the "status" property.
> > +
> > +Required properties (lane sub-node):
> > + - reg:
> > +       * lane number
> > +
> > +Optional properties (lane sub-node):
> > + - control-rate:
> > +       * Lane control rate
> > +               0: full rate
> > +               1: half rate
> > +               2: quarter 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 output voltage
> > +         configurations.
> > +       * Must be array of 5 integers.
> > + - loopback:
> > +       * Include this property to enable loopback at the SerDes
> > +         lane level.
> 
> This seems overly complicated. Do you really expect these to be
> different per lane?
> 

It is an requirement that each lane can be enabled/disabled
and configured individually.  Also it is potentially possible
that some of them are different due to calibration results.

> > +
> > +Example for Keystone K2E GBE:
> > +-----------------------------
> > +
> > +gbe_serdes0: gbe_serdes@232a000 {
> > +       #phy-cells              = <0>;
> > +       compatible              = "ti,keystone-serdes-gbe";
> > +       reg                     = <0x0232a000 0x2000>;
> > +       reg-names               = "serdes";
> > +       link-rate-kbps          = <1250000>;
> > +       num-lanes               = <4>;
> > +       lanes {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               lane@0 {
> > +                       /*loopback;*/
> > +                       reg             = <0>;
> > +                       control-rate    = <2>; /* quart */
> > +                       rx-start        = <7 5>;
> > +                       rx-force        = <1 1>;
> > +                       tx-coeff        = <0 0 0 12 4>;
> > +                              /* c1 c2 cm att vreg */
> > +               };
> > +               lane@1 {
> > +                       /*loopback;*/
> > +                       reg             = <1>;
> > +                       control-rate    = <2>; /* quart */
> > +                       rx-start        = <7 5>;
> > +                       rx-force        = <1 1>;
> > +                       tx-coeff        = <0 0 0 12 4>;
> > +                              /* c1 c2 cm att vreg */
> > +               };
> > +       };
> > +};
> > +
> > +gbe_serdes1: gbe_serdes@2324000 {
> > +       #phy-cells              = <0>;
> > +       compatible              = "ti,keystone-serdes-gbe";
> > +       reg                     = <0x02324000 0x2000>;
> > +       reg-names               = "serdes";
> > +       link-rate-kbps          = <1250000>;
> > +       num-lanes               = <4>;
> 
> 4 lanes, but only 2 child nodes?
> 

since each lane can be enabled/disabled individually, a disabled
lane can have a node defined but with a status = "disabled" or
does not have a lane node defined at all.  this example shows the
latter.

> > +       lanes {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               lane@0 {
> > +                       /*loopback;*/
> > +                       reg             = <0>;
> > +                       control-rate    = <2>; /* quart */
> > +                       rx-start        = <7 5>;
> > +                       rx-force        = <1 1>;
> > +                       tx-coeff        = <0 0 0 12 4>;
> > +                              /* c1 c2 cm att vreg */
> > +               };
> > +               lane@1 {
> > +                       /*loopback;*/
> > +                       reg             = <1>;
> > +                       control-rate    = <2>; /* quart */
> > +                       rx-start        = <7 5>;
> > +                       rx-force        = <1 1>;
> > +                       tx-coeff        = <0 0 0 12 4>;
> > +                              /* c1 c2 cm att vreg */
> > +               };
> > +       };
> > +};
> > +
> > +netcp: netcp@24000000 {
> > +       ...
> > +
> > +       netcp-devices {
> > +               ...
> > +
> > +               gbe@200000 { /* ETHSS */
> > +                       ...
> > +                       serdeses {
> > +                               #address-cells = <1>;
> > +                               #size-cells = <0>;
> > +                               serdes@0 {
> > +                                       reg = <0>;
> > +                                       phys = <&gbe_serdes0>;
> > +                                       status = "ok";
> > +                               };
> > +                               serdes@1 {
> > +                                       reg = <1>;
> > +                                       phys = <&gbe_serdes1>;
> > +                                       status = "ok";
> 
> This is way too complex. Just do:
> 
> phys = <&gbe_serdes0, &gbe_serdes1>;
> 
> in the gbe node.
> 

good point. will change to phys = <&gbe_serdes0>, <&gbe_serdes1>;

> Rob

Thanks,
WingMan
��.n��������+%������w��{.n�����{���"�)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux