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

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 <[email protected]> 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