[PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Set the speed optimization bit on the DP83867 PHY.
This feature can also be strapped on the 64 pin PHY devices
but the 48 pin devices do not have the strap pin available to enable
this feature in the hardware.  PHY team suggests to have this bit set.

With this bit set the PHY will auto negotiate and report the link
parameters in the PHYSTS register.  This register provides a single
location within the register set for quick access to commonly accessed
information.

In this case when auto negotiation is on the PHY core reads the bits
that have been configured or if auto negotiation is off the PHY core
reads the BMCR register and sets the phydev parameters accordingly.

This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
4-wire cable.  If this should occur the PHYSTS register contains the
current negotiated speed and duplex mode.

In overriding the genphy_read_status the dp83867_read_status will do a
genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
register is read and the phydev speed and duplex mode settings are
updated.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
v2 - Updated read status to call genphy_read_status first, added link_change
callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ 

 drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 967f57ed0b65..6f86ca1ebb51 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -21,6 +21,7 @@
 #define DP83867_DEVADDR		0x1f
 
 #define MII_DP83867_PHYCTRL	0x10
+#define MII_DP83867_PHYSTS	0x11
 #define MII_DP83867_MICR	0x12
 #define MII_DP83867_ISR		0x13
 #define DP83867_CFG2		0x14
@@ -118,6 +119,15 @@
 #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
 #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
 
+/* PHY STS bits */
+#define DP83867_PHYSTS_1000			BIT(15)
+#define DP83867_PHYSTS_100			BIT(14)
+#define DP83867_PHYSTS_DUPLEX			BIT(13)
+#define DP83867_PHYSTS_LINK			BIT(10)
+
+/* CFG2 bits */
+#define DP83867_SPEED_OPTIMIZED_EN		(BIT(8) | BIT(9))
+
 /* CFG3 bits */
 #define DP83867_CFG3_INT_OE			BIT(7)
 #define DP83867_CFG3_ROBUST_AUTO_MDIX		BIT(9)
@@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev)
 	return phy_write(phydev, MII_DP83867_MICR, micr_status);
 }
 
+static void dp83867_link_change_notify(struct phy_device *phydev)
+{
+	if (phydev->state != PHY_RUNNING)
+		return;
+
+	if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
+		phydev_warn(phydev, "Downshift detected connection is %iMbps\n",
+			    phydev->speed);
+}
+
+static int dp83867_read_status(struct phy_device *phydev)
+{
+	int status = phy_read(phydev, MII_DP83867_PHYSTS);
+	int ret;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	if (status < 0)
+		return status;
+
+	if (status & DP83867_PHYSTS_DUPLEX)
+		phydev->duplex = DUPLEX_FULL;
+	else
+		phydev->duplex = DUPLEX_HALF;
+
+	if (status & DP83867_PHYSTS_1000)
+		phydev->speed = SPEED_1000;
+	else if (status & DP83867_PHYSTS_100)
+		phydev->speed = SPEED_100;
+	else
+		phydev->speed = SPEED_10;
+
+	return 0;
+}
+
 static int dp83867_config_port_mirroring(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 =
@@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev)
 	int ret, val, bs;
 	u16 delay;
 
+	/* Force speed optimization for the PHY even if it strapped */
+	ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN);
+	if (ret)
+		return ret;
+
 	ret = dp83867_verify_rgmii_cfg(phydev);
 	if (ret)
 		return ret;
@@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = {
 		.config_init	= dp83867_config_init,
 		.soft_reset	= dp83867_phy_reset,
 
+		.read_status	= dp83867_read_status,
+		.link_change_notify = dp83867_link_change_notify,
+
 		.get_wol	= dp83867_get_wol,
 		.set_wol	= dp83867_set_wol,
 
-- 
2.25.0

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Heiner Kallweit]

On 04.02.2020 19:13, Dan Murphy wrote:
> Set the speed optimization bit on the DP83867 PHY.
> This feature can also be strapped on the 64 pin PHY devices
> but the 48 pin devices do not have the strap pin available to enable
> this feature in the hardware.  PHY team suggests to have this bit set.
> 
> With this bit set the PHY will auto negotiate and report the link
> parameters in the PHYSTS register.  This register provides a single
> location within the register set for quick access to commonly accessed
> information.
> 
> In this case when auto negotiation is on the PHY core reads the bits
> that have been configured or if auto negotiation is off the PHY core
> reads the BMCR register and sets the phydev parameters accordingly.
> 
> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
> 4-wire cable.  If this should occur the PHYSTS register contains the
> current negotiated speed and duplex mode.
> 
> In overriding the genphy_read_status the dp83867_read_status will do a
> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
> register is read and the phydev speed and duplex mode settings are
> updated.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

net-next is closed currently. See here for details:
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
Reopening will be announced on the netdev mailing list, you can also
check net-next status here: http://vger.kernel.org/~davem/net-next.html

> ---
> v2 - Updated read status to call genphy_read_status first, added link_change
> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ 
> 
>  drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 967f57ed0b65..6f86ca1ebb51 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -21,6 +21,7 @@
>  #define DP83867_DEVADDR		0x1f
>  
>  #define MII_DP83867_PHYCTRL	0x10
> +#define MII_DP83867_PHYSTS	0x11
>  #define MII_DP83867_MICR	0x12
>  #define MII_DP83867_ISR		0x13
>  #define DP83867_CFG2		0x14
> @@ -118,6 +119,15 @@
>  #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
>  #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>  
> +/* PHY STS bits */
> +#define DP83867_PHYSTS_1000			BIT(15)
> +#define DP83867_PHYSTS_100			BIT(14)
> +#define DP83867_PHYSTS_DUPLEX			BIT(13)
> +#define DP83867_PHYSTS_LINK			BIT(10)
> +
> +/* CFG2 bits */
> +#define DP83867_SPEED_OPTIMIZED_EN		(BIT(8) | BIT(9))
> +
>  /* CFG3 bits */
>  #define DP83867_CFG3_INT_OE			BIT(7)
>  #define DP83867_CFG3_ROBUST_AUTO_MDIX		BIT(9)
> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev)
>  	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>  }
>  
> +static void dp83867_link_change_notify(struct phy_device *phydev)
> +{
> +	if (phydev->state != PHY_RUNNING)
> +		return;
> +
> +	if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
> +		phydev_warn(phydev, "Downshift detected connection is %iMbps\n",
> +			    phydev->speed);
> +}
> +
> +static int dp83867_read_status(struct phy_device *phydev)
> +{
> +	int status = phy_read(phydev, MII_DP83867_PHYSTS);
> +	int ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (status < 0)
> +		return status;
> +
> +	if (status & DP83867_PHYSTS_DUPLEX)
> +		phydev->duplex = DUPLEX_FULL;
> +	else
> +		phydev->duplex = DUPLEX_HALF;
> +
> +	if (status & DP83867_PHYSTS_1000)
> +		phydev->speed = SPEED_1000;
> +	else if (status & DP83867_PHYSTS_100)
> +		phydev->speed = SPEED_100;
> +	else
> +		phydev->speed = SPEED_10;
> +
> +	return 0;
> +}
> +
>  static int dp83867_config_port_mirroring(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867 =
> @@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev)
>  	int ret, val, bs;
>  	u16 delay;
>  
> +	/* Force speed optimization for the PHY even if it strapped */
> +	ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN);
> +	if (ret)
> +		return ret;
> +
>  	ret = dp83867_verify_rgmii_cfg(phydev);
>  	if (ret)
>  		return ret;
> @@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = {
>  		.config_init	= dp83867_config_init,
>  		.soft_reset	= dp83867_phy_reset,
>  
> +		.read_status	= dp83867_read_status,
> +		.link_change_notify = dp83867_link_change_notify,
> +
>  		.get_wol	= dp83867_get_wol,
>  		.set_wol	= dp83867_set_wol,
>  
> 

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Heiner

On 2/4/20 2:08 PM, Heiner Kallweit wrote:
> On 04.02.2020 19:13, Dan Murphy wrote:
>> Set the speed optimization bit on the DP83867 PHY.
>> This feature can also be strapped on the 64 pin PHY devices
>> but the 48 pin devices do not have the strap pin available to enable
>> this feature in the hardware.  PHY team suggests to have this bit set.
>>
>> With this bit set the PHY will auto negotiate and report the link
>> parameters in the PHYSTS register.  This register provides a single
>> location within the register set for quick access to commonly accessed
>> information.
>>
>> In this case when auto negotiation is on the PHY core reads the bits
>> that have been configured or if auto negotiation is off the PHY core
>> reads the BMCR register and sets the phydev parameters accordingly.
>>
>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>> 4-wire cable.  If this should occur the PHYSTS register contains the
>> current negotiated speed and duplex mode.
>>
>> In overriding the genphy_read_status the dp83867_read_status will do a
>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>> register is read and the phydev speed and duplex mode settings are
>> updated.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> net-next is closed currently. See here for details:
> https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
> Reopening will be announced on the netdev mailing list, you can also
> check net-next status here: http://vger.kernel.org/~davem/net-next.html
>
Thanks Heiner for the link I RTM.  I will wait for the opening.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Heiner Kallweit]

