| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <66abce08-e50e-491f-932b-bd36ec6b464c@redhat.com>
Date: Thu, 6 Mar 2025 10:30:16 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>, andrew@...n.ch,
hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, sander@...nheule.net,
markus.stockhausen@....de
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH net-next v8 1/1] net: mdio: Add RTL9300 MDIO driver
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@...iedtelesis.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
Powered by blists - more mailing lists