[PATCH] USB: isp116x: isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA

[Geert Uytterhoeven]

If CONFIG_ATARI_ROM_ISA=3Dn:

drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_write_addr=E2=80=
=99:
drivers/usb/host/isp116x.h:382: error: implicit declaration of function=
=E2=80=98isa_rom_writew_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_write_data=
16=E2=80=99:
drivers/usb/host/isp116x.h:394: error: implicit declaration of function=
=E2=80=98isa_rom_writew=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_read_data16=E2=
=80=99:
drivers/usb/host/isp116x.h:402: error: implicit declaration of function=
=E2=80=98isa_rom_readw_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_read_data1=
6=E2=80=99:
drivers/usb/host/isp116x.h:411: error: implicit declaration of function=
=E2=80=98isa_rom_readw=E2=80=99

The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead of
on CONFIG_ATARI.

Signed-off-by: Geert Uytterhoeven <***@linux-m68k.org>
---
To be folded into "m68k/atari: USB - add ISP1160 USB host controller
support"
---
drivers/usb/host/isp116x.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/isp116x.h b/drivers/usb/host/isp116x.h
index c436f55f2882..634258df159b 100644
--- a/drivers/usb/host/isp116x.h
+++ b/drivers/usb/host/isp116x.h
@@ -355,7 +355,7 @@ struct isp116x_ep {
#endif
=20
=20
-#ifdef CONFIG_ATARI
+#ifdef CONFIG_ATARI_ROM_ISA
/*
* 16 bit data bus byte swapped in hardware on both Atari variants.
* EtherNAT variant of ISP1160 integration is memory mapped at 0x800=
000XX,
--=20
1.9.1

[Michael Schmitz]

Hi Geert,

that's certainly correct - the EtherNAT version of the USB driver didn'=
t=20
need the ROM accessors but the version including NetUSBee support does=20
for sure.

Thanks!

Post by Geert Uytterhoeven
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_write_addr=E2=
drivers/usb/host/isp116x.h:382: error: implicit declaration of functi=

on =E2=80=98isa_rom_writew_raw=E2=80=99

Post by Geert Uytterhoeven
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_write_da=
drivers/usb/host/isp116x.h:394: error: implicit declaration of functi=

on =E2=80=98isa_rom_writew=E2=80=99

Post by Geert Uytterhoeven
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_read_data16=E2=
drivers/usb/host/isp116x.h:402: error: implicit declaration of functi=

on =E2=80=98isa_rom_readw_raw=E2=80=99

Post by Geert Uytterhoeven
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_read_dat=
drivers/usb/host/isp116x.h:411: error: implicit declaration of functi=

on =E2=80=98isa_rom_readw=E2=80=99

Post by Geert Uytterhoeven
The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead o=

f

Post by Geert Uytterhoeven
on CONFIG_ATARI.
---
To be folded into "m68k/atari: USB - add ISP1160 USB host controller
support"
---
drivers/usb/host/isp116x.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/isp116x.h b/drivers/usb/host/isp116x.h
index c436f55f2882..634258df159b 100644
--- a/drivers/usb/host/isp116x.h
+++ b/drivers/usb/host/isp116x.h
@@ -355,7 +355,7 @@ struct isp116x_ep {
#endif
=20
=20
-#ifdef CONFIG_ATARI
+#ifdef CONFIG_ATARI_ROM_ISA
/*
* 16 bit data bus byte swapped in hardware on both Atari variant=

s.

Post by Geert Uytterhoeven
* EtherNAT variant of ISP1160 integration is memory mapped at 0x=

800000XX,

[Geert Uytterhoeven]

Hi Michael,

that's certainly correct - the EtherNAT version of the USB driver did=

n't

need the ROM accessors but the version including NetUSBee support doe=

s for

sure.
Thanks!

Post by Geert Uytterhoeven
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_write_addr=E2=
drivers/usb/host/isp116x.h:382: error: implicit declaration of funct=

ion

Post by Geert Uytterhoeven
=E2=80=98isa_rom_writew_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_write_d=
drivers/usb/host/isp116x.h:394: error: implicit declaration of funct=

ion

Post by Geert Uytterhoeven
=E2=80=98isa_rom_writew=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_read_data16=
drivers/usb/host/isp116x.h:402: error: implicit declaration of funct=

ion

Post by Geert Uytterhoeven
=E2=80=98isa_rom_readw_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_read_da=
drivers/usb/host/isp116x.h:411: error: implicit declaration of funct=

ion

Post by Geert Uytterhoeven
=E2=80=98isa_rom_readw=E2=80=99
The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead =

of

Post by Geert Uytterhoeven
on CONFIG_ATARI.

Upon closer look, I think I broke the NetUSBee support if CONFIG_ATARI_=
ROM_ISA
is not set, as the mapping from isp_*() to low-level I/O accessors is
different.

On NetUSBee with CONFIG_ATARI_ROM_ISA=3Dy, we now have:

#ifdef CONFIG_ATARI_ROM_ISA
#define isp_readw(p) (__raw_readw((p)))
#define isp_writew(v, p) (__raw_writew((v), (p)))
#define isp_raw_readw(p) (readw((p)))
#define isp_raw_writew(v, p) (writew((v), (p)))
#else
/* sane hardware */
#define isp_readw readw
#define isp_writew writew
#define isp_raw_readw __raw_readw
#define isp_raw_writew __raw_writew
#endif

Before my patch, there was an "#ifdef CONFIG_ATARI", so the plain isp_*=
()
mapped to the raw ops on Atari, but to the plain ones elsewhere, while =
the
isp_raw_*() mapped to the plain ops on Atari, but the raw ops elsewhere=
=2E

The plain ops do byteswapping on m68k. If you change the mapping, perha=
ps
you can get rid of some of the #ifdefs in drivers/usb/host/isp116x-hcd.=
c?

P.S. Shall I revert my patch for now?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds

[Michael Schmitz]

Hi Geert,

Post by Geert Uytterhoeven
Hi Michael,

that's certainly correct - the EtherNAT version of the USB driver di=

dn't

Post by Geert Uytterhoeven

need the ROM accessors but the version including NetUSBee support do=

es for

Post by Geert Uytterhoeven

sure.
Thanks!

drivers/usb/host/isp116x.h:382: error: implicit declaration of func=

tion

Post by Geert Uytterhoeven

=91isa_rom_writew_raw=92
drivers/usb/host/isp116x.h: In function =91isp116x_raw_write_data16=
drivers/usb/host/isp116x.h:394: error: implicit declaration of func=

tion

Post by Geert Uytterhoeven

=91isa_rom_writew=92
drivers/usb/host/isp116x.h:402: error: implicit declaration of func=

tion

Post by Geert Uytterhoeven

=91isa_rom_readw_raw=92
drivers/usb/host/isp116x.h: In function =91isp116x_raw_read_data16=92=
drivers/usb/host/isp116x.h:411: error: implicit declaration of func=

tion

Post by Geert Uytterhoeven

=91isa_rom_readw=92
The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead=

of

Post by Geert Uytterhoeven

on CONFIG_ATARI.

Upon closer look, I think I broke the NetUSBee support if CONFIG_ATAR=

I_ROM_ISA

Post by Geert Uytterhoeven
is not set, as the mapping from isp_*() to low-level I/O accessors is
different.

That is right - CONFIG_ATARI_ROM_ISA is required for proper function of=
=20
the NetUSBee. The option should have been set by Kconfig magic anyway -=
=20
need to check that but obviously you did succeed to configure it=20
without... That option ensures that the correct bus accessor is availab=
le.

BTW, what I have in this case is:

#ifdef CONFIG_ATARI
/*
* 16 bit data bus byte swapped in hardware on both Atari variants.
* EtherNAT variant of ISP1160 integration is memory mapped at 0x800000X=
X,
* and uses regular readw/__raw_readw (but semantics swapped).
* NetUSBee variant is hooked up through ROM port and uses ROM port
* IO access, with fake IO address of 0x3XX.
* Selection of the appropriate accessors relies on ioremapped addresses=
=20
still
* retaining the 0x3XX bit.
*/
#define isp_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=3D=20
0x00000300UL) ? isa_rom_readw_raw(__pa(p)) : __raw_readw((p)))
#define isp_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=3D=
=20
0x00000300UL) ? isa_rom_writew_raw((v), __pa(p)) : __raw_writew((v), (p=
)))
#define isp_raw_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=3D=
=20
0x00000300UL) ? isa_rom_readw(__pa(p)) : readw((p)))
#define isp_raw_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00)=20
=3D=3D 0x00000300UL) ? isa_rom_writew((v), __pa(p)) : writew((v), (p)))
#else
/* sane hardware */
#define isp_readw readw
#define isp_writew writew
#define isp_raw_readw __raw_readw
#define isp_raw_writew __raw_writew
#endif

