| Message ID | 20190411230139.13160-1-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: fixed-link interface is reporting SPEED_UNKNOWN | expand |
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 <olteanv@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); > >
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 <olteanv@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
On May 5, 2019 3:00:49 PM PDT, Vladimir Oltean <olteanv@gmail.com> wrote: >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 <olteanv@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. If you have patches for that idea, I would be glad to take a look. The current way of supporting DSA and CPU ports in DSA is now starting to show its limitations and we are not able to properly make use of PHYLINK at all for those ports. For PHYLIB (outside of PHYLINK), I would not want the decoupling to lead to e.g.: supporting Ethernet switches with only a single net_device instance and managing all the ports using the PHYLIB notifier, because that would bypass the model that DSA offers so we could miss opportunities to fix it, if needed.
On Mon, May 06, 2019 at 01:00:49AM +0300, Vladimir Oltean wrote: > 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. As Florian said, if you have patches, please post them and we will consider them. But i think we also need to take a step back and consider the big picture. There has been a lot of work recently to support multi-G PHYs. It is clear we soon need to make changes to fixed-link. It only supports up to 1G. But we have use cases where we need multi-G fixed links. We could also consider making the tie to the MAC much stronger. We have been encouraging MAC driver writers to make use of the ndev->phylib pointer. We could even enforce that, and use container_of() to determine the MAC associated to a PHY. Andrew
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);
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. 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. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- net/dsa/port.c | 3 --- 1 file changed, 3 deletions(-)