| 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, 26 Apr 2018 07:02:23 +0000
From: <Nisar.Sayed@...rochip.com>
To: <f.fainelli@...il.com>, <davem@...emloft.net>
CC: <UNGLinuxDriver@...rochip.com>, <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next] microchipT1phy: Add driver for Microchip
LAN87XX T1 PHYs
Hi Florian
> On 04/25/2018 11:49 AM, Nisar Sayed wrote:
> > Add driver for Microchip LAN87XX T1 PHYs
> >
> > This patch support driver for Microchp T1 PHYs.
> > There will be followup patches to this driver to support T1 PHY
> > features such as cable diagnostics, signal quality indicator(SQI),
> > sleep and wakeup (TC10) support.
> >
> > Signed-off-by: Nisar Sayed <Nisar.Sayed@...rochip.com>
> > ---
> > drivers/net/phy/Kconfig | 5 ++
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/microchipT1phy.c | 116
> +++++++++++++++++++++++++++++++++++++++
> > include/linux/microchipT1phy.h | 28 ++++++++++
> > 4 files changed, 150 insertions(+)
> > create mode 100644 drivers/net/phy/microchipT1phy.c create mode
> > 100644 include/linux/microchipT1phy.h
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > bdfbabb..7b0b351 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -354,6 +354,11 @@ config MICROCHIP_PHY
> > help
> > Supports the LAN88XX PHYs.
> >
> > +config MICROCHIP_T1_PHY
> > + tristate "Microchip T1 PHYs"
> > + ---help---
> > + Supports the LAN87XX PHYs.
> > +
> > config MICROSEMI_PHY
> > tristate "Microsemi PHYs"
> > ---help---
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 01acbcb..5706613 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY) += meson-
> gxl.o
> > obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
> > obj-$(CONFIG_MICREL_PHY) += micrel.o
> > obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> > +obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o
> > obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
> > obj-$(CONFIG_NATIONAL_PHY) += national.o
> > obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> > diff --git a/drivers/net/phy/microchipT1phy.c
> > b/drivers/net/phy/microchipT1phy.c
> > new file mode 100644
> > index 0000000..15fbb04
> > --- /dev/null
> > +++ b/drivers/net/phy/microchipT1phy.c
>
> Can we avoid CamelCase file names? Any reasons why this is not part of
> microchip.c?
>
Fine, will change the filename.
The reason for moving to separate file is that we have a series of T1 standard PHYs, which support cable diagnostics, signal quality indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to keep all T1 standard PHYs separate to support additional features supported by these PHYs.
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright (C) 2018 Microchip Technology
>
> Please use a SPDX license tag.
>
Thanks, will update.
> > + *
> > + * 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, or (at your option) any later version.
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +#include <linux/microchipT1phy.h>
> > +
> > +#define DRIVER_AUTHOR "Nisar Sayed <nisar.sayed@...rochip.com>"
> > +#define DRIVER_DESC "Microchip LAN87XX T1 PHY driver"
> > +
> > +struct lan87xx_priv {
> > + int chip_id;
> > + int chip_rev;
> > +};
> > +
> > +static int lan87xx_phy_config_intr(struct phy_device *phydev) {
> > + int rc = 0;
> > +
> > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > + /* unmask all source and clear them before enable */
> > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> 0x7FFF);
> > + rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> > + LAN87XX_MASK_LINK_UP |
> LAN87XX_MASK_LINK_DOWN);
> > + } else {
> > + rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
> > + }
>
> Since the last thing you do in both branches is write to
> LAN87XX_INTERRUPT_MASK, just move that write outside of the branches
> and make sure the value you will set is correct in both cases.
>
Thanks, Sure will modify as suggested.
> > +
> > + return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) {
> > + int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +
> > + return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_probe(struct phy_device *phydev) {
> > + struct device *dev = &phydev->mdio.dev;
> > + struct lan87xx_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + /* these values can be used to identify internal PHY */
> > + priv->chip_id = phy_read(phydev, MII_PHYSID1);
> > + priv->chip_rev = phy_read(phydev, MII_PHYSID2);
>
> This is absolutely not necessary, phydev->phy_id stores that information and
> you can use that at any point in your driver's lifetime. Besides, the revision is
> not the full MII_PHYSID2, it's only the lower 4 bits of it.
>
> Your probe() and remove() function then become unnecessary.
>
Thanks, will address in next version of submission.
> > +
> > + phydev->priv = priv;
> > +
> > + return 0;
> > +}
> > +
> > +static void lan87xx_remove(struct phy_device *phydev) {
> > + struct device *dev = &phydev->mdio.dev;
> > + struct lan87xx_priv *priv = phydev->priv;
> > +
> > + if (priv)
> > + devm_kfree(dev, priv);
> > +}
> > +
> > +static struct phy_driver microchip_T1_phy_driver[] = {
> > + {
> > + .phy_id = 0x0007c150,
> > + .phy_id_mask = 0xfffffff0,
> > + .name = "Microchip LAN87xx",
> > +
> > + .features = SUPPORTED_100baseT_Full,
>
> PHY_BASIC_FEATURES maybe? You cannot possibly be supporting just
> 100BaseT full duplex, 100BaseT half duplex, 10BaseT full and half duplex are
> also supported, right?
>
Yes, the current T1 PHY support only 100BaseT full duplex.
> > + .flags = PHY_HAS_INTERRUPT,
> > +
> > + .probe = lan87xx_probe,
> > + .remove = lan87xx_remove,
> > +
> > + .config_init = genphy_config_init,
> > + .config_aneg = genphy_config_aneg,
> > +
> > + .ack_interrupt = lan87xx_phy_ack_interrupt,
> > + .config_intr = lan87xx_phy_config_intr,
> > +
> > + .suspend = genphy_suspend,
> > + .resume = genphy_resume,
> > + }
> > +};
> > +
> > +module_phy_driver(microchip_T1_phy_driver);
> > +
> > +static struct mdio_device_id __maybe_unused microchip_T1_tbl[] = {
> > + { 0x0007c150, 0xfffffff0 },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(mdio, microchip_T1_tbl);
> > +
> > +MODULE_AUTHOR(DRIVER_AUTHOR);
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/microchipT1phy.h
> > b/include/linux/microchipT1phy.h new file mode 100644 index
> > 0000000..7353e7c
> > --- /dev/null
> > +++ b/include/linux/microchipT1phy.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2018 Microchip Technology
>
> Same comment as before, please use SPDX license identifiers.
>
Thanks, will update.
> > + *
> > + * 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, or (at your option) any later version.
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#ifndef _MICROCHIPT1PHY_H_
> > +#define _MICROCHIPT1PHY_H_
> > +
> > +/* Interrupt Source Register */
> > +#define LAN87XX_INTERRUPT_SOURCE (0x18)
> > +
> > +/* Interrupt Mask Register */
> > +#define LAN87XX_INTERRUPT_MASK (0x19)
> > +#define LAN87XX_MASK_LINK_UP (0x0004)
> > +#define LAN87XX_MASK_LINK_DOWN (0x0002)
>
> What's the point of that header file if all definitions are consumed by the
> same driver?
>
We have planned a series of patches where we planned to use this further.
- Nisar
Powered by blists - more mailing lists