Messages in this thread |  | | Date | Tue, 21 Jan 2025 09:32:52 +1300 | | Subject | Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver | | From | Chris Packham <> |
| |
Hi Sander,
On 20/01/2025 23:28, Sander Vanheule wrote: > Hi Chris, > > On Mon, 2025-01-20 at 17:02 +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@alliedtelesis.co.nz> >> --- > [...] > > >> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c >> new file mode 100644 >> index 000000000000..a9b894eff407 >> --- /dev/null >> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c >> @@ -0,0 +1,417 @@ >> +// 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 with the actual MDIO >> + * bus and address having been setup via the realtek,smi-address property. > realtek,smi-address is a leftover from a previous spin? Oops, will fix > >> + */ >> + >> +#include <linux/cleanup.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_RWOP BIT(2) > With > > #define PHY_CTRL_WRITE BIT(2) > #define PHY_CTRL_READ 0 > > you could use both macros in the write/read functions. Now I have to go and parse the write/read > functions to see what it means when this bit is set. Ack. >> +#define PHY_CTRL_TYPE BIT(1) > Similar here: > #define PHY_CTRL_TYPE_C22 0 > #define PHY_CTRL_TYPE_C45 BIT(1)
Ack
>> +#define PHY_CTRL_CMD BIT(0) >> +#define PHY_CTRL_FAIL BIT(25) >> +#define SMI_ACCESS_PHY_CTRL_2 0xcb78 >> +#define SMI_ACCESS_PHY_CTRL_3 0xcb7c >> +#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 rtl9300_mdio_chan { >> + struct rtl9300_mdio_priv *priv; >> + u8 smi_bus; >> +}; >> + >> +struct rtl9300_mdio_priv { >> + struct regmap *regmap; >> + struct mutex lock; /* protect HW access */ >> + u8 smi_bus[MAX_PORTS]; >> + u8 smi_addr[MAX_PORTS]; >> + bool smi_bus_isc45[MAX_SMI_BUSSES]; > Nit: add an underscore: smi_bus_is_c45
Ack
> >> + struct mii_bus *bus[MAX_SMI_BUSSES]; >> +}; >> + >> +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 (i = 0; i < MAX_PORTS; i++) >> + if (priv->smi_bus[i] == chan->smi_bus && >> + priv->smi_addr[i] == phy_id) >> + return i; > This may break if some lower port numbers are not configured by the user, e.g. phy 0-7 on bus 0 are > mapped to ports 8-15 and ports 0-7 are unused. > When looking up the port number of phy 0 on bus 0, you would get a match on an unconfigured port > (port 0) since smi_bus/smi_addr are zero-initialized. This could be fixed by adding a bitmap > indicating which ports are actually configured. Yes that makes sense. > >> + >> + return -ENOENT; >> +} > [...] > >> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum) >> +{ > [...] > >> + err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1, >> + regnum << 20 | 0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD); > You could use FIELD_PREP() to pack the bitfields. Ack. > >> + if (err) >> + return err; >> + >> + err = rtl9300_mdio_wait_ready(priv); >> + if (err) >> + return err; >> + >> + err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val); >> + if (err) >> + return err; >> + >> + return val & 0xffff; > ... and FIELD_GET() to unpack. Not sure it buys us much in this case since it's just the lower 16 bits but for symmetry and a little extra type checking may as well. > >> +} >> + > [...] > >> + >> +static int rtl9300_mdiobus_init(struct rtl9300_mdio_priv *priv) >> +{ >> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0; >> + struct regmap *regmap = priv->regmap; >> + u32 port_addr[5] = { 0 }; >> + u32 poll_sel[2] = { 0 }; >> + int i, err; >> + >> + /* Associate the port with the SMI interface and PHY */ >> + for (i = 0; i < MAX_PORTS; i++) { >> + int pos; >> + >> + if (priv->smi_bus[i] > 3) >> + continue; >> + >> + pos = (i % 6) * 5; >> + port_addr[i / 6] |= priv->smi_addr[i] << pos; >> + >> + pos = (i % 16) * 2; >> + poll_sel[i / 16] |= priv->smi_bus[i] << pos; > I've read the discussion on v1-v3 and had a quick look at the available documentation. Thinking out > loud here, so you can correct me if I'm making any false assumptions. > > As I understand, the SoC has four physical MDIO/MDC pin pairs. Using the DT description, phy-s are > matched with to specific MDIO bus. PORT_ADDR tells the switch which phy address a port maps to. > POLL_SEL then tells the MDIO controller which bus this port (phy) is assigned to. I have the > impression this [port] <-> [bus, phy] mapping is completely arbitrary. If configured correctly, it > can probably serve as a convenience to match a front panel port number to a specific phy. > > If the port numbers are in fact arbitrary, I think they could be hidden from the user, removing the > need for a custom DT property. You could probably populate the port numbers one by one as phy-s are > enumerated, as you are already storing the assigned port number in the MDIO controller's private > data. > > One complication this might have, is that I suspect these port numbers are not used exclusively by > the MDIO controller, but also by the switch itself. So then there may have to be a way to resolve > (auto-assigned) port numbers outside of this driver too.
I believe the POLL_SEL configuration actually affects an internal port polling unit. From the datasheets I have it seems pretty configurable, you can tell it which phy registers to poll and what values indicate link up/down (the defaults are conveniently setup to match the Realtek PHYs). So I don't think they are arbitrary and I don't think it would be a good idea to change them on the fly. I did consider at one point finding an unused port and re-mapping that to the desired bus/addr on the fly but I'm not sure what that'd do to the PPU and there's no guarantee that there will be a unused port.
>> + } >> + >> + /* Put the interfaces into C45 mode if required */ >> + for (i = 0; i < MAX_SMI_BUSSES; i++) { >> + if (priv->smi_bus_isc45[i]) { >> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i); > Can't glb_ctrl_mask be fixed to GENMASK(19, 16)? I guess it could be. Doing it this way avoids undoing things that may have been set by an earlier boot stage but even as I type that I don't find it a good argument against GENMASK(19, 16). > >> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i); >> + } >> + } > [...] > >> +static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv, >> + struct fwnode_handle *node) >> +{ >> + struct rtl9300_mdio_chan *chan; >> + struct fwnode_handle *child; >> + struct mii_bus *bus; >> + u32 smi_bus; >> + int err; >> + >> + err = fwnode_property_read_u32(node, "reg", &smi_bus); >> + if (err) >> + return err; >> + >> + if (smi_bus >= MAX_SMI_BUSSES) >> + return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n", smi_bus); >> + >> + fwnode_for_each_child_node(node, child) { >> + u32 smi_addr; >> + u32 pn; >> + >> + err = fwnode_property_read_u32(child, "reg", &smi_addr); >> + if (err) >> + return err; > [...] > >> + >> + priv->smi_bus[pn] = smi_bus; >> + priv->smi_addr[pn] = smi_addr; > Nitpicking a bit here, but perhaps the code might be a tad bit easier to read for the non-Realtek > initiated by renaming: > - smi_bus -> mdio_bus or just bus_id (matching the mii_bus *bus member) > - smi_addr -> phy_addr I'll consider that. Certainly `u32 smi_bus` and `u32 smi_addr` can be renamed to match their usage from the dts. Not so sure about priv->smi_bus/priv->smi_addr as I am trying to match the usage in the datasheet. I guess technically they should be smi_set and port_addr but I find "port" here particularly confusing. >> + } > [...] > >> +static int rtl9300_mdiobus_probe(struct platform_device *pdev) >> +{ > [...] > >> + >> + if (device_get_child_node_count(dev) >= MAX_SMI_BUSSES) >> + return dev_err_probe(dev, -EINVAL, "Too many SMI busses\n"); > This seems redundant with the MAX_SMI_BUSSES comparison in probe_one(). The check in probe_one() checks that the mdio bus number is valid whereas this checks that there are at most 4. So not totally redundant but could probably be removed without doing any harm. > > Best, > Sander
|  |