Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

+cc netdev, lkml

On 14/03/2025 01:34, Andrew Lunn wrote:
>> +	/* Put the interfaces into C45 mode if required */
>> +	glb_ctrl_mask = GENMASK(19, 16);
>> +	for (i = 0; i < MAX_SMI_BUSSES; i++)
>> +		if (priv->smi_bus_is_c45[i])
>> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>> +
>> +	fwnode_for_each_child_node(node, child)
>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>> +			priv->smi_bus_is_c45[mdio_bus] = true;
>> +
> This needs more explanation. Some PHYs mix C22 and C45, e.g. the > 1G
> speed support registers are in the C45 address space, but <= 1G is in
> the C22 space. And 1G PHYs which support EEE need access to C45 space
> for the EEE registers.

Ah good point. The MDIO interfaces are either in GPHY (i.e. clause 22) 
or 10GPHY mode (i.e. clause 45). This does mean we can't support support 
both c45 and c22 on the same MDIO bus (whether that's one PHY that 
supports both or two different PHYs). I'll add a comment to that effect 
and I should probably only provide bus->read/write or 
bus->read_c45/write_c45 depending on the mode.

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

On Thu, Mar 13, 2025 at 07:54:39PM +0000, Chris Packham wrote:
> +cc netdev, lkml
> 
> On 14/03/2025 01:34, Andrew Lunn wrote:
> >> +	/* Put the interfaces into C45 mode if required */
> >> +	glb_ctrl_mask = GENMASK(19, 16);
> >> +	for (i = 0; i < MAX_SMI_BUSSES; i++)
> >> +		if (priv->smi_bus_is_c45[i])
> >> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> >> +
> >> +	fwnode_for_each_child_node(node, child)
> >> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> >> +			priv->smi_bus_is_c45[mdio_bus] = true;
> >> +
> > This needs more explanation. Some PHYs mix C22 and C45, e.g. the > 1G
> > speed support registers are in the C45 address space, but <= 1G is in
> > the C22 space. And 1G PHYs which support EEE need access to C45 space
> > for the EEE registers.
> 
> Ah good point. The MDIO interfaces are either in GPHY (i.e. clause 22) 
> or 10GPHY mode (i.e. clause 45). This does mean we can't support support 
> both c45 and c22 on the same MDIO bus (whether that's one PHY that 
> supports both or two different PHYs). I'll add a comment to that effect 
> and I should probably only provide bus->read/write or 
> bus->read_c45/write_c45 depending on the mode.

Is there more to it than this? Because why not just set the mode per
bus transaction?

	Andrew

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 14/03/2025 09:35, Andrew Lunn wrote:
> On Thu, Mar 13, 2025 at 07:54:39PM +0000, Chris Packham wrote:
>> +cc netdev, lkml
>>
>> On 14/03/2025 01:34, Andrew Lunn wrote:
>>>> +	/* Put the interfaces into C45 mode if required */
>>>> +	glb_ctrl_mask = GENMASK(19, 16);
>>>> +	for (i = 0; i < MAX_SMI_BUSSES; i++)
>>>> +		if (priv->smi_bus_is_c45[i])
>>>> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>>>> +
>>>> +	fwnode_for_each_child_node(node, child)
>>>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>>>> +			priv->smi_bus_is_c45[mdio_bus] = true;
>>>> +
>>> This needs more explanation. Some PHYs mix C22 and C45, e.g. the > 1G
>>> speed support registers are in the C45 address space, but <= 1G is in
>>> the C22 space. And 1G PHYs which support EEE need access to C45 space
>>> for the EEE registers.
>> Ah good point. The MDIO interfaces are either in GPHY (i.e. clause 22)
>> or 10GPHY mode (i.e. clause 45). This does mean we can't support support
>> both c45 and c22 on the same MDIO bus (whether that's one PHY that
>> supports both or two different PHYs). I'll add a comment to that effect
>> and I should probably only provide bus->read/write or
>> bus->read_c45/write_c45 depending on the mode.
> Is there more to it than this? Because why not just set the mode per
> bus transaction?

