netdev - Re: [PATCH V4 net-next 5/7] net: bcmgenet: Refactor register access in bcmgenet_mii

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] [day] [month] [year] [list]

Date:   Mon, 11 Nov 2019 10:19:53 -0800
From:   Doug Berger <opendmb@...il.com>
To:     Stefan Wahren <wahrenst@....net>,
        Matthias Brugger <matthias.bgg@...nel.org>,
        Matthias Brugger <mbrugger@...e.com>,
        "David S . Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>
Cc:     Eric Anholt <eric@...olt.net>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH V4 net-next 5/7] net: bcmgenet: Refactor register access
 in bcmgenet_mii_config

On 11/10/19 10:55 PM, Stefan Wahren wrote:
> The register access in bcmgenet_mii_config() is a little bit opaque and
> not easy to extend. In preparation for the missing RGMII PHY modes
> move all the phy name assignments into the switch statement and the
> port register access to the end of the function. This make the code easier
> to read and extend.
> 
> Signed-off-by: Stefan Wahren <wahrenst@....net>
> =2D--
>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 35 +++++++++++++----------=
> -----
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/et=
> hernet/broadcom/genet/bcmmii.c
> index 6f291ee..611a6c0 100644
> =2D-- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -213,11 +213,10 @@ int bcmgenet_mii_config(struct net_device *dev, bool=
>  init)
>  		udelay(2);
>  	}
> 
> -	priv->ext_phy =3D !priv->internal_phy &&
> -			(priv->phy_interface !=3D PHY_INTERFACE_MODE_MOCA);
> -
>  	switch (priv->phy_interface) {
>  	case PHY_INTERFACE_MODE_INTERNAL:
> +		phy_name =3D "internal PHY";
> +		/* fall through */
>  	case PHY_INTERFACE_MODE_MOCA:
>  		/* Irrespective of the actually configured PHY speed (100 or
>  		 * 1000) GENETv4 only has an internal GPHY so we will just end
> @@ -229,11 +228,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool =
> init)
>  		else
>  			port_ctrl =3D PORT_MODE_INT_EPHY;
> 
> -		bcmgenet_sys_writel(priv, port_ctrl, SYS_PORT_CTRL);
> -
> -		if (priv->internal_phy) {
> -			phy_name =3D "internal PHY";
> -		} else if (priv->phy_interface =3D=3D PHY_INTERFACE_MODE_MOCA) {
> +		if (!phy_name) {
>  			phy_name =3D "MoCA";
>  			bcmgenet_moca_phy_setup(priv);
>  		}
> @@ -242,8 +237,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool i=
> nit)
>  	case PHY_INTERFACE_MODE_MII:
>  		phy_name =3D "external MII";
>  		phy_set_max_speed(phydev, SPEED_100);
> -		bcmgenet_sys_writel(priv,
> -				    PORT_MODE_EXT_EPHY, SYS_PORT_CTRL);
> +		port_ctrl =3D PORT_MODE_EXT_EPHY;
>  		/* Restore the MII PHY after isolation */
>  		if (bmcr >=3D 0)
>  			phy_write(phydev, MII_BMCR, bmcr);
This changes the behavior. The SYS_PORT_CTRL register must be written to
the correct setting (i.e PORT_MODE_EXT_EPHY) before bringing the PHY
back out of isolate mode.

> @@ -261,31 +255,34 @@ int bcmgenet_mii_config(struct net_device *dev, bool=
>  init)
>  			port_ctrl =3D PORT_MODE_EXT_RVMII_50;
>  		else
>  			port_ctrl =3D PORT_MODE_EXT_RVMII_25;
> -		bcmgenet_sys_writel(priv, port_ctrl, SYS_PORT_CTRL);
>  		break;
> 
>  	case PHY_INTERFACE_MODE_RGMII:
>  		/* RGMII_NO_ID: TXC transitions at the same time as TXD
>  		 *		(requires PCB or receiver-side delay)
> -		 * RGMII:	Add 2ns delay on TXC (90 degree shift)
>  		 *
>  		 * ID is implicitly disabled for 100Mbps (RG)MII operation.
>  		 */
> +		phy_name =3D "external RGMII (no delay)";
>  		id_mode_dis =3D BIT(16);
> -		/* fall through */
> +		port_ctrl =3D PORT_MODE_EXT_GPHY;
> +		break;
> +
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		if (id_mode_dis)
> -			phy_name =3D "external RGMII (no delay)";
> -		else
> -			phy_name =3D "external RGMII (TX delay)";
> -		bcmgenet_sys_writel(priv,
> -				    PORT_MODE_EXT_GPHY, SYS_PORT_CTRL);
> +		/* RGMII_TXID:	Add 2ns delay on TXC (90 degree shift) */
> +		phy_name =3D "external RGMII (TX delay)";
> +		port_ctrl =3D PORT_MODE_EXT_GPHY;
>  		break;
>  	default:
>  		dev_err(kdev, "unknown phy mode: %d\n", priv->phy_interface);
>  		return -EINVAL;
>  	}
> 
> +	bcmgenet_sys_writel(priv, port_ctrl, SYS_PORT_CTRL);
You can probably just move this clause here:
	/* Restore the MII PHY after isolation */
	if (bmcr >= 0)
		phy_write(phydev, MII_BMCR, bmcr);
to correct it.

> +
> +	priv->ext_phy =3D !priv->internal_phy &&
> +			(priv->phy_interface !=3D PHY_INTERFACE_MODE_MOCA);
> +
>  	/* This is an external PHY (xMII), so we need to enable the RGMII
>  	 * block for the interface to work
>  	 */
> =2D-
> 2.7.4
> 

Thanks,
    Doug