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

[Lei Wei <quic_leiwei@quicinc.com>]

On 12/7/2024 12:28 AM, Russell King (Oracle) wrote:
> On Sat, Dec 07, 2024 at 12:20:25AM +0800, Lei Wei wrote:
>> 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().
> 
> Right, because failure is essentially fatal in that path - we have no
> context to return an error. I suppose we could stop processing at
> that point, but then it brings up the question of how to unwind anything
> we've already done, which is basically impossible at that point.
> 

Sure, understand. I will remove the checks.

>>>> +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".
> 
> ... because by that time it's way too late. Phylink will have already
> looked at what the PHY can do when the PHY is attached, and eliminated
> any link modes that would cause an invalid configuration (provided
> phylink knows what the PHY is capable of.)
> 
> However, that assumes phylink knows what the details are of the PCS,
> which is dependent on the .pcs_validate method being implemented.
> 

Yes, agree that pcs_validate() is necessary to be implemented and 
Phylink will validate the PHY when the PHY is attached. I will implement 
this method in the next update. Thanks for pointing to the details here.

We will also remove the debug error print for the 'default' case. 
However, I would like to retain the switch statement since we have 
different routines for SGMII/USXGMII modes, and we plan to add more 
interfaces modes later when we enhance the driver for other IPQ SoC.