| 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: <19074ccb-5356-469b-8d1e-8076be135a21@www.fastmail.com>
Date: Tue, 30 Jul 2019 11:53:32 +0930
From: "Andrew Jeffery" <andrew@...id.au>
To: "Andrew Lunn" <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>,
"David Miller" <davem@...emloft.net>,
"Rob Herring" <robh+dt@...nel.org>, mark.rutland@....com,
"Joel Stanley" <joel@....id.au>,
"Florian Fainelli" <f.fainelli@...il.com>,
"Heiner Kallweit" <hkallweit1@...il.com>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] net: phy: Add mdio-aspeed
On Mon, 29 Jul 2019, at 23:03, Andrew Lunn wrote:
> On Mon, Jul 29, 2019 at 02:09:24PM +0930, Andrew Jeffery wrote:
> > The AST2600 design separates the MDIO controllers from the MAC, which is
> > where they were placed in the AST2400 and AST2500. Further, the register
> > interface is reworked again, so now we have three possible different
> > interface implementations, however this driver only supports the
> > interface provided by the AST2600. The AST2400 and AST2500 will continue
> > to be supported by the MDIO support embedded in the FTGMAC100 driver.
> >
> > Signed-off-by: Andrew Jeffery <andrew@...id.au>
> > ---
> > drivers/net/phy/Kconfig | 13 +++
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/mdio-aspeed.c | 159 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 173 insertions(+)
> > create mode 100644 drivers/net/phy/mdio-aspeed.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 20f14c5fbb7e..206d8650ee7f 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -21,6 +21,19 @@ config MDIO_BUS
> >
> > if MDIO_BUS
> >
> > +config MDIO_ASPEED
> > + tristate "ASPEED MDIO bus controller"
> > + depends on ARCH_ASPEED || COMPILE_TEST
> > + depends on OF_MDIO && HAS_IOMEM
> > + help
> > + This module provides a driver for the independent MDIO bus
> > + controllers found in the ASPEED AST2600 SoC. This is a driver for the
> > + third revision of the ASPEED MDIO register interface - the first two
> > + revisions are the "old" and "new" interfaces found in the AST2400 and
> > + AST2500, embedded in the MAC. For legacy reasons, FTGMAC100 driver
> > + continues to drive the embedded MDIO controller for the AST2400 and
> > + AST2500 SoCs, so say N if AST2600 support is not required.
> > +
> > config MDIO_BCM_IPROC
> > tristate "Broadcom iProc MDIO bus controller"
> > depends on ARCH_BCM_IPROC || COMPILE_TEST
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 839acb292c38..ba07c27e4208 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -22,6 +22,7 @@ libphy-$(CONFIG_LED_TRIGGER_PHY) += phy_led_triggers.o
> > obj-$(CONFIG_PHYLINK) += phylink.o
> > obj-$(CONFIG_PHYLIB) += libphy.o
> >
> > +obj-$(CONFIG_MDIO_ASPEED) += mdio-aspeed.o
> > obj-$(CONFIG_MDIO_BCM_IPROC) += mdio-bcm-iproc.o
> > obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o
> > obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o
> > diff --git a/drivers/net/phy/mdio-aspeed.c b/drivers/net/phy/mdio-aspeed.c
> > new file mode 100644
> > index 000000000000..71496a9ff54a
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-aspeed.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* Copyright (C) 2019 IBM Corp. */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/mdio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRV_NAME "mdio-aspeed"
> > +
> > +#define ASPEED_MDIO_CTRL 0x0
> > +#define ASPEED_MDIO_CTRL_FIRE BIT(31)
> > +#define ASPEED_MDIO_CTRL_ST BIT(28)
> > +#define ASPEED_MDIO_CTRL_ST_C45 0
> > +#define ASPEED_MDIO_CTRL_ST_C22 1
> > +#define ASPEED_MDIO_CTRL_OP GENMASK(27, 26)
> > +#define MDIO_C22_OP_WRITE 0b01
> > +#define MDIO_C22_OP_READ 0b10
> > +#define ASPEED_MDIO_CTRL_PHYAD GENMASK(25, 21)
> > +#define ASPEED_MDIO_CTRL_REGAD GENMASK(20, 16)
> > +#define ASPEED_MDIO_CTRL_MIIWDATA GENMASK(15, 0)
> > +
> > +#define ASPEED_MDIO_DATA 0x4
> > +#define ASPEED_MDIO_DATA_MDC_THRES GENMASK(31, 24)
> > +#define ASPEED_MDIO_DATA_MDIO_EDGE BIT(23)
> > +#define ASPEED_MDIO_DATA_MDIO_LATCH GENMASK(22, 20)
> > +#define ASPEED_MDIO_DATA_IDLE BIT(16)
> > +#define ASPEED_MDIO_DATA_MIIRDATA GENMASK(15, 0)
> > +
> > +#define ASPEED_MDIO_RETRIES 10
> > +
> > +struct aspeed_mdio {
> > + void __iomem *base;
> > +};
> > +
> > +static int aspeed_mdio_read(struct mii_bus *bus, int addr, int regnum)
> > +{
> > + struct aspeed_mdio *ctx = bus->priv;
> > + u32 ctrl;
> > + int i;
> > +
> > + dev_dbg(&bus->dev, "%s: addr: %d, regnum: %d\n", __func__, addr,
> > + regnum);
> > +
> > + /* Just clause 22 for the moment */
> > + ctrl = ASPEED_MDIO_CTRL_FIRE
>
> Hi Andrew
>
> In the binding, you say C45 is supported. Here you don't. It would be
> nice to be consistent.
Right - but the bindings describe the hardware, and the hardware is capable.
Just that the driver as it stands can't drive it that way.
Having said that I do need to do more digging to understand how to cater to
C45 PHYs wrt the read/write calls.
>
>
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_ST, ASPEED_MDIO_CTRL_ST_C22)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_OP, MDIO_C22_OP_READ)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_PHYAD, addr)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_REGAD, regnum);
> > +
> > + iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
> > +
> > + for (i = 0; i < ASPEED_MDIO_RETRIES; i++) {
> > + u32 data;
> > +
> > + data = ioread32(ctx->base + ASPEED_MDIO_DATA);
> > + if (data & ASPEED_MDIO_DATA_IDLE)
> > + return FIELD_GET(ASPEED_MDIO_DATA_MIIRDATA, data);
> > +
> > + udelay(100);
> > + }
>
> One of the readx_poll_timeout functions could be used.
Thanks, I'll take a look.
>
> > +
> > + dev_err(&bus->dev, "MDIO read failed\n");
> > + return -EIO;
> > +}
> > +
> > +static int aspeed_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
> > +{
> > + struct aspeed_mdio *ctx = bus->priv;
> > + u32 ctrl;
> > + int i;
> > +
> > + dev_dbg(&bus->dev, "%s: addr: %d, regnum: %d, val: 0x%x\n",
> > + __func__, addr, regnum, val);
> > +
> > + /* Just clause 22 for the moment */
> > + ctrl = ASPEED_MDIO_CTRL_FIRE
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_ST, ASPEED_MDIO_CTRL_ST_C22)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_OP, MDIO_C22_OP_WRITE)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_PHYAD, addr)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_REGAD, regnum)
> > + | FIELD_PREP(ASPEED_MDIO_CTRL_MIIWDATA, val);
> > +
> > + iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
> > +
> > + for (i = 0; i < ASPEED_MDIO_RETRIES; i++) {
> > + ctrl = ioread32(ctx->base + ASPEED_MDIO_CTRL);
> > + if (!(ctrl & ASPEED_MDIO_CTRL_FIRE))
> > + return 0;
> > +
> > + udelay(100);
> > + }
>
> readx_poll_timeout() here as well.
>
> Otherwise this looks good.
Thanks for the review!
Andrew
Powered by blists - more mailing lists