Re: [PATCH] net: phy: TLK10X initial driver submission

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

 



On Thu, Apr 19, 2018 at 10:28 AM, Måns Andersson <mans.andersson@xxxxxxx> wrote:
> From: Mans Andersson <mans.andersson@xxxxxxx>
>
> Add suport for the TI TLK105 and TLK106 10/100Mbit ethernet phys.
>

Hi Mans,

Some quick notes.

> In addition the TLK10X needs to be removed from DP83848 driver as the
> power back off support is added here for this device.
>
> Datasheet:
> http://www.ti.com/lit/gpn/tlk106

Missing signature.

> ---
>  .../devicetree/bindings/net/ti,tlk10x.txt          |  27 +++
>  drivers/net/phy/Kconfig                            |   5 +
>  drivers/net/phy/Makefile                           |   1 +
>  drivers/net/phy/dp83848.c                          |   3 -
>  drivers/net/phy/tlk10x.c                           | 209 +++++++++++++++++++++
>  5 files changed, 242 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,tlk10x.txt
>  create mode 100644 drivers/net/phy/tlk10x.c
>
> diff --git a/Documentation/devicetree/bindings/net/ti,tlk10x.txt b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> new file mode 100644
> index 0000000..371d0d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,tlk10x.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments - TLK105 / TLK106 ethernet PHYs
> +
> +Required properties:
> +       - reg - The ID number for the phy, usually a small integer
> +
> +Optional properties:
> +       - ti,power-back-off - Power Back Off Level
> +               Please refer to data sheet chapter 8.6 and TI Application
> +               Note SLLA3228
> +               0 - Normal Operation
> +               1 - Level 1 (up to 140m cable between TLK link partners)
> +               2 - Level 2 (up to 100m cable between TLK link partners)
> +               3 - Level 3 (up to 80m cable between TLK link partners)
> +
> +Default child nodes are standard Ethernet PHY device
> +nodes as described in Documentation/devicetree/bindings/net/phy.txt
> +
> +Example:
> +
> +       ethernet-phy@0 {
> +               reg = <0>;
> +               ti,power-back-off = <2>;
> +       };
> +
> +Datasheets and documentation can be found at:
> +http://www.ti.com/lit/gpn/tlk106
> +http://www.ti.com/lit/an/slla328/slla328.pdf
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index bdfbabb..c980240 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -295,6 +295,11 @@ config DP83867_PHY
>         ---help---
>           Currently supports the DP83867 PHY.
>
> +config TLK10X_PHY
> +       tristate "Texas Instruments TLK10x PHY"
> +       ---help---
> +         Supports the TLK105 and TLK106 PHYs.
> +
>  config FIXED_PHY
>         tristate "MDIO Bus/PHY emulation with fixed speed/link PHYs"
>         depends on PHYLIB
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 01acbcb..37e4e02 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -79,5 +79,6 @@ obj-$(CONFIG_ROCKCHIP_PHY)    += rockchip.o
>  obj-$(CONFIG_SMSC_PHY)         += smsc.o
>  obj-$(CONFIG_STE10XP)          += ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)   += teranetics.o
> +obj-$(CONFIG_TLK10X_PHY)       += tlk10x.o
>  obj-$(CONFIG_VITESSE_PHY)      += vitesse.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
> index cd09c3a..435f401 100644
> --- a/drivers/net/phy/dp83848.c
> +++ b/drivers/net/phy/dp83848.c
> @@ -19,7 +19,6 @@
>  #define TI_DP83848C_PHY_ID             0x20005ca0
>  #define TI_DP83620_PHY_ID              0x20005ce0
>  #define NS_DP83848C_PHY_ID             0x20005c90
> -#define TLK10X_PHY_ID                  0x2000a210
>
>  /* Registers */
>  #define DP83848_MICR                   0x11 /* MII Interrupt Control Register */
> @@ -78,7 +77,6 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
>         { TI_DP83848C_PHY_ID, 0xfffffff0 },
>         { NS_DP83848C_PHY_ID, 0xfffffff0 },
>         { TI_DP83620_PHY_ID, 0xfffffff0 },
> -       { TLK10X_PHY_ID, 0xfffffff0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
> @@ -105,7 +103,6 @@ static struct phy_driver dp83848_driver[] = {
>         DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
>         DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
>         DP83848_PHY_DRIVER(TI_DP83620_PHY_ID, "TI DP83620 10/100 Mbps PHY"),
> -       DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
>  };
>  module_phy_driver(dp83848_driver);
>
> diff --git a/drivers/net/phy/tlk10x.c b/drivers/net/phy/tlk10x.c
> new file mode 100644
> index 0000000..1efc81e
> --- /dev/null
> +++ b/drivers/net/phy/tlk10x.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * Driver for the Texas Instruments TLK105 / TLK106
> + *
> + * Copyright (C) 2018 NIBE Industrier AB - http://www.nibe.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Since you are using the SPDX id, please remove the license text (which
is actually wrong: it seems you have cut the v2+ version and then
removed the last sentence of the first paragraph? :-).

