From: "Russell King (Oracle)" <linux@armlinux•org.uk>
To: Stefan Eichenberger <eichest@gmail•com>
Cc: Andrew Lunn <andrew@lunn•ch>,
netdev@vger•kernel.org, hkallweit1@gmail•com,
francesco.dolcini@toradex•com, davem@davemloft•net,
edumazet@google•com, kuba@kernel•org, pabeni@redhat•com
Subject: Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY
Date: Thu, 13 Jul 2023 11:14:23 +0100 [thread overview]
Message-ID: <ZK/Of27YzREq+Z9V@shell.armlinux.org.uk> (raw)
In-Reply-To: <ZK/G9FMPSabQCGNk@eichest-laptop>
On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote:
> Hi Andrew,
>
> Thanks a lot for the review and all the hints, I have one short question
> below.
>
> > > +static int mv88q2xxx_read_link(struct phy_device *phydev)
> > > +{
> > > + u16 ret1, ret2;
> > > +
> > > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available,
> > > + * therefore we need to read the link status from the vendor specific
> > > + * registers.
> > > + */
> > > + if (phydev->speed == SPEED_1000) {
> > > + /* Read twice to clear the latched status */
> > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> >
> > This is generally wrong. See for example genphy_update_link() and
> > genphy_c45_read_link().
>
> Would something like this look fine to you? The issue is that I mix
> realtime data with latched data because the local and remote rx status
> is only available in realtime from what I can find in the datasheet.
> This would be for gbit, I split that up compared to the last version:
I've never really understood this kind of reaction from people. A bit
of example code gets suggested, and then the code gets sort of used
as a half-hearted template...
> static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
> {
> int ret1, ret2;
>
> /* The link state is latched low so that momentary link drops can be
> * detected. Do not double-read the status in polling mode to detect
> * such short link drops except the link was already down. In case we
> * are not polling, we always read the realtime status.
> */
> if (!phy_polling_mode(phydev) || !phydev->link) {
> ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> if (ret1 < 0)
> return ret1;
Both genphy_update_link() and genphy_c45_read_link() check here whether
the link is up, and if it is, it bypasses the second read (because
there's no point re-reading it.) I've no idea why you dropped that
optimisation.
MDIO accesses aren't the cheapest thing...
> With this we will detect link loss in polling mode and read the realtime
> status in non-polling mode. Compared to genphy_c45_read_link we will not
> immediately return "link up" in non polling mode but always do the
> second read to get the realtime link status.
Why do you think that's better? "Link" only latches low, and the
entire point of that behaviour is so that management software can
detect when the link has failed at some point since it last read
the link status.
There is only any point in re-reading the status register if on the
first read it reports that the link has failed, and only then if we
already knew that the link has failed.
If we're using interrupt mode, then we need the current link status
but that's only because of the way phylib has been written.
> If we are only interested in the link status we could also skip the
> remote and local receiver check. However, as I understand the software
> initialization guide it could be that the receivers are not ready in
> that moment.
With copper PHYs, link up status means that the link is ready to pass
data. Is this not the case with T1 PHYs?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
next prev parent reply other threads:[~2023-07-13 10:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-10 20:58 [PATCH net-next v2 0/4] Add a driver for the Marvell 88Q2110 PHY Stefan Eichenberger
2023-07-10 20:58 ` [PATCH net-next v2 1/4] net: phy: add the link modes for 1000BASE-T1 Ethernet PHY Stefan Eichenberger
2023-07-10 21:10 ` Andrew Lunn
2023-07-13 14:10 ` Stefan Eichenberger
2023-07-13 15:18 ` Andrew Lunn
2023-07-14 3:51 ` Stefan Eichenberger
2023-07-14 4:05 ` Andrew Lunn
2023-07-10 20:58 ` [PATCH net-next v2 2/4] net: phy: add registers to support 1000BASE-T1 Stefan Eichenberger
2023-07-10 20:58 ` [PATCH net-next v2 3/4] net: phy: c45: add support for 1000BASE-T1 Stefan Eichenberger
2023-07-10 21:14 ` Francesco Dolcini
2023-07-10 20:59 ` [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY Stefan Eichenberger
2023-07-10 21:20 ` Andrew Lunn
2023-07-13 9:42 ` Stefan Eichenberger
2023-07-13 10:14 ` Russell King (Oracle) [this message]
2023-07-13 11:41 ` Stefan Eichenberger
2023-07-10 21:26 ` Francesco Dolcini
2023-07-10 21:40 ` Andrew Lunn
2023-07-13 7:00 ` Russell King (Oracle)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZK/Of27YzREq+Z9V@shell.armlinux.org.uk \
--to=linux@armlinux$(echo .)org.uk \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=eichest@gmail$(echo .)com \
--cc=francesco.dolcini@toradex$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox