| Message ID | 1622611267-15825-2-git-send-email-schmitzmic@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix m68k multiplatform ISA support | expand |
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) >
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) >>
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.
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
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
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 >
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
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()...
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)))
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 >
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
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
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 >
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
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.
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 >
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
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
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
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)
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(-)