On 27/10/2024 16:29, Marek Vasut wrote: > On 10/24/24 5:24 PM, Paul Barker wrote: >> The Renesas R9A07G044L (RZ/G2L) SoC includes two Gigabit Ethernet >> interfaces which can be supported using the ravb driver. Some RZ/G2L >> specific steps need to be taken during initialization due to differences >> between this SoC and previously supported SoCs. We also need to ensure >> that the module reset is de-asserted after the module clock is enabled >> but before any Ethernet register reads/writes take place. >> >> Signed-off-by: Paul Barker <[email protected]> >> --- >> arch/arm/mach-renesas/Kconfig | 1 + >> drivers/net/Kconfig | 2 + >> drivers/net/ravb.c | 183 ++++++++++++++++++++++++++++++++-- >> 3 files changed, 176 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm/mach-renesas/Kconfig b/arch/arm/mach-renesas/Kconfig >> index aeb55da609bd..d373ab56ce91 100644 >> --- a/arch/arm/mach-renesas/Kconfig >> +++ b/arch/arm/mach-renesas/Kconfig >> @@ -76,6 +76,7 @@ config RZG2L >> imply MULTI_DTB_FIT >> imply MULTI_DTB_FIT_USER_DEFINED_AREA >> imply PINCTRL_RZG2L >> + imply RENESAS_RAVB >> imply RENESAS_SDHI >> imply RZG2L_GPIO >> imply SCIF_CONSOLE >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> index 89f7411bdf33..d009acdcd94f 100644 >> --- a/drivers/net/Kconfig >> +++ b/drivers/net/Kconfig >> @@ -822,6 +822,8 @@ config RENESAS_RAVB >> depends on RCAR_64 >> select PHYLIB >> select PHY_ETHERNET_ID >> + select BITBANGMII >> + select BITBANGMII_MULTI > > Keep the list sorted.
Will fix in v2.
>
>> help
>> This driver implements support for the Ethernet AVB block in
>> Renesas M3 and H3 SoCs.
>> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
>> index fb869cd0872e..e2ab929858c8 100644
>> --- a/drivers/net/ravb.c
>> +++ b/drivers/net/ravb.c
>
> [...]
>
>> @@ -108,6 +122,16 @@
>>
>> #define RAVB_TX_TIMEOUT_MS 1000
>>
>> +#define RAVB_RCV_BUFF_MAX 8192
>> +
>> +struct ravb_device_ops {
>> + int (*mac_init)(struct udevice *dev);
>> + int (*dmac_init)(struct udevice *dev);
>> + int (*config)(struct udevice *dev);
>> + int (*reset_deassert)(struct udevice *dev);
>> + void (*reset_assert)(struct udevice *dev);
>> +};
>
> [...]
>
>> +static int ravb_reset_deassert_rcar(struct udevice *dev)
>> +{
>
> The callsites should check if a callback is assigned or NULL and only
> call the callback if it is assigned. Then you won't need empty callbacks
> like this.
>
> Basically add if (ops->reset_deassert) ops->reset_deassert() and remove
> this empty function.
Will fix in v2.
>
>> + return 0;
>> +}
>> +
>> +static void ravb_reset_assert_rzg2l(struct udevice *dev)
>> +{
>> + struct ravb_priv *eth = dev_get_priv(dev);
>> +
>> + reset_assert(ð->rst);
>> + reset_free(ð->rst);
>> +}
>
> A bit of a design question -- would it make sense to have ravb-rcar.c
> and ravb-rzg2l.c to contain the differences between the ravb variants,
> and keep common code only in ravb.c ?
That would probably be an improvement. I'll do that for v2.
Thanks,
--
Paul Barker
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
