On 7/7/25 7:03 PM, Magnus Damm wrote:
Hello Magnus,
I'm sorry for the late reply.
+++ work/drivers/net/sh_eth.c 2025-07-05 17:45:07.333754799 +0900
@@ -144,10 +144,10 @@ static int sh_eth_reset(struct sh_eth_in
{
#if defined(SH_ETH_TYPE_GETHER) || defined(SH_ETH_TYPE_RZ)
int ret = 0, i;
-
Does something like this ...
if (!device_is_compatible(dev, "..."))
... work instead of the ifdef ?
If yes, it might be even improved into dedicated function:
static bool device_is_rza2(struct udevice *dev)
{
if (!IS_ENABLED(CONFIG_RZA2))
return false;
else
return device_is_compatible(dev, "...");
}
That should be compile-time checked and then optimized out, and in case
of non-RZA2, this should be optimized out entirely.
While I'm all for trying to clean up the code, unfortunately I don't
think only adding compat string handling to RZ is going to help that
much. My apologies for prodding with DT in the SCIF driver for RZA2
only. The same thing applies there in my opinion. =) But we have to
start somewhere, that's for sure.
I suspect cleaning up the sh_eth will be a larger undertaking, yes.
By the way, did you see the #ifdeffery and the compile time constants
in sh_eth.h? Also there seems to be a lot of overlap between
TYPE_GETHER and TYPE_RZ.
I am aware of the ifdeffery, yes.
My first take on cleaning up the sh_eth.c driver would probably be to
dive deeper into the offset tables (such as sh_eth_offset_rz in
sh_eth.h) and convert the code to during runtime use the offset table
to determine if a register exists or not. That would take care of a
bunch of TYPE_GETHER and TYPE_RZ differences and remove some #ifdefs
from the code as long as the offset tables are correct (which they
don't seem to be for RZ so those need to be validated as well). Then
perhaps add some feature flag to handle corner cases like
'device_needs_polled_reset' to clean up sh_eth_reset().
The one thing you have to be careful about is to avoid increasing the
code size too much. Use if (IS_ENABLED()) and such macros, which do
compile the code, but then let gcc optimize it out. This retains code
coverage, but shouldn't increase the code size.
And during probe() or U-Boot equivalent then using the compat string
detection mechanism to select appropriate offset table would make
sense to me.
What do you think? This seems like incremental cleanups though, not
neccessarily related to RZ. I am happy to clean it up though, if you
think it would be helpful.
I am happy with incremental clean ups .
+#if !defined(CONFIG_RZA2)
/* Start e-dmac transmitter and receiver */
sh_eth_write(port_info, EDSR_ENALL, EDSR);
-
[...]
@@ -722,8 +726,11 @@ static int sh_ether_probe(struct udevice
goto err_mdio_register;
priv->bus = mdiodev;
-
+#ifdef BASE_IO_ADDR
port_info->iobase = (void __iomem *)(uintptr_t)BASE_IO_ADDR;
+#else
+ port_info->iobase = (void __iomem *)pdata->iobase;
+#endif
Can the base address be pulled from DT?
Actually pdata->iobase comes from DT. Currently BASE_IO_ADDR is not
set in sh_eth.h for RZ/A1 and RZ/A2. In such case the code above uses
DT to get the device base address. Other processor variants still rely
on the compile time constants.
Looking at sh_eth.h , it seems to me that every processor that is
currently supported can already pull the address from DT (Gen2, RZ/A1,
Gen3 V3H), the rest of the CPUs are not supported. Maybe the
BASE_IO_ADDR can be dropped outright instead ?
And good news, as I wrote in some cover
email, this driver is with this patch known to work in multi-port
configuration since RZ/A2 GR-Peach is using ch0 with MII and RZ/A2
RZA2MBTC is using ch1 RMII. The configuration all comes from DT.
That is nice.
