Message-ID: <Zy0OuPMbUaZtIosj@shell.armlinux.org.uk>
Date: Thu, 7 Nov 2024 19:02:16 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Lei Wei <quic_leiwei@...cinc.com>
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>, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>, 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

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? 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?

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

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

> +/**
> + * 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().

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists