netdev - Re: [PATCH v2 net-next 4/9] net: dsa: microchip: add DSA support for microchip lan937x

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: <291ac605fe404b90c571f23f457f7f855eebf970.camel@microchip.com>
Date:   Mon, 26 Apr 2021 14:24:35 +0530
From:   Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
To:     Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>
CC:     <netdev@...r.kernel.org>, <robh+dt@...nel.org>,
        <UNGLinuxDriver@...rochip.com>, <hkallweit1@...il.com>,
        <linux@...linux.org.uk>, <davem@...emloft.net>, <kuba@...nel.org>,
        <linux-kernel@...r.kernel.org>, <vivien.didelot@...il.com>,
        <f.fainelli@...il.com>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 4/9] net: dsa: microchip: add DSA support
 for microchip lan937x

On Fri, 2021-04-23 at 01:28 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > > +
> > > +           lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
> > > +
> > > +           /* clear MII selection & set it based on interface later */
> > > +           data8 &= ~PORT_MII_SEL_M;
> > > +
> > > +           /* configure MAC based on p->interface */
> > > +           switch (p->interface) {
> > > +           case PHY_INTERFACE_MODE_MII:
> > > +                   lan937x_set_gbit(dev, false, &data8);
> > > +                   data8 |= PORT_MII_SEL;
> > > +                   break;
> > > +           case PHY_INTERFACE_MODE_RMII:
> > > +                   lan937x_set_gbit(dev, false, &data8);
> > > +                   data8 |= PORT_RMII_SEL;
> > > +                   break;
> > > +           default:
> > > +                   lan937x_set_gbit(dev, true, &data8);
> > > +                   data8 |= PORT_RGMII_SEL;
> > > +
> > > +                   data8 &= ~PORT_RGMII_ID_IG_ENABLE;
> > > +                   data8 &= ~PORT_RGMII_ID_EG_ENABLE;
> > > +
> > > +                   if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > +                       p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > > +                           data8 |= PORT_RGMII_ID_IG_ENABLE;
> > > +
> > > +                   if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > > +                       p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > > +                           data8 |= PORT_RGMII_ID_EG_ENABLE;
> > 
> > This is interesting. If you have an RGMII port connected to an external
> > PHY, how do you ensure that either the lan937x driver, or the PHY driver,
> > but not both, enable RGMII delays?
> 
> What generally happens is the MAC adds no delays, and the PHY acts
> upon the interface mode, inserting delays as requested.
> 
> There are a very small number of exceptions to this, for boards which
> have a PHY which cannot do delays, and the MAC can. If i remember
> correctly, this pretty much limited to one MAC vendor. In that case,
> the MAC adds delays, if the interface mode requests it, and it always
> passes PHY_INTERFACE_MODE_RGMII to the PHY so it does not add delays.
> 
> So what needs to be looked at here is what is passed to the phy
> connect call? passing p->interface is definitely wrong if the MAC is
> acting on it.
> 
> If even if the connect is correct, i would still prefer the MAC not do
> the delays, let the PHY do it, like nearly every other setup.
> 
>         Andrew
It comes here only if the port is not internal phy which means for MII
interface. As Andrew said, let the phy driver handles the delay if it has the
associated phy vendor driver, otherwise can still be added by MAC if required
(like for cpu port)?

What do you think on the following code?

	struct dsa_port *dp = dsa_to_port(dev->ds, port);
	struct phy_device *phy_dev = dp->slave->phydev;
	.
	.
	.

    	if (!phydev || phy_driver_is_genphy(phydev)) {
		/*Add RGMII internal delay*/
    	}