[PATCH v3 7/9] net: sh_eth: Adjust RZ/A1, add RZ/A2 support

[Marek Vasut]

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.