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

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

[Geert Uytterhoeven]

If CONFIG_ATARI_ROM_ISA=n:

drivers/usb/host/isp116x.h: In function ‘isp116x_write_addr’:
drivers/usb/host/isp116x.h:382: error: implicit declaration of function ‘isa_rom_writew_raw’
drivers/usb/host/isp116x.h: In function ‘isp116x_raw_write_data16’:
drivers/usb/host/isp116x.h:394: error: implicit declaration of function ‘isa_rom_writew’
drivers/usb/host/isp116x.h: In function ‘isp116x_read_data16’:
drivers/usb/host/isp116x.h:402: error: implicit declaration of function ‘isa_rom_readw_raw’
drivers/usb/host/isp116x.h: In function ‘isp116x_raw_read_data16’:
drivers/usb/host/isp116x.h:411: error: implicit declaration of function ‘isa_rom_readw’

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

Signed-off-by: Geert Uytterhoeven <geert@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
 
 
-#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 0x800000XX,
-- 
1.9.1

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

[Michael Schmitz]

Hi Geert,

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

Thanks!

Ack'ed-by: Michael Schmitz <schmitzmic@gmail.com>

> If CONFIG_ATARI_ROM_ISA=n:
>
> drivers/usb/host/isp116x.h: In function ‘isp116x_write_addr’:
> drivers/usb/host/isp116x.h:382: error: implicit declaration of function ‘isa_rom_writew_raw’
> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_write_data16’:
> drivers/usb/host/isp116x.h:394: error: implicit declaration of function ‘isa_rom_writew’
> drivers/usb/host/isp116x.h: In function ‘isp116x_read_data16’:
> drivers/usb/host/isp116x.h:402: error: implicit declaration of function ‘isa_rom_readw_raw’
> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_read_data16’:
> drivers/usb/host/isp116x.h:411: error: implicit declaration of function ‘isa_rom_readw’
>
> The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead of
> on CONFIG_ATARI.
>
> Signed-off-by: Geert Uytterhoeven <geert@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
>   
>   
> -#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 0x800000XX,

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

[Geert Uytterhoeven]

Hi Michael,

On Sat, Aug 30, 2014 at 3:25 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> that's certainly correct - the EtherNAT version of the USB driver didn't
> need the ROM accessors but the version including NetUSBee support does for
> sure.
>
> Thanks!
>
> Ack'ed-by: Michael Schmitz <schmitzmic@gmail.com>
>
>> If CONFIG_ATARI_ROM_ISA=n:
>>
>> drivers/usb/host/isp116x.h: In function ‘isp116x_write_addr’:
>> drivers/usb/host/isp116x.h:382: error: implicit declaration of function
>> ‘isa_rom_writew_raw’
>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_write_data16’:
>> drivers/usb/host/isp116x.h:394: error: implicit declaration of function
>> ‘isa_rom_writew’
>> drivers/usb/host/isp116x.h: In function ‘isp116x_read_data16’:
>> drivers/usb/host/isp116x.h:402: error: implicit declaration of function
>> ‘isa_rom_readw_raw’
>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_read_data16’:
>> drivers/usb/host/isp116x.h:411: error: implicit declaration of function
>> ‘isa_rom_readw’
>>
>> The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead of
>> 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=y, 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.

The plain ops do byteswapping on m68k. If you change the mapping, perhaps
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 -- geert@linux-m68k.org

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

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

[Michael Schmitz]

Hi Geert,
> Hi Michael,
>
> On Sat, Aug 30, 2014 at 3:25 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
>> that's certainly correct - the EtherNAT version of the USB driver didn't
>> need the ROM accessors but the version including NetUSBee support does for
>> sure.
>>
>> Thanks!
>>
>> Ack'ed-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>>> If CONFIG_ATARI_ROM_ISA=n:
>>>
>>> drivers/usb/host/isp116x.h: In function ‘isp116x_write_addr’:
>>> drivers/usb/host/isp116x.h:382: error: implicit declaration of function
>>> ‘isa_rom_writew_raw’
>>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_write_data16’:
>>> drivers/usb/host/isp116x.h:394: error: implicit declaration of function
>>> ‘isa_rom_writew’
>>> drivers/usb/host/isp116x.h: In function ‘isp116x_read_data16’:
>>> drivers/usb/host/isp116x.h:402: error: implicit declaration of function
>>> ‘isa_rom_readw_raw’
>>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_read_data16’:
>>> drivers/usb/host/isp116x.h:411: error: implicit declaration of function
>>> ‘isa_rom_readw’
>>>
>>> The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead of
>>> 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.

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 - 
need to check that but obviously you did succeed to configure it 
without... That option ensures that the correct bus accessor is available.

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 0x800000XX,
* 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 
still
* retaining the 0x3XX bit.
*/
#define isp_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) == 
0x00000300UL) ? isa_rom_readw_raw(__pa(p)) : __raw_readw((p)))
#define isp_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) == 
0x00000300UL) ? isa_rom_writew_raw((v), __pa(p)) : __raw_writew((v), (p)))
#define isp_raw_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) == 
0x00000300UL) ? isa_rom_readw(__pa(p)) : readw((p)))
#define isp_raw_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) 
== 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.

