diff mbox series

[09/13] ARM: imx: Remove unused IO_ADDRESS() macros

Message ID 20200916205522.8783-10-festevam@gmail.com
State New
Headers show
Series ARM: imx: Further cleanups due to dt-only conversion | expand

Commit Message

Fabio Estevam Sept. 16, 2020, 8:55 p.m. UTC
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(-)

Comments

Arnd Bergmann Sept. 16, 2020, 9:34 p.m. UTC | #1
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
Fabio Estevam Sept. 16, 2020, 9:56 p.m. UTC | #2
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.
Arnd Bergmann Sept. 17, 2020, 7:16 a.m. UTC | #3
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
Fabio Estevam Sept. 17, 2020, 10:17 a.m. UTC | #4
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
Linus Walleij Sept. 29, 2020, 1:11 p.m. UTC | #5
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
Arnd Bergmann Sept. 29, 2020, 1:21 p.m. UTC | #6
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
Linus Walleij Sept. 30, 2020, 9:34 a.m. UTC | #7
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 mbox series

Patch

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