Hello Chris,
> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
I wonder if the name might be dubious in the future with other realtek
products with MDIO. Realtek is quite a large company with many
products. Would a version/model/family/usage in that name help a far
future reader to identify what this file is about?
How would this realtek MDIO driver interact with the
drivers/net/dsa/realtek drivers? I guess it might not be too much as
this is the SoC MDIO bus and not the user MDIO bus (also something
called "realtek MDIO driver"). Also, the code logic there just rhymes
with some register access implemented there.
> @@ -0,0 +1,341 @@
> +// 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.
> + */
> +
> +#include <linux/mdio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.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 0x000
> +#define GLB_CTRL_INTF_SEL(intf) BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL 0x008
> +#define SMI_ACCESS_PHY_CTRL_0 0x170
> +#define SMI_ACCESS_PHY_CTRL_1 0x174
> +#define PHY_CTRL_RWOP BIT(2)
> +#define PHY_CTRL_TYPE BIT(1)
> +#define PHY_CTRL_CMD BIT(0)
> +#define PHY_CTRL_FAIL BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2 0x178
> +#define SMI_ACCESS_PHY_CTRL_3 0x17c
> +#define SMI_PORT0_5_ADDR_CTRL 0x180
> +
> +#define MAX_PORTS 28
> +#define MAX_SMI_BUSSES 4
> +#define MAX_SMI_ADDR 0x1f
> +
> +struct realtek_mdio_priv {
> + struct regmap *regmap;
> + u8 smi_bus[MAX_PORTS];
> + u8 smi_addr[MAX_PORTS];
> + bool smi_bus_isc45[MAX_SMI_BUSSES];
> + u32 reg_base;
> +};
> +
> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
All those realtek_mdio_* prefix might collide with realtek_mdio_* from
drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
is about the interface (MDIO or SMI) between (the other vendor) SoC
and the switch. I don't know if the maintainers are OK with it but
listing those symbols in alphabetic order from both sources might be
confusing.
> +{
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + u32 val;
> +
> + return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
All regmap funcs are adding reg_base to the register address. Isn't a
remap job to do that sum? It just looks odd but I never worked with
MFD. It looks like it is missing a subregmap-like variant.
> + val, !(val & PHY_CTRL_CMD), 10, 500);
> +}
> +
> +static int realtek_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> +{
> + struct realtek_mdio_priv *priv = bus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + u32 val;
> + int err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + regnum << 20 | 0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
> + if (err)
> + return err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> + if (err)
> + return err;
> +
> + return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +{
> + struct realtek_mdio_priv *priv = bus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + u32 val;
> + int err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + regnum << 20 | 0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
> + if (err)
> + return err;
> +
> + err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + val, !(val & PHY_CTRL_CMD), 10, 100);
> + if (err)
> + return err;
> +
> + if (val & PHY_CTRL_FAIL)
> + return -ENXIO;
> +
> + return 0;
> +}
> +
> +static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
> +{
> + struct realtek_mdio_priv *priv = bus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + u32 val;
> + int err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> + dev_addr << 16 | (regnum & 0xffff));
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + PHY_CTRL_TYPE | PHY_CTRL_CMD);
> + if (err)
> + return err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> + if (err)
> + return err;
> +
> + return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> + int regnum, u16 value)
> +{
> + struct realtek_mdio_priv *priv = bus->priv;
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + u32 val;
> + int err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> + dev_addr << 16 | (regnum & 0xffff));
> + if (err)
> + return err;
> +
> + err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
> + if (err)
> + return err;
> +
> + err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> + val, !(val & PHY_CTRL_CMD), 10, 100);
> + if (err)
> + return err;
> +
> + if (val & PHY_CTRL_FAIL)
> + return -ENXIO;
> +
> + return 0;
> +}
> +
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> + struct regmap *regmap = priv->regmap;
> + u32 reg_base = priv->reg_base;
> + 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;
> + }
> +
> + /* 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);
> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> + }
> + }
> +
> + err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_5_ADDR_CTRL,
> + port_addr, 5);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_15_POLLING_SEL,
> + poll_sel, 2);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(regmap, reg_base + SMI_GLB_CTRL,
> + glb_ctrl_mask, glb_ctrl_val);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct realtek_mdio_priv *priv;
> + struct fwnode_handle *child;
> + struct mii_bus *bus;
> + int err;
> +
> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "Reaktek Switch MDIO Bus";
> + bus->read = realtek_mdio_read_c22;
> + bus->write = realtek_mdio_write_c22;
> + bus->read_c45 = realtek_mdio_read_c45;
> + bus->write_c45 = realtek_mdio_write_c45;
> + bus->parent = dev;
> + priv = bus->priv;
> +
> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
> + if (err)
> + return err;
> +
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> + device_for_each_child_node(dev, child) {
> + u32 pn, smi_addr[2];
> +
> + err = fwnode_property_read_u32(child, "reg", &pn);
> + if (err)
> + return err;
> +
> + if (pn >= MAX_PORTS)
> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> + if (err) {
> + smi_addr[0] = 0;
> + smi_addr[1] = pn;
> + }
> +
> + if (smi_addr[0] > MAX_SMI_BUSSES)
> + return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> + smi_addr[0]);
> +
> + if (smi_addr[1] > MAX_SMI_ADDR)
> + return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> +
> + if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> + priv->smi_bus_isc45[smi_addr[0]] = true;
> +
> + priv->smi_bus[pn] = smi_addr[0];
> + priv->smi_addr[pn] = smi_addr[1];
> + }
> +
> + err = realtek_mdiobus_init(priv);
> + if (err)
> + return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> + err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> + if (err)
> + return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id realtek_mdio_ids[] = {
> + { .compatible = "realtek,rtl9301-mdio" },
> + { .compatible = "realtek,rtl9302b-mdio" },
> + { .compatible = "realtek,rtl9302c-mdio" },
> + { .compatible = "realtek,rtl9303-mdio" },
Do these different compatible strings really matter? AFAIK, compatible
are not for listing all supported models/variants but to describe
devices that have a different behavior and indicating that (with
different strings) is needed to decide how the driver will work. If
the driver does not use which compatible was set, it might indicate
that we don't really need 4 compatible but 1.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, realtek_mdio_ids);
> +
> +static struct platform_driver rtl9300_mdio_driver = {
> + .probe = realtek_mdiobus_probe,
> + .driver = {
> + .name = "mdio-rtl9300",
> + .of_match_table = realtek_mdio_ids,
> + },
> +};
> +
> +module_platform_driver(rtl9300_mdio_driver);
> +
> +MODULE_DESCRIPTION("RTL9300 MDIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.47.1
>
>
Regards,
Luiz
