[PATCH RFC 0/2] Fix m68k multiplatform ISA support

[PATCH RFC 0/2] Fix m68k multiplatform ISA support

[Michael Schmitz]

Finn Thain found out that our multiplatform support is broken when
attempting to build a kernel with support for both Q40 and Atari (when
setting CONFIG_ISA through CONFIG_Q40, but not CONFIG_ATARI_ROM_ISA).

Inspection of the code in io_mm.h reveals that in that case, ISA I/O
accessors with address translation are used for the Atari IDE driver, where
MMIO accessors with no address translation would have been appropriate.

Setting CONFIG_ATARI_ROM_ISA will fix the issue for Atari, but then force the
use of I/O accessors without address translation for Q40. The code in
io_mm.h is fundamentally broken for kernels that have both Q40 and Atari
support. 

Address this by adding a check for address < 1024 to the Atari address
translation case, changing the default address translation to return the
untranslated address, and changing the CONFIG_ATARI_ROM_ISA section to use
ISA I/O accessors for addresses > 1024 (i.e., those not intended to map to
the ROM port).

Tested on ARAnyM with all combinations of CONFIG_ATARI_ROM_ISA and
CONFIG_ISA. Earlier versions tested on my Falcon hardware, all using
pata_falcon. 

A thorough review in particular in regard to ramifications for Q40 and Amiga
would be welcome. If there is a better way to implement these changes, I'd
appreciate a hint. This is all academic as long as support for Q40 and Atari
in the same kernel image is not needed, of course. 

I suspect patch 2 may not be needed, since the isa_sex variable should always 
be zero? 

Cheers,

   Michael

[PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Current io_mm.h uses address translation and ROM port IO primitives when
port addresses are below 1024, and raw untranslated MMIO IO primitives
else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
m68k machine type a multi-platform kernel runs on. As a consequence,
the Q40 IDE driver in multiplatform kernels cannot work.
Conversely, the Atari IDE driver uses wrong address translation if a
multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.

Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
is ISA_TYPE_ENEC), and change the ISA address translation used for
Atari to a no-op for those addresses.

Switch readb()/writeb() and readw()/writew() to their ISA equivalents
also. Change the address translation functions to return the identity
translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
for kernels where Q40 and Atari are both configured so this can actually
work (isa_type set to Q40 at compile time else).

Signed-off-by: Michael Schmity <schmitzmic@gmail.com>
---
 arch/m68k/include/asm/io_mm.h | 64 +++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index d41fa48..2275e54 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -52,7 +52,11 @@
 #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
 #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
 
+#ifdef CONFIG_ATARI
+#define MULTI_ISA 1
+#else
 #define MULTI_ISA 0
+#endif /* Atari */
 #endif /* Q40 */
 
 #ifdef CONFIG_AMIGA_PCMCIA
@@ -135,9 +139,12 @@ 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 */
+    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u16 __iomem *isa_itw(unsigned long addr)
@@ -151,9 +158,12 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+			else
+				return (u16 __iomem *)(addr);
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u32 __iomem *isa_itl(unsigned long addr)
@@ -177,9 +187,12 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+			else
+				return (u8 __iomem *)(addr);
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u16 __iomem *isa_mtw(unsigned long addr)
@@ -193,9 +206,12 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+			else
+				return (u16 __iomem *)(addr);
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 
@@ -344,31 +360,31 @@ 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_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))
 #define inl		isa_inl
 #define inl_p		isa_inl_p
 
-#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
-#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
-#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
-#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
+#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
+#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
+#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
+#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
 #define outl		isa_outl
 #define outl_p		isa_outl_p
 
-#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
-#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
+#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
+#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
 #define insl			isa_insl
-#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
-#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
+#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
+#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
 #define outsl			isa_outsl
 
-#define readb(addr)		in_8(addr)
-#define writeb(val, addr)	out_8((addr), (val))
-#define readw(addr)		in_le16(addr)
-#define writew(val, addr)	out_le16((addr), (val))
+#define readb   isa_readb
+#define readw   isa_readw
+#define writeb  isa_writeb
+#define writew  isa_writew
 #endif /* CONFIG_ATARI_ROM_ISA */
 
 #define readl(addr)      in_le32(addr)
-- 
2.7.4

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain]

On Wed, 2 Jun 2021, Michael Schmitz wrote:

> Current io_mm.h uses address translation and ROM port IO primitives when
> port addresses are below 1024, and raw untranslated MMIO IO primitives
> else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
> m68k machine type a multi-platform kernel runs on. As a consequence,
> the Q40 IDE driver in multiplatform kernels cannot work.
> Conversely, the Atari IDE driver uses wrong address translation if a
> multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.
> 
> Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
> is ISA_TYPE_ENEC), and change the ISA address translation used for
> Atari to a no-op for those addresses.
> 
> Switch readb()/writeb() and readw()/writew() to their ISA equivalents
> also. Change the address translation functions to return the identity
> translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
> for kernels where Q40 and Atari are both configured so this can actually
> work (isa_type set to Q40 at compile time else).
> 

Thanks, this does fix the problem I had with CONFIG_ATARI && CONFIG_ISA.

> Signed-off-by: Michael Schmity <schmitzmic@gmail.com>

Checkpatch points out a typo here.

Also, I think this should get a 'Fixes' tag so it will be backported.

> ---
>  arch/m68k/include/asm/io_mm.h | 64 +++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
> index d41fa48..2275e54 100644
> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -52,7 +52,11 @@
>  #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
>  #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
>  
> +#ifdef CONFIG_ATARI
> +#define MULTI_ISA 1
> +#else
>  #define MULTI_ISA 0
> +#endif /* Atari */
>  #endif /* Q40 */
>  

I have to wonder whether there is a nice simple definition for MULTI_ISA. 
Such a definition would make this file a lot more easily understood. Maybe 
that flag could be implemented as a Kconfig symbol. I guess that's out of 
scope though.

>  #ifdef CONFIG_AMIGA_PCMCIA
> @@ -135,9 +139,12 @@ 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);

There is some trailing whitespace added here that 'git am' complains 
about.

Also, I think a 'fallthrough' statement would be better than adding a new 
else branch that duplicates the new default case below.

>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
>      }
>  }
>  static inline u16 __iomem *isa_itw(unsigned long addr)
> @@ -151,9 +158,12 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
>      case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
>  #endif
>  #ifdef CONFIG_ATARI_ROM_ISA
> -    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
> +    case ISA_TYPE_ENEC: if (addr < 1024)
> +				return (u16 __iomem *)ENEC_ISA_IO_W(addr);
> +			else
> +				return (u16 __iomem *)(addr);
>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
> +    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */

Same here.

>      }
>  }
>  static inline u32 __iomem *isa_itl(unsigned long addr)
> @@ -177,9 +187,12 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
>      case ISA_TYPE_AG: return (u8 __iomem *)addr;
>  #endif
>  #ifdef CONFIG_ATARI_ROM_ISA
> -    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
> +    case ISA_TYPE_ENEC: if (addr < 1024)
> +				return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
> +			else
> +				return (u8 __iomem *)(addr);
>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */

And here.

>      }
>  }
>  static inline u16 __iomem *isa_mtw(unsigned long addr)
> @@ -193,9 +206,12 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>      case ISA_TYPE_AG: return (u16 __iomem *)addr;
>  #endif
>  #ifdef CONFIG_ATARI_ROM_ISA
> -    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
> +    case ISA_TYPE_ENEC: if (addr < 1024)
> +				return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
> +			else
> +				return (u16 __iomem *)(addr);
>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
> +    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */

And here.

>      }
>  }
>  
> @@ -344,31 +360,31 @@ 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_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))
>  #define inl		isa_inl
>  #define inl_p		isa_inl_p
>  
> -#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
> -#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
> -#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
> -#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
> +#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
> +#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
> +#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
> +#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
>  #define outl		isa_outl
>  #define outl_p		isa_outl_p
>  
> -#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
> -#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
> +#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
> +#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
>  #define insl			isa_insl
> -#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
> -#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
> +#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
> +#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
>  #define outsl			isa_outsl
>  
> -#define readb(addr)		in_8(addr)
> -#define writeb(val, addr)	out_8((addr), (val))
> -#define readw(addr)		in_le16(addr)
> -#define writew(val, addr)	out_le16((addr), (val))
> +#define readb   isa_readb
> +#define readw   isa_readw
> +#define writeb  isa_writeb
> +#define writew  isa_writew
>  #endif /* CONFIG_ATARI_ROM_ISA */
>  
>  #define readl(addr)      in_le32(addr)
> 

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Finn,

thanks for your review!

On 3/06/21 8:23 pm, Finn Thain wrote:
> On Wed, 2 Jun 2021, Michael Schmitz wrote:
>
>> Current io_mm.h uses address translation and ROM port IO primitives when
>> port addresses are below 1024, and raw untranslated MMIO IO primitives
>> else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
>> m68k machine type a multi-platform kernel runs on. As a consequence,
>> the Q40 IDE driver in multiplatform kernels cannot work.
>> Conversely, the Atari IDE driver uses wrong address translation if a
>> multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.
>>
>> Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
>> is ISA_TYPE_ENEC), and change the ISA address translation used for
>> Atari to a no-op for those addresses.
>>
>> Switch readb()/writeb() and readw()/writew() to their ISA equivalents
>> also. Change the address translation functions to return the identity
>> translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
>> for kernels where Q40 and Atari are both configured so this can actually
>> work (isa_type set to Q40 at compile time else).
>>
> Thanks, this does fix the problem I had with CONFIG_ATARI && CONFIG_ISA.
>
>> Signed-off-by: Michael Schmity <schmitzmic@gmail.com>
> Checkpatch points out a typo here.
I noticed :-) Fat-fingered the commit in a hurry (and didn't run 
checkpatch).
>
> Also, I think this should get a 'Fixes' tag so it will be backported.
I wasn't sure what to use as commit ID to be fixed ... Looks like 
84b16b7b0d5c818fadc731a69965dc76dce0c91e is the one to blame. Hmm - I 
thought that code was older than that...
>
>> ---
>>   arch/m68k/include/asm/io_mm.h | 64 +++++++++++++++++++++++++++----------------
>>   1 file changed, 40 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
>> index d41fa48..2275e54 100644
>> --- a/arch/m68k/include/asm/io_mm.h
>> +++ b/arch/m68k/include/asm/io_mm.h
>> @@ -52,7 +52,11 @@
>>   #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
>>   #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
>>   
>> +#ifdef CONFIG_ATARI
>> +#define MULTI_ISA 1
>> +#else
>>   #define MULTI_ISA 0
>> +#endif /* Atari */
>>   #endif /* Q40 */
>>   
> I have to wonder whether there is a nice simple definition for MULTI_ISA.

As I understand it, MULTI_ISA means that different byte orders and/or 
different address translations need to be used in the same kernel, so 
all that cannot be decided at build time.

As long as there is only a single platform that will use this code (ISA 
only used on a single platform, and neither Atari IDE nor EtherNEC 
used), MULTI_ISA is not needed.

If we have Kconfig symbols for 'single platform only', and 
'multi-platform ISA use', that might be shorter to write and easier to 
understand. Geert?

> Such a definition would make this file a lot more easily understood. Maybe
> that flag could be implemented as a Kconfig symbol. I guess that's out of
> scope though.

That Kconfig symbol would best sit in Kconfig.machine but have to be 
aware of definitions in a number of driver Kconfigs. Haven't tried if 
that works. But I agree it is out of scope for now.

The question then becomes - should I move the MULTI_ISA definitions out 
of the Q40 branch and out of the later Amiga Gayle PCMCIA branch to the 
head of the file and add a comment explaining the rationale? Too much 
churn for now?

>
>>   #ifdef CONFIG_AMIGA_PCMCIA
>> @@ -135,9 +139,12 @@ 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);
> There is some trailing whitespace added here that 'git am' complains
> about.
>
> Also, I think a 'fallthrough' statement would be better than adding a new
> else branch that duplicates the new default case below.

I'm still unsure whether changing the default branch for the sake of 
fixing Atari behaviour is a sane idea - I was hoping for comments either 
way.

But if it's changed, you are correct that having the control flow fall 
through instead of taking a redundant else branch is better.

Essentially, changing that hunk to

  #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);

here (and elsewhere below)?

Cheers,

     Michael


>
>>   #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
>>       }
>>   }
>>   static inline u16 __iomem *isa_itw(unsigned long addr)
>> @@ -151,9 +158,12 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
>>       case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
>>   #endif
>>   #ifdef CONFIG_ATARI_ROM_ISA
>> -    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
>> +    case ISA_TYPE_ENEC: if (addr < 1024)
>> +				return (u16 __iomem *)ENEC_ISA_IO_W(addr);
>> +			else
>> +				return (u16 __iomem *)(addr);
>>   #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>> +    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
> Same here.
>
>>       }
>>   }
>>   static inline u32 __iomem *isa_itl(unsigned long addr)
>> @@ -177,9 +187,12 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
>>       case ISA_TYPE_AG: return (u8 __iomem *)addr;
>>   #endif
>>   #ifdef CONFIG_ATARI_ROM_ISA
>> -    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
>> +    case ISA_TYPE_ENEC: if (addr < 1024)
>> +				return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
>> +			else
>> +				return (u8 __iomem *)(addr);
>>   #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
> And here.
>
>>       }
>>   }
>>   static inline u16 __iomem *isa_mtw(unsigned long addr)
>> @@ -193,9 +206,12 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>>       case ISA_TYPE_AG: return (u16 __iomem *)addr;
>>   #endif
>>   #ifdef CONFIG_ATARI_ROM_ISA
>> -    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
>> +    case ISA_TYPE_ENEC: if (addr < 1024)
>> +				return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
>> +			else
>> +				return (u16 __iomem *)(addr);
>>   #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>> +    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
> And here.
>
>>       }
>>   }
>>   
>> @@ -344,31 +360,31 @@ 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_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))
>>   #define inl		isa_inl
>>   #define inl_p		isa_inl_p
>>   
>> -#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
>> -#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
>> -#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
>> -#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
>> +#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
>> +#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
>> +#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
>> +#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
>>   #define outl		isa_outl
>>   #define outl_p		isa_outl_p
>>   
>> -#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
>> -#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
>> +#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
>> +#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
>>   #define insl			isa_insl
>> -#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
>> -#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
>> +#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
>> +#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
>>   #define outsl			isa_outsl
>>   
>> -#define readb(addr)		in_8(addr)
>> -#define writeb(val, addr)	out_8((addr), (val))
>> -#define readw(addr)		in_le16(addr)
>> -#define writew(val, addr)	out_le16((addr), (val))
>> +#define readb   isa_readb
>> +#define readw   isa_readw
>> +#define writeb  isa_writeb
>> +#define writew  isa_writew
>>   #endif /* CONFIG_ATARI_ROM_ISA */
>>   
>>   #define readl(addr)      in_le32(addr)
>>

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain]

On Fri, 4 Jun 2021, Michael Schmitz wrote:

> of the Q40 branch and out of the later Amiga Gayle PCMCIA branch to the 
> head of the file and add a comment explaining the rationale? Too much 
> churn for now?
> 

Right, it could be too much churn for a bug-fix patch.

> > 
> > >   #ifdef CONFIG_AMIGA_PCMCIA
> > > @@ -135,9 +139,12 @@ 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);
> > There is some trailing whitespace added here that 'git am' complains
> > about.
> > 
> > Also, I think a 'fallthrough' statement would be better than adding a new
> > else branch that duplicates the new default case below.
> 
> I'm still unsure whether changing the default branch for the sake of 
> fixing Atari behaviour is a sane idea - I was hoping for comments either 
> way.
> 

