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