RE: [RFC] soc: renesas: Add RZ/V2M SYS driver

[Phil Edworthy <phil.edworthy@renesas.com>]

Hi Geert,

On 01 July 2022 10:25 Geert Uytterhoeven wrote:
> On Fri, Jul 1, 2022 at 11:15 AM Phil Edworthy wrote:
> > On 01 July 2022 09:37 Geert Uytterhoeven wrote:
> > > On Tue, Jun 28, 2022 at 7:40 PM Phil Edworthy wrote:
> > > > The System Configuration (SYS) module on the Renesas RZ/V2M
> > > > (r9a09g011) contains registers for many different aspects of the
> SoC.
> > > >
> > > > Some of the peripherals on the SoC are only 32-bit address capable
> > > > bus masters. To select the lower 4GiB or upper 4GiB of memory, the
> > > > SYS PERI0_BANK and SYS_PERI1_BANK registers can be programmed to
> > > > set the 33rd address bit.
> > > > Due to the use of firmware with the SoC, uboot is often set up
> > > > such that these peripherals can only access the upper 4GiB. In
> > > > order to allow Linux to use bounce buffers for drivers, we set
> > > > aside some memory in the lower 4GiB for Linux.
> > > > Thus this requires the SYS PERIx_BANK registers to be reprogrammed.
> > >
> > > Does this interfere with the firmware?
> > > If yes, why is this not bad?
> > > If not, why can't U-Boot set this up correctly for Linux?
> > Yes, there is potentially a problem with the firmware trying to write
> > to the registers at the same time. It's unlikely, but possible.
> 
> I'm mainly worried about the firmware relying on the u-boot settings, and
> using the devices?  But I guess that won't happen, if they're assigned to
> Linux?
That's right. The firmware doesn't handle anything related to SD, USB, PCIe
or Eth. I don't know about VCD (multimedia codec) or GRP (graphics engine),
but I am reasonable sure they will be assigned to one of firmware or Linux.


> > You make a very good point about U-Boot setting it correctly.
> 
> Typically things like this are supposed to be handled by U-Boot, in a
> correct way.
> 
> > > As most RAM available to Linux is in the upper 4 GiB, wouldn't it be
> > > better to use the upper 4 GiB, so bounce buffer can be avoided for
> > > most transfers? Or is it impossible to set up bounce buffers in the
> > > upper 4 GiB?
> >
> > Avoiding bounce buffers would be best. I guess I have been guilty of
> > trying to ease my work as some drivers need non-trivial changes. In
> > particular, the usb xhci driver.
> >
> > Perhaps we can continue development for the time being with U-Boot
> > setting the bank addr registers so the peripherals access the lower
> > 4GiB and give Linux some mem in the lower 4GiB for bounce buffers.
> >
> > Can we look at making the drivers use the higher 4GiB later on?
> 
> Sure.
Ok, great.

 
> > > > --- /dev/null
> > > > +++ b/drivers/soc/renesas/r9a09g011-sys.c
> > > > @@ -0,0 +1,67 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Renesas RZ/V2M SYS driver
> > > > + *
> > > > + * Copyright (C) 2022  Renesas Electronics Corporation  */
> > > > +
> > > > +#include <linux/io.h>
> > > > +#include <linux/of_address.h>
> > > > +
> > > > +#define SYS_PERI0_BANK         0x30
> > > > +#define SDI0_SHIFT             0
> > > > +#define SDI1_SHIFT             2
> > > > +#define EMMC_SHIFT             4
> > > > +#define USB_HOST_SHIFT         8
> > > > +#define USB_PERI_SHIFT         10
> > > > +#define PCIE_SHIFT             12
> > > > +
> > > > +#define SYS_PERI1_BANK         0x34
> > > > +#define ETH_SHIFT              0
> > >
> > > These fields look like perfect users of GENMASK() and FIELD_PREP().
> >
> > Another macro I am not familiar with! Thanks for pointing it out.
> 
> > > > +static void write_peri_bank(void __iomem *addr, uint32_t val, int
> > > shift)
> > > > +{
> > > > +       /* Set the write enable bits */
> > > > +       writel(((3 << 16) | val) << shift, addr);
> > >
> > > writel((field << 16) | FIELD_PREP(field, addr_high), addr)?
> 
> Oops, this won't work, as FIELD_PREP() needs a compile-time constant.
> Of course you can still pass the result of FIELD_PREP() as a parameter to
> the function instead.
> 
> Or push for "[PATCH 01/17] bitfield: Add non-constant field_{prep,get}()
> helpers"[1] ;-)  Or open code the proposed field_prep().

Thanks!
Phil