Hi Florian,

On Fri, Mar 01, 2019 at 07:08:56PM -0800, Florian Fainelli wrote:

On 3/1/2019 7:07 AM, Antoine Tenart wrote:

quoted

On Fri, Mar 01, 2019 at 03:19:53PM +0100, Andrew Lunn wrote:

quoted

On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:

quoted

When the Marvell 10G PHYs are set out of reset, the LPOWER bit is set
depending on an hardware configuration choice. We also do not know what
is the PHY state at boot time. Hence, set the PHY in low power by
default when this driver probes.

Florian did some work for c22 PHYs so that the existing link state
could be used at boot. So for example, the bootloader configured the
PHY up and it got link, there is no need to down/up the PHY when linux
takes control. The networking comes up faster that way.

Can this work for this PHY?

This use case (the bootloader configures the PHY, Linux boots and sets
an interface using this PHY up) would work, and is what's happening in
some situations right now (the 3310 reset is never asserted prior to
this series).

But consider this case (let's say we use a 10G link):

  ----------------               ----------------
  |    Board 1   |               |    Board 2   |
  | MAC — 3310 — | — SFP cable — | — 3310 — MAC |
  ----------------               ----------------

Board 1: The userspace do not set the interface up. The MAC is in reset
         (default state during the MAC driver probe), the PHY was
	 configured by the bootloader.
Board 2: The userspace set the interface up. The MAC is configured, the
         PHY is configured as well.

The two PHY's PCS will establish a link and report it as being up. In
this case, phylink's AN mode is MLO_AN_PHY and thus will report the
overall link as being the PHY's link status: up.

My understanding is that the issue arises because the PHYs were never
set in reset, or low power, and thus act as if the user wanted the port
to be up. As the default behaviour for networking ports is to be down at
boot, I thought to set the PHY as well in a default low power state.

The policy you are creating here for the marvell10g driver is entirely
applicable to any PHY <=> PHY configuration where either of the two
software agents on Board 1 or Board 2 has not had a chance to bring-up
its bootloader/OS/applications to control the PHY.

Right.

A number of PHYs come up fully on (or in isolate or super isolate mode)
and will AN with their link partner if connected. For some people it's a
feature, for some it is a waste of power. I don't necessarily have an
issue with your patch per-se, but it does create an one off behavior
that other PHY drivers may not follow.

I agree having a per-driver behaviour is not something we want. As I
understand it, there is no behaviour enforced currently regarding this
matter. I agree both cases have their pros and cons:
- It's weird to have an interface reporting being UP when it's not
  really.
- Having the link come up faster can be a feature.

I have some questions then:
- Do you think calling suspend() in the core when probing a PHY driver
  would work for all PHYs?
- Would a new Kconfig option selecting the default behaviour at boot
  time be a solution?
- Or this is a WONTFIX kind of (small) issues? :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com