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

Finn Thain
2021-06-17T06:59:03Z

On Thu, 17 Jun 2021, Michael Schmitz wrote:

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

Was the re-ordering of definitions deliberate?

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

I think most of these "#ifdef CONFIG_APNE100MBIT" conditionals are
redundant. case ISA_TYPE_AG16 should be optimized away as dead code in the
MULTI_ISA == 0 configuration. And in the MULTI_ISA == 1 configuration, the
logic used to assign isa_type already depends on
defined(CONFIG_APNE100MBIT).