Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

[Lei Wei]

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.

> > +static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
> > +				int index,
> > +				unsigned int neg_mode,
> > +				phy_interface_t interface)
> > +{
> > +	int ret;
> > +
> > +	/* Access to PCS registers such as PCS_MODE_CTRL which are
> > +	 * common to all MIIs, is lock protected and configured
> > +	 * only once. This is required only for interface modes
> > +	 * such as QSGMII.
> > +	 */
> > +	if (interface == PHY_INTERFACE_MODE_QSGMII)
> > +		mutex_lock(&qpcs->config_lock);
>
> Is there a lot of contention on this lock? Why not take it for every
> interface mode? It would make the code simpler.

I agree, the contention should be minimal since the lock is common for 
all MII ports in a PCS and is used only during configuration time. I 
will remove the interface mode check.

> > +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.

> > +	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.

> 	Andrew