On 04.02.2020 19:13, Dan Murphy wrote:
> Set the speed optimization bit on the DP83867 PHY.
> This feature can also be strapped on the 64 pin PHY devices
> but the 48 pin devices do not have the strap pin available to enable
> this feature in the hardware.  PHY team suggests to have this bit set.
> 
> With this bit set the PHY will auto negotiate and report the link
> parameters in the PHYSTS register.  This register provides a single
> location within the register set for quick access to commonly accessed
> information.
> 
> In this case when auto negotiation is on the PHY core reads the bits
> that have been configured or if auto negotiation is off the PHY core
> reads the BMCR register and sets the phydev parameters accordingly.
> 
> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
> 4-wire cable.  If this should occur the PHYSTS register contains the
> current negotiated speed and duplex mode.
> 
> In overriding the genphy_read_status the dp83867_read_status will do a
> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
> register is read and the phydev speed and duplex mode settings are
> updated.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> v2 - Updated read status to call genphy_read_status first, added link_change
> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ 
> 

As stated in the first review, it would be appreciated if you implement
also the downshift tunable. This could be a separate patch in this series.
Most of the implementation would be boilerplate code.

And I have to admit that I'm not too happy with the term "speed optimization".
This sounds like the PHY has some magic to establish a 1.2Gbps link.
Even though the vendor may call it this way in the datasheet, the standard
term is "downshift". I'm fine with using "speed optimization" in constants
to be in line with the datasheet. Just a comment in the code would be helpful
that speed optimization is the vendor's term for downshift.

>  drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 967f57ed0b65..6f86ca1ebb51 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -21,6 +21,7 @@
>  #define DP83867_DEVADDR		0x1f
>  
>  #define MII_DP83867_PHYCTRL	0x10
> +#define MII_DP83867_PHYSTS	0x11
>  #define MII_DP83867_MICR	0x12
>  #define MII_DP83867_ISR		0x13
>  #define DP83867_CFG2		0x14
> @@ -118,6 +119,15 @@
>  #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
>  #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>  
> +/* PHY STS bits */
> +#define DP83867_PHYSTS_1000			BIT(15)
> +#define DP83867_PHYSTS_100			BIT(14)
> +#define DP83867_PHYSTS_DUPLEX			BIT(13)
> +#define DP83867_PHYSTS_LINK			BIT(10)
> +
> +/* CFG2 bits */
> +#define DP83867_SPEED_OPTIMIZED_EN		(BIT(8) | BIT(9))
> +
>  /* CFG3 bits */
>  #define DP83867_CFG3_INT_OE			BIT(7)
>  #define DP83867_CFG3_ROBUST_AUTO_MDIX		BIT(9)
> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev)
>  	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>  }
>  
> +static void dp83867_link_change_notify(struct phy_device *phydev)
> +{
> +	if (phydev->state != PHY_RUNNING)
> +		return;
> +
> +	if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
> +		phydev_warn(phydev, "Downshift detected connection is %iMbps\n",
> +			    phydev->speed);

The link partner may simply not advertise 1Gbps. How do you know that
a link speed of e.g. 100Mbps is caused by a downshift?
Some PHY's I've seen with this feature have a flag somewhere indicating
that downshift occurred. How about the PHY here?

> +}
> +
> +static int dp83867_read_status(struct phy_device *phydev)
> +{
> +	int status = phy_read(phydev, MII_DP83867_PHYSTS);
> +	int ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (status < 0)
> +		return status;
> +
> +	if (status & DP83867_PHYSTS_DUPLEX)
> +		phydev->duplex = DUPLEX_FULL;
> +	else
> +		phydev->duplex = DUPLEX_HALF;
> +
> +	if (status & DP83867_PHYSTS_1000)
> +		phydev->speed = SPEED_1000;
> +	else if (status & DP83867_PHYSTS_100)
> +		phydev->speed = SPEED_100;
> +	else
> +		phydev->speed = SPEED_10;
> +
> +	return 0;
> +}
> +
>  static int dp83867_config_port_mirroring(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867 =
> @@ -467,6 +514,11 @@ static int dp83867_config_init(struct phy_device *phydev)
>  	int ret, val, bs;
>  	u16 delay;
>  
> +	/* Force speed optimization for the PHY even if it strapped */
> +	ret = phy_set_bits(phydev, DP83867_CFG2, DP83867_SPEED_OPTIMIZED_EN);
> +	if (ret)
> +		return ret;
> +
>  	ret = dp83867_verify_rgmii_cfg(phydev);
>  	if (ret)
>  		return ret;
> @@ -655,6 +707,9 @@ static struct phy_driver dp83867_driver[] = {
>  		.config_init	= dp83867_config_init,
>  		.soft_reset	= dp83867_phy_reset,
>  
> +		.read_status	= dp83867_read_status,
> +		.link_change_notify = dp83867_link_change_notify,
> +
>  		.get_wol	= dp83867_get_wol,
>  		.set_wol	= dp83867_set_wol,
>  
> 

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Heiner

On 2/5/20 3:16 PM, Heiner Kallweit wrote:
> On 04.02.2020 19:13, Dan Murphy wrote:
>> Set the speed optimization bit on the DP83867 PHY.
>> This feature can also be strapped on the 64 pin PHY devices
>> but the 48 pin devices do not have the strap pin available to enable
>> this feature in the hardware.  PHY team suggests to have this bit set.
>>
>> With this bit set the PHY will auto negotiate and report the link
>> parameters in the PHYSTS register.  This register provides a single
>> location within the register set for quick access to commonly accessed
>> information.
>>
>> In this case when auto negotiation is on the PHY core reads the bits
>> that have been configured or if auto negotiation is off the PHY core
>> reads the BMCR register and sets the phydev parameters accordingly.
>>
>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>> 4-wire cable.  If this should occur the PHYSTS register contains the
>> current negotiated speed and duplex mode.
>>
>> In overriding the genphy_read_status the dp83867_read_status will do a
>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>> register is read and the phydev speed and duplex mode settings are
>> updated.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> v2 - Updated read status to call genphy_read_status first, added link_change
>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>
> As stated in the first review, it would be appreciated if you implement
> also the downshift tunable. This could be a separate patch in this series.
> Most of the implementation would be boilerplate code.

I just don't have a requirement from our customer to make it adjustable 
so I did not want to add something extra.

I can add in for v3.

>
> And I have to admit that I'm not too happy with the term "speed optimization".
> This sounds like the PHY has some magic to establish a 1.2Gbps link.
> Even though the vendor may call it this way in the datasheet, the standard
> term is "downshift". I'm fine with using "speed optimization" in constants
> to be in line with the datasheet. Just a comment in the code would be helpful
> that speed optimization is the vendor's term for downshift.

Ack.  The data sheet actually says "Speed optimization, also known as 
link downshift"

So I probably will just rename everything down shift.

>
>>   drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>> index 967f57ed0b65..6f86ca1ebb51 100644
>> --- a/drivers/net/phy/dp83867.c
>> +++ b/drivers/net/phy/dp83867.c
>> @@ -21,6 +21,7 @@
>>   #define DP83867_DEVADDR		0x1f
>>   
>>   #define MII_DP83867_PHYCTRL	0x10
>> +#define MII_DP83867_PHYSTS	0x11
>>   #define MII_DP83867_MICR	0x12
>>   #define MII_DP83867_ISR		0x13
>>   #define DP83867_CFG2		0x14
>> @@ -118,6 +119,15 @@
>>   #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK	(0x1f << 8)
>>   #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT	8
>>   
>> +/* PHY STS bits */
>> +#define DP83867_PHYSTS_1000			BIT(15)
>> +#define DP83867_PHYSTS_100			BIT(14)
>> +#define DP83867_PHYSTS_DUPLEX			BIT(13)
>> +#define DP83867_PHYSTS_LINK			BIT(10)
>> +
>> +/* CFG2 bits */
>> +#define DP83867_SPEED_OPTIMIZED_EN		(BIT(8) | BIT(9))
>> +
>>   /* CFG3 bits */
>>   #define DP83867_CFG3_INT_OE			BIT(7)
>>   #define DP83867_CFG3_ROBUST_AUTO_MDIX		BIT(9)
>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device *phydev)
>>   	return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>   }
>>   
>> +static void dp83867_link_change_notify(struct phy_device *phydev)
>> +{
>> +	if (phydev->state != PHY_RUNNING)
>> +		return;
>> +
>> +	if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>> +		phydev_warn(phydev, "Downshift detected connection is %iMbps\n",
>> +			    phydev->speed);
> The link partner may simply not advertise 1Gbps. How do you know that
> a link speed of e.g. 100Mbps is caused by a downshift?
> Some PHY's I've seen with this feature have a flag somewhere indicating
> that downshift occurred. How about the PHY here?

I don't see a register that gives us that status

I will ask the hardware team if there is one.

This is a 1Gbps PHY by default so if a slower connection is established 
due to faulty cabling or LP advertisement then this would be a down 
shift IMO.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Florian Fainelli]

