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

[Michael Schmitz <schmitzmic@gmail.com>]

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