[PATCH 09/14] net: ravb: Support up to two instances

[Marek Vasut]

On 10/28/24 10:50 AM, Paul Barker wrote:
> 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 at 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?
Patches posted somewhat inline with what you suggested:

https://patchwork.ozlabs.org/project/uboot/patch/20250118061304.124807-1-marek.vasut+renesas@mailbox.org/
https://patchwork.ozlabs.org/project/uboot/patch/20250118061445.124932-1-marek.vasut+renesas@mailbox.org/
https://patchwork.ozlabs.org/project/uboot/patch/20250118061558.124963-1-marek.vasut+renesas@mailbox.org/
https://patchwork.ozlabs.org/project/uboot/patch/20250118061629.125005-1-marek.vasut+renesas@mailbox.org/
https://patchwork.ozlabs.org/project/uboot/patch/20250118063458.137258-1-marek.vasut+renesas@mailbox.org/
https://patchwork.ozlabs.org/project/uboot/patch/20250118065402.172253-1-marek.vasut+renesas@mailbox.org/