| 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
| ||
|
Message-ID: <20180420070755.GB110571@ninb0909vm2.ninb0909.nibe.se>
Date: Fri, 20 Apr 2018 09:07:55 +0200
From: Måns Andersson <mans.andersson@...e.se>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
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 02:24:27PM +0200, Miguel Ojeda wrote:
> 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.
Hi Miguel,
My bad, will fix.
>
> > ---
> > .../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? :-).
>
Ok.
> > + */
> > +
> > +#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.
>
That's true, will fix that.
> > + /* 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.
>
Ok.
> > + /* 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.
>
What's the preferred way by the kernel maintainers? I've seen both solutions
used but figured this was the most common.
> > +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.
>
Will look into this.
> > + 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;
>
> ?
>
True, will fix that.
> > +
> > + 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
Good catch, will update that as well. Thanks for all the input! I will
get back in a couple of days with an updated patch.
// Måns
Powered by blists - more mailing lists