Message-ID: <7269cf0f-21c6-45e1-a1f3-5463bdd9fd5c@alliedtelesis.co.nz>
Date: Thu, 13 Mar 2025 01:04:42 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Daniel Golle <daniel@...rotopia.org>
CC: "andrew@...n.ch" <andrew@...n.ch>, "hkallweit1@...il.com"
	<hkallweit1@...il.com>, "linux@...linux.org.uk" <linux@...linux.org.uk>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "sander@...nheule.net"
	<sander@...nheule.net>, "markus.stockhausen@....de"
	<markus.stockhausen@....de>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v9] net: mdio: Add RTL9300 MDIO driver

Hi Daniel,

On 11/03/2025 04:28, Daniel Golle wrote:
> On Mon, Mar 10, 2025 at 02:07:26AM +0000, Chris Packham wrote:
>> Hi Daniel,
>>
>> On 10/03/2025 14:31, Daniel Golle wrote:
>>> Hi Chris,
>>>
>>> On Mon, Mar 10, 2025 at 12:25:36PM +1300, Chris Packham wrote:
>>>> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
>>>> switches with integrated SoC. There are 4 physical SMI interfaces on the
>>>> RTL9300 however access is done using the switch ports. The driver takes
>>>> the MDIO bus hierarchy from the DTS and uses this to configure the
>>>> switch ports so they are associated with the correct PHY. This mapping
>>>> is also used when dealing with software requests from phylib.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>>>> ---
>>>> ...
>>>> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>>>> +{
>>>> +	struct rtl9300_mdio_chan *chan = bus->priv;
>>>> +	struct rtl9300_mdio_priv *priv;
>>>> +	struct regmap *regmap;
>>>> +	int port;
>>>> +	u32 val;
>>>> +	int err;
>>>> +
>>>> +	priv = chan->priv;
>>>> +	regmap = priv->regmap;
>>>> +
>>>> +	port = rtl9300_mdio_phy_to_port(bus, phy_id);
>>>> +	if (port < 0)
>>>> +		return port;
>>>> +
>>>> +	mutex_lock(&priv->lock);
>>>> +	err = rtl9300_mdio_wait_ready(priv);
>>>> +	if (err)
>>>> +		goto out_err;
>>>> +
>>>> +	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, FIELD_PREP(PHY_CTRL_INDATA, port));
>>>> +	if (err)
>>>> +		goto out_err;
>>>> +
>>>> +	val = FIELD_PREP(PHY_CTRL_REG_ADDR, regnum) |
>>>> +	      FIELD_PREP(PHY_CTRL_PARK_PAGE, 0x1f) |
>>>> +	      FIELD_PREP(PHY_CTRL_MAIN_PAGE, 0xfff) |
>>>> +	      PHY_CTRL_READ | PHY_CTRL_TYPE_C22 | PHY_CTRL_CMD;
>>> Using "raw" access to the PHY and thereby bypassing the MDIO
>>> controller's support for hardware-assisted page access is problematic.
>>> The MDIO controller also polls all PHYs status in hardware and hence
>>> be aware of the currently selected page. Using raw access to switch
>>> the page of a PHY "behind the back" of the hardware polling mechanism
>>> results in in occassional havoc on link status changes in case Linux'
>>> reading the phy status overlaps with the hardware polling.
>>> This is esp. when using RealTek's 2.5GBit/s PHYs which require using
>>> paged access in their read_status() function.
>>>
>>> Markus Stockhausen (already in Cc) has implemented a nice solution to
>>> this problem, including documentation, see
>>> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-6.6/drivers/net/ethernet/rtl838x_eth.c;h=4b79090696e341ed1e432a7ec5c0f7f92776f0e1;hb=HEAD#l1631
>> I read that code/comment a few times and I must admit I still don't
>> quite get it. Part of the problem might be that my hardware platform is
>> using C45 PHYs and that's what I've been testing with. The C22
>> support
>> is based on my reading of the datasheet and some of what I can glean
>> from openwrt (although I completely missed that comment when I read
>> through the driver the first time).
> Yes, this issue exists only with Clause-22 access, Clause-45 doesn't
> require any of that. Also note that OpenWrt has recently switched
> implementation from a (not very upstream friendly) approach requiring
> dedicated support for paged access to Markus' new implementation which
> also added the comment.
>
>>> Including a similar mechanism in this driver for C22 read and write
>>> operations would be my advise, so hardware-assisted access to the PHY
>>> pages is always used, and hence the hardware polling mechanism is aware
>>> of the currently selected page.
>> So far upstream Linux doesn't have generic paged PHY register functions.
>> It sounds like that'd be a prerequisite for this.
> No, that's exactly what Markus has improved about the implementation
> compared to the previous approach:
> We simply intercept access to C22 register 0x1f. For write access the
> to-be-selected page is stored in the priv struct of the MDIO bus, in a
> 0-initialized array for each MDIO bus address. C22 read access to
> register 0x1f can read back that value, all without actually acessing the
> hardware.
> Any subsequent C22 read or write operation will then use the selected
> page stored for that PHY in the memory associated with the MDIO bus.
>
> In this way page selection is left to the MDIO controller which also
> carries out polling in background (see SMI_POLL_CTRL), and clashes due
> to congruent access by Linux and hardware polling don't occur.

Ah I get it. It's about tracking the current page for a particular 
device address and using that instead of 0xfff in 
FIELD_PREP(PHY_CTRL_MAIN_PAGE, 0xfff). I'm not sure it's going to work 
generically. Realtek switches know about Realtek PHYs but I've seen 
plenty of other PHYs that do paging via addresses other than 0x1f 
(Marvell 88E1111 for example uses 0x1d for its extaddr, some Broadcom 
PHYs seem to use 0x1c). I'm not sure how many systems are mixing vendors 
for C22 PHYs (the Zyxel boards seem to have Marvell AQR 10G PHYs but 
that's C45).

I'm going to send v10 out shortly with the minor additions suggested by 
Christophe. If we can come up with something suitable for the C22 paging 
it's probably best done as new code on top of that.

Powered by blists - more mailing lists