diff mbox series

[4/4] sh: machvec: remove custom ioport_{un,}map()

Message ID 20230802184849.1019466-4-arnd@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/4] sh: remove stale microdev board | expand

Commit Message

Arnd Bergmann Aug. 2, 2023, 6:48 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

These functions were only used on the microdev
board that is now gone, so remove them to simplify
the ioport handling.

This could be further simplified to use the generic
I/O port accessors now.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/sh/include/asm/io.h      |  4 ++--
 arch/sh/include/asm/machvec.h |  5 -----
 arch/sh/kernel/ioport.c       | 13 +------------
 3 files changed, 3 insertions(+), 19 deletions(-)

Comments

John Paul Adrian Glaubitz Sept. 8, 2023, 10:10 a.m. UTC | #1
Hi Arnd!

On Wed, 2023-08-02 at 20:48 +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> These functions were only used on the microdev
> board that is now gone, so remove them to simplify
> the ioport handling.
> 
> This could be further simplified to use the generic
> I/O port accessors now.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/sh/include/asm/io.h      |  4 ++--
>  arch/sh/include/asm/machvec.h |  5 -----
>  arch/sh/kernel/ioport.c       | 13 +------------
>  3 files changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
> index f2f38e9d489ac..ac521f287fa59 100644
> --- a/arch/sh/include/asm/io.h
> +++ b/arch/sh/include/asm/io.h
> @@ -181,7 +181,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port)	\
>  {									\
>  	volatile type *__addr;						\
>  									\
> -	__addr = __ioport_map(port, sizeof(type));			\
> +	__addr = (void __iomem *)sh_io_port_base + port;		\
>  	*__addr = val;							\
>  	slow;								\
>  }									\
> @@ -191,7 +191,7 @@ static inline type pfx##in##bwlq##p(unsigned long port)			\
>  	volatile type *__addr;						\
>  	type __val;							\
>  									\
> -	__addr = __ioport_map(port, sizeof(type));			\
> +	__addr = (void __iomem *)sh_io_port_base + port;		\
>  	__val = *__addr;						\
>  	slow;								\
>  									\
> diff --git a/arch/sh/include/asm/machvec.h b/arch/sh/include/asm/machvec.h
> index 2b4b085e8f219..4e5314b921f19 100644
> --- a/arch/sh/include/asm/machvec.h
> +++ b/arch/sh/include/asm/machvec.h
> @@ -19,11 +19,6 @@ struct sh_machine_vector {
>  	int (*mv_irq_demux)(int irq);
>  	void (*mv_init_irq)(void);
>  
> -#ifdef CONFIG_HAS_IOPORT_MAP
> -	void __iomem *(*mv_ioport_map)(unsigned long port, unsigned int size);
> -	void (*mv_ioport_unmap)(void __iomem *);
> -#endif
> -
>  	int (*mv_clk_init)(void);
>  	int (*mv_mode_pins)(void);
>  
> diff --git a/arch/sh/kernel/ioport.c b/arch/sh/kernel/ioport.c
> index f39446a658bdb..c8aff8a20164d 100644
> --- a/arch/sh/kernel/ioport.c
> +++ b/arch/sh/kernel/ioport.c
> @@ -12,15 +12,6 @@
>  unsigned long sh_io_port_base __read_mostly = -1;
>  EXPORT_SYMBOL(sh_io_port_base);
>  
> -void __iomem *__ioport_map(unsigned long addr, unsigned int size)
> -{
> -	if (sh_mv.mv_ioport_map)
> -		return sh_mv.mv_ioport_map(addr, size);
> -
> -	return (void __iomem *)(addr + sh_io_port_base);
> -}
> -EXPORT_SYMBOL(__ioport_map);
> -
>  void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  {
>  	void __iomem *ret;
> @@ -29,13 +20,11 @@ void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  	if (ret)
>  		return ret;
>  
> -	return __ioport_map(port, nr);
> +	return (void __iomem *)(port + sh_io_port_base);
>  }
>  EXPORT_SYMBOL(ioport_map);
>  
>  void ioport_unmap(void __iomem *addr)
>  {
> -	if (sh_mv.mv_ioport_unmap)
> -		sh_mv.mv_ioport_unmap(addr);
>  }
>  EXPORT_SYMBOL(ioport_unmap);

Why aren't you removing the function ioport_unmap(void __iomem *addr) completely
and just turn it into stub? Is it still referenced somewhere?

Adrian
Geert Uytterhoeven Sept. 8, 2023, 10:20 a.m. UTC | #2
Hi Adrian,

On Fri, Sep 8, 2023 at 12:11 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Wed, 2023-08-02 at 20:48 +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > These functions were only used on the microdev
> > board that is now gone, so remove them to simplify
> > the ioport handling.
> >
> > This could be further simplified to use the generic
> > I/O port accessors now.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> >  void ioport_unmap(void __iomem *addr)
> >  {
> > -     if (sh_mv.mv_ioport_unmap)
> > -             sh_mv.mv_ioport_unmap(addr);
> >  }
> >  EXPORT_SYMBOL(ioport_unmap);
>
> Why aren't you removing the function ioport_unmap(void __iomem *addr) completely
> and just turn it into stub? Is it still referenced somewhere?

Because architectures are supposed to implement it (there is no
__weak default).

An alternative would be to provide a dummy static inline, like
e.g. m68k does:

arch/m68k/include/asm/kmap.h:#define ioport_unmap ioport_unmap
arch/m68k/include/asm/kmap.h:static inline void ioport_unmap(void __iomem *p)
arch/m68k/include/asm/kmap.h-{
arch/m68k/include/asm/kmap.h-}

Gr{oetje,eeting}s,

                        Geert
John Paul Adrian Glaubitz Sept. 8, 2023, 10:21 a.m. UTC | #3
Hi Geert!

On Fri, 2023-09-08 at 12:20 +0200, Geert Uytterhoeven wrote:
> > Why aren't you removing the function ioport_unmap(void __iomem *addr) completely
> > and just turn it into stub? Is it still referenced somewhere?
> 
> Because architectures are supposed to implement it (there is no
> __weak default).
> 
> An alternative would be to provide a dummy static inline, like
> e.g. m68k does:
> 
> arch/m68k/include/asm/kmap.h:#define ioport_unmap ioport_unmap
> arch/m68k/include/asm/kmap.h:static inline void ioport_unmap(void __iomem *p)
> arch/m68k/include/asm/kmap.h-{
> arch/m68k/include/asm/kmap.h-}

OK, that explains it. Thanks!

Adrian
John Paul Adrian Glaubitz Sept. 8, 2023, 10:23 a.m. UTC | #4
On Wed, 2023-08-02 at 20:48 +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> These functions were only used on the microdev
> board that is now gone, so remove them to simplify
> the ioport handling.
> 
> This could be further simplified to use the generic
> I/O port accessors now.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/sh/include/asm/io.h      |  4 ++--
>  arch/sh/include/asm/machvec.h |  5 -----
>  arch/sh/kernel/ioport.c       | 13 +------------
>  3 files changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
> index f2f38e9d489ac..ac521f287fa59 100644
> --- a/arch/sh/include/asm/io.h
> +++ b/arch/sh/include/asm/io.h
> @@ -181,7 +181,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port)	\
>  {									\
>  	volatile type *__addr;						\
>  									\
> -	__addr = __ioport_map(port, sizeof(type));			\
> +	__addr = (void __iomem *)sh_io_port_base + port;		\
>  	*__addr = val;							\
>  	slow;								\
>  }									\
> @@ -191,7 +191,7 @@ static inline type pfx##in##bwlq##p(unsigned long port)			\
>  	volatile type *__addr;						\
>  	type __val;							\
>  									\
> -	__addr = __ioport_map(port, sizeof(type));			\
> +	__addr = (void __iomem *)sh_io_port_base + port;		\
>  	__val = *__addr;						\
>  	slow;								\
>  									\
> diff --git a/arch/sh/include/asm/machvec.h b/arch/sh/include/asm/machvec.h
> index 2b4b085e8f219..4e5314b921f19 100644
> --- a/arch/sh/include/asm/machvec.h
> +++ b/arch/sh/include/asm/machvec.h
> @@ -19,11 +19,6 @@ struct sh_machine_vector {
>  	int (*mv_irq_demux)(int irq);
>  	void (*mv_init_irq)(void);
>  
> -#ifdef CONFIG_HAS_IOPORT_MAP
> -	void __iomem *(*mv_ioport_map)(unsigned long port, unsigned int size);
> -	void (*mv_ioport_unmap)(void __iomem *);
> -#endif
> -
>  	int (*mv_clk_init)(void);
>  	int (*mv_mode_pins)(void);
>  
> diff --git a/arch/sh/kernel/ioport.c b/arch/sh/kernel/ioport.c
> index f39446a658bdb..c8aff8a20164d 100644
> --- a/arch/sh/kernel/ioport.c
> +++ b/arch/sh/kernel/ioport.c
> @@ -12,15 +12,6 @@
>  unsigned long sh_io_port_base __read_mostly = -1;
>  EXPORT_SYMBOL(sh_io_port_base);
>  
> -void __iomem *__ioport_map(unsigned long addr, unsigned int size)
> -{
> -	if (sh_mv.mv_ioport_map)
> -		return sh_mv.mv_ioport_map(addr, size);
> -
> -	return (void __iomem *)(addr + sh_io_port_base);
> -}
> -EXPORT_SYMBOL(__ioport_map);
> -
>  void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  {
>  	void __iomem *ret;
> @@ -29,13 +20,11 @@ void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  	if (ret)
>  		return ret;
>  
> -	return __ioport_map(port, nr);
> +	return (void __iomem *)(port + sh_io_port_base);
>  }
>  EXPORT_SYMBOL(ioport_map);
>  
>  void ioport_unmap(void __iomem *addr)
>  {
> -	if (sh_mv.mv_ioport_unmap)
> -		sh_mv.mv_ioport_unmap(addr);
>  }
>  EXPORT_SYMBOL(ioport_unmap);

Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Geert Uytterhoeven Sept. 13, 2023, 12:32 p.m. UTC | #5
Hi Arnd,

On Wed, Aug 2, 2023 at 8:49 PM Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> These functions were only used on the microdev
> board that is now gone, so remove them to simplify
> the ioport handling.
>
> This could be further simplified to use the generic
> I/O port accessors now.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for your patch!

> --- a/arch/sh/include/asm/io.h
> +++ b/arch/sh/include/asm/io.h
> @@ -181,7 +181,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port)  \
>  {                                                                      \
>         volatile type *__addr;                                          \
>                                                                         \
> -       __addr = __ioport_map(port, sizeof(type));                      \
> +       __addr = (void __iomem *)sh_io_port_base + port;                \

Note that this adds unconditional users of sh_io_port_base, while
sh_io_port_base is provided by arch/sh/kernel/ioport.c, which is
currently only built if CONFIG_GENERIC_IOMAP=n.

This is not a problem yet, as the final part to enable GENERIC_IOMAP
on SH never made it upstream.  However, Sato-san's series enables
GENERIC_IOMAP for SH_DEVICE_TREE=y builds, leading to a link failure.

A quick fix would be to always build the relevant parts:

--- a/arch/sh/kernel/Makefile
+++ b/arch/sh/kernel/Makefile
@@ -23,8 +23,8 @@ obj-y := head_32.o debugtraps.o dumpstack.o
                 \

 ifndef CONFIG_GENERIC_IOMAP
 obj-y                          += iomap.o
-obj-$(CONFIG_HAS_IOPORT_MAP)   += ioport.o
 endif
+obj-$(CONFIG_HAS_IOPORT_MAP)   += ioport.o

 obj-y                          += sys_sh32.o
 obj-y                          += cpu/
--- a/arch/sh/kernel/ioport.c
+++ b/arch/sh/kernel/ioport.c
@@ -12,6 +12,7 @@
 unsigned long sh_io_port_base __read_mostly = -1;
 EXPORT_SYMBOL(sh_io_port_base);

+#ifndef CONFIG_GENERIC_IOMAP
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
        void __iomem *ret;
@@ -28,3 +29,4 @@ void ioport_unmap(void __iomem *addr)
 {
 }
 EXPORT_SYMBOL(ioport_unmap);
+#endif /* !CONFIG_GENERIC_IOMAP */

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann Sept. 13, 2023, 2:08 p.m. UTC | #6
On Wed, Sep 13, 2023, at 14:32, Geert Uytterhoeven wrote:
> On Wed, Aug 2, 2023 at 8:49 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> These functions were only used on the microdev
>> board that is now gone, so remove them to simplify
>> the ioport handling.
>>
>> This could be further simplified to use the generic
>> I/O port accessors now.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>> --- a/arch/sh/include/asm/io.h
>> +++ b/arch/sh/include/asm/io.h
>> @@ -181,7 +181,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port)  \
>>  {                                                                      \
>>         volatile type *__addr;                                          \
>>                                                                         \
>> -       __addr = __ioport_map(port, sizeof(type));                      \
>> +       __addr = (void __iomem *)sh_io_port_base + port;                \
>
> Note that this adds unconditional users of sh_io_port_base, while
> sh_io_port_base is provided by arch/sh/kernel/ioport.c, which is
> currently only built if CONFIG_GENERIC_IOMAP=n.
>
> This is not a problem yet, as the final part to enable GENERIC_IOMAP
> on SH never made it upstream.  However, Sato-san's series enables
> GENERIC_IOMAP for SH_DEVICE_TREE=y builds, leading to a link failure.

Do you have a link to that series? I don't understand why you'd
want to enable GENERIC_IOMAP on sh, given that its PIO accesses
are always memory mapped in the end.

Is this needed for the trapped_io CF stuff?

       Arnd
Geert Uytterhoeven Sept. 13, 2023, 2:13 p.m. UTC | #7
Hi Arnd,

On Wed, Sep 13, 2023 at 4:08 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 13, 2023, at 14:32, Geert Uytterhoeven wrote:
> > On Wed, Aug 2, 2023 at 8:49 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >>
> >> These functions were only used on the microdev
> >> board that is now gone, so remove them to simplify
> >> the ioport handling.
> >>
> >> This could be further simplified to use the generic
> >> I/O port accessors now.
> >>
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> >> --- a/arch/sh/include/asm/io.h
> >> +++ b/arch/sh/include/asm/io.h
> >> @@ -181,7 +181,7 @@ static inline void pfx##out##bwlq##p(type val, unsigned long port)  \
> >>  {                                                                      \
> >>         volatile type *__addr;                                          \
> >>                                                                         \
> >> -       __addr = __ioport_map(port, sizeof(type));                      \
> >> +       __addr = (void __iomem *)sh_io_port_base + port;                \
> >
> > Note that this adds unconditional users of sh_io_port_base, while
> > sh_io_port_base is provided by arch/sh/kernel/ioport.c, which is
> > currently only built if CONFIG_GENERIC_IOMAP=n.
> >
> > This is not a problem yet, as the final part to enable GENERIC_IOMAP
> > on SH never made it upstream.  However, Sato-san's series enables
> > GENERIC_IOMAP for SH_DEVICE_TREE=y builds, leading to a link failure.
>
> Do you have a link to that series? I don't understand why you'd
> want to enable GENERIC_IOMAP on sh, given that its PIO accesses
> are always memory mapped in the end.

"[RESEND RFC PATCH 00/12] DeviceTree support for SH7751 based boards."
https://lore.kernel.org/linux-sh/cover.1693444193.git.ysato@users.sourceforge.jp/

In the meantime, there is a v2, which I wasn't aware of when I wrote
my previous email, so perhaps my comment is no longer valid.
"[RFC PATCH v2 00/30] Device Tree support for SH7751 based board"
https://lore.kernel.org/linux-sh/cover.1694596125.git.ysato@users.sourceforge.jp

> Is this needed for the trapped_io CF stuff?

I don't know.

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann Sept. 13, 2023, 2:30 p.m. UTC | #8
On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
> On Wed, Sep 13, 2023 at 4:08 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Sep 13, 2023, at 14:32, Geert Uytterhoeven wrote:
>> > On Wed, Aug 2, 2023 at 8:49 PM Arnd Bergmann <arnd@kernel.org> wrote:
>>
>> Do you have a link to that series? I don't understand why you'd
>> want to enable GENERIC_IOMAP on sh, given that its PIO accesses
>> are always memory mapped in the end.
>
> "[RESEND RFC PATCH 00/12] DeviceTree support for SH7751 based boards."
> https://lore.kernel.org/linux-sh/cover.1693444193.git.ysato@users.sourceforge.jp/

Ok, thanks.

> In the meantime, there is a v2, which I wasn't aware of when I wrote
> my previous email, so perhaps my comment is no longer valid.
> "[RFC PATCH v2 00/30] Device Tree support for SH7751 based board"
> https://lore.kernel.org/linux-sh/cover.1694596125.git.ysato@users.sourceforge.jp

Right, it looks like the GENERIC_IOMAP part if gone from that
series, and I also see that the PCI host bridge does not actually
map the port I/O window. That's usually fine because very few
drivers actually need it, and it also means that there should be
no need for GENERIC_IOMAP or the simpler alternative.

The first version probably only did it accidentally, which is a
common mistake, and I think the ones for hexagon, m68k, and
mips can probably be removed as well with some simplifiations.

x86 and ia64 want GENERIC_IOMAP because they require using
custom instructions for accessing IORESOURCE_IO registers,
but it's not really generic.

    Arnd
John Paul Adrian Glaubitz Sept. 14, 2023, 6:23 a.m. UTC | #9
Hi Arnd!

On Wed, 2023-09-13 at 16:30 +0200, Arnd Bergmann wrote:
> > In the meantime, there is a v2, which I wasn't aware of when I wrote
> > my previous email, so perhaps my comment is no longer valid.
> > "[RFC PATCH v2 00/30] Device Tree support for SH7751 based board"
> > https://lore.kernel.org/linux-sh/cover.1694596125.git.ysato@users.sourceforge.jp
> 
> Right, it looks like the GENERIC_IOMAP part if gone from that
> series, and I also see that the PCI host bridge does not actually
> map the port I/O window. That's usually fine because very few
> drivers actually need it, and it also means that there should be
> no need for GENERIC_IOMAP or the simpler alternative.
> 
> The first version probably only did it accidentally, which is a
> common mistake, and I think the ones for hexagon, m68k, and
> mips can probably be removed as well with some simplifiations.
> 
> x86 and ia64 want GENERIC_IOMAP because they require using
> custom instructions for accessing IORESOURCE_IO registers,
> but it's not really generic.

Would you suggest to apply your series before or after Yoshinori's series to
convert arch/sh to device trees? If you want it to go in before the conversion,
could you rebase your series against v6.6-rc1?

I'm asking because the current version did not apply against v6.5-rc2.

Adrian
Arnd Bergmann Sept. 14, 2023, 3:56 p.m. UTC | #10
On Thu, Sep 14, 2023, at 08:23, John Paul Adrian Glaubitz wrote:

> On Wed, 2023-09-13 at 16:30 +0200, Arnd Bergmann wrote:
>> > In the meantime, there is a v2, which I wasn't aware of when I wrote
>> > my previous email, so perhaps my comment is no longer valid.
>> > "[RFC PATCH v2 00/30] Device Tree support for SH7751 based board"
>> > https://lore.kernel.org/linux-sh/cover.1694596125.git.ysato@users.sourceforge.jp
>> 
>> Right, it looks like the GENERIC_IOMAP part if gone from that
>> series, and I also see that the PCI host bridge does not actually
>> map the port I/O window. That's usually fine because very few
>> drivers actually need it, and it also means that there should be
>> no need for GENERIC_IOMAP or the simpler alternative.
>> 
>> The first version probably only did it accidentally, which is a
>> common mistake, and I think the ones for hexagon, m68k, and
>> mips can probably be removed as well with some simplifiations.
>> 
>> x86 and ia64 want GENERIC_IOMAP because they require using
>> custom instructions for accessing IORESOURCE_IO registers,
>> but it's not really generic.
>
> Would you suggest to apply your series before or after Yoshinori's series to
> convert arch/sh to device trees? If you want it to go in before the conversion,
> could you rebase your series against v6.6-rc1?
>
> I'm asking because the current version did not apply against v6.5-rc2.

I've rebased and resent the series now, with no other changes
added in. I expect that you can apply it in either order now
if the other series no longer relies on GENERIC_IOMAP.

    Arnd
Geert Uytterhoeven Sept. 15, 2023, 3:41 p.m. UTC | #11
Hi Arnd,

On Wed, Sep 13, 2023 at 4:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
> > On Wed, Sep 13, 2023 at 4:08 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Wed, Sep 13, 2023, at 14:32, Geert Uytterhoeven wrote:
> >> > On Wed, Aug 2, 2023 at 8:49 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >>
> >> Do you have a link to that series? I don't understand why you'd
> >> want to enable GENERIC_IOMAP on sh, given that its PIO accesses
> >> are always memory mapped in the end.
> >
> > "[RESEND RFC PATCH 00/12] DeviceTree support for SH7751 based boards."
> > https://lore.kernel.org/linux-sh/cover.1693444193.git.ysato@users.sourceforge.jp/
>
> Ok, thanks.
>
> > In the meantime, there is a v2, which I wasn't aware of when I wrote
> > my previous email, so perhaps my comment is no longer valid.
> > "[RFC PATCH v2 00/30] Device Tree support for SH7751 based board"
> > https://lore.kernel.org/linux-sh/cover.1694596125.git.ysato@users.sourceforge.jp
>
> Right, it looks like the GENERIC_IOMAP part if gone from that
> series, and I also see that the PCI host bridge does not actually

No, 02/30 still enables it.

> map the port I/O window. That's usually fine because very few
> drivers actually need it, and it also means that there should be
> no need for GENERIC_IOMAP or the simpler alternative.
>
> The first version probably only did it accidentally, which is a
> common mistake, and I think the ones for hexagon, m68k, and
> mips can probably be removed as well with some simplifiations.

When not selecting GENERIC_IOMAP in v2, the build fails with:

sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iomap_release':
devres.c:(.text+0x234): undefined reference to `pci_iounmap'
sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iounmap':
devres.c:(.text+0x278): undefined reference to `pci_iounmap'
sh4-linux-gnu-ld: drivers/pci/quirks.o: in function `disable_igfx_irq':
quirks.c:(.text+0x1738): undefined reference to `pci_iounmap'
sh4-linux-gnu-ld: drivers/pci/quirks.o: in function
`quirk_switchtec_ntb_dma_alias':
quirks.c:(.text+0x1a04): undefined reference to `pci_iounmap'
sh4-linux-gnu-ld: drivers/pci/quirks.o: in function `reset_hinic_vf_dev':
quirks.c:(.text+0x2260): undefined reference to `pci_iounmap'

So I'm back to building the part of arch/sh/kernel/ioport.c
that provides sh_io_port_base...

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
Arnd Bergmann Sept. 15, 2023, 3:49 p.m. UTC | #12
On Fri, Sep 15, 2023, at 17:41, Geert Uytterhoeven wrote:
> On Wed, Sep 13, 2023 at 4:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
>>
>> Right, it looks like the GENERIC_IOMAP part if gone from that
>> series, and I also see that the PCI host bridge does not actually
>
> No, 02/30 still enables it.

Ok.

>> map the port I/O window. That's usually fine because very few
>> drivers actually need it, and it also means that there should be
>> no need for GENERIC_IOMAP or the simpler alternative.
>>
>> The first version probably only did it accidentally, which is a
>> common mistake, and I think the ones for hexagon, m68k, and
>> mips can probably be removed as well with some simplifiations.
>
> When not selecting GENERIC_IOMAP in v2, the build fails with:
>
> sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iomap_release':
> devres.c:(.text+0x234): undefined reference to `pci_iounmap'

Odd, that one is provided based on CONFIG_GENERIC_PCI_IOMAP
and should be provided by common code, despite the similar
naming this is unrelated to CONFIG_GENERIC_IOMAP.

    Arnd
John Paul Adrian Glaubitz Sept. 21, 2023, 7:45 a.m. UTC | #13
Hi!

On Fri, 2023-09-15 at 17:49 +0200, Arnd Bergmann wrote:
> On Fri, Sep 15, 2023, at 17:41, Geert Uytterhoeven wrote:
> > On Wed, Sep 13, 2023 at 4:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
> > > 
> > > Right, it looks like the GENERIC_IOMAP part if gone from that
> > > series, and I also see that the PCI host bridge does not actually
> > 
> > No, 02/30 still enables it.
> 
> Ok.
> 
> > > map the port I/O window. That's usually fine because very few
> > > drivers actually need it, and it also means that there should be
> > > no need for GENERIC_IOMAP or the simpler alternative.
> > > 
> > > The first version probably only did it accidentally, which is a
> > > common mistake, and I think the ones for hexagon, m68k, and
> > > mips can probably be removed as well with some simplifiations.
> > 
> > When not selecting GENERIC_IOMAP in v2, the build fails with:
> > 
> > sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iomap_release':
> > devres.c:(.text+0x234): undefined reference to `pci_iounmap'
> 
> Odd, that one is provided based on CONFIG_GENERIC_PCI_IOMAP
> and should be provided by common code, despite the similar
> naming this is unrelated to CONFIG_GENERIC_IOMAP.

So, what would be the suggestion now to move forward? Shall I include this
series for 6.7 or better wait until after Yoshinori's series to convert
to device tree has been merged?

Adrian
Geert Uytterhoeven Sept. 21, 2023, 8:52 a.m. UTC | #14
Hi Adrian,

On Thu, Sep 21, 2023 at 9:45 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Fri, 2023-09-15 at 17:49 +0200, Arnd Bergmann wrote:
> > On Fri, Sep 15, 2023, at 17:41, Geert Uytterhoeven wrote:
> > > On Wed, Sep 13, 2023 at 4:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
> > > >
> > > > Right, it looks like the GENERIC_IOMAP part if gone from that
> > > > series, and I also see that the PCI host bridge does not actually
> > >
> > > No, 02/30 still enables it.
> >
> > Ok.
> >
> > > > map the port I/O window. That's usually fine because very few
> > > > drivers actually need it, and it also means that there should be
> > > > no need for GENERIC_IOMAP or the simpler alternative.
> > > >
> > > > The first version probably only did it accidentally, which is a
> > > > common mistake, and I think the ones for hexagon, m68k, and
> > > > mips can probably be removed as well with some simplifiations.
> > >
> > > When not selecting GENERIC_IOMAP in v2, the build fails with:
> > >
> > > sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iomap_release':
> > > devres.c:(.text+0x234): undefined reference to `pci_iounmap'
> >
> > Odd, that one is provided based on CONFIG_GENERIC_PCI_IOMAP
> > and should be provided by common code, despite the similar
> > naming this is unrelated to CONFIG_GENERIC_IOMAP.
>
> So, what would be the suggestion now to move forward? Shall I include this
> series for 6.7 or better wait until after Yoshinori's series to convert
> to device tree has been merged?

I think including Arnd's cleanups (that is, his v2) in v6.7 is fine.
Sato-san's series needs more work, and is easy to fix for Arnd's cleanup
(just provide sh_io_port_base unconditionally).

Gr{oetje,eeting}s,

                        Geert
Yoshinori Sato Sept. 25, 2023, 7:08 a.m. UTC | #15
On Thu, 21 Sep 2023 17:52:29 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Adrian,
> 
> On Thu, Sep 21, 2023 at 9:45 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> > On Fri, 2023-09-15 at 17:49 +0200, Arnd Bergmann wrote:
> > > On Fri, Sep 15, 2023, at 17:41, Geert Uytterhoeven wrote:
> > > > On Wed, Sep 13, 2023 at 4:30 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > On Wed, Sep 13, 2023, at 16:13, Geert Uytterhoeven wrote:
> > > > >
> > > > > Right, it looks like the GENERIC_IOMAP part if gone from that
> > > > > series, and I also see that the PCI host bridge does not actually
> > > >
> > > > No, 02/30 still enables it.
> > >
> > > Ok.
> > >
> > > > > map the port I/O window. That's usually fine because very few
> > > > > drivers actually need it, and it also means that there should be
> > > > > no need for GENERIC_IOMAP or the simpler alternative.
> > > > >
> > > > > The first version probably only did it accidentally, which is a
> > > > > common mistake, and I think the ones for hexagon, m68k, and
> > > > > mips can probably be removed as well with some simplifiations.
> > > >
> > > > When not selecting GENERIC_IOMAP in v2, the build fails with:
> > > >
> > > > sh4-linux-gnu-ld: lib/devres.o: in function `pcim_iomap_release':
> > > > devres.c:(.text+0x234): undefined reference to `pci_iounmap'
> > >
> > > Odd, that one is provided based on CONFIG_GENERIC_PCI_IOMAP
> > > and should be provided by common code, despite the similar
> > > naming this is unrelated to CONFIG_GENERIC_IOMAP.
> >
> > So, what would be the suggestion now to move forward? Shall I include this
> > series for 6.7 or better wait until after Yoshinori's series to convert
> > to device tree has been merged?
> 
> I think including Arnd's cleanups (that is, his v2) in v6.7 is fine.
> Sato-san's series needs more work, and is easy to fix for Arnd's cleanup
> (just provide sh_io_port_base unconditionally).

For devicetree support, we have been using GENERIC_IOMAP and GENERIC_PCI_IOMAP.
This change has no effect, so it's okay to be merged first.

> 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
Arnd Bergmann Sept. 25, 2023, 7:36 a.m. UTC | #16
On Mon, Sep 25, 2023, at 09:08, Yoshinori Sato wrote:
> On Thu, 21 Sep 2023 17:52:29 +0900, Geert Uytterhoeven wrote:
>> On Thu, Sep 21, 2023 at 9:45 AM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
>> 
>> I think including Arnd's cleanups (that is, his v2) in v6.7 is fine.
>> Sato-san's series needs more work, and is easy to fix for Arnd's cleanup
>> (just provide sh_io_port_base unconditionally).
>
> For devicetree support, we have been using GENERIC_IOMAP and GENERIC_PCI_IOMAP.
> This change has no effect, so it's okay to be merged first.

Ok, thanks for confirming.

I would still suggest that you try to avoid GENERIC_IOMAP altogether
since sh has no custom inb/outb instructions and can just implement
ioport_map() to return an __iomem token that can be passed into 
readb/writeb, the same as the non-GENERIC_IOMAP version in sh does.

Ideally you can however remove the iomap.c file and the
asm-generic/iomap.h include and instead get the inline definitions
from the defaults in include/asm-generic/io.h.

    Arnd
John Paul Adrian Glaubitz Sept. 26, 2023, 10:15 p.m. UTC | #17
Hi Yoshinori!

On Mon, 2023-09-25 at 16:08 +0900, Yoshinori Sato wrote:
> > I think including Arnd's cleanups (that is, his v2) in v6.7 is fine.
> > Sato-san's series needs more work, and is easy to fix for Arnd's cleanup
> > (just provide sh_io_port_base unconditionally).
> 
> For devicetree support, we have been using GENERIC_IOMAP and GENERIC_PCI_IOMAP.
> This change has no effect, so it's okay to be merged first.

Thanks for the confirmation. I will proceed to review and pick up Arnd's patches
later this week. I am currently on a business trip and therefore a bit restricted
on the time I can spend on kernel work.

Adrian
diff mbox series

Patch

diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
index f2f38e9d489ac..ac521f287fa59 100644
--- a/arch/sh/include/asm/io.h
+++ b/arch/sh/include/asm/io.h
@@ -181,7 +181,7 @@  static inline void pfx##out##bwlq##p(type val, unsigned long port)	\
 {									\
 	volatile type *__addr;						\
 									\
-	__addr = __ioport_map(port, sizeof(type));			\
+	__addr = (void __iomem *)sh_io_port_base + port;		\
 	*__addr = val;							\
 	slow;								\
 }									\
@@ -191,7 +191,7 @@  static inline type pfx##in##bwlq##p(unsigned long port)			\
 	volatile type *__addr;						\
 	type __val;							\
 									\
-	__addr = __ioport_map(port, sizeof(type));			\
+	__addr = (void __iomem *)sh_io_port_base + port;		\
 	__val = *__addr;						\
 	slow;								\
 									\
diff --git a/arch/sh/include/asm/machvec.h b/arch/sh/include/asm/machvec.h
index 2b4b085e8f219..4e5314b921f19 100644
--- a/arch/sh/include/asm/machvec.h
+++ b/arch/sh/include/asm/machvec.h
@@ -19,11 +19,6 @@  struct sh_machine_vector {
 	int (*mv_irq_demux)(int irq);
 	void (*mv_init_irq)(void);
 
-#ifdef CONFIG_HAS_IOPORT_MAP
-	void __iomem *(*mv_ioport_map)(unsigned long port, unsigned int size);
-	void (*mv_ioport_unmap)(void __iomem *);
-#endif
-
 	int (*mv_clk_init)(void);
 	int (*mv_mode_pins)(void);
 
diff --git a/arch/sh/kernel/ioport.c b/arch/sh/kernel/ioport.c
index f39446a658bdb..c8aff8a20164d 100644
--- a/arch/sh/kernel/ioport.c
+++ b/arch/sh/kernel/ioport.c
@@ -12,15 +12,6 @@ 
 unsigned long sh_io_port_base __read_mostly = -1;
 EXPORT_SYMBOL(sh_io_port_base);
 
-void __iomem *__ioport_map(unsigned long addr, unsigned int size)
-{
-	if (sh_mv.mv_ioport_map)
-		return sh_mv.mv_ioport_map(addr, size);
-
-	return (void __iomem *)(addr + sh_io_port_base);
-}
-EXPORT_SYMBOL(__ioport_map);
-
 void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
 	void __iomem *ret;
@@ -29,13 +20,11 @@  void __iomem *ioport_map(unsigned long port, unsigned int nr)
 	if (ret)
 		return ret;
 
-	return __ioport_map(port, nr);
+	return (void __iomem *)(port + sh_io_port_base);
 }
 EXPORT_SYMBOL(ioport_map);
 
 void ioport_unmap(void __iomem *addr)
 {
-	if (sh_mv.mv_ioport_unmap)
-		sh_mv.mv_ioport_unmap(addr);
 }
 EXPORT_SYMBOL(ioport_unmap);