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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Phil,

Thanks for your patch!

On Tue, Jun 28, 2022 at 7:40 PM Phil Edworthy <phil.edworthy@renesas.com> 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?

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?

> --- /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().

#define PERI0_SDI0    GENMASK(1, 0)

> +
> +#define BANK_LOWER_4GB         0
> +#define BANK_UPPER_4GB         1

I'm not sure these are useful.  The values are just the high
address bits.

> +
> +static const struct of_device_id renesas_socs[] __initconst = {
> +       { .compatible = "renesas,r9a09g011-sys" },
> +       { /* sentinel */ }
> +};
> +
> +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)?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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