You mean, what happens if a random m68k platform (other than amiga, atari 
and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would 
'just work' but it's probably never going to be needed.

> But if it's changed, you are correct that having the control flow fall 
> through instead of taking a redundant else branch is better.
> 
> Essentially, changing that hunk to
> 
>  #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);
> 
> here (and elsewhere below)?
> 

I worry about the static checkers that look for missing 'fallthrough' and 
'break' statements. So I was thinking of something like this:

static inline u8 __iomem *isa_itb(unsigned long addr)
{
  switch(ISA_TYPE)
    {
#ifdef CONFIG_Q40
    case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
    case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
    case ISA_TYPE_ENEC: 
        if (addr < 1024)
            return (u8 __iomem *)ENEC_ISA_IO_B(addr);
        fallthrough;
#endif
    default: return (u8 __iomem *)(addr);
    }
}


Alternatively you could just move the default out of the switch:

static inline u8 __iomem *isa_itb(unsigned long addr)
{
  switch(ISA_TYPE)
    {
#ifdef CONFIG_Q40
    case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
#endif
#ifdef CONFIG_AMIGA_PCMCIA
    case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
#endif
#ifdef CONFIG_ATARI_ROM_ISA
    case ISA_TYPE_ENEC:
        if (addr < 1024)
            return (u8 __iomem *)ENEC_ISA_IO_B(addr);
        break;
#endif
    }
    return (u8 __iomem *)(addr);
}


The latter is probably the more flexible style because 'break' doesn't 
care about the order of case labels.

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Finn,

Am 04.06.2021 um 17:55 schrieb Finn Thain:
> On Fri, 4 Jun 2021, Michael Schmitz wrote:
>
>> of the Q40 branch and out of the later Amiga Gayle PCMCIA branch to the
>> head of the file and add a comment explaining the rationale? Too much
>> churn for now?
>>
>
> Right, it could be too much churn for a bug-fix patch.

I'll leave that for later (but add a comment in the lines inserted).

>>>
>>>>   #ifdef CONFIG_AMIGA_PCMCIA
>>>> @@ -135,9 +139,12 @@ 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);
>>> There is some trailing whitespace added here that 'git am' complains
>>> about.
>>>
>>> Also, I think a 'fallthrough' statement would be better than adding a new
>>> else branch that duplicates the new default case below.
>>
>> I'm still unsure whether changing the default branch for the sake of
>> fixing Atari behaviour is a sane idea - I was hoping for comments either
>> way.
>>
>
> You mean, what happens if a random m68k platform (other than amiga, atari
> and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would
> 'just work' but it's probably never going to be needed.

The NULL default was meant to catch incorrect use of config options 
related to CONFIG_ISA. My repurposing the default branch for Atari's 
needs (no translation for IDE) defeats that. But the chance that we run 
into such incorrect use in the future are pretty slim indeed.

Still, my patch changes behaviour in existing drivers. We're quite 
certain it does not matter, but is that good enough to be accepted?

>
>> But if it's changed, you are correct that having the control flow fall
>> through instead of taking a redundant else branch is better.
>>
>> Essentially, changing that hunk to
>>
>>  #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);
>>
>> here (and elsewhere below)?
>>
>
> I worry about the static checkers that look for missing 'fallthrough' and

Never ran into 'fallthrough' except as comment annotation, but I now see 
that really is a thing these days. Amazing.

> 'break' statements. So I was thinking of something like this:
>
> static inline u8 __iomem *isa_itb(unsigned long addr)
> {
>   switch(ISA_TYPE)
>     {
> #ifdef CONFIG_Q40
>     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
> #endif
> #ifdef CONFIG_AMIGA_PCMCIA
>     case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
> #endif
> #ifdef CONFIG_ATARI_ROM_ISA
>     case ISA_TYPE_ENEC:
>         if (addr < 1024)
>             return (u8 __iomem *)ENEC_ISA_IO_B(addr);
>         fallthrough;
> #endif
>     default: return (u8 __iomem *)(addr);
>     }
> }

This one makes it easy to eventually add another ISA_TYPE_ATARI case 
before the default case (which could then revert to NULL if desired).

>
>
> Alternatively you could just move the default out of the switch:
>
> static inline u8 __iomem *isa_itb(unsigned long addr)
> {
>   switch(ISA_TYPE)
>     {
> #ifdef CONFIG_Q40
>     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
> #endif
> #ifdef CONFIG_AMIGA_PCMCIA
>     case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
> #endif
> #ifdef CONFIG_ATARI_ROM_ISA
>     case ISA_TYPE_ENEC:
>         if (addr < 1024)
>             return (u8 __iomem *)ENEC_ISA_IO_B(addr);
>         break;
> #endif
>     }
>     return (u8 __iomem *)(addr);
> }
>
>
> The latter is probably the more flexible style because 'break' doesn't
> care about the order of case labels.

Yes, but it rules out reverting the default case to NULL.

On balance, I'll go with the fallback (and will annotate that other 
Atari drivers fall back to the clause below on purpose).

Cheers,

	Michael

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Brad Boyer]

On Fri, Jun 04, 2021 at 07:30:00PM +1200, Michael Schmitz wrote:
> >>I'm still unsure whether changing the default branch for the sake of
> >>fixing Atari behaviour is a sane idea - I was hoping for comments either
> >>way.
> >>
> >
> >You mean, what happens if a random m68k platform (other than amiga, atari
> >and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would
> >'just work' but it's probably never going to be needed.
> 
> The NULL default was meant to catch incorrect use of config options related
> to CONFIG_ISA. My repurposing the default branch for Atari's needs (no
> translation for IDE) defeats that. But the chance that we run into such
> incorrect use in the future are pretty slim indeed.

Well, we could in theory add a trex socket driver to get PCMCIA support
for the PowerBook 190 hardware. There was a driver for that in ppc for
the PowerBook 5300 which uses the same chipset. I believe the PCMCIA
drivers use these same macros in spite of not being considered ISA.
I don't see anything in drivers/pcmcia that is obviously an m68k
system even though I'm pretty sure I remember discussions of supporting
such hardware in the past.

Is PCMCIA support something we should also consider in all of this?

	Brad Boyer
	flar@allandria.com

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Brad,

Am 05.06.2021 um 10:49 schrieb Brad Boyer:
> On Fri, Jun 04, 2021 at 07:30:00PM +1200, Michael Schmitz wrote:
>>>> I'm still unsure whether changing the default branch for the sake of
>>>> fixing Atari behaviour is a sane idea - I was hoping for comments either
>>>> way.
>>>>
>>>
>>> You mean, what happens if a random m68k platform (other than amiga, atari
>>> and q40) were to adopt CONFIG_ISA? I guess it would be nice if that would
>>> 'just work' but it's probably never going to be needed.
>>
>> The NULL default was meant to catch incorrect use of config options related
>> to CONFIG_ISA. My repurposing the default branch for Atari's needs (no
>> translation for IDE) defeats that. But the chance that we run into such
>> incorrect use in the future are pretty slim indeed.
>
> Well, we could in theory add a trex socket driver to get PCMCIA support
> for the PowerBook 190 hardware. There was a driver for that in ppc for
> the PowerBook 5300 which uses the same chipset. I believe the PCMCIA
> drivers use these same macros in spite of not being considered ISA.

Correct - the PCMCIA device drivers use IO port addresses in the ISA 
port range.

> I don't see anything in drivers/pcmcia that is obviously an m68k
> system even though I'm pretty sure I remember discussions of supporting
> such hardware in the past.

There's the APNE driver (Amiga PCMCIA NE2000 clone), which is already 
catered for by the current code in io_mm.h. I remember seeing patches 
for that driver that would allow support of a variant of the APNE card 
that were hard to integrate in the current NE clone code framework. 
Didn't consider adding another isa_type for that card at the time - I'll 
revisit these patches if I can find them again.

Supporting PB190 PCMCIA hardware requires adding a new isa_type and the 
corresponding IO translation cases. Not much more, for all I can see. 
Existing chipset drivers from other architectures ought to work already. 
Maybe add a specific block_input() hook as for APNE (but I surmise that 
might just be code duplication from generic code in lib8390.c - didn't 
check).

Not sure what card socket code the APNE driver uses - must be one of the 
generic variants from drivers/pcmcia. If your PB190 needs something not 
already in there, we'd need to add that as well.

> Is PCMCIA support something we should also consider in all of this?

Absolutely.

Cheers,

	Michael


>
> 	Brad Boyer
> 	flar@allandria.com
>

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Brad Boyer]

On Sat, Jun 05, 2021 at 01:41:22PM +1200, Michael Schmitz wrote:
> Am 05.06.2021 um 10:49 schrieb Brad Boyer:
> >I don't see anything in drivers/pcmcia that is obviously an m68k
> >system even though I'm pretty sure I remember discussions of supporting
> >such hardware in the past.
> 
> There's the APNE driver (Amiga PCMCIA NE2000 clone), which is already
> catered for by the current code in io_mm.h. I remember seeing patches for
> that driver that would allow support of a variant of the APNE card that were
> hard to integrate in the current NE clone code framework. Didn't consider
> adding another isa_type for that card at the time - I'll revisit these
> patches if I can find them again.
> 
> Supporting PB190 PCMCIA hardware requires adding a new isa_type and the
> corresponding IO translation cases. Not much more, for all I can see.
> Existing chipset drivers from other architectures ought to work already.
> Maybe add a specific block_input() hook as for APNE (but I surmise that
> might just be code duplication from generic code in lib8390.c - didn't
> check).
> 
> Not sure what card socket code the APNE driver uses - must be one of the
> generic variants from drivers/pcmcia. If your PB190 needs something not
> already in there, we'd need to add that as well.

I had to look a bit, but I found it. The apne driver doesn't use the
normal PCMCIA infrastructure at all. There is a custom Amiga PCMCIA
thing found in arch/m68k/amiga/pcmcia.c. This could complicate things
if we are able to use the common PCMCIA code for trex and try to
build a kernel with both that and amiga/pcmcia + apne.

At least it does sound like the io macros won't be an issue.

	Brad Boyer
	flar@allandria.com

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Brad,

Am 05.06.2021 um 18:04 schrieb Brad Boyer:
>>
>> Not sure what card socket code the APNE driver uses - must be one of the
>> generic variants from drivers/pcmcia. If your PB190 needs something not
>> already in there, we'd need to add that as well.
>
> I had to look a bit, but I found it. The apne driver doesn't use the
> normal PCMCIA infrastructure at all. There is a custom Amiga PCMCIA
> thing found in arch/m68k/amiga/pcmcia.c. This could complicate things
> if we are able to use the common PCMCIA code for trex and try to
> build a kernel with both that and amiga/pcmcia + apne.

Thanks, I had missed that one.

> At least it does sound like the io macros won't be an issue.

The arch/m68k/amiga/pcmcia.c API is different from that of the drivers 
in drivers/pcmcia/ from what I've seen, so I think adding or reusing 
trex support together with Amiga PCMCIA support will be an issue.

Cheers,

	Michael

>
> 	Brad Boyer
> 	flar@allandria.com
>

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Brad,

Am 05.06.2021 um 18:04 schrieb Brad Boyer:
> On Sat, Jun 05, 2021 at 01:41:22PM +1200, Michael Schmitz wrote:
>> Am 05.06.2021 um 10:49 schrieb Brad Boyer:
>>> I don't see anything in drivers/pcmcia that is obviously an m68k
>>> system even though I'm pretty sure I remember discussions of supporting
>>> such hardware in the past.
>>
>> There's the APNE driver (Amiga PCMCIA NE2000 clone), which is already
>> catered for by the current code in io_mm.h. I remember seeing patches for
>> that driver that would allow support of a variant of the APNE card that were
>> hard to integrate in the current NE clone code framework. Didn't consider
>> adding another isa_type for that card at the time - I'll revisit these
>> patches if I can find them again.

Refreshed my memory - Alex submitted a patch to netdev three years ago 
that essentially boiled down to changing our isa_inb() to use isa_inw().

This patch to io_mm.h (on top of my current patch), plus setting 
isa_type to ISA_TPYE_AG100 using a module parameter, should do the trick:

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index f6b487b..6f79a5e 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -102,6 +102,11 @@
  #define ISA_TYPE_AG   (2)
  #define ISA_TYPE_ENEC (3)

+#if defined(CONFIG_AMIGA_PCMCIA_100)
+#define ISA_TYPE_AG100 (4)     /* for 100 MBit APNE card */
+#define MULTI_ISA 1
+#endif
+
  #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
  #define ISA_TYPE ISA_TYPE_Q40
  #define ISA_SEX  0
@@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
  #ifdef CONFIG_Q40
      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
  #endif
+#if defined(CONFIG_AMIGA_PCMCIA_100)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
  #ifdef CONFIG_AMIGA_PCMCIA
      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
  #endif
@@ -153,6 +161,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
  #ifdef CONFIG_Q40
      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
  #endif
+#if defined(CONFIG_AMIGA_PCMCIA_100)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
  #ifdef CONFIG_AMIGA_PCMCIA
      case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
  #endif
@@ -168,6 +179,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
  {
    switch(ISA_TYPE)
      {
+#if defined(CONFIG_AMIGA_PCMCIA_100)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
  #ifdef CONFIG_AMIGA_PCMCIA
      case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
  #endif
@@ -181,6 +195,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
  #ifdef CONFIG_Q40
      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
  #endif
+#if defined(CONFIG_AMIGA_PCMCIA_100)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
  #ifdef CONFIG_AMIGA_PCMCIA
      case ISA_TYPE_AG: return (u8 __iomem *)addr;
  #endif
@@ -199,6 +216,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
  #ifdef CONFIG_Q40
      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
  #endif
+#if defined(CONFIG_AMIGA_PCMCIA_100)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
  #ifdef CONFIG_AMIGA_PCMCIA
      case ISA_TYPE_AG: return (u16 __iomem *)addr;
  #endif
@@ -219,6 +239,11 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : 
out_le16(isa_itw(port),(val)))
  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : 
out_le32(isa_itl(port),(val)))

+#if defined(CONFIG_AMIGA_PCMCIA_100)
+#undef isa_inb
+#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 
? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
+#endif
+
  #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
  #define isa_readw(p)       \
         (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \

(linebreak-mangled, sorry).

The card reset patch hunk from Alex' patch can probably go into the APNE 
driver regardless?

It's been quite a while - can you still try and build/test this change, 
Alex?

Cheers,

	Michael

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain]

On Sun, 6 Jun 2021, Michael Schmitz wrote:

> 
> This patch to io_mm.h (on top of my current patch), plus setting isa_type to
> ISA_TPYE_AG100 using a module parameter, should do the trick:
> 
> diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
> index f6b487b..6f79a5e 100644
> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -102,6 +102,11 @@
>  #define ISA_TYPE_AG   (2)
>  #define ISA_TYPE_ENEC (3)
> 
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +#define ISA_TYPE_AG100 (4)     /* for 100 MBit APNE card */

IMHO this would be simpler if that #define was unconditional like the 
others.

> +#define MULTI_ISA 1

Hmm...

> +#endif
> +
>  #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
>  #define ISA_TYPE ISA_TYPE_Q40
>  #define ISA_SEX  0
> @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>  #ifdef CONFIG_Q40
>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>  #endif
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +    case ISA_TYPE_AG100: fallthrough;
> +#endif

I wonder whether that 'fallthrough' is needed. One would hope not...

>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>  #endif
> @@ -153,6 +161,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
>  #ifdef CONFIG_Q40
>      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
>  #endif
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +    case ISA_TYPE_AG100: fallthrough;
> +#endif
>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
>  #endif
> @@ -168,6 +179,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
>  {
>    switch(ISA_TYPE)
>      {
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +    case ISA_TYPE_AG100: fallthrough;
> +#endif
>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
>  #endif
> @@ -181,6 +195,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
>  #ifdef CONFIG_Q40
>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
>  #endif
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +    case ISA_TYPE_AG100: fallthrough;
> +#endif
>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u8 __iomem *)addr;
>  #endif
> @@ -199,6 +216,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>  #ifdef CONFIG_Q40
>      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
>  #endif
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +    case ISA_TYPE_AG100: fallthrough;
> +#endif
>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u16 __iomem *)addr;
>  #endif
> @@ -219,6 +239,11 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) :
> out_le16(isa_itw(port),(val)))
>  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) :
> out_le32(isa_itl(port),(val)))
> 
> +#if defined(CONFIG_AMIGA_PCMCIA_100)
> +#undef isa_inb
> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ?
> isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
> +#endif
> +