Much messier than yours due to the ioremap stuff.

Post by Geert Uytterhoeven
#ifdef CONFIG_ATARI_ROM_ISA
#define isp_readw(p) (__raw_readw((p)))
#define isp_writew(v, p) (__raw_writew((v), (p)))
#define isp_raw_readw(p) (readw((p)))
#define isp_raw_writew(v, p) (writew((v), (p)))
#else
/* sane hardware */
#define isp_readw readw
#define isp_writew writew
#define isp_raw_readw __raw_readw
#define isp_raw_writew __raw_writew
#endif
Before my patch, there was an "#ifdef CONFIG_ATARI", so the plain isp=

_*()

Post by Geert Uytterhoeven
mapped to the raw ops on Atari, but to the plain ones elsewhere, whil=

e the

Post by Geert Uytterhoeven
isp_raw_*() mapped to the plain ops on Atari, but the raw ops elsewhe=

re.

Correct - the semantics of raw vs. normal access is reversed on _both_=20
the Atari variants of the ISP1160 due to byte-swapping in the bus=20
interface.

Now, if NetUSBee is _not_ configured, and CONFIG_ATARI_ROM_ISA is not=20
set, your patch selects the 'sane' option for Atari (the EtherNAT=20
interface). That won't work - the weird vs. sane selection needs to be=20
done based on platform not bus type. I missed that earlier.

