Re: [PATCH net] net: dpaa: always set a valid mode I/F mode

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

On 7/10/26 05:39, Michael Walle wrote:
> Hi,
> 
> On Fri Jul 10, 2026 at 1:45 AM CEST, Sean Anderson wrote:
>> On 7/6/26 08:08, Michael Walle wrote:
>>> Before converting to the phylink interface, the init function would have
>>> set the correct mode in the maccfg2.
>>
>> So for reference, the old logic is
>>
>>          if (iface_speed < SPEED_1000)
>>                  tmp |= MACCFG2_NIBBLE_MODE;
>>          else if (iface_speed == SPEED_1000)
>>                  tmp |= MACCFG2_BYTE_MODE;
>>
>> which changes between nibble/byte mode depending on the max link speed
>> (e.g. phylink_interface_max_speed). Notably, neither is set for 2.5G.
> 
> Ahh you're right, I totally missed, that the speed parameter was
> actually max_speed, which was set by
> 
>    priv->speed = phy2speed[macdev->phy_if];
>    priv->max_speed = priv->speed;
> 
> FWIW, in the DPAA RM I have, there is no 2.5G speed. Are you sure,
> there are DTSEC controllers with 2.5Gbps support? The only board I
> found in the device trees is the T1023RDB which is using the memac.

It's listed as an option in the P5020/P5040 RMs (under "SerDes Lane
Assignments and Multiplexing"). Of course despite other parts of the
DPAARM referring to the dTSEC's 2.5G capabilities, the the dTSEC chapter
itself makes no mention of any speed higher than 1G.

As unlike U-Boot, Linux promises backwards compatibility even for
out-of-tree devicetrees, I kept the existing behavior when doing my
conversion on the off chance that someone had a board configured for
2.5G.

Given the similarities of 2.5G "SGMII" and SGMII, I suspect that the
settings should probably be the same for 2.5G as for 1G.

>> Can you try moving this write to dtsec_mac_config?
> 
> That worked! At least for SGMII. I don't have a board with another
> interface to test tough. I'll prepare a new version, making
> phylink_interface_max_speed() public and setting the correct value
> in .mac_config().

Well, we already have switch/case there, so you could just use that
since only RMII would default to nibble mode.

> Alternatively, we could move
> phylink_interface_max_speed() to the header as static inline, but
> that function isn't that small.
> 
>> And check in the RM
>> whether this is configured based on the interface (in which case we should
>> remove it from dtsec_link_up) or the link speed. And please also check what
>> the correct behavior for 2.5G should be.
> 
> This is an excerpt from the RM:
> 
> I/F Mode (bits 22-23):
>   This field determines the type of interface to which the MAC is
>   connected. Its default is 00.

Of course, this implies that it's OK to leave this field as 0, which is probably
why I left the logic as-is. But since you actually tested this (and I just tried
to piece things together from the RMs) we should set it.

>    00 Reserved
>    01 Used for all 10Mbps and 100Mbps speeds
>    10 Used for all 1Gbps speeds
>    11 Reserved
> 
> So you could read it both ways? But since the older driver was using
> the maximum interface speed..
> 
>> At one point I had the P-series RMs downloaded, but it appears I've misplaced
>> them...
> 
> FWIW, I think it's freely available as long as you're logged in with
> your NXP account.

I was at home without access to my NXP account :)

--Sean

> -michael
> 
>>
>> --Sean
>>
>>> After converting, init will just
>>> set 0 as the mode. According to the "QorIQ Data Path Acceleration
>>> Architecture (DPAA) Reference Manual", this is a reserved value. In
>>> fact, this will prevent the PCS to establish a link to a connected SGMII
>>> PHY. In turn, mac_link_up() is never called. Fix it by setting a
>>> non-reserved mode; mac_link_up() will then set the correct mode later.
>>>
>>> Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink")
>>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>>> ---
>>> FWIW, I've tested this with a Marvell 88E1112 PHY.
>>>
>>>    drivers/net/ethernet/freescale/fman/fman_dtsec.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>> index fe35703c509e..566921d3a884 100644
>>> --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>> +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
>>> @@ -402,7 +402,10 @@ static int init(struct dtsec_regs __iomem *regs, struct dtsec_cfg *cfg,
>>>    	tmp |= MACCFG1_TX_FLOW;
>>>    	iowrite32be(tmp, &regs->maccfg1);
>>>    
>>> -	tmp = 0;
>>> +	/* write a non-reserved mode, otherwise the PCS won't establish a link
>>> +	 * and .mac_link_up() is never called.
>>> +	 */
>>> +	tmp = MACCFG2_NIBBLE_MODE;
>>>    
>>>    	tmp |= (cfg->preamble_len << MACCFG2_PREAMBLE_LENGTH_SHIFT) &
>>>    		MACCFG2_PREAMBLE_LENGTH_MASK;
>