Would (port & ~1) be faster than (port - 1) here?

Also, I don't think you need '#if defined' here. When 
!defined(CONFIG_AMIGA_PCMCIA_100), I think ISA_TYPE should be a 
compile-time constant 0, and the dead code will get tossed out anyway.

>  #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
>  #define isa_readw(p)       \
>         (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \
> 
> (linebreak-mangled, sorry).
> 
> The card reset patch hunk from Alex' patch can probably go into the APNE
> driver regardless?
> 
> It's been quite a while - can you still try and build/test this change, Alex?

Note that isa_type is never assigned to ISA_TYPE_AG100 in 
arch/m68k/kernel/setup_mm.c which means (IIUC) this patch won't take 
effect with MULTI_ISA == 1.

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Finn,

Thanks for the quick review!

Am 06.06.2021 um 16:53 schrieb Finn Thain:
> On Sun, 6 Jun 2021, Michael Schmitz wrote:
>
>>
>> This patch to io_mm.h (on top of my current patch), plus setting isa_type to
>> ISA_TPYE_AG100 using a module parameter, should do the trick:
>>
>> diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
>> index f6b487b..6f79a5e 100644
>> --- a/arch/m68k/include/asm/io_mm.h
>> +++ b/arch/m68k/include/asm/io_mm.h
>> @@ -102,6 +102,11 @@
>>  #define ISA_TYPE_AG   (2)
>>  #define ISA_TYPE_ENEC (3)
>>
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +#define ISA_TYPE_AG100 (4)     /* for 100 MBit APNE card */
>
> IMHO this would be simpler if that #define was unconditional like the
> others.

Yep, that one could be unconditional (and we could rearrange the defs to 
keep the new type between AG and ENEC).

>
>> +#define MULTI_ISA 1
>
> Hmm...
>

That one I didn't want to make unconditional, in order to allow the 
optimization below.

>> +#endif
>> +
>>  #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
>>  #define ISA_TYPE ISA_TYPE_Q40
>>  #define ISA_SEX  0
>> @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>  #ifdef CONFIG_Q40
>>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>>  #endif
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +    case ISA_TYPE_AG100: fallthrough;
>> +#endif
>
> I wonder whether that 'fallthrough' is needed. One would hope not...

It won't be, but it ought to shut up compiler warnings, no?

>
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>>  #endif
>> @@ -153,6 +161,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
>>  #ifdef CONFIG_Q40
>>      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
>>  #endif
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +    case ISA_TYPE_AG100: fallthrough;
>> +#endif
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
>>  #endif
>> @@ -168,6 +179,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
>>  {
>>    switch(ISA_TYPE)
>>      {
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +    case ISA_TYPE_AG100: fallthrough;
>> +#endif
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
>>  #endif
>> @@ -181,6 +195,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
>>  #ifdef CONFIG_Q40
>>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
>>  #endif
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +    case ISA_TYPE_AG100: fallthrough;
>> +#endif
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u8 __iomem *)addr;
>>  #endif
>> @@ -199,6 +216,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>>  #ifdef CONFIG_Q40
>>      case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
>>  #endif
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +    case ISA_TYPE_AG100: fallthrough;
>> +#endif
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u16 __iomem *)addr;
>>  #endif
>> @@ -219,6 +239,11 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>>  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) :
>> out_le16(isa_itw(port),(val)))
>>  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) :
>> out_le32(isa_itl(port),(val)))
>>
>> +#if defined(CONFIG_AMIGA_PCMCIA_100)
>> +#undef isa_inb
>> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ?
>> isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
>> +#endif
>> +
>
> Would (port & ~1) be faster than (port - 1) here?

Perhaps it would - I'd hope the compiler will pick the most efficient 
solution here.

>
> Also, I don't think you need '#if defined' here. When
> !defined(CONFIG_AMIGA_PCMCIA_100), I think ISA_TYPE should be a
> compile-time constant 0, and the dead code will get tossed out anyway.

Right, that's a good point.

>>  #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
>>  #define isa_readw(p)       \
>>         (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \
>>
>> (linebreak-mangled, sorry).
>>
>> The card reset patch hunk from Alex' patch can probably go into the APNE
>> driver regardless?
>>
>> It's been quite a while - can you still try and build/test this change, Alex?
>
> Note that isa_type is never assigned to ISA_TYPE_AG100 in
> arch/m68k/kernel/setup_mm.c which means (IIUC) this patch won't take
> effect with MULTI_ISA == 1.

In order to make this useful, a Kconfig option is needed, and a module 
parameter will set isa_type to ISA_TYPE_AG100 in the APNE probe routine. 
I'll send the complete set as RFC soon. Will include your suggestions 
before doing that, of course.

Cheers,

	Michael



>

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Brad Boyer]

On Sun, Jun 06, 2021 at 05:42:30PM +1200, Michael Schmitz wrote:
> >Would (port & ~1) be faster than (port - 1) here?
> 
> Perhaps it would - I'd hope the compiler will pick the most efficient
> solution here.

I'm pretty sure the compiler couldn't do that optimization. Without
more context, those two statements are not equivalent. I don't see
how the compiler could know that.

Looking at the 68040 manual, the instruction timing of ANDI and SUBQ
is mostly the same (except for immediate addressing, where ANDI is
slower). The ANDI is also going to take up extra bytes in the code
to put the bitmask in an extension word. BCLR can be faster than
ANDI or SUBQ in some cases, but it's slower in others. It also needs
an extension word when using an immediate value for the bit number.
If you presume port is in a data register, then ANDI and SUBQ have
the same timing and BCLR is slower. At that point, the difference
between ANDI and SUBQ is the size of the instruction which would
favor using SUBQ. I haven't checked if other m68k chips have similar
timings.

Unless I missed something, I don't see a better way to do the
equivalent of the and with a constant bitmask.

	Brad Boyer
	flar@allandria.com

[PATCH RFC 0/2] Add APNE PCMCIA 100 Mbit support

[Michael Schmitz]

Now that we've sanitzed io_mm.h, revisit support for the 100 Mbit APNE
PCMCIA card variants that didn't find favour with the netdev folks three
years ago.

Add another ISA type for these cards, and switch to that type from the
APNE probe code in response to a module parameter. 

Entirely untested, just for comment (and hopefully someone will give
this a spin). 

Cheers,

   Michael

[PATCH RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Add code to support 10 Mbit and 100 Mbit mode for APNE driver.

A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
to word access as required by the 100 Mbit cards.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/include/asm/io_mm.h | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index f6b487b..71f694a 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -101,6 +101,11 @@
 #define ISA_TYPE_Q40  (1)
 #define ISA_TYPE_AG   (2)
 #define ISA_TYPE_ENEC (3)
+#define ISA_TYPE_AG100 (4)	/* for 100 MBit APNE card */
+
+#if defined(CONFIG_APNE100MBIT)
+#define MULTI_ISA 1
+#endif
 
 #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
 #define ISA_TYPE ISA_TYPE_Q40
@@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
 #ifdef CONFIG_Q40
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
 #endif
+#if defined(CONFIG_APNE100MBIT)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
 #endif
@@ -153,6 +161,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
 #ifdef CONFIG_Q40
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
 #endif
+#if defined(CONFIG_APNE100MBIT)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
@@ -168,6 +179,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
 {
   switch(ISA_TYPE)
     {
+#if defined(CONFIG_APNE100MBIT)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
 #endif
@@ -181,6 +195,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
 #ifdef CONFIG_Q40
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
 #endif
+#if defined(CONFIG_APNE100MBIT)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
@@ -199,6 +216,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
 #ifdef CONFIG_Q40
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
 #endif
+#if defined(CONFIG_APNE100MBIT)
+    case ISA_TYPE_AG100: fallthrough;
+#endif
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
@@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
 }
 
 
-#define isa_inb(port)      in_8(isa_itb(port))
 #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
 #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
 #define isa_outb(val,port) out_8(isa_itb(port),(val))
 #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
 #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
 
+/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
+ * dead code if MULTI_ISA is not set */
+#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
+
 #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
 #define isa_readw(p)       \
 	(ISA_SEX ? in_be16(isa_mtw((unsigned long)(p)))	\
-- 
2.7.4

Re: [PATCH RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add code to support 10 Mbit and 100 Mbit mode for APNE driver.
>
> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
> to word access as required by the 100 Mbit cards.

missing "dynamically".

>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!

> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -101,6 +101,11 @@
>  #define ISA_TYPE_Q40  (1)
>  #define ISA_TYPE_AG   (2)
>  #define ISA_TYPE_ENEC (3)
> +#define ISA_TYPE_AG100 (4)     /* for 100 MBit APNE card */

As this type may be needed for other cards, perhaps it should be
named after what it really does, i.e. always using 16-bit accesses?
ISA_TYPE_AG16?

> +
> +#if defined(CONFIG_APNE100MBIT)

#ifdef

> +#define MULTI_ISA 1
> +#endif
>
>  #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
>  #define ISA_TYPE ISA_TYPE_Q40
> @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>  #ifdef CONFIG_Q40
>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>  #endif
> +#if defined(CONFIG_APNE100MBIT)

#ifdef

> +    case ISA_TYPE_AG100: fallthrough;

Fallthrough to what? Oh, to ISA_TYPE_AG.
Please make this clear, and safer, by moving this inside the #ifdef
below (CONFIG_APNE100MBIT depends on CONFIG_AMIGA_PCMCIA).

> +#endif
>  #ifdef CONFIG_AMIGA_PCMCIA
>      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>  #endif

> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>  }
>
>
> -#define isa_inb(port)      in_8(isa_itb(port))

Why move it below? Because its comment does not apply to the below?

>  #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
>  #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
>  #define isa_outb(val,port) out_8(isa_itb(port),(val))
>  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
>  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
>
> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
> + * dead code if MULTI_ISA is not set */

I don't think this comment is needed.

> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
> +
>  #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
>  #define isa_readw(p)       \
>         (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \

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 RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Geert,

Thanks for reviewing this one as well!

Am 07.06.2021 um 20:01 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Add code to support 10 Mbit and 100 Mbit mode for APNE driver.
>>
>> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
>> to word access as required by the 100 Mbit cards.
>
> missing "dynamically".

OK...

>
>>
>> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
>> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
>> Kazik <alex@kazik.de>.
>>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Thanks for your patch!
>
>> --- a/arch/m68k/include/asm/io_mm.h
>> +++ b/arch/m68k/include/asm/io_mm.h
>> @@ -101,6 +101,11 @@
>>  #define ISA_TYPE_Q40  (1)
>>  #define ISA_TYPE_AG   (2)
>>  #define ISA_TYPE_ENEC (3)
>> +#define ISA_TYPE_AG100 (4)     /* for 100 MBit APNE card */
>
> As this type may be needed for other cards, perhaps it should be
> named after what it really does, i.e. always using 16-bit accesses?
> ISA_TYPE_AG16?

Yep, that sounds better.

>
>> +
>> +#if defined(CONFIG_APNE100MBIT)
>
> #ifdef
>
>> +#define MULTI_ISA 1
>> +#endif
>>
>>  #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
>>  #define ISA_TYPE ISA_TYPE_Q40
>> @@ -135,6 +140,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>  #ifdef CONFIG_Q40
>>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>>  #endif
>> +#if defined(CONFIG_APNE100MBIT)
>
> #ifdef
>
>> +    case ISA_TYPE_AG100: fallthrough;
>
> Fallthrough to what? Oh, to ISA_TYPE_AG.
> Please make this clear, and safer, by moving this inside the #ifdef
> below (CONFIG_APNE100MBIT depends on CONFIG_AMIGA_PCMCIA).

OK, good point.

>> +#endif
>>  #ifdef CONFIG_AMIGA_PCMCIA
>>      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>>  #endif
>
>> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>>  }
>>
>>
>> -#define isa_inb(port)      in_8(isa_itb(port))
>
> Why move it below? Because its comment does not apply to the below?

Because the new form depends on isa_inw()? I've seen order matter in CPP 
statements before ...

>>  #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
>>  #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
>>  #define isa_outb(val,port) out_8(isa_itb(port),(val))
>>  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
>>  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
>>
>> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
>> + * dead code if MULTI_ISA is not set */
>
> I don't think this comment is needed.

I'll remove it, then.

Cheers,

	Michael


>
>> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
>> +
>>  #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
>>  #define isa_readw(p)       \
>>         (ISA_SEX ? in_be16(isa_mtw((unsigned long)(p))) \
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add code to support 10 Mbit and 100 Mbit mode for APNE driver.
>
> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
> to word access as required by the 100 Mbit cards.
>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>  }
>
>
> -#define isa_inb(port)      in_8(isa_itb(port))
>  #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
>  #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
>  #define isa_outb(val,port) out_8(isa_itb(port),(val))
>  #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
>  #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
>
> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
> + * dead code if MULTI_ISA is not set */
> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))

This fails to compile due to a missing closing parenthesis.

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 RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Geert,

On 7/06/21 11:15 pm, Geert Uytterhoeven wrote:
> Hi Michael,
>
> On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Add code to support 10 Mbit and 100 Mbit mode for APNE driver.
>>
>> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
>> to word access as required by the 100 Mbit cards.
>>
>> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
>> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
>> Kazik <alex@kazik.de>.
>>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>> --- a/arch/m68k/include/asm/io_mm.h
>> +++ b/arch/m68k/include/asm/io_mm.h
>> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
>>   }
>>
>>
>> -#define isa_inb(port)      in_8(isa_itb(port))
>>   #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
>>   #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
>>   #define isa_outb(val,port) out_8(isa_itb(port),(val))
>>   #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
>>   #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
>>
>> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
>> + * dead code if MULTI_ISA is not set */
>> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
> This fails to compile due to a missing closing parenthesis.

Sorry - looks like brown paper bag time today. (I did say 'entirely 
untested'? Didn't expect such a thorough review for a first RFC patch ...)

Apologies,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Re: [PATCH RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Geert Uytterhoeven]

Hi Michael,

On Mon, Jun 7, 2021 at 9:58 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 7/06/21 11:15 pm, Geert Uytterhoeven wrote:
> > On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> >> Add code to support 10 Mbit and 100 Mbit mode for APNE driver.
> >>
> >> A new ISA type ISA_TYPE_AG100 switches the Amiga ISA inb accessor
> >> to word access as required by the 100 Mbit cards.
> >>
> >> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> >> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> >> Kazik <alex@kazik.de>.
> >>
> >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> >> --- a/arch/m68k/include/asm/io_mm.h
> >> +++ b/arch/m68k/include/asm/io_mm.h
> >> @@ -212,13 +232,16 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
> >>   }
> >>
> >>
> >> -#define isa_inb(port)      in_8(isa_itb(port))
> >>   #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
> >>   #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
> >>   #define isa_outb(val,port) out_8(isa_itb(port),(val))
> >>   #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
> >>   #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
> >>
> >> +/* for APNE 100 Mbit cards - hope the APNE 100 case will be eliminated as
> >> + * dead code if MULTI_ISA is not set */
> >> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
> > This fails to compile due to a missing closing parenthesis.
>
> Sorry - looks like brown paper bag time today. (I did say 'entirely
> untested'? Didn't expect such a thorough review for a first RFC patch ...)

Sorry, I missed that part in the cover letter ;-)

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 RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Geert,

