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

[Florian Fainelli <f.fainelli@gmail.com>]

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.
-- 
Florian