On 2/5/20 1:51 PM, Dan Murphy wrote:
> Heiner
> 
> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>> On 04.02.2020 19:13, Dan Murphy wrote:
>>> Set the speed optimization bit on the DP83867 PHY.
>>> This feature can also be strapped on the 64 pin PHY devices
>>> but the 48 pin devices do not have the strap pin available to enable
>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>
>>> With this bit set the PHY will auto negotiate and report the link
>>> parameters in the PHYSTS register.  This register provides a single
>>> location within the register set for quick access to commonly accessed
>>> information.
>>>
>>> In this case when auto negotiation is on the PHY core reads the bits
>>> that have been configured or if auto negotiation is off the PHY core
>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>
>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to
>>> accomodate a
>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>> current negotiated speed and duplex mode.
>>>
>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>> register is read and the phydev speed and duplex mode settings are
>>> updated.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> v2 - Updated read status to call genphy_read_status first, added
>>> link_change
>>> callback to notify of speed change and use phy_set_bits -
>>> https://lore.kernel.org/patchwork/patch/1188348/
>>>
>> As stated in the first review, it would be appreciated if you implement
>> also the downshift tunable. This could be a separate patch in this
>> series.
>> Most of the implementation would be boilerplate code.
> 
> I just don't have a requirement from our customer to make it adjustable
> so I did not want to add something extra.
> 
> I can add in for v3.
> 
>>
>> And I have to admit that I'm not too happy with the term "speed
>> optimization".
>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>> Even though the vendor may call it this way in the datasheet, the
>> standard
>> term is "downshift". I'm fine with using "speed optimization" in
>> constants
>> to be in line with the datasheet. Just a comment in the code would be
>> helpful
>> that speed optimization is the vendor's term for downshift.
> 
> Ack.  The data sheet actually says "Speed optimization, also known as
> link downshift"
> 
> So I probably will just rename everything down shift.
> 
>>
>>>   drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>> --- a/drivers/net/phy/dp83867.c
>>> +++ b/drivers/net/phy/dp83867.c
>>> @@ -21,6 +21,7 @@
>>>   #define DP83867_DEVADDR        0x1f
>>>     #define MII_DP83867_PHYCTRL    0x10
>>> +#define MII_DP83867_PHYSTS    0x11
>>>   #define MII_DP83867_MICR    0x12
>>>   #define MII_DP83867_ISR        0x13
>>>   #define DP83867_CFG2        0x14
>>> @@ -118,6 +119,15 @@
>>>   #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>   #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>   +/* PHY STS bits */
>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>> +#define DP83867_PHYSTS_100            BIT(14)
>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>> +
>>> +/* CFG2 bits */
>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>> +
>>>   /* CFG3 bits */
>>>   #define DP83867_CFG3_INT_OE            BIT(7)
>>>   #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device
>>> *phydev)
>>>       return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>   }
>>>   +static void dp83867_link_change_notify(struct phy_device *phydev)
>>> +{
>>> +    if (phydev->state != PHY_RUNNING)
>>> +        return;
>>> +
>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>> +        phydev_warn(phydev, "Downshift detected connection is
>>> %iMbps\n",
>>> +                phydev->speed);
>> The link partner may simply not advertise 1Gbps. How do you know that
>> a link speed of e.g. 100Mbps is caused by a downshift?
>> Some PHY's I've seen with this feature have a flag somewhere indicating
>> that downshift occurred. How about the PHY here?
> 
> I don't see a register that gives us that status
> 
> I will ask the hardware team if there is one.
> 
> This is a 1Gbps PHY by default so if a slower connection is established
> due to faulty cabling or LP advertisement then this would be a down
> shift IMO.

With your current link_change_notify function it would not be possible
to know whether the PHY was connected to a link partner that advertised
only 10/100 and so 100 ended up being the link speed, or the link
partner was capable of 10/100/1000 and downshift reduced the link speed.

If you cannot tell the difference from a register, it might be better to
simply omit that function then.
-- 
Florian

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Florian

On 2/5/20 4:00 PM, Florian Fainelli wrote:
> On 2/5/20 1:51 PM, Dan Murphy wrote:
>> Heiner
>>
>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>> Set the speed optimization bit on the DP83867 PHY.
>>>> This feature can also be strapped on the 64 pin PHY devices
>>>> but the 48 pin devices do not have the strap pin available to enable
>>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>>
>>>> With this bit set the PHY will auto negotiate and report the link
>>>> parameters in the PHYSTS register.  This register provides a single
>>>> location within the register set for quick access to commonly accessed
>>>> information.
>>>>
>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>> that have been configured or if auto negotiation is off the PHY core
>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>
>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to
>>>> accomodate a
>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>> current negotiated speed and duplex mode.
>>>>
>>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>>> register is read and the phydev speed and duplex mode settings are
>>>> updated.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>> v2 - Updated read status to call genphy_read_status first, added
>>>> link_change
>>>> callback to notify of speed change and use phy_set_bits -
>>>> https://lore.kernel.org/patchwork/patch/1188348/
>>>>
>>> As stated in the first review, it would be appreciated if you implement
>>> also the downshift tunable. This could be a separate patch in this
>>> series.
>>> Most of the implementation would be boilerplate code.
>> I just don't have a requirement from our customer to make it adjustable
>> so I did not want to add something extra.
>>
>> I can add in for v3.
>>
>>> And I have to admit that I'm not too happy with the term "speed
>>> optimization".
>>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>>> Even though the vendor may call it this way in the datasheet, the
>>> standard
>>> term is "downshift". I'm fine with using "speed optimization" in
>>> constants
>>> to be in line with the datasheet. Just a comment in the code would be
>>> helpful
>>> that speed optimization is the vendor's term for downshift.
>> Ack.  The data sheet actually says "Speed optimization, also known as
>> link downshift"
>>
>> So I probably will just rename everything down shift.
>>
>>>>    drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 55 insertions(+)
>>>>
>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>>> --- a/drivers/net/phy/dp83867.c
>>>> +++ b/drivers/net/phy/dp83867.c
>>>> @@ -21,6 +21,7 @@
>>>>    #define DP83867_DEVADDR        0x1f
>>>>      #define MII_DP83867_PHYCTRL    0x10
>>>> +#define MII_DP83867_PHYSTS    0x11
>>>>    #define MII_DP83867_MICR    0x12
>>>>    #define MII_DP83867_ISR        0x13
>>>>    #define DP83867_CFG2        0x14
>>>> @@ -118,6 +119,15 @@
>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>>    +/* PHY STS bits */
>>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>>> +#define DP83867_PHYSTS_100            BIT(14)
>>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>>> +
>>>> +/* CFG2 bits */
>>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>>> +
>>>>    /* CFG3 bits */
>>>>    #define DP83867_CFG3_INT_OE            BIT(7)
>>>>    #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device
>>>> *phydev)
>>>>        return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>>    }
>>>>    +static void dp83867_link_change_notify(struct phy_device *phydev)
>>>> +{
>>>> +    if (phydev->state != PHY_RUNNING)
>>>> +        return;
>>>> +
>>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>>> +        phydev_warn(phydev, "Downshift detected connection is
>>>> %iMbps\n",
>>>> +                phydev->speed);
>>> The link partner may simply not advertise 1Gbps. How do you know that
>>> a link speed of e.g. 100Mbps is caused by a downshift?
>>> Some PHY's I've seen with this feature have a flag somewhere indicating
>>> that downshift occurred. How about the PHY here?
>> I don't see a register that gives us that status
>>
>> I will ask the hardware team if there is one.
>>
>> This is a 1Gbps PHY by default so if a slower connection is established
>> due to faulty cabling or LP advertisement then this would be a down
>> shift IMO.
> With your current link_change_notify function it would not be possible
> to know whether the PHY was connected to a link partner that advertised
> only 10/100 and so 100 ended up being the link speed, or the link
> partner was capable of 10/100/1000 and downshift reduced the link speed.
>
> If you cannot tell the difference from a register, it might be better to
> simply omit that function then.

Yeah I thought it was a bit redundant and wonky to see in the log that 
the link established to xG/Mbps and then see another message saying the 
downshift occurred.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Heiner

On 2/5/20 3:16 PM, Heiner Kallweit wrote:
> On 04.02.2020 19:13, Dan Murphy wrote:
>> Set the speed optimization bit on the DP83867 PHY.
>> This feature can also be strapped on the 64 pin PHY devices
>> but the 48 pin devices do not have the strap pin available to enable
>> this feature in the hardware.  PHY team suggests to have this bit set.
>>
>> With this bit set the PHY will auto negotiate and report the link
>> parameters in the PHYSTS register.  This register provides a single
>> location within the register set for quick access to commonly accessed
>> information.
>>
>> In this case when auto negotiation is on the PHY core reads the bits
>> that have been configured or if auto negotiation is off the PHY core
>> reads the BMCR register and sets the phydev parameters accordingly.
>>
>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>> 4-wire cable.  If this should occur the PHYSTS register contains the
>> current negotiated speed and duplex mode.
>>
>> In overriding the genphy_read_status the dp83867_read_status will do a
>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>> register is read and the phydev speed and duplex mode settings are
>> updated.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> v2 - Updated read status to call genphy_read_status first, added link_change
>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>
> As stated in the first review, it would be appreciated if you implement
> also the downshift tunable. This could be a separate patch in this series.
> Most of the implementation would be boilerplate code.


I looked at this today and there are no registers that allow tuning the 
downshift attempts.  There is only a RO register that tells you how many 
attempts it took to achieve a link.  So at the very least we could put 
in the get_tunable but there will be no set.

So we should probably skip this for this PHY.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Heiner Kallweit]

