* v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed
@ 2024-10-03 4:45 Qingtao Cao
2024-10-03 6:53 ` Mateusz Polchlopek
0 siblings, 1 reply; 3+ messages in thread
From: Qingtao Cao @ 2024-10-03 4:45 UTC (permalink / raw)
Cc: qingtao.cao.au, Qingtao Cao, Andrew Lunn, Heiner Kallweit,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel
On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
activated, the device assumes a link-up status with existing configuration
in BMCR, avoid bringing down the fibre link in this case
Test case:
1. Two 88E151x connected with SFP, both enable autoneg, link is up with speed
1000M
2. Disable autoneg on one device and explicitly set its speed to 1000M
3. The fibre link can still up with this change, otherwise not.
Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
---
drivers/net/phy/marvell.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 9964bf3dea2f..e3a8ad8b08dd 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -195,6 +195,10 @@
#define MII_88E1510_MSCR_2 0x15
+#define MII_88E1510_FSCR2 0x1a
+#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6)
+#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5)
+
#define MII_VCT5_TX_RX_MDI0_COUPLING 0x10
#define MII_VCT5_TX_RX_MDI1_COUPLING 0x11
#define MII_VCT5_TX_RX_MDI2_COUPLING 0x12
@@ -1623,11 +1627,28 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
static int marvell_read_status_page_an(struct phy_device *phydev,
int fiber, int status)
{
+ int fscr2;
int lpa;
int err;
if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
- phydev->link = 0;
+ if (!fiber) {
+ phydev->link = 0;
+ } else {
+ fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
+ if (fscr2 > 0) {
+ if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) &&
+ (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS)) {
+ if (genphy_read_status_fixed(phydev) < 0)
+ phydev->link = 0;
+ } else {
+ phydev->link = 0;
+ }
+ } else {
+ phydev->link = 0;
+ }
+ }
+
return 0;
}
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed
2024-10-03 4:45 v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed Qingtao Cao
@ 2024-10-03 6:53 ` Mateusz Polchlopek
[not found] ` <CAPcThSGqLiv-B+VUNtJoq9NsgtCWfiposknbJySkWzLJkC78dg@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Mateusz Polchlopek @ 2024-10-03 6:53 UTC (permalink / raw)
To: Qingtao Cao
Cc: Qingtao Cao, Andrew Lunn, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On 10/3/2024 6:45 AM, Qingtao Cao wrote:
> On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
> activated, the device assumes a link-up status with existing configuration
> in BMCR, avoid bringing down the fibre link in this case
>
> Test case:
> 1. Two 88E151x connected with SFP, both enable autoneg, link is up with speed
> 1000M
checkpatch.pl complains about this line, it exceeds 75 chars allowed for
commit msg. Please adjust.
> 2. Disable autoneg on one device and explicitly set its speed to 1000M
> 3. The fibre link can still up with this change, otherwise not.
>
> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>
> ---
> drivers/net/phy/marvell.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 9964bf3dea2f..e3a8ad8b08dd 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -195,6 +195,10 @@
>
> #define MII_88E1510_MSCR_2 0x15
>
> +#define MII_88E1510_FSCR2 0x1a
Please use GENMASK_ULL for creating mask.
> +#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6)
> +#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5)
> +
> #define MII_VCT5_TX_RX_MDI0_COUPLING 0x10
> #define MII_VCT5_TX_RX_MDI1_COUPLING 0x11
> #define MII_VCT5_TX_RX_MDI2_COUPLING 0x12
> @@ -1623,11 +1627,28 @@ static void fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
> static int marvell_read_status_page_an(struct phy_device *phydev,
> int fiber, int status)
> {
> + int fscr2;
> int lpa;
> int err;
>
> if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> - phydev->link = 0;
> + if (!fiber) {
> + phydev->link = 0;
> + } else {
> + fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
> + if (fscr2 > 0) {
> + if ((fscr2 & MII_88E1510_FSCR2_BYPASS_ENABLE) &&
> + (fscr2 & MII_88E1510_FSCR2_BYPASS_STATUS)) {
> + if (genphy_read_status_fixed(phydev) < 0)
> + phydev->link = 0;
> + } else {
> + phydev->link = 0;
> + }
> + } else {
> + phydev->link = 0;
> + }
> + }
> +
> return 0;
> }
>
So many levels of indentation... Couldn't it be merged somehow? I do not
know, maybe create local variable, store the current state of
phydev->link, then set phydev->link = 0 and restore it from local
variable only if (fiber && fscr2 > 0 && (fscr2 &
MII_88E1510_FSCR2_BYPASS_ENABLE) && (fscr2 &
MII_88E1510_FSCR2_BYPASS_STATUS) && genphy_read_status_fixed(phydev) >=0
) ...
or other way? Now you have 5 (!) levels of indentation and almost
everywhere you just set phydev->link to 0 depends on the condition.
BTW. We put "v2" inside the tag in the topic and specify the tree, so
instead of:
v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when
autoneg is bypassed
it should be:
[PATCH net-next v2 1/1] net: phy: marvell: avoid bringing down fibre
link when autoneg is bypassed
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed
[not found] ` <CAPcThSGqLiv-B+VUNtJoq9NsgtCWfiposknbJySkWzLJkC78dg@mail.gmail.com>
@ 2024-10-03 7:36 ` Mateusz Polchlopek
0 siblings, 0 replies; 3+ messages in thread
From: Mateusz Polchlopek @ 2024-10-03 7:36 UTC (permalink / raw)
To: Qingtao Cao
Cc: Qingtao Cao, Andrew Lunn, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
On 10/3/2024 9:14 AM, Qingtao Cao wrote:
> Hi Mateusz,
>
> Please see my inline relies.
>
> On Thu, Oct 3, 2024 at 4:54 PM Mateusz Polchlopek
> <mateusz.polchlopek@intel.com <mailto:mateusz.polchlopek@intel.com>> wrote:
>
>
> On 10/3/2024 6:45 AM, Qingtao Cao wrote:
> > On 88E151x the SGMII autoneg bypass mode defaults to be enabled.
> When it is
> > activated, the device assumes a link-up status with existing
> configuration
> > in BMCR, avoid bringing down the fibre link in this case
> >
> > Test case:
> > 1. Two 88E151x connected with SFP, both enable autoneg, link is
> up with speed
> > 1000M
>
> checkpatch.pl <http://checkpatch.pl> complains about this line, it
> exceeds 75 chars allowed for
> commit msg. Please adjust.
>
> > 2. Disable autoneg on one device and explicitly set its speed to
> 1000M
> > 3. The fibre link can still up with this change, otherwise not.
> >
> > Signed-off-by: Qingtao Cao <qingtao.cao@digi.com
> <mailto:qingtao.cao@digi.com>>
> > ---
> > drivers/net/phy/marvell.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 9964bf3dea2f..e3a8ad8b08dd 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -195,6 +195,10 @@
> >
> > #define MII_88E1510_MSCR_2 0x15
> >
> > +#define MII_88E1510_FSCR2 0x1a
>
> Please use GENMASK_ULL for creating mask.
>
>
> The serial interface autoneg bypass mode and active status are just two
> separate bit in the FSCR2 register, instead of complicated masks, so I
> think BIT() serves them right.
Ach, sorry, the GENMASK_ULL usage is not applicable here.
>
> > +#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6)
> > +#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5)
> > +
> > #define MII_VCT5_TX_RX_MDI0_COUPLING 0x10
> > #define MII_VCT5_TX_RX_MDI1_COUPLING 0x11
> > #define MII_VCT5_TX_RX_MDI2_COUPLING 0x12
> > @@ -1623,11 +1627,28 @@ static void
> fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
> > static int marvell_read_status_page_an(struct phy_device *phydev,
> > int fiber, int status)
> > {
> > + int fscr2;
> > int lpa;
> > int err;
> >
> > if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> > - phydev->link = 0;
> > + if (!fiber) {
> > + phydev->link = 0;
> > + } else {
> > + fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
> > + if (fscr2 > 0) {
> > + if ((fscr2 &
> MII_88E1510_FSCR2_BYPASS_ENABLE) &&
> > + (fscr2 &
> MII_88E1510_FSCR2_BYPASS_STATUS)) {
> > + if
> (genphy_read_status_fixed(phydev) < 0)
> > + phydev->link = 0;
> > + } else {
> > + phydev->link = 0;
> > + }
> > + } else {
> > + phydev->link = 0;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
>
> So many levels of indentation... Couldn't it be merged somehow? I do not
> know, maybe create local variable, store the current state of
> phydev->link, then set phydev->link = 0 and restore it from local
> variable only if (fiber && fscr2 > 0 && (fscr2 &
> MII_88E1510_FSCR2_BYPASS_ENABLE) && (fscr2 &
> MII_88E1510_FSCR2_BYPASS_STATUS) && genphy_read_status_fixed(phydev)
> >=0
> ) ...
>
> or other way? Now you have 5 (!) levels of indentation and almost
> everywhere you just set phydev->link to 0 depends on the condition.
>
>
> Thanks for the feedback, please check the v3 patch just sent.
>
I will take a look but a general rule is to wait a little bit longer (at
least two working days?) before sending next version of patch. It is
mainly to give a chance for taking a look by others. Thanks
>
> BTW. We put "v2" inside the tag in the topic and specify the tree, so
> instead of:
> v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when
> autoneg is bypassed
>
> it should be:
> [PATCH net-next v2 1/1] net: phy: marvell: avoid bringing down fibre
> link when autoneg is bypassed
>
>
> Thanks again, also improved in the v3 patch header.
>
> Warm regards,
> Qingtao
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-03 7:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-03 4:45 v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed Qingtao Cao
2024-10-03 6:53 ` Mateusz Polchlopek
[not found] ` <CAPcThSGqLiv-B+VUNtJoq9NsgtCWfiposknbJySkWzLJkC78dg@mail.gmail.com>
2024-10-03 7:36 ` Mateusz Polchlopek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®