Re: [PATCH net v2] net: dpaa: fix mode setting

[Sean Anderson <sean.anderson@linux.dev>]

On 7/17/26 07:11, Paolo Abeni wrote:
> On 7/17/26 12:13 PM, Michael Walle wrote:
>> On Fri Jul 17, 2026 at 12:01 PM CEST, Paolo Abeni wrote:
>>> On 7/10/26 4:47 PM, Michael Walle wrote:
>>>> On Fri Jul 10, 2026 at 4:39 PM CEST, Sean Anderson wrote:
>>>>> On 7/10/26 10:22, Michael Walle wrote:
>>>>>> Before converting to the phylink interface, the init function would have
>>>>>> set the correct I/F mode depending on the maximum link speed of an
>>>>>> interface. After converting to phylink, the established link speed
>>>>>> is used to determine this setting and is set in the .link_up()
>>>>>> callback. The callback isn't called because the link is never
>>>>>> established between the PCS and a connected SGMII PHY.
>>>>>> To fix it, don't use the current speed, but set the mode depending on
>>>>>> the interface (which implies the maximum speed) in .mac_config().
>>>>>>
>>>>>> Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink")
>>>>>> Suggested-by: Sean Anderson <sean.anderson@linux.dev>
>>>>>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>>>>>> ---
>>>>>> FWIW, I dropped setting a non-reserved mode in init(). The hardware
>>>>>> default is 0 and the mac_config() will set a valid mode anyway.
>>>>>>
>>>>>> Changes in v2:
>>>>>>    - the setting is/was based on the maximum speed, not the current
>>>>>>      speed. thus, move the setting into mac_config().
>>>>>>    - Link to v1: https://lore.kernel.org/r/20260706121011.1948906-1-mwalle@kernel.org/
>>>>>>
>>>>>>    .../net/ethernet/freescale/fman/fman_dtsec.c  | 26 ++++++++++---------
>>>>>>    1 file changed, 14 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>>>>> index fe35703c509e..7075f93bab49 100644
>>>>>> --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>>>>> +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>>>>> @@ -900,22 +900,28 @@ static void dtsec_mac_config(struct phylink_config *config, unsigned int mode,
>>>>>>    {
>>>>>>    	struct mac_device *mac_dev = fman_config_to_mac(config);
>>>>>>    	struct dtsec_regs __iomem *regs = mac_dev->fman_mac->regs;
>>>>>> -	u32 tmp;
>>>>>> +	u32 ecntrl, maccfg2;
>>>>>> +
>>>>>> +	maccfg2 = ioread32be(&regs->maccfg2);
>>>>>> +	maccfg2 &= ~(MACCFG2_NIBBLE_MODE | MACCFG2_BYTE_MODE);
>>>>>>    
>>>>>>    	switch (state->interface) {
>>>>>>    	case PHY_INTERFACE_MODE_RMII:
>>>>>> -		tmp = DTSEC_ECNTRL_RMM;
>>>>>> +		ecntrl = DTSEC_ECNTRL_RMM;
>>>>>> +		maccfg2 |= MACCFG2_NIBBLE_MODE;
>>>>>>    		break;
>>>>>>    	case PHY_INTERFACE_MODE_RGMII:
>>>>>>    	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>>>    	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>>>>    	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>>>> -		tmp = DTSEC_ECNTRL_GMIIM | DTSEC_ECNTRL_RPM;
>>>>>> +		ecntrl = DTSEC_ECNTRL_GMIIM | DTSEC_ECNTRL_RPM;
>>>>>> +		maccfg2 |= MACCFG2_BYTE_MODE;
>>>>>>    		break;
>>>>>>    	case PHY_INTERFACE_MODE_SGMII:
>>>>>>    	case PHY_INTERFACE_MODE_1000BASEX:
>>>>>>    	case PHY_INTERFACE_MODE_2500BASEX:
>>>>>> -		tmp = DTSEC_ECNTRL_TBIM | DTSEC_ECNTRL_SGMIIM;
>>>>>> +		ecntrl = DTSEC_ECNTRL_TBIM | DTSEC_ECNTRL_SGMIIM;
>>>>>> +		maccfg2 |= MACCFG2_BYTE_MODE;
>>>>>>    		break;
>>>>>>    	default:
>>>>>>    		dev_warn(mac_dev->dev, "cannot configure dTSEC for %s\n",
>>>>>> @@ -923,7 +929,8 @@ static void dtsec_mac_config(struct phylink_config *config, unsigned int mode,
>>>>>>    		return;
>>>>>>    	}
>>>>>>    
>>>>>> -	iowrite32be(tmp, &regs->ecntrl);
>>>>>> +	iowrite32be(ecntrl, &regs->ecntrl);
>>>>>> +	iowrite32be(maccfg2, &regs->maccfg2);
>>>>>>    }
>>>>>>    
>>>>>>    static void dtsec_link_up(struct phylink_config *config, struct phy_device *phy,
>>>>>> @@ -948,15 +955,10 @@ static void dtsec_link_up(struct phylink_config *config, struct phy_device *phy,
>>>>>>    	iowrite32be(tmp, &regs->ecntrl);
>>>>>>    
>>>>>>    	tmp = ioread32be(&regs->maccfg2);
>>>>>> -	tmp &= ~(MACCFG2_NIBBLE_MODE | MACCFG2_BYTE_MODE | MACCFG2_FULL_DUPLEX);
>>>>>> -	if (speed >= SPEED_1000)
>>>>>> -		tmp |= MACCFG2_BYTE_MODE;
>>>>>> -	else
>>>>>> -		tmp |= MACCFG2_NIBBLE_MODE;
>>>>>> -
>>>>>>    	if (duplex == DUPLEX_FULL)
>>>>>>    		tmp |= MACCFG2_FULL_DUPLEX;
>>>>>> -
>>>>>> +	else
>>>>>> +		tmp &= ~MACCFG2_FULL_DUPLEX;
>>>>>
>>>>> Did you test this when forcing 10/100 speed?
>>>>
>>>> No I didn't. Well I can't. I have a very weird board which only
>>>> supports 1000base-X (and copper SFPs in 1000basex autoneg mode). On
>>>> top of that there is a Marvell 88E1112 in between the SFP and the
>>>> MAC, for which the PHY driver is completely broken. Long story
>>>> short, I'm not able to test that (yet/at all? Not sure).
>>> FTR, sashiko suspect this patch will broke such setup:
>>> https://sashiko.dev/#/patchset/20260710143430.2276141-1-mwalle%40kernel.org
>>
>> I've seen that, but.. that was the actual change between v1 and v2
>> as suggested by Sean. It does not depend on the actual link speed,
>> but the maximum link speed. So it is not relevant if the link
>> negotiates to a slower speed or not. At least that now matches the
>> behavior prior to the phylink conversion. If that was working -
>> that I can't tell you.
> 
> I'm sorry, following all cross revision discussion is a bit hard here.
> 
> I don't understand if the 'link never established' is specific of your
> board, or it a constant with this driver. Could you please clarify?

I think we should set byte/nibble mode in link_up as well as mac_config. That
should fix the problem where the link never comes up (possibly broken since
the phylink conversion). I think it's unlikely that this config is supposed
to match the phy interface speed and not the link speed, but you never know...

--Sean