It's a bus level setting at init time. You can't dynamically switch modes.

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

On Thu, Mar 13, 2025 at 08:37:18PM +0000, Chris Packham wrote:
> On 14/03/2025 09:35, Andrew Lunn wrote:
> > On Thu, Mar 13, 2025 at 07:54:39PM +0000, Chris Packham wrote:
> >> +cc netdev, lkml
> >>
> >> On 14/03/2025 01:34, Andrew Lunn wrote:
> >>>> +	/* Put the interfaces into C45 mode if required */
> >>>> +	glb_ctrl_mask = GENMASK(19, 16);
> >>>> +	for (i = 0; i < MAX_SMI_BUSSES; i++)
> >>>> +		if (priv->smi_bus_is_c45[i])
> >>>> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> >>>> +
> >>>> +	fwnode_for_each_child_node(node, child)
> >>>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> >>>> +			priv->smi_bus_is_c45[mdio_bus] = true;
> >>>> +
> >>> This needs more explanation. Some PHYs mix C22 and C45, e.g. the > 1G
> >>> speed support registers are in the C45 address space, but <= 1G is in
> >>> the C22 space. And 1G PHYs which support EEE need access to C45 space
> >>> for the EEE registers.
> >> Ah good point. The MDIO interfaces are either in GPHY (i.e. clause 22)
> >> or 10GPHY mode (i.e. clause 45). This does mean we can't support support
> >> both c45 and c22 on the same MDIO bus (whether that's one PHY that
> >> supports both or two different PHYs). I'll add a comment to that effect
> >> and I should probably only provide bus->read/write or
> >> bus->read_c45/write_c45 depending on the mode.
> > Is there more to it than this? Because why not just set the mode per
> > bus transaction?
> 
> It's a bus level setting at init time. You can't dynamically switch modes.

Why not? The bus is only every doing one transaction at a time, so why
not switch it per transaction?

	Andrew

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 14/03/2025 09:40, Andrew Lunn wrote:
> On Thu, Mar 13, 2025 at 08:37:18PM +0000, Chris Packham wrote:
>> On 14/03/2025 09:35, Andrew Lunn wrote:
>>> On Thu, Mar 13, 2025 at 07:54:39PM +0000, Chris Packham wrote:
>>>> +cc netdev, lkml
>>>>
>>>> On 14/03/2025 01:34, Andrew Lunn wrote:
>>>>>> +	/* Put the interfaces into C45 mode if required */
>>>>>> +	glb_ctrl_mask = GENMASK(19, 16);
>>>>>> +	for (i = 0; i < MAX_SMI_BUSSES; i++)
>>>>>> +		if (priv->smi_bus_is_c45[i])
>>>>>> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>>>>>> +
>>>>>> +	fwnode_for_each_child_node(node, child)
>>>>>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>>>>>> +			priv->smi_bus_is_c45[mdio_bus] = true;
>>>>>> +
>>>>> This needs more explanation. Some PHYs mix C22 and C45, e.g. the > 1G
>>>>> speed support registers are in the C45 address space, but <= 1G is in
>>>>> the C22 space. And 1G PHYs which support EEE need access to C45 space
>>>>> for the EEE registers.
>>>> Ah good point. The MDIO interfaces are either in GPHY (i.e. clause 22)
>>>> or 10GPHY mode (i.e. clause 45). This does mean we can't support support
>>>> both c45 and c22 on the same MDIO bus (whether that's one PHY that
>>>> supports both or two different PHYs). I'll add a comment to that effect
>>>> and I should probably only provide bus->read/write or
>>>> bus->read_c45/write_c45 depending on the mode.
>>> Is there more to it than this? Because why not just set the mode per
>>> bus transaction?
>> It's a bus level setting at init time. You can't dynamically switch modes.
> Why not? The bus is only every doing one transaction at a time, so why
> not switch it per transaction?

