[PATCH 0/3] net:phy:dp83867: add some fixes

[PATCH 0/3] net:phy:dp83867: add some fixes

[Max Uvarov]

Patch "fix speed 10 in sgmii mode" I already sent before
and the was no mutch objections. Just resending it.

Patch "increase SGMII autoneg timer duration" fixes autoneg
between mac and the phy.

And the latest "do not call config_init twice" is the logical
code clean up.

BR,
Max.

Max Uvarov (3):
  net:phy:dp83867: fix speed 10 in sgmii mode
  net:phy:dp83867: increase SGMII autoneg timer duration
  net:phy:dp83867: do not call config_init twice

 drivers/net/phy/dp83867.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/3] net:phy:dp83867: fix speed 10 in sgmii mode

[Max Uvarov]

For support 10Mps sped in SGMII mode DP83867_10M_SGMII_RATE_ADAPT bit
of DP83867_10M_SGMII_CFG register has to be cleared by software.
That does not affect speeds 100 and 1000 so can be done on init.

Signed-off-by: Max Uvarov <muvarov@gmail.com>
---
 drivers/net/phy/dp83867.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index fd35131a0c39..afd31c516cc7 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -30,6 +30,7 @@
 #define DP83867_STRAP_STS1	0x006E
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
+#define DP83867_10M_SGMII_CFG  0x016F
 
 #define DP83867_SW_RESET	BIT(15)
 #define DP83867_SW_RESTART	BIT(14)
@@ -74,6 +75,9 @@
 /* CFG4 bits */
 #define DP83867_CFG4_PORT_MIRROR_EN              BIT(0)
 
+/* 10M_SGMII_CFG bits */
+#define DP83867_10M_SGMII_RATE_ADAPT		 BIT(7)
+
 enum {
 	DP83867_PORT_MIRROING_KEEP,
 	DP83867_PORT_MIRROING_EN,
@@ -277,6 +281,24 @@ static int dp83867_config_init(struct phy_device *phydev)
 				       DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
 	}
 
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		/* For support SPEED_10 in SGMII mode
+		 * DP83867_10M_SGMII_RATE_ADAPT bit
+		 * has to be cleared by software. That
+		 * does not affect SPEED_100 and
+		 * SPEED_1000.
+		 */
+		val = phy_read_mmd(phydev, DP83867_DEVADDR,
+				   DP83867_10M_SGMII_CFG);
+		val &= ~DP83867_10M_SGMII_RATE_ADAPT;
+		ret = phy_write_mmd(phydev, DP83867_DEVADDR,
+				    DP83867_10M_SGMII_CFG, val);
+		if (ret) {
+			WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
+			return ret;
+		}
+	}
+
 	/* Enable Interrupt output INT_OE in CFG3 register */
 	if (phy_interrupt_is_valid(phydev)) {
 		val = phy_read(phydev, DP83867_CFG3);
-- 
2.17.1

[PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer duration

[Max Uvarov]

After reset SGMII Autoneg timer is set to 2us (bits 6 and 5 are 01).
That us not enough to finalize autonegatiation on some devices.
Increase this timer duration to maximum supported 16ms.

Signed-off-by: Max Uvarov <muvarov@gmail.com>
---
 drivers/net/phy/dp83867.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index afd31c516cc7..66b0a09ad094 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -297,6 +297,19 @@ static int dp83867_config_init(struct phy_device *phydev)
 			WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
 			return ret;
 		}
+
+		/* After reset SGMII Autoneg timer is set to 2us (bits 6 and 5
+		 * are 01). That us not enough to finalize autoneg on some
+		 * devices. Increase this timer duration to maximum 16ms.
+		 */
+		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
+		val &= ~(BIT(5) | BIT(6));
+		ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
+		if (ret) {
+			WARN_ONCE(1, "dp83867: error config sgmii auto-neg timer\n");
+			return ret;
+		}
+
 	}
 
 	/* Enable Interrupt output INT_OE in CFG3 register */
-- 
2.17.1

[PATCH 3/3] net:phy:dp83867: do not call config_init twice

[Max Uvarov]

Phy state machine calls _config_init just after
reset.

