netdev - RE: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531

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: <BYAPR12MB47738A21AE76E4CDEE28DFFF9E659@BYAPR12MB4773.namprd12.prod.outlook.com>
Date:   Wed, 26 Apr 2023 12:21:47 +0000
From:   "Katakam, Harini" <harini.katakam@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "wsa+renesas@...g-engineering.com" <wsa+renesas@...g-engineering.com>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "simon.horman@...igine.com" <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "harinikatakamlinux@...il.com" <harinikatakamlinux@...il.com>,
        "Simek, Michal" <michal.simek@....com>,
        "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>
Subject: RE: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
 with RGMII tuning

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> Sent: Wednesday, April 26, 2023 4:48 PM
> To: Katakam, Harini <harini.katakam@....com>
> Cc: robh+dt@...nel.org; andrew@...n.ch; hkallweit1@...il.com;
> linux@...linux.org.uk; davem@...emloft.net; kuba@...nel.org;
> edumazet@...gle.com; pabeni@...hat.com; wsa+renesas@...g-
> engineering.com; krzysztof.kozlowski+dt@...aro.org;
> simon.horman@...igine.com; netdev@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
> harinikatakamlinux@...il.com; Simek, Michal <michal.simek@....com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
> 
> On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> > From: Harini Katakam <harini.katakam@...inx.com>
> >
> > Add support for VSC8531_02 (Rev 2) device.
> > Add support for optional RGMII RX and TX delay tuning via devicetree.
> > The hierarchy is:
> > - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> > - Retain the default 2ns delay when RGMII tuning is set and DT delay
> > property is NOT specified.
> > - Use the DT delay value when RGMII tuning is set and a DT delay
> > property is specified.
> >
> > Signed-off-by: Harini Katakam <harini.katakam@....com>
> > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@...inx.com>
> > Signed-off-by: Michal Simek <michal.simek@...inx.com>
> > ---
<snip>
> > @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct
> phy_device *phydev, u32 rgmii_cntl,
> >
> >  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> >  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > -		reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_rx_delay_pos;
> > +		reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
> >  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> >  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > -		reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_tx_delay_pos;
> > +		reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
> >
> >  	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> >  			      rgmii_cntl,
> > @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device
> *phydev)
> >  {
> >  	int rc, i, phy_id;
> >  	struct vsc8531_private *vsc8531 = phydev->priv;
> > +	struct device_node *of_node = phydev->mdio.dev.of_node;
> > +
> > +	vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> > +	rc = of_property_read_u32(of_node, "mscc,rx-delay",
> > +				  &vsc8531->rx_delay);
> > +
> > +	vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
> > +	rc = of_property_read_u32(of_node, "mscc,tx-delay",
> > +				  &vsc8531->tx_delay);
> 
> Since the dt-bindings document says "If this property is present then
> the PHY applies the RX|TX delay", then I guess the precedence as applied
> by vsc85xx_rgmii_set_skews() should be different. The RX delays should
> be applied based on rx-internal-delay-ps (if present) regardless of
> phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid phy_get_internal_delay
> or rgmii-id modes. Similar for tx.

Thanks for the review.
The intention is to have the following precedence (I'll rephrase the commit if required)
-> If phy-mode is rgmii, current behavior persists for all devices
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices
(i.e. delay of RGMII_CLK_DELAY_2_0_NS)
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps
is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode)

I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode
in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like
the above is allowed. Please do let me know otherwise.

I will re-spin the series using phy_get_internal_delay.

Regards,
Harini