Post by Geert Uytterhoeven
The plain ops do byteswapping on m68k. If you change the mapping, per=

haps

Post by Geert Uytterhoeven
you can get rid of some of the #ifdefs in drivers/usb/host/isp116x-hc=

d.c?

We had tried that to no avail. The #ifdefs are there only for cases=20
where the buffer is not aligned properly to a 16-bit boundary, so swaps=
=20
of the first and last word would be messed up.

The original driver uses normal reads for everything but sending USB=20
data to the chip, assuming a native endian bus interface for the chip s=
o=20
byte swapping is done by the readw / writew. USB data are prepared as=20
little endian in the memory buffer, so no swapping must be done there.

The Atari implementation has the wires (bytes) crossed in hardware (jus=
t=20
like with the IDE interface - why stop once you've made a stupid design=
=20
mistake). No swapping by readw/writew must be done in software (taken=20
care by hardware) but we must swap the USB packet data (to cancel the=20
swap done in hardware).

I've pondered this many times and seen no way to avoid the defines.

Post by Geert Uytterhoeven
P.S. Shall I revert my patch for now?

Yes, please. I forgot about the consequences for the EtherNAT earlier.=20
Still need to ensure CONFIG_ATARI_ROM_ISA is set for NetUSBee though.=20
Maybe select ATARI_ROM_ISA if ATARI_USB is selected in Kconfig.bus, jus=
t=20
to be safe?

The help text there could use an update as well - mention NetUSBee as=20
user of ATARI_ROM_ISA in addition to EtherNEC, and mention the need for=
=20
ATARI_ROM_ISA for the NetUSBee in the ATARI_USB section, if you rather=20
not pre-select ATARI_ROM_ISA there. Does this make sense?


Cheers,

Michael

[Geert Uytterhoeven]

Hi Michael,
om>

Post by Geert Uytterhoeven

that's certainly correct - the EtherNAT version of the USB driver d=

idn't

Post by Geert Uytterhoeven

need the ROM accessors but the version including NetUSBee support d=

oes

Post by Geert Uytterhoeven

for
sure.
Thanks!

drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_write_add=
drivers/usb/host/isp116x.h:382: error: implicit declaration of fun=

ction

Post by Geert Uytterhoeven

=E2=80=98isa_rom_writew_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_write=
drivers/usb/host/isp116x.h:394: error: implicit declaration of fun=

ction