On 06.02.2020 23:13, Dan Murphy wrote:
> Heiner
> 
> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>> On 04.02.2020 19:13, Dan Murphy wrote:
>>> Set the speed optimization bit on the DP83867 PHY.
>>> This feature can also be strapped on the 64 pin PHY devices
>>> but the 48 pin devices do not have the strap pin available to enable
>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>
>>> With this bit set the PHY will auto negotiate and report the link
>>> parameters in the PHYSTS register.  This register provides a single
>>> location within the register set for quick access to commonly accessed
>>> information.
>>>
>>> In this case when auto negotiation is on the PHY core reads the bits
>>> that have been configured or if auto negotiation is off the PHY core
>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>
>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>> current negotiated speed and duplex mode.
>>>
>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>> register is read and the phydev speed and duplex mode settings are
>>> updated.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> v2 - Updated read status to call genphy_read_status first, added link_change
>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>>
>> As stated in the first review, it would be appreciated if you implement
>> also the downshift tunable. This could be a separate patch in this series.
>> Most of the implementation would be boilerplate code.
> 
> 
> I looked at this today and there are no registers that allow tuning the downshift attempts.  There is only a RO register that tells you how many attempts it took to achieve a link.  So at the very least we could put in the get_tunable but there will be no set.
> 
The get operation for the downshift tunable should return after how many failed
attempts the PHY starts a downshift. This doesn't match with your description of
this register, so yes: Implementing the tunable for this PHY doesn't make sense.

However this register may be useful in the link_change_notify() callback to
figure out whether a downshift happened, to trigger the info message you had in
your first version.

> So we should probably skip this for this PHY.
> 
> Dan
> 
Heiner

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Heiner

On 2/6/20 4:28 PM, Heiner Kallweit wrote:
> On 06.02.2020 23:13, Dan Murphy wrote:
>> Heiner
>>
>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>> Set the speed optimization bit on the DP83867 PHY.
>>>> This feature can also be strapped on the 64 pin PHY devices
>>>> but the 48 pin devices do not have the strap pin available to enable
>>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>>
>>>> With this bit set the PHY will auto negotiate and report the link
>>>> parameters in the PHYSTS register.  This register provides a single
>>>> location within the register set for quick access to commonly accessed
>>>> information.
>>>>
>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>> that have been configured or if auto negotiation is off the PHY core
>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>
>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>> current negotiated speed and duplex mode.
>>>>
>>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>>> register is read and the phydev speed and duplex mode settings are
>>>> updated.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>> v2 - Updated read status to call genphy_read_status first, added link_change
>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>>>
>>> As stated in the first review, it would be appreciated if you implement
>>> also the downshift tunable. This could be a separate patch in this series.
>>> Most of the implementation would be boilerplate code.
>>
>> I looked at this today and there are no registers that allow tuning the downshift attempts.  There is only a RO register that tells you how many attempts it took to achieve a link.  So at the very least we could put in the get_tunable but there will be no set.
>>
> The get operation for the downshift tunable should return after how many failed
> attempts the PHY starts a downshift. This doesn't match with your description of
> this register, so yes: Implementing the tunable for this PHY doesn't make sense.
True.  This register is only going to return 1,2,4 and 8.  And it is 
defaulted to 4 attempts.
>
> However this register may be useful in the link_change_notify() callback to
> figure out whether a downshift happened, to trigger the info message you had in
> your first version.

Thats a good idea but.. The register is defaulted to always report 4 
attempts were made. It never reports 0 attempts so we would never know 
the truth behind the reporting.  Kinda like matching the speeds.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Heiner Kallweit]

On 06.02.2020 23:36, Dan Murphy wrote:
> Heiner
> 
> On 2/6/20 4:28 PM, Heiner Kallweit wrote:
>> On 06.02.2020 23:13, Dan Murphy wrote:
>>> Heiner
>>>
>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>>> Set the speed optimization bit on the DP83867 PHY.
>>>>> This feature can also be strapped on the 64 pin PHY devices
>>>>> but the 48 pin devices do not have the strap pin available to enable
>>>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>>>
>>>>> With this bit set the PHY will auto negotiate and report the link
>>>>> parameters in the PHYSTS register.  This register provides a single
>>>>> location within the register set for quick access to commonly accessed
>>>>> information.
>>>>>
>>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>>> that have been configured or if auto negotiation is off the PHY core
>>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>>
>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>>> current negotiated speed and duplex mode.
>>>>>
>>>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>>>> register is read and the phydev speed and duplex mode settings are
>>>>> updated.
>>>>>
>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>> ---
>>>>> v2 - Updated read status to call genphy_read_status first, added link_change
>>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>>>>
>>>> As stated in the first review, it would be appreciated if you implement
>>>> also the downshift tunable. This could be a separate patch in this series.
>>>> Most of the implementation would be boilerplate code.
>>>
>>> I looked at this today and there are no registers that allow tuning the downshift attempts.  There is only a RO register that tells you how many attempts it took to achieve a link.  So at the very least we could put in the get_tunable but there will be no set.
>>>
>> The get operation for the downshift tunable should return after how many failed
>> attempts the PHY starts a downshift. This doesn't match with your description of
>> this register, so yes: Implementing the tunable for this PHY doesn't make sense.
> True.  This register is only going to return 1,2,4 and 8.  And it is defaulted to 4 attempts.
>>
>> However this register may be useful in the link_change_notify() callback to
>> figure out whether a downshift happened, to trigger the info message you had in
>> your first version.
> 
> Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting.  Kinda like matching the speeds.
> 

I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf
It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45)
Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's
an RW register. It's marked as RO however, maybe it's a typo in the datasheet.
Did you test whether register is writable?
Last but not least this register is exactly what's needed for the downshift tunable.

Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR.
In interrupt mode however this may require a custom interrupt handler (implementation of
handle_interrupt callback).

Alternatively you could check SPEED_OPT_STATUS in PHYSTS. It says "valid only during aneg"
but that sounds a little bit weird. Would need to be tested whether bit remains set after
downshifted aneg is finished.

> Dan
> 
Heiner

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Heiner

On 2/6/20 5:04 PM, Heiner Kallweit wrote:
> On 06.02.2020 23:36, Dan Murphy wrote:
>> Heiner
>>
>> On 2/6/20 4:28 PM, Heiner Kallweit wrote:
>>> On 06.02.2020 23:13, Dan Murphy wrote:
>>>> Heiner
>>>>
>>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>>>> Set the speed optimization bit on the DP83867 PHY.
>>>>>> This feature can also be strapped on the 64 pin PHY devices
>>>>>> but the 48 pin devices do not have the strap pin available to enable
>>>>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>>>>
>>>>>> With this bit set the PHY will auto negotiate and report the link
>>>>>> parameters in the PHYSTS register.  This register provides a single
>>>>>> location within the register set for quick access to commonly accessed
>>>>>> information.
>>>>>>
>>>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>>>> that have been configured or if auto negotiation is off the PHY core
>>>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>>>
>>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a
>>>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>>>> current negotiated speed and duplex mode.
>>>>>>
>>>>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>>>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>>>>> register is read and the phydev speed and duplex mode settings are
>>>>>> updated.
>>>>>>
>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>>> ---
>>>>>> v2 - Updated read status to call genphy_read_status first, added link_change
>>>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/
>>>>>>
>>>>> As stated in the first review, it would be appreciated if you implement
>>>>> also the downshift tunable. This could be a separate patch in this series.
>>>>> Most of the implementation would be boilerplate code.
>>>> I looked at this today and there are no registers that allow tuning the downshift attempts.  There is only a RO register that tells you how many attempts it took to achieve a link.  So at the very least we could put in the get_tunable but there will be no set.
>>>>
>>> The get operation for the downshift tunable should return after how many failed
>>> attempts the PHY starts a downshift. This doesn't match with your description of
>>> this register, so yes: Implementing the tunable for this PHY doesn't make sense.
>> True.  This register is only going to return 1,2,4 and 8.  And it is defaulted to 4 attempts.
>>> However this register may be useful in the link_change_notify() callback to
>>> figure out whether a downshift happened, to trigger the info message you had in
>>> your first version.
>> Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting.  Kinda like matching the speeds.
>>
> I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf
> It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45)
> Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's
> an RW register. It's marked as RO however, maybe it's a typo in the datasheet.
> Did you test whether register is writable?

Yes I did and it was a no go.  It is definitely a RO.  I will complain 
to the HW team and get it straightened out.  We have time before 
net-next opens


