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-kw...@ti.com>
> ---
>  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?

> + - 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?

> + - 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?

> + - control-rate:
> +     * Lane control rate
> +             0: full rate
> +             1: half rate
> +             2: quarter rate

Likewise on both points.

> + - 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/

> + - 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.

> +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.

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

> +     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?

> +     if (of_find_property(np, "loopback", NULL))
> +             lc->loopback = 1;
> +     else
> +             lc->loopback = 0;

Likewise.

> +     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?

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

Likewise.

> +     sc->debug = of_property_read_bool(np, "debug");

As stated above, NAK for this property.

> +
> +     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.

> +
> +     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.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to