Re: [PATCH] m68k/mac: Replace macide driver with generic platform driver

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

On 1/06/21 8:43 pm, Geert Uytterhoeven wrote:
>
>> Untested (and mangled formatting, sorry):
>>
>>> diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
>>> index d41fa48..aaae12f 100644
>>> --- a/arch/m68k/include/asm/io_mm.h
>>> +++ b/arch/m68k/include/asm/io_mm.h
>>> @@ -135,7 +135,10 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>>       case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>>>   #endif
>>>   #ifdef CONFIG_ATARI_ROM_ISA
>>> -    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
>>>   #endif
> Bogus hunk?

Copy&paste error, more likely. What I have is:

@@ -135,7 +135,10 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
  #endif
  #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+                               return (u8 __iomem *)ENEC_ISA_IO_B(addr);
+                       else
+                               return (u8 __iomem *)(addr);
  #endif
      default: return NULL; /* avoid warnings, just in case */
      }

>>> @@ -344,17 +356,17 @@ static inline void isa_delay(void)
>>>    * ROM port ISA and everything else regular ISA for IDE. read,write defined
>>>    * below.
>>>    */
>>> -#define inb(port)      ((port) < 1024 ? isa_rom_inb(port) : in_8(port))
>>> -#define inb_p(port)    ((port) < 1024 ? isa_rom_inb_p(port) : in_8(port))
>>> -#define inw(port)      ((port) < 1024 ? isa_rom_inw(port) : in_le16(port))
>>> -#define inw_p(port)    ((port) < 1024 ? isa_rom_inw_p(port) : in_le16(port))
>>> +#define inb(port)      ((port) < 1024 ? isa_rom_inb(port) : isa_inb(port))
>>> +#define inb_p(port)    ((port) < 1024 ? isa_rom_inb_p(port) : isa_inb_p(port))
>>> +#define inw(port)      ((port) < 1024 ? isa_rom_inw(port) : isa_inw(port))
>>> +#define inw_p(port)    ((port) < 1024 ? isa_rom_inw_p(port) : isa_inw_p(port))


Turns out this still won't work on Q40 - the IDE port addresses are 
0x1f0 and 0x170 there. This should do:

#define inb(port)       (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? 
isa_rom_inb(port) : isa_inb(port))
#define inb_p(port)     (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? 
isa_rom_inb_p(port) : isa_inb_p(port))
#define inw(port)       (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? 
isa_rom_inw(port) : isa_inw(port))
#define inw_p(port)     (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? 
isa_rom_inw_p(port) : isa_inw_p(port))

>> Could that have any new side effects on multiplatform kernels (other
> Sorry, being an Amiga MMIO user, I don't know my way through the
> maze of ISA I/O accessors...
'Maze' is putting it politely indeed.
>
>> than addresses < 1024 still mangled by wrong translation and IO
>> primitive on non-Atari platforms)?
> Hence q40ide is still broken on Q40 in multi-platform kernels?

That's what I _think_.

Finn found out that Atari IDE is broken if setting CONFIG_IDE only but 
not CONFIG_ATARI_ROM_ISA. It's hard to test Q40 these days, but I am 
quite sure that as soon as CONFIG_ATARI_ROM_ISA is defined, Q40 IDE 
register access does not use address translation, and will fail.

I blame that on using isa_inb() over in_8(), but I may be wrong and it's 
isa_readb() vs  in_8() instead (m68k IDE drivers should use MMIO I/O 
accessors after all). In that case, we must stop defining the special 
readb()/writeb() and readw()/writew() in the CONFIG_ATARI_ROM_ISA 
section, and use generic CONFIG_ISA variants instead.

That would require a different default address translation to catch the 
!CONFIG_ATARI_ROM_ISA case on Atari though (i.e. return (addr), not 
NULL). Not sure this will much improve the stability of the code.

I'll give this a spin on ARAnyM to explore the ramifications.

Cheers,

     Michael