Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!