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

[Marek Vasut]

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.

        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.

+       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 ?

[...]

@@ -684,9 +821,35 @@ int ravb_of_to_plat(struct udevice *dev)
        return 0;
  }
  
+static const struct ravb_device_ops ravb_device_ops_rzg2l = {

Keep the list sorted, rzg is below rcar (Z is after C) .

+       .mac_init = ravb_mac_init_rzg2l,
+       .dmac_init = ravb_dmac_init_rzg2l,
+       .config = ravb_config_rzg2l,
+       .reset_deassert = ravb_reset_deassert_rzg2l,
+       .reset_assert = ravb_reset_assert_rzg2l,
+};
+
+static const struct ravb_device_ops ravb_device_ops_rcar = {
+       .mac_init = ravb_mac_init_rcar,
+       .dmac_init = ravb_dmac_init_rcar,
+       .config = ravb_config_rcar,
+       .reset_deassert = ravb_reset_deassert_rcar,
+       .reset_assert = ravb_reset_assert_rcar,
+};
[...]