Signed-off-by: Max Uvarov <muvarov@gmail.com>
---
 drivers/net/phy/dp83867.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 66b0a09ad094..2984fd5ae495 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -342,7 +342,7 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 
 	usleep_range(10, 20);
 
-	return dp83867_config_init(phydev);
+	return 0;
 }
 
 static struct phy_driver dp83867_driver[] = {
-- 
2.17.1

Re: [PATCH 1/3] net:phy:dp83867: fix speed 10 in sgmii mode

[Heiner Kallweit]

On 24.05.2019 12:25, Max Uvarov wrote:
> For support 10Mps sped in SGMII mode DP83867_10M_SGMII_RATE_ADAPT bit
> of DP83867_10M_SGMII_CFG register has to be cleared by software.
> That does not affect speeds 100 and 1000 so can be done on init.
> 
> Signed-off-by: Max Uvarov <muvarov@gmail.com>
> ---
>  drivers/net/phy/dp83867.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index fd35131a0c39..afd31c516cc7 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -30,6 +30,7 @@
>  #define DP83867_STRAP_STS1	0x006E
>  #define DP83867_RGMIIDCTL	0x0086
>  #define DP83867_IO_MUX_CFG	0x0170
> +#define DP83867_10M_SGMII_CFG  0x016F
>  
>  #define DP83867_SW_RESET	BIT(15)
>  #define DP83867_SW_RESTART	BIT(14)
> @@ -74,6 +75,9 @@
>  /* CFG4 bits */
>  #define DP83867_CFG4_PORT_MIRROR_EN              BIT(0)
>  
> +/* 10M_SGMII_CFG bits */
> +#define DP83867_10M_SGMII_RATE_ADAPT		 BIT(7)
> +
>  enum {
>  	DP83867_PORT_MIRROING_KEEP,
>  	DP83867_PORT_MIRROING_EN,
> @@ -277,6 +281,24 @@ static int dp83867_config_init(struct phy_device *phydev)
>  				       DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
>  	}
>  
> +	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
> +		/* For support SPEED_10 in SGMII mode
> +		 * DP83867_10M_SGMII_RATE_ADAPT bit
> +		 * has to be cleared by software. That
> +		 * does not affect SPEED_100 and
> +		 * SPEED_1000.
> +		 */
> +		val = phy_read_mmd(phydev, DP83867_DEVADDR,
> +				   DP83867_10M_SGMII_CFG);
> +		val &= ~DP83867_10M_SGMII_RATE_ADAPT;
> +		ret = phy_write_mmd(phydev, DP83867_DEVADDR,
> +				    DP83867_10M_SGMII_CFG, val);

This could be simplified by using phy_modify_mmd().

> +		if (ret) {
> +			WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");

This error message says more or less nothing. The context is visible in the
stack trace, so you can remove the message w/o losing anything.
As we're in the config_init callback, I don't think the "ONCE" version is
needed. So you could simply use WARN_ON(1). Typically just the errno is
returned w/o additional message, so you could also simply do:
return phy_modify_mmd(phydev, ...)

> +			return ret;
> +		}
> +	}
> +
>  	/* Enable Interrupt output INT_OE in CFG3 register */
>  	if (phy_interrupt_is_valid(phydev)) {
>  		val = phy_read(phydev, DP83867_CFG3);
> 

Re: [PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer duration

[Heiner Kallweit]

On 24.05.2019 12:25, Max Uvarov wrote:
> After reset SGMII Autoneg timer is set to 2us (bits 6 and 5 are 01).
> That us not enough to finalize autonegatiation on some devices.
> Increase this timer duration to maximum supported 16ms.
> 
> Signed-off-by: Max Uvarov <muvarov@gmail.com>
> ---
>  drivers/net/phy/dp83867.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index afd31c516cc7..66b0a09ad094 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -297,6 +297,19 @@ static int dp83867_config_init(struct phy_device *phydev)
>  			WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
>  			return ret;
>  		}
> +
> +		/* After reset SGMII Autoneg timer is set to 2us (bits 6 and 5
> +		 * are 01). That us not enough to finalize autoneg on some
> +		 * devices. Increase this timer duration to maximum 16ms.
> +		 */
In the public datasheet the bits are described as reserved. However, based on
the value, I suppose it's not a timer value but the timer resolution.

> +		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
> +		val &= ~(BIT(5) | BIT(6));
> +		ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
> +		if (ret) {
> +			WARN_ONCE(1, "dp83867: error config sgmii auto-neg timer\n");
> +			return ret;

Same comment as for patch 1.

> +		}
> +
>  	}
>  
>  	/* Enable Interrupt output INT_OE in CFG3 register */
> 

