m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals
diff mbox series

Message ID 20190429081937.7544-1-geert@linux-m68k.org
State New, archived
Headers show
Series
  • m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals
Related show

Commit Message

Geert Uytterhoeven April 29, 2019, 8:19 a.m. UTC
The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
assume that the {read,write}[wl]() I/O accessors always use little
endian accesses, and swap the result.

However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
special, in that they use native big endian instead of little endian for
accesses to the on-SoC peripheral block, thus violating the assumption.

Fix this by providing our own variants, using the raw accessors,
reinstating the old behavior.  This is fine on m68k, as no special
barriers are needed, and also avoids swapping data twice.

Reported-by: Angelo Dureghello <angelo@sysam.it>
Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
This can be reverted later, after this oddity of the Coldfire I/O
support has been fixed, and drivers have been updated.
---
 arch/m68k/include/asm/io.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Arnd Bergmann April 29, 2019, 12:39 p.m. UTC | #1
On Mon, Apr 29, 2019 at 10:19 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
> assume that the {read,write}[wl]() I/O accessors always use little
> endian accesses, and swap the result.
>
> However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
> special, in that they use native big endian instead of little endian for
> accesses to the on-SoC peripheral block, thus violating the assumption.
>
> Fix this by providing our own variants, using the raw accessors,
> reinstating the old behavior.  This is fine on m68k, as no special
> barriers are needed, and also avoids swapping data twice.
>
> Reported-by: Angelo Dureghello <angelo@sysam.it>
> Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> This can be reverted later, after this oddity of the Coldfire I/O
> support has been fixed, and drivers have been updated.
> ---
>  arch/m68k/include/asm/io.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
> index aabe6420ead2a599..d47e7384681ab1cd 100644
> --- a/arch/m68k/include/asm/io.h
> +++ b/arch/m68k/include/asm/io.h
> @@ -8,6 +8,12 @@
>  #include <asm/io_mm.h>
>  #endif
>
> +#define mmio_read16be(addr)            __raw_readw(addr)
> +#define mmio_read32be(addr)            __raw_readl(addr)
> +
> +#define mmio_write16be(val, port)      __raw_writew((val), (port))
> +#define mmio_write32be(val, port)      __raw_writel((val), (port))
> +
>  #include <asm-generic/io.h>

This looks correct to me, but there are two points that stick out to me:

- why do you only do this for mmio and not for pio?
- why do you even use the generic_iomap wrappers rather than
  the trivial asm-generic versions of those functions?

       Arnd
Angelo Dureghello May 11, 2019, 10:55 a.m. UTC | #2
Hi,

On Mon, Apr 29, 2019 at 02:39:47PM +0200, Arnd Bergmann wrote:
> On Mon, Apr 29, 2019 at 10:19 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> >
> > The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
> > assume that the {read,write}[wl]() I/O accessors always use little
> > endian accesses, and swap the result.
> >
> > However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
> > special, in that they use native big endian instead of little endian for
> > accesses to the on-SoC peripheral block, thus violating the assumption.
> >
> > Fix this by providing our own variants, using the raw accessors,
> > reinstating the old behavior.  This is fine on m68k, as no special
> > barriers are needed, and also avoids swapping data twice.
> >
> > Reported-by: Angelo Dureghello <angelo@sysam.it>
> > Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > This can be reverted later, after this oddity of the Coldfire I/O
> > support has been fixed, and drivers have been updated.
> > ---
> >  arch/m68k/include/asm/io.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
> > index aabe6420ead2a599..d47e7384681ab1cd 100644
> > --- a/arch/m68k/include/asm/io.h
> > +++ b/arch/m68k/include/asm/io.h
> > @@ -8,6 +8,12 @@
> >  #include <asm/io_mm.h>
> >  #endif
> >
> > +#define mmio_read16be(addr)            __raw_readw(addr)
> > +#define mmio_read32be(addr)            __raw_readl(addr)
> > +
> > +#define mmio_write16be(val, port)      __raw_writew((val), (port))
> > +#define mmio_write32be(val, port)      __raw_writel((val), (port))
> > +
> >  #include <asm-generic/io.h>
> 
> This looks correct to me, but there are two points that stick out to me:
> 
> - why do you only do this for mmio and not for pio?
> - why do you even use the generic_iomap wrappers rather than
>   the trivial asm-generic versions of those functions?
> 
>        Arnd