> Last but not least this register is exactly what's needed for the downshift tunable.
>
> Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR.
> In interrupt mode however this may require a custom interrupt handler (implementation of
> handle_interrupt callback).

Yes the HW team did say R13b5 could be checked but after thinking about 
it the issue with that is that is a clear on read register so other 
status would be lost.  There could be a race condition between the 
interrupt handler and the link notification change to be able to 
indicate whether the downshift happened or not.

Same with polling mode can we be guaranteed that the status would be 
updated before the link change was called?

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Grygorii

On 2/14/20 12:32 PM, Grygorii Strashko wrote:
>
>
> On 06/02/2020 00:01, Dan Murphy wrote:
>> Florian
>>
>> On 2/5/20 4:00 PM, Florian Fainelli wrote:
>>> On 2/5/20 1:51 PM, Dan Murphy wrote:
>>>> Heiner
>>>>
>>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>>>> Set the speed optimization bit on the DP83867 PHY.
>>>>>> This feature can also be strapped on the 64 pin PHY devices
>>>>>> but the 48 pin devices do not have the strap pin available to enable
>>>>>> this feature in the hardware.  PHY team suggests to have this bit 
>>>>>> set.
>>>>>>
>>>>>> With this bit set the PHY will auto negotiate and report the link
>>>>>> parameters in the PHYSTS register.  This register provides a single
>>>>>> location within the register set for quick access to commonly 
>>>>>> accessed
>>>>>> information.
>>>>>>
>>>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>>>> that have been configured or if auto negotiation is off the PHY core
>>>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>>>
>>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to
>>>>>> accomodate a
>>>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>>>> current negotiated speed and duplex mode.
>>>>>>
>>>>>> In overriding the genphy_read_status the dp83867_read_status will 
>>>>>> do a
>>>>>> genphy_read_status to setup the LP and pause bits.  And then the 
>>>>>> PHYSTS
>>>>>> register is read and the phydev speed and duplex mode settings are
>>>>>> updated.
>>>>>>
>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>>> ---
>>>>>> v2 - Updated read status to call genphy_read_status first, added
>>>>>> link_change
>>>>>> callback to notify of speed change and use phy_set_bits -
>>>>>> https://lore.kernel.org/patchwork/patch/1188348/
>>>>>>
>>>>> As stated in the first review, it would be appreciated if you 
>>>>> implement
>>>>> also the downshift tunable. This could be a separate patch in this
>>>>> series.
>>>>> Most of the implementation would be boilerplate code.
>>>> I just don't have a requirement from our customer to make it 
>>>> adjustable
>>>> so I did not want to add something extra.
>>>>
>>>> I can add in for v3.
>>>>
>>>>> And I have to admit that I'm not too happy with the term "speed
>>>>> optimization".
>>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>>>>> Even though the vendor may call it this way in the datasheet, the
>>>>> standard
>>>>> term is "downshift". I'm fine with using "speed optimization" in
>>>>> constants
>>>>> to be in line with the datasheet. Just a comment in the code would be
>>>>> helpful
>>>>> that speed optimization is the vendor's term for downshift.
>>>> Ack.  The data sheet actually says "Speed optimization, also known as
>>>> link downshift"
>>>>
>>>> So I probably will just rename everything down shift.
>>>>
>>>>>>    drivers/net/phy/dp83867.c | 55 
>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>    1 file changed, 55 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>>>>> --- a/drivers/net/phy/dp83867.c
>>>>>> +++ b/drivers/net/phy/dp83867.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>>    #define DP83867_DEVADDR        0x1f
>>>>>>      #define MII_DP83867_PHYCTRL    0x10
>>>>>> +#define MII_DP83867_PHYSTS    0x11
>>>>>>    #define MII_DP83867_MICR    0x12
>>>>>>    #define MII_DP83867_ISR        0x13
>>>>>>    #define DP83867_CFG2        0x14
>>>>>> @@ -118,6 +119,15 @@
>>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>>>>    +/* PHY STS bits */
>>>>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>>>>> +#define DP83867_PHYSTS_100            BIT(14)
>>>>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>>>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>>>>> +
>>>>>> +/* CFG2 bits */
>>>>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>>>>> +
>>>>>>    /* CFG3 bits */
>>>>>>    #define DP83867_CFG3_INT_OE            BIT(7)
>>>>>>    #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct 
>>>>>> phy_device
>>>>>> *phydev)
>>>>>>        return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>>>>    }
>>>>>>    +static void dp83867_link_change_notify(struct phy_device 
>>>>>> *phydev)
>>>>>> +{
>>>>>> +    if (phydev->state != PHY_RUNNING)
>>>>>> +        return;
>>>>>> +
>>>>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>>>>> +        phydev_warn(phydev, "Downshift detected connection is
>>>>>> %iMbps\n",
>>>>>> +                phydev->speed);
>>>>> The link partner may simply not advertise 1Gbps. How do you know that
>>>>> a link speed of e.g. 100Mbps is caused by a downshift?
>>>>> Some PHY's I've seen with this feature have a flag somewhere 
>>>>> indicating
>>>>> that downshift occurred. How about the PHY here?
>>>> I don't see a register that gives us that status
>>>>
>>>> I will ask the hardware team if there is one.
>>>>
>>>> This is a 1Gbps PHY by default so if a slower connection is 
>>>> established
>>>> due to faulty cabling or LP advertisement then this would be a down
>>>> shift IMO.
>>> With your current link_change_notify function it would not be possible
>>> to know whether the PHY was connected to a link partner that advertised
>>> only 10/100 and so 100 ended up being the link speed, or the link
>>> partner was capable of 10/100/1000 and downshift reduced the link 
>>> speed.
>>>
>>> If you cannot tell the difference from a register, it might be 
>>> better to
>>> simply omit that function then.
>>
>> Yeah I thought it was a bit redundant and wonky to see in the log 
>> that the link established to xG/Mbps and then see another message 
>> saying the downshift occurred.
>
> I think it's good idea to have this message as just wrong cable might 
> be used.
>
> But this notifier make no sense in it current form - it will produce 
> noise in case of forced 100m/10M.
>
> FYI. PHY sequence to update link:
> phy_state_machine()
> |-phy_check_link_status()
>   |-phy_link_down/up()
>     |- .phy_link_change()->phy_link_change()
>     |-adjust_link() ----> netdev callback
> |-phydev->drv->link_change_notify(phydev);
>
> So, log output has to be done or in .read_status() or
> some info has to be saved in .read_status() and then re-used in
> .link_change_notify().
>
OK I will try to find a way to give some sort of message.

Also we did get confirmation from HW guys and you also confirmed that 
the number of attempts for downshift is configurable.  So I will be 
adding back the tunable code once net-next opens.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Grygorii Strashko]