On 8/06/21 6:42 pm, Geert Uytterhoeven wrote:
> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
>>> This fails to compile due to a missing closing parenthesis.
>> Sorry - looks like brown paper bag time today. (I did say 'entirely
>> untested'? Didn't expect such a thorough review for a first RFC patch ...)
> Sorry, I missed that part in the cover letter ;-)

I'll put it more prominently next time.

Ran all patches through checkpatch now, and I still get warnings and 
even a few errors ('trailing statements should be on the next line'). 
All due to my keeping to the code style used in io_mm.h, as far as I 
could see.

What's your preference - additions in new style, or keep the old style?

Cheers,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Re: [PATCH RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Geert Uytterhoeven]

Hi Michael,

On Tue, Jun 8, 2021 at 11:55 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 8/06/21 6:42 pm, Geert Uytterhoeven wrote:
> > +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
> >>> This fails to compile due to a missing closing parenthesis.
> >> Sorry - looks like brown paper bag time today. (I did say 'entirely
> >> untested'? Didn't expect such a thorough review for a first RFC patch ...)
> > Sorry, I missed that part in the cover letter ;-)
>
> I'll put it more prominently next time.
>
> Ran all patches through checkpatch now, and I still get warnings and
> even a few errors ('trailing statements should be on the next line').
> All due to my keeping to the code style used in io_mm.h, as far as I
> could see.
>
> What's your preference - additions in new style, or keep the old style?

Please use the existing style, unless you're willing to modernize the
whole file in a separate patch.

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 RFC 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Geert,

On 8/06/21 6:42 pm, Geert Uytterhoeven wrote:
> +#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG100) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port))
>>> This fails to compile due to a missing closing parenthesis.
>> Sorry - looks like brown paper bag time today. (I did say 'entirely
>> untested'? Didn't expect such a thorough review for a first RFC patch ...)
> Sorry, I missed that part in the cover letter ;-)

I'll put it more prominently next time.

Ran all patches through checkpatch now, and I still get warnings and 
even a few errors ('trailing statements should be on the next line'). 
All due to my keeping to the code style used in io_mm.h, as far as I 
could see.

What's your preference - additions in new style, or keep the old style?

Cheers,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>

[PATCH RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz]

Add Kconfig option, module parameter and PCMCIA reset code
required to support 100 Mbit PCMCIA ethernet cards on Amiga.

10 Mbit and 100 Mbit mode are supported by the same module.
A module parameter switches Amiga ISA IO accessors to word
access by changing isa_type at runtime. Additional code to
reset the PCMCIA hardware is also added to the driver probe.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/net/ethernet/8390/Kconfig | 13 +++++++++++++
 drivers/net/ethernet/8390/apne.c  | 17 +++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
index 9f4b302..10103fb 100644
--- a/drivers/net/ethernet/8390/Kconfig
+++ b/drivers/net/ethernet/8390/Kconfig
@@ -143,6 +143,19 @@ config APNE
 	  To compile this driver as a module, choose M here: the module
 	  will be called apne.
 