Re: [PATCH 1/3] net:phy:dp83867: fix speed 10 in sgmii mode

[Maxim Uvarov]

пт, 24 мая 2019 г. в 20:24, Heiner Kallweit <hkallweit1@gmail.com>:
>
> On 24.05.2019 12:25, Max Uvarov wrote:
> > For support 10Mps sped in SGMII mode DP83867_10M_SGMII_RATE_ADAPT bit
> > of DP83867_10M_SGMII_CFG register has to be cleared by software.
> > That does not affect speeds 100 and 1000 so can be done on init.
> >
> > Signed-off-by: Max Uvarov <muvarov@gmail.com>
> > ---
> >  drivers/net/phy/dp83867.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> > index fd35131a0c39..afd31c516cc7 100644
> > --- a/drivers/net/phy/dp83867.c
> > +++ b/drivers/net/phy/dp83867.c
> > @@ -30,6 +30,7 @@
> >  #define DP83867_STRAP_STS1   0x006E
> >  #define DP83867_RGMIIDCTL    0x0086
> >  #define DP83867_IO_MUX_CFG   0x0170
> > +#define DP83867_10M_SGMII_CFG  0x016F
> >
> >  #define DP83867_SW_RESET     BIT(15)
> >  #define DP83867_SW_RESTART   BIT(14)
> > @@ -74,6 +75,9 @@
> >  /* CFG4 bits */
> >  #define DP83867_CFG4_PORT_MIRROR_EN              BIT(0)
> >
> > +/* 10M_SGMII_CFG bits */
> > +#define DP83867_10M_SGMII_RATE_ADAPT          BIT(7)
> > +
> >  enum {
> >       DP83867_PORT_MIRROING_KEEP,
> >       DP83867_PORT_MIRROING_EN,
> > @@ -277,6 +281,24 @@ static int dp83867_config_init(struct phy_device *phydev)
> >                                      DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL);
> >       }
> >
> > +     if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
> > +             /* For support SPEED_10 in SGMII mode
> > +              * DP83867_10M_SGMII_RATE_ADAPT bit
> > +              * has to be cleared by software. That
> > +              * does not affect SPEED_100 and
> > +              * SPEED_1000.
> > +              */
> > +             val = phy_read_mmd(phydev, DP83867_DEVADDR,
> > +                                DP83867_10M_SGMII_CFG);
> > +             val &= ~DP83867_10M_SGMII_RATE_ADAPT;
> > +             ret = phy_write_mmd(phydev, DP83867_DEVADDR,
> > +                                 DP83867_10M_SGMII_CFG, val);
>
> This could be simplified by using phy_modify_mmd().
>
> > +             if (ret) {
> > +                     WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
>
> This error message says more or less nothing. The context is visible in the
> stack trace, so you can remove the message w/o losing anything.
> As we're in the config_init callback, I don't think the "ONCE" version is
> needed. So you could simply use WARN_ON(1). Typically just the errno is
> returned w/o additional message, so you could also simply do:
> return phy_modify_mmd(phydev, ...)

The error shoud indicate that something is wrong with mdio bus. I.e.
write returned error. And it's more likely hardware bug which needs to
be fixed. Once I used to not print this error on each ifconfig
up/down.

Max.

>
> > +                     return ret;
> > +             }
> > +     }
> > +
> >       /* Enable Interrupt output INT_OE in CFG3 register */
> >       if (phy_interrupt_is_valid(phydev)) {
> >               val = phy_read(phydev, DP83867_CFG3);
> >
>


-- 
Best regards,
Maxim Uvarov

Re: [PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer duration

[Maxim Uvarov]

пт, 24 мая 2019 г. в 20:24, Heiner Kallweit <hkallweit1@gmail.com>:
>
> On 24.05.2019 12:25, Max Uvarov wrote:
> > After reset SGMII Autoneg timer is set to 2us (bits 6 and 5 are 01).
> > That us not enough to finalize autonegatiation on some devices.
> > Increase this timer duration to maximum supported 16ms.
> >
> > Signed-off-by: Max Uvarov <muvarov@gmail.com>
> > ---
> >  drivers/net/phy/dp83867.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> > index afd31c516cc7..66b0a09ad094 100644
> > --- a/drivers/net/phy/dp83867.c
> > +++ b/drivers/net/phy/dp83867.c
> > @@ -297,6 +297,19 @@ static int dp83867_config_init(struct phy_device *phydev)
> >                       WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
> >                       return ret;
> >               }
> > +
> > +             /* After reset SGMII Autoneg timer is set to 2us (bits 6 and 5
> > +              * are 01). That us not enough to finalize autoneg on some
> > +              * devices. Increase this timer duration to maximum 16ms.
> > +              */
> In the public datasheet the bits are described as reserved. However, based on
> the value, I suppose it's not a timer value but the timer resolution.

No, it's public:
http://www.ti.com/lit/ds/symlink/dp83867e.pdf page 72.

SGMII Auto-Negotiation Timer Duration.

>
> > +             val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
> > +             val &= ~(BIT(5) | BIT(6));
> > +             ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
> > +             if (ret) {
> > +                     WARN_ONCE(1, "dp83867: error config sgmii auto-neg timer\n");
> > +                     return ret;
>
> Same comment as for patch 1.

Yes, the same answer. I want to capture hardware error then silently
return error and then debug it.
WARN is more informative then some random "phy not detected" things.

Max.

>
> > +             }
> > +
> >       }
> >
> >       /* Enable Interrupt output INT_OE in CFG3 register */
> >
>