On 06/02/2020 00:01, Dan Murphy wrote:
> Florian
> 
> On 2/5/20 4:00 PM, Florian Fainelli wrote:
>> On 2/5/20 1:51 PM, Dan Murphy wrote:
>>> Heiner
>>>
>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>>> Set the speed optimization bit on the DP83867 PHY.
>>>>> This feature can also be strapped on the 64 pin PHY devices
>>>>> but the 48 pin devices do not have the strap pin available to enable
>>>>> this feature in the hardware.  PHY team suggests to have this bit set.
>>>>>
>>>>> With this bit set the PHY will auto negotiate and report the link
>>>>> parameters in the PHYSTS register.  This register provides a single
>>>>> location within the register set for quick access to commonly accessed
>>>>> information.
>>>>>
>>>>> In this case when auto negotiation is on the PHY core reads the bits
>>>>> that have been configured or if auto negotiation is off the PHY core
>>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>>
>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to
>>>>> accomodate a
>>>>> 4-wire cable.  If this should occur the PHYSTS register contains the
>>>>> current negotiated speed and duplex mode.
>>>>>
>>>>> In overriding the genphy_read_status the dp83867_read_status will do a
>>>>> genphy_read_status to setup the LP and pause bits.  And then the PHYSTS
>>>>> register is read and the phydev speed and duplex mode settings are
>>>>> updated.
>>>>>
>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>> ---
>>>>> v2 - Updated read status to call genphy_read_status first, added
>>>>> link_change
>>>>> callback to notify of speed change and use phy_set_bits -
>>>>> https://lore.kernel.org/patchwork/patch/1188348/
>>>>>
>>>> As stated in the first review, it would be appreciated if you implement
>>>> also the downshift tunable. This could be a separate patch in this
>>>> series.
>>>> Most of the implementation would be boilerplate code.
>>> I just don't have a requirement from our customer to make it adjustable
>>> so I did not want to add something extra.
>>>
>>> I can add in for v3.
>>>
>>>> And I have to admit that I'm not too happy with the term "speed
>>>> optimization".
>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>>>> Even though the vendor may call it this way in the datasheet, the
>>>> standard
>>>> term is "downshift". I'm fine with using "speed optimization" in
>>>> constants
>>>> to be in line with the datasheet. Just a comment in the code would be
>>>> helpful
>>>> that speed optimization is the vendor's term for downshift.
>>> Ack.  The data sheet actually says "Speed optimization, also known as
>>> link downshift"
>>>
>>> So I probably will just rename everything down shift.
>>>
>>>>>    drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>>>> --- a/drivers/net/phy/dp83867.c
>>>>> +++ b/drivers/net/phy/dp83867.c
>>>>> @@ -21,6 +21,7 @@
>>>>>    #define DP83867_DEVADDR        0x1f
>>>>>      #define MII_DP83867_PHYCTRL    0x10
>>>>> +#define MII_DP83867_PHYSTS    0x11
>>>>>    #define MII_DP83867_MICR    0x12
>>>>>    #define MII_DP83867_ISR        0x13
>>>>>    #define DP83867_CFG2        0x14
>>>>> @@ -118,6 +119,15 @@
>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>>>    +/* PHY STS bits */
>>>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>>>> +#define DP83867_PHYSTS_100            BIT(14)
>>>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>>>> +
>>>>> +/* CFG2 bits */
>>>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>>>> +
>>>>>    /* CFG3 bits */
>>>>>    #define DP83867_CFG3_INT_OE            BIT(7)
>>>>>    #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device
>>>>> *phydev)
>>>>>        return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>>>    }
>>>>>    +static void dp83867_link_change_notify(struct phy_device *phydev)
>>>>> +{
>>>>> +    if (phydev->state != PHY_RUNNING)
>>>>> +        return;
>>>>> +
>>>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>>>> +        phydev_warn(phydev, "Downshift detected connection is
>>>>> %iMbps\n",
>>>>> +                phydev->speed);
>>>> The link partner may simply not advertise 1Gbps. How do you know that
>>>> a link speed of e.g. 100Mbps is caused by a downshift?
>>>> Some PHY's I've seen with this feature have a flag somewhere indicating
>>>> that downshift occurred. How about the PHY here?
>>> I don't see a register that gives us that status
>>>
>>> I will ask the hardware team if there is one.
>>>
>>> This is a 1Gbps PHY by default so if a slower connection is established
>>> due to faulty cabling or LP advertisement then this would be a down
>>> shift IMO.
>> With your current link_change_notify function it would not be possible
>> to know whether the PHY was connected to a link partner that advertised
>> only 10/100 and so 100 ended up being the link speed, or the link
>> partner was capable of 10/100/1000 and downshift reduced the link speed.
>>
>> If you cannot tell the difference from a register, it might be better to
>> simply omit that function then.
> 
> Yeah I thought it was a bit redundant and wonky to see in the log that the link established to xG/Mbps and then see another message saying the downshift occurred.

I think it's good idea to have this message as just wrong cable might be used.

But this notifier make no sense in it current form - it will produce noise in case of forced 100m/10M.

FYI. PHY sequence to update link:
phy_state_machine()
|-phy_check_link_status()
   |-phy_link_down/up()
     |- .phy_link_change()->phy_link_change()
	|-adjust_link() ----> netdev callback
|-phydev->drv->link_change_notify(phydev);

So, log output has to be done or in .read_status() or
some info has to be saved in .read_status() and then re-used in
.link_change_notify().

-- 
Best regards,
grygorii

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Grygorii

On 2/14/20 12:31 PM, Dan Murphy wrote:
> Grygorii
>
> On 2/14/20 12:32 PM, Grygorii Strashko wrote:
>>
>>
>> On 06/02/2020 00:01, Dan Murphy wrote:
>>> Florian
>>>
>>> On 2/5/20 4:00 PM, Florian Fainelli wrote:
>>>> On 2/5/20 1:51 PM, Dan Murphy wrote:
>>>>> Heiner
>>>>>
>>>>> On 2/5/20 3:16 PM, Heiner Kallweit wrote:
>>>>>> On 04.02.2020 19:13, Dan Murphy wrote:
>>>>>>> Set the speed optimization bit on the DP83867 PHY.
>>>>>>> This feature can also be strapped on the 64 pin PHY devices
>>>>>>> but the 48 pin devices do not have the strap pin available to 
>>>>>>> enable
>>>>>>> this feature in the hardware.  PHY team suggests to have this 
>>>>>>> bit set.
>>>>>>>
>>>>>>> With this bit set the PHY will auto negotiate and report the link
>>>>>>> parameters in the PHYSTS register.  This register provides a single
>>>>>>> location within the register set for quick access to commonly 
>>>>>>> accessed
>>>>>>> information.
>>>>>>>
>>>>>>> In this case when auto negotiation is on the PHY core reads the 
>>>>>>> bits
>>>>>>> that have been configured or if auto negotiation is off the PHY 
>>>>>>> core
>>>>>>> reads the BMCR register and sets the phydev parameters accordingly.
>>>>>>>
>>>>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to
>>>>>>> accomodate a
>>>>>>> 4-wire cable.  If this should occur the PHYSTS register contains 
>>>>>>> the
>>>>>>> current negotiated speed and duplex mode.
>>>>>>>
>>>>>>> In overriding the genphy_read_status the dp83867_read_status 
>>>>>>> will do a
>>>>>>> genphy_read_status to setup the LP and pause bits. And then the 
>>>>>>> PHYSTS
>>>>>>> register is read and the phydev speed and duplex mode settings are
>>>>>>> updated.
>>>>>>>
>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>>>> ---
>>>>>>> v2 - Updated read status to call genphy_read_status first, added
>>>>>>> link_change
>>>>>>> callback to notify of speed change and use phy_set_bits -
>>>>>>> https://lore.kernel.org/patchwork/patch/1188348/
>>>>>>>
>>>>>> As stated in the first review, it would be appreciated if you 
>>>>>> implement
>>>>>> also the downshift tunable. This could be a separate patch in this
>>>>>> series.
>>>>>> Most of the implementation would be boilerplate code.
>>>>> I just don't have a requirement from our customer to make it 
>>>>> adjustable
>>>>> so I did not want to add something extra.
>>>>>
>>>>> I can add in for v3.
>>>>>
>>>>>> And I have to admit that I'm not too happy with the term "speed
>>>>>> optimization".
>>>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>>>>>> Even though the vendor may call it this way in the datasheet, the
>>>>>> standard
>>>>>> term is "downshift". I'm fine with using "speed optimization" in
>>>>>> constants
>>>>>> to be in line with the datasheet. Just a comment in the code 
>>>>>> would be
>>>>>> helpful
>>>>>> that speed optimization is the vendor's term for downshift.
>>>>> Ack.  The data sheet actually says "Speed optimization, also known as
>>>>> link downshift"
>>>>>
>>>>> So I probably will just rename everything down shift.
>>>>>
>>>>>>>    drivers/net/phy/dp83867.c | 55 
>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>    1 file changed, 55 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>>>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>>>>>> --- a/drivers/net/phy/dp83867.c
>>>>>>> +++ b/drivers/net/phy/dp83867.c
>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>    #define DP83867_DEVADDR        0x1f
>>>>>>>      #define MII_DP83867_PHYCTRL    0x10
>>>>>>> +#define MII_DP83867_PHYSTS    0x11
>>>>>>>    #define MII_DP83867_MICR    0x12
>>>>>>>    #define MII_DP83867_ISR        0x13
>>>>>>>    #define DP83867_CFG2        0x14
>>>>>>> @@ -118,6 +119,15 @@
>>>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>>>>>    +/* PHY STS bits */
>>>>>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>>>>>> +#define DP83867_PHYSTS_100            BIT(14)
>>>>>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>>>>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>>>>>> +
>>>>>>> +/* CFG2 bits */
>>>>>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>>>>>> +
>>>>>>>    /* CFG3 bits */
>>>>>>>    #define DP83867_CFG3_INT_OE            BIT(7)
>>>>>>>    #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct 
>>>>>>> phy_device
>>>>>>> *phydev)
>>>>>>>        return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>>>>>    }
>>>>>>>    +static void dp83867_link_change_notify(struct phy_device 
>>>>>>> *phydev)
>>>>>>> +{
>>>>>>> +    if (phydev->state != PHY_RUNNING)
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>>>>>> +        phydev_warn(phydev, "Downshift detected connection is
>>>>>>> %iMbps\n",
>>>>>>> +                phydev->speed);
>>>>>> The link partner may simply not advertise 1Gbps. How do you know 
>>>>>> that
>>>>>> a link speed of e.g. 100Mbps is caused by a downshift?
>>>>>> Some PHY's I've seen with this feature have a flag somewhere 
>>>>>> indicating
>>>>>> that downshift occurred. How about the PHY here?
>>>>> I don't see a register that gives us that status
>>>>>
>>>>> I will ask the hardware team if there is one.
>>>>>
>>>>> This is a 1Gbps PHY by default so if a slower connection is 
>>>>> established
>>>>> due to faulty cabling or LP advertisement then this would be a down
>>>>> shift IMO.
>>>> With your current link_change_notify function it would not be possible
>>>> to know whether the PHY was connected to a link partner that 
>>>> advertised
>>>> only 10/100 and so 100 ended up being the link speed, or the link
>>>> partner was capable of 10/100/1000 and downshift reduced the link 
>>>> speed.
>>>>
>>>> If you cannot tell the difference from a register, it might be 
>>>> better to
>>>> simply omit that function then.
>>>
>>> Yeah I thought it was a bit redundant and wonky to see in the log 
>>> that the link established to xG/Mbps and then see another message 
>>> saying the downshift occurred.
>>
>> I think it's good idea to have this message as just wrong cable might 
>> be used.
>>
>> But this notifier make no sense in it current form - it will produce 
>> noise in case of forced 100m/10M.
>>
>> FYI. PHY sequence to update link:
>> phy_state_machine()
>> |-phy_check_link_status()
>>   |-phy_link_down/up()
>>     |- .phy_link_change()->phy_link_change()
>>     |-adjust_link() ----> netdev callback
>> |-phydev->drv->link_change_notify(phydev);
>>
>> So, log output has to be done or in .read_status() or
>> some info has to be saved in .read_status() and then re-used in
>> .link_change_notify().
>>
> OK I will try to find a way to give some sort of message.
>
> Also we did get confirmation from HW guys and you also confirmed that 
> the number of attempts for downshift is configurable.  So I will be 
> adding back the tunable code once net-next opens.
>
I worked on this a bit and I think the notification is a bit complicated 
to get into the code just to print a message.  First the notification 
comes from the interrupt register which is COR. So if I read the 
interrupt register in read_status then the ack_interrupt call back won't 
do anything and status will be lost so if we need to implement other 
features that depend on the interrupt status that status is cleared.  In 
addition the downshift interrupt will be read and cleared so the state 
of any downshift is lost after the message.  The link_change_notifier is 
called first then the ack_interrupt function is called so as I stated 
the downshift status will be reset to no downshift as the bit is 
cleared.  So I don't think adding this notifier is worth the complex 
code to print a message.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Russell King - ARM Linux admin]

