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

Lei Wei
2024-12-06T16:21:08Z

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