+if APNE
+config APNE100MBIT
+	bool "PCMCIA NE2000 100MBit support"
+	default n
+	---help---
+	  This changes the driver to support 10/100Mbit cards (e.g. Netgear
+	  FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
+	  supported by the same driver.
+
+	  To activate 100 Mbit support at runtime, use the apne100 module
+	  parameter. 
+endif
+
 config PCMCIA_PCNET
 	tristate "NE2000 compatible PCMCIA support"
 	depends on PCMCIA
diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
index fe6c834..9648e45 100644
--- a/drivers/net/ethernet/8390/apne.c
+++ b/drivers/net/ethernet/8390/apne.c
@@ -120,6 +120,10 @@ static u32 apne_msg_enable;
 module_param_named(msg_enable, apne_msg_enable, uint, 0444);
 MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
 
+static u32 apne_100_mbit;
+module_param_named(apne_100_mbit, uint, 0);
+MODULE_PARM_DESC(apne_100_mbit, "Enable 100 Mbit support");
+
 struct net_device * __init apne_probe(int unit)
 {
 	struct net_device *dev;
@@ -139,6 +143,9 @@ struct net_device * __init apne_probe(int unit)
 	if ( !(AMIGAHW_PRESENT(PCMCIA)) )
 		return ERR_PTR(-ENODEV);
 
+        if (apne_100_mbit)
+                isa_type = ISA_TYPE_AG100;
+
 	pr_info("Looking for PCMCIA ethernet card : ");
 
 	/* check if a card is inserted */
@@ -590,6 +597,16 @@ static int init_pcmcia(void)
 #endif
 	u_long offset;
 
+#ifdef CONFIG_APNE100MBIT
+	/* reset card (idea taken from CardReset by Artur Pogoda) */
+	{
+		u_char  tmp = gayle.intreq;
+
+		gayle.intreq = 0xff;    mdelay(1);
+		gayle.intreq = tmp;     mdelay(300);
+	}
+#endif
+
 	pcmcia_reset();
 	pcmcia_program_voltage(PCMCIA_0V);
 	pcmcia_access_speed(PCMCIA_SPEED_250NS);
-- 
2.7.4

Re: [PATCH RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add Kconfig option, module parameter and PCMCIA reset code
> required to support 100 Mbit PCMCIA ethernet cards on Amiga.
>
> 10 Mbit and 100 Mbit mode are supported by the same module.
> A module parameter switches Amiga ISA IO accessors to word
> access by changing isa_type at runtime. Additional code to
> reset the PCMCIA hardware is also added to the driver probe.
>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/8390/Kconfig
> +++ b/drivers/net/ethernet/8390/Kconfig
> @@ -143,6 +143,19 @@ config APNE
>           To compile this driver as a module, choose M here: the module
>           will be called apne.
>
> +if APNE

Please use "depends on APNE" instead of an if/endif block, as there's
only a single symbol to cover.

> +config APNE100MBIT
> +       bool "PCMCIA NE2000 100MBit support"
> +       default n
> +       ---help---
> +         This changes the driver to support 10/100Mbit cards (e.g. Netgear
> +         FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
> +         supported by the same driver.
> +
> +         To activate 100 Mbit support at runtime, use the apne100 module
> +         parameter.

Trailing space.

> +endif
> +
>  config PCMCIA_PCNET
>         tristate "NE2000 compatible PCMCIA support"
>         depends on PCMCIA
> diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
> index fe6c834..9648e45 100644
> --- a/drivers/net/ethernet/8390/apne.c
> +++ b/drivers/net/ethernet/8390/apne.c
> @@ -120,6 +120,10 @@ static u32 apne_msg_enable;
>  module_param_named(msg_enable, apne_msg_enable, uint, 0444);
>  MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
>
> +static u32 apne_100_mbit;
> +module_param_named(apne_100_mbit, uint, 0);
> +MODULE_PARM_DESC(apne_100_mbit, "Enable 100 Mbit support");

Shouldn't this depend on CONFIG_APNE100MBIT, too?
Perhaps we shouldn't bother with the config symbol, and include this
unconditionally?

> +
>  struct net_device * __init apne_probe(int unit)
>  {
>         struct net_device *dev;
> @@ -139,6 +143,9 @@ struct net_device * __init apne_probe(int unit)
>         if ( !(AMIGAHW_PRESENT(PCMCIA)) )
>                 return ERR_PTR(-ENODEV);
>
> +        if (apne_100_mbit)
> +                isa_type = ISA_TYPE_AG100;

Likewise.

Can we enable this automatically when needed, based on the chip
detected?

+ spaces instead of TABs (scripts/checkpatch.pl is your friend).

> +
>         pr_info("Looking for PCMCIA ethernet card : ");
>
>         /* check if a card is inserted */
> @@ -590,6 +597,16 @@ static int init_pcmcia(void)
>  #endif
>         u_long offset;
>
> +#ifdef CONFIG_APNE100MBIT
> +       /* reset card (idea taken from CardReset by Artur Pogoda) */
> +       {
> +               u_char  tmp = gayle.intreq;
> +
> +               gayle.intreq = 0xff;    mdelay(1);
> +               gayle.intreq = tmp;     mdelay(300);
> +       }

Is this safe for all cards?

> +#endif
> +
>         pcmcia_reset();
>         pcmcia_program_voltage(PCMCIA_0V);
>         pcmcia_access_speed(PCMCIA_SPEED_250NS);

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 RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz]

Hi Geert,

Am 07.06.2021 um 20:08 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Add Kconfig option, module parameter and PCMCIA reset code
>> required to support 100 Mbit PCMCIA ethernet cards on Amiga.
>>
>> 10 Mbit and 100 Mbit mode are supported by the same module.
>> A module parameter switches Amiga ISA IO accessors to word
>> access by changing isa_type at runtime. Additional code to
>> reset the PCMCIA hardware is also added to the driver probe.
>>
>> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
>> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
>> Kazik <alex@kazik.de>.
>>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Thanks for your patch!
>
>> --- a/drivers/net/ethernet/8390/Kconfig
>> +++ b/drivers/net/ethernet/8390/Kconfig
>> @@ -143,6 +143,19 @@ config APNE
>>           To compile this driver as a module, choose M here: the module
>>           will be called apne.
>>
>> +if APNE
>
> Please use "depends on APNE" instead of an if/endif block, as there's
> only a single symbol to cover.

True - I had copied that straight from Alex' patch ..

>
>> +config APNE100MBIT
>> +       bool "PCMCIA NE2000 100MBit support"
>> +       default n
>> +       ---help---
>> +         This changes the driver to support 10/100Mbit cards (e.g. Netgear
>> +         FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
>> +         supported by the same driver.
>> +
>> +         To activate 100 Mbit support at runtime, use the apne100 module
>> +         parameter.
>
> Trailing space.

And ---help--- did confuse kbuild, as I found out.

>> +endif
>> +
>>  config PCMCIA_PCNET
>>         tristate "NE2000 compatible PCMCIA support"
>>         depends on PCMCIA
>> diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
>> index fe6c834..9648e45 100644
>> --- a/drivers/net/ethernet/8390/apne.c
>> +++ b/drivers/net/ethernet/8390/apne.c
>> @@ -120,6 +120,10 @@ static u32 apne_msg_enable;
>>  module_param_named(msg_enable, apne_msg_enable, uint, 0444);
>>  MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
>>
>> +static u32 apne_100_mbit;
>> +module_param_named(apne_100_mbit, uint, 0);
>> +MODULE_PARM_DESC(apne_100_mbit, "Enable 100 Mbit support");
>
> Shouldn't this depend on CONFIG_APNE100MBIT, too?
> Perhaps we shouldn't bother with the config symbol, and include this
> unconditionally?

At least the parameter should not hurt - this is entirely untested so 
I'd hate to break APNE by including the io_mm.h changes unconditionally.

>> +
>>  struct net_device * __init apne_probe(int unit)
>>  {
>>         struct net_device *dev;
>> @@ -139,6 +143,9 @@ struct net_device * __init apne_probe(int unit)
>>         if ( !(AMIGAHW_PRESENT(PCMCIA)) )
>>                 return ERR_PTR(-ENODEV);
>>
>> +        if (apne_100_mbit)
>> +                isa_type = ISA_TYPE_AG100;
>
> Likewise.

And that one breaks if 16 bit support isn't always enabled. I'll 
conditionalize it.

> Can we enable this automatically when needed, based on the chip
> detected?

I wondered about that - we'd have to poke the chip with 16 bit IO if 8 
bit IO fails, no idea what side effects that would have.

Maybe retrying the card reset in apne_probe1 again after changing 
isa_type would work, but someone would have to try that and report back.

> + spaces instead of TABs (scripts/checkpatch.pl is your friend).

Thanks, I'll fix that. Got sloppy with checkpatch, sorry.

>
>> +
>>         pr_info("Looking for PCMCIA ethernet card : ");
>>
>>         /* check if a card is inserted */
>> @@ -590,6 +597,16 @@ static int init_pcmcia(void)
>>  #endif
>>         u_long offset;
>>
>> +#ifdef CONFIG_APNE100MBIT
>> +       /* reset card (idea taken from CardReset by Artur Pogoda) */
>> +       {
>> +               u_char  tmp = gayle.intreq;
>> +
>> +               gayle.intreq = 0xff;    mdelay(1);
>> +               gayle.intreq = tmp;     mdelay(300);
>> +       }
>
> Is this safe for all cards?

I _think_ so, but this really is a question for Alex and others involved 
in developing the original patch. (The original patch had that 
conditionalized as well so I might be wrong ...)

I'll send v2 once I've fixed these all up...

Cheers,

	Michael


>> +#endif
>> +
>>         pcmcia_reset();
>>         pcmcia_program_voltage(PCMCIA_0V);
>>         pcmcia_access_speed(PCMCIA_SPEED_250NS);
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[ALeX Kazik]

Hi all,

> It's been quite a while - can you still try and build/test this change, Alex?

I'm happy to test it again (assuming my Amiga still works, not used
for a while).
But I'm waiting until you guys have perfected the patch.

> >> +
> >>         pr_info("Looking for PCMCIA ethernet card : ");
> >>
> >>         /* check if a card is inserted */
> >> @@ -590,6 +597,16 @@ static int init_pcmcia(void)
> >>  #endif
> >>         u_long offset;
> >>
> >> +#ifdef CONFIG_APNE100MBIT
> >> +       /* reset card (idea taken from CardReset by Artur Pogoda) */
> >> +       {
> >> +               u_char  tmp = gayle.intreq;
> >> +
> >> +               gayle.intreq = 0xff;    mdelay(1);
> >> +               gayle.intreq = tmp;     mdelay(300);
> >> +       }
> >
> > Is this safe for all cards?
>
> I _think_ so, but this really is a question for Alex and others involved
> in developing the original patch. (The original patch had that
> conditionalized as well so I might be wrong ...)

My code is based on the patches here: http://www.g-mb.de/pcmcia_e.html

I'm not deep into it, just adapted the patch. And that's why I can't
tell if the other drivers will work with it always enabled, sorry.

Bye,
Alex.

Re: [PATCH RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz]

Hi Alex,


Am 07.06.2021 um 20:46 schrieb ALeX Kazik:
> Hi all,
>
>> It's been quite a while - can you still try and build/test this change, Alex?
>
> I'm happy to test it again (assuming my Amiga still works, not used
> for a while).
> But I'm waiting until you guys have perfected the patch.

Thanks, I'll keep you in the CC list.
>
>>>> +
>>>>         pr_info("Looking for PCMCIA ethernet card : ");
>>>>
>>>>         /* check if a card is inserted */
>>>> @@ -590,6 +597,16 @@ static int init_pcmcia(void)
>>>>  #endif
>>>>         u_long offset;
>>>>
>>>> +#ifdef CONFIG_APNE100MBIT
>>>> +       /* reset card (idea taken from CardReset by Artur Pogoda) */
>>>> +       {
>>>> +               u_char  tmp = gayle.intreq;
>>>> +
>>>> +               gayle.intreq = 0xff;    mdelay(1);
>>>> +               gayle.intreq = tmp;     mdelay(300);
>>>> +       }
>>>
>>> Is this safe for all cards?
>>
>> I _think_ so, but this really is a question for Alex and others involved
>> in developing the original patch. (The original patch had that
>> conditionalized as well so I might be wrong ...)
>
> My code is based on the patches here: http://www.g-mb.de/pcmcia_e.html
>
> I'm not deep into it, just adapted the patch. And that's why I can't
> tell if the other drivers will work with it always enabled, sorry.

According to that site, the reset is needed for all 100 Mbit cards, and 
in your patch from 2018, you mentioned that this is because the PCMCIA 
reset line isn't connected on these cards. Based on that reading, this 
_should_ be safe for those cards that have a reset line wired as well.

Who could we ask - I guess Rolf Anders was the author of a previous 
patch for the 100 Mbit APNE compatible cards? Not sure I've seen an 
e-mail address for Artur Pogoda?

Cheers,

	Michael




>
> Bye,
> Alex.
>

Re: [PATCH RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add Kconfig option, module parameter and PCMCIA reset code
> required to support 100 Mbit PCMCIA ethernet cards on Amiga.
>
> 10 Mbit and 100 Mbit mode are supported by the same module.
> A module parameter switches Amiga ISA IO accessors to word
> access by changing isa_type at runtime. Additional code to
> reset the PCMCIA hardware is also added to the driver probe.
>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

> --- a/drivers/net/ethernet/8390/Kconfig
> +++ b/drivers/net/ethernet/8390/Kconfig
> @@ -143,6 +143,19 @@ config APNE
>           To compile this driver as a module, choose M here: the module
>           will be called apne.
>
> +if APNE
> +config APNE100MBIT
> +       bool "PCMCIA NE2000 100MBit support"
> +       default n
> +       ---help---

"help" (support for "---help---" was removed in v5.9).

> +         This changes the driver to support 10/100Mbit cards (e.g. Netgear
> +         FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
> +         supported by the same driver.
> +
> +         To activate 100 Mbit support at runtime, use the apne100 module
> +         parameter.
> +endif
> +
>  config PCMCIA_PCNET
>         tristate "NE2000 compatible PCMCIA support"
>         depends on PCMCIA

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 RFC 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:54 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Add Kconfig option, module parameter and PCMCIA reset code
> required to support 100 Mbit PCMCIA ethernet cards on Amiga.
>
> 10 Mbit and 100 Mbit mode are supported by the same module.
> A module parameter switches Amiga ISA IO accessors to word
> access by changing isa_type at runtime. Additional code to
> reset the PCMCIA hardware is also added to the driver probe.
>
> Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
> 100 MBit card support" submitted to netdev 2018/09/16 by Alex
> Kazik <alex@kazik.de>.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

> --- a/drivers/net/ethernet/8390/apne.c
> +++ b/drivers/net/ethernet/8390/apne.c
> @@ -120,6 +120,10 @@ static u32 apne_msg_enable;
>  module_param_named(msg_enable, apne_msg_enable, uint, 0444);
>  MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
>
> +static u32 apne_100_mbit;
> +module_param_named(apne_100_mbit, uint, 0);

error: macro "module_param_named" requires 4 arguments, but only 3 given

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

[PATCH v1 0/2] Add APNE PCMCIA 100 Mbit support

[Michael Schmitz]

Now that we've sanitzed io_mm.h, revisit support for the 100 Mbit APNE
PCMCIA card variants that didn't find favour with the netdev folks three
years ago.

Add another ISA type for these cards, and switch to that type from the
APNE probe code in response to a module parameter. 

I've addressed review comments received so far, to the best of my ability.

Compile tested only - hopefully someone will be encouraged to give this
a try before I submit the apne.c patch to netdev. 

Cheers,

   Michael

[PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Add code to support 10 Mbit and 100 Mbit mode for APNE driver.

A new ISA type ISA_TYPE_AG16 dynamically switches the Amiga
ISA inb accessor to word access as required by the 100 Mbit cards.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from RFC:

Geert Uytterhoeven:
- rename ISA_TYPE_AG100 to ISA_TYPE_AG16 (16 bit cards)
- move ISA_TYPE_AG16 case inside #ifdef CONFIG_AMIGA_PCMCIA
- change #if defined(CONFIG_APNE_100MBIT) to #ifdef
- fix parentheses in isa_inb() define
- omit comment about compiler optimization

- add ISA_TYPE_AG16 case to isa_delay()
---
 arch/m68k/include/asm/io_mm.h | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index 6eae9ec..a965a0e 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -101,6 +101,11 @@
 #define ISA_TYPE_Q40  (1)
 #define ISA_TYPE_AG   (2)
 #define ISA_TYPE_ENEC (3)
+#define ISA_TYPE_AG16 (4)	/* for 100 MBit APNE card */
+
+#if defined(CONFIG_APNE100MBIT)
+#define MULTI_ISA 1
+#endif
 
 #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
 #define ISA_TYPE ISA_TYPE_Q40
@@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: fallthrough;
+#endif
     case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -155,6 +163,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: fallthrough;
+#endif
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -171,6 +182,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
   switch(ISA_TYPE)
     {
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: fallthrough;
+#endif
     case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
 #endif
     }
@@ -184,6 +198,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: fallthrough;
+#endif
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -203,6 +220,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: fallthrough;
+#endif
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -216,13 +236,14 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
 }
 
 
-#define isa_inb(port)      in_8(isa_itb(port))
 #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
 #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
 #define isa_outb(val,port) out_8(isa_itb(port),(val))
 #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
 #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
 
+#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG16) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)))
+
 #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
 #define isa_readw(p)       \
 	(ISA_SEX ? in_be16(isa_mtw((unsigned long)(p)))	\
@@ -270,6 +291,9 @@ static inline void isa_delay(void)
     case ISA_TYPE_Q40: isa_outb(0,0x80); break;
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: break;
+#endif
     case ISA_TYPE_AG: break;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-- 
2.7.4

Re: [PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Andreas Schwab]

On Jun 09 2021, Michael Schmitz wrote:

> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>      case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>  #endif
>  #ifdef CONFIG_AMIGA_PCMCIA
> +#ifdef CONFIG_APNE100MBIT
> +    case ISA_TYPE_AG16: fallthrough;
> +#endif
>      case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);

Is the fallthrough annotation really needed?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Andreas,

On 9/06/21 8:04 pm, Andreas Schwab wrote:
> On Jun 09 2021, Michael Schmitz wrote:
>
>> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>       case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>>   #endif
>>   #ifdef CONFIG_AMIGA_PCMCIA
>> +#ifdef CONFIG_APNE100MBIT
>> +    case ISA_TYPE_AG16: fallthrough;
>> +#endif
>>       case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
> Is the fallthrough annotation really needed?

Just to shut up compiler warnings, and even that I haven't seen myself.

I have seen a number of patches adding either comments or this 
annotation in the core NCR5380 driver (which Finn maintains, who 
suggested this annotation to an earlier version of the Q40/Atari io_mm.h 
patch), so adding annotations appears to be encouraged now.

I personally think these annotations are over the top generally, but 
I've learned to program when computed goto statements were still en vogue.

In this particular case, there can be no doubt that the fallthrough is 
intentional, so on balance, I'll remove those annotations as well 
(unless Finn strongly objects?).

Cheers,

     Michael


Cheers,

     Michael


>
> Andreas.
>

Re: [PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Finn Thain]

On Thu, 10 Jun 2021, Michael Schmitz wrote:

> On 9/06/21 8:04 pm, Andreas Schwab wrote:
> > On Jun 09 2021, Michael Schmitz wrote:
> > 
> > > @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
> > >       case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
> > >   #endif
> > >   #ifdef CONFIG_AMIGA_PCMCIA
> > > +#ifdef CONFIG_APNE100MBIT
> > > +    case ISA_TYPE_AG16: fallthrough;
> > > +#endif
> > >       case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
> > Is the fallthrough annotation really needed?
> 
> Just to shut up compiler warnings, and even that I haven't seen myself.
> 
> I have seen a number of patches adding either comments or this annotation in
> the core NCR5380 driver (which Finn maintains, who suggested this annotation
> to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations
> appears to be encouraged now.
> 
> I personally think these annotations are over the top generally, but I've
> learned to program when computed goto statements were still en vogue.
> 
> In this particular case, there can be no doubt that the fallthrough is
> intentional, so on balance, I'll remove those annotations as well (unless Finn
> strongly objects?).
> 

I don't object to removing it. On the contrary, in a previous message I 
also questioned adding this particular 'fallthrough' (though I did 
recommended adding a different one).

In general, there's no way to predict which static checkers are going to 
complain about any given line of code. They don't all agree about 
correctness and they are a moving target, just like fashion or reviewers' 
preferred code styles.

The 'fallthrough' that I recommended was merely the one that seemed to 
improve readability to my eyes (and not to some algorithm). I didn't 
actually run "make C=1 W=1" or any other checker.

Re: [PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Geert Uytterhoeven]

Hi Finn,

On Thu, Jun 10, 2021 at 3:09 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Thu, 10 Jun 2021, Michael Schmitz wrote:
> > On 9/06/21 8:04 pm, Andreas Schwab wrote:
> > > On Jun 09 2021, Michael Schmitz wrote:
> > >
> > > > @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
> > > >       case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
> > > >   #endif
> > > >   #ifdef CONFIG_AMIGA_PCMCIA
> > > > +#ifdef CONFIG_APNE100MBIT
> > > > +    case ISA_TYPE_AG16: fallthrough;
> > > > +#endif
> > > >       case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
> > > Is the fallthrough annotation really needed?
> >
> > Just to shut up compiler warnings, and even that I haven't seen myself.
> >
> > I have seen a number of patches adding either comments or this annotation in
> > the core NCR5380 driver (which Finn maintains, who suggested this annotation
> > to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations
> > appears to be encouraged now.
> >
> > I personally think these annotations are over the top generally, but I've
> > learned to program when computed goto statements were still en vogue.
> >
> > In this particular case, there can be no doubt that the fallthrough is
> > intentional, so on balance, I'll remove those annotations as well (unless Finn
> > strongly objects?).
> >
>
> I don't object to removing it. On the contrary, in a previous message I
> also questioned adding this particular 'fallthrough' (though I did
> recommended adding a different one).
>
> In general, there's no way to predict which static checkers are going to
> complain about any given line of code. They don't all agree about
> correctness and they are a moving target, just like fashion or reviewers'
> preferred code styles.

AFAIK, they only complain when the switch() operates on an enum,
and not all enum values are handled.

When operating on an int, there's not enough address space on
32-bit machines to handle all cases ;-)

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 v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Hi Geert,

thanks for the explanation - and thanks Finn and Andreas for clarifying. 
I've removed the annotation in v2.

Cheers,

     Michael

On 10/06/21 7:32 pm, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Thu, Jun 10, 2021 at 3:09 AM Finn Thain <fthain@linux-m68k.org> wrote:
>> On Thu, 10 Jun 2021, Michael Schmitz wrote:
>>> On 9/06/21 8:04 pm, Andreas Schwab wrote:
>>>> On Jun 09 2021, Michael Schmitz wrote:
>>>>
>>>>> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>>>>        case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>>>>>    #endif
>>>>>    #ifdef CONFIG_AMIGA_PCMCIA
>>>>> +#ifdef CONFIG_APNE100MBIT
>>>>> +    case ISA_TYPE_AG16: fallthrough;
>>>>> +#endif
>>>>>        case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>>>> Is the fallthrough annotation really needed?
>>> Just to shut up compiler warnings, and even that I haven't seen myself.
>>>
>>> I have seen a number of patches adding either comments or this annotation in
>>> the core NCR5380 driver (which Finn maintains, who suggested this annotation
>>> to an earlier version of the Q40/Atari io_mm.h patch), so adding annotations
>>> appears to be encouraged now.
>>>
>>> I personally think these annotations are over the top generally, but I've
>>> learned to program when computed goto statements were still en vogue.
>>>
>>> In this particular case, there can be no doubt that the fallthrough is
>>> intentional, so on balance, I'll remove those annotations as well (unless Finn
>>> strongly objects?).
>>>
>> I don't object to removing it. On the contrary, in a previous message I
>> also questioned adding this particular 'fallthrough' (though I did
>> recommended adding a different one).
>>
>> In general, there's no way to predict which static checkers are going to
>> complain about any given line of code. They don't all agree about
>> correctness and they are a moving target, just like fashion or reviewers'
>> preferred code styles.
> AFAIK, they only complain when the switch() operates on an enum,
> and not all enum values are handled.
>
> When operating on an int, there's not enough address space on
> 32-bit machines to handle all cases ;-)
>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Re: [PATCH v1 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Andreas Schwab]

On Jun 10 2021, Michael Schmitz wrote:

> Hi Andreas,
>
> On 9/06/21 8:04 pm, Andreas Schwab wrote:
>> On Jun 09 2021, Michael Schmitz wrote:
>>
>>> @@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
>>>       case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
>>>   #endif
>>>   #ifdef CONFIG_AMIGA_PCMCIA
>>> +#ifdef CONFIG_APNE100MBIT
>>> +    case ISA_TYPE_AG16: fallthrough;
>>> +#endif
>>>       case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
>> Is the fallthrough annotation really needed?
>
> Just to shut up compiler warnings, and even that I haven't seen myself.

If there is no warning, there is nothing to shut up.

> In this particular case, there can be no doubt that the fallthrough is
> intentional, so on balance, I'll remove those annotations as well 
> (unless Finn strongly objects?).

There is no fallthrough, because there is no code.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH v1 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz]

Add Kconfig option, module parameter and PCMCIA reset code
required to support 100 Mbit PCMCIA ethernet cards on Amiga.

10 Mbit and 100 Mbit mode are supported by the same module.
A module parameter switches Amiga ISA IO accessors to word
access by changing isa_type at runtime. Additional code to
reset the PCMCIA hardware is also added to the driver probe.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from RFC:

Geert Uytterhoeven:
- change APNE_100MBIT to depend on APNE
- change '---help---' to 'help' (former no longer supported)
- fix whitespace errors
- fix module_param_named() arg count
- protect all added code by #ifdef CONFIG_APNE_100MBIT
---
 drivers/net/ethernet/8390/Kconfig | 12 ++++++++++++
 drivers/net/ethernet/8390/apne.c  | 21 +++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
index 9f4b302..8ff8da6 100644
--- a/drivers/net/ethernet/8390/Kconfig
+++ b/drivers/net/ethernet/8390/Kconfig
@@ -143,6 +143,18 @@ config APNE
 	  To compile this driver as a module, choose M here: the module
 	  will be called apne.
 
+config APNE100MBIT
+	bool "PCMCIA NE2000 100MBit support"
+	depends on APNE
+	default n
+	help
+	  This changes the driver to support 10/100Mbit cards (e.g. Netgear
+	  FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
+	  supported by the same driver.
+
+	  To activate 100 Mbit support at runtime, use the apne100 module
+	  parameter.
+
 config PCMCIA_PCNET
 	tristate "NE2000 compatible PCMCIA support"
 	depends on PCMCIA
diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
index fe6c834..cd99cc3 100644
--- a/drivers/net/ethernet/8390/apne.c
+++ b/drivers/net/ethernet/8390/apne.c
@@ -120,6 +120,12 @@ static u32 apne_msg_enable;
 module_param_named(msg_enable, apne_msg_enable, uint, 0444);
 MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
 
+#ifdef CONFIG_APNE100MBIT
+static u32 apne_100_mbit;
+module_param_named(apne_100_mbit_msg, apne_100_mbit, uint, 0);
+MODULE_PARM_DESC(apne_100_mbit_msg, "Enable 100 Mbit support");
+#endif
+
 struct net_device * __init apne_probe(int unit)
 {
 	struct net_device *dev;
@@ -139,6 +145,11 @@ struct net_device * __init apne_probe(int unit)
 	if ( !(AMIGAHW_PRESENT(PCMCIA)) )
 		return ERR_PTR(-ENODEV);
 
+#ifdef CONFIG_APNE100MBIT
+	if (apne_100_mbit)
+		isa_type = ISA_TYPE_AG16;
+#endif
+
 	pr_info("Looking for PCMCIA ethernet card : ");
 
 	/* check if a card is inserted */
@@ -590,6 +601,16 @@ static int init_pcmcia(void)
 #endif
 	u_long offset;
 
+#ifdef CONFIG_APNE100MBIT
+	/* reset card (idea taken from CardReset by Artur Pogoda) */
+	{
+		u_char  tmp = gayle.intreq;
+
+		gayle.intreq = 0xff;    mdelay(1);
+		gayle.intreq = tmp;     mdelay(300);
+	}
+#endif
+
 	pcmcia_reset();
 	pcmcia_program_voltage(PCMCIA_0V);
 	pcmcia_access_speed(PCMCIA_SPEED_250NS);
-- 
2.7.4

[PATCH v2 0/2] Add APNE PCMCIA 100 Mbit support

[Michael Schmitz]

Now that we've sanitzed io_mm.h, revisit support for the 100 Mbit APNE
PCMCIA card variants that didn't find favour with the netdev folks three
years ago.

Add another ISA type for these cards, and switch to that type from the
APNE probe code in response to a module parameter. 

I've addressed review comments received so far, to the best of my ability.
Patch 2 not changed in v2, only comments on patch 1 by Andreas Schwab were
addressed.

Compile tested only - hopefully someone will be encouraged to give this
a try before I submit the apne.c patch to netdev. 

Cheers,

   Michael

[PATCH v2 1/2] m68k: io_mm.h - add APNE 100 MBit support

[Michael Schmitz]

Add code to support 10 Mbit and 100 Mbit mode for APNE driver.

A new ISA type ISA_TYPE_AG16 dynamically switches the Amiga
ISA inb accessor to word access as required by the 100 Mbit cards.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from v1:

Andreas Schwab:
- remove redundant fallthrough annotations

Changes from RFC:

Geert Uytterhoeven:
- rename ISA_TYPE_AG100 to ISA_TYPE_AG16 (16 bit cards)
- move ISA_TYPE_AG16 case inside #ifdef CONFIG_AMIGA_PCMCIA
- change #if defined(CONFIG_APNE_100MBIT) to #ifdef
- fix parentheses in isa_inb() define
- omit comment about compiler optimization

- add ISA_TYPE_AG16 case to isa_delay()

fixup - review comment Andreas Schwab (fallthrough)
---
 arch/m68k/include/asm/io_mm.h | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index 9c521b0..3ab2f1d 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -101,6 +101,11 @@
 #define ISA_TYPE_Q40  (1)
 #define ISA_TYPE_AG   (2)
 #define ISA_TYPE_ENEC (3)
+#define ISA_TYPE_AG16 (4)	/* for 100 MBit APNE card */
+
+#if defined(CONFIG_APNE100MBIT)
+#define MULTI_ISA 1
+#endif
 
 #if defined(CONFIG_Q40) && !defined(MULTI_ISA)
 #define ISA_TYPE ISA_TYPE_Q40
@@ -136,6 +141,9 @@ static inline u8 __iomem *isa_itb(unsigned long addr)
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_IO_B(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16:
+#endif
     case ISA_TYPE_AG: return (u8 __iomem *)AG_ISA_IO_B(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -155,6 +163,9 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16:
+#endif
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -171,6 +182,9 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
   switch(ISA_TYPE)
     {
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16:
+#endif
     case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
 #endif
     }
@@ -184,6 +198,9 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_Q40: return (u8 __iomem *)Q40_ISA_MEM_B(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16:
+#endif
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -203,6 +220,9 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_Q40: return (u16 __iomem *)Q40_ISA_MEM_W(addr);
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16:
+#endif
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
@@ -216,13 +236,14 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
 }
 
 
-#define isa_inb(port)      in_8(isa_itb(port))
 #define isa_inw(port)      (ISA_SEX ? in_be16(isa_itw(port)) : in_le16(isa_itw(port)))
 #define isa_inl(port)      (ISA_SEX ? in_be32(isa_itl(port)) : in_le32(isa_itl(port)))
 #define isa_outb(val,port) out_8(isa_itb(port),(val))
 #define isa_outw(val,port) (ISA_SEX ? out_be16(isa_itw(port),(val)) : out_le16(isa_itw(port),(val)))
 #define isa_outl(val,port) (ISA_SEX ? out_be32(isa_itl(port),(val)) : out_le32(isa_itl(port),(val)))
 
+#define isa_inb(port)      ((ISA_TYPE == ISA_TYPE_AG16) ? ((port) & 1 ? isa_inw((port) - 1) & 0xff : isa_inw(port) >> 8) : in_8(isa_itb(port)))
+
 #define isa_readb(p)       in_8(isa_mtb((unsigned long)(p)))
 #define isa_readw(p)       \
 	(ISA_SEX ? in_be16(isa_mtw((unsigned long)(p)))	\
@@ -270,6 +291,9 @@ static inline void isa_delay(void)
     case ISA_TYPE_Q40: isa_outb(0,0x80); break;
 #endif
 #ifdef CONFIG_AMIGA_PCMCIA
+#ifdef CONFIG_APNE100MBIT
+    case ISA_TYPE_AG16: break;
+#endif
     case ISA_TYPE_AG: break;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-- 
2.7.4

[PATCH v2 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz]

Add Kconfig option, module parameter and PCMCIA reset code
required to support 100 Mbit PCMCIA ethernet cards on Amiga.

10 Mbit and 100 Mbit mode are supported by the same module.
A module parameter switches Amiga ISA IO accessors to word
access by changing isa_type at runtime. Additional code to
reset the PCMCIA hardware is also added to the driver probe.

Patch modified after patch "[PATCH RFC net-next] Amiga PCMCIA
100 MBit card support" submitted to netdev 2018/09/16 by Alex
Kazik <alex@kazik.de>.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from RFC:

Geert Uytterhoeven:
- change APNE_100MBIT to depend on APNE
- change '---help---' to 'help' (former no longer supported)
- fix whitespace errors
- fix module_param_named() arg count
- protect all added code by #ifdef CONFIG_APNE_100MBIT
---
 drivers/net/ethernet/8390/Kconfig | 12 ++++++++++++
 drivers/net/ethernet/8390/apne.c  | 21 +++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/8390/Kconfig b/drivers/net/ethernet/8390/Kconfig
index 9f4b302..8ff8da6 100644
--- a/drivers/net/ethernet/8390/Kconfig
+++ b/drivers/net/ethernet/8390/Kconfig
@@ -143,6 +143,18 @@ config APNE
 	  To compile this driver as a module, choose M here: the module
 	  will be called apne.
 
+config APNE100MBIT
+	bool "PCMCIA NE2000 100MBit support"
+	depends on APNE
+	default n
+	help
+	  This changes the driver to support 10/100Mbit cards (e.g. Netgear
+	  FA411, CNet Singlepoint). 10 MBit cards and 100 MBit cards are
+	  supported by the same driver.
+
+	  To activate 100 Mbit support at runtime, use the apne100 module
+	  parameter.
+
 config PCMCIA_PCNET
 	tristate "NE2000 compatible PCMCIA support"
 	depends on PCMCIA
diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
index fe6c834..cd99cc3 100644
--- a/drivers/net/ethernet/8390/apne.c
+++ b/drivers/net/ethernet/8390/apne.c
@@ -120,6 +120,12 @@ static u32 apne_msg_enable;
 module_param_named(msg_enable, apne_msg_enable, uint, 0444);
 MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
 
+#ifdef CONFIG_APNE100MBIT
+static u32 apne_100_mbit;
+module_param_named(apne_100_mbit_msg, apne_100_mbit, uint, 0);
+MODULE_PARM_DESC(apne_100_mbit_msg, "Enable 100 Mbit support");
+#endif
+
 struct net_device * __init apne_probe(int unit)
 {
 	struct net_device *dev;
@@ -139,6 +145,11 @@ struct net_device * __init apne_probe(int unit)
 	if ( !(AMIGAHW_PRESENT(PCMCIA)) )
 		return ERR_PTR(-ENODEV);
 
+#ifdef CONFIG_APNE100MBIT
+	if (apne_100_mbit)
+		isa_type = ISA_TYPE_AG16;
+#endif
+
 	pr_info("Looking for PCMCIA ethernet card : ");
 
 	/* check if a card is inserted */
@@ -590,6 +601,16 @@ static int init_pcmcia(void)
 #endif
 	u_long offset;
 
+#ifdef CONFIG_APNE100MBIT
+	/* reset card (idea taken from CardReset by Artur Pogoda) */
+	{
+		u_char  tmp = gayle.intreq;
+
+		gayle.intreq = 0xff;    mdelay(1);
+		gayle.intreq = tmp;     mdelay(300);
+	}
+#endif
+
 	pcmcia_reset();
 	pcmcia_program_voltage(PCMCIA_0V);
 	pcmcia_access_speed(PCMCIA_SPEED_250NS);
-- 
2.7.4

Re: [PATCH v2 0/2] Add APNE PCMCIA 100 Mbit support

[ALeX Kazik]

Hi,

I've tested the patches and they work.

At first I got it only to work with the following line removed/commented out:
  if (apne_100_mbit)
And thus enabling the following line always.

I've changed, with the help of Michael, the parameters to:

  static u32 apne_100_mbit = 0;
  module_param_named(100_mbit, apne_100_mbit, uint, 0644);
  MODULE_PARM_DESC(100_mbit, "Enable 100 Mbit support");

And was able to enable it with the kernel option "apne.100_mbit=1".

It's also available as /sys/module/apne/parameters/100_mbit

The 0644 is the permission (root can change it), If it shouldn't be
changed at runtime 0444 or 0 would be used.

(I think there is also a bool option instead of the uint, but I'm glad
it works like this.)

Alex.

Re: [PATCH v2 0/2] Add APNE PCMCIA 100 Mbit support

[Michael Schmitz]

Hi Alex,

Thanks for testing!

On 17/06/21 9:11 am, ALeX Kazik wrote:
> Hi,
>
> I've tested the patches and they work.
>
> At first I got it only to work with the following line removed/commented out:
>    if (apne_100_mbit)
> And thus enabling the following line always.
>
> I've changed, with the help of Michael, the parameters to:
>
>    static u32 apne_100_mbit = 0;
>    module_param_named(100_mbit, apne_100_mbit, uint, 0644);
>    MODULE_PARM_DESC(100_mbit, "Enable 100 Mbit support");
>
> And was able to enable it with the kernel option "apne.100_mbit=1".
>
> It's also available as /sys/module/apne/parameters/100_mbit
>
> The 0644 is the permission (root can change it), If it shouldn't be
> changed at runtime 0444 or 0 would be used.
Changing that parameter at runtime won't do anything (it's only tested 
at device probe time). But I'd better change the permission to 444.
> (I think there is also a bool option instead of the uint, but I'm glad
> it works like this.)

True, that would be the better option indeed. I'll change that in the 
next version (which will go to netdev as well).

Cheers,

     Michael


>
> Alex.

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Geert Uytterhoeven]

Hi Michael,

On Fri, Jun 4, 2021 at 2:19 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 3/06/21 8:23 pm, Finn Thain wrote:
> > On Wed, 2 Jun 2021, Michael Schmitz wrote:
> >> --- a/arch/m68k/include/asm/io_mm.h
> >> +++ b/arch/m68k/include/asm/io_mm.h
> >> @@ -52,7 +52,11 @@
> >>   #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
> >>   #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
> >>
> >> +#ifdef CONFIG_ATARI
> >> +#define MULTI_ISA 1
> >> +#else
> >>   #define MULTI_ISA 0
> >> +#endif /* Atari */
> >>   #endif /* Q40 */
> >>
> > I have to wonder whether there is a nice simple definition for MULTI_ISA.
>
> As I understand it, MULTI_ISA means that different byte orders and/or
> different address translations need to be used in the same kernel, so
> all that cannot be decided at build time.
>
> As long as there is only a single platform that will use this code (ISA
> only used on a single platform, and neither Atari IDE nor EtherNEC
> used), MULTI_ISA is not needed.
>
> If we have Kconfig symbols for 'single platform only', and
> 'multi-platform ISA use', that might be shorter to write and easier to
> understand. Geert?

It would be nice to have that automatically, like with the current
MULTI_ISA define (and all the MACH_* in arch/m68k/include/asm/setup.h).
Perhaps we should extend kconfig syntax to define a group of
related symbols, and to automatically generate CONFIG_FOO_MULTI or
CONFIG_FOO_SINGLE (and even CONFIG_BAR_ONLY?) symbols?

group ISA
     item ATARI_ROM_ISA
     item AMIGA_PCMCIA
     item Q40

=> CONFIG_ISA_MULTI or CONFIG_ISA_SINGLE (+ e.g. ATARI_ROM_ISA_ONLY
   if appropriate).

Are there other users who can benefit from this?

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 RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Geert,

Am 04.06.2021 um 19:54 schrieb Geert Uytterhoeven:
>>> I have to wonder whether there is a nice simple definition for MULTI_ISA.
>>
>> As I understand it, MULTI_ISA means that different byte orders and/or
>> different address translations need to be used in the same kernel, so
>> all that cannot be decided at build time.
>>
>> As long as there is only a single platform that will use this code (ISA
>> only used on a single platform, and neither Atari IDE nor EtherNEC
>> used), MULTI_ISA is not needed.
>>
>> If we have Kconfig symbols for 'single platform only', and
>> 'multi-platform ISA use', that might be shorter to write and easier to
>> understand. Geert?
>
> It would be nice to have that automatically, like with the current
> MULTI_ISA define (and all the MACH_* in arch/m68k/include/asm/setup.h).
> Perhaps we should extend kconfig syntax to define a group of
> related symbols, and to automatically generate CONFIG_FOO_MULTI or
> CONFIG_FOO_SINGLE (and even CONFIG_BAR_ONLY?) symbols?

I take it this is not supported by our current Kconfig syntax?

>
> group ISA
>      item ATARI_ROM_ISA
>      item AMIGA_PCMCIA
>      item Q40
>
> => CONFIG_ISA_MULTI or CONFIG_ISA_SINGLE (+ e.g. ATARI_ROM_ISA_ONLY
>    if appropriate).
>
> Are there other users who can benefit from this?

Nothing comes to mind immediately. But there will be other drivers 
shared between different architectures (Mac / PowerPC?) that might benefit.

Cheers,

	Michael


>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain]

On Sat, 5 Jun 2021, Michael Schmitz wrote:

> Am 04.06.2021 um 19:54 schrieb Geert Uytterhoeven:
> > > > I have to wonder whether there is a nice simple definition for 
> > > > MULTI_ISA.
> > > 
> > > As I understand it, MULTI_ISA means that different byte orders 
> > > and/or different address translations need to be used in the same 
> > > kernel, so all that cannot be decided at build time.
> > > 
> > > As long as there is only a single platform that will use this code 
> > > (ISA only used on a single platform, and neither Atari IDE nor 
> > > EtherNEC used), MULTI_ISA is not needed.
> > > 
> > > If we have Kconfig symbols for 'single platform only', and 
> > > 'multi-platform ISA use', that might be shorter to write and easier 
> > > to understand. Geert?
> > 
> > It would be nice to have that automatically, like with the current 
> > MULTI_ISA define (and all the MACH_* in 
> > arch/m68k/include/asm/setup.h). Perhaps we should extend kconfig 
> > syntax to define a group of related symbols, and to automatically 
> > generate CONFIG_FOO_MULTI or CONFIG_FOO_SINGLE (and even 
> > CONFIG_BAR_ONLY?) symbols?
> 
> I take it this is not supported by our current Kconfig syntax?
> 

That may be because CPP hacking is seen as a competitive sport in some 
circles.

> > group ISA
> >      item ATARI_ROM_ISA
> >      item AMIGA_PCMCIA
> >      item Q40
> > 
> > => CONFIG_ISA_MULTI or CONFIG_ISA_SINGLE (+ e.g. ATARI_ROM_ISA_ONLY
> >    if appropriate).
> > 

Since the items may be bools or tristates, it not clear what type the 
group has.

Anyway, I see that we can already write this:

#define IS_MULTI(a,b) __or(IS_ENABLED(a), IS_ENABLED(b))

So maybe we just need an exclusive-OR macro to go with the other operators 
defined in include/linux/kconfig.h? Then we could write this:

#define IS_SINGLE(a,b) __xor(IS_ENABLED(a), IS_ENABLED(b))

But these only work for a 2-way group. Extending them to N-way groups is 
beyond my CPP abilities. It probably requires N-way __or() and __xor()...

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Finn Thain]

On Sat, 5 Jun 2021, I wrote:

> 
> Anyway, I see that we can already write this:
> 
> #define IS_MULTI(a,b) __or(IS_ENABLED(a), IS_ENABLED(b))
> 

Oops. The 2-way case would be,
#define IS_MULTI(a,b) __and(IS_ENABLED(a), IS_ENABLED(b))

> So maybe we just need an exclusive-OR macro to go with the other operators 
> defined in include/linux/kconfig.h? Then we could write this:
> 
> #define IS_SINGLE(a,b) __xor(IS_ENABLED(a), IS_ENABLED(b))
> 
> But these only work for a 2-way group. Extending them to N-way groups is 
> beyond my CPP abilities. It probably requires N-way __or() and __xor()...
> 

For the 3 way case, 
#define IS_SINGLE(a,b,c) (__xor(IS_ENABLED(a), IS_ENABLED(b), IS_ENABLED(c)) &&
                          !__and(IS_ENABLED(a), IS_ENABLED(b), IS_ENABLED(c)))
#define IS_MULTI(a,b,c) (!IS_SINGLE(a,b,c) &&
                         __or(IS_ENABLED(a), IS_ENABLED(b), IS_ENABLED(c)))

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Finn,

Am 05.06.2021 um 11:31 schrieb Finn Thain:
>>>> If we have Kconfig symbols for 'single platform only', and
>>>> 'multi-platform ISA use', that might be shorter to write and easier
>>>> to understand. Geert?
>>>
>>> It would be nice to have that automatically, like with the current
>>> MULTI_ISA define (and all the MACH_* in
>>> arch/m68k/include/asm/setup.h). Perhaps we should extend kconfig
>>> syntax to define a group of related symbols, and to automatically
>>> generate CONFIG_FOO_MULTI or CONFIG_FOO_SINGLE (and even
>>> CONFIG_BAR_ONLY?) symbols?
>>
>> I take it this is not supported by our current Kconfig syntax?
>>
>
> That may be because CPP hacking is seen as a competitive sport in some
> circles.

Good one.

>
>>> group ISA
>>>      item ATARI_ROM_ISA
>>>      item AMIGA_PCMCIA
>>>      item Q40
>>>
>>> => CONFIG_ISA_MULTI or CONFIG_ISA_SINGLE (+ e.g. ATARI_ROM_ISA_ONLY
>>>    if appropriate).
>>>
>
> Since the items may be bools or tristates, it not clear what type the
> group has.

All three are of type bool.

>
> Anyway, I see that we can already write this:
>
> #define IS_MULTI(a,b) __or(IS_ENABLED(a), IS_ENABLED(b))
>
> So maybe we just need an exclusive-OR macro to go with the other operators
> defined in include/linux/kconfig.h? Then we could write this:
>
> #define IS_SINGLE(a,b) __xor(IS_ENABLED(a), IS_ENABLED(b))
>
> But these only work for a 2-way group. Extending them to N-way groups is
> beyond my CPP abilities. It probably requires N-way __or() and __xor()...

I'll pass on this one for now ...

Cheers,

	Michael

Re: [PATCH RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Geert Uytterhoeven]

Hi Michael,

On Wed, Jun 2, 2021 at 7:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Current io_mm.h uses address translation and ROM port IO primitives when
> port addresses are below 1024, and raw untranslated MMIO IO primitives
> else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
> m68k machine type a multi-platform kernel runs on. As a consequence,
> the Q40 IDE driver in multiplatform kernels cannot work.
> Conversely, the Atari IDE driver uses wrong address translation if a
> multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.
>
> Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
> is ISA_TYPE_ENEC), and change the ISA address translation used for
> Atari to a no-op for those addresses.
>
> Switch readb()/writeb() and readw()/writew() to their ISA equivalents
> also. Change the address translation functions to return the identity
> translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
> for kernels where Q40 and Atari are both configured so this can actually
> work (isa_type set to Q40 at compile time else).
>
> Signed-off-by: Michael Schmity <schmitzmic@gmail.com>

> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -52,7 +52,11 @@
>  #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
>  #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
>
> +#ifdef CONFIG_ATARI
> +#define MULTI_ISA 1
> +#else
>  #define MULTI_ISA 0
> +#endif /* Atari */
>  #endif /* Q40 */
>
>  #ifdef CONFIG_AMIGA_PCMCIA
> @@ -135,9 +139,12 @@ 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);

