On 4/12/19 8:57 PM, Heiner Kallweit wrote:
On 12.04.2019 01:01, Vladimir Oltean wrote:
With Heiner's recent patch "b6163f194c69 net: phy: improve
genphy_read_status", the phydev->speed is now initialized by default to
SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
is not correct to call genphy_config_init() and genphy_read_status() for
a fixed PHY.
What do you mean with "it is not correct"? Whether the calls are always
needed may be a valid question, but it's not forbidden to use these calls
with a fixed PHY. Actually in phylib polling mode genphy_read_status is
called every second also for a fixed PHY. swphy emulates all relevant
PHY registers.
This dates back all the way to "39b0c705195e net: dsa: Allow
configuration of CPU & DSA port speeds/duplex" (discussion thread:
https://www.spinics.net/lists/netdev/msg340862.html).
I don't seem to understand why these calls were necessary back then, but
removing these calls seemingly has no impact now apart from preventing
the phydev->speed that was set in of_phy_register_fixed_link() from
getting overwritten.
I tend to agree with the change itself, but not with the justification.
For genphy_config_init I'd rather say:
genphy_config_init removes invalid modes based on the abilities read
from the chip. But as we emulate all registers anyway, this doesn't
provide any benefit for a swphy.
Signed-off-by: Vladimir Oltean <olte...@gmail.com>
---
net/dsa/port.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 87769cb38c31..481412c892a7 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -485,9 +485,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port
*dp)
mode = PHY_INTERFACE_MODE_NA;
phydev->interface = mode;
- genphy_config_init(phydev);
- genphy_read_status(phydev);
-
if (ds->ops->adjust_link)
ds->ops->adjust_link(ds, port, phydev);
Hi,
I'd like to resend this, but I'm actually thinking of a nicer way to
deal with the whole situation.
Would people be interested in making phylib/phylink decoupled from the
phydev->attached_dev? The PHY state machine could be made to simply call
a notifier block (similar to how switchdev works) registered by
interested parties (MAC driver).
To keep API compatibility (phylink_create), this notifier block could be
put inside the net_device structure and the PHY state machine would call
it. But a new phylink_create_raw could be added, which passes only the
notifier block with no net_device. The CPU port and DSA ports would be
immediate and obvious users of this (since they don't register net
devices), but there are some other out-of-tree Ethernet drivers out
there that have strange workarounds (create a net device that they don't
register) to have the PHY driver do its work.
Wondering what's your opinion on this and if it would be worth exploring.
Thanks,
-Vladimir