I'm pretty sure it would upset the hardware polling mechanism which 
unfortunately we can't disable (earlier I thought we could but there are 
various switch features that rely on it).

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

> I'm pretty sure it would upset the hardware polling mechanism which 
> unfortunately we can't disable (earlier I thought we could but there are 
> various switch features that rely on it).

So we need to get a better understanding of that polling. How are you
telling it about the aquantia PHY features? How does it know it needs
to get the current link rate from MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1
which is a vendor register, not a standard C45 register? How do you
teach it to decode bits in that register?

	Andrew

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 14/03/2025 11:07, Andrew Lunn wrote:
>> I'm pretty sure it would upset the hardware polling mechanism which
>> unfortunately we can't disable (earlier I thought we could but there are
>> various switch features that rely on it).
> So we need to get a better understanding of that polling. How are you
> telling it about the aquantia PHY features? How does it know it needs
> to get the current link rate from MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1
> which is a vendor register, not a standard C45 register? How do you
> teach it to decode bits in that register?

The hardware polling for C45 PHYs is reasonably configurable so I think 
you can define which MMD device/register to look at and what bit masks 
to apply to determine the link status. I think when we get to a complete 
switch driver (hoping for switchdev but could be dsa) it may need to 
know some details about the specific PHY that is attached and have some 
way of telling the mdio controller about this.

Right now I'm focusing on a platform that is RTL9300 + RTL8224 (clause 
45) so the defaults mostly just work. I do have one of the Zyxel boards 
with an AQR PHY but haven't been able to root it yet (Markus gave me 
some tips for that just haven't tried them yet).

Re: [PATCH v10] net: mdio: Add RTL9300 MDIO driver

[Daniel Golle]

On Thu, Mar 13, 2025 at 11:07:55PM +0100, Andrew Lunn wrote:
> > I'm pretty sure it would upset the hardware polling mechanism which 
> > unfortunately we can't disable (earlier I thought we could but there are 
> > various switch features that rely on it).
> 
> So we need to get a better understanding of that polling. How are you
> telling it about the aquantia PHY features? How does it know it needs
> to get the current link rate from MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1
> which is a vendor register, not a standard C45 register? How do you
> teach it to decode bits in that register?

There are several registers of the MDIO controller to control which
non-standard registers are polled as well as information about the
register layout [1].

There are lots of constraints which is why not all PHYs can even be
used at all with those switch SoCs -- PHYs which are more or less
standard C45 are easy to support, all one got to do is define MMD
device and registers as well as register layouts for things which
aren't covered by the C45 standard (1G Master/Slave status and control,
as well as a way to access the equivalent of C22 register 0).

But C22 PHYs which aren't RealTek's won't ever work.
Anything which doesn't use register 0x1f for paging is disqualified and
can't be used. I've also just never seen any of those SoCs being used with
anything else than RealTek's 1000Base-T or 2500Base-T PHYs.

Only for 10GBase-T you will find variation, Marvell, Aquantia and some
with Broadcom.

Obviously that's all largely incompatible with Linux' approach to PHY
drivers. Luckily *most* (but not all) switches based on those RealTek
SoC's initialize the PHY polling registers in U-Boot, so usually Linux
doesn't have to touch that (that's why usually we have to make sure
that 'rtk network on' is called in RealTek's U-Boot before launching
Linux).


[1]: There is a very useful reverse-engineered register documentation for
those RealTek SoCs which also covers those registers of the RTL9300:

https://svanheule.net/realtek/longan/feature/mac_control

See SMI_REG_CHK_* and everything with 'POLL' in the register name to get
an idea...

For illustation see the default value of SMI_10GPHY_POLLING_SEL_0 which
is 0x001f_a434. So that's what is called 'RTL_VND2_PHYSR' in the Linux
driver for RealTek PHYs...