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!