[PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Arnd Bergmann]

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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Angelo Dureghello]

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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Angelo Dureghello]

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
> 

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Greg Ungerer]

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
>>
> 

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Greg Ungerer]

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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH] m68k: io: Fix io{read,write}{16,32}be() for Coldfire peripherals

[Arnd Bergmann]

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