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?
> +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.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
