| Message ID | 20241024152448.102-10-paul.barker.ct@bp.renesas.com |
|---|---|
| State | New |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Add support for Ethernet interfaces on RZ/G2L | expand |
On 10/24/24 5:24 PM, Paul Barker wrote: > Several Renesas SoCs in the RZ/G2L family have two Ethernet interfaces. > To support this second interface, we extend the bb_miiphy_buses[] array > and keep track of the current bus index in ravb_of_to_plat(). > > Support for an arbitrary number of instances is not implemented - it is > expected that bb_miiphy_buses will be replaced with a proper device > model/uclass implementation before that is needed. > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > --- > drivers/net/ravb.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c > index f1401d2f6ed2..9b33ce929618 100644 > --- a/drivers/net/ravb.c > +++ b/drivers/net/ravb.c > @@ -11,6 +11,7 @@ > #include <clk.h> > #include <cpu_func.h> > #include <dm.h> > +#include <dm/device_compat.h> > #include <errno.h> > #include <log.h> > #include <miiphy.h> > @@ -494,6 +495,7 @@ static int ravb_probe(struct udevice *dev) > { > struct eth_pdata *pdata = dev_get_plat(dev); > struct ravb_priv *eth = dev_get_priv(dev); > + struct bb_miiphy_bus *phybus; > struct mii_dev *mdiodev; > void __iomem *iobase; > int ret; > @@ -513,7 +515,8 @@ static int ravb_probe(struct udevice *dev) > > mdiodev->read = bb_miiphy_read; > mdiodev->write = bb_miiphy_write; > - bb_miiphy_buses[0].priv = eth; > + phybus = (struct bb_miiphy_bus *)pdata->priv_pdata; > + phybus->priv = eth; > snprintf(mdiodev->name, sizeof(mdiodev->name), dev->name); > > ret = mdio_register(mdiodev); > @@ -625,7 +628,17 @@ int ravb_bb_delay(struct bb_miiphy_bus *bus) > > struct bb_miiphy_bus bb_miiphy_buses[] = { > { > - .name = "ravb", > + .name = "ravb0", > + .init = ravb_bb_init, > + .mdio_active = ravb_bb_mdio_active, > + .mdio_tristate = ravb_bb_mdio_tristate, > + .set_mdio = ravb_bb_set_mdio, > + .get_mdio = ravb_bb_get_mdio, > + .set_mdc = ravb_bb_set_mdc, > + .delay = ravb_bb_delay, > + }, > + { > + .name = "ravb1", > .init = ravb_bb_init, > .mdio_active = ravb_bb_mdio_active, > .mdio_tristate = ravb_bb_mdio_tristate, > @@ -646,10 +659,16 @@ static const struct eth_ops ravb_ops = { > .write_hwaddr = ravb_write_hwaddr, > }; > > +static int bb_miiphy_index; > + > int ravb_of_to_plat(struct udevice *dev) > { > struct eth_pdata *pdata = dev_get_plat(dev); > - const fdt32_t *cell; > + > + if (bb_miiphy_index >= bb_miiphy_buses_num) { > + dev_err(dev, "ravb driver supports only 1 or 2 devices!\n"); Hmmmm, I really do not like this, can we make this dynamic ? Unless you want to take a look at this yourself, I can add it into my todo ?
On 27/10/2024 16:25, Marek Vasut wrote: > On 10/24/24 5:24 PM, Paul Barker wrote: >> Several Renesas SoCs in the RZ/G2L family have two Ethernet interfaces. >> To support this second interface, we extend the bb_miiphy_buses[] array >> and keep track of the current bus index in ravb_of_to_plat(). >> >> Support for an arbitrary number of instances is not implemented - it is >> expected that bb_miiphy_buses will be replaced with a proper device >> model/uclass implementation before that is needed. >> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> >> --- >> drivers/net/ravb.c | 28 ++++++++++++++++++++++++---- >> 1 file changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c >> index f1401d2f6ed2..9b33ce929618 100644 >> --- a/drivers/net/ravb.c >> +++ b/drivers/net/ravb.c >> @@ -11,6 +11,7 @@ >> #include <clk.h> >> #include <cpu_func.h> >> #include <dm.h> >> +#include <dm/device_compat.h> >> #include <errno.h> >> #include <log.h> >> #include <miiphy.h> >> @@ -494,6 +495,7 @@ static int ravb_probe(struct udevice *dev) >> { >> struct eth_pdata *pdata = dev_get_plat(dev); >> struct ravb_priv *eth = dev_get_priv(dev); >> + struct bb_miiphy_bus *phybus; >> struct mii_dev *mdiodev; >> void __iomem *iobase; >> int ret; >> @@ -513,7 +515,8 @@ static int ravb_probe(struct udevice *dev) >> >> mdiodev->read = bb_miiphy_read; >> mdiodev->write = bb_miiphy_write; >> - bb_miiphy_buses[0].priv = eth; >> + phybus = (struct bb_miiphy_bus *)pdata->priv_pdata; >> + phybus->priv = eth; >> snprintf(mdiodev->name, sizeof(mdiodev->name), dev->name); >> >> ret = mdio_register(mdiodev); >> @@ -625,7 +628,17 @@ int ravb_bb_delay(struct bb_miiphy_bus *bus) >> >> struct bb_miiphy_bus bb_miiphy_buses[] = { >> { >> - .name = "ravb", >> + .name = "ravb0", >> + .init = ravb_bb_init, >> + .mdio_active = ravb_bb_mdio_active, >> + .mdio_tristate = ravb_bb_mdio_tristate, >> + .set_mdio = ravb_bb_set_mdio, >> + .get_mdio = ravb_bb_get_mdio, >> + .set_mdc = ravb_bb_set_mdc, >> + .delay = ravb_bb_delay, >> + }, >> + { >> + .name = "ravb1", >> .init = ravb_bb_init, >> .mdio_active = ravb_bb_mdio_active, >> .mdio_tristate = ravb_bb_mdio_tristate, >> @@ -646,10 +659,16 @@ static const struct eth_ops ravb_ops = { >> .write_hwaddr = ravb_write_hwaddr, >> }; >> >> +static int bb_miiphy_index; >> + >> int ravb_of_to_plat(struct udevice *dev) >> { >> struct eth_pdata *pdata = dev_get_plat(dev); >> - const fdt32_t *cell; >> + >> + if (bb_miiphy_index >= bb_miiphy_buses_num) { >> + dev_err(dev, "ravb driver supports only 1 or 2 devices!\n"); > Hmmmm, I really do not like this, can we make this dynamic ? > > Unless you want to take a look at this yourself, I can add it into my todo ? I think the real solution here would be to separate the bb_miiphy operations from the bus instance, so we would have something like: struct bb_miiphy_bus { struct bb_miiphy_ops *ops; void *priv; }; struct bb_miiphy_ops { int (*init)(struct bb_miiphy_bus *bus); int (*mdio_active)(struct bb_miiphy_bus *bus); int (*mdio_tristate)(struct bb_miiphy_bus *bus); int (*set_mdio)(struct bb_miiphy_bus *bus, int v); int (*get_mdio)(struct bb_miiphy_bus *bus, int *v); int (*set_mdc)(struct bb_miiphy_bus *bus, int v); int (*delay)(struct bb_miiphy_bus *bus); }; int bb_miiphy_bus_register(const char *name, struct bb_miiphy_ops *ops, void *priv); Where drivers will call `bb_miiphy_bus_register()` from the probe function, it will create a `struct bb_miiphy_bus` instance and a `struct mii_dev` instance then call `mdio_register()`. The driver can then support an arbitrary number of MDIO busses from a single constant `struct bb_miiphy_ops` instance. The bb_miiphy_getbus() function should be dropped from miiphy.c. Instead, the priv pointer in the `struct mii_dev` instance can point to the appropriate `struct bb_miiphy_bus` instance. It looks like all users of CONFIG_BITBANGMII also set CONFIG_BITBANGMII_MULTI, and there don't seem to be any targets that define the macros documented in README.bitbangMII (lines 15-22). So, we can drop the non-BITBANGMII_MULTI code from miiphybb.c and simplify things a lot. That's non-trivial but it's not a huge set of changes, maybe something we could target for v2024.04? Thanks,
diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c index f1401d2f6ed2..9b33ce929618 100644 --- a/drivers/net/ravb.c +++ b/drivers/net/ravb.c @@ -11,6 +11,7 @@ #include <clk.h> #include <cpu_func.h> #include <dm.h> +#include <dm/device_compat.h> #include <errno.h> #include <log.h> #include <miiphy.h> @@ -494,6 +495,7 @@ static int ravb_probe(struct udevice *dev) { struct eth_pdata *pdata = dev_get_plat(dev); struct ravb_priv *eth = dev_get_priv(dev); + struct bb_miiphy_bus *phybus; struct mii_dev *mdiodev; void __iomem *iobase; int ret; @@ -513,7 +515,8 @@ static int ravb_probe(struct udevice *dev) mdiodev->read = bb_miiphy_read; mdiodev->write = bb_miiphy_write; - bb_miiphy_buses[0].priv = eth; + phybus = (struct bb_miiphy_bus *)pdata->priv_pdata; + phybus->priv = eth; snprintf(mdiodev->name, sizeof(mdiodev->name), dev->name); ret = mdio_register(mdiodev); @@ -625,7 +628,17 @@ int ravb_bb_delay(struct bb_miiphy_bus *bus) struct bb_miiphy_bus bb_miiphy_buses[] = { { - .name = "ravb", + .name = "ravb0", + .init = ravb_bb_init, + .mdio_active = ravb_bb_mdio_active, + .mdio_tristate = ravb_bb_mdio_tristate, + .set_mdio = ravb_bb_set_mdio, + .get_mdio = ravb_bb_get_mdio, + .set_mdc = ravb_bb_set_mdc, + .delay = ravb_bb_delay, + }, + { + .name = "ravb1", .init = ravb_bb_init, .mdio_active = ravb_bb_mdio_active, .mdio_tristate = ravb_bb_mdio_tristate, @@ -646,10 +659,16 @@ static const struct eth_ops ravb_ops = { .write_hwaddr = ravb_write_hwaddr, }; +static int bb_miiphy_index; + int ravb_of_to_plat(struct udevice *dev) { struct eth_pdata *pdata = dev_get_plat(dev); - const fdt32_t *cell; + + if (bb_miiphy_index >= bb_miiphy_buses_num) { + dev_err(dev, "ravb driver supports only 1 or 2 devices!\n"); + return -EOVERFLOW; + } pdata->iobase = dev_read_addr(dev); @@ -662,7 +681,8 @@ int ravb_of_to_plat(struct udevice *dev) if (cell) pdata->max_speed = fdt32_to_cpu(*cell); - sprintf(bb_miiphy_buses[0].name, dev->name); + pdata->priv_pdata = &bb_miiphy_buses[bb_miiphy_index]; + sprintf(bb_miiphy_buses[bb_miiphy_index++].name, dev->name); return 0; }
Several Renesas SoCs in the RZ/G2L family have two Ethernet interfaces. To support this second interface, we extend the bb_miiphy_buses[] array and keep track of the current bus index in ravb_of_to_plat(). Support for an arbitrary number of instances is not implemented - it is expected that bb_miiphy_buses will be replaced with a proper device model/uclass implementation before that is needed. Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> --- drivers/net/ravb.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)