On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
> Grygorii
> 
> On 2/14/20 12:32 PM, Grygorii Strashko wrote:
> > I think it's good idea to have this message as just wrong cable might be
> > used.
> > 
> > But this notifier make no sense in it current form - it will produce
> > noise in case of forced 100m/10M.
> > 
> > FYI. PHY sequence to update link:
> > phy_state_machine()
> > |-phy_check_link_status()
> >   |-phy_link_down/up()
> >     |- .phy_link_change()->phy_link_change()
> >     |-adjust_link() ----> netdev callback
> > |-phydev->drv->link_change_notify(phydev);
> > 
> > So, log output has to be done or in .read_status() or
> > some info has to be saved in .read_status() and then re-used in
> > .link_change_notify().
> > 
> OK I will try to find a way to give some sort of message.

How do you know the speed that the PHY downshifted to?

If the speed and duplex are available in some PHY specific status
register, then one way you can detect downshift is to decode the
negotiated speed/duplex from the advertisements (specifically the LPA
read from the registers and the advertisement that we should be
advertising - some PHYs modify their registers when downshifting) and
check whether it matches the negotiated parameters in the PHY
specific status register.

Alternatively, if the PHY modifies the advertisement register on
downshift, comparing the advertisement register with what it should
be will tell you if downshift has occurred.

Note, however, that if both ends of the link are capable of
downshift, and they downshift at the same time, it can be difficult
to reliably tell whether the downshift was performed by the local
PHY or the remote PHY - even if the local PHY gives you status bits
for downshift, you won't know if the remote end downshifted instead.

It's a bit like auto MDI/MDIX - if pairswap is needed, either end
may do it, and which end does it may change each time the link comes
up.

So, reporting downshift in the kernel log may not be all that useful,
it may be more suited to being reported through a future ethtool
interface just like MDI/MDIX.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Russell

On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote:
> On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
>> Grygorii
>>
>> On 2/14/20 12:32 PM, Grygorii Strashko wrote:
>>> I think it's good idea to have this message as just wrong cable might be
>>> used.
>>>
>>> But this notifier make no sense in it current form - it will produce
>>> noise in case of forced 100m/10M.
>>>
>>> FYI. PHY sequence to update link:
>>> phy_state_machine()
>>> |-phy_check_link_status()
>>>    |-phy_link_down/up()
>>>      |- .phy_link_change()->phy_link_change()
>>>      |-adjust_link() ----> netdev callback
>>> |-phydev->drv->link_change_notify(phydev);
>>>
>>> So, log output has to be done or in .read_status() or
>>> some info has to be saved in .read_status() and then re-used in
>>> .link_change_notify().
>>>
>> OK I will try to find a way to give some sort of message.
> How do you know the speed that the PHY downshifted to?

The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed 
that the PHY negotiated.

In the same register BIT 13 indicates the duplex mode.

> If the speed and duplex are available in some PHY specific status
> register, then one way you can detect downshift is to decode the
> negotiated speed/duplex from the advertisements (specifically the LPA
> read from the registers and the advertisement that we should be
> advertising - some PHYs modify their registers when downshifting) and
> check whether it matches the negotiated parameters in the PHY
> specific status register.
>
> Alternatively, if the PHY modifies the advertisement register on
> downshift, comparing the advertisement register with what it should
> be will tell you if downshift has occurred.

The ISR register BIT 5 indicates if a downshift occurred or not. So we 
can indicate that the PHY downshifted but there is no cause in the 
registers bit field.  My concern for this bit though is the register is 
clear on read so all other interrupts are lost if we only read to check 
downshift.  And the link_change_notifier is called before the interrupt 
ACK call back.  We could call the interrupt function and get the 
downshift status but again it will clear the interrupt register and any 
other statuses may be lost.

Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Russell King - ARM Linux admin]

On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote:
> Russell
> 
> On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
> > > Grygorii
> > > 
> > > On 2/14/20 12:32 PM, Grygorii Strashko wrote:
> > > > I think it's good idea to have this message as just wrong cable might be
> > > > used.
> > > > 
> > > > But this notifier make no sense in it current form - it will produce
> > > > noise in case of forced 100m/10M.
> > > > 
> > > > FYI. PHY sequence to update link:
> > > > phy_state_machine()
> > > > |-phy_check_link_status()
> > > >    |-phy_link_down/up()
> > > >      |- .phy_link_change()->phy_link_change()
> > > >      |-adjust_link() ----> netdev callback
> > > > |-phydev->drv->link_change_notify(phydev);
> > > > 
> > > > So, log output has to be done or in .read_status() or
> > > > some info has to be saved in .read_status() and then re-used in
> > > > .link_change_notify().
> > > > 
> > > OK I will try to find a way to give some sort of message.
> > How do you know the speed that the PHY downshifted to?
> 
> The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that
> the PHY negotiated.
> 
> In the same register BIT 13 indicates the duplex mode.
> 
> > If the speed and duplex are available in some PHY specific status
> > register, then one way you can detect downshift is to decode the
> > negotiated speed/duplex from the advertisements (specifically the LPA
> > read from the registers and the advertisement that we should be
> > advertising - some PHYs modify their registers when downshifting) and
> > check whether it matches the negotiated parameters in the PHY
> > specific status register.
> > 
> > Alternatively, if the PHY modifies the advertisement register on
> > downshift, comparing the advertisement register with what it should
> > be will tell you if downshift has occurred.
> 
> The ISR register BIT 5 indicates if a downshift occurred or not. So we can
> indicate that the PHY downshifted but there is no cause in the registers bit
> field.  My concern for this bit though is the register is clear on read so
> all other interrupts are lost if we only read to check downshift.  And the
> link_change_notifier is called before the interrupt ACK call back.  We could
> call the interrupt function and get the downshift status but again it will
> clear the interrupt register and any other statuses may be lost.

What's wrong with having an ack_interrupt() method that reads the
PHY ISR register, and records in a driver private flag that bit 5
has been set?  The read_status() method can clear the flag if link
goes down, or check the flag if link is up and report that a
downshift event occurred.

If IRQs are not in use, then read_status() would have to read the
ISR itself.

It may be better to move ack_interrupt() to did_interrupt(), which
will ensure that it gets executed before the PHY state machine is
triggered by phy_interrupt().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Russell

