Re: [PATCH 11/14] net: ravb: Add RZ/G2L Support

[Paul Barker]

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 <paul.barker...@bp.renesas.com>
>> ---
>>   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(&eth->rst);
>> +    reset_free(&eth->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