Message-ID: <71a69eb6-9e24-48ab-8301-93ec3ff43cc7@quicinc.com>
Date: Wed, 19 Feb 2025 18:46:57 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
        Jakub Kicinski
	<kuba@...nel.org>
CC: Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller"
	<davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
	<pabeni@...hat.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, Andrew Lunn
	<andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <quic_kkumarcs@...cinc.com>,
        <quic_suruchia@...cinc.com>, <quic_pavir@...cinc.com>,
        <quic_linchen@...cinc.com>, <quic_luoj@...cinc.com>,
        <srinivas.kandagatla@...aro.org>, <bartosz.golaszewski@...aro.org>,
        <vsmuthu@....qualcomm.com>, <john@...ozen.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC



On 2/12/2025 6:19 PM, Russell King (Oracle) wrote:
> On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
>> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
>>> The 'UNIPHY' PCS block in the Qualcomm IPQ9574 SoC provides Ethernet
>>> PCS and SerDes functions. It supports 1Gbps mode PCS and 10-Gigabit
>>> mode PCS (XPCS) functions, and supports various interface modes for
>>> the connectivity between the Ethernet MAC and the external PHYs/Switch.
>>> There are three UNIPHY (PCS) instances in IPQ9574, supporting the six
>>> Ethernet ports.
>>>
>>> This patch series adds base driver support for initializing the PCS,
>>> and PCS phylink ops for managing the PCS modes/states. Support for
>>> SGMII/QSGMII (PCS) and USXGMII (XPCS) modes is being added initially.
>>>
>>> The Ethernet driver which handles the MAC operations will create the
>>> PCS instances and phylink for the MAC, by utilizing the API exported
>>> by this driver.
>>>
>>> While support is being added initially for IPQ9574, the driver is
>>> expected to be easily extendable later for other SoCs in the IPQ
>>> family such as IPQ5332.
>>
>> Could someone with PHY, or even, dare I say, phylink expertise
>> take a look here?
> 
> I've not had the time, sorry. Looking at it now, I have lots of
> questions over this.
> 
> 1) clocks.
> 
> - Patch 2 provides clocks from this driver which are exported to the
>    NSCCC block that are then used to provide the MII clocks.
> - Patch 3 consumes clocks from the NSCCC block for use with each PCS.
> 
> Surely this leads to a circular dependency, where the MSCCC driver
> can't get the clocks it needs until this driver has initialised, but
> this driver can't get the clocks it needs for each PCS from the NSCCC
> because the MSCCC driver needs this driver to initialise.
> 

Sorry for the delay in response. Below is a description of the 
dependencies between the PCS/NSSCC drivers during initialization time 
and how the clock relationships are set up. Based on this, there should 
not any issue due to circular dependency, but please let me know if any 
improvement is possible here given the hardware clock dependency. The 
module loading order is as follows:

Step 1.) NSCC driver module
Step 2.) PCS driver module
Step 3.) Ethernet driver module

The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be 
available at the time of registration of PCS MII clocks (NSSCC to PCS 
MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered 
before 'UNIPHY' PCS clock is registered, since by default the parent is 
initialized to 'xo' clock. Below is the output of clock tree on the 
board before the PCS driver is loaded.

xo-board-clk
     nss_cc_port1_rx_clk_src
         nss_cc_port1_rx_div_clk_src
             nss_cc_uniphy_port1_rx_clk
             nss_cc_port1_rx_clk

The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII 
clock at the time when the Ethernet and PCS drivers are enabled (step3) 
and the MAC links up. At link up time, the NSSCC driver sets the NSSCC 
port clock rate (by configuring the divider) based on the link speed, 
during which time the NSSCC port clock's parent is switched to 'UNIPHY' 
PCS clock. Below is the clock tree dump after this step.

7a00000.ethernet-pcs::rx_clk
     nss_cc_port1_rx_clk_src
         nss_cc_port1_rx_div_clk_src
             nss_cc_uniphy_port1_rx_clk
             nss_cc_port1_rx_clk

> 2) there's yet another open coded "_get" function for getting the
> PCS given a DT node which is different from every other "_get"
> function - this one checks the parent DT node has an appropriate
> compatible whereas others don't. The whole poliferation of "_get"
> methods that are specific to each PCS still needs solving, and I
> still have the big question around what happens when the PCS driver
> gets unbound - and whether that causes the kernel to oops. I'm also
> not a fan of "look up the struct device and then get its driver data".
> There is *no* locking over accessing the driver data.
> 

The PCS device in IPQ9574 chipset is built into the SoC chip and is not 
pluggable. Also, the PCS driver module is not unloadable until the MAC 
driver that depends on it is unloaded. Therefore, marking the driver 
'.suppress_bind_attrs = true' to disable user unbind action may be good 
enough to cover all possible scenarios of device going away for IPQ9574 
PCS driver.

To avoid looking up the device and getting its driver data (which is 
also seen in other PCS device drivers currently), a common 
infrastructure is certainly preferable for the longer term to have a 
consistent lookup. As far as I understand, the urgency for the common 
infrastructure for lookup is perhaps more to resolve the issue of 
hot-pluggable devices going away, and less for devices that do not 
support it.

