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