While putting a simple return on the same line as the case keyword,
please start the if statement on a new line.

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 RFC 1/2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Geert,

Am 09.06.2021 um 18:35 schrieb Geert Uytterhoeven:
>> @@ -135,9 +139,12 @@ 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);
>
> While putting a simple return on the same line as the case keyword,
> please start the if statement on a new line.

OK, changed that for v2.

Thanks,

	Michael

[PATCH RFC 2/2] m68k: setup_mm.c: set isa_sex for Atari if ATARI_ROM_ISA not used

[Michael Schmitz]

For multiplatform kernels where CONFIG_ATARI_ROM_ISA is not set,
at least isa_sex must be set correctly to allow for correct I/O
primitive selection in shared drivers.

Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/kernel/setup_mm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index 017bac3..bb43b99 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -386,6 +386,10 @@ void __init setup_arch(char **cmdline_p)
 		isa_type = ISA_TYPE_ENEC;
 		isa_sex = 0;
 	}
+#else
+	if (MACH_IS_ATARI) {
+		isa_sex = 0;
+	}
 #endif
 #endif
 }
-- 
2.7.4

Re: [PATCH RFC 2/2] m68k: setup_mm.c: set isa_sex for Atari if ATARI_ROM_ISA not used

[Geert Uytterhoeven]

