| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 Apr 2018 14:24:27 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Måns Andersson <mans.andersson@...e.se>
Cc: Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Network Development <netdev@...r.kernel.org>,
devicetree@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: phy: TLK10X initial driver submission
On Thu, Apr 19, 2018 at 10:28 AM, Måns Andersson <mans.andersson@...e.se> wrote:
> From: Mans Andersson <mans.andersson@...e.se>
>
> 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@...e.se>");
> +MODULE_LICENSE("GPL");
Should be "GPL v2".
Cheers,
Miguel
Powered by blists - more mailing lists