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

[Lei Wei]

On 11/8/2024 3:02 AM, Russell King (Oracle) wrote:
> Hi,
>
> On Fri, Nov 01, 2024 at 06:32:51PM +0800, Lei Wei 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);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> > +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> > +					 PCS_MODE_QSGMII);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	default:
> > +		dev_err(qpcs->dev,
> > +			"Unsupported interface %s\n", phy_modes(interface));
> > +		return -EOPNOTSUPP;
> > +	}
>
> I think:
>
> 	unsigned int mode;
>
> 	switch (interface) {
> 	case PHY_INTERFACE_MODE_SGMII:
> 		/* Select Qualcomm SGMII AN mode */
> 		mode = PCS_MODE_SGMII;
> 		break;
>
> 	case PHY_INTERFACE_MODE_QSGMII:
> 		mode = PCS_MODE_QSGMII;
> 		break;
>
> 	default:
> 		...
> 	}
>
> 	ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> 				 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE, mode);
> 	if (ret)
> 		return ret;
>
> might be easier to read? 

Thanks for the suggestion, I will make this change.

I notice that in patch 4, the USXGMII case
> drops PCS_MODE_AN_MODE from the mask, leaving this bit set to whatever
> value it previously was. Is that intentional?

Yes, this bit is used for configuring the SGMII AN mode - Cisco AN or 
Qualcomm AN. The default setting is Cisco SGMII AN mode.

Please note that as per our discussion with Andrew on his comment 
regarding the default mode to use, we would like to change the default 
setting for SGMII/QSGMII to Cisco AN Mode in the next patch update.

> > +static int ipq_pcs_link_up_config_sgmii(struct ipq_pcs *qpcs,
> > +					int index,
> > +					unsigned int neg_mode,
> > +					int speed)
> > +{
> > +	int ret;
> > +
> > +	/* PCS speed need not be configured if in-band autoneg is enabled */
> > +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> > +		goto pcs_adapter_reset;
> > +
> > +	/* PCS speed set for force mode */
> > +	switch (speed) {
> > +	case SPEED_1000:
> > +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> > +					 PCS_MII_SPEED_MASK,
> > +					 PCS_MII_SPEED_1000);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	case SPEED_100:
> > +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> > +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_100);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	case SPEED_10:
> > +		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> > +					 PCS_MII_SPEED_MASK, PCS_MII_SPEED_10);
> > +		if (ret)
> > +			return ret;
> > +		break;
> > +	default:
> > +		dev_err(qpcs->dev, "Invalid SGMII speed %d\n", speed);
> > +		return -EINVAL;
> > +	}
>
> I think it's worth having the same structure here, and with fewer lines
> (and fewer long lines) maybe:
>
> 	if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
> 		switch (speed) {
> 		...
> 		}
>
> 		ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> 					 PCS_MII_SPEED_MASK, ctrl);
> 		if (ret)
> 			return ret;
> 	}
>
> means you don't need the pcs_adapter_reset label.

Sure, thanks for the suggestion. I will make this change.

> > +
> > +pcs_adapter_reset:
> > +	/* PCS adapter reset */
> > +	ret = regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> > +				 PCS_MII_ADPT_RESET, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(qpcs->regmap, PCS_MII_CTRL(index),
> > +				  PCS_MII_ADPT_RESET, PCS_MII_ADPT_RESET);
> > +}
> > +
> > +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
> > +			      struct phylink_link_state *state)
> > +{
> > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > +	int index = qpcs_mii->index;
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		ipq_pcs_get_state_sgmii(qpcs, index, state);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	dev_dbg(qpcs->dev,
> > +		"mode=%s/%s/%s link=%u\n",
> > +		phy_modes(state->interface),
> > +		phy_speed_to_str(state->speed),
> > +		phy_duplex_to_str(state->duplex),
> > +		state->link);
>
> This will get very noisy given that in polling mode, phylink will call
> this once a second - and I see you are using polling mode.

Sure, I will use dev_dbg_ratelimited() API instead.

> > +/**
> > + * ipq_pcs_create() - Create an IPQ PCS MII instance
> > + * @np: Device tree node to the PCS MII
> > + *
> > + * Description: Create a phylink PCS instance for the given PCS MII node @np
> > + * and enable the MII clocks. This instance is associated with the specific
> > + * MII of the PCS and the corresponding Ethernet netdevice.
> > + *
> > + * Return: A pointer to the phylink PCS instance or an error-pointer value.
> > + */
> > +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);
> > +	}
> > +
> > +	pdev = of_find_device_by_node(pcs_np);
> > +	of_node_put(pcs_np);
> > +	if (!pdev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	qpcs = platform_get_drvdata(pdev);
> > +	put_device(&pdev->dev);
> > +
> > +	/* If probe is not yet completed, return DEFER to
> > +	 * the dependent driver.
> > +	 */
> > +	if (!qpcs)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	qpcs_mii = kzalloc(sizeof(*qpcs_mii), GFP_KERNEL);
> > +	if (!qpcs_mii)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	qpcs_mii->qpcs = qpcs;
> > +	qpcs_mii->index = index;
> > +	qpcs_mii->pcs.ops = &ipq_pcs_phylink_ops;
> > +	qpcs_mii->pcs.neg_mode = true;
> > +	qpcs_mii->pcs.poll = true;
> > +
> > +	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;
> > +		}
>
> Do you always need the clock prepared and enabled? If not, you could
> do this in the pcs_enable() method, and turn it off in pcs_disable().

Yes, it can be moved to pcs_enable/pcs_disable method. I will make this 
change.

> I think phylink may need a tweak to "unuse" the current PCS when
> phylink_stop() is called to make that more effective at disabling the
> PCS, which is something that should be done - that's a subject for a
> separate patch which I may send.