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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5121 bytes --]

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 think we need either an explicit test from a 3rd party or a new revision.
I'm not sure what you mean with a v3. Going back to v1? 

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]