[PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up the driver

[PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up the driver

[Maxime Chevallier]

Hello everyone,

This small series does a bit of code cleanup in the altera TSE pcs
driver, removong unused register definitions, handling 1000BaseX speed
configuration correctly according to the datasheet, and making use of
proper poll_timeout helpers.

No functional change is introduced.

Best regards,

Maxime

Maxime Chevallier (3):
  net: pcs: altera-tse: use read_poll_timeout to wait for reset
  net: pcs: altera-tse: don't set the speed for 1000BaseX
  net: pcs: altera-tse: remove unnecessary register definitions

 drivers/net/pcs/pcs-altera-tse.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

-- 
2.38.1

[PATCH net-next 1/3] net: pcs: altera-tse: use read_poll_timeout to wait for reset

[Maxime Chevallier]

Software resets on the TSE PCS don't clear registers, but rather reset
all internal state machines regarding AN, comma detection and
encoding/decoding. Use read_poll_timeout to wait for the reset to clear
instead of manually polling the register.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/pcs/pcs-altera-tse.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
index 97a7cabff962..e86cadc391e8 100644
--- a/drivers/net/pcs/pcs-altera-tse.c
+++ b/drivers/net/pcs/pcs-altera-tse.c
@@ -60,7 +60,6 @@ static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int regnum,
 
 static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
 {
-	int i = 0;
 	u16 bmcr;
 
 	/* Reset PCS block */
@@ -68,13 +67,9 @@ static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
 	bmcr |= BMCR_RESET;
 	tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
 
-	for (i = 0; i < SGMII_PCS_SW_RESET_TIMEOUT; i++) {
-		if (!(tse_pcs_read(tse_pcs, MII_BMCR) & BMCR_RESET))
-			return 0;
-		udelay(1);
-	}
-
-	return -ETIMEDOUT;
+	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
+				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
+				 tse_pcs, MII_BMCR);
 }
 
 static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
-- 
2.38.1

[PATCH net-next 2/3] net: pcs: altera-tse: don't set the speed for 1000BaseX

[Maxime Chevallier]

When disabling the SGMII mode bit, the PCS defaults to 1000BaseX mode.
In that mode, we don't need to set the speed since it's always 1000Mbps.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/pcs/pcs-altera-tse.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
index e86cadc391e8..be65271ff5de 100644
--- a/drivers/net/pcs/pcs-altera-tse.c
+++ b/drivers/net/pcs/pcs-altera-tse.c
@@ -102,7 +102,6 @@ static int alt_tse_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 		if_mode |= PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA;
 	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
 		if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA);
-		if_mode |= PCS_IF_MODE_SGMI_SPEED_1000;
 	}
 
 	ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
-- 
2.38.1

[PATCH net-next 3/3] net: pcs: altera-tse: remove unnecessary register definitions

[Maxime Chevallier]

remove unused register definitions, left from the split with the
altera-tse mac driver.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/pcs/pcs-altera-tse.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
index be65271ff5de..d616749761f4 100644
--- a/drivers/net/pcs/pcs-altera-tse.c
+++ b/drivers/net/pcs/pcs-altera-tse.c
@@ -12,22 +12,13 @@
 
 /* SGMII PCS register addresses
  */
-#define SGMII_PCS_SCRATCH	0x10
-#define SGMII_PCS_REV		0x11
 #define SGMII_PCS_LINK_TIMER_0	0x12
-#define   SGMII_PCS_LINK_TIMER_REG(x)		(0x12 + (x))
 #define SGMII_PCS_LINK_TIMER_1	0x13
 #define SGMII_PCS_IF_MODE	0x14
 #define   PCS_IF_MODE_SGMII_ENA		BIT(0)
 #define   PCS_IF_MODE_USE_SGMII_AN	BIT(1)
-#define   PCS_IF_MODE_SGMI_SPEED_MASK	GENMASK(3, 2)
-#define   PCS_IF_MODE_SGMI_SPEED_10	(0 << 2)
-#define   PCS_IF_MODE_SGMI_SPEED_100	(1 << 2)
-#define   PCS_IF_MODE_SGMI_SPEED_1000	(2 << 2)
 #define   PCS_IF_MODE_SGMI_HALF_DUPLEX	BIT(4)
 #define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)
-#define SGMII_PCS_DIS_READ_TO	0x15
-#define SGMII_PCS_READ_TO	0x16
 #define SGMII_PCS_SW_RESET_TIMEOUT 100 /* usecs */
 
 struct altera_tse_pcs {
-- 
2.38.1

Re: [PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up the driver

[Russell King (Oracle)]

On Fri, Nov 25, 2022 at 02:17:58PM +0100, Maxime Chevallier wrote:
> Hello everyone,
> 
> This small series does a bit of code cleanup in the altera TSE pcs
> driver, removong unused register definitions, handling 1000BaseX speed
> configuration correctly according to the datasheet, and making use of
> proper poll_timeout helpers.
> 
> No functional change is introduced.
> 
> Best regards,
> 
> Maxime
> 
> Maxime Chevallier (3):
>   net: pcs: altera-tse: use read_poll_timeout to wait for reset
>   net: pcs: altera-tse: don't set the speed for 1000BaseX
>   net: pcs: altera-tse: remove unnecessary register definitions

Hi Maxime,

Please can you check the link timer settings:

        /* Set link timer to 1.6ms, as per the MegaCore Function User Guide */
        tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, 0x0D40);
        tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, 0x03);

