Hi Michael,
On Mon, Sep 8, 2014 at 10:47 AM, Michael Schmitz <[email protected]> wrote:
>> On Sat, Aug 30, 2014 at 3:25 AM, Michael Schmitz <[email protected]>
>> 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 <[email protected]>
>>>
>>>> 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 -- [email protected]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