Also, the _get() API is only called once during MAC port initialization 
and never later, so if the device is not pluggable and unbind is not 
possible, there may not be any race concerns when accessing the driver 
data using the _get() API. Please let me know if this understanding is 
incorrect.

> 3) doesn't populate supported_interfaces for the PCS - which would
> make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.)
> 

Agree, we will update the patch to advertise 'supported interfaces' and 
use the 'pcs_validate' op only for patch4 as you pointed (for filtering 
half duplex modes for USXGMII.).
[The 'pcs_validate()' was suggested by you and added in the version 3 of 
this driver, and at that time, the pcs supported_interfaces is not 
introduced.]

> 4)
> "+       /* Nothing to do here as in-band autoneg mode is enabled
> +        * by default for each PCS MII port."
> 
> "by default" doesn't matter - what if in-band is disabled and then
> subsequently enabled.
> 

OK, I will fix this function to handle both in-band neg enabled and 
disabled cases in next update.

> 5) there seems to be an open-coded decision about the clock rate but
> there's also ipq_pcs_clk_rate_get() which seems to make the same
> decision.
> 

I think you may be referring to both ipq_pcs_config_mode() and 
ipq_pcs_clk_rate_get() functions having the similar switch case to 
decide the clock rate based on the interface mode. I do agree, we can 
simplify this by saving the clock rate in ipq_pcs_config_mode() before 
the clk_set_rate() is called, and then simply returning this clock rate 
from the recalc_rate() op.


> 6) it seems this block has N PCS, but all PCS must operate in the same
> mode (e.g. one PCS can't operate in SGMII mode, another in USXGMII
> mode.) Currently, the last "config" wins over previous configs across
> all interfaces. Is this the best solution? Should we be detecting
> conflicting configurations? Unfortunately, pcs->supported_interfaces
> can't really be changed after the PCS is being used, so I guess
> any such restrictions would need to go in ipq_pcs_validate() which
> should work fine - although it would mean that a MAC populating
> its phylink_config->supported_interfaces using pcs->supported_interfaces
> may end up with too many interface bits set.
> 

I would like to clarify on the hardware supported configurations for the
UNIPHY PCS hardware instances. [Note: There are three instances of 
'UNIPHY PCS' in IPQ9574. However we take the example here for PCS0]

UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
Possible combinations: QSGMII (4x 1 SGMII)
			PSGMII (5 x 1 SGMII),
			SGMII (1 x 1 SGMII)
			USXGMII (1 x 1 USXGMII)
	
As we can see above, different PCS channels in a 'UNIPHY' PCS block 
working in different PHY interface modes is not supported by the 
hardware. So, it might not be necessary to detect that conflict. If the 
interface mode changes from one to another, the same interface mode is 
applicable to all the PCS channels that are associated with the UNIPHY 
PCS block.

Below is an example of a DTS configuration which depicts one board 
configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad 
PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports, 
and all the PCS MII channels are in QSGMII mode. For the 'UNIPHY' 
connected with single SGMII or USXGMII PHY (PCS1), only one MII channel 
is enabled and connected with one PPE MAC port.

PHY:
&mdio {
	ethernet-phy-package@0 {
                 compatible = "qcom,qca8075-package";
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <0x10>;
                 qcom,package-mode = "qsgmii";

                 phy0: ethernet-phy@10 {
                         reg = <0x10>;
                 };

                 phy1: ethernet-phy@11 {
                         reg = <0x11>;
                 };

                 phy2: ethernet-phy@12 {
                         reg = <0x12>;
                 };

                 phy3: ethernet-phy@13 {
                         reg = <0x13>;
                 };
	};
	phy4: ethernet-phy@8 {
                 compatible ="ethernet-phy-ieee802.3-c45";
                 reg = <8>;
         };
}

PCS:
pcs0: ethernet-pcs@...0000 {
	......
	pcs0_mii0: pcs-mii@0 {
		reg = <0>;
		status = "enabled";
	};

	......

	pcs0_mii3: pcs-mii@3 {
		reg = <3>;
		status = "enabled";
	};
};

pcs1: ethernet-pcs@...0000 {
	......

	pcs1_mii0: pcs-mii@0 {
		reg = <0>;
		status = "enabled";
	};
};

MAC:
port@1 {
	phy-mode = "qsgmii";
	phy-handle = <&phy0>;
	pcs-handle = <&pcs0_mii0>;
}

port@2 {
	phy-mode = "qsgmii";
	phy-handle = <&phy1>;
	pcs-handle = <&pcs0_mii1>;
}
port@3 {
	phy-mode = "qsgmii";
	phy-handle = <&phy2>;
	pcs-handle = <&pcs0_mii2>;
}
port@4 {
	phy-mode = "qsgmii";
	phy-handle = <&phy3>;
	pcs-handle = <&pcs0_mii3>;
}
port@5 {
         phy-mode = "usxgmii";
         phy-handle = <&phy4>;
         pcs-handle = <&pcs1_mii0>;
}

> (1), (2) and (6) are probably the major issues at the moment, and (2)
> has been around for a while.
> 
> Given (1), I'm just left wondering whether this has been runtime
> tested, and how the driver model's driver dependencies cope with it
> if the NSCCC driver is both a clock consumer of/provider to this
> driver.
> 

Yes, I have tested the PCS driver along with NSSCC driver and PPE 
Ethernet driver.

Powered by blists - more mailing lists