>
> On NetUSBee with CONFIG_ATARI_ROM_ISA=y, 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.

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

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

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

We had tried that to no avail. The #ifdefs are there only for cases 
where 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 
little endian in the memory buffer, so no swapping must be done there.

The Atari implementation has the wires (bytes) crossed in hardware (just 
like with the IDE interface - why stop once you've made a stupid design 
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.

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

Yes, please. I forgot about the consequences for the EtherNAT earlier. 
Still 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?

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?


Cheers,

Michael

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

[Geert Uytterhoeven]

Hi Michael,

On Mon, Sep 8, 2014 at 10:47 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
>> On Sat, Aug 30, 2014 at 3:25 AM, Michael Schmitz <schmitzmic@gmail.com>
>> wrote:
>>>
>>> that's certainly correct - the EtherNAT version of the USB driver didn't
>>> need the ROM accessors but the version including NetUSBee support does
>>> for
>>> sure.
>>>
>>> Thanks!
>>>
>>> Ack'ed-by: Michael Schmitz <schmitzmic@gmail.com>
>>>
>>>> If CONFIG_ATARI_ROM_ISA=n:
>>>>
>>>> drivers/usb/host/isp116x.h: In function ‘isp116x_write_addr’:
>>>> drivers/usb/host/isp116x.h:382: error: implicit declaration of function
>>>> ‘isa_rom_writew_raw’
>>>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_write_data16’:
>>>> drivers/usb/host/isp116x.h:394: error: implicit declaration of function
>>>> ‘isa_rom_writew’
>>>> drivers/usb/host/isp116x.h: In function ‘isp116x_read_data16’:
>>>> drivers/usb/host/isp116x.h:402: error: implicit declaration of function
>>>> ‘isa_rom_readw_raw’
>>>> drivers/usb/host/isp116x.h: In function ‘isp116x_raw_read_data16’:
>>>> drivers/usb/host/isp116x.h:411: error: implicit declaration of function
>>>> ‘isa_rom_readw’
>>>>
>>>> The isa_rom_*() calls should depend on CONFIG_ATARI_ROM_ISA instead of
>>>> 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 and
who's using memory mapped I/O.

>> CONFIG_ATARI_ROM_ISA
>> 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 the
> NetUSBee. The option should have been set by Kconfig magic anyway - need to
> check that but obviously you did succeed to configure it without... That
> option ensures that the correct bus accessor is available.
>
> 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 0x800000XX,
> * 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
> still
> * retaining the 0x3XX bit.
> */
> #define isp_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) ==
> 0x00000300UL) ? isa_rom_readw_raw(__pa(p)) : __raw_readw((p)))
> #define isp_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) ==
> 0x00000300UL) ? isa_rom_writew_raw((v), __pa(p)) : __raw_writew((v), (p)))
> #define isp_raw_readw(p) ((((unsigned long)(__pa(p)) & 0x00000F00) ==
> 0x00000300UL) ? isa_rom_readw(__pa(p)) : readw((p)))
> #define isp_raw_writew(v, p) ((((unsigned long)(__pa(p)) & 0x00000F00) ==
> 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.

>> On NetUSBee with CONFIG_ATARI_ROM_ISA=y, 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.
>
> 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 interface.

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 interface).
> That won't work - the weird vs. sane selection needs to be done based on
> platform not bus type. I missed that earlier.

OK.

>> The plain ops do byteswapping on m68k. If you change the mapping, perhaps
>> you can get rid of some of the #ifdefs in drivers/usb/host/isp116x-hcd.c?
>
> We had tried that to no avail. The #ifdefs are there only for cases where
> 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 little
> endian in the memory buffer, so no swapping must be done there.
>
> The Atari implementation has the wires (bytes) crossed in hardware (just
> like with the IDE interface - why stop once you've made a stupid design
> 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.

>> P.S. Shall I revert my patch for now?
>
> Yes, please. I forgot about the consequences for the EtherNAT earlier. 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 safe?

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

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

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

Gr{oetje,eeting}s,

                        Geert

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

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

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

[Michael Schmitz]

Hi Geert,

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

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

[Geert Uytterhoeven]

Hi Michael,

On Tue, Sep 9, 2014 at 10:33 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
>>> 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 -- geert@linux-m68k.org

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