| Message ID | 20220628173947.91519-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] soc: renesas: Add RZ/V2M SYS driver | expand |
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
Hi Geert, Thank you for looking at this. 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. You make a very good point about U-Boot setting it correctly. > 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? > > --- /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. > #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)? > Thanks Phil
Hi Phil, On Fri, Jul 1, 2022 at 11:15 AM Phil Edworthy <phil.edworthy@renesas.com> 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? > 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. > > > --- /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(). [1] https://lore.kernel.org/all/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be 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
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
diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig index c50a6ce1b99d..b9e3dc879ddc 100644 --- a/drivers/soc/renesas/Kconfig +++ b/drivers/soc/renesas/Kconfig @@ -327,6 +327,7 @@ config ARCH_R9A09G011 bool "ARM64 Platform support for RZ/V2M" select PM select PM_GENERIC_DOMAINS + select SYS_R9A09G011 help This enables support for the Renesas RZ/V2M SoC. @@ -440,4 +441,7 @@ config SYSC_R8A774B1 bool "System Controller support for RZ/G2N" if COMPILE_TEST select SYSC_RCAR +config SYS_R9A09G011 + bool "System Controller support for RZ/V2M" if COMPILE_TEST + endif # SOC_RENESAS diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile index 535868c9c7e4..7e269ab6343e 100644 --- a/drivers/soc/renesas/Makefile +++ b/drivers/soc/renesas/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_SYSC_R8A779G0) += r8a779g0-sysc.o ifdef CONFIG_SMP obj-$(CONFIG_ARCH_R9A06G032) += r9a06g032-smp.o endif +obj-$(CONFIG_SYS_R9A09G011) += r9a09g011-sys.o # Family obj-$(CONFIG_RST_RCAR) += rcar-rst.o diff --git a/drivers/soc/renesas/r9a09g011-sys.c b/drivers/soc/renesas/r9a09g011-sys.c new file mode 100644 index 000000000000..6a72ab15cc89 --- /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 + +#define BANK_LOWER_4GB 0 +#define BANK_UPPER_4GB 1 + +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); +} + +static int __init r9a09g011_init(void) +{ + const struct of_device_id *match; + struct device_node *np; + void __iomem *base; + int error = 0; + + np = of_find_matching_node_and_match(NULL, renesas_socs, &match); + if (!np) + return -ENODEV; + + base = of_iomap(np, 0); + if (!base) { + pr_warn("%pOF: Cannot map regs\n", np); + error = -ENOMEM; + goto out_put; + } + + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, SDI0_SHIFT); + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, SDI1_SHIFT); + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, EMMC_SHIFT); + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, USB_HOST_SHIFT); + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, USB_PERI_SHIFT); + write_peri_bank(base + SYS_PERI0_BANK, BANK_LOWER_4GB, PCIE_SHIFT); + write_peri_bank(base + SYS_PERI1_BANK, BANK_LOWER_4GB, ETH_SHIFT); + +out_put: + of_node_put(np); + return error; +} + +core_initcall(r9a09g011_init);