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!