Post by Geert Uytterhoeven

=E2=80=98isa_rom_writew=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_read_data=
drivers/usb/host/isp116x.h:402: error: implicit declaration of fun=

ction

Post by Geert Uytterhoeven

=E2=80=98isa_rom_readw_raw=E2=80=99
drivers/usb/host/isp116x.h: In function =E2=80=98isp116x_raw_read_=
drivers/usb/host/isp116x.h:411: error: implicit declaration of fun=

ction

Post by Geert Uytterhoeven

=E2=80=98isa_rom_readw=E2=80=99
The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instea=

d of

Post by Geert Uytterhoeven

on CONFIG_ATARI.

Upon closer look, I think I broke the NetUSBee support if

Aarghl, I meant EtherNAT. Sorry, I misread who's using ATARI ROM ISA an=
d
who's using memory mapped I/O.

Post by Geert Uytterhoeven
CONFIG_ATARI_ROM_ISA
is not set, as the mapping from isp_*() to low-level I/O accessors i=

s

Post by Geert Uytterhoeven
different.

That is right - CONFIG_ATARI_ROM_ISA is required for proper function =

of the

NetUSBee. The option should have been set by Kconfig magic anyway - n=

eed to

check that but obviously you did succeed to configure it without... T=

hat

option ensures that the correct bus accessor is available.
#ifdef CONFIG_ATARI
/*
* 16 bit data bus byte swapped in hardware on both Atari variants.
* EtherNAT variant of ISP1160 integration is memory mapped at 0x80000=

0XX,

* and uses regular readw/__raw_readw (but semantics swapped).
* NetUSBee variant is hooked up through ROM port and uses ROM port
* IO access, with fake IO address of 0x3XX.
* Selection of the appropriate accessors relies on ioremapped address=

es

still
* retaining the 0x3XX bit.
*/
#define isp_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=3D
0x00000300UL) ? isa_rom_readw_raw(__pa(p)) : __raw_readw((p)))
#define isp_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=

=3D

0x00000300UL) ? isa_rom_writew_raw((v), __pa(p)) : __raw_writew((v), =

(p)))

#define isp_raw_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) =3D=

=3D

0x00000300UL) ? isa_rom_readw(__pa(p)) : readw((p)))
#define isp_raw_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00=

) =3D=3D

0x00000300UL) ? isa_rom_writew((v), __pa(p)) : writew((v), (p)))
#else
/* sane hardware */
#define isp_readw readw
#define isp_writew writew
#define isp_raw_readw __raw_readw
#define isp_raw_writew __raw_writew
#endif
Much messier than yours due to the ioremap stuff.

I had left out the isa_rom_*() clutter, as I was really talking about
EtherNAT.

Post by Geert Uytterhoeven
#ifdef CONFIG_ATARI_ROM_ISA
#define isp_readw(p) (__raw_readw((p)))
#define isp_writew(v, p) (__raw_writew((v), (p)))
#define isp_raw_readw(p) (readw((p)))
#define isp_raw_writew(v, p) (writew((v), (p)))
#else
/* sane hardware */
#define isp_readw readw
#define isp_writew writew
#define isp_raw_readw __raw_readw
#define isp_raw_writew __raw_writew
#endif
Before my patch, there was an "#ifdef CONFIG_ATARI", so the plain is=

p_*()

Post by Geert Uytterhoeven
mapped to the raw ops on Atari, but to the plain ones elsewhere, whi=

le the

Post by Geert Uytterhoeven
isp_raw_*() mapped to the plain ops on Atari, but the raw ops elsewh=

ere.

Correct - the semantics of raw vs. normal access is reversed on _both=

_ the

Atari variants of the ISP1160 due to byte-swapping in the bus interfa=

ce.

OK.

Now, if NetUSBee is _not_ configured, and CONFIG_ATARI_ROM_ISA is not=

set,

your patch selects the 'sane' option for Atari (the EtherNAT interfac=

e).

That won't work - the weird vs. sane selection needs to be done based=

on

platform not bus type. I missed that earlier.

OK.

Post by Geert Uytterhoeven
The plain ops do byteswapping on m68k. If you change the mapping, pe=

rhaps

Post by Geert Uytterhoeven
you can get rid of some of the #ifdefs in drivers/usb/host/isp116x-h=

cd.c?

We had tried that to no avail. The #ifdefs are there only for cases w=

here

the buffer is not aligned properly to a 16-bit boundary, so swaps of =

the

first and last word would be messed up.
The original driver uses normal reads for everything but sending USB =

data to

the chip, assuming a native endian bus interface for the chip so byte
swapping is done by the readw / writew. USB data are prepared as litt=

le

endian in the memory buffer, so no swapping must be done there.
The Atari implementation has the wires (bytes) crossed in hardware (j=

ust

like with the IDE interface - why stop once you've made a stupid desi=

gn

mistake). No swapping by readw/writew must be done in software (taken=

care

by hardware) but we must swap the USB packet data (to cancel the swap=

done

in hardware).
I've pondered this many times and seen no way to avoid the defines.

OK.

Post by Geert Uytterhoeven
P.S. Shall I revert my patch for now?

Yes, please. I forgot about the consequences for the EtherNAT earlier=

=2E Still

OK, will do.

need to ensure CONFIG_ATARI_ROM_ISA is set for NetUSBee though. Maybe=

select

ATARI_ROM_ISA if ATARI_USB is selected in Kconfig.bus, just to be saf=

e?

But ATARI_ROM_ISA is not needed for EtherNAT? So I'd rather not add thi=
s
dependency.

This does mean we have to distinguish in isp116x.h between three case, =
right?
- #ifdef CONFIG_ATARI_ROM_ISA=3Dy: handle both
- elif defined(CONFIG_ATARI): handle EtherNAT only
- else: others

The help text there could use an update as well - mention NetUSBee as=

user

of ATARI_ROM_ISA in addition to EtherNEC, and mention the need for
ATARI_ROM_ISA for the NetUSBee in the ATARI_USB section, if you rathe=

r not

pre-select ATARI_ROM_ISA there. Does this make sense?

Yes it does.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds

[Michael Schmitz]

Hi Geert,

Post by Geert Uytterhoeven
Upon closer look, I think I broke the NetUSBee support if

Aarghl, I meant EtherNAT. Sorry, I misread who's using ATARI ROM ISA and
who's using memory mapped I/O.

Makes sense now.

I had left out the isa_rom_*() clutter, as I was really talking about
EtherNAT.

OK, so we are on the same version after all.

need to ensure CONFIG_ATARI_ROM_ISA is set for NetUSBee though. Maybe select
ATARI_ROM_ISA if ATARI_USB is selected in Kconfig.bus, just to be safe?

But ATARI_ROM_ISA is not needed for EtherNAT? So I'd rather not add this
dependency.

Correct - compiling for EtherNAT only would pull in ROM support where it
is not in fact needed. Might do more harm than good.

This does mean we have to distinguish in isp116x.h between three case, right?
- #ifdef CONFIG_ATARI_ROM_ISA=y: handle both
- elif defined(CONFIG_ATARI): handle EtherNAT only
- else: others

If we want to avoid triggering the same compile error again, yes. To
optimize away the clutter, doubly yes. I doubt someone would want to
build for NetUSBee only - there is no config option that would allow
that at present. We did discuss simplifying the option mess at some
stage (for EtherNEC or EtherNAT) - I would be hesitant to add NetUSBee
or EtherNAT specific options back in.

The help text there could use an update as well - mention NetUSBee as user
of ATARI_ROM_ISA in addition to EtherNEC, and mention the need for
ATARI_ROM_ISA for the NetUSBee in the ATARI_USB section, if you rather not
pre-select ATARI_ROM_ISA there. Does this make sense?

Yes it does.

Shall I submit a help text patch, or are you happy to do that?

Cheers,

Michael

[Geert Uytterhoeven]

Hi Michael,

Post by Michael Schmitz

Post by Geert Uytterhoeven

The help text there could use an update as well - mention NetUSBee as user
of ATARI_ROM_ISA in addition to EtherNEC, and mention the need for
ATARI_ROM_ISA for the NetUSBee in the ATARI_USB section, if you rather not
pre-select ATARI_ROM_ISA there. Does this make sense?

Yes it does.

Shall I submit a help text patch, or are you happy to do that?

Feel free to send patches. If not, I'll see if I find some spare time...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@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