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

[Vladimir Oltean <olteanv@gmail.com>]

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@gmail.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
>