Re: [RESEND PATCH net-next v8 1/1] net: mdio: Add RTL9300 MDIO driver

[Paolo Abeni <pabeni@redhat.com>]

Hi,


On 3/4/25 2:52 AM, 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@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v8:
>     - Fix typo in user visible string
>     Changes in v7:
>     - Update out of date comment
>     - Use for_each_set_bit() instead of open-coded iteration
>     - Use FIELD_PREP() in a few more places
>     - Add #defines for register field masks
>     Changes in v6:
>     - Parse port->phy mapping from devicetree removing the need for the
>       realtek,port property
>     - Remove erroneous code dealing with SMI_POLL_CTRL. When actually
>       implemented this stops the LED unit from updating correctly.
>     Changes in v5:
>     - Reword out of date comment
>     - Use GENMASK/FIELD_PREP where appropriate
>     - Introduce port validity bitmap.
>     - Use more obvious names for PHY_CTRL_READ/WRITE and
>       PHY_CTRL_TYPE_C45/C22
>     Changes in v4:
>     - rename to realtek-rtl9300
>     - s/realtek_/rtl9300_/
>     - add locking to support concurrent access
>     - The dtbinding now represents the MDIO bus hierarchy so we consume this
>       information and use it to configure the switch port to MDIO bus+addr.
>     Changes in v3:
>     - Fix (another) off-by-one error
>     Changes in v2:
>     - Add clause 22 support
>     - Remove commented out code
>     - Formatting cleanup
>     - Set MAX_PORTS correctly for MDIO interface
>     - Fix off-by-one error in pn check
> 
>  drivers/net/mdio/Kconfig                |   7 +
>  drivers/net/mdio/Makefile               |   1 +
>  drivers/net/mdio/mdio-realtek-rtl9300.c | 475 ++++++++++++++++++++++++
>  3 files changed, 483 insertions(+)
>  create mode 100644 drivers/net/mdio/mdio-realtek-rtl9300.c
> 
> diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
> index 4a7a303be2f7..058fcdaf6c18 100644
> --- a/drivers/net/mdio/Kconfig
> +++ b/drivers/net/mdio/Kconfig
> @@ -185,6 +185,13 @@ config MDIO_IPQ8064
>  	  This driver supports the MDIO interface found in the network
>  	  interface units of the IPQ8064 SoC
>  
> +config MDIO_REALTEK_RTL9300
> +	tristate "Realtek RTL9300 MDIO interface support"
> +	depends on MACH_REALTEK_RTL || COMPILE_TEST
> +	help
> +	  This driver supports the MDIO interface found in the Realtek
> +	  RTL9300 family of Ethernet switches with integrated SoC.
> +
>  config MDIO_REGMAP
>  	tristate
>  	help
> diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
> index 1015f0db4531..c23778e73890 100644
> --- a/drivers/net/mdio/Makefile
> +++ b/drivers/net/mdio/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
>  obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
>  obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
>  obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
> +obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
>  obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
>  obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
>  obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> new file mode 100644
> index 000000000000..0a97c2a9c46d
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -0,0 +1,475 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MDIO controller for RTL9300 switches with integrated SoC.
> + *
> + * The MDIO communication is abstracted by the switch. At the software level
> + * communication uses the switch port to address the PHY. We work out the
> + * mapping based on the MDIO bus described in device tree and phandles on the
> + * ethernet-ports property.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/find.h>
> +#include <linux/mdio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define SMI_GLB_CTRL			0xca00
> +#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL	0xca08
> +#define SMI_ACCESS_PHY_CTRL_0		0xcb70
> +#define SMI_ACCESS_PHY_CTRL_1		0xcb74
> +#define   PHY_CTRL_REG_ADDR		GENMASK(24, 20)
> +#define   PHY_CTRL_PARK_PAGE		GENMASK(19, 15)
> +#define   PHY_CTRL_MAIN_PAGE		GENMASK(14, 3)
> +#define   PHY_CTRL_WRITE		BIT(2)
> +#define   PHY_CTRL_READ			0
> +#define   PHY_CTRL_TYPE_C45		BIT(1)
> +#define   PHY_CTRL_TYPE_C22		0
> +#define   PHY_CTRL_CMD			BIT(0)
> +#define   PHY_CTRL_FAIL			BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2		0xcb78
> +#define   PHY_CTRL_INDATA		GENMASK(31, 16)
> +#define   PHY_CTRL_DATA			GENMASK(15, 0)
> +#define SMI_ACCESS_PHY_CTRL_3		0xcb7c
> +#define   PHY_CTRL_MMD_DEVAD		GENMASK(20, 16)
> +#define   PHY_CTRL_MMD_REG		GENMASK(15, 0)
> +#define SMI_PORT0_5_ADDR_CTRL		0xcb80
> +
> +#define MAX_PORTS       28
> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR	0x1f
> +
> +struct rtl9300_mdio_priv {
> +	struct regmap *regmap;
> +	struct mutex lock; /* protect HW access */
> +	DECLARE_BITMAP(valid_ports, MAX_PORTS);
> +	u8 smi_bus[MAX_PORTS];
> +	u8 smi_addr[MAX_PORTS];
> +	bool smi_bus_is_c45[MAX_SMI_BUSSES];
> +	struct mii_bus *bus[MAX_SMI_BUSSES];
> +};
> +
> +struct rtl9300_mdio_chan {
> +	struct rtl9300_mdio_priv *priv;
> +	u8 mdio_bus;
> +};
> +
> +static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id)
> +{
> +	struct rtl9300_mdio_chan *chan = bus->priv;
> +	struct rtl9300_mdio_priv *priv = chan->priv;
> +	int i;
> +
> +	for_each_set_bit(i, priv->valid_ports, MAX_PORTS)
> +		if (priv->smi_bus[i] == chan->mdio_bus &&
> +		    priv->smi_addr[i] == phy_id)
> +			return i;
> +
> +	return -ENOENT;
> +}
> +
> +static int rtl9300_mdio_wait_ready(struct rtl9300_mdio_priv *priv)
> +{
> +	struct regmap *regmap = priv->regmap;
> +	u32 val;
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	return regmap_read_poll_timeout(regmap, SMI_ACCESS_PHY_CTRL_1,
> +					val, !(val & PHY_CTRL_CMD), 10, 1000);
> +}
> +
> +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 = chan->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int port;
> +	u32 val;
> +	int err;
> +
> +	guard(mutex)(&priv->lock);

I'm sorry for the late feedback but quoting
Documentation/process/maintainer-netdev.rst:

"""
Use of ``guard()`` is discouraged within any function longer than 20 lines,
"""

I suggest plain mutex_lock()/unlock() usage in a

Also please respect the reverse christmass tree in the variable
definiton. You will have to do something alike:

	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;

Finaly, the cover letter is not required for a single patch.

Thanks,

Paolo