At least i applied it and it works properly, dspi / dma drivers are
back working.

Thanks, 

Regards
Angelo
Angelo Dureghello June 3, 2019, 12:26 p.m. UTC | #3
Hi Greg (and all),

couldn't seen any follow up on this patch. I tested it and at least
for mcf5441x it works properly and solves all issues.

Do you think it may be accepted as an initial fix ?

Regards,
Angelo

On Mon, Apr 29, 2019 at 10:19:37AM +0200, Geert Uytterhoeven wrote:
> The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
> assume that the {read,write}[wl]() I/O accessors always use little
> endian accesses, and swap the result.
> 
> However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
> special, in that they use native big endian instead of little endian for
> accesses to the on-SoC peripheral block, thus violating the assumption.
> 
> Fix this by providing our own variants, using the raw accessors,
> reinstating the old behavior.  This is fine on m68k, as no special
> barriers are needed, and also avoids swapping data twice.
> 
> Reported-by: Angelo Dureghello <angelo@sysam.it>
> Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> This can be reverted later, after this oddity of the Coldfire I/O
> support has been fixed, and drivers have been updated.
> ---
>  arch/m68k/include/asm/io.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
> index aabe6420ead2a599..d47e7384681ab1cd 100644
> --- a/arch/m68k/include/asm/io.h
> +++ b/arch/m68k/include/asm/io.h
> @@ -8,6 +8,12 @@
>  #include <asm/io_mm.h>
>  #endif
>  
> +#define mmio_read16be(addr)		__raw_readw(addr)
> +#define mmio_read32be(addr)		__raw_readl(addr)
> +
> +#define mmio_write16be(val, port)	__raw_writew((val), (port))
> +#define mmio_write32be(val, port)	__raw_writel((val), (port))
> +
>  #include <asm-generic/io.h>
>  
>  #endif /* _M68K_IO_H */
> -- 
> 2.17.1
>
Greg Ungerer June 4, 2019, 7:18 a.m. UTC | #4
Hi Angelo,

On 3/6/19 10:26 pm, Angelo Dureghello wrote:
> couldn't seen any follow up on this patch. I tested it and at least
> for mcf5441x it works properly and solves all issues.
> 
> Do you think it may be accepted as an initial fix ?

I'll add it to the m68knommu git tree.
Seeing as you wrote it Geert I assume you have no problem with that?  :-)

Regards
Greg


> On Mon, Apr 29, 2019 at 10:19:37AM +0200, Geert Uytterhoeven wrote:
>> The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
>> assume that the {read,write}[wl]() I/O accessors always use little
>> endian accesses, and swap the result.
>>
>> However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
>> special, in that they use native big endian instead of little endian for
>> accesses to the on-SoC peripheral block, thus violating the assumption.
>>
>> Fix this by providing our own variants, using the raw accessors,
>> reinstating the old behavior.  This is fine on m68k, as no special
>> barriers are needed, and also avoids swapping data twice.
>>
>> Reported-by: Angelo Dureghello <angelo@sysam.it>
>> Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> ---
>> This can be reverted later, after this oddity of the Coldfire I/O
>> support has been fixed, and drivers have been updated.
>> ---
>>   arch/m68k/include/asm/io.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
>> index aabe6420ead2a599..d47e7384681ab1cd 100644
>> --- a/arch/m68k/include/asm/io.h
>> +++ b/arch/m68k/include/asm/io.h
>> @@ -8,6 +8,12 @@
>>   #include <asm/io_mm.h>
>>   #endif
>>   
>> +#define mmio_read16be(addr)		__raw_readw(addr)
>> +#define mmio_read32be(addr)		__raw_readl(addr)
>> +
>> +#define mmio_write16be(val, port)	__raw_writew((val), (port))
>> +#define mmio_write32be(val, port)	__raw_writel((val), (port))
>> +
>>   #include <asm-generic/io.h>
>>   
>>   #endif /* _M68K_IO_H */
>> -- 
>> 2.17.1
>>
>
Geert Uytterhoeven June 4, 2019, 7:34 a.m. UTC | #5
Hi Greg,

