| Message ID | 20211218081425.18722-8-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,01/13] dt-bindings: net: dsa: realtek-smi: mark unsupported switches | expand |
On 12/18/21 09:14, Luiz Angelo Daros de Luca wrote: > This driver is a mdio_driver instead of a platform driver (like > realtek-smi). I noticed in the vendor driver that the PHY register access procedure is different depending on whether we are connected via MDIO or SMI. For SMI it is actually pretty complicated due to indirect register access logic, but when you are using MDIO already it seems rather more straightforward. Did you check that PHY register access is working correctly? I assume so since otherwise you wouldn't even get a link up. Second question is whether you considered simplifying it in the MDIO case, but I suppose if it works for both cases then there is no harm done. > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/Kconfig | 11 +- > drivers/net/dsa/realtek/Makefile | 1 + > drivers/net/dsa/realtek/realtek-mdio.c | 270 +++++++++++++++++++++++++ > drivers/net/dsa/realtek/realtek.h | 2 + > 4 files changed, 282 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/dsa/realtek/realtek-mdio.c > > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig > index cd1aa95b7bf0..73b26171fade 100644 > --- a/drivers/net/dsa/realtek/Kconfig > +++ b/drivers/net/dsa/realtek/Kconfig > @@ -9,6 +9,13 @@ menuconfig NET_DSA_REALTEK > help > Select to enable support for Realtek Ethernet switch chips. > > +config NET_DSA_REALTEK_MDIO > + tristate "Realtek MDIO connected switch driver" > + depends on NET_DSA_REALTEK > + default y > + help > + Select to enable support for registering switches configured > + through MDIO. > config NET_DSA_REALTEK_SMI > tristate "Realtek SMI connected switch driver" > depends on NET_DSA_REALTEK > @@ -21,7 +28,7 @@ config NET_DSA_REALTEK_RTL8365MB > tristate "Realtek RTL8365MB switch subdriver" > default y > depends on NET_DSA_REALTEK > - depends on NET_DSA_REALTEK_SMI > + depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO > select NET_DSA_TAG_RTL8_4 > help > Select to enable support for Realtek RTL8365MB > @@ -30,7 +37,7 @@ config NET_DSA_REALTEK_RTL8366RB > tristate "Realtek RTL8366RB switch subdriver" > default y > depends on NET_DSA_REALTEK > - depends on NET_DSA_REALTEK_SMI > + depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO > select NET_DSA_TAG_RTL4_A > help > Select to enable support for Realtek RTL8366RB > diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile > index 8b5a4abcedd3..0aab57252a7c 100644 > --- a/drivers/net/dsa/realtek/Makefile > +++ b/drivers/net/dsa/realtek/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o > obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o > obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o > rtl8366-objs := rtl8366-core.o rtl8366rb.o > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > new file mode 100644 > index 000000000000..08d13bb94d91 > --- /dev/null > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Realtek MDIO interface driver > + * > + * ASICs we intend to support with this driver: > + * > + * RTL8366 - The original version, apparently > + * RTL8369 - Similar enough to have the same datsheet as RTL8366 > + * RTL8366RB - Probably reads out "RTL8366 revision B", has a quite > + * different register layout from the other two > + * RTL8366S - Is this "RTL8366 super"? > + * RTL8367 - Has an OpenWRT driver as well > + * RTL8368S - Seems to be an alternative name for RTL8366RB > + * RTL8370 - Also uses SMI > + * > + * Copyright (C) 2017 Linus Walleij <linus.walleij@linaro.org> > + * Copyright (C) 2010 Antti Seppälä <a.seppala@gmail.com> > + * Copyright (C) 2010 Roman Yeryomin <roman@advem.lv> > + * Copyright (C) 2011 Colin Leitner <colin.leitner@googlemail.com> > + * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org> > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/spinlock.h> I don't think you need all of these includes (e.g. spinlock). > +#include <linux/skbuff.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_mdio.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/bitops.h> > +#include <linux/if_bridge.h> > + > +#include "realtek.h" > + > +/* Read/write via mdiobus */ > +#define MDC_MDIO_CTRL0_REG 31 > +#define MDC_MDIO_START_REG 29 > +#define MDC_MDIO_CTRL1_REG 21 > +#define MDC_MDIO_ADDRESS_REG 23 > +#define MDC_MDIO_DATA_WRITE_REG 24 > +#define MDC_MDIO_DATA_READ_REG 25 > + > +#define MDC_MDIO_START_OP 0xFFFF > +#define MDC_MDIO_ADDR_OP 0x000E > +#define MDC_MDIO_READ_OP 0x0001 > +#define MDC_MDIO_WRITE_OP 0x0003 > +#define MDC_REALTEK_DEFAULT_PHY_ADDR 0x0 > + > +int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) > +{ > + u32 phy_id = priv->phy_id; > + struct mii_bus *bus = priv->bus; > + > + BUG_ON(in_interrupt()); Again, please don't use BUG here - just make sure this never happens (it looks OK to me). There is a separate warning in mutex_lock which may print a stacktrace if the kernel is configured to do so. > + > + mutex_lock(&bus->mdio_lock); Newline (to balance the newline before mutex_unlock)? > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); I'm curious where these START commands came from because I cannot find an anlogous procedure in the Realtek driver dump I have. Did you figure this out empirically? Does it not work without? Same goes for the other ones downstairs. > + > + /* Write address control code to register 31 */ Not particularly informative to state the register number when we have macros... > + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write address to register 23 */ > + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write read control code to register 21 */ > + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_READ_OP); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Read data from register 25 */ > + *data = bus->read(bus, phy_id, MDC_MDIO_DATA_READ_REG); > + > + mutex_unlock(&bus->mdio_lock); > + > + return 0; > +} > + > +static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data) > +{ > + u32 phy_id = priv->phy_id; > + struct mii_bus *bus = priv->bus; > + > + BUG_ON(in_interrupt()); > + > + mutex_lock(&bus->mdio_lock); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write address control code to register 31 */ > + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write address to register 23 */ > + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write data to register 24 */ > + bus->write(bus, phy_id, MDC_MDIO_DATA_WRITE_REG, data); > + > + /* Write Start command to register 29 */ > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > + > + /* Write data control code to register 21 */ > + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_WRITE_OP); > + > + mutex_unlock(&bus->mdio_lock); > + return 0; Newline before return? > +} > + > +/* Regmap accessors */ > + > +static int realtek_mdio_write(void *ctx, u32 reg, u32 val) > +{ > + struct realtek_priv *priv = ctx; > + > + return realtek_mdio_write_reg(priv, reg, val); > +} > + > +static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > +{ > + struct realtek_priv *priv = ctx; > + > + return realtek_mdio_read_reg(priv, reg, val); > +} > + > +static const struct regmap_config realtek_mdio_regmap_config = { > + .reg_bits = 10, /* A4..A0 R4..R0 */ > + .val_bits = 16, > + .reg_stride = 1, > + /* PHY regs are at 0x8000 */ > + .max_register = 0xffff, > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + .reg_read = realtek_mdio_read, > + .reg_write = realtek_mdio_write, > + .cache_type = REGCACHE_NONE, > +}; > + > +static int realtek_mdio_probe(struct mdio_device *mdiodev) > +{ > + struct realtek_priv *priv; > + struct device *dev = &mdiodev->dev; > + const struct realtek_variant *var; > + int ret; > + struct device_node *np; > + > + var = of_device_get_match_data(dev); > + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->phy_id = mdiodev->addr; > + > + // Start by setting up the register mapping Please use C style comments. > + priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config); > + > + priv->bus = mdiodev->bus; > + priv->dev = &mdiodev->dev; > + priv->chip_data = (void *)priv + sizeof(*priv); > + > + priv->clk_delay = var->clk_delay; > + priv->cmd_read = var->cmd_read; > + priv->cmd_write = var->cmd_write; > + priv->ops = var->ops; > + > + if (IS_ERR(priv->map)) > + dev_warn(dev, "regmap initialization failed"); Shouldn't you bail out here? Like from the smi part: smi->map = devm_regmap_init(dev, NULL, smi, &realtek_smi_mdio_regmap_config); if (IS_ERR(smi->map)) { ret = PTR_ERR(smi->map); dev_err(dev, "regmap init failed: %d\n", ret); return ret; } It is also more common to check the return value (in this case from devm_regmap_init) immediately after the call, not after filling the rest of a struct. > + > + priv->write_reg_noack = realtek_mdio_write_reg; > + > + np = dev->of_node; > + > + dev_set_drvdata(dev, priv); > + spin_lock_init(&priv->lock); > + > + /* TODO: if power is software controlled, set up any regulators here */ > + priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); > + > + ret = priv->ops->detect(priv); > + if (ret) { > + dev_err(dev, "unable to detect switch\n"); > + return ret; > + } > + > + priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); > + if (!priv->ds) > + return -ENOMEM; > + > + priv->ds->dev = dev; > + priv->ds->num_ports = priv->num_ports; > + priv->ds->priv = priv; > + priv->ds->ops = var->ds_ops; > + > + ret = dsa_register_switch(priv->ds); > + if (ret) { > + dev_err(priv->dev, "unable to register switch ret = %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static void realtek_mdio_remove(struct mdio_device *mdiodev) > +{ > + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > + > + if (!priv) > + return; > + > + dsa_unregister_switch(priv->ds); > + //gpiod_set_value(priv->reset, 1); What's going on here? > + dev_set_drvdata(&mdiodev->dev, NULL); > +} > + > +static void realtek_mdio_shutdown(struct mdio_device *mdiodev) > +{ > + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > + > + if (!priv) > + return; > + > + dsa_switch_shutdown(priv->ds); > + > + dev_set_drvdata(&mdiodev->dev, NULL); > +} > + > +static const struct of_device_id realtek_mdio_of_match[] = { > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB) > + { .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, }, > +#endif > + /* FIXME: add support for RTL8366S and more */ > + { .compatible = "realtek,rtl8366s", .data = NULL, }, Won't this cause a NULL-pointer dereference in realtek_mdio_probe() since we don't check if of_device_get_match_data() returns NULL? It's also the same in the SMI part. Linus, should we just delete this line for now? Surely it never worked? > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) > + { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, > +#endif > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, realtek_mdio_of_match); > + > +static struct mdio_driver realtek_mdio_driver = { > + .mdiodrv.driver = { > + .name = "realtek-mdio", > + .of_match_table = of_match_ptr(realtek_mdio_of_match), > + }, > + .probe = realtek_mdio_probe, > + .remove = realtek_mdio_remove, > + .shutdown = realtek_mdio_shutdown, > +}; > + > +mdio_module_driver(realtek_mdio_driver); > + > +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); > +MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index a03de15c4a94..97274273cb3b 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -50,6 +50,8 @@ struct realtek_priv { > struct gpio_desc *mdio; > struct regmap *map; > struct mii_bus *slave_mii_bus; > + struct mii_bus *bus; > + int phy_id; > > unsigned int clk_delay; > u8 cmd_read;
> > +int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) > > +{ > > + u32 phy_id = priv->phy_id; > > + struct mii_bus *bus = priv->bus; > > + > > + BUG_ON(in_interrupt()); > > Again, please don't use BUG here - just make sure this never happens (it > looks OK to me). There is a separate warning in mutex_lock which may > print a stacktrace if the kernel is configured to do so. I did not look at the full patch. But if this is a standard MDIO driver, there is no way we are in interrupt context. The MDIO layer takes a mutex to prevent parallel operations. Also, if you really do want to put a test here, it is better to make use of the kernel infrastructure. might_sleep() if the correct way to do this. Andrew
On Sun, Dec 19, 2021 at 10:17 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > > + /* FIXME: add support for RTL8366S and more */ > > + { .compatible = "realtek,rtl8366s", .data = NULL, }, > > Won't this cause a NULL-pointer dereference in realtek_mdio_probe() > since we don't check if of_device_get_match_data() returns NULL? > > It's also the same in the SMI part. Linus, should we just delete this > line for now? Surely it never worked? It's fine to delete this. I don't know what I was thinking. I think someone said they were working on an RTL8366s driver at the time and wanted to have a placeholder, but as it turns out that driver never materialized. Yours, Linus Walleij
Em dom., 19 de dez. de 2021 18:17, Alvin Šipraga <ALSI@bang-olufsen.dk> escreveu: > > On 12/18/21 09:14, Luiz Angelo Daros de Luca wrote: > > This driver is a mdio_driver instead of a platform driver (like > > realtek-smi). > > I noticed in the vendor driver that the PHY register access procedure is > different depending on whether we are connected via MDIO or SMI. For SMI > it is actually pretty complicated due to indirect register access logic, > but when you are using MDIO already it seems rather more > straightforward. Did you check that PHY register access is working > correctly? I assume so since otherwise you wouldn't even get a link up. > Second question is whether you considered simplifying it in the MDIO > case, but I suppose if it works for both cases then there is no harm done. AFAIK, indirect access is working as expected and for both cases. > > > > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com> > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/realtek/Kconfig | 11 +- > > drivers/net/dsa/realtek/Makefile | 1 + > > drivers/net/dsa/realtek/realtek-mdio.c | 270 +++++++++++++++++++++++++ > > drivers/net/dsa/realtek/realtek.h | 2 + > > 4 files changed, 282 insertions(+), 2 deletions(-) > > create mode 100644 drivers/net/dsa/realtek/realtek-mdio.c > > > > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig > > index cd1aa95b7bf0..73b26171fade 100644 > > --- a/drivers/net/dsa/realtek/Kconfig > > +++ b/drivers/net/dsa/realtek/Kconfig > (...) > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/device.h> > > +#include <linux/spinlock.h> > > I don't think you need all of these includes (e.g. spinlock). OK. I'll take a look. > > > +#include <linux/skbuff.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/of_mdio.h> > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/bitops.h> > > +#include <linux/if_bridge.h> > > + > > +#include "realtek.h" > > + > > +/* Read/write via mdiobus */ > > +#define MDC_MDIO_CTRL0_REG 31 > > +#define MDC_MDIO_START_REG 29 > > +#define MDC_MDIO_CTRL1_REG 21 > > +#define MDC_MDIO_ADDRESS_REG 23 > > +#define MDC_MDIO_DATA_WRITE_REG 24 > > +#define MDC_MDIO_DATA_READ_REG 25 > > + > > +#define MDC_MDIO_START_OP 0xFFFF > > +#define MDC_MDIO_ADDR_OP 0x000E > > +#define MDC_MDIO_READ_OP 0x0001 > > +#define MDC_MDIO_WRITE_OP 0x0003 > > +#define MDC_REALTEK_DEFAULT_PHY_ADDR 0x0 > > + > > +int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) > > +{ > > + u32 phy_id = priv->phy_id; > > + struct mii_bus *bus = priv->bus; > > + > > + BUG_ON(in_interrupt()); > > Again, please don't use BUG here - just make sure this never happens (it > looks OK to me). There is a separate warning in mutex_lock which may > print a stacktrace if the kernel is configured to do so. OK. So, is it safe to simply drop it? I'm not sure what this check is trying to avoid but it looks like mdio read/write are not allowed to come from interruptions. The subdrivers can configure an interrupt controller property to use it instead of polling port status. Would that trigger this BUG_ON? Should I try harder to avoid the presence of an interrupt controller? In my case, there is no dedicated interrupt GPIO for my Realtek switch and maybe it is a requirement for mdio-connected switches. > > > + > > + mutex_lock(&bus->mdio_lock); > > Newline (to balance the newline before mutex_unlock)? OK > > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > I'm curious where these START commands came from because I cannot find > an anlogous procedure in the Realtek driver dump I have. Did you figure > this out empirically? Does it not work without? Same goes for the other > ones downstairs. It came from OpenWrt rtl8667b driver: https://github.com/openwrt/openwrt/blob/f0c0b18234418c6ed6d35fcf1c6e5b0cbdceed49/target/linux/generic/files/drivers/net/phy/rtl8366_smi.c#L266 Trully, I didn't study the details in the MDIO read/write. I just used a code that has been working for years in OpenWrt. I didn't test it without the start but I'll do it in a couple of days. Now I'm away from my device. > > > > > + > > + /* Write address control code to register 31 */ > > Not particularly informative to state the register number when we have > macros... I'll improve it or drop it. > > > + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write address to register 23 */ > > + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write read control code to register 21 */ > > + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_READ_OP); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Read data from register 25 */ > > + *data = bus->read(bus, phy_id, MDC_MDIO_DATA_READ_REG); > > + > > + mutex_unlock(&bus->mdio_lock); > > + > > + return 0; > > +} > > + > > +static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data) > > +{ > > + u32 phy_id = priv->phy_id; > > + struct mii_bus *bus = priv->bus; > > + > > + BUG_ON(in_interrupt()); > > + > > + mutex_lock(&bus->mdio_lock); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write address control code to register 31 */ > > + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write address to register 23 */ > > + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write data to register 24 */ > > + bus->write(bus, phy_id, MDC_MDIO_DATA_WRITE_REG, data); > > + > > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > + > > + /* Write data control code to register 21 */ > > + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_WRITE_OP); > > + > > + mutex_unlock(&bus->mdio_lock); > > + return 0; > > Newline before return? ok > > > +} > > + > > +/* Regmap accessors */ > > + > > +static int realtek_mdio_write(void *ctx, u32 reg, u32 val) > > +{ > > + struct realtek_priv *priv = ctx; > > + > > + return realtek_mdio_write_reg(priv, reg, val); > > +} > > + > > +static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > > +{ > > + struct realtek_priv *priv = ctx; > > + > > + return realtek_mdio_read_reg(priv, reg, val); > > +} > > + > > +static const struct regmap_config realtek_mdio_regmap_config = { > > + .reg_bits = 10, /* A4..A0 R4..R0 */ > > + .val_bits = 16, > > + .reg_stride = 1, > > + /* PHY regs are at 0x8000 */ > > + .max_register = 0xffff, > > + .reg_format_endian = REGMAP_ENDIAN_BIG, > > + .reg_read = realtek_mdio_read, > > + .reg_write = realtek_mdio_write, > > + .cache_type = REGCACHE_NONE, > > +}; > > + > > +static int realtek_mdio_probe(struct mdio_device *mdiodev) > > +{ > > + struct realtek_priv *priv; > > + struct device *dev = &mdiodev->dev; > > + const struct realtek_variant *var; > > + int ret; > > + struct device_node *np; > > + > > + var = of_device_get_match_data(dev); > > + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->phy_id = mdiodev->addr; > > + > > + // Start by setting up the register mapping > > Please use C style comments. I'll just drop it. It is not very informative. > > > + priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config); > > + > > + priv->bus = mdiodev->bus; > > + priv->dev = &mdiodev->dev; > > + priv->chip_data = (void *)priv + sizeof(*priv); > > + > > + priv->clk_delay = var->clk_delay; > > + priv->cmd_read = var->cmd_read; > > + priv->cmd_write = var->cmd_write; > > + priv->ops = var->ops; > > + > > + if (IS_ERR(priv->map)) > > + dev_warn(dev, "regmap initialization failed"); > > Shouldn't you bail out here? Like from the smi part: > > smi->map = devm_regmap_init(dev, NULL, smi, > &realtek_smi_mdio_regmap_config); > if (IS_ERR(smi->map)) { > ret = PTR_ERR(smi->map); > dev_err(dev, "regmap init failed: %d\n", ret); > return ret; > } > > It is also more common to check the return value (in this case from > devm_regmap_init) immediately after the call, not after filling the rest > of a struct. Yes. Thanks. > > > + > > + priv->write_reg_noack = realtek_mdio_write_reg; > > + > > + np = dev->of_node; > > + > > + dev_set_drvdata(dev, priv); > > + spin_lock_init(&priv->lock); > > + > > + /* TODO: if power is software controlled, set up any regulators here */ > > + priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); > > + > > + ret = priv->ops->detect(priv); > > + if (ret) { > > + dev_err(dev, "unable to detect switch\n"); > > + return ret; > > + } > > + > > + priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); > > + if (!priv->ds) > > + return -ENOMEM; > > + > > + priv->ds->dev = dev; > > + priv->ds->num_ports = priv->num_ports; > > + priv->ds->priv = priv; > > + priv->ds->ops = var->ds_ops; > > + > > + ret = dsa_register_switch(priv->ds); > > + if (ret) { > > + dev_err(priv->dev, "unable to register switch ret = %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void realtek_mdio_remove(struct mdio_device *mdiodev) > > +{ > > + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > + > > + if (!priv) > > + return; > > + > > + dsa_unregister_switch(priv->ds); > > + //gpiod_set_value(priv->reset, 1); > > What's going on here? It is a leftover from realtek-smi code. The SMI code deasserts the reset pin during the probe, because it asserted it during removal. For SMI, the reset gpio is required but I'm not sure it should be even there. In my device, there is a reset pin for the internal switch but I'm not sure if it is shared with the realtek switch. It would give an interesting state if the internal switch resets when someone removes the external switch driver. I'm not sure if I should reenable this or simply drop it. Even if the device requires a reset, it would only happen after it was unloaded. Anyway, I think that requiring a reset is just a symptom that the driver initialization steps is missing something. > > + dev_set_drvdata(&mdiodev->dev, NULL); > > +} > > + > > +static void realtek_mdio_shutdown(struct mdio_device *mdiodev) > > +{ > > + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > + > > + if (!priv) > > + return; > > + > > + dsa_switch_shutdown(priv->ds); > > + > > + dev_set_drvdata(&mdiodev->dev, NULL); > > +} > > + > > +static const struct of_device_id realtek_mdio_of_match[] = { > > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB) > > + { .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, }, > > +#endif > > + /* FIXME: add support for RTL8366S and more */ > > + { .compatible = "realtek,rtl8366s", .data = NULL, }, > > Won't this cause a NULL-pointer dereference in realtek_mdio_probe() > since we don't check if of_device_get_match_data() returns NULL? > > It's also the same in the SMI part. Linus, should we just delete this > line for now? Surely it never worked? > > > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) > > + { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, > > +#endif > > + { /* sentinel */ }, > > +}; > > +MODULE_DEVICE_TABLE(of, realtek_mdio_of_match); > > + > > +static struct mdio_driver realtek_mdio_driver = { > > + .mdiodrv.driver = { > > + .name = "realtek-mdio", > > + .of_match_table = of_match_ptr(realtek_mdio_of_match), > > + }, > > + .probe = realtek_mdio_probe, > > + .remove = realtek_mdio_remove, > > + .shutdown = realtek_mdio_shutdown, > > +}; > > + > > +mdio_module_driver(realtek_mdio_driver); > > + > > +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); > > +MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > > index a03de15c4a94..97274273cb3b 100644 > > --- a/drivers/net/dsa/realtek/realtek.h > > +++ b/drivers/net/dsa/realtek/realtek.h > > @@ -50,6 +50,8 @@ struct realtek_priv { > > struct gpio_desc *mdio; > > struct regmap *map; > > struct mii_bus *slave_mii_bus; > > + struct mii_bus *bus; > > + int phy_id; > > > > unsigned int clk_delay; > > u8 cmd_read; >
> > > +int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) > > > +{ > > > + u32 phy_id = priv->phy_id; > > > + struct mii_bus *bus = priv->bus; > > > + > > > + BUG_ON(in_interrupt()); > > > > Again, please don't use BUG here - just make sure this never happens (it > > looks OK to me). There is a separate warning in mutex_lock which may > > print a stacktrace if the kernel is configured to do so. > > OK. So, is it safe to simply drop it? Yes. The MDIO core will never call this in interrupt context. If you want to be paranoid, use might_sleep(); and build your kernel with CONFIG_DEBUG_ATOMIC_SLEEP. Andrew
> > + /* Write Start command to register 29 */ > > + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); > > I'm curious where these START commands came from because I cannot find > an anlogous procedure in the Realtek driver dump I have. Did you figure > this out empirically? Does it not work without? Same goes for the other > ones downstairs. It works without the MDC_MDIO_START_REG. I'm also curious that it really does. It is present in uboot driver and OpenWrt driver. That MDC_MDIO_START_REG is even defined in Realtek rtl8637c, but not used. I think it might be something that an old family (rtl8637b) required but not newer ones. I'll drop it.
diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig index cd1aa95b7bf0..73b26171fade 100644 --- a/drivers/net/dsa/realtek/Kconfig +++ b/drivers/net/dsa/realtek/Kconfig @@ -9,6 +9,13 @@ menuconfig NET_DSA_REALTEK help Select to enable support for Realtek Ethernet switch chips. +config NET_DSA_REALTEK_MDIO + tristate "Realtek MDIO connected switch driver" + depends on NET_DSA_REALTEK + default y + help + Select to enable support for registering switches configured + through MDIO. config NET_DSA_REALTEK_SMI tristate "Realtek SMI connected switch driver" depends on NET_DSA_REALTEK @@ -21,7 +28,7 @@ config NET_DSA_REALTEK_RTL8365MB tristate "Realtek RTL8365MB switch subdriver" default y depends on NET_DSA_REALTEK - depends on NET_DSA_REALTEK_SMI + depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO select NET_DSA_TAG_RTL8_4 help Select to enable support for Realtek RTL8365MB @@ -30,7 +37,7 @@ config NET_DSA_REALTEK_RTL8366RB tristate "Realtek RTL8366RB switch subdriver" default y depends on NET_DSA_REALTEK - depends on NET_DSA_REALTEK_SMI + depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO select NET_DSA_TAG_RTL4_A help Select to enable support for Realtek RTL8366RB diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile index 8b5a4abcedd3..0aab57252a7c 100644 --- a/drivers/net/dsa/realtek/Makefile +++ b/drivers/net/dsa/realtek/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o rtl8366-objs := rtl8366-core.o rtl8366rb.o diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c new file mode 100644 index 000000000000..08d13bb94d91 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -0,0 +1,270 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Realtek MDIO interface driver + * + * ASICs we intend to support with this driver: + * + * RTL8366 - The original version, apparently + * RTL8369 - Similar enough to have the same datsheet as RTL8366 + * RTL8366RB - Probably reads out "RTL8366 revision B", has a quite + * different register layout from the other two + * RTL8366S - Is this "RTL8366 super"? + * RTL8367 - Has an OpenWRT driver as well + * RTL8368S - Seems to be an alternative name for RTL8366RB + * RTL8370 - Also uses SMI + * + * Copyright (C) 2017 Linus Walleij <linus.walleij@linaro.org> + * Copyright (C) 2010 Antti Seppälä <a.seppala@gmail.com> + * Copyright (C) 2010 Roman Yeryomin <roman@advem.lv> + * Copyright (C) 2011 Colin Leitner <colin.leitner@googlemail.com> + * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/spinlock.h> +#include <linux/skbuff.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_mdio.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/bitops.h> +#include <linux/if_bridge.h> + +#include "realtek.h" + +/* Read/write via mdiobus */ +#define MDC_MDIO_CTRL0_REG 31 +#define MDC_MDIO_START_REG 29 +#define MDC_MDIO_CTRL1_REG 21 +#define MDC_MDIO_ADDRESS_REG 23 +#define MDC_MDIO_DATA_WRITE_REG 24 +#define MDC_MDIO_DATA_READ_REG 25 + +#define MDC_MDIO_START_OP 0xFFFF +#define MDC_MDIO_ADDR_OP 0x000E +#define MDC_MDIO_READ_OP 0x0001 +#define MDC_MDIO_WRITE_OP 0x0003 +#define MDC_REALTEK_DEFAULT_PHY_ADDR 0x0 + +int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data) +{ + u32 phy_id = priv->phy_id; + struct mii_bus *bus = priv->bus; + + BUG_ON(in_interrupt()); + + mutex_lock(&bus->mdio_lock); + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write address control code to register 31 */ + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write address to register 23 */ + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write read control code to register 21 */ + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_READ_OP); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Read data from register 25 */ + *data = bus->read(bus, phy_id, MDC_MDIO_DATA_READ_REG); + + mutex_unlock(&bus->mdio_lock); + + return 0; +} + +static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data) +{ + u32 phy_id = priv->phy_id; + struct mii_bus *bus = priv->bus; + + BUG_ON(in_interrupt()); + + mutex_lock(&bus->mdio_lock); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write address control code to register 31 */ + bus->write(bus, phy_id, MDC_MDIO_CTRL0_REG, MDC_MDIO_ADDR_OP); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write address to register 23 */ + bus->write(bus, phy_id, MDC_MDIO_ADDRESS_REG, addr); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write data to register 24 */ + bus->write(bus, phy_id, MDC_MDIO_DATA_WRITE_REG, data); + + /* Write Start command to register 29 */ + bus->write(bus, phy_id, MDC_MDIO_START_REG, MDC_MDIO_START_OP); + + /* Write data control code to register 21 */ + bus->write(bus, phy_id, MDC_MDIO_CTRL1_REG, MDC_MDIO_WRITE_OP); + + mutex_unlock(&bus->mdio_lock); + return 0; +} + +/* Regmap accessors */ + +static int realtek_mdio_write(void *ctx, u32 reg, u32 val) +{ + struct realtek_priv *priv = ctx; + + return realtek_mdio_write_reg(priv, reg, val); +} + +static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) +{ + struct realtek_priv *priv = ctx; + + return realtek_mdio_read_reg(priv, reg, val); +} + +static const struct regmap_config realtek_mdio_regmap_config = { + .reg_bits = 10, /* A4..A0 R4..R0 */ + .val_bits = 16, + .reg_stride = 1, + /* PHY regs are at 0x8000 */ + .max_register = 0xffff, + .reg_format_endian = REGMAP_ENDIAN_BIG, + .reg_read = realtek_mdio_read, + .reg_write = realtek_mdio_write, + .cache_type = REGCACHE_NONE, +}; + +static int realtek_mdio_probe(struct mdio_device *mdiodev) +{ + struct realtek_priv *priv; + struct device *dev = &mdiodev->dev; + const struct realtek_variant *var; + int ret; + struct device_node *np; + + var = of_device_get_match_data(dev); + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->phy_id = mdiodev->addr; + + // Start by setting up the register mapping + priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config); + + priv->bus = mdiodev->bus; + priv->dev = &mdiodev->dev; + priv->chip_data = (void *)priv + sizeof(*priv); + + priv->clk_delay = var->clk_delay; + priv->cmd_read = var->cmd_read; + priv->cmd_write = var->cmd_write; + priv->ops = var->ops; + + if (IS_ERR(priv->map)) + dev_warn(dev, "regmap initialization failed"); + + priv->write_reg_noack = realtek_mdio_write_reg; + + np = dev->of_node; + + dev_set_drvdata(dev, priv); + spin_lock_init(&priv->lock); + + /* TODO: if power is software controlled, set up any regulators here */ + priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); + + ret = priv->ops->detect(priv); + if (ret) { + dev_err(dev, "unable to detect switch\n"); + return ret; + } + + priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); + if (!priv->ds) + return -ENOMEM; + + priv->ds->dev = dev; + priv->ds->num_ports = priv->num_ports; + priv->ds->priv = priv; + priv->ds->ops = var->ds_ops; + + ret = dsa_register_switch(priv->ds); + if (ret) { + dev_err(priv->dev, "unable to register switch ret = %d\n", ret); + return ret; + } + + return 0; +} + +static void realtek_mdio_remove(struct mdio_device *mdiodev) +{ + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); + + if (!priv) + return; + + dsa_unregister_switch(priv->ds); + //gpiod_set_value(priv->reset, 1); + dev_set_drvdata(&mdiodev->dev, NULL); +} + +static void realtek_mdio_shutdown(struct mdio_device *mdiodev) +{ + struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); + + if (!priv) + return; + + dsa_switch_shutdown(priv->ds); + + dev_set_drvdata(&mdiodev->dev, NULL); +} + +static const struct of_device_id realtek_mdio_of_match[] = { +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB) + { .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, }, +#endif + /* FIXME: add support for RTL8366S and more */ + { .compatible = "realtek,rtl8366s", .data = NULL, }, +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) + { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, +#endif + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, realtek_mdio_of_match); + +static struct mdio_driver realtek_mdio_driver = { + .mdiodrv.driver = { + .name = "realtek-mdio", + .of_match_table = of_match_ptr(realtek_mdio_of_match), + }, + .probe = realtek_mdio_probe, + .remove = realtek_mdio_remove, + .shutdown = realtek_mdio_shutdown, +}; + +mdio_module_driver(realtek_mdio_driver); + +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); +MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface"); +MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index a03de15c4a94..97274273cb3b 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -50,6 +50,8 @@ struct realtek_priv { struct gpio_desc *mdio; struct regmap *map; struct mii_bus *slave_mii_bus; + struct mii_bus *bus; + int phy_id; unsigned int clk_delay; u8 cmd_read;