| Message ID | 20200916205522.8783-10-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: imx: Further cleanups due to dt-only conversion | expand |
On Wed, Sep 16, 2020 at 10:56 PM Fabio Estevam <festevam@gmail.com> wrote: > > The IO_ADDRESS() macros were used to retrieve the peripherals > base address for board related code. > > Now that i.MX is a devicetree-only platforms, all base addresses are > retrieved from devicetree and these macros are unused. > > Remove the unused IO_ADDRESS() macros. > > Signed-off-by: Fabio Estevam <festevam@gmail.com> > --- > arch/arm/mach-imx/mx27.h | 1 - > arch/arm/mach-imx/mx31.h | 1 - > arch/arm/mach-imx/mx35.h | 1 - > 3 files changed, 3 deletions(-) > > diff --git a/arch/arm/mach-imx/mx27.h b/arch/arm/mach-imx/mx27.h > index c6f7aae02b67..d6dae9fa8610 100644 > --- a/arch/arm/mach-imx/mx27.h > +++ b/arch/arm/mach-imx/mx27.h > @@ -112,7 +112,6 @@ > #define MX27_IRAM_BASE_ADDR 0xffff4c00 /* internal ram */ > > #define MX27_IO_P2V(x) IMX_IO_P2V(x) > -#define MX27_IO_ADDRESS(x) IOMEM(MX27_IO_P2V(x)) As far as I can tell, the MX27_IO_P2V() macro should be removed here as well if MX27_IO_ADDRESS() gets removed. What about the other constants in these files? Are there any remaining references to the MX*_BASE_ADDRESS, MX*_INT_*, and MX*_DMA_REQ_* constants after the series? Arnd
Hi Arnd, On Wed, Sep 16, 2020 at 6:35 PM Arnd Bergmann <arnd@arndb.de> wrote: > As far as I can tell, the MX27_IO_P2V() macro should be removed here as > well if MX27_IO_ADDRESS() gets removed. The MX27_IO_P2V() macro is still used. Here is the error when I try to remove it: In file included from arch/arm/mach-imx/mach-imx27.c:17:0: arch/arm/mach-imx/mach-imx27.c:30:16: error: implicit declaration of function ‘MX27_IO_P2V’; did you mean ‘MX35_IO_P2V’? [-Werror=implicit-function-declaration] imx_map_entry(MX27, AIPI, MT_DEVICE), ^ arch/arm/mach-imx/hardware.h:103:13: note: in definition of macro ‘imx_map_entry’ .virtual = soc ## _IO_P2V(soc ## _ ## name ## _BASE_ADDR), \ ^~~ arch/arm/mach-imx/mach-imx27.c:30:16: error: initializer element is not constant imx_map_entry(MX27, AIPI, MT_DEVICE), > What about the other constants in these files? Are there any remaining > references to the MX*_BASE_ADDRESS, MX*_INT_*, and MX*_DMA_REQ_* > constants after the series? Some of these can go away. I will remove what is possible in v2.
On Wed, Sep 16, 2020 at 11:56 PM Fabio Estevam <festevam@gmail.com> wrote: > On Wed, Sep 16, 2020 at 6:35 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > As far as I can tell, the MX27_IO_P2V() macro should be removed here as > > well if MX27_IO_ADDRESS() gets removed. > > The MX27_IO_P2V() macro is still used. Here is the error when I try to > remove it: > > In file included from arch/arm/mach-imx/mach-imx27.c:17:0: > arch/arm/mach-imx/mach-imx27.c:30:16: error: implicit declaration of > function ‘MX27_IO_P2V’; did you mean ‘MX35_IO_P2V’? > [-Werror=implicit-function-declaration] > imx_map_entry(MX27, AIPI, MT_DEVICE), > ^ > arch/arm/mach-imx/hardware.h:103:13: note: in definition of macro > ‘imx_map_entry’ > .virtual = soc ## _IO_P2V(soc ## _ ## name ## _BASE_ADDR), \ Ah, right. My grep did not catch the static mappings: arch/arm/mach-imx/mm-imx21.c-/* MX21 memory map definition */ arch/arm/mach-imx/mm-imx21.c-static struct map_desc imx21_io_desc[] __initdata = { arch/arm/mach-imx/mm-imx21.c: imx_map_entry(MX21, AIPI, MT_DEVICE), arch/arm/mach-imx/mm-imx21.c: imx_map_entry(MX21, SAHB1, MT_DEVICE), arch/arm/mach-imx/mm-imx21.c: imx_map_entry(MX21, X_MEMC, MT_DEVICE), arch/arm/mach-imx/mm-imx21.c-}; -- arch/arm/mach-imx/mm-imx27.c-/* MX27 memory map definition */ arch/arm/mach-imx/mm-imx27.c-static struct map_desc imx27_io_desc[] __initdata = { arch/arm/mach-imx/mm-imx27.c: imx_map_entry(MX27, AIPI, MT_DEVICE), arch/arm/mach-imx/mm-imx27.c: imx_map_entry(MX27, SAHB1, MT_DEVICE), arch/arm/mach-imx/mm-imx27.c: imx_map_entry(MX27, X_MEMC, MT_DEVICE), arch/arm/mach-imx/mm-imx27.c-}; -- arch/arm/mach-imx/mm-imx3.c-static struct map_desc mx31_io_desc[] __initdata = { arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX31, X_MEMC, MT_DEVICE), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX31, AVIC, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX31, AIPS1, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX31, AIPS2, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX31, SPBA0, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c-}; -- arch/arm/mach-imx/mm-imx3.c-#ifdef CONFIG_SOC_IMX35 arch/arm/mach-imx/mm-imx3.c-static struct map_desc mx35_io_desc[] __initdata = { arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX35, X_MEMC, MT_DEVICE), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX35, AVIC, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX35, AIPS1, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX35, AIPS2, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c: imx_map_entry(MX35, SPBA0, MT_DEVICE_NONSHARED), arch/arm/mach-imx/mm-imx3.c-}; If all of the devices in here are now also mapped using of_iomap() or ioremap(), then the iotables are not strictly needed any more, but most of them are 1MB section sized, which helps slightly reduce TLB usage compared to non-section ioremap() mappings, so we might want to keep them anyway, possibly with the constants moved from the header into the mm-imx??.c files. Adding Linus to Cc for further thoughts, as he was looking at the early io mappings recently. Arnd
Hi Arnd, On Thu, Sep 17, 2020 at 4:16 AM Arnd Bergmann <arnd@arndb.de> wrote: > If all of the devices in here are now also mapped using of_iomap() or > ioremap(), then the iotables are not strictly needed any more, but most > of them are 1MB section sized, which helps slightly reduce TLB usage > compared to non-section ioremap() mappings, so we might want to > keep them anyway, possibly with the constants moved from the header > into the mm-imx??.c files. Yes, I can move the definitions to the mm-imx files as a follow-up patch after this series gets applied. Thanks, Fabio Estevam
On Thu, Sep 17, 2020 at 9:16 AM Arnd Bergmann <arnd@arndb.de> wrote: > If all of the devices in here are now also mapped using of_iomap() or > ioremap(), then the iotables are not strictly needed any more, but most > of them are 1MB section sized, which helps slightly reduce TLB usage > compared to non-section ioremap() mappings, so we might want to > keep them anyway, possibly with the constants moved from the header > into the mm-imx??.c files. > > Adding Linus to Cc for further thoughts, as he was looking at the early > io mappings recently. There is early fixmap which I think should replace the iotables completely if we can, since it is generic kernel code. I was meaning to look into this "at some point". The early fixmap seems to be page (0x1000) based so the iotables might be more efficient if using 1MB sections but I doubt that it is worth it. The fixmaps are converted to proper ioremaps (at the exact same virtual address) later during boot. At least that is how it looks to me :D Yours, Linus Walleij
On Tue, Sep 29, 2020 at 3:11 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Sep 17, 2020 at 9:16 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > If all of the devices in here are now also mapped using of_iomap() or > > ioremap(), then the iotables are not strictly needed any more, but most > > of them are 1MB section sized, which helps slightly reduce TLB usage > > compared to non-section ioremap() mappings, so we might want to > > keep them anyway, possibly with the constants moved from the header > > into the mm-imx??.c files. > > > > Adding Linus to Cc for further thoughts, as he was looking at the early > > io mappings recently. > > There is early fixmap which I think should replace the iotables > completely if we can, since it is generic kernel code. I was meaning > to look into this "at some point". > > The early fixmap seems to be page (0x1000) based so the iotables > might be more efficient if using 1MB sections but I doubt that it is > worth it. As far as I can tell, the 1MB section maps are the only reason this exists, as all of them are actually ioremap()ed later on before use. I don't know if it makes a difference to real-world performance. Note that there are only 64 TLB entries on ARM926, or 128 on ARM1136, so avoiding the lookup/eviction at least theoretically should be measurably faster. Arnd
On Tue, Sep 29, 2020 at 3:22 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Sep 29, 2020 at 3:11 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Sep 17, 2020 at 9:16 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > If all of the devices in here are now also mapped using of_iomap() or > > > ioremap(), then the iotables are not strictly needed any more, but most > > > of them are 1MB section sized, which helps slightly reduce TLB usage > > > compared to non-section ioremap() mappings, so we might want to > > > keep them anyway, possibly with the constants moved from the header > > > into the mm-imx??.c files. > > > > > > Adding Linus to Cc for further thoughts, as he was looking at the early > > > io mappings recently. > > > > There is early fixmap which I think should replace the iotables > > completely if we can, since it is generic kernel code. I was meaning > > to look into this "at some point". > > > > The early fixmap seems to be page (0x1000) based so the iotables > > might be more efficient if using 1MB sections but I doubt that it is > > worth it. > > As far as I can tell, the 1MB section maps are the only reason > this exists, as all of them are actually ioremap()ed later on before > use. > > I don't know if it makes a difference to real-world performance. > Note that there are only 64 TLB entries on ARM926, or 128 on > ARM1136, so avoiding the lookup/eviction at least theoretically > should be measurably faster. Hm someone should benchmark this. If this is the case we are kind of "punishing" all the all-out device tree platforms. It would make sense to then support an optional 1MB section mapping and make some of the crucial remaps (of_iomap_hot_io or something), such as for the timer and interrupt controllers, use this by default since they are on the very-hot path. Yours, Linus Walleij
diff --git a/arch/arm/mach-imx/mx27.h b/arch/arm/mach-imx/mx27.h index c6f7aae02b67..d6dae9fa8610 100644 --- a/arch/arm/mach-imx/mx27.h +++ b/arch/arm/mach-imx/mx27.h @@ -112,7 +112,6 @@ #define MX27_IRAM_BASE_ADDR 0xffff4c00 /* internal ram */ #define MX27_IO_P2V(x) IMX_IO_P2V(x) -#define MX27_IO_ADDRESS(x) IOMEM(MX27_IO_P2V(x)) /* fixed interrupt numbers */ #include <asm/irq.h> diff --git a/arch/arm/mach-imx/mx31.h b/arch/arm/mach-imx/mx31.h index d9574671ca5c..192499dcc792 100644 --- a/arch/arm/mach-imx/mx31.h +++ b/arch/arm/mach-imx/mx31.h @@ -117,7 +117,6 @@ #define MX31_PCMCIA_MEM_BASE_ADDR 0xbc000000 #define MX31_IO_P2V(x) IMX_IO_P2V(x) -#define MX31_IO_ADDRESS(x) IOMEM(MX31_IO_P2V(x)) /* * Interrupt numbers diff --git a/arch/arm/mach-imx/mx35.h b/arch/arm/mach-imx/mx35.h index 760de6a0af7e..4f0b939c6e03 100644 --- a/arch/arm/mach-imx/mx35.h +++ b/arch/arm/mach-imx/mx35.h @@ -116,7 +116,6 @@ #define MX35_PCMCIA_MEM_BASE_ADDR 0xbc000000 #define MX35_IO_P2V(x) IMX_IO_P2V(x) -#define MX35_IO_ADDRESS(x) IOMEM(MX35_IO_P2V(x)) /* * Interrupt numbers
The IO_ADDRESS() macros were used to retrieve the peripherals base address for board related code. Now that i.MX is a devicetree-only platforms, all base addresses are retrieved from devicetree and these macros are unused. Remove the unused IO_ADDRESS() macros. Signed-off-by: Fabio Estevam <festevam@gmail.com> --- arch/arm/mach-imx/mx27.h | 1 - arch/arm/mach-imx/mx31.h | 1 - arch/arm/mach-imx/mx35.h | 1 - 3 files changed, 3 deletions(-)