CC Niklas
On Tue, 25 Mar 2025 at 00:05, Marek Vasut <marek.va...@mailbox.org> wrote:
> On 3/24/25 11:12 AM, Paul Barker wrote:
> > On 24/03/2025 01:11, Marek Vasut wrote:
> >> On 3/19/25 1:03 PM, Paul Barker wrote:
> >>
> >> [...]
> >>
> >>> +++ b/drivers/net/Kconfig
> >>> @@ -864,7 +864,7 @@ config RENESAS_RAVB
> >>> select PHY_ETHERNET_ID
> >>> help
> >>> This driver implements support for the Ethernet AVB block in
> >>> - Renesas M3 and H3 SoCs.
> >>> + several Renesas R-Car and RZ SoCs.
> >>
> >> RZ/G instead of RZ , right ?
> >
> > This will also be used for RZ/V2L.
>
> Ahh, thank you for clarifying.
>
> [...]
>
> >>> +static void ravb_mac_init_rzg2l(struct udevice *dev)
> >>> +{
> >>> + struct ravb_priv *eth = dev_get_priv(dev);
> >>> +
> >>> + setbits_32(eth->iobase + RAVB_REG_ECMR,
> >>> + ECMR_PRM | ECMR_RXF | ECMR_TXF | ECMR_RCPT |
> >>> + ECMR_TE | ECMR_RE | ECMR_RZPF |
> >>> + (eth->phydev->duplex ? ECMR_DM : 0));
> >>
> >> Can you configure the ECMR extras in ravb_config() just before
> >> writel(mask, eth->iobase + RAVB_REG_ECMR); based on some private data
> >> flag, like '.is_rzg2l' , instead ?
> >
> > ravb_config() has been split into ravb_config_rcar() &
> > ravb_config_rzg2l() by this patch series. So there is no longer a common
> > write to RAVB_REG_ECMR.
> >
> >>
> >>> +}
> >>> +
> >>> /* AVB-DMAC init function */
> >>> static int ravb_dmac_init(struct udevice *dev)
> >>> {
> >>> @@ -459,6 +481,14 @@ static void ravb_dmac_init_rcar(struct udevice *dev)
> >>> writel(mode, eth->iobase + RAVB_REG_APSR);
> >>> }
> >>>
> >>> +static void ravb_dmac_init_rzg2l(struct udevice *dev)
> >>> +{
> >>> + struct ravb_priv *eth = dev_get_priv(dev);
> >>> +
> >>> + /* Set Max Frame Length (RTC) */
> >>> + writel(0x7ffc0000 | RFLR_RFL_MIN, eth->iobase + RAVB_REG_RTC);
> >>
> >> I assume this register is actually RZ/G2L specific ?
> >
> > This register also exists on RZ/G2{H,M,N,E}, but in the Linux kernel
> > ravb driver it is only modified for RZ/G2L.
>
> Is this deliberate or is that a bug ?
>
> +CC Geert.
>
> >>> +}
> >>> +
> >>> static int ravb_config(struct udevice *dev)
> >>> {
> >>> struct ravb_device_ops *device_ops =
> >>> @@ -501,6 +531,22 @@ static void ravb_config_rcar(struct udevice *dev)
> >>> writel(mask, eth->iobase + RAVB_REG_ECMR);
> >>> }
> >>>
> >>> +static void ravb_config_rzg2l(struct udevice *dev)
> >>> +{
> >>> + struct ravb_priv *eth = dev_get_priv(dev);
> >>> + struct phy_device *phy = eth->phydev;
> >>> +
> >>> + writel(CSR0_TPE | CSR0_RPE, eth->iobase + RAVB_REG_CSR0);
> >>> +
> >>> + /* Set the transfer speed */
> >>> + if (phy->speed == 10)
> >>> + writel(GECMR_SPEED_10M, eth->iobase + RAVB_REG_GECMR);
> >>
> >> Since this patch adds .has_reset flag, wouldn't it be possible to add
> >> another flag called something like .has_10 flag and simply do:
> >>
> >> if (eth->has_10 && phy->speed == 10)
> >> writel(GECMR_SPEED_10M, eth->iobase + RAVB_REG_GECMR);
> >>
> >> ?
> >
> > The layout of RAVB_REG_GECMR differs between RZ/G2L and RZ/G2{H,M,N,E},
> > so we can't use the same constants for all platforms. We also need the
> > additional write to RAVB_REG_CSR0 for RZ/G2L so it's worth having the
> > ravb_config_rzg2l() function instead of a couple of flags.
> Ah, thank you for clarifying. This register layout bit is what I was
> missing. The CSR0 part could have been isolated in some if (..), but the
> register layout is a bit more unpleasant to deal with.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