This is true for Cisco SGMII mode - which is specified to use a 1.6ms
link timer, but 1000baseX is specified by 802.3 to use a 10ms link
timer interval, so this is technically incorrect for 1000base-X. So,
if the MegaCore Function User Guide specifies 1.6ms for everything, it
would appear to contradict 802.3.

From what I gather from the above, the link timer uses a value of
200000 for 1.6ms, which means it is using a 8ns clock period or 125MHz.

If you wish to correct the link timer, you can use this:

	int link_timer;

	link_timer = phylink_get_link_timer_ns(interface) / 8;
	if (link_timer > 0) {
		tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, link_timer);
		tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, link_timer >> 16);
	}

so that it gets set correctly depending on 'interface'.
phylink_get_link_timer_ns() is an inline static function, so you
should end up with the above fairly optimised, not that it really
matters. Also worth documenting that the "8" there is 125MHz in
nanoseconds - maybe in a similar way to pcs-lynx does.

It does look like this Altera TSE PCS is very similar to pcs-lynx,
maybe there's a possibility of refactoring both drivers to share
code?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up the driver

[Maxime Chevallier]

Hello Russell,

On Sun, 27 Nov 2022 17:34:53 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Fri, Nov 25, 2022 at 02:17:58PM +0100, Maxime Chevallier wrote:
> > Hello everyone,
> > 
> > This small series does a bit of code cleanup in the altera TSE pcs
> > driver, removong unused register definitions, handling 1000BaseX
> > speed configuration correctly according to the datasheet, and
> > making use of proper poll_timeout helpers.
> > 
> > No functional change is introduced.
> > 
> > Best regards,
> > 
> > Maxime
> > 
> > Maxime Chevallier (3):
> >   net: pcs: altera-tse: use read_poll_timeout to wait for reset
> >   net: pcs: altera-tse: don't set the speed for 1000BaseX
> >   net: pcs: altera-tse: remove unnecessary register definitions  
> 
> Hi Maxime,
> 
> Please can you check the link timer settings:
> 
>         /* Set link timer to 1.6ms, as per the MegaCore Function User
> Guide */ tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, 0x0D40);
>         tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, 0x03);
> 
> This is true for Cisco SGMII mode - which is specified to use a 1.6ms
> link timer, but 1000baseX is specified by 802.3 to use a 10ms link
> timer interval, so this is technically incorrect for 1000base-X. So,
> if the MegaCore Function User Guide specifies 1.6ms for everything, it
> would appear to contradict 802.3.
> 
> From what I gather from the above, the link timer uses a value of
> 200000 for 1.6ms, which means it is using a 8ns clock period or
> 125MHz.
> 
> If you wish to correct the link timer, you can use this:
> 
> 	int link_timer;
> 
> 	link_timer = phylink_get_link_timer_ns(interface) / 8;
> 	if (link_timer > 0) {
> 		tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0,
> link_timer); tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1,
> link_timer >> 16); }
> 
> so that it gets set correctly depending on 'interface'.
> phylink_get_link_timer_ns() is an inline static function, so you
> should end up with the above fairly optimised, not that it really
> matters. Also worth documenting that the "8" there is 125MHz in
> nanoseconds - maybe in a similar way to pcs-lynx does.

Ouh that's perfect, thanks !

> It does look like this Altera TSE PCS is very similar to pcs-lynx,
> maybe there's a possibility of refactoring both drivers to share
> code?

Indeed, I've some patches I'm testing to port pcs-lynx to regmap then
do the merge. The one thing that would differ is the reset
handling in the TSE driver, since we need to perform it at every
configuration change basically. But that's not worth having duplicate
drivers just for that, I agree.

I'll post that merge when I'll have the chance to give it a more
thourough test.

Thanks,

Maxime

Re: [PATCH net-next 1/3] net: pcs: altera-tse: use read_poll_timeout to wait for reset

[Jakub Kicinski]

On Fri, 25 Nov 2022 14:17:59 +0100 Maxime Chevallier wrote:
> -	for (i = 0; i < SGMII_PCS_SW_RESET_TIMEOUT; i++) {
> -		if (!(tse_pcs_read(tse_pcs, MII_BMCR) & BMCR_RESET))
> -			return 0;
> -		udelay(1);
> -	}
> -
> -	return -ETIMEDOUT;
> +	return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
> +				 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
> +				 tse_pcs, MII_BMCR);

You say "no functional change intended" in the cover letter but you
switch from udelay to usleep and change timeouts here. I presume this
is intentional but should be mentioned in the commit message, I think.

Re: [PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up the driver

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 25 Nov 2022 14:17:58 +0100 you wrote:
> Hello everyone,
> 
> This small series does a bit of code cleanup in the altera TSE pcs
> driver, removong unused register definitions, handling 1000BaseX speed
> configuration correctly according to the datasheet, and making use of
> proper poll_timeout helpers.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: pcs: altera-tse: use read_poll_timeout to wait for reset
    https://git.kernel.org/netdev/net-next/c/d1a0ff5ff9ef
  - [net-next,2/3] net: pcs: altera-tse: don't set the speed for 1000BaseX
    https://git.kernel.org/netdev/net-next/c/b4a7bf9f5bb8
  - [net-next,3/3] net: pcs: altera-tse: remove unnecessary register definitions
    https://git.kernel.org/netdev/net-next/c/befd851de295

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html