Hi Marek,
Thanks for all the feedback.
On Mon, Jul 7, 2025 at 5:52 PM Marek Vasut <marek.va...@mailbox.org> wrote:
>
> On 7/6/25 1:29 PM, Magnus Damm wrote:
>
> > +++ 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.
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.
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().
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.
> > +#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. 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.
Thanks,
Magnus
