On 2/13/24 14:03, Pawel Dembicki wrote:
> This patch replaces the adjust_link api with the phylink apis that provide
> equivalent functionality.
>
> The remaining functionality from the adjust_link is now covered in the
> phylink_mac_link_* and phylink_mac_config.
>
> Removes:
> .adjust_link
> Adds:
> .phylink_mac_config
> .phylink_mac_link_up
> .phylink_mac_link_down
The implementation of phylink_mac_link_down() strictly mimics what had
been done by adjust_link() in the phydev->link == 0 case, but it really
makes me wonder whether some bits do not logically belong to
phylink_mac_link_up(), like "Accept packets again" for instance.
Are we certain there was not an assumption before that we would get
adjust_link() called first with phydev->link = 0, and then phydev->link
=1 and that this specific sequence would program things just the way we
want?
--
Florian
śr., 14 lut 2024 o 00:19 Florian Fainelli <[email protected]> napisał(a):
>
> On 2/13/24 14:03, Pawel Dembicki wrote:
> > This patch replaces the adjust_link api with the phylink apis that provide
> > equivalent functionality.
> >
> > The remaining functionality from the adjust_link is now covered in the
> > phylink_mac_link_* and phylink_mac_config.
> >
> > Removes:
> > .adjust_link
> > Adds:
> > .phylink_mac_config
> > .phylink_mac_link_up
> > .phylink_mac_link_down
>
> The implementation of phylink_mac_link_down() strictly mimics what had
> been done by adjust_link() in the phydev->link == 0 case, but it really
> makes me wonder whether some bits do not logically belong to
> phylink_mac_link_up(), like "Accept packets again" for instance.
>
> Are we certain there was not an assumption before that we would get
> adjust_link() called first with phydev->link = 0, and then phydev->link
> =1 and that this specific sequence would program things just the way we
> want?
Yes, it was the simplest conversion possible, without any improvements.
Some part is implementation of datasheet (description of ARBEMPTY register):
/* Discard packets */
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_ARBDISC, BIT(port), BIT(port));
/* Wait until queue is empty */
ret = read_poll_timeout(vsc73xx_read, err, err < 0 || (val & BIT(port)),
1000, 10000, false, vsc, VSC73XX_BLOCK_ARBITER,
0, VSC73XX_ARBEMPTY, &val);
if (ret)
dev_err(vsc->dev,
"timeout waiting for block arbiter\n");
else if (err < 0)
dev_err(vsc->dev, "error reading arbiter\n");
/* Put this port into reset */
vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
VSC73XX_MAC_CFG_RESET);
I agree that VSC73XX_ARBDISC should be moved to phylink_mac_link_up.
Other things could be optimised and it needs more care. (eg. This
implementation doesn't disable phy when the interface goes down.) I
plan to tweak it after the driver becomes usable. Please let me know
if it should be fixed in this patch.
--
Best Regards,
Pawel Dembicki
On Wed, Feb 14, 2024 at 01:56:10PM +0100, Paweł Dembicki wrote:
> śr., 14 lut 2024 o 00:19 Florian Fainelli <[email protected]> napisał(a):
> >
> > On 2/13/24 14:03, Pawel Dembicki wrote:
> > > This patch replaces the adjust_link api with the phylink apis that provide
> > > equivalent functionality.
> > >
> > > The remaining functionality from the adjust_link is now covered in the
> > > phylink_mac_link_* and phylink_mac_config.
> > >
> > > Removes:
> > > .adjust_link
> > > Adds:
> > > .phylink_mac_config
> > > .phylink_mac_link_up
> > > .phylink_mac_link_down
> >
> > The implementation of phylink_mac_link_down() strictly mimics what had
> > been done by adjust_link() in the phydev->link == 0 case, but it really
> > makes me wonder whether some bits do not logically belong to
> > phylink_mac_link_up(), like "Accept packets again" for instance.
> >
> > Are we certain there was not an assumption before that we would get
> > adjust_link() called first with phydev->link = 0, and then phydev->link
> > =1 and that this specific sequence would program things just the way we
> > want?
>
> Yes, it was the simplest conversion possible, without any improvements.
>
> Some part is implementation of datasheet (description of ARBEMPTY register):
>
> /* Discard packets */
> vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> VSC73XX_ARBDISC, BIT(port), BIT(port));
>
> /* Wait until queue is empty */
> ret = read_poll_timeout(vsc73xx_read, err, err < 0 || (val & BIT(port)),
> 1000, 10000, false, vsc, VSC73XX_BLOCK_ARBITER,
> 0, VSC73XX_ARBEMPTY, &val);
> if (ret)
> dev_err(vsc->dev,
> "timeout waiting for block arbiter\n");
> else if (err < 0)
> dev_err(vsc->dev, "error reading arbiter\n");
>
> /* Put this port into reset */
> vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> VSC73XX_MAC_CFG_RESET);
>
>
> I agree that VSC73XX_ARBDISC should be moved to phylink_mac_link_up.
FWIW, ocelot_phylink_mac_link_down() also calls ocelot_port_flush()
which is more or less the same procedure for a different piece of hw.
By re-reading the commit message of eb4733d7cffc ("net: dsa: felix:
implement port flushing on .phylink_mac_link_down"), I can find a good
reason to flush the port on link down and not on link up. With flow
control enabled, packets would remain in the queue system until there's
link again if not flushed there, otherwise.
Paweł, maybe it is simply the case that you should move the procedure
from the datasheet into a more clearly named sub-function?
> Other things could be optimised and it needs more care. (eg. This
> implementation doesn't disable phy when the interface goes down.) I
> plan to tweak it after the driver becomes usable. Please let me know
> if it should be fixed in this patch.
What do you mean by disabling the PHY when the interface goes down,
exactly? Down as in administratively down, aka "ip link set swp0 down",
not when the link drops?
That's a thing for the PHY driver to handle, by implementing .suspend()
and .resume(), I guess?
What driver do the internal PHYs use?