| Message ID | 20231027190910.27044-4-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: support reset controller | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> - /* TODO: if power is software controlled, set up any regulators here */ Please don't remove these comments. The switch might be powered using a regulator. And this does look like the correct place to get the regulator, and turn it on before doing the reset. Somebody thought such boards might exist... > - /* TODO: if power is software controlled, set up any regulators here */ etc. Andrew --- pw-bot: cr
On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote: > The 'reset-gpios' will not work when the switch reset is controlled by a > reset controller. > > Although the reset is optional and the driver performs a soft reset > during setup, if the initial reset state was asserted, the driver will > not detect it. > > The reset controller will take precedence over the reset GPIO. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++---- > drivers/net/dsa/realtek/realtek-smi.c | 49 ++++++++++++++++++++++--- > drivers/net/dsa/realtek/realtek.h | 2 + > 3 files changed, 89 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 292e6d087e8b..aad94e49d4c9 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = { > .disable_locking = true, > }; > > +static void realtek_mdio_reset_assert(struct realtek_priv *priv) > +{ > + int ret; > + > + if (priv->reset_ctl) { > + ret = reset_control_assert(priv->reset_ctl); > + if (ret) > + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", > + ret); Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret) which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y) rather than just a numeric value. Also, I don't know if this is explicit in the coding style, but I believe it is more consistent if single function calls are enveloped in curly braces if they span multiple lines, like so: if (ret) { dev_warn(priv->dev, "Failed to assert the switch reset control: %pe", ERR_PTR(ret)); } Also, please note that netdev still prefers the 80 character line limit. > + > + return; > + } > + > + if (priv->reset) > + gpiod_set_value(priv->reset, true); > +} > + > +static void realtek_mdio_reset_deassert(struct realtek_priv *priv) > +{ > + int ret; > + > + if (priv->reset_ctl) { > + ret = reset_control_deassert(priv->reset_ctl); > + if (ret) > + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", > + ret); > + > + return; Is there a particular reason why this has to ignore a reset GPIO if present, rather than fall through, checking for the latter as well? > + } > + > + if (priv->reset) > + gpiod_set_value(priv->reset, false); > +} > + > static int realtek_mdio_probe(struct mdio_device *mdiodev) > { > struct realtek_priv *priv; > @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) > > dev_set_drvdata(dev, priv); > > - /* TODO: if power is software controlled, set up any regulators here */ As Andrew mentions, this commit does not make power software-controlled, so don't remove this. > priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); > > + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); > + if (IS_ERR(priv->reset_ctl)) { > + ret = PTR_ERR(priv->reset_ctl); > + return dev_err_probe(dev, ret, "failed to get reset control\n"); > + } > + > priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(priv->reset)) { > dev_err(dev, "failed to get RESET GPIO\n"); > return PTR_ERR(priv->reset); > } > - > - if (priv->reset) { > - gpiod_set_value(priv->reset, 1); > + if (priv->reset_ctl || priv->reset) { > + realtek_mdio_reset_assert(priv); > dev_dbg(dev, "asserted RESET\n"); > msleep(REALTEK_HW_STOP_DELAY); > - gpiod_set_value(priv->reset, 0); > + realtek_mdio_reset_deassert(priv); > msleep(REALTEK_HW_START_DELAY); > dev_dbg(dev, "deasserted RESET\n"); > } > @@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev) > dsa_unregister_switch(priv->ds); > > /* leave the device reset asserted */ > - if (priv->reset) > - gpiod_set_value(priv->reset, 1); > + realtek_mdio_reset_assert(priv); > } > > static void realtek_mdio_shutdown(struct mdio_device *mdiodev) > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index bfd11591faf4..a99e53b5b662 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > return ret; > } > > +static void realtek_smi_reset_assert(struct realtek_priv *priv) > +{ > + int ret; > + > + if (priv->reset_ctl) { > + ret = reset_control_assert(priv->reset_ctl); > + if (ret) > + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", > + ret); > + > + return; > + } > + > + if (priv->reset) > + gpiod_set_value(priv->reset, true); > +} > + > +static void realtek_smi_reset_deassert(struct realtek_priv *priv) > +{ > + int ret; > + > + if (priv->reset_ctl) { > + ret = reset_control_deassert(priv->reset_ctl); > + if (ret) > + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", > + ret); > + > + return; > + } > + > + if (priv->reset) > + gpiod_set_value(priv->reset, false); > +} > + To respond here, in a single email, to your earlier question (sorry): https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/ | Both interface modules, realtek-smi and realtek-mdio, do not share | code, except for the realtek.h header file. I don't know if it is | worth it to put the code in a new shared module. What is the best | practice here? Create a realtek_common.c linked to both modules? The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c, and the probe, remove and shutdown functions are surprisingly similar already, and perhaps might become even more similar in the future. I think it is worth introducing a common kernel module for both interface drivers as a preliminary patch, rather than keeping duplicated probe/remove/shutdown code. > static int realtek_smi_probe(struct platform_device *pdev) > { > const struct realtek_variant *var; > @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev) > dev_set_drvdata(dev, priv); > spin_lock_init(&priv->lock); > > - /* TODO: if power is software controlled, set up any regulators here */ > + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); > + if (IS_ERR(priv->reset_ctl)) { > + ret = PTR_ERR(priv->reset_ctl); > + return dev_err_probe(dev, ret, "failed to get reset control\n"); > + } > > priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(priv->reset)) { > dev_err(dev, "failed to get RESET GPIO\n"); > return PTR_ERR(priv->reset); > } > - if (priv->reset) { > - gpiod_set_value(priv->reset, 1); > + if (priv->reset_ctl || priv->reset) { > + realtek_smi_reset_assert(priv); > dev_dbg(dev, "asserted RESET\n"); > msleep(REALTEK_HW_STOP_DELAY); > - gpiod_set_value(priv->reset, 0); > + realtek_smi_reset_deassert(priv); > msleep(REALTEK_HW_START_DELAY); > dev_dbg(dev, "deasserted RESET\n"); > } > @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev) > of_node_put(priv->slave_mii_bus->dev.of_node); slave_mii_bus was renamed to user_mii_bus, and this prevents the application of the patch currently, so you will need to respin. But I think net-next is going to close soon for 2 weeks, so either you respin as RFC or you wait until it reopens. > > /* leave the device reset asserted */ > - if (priv->reset) > - gpiod_set_value(priv->reset, 1); > + realtek_smi_reset_assert(priv); > } >
> On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote: > > The 'reset-gpios' will not work when the switch reset is controlled by a > > reset controller. > > > > Although the reset is optional and the driver performs a soft reset > > during setup, if the initial reset state was asserted, the driver will > > not detect it. > > > > The reset controller will take precedence over the reset GPIO. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++---- > > drivers/net/dsa/realtek/realtek-smi.c | 49 ++++++++++++++++++++++--- > > drivers/net/dsa/realtek/realtek.h | 2 + > > 3 files changed, 89 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > > index 292e6d087e8b..aad94e49d4c9 100644 > > --- a/drivers/net/dsa/realtek/realtek-mdio.c > > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > > @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = { > > .disable_locking = true, > > }; > > > > +static void realtek_mdio_reset_assert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + if (priv->reset_ctl) { > > + ret = reset_control_assert(priv->reset_ctl); > > + if (ret) > > + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", > > + ret); > > Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret) > which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y) > rather than just a numeric value. > > Also, I don't know if this is explicit in the coding style, but I > believe it is more consistent if single function calls are enveloped in > curly braces if they span multiple lines, like so: > > if (ret) { > dev_warn(priv->dev, > "Failed to assert the switch reset control: %pe", > ERR_PTR(ret)); > } > > Also, please note that netdev still prefers the 80 character line limit. Sure. > > + > > + return; > > + } > > + > > + if (priv->reset) > > + gpiod_set_value(priv->reset, true); > > +} > > + > > +static void realtek_mdio_reset_deassert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + if (priv->reset_ctl) { > > + ret = reset_control_deassert(priv->reset_ctl); > > + if (ret) > > + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", > > + ret); > > + > > + return; > > Is there a particular reason why this has to ignore a reset GPIO if > present, rather than fall through, checking for the latter as well? Something like this, disregard white space issues? static void realtek_smi_reset_assert(struct realtek_priv *priv) { int ret; if (priv->reset_ctl) { ret = reset_control_assert(priv->reset_ctl); if (!ret) return; dev_warn(priv->dev, "Failed to assert the switch reset control: %pe\n", ERR_PTR(ret)); } if (priv->reset) gpiod_set_value(priv->reset, true); } > > + } > > + > > + if (priv->reset) > > + gpiod_set_value(priv->reset, false); > > +} > > + > > static int realtek_mdio_probe(struct mdio_device *mdiodev) > > { > > struct realtek_priv *priv; > > @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) > > > > dev_set_drvdata(dev, priv); > > > > - /* TODO: if power is software controlled, set up any regulators here */ > > As Andrew mentions, this commit does not make power software-controlled, > so don't remove this. I'll return it and move specific this TODO after the leds_disabled as it should be before the reset. The one in realtek-smi was in the right position. > > priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); > > > > + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); > > + if (IS_ERR(priv->reset_ctl)) { > > + ret = PTR_ERR(priv->reset_ctl); > > + return dev_err_probe(dev, ret, "failed to get reset control\n"); > > + } > > + > > priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > if (IS_ERR(priv->reset)) { > > dev_err(dev, "failed to get RESET GPIO\n"); > > return PTR_ERR(priv->reset); > > } > > - > > - if (priv->reset) { > > - gpiod_set_value(priv->reset, 1); > > + if (priv->reset_ctl || priv->reset) { > > + realtek_mdio_reset_assert(priv); > > dev_dbg(dev, "asserted RESET\n"); > > msleep(REALTEK_HW_STOP_DELAY); > > - gpiod_set_value(priv->reset, 0); > > + realtek_mdio_reset_deassert(priv); > > msleep(REALTEK_HW_START_DELAY); > > dev_dbg(dev, "deasserted RESET\n"); > > } > > @@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev) > > dsa_unregister_switch(priv->ds); > > > > /* leave the device reset asserted */ > > - if (priv->reset) > > - gpiod_set_value(priv->reset, 1); > > + realtek_mdio_reset_assert(priv); > > } > > > > static void realtek_mdio_shutdown(struct mdio_device *mdiodev) > > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > > index bfd11591faf4..a99e53b5b662 100644 > > --- a/drivers/net/dsa/realtek/realtek-smi.c > > +++ b/drivers/net/dsa/realtek/realtek-smi.c > > @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > > return ret; > > } > > > > +static void realtek_smi_reset_assert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + if (priv->reset_ctl) { > > + ret = reset_control_assert(priv->reset_ctl); > > + if (ret) > > + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", > > + ret); > > + > > + return; > > + } > > + > > + if (priv->reset) > > + gpiod_set_value(priv->reset, true); > > +} > > + > > +static void realtek_smi_reset_deassert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + if (priv->reset_ctl) { > > + ret = reset_control_deassert(priv->reset_ctl); > > + if (ret) > > + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", > > + ret); > > + > > + return; > > + } > > + > > + if (priv->reset) > > + gpiod_set_value(priv->reset, false); > > +} > > + > > To respond here, in a single email, to your earlier question (sorry): > https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/ > > | Both interface modules, realtek-smi and realtek-mdio, do not share > | code, except for the realtek.h header file. I don't know if it is > | worth it to put the code in a new shared module. What is the best > | practice here? Create a realtek_common.c linked to both modules? > > The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c, > and the probe, remove and shutdown functions are surprisingly similar > already, and perhaps might become even more similar in the future. > I think it is worth introducing a common kernel module for both > interface drivers as a preliminary patch, rather than keeping duplicated > probe/remove/shutdown code. The remove/shutdown are probably similar to any other DSA driver. I think the extra code around a shared code in a new module would be bigger than the duplicated code. realtek-mdio is an MDIO driver while realtek-smi is a platform driver implementing a bitbang protocol. They might never be used together in a system to share RAM and not even installed together in small systems. If I do need to share the code, I would just link it twice. Would something like this be acceptable? drivers/net/dsa/realtek/Makefile -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o -obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o realtek_common.o +obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o realtek_common.o drivers/net/dsa/realtek/realtek.h: +void realtek_reset_assert(struct realtek_priv *priv); +void realtek_reset_deassert(struct realtek_priv *priv); realtek_common: +void realtek_reset_assert(struct realtek_priv *priv) {} +void realtek_reset_deassert(struct realtek_priv *priv) {} If that realtek_common grows, we could convert it into a module. For now, it would just introduce extra complexity. > > static int realtek_smi_probe(struct platform_device *pdev) > > { > > const struct realtek_variant *var; > > @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev) > > dev_set_drvdata(dev, priv); > > spin_lock_init(&priv->lock); > > > > - /* TODO: if power is software controlled, set up any regulators here */ > > + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); > > + if (IS_ERR(priv->reset_ctl)) { > > + ret = PTR_ERR(priv->reset_ctl); > > + return dev_err_probe(dev, ret, "failed to get reset control\n"); > > + } > > > > priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > if (IS_ERR(priv->reset)) { > > dev_err(dev, "failed to get RESET GPIO\n"); > > return PTR_ERR(priv->reset); > > } > > - if (priv->reset) { > > - gpiod_set_value(priv->reset, 1); > > + if (priv->reset_ctl || priv->reset) { > > + realtek_smi_reset_assert(priv); > > dev_dbg(dev, "asserted RESET\n"); > > msleep(REALTEK_HW_STOP_DELAY); > > - gpiod_set_value(priv->reset, 0); > > + realtek_smi_reset_deassert(priv); > > msleep(REALTEK_HW_START_DELAY); > > dev_dbg(dev, "deasserted RESET\n"); > > } > > @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev) > > of_node_put(priv->slave_mii_bus->dev.of_node); > > slave_mii_bus was renamed to user_mii_bus, and this prevents the > application of the patch currently, so you will need to respin. But I > think net-next is going to close soon for 2 weeks, so either you respin > as RFC or you wait until it reopens. I'll wait. I hope the comments on this thread might be enough to get this patch sorted out. > > > > > /* leave the device reset asserted */ > > - if (priv->reset) > > - gpiod_set_value(priv->reset, 1); > > + realtek_smi_reset_assert(priv); > > } > > Regards, Luiz
Hi Vladimir, > realtek-mdio is an MDIO driver while realtek-smi is a platform driver > implementing a bitbang protocol. They might never be used together in > a system to share RAM and not even installed together in small > systems. If I do need to share the code, I would just link it twice. > Would something like this be acceptable? > > drivers/net/dsa/realtek/Makefile > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o > -obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o realtek_common.o > +obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o realtek_common.o Just a follow up. It is not that simple to include a .c file into an existing single file module. It looks like you need to rename the original file as all linked objects must not conflict with the module name. The kernel build seems to create a new object file for each module. Is there a clearer way? I think #include a common .c file would not be acceptable. I tested your shared module suggestion. It is the clearest one but it also increased the overall size quite a bit. Even linking two objects seems to use the double of space used by the functions alone. These are some values (mips) drivers/net/dsa/realtek/realtek_common.o 5744 without exports drivers/net/dsa/realtek/realtek_common.o 33472 exporting the two reset functions (assert/deassert) drivers/net/dsa/realtek/realtek-mdio.o 18756 without the reset funcs (to be used as a module) drivers/net/dsa/realtek/realtek-mdio.o 20480 including the realtek_common.c (#include <realtek_common.c>) drivers/net/dsa/realtek/realtek-mdio.o 22696 linking the realtek_common.o drivers/net/dsa/realtek/realtek-smi.o 30712 without the reset funcs (to be used as a module) drivers/net/dsa/realtek/realtek-smi.o 34604 linking the realtek_common.o drivers/net/dsa/realtek/realtek-mdio.ko 28800 without the reset funcs (it will use realtek_common.ko) drivers/net/dsa/realtek/realtek-mdio.ko 32736 linking the realtek_common.o drivers/net/dsa/realtek/realtek-smi.ko 40708 without the reset funcs (it will use realtek_common.ko) drivers/net/dsa/realtek/realtek-smi.ko 44612 linking the realtek_common.o In summary, we get about 1.5kb of code with the extra functions, almost 4kb if we link a common object containing the functions and 33kb if we use a module for those two functions. I can go with any option. I just need to know which one would be accepted to update my patches. 1) keep duplicated functions on each file 2) share the code including the .c on both 3) share the code linking a common object in both modules (and renaming existing .c files) 4) create a new module used by both modules. The devices that would use this driver have very restricted storage space. Every kbyte counts. Regards, Luiz
On Wed, Nov 01, 2023 at 04:55:19PM -0300, Luiz Angelo Daros de Luca wrote: > Hi Vladimir, > > > realtek-mdio is an MDIO driver while realtek-smi is a platform driver > > implementing a bitbang protocol. They might never be used together in > > a system to share RAM and not even installed together in small > > systems. If I do need to share the code, I would just link it twice. > > Would something like this be acceptable? > > > > drivers/net/dsa/realtek/Makefile > > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o > > -obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o > > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o realtek_common.o > > +obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o realtek_common.o You cannot link realtek_common.o into multiple .ko files. It generates build warnings and it is being phased out. https://patchwork.kernel.org/project/netdevbpf/cover/20221119225650.1044591-1-alobakin@pm.me/ > Just a follow up. > > It is not that simple to include a .c file into an existing single > file module. It looks like you need to rename the original file as all > linked objects must not conflict with the module name. The kernel > build seems to create a new object file for each module. Is there a > clearer way? I think #include a common .c file would not be > acceptable. > > I tested your shared module suggestion. It is the clearest one but it > also increased the overall size quite a bit. Even linking two objects > seems to use the double of space used by the functions alone. These > are some values (mips) > > drivers/net/dsa/realtek/realtek_common.o 5744 without exports > drivers/net/dsa/realtek/realtek_common.o 33472 exporting the two reset functions (assert/deassert) > > drivers/net/dsa/realtek/realtek-mdio.o 18756 without the reset funcs (to be used as a module) > drivers/net/dsa/realtek/realtek-mdio.o 20480 including the realtek_common.c (#include <realtek_common.c>) > drivers/net/dsa/realtek/realtek-mdio.o 22696 linking the realtek_common.o > > drivers/net/dsa/realtek/realtek-smi.o 30712 without the reset funcs (to be used as a module) > drivers/net/dsa/realtek/realtek-smi.o 34604 linking the realtek_common.o > > drivers/net/dsa/realtek/realtek-mdio.ko 28800 without the reset funcs (it will use realtek_common.ko) > drivers/net/dsa/realtek/realtek-mdio.ko 32736 linking the realtek_common.o > > drivers/net/dsa/realtek/realtek-smi.ko 40708 without the reset funcs (it will use realtek_common.ko) > drivers/net/dsa/realtek/realtek-smi.ko 44612 linking the realtek_common.o > > In summary, we get about 1.5kb of code with the extra functions, > almost 4kb if we link a common object containing the functions and > 33kb if we use a module for those two functions. > > I can go with any option. I just need to know which one would be > accepted to update my patches. > 1) keep duplicated functions on each file Disadvantage: need to update the same functions twice, it becomes possible for them to diverge, increases maintenance difficulty. > 2) share the code including the .c on both Including a .c file with a preprocessor #include is fragile, has to be placed very carefully, etc. So it is also a time bomb from a maintenance PoV. Maybe a header with static inline function definitions would be worth considering, although I don't think it's common practice to do this. > 3) share the code linking a common object in both modules (and > renaming existing .c files) Sharing object files is being phased out. > 4) create a new module used by both modules. I am suspicious of the numbers you provided above. What needs to be compared is the reduction in size of realtek_mdio.ko and realtek_smi.ko, compared to the size of the new realtek_common.ko. Also, this starts being more and more worthwhile as more code goes into realtek_common.ko, and I also mentioned a common probe/remove/shutdown as being viable candidates. Looking at your figures, I'm not sure at which ones I need to look in order to compute that metric. > The devices that would use this driver have very restricted storage > space. Every kbyte counts. Well, in that case you could compile the code as built into the kernel, and the module overhead would go away.
On Mon, Oct 30, 2023 at 09:30:45PM -0300, Luiz Angelo Daros de Luca wrote: > The remove/shutdown are probably similar to any other DSA driver. I > think the extra code around a shared code in a new module would be > bigger than the duplicated code. When you start looking at the duplicated parsing of vendor-specific OF properties like "realtek,disable-leds", I am starting to question that. There are also differences in the handling of the user_mii_bus, which stem from the duplicate implementations, which are hard to justify if you are saying that the only difference is that the switch is controlled through a different interface and it is otherwise the same.
On Wed, Nov 1, 2023 at 8:55 PM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > > drivers/net/dsa/realtek/Makefile > > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o > > -obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o > > +obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o realtek_common.o > > +obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o realtek_common.o > > Just a follow up. > > It is not that simple to include a .c file into an existing single > file module. It looks like you need to rename the original file as all > linked objects must not conflict with the module name. The kernel > build seems to create a new object file for each module. Is there a > clearer way? I think #include a common .c file would not be > acceptable. I don't know if this is an answer to your question, but look at what I did in drivers/usb/fotg210/Makefile: # This setup links the different object files into one single # module so we don't have to EXPORT() a lot of internal symbols # or create unnecessary submodules. fotg210-objs-y += fotg210-core.o fotg210-objs-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o fotg210-objs-$(CONFIG_USB_FOTG210_UDC) += fotg210-udc.o fotg210-objs := $(fotg210-objs-y) obj-$(CONFIG_USB_FOTG210) += fotg210.o Everything starting with CONFIG_* is a Kconfig option obviously. The final module is just one file named fotg210.ko no matter whether HCD (host controller), UDC (device controller) or both parts were compiled into it. Often you just need one of them, sometimes you may need both. It's a pretty clean example of how you do this "one module from several optional parts" using Kbuild. It's not super-intuitive, copy/paste/modify is a viable way to get to this. Yours, Linus Walleij
On Thu, Nov 02, 2023 at 03:59:48PM +0100, Linus Walleij wrote: > I don't know if this is an answer to your question, but look at what I did in > > drivers/usb/fotg210/Makefile: > > # This setup links the different object files into one single > # module so we don't have to EXPORT() a lot of internal symbols > # or create unnecessary submodules. > fotg210-objs-y += fotg210-core.o > fotg210-objs-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o > fotg210-objs-$(CONFIG_USB_FOTG210_UDC) += fotg210-udc.o > fotg210-objs := $(fotg210-objs-y) > obj-$(CONFIG_USB_FOTG210) += fotg210.o > > Everything starting with CONFIG_* is a Kconfig option obviously. > > The final module is just one file named fotg210.ko no matter whether > HCD (host controller), UDC (device controller) or both parts were > compiled into it. Often you just need one of them, sometimes you may > need both. > > It's a pretty clean example of how you do this "one module from > several optional parts" using Kbuild. To be clear, something like this is what you mean, right? diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig index 060165a85fb7..857a039fb0f1 100644 --- a/drivers/net/dsa/realtek/Kconfig +++ b/drivers/net/dsa/realtek/Kconfig @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK if NET_DSA_REALTEK +config NET_DSA_REALTEK_INTERFACE + tristate + help + Common interface driver for accessing Realtek switches, either + through MDIO or SMI. + config NET_DSA_REALTEK_MDIO - tristate "Realtek MDIO interface driver" - depends on OF - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB + tristate "Realtek MDIO interface support" help Select to enable support for registering switches configured through MDIO. config NET_DSA_REALTEK_SMI - tristate "Realtek SMI interface driver" - depends on OF - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB + bool "Realtek SMI interface support" help Select to enable support for registering switches connected through SMI. config NET_DSA_REALTEK_RTL8365MB tristate "Realtek RTL8365MB switch subdriver" - imply NET_DSA_REALTEK_SMI - imply NET_DSA_REALTEK_MDIO + select NET_DSA_REALTEK_INTERFACE select NET_DSA_TAG_RTL8_4 + depends on OF help Select to enable support for Realtek RTL8365MB-VC and RTL8367S. config NET_DSA_REALTEK_RTL8366RB tristate "Realtek RTL8366RB switch subdriver" - imply NET_DSA_REALTEK_SMI - imply NET_DSA_REALTEK_MDIO + select NET_DSA_REALTEK_INTERFACE select NET_DSA_TAG_RTL4_A + depends on OF help Select to enable support for Realtek RTL8366RB. diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile index 0aab57252a7c..35b7734c0ad0 100644 --- a/drivers/net/dsa/realtek/Makefile +++ b/drivers/net/dsa/realtek/Makefile @@ -1,6 +1,15 @@ # 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_INTERFACE) := realtek-interface.o + +realtek-interface-objs := realtek-interface-common.o +ifdef CONFIG_NET_DSA_REALTEK_MDIO +realtek-interface-objs += realtek-mdio.o +endif +ifdef CONFIG_NET_DSA_REALTEK_SMI +realtek-interface-objs += realtek-smi.o +endif + obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o rtl8366-objs := rtl8366-core.o rtl8366rb.o obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o diff --git a/drivers/net/dsa/realtek/realtek-interface-common.c b/drivers/net/dsa/realtek/realtek-interface-common.c new file mode 100644 index 000000000000..bb7c77cdb9e2 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-interface-common.c @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <linux/module.h> + +#include "realtek-mdio.h" +#include "realtek-smi.h" + +static int __init realtek_interface_init(void) +{ + int err; + + err = realtek_mdio_init(); + if (err) + return err; + + err = realtek_smi_init(); + if (err) { + realtek_smi_exit(); + return err; + } + + return 0; +} +module_init(realtek_interface_init); + +static void __exit realtek_interface_exit(void) +{ + realtek_smi_exit(); + realtek_mdio_exit(); +} +module_exit(realtek_interface_exit); + +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); +MODULE_DESCRIPTION("Driver for interfacing with Realtek switches via MDIO or SMI"); +MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 292e6d087e8b..6997dec14de2 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0+ -/* Realtek MDIO interface driver +/* Realtek MDIO interface support * * ASICs we intend to support with this driver: * @@ -19,12 +19,12 @@ * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org> */ -#include <linux/module.h> #include <linux/of.h> #include <linux/overflow.h> #include <linux/regmap.h> #include "realtek.h" +#include "realtek-mdio.h" /* Read/write via mdiobus */ #define REALTEK_MDIO_CTRL0_REG 31 @@ -283,8 +283,12 @@ static struct mdio_driver realtek_mdio_driver = { .shutdown = realtek_mdio_shutdown, }; -mdio_module_driver(realtek_mdio_driver); +int realtek_mdio_init(void) +{ + return mdio_driver_register(&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"); +void realtek_mdio_exit(void) +{ + mdio_driver_unregister(&realtek_mdio_driver); +} diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h new file mode 100644 index 000000000000..941b4ef9d531 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-mdio.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ + +#ifndef _REALTEK_MDIO_H +#define _REALTEK_MDIO_H + +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) + +int realtek_mdio_init(void); +void realtek_mdio_exit(void); + +#else + +static inline int realtek_mdio_init(void) +{ + return 0; +} + +static inline void realtek_mdio_exit(void) +{ +} + +#endif + +#endif diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 755546ed8db6..4c282bfc884d 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0+ -/* Realtek Simple Management Interface (SMI) driver +/* Realtek Simple Management Interface (SMI) interface * It can be discussed how "simple" this interface is. * * The SMI protocol piggy-backs the MDIO MDC and MDIO signals levels @@ -26,7 +26,6 @@ */ #include <linux/kernel.h> -#include <linux/module.h> #include <linux/device.h> #include <linux/spinlock.h> #include <linux/skbuff.h> @@ -40,6 +39,7 @@ #include <linux/if_bridge.h> #include "realtek.h" +#include "realtek-smi.h" #define REALTEK_SMI_ACK_RETRY_COUNT 5 @@ -560,8 +560,13 @@ static struct platform_driver realtek_smi_driver = { .remove_new = realtek_smi_remove, .shutdown = realtek_smi_shutdown, }; -module_platform_driver(realtek_smi_driver); -MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface"); -MODULE_LICENSE("GPL"); +int realtek_smi_init(void) +{ + return platform_driver_register(&realtek_smi_driver); +} + +void realtek_smi_exit(void) +{ + platform_driver_unregister(&realtek_smi_driver); +} diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h new file mode 100644 index 000000000000..9a4838321f94 --- /dev/null +++ b/drivers/net/dsa/realtek/realtek-smi.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ + +#ifndef _REALTEK_SMI_H +#define _REALTEK_SMI_H + +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) + +int realtek_smi_init(void); +void realtek_smi_exit(void); + +#else + +static inline int realtek_smi_init(void) +{ + return 0; +} + +static inline void realtek_smi_exit(void) +{ +} + +#endif + +#endif
On Thu, Nov 02, 2023 at 05:55:21PM +0200, Vladimir Oltean wrote: > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig > index 060165a85fb7..857a039fb0f1 100644 > --- a/drivers/net/dsa/realtek/Kconfig > +++ b/drivers/net/dsa/realtek/Kconfig > @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK > > if NET_DSA_REALTEK > > +config NET_DSA_REALTEK_INTERFACE > + tristate > + help > + Common interface driver for accessing Realtek switches, either > + through MDIO or SMI. > + > config NET_DSA_REALTEK_MDIO > - tristate "Realtek MDIO interface driver" > - depends on OF > - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB > - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB > - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB > + tristate "Realtek MDIO interface support" I meant to also make "config NET_DSA_REALTEK_MDIO" a bool and not tristate. > help > Select to enable support for registering switches configured > through MDIO. > > config NET_DSA_REALTEK_SMI > - tristate "Realtek SMI interface driver" > - depends on OF > - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB > - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB > - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB > + bool "Realtek SMI interface support" > help > Select to enable support for registering switches connected > through SMI. > > config NET_DSA_REALTEK_RTL8365MB > tristate "Realtek RTL8365MB switch subdriver" > - imply NET_DSA_REALTEK_SMI > - imply NET_DSA_REALTEK_MDIO > + select NET_DSA_REALTEK_INTERFACE > select NET_DSA_TAG_RTL8_4 > + depends on OF > help > Select to enable support for Realtek RTL8365MB-VC and RTL8367S. > > config NET_DSA_REALTEK_RTL8366RB > tristate "Realtek RTL8366RB switch subdriver" > - imply NET_DSA_REALTEK_SMI > - imply NET_DSA_REALTEK_MDIO > + select NET_DSA_REALTEK_INTERFACE > select NET_DSA_TAG_RTL4_A > + depends on OF > help > Select to enable support for Realtek RTL8366RB.
On Thu, Nov 02, 2023 at 05:55:21PM +0200, Vladimir Oltean wrote: > +static int __init realtek_interface_init(void) > +{ > + int err; > + > + err = realtek_mdio_init(); > + if (err) > + return err; > + > + err = realtek_smi_init(); > + if (err) { > + realtek_smi_exit(); One more correction, this was supposed to be realtek_mdio_exit(). > + return err; > + } > + > + return 0; > +} > +module_init(realtek_interface_init);
On Thu, Nov 2, 2023 at 4:55 PM Vladimir Oltean <olteanv@gmail.com> wrote: > To be clear, something like this is what you mean, right? Hey, it's 90% done already! :D And we do away with all the hopeless IMPLY stuff. > +realtek-interface-objs := realtek-interface-common.o > +ifdef CONFIG_NET_DSA_REALTEK_MDIO > +realtek-interface-objs += realtek-mdio.o > +endif > +ifdef CONFIG_NET_DSA_REALTEK_SMI > +realtek-interface-objs += realtek-smi.o > +endif I would try to do realtek-interface-objs-y := realtek-interface-common.o realtek-interface-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o realtek-interface-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o realtek-interface-objs := $(realtek-interface-objs-y) I think it's equivalent just more compact? Other than that it looks like what I would have done myself. Yours, Linus Walleij
> On Thu, Nov 02, 2023 at 03:59:48PM +0100, Linus Walleij wrote: > > I don't know if this is an answer to your question, but look at what I did in > > > > drivers/usb/fotg210/Makefile: > > > > # This setup links the different object files into one single > > # module so we don't have to EXPORT() a lot of internal symbols > > # or create unnecessary submodules. > > fotg210-objs-y += fotg210-core.o > > fotg210-objs-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o > > fotg210-objs-$(CONFIG_USB_FOTG210_UDC) += fotg210-udc.o > > fotg210-objs := $(fotg210-objs-y) > > obj-$(CONFIG_USB_FOTG210) += fotg210.o > > > > Everything starting with CONFIG_* is a Kconfig option obviously. > > > > The final module is just one file named fotg210.ko no matter whether > > HCD (host controller), UDC (device controller) or both parts were > > compiled into it. Often you just need one of them, sometimes you may > > need both. > > > > It's a pretty clean example of how you do this "one module from > > several optional parts" using Kbuild. > > To be clear, something like this is what you mean, right? > > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig > index 060165a85fb7..857a039fb0f1 100644 > --- a/drivers/net/dsa/realtek/Kconfig > +++ b/drivers/net/dsa/realtek/Kconfig > @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK > > if NET_DSA_REALTEK > > +config NET_DSA_REALTEK_INTERFACE > + tristate > + help > + Common interface driver for accessing Realtek switches, either > + through MDIO or SMI. > + > config NET_DSA_REALTEK_MDIO > - tristate "Realtek MDIO interface driver" > - depends on OF > - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB > - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB > - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB > + tristate "Realtek MDIO interface support" > help > Select to enable support for registering switches configured > through MDIO. > > config NET_DSA_REALTEK_SMI > - tristate "Realtek SMI interface driver" > - depends on OF > - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB > - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB > - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB > + bool "Realtek SMI interface support" > help > Select to enable support for registering switches connected > through SMI. > > config NET_DSA_REALTEK_RTL8365MB > tristate "Realtek RTL8365MB switch subdriver" > - imply NET_DSA_REALTEK_SMI > - imply NET_DSA_REALTEK_MDIO > + select NET_DSA_REALTEK_INTERFACE > select NET_DSA_TAG_RTL8_4 > + depends on OF > help > Select to enable support for Realtek RTL8365MB-VC and RTL8367S. > > config NET_DSA_REALTEK_RTL8366RB > tristate "Realtek RTL8366RB switch subdriver" > - imply NET_DSA_REALTEK_SMI > - imply NET_DSA_REALTEK_MDIO > + select NET_DSA_REALTEK_INTERFACE > select NET_DSA_TAG_RTL4_A > + depends on OF > help > Select to enable support for Realtek RTL8366RB. > > diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile > index 0aab57252a7c..35b7734c0ad0 100644 > --- a/drivers/net/dsa/realtek/Makefile > +++ b/drivers/net/dsa/realtek/Makefile > @@ -1,6 +1,15 @@ > # 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_INTERFACE) := realtek-interface.o > + > +realtek-interface-objs := realtek-interface-common.o > +ifdef CONFIG_NET_DSA_REALTEK_MDIO > +realtek-interface-objs += realtek-mdio.o > +endif > +ifdef CONFIG_NET_DSA_REALTEK_SMI > +realtek-interface-objs += realtek-smi.o > +endif > + > obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o > rtl8366-objs := rtl8366-core.o rtl8366rb.o > obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o > diff --git a/drivers/net/dsa/realtek/realtek-interface-common.c b/drivers/net/dsa/realtek/realtek-interface-common.c > new file mode 100644 > index 000000000000..bb7c77cdb9e2 > --- /dev/null > +++ b/drivers/net/dsa/realtek/realtek-interface-common.c > @@ -0,0 +1,36 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/module.h> > + > +#include "realtek-mdio.h" > +#include "realtek-smi.h" > + > +static int __init realtek_interface_init(void) > +{ > + int err; > + > + err = realtek_mdio_init(); > + if (err) > + return err; > + > + err = realtek_smi_init(); > + if (err) { > + realtek_smi_exit(); > + return err; > + } > + > + return 0; > +} > +module_init(realtek_interface_init); > + > +static void __exit realtek_interface_exit(void) > +{ > + realtek_smi_exit(); > + realtek_mdio_exit(); > +} > +module_exit(realtek_interface_exit); > + > +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); > +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); > +MODULE_DESCRIPTION("Driver for interfacing with Realtek switches via MDIO or SMI"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 292e6d087e8b..6997dec14de2 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0+ > -/* Realtek MDIO interface driver > +/* Realtek MDIO interface support > * > * ASICs we intend to support with this driver: > * > @@ -19,12 +19,12 @@ > * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org> > */ > > -#include <linux/module.h> > #include <linux/of.h> > #include <linux/overflow.h> > #include <linux/regmap.h> > > #include "realtek.h" > +#include "realtek-mdio.h" > > /* Read/write via mdiobus */ > #define REALTEK_MDIO_CTRL0_REG 31 > @@ -283,8 +283,12 @@ static struct mdio_driver realtek_mdio_driver = { > .shutdown = realtek_mdio_shutdown, > }; > > -mdio_module_driver(realtek_mdio_driver); > +int realtek_mdio_init(void) > +{ > + return mdio_driver_register(&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"); > +void realtek_mdio_exit(void) > +{ > + mdio_driver_unregister(&realtek_mdio_driver); > +} > diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h > new file mode 100644 > index 000000000000..941b4ef9d531 > --- /dev/null > +++ b/drivers/net/dsa/realtek/realtek-mdio.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > + > +#ifndef _REALTEK_MDIO_H > +#define _REALTEK_MDIO_H > + > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) > + > +int realtek_mdio_init(void); > +void realtek_mdio_exit(void); > + > +#else > + > +static inline int realtek_mdio_init(void) > +{ > + return 0; > +} > + > +static inline void realtek_mdio_exit(void) > +{ > +} > + > +#endif > + > +#endif > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 755546ed8db6..4c282bfc884d 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0+ > -/* Realtek Simple Management Interface (SMI) driver > +/* Realtek Simple Management Interface (SMI) interface > * It can be discussed how "simple" this interface is. > * > * The SMI protocol piggy-backs the MDIO MDC and MDIO signals levels > @@ -26,7 +26,6 @@ > */ > > #include <linux/kernel.h> > -#include <linux/module.h> > #include <linux/device.h> > #include <linux/spinlock.h> > #include <linux/skbuff.h> > @@ -40,6 +39,7 @@ > #include <linux/if_bridge.h> > > #include "realtek.h" > +#include "realtek-smi.h" > > #define REALTEK_SMI_ACK_RETRY_COUNT 5 > > @@ -560,8 +560,13 @@ static struct platform_driver realtek_smi_driver = { > .remove_new = realtek_smi_remove, > .shutdown = realtek_smi_shutdown, > }; > -module_platform_driver(realtek_smi_driver); > > -MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); > -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface"); > -MODULE_LICENSE("GPL"); > +int realtek_smi_init(void) > +{ > + return platform_driver_register(&realtek_smi_driver); > +} > + > +void realtek_smi_exit(void) > +{ > + platform_driver_unregister(&realtek_smi_driver); > +} > diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h > new file mode 100644 > index 000000000000..9a4838321f94 > --- /dev/null > +++ b/drivers/net/dsa/realtek/realtek-smi.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > + > +#ifndef _REALTEK_SMI_H > +#define _REALTEK_SMI_H > + > +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) > + > +int realtek_smi_init(void); > +void realtek_smi_exit(void); > + > +#else > + > +static inline int realtek_smi_init(void) > +{ > + return 0; > +} > + > +static inline void realtek_smi_exit(void) > +{ > +} > + > +#endif > + > +#endif > -- > 2.34.1 > > > It looks pretty reasonable to me. More stuff could go into > realtek-interface-common.c, that could be called directly from > realtek-smi.c and realtek-mdio.c without exporting anything. > > I've eliminated the possibility for the SMI and MDIO options to be > anything other than y or n, because only a single interface module > (the common one) exists, and the y/n/m quality of that is > implied/selected by the drivers which depend on it. I hope I wasn't too > trigger-happy with this. Vladimir, you did all the work ;-) Thanks! I implemented this code (with the fixes), and this is the result: 30272 drivers/net/dsa/realtek/realtek-mdio.ko 42176 drivers/net/dsa/realtek/realtek-smi.ko 114544 drivers/net/dsa/realtek/rtl8365mb.ko 115080 drivers/net/dsa/realtek/rtl8366.ko 87264 drivers/net/dsa/realtek/realtek-interface.ko 114544 drivers/net/dsa/realtek/rtl8365mb.ko 115080 drivers/net/dsa/realtek/rtl8366.ko It is still strange why merging both modules into a single realtek-interface.ko results in a slightly larger file. Anyway, the difference is not that significant. I still think that some systems will miss that extra 30kb or 40kb for the interface code they don't need. Your proposed Kconfig does not attempt to avoid a realtek-interface without both interfaces or without support for both switch families. Is it possible in Kconfig to force it to, at least, select one of the interfaces and one of the switches? Is it okay to leave it unconstrained? If merging the modules is the accepted solution, it makes me wonder if rtl8365mb.ko and rtl8366.ko should get merged as well into a single realtek-switch.ko. They are a hard dependency for realtek-interface.ko (previously on each interface module). If the kernel is custom-built, it would still be possible to exclude one switch family at build time. I'll use these modules in OpenWrt, which builds a single kernel for a bunch of devices. Is there a way to weakly depend on a module, allowing the system to load only a single subdriver? Is it worth it? Regards, Luiz
On Mon, Nov 6, 2023 at 11:37 PM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > Your proposed Kconfig does not attempt to avoid a realtek-interface > without both interfaces or without support for both switch families. > Is it possible in Kconfig to force it to, at least, select one of the > interfaces and one of the switches? Is it okay to leave it > unconstrained? Can't you just remove the help text under NET_DSA_REALTEK_INTERFACE so it becomes a hidden option? The other options just select it anyway. > If merging the modules is the accepted solution, it makes me wonder if > rtl8365mb.ko and rtl8366.ko should get merged as well into a single > realtek-switch.ko. They are a hard dependency for realtek-interface.ko > (previously on each interface module). If the kernel is custom-built, > it would still be possible to exclude one switch family at build time. That's not a good idea, because we want to be able to load a single module into the kernel to support a single switch family at runtime. If you have a kernel that boots on several systems and some of them have one of the switches and some of them have another switch, I think you see the problem with this approach. > I'll use these modules in OpenWrt, which builds a single kernel for a > bunch of devices. Is there a way to weakly depend on a module, > allowing the system to load only a single subdriver? Is it worth it? Last time I looked actually having DSA:s as loadable modules didn't work so well, so they are all compiled in. In OpenWrt I didn't find any DSA modules packaged as modules. But maybe I didn't try hard enough. IIRC the problem is that it needs to also have a tag module (for NET_DSA_TAG_*) and that didn't modularize so well. Yours, Linus Walleij
> > Your proposed Kconfig does not attempt to avoid a realtek-interface > > without both interfaces or without support for both switch families. > > Is it possible in Kconfig to force it to, at least, select one of the > > interfaces and one of the switches? Is it okay to leave it > > unconstrained? > > Can't you just remove the help text under > NET_DSA_REALTEK_INTERFACE so it becomes a hidden > option? The other options just select it anyway. Without a text after the tristate, it will already be hidden. However, we can still ask to build a module with no SMI and MDIO. > > If merging the modules is the accepted solution, it makes me wonder if > > rtl8365mb.ko and rtl8366.ko should get merged as well into a single > > realtek-switch.ko. They are a hard dependency for realtek-interface.ko > > (previously on each interface module). If the kernel is custom-built, > > it would still be possible to exclude one switch family at build time. > > That's not a good idea, because we want to be able to load > a single module into the kernel to support a single switch > family at runtime. If you have a kernel that boots on several > systems and some of them have one of the switches and > some of them have another switch, I think you see the problem > with this approach. We already have this situation. As the interface module uses rtl8366rb_variant and rtl8365mb_variant, we cannot select one or the other at runtime. rtl8365mb 14802 1 realtek_smi rtl8366 20870 1 realtek_smi tag_rtl4_a 1522 1 If we build it with support for both switches, both modules need to be loaded together. Somehow initializing the switch selectively autoloads the tag module. Is it possible to have something like this for subdrivers? > > I'll use these modules in OpenWrt, which builds a single kernel for a > > bunch of devices. Is there a way to weakly depend on a module, > > allowing the system to load only a single subdriver? Is it worth it? > > Last time I looked actually having DSA:s as loadable modules > didn't work so well, so they are all compiled in. In OpenWrt > I didn't find any DSA modules packaged as modules. But maybe > I didn't try hard enough. IIRC the problem is that it needs to > also have a tag module (for NET_DSA_TAG_*) and that didn't > modularize so well. It does work, even the tag module. As I mentioned, the tag modules are even loaded on demand. You just need to load it in the correct sequence. > > Yours, > Linus Walleij
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 292e6d087e8b..aad94e49d4c9 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = { .disable_locking = true, }; +static void realtek_mdio_reset_assert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_assert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, true); +} + +static void realtek_mdio_reset_deassert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_deassert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, false); +} + static int realtek_mdio_probe(struct mdio_device *mdiodev) { struct realtek_priv *priv; @@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) dev_set_drvdata(dev, priv); - /* TODO: if power is software controlled, set up any regulators here */ priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); + if (IS_ERR(priv->reset_ctl)) { + ret = PTR_ERR(priv->reset_ctl); + return dev_err_probe(dev, ret, "failed to get reset control\n"); + } + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(priv->reset)) { dev_err(dev, "failed to get RESET GPIO\n"); return PTR_ERR(priv->reset); } - - if (priv->reset) { - gpiod_set_value(priv->reset, 1); + if (priv->reset_ctl || priv->reset) { + realtek_mdio_reset_assert(priv); dev_dbg(dev, "asserted RESET\n"); msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); + realtek_mdio_reset_deassert(priv); msleep(REALTEK_HW_START_DELAY); dev_dbg(dev, "deasserted RESET\n"); } @@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev) dsa_unregister_switch(priv->ds); /* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_mdio_reset_assert(priv); } static void realtek_mdio_shutdown(struct mdio_device *mdiodev) diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index bfd11591faf4..a99e53b5b662 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) return ret; } +static void realtek_smi_reset_assert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_assert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, true); +} + +static void realtek_smi_reset_deassert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_deassert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, false); +} + static int realtek_smi_probe(struct platform_device *pdev) { const struct realtek_variant *var; @@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev) dev_set_drvdata(dev, priv); spin_lock_init(&priv->lock); - /* TODO: if power is software controlled, set up any regulators here */ + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); + if (IS_ERR(priv->reset_ctl)) { + ret = PTR_ERR(priv->reset_ctl); + return dev_err_probe(dev, ret, "failed to get reset control\n"); + } priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(priv->reset)) { dev_err(dev, "failed to get RESET GPIO\n"); return PTR_ERR(priv->reset); } - if (priv->reset) { - gpiod_set_value(priv->reset, 1); + if (priv->reset_ctl || priv->reset) { + realtek_smi_reset_assert(priv); dev_dbg(dev, "asserted RESET\n"); msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); + realtek_smi_reset_deassert(priv); msleep(REALTEK_HW_START_DELAY); dev_dbg(dev, "deasserted RESET\n"); } @@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev) of_node_put(priv->slave_mii_bus->dev.of_node); /* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_smi_reset_assert(priv); } static void realtek_smi_shutdown(struct platform_device *pdev) diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index 4fa7c6ba874a..516450837b74 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/gpio/consumer.h> #include <net/dsa.h> +#include <linux/reset.h> #define REALTEK_HW_STOP_DELAY 25 /* msecs */ #define REALTEK_HW_START_DELAY 100 /* msecs */ @@ -48,6 +49,7 @@ struct rtl8366_vlan_4k { struct realtek_priv { struct device *dev; + struct reset_control *reset_ctl; struct gpio_desc *reset; struct gpio_desc *mdc; struct gpio_desc *mdio;
The 'reset-gpios' will not work when the switch reset is controlled by a reset controller. Although the reset is optional and the driver performs a soft reset during setup, if the initial reset state was asserted, the driver will not detect it. The reset controller will take precedence over the reset GPIO. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++---- drivers/net/dsa/realtek/realtek-smi.c | 49 ++++++++++++++++++++++--- drivers/net/dsa/realtek/realtek.h | 2 + 3 files changed, 89 insertions(+), 13 deletions(-)