On Tue, Jun 4, 2019 at 9:18 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 3/6/19 10:26 pm, Angelo Dureghello wrote:
> > couldn't seen any follow up on this patch. I tested it and at least
> > for mcf5441x it works properly and solves all issues.
> >
> > Do you think it may be accepted as an initial fix ?
>
> I'll add it to the m68knommu git tree.
> Seeing as you wrote it Geert I assume you have no problem with that?  :-)

Actually I wanted to look into the issues pointed out by Arnd, but didn't
get to that yet...

Gr{oetje,eeting}s,

                        Geert
Greg Ungerer June 5, 2019, 12:52 p.m. UTC | #6
Hi Geert,

On 4/6/19 5:34 pm, Geert Uytterhoeven wrote:
> On Tue, Jun 4, 2019 at 9:18 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 3/6/19 10:26 pm, Angelo Dureghello wrote:
>>> couldn't seen any follow up on this patch. I tested it and at least
>>> for mcf5441x it works properly and solves all issues.
>>>
>>> Do you think it may be accepted as an initial fix ?
>>
>> I'll add it to the m68knommu git tree.
>> Seeing as you wrote it Geert I assume you have no problem with that?  :-)
> 
> Actually I wanted to look into the issues pointed out by Arnd, but didn't
> get to that yet...

Ok, no worries. I won't do anything with this right now then.

Regards
Greg
Geert Uytterhoeven June 14, 2019, 2:49 p.m. UTC | #7
Hi Arnd,

On Mon, Apr 29, 2019 at 2:40 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Apr 29, 2019 at 10:19 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > The generic definitions of mmio_{read,write}{16,32}be() in lib/iomap.c
> > assume that the {read,write}[wl]() I/O accessors always use little
> > endian accesses, and swap the result.
> >
> > However, the Coldfire versions of the {read,write}[wl]() I/O accessors are
> > special, in that they use native big endian instead of little endian for
> > accesses to the on-SoC peripheral block, thus violating the assumption.
> >
> > Fix this by providing our own variants, using the raw accessors,
> > reinstating the old behavior.  This is fine on m68k, as no special
> > barriers are needed, and also avoids swapping data twice.
> >
> > Reported-by: Angelo Dureghello <angelo@sysam.it>
> > Fixes: aecc787c06f4300f ("iomap: Use non-raw io functions for io{read|write}XXbe")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > This can be reverted later, after this oddity of the Coldfire I/O
> > support has been fixed, and drivers have been updated.
> > ---
> >  arch/m68k/include/asm/io.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
> > index aabe6420ead2a599..d47e7384681ab1cd 100644
> > --- a/arch/m68k/include/asm/io.h
> > +++ b/arch/m68k/include/asm/io.h
> > @@ -8,6 +8,12 @@
> >  #include <asm/io_mm.h>
> >  #endif
> >
> > +#define mmio_read16be(addr)            __raw_readw(addr)
> > +#define mmio_read32be(addr)            __raw_readl(addr)
> > +
> > +#define mmio_write16be(val, port)      __raw_writew((val), (port))
> > +#define mmio_write32be(val, port)      __raw_writel((val), (port))
> > +
> >  #include <asm-generic/io.h>
>
> This looks correct to me, but there are two points that stick out to me:
>
> - why do you only do this for mmio and not for pio?

Because no one had a need for it? ;-)