Hi Michael,

On Wed, Jun 2, 2021 at 7:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> For multiplatform kernels where CONFIG_ATARI_ROM_ISA is not set,
> at least isa_sex must be set correctly to allow for correct I/O
> primitive selection in shared drivers.
>
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks for your patch!


> --- a/arch/m68k/kernel/setup_mm.c
> +++ b/arch/m68k/kernel/setup_mm.c
> @@ -386,6 +386,10 @@ void __init setup_arch(char **cmdline_p)
>                 isa_type = ISA_TYPE_ENEC;
>                 isa_sex = 0;
>         }
> +#else
> +       if (MACH_IS_ATARI) {
> +               isa_sex = 0;

I find it strange that you set isa_sex, but not isa_type?
However, this is inside the CONFIG_ISA && MULTI_ISA block, so
what kind of ISA does this correspond to?

> +       }
>  #endif
>  #endif

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 RFC 2/2] m68k: setup_mm.c: set isa_sex for Atari if ATARI_ROM_ISA not used

[Michael Schmitz]

Hi Geert,

Am 02.06.2021 um 19:09 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Wed, Jun 2, 2021 at 7:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> For multiplatform kernels where CONFIG_ATARI_ROM_ISA is not set,
>> at least isa_sex must be set correctly to allow for correct I/O
>> primitive selection in shared drivers.
>>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Thanks for your patch!
>
>
>> --- a/arch/m68k/kernel/setup_mm.c
>> +++ b/arch/m68k/kernel/setup_mm.c
>> @@ -386,6 +386,10 @@ void __init setup_arch(char **cmdline_p)
>>                 isa_type = ISA_TYPE_ENEC;
>>                 isa_sex = 0;
>>         }
>> +#else
>> +       if (MACH_IS_ATARI) {
>> +               isa_sex = 0;
>
> I find it strange that you set isa_sex, but not isa_type?

Yes, as I said this is only to ensure isa_sex has the correct value (0) 
so isa_readw() resolves to in_le16() for the Atari IDE driver after the 
change at the end of the first patch. Might have been better to rather say

int isa_sex=0;

at the start of setup_mm.c ... or rely on that variable being 
initialized as zero anyway.

> However, this is inside the CONFIG_ISA && MULTI_ISA block, so
> what kind of ISA does this correspond to?

No ISA at all in that case, but since we have to route all readw() calls 
through isa_readw() for the sake of generality if we want to support Q40 
ISA and Atari MMIO in the same kernel image, we need to make sure 
isa_readw() does the right thing on Atari if CONFIG_ISA is set.

I know io_mm.h says it's about various ISA bridges, but as I found out, 
drivers using only MMIO are also affected by these definitions.

'Maze' does not begin to describe it, 'mess' might be getting closer, 
but I can't see how we would avoid use of definitions in io_mm.h by 
non-ISA drivers.

Cheers,

	Michael

>
>> +       }
>>  #endif
>>  #endif
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC 2/2] m68k: setup_mm.c: set isa_sex for Atari if ATARI_ROM_ISA not used

[Finn Thain]

On Wed, 2 Jun 2021, Michael Schmitz wrote:

> Hi Geert,
> 
> Am 02.06.2021 um 19:09 schrieb Geert Uytterhoeven:
> > Hi Michael,
> > 
> > On Wed, Jun 2, 2021 at 7:21 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> > > For multiplatform kernels where CONFIG_ATARI_ROM_ISA is not set,
> > > at least isa_sex must be set correctly to allow for correct I/O
> > > primitive selection in shared drivers.
> > > 
> > > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
> > 
> > Thanks for your patch!
> > 
> > 
> > > --- a/arch/m68k/kernel/setup_mm.c
> > > +++ b/arch/m68k/kernel/setup_mm.c
> > > @@ -386,6 +386,10 @@ void __init setup_arch(char **cmdline_p)
> > >                 isa_type = ISA_TYPE_ENEC;
> > >                 isa_sex = 0;
> > >         }
> > > +#else
> > > +       if (MACH_IS_ATARI) {
> > > +               isa_sex = 0;
> > 
> > I find it strange that you set isa_sex, but not isa_type?
> 
> Yes, as I said this is only to ensure isa_sex has the correct value (0) so
> isa_readw() resolves to in_le16() for the Atari IDE driver after the change at
> the end of the first patch. Might have been better to rather say
> 
> int isa_sex=0;
> 
> at the start of setup_mm.c ... or rely on that variable being initialized as
> zero anyway.
> 
> > However, this is inside the CONFIG_ISA && MULTI_ISA block, so what 
> > kind of ISA does this correspond to?
> 
> No ISA at all in that case, but since we have to route all readw() calls 
> through isa_readw() for the sake of generality if we want to support Q40 
> ISA and Atari MMIO in the same kernel image, we need to make sure 
> isa_readw() does the right thing on Atari if CONFIG_ISA is set.
> 
> I know io_mm.h says it's about various ISA bridges, but as I found out, 
> drivers using only MMIO are also affected by these definitions.
> 
> 'Maze' does not begin to describe it, 'mess' might be getting closer, 
> but I can't see how we would avoid use of definitions in io_mm.h by 
> non-ISA drivers.
> 

It seems that the need for explicit zero initialization has more to do 
with subtelty over in asm/io_mm.h than anything else. If so, it would be 
better to add commentary there than to add redundant code here.

> Cheers,
> 
> 	Michael
> 
> > 
> > > +       }
> > >  #endif
> > >  #endif
> > 
> > Gr{oetje,eeting}s,
> > 
> >                         Geert
> > 
> 

Re: [PATCH RFC 2/2] m68k: setup_mm.c: set isa_sex for Atari if ATARI_ROM_ISA not used

[Michael Schmitz]

Hi Finn,

Am 03.06.2021 um 20:29 schrieb Finn Thain:
>>> However, this is inside the CONFIG_ISA && MULTI_ISA block, so what
>>> kind of ISA does this correspond to?
>>
>> No ISA at all in that case, but since we have to route all readw() calls
>> through isa_readw() for the sake of generality if we want to support Q40
>> ISA and Atari MMIO in the same kernel image, we need to make sure
>> isa_readw() does the right thing on Atari if CONFIG_ISA is set.
>>
>> I know io_mm.h says it's about various ISA bridges, but as I found out,
>> drivers using only MMIO are also affected by these definitions.
>>
>> 'Maze' does not begin to describe it, 'mess' might be getting closer,
>> but I can't see how we would avoid use of definitions in io_mm.h by
>> non-ISA drivers.
>>
>
> It seems that the need for explicit zero initialization has more to do
> with subtelty over in asm/io_mm.h than anything else. If so, it would be
> better to add commentary there than to add redundant code here.

OK, I'll drop this one and rely on default zero initialization of 
globals instead.

More explicit setting of MULTI_ISA along with comments will be included 
in v2 of the remaining patch.

Thanks!

	Michael

[PATCH] m68k: Fix multiplatform ISA support

[Michael Schmitz]

Current io_mm.h uses address translation and ROM port IO primitives when
port addresses are below 1024, and raw untranslated MMIO IO primitives
else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
m68k machine type a multi-platform kernel runs on. As a consequence,
the Q40 IDE driver in multiplatform kernels cannot work.
Conversely, the Atari IDE driver uses wrong address translation if a
multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.

Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
is ISA_TYPE_ENEC), and change the ISA address translation used for
Atari to a no-op for those addresses.

Switch readb()/writeb() and readw()/writew() to their ISA equivalents
also. Change the address translation functions to return the identity
translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
for kernels where Q40 and Atari are both configured so this can actually
work (isa_type set to Q40 at compile time else).

Fixes:  84b16b7b0d5c818fadc731a69965dc76dce0c91e
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from RFC:

Geert Uytterhoeven;
- drop setup_mm.c patch

Finn Thain:
- Add 'Fixes' tag
- Annotate rationale for MULTI_ISA setting on Atari
- Use fallthrough macro annotation in Atari address translation
---
 arch/m68k/include/asm/io_mm.h | 62 ++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index d41fa48..f6b487b 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -52,7 +52,11 @@
 #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
 #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
 
+#ifdef CONFIG_ATARI
+#define MULTI_ISA 1	/* Need MULTI_ISA if Atari drivers (IDE) present !! */
+#else
 #define MULTI_ISA 0
+#endif /* Atari */
 #endif /* Q40 */
 
 #ifdef CONFIG_AMIGA_PCMCIA
@@ -113,7 +117,7 @@
 
 #ifdef MULTI_ISA
 extern int isa_type;
-extern int isa_sex;
+extern int isa_sex;	/* Atari relies on this to be zero-initialized */
 
 #define ISA_TYPE isa_type
 #define ISA_SEX  isa_sex
@@ -135,9 +139,11 @@ 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);
+			fallthrough; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u16 __iomem *isa_itw(unsigned long addr)
@@ -151,9 +157,11 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+			fallthrough; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u32 __iomem *isa_itl(unsigned long addr)
@@ -177,9 +185,11 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+			fallthrough; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 static inline u16 __iomem *isa_mtw(unsigned long addr)
@@ -193,9 +203,11 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+    case ISA_TYPE_ENEC: if (addr < 1024)
+				return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+			fallthrough; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
+    default: return (u16 __iomem *)(addr); /* avoid warnings, just in case */
     }
 }
 
@@ -344,31 +356,31 @@ 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_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))
 #define inl		isa_inl
 #define inl_p		isa_inl_p
 
-#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
-#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
-#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
-#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
+#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
+#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
+#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
+#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
 #define outl		isa_outl
 #define outl_p		isa_outl_p
 
-#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
-#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
+#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
+#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
 #define insl			isa_insl
-#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
-#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
+#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
+#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
 #define outsl			isa_outsl
 
-#define readb(addr)		in_8(addr)
-#define writeb(val, addr)	out_8((addr), (val))
-#define readw(addr)		in_le16(addr)
-#define writew(val, addr)	out_le16((addr), (val))
+#define readb   isa_readb
+#define readw   isa_readw
+#define writeb  isa_writeb
+#define writew  isa_writew
 #endif /* CONFIG_ATARI_ROM_ISA */
 
 #define readl(addr)      in_le32(addr)
