The Microsemi PHYs have several counters so let's make them available as PHY
statistics.
The VSC 8530/31/40/41 also need to update their EEE init sequence in order to
avoid packet losses and improve performance.
This patch series also makes some minor cosmetic changes to the driver.
Quentin Schulz (3):
net: phy: mscc: remove unneeded parenthesis
net: phy: mscc: shorten `x != 0` condition to `x`
net: phy: mscc: remove unneeded temporary variable
Raju Lakkaraju (2):
net: phy: mscc: add ethtool statistics counters
net: phy: mscc: Add EEE init sequence
drivers/net/phy/mscc.c | 229 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 202 insertions(+), 27 deletions(-)
base-commit: 00989856964175eafbe1435a70862c2ac66cffc0
--
git-series 0.9.1
Here, the rc variable is either used only for the condition right after
the assignment or right before being used as the return value of the
function it's being used in.
So let's remove this unneeded temporary variable whenever possible.
Signed-off-by: Quentin Schulz <redacted>
---
drivers/net/phy/mscc.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
From: Raju Lakkaraju <redacted>
Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.
Signed-off-by: Raju Lakkaraju <redacted>
Signed-off-by: Quentin Schulz <redacted>
---
drivers/net/phy/mscc.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+)
`if (x != 0)` is basically a more verbose version of `if (x)` so let's
use the latter so it's consistent throughout the whole driver.
Signed-off-by: Quentin Schulz <redacted>
---
drivers/net/phy/mscc.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
@@ -401,7 +401,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,mutex_lock(&phydev->lock);rc=vsc85xx_phy_page_set(phydev,MSCC_PHY_PAGE_EXTENDED_2);-if(rc!=0)+if(rc)gotoout_unlock;if(wol->wolopts&WAKE_MAGIC){
@@ -439,7 +439,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,phy_write(phydev,MSCC_PHY_WOL_MAC_CONTROL,reg_val);rc=vsc85xx_phy_page_set(phydev,MSCC_PHY_PAGE_STANDARD);-if(rc!=0)+if(rc)gotoout_unlock;if(wol->wolopts&WAKE_MAGIC){
@@ -447,14 +447,14 @@ static int vsc85xx_wol_set(struct phy_device *phydev,reg_val=phy_read(phydev,MII_VSC85XX_INT_MASK);reg_val|=MII_VSC85XX_INT_MASK_WOL;rc=phy_write(phydev,MII_VSC85XX_INT_MASK,reg_val);-if(rc!=0)+if(rc)gotoout_unlock;}else{/* Disable the WOL interrupt */reg_val=phy_read(phydev,MII_VSC85XX_INT_MASK);reg_val&=(~MII_VSC85XX_INT_MASK_WOL);rc=phy_write(phydev,MII_VSC85XX_INT_MASK,reg_val);-if(rc!=0)+if(rc)gotoout_unlock;}/* Clear WOL iterrupt status */
From: Raju Lakkaraju <redacted>
There are a few counters available in the PHY: receive errors, false
carriers, link disconnects, media CRC errors and valids counters.
So let's expose those in the PHY driver.
Use the priv structure as the next PHY to be supported has a few
additional counters.
Signed-off-by: Raju Lakkaraju <redacted>
Signed-off-by: Quentin Schulz <redacted>
---
drivers/net/phy/mscc.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 128 insertions(+)
The == operator precedes the || operator, so we can remove the
parenthesis around (a == b) || (c == d).
The condition is rather explicit and short so removing the parenthesis
definitely does not make it harder to read.
Signed-off-by: Quentin Schulz <redacted>
---
drivers/net/phy/mscc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I might of asked this before...
Does changing the page effect registers in the lower range? It is
possible for other operations to happen at the same time, and you
don't want for example a status read to happen from some other
extended page register because a statistics read is happening.
phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-14 13:04:51
On Fri, Sep 14, 2018 at 10:33:45AM +0200, Quentin Schulz wrote:
The == operator precedes the || operator, so we can remove the
parenthesis around (a == b) || (c == d).
The condition is rather explicit and short so removing the parenthesis
definitely does not make it harder to read.
Signed-off-by: Quentin Schulz <redacted>
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-14 13:05:31
On Fri, Sep 14, 2018 at 10:33:46AM +0200, Quentin Schulz wrote:
`if (x != 0)` is basically a more verbose version of `if (x)` so let's
use the latter so it's consistent throughout the whole driver.
Signed-off-by: Quentin Schulz <redacted>
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-14 13:06:23
On Fri, Sep 14, 2018 at 10:33:47AM +0200, Quentin Schulz wrote:
Here, the rc variable is either used only for the condition right after
the assignment or right before being used as the return value of the
function it's being used in.
So let's remove this unneeded temporary variable whenever possible.
Signed-off-by: Quentin Schulz <redacted>
I might of asked this before...
Does changing the page effect registers in the lower range? It is
possible for other operations to happen at the same time, and you
don't want for example a status read to happen from some other
extended page register because a statistics read is happening.
When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.
phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.
That's awesome! Didn't know it existed. Thanks a ton!
Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.
That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations. Couldn't we use the
phy_device mutex instead (as it's currently done in the whole driver)?
Or is it worse/comparable in performance to the suggested solution?
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-14 13:30:07
When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.
quoted
phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.
That's awesome! Didn't know it existed. Thanks a ton!
Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.
That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations.
You can use the lower level locking primatives. See m88e1318_set_wol()
for example.
Couldn't we use the
phy_device mutex instead (as it's currently done in the whole driver)?
Or is it worse/comparable in performance to the suggested solution?
Russell King found a race condition where this breaks. You cannot hold
the phy_device mutex everywhere.
Andrew
The == operator precedes the || operator, so we can remove the
parenthesis around (a == b) || (c == d).
The condition is rather explicit and short so removing the parenthesis
definitely does not make it harder to read.
Signed-off-by: Quentin Schulz <redacted>
`if (x != 0)` is basically a more verbose version of `if (x)` so let's
use the latter so it's consistent throughout the whole driver.
Signed-off-by: Quentin Schulz <redacted>
Here, the rc variable is either used only for the condition right after
the assignment or right before being used as the return value of the
function it's being used in.
So let's remove this unneeded temporary variable whenever possible.
Signed-off-by: Quentin Schulz <redacted>
From: Raju Lakkaraju <redacted>
Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.
Signed-off-by: Raju Lakkaraju <redacted>
Signed-off-by: Quentin Schulz <redacted>
---
[snip]
+ vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
Can you just make this an array of register + value pair? That would be
less error prone in case you need to update that sequence in the future.
With that:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
Hi Florian,
On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
On 09/14/18 01:33, Quentin Schulz wrote:
quoted
From: Raju Lakkaraju <redacted>
Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.
Signed-off-by: Raju Lakkaraju <redacted>
Signed-off-by: Quentin Schulz <redacted>
---
[snip]
quoted
+ vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
Can you just make this an array of register + value pair? That would be
Sure, I'll.
less error prone in case you need to update that sequence in the future.
I'm curious about the kind of errors you're worrying about or have
experienced. Do you have any particular example or thought in mind?
With that:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hi Florian,
On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
quoted
On 09/14/18 01:33, Quentin Schulz wrote:
quoted
From: Raju Lakkaraju <redacted>
Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.
Signed-off-by: Raju Lakkaraju <redacted>
Signed-off-by: Quentin Schulz <redacted>
---
[snip]
quoted
+ vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
Can you just make this an array of register + value pair? That would be
Sure, I'll.
quoted
less error prone in case you need to update that sequence in the future.
I'm curious about the kind of errors you're worrying about or have
experienced. Do you have any particular example or thought in mind?
Since this is just a completely non documented sequence likely given
as-is by the vendor, there could be in the future an arbitrary number of
changes made to that sequence because reasons. It seems to me that
putting that sequence in an array, instead of having to produce the
right sequence of calls, inlined in the source, is more manageable, and
will lead to an easier process if back porting/forward porting is necessary.
--
Florian
Hi Russel,
Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.
On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
quoted
When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.
quoted
phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.
That's awesome! Didn't know it existed. Thanks a ton!
Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.
That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations.
You can use the lower level locking primatives. See m88e1318_set_wol()
for example.
I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.
Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.
Secondly, I should refactor the driver to do the following:
oldpage = phy_select_page();
if (oldpage < 0) {
phy_restore_page();
error_path;
}
[...]
/* paged accesses */
__phy_write/read();
[...]
phy_restore_page();
I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).
Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].
The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.
ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).
In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.
That would mean that with:
if (ret >= 0 && r < 0)
ret = r;
If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.
One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);
If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.
This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).
Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);
Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);
Is there any reason for not wanting to overwrite the ret value when
__phy_write_page is successful in phy_restore_page?
I'd say that it could be more readable without the ternary condition in
the argument of phy_restore_page.
Let me know your thoughts on this.
Thanks,
Quentin
[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L444
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L454
Hi Russel,
On Tue, Oct 02, 2018 at 03:51:11PM +0200, Quentin Schulz wrote:
Hi Russel,
Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.
On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
quoted
quoted
When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.
quoted
phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.
That's awesome! Didn't know it existed. Thanks a ton!
Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.
That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations.
You can use the lower level locking primatives. See m88e1318_set_wol()
for example.
I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.
Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.
Secondly, I should refactor the driver to do the following:
oldpage = phy_select_page();
if (oldpage < 0) {
phy_restore_page();
error_path;
}
[...]
/* paged accesses */
__phy_write/read();
[...]
phy_restore_page();
I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).
Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].
The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.
ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).
In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.
That would mean that with:
if (ret >= 0 && r < 0)
ret = r;
If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.
One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);
If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.
This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).
Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);
Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);
The whole point was there. We're trying to propagate return values
through phy_restore_page and only overwrite it if it's 0. However, there
are some functions that return something different from 0 (e.g. size of
something that is handled or returned) and are still valid and wanted to
be propagated. If we were to overwrite the return value with 0 if
__phy_write_page is returning 0, we would need to use a temporary
variable to not overwrite the return value before calling
phy_restore_page.
With my suggestion, we would need to use a temporary variable to keep a
0 return values while calling phy_restore_page but not when we want
phy_restore_page to return 0 even when the return value before calling
phy_restore_page is > 0.
With the current behaviour, we would need to use a temporary value (or a
ternary condition as given as an example in original mail) when we want
to return 0 only when no error happens in phy_restore_page and the
return value before calling phy_restore_page was >= 0. We would not need
to use a temporary variable when phy_restore_page finishes without error
and we want to keep the return value before calling phy_restore_page if
it's >=0.
So basically, that's down to a technical choice and none is perfect.
Sorry for bothering.
Thanks,
Quentin