Message-ID: <9b7def00-e900-4c5e-ba95-671bd1ef9240@quicinc.com>
Date: Fri, 8 Nov 2024 19:31:31 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Heiner Kallweit
	<hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...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>
Subject: Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and
 phylink operations for IPQ9574



On 11/6/2024 11:43 AM, Andrew Lunn wrote:
> On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
>>
>>
>> On 11/1/2024 9:21 PM, Andrew Lunn wrote:
>>>> +static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
>>>> +			       phy_interface_t interface)
>>>> +{
>>>> +	unsigned int val;
>>>> +	int ret;
>>>> +
>>>> +	/* Configure PCS interface mode */
>>>> +	switch (interface) {
>>>> +	case PHY_INTERFACE_MODE_SGMII:
>>>> +		/* Select Qualcomm SGMII AN mode */
>>>> +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
>>>> +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
>>>> +					 PCS_MODE_SGMII);
>>>
>>> How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
>>>
>>
>> Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
>> bit support in the SGMII word format. It re-uses two of the reserved bits
>> 1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
>> SGMII AN modes.
> 
> Is Qualcomm SGMII AN actually needed? I assume it only works against a
> Qualcomm PHY? What interoperability testing have you do against
> non-Qualcomm PHYs?
> 

I agree that using Cisco SGMII AN mode as default is more appropriate,
since is more commonly used with PHYs. I will make this change.

Qualcomm SGMII AN is an extension of top of Cisco SGMII AN (only
pause bits difference). So it is expected to be compatible with
non-Qualcomm PHYs which use Cisco SGMII AN.

>>>> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
>>>> +{
>>>> +	struct platform_device *pdev;
>>>> +	struct ipq_pcs_mii *qpcs_mii;
>>>> +	struct device_node *pcs_np;
>>>> +	struct ipq_pcs *qpcs;
>>>> +	int i, ret;
>>>> +	u32 index;
>>>> +
>>>> +	if (!of_device_is_available(np))
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	if (of_property_read_u32(np, "reg", &index))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	if (index >= PCS_MAX_MII_NRS)
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	pcs_np = of_get_parent(np);
>>>> +	if (!pcs_np)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	if (!of_device_is_available(pcs_np)) {
>>>> +		of_node_put(pcs_np);
>>>> +		return ERR_PTR(-ENODEV);
>>>> +	}
>>>
>>> How have you got this far if the parent is not available?
>>>
>>
>> This check can fail only if the parent node is disabled in the board DTS. I
>> think this error situation may not be caught earlier than this point.
>> However I agree, the above check is redundant, since this check is
>> immediately followed by a validity check on the 'pdev' of the parent node,
>> which should be able cover any such errors as well.
> 
> This was also because the driver does not work as i expected. I was
> expecting the PCS driver to walk its own DT and instantiate the PCS
> devices listed. If the parent is disabled, it is clearly not going to
> start its own children.  But it is in fact some other device which
> walks the PCS DT blob, and as a result the child/parent relationship
> is broken, a child could exist without its parent.
> 

Currently the PCS driver walks the DT and instantiates the parent PCS 
nodes during probe, where as the child PCS (the per-MII PCS instance) is 
instantiated later by the network device that is associated with the MII.

Alternatively as you mention, we could instantiate the child PCS during 
probe itself. The network driver when it comes up, can just issue a 
'get' operation on the PCS driver, to retrieve the PCS phylink given the 
PCS node associated with the MAC. Agree that this is architecturally 
simpler for instantiating the PCS nodes, and the interaction between 
network driver and PCS is simpler (single 'get_phylink_pcs' API exported 
from PCS driver instead of PCS create/destroy API exported). The PCS 
instances are freed up during platform device remove.

>>>> +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
>>>> +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
>>>> +		if (IS_ERR(qpcs_mii->clk[i])) {
>>>> +			dev_err(qpcs->dev,
>>>> +				"Failed to get MII %d interface clock %s\n",
>>>> +				index, pcs_mii_clk_name[i]);
>>>> +			goto err_clk_get;
>>>> +		}
>>>> +
>>>> +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
>>>> +		if (ret) {
>>>> +			dev_err(qpcs->dev,
>>>> +				"Failed to enable MII %d interface clock %s\n",
>>>> +				index, pcs_mii_clk_name[i]);
>>>> +			goto err_clk_en;
>>>> +		}
>>>> +	}
>>>
>>> Maybe devm_clk_bulk_get() etc will help you here? I've never actually
>>> used them, so i don't know for sure.
>>>
>>
>> We don't have a 'device' associated with the 'np', so we could not consider
>> using the "devm_clk_bulk_get()" API.
> 
> Another artefact of not have a child-parent relationship. I wounder if
> it makes sense to change the architecture. Have the PCS driver
> instantiate the PCS devices as its children. They then have a device
> structure for calls like clk_bulk_get(), and a more normal
> consumer/provider setup.
> 

I think you may be suggesting to drop the child node usage in the DTS, 
so that we can attach all the MII clocks to the single PCS node, to 
facilitate usage of bulk get() API to retrieve the MII clocks for that 
PCS. We reviewed this option, and believe that retaining the current 
parent-child relationship is a better option instead. This is because 
this option allows us the flexibility to enable/disable the MII channels 
(child nodes) in the board DTS as per the ethernet/MII configuration 
relevant for the board.

We would like to explain this using an example of SoC/board DTSI below.

For IPQ9574, port5 can be connected with PCS0 (PCS0: PSGMII, PCS1 not 
used) or PCS1 (PCS0: QSGMII, PCS1: USXGMII).

IPQ9574 SoC DTSI for PCS0 (5 max MII channels) and PCS1:
--------------------------------------------------------
pcs0: ethernet-pcs@...0000 {
	clocks = <&gcc GCC_UNIPHY0_SYS_CLK>,
		 <&gcc GCC_UNIPHY0_AHB_CLK>;
	clock-names = "sys",
		      "ahb";

	status = "disabled";

	pcs0_mii0: pcs-mii@0 {
		reg = <0>;
		status = "disabled";
	};

	......

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

pcs1: ethernet-pcs@...0000 {
	clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
		 <&gcc GCC_UNIPHY1_AHB_CLK>;
	clock-names = "sys",
		      "ahb";

	status = "disabled";

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


board1 DTS (PCS0 - QSGMII (port1 - port4), PCS1 USXGMII (port 5))
-----------------------------------------------------------------
&pcs0 {
	status = "okay";
};

/* Enable only four MII channels for PCS0 for this board */
&pcs0_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

......

&pcs0_mii3 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT4_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT4_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

&pcs1 {
	status = "okay";
};

&pcs1_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

board2 DTS: (PCS0 - PSGMII (port1 - port5), PCS1 - disabled):
-------------------------------------------------------------
&pcs0 {
	status = "okay";
};

/* For PCS0, Enable all 5 MII channels for this board,
    PCS1 is disabled */
&pcs0_mii0 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

......

&pcs0_mii4 {
	clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
		 <&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
	clock-names = "mii_rx",
		      "mii_tx";
	status = "okay";
};

If we drop the child node in DTS, then all MII clocks will have to be 
combined with the SoC clocks (AHB/SYS) and added to the board DTS, which 
may not be correct. Also, I think the child-parent relationship in DTS 
will make it more clear to reflect the PCS-to--MII-channel relationship.

Kindly let us know if this approach is fine.

> 	Andrew

Powered by blists - more mailing lists