| 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: <20220510182818.w7kl3vmlgvqjjj4u@skbuf>
Date: Tue, 10 May 2022 21:28:18 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>,
Sean Wang <sean.wang@...iatek.com>,
Landen Chao <Landen.Chao@...iatek.com>,
DENG Qingfang <dqfext@...il.com>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Joakim Zhang <qiangqing.zhang@....com>,
Sergey Shtylyov <s.shtylyov@....ru>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Tobias Waldekranz <tobias@...dekranz.com>,
Marcin Wojtas <mw@...ihalf.com>,
Calvin Johnson <calvin.johnson@....nxp.com>,
Markus Koch <markus@...syncing.net>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Yang Yingliang <yangyingliang@...wei.com>,
Hao Chen <chenhao288@...ilicon.com>
Subject: Re: [PATCH net-next 07/10] net: ethernet: freescale: xgmac: Separate
C22 and C45 transactions for xgmac
On Sun, May 08, 2022 at 05:30:46PM +0200, Andrew Lunn wrote:
> The xgmac MDIO bus driver can perform both C22 and C45 transfers.
> Create separate functions for each and register the C45 versions using
> the new API calls where appropriate.
>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
> drivers/net/ethernet/freescale/xgmac_mdio.c | 154 +++++++++++++++-----
> 1 file changed, 117 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
> index ec90da1de030..ddfe6bf1f231 100644
> --- a/drivers/net/ethernet/freescale/xgmac_mdio.c
> +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
> @@ -128,30 +128,59 @@ static int xgmac_wait_until_done(struct device *dev,
> return 0;
> }
>
> -/*
> - * Write value to the PHY for this device to the register at regnum,waiting
> +/* Write value to the PHY for this device to the register at regnum,waiting
> * until the write is done before it returns. All PHY configuration has to be
> * done through the TSEC1 MIIM regs.
> */
> -static int xgmac_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +static int xgmac_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum,
> + u16 value)
> {
> struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
> struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
> - uint16_t dev_addr;
> + bool endian = priv->is_little_endian;
> u32 mdio_ctl, mdio_stat;
> + u16 dev_addr;
> int ret;
> +
> + mdio_stat = xgmac_read32(®s->mdio_stat, endian);
> + dev_addr = regnum & 0x1f;
Please move this either to the variable declaration, or near the mdio_ctl write,
or just integrate it into the macro argument.
> + mdio_stat &= ~MDIO_STAT_ENC;
> +
You can remove this empty line during read-modify-write patterns.
> + xgmac_write32(mdio_stat, ®s->mdio_stat, endian);
> +
> + ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> + if (ret)
> + return ret;
> +
> + /* Set the port and dev addr */
> + mdio_ctl = MDIO_CTL_PORT_ADDR(phy_id) | MDIO_CTL_DEV_ADDR(dev_addr);
> + xgmac_write32(mdio_ctl, ®s->mdio_ctl, endian);
> +
> + /* Write the value to the register */
> + xgmac_write32(MDIO_DATA(value), ®s->mdio_data, endian);
> +
> + ret = xgmac_wait_until_done(&bus->dev, regs, endian);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +/* Write value to the PHY for this device to the register at regnum,waiting
> + * until the write is done before it returns. All PHY configuration has to be
> + * done through the TSEC1 MIIM regs.
> + */
> +static int xgmac_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> + int regnum, u16 value)
> +{
> + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
> + struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
> bool endian = priv->is_little_endian;
> + u32 mdio_ctl, mdio_stat;
> + int ret;
>
> mdio_stat = xgmac_read32(®s->mdio_stat, endian);
> - if (regnum & MII_ADDR_C45) {
> - /* Clause 45 (ie 10G) */
> - dev_addr = (regnum >> 16) & 0x1f;
> - mdio_stat |= MDIO_STAT_ENC;
> - } else {
> - /* Clause 22 (ie 1G) */
> - dev_addr = regnum & 0x1f;
> - mdio_stat &= ~MDIO_STAT_ENC;
> - }
> + mdio_stat |= MDIO_STAT_ENC;
>
> xgmac_write32(mdio_stat, ®s->mdio_stat, endian);
>
> @@ -164,13 +193,11 @@ static int xgmac_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 val
> xgmac_write32(mdio_ctl, ®s->mdio_ctl, endian);
>
> /* Set the register address */
> - if (regnum & MII_ADDR_C45) {
> - xgmac_write32(regnum & 0xffff, ®s->mdio_addr, endian);
> + xgmac_write32(regnum & 0xffff, ®s->mdio_addr, endian);
Does regnum ever exceed 0xffff now that the MMD is no longer encoded into it?
>
> - ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> - if (ret)
> - return ret;
> - }
> + ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> + if (ret)
> + return ret;
>
> /* Write the value to the register */
> xgmac_write32(MDIO_DATA(value), ®s->mdio_data, endian);
> @@ -182,31 +209,84 @@ static int xgmac_mdio_write(struct mii_bus *bus, int phy_id, int regnum, u16 val
> return 0;
> }
>
> -/*
> - * Reads from register regnum in the PHY for device dev, returning the value.
> +/* Reads from register regnum in the PHY for device dev, returning the value.
> * Clears miimcom first. All PHY configuration has to be done through the
> * TSEC1 MIIM regs.
> */
> -static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
> +static int xgmac_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> {
> struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
> struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
> + bool endian = priv->is_little_endian;
> unsigned long flags;
> - uint16_t dev_addr;
> uint32_t mdio_stat;
> uint32_t mdio_ctl;
> + u16 dev_addr;
> int ret;
> - bool endian = priv->is_little_endian;
>
> mdio_stat = xgmac_read32(®s->mdio_stat, endian);
> - if (regnum & MII_ADDR_C45) {
> - dev_addr = (regnum >> 16) & 0x1f;
> - mdio_stat |= MDIO_STAT_ENC;
> + dev_addr = regnum & 0x1f;
I'm thinking we could just pass "regnum & 0x1f" (or just regnum) to
MDIO_CTL_DEV_ADDR() for the c22 functions.
> + mdio_stat &= ~MDIO_STAT_ENC;
> +
> + xgmac_write32(mdio_stat, ®s->mdio_stat, endian);
> +
> + ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> + if (ret)
> + return ret;
> +
> + /* Set the Port and Device Addrs */
> + mdio_ctl = MDIO_CTL_PORT_ADDR(phy_id) | MDIO_CTL_DEV_ADDR(dev_addr);
> + xgmac_write32(mdio_ctl, ®s->mdio_ctl, endian);
> +
> + if (priv->has_a009885)
> + /* Once the operation completes, i.e. MDIO_STAT_BSY clears, we
> + * must read back the data register within 16 MDC cycles.
> + */
> + local_irq_save(flags);
> +
> + /* Initiate the read */
> + xgmac_write32(mdio_ctl | MDIO_CTL_READ, ®s->mdio_ctl, endian);
> +
> + ret = xgmac_wait_until_done(&bus->dev, regs, endian);
> + if (ret)
> + goto irq_restore;
> +
> + /* Return all Fs if nothing was there */
> + if ((xgmac_read32(®s->mdio_stat, endian) & MDIO_STAT_RD_ER) &&
> + !priv->has_a011043) {
> + dev_dbg(&bus->dev,
> + "Error while reading PHY%d reg at %d.%d\n",
> + phy_id, dev_addr, regnum);
> + ret = 0xffff;
> } else {
> - dev_addr = regnum & 0x1f;
> - mdio_stat &= ~MDIO_STAT_ENC;
> + ret = xgmac_read32(®s->mdio_data, endian) & 0xffff;
> + dev_dbg(&bus->dev, "read %04x\n", ret);
> }
>
> +irq_restore:
> + if (priv->has_a009885)
> + local_irq_restore(flags);
> +
> + return ret;
> +}
> +
> +/* Reads from register regnum in the PHY for device dev, returning the value.
> + * Clears miimcom first. All PHY configuration has to be done through the
> + * TSEC1 MIIM regs.
> + */
> +static int xgmac_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> + int regnum)
> +{
> + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
> + struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
> + bool endian = priv->is_little_endian;
> + u32 mdio_stat, mdio_ctl;
> + unsigned long flags;
> + int ret;
> +
> + mdio_stat = xgmac_read32(®s->mdio_stat, endian);
> + mdio_stat |= MDIO_STAT_ENC;
> +
> xgmac_write32(mdio_stat, ®s->mdio_stat, endian);
>
> ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> @@ -218,13 +298,11 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
> xgmac_write32(mdio_ctl, ®s->mdio_ctl, endian);
>
> /* Set the register address */
> - if (regnum & MII_ADDR_C45) {
> - xgmac_write32(regnum & 0xffff, ®s->mdio_addr, endian);
> + xgmac_write32(regnum & 0xffff, ®s->mdio_addr, endian);
>
> - ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> - if (ret)
> - return ret;
> - }
> + ret = xgmac_wait_until_free(&bus->dev, regs, endian);
> + if (ret)
> + return ret;
>
> if (priv->has_a009885)
> /* Once the operation completes, i.e. MDIO_STAT_BSY clears, we
> @@ -326,8 +404,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> bus->name = "Freescale XGMAC MDIO Bus";
> - bus->read = xgmac_mdio_read;
> - bus->write = xgmac_mdio_write;
> + bus->read = xgmac_mdio_read_c22;
> + bus->write = xgmac_mdio_write_c22;
> + bus->read_c45 = xgmac_mdio_read_c45;
> + bus->write_c45 = xgmac_mdio_write_c45;
> bus->parent = &pdev->dev;
> bus->probe_capabilities = MDIOBUS_C22_C45;
> snprintf(bus->id, MII_BUS_ID_SIZE, "%pa", &res->start);
> --
> 2.36.0
>
Powered by blists - more mailing lists