> + */
> +
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
> +
> +#define TLK10X_PHY_ID                  0x2000a210
> +
> +/* Registers */
> +#define TLK10X_MICR                    0x11 /* MII Interrupt Control Reg */
> +#define TLK10X_MISR                    0x12 /* MII Interrupt Status Reg */
> +#define TLK10X_REGCR                   0x0d /* Register Control Register */
> +#define TLK10X_ADDAR                   0x0e /* Data Register */
> +#define TLK10X_PWRBOCR                 0xae /* Power Backoff Register */
> +
> +/* MICR Register Fields */
> +#define TLK10X_MICR_INT_OE             BIT(0) /* Interrupt Output Enable */
> +#define TLK10X_MICR_INTEN              BIT(1) /* Interrupt Enable */
> +
> +/* MISR Register Fields */
> +#define TLK10X_MISR_RHF_INT_EN         BIT(0) /* Receive Error Counter */
> +#define TLK10X_MISR_FHF_INT_EN         BIT(1) /* False Carrier Counter */
> +#define TLK10X_MISR_ANC_INT_EN         BIT(2) /* Auto-negotiation complete */
> +#define TLK10X_MISR_DUP_INT_EN         BIT(3) /* Duplex Status */
> +#define TLK10X_MISR_SPD_INT_EN         BIT(4) /* Speed status */
> +#define TLK10X_MISR_LINK_INT_EN                BIT(5) /* Link status */
> +#define TLK10X_MISR_ED_INT_EN          BIT(6) /* Energy detect */
> +#define TLK10X_MISR_LQM_INT_EN         BIT(7) /* Link Quality Monitor */
> +
> +/* PWRBOCR Register Fields */
> +#define TLK10X_PWRBOCR_MASK            0xe0 /* Power Backoff Mask */
> +
> +#define TLK10X_INT_EN_MASK             \
> +       (TLK10X_MISR_ANC_INT_EN |       \
> +        TLK10X_MISR_DUP_INT_EN |       \
> +        TLK10X_MISR_SPD_INT_EN |       \
> +        TLK10X_MISR_LINK_INT_EN)
> +
> +struct tlk10x_private {
> +       int pwrbo_level;
> +};
> +
> +static int tlk10x_read(struct phy_device *phydev, int reg)
> +{
> +       if (reg & ~0x1f) {

0x1f or ~0x1f should probably have a #define name.

> +               /* Extended register */
> +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> +               phy_write(phydev, TLK10X_ADDAR, reg);
> +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> +               reg = TLK10X_ADDAR;
> +       }
> +
> +       return phy_read(phydev, reg);
> +}
> +
> +static int tlk10x_write(struct phy_device *phydev, int reg, int val)
> +{
> +       if (reg & ~0x1f) {

Ditto.

> +               /* Extended register */
> +               phy_write(phydev, TLK10X_REGCR, 0x001F);
> +               phy_write(phydev, TLK10X_ADDAR, reg);
> +               phy_write(phydev, TLK10X_REGCR, 0x401F);
> +               reg = TLK10X_ADDAR;
> +       }
> +
> +       return phy_write(phydev, reg, val);
> +}
> +
> +#ifdef CONFIG_OF_MDIO

Maybe you want the #ifdef inside.

> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +       struct tlk10x_private *tlk10x = phydev->priv;
> +       struct device *dev = &phydev->mdio.dev;
> +       struct device_node *of_node = dev->of_node;
> +       int ret;
> +
> +       if (!of_node)
> +               return 0;
> +
> +       ret = of_property_read_u32(of_node, "ti,power-back-off",
> +                                  &tlk10x->pwrbo_level);
> +       if (ret) {
> +               dev_err(dev, "missing ti,power-back-off property");
> +               tlk10x->pwrbo_level = 0;
> +       }
> +
> +       return 0;
> +}
> +#else
> +static int tlk10x_of_init(struct phy_device *phydev)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_OF_MDIO */
> +
> +static int tlk10x_config_init(struct phy_device *phydev)
> +{
> +       int ret, reg;
> +       struct tlk10x_private *tlk10x;
> +
> +       ret = genphy_config_init(phydev);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!phydev->priv) {
> +               tlk10x = devm_kzalloc(&phydev->mdio.dev, sizeof(*tlk10x),
> +                                     GFP_KERNEL);
> +               if (!tlk10x)
> +                       return -ENOMEM;
> +
> +               phydev->priv = tlk10x;
> +               ret = tlk10x_of_init(phydev);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               tlk10x = (struct tlk10x_private *)phydev->priv;
> +       }
> +
> +       // Power back off
> +       if (tlk10x->pwrbo_level < 0 || tlk10x->pwrbo_level > 3)
> +               tlk10x->pwrbo_level = 0;
> +       reg = tlk10x_read(phydev, TLK10X_PWRBOCR);
> +       reg = ((reg & ~TLK10X_PWRBOCR_MASK)
> +               | (tlk10x->pwrbo_level << 6));

Maybe the 6 should have a name, or maybe a bigger macro for this would clarify.

> +       ret = tlk10x_write(phydev, TLK10X_PWRBOCR, reg);
> +       if (ret < 0) {
> +               dev_err(&phydev->mdio.dev,
> +                       "unable to set power back-off (err=%d)\n", ret);
> +               return ret;
> +       }
> +       dev_info(&phydev->mdio.dev, "power back-off set to level %d\n",
> +                tlk10x->pwrbo_level);
> +
> +       return 0;
> +}
> +
> +static int tlk10x_ack_interrupt(struct phy_device *phydev)
> +{
> +       int err = tlk10x_read(phydev, TLK10X_MISR);

Following the style of the rest of the file, shouldn't this be:

    if (err < 0)
        return err;

    return 0;

?

> +
> +       return err < 0 ? err : 0;
> +}
> +
> +static int tlk10x_config_intr(struct phy_device *phydev)
> +{
> +       int control, ret;
> +
> +       control = tlk10x_read(phydev, TLK10X_MICR);
> +       if (control < 0)
> +               return control;
> +
> +       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +               control |= TLK10X_MICR_INT_OE;
> +               control |= TLK10X_MICR_INTEN;
> +
> +               ret = tlk10x_write(phydev, TLK10X_MISR, TLK10X_INT_EN_MASK);
> +               if (ret < 0)
> +                       return ret;
> +       } else {
> +               control &= ~TLK10X_MICR_INTEN;
> +       }
> +
> +       return tlk10x_write(phydev, TLK10X_MICR, control);
> +}
> +
> +static struct phy_driver tlk10x_driver[] = {
> +       {
> +               .phy_id         = TLK10X_PHY_ID,
> +               .phy_id_mask    = 0xfffffff0,
> +               .name           = "TI TLK10X 10/100 Mbps PHY",
> +               .features       = PHY_BASIC_FEATURES,
> +               .flags          = PHY_HAS_INTERRUPT,
> +
> +               .config_init    = tlk10x_config_init,
> +               .soft_reset     = genphy_soft_reset,
> +
> +               /* IRQ related */
> +               .ack_interrupt  = tlk10x_ack_interrupt,
> +               .config_intr    = tlk10x_config_intr,
> +
> +               .suspend        = genphy_suspend,
> +               .resume         = genphy_resume,
> +       },
> +};
> +module_phy_driver(tlk10x_driver);
> +
> +static struct mdio_device_id __maybe_unused tlk10x_tbl[] = {
> +       { TLK10X_PHY_ID, 0xfffffff0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(mdio, tlk10x_tbl);
> +
> +MODULE_DESCRIPTION("Texas Instruments TLK105 / TLK106 PHY driver");
> +MODULE_AUTHOR("Mans Andersson <mans.andersson@xxxxxxx>");
> +MODULE_LICENSE("GPL");

Should be "GPL v2".

Cheers,
Miguel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux