linux-kernel - Re: [net-next PATCH 03/19] net: dsa: qca8k: skip sgmii delay on double cpu conf

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: <20211119005826.omgn7pj3cx3lwwr7@skbuf>
Date:   Fri, 19 Nov 2021 02:58:26 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH 03/19] net: dsa: qca8k: skip sgmii delay on
 double cpu conf

On Wed, Nov 17, 2021 at 10:04:35PM +0100, Ansuel Smith wrote:
> With a dual cpu configuration rgmii+sgmii, skip configuring sgmii delay
> as it does cause no traffic. Configure only rgmii delay in this
> specific configuration.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> ---

I feel that you owe a serious explanation about this SGMII delay
business, it's getting stranger and stranger. I chalked it up to the
fact that maybe QCA8334 has a strange SGMII implementation, where the
clock it is source-synchronous, as opposed to the data lanes themselves
being self-clocking. But the fact is, I don't really know, I never was
sure, never got an explanation, and now I am even less sure. And now
that I look in the datasheet for the pinout, I see a regular pair of
pins (SOP, SON) for the TX differential pair, and a regular pair of pins
(SIP, SIN) for the RX differential pair. No pin for any source
synchronous clock. So where is this SGMII_CLK125M clock localized, and
if it's inside the switch, and why do you need to configure its sampling
edge and delay, what is different between one board and another?

The RGMII and the SGMII CPU ports are different physical ports, are they not?
Why would the configuration of one affect the other? Do they share any
physical resource?

>  drivers/net/dsa/qca8k.c | 12 ++++++++++--
>  drivers/net/dsa/qca8k.h |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index bfffc1fb7016..ace465c878f8 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1041,8 +1041,13 @@ qca8k_parse_port_config(struct qca8k_priv *priv)
>  			if (mode == PHY_INTERFACE_MODE_RGMII ||
>  			    mode == PHY_INTERFACE_MODE_RGMII_ID ||
>  			    mode == PHY_INTERFACE_MODE_RGMII_TXID ||
> -			    mode == PHY_INTERFACE_MODE_RGMII_RXID)
> +			    mode == PHY_INTERFACE_MODE_RGMII_RXID) {
> +				if (priv->ports_config.rgmii_tx_delay[cpu_port_index] ||
> +				    priv->ports_config.rgmii_rx_delay[cpu_port_index])
> +					priv->ports_config.skip_sgmii_delay = true;
> +
>  				break;
> +			}
>  
>  			if (of_property_read_bool(port_dn, "qca,sgmii-txclk-falling-edge"))
>  				priv->ports_config.sgmii_tx_clk_falling_edge = true;
> @@ -1457,8 +1462,11 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  
>  		/* From original code is reported port instability as SGMII also
>  		 * require delay set. Apply advised values here or take them from DT.
> +		 * In dual CPU configuration, apply only delay to rgmii as applying
> +		 * it also to the SGMII line cause no traffic to the entire switch.
>  		 */
> -		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> +		if (state->interface == PHY_INTERFACE_MODE_SGMII &&
> +		    !priv->ports_config.skip_sgmii_delay)

I thought that the deal was that with the new "tx-internal-delay-ps" and
"rx-internal-delay-ps" properties, you would not enable any delays by
default in their absence. So if system is broken by the fact that delays
are applied on the SGMII port when they shouldn't have, the issue is in
the device tree and the fix is also there. This "skip_sgmii_delay" logic
on the other hand is fixing up the default delays that get applied in
lack of device tree properties.

>  			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
>  
>  		break;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 128b8cf85e08..57c4c0d93558 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -275,6 +275,7 @@ struct qca8k_ports_config {
>  	bool sgmii_rx_clk_falling_edge;
>  	bool sgmii_tx_clk_falling_edge;
>  	bool sgmii_enable_pll;
> +	bool skip_sgmii_delay;
>  	u8 rgmii_rx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
>  	u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
>  };
> -- 
> 2.32.0
>