Message-ID: <c6a78dd6-763c-41a0-8a6e-2e81723412be@quicinc.com>
Date: Thu, 15 May 2025 00:03:54 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: <mr.nuke.me@...il.com>, "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 5/13/2025 6:56 AM, mr.nuke.me@...il.com wrote:
> On 2/19/25 4:46 AM, Lei Wei wrote:
>>
>> 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
>>
> 
> I tried this PCS driver, and I am seeing a circular dependency in the 
> clock init. If the clock tree is:
>      GCC -> NSSCC -> PCS(uniphy) -> NSSCC -> PCS(mii)
> 
> The way I understand it, the UNIPHY probe depends on the MII probe. If 
> MII .probe() returns -EPROBE_DEFER, then so will the UNIPHY .probe(). 
> But the MII cannot probe until the UNIPHY is done, due to the clock 
> dependency. How is it supposed to work?
> 
> The way I found to resolve this is to move the probing of the MII clocks 
> to ipq_pcs_get().
> 
> This is the kernel log that I see:
> 
> [   12.008754] platform 39b00000.clock-controller: deferred probe 
> pending: platform: supplier 7a00000.ethernet-pcs not ready
> [   12.008788] mdio_bus 90000.mdio-1:18: deferred probe pending: 
> mdio_bus: supplier 7a20000.ethernet-pcs not ready
> [   12.018704] mdio_bus 90000.mdio-1:00: deferred probe pending: 
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [   12.028588] mdio_bus 90000.mdio-1:01: deferred probe pending: 
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [   12.038310] mdio_bus 90000.mdio-1:02: deferred probe pending: 
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [   12.047943] mdio_bus 90000.mdio-1:03: deferred probe pending: 
> mdio_bus: supplier 90000.mdio-1:18 not ready
> [   12.057579] platform 7a00000.ethernet-pcs: deferred probe pending: 
> ipq9574_pcs: Failed to get MII 0 RX clock
> [   12.067209] platform 7a20000.ethernet-pcs: deferred probe pending: 
> ipq9574_pcs: Failed to get MII 0 RX clock
> [   12.077200] platform 3a000000.qcom-ppe: deferred probe pending: 
> platform: supplier 39b00000.clock-controller not ready
> 
> 

Hello, thanks for bringing this to our notice. Let me try to understand 
the reason for the probe failure:

The merged NSSCC DTS does not reference the PCS node directly in the 
"clocks" property. It uses a placeholder phandle '<0>' for the 
reference. Please see below patch which is merged.
https://lore.kernel.org/all/20250313110359.242491-6-quic_mmanikan@quicinc.com/

Ideally there should be no direct dependency from NSSCC to PCS driver if
we use this version of the NSSCC DTS.

Hence it seems that you may have a modified patch here, and DTS changes 
have been applied to enable all the Ethernet components including PCS 
and NSSCC, and NSSCC modified to have a direct reference to PCS? However 
even in this case, I think the driver probe should work if the drivers 
are built as modules. Can you please confirm if the NSSCC and PCS 
drivers are built-in to the kernel and not built as modules?

For the case where the drivers are built-in to kernel, and the NSSCC DTS
node has a direct reference to PCS node, we can use the below solution:
[Note that the 'UNIPHY' PCS clocks are not needed for NSSCC clocks
initialization/registration.]

     Enable 'post-init-providers' property in the NSSCC DTS node to mark
    'UNIPHY' PCS as post-initialization providers to NSSCC. This will
     ensure following probe order by the kernel:

     1.) NSSCC driver
     2.) PCS driver.

Please let me know if the above suggestion can help.

Later once the IPQ PCS driver is merged, we are planning to push the PCS 
DTS changes, along with an update of the NSSCC DTS to point to the PCS 
node and mark the "post-init-providers" property. This should work for 
all cases.

Also, in my view, it is not suitable to move PCS MII clocks get to
"ipq_pcs_get()" because the natural loading order for the drivers
is as below:

1) NSSCC driver
2) PCS driver
3) Ethernet driver.

Additionally, the community is currently working on an infrastructure to
provide a common pcs get method. (Christian and Sean Anderson has been 
working on this). Therefore, I expect "ipq_pcs_get" to be dropped in the 
future and replaced with the common pcs get method once this common 
infra is merged.

This is the post-init-providers dtschma:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/post-init-providers.yaml

> PHY:
> &mdio {
>      qca8k_nsscc: clock-controller@18 {
>          compatible = "qcom,qca8084-nsscc";
>          ...
>      };
> 
>      ethernet-phy-package@0 {
>          compatible = "qcom,qca8084-package";
>          ...
> 
>          qca8084_0: ethernet-phy@0 {
>              compatible = "ethernet-phy-id004d.d180";
>              reg = <0>;
>              clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
>              resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>;
>          };
>          qca8084_1: ethernet-phy@1 {
>              compatible = "ethernet-phy-id004d.d180";
>              reg = <1>;
>              clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
>              resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>;
>          };
>          qca8084_2: ethernet-phy@2 {
>              compatible = "ethernet-phy-id004d.d180";
>              reg = <2>;
>              clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
>              resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>;
>          };
>          qca8084_3: ethernet-phy@3 {
>              compatible = "ethernet-phy-id004d.d180";
>              reg = <3>;
>              clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
>              resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>;
>          };
>      };
> 
>      qca8081_12: ethernet-phy@12 {
>          reset-gpios = <&tlmm 36 GPIO_ACTIVE_LOW>;
>          reg = <12>;
>      };
> 
> PCS:
>      pcs_uniphy0: ethernet-pcs@...0000 {
>          compatible = "qcom,ipq9574-pcs";
>          ...
>          pcsuniphy0_ch0: pcs-mii@0 {
>              reg = <0>;
>              clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
>                   <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
>              clock-names = "rx",
>                        "tx";
>          };
>          ...
> 
> MAC:
>          port@1 {
>              reg = <1>;
>              phy-mode = "usxgmii";
>              managed = "in-band-status";
>              phy-handle = <&qca8084_0>;
>              pcs-handle = <&pcsuniphy0_ch0>;
>              ...
>          };

Powered by blists - more mailing lists