Now seriously, m68k only has MMIO, no PIO.  Any PIO, if used, is for ISA or
PCMCIA I/O accesses, which are little endian.

> - why do you even use the generic_iomap wrappers rather than
>   the trivial asm-generic versions of those functions?

Looking at git history, that was done to fix some link errors, which no
longer seem to happen with the more mature asm-generic infrastructure we
have now.

So probably we can drop the selection of GENERIC_IOMAP and inclusion
of <asm-generic/iomap.h>, after fixing a few compiler warnings like:

    include/asm-generic/io.h: In function ‘ioread8_rep’
    arch/m68k/include/asm/io_mm.h:371:44: warning: passing argument 1
of ‘raw_insb’ discard ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
     #define readsb(port, buf, nr)     raw_insb((port), (u8 *)(buf), (nr))
    arch/m68k/include/asm/raw_io.h:101:50: note: expected ‘volatile u8
*’ {aka ‘volatile unsigned char *’} but argument is of type ‘const
volatile void *’
    static inline void raw_insb(volatile u8 __iomem *port, u8 *buf,
unsigned int len)

and some regression testing, of course.

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann June 17, 2019, 8:23 p.m. UTC | #8
On Fri, Jun 14, 2019 at 4:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Apr 29, 2019 at 2:40 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Apr 29, 2019 at 10:19 AM Geert Uytterhoeven
> > This looks correct to me, but there are two points that stick out to me:
> >
> > - why do you only do this for mmio and not for pio?
>
> Because no one had a need for it? ;-)
>
> Now seriously, m68k only has MMIO, no PIO.  Any PIO, if used, is for ISA or
> PCMCIA I/O accesses, which are little endian.

I suppose the majority of PIO operations are single-byte only,
so endianess doesn't apply at all ;-). You are certainly right
that big-endian PIO access has close to zero uses in the
kernel (I could not find any either).

If you don't have ISA or PCMCIA devices, then using the
trivial readl/writel based wrappers is clearly the easiest way.

If you do have PIO based devices, then defining pio_read16be
etc would save a few bytes in lib/iomap.c.

> > - why do you even use the generic_iomap wrappers rather than
> >   the trivial asm-generic versions of those functions?
>
> Looking at git history, that was done to fix some link errors, which no
> longer seem to happen with the more mature asm-generic infrastructure we
> have now.
>
> So probably we can drop the selection of GENERIC_IOMAP and inclusion
> of <asm-generic/iomap.h>, after fixing a few compiler warnings like:
>
>     include/asm-generic/io.h: In function ‘ioread8_rep’
>     arch/m68k/include/asm/io_mm.h:371:44: warning: passing argument 1
> of ‘raw_insb’ discard ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
>      #define readsb(port, buf, nr)     raw_insb((port), (u8 *)(buf), (nr))
>     arch/m68k/include/asm/raw_io.h:101:50: note: expected ‘volatile u8
> *’ {aka ‘volatile unsigned char *’} but argument is of type ‘const
> volatile void *’
>     static inline void raw_insb(volatile u8 __iomem *port, u8 *buf,
> unsigned int len)

Those should just be missing 'volatile' and 'const' modifiers
on the arguments.

         Arnd

Patch
diff mbox series

diff --git a/arch/m68k/include/asm/io.h b/arch/m68k/include/asm/io.h
index aabe6420ead2a599..d47e7384681ab1cd 100644
--- a/arch/m68k/include/asm/io.h
+++ b/arch/m68k/include/asm/io.h
@@ -8,6 +8,12 @@ 
 #include <asm/io_mm.h>
 #endif
 
+#define mmio_read16be(addr)		__raw_readw(addr)
+#define mmio_read32be(addr)		__raw_readl(addr)
+
+#define mmio_write16be(val, port)	__raw_writew((val), (port))
+#define mmio_write32be(val, port)	__raw_writel((val), (port))
+
 #include <asm-generic/io.h>
 
 #endif /* _M68K_IO_H */