Re: [PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer duration

[Heiner Kallweit]

On 24.05.2019 21:55, Maxim Uvarov wrote:
> пт, 24 мая 2019 г. в 20:24, Heiner Kallweit <hkallweit1@gmail.com>:
>>
>> On 24.05.2019 12:25, Max Uvarov wrote:
>>> After reset SGMII Autoneg timer is set to 2us (bits 6 and 5 are 01).
>>> That us not enough to finalize autonegatiation on some devices.
>>> Increase this timer duration to maximum supported 16ms.
>>>
>>> Signed-off-by: Max Uvarov <muvarov@gmail.com>
>>> ---
>>>  drivers/net/phy/dp83867.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>> index afd31c516cc7..66b0a09ad094 100644
>>> --- a/drivers/net/phy/dp83867.c
>>> +++ b/drivers/net/phy/dp83867.c
>>> @@ -297,6 +297,19 @@ static int dp83867_config_init(struct phy_device *phydev)
>>>                       WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
>>>                       return ret;
>>>               }
>>> +
>>> +             /* After reset SGMII Autoneg timer is set to 2us (bits 6 and 5
>>> +              * are 01). That us not enough to finalize autoneg on some
>>> +              * devices. Increase this timer duration to maximum 16ms.
>>> +              */
>> In the public datasheet the bits are described as reserved. However, based on
>> the value, I suppose it's not a timer value but the timer resolution.
> 
> No, it's public:
> http://www.ti.com/lit/ds/symlink/dp83867e.pdf page 72.
> 
I just searched for Dp83867 and found this one:
http://www.ti.com/lit/ds/symlink/dp83867cr.pdf
This PHY seems to have the same ID, but the timer bits are at least not
documented.

> SGMII Auto-Negotiation Timer Duration.
> 
I see, we talk about SGMII aneg, not PHY aneg. My bad.

>>
>>> +             val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
>>> +             val &= ~(BIT(5) | BIT(6));
>>> +             ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
>>> +             if (ret) {
>>> +                     WARN_ONCE(1, "dp83867: error config sgmii auto-neg timer\n");
>>> +                     return ret;
>>
>> Same comment as for patch 1.
> 
> Yes, the same answer. I want to capture hardware error then silently
> return error and then debug it.

If you have hardware with buggy MDIO support, then the problem could occur
in any MDIO access. I think then the WARN should be in the MDIO bus ops.

> WARN is more informative then some random "phy not detected" things.
> 
> Max.
> 
>>
>>> +             }
>>> +
>>>       }
>>>
>>>       /* Enable Interrupt output INT_OE in CFG3 register */
>>>
>>
>