netdev - Re: [PATCH net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Aug 2018 17:08:12 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>
Subject: Re: [PATCH net-next 5/9] net: stmmac: Add MDIO related functions for
 XGMAC2

Hi Jose

> +static int stmmac_xgmac2_mdio_read(struct stmmac_priv *priv, int phyaddr,
> +				   int phyreg)
> +{
> +	unsigned int mii_address = priv->hw->mii.addr;
> +	unsigned int mii_data = priv->hw->mii.data;
> +	u32 tmp, addr, value = MII_XGMAC_BUSY;
> +	int data;
> +
> +	if (phyreg & MII_ADDR_C45) {
> +		addr = ((phyreg >> 16) & 0x1f) << 21;
> +		addr |= (phyaddr << 16) | (phyreg & 0xffff);

Do you need to tell the hardware this is a C45 transfer? Normally an
extra bit needs setting somewhere.

> +	} else {
> +		if (phyaddr >= 4)
> +			return -ENODEV;

Can the MDIO bus be external? If so, is there a reason why there
cannot be a PHY at addresses > 4. So maybe there is an Ethernet
switch, which needs lots of addresses? And C45 can have devices > 4
but C22 cannot?

> +		writel(~0x0, priv->ioaddr + 0x220);
> +		addr = (phyaddr << 16) | (phyreg & 0x1f);
> +	}
> +
> +	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> +		& priv->hw->mii.clk_csr_mask;
> +	value |= BIT(18);

Please add a #define for this bit.

> +	value |= MII_XGMAC_READ;
> +
> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> +		return -EBUSY;
> +
> +	writel(addr, priv->ioaddr + mii_address);
> +	writel(value, priv->ioaddr + mii_data);
> +
> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> +		return -EBUSY;
> +
> +	/* Read the data from the MII data register */
> +	data = (int)readl(priv->ioaddr + mii_data) & GENMASK(15, 0);

Is the cast needed? And why use GENMASK here, but not in all the other
places you have masks in this code?

>  /**
>   * stmmac_mdio_read
>   * @bus: points to the mii_bus structure
> @@ -59,6 +141,9 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
>  	int data;
>  	u32 value = MII_BUSY;
>  
> +	if (priv->plat->has_xgmac)
> +		return stmmac_xgmac2_mdio_read(priv, phyaddr, phyreg);

It would be cleaner to instead do this in stmmac_mdio_register() when
setting new_bus->read.

	Andrew