-- 
2.7.4

Re: [PATCH] m68k: Fix multiplatform ISA support

[Geert Uytterhoeven]

Hi Michael,

On Sun, Jun 6, 2021 at 7:28 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Current io_mm.h uses address translation and ROM port IO primitives when
> port addresses are below 1024, and raw untranslated MMIO IO primitives
> else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
> m68k machine type a multi-platform kernel runs on. As a consequence,
> the Q40 IDE driver in multiplatform kernels cannot work.
> Conversely, the Atari IDE driver uses wrong address translation if a
> multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.
>
> Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
> is ISA_TYPE_ENEC), and change the ISA address translation used for
> Atari to a no-op for those addresses.
>
> Switch readb()/writeb() and readw()/writew() to their ISA equivalents
> also. Change the address translation functions to return the identity
> translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
> for kernels where Q40 and Atari are both configured so this can actually
> work (isa_type set to Q40 at compile time else).

Thanks for your patch!

> Fixes:  84b16b7b0d5c818fadc731a69965dc76dce0c91e

Fixes: 84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")

Hint:

    $ git help fixes
    'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'
    $ git fixes 84b16b7b0d5c818fadc731a69965dc76dce0c91e
    Fixes: 84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")

> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

> --- a/arch/m68k/include/asm/io_mm.h
> +++ b/arch/m68k/include/asm/io_mm.h
> @@ -113,7 +117,7 @@
>
>  #ifdef MULTI_ISA
>  extern int isa_type;
> -extern int isa_sex;
> +extern int isa_sex;    /* Atari relies on this to be zero-initialized */

Shouldn't that comment be next to its definition in
arch/m68k/kernel/setup_mm.c?


> @@ -135,9 +139,11 @@ 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);
> +                       fallthrough; /* not ROM port? fallback below! */
>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
>      }

Moving the default out of the switch() statement, as suggested by Finn,
would make this more future-proof, as any case (not just the last one)
can fall through to the default, using break.

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: Fix multiplatform ISA support

[Michael Schmitz]

Hi Geert,

thanks for your review!

Am 07.06.2021 um 19:49 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Sun, Jun 6, 2021 at 7:28 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Current io_mm.h uses address translation and ROM port IO primitives when
>> port addresses are below 1024, and raw untranslated MMIO IO primitives
>> else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
>> m68k machine type a multi-platform kernel runs on. As a consequence,
>> the Q40 IDE driver in multiplatform kernels cannot work.
>> Conversely, the Atari IDE driver uses wrong address translation if a
>> multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.
>>
>> Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
>> is ISA_TYPE_ENEC), and change the ISA address translation used for
>> Atari to a no-op for those addresses.
>>
>> Switch readb()/writeb() and readw()/writew() to their ISA equivalents
>> also. Change the address translation functions to return the identity
>> translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
>> for kernels where Q40 and Atari are both configured so this can actually
>> work (isa_type set to Q40 at compile time else).
>
> Thanks for your patch!
>
>> Fixes:  84b16b7b0d5c818fadc731a69965dc76dce0c91e
>
> Fixes: 84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")
>
> Hint:
>
>     $ git help fixes
>     'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'
>     $ git fixes 84b16b7b0d5c818fadc731a69965dc76dce0c91e
>     Fixes: 84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")

Thanks, I'll change that.

>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
>> --- a/arch/m68k/include/asm/io_mm.h
>> +++ b/arch/m68k/include/asm/io_mm.h
>> @@ -113,7 +117,7 @@
>>
>>  #ifdef MULTI_ISA
>>  extern int isa_type;
>> -extern int isa_sex;
>> +extern int isa_sex;    /* Atari relies on this to be zero-initialized */
>
> Shouldn't that comment be next to its definition in
> arch/m68k/kernel/setup_mm.c?

I ended up dropping changes to that file, and would rather omit the 
comment here than touching setup_mm.c just for the sake of adding a 
comment.

We might omit the isa_sex = 0 stuff in setup_mm.c altogether, OTOH ...

>
>> @@ -135,9 +139,11 @@ 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);
>> +                       fallthrough; /* not ROM port? fallback below! */
>>  #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>> +    default: return (u8 __iomem *)(addr); /* avoid warnings, just in case */
>>      }
>
> Moving the default out of the switch() statement, as suggested by Finn,
> would make this more future-proof, as any case (not just the last one)
> can fall through to the default, using break.

OK, I take that to indicate the default may be changed in that way, and 
will move it outside the switch().

Cheers,

	Michael


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

[PATCH v2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Current io_mm.h uses address translation and ROM port IO primitives when
port addresses are below 1024, and raw untranslated MMIO IO primitives
else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
m68k machine type a multi-platform kernel runs on. As a consequence,
the Q40 IDE driver in multiplatform kernels cannot work.
Conversely, the Atari IDE driver uses wrong address translation if a
multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.

Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
is ISA_TYPE_ENEC), and change the ISA address translation used for
Atari to a no-op for those addresses.

Switch readb()/writeb() and readw()/writew() to their ISA equivalents
also. Change the address translation functions to return the identity
translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
for kernels where Q40 and Atari are both configured so this can actually
work (isa_type set to Q40 at compile time else).

Fixes:  84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from v1:

Geert Uytterhoeven;
- fix Fixes tag
- remove comment about isa_sex zero initialization
- move default return outside switch statement in address
  translation functions to allow other code to break to default.
- fix indentation in switch statement

Changes from RFC:

Geert Uytterhoeven;
- drop setup_mm.c patch

Finn Thain:
- Add 'Fixes' tag
- Annotate rationale for MULTI_ISA setting on Atari
- Use fallthrough macro annotation in Atari address translation
---
 arch/m68k/include/asm/io_mm.h | 66 +++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index d41fa48..6eae9ec 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -52,7 +52,11 @@
 #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
 #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
 
+#ifdef CONFIG_ATARI
+#define MULTI_ISA 1	/* Need MULTI_ISA if Atari drivers (IDE) present !! */
+#else
 #define MULTI_ISA 0
+#endif /* Atari */
 #endif /* Q40 */
 
 #ifdef CONFIG_AMIGA_PCMCIA
@@ -135,10 +139,13 @@ 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);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u8 __iomem *)(addr); /* avoid warnings, just in case */
 }
 static inline u16 __iomem *isa_itw(unsigned long addr)
 {
@@ -151,10 +158,13 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u16 __iomem *)(addr); /* avoid warnings, just in case */
 }
 static inline u32 __iomem *isa_itl(unsigned long addr)
 {
@@ -163,8 +173,8 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
 #endif
-    default: return 0; /* avoid warnings, just in case */
     }
+    return 0; /* avoid warnings, just in case */
 }
 static inline u8 __iomem *isa_mtb(unsigned long addr)
 {
@@ -177,10 +187,13 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u8 __iomem *)(addr); /* avoid warnings, just in case */
 }
 static inline u16 __iomem *isa_mtw(unsigned long addr)
 {
@@ -193,10 +206,13 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u16 __iomem *)(addr); /* avoid warnings, just in case */
 }
 
 
@@ -344,31 +360,31 @@ 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_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))
 #define inl		isa_inl
 #define inl_p		isa_inl_p
 
-#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
-#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
-#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
-#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
+#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
+#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
+#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
+#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
 #define outl		isa_outl
 #define outl_p		isa_outl_p
 
-#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
-#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
+#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
+#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
 #define insl			isa_insl
-#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
-#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
+#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
+#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
 #define outsl			isa_outsl
 
-#define readb(addr)		in_8(addr)
-#define writeb(val, addr)	out_8((addr), (val))
-#define readw(addr)		in_le16(addr)
-#define writew(val, addr)	out_le16((addr), (val))
+#define readb   isa_readb
+#define readw   isa_readw
+#define writeb  isa_writeb
+#define writew  isa_writew
 #endif /* CONFIG_ATARI_ROM_ISA */
 
 #define readl(addr)      in_le32(addr)
-- 
2.7.4

Re: [PATCH v2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Andreas Schwab]

On Jun 09 2021, Michael Schmitz wrote:

> @@ -135,10 +139,13 @@ 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);
> +	break; /* not ROM port? fallback below! */
>  #endif
> -    default: return NULL; /* avoid warnings, just in case */
>      }
> +    return (u8 __iomem *)(addr); /* avoid warnings, just in case */

I don't think the comment still fits here.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Hi Andreas,

On 9/06/21 7:57 pm, Andreas Schwab wrote:
> On Jun 09 2021, Michael Schmitz wrote:
>
>> @@ -135,10 +139,13 @@ 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);
>> +	break; /* not ROM port? fallback below! */
>>   #endif
>> -    default: return NULL; /* avoid warnings, just in case */
>>       }
>> +    return (u8 __iomem *)(addr); /* avoid warnings, just in case */
> I don't think the comment still fits here.

True - I'll remove it.

Cheers,

     Michael


>
> Andreas.
>

[PATCH v3] m68k: io_mm.h: conditionalize ISA address translation on Atari

[Michael Schmitz]

Current io_mm.h uses address translation and ROM port IO primitives when
port addresses are below 1024, and raw untranslated MMIO IO primitives
else when CONFIG_ATARI_ROM_ISA is set. This is done regardless of the
m68k machine type a multi-platform kernel runs on. As a consequence,
the Q40 IDE driver in multiplatform kernels cannot work.
Conversely, the Atari IDE driver uses wrong address translation if a
multiplatform Q40 and Atari kernel does _not_ set CONFIG_ATARI_ROM_ISA.

Replace MMIO by ISA IO primitives for addresses > 1024 (if isa_type
is ISA_TYPE_ENEC), and change the ISA address translation used for
Atari to a no-op for those addresses.

Switch readb()/writeb() and readw()/writew() to their ISA equivalents
also. Change the address translation functions to return the identity
translation if CONFIG_ATARI_ROM_ISA is not defined, and set MULTI_ISA
for kernels where Q40 and Atari are both configured so this can actually
work (isa_type set to Q40 at compile time else).

Fixes:  84b16b7b0d5c818f ("m68k/atari: ROM port ISA adapter support")
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

--
Changes from v2:

Andreas Schwab:
- drop misleading default return comment

Changes from v1:

Geert Uytterhoeven;
- fix Fixes tag
- remove comment about isa_sex zero initialization
- move default return outside switch statement in address
  translation functions to allow other code to break to default.
- fix indentation in switch statement

Changes from RFC:

Geert Uytterhoeven;
- drop setup_mm.c patch

Finn Thain:
- Add 'Fixes' tag
- Annotate rationale for MULTI_ISA setting on Atari
- Use fallthrough macro annotation in Atari address translation
---
 arch/m68k/include/asm/io_mm.h | 66 +++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/arch/m68k/include/asm/io_mm.h b/arch/m68k/include/asm/io_mm.h
index d41fa48..9c521b0 100644
--- a/arch/m68k/include/asm/io_mm.h
+++ b/arch/m68k/include/asm/io_mm.h
@@ -52,7 +52,11 @@
 #define Q40_ISA_MEM_B(madr)  (q40_isa_mem_base+1+4*((unsigned long)(madr)))
 #define Q40_ISA_MEM_W(madr)  (q40_isa_mem_base+  4*((unsigned long)(madr)))
 
+#ifdef CONFIG_ATARI
+#define MULTI_ISA 1	/* Need MULTI_ISA if Atari drivers (IDE) present !! */
+#else
 #define MULTI_ISA 0
+#endif /* Atari */
 #endif /* Q40 */
 
 #ifdef CONFIG_AMIGA_PCMCIA
@@ -135,10 +139,13 @@ 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);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u8 __iomem *)(addr);
 }
 static inline u16 __iomem *isa_itw(unsigned long addr)
 {
@@ -151,10 +158,13 @@ static inline u16 __iomem *isa_itw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)AG_ISA_IO_W(addr);
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u16 __iomem *)ENEC_ISA_IO_W(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u16 __iomem *)(addr);
 }
 static inline u32 __iomem *isa_itl(unsigned long addr)
 {
@@ -163,8 +173,8 @@ static inline u32 __iomem *isa_itl(unsigned long addr)
 #ifdef CONFIG_AMIGA_PCMCIA
     case ISA_TYPE_AG: return (u32 __iomem *)AG_ISA_IO_W(addr);
 #endif
-    default: return 0; /* avoid warnings, just in case */
     }
+    return 0; /* avoid warnings, just in case */
 }
 static inline u8 __iomem *isa_mtb(unsigned long addr)
 {
@@ -177,10 +187,13 @@ static inline u8 __iomem *isa_mtb(unsigned long addr)
     case ISA_TYPE_AG: return (u8 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u8 __iomem *)ENEC_ISA_MEM_B(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u8 __iomem *)(addr);
 }
 static inline u16 __iomem *isa_mtw(unsigned long addr)
 {
@@ -193,10 +206,13 @@ static inline u16 __iomem *isa_mtw(unsigned long addr)
     case ISA_TYPE_AG: return (u16 __iomem *)addr;
 #endif
 #ifdef CONFIG_ATARI_ROM_ISA
-    case ISA_TYPE_ENEC: return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+    case ISA_TYPE_ENEC:
+	if (addr < 1024)
+		return (u16 __iomem *)ENEC_ISA_MEM_W(addr);
+	break; /* not ROM port? fallback below! */
 #endif
-    default: return NULL; /* avoid warnings, just in case */
     }
+    return (u16 __iomem *)(addr);
 }
 
 
@@ -344,31 +360,31 @@ 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_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))
 #define inl		isa_inl
 #define inl_p		isa_inl_p
 
-#define outb(val, port)	((port) < 1024 ? isa_rom_outb((val), (port)) : out_8((port), (val)))
-#define outb_p(val, port) ((port) < 1024 ? isa_rom_outb_p((val), (port)) : out_8((port), (val)))
-#define outw(val, port)	((port) < 1024 ? isa_rom_outw((val), (port)) : out_le16((port), (val)))
-#define outw_p(val, port) ((port) < 1024 ? isa_rom_outw_p((val), (port)) : out_le16((port), (val)))
+#define outb(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb((val), (port)) : isa_outb((val), (port)))
+#define outb_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outb_p((val), (port)) : isa_outb_p((val), (port)))
+#define outw(val, port)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw((val), (port)) : isa_outw((val), (port)))
+#define outw_p(val, port) (((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outw_p((val), (port)) : isa_outw_p((val), (port)))
 #define outl		isa_outl
 #define outl_p		isa_outl_p
 
-#define insb(port, buf, nr)	((port) < 1024 ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
-#define insw(port, buf, nr)	((port) < 1024 ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
+#define insb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insb((port), (buf), (nr)) : isa_insb((port), (buf), (nr)))
+#define insw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_insw((port), (buf), (nr)) : isa_insw((port), (buf), (nr)))
 #define insl			isa_insl
-#define outsb(port, buf, nr)	((port) < 1024 ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
-#define outsw(port, buf, nr)	((port) < 1024 ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
+#define outsb(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsb((port), (buf), (nr)) : isa_outsb((port), (buf), (nr)))
+#define outsw(port, buf, nr)	(((port) < 1024 && ISA_TYPE == ISA_TYPE_ENEC) ? isa_rom_outsw((port), (buf), (nr)) : isa_outsw((port), (buf), (nr)))
 #define outsl			isa_outsl
 
-#define readb(addr)		in_8(addr)
-#define writeb(val, addr)	out_8((addr), (val))
-#define readw(addr)		in_le16(addr)
-#define writew(val, addr)	out_le16((addr), (val))
+#define readb   isa_readb
+#define readw   isa_readw
+#define writeb  isa_writeb
+#define writew  isa_writew
 #endif /* CONFIG_ATARI_ROM_ISA */
 
 #define readl(addr)      in_le32(addr)
-- 
2.7.4