On 2/18/20 10:49 AM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote:
>> Russell
>>
>> On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote:
>>> On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
>>>> Grygorii
>>>>
>>>> On 2/14/20 12:32 PM, Grygorii Strashko wrote:
>>>>> I think it's good idea to have this message as just wrong cable might be
>>>>> used.
>>>>>
>>>>> But this notifier make no sense in it current form - it will produce
>>>>> noise in case of forced 100m/10M.
>>>>>
>>>>> FYI. PHY sequence to update link:
>>>>> phy_state_machine()
>>>>> |-phy_check_link_status()
>>>>>     |-phy_link_down/up()
>>>>>       |- .phy_link_change()->phy_link_change()
>>>>>       |-adjust_link() ----> netdev callback
>>>>> |-phydev->drv->link_change_notify(phydev);
>>>>>
>>>>> So, log output has to be done or in .read_status() or
>>>>> some info has to be saved in .read_status() and then re-used in
>>>>> .link_change_notify().
>>>>>
>>>> OK I will try to find a way to give some sort of message.
>>> How do you know the speed that the PHY downshifted to?
>> The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that
>> the PHY negotiated.
>>
>> In the same register BIT 13 indicates the duplex mode.
>>
>>> If the speed and duplex are available in some PHY specific status
>>> register, then one way you can detect downshift is to decode the
>>> negotiated speed/duplex from the advertisements (specifically the LPA
>>> read from the registers and the advertisement that we should be
>>> advertising - some PHYs modify their registers when downshifting) and
>>> check whether it matches the negotiated parameters in the PHY
>>> specific status register.
>>>
>>> Alternatively, if the PHY modifies the advertisement register on
>>> downshift, comparing the advertisement register with what it should
>>> be will tell you if downshift has occurred.
>> The ISR register BIT 5 indicates if a downshift occurred or not. So we can
>> indicate that the PHY downshifted but there is no cause in the registers bit
>> field.  My concern for this bit though is the register is clear on read so
>> all other interrupts are lost if we only read to check downshift.  And the
>> link_change_notifier is called before the interrupt ACK call back.  We could
>> call the interrupt function and get the downshift status but again it will
>> clear the interrupt register and any other statuses may be lost.
> What's wrong with having an ack_interrupt() method that reads the
> PHY ISR register, and records in a driver private flag that bit 5
> has been set?  The read_status() method can clear the flag if link
> goes down, or check the flag if link is up and report that a
> downshift event occurred.
>
> If IRQs are not in use, then read_status() would have to read the
> ISR itself.
>
> It may be better to move ack_interrupt() to did_interrupt(), which
> will ensure that it gets executed before the PHY state machine is
> triggered by phy_interrupt().
>
Well now the read_status is becoming a lot more complex.  It would be 
better to remove the ack_interrupt call back and just have read_status 
call the interrupt function regardless of interrupts or not.  Because 
all the interrupt function would be doing is clearing all the interrupts 
in the ISR register on a link up/down event.  And as you pointed out we 
can check and set a flag that indicates if a downshift has happened on 
link up status and clear it on link down. We would need to set the 
downshift interrupt mask to always report that bit.  As opposed to not 
setting any interrupts if interrupts are not enabled.  If the user wants 
to track WoL interrupt or any other feature interrupt we would have to 
add that flag to the read_status as well seems like it could get a bit 
out of control.

Again this is a lot of error prone complex changes and tracking just to 
populate a message in the kernel log.  There is no guarantee that the LP 
did not force the downshift or advertise that it supports 1Gbps.  So 
what condition is really being reported here?  There seems like there 
are so many different scenarios why the PHY could not negotiate to its 
advertised 1Gbps.


Dan

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Russell King - ARM Linux admin]

On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote:
> Well now the read_status is becoming a lot more complex.  It would be better
> to remove the ack_interrupt call back and just have read_status call the
> interrupt function regardless of interrupts or not.  Because all the
> interrupt function would be doing is clearing all the interrupts in the ISR
> register on a link up/down event.  And as you pointed out we can check and
> set a flag that indicates if a downshift has happened on link up status and
> clear it on link down. We would need to set the downshift interrupt mask to
> always report that bit.  As opposed to not setting any interrupts if
> interrupts are not enabled.  If the user wants to track WoL interrupt or any
> other feature interrupt we would have to add that flag to the read_status as
> well seems like it could get a bit out of control.

To be honest, I don't like phylib's interrupt implementation; it
imposes a fixed way of dealing with interrupts on phylib drivers,
and it sounds like its ideas don't match what modern PHYs want.

For example, the Marvell 88x3310 can produce interrupts for GPIOs
that are on-board, or temperature alarms, or other stuff... but
phylib's idea is that a PHY interrupt is for signalling that the
link changed somehow, and imposes a did_interrupt(), handle
interrupt, clear_interrupt() structure whether or not it's
suitable for the PHY.  If you don't provide the functions phylib
wants, then phydev->irq gets killed.  Plus, phylib claims the
interrupt for you at certain points depending on whether the
PHY is bound to a network interface or not.

So, my suggestion to move ack_interrupt to did_interrupt will
result in phylib forcing phydev->irq to PHY_POLL, killing any
support for interrupts what so ever...

> Again this is a lot of error prone complex changes and tracking just to
> populate a message in the kernel log.  There is no guarantee that the LP did
> not force the downshift or advertise that it supports 1Gbps.  So what
> condition is really being reported here?  There seems like there are so many
> different scenarios why the PHY could not negotiate to its advertised 1Gbps.

Note that when a PHY wants to downshift, it does that by changing its
advertisement that is sent to the other PHY.

So, if both ends support 1G, 100M and 10M, and the remote end decides
to downshift to 100M, the remote end basically stops advertising 1G
and restarts negotiation afresh with the new advertisement.

In that case, if you look at ethtool's output, it will indicate that
the link partner is not advertising 1G, and the link is operating at
100M.

If 100M doesn't work (and there are cases where connections become
quite flakey) then 100M may also be omitted as well, negotiation
restarted, causing a downshift to 10M.

That's basically how downshift works - a PHY will attempt to
establish link a number of times before deciding to restart
negotiation with some of the advertisement omitted, and the
reduced negotiation advertisement appears in the remote PHY's
link partner registers.

As I mentioned, the PHY on either end of the link can be the one
which decides to downshift, and the partner PHY has no idea that
a downshift has happened.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Dan Murphy]

Russell

On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote:
>> Well now the read_status is becoming a lot more complex.  It would be better
>> to remove the ack_interrupt call back and just have read_status call the
>> interrupt function regardless of interrupts or not.  Because all the
>> interrupt function would be doing is clearing all the interrupts in the ISR
>> register on a link up/down event.  And as you pointed out we can check and
>> set a flag that indicates if a downshift has happened on link up status and
>> clear it on link down. We would need to set the downshift interrupt mask to
>> always report that bit.  As opposed to not setting any interrupts if
>> interrupts are not enabled.  If the user wants to track WoL interrupt or any
>> other feature interrupt we would have to add that flag to the read_status as
>> well seems like it could get a bit out of control.
> To be honest, I don't like phylib's interrupt implementation; it
> imposes a fixed way of dealing with interrupts on phylib drivers,
> and it sounds like its ideas don't match what modern PHYs want.
>
> For example, the Marvell 88x3310 can produce interrupts for GPIOs
> that are on-board, or temperature alarms, or other stuff... but
> phylib's idea is that a PHY interrupt is for signalling that the
> link changed somehow, and imposes a did_interrupt(), handle
> interrupt, clear_interrupt() structure whether or not it's
> suitable for the PHY.  If you don't provide the functions phylib
> wants, then phydev->irq gets killed.  Plus, phylib claims the
> interrupt for you at certain points depending on whether the
> PHY is bound to a network interface or not.
>
> So, my suggestion to move ack_interrupt to did_interrupt will
> result in phylib forcing phydev->irq to PHY_POLL, killing any
> support for interrupts what so ever...

Does it really make sense to do all of this for a downshift message in 
the kernel log?

>> Again this is a lot of error prone complex changes and tracking just to
>> populate a message in the kernel log.  There is no guarantee that the LP did
>> not force the downshift or advertise that it supports 1Gbps.  So what
>> condition is really being reported here?  There seems like there are so many
>> different scenarios why the PHY could not negotiate to its advertised 1Gbps.
> Note that when a PHY wants to downshift, it does that by changing its
> advertisement that is sent to the other PHY.
>
> So, if both ends support 1G, 100M and 10M, and the remote end decides
> to downshift to 100M, the remote end basically stops advertising 1G
> and restarts negotiation afresh with the new advertisement.
>
> In that case, if you look at ethtool's output, it will indicate that
> the link partner is not advertising 1G, and the link is operating at
> 100M.
>
> If 100M doesn't work (and there are cases where connections become
> quite flakey) then 100M may also be omitted as well, negotiation
> restarted, causing a downshift to 10M.
>
> That's basically how downshift works - a PHY will attempt to
> establish link a number of times before deciding to restart
> negotiation with some of the advertisement omitted, and the
> reduced negotiation advertisement appears in the remote PHY's
> link partner registers.
This is the way I understand it too.
> As I mentioned, the PHY on either end of the link can be the one
> which decides to downshift, and the partner PHY has no idea that
> a downshift has happened.
>
Exactly so we can only report that if the PHY on our end caused the 
downshift.  If this PHY does not cause the downshift then the message 
will not be presented even though a downshift occurred. So what is the 
value in presenting this message?

Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Russell King - ARM Linux admin]

On Tue, Feb 18, 2020 at 11:38:33AM -0600, Dan Murphy wrote:
> Russell
> 
> On 2/18/20 11:33 AM, Russell King - ARM Linux admin wrote:
> > As I mentioned, the PHY on either end of the link can be the one
> > which decides to downshift, and the partner PHY has no idea that
> > a downshift has happened.
> > 
> Exactly so we can only report that if the PHY on our end caused the
> downshift.  If this PHY does not cause the downshift then the message will
> not be presented even though a downshift occurred. So what is the value in
> presenting this message?

I think we are in agreement over questioning the value of reporting
the downshift!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up