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
remove unused register definitions, left from the split with the
altera-tse mac driver.
Signed-off-by: Maxime Chevallier <[email protected]>
---
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
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)" <[email protected]> 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
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
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