Message-ID: <92332a2e-8e87-567d-7b4c-6ca779c866aa@gmail.com>
Date:   Sun, 26 Feb 2023 17:42:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
To:     Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-renesas-soc@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>, Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230223070519.2211-1-wsa+renesas@sang-engineering.com>
Content-Language: en-US
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [REGRESSION PATCH RFC] net: phy: don't resume PHY via MDIO when
 iface is not up
In-Reply-To: <20230223070519.2211-1-wsa+renesas@sang-engineering.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23.02.2023 08:05, Wolfram Sang wrote:
> TLDR; Commit 96fb2077a517 ("net: phy: consider that suspend2ram may cut
> off PHY power") caused regressions for us when resuming an interface
> which is not up. It turns out the problem is another one, the above
> commit only makes it visible. The attached patch is probably not the
> right fix, but at least is proving my assumptions AFAICS.
> 
> Setup: I used Renesas boards for my tests, namely Salvator-XS and Ebisu.
> They both use RAVB driver (drivers/net/ethernet/renesas/ravb_main.c) and
> a Micrel KSZ9031 PHY (drivers/net/phy/micrel.c). I think the problems
> are generic, though.
> 
> Long text: After the above commit, we could see various resume failures
> on our boards, like timeouts when resetting the MDIO bus, or warning
> about skew values in non-RGMII mode, although RGMII was used. All of
> these happened, because phy_init_hw() was now called in
> mdio_bus_phy_resume() which wasn't the case before. But the interface
> was not up yet, e.g. phydev->interface was still the default and not
> RGMII, so the initialization didn't work properly. phy_attach_direct()
> pays attention to this:
> 
> 1504         /* Do initial configuration here, now that
> 1505          * we have certain key parameters
> 1506          * (dev_flags and interface)
> 1507          */
> 1508         err = phy_init_hw(phydev);
> 
> But phy_init_hw() doesn't if the interface is not up, AFAICS.
> 
> This may be a problem in itself, but I then wondered why
> mdio_bus_phy_resume() gets called anyhow because the RAVB driver sets
> 'phydev->mac_managed_pm = true' so once the interface is up
> mdio_bus_phy_resume() never gets called. But again, the interface was
> not up yet, so mac_managed_pm was not set yet.
> 
Setting phydev->mac_managed_pm in the open() callback is too late.
It should be set as soon as the phydev is created. That's in
ravb_mdio_init() after the call to of_mdiobus_register().

It should be possible to get the phydev with:
pn = of_parse_phandle(np, "phy-handle", 0);
phy = of_phy_find_device(pn);


> So, in my quest to avoid mdio_bus_phy_resume() being called, I tried
> this patch declaring the PHY being in suspend state when being probed.
> The KSZ9031 has a soft_reset() callback, so phy_init_hw() will reset the
> suspended flag when the PHY is attached. It works for me(tm),
> suspend/resume now works independently of the interface being up or not.
> 
> I don't think this is the proper solution, though. It will e.g. fail if
> some PHY is not using the soft_reset() callback. And I am missing the
> experience in this subsystem to decide if we can clear the resume flag
> in phy_init_hw() unconditionally. My gut feeling is that we can't.
> 
> So, this patch mostly demonstrates the issues we have and the things I
> found out. I'd be happy if someone could point me to a proper solution,
> or more information that I am missing here. Thank you in advance and
> happy hacking!
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/net/phy/phy_device.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8cff61dbc4b5..5cbb471700a8 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3108,6 +3108,7 @@ static int phy_probe(struct device *dev)
>  
>  	/* Set the state to READY by default */
>  	phydev->state = PHY_READY;
> +	phydev->suspended = 1;
>  
>  out:
>  	/* Assert the reset signal */