Message-ID: <4bf812ec-f59b-6f64-b1e0-0feb54138bad@seco.com>
Date:   Mon, 21 Nov 2022 17:42:44 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

On 11/21/22 14:44, Vladimir Oltean wrote:
> Hi Sean,
> 
> On Mon, Nov 21, 2022 at 01:38:31PM -0500, Sean Anderson wrote:
>> On 11/17/22 19:01, Vladimir Oltean wrote:
>> > Compared to other solutions
>> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > 
>> > Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
>> > sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
>> > all problems, because we don't *know* that the PHY is set for in-band
>> > autoneg. For example with the VSC8514, it all depends on what the
>> > bootloader has/has not done. This solution eliminates the bootloader
>> > dependency by actually programming in-band autoneg in the VSC8514 PHY.
>> 
>> I tested this on an LS1046ARDB. Unfortunately, although the links came
>> up, the SGMII interfaces could not transfer data:
>> 
>> # dmesg | grep net6
>> [    3.846249] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
>> [    5.047334] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming false
>> [    5.073739] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
>> [    5.073749] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
>> [    5.073798] fsl_dpaa_mac 1aea000.ethernet net6: configuring for phy/sgmii link mode
>> [    5.073803] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
>> [    5.075369] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=phy/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,00000000 pause=00 link=0 an=0
>> [    5.102308] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
>> [    9.186216] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
>> [    9.186261] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx
>> 
>> I believe this is the same issue I ran into before. This is why I
>> defaulted to in-band.
> 
> Thanks for testing. Somehow it did not come to me that this kind of
> issue might happen when converting a driver that used to use ovr_an_inband
> such as dpaa1, but ok, here we are.
> 
> The problem, of course, is that the Realtek PHY driver does not report
> what the hardware supports, and we're back to trusting the device tree.
> 
> I don't think there were that many more PHYs used on NXP evaluation
> boards than the Realteks, but of course there are also customer boards
> to consider. Considering past history, it might be safer in terms of
> regressions to use ovr_an_inband, but eventually, getting regression
> reports in is going to make more PHY drivers report their capabilities,
> which will improve the situation.

Are you certain this is the cause of the issue? It's also possible that
there is some errata for the PCS which is causing the issue. I have
gotten no review/feedback from NXP regarding the phylink conversion
(aside from acks for the cleanups).

> Anyways, we can still keep dpaa1 unconverted for now, and maybe convert
> it for the next release cycle.
> 
> I also thought of a way of logically combining ovr_an_inband with
> sync_an_inband (like say that ovr_an_inband is a "soft" override, and it
> only takes place if syncing is not possible), but I'm not sure if that
> isn't in fact overkill.
>
> Could you please test the patch below? I only compile-tested it:
> 
> -----------------------------[ cut here ]-----------------------------
> From 025f8dedf10defa6d5fd10b4e3dd2a505fdbd313 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@....com>
> Date: Mon, 21 Nov 2022 21:34:20 +0200
> Subject: [PATCH] net: phy: realtek: validate SGMII in-band autoneg for
>  RTL8211FS
> 
> Sean Anderson reports that the RTL8211FS on the NXP LS1046A-RDB has
> in-band autoneg enabled, and this needs to be detectable by phylink if
> the dpaa1 driver is going to use the sync_an_inband mechanism rather
> than forcing in-band on via ovr_an_inband.
> 
> Reading through the datasheet, it seems like the SGMII Auto-Negotiation
> Advertising Register bit 11 (En_Select Link Info) might be responsible
> with this.

This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
to contain useful information for UTP mode (figure 1).

> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/phy/realtek.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 3d99fd6664d7..53e7c1a10ab4 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -24,6 +24,10 @@
>  
>  #define RTL821x_INSR				0x13
>  
> +#define RTL8211FS_SGMII_ANARSEL			0x14
> +
> +#define RTL8211FS_SGMII_ANARSEL_EN		BIT(11)
> +
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> @@ -849,6 +853,30 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
>  
> +/* RTL8211F and RTL8211FS seem to have the same PHY ID. We really only mean to
> + * run this for the S model which supports SGMII, so report unknown for
> + * everything else.
> + */
> +static int rtl8211fs_validate_an_inband(struct phy_device *phydev,
> +					phy_interface_t interface)
> +{
> +	int ret;
> +
> +	if (interface != PHY_INTERFACE_MODE_SGMII)
> +		return PHY_AN_INBAND_UNKNOWN;
> +
> +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);

That said, you have to use the "Indirect access method" to access this
register (per section 8.5). This is something like

#define RTL8211F_IAAR				0x1b
#define RTL8211F_IADR				0x1c

#define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
#define RTL8211F_IAAR_REG			GENMASK(3, 1)
#define INDIRECT_ADDRESS(page, reg) \
	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))

	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
	if (ret < 0)
		return ret;

	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
	if (ret < 0)
		return ret;

I dumped the rest of the serdes registers using this method, but I
didn't see anything interesting (all defaults). I think it would be
better to just return PHY_AN_INBAND_ON when using SGMII.

--Sean

> +	if (ret < 0)
> +		return ret;
> +
> +	phydev_err(phydev, "%s: SGMII_ANARSEL 0x%x\n", __func__, ret);
> +
> +	if (ret & RTL8211FS_SGMII_ANARSEL_EN)
> +		return PHY_AN_INBAND_ON;
> +
> +	return PHY_AN_INBAND_OFF;
> +}
> +
>  static struct phy_driver realtek_drvs[] = {
>  	{
>  		PHY_ID_MATCH_EXACT(0x00008201),
> @@ -931,6 +959,7 @@ static struct phy_driver realtek_drvs[] = {
>  		.resume		= rtl821x_resume,
>  		.read_page	= rtl821x_read_page,
>  		.write_page	= rtl821x_write_page,
> +		.validate_an_inband = rtl8211fs_validate_an_inband,
>  	}, {
>  		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
>  		.name		= "RTL8211F-VD Gigabit Ethernet",

Powered by blists - more mailing lists