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

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

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!

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