Re: [PATCH 1/7] m68k: coldfire: create IO access functions for internal registers

Greg Ungerer
2026-04-30T11:10:45Z

Hi Arnd, Geert,

On 30/4/26 17:20, Arnd Bergmann wrote:
> On Thu, Apr 30, 2026, at 09:13, Geert Uytterhoeven wrote:
>> On Thu, 30 Apr 2026 at 07:23, Greg Ungerer <gerg@linux-m68k.org> wrote:
>>> --- a/arch/m68k/include/asm/io_no.h
>>> +++ b/arch/m68k/include/asm/io_no.h
>>> @@ -107,6 +107,22 @@ static inline void writel(u32 value, volatile void __iomem *addr)
>>>
>>> #endif /* IOMEMBASE */
>>>
>>> +#if defined(CONFIG_COLDFIRE)
>>> +/*
>>> + * The ColdFire internal peripheral registers are big-endian, so you
>>> + * cannot use the conventional little-endian readb/readw/readl and
>>> + * writeb/writew/writel access functions. Define a family of access
>>> + * functions to give correct endian access that can be used by all
>>> + * architecture code.
>>> + */
>>> +#define mcf_read8 __raw_readb
>>> +#define mcf_read16 __raw_readw
>>> +#define mcf_read32 __raw_readl
>>> +#define mcf_write8 __raw_writeb
>>> +#define mcf_write16 __raw_writew
>>> +#define mcf_write32 __raw_writel
>>
>> Why not call them io{read,write}{8,16be,32be}(), like parisc, powerpc,
>> and sparc, do? Sparc seems to be the closest match:
>> https://elixir.bootlin.com/linux/v7.0.1/source/arch/sparc/include/asm/io_64.h#L439
>
> I think that would be the right choice for introducing these from
> scratch, but we can't easily change the existing code in small steps,
> for two reasons:
>
> - the asm-generic/io.h already provides ioread32be() etc functions,
> and these are used today on some of the coldfire drivers, but by
> accident these have little-endian semantics (since readl() is
> big-endian here), and at least one driver relies on it being that
> way.
>
> - readl32be() really wants an 'void __iomem *' pointer, and we should
> not have architectures that do something different here. The coldfire
> __raw_readl() accepts both pointer and integer (phys_addr_t etc)
> addresses at the moment, and the arch/m68k/coldfire code for nommu
> targets uses that. Changing these to use ioremap() and pass pointers
> would be a much bigger change.

Yes, exactly. I steered clear of reusing anything that already existed.
Especially since the existing broken ones are actually used in drivers.

Regards
Greg