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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y4Ofvemx5AnWJHrp@shell.armlinux.org.uk>
Date:   Sun, 27 Nov 2022 17:34:53 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 0/3] net: pcs: altera-tse: simplify and clean-up
 the driver

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!