Re: [PATCH 2/2] net: fec: Create device link between phy dev and mac dev

[Wang, Xiaolei]

On 11/17/2022 8:21 AM, Florian Fainelli wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender 
> and know the content is safe.
>
> On 11/16/22 15:57, Andrew Lunn wrote:
> > On Wed, Nov 16, 2022 at 03:27:39PM -0800, Florian Fainelli wrote:
> > > On 11/16/22 07:07, Andrew Lunn wrote:
> > > > On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
> > > > > On imx6sx, there are two fec interfaces, but the external
> > > > > phys can only be configured by fec0 mii_bus. That means
> > > > > the fec1 can't work independently, it only work when the
> > > > > fec0 is active. It is alright in the normal boot since the
> > > > > fec0 will be probed first. But then the fec0 maybe moved
> > > > > behind of fec1 in the dpm_list due to various device link.
> > >
> > > Humm, but if FEC1 depends upon its PHY to be available by the FEC0 
> > > MDIO bus
> > > provider, then surely we will need to make sure that FEC0's MDIO bus is
> > > always functional, and that includes surviving re-ordering as well 
> > > as any
> > > sort of run-time power management that can occur.
> >
> > Runtime PM is solved for the FECs MDIO bus, because there are switches
> > hanging off it, which have their own life cycle independent of the
> > MAC. This is something i had to fix many moons ago, when the FEC would
> > power off the MDIO bus when the interface is admin down, stopping
> > access to the switch. The mdio read and write functions now do run
> > time pm get and put as needed.
> >
> > I've never done suspend/resume with a switch, it is not something
> > needed in the use cases i've covered.
>
> All of the systems with integrated I worked on had to support
> suspend/resume both with HW maintaining the state, and with HW losing
> the state because of being powered off. The whole thing is IMHO still
> not quite well supported upstream if you have applied some configuration
> more complicated than a bridge or standalone ports. Anyway, this is a
> topic for another 10 years to come :)
>
> > > > > So in system suspend and resume, we would get the following
> > > > > warning when configuring the external phy of fec1 via the
> > > > > fec0 mii_bus due to the inactive of fec0. In order to fix
> > > > > this issue, we create a device link between phy dev and fec0.
> > > > > This will make sure that fec0 is always active when fec1
> > > > > is in active mode.
> > >
> > > Still not clear to me how the proposed fix works, let alone how it 
> > > does not
> > > leak device links since there is no device_link_del(), also you are 
> > > going to
> > > be creating guaranteed regressions by putting that change in the PHY
> > > library.
> >
> > The reference leak is bad, but i think phylib is the correct place to
> > fix this general issue. It is not specific to the FEC. There are other
> > boards with dual MAC SoCs and they save a couple of pins by putting
> > both PHYs on one MDIO bus. Having the link should help better
> > represent the device tree so that suspend/resume can do stuff in the
> > right order.
>
> My concern is that we already have had a hard time solving the proper
> suspend/resume sequence whether the MAC suspends the PHY or the MDIO bus
> suspends the PHY and throwing device links will either change the
> ordering in subtle ways, or hopefully just provide the same piece of
> information we already have via mac_managed_pm.
>
> It seems like in Xiaolei's case, the MDIO bus should suspend the PHY and
> that ought to take care of all dependencies, one would think.

Hi

mac_managed_pm solves the soft reset triggered during aeg. If you modify 
it back to MDIO bus to suspend phy, you still need to solve the problem 
of auto-negotiation,

thanks

xiaolei

> --
> Florian