linux-kernel - RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

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]

Message-ID: <D6759987A7968C4889FDA6FA91D5CBC814738B36@PGSMSX103.gar.corp.intel.com>
Date:   Fri, 5 Jul 2019 03:02:49 +0000
From:   "Voon, Weifeng" <weifeng.voon@...el.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "David S. Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jose Abreu <joabreu@...opsys.com>,
        "Giuseppe Cavallaro" <peppe.cavallaro@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        biao huang <biao.huang@...iatek.com>,
        "Ong, Boon Leong" <boon.leong.ong@...el.com>,
        "Kweh, Hock Leong" <hock.leong.kweh@...el.com>
Subject: RE: [PATCH v1 net-next] net: stmmac: enable clause 45 mdio support

> I think there is too much passing variables around by reference than by
> value, to make this code easy to understand.
> 
> Maybe a better structure would be
> 
> static int stmmac_mdion_c45_read(struct stmmac_priv *priv, int phyaddr,
> int phyreg) {
> 
> 	unsigned int reg_shift = priv->hw->mii.reg_shift;
> 	unsigned int reg_mask = priv->hw->mii.reg_mask;
> 	u32 mii_addr_val, mii_data_val;
> 
> 	mii_addr_val = MII_GMAC4_C45E |
>                        ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift)
> & reg_mask;
>         mii_data_val = (phyreg & MII_REGADDR_C45_MASK) <<
> MII_GMAC4_REG_ADDR_SHIFT;
> 
> 	writel(mii_data_val, priv->ioaddr + priv->hw->mii_data);
> 	writel(mii_addr_val, priv->ioaddr + priv->hw->mii_addrress);
> 
> 	return (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
> }
> 
> static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> {
> 
> ...
> 	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v &
> MII_BUSY),
>  	   		      100, 10000))
>  		return -EBUSY;
> 
>       if (priv->plat->has_gmac4 && phyreg & MII_ADDR_C45)
>       	return stmmac_mdio_c45_read(priv, phyaddr, phyreg);
> 
> 	Andrew

Both c45 read/write needs to set c45 enable bit(MII_ADDR_C45) in mii_adrress
and set the register address in mii_data. Besides this, the whole programming
flow will be the same as c22. With the current design, user can easily know
that the different between c22 and c45 is just in stmmac_mdio_c45_setup(). 

If the community prefers readability, I will suggest to do the c45 setup in
both stmmac_mdio_read() and stmmac_mdio_write() 's if(C45) condition rather
than splitting into 2 new c45_read() and c45_write() functions.     

static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
{

...
	if (phyreg & MII_ADDR_C45)
       *val |= MII_GMAC4_C45E;
       *val &= ~reg_mask;
       *val |= ((phyreg >> MII_DEVADDR_C45_SHIFT) << reg_shift) & reg_mask;

       *data |= (phyreg & MII_REGADDR_C45_MASK) << MII_GMAC4_REG_ADDR_SHIFT;

Weifeng