On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:+static int ipq_pcs_enable(struct phylink_pcs *pcs) +{ + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs); + struct ipq_pcs *qpcs = qpcs_mii->qpcs; + int index = qpcs_mii->index; + int ret; + + ret = clk_prepare_enable(qpcs_mii->rx_clk); + if (ret) { + dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index); + return ret; + } + + ret = clk_prepare_enable(qpcs_mii->tx_clk); + if (ret) { + dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index); + clk_disable_unprepare(qpcs_mii->rx_clk); + return ret; + } + + return 0; +} + +static void ipq_pcs_disable(struct phylink_pcs *pcs) +{ + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs); + + if (__clk_is_enabled(qpcs_mii->rx_clk)) + clk_disable_unprepare(qpcs_mii->rx_clk); + + if (__clk_is_enabled(qpcs_mii->tx_clk)) + clk_disable_unprepare(qpcs_mii->tx_clk);Why do you need the __clk_is_enabled() calls here? Phylink should be calling pcs_enable() once when the PCS when starting to use the PCS, and then pcs_disable() when it stops using it - it won't call pcs_disable() without a preceeding call to pcs_enable(). Are you seeing something different?
Yes, understand that phylink won't call pcs_disable() without a preceeding call to pcs_enable(). However, the "clk_prepare_enable" may fail in the pcs_enable() method, so I added the __clk_is_enabled() check in pcs_disable() method. This is because the phylink_major_config() function today does not interpret the return value of phylink_pcs_enable().
+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;...+static int ipq_pcs_config(struct phylink_pcs *pcs, + unsigned int neg_mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit) +{ + 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 (interface) { + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_QSGMII: + return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface); + default: + dev_err(qpcs->dev, + "Unsupported interface %s\n", phy_modes(interface)); + return -EOPNOTSUPP; + }; +} + +static void ipq_pcs_link_up(struct phylink_pcs *pcs, + unsigned int neg_mode, + phy_interface_t interface, + int speed, int duplex) +{ + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs); + struct ipq_pcs *qpcs = qpcs_mii->qpcs; + int index = qpcs_mii->index; + int ret; + + switch (interface) { + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_QSGMII: + ret = ipq_pcs_link_up_config_sgmii(qpcs, index, + neg_mode, speed); + break; + default: + dev_err(qpcs->dev, + "Unsupported interface %s\n", phy_modes(interface)); + return; + }So you only support SGMII and QSGMII. Rather than checking this in every method implementation, instead provide a .pcs_validate method that returns an error for unsupported interfaces please.
Yes, I can add the pcs_validate() method to validate the link configurations. This will catch invalid interface mode during the PCS initialization time, earlier than the pcs_config and pcs_link_up contexts.
But after of the PCS init, if at a later point the PHY interface mode changes, it seems phylink today is not calling the pcs_validate() op to validate the changed new interface mode at the time of "phylink_resolve". (Hope my understanding is correct). So, In the pcs ops methods, I will keep the switch case to check and handle the unsupported interface modes.
Thanks.
