(sorry for the long post, it's been overdue :), break it up, as you need to
in replies)
Hi Lothar,
Thank you for sharing your patch with us. I've finally gotten around to
merging your Nov 17 patch with my code base. It turned out to be too many
changes to just merge your patch into my code base, so instead, I've started
with your patch, applied my changes to it and then applied Olav's first
patch on top of that. This time around, I've made sure to keep track of all
the changes that I had to make to get this running on my PPC750 and kernel
2.6.6. I know you don't have a lot of bandwidth towards this driver now,
but I would appreciate if you could briefly review the changes and import
them on a case-by-case basis as you see fit. To make it easier for you, I
am going to send you the files separately from this post, at different
stages, so you can easily use a differencing tool to accept/reject
individual changes. These changes should not impact the rest of you, but
would allow me to share code / bug fixes with you more easily.
I've broken the changes into the following sections, which will correspond
to directories in the file that I will send you, so you can do easy
directory differences between code changes.
(1) Changes required to make patch compile
Mostly to do with functions / files / not available in my kernel tree /
architecture.
(2) Changes to remove additional compile time warnings
Maybe I'm getting too picky ;)
(3) Applying Big Endian Changes
All of these are le[16|32]_to_cpu macros (and vice versa) that I needed in
order to get this working on my big endian architecture. These bugs are
very tricky to find, so I don't claim I found them all yet. The crux of the
problem is the fact that OHCI is little endian, so care must be taken when
accessing any of the ED/TD data structures, as well as, the PTD data
structure. These issues are hidden on LE architectures. All these macros
should translate into nothing, if you are on a LE platform.
(4) Importing Olav's first patch
This also includes a few of my own changes and hacks for the time being.
Olav, when do you expect you could share your patch you are currently
working on? I would be interested to see how you have solved the ACTIVE bit
problem. From the post below, I am assuming you will be basing it on
Lothar's latest patch.
(5) Other Changes
A few other items that Michael had pointed out before on this list.
BTW, I find patches quite cumbersome to work with, as they never seem to
apply cleanly, so then you have to go through the rejects manually and
figure out where to introduce the changes. Since we are only dealing with a
handfull of files, it may be easier to just share the entire files, so you
can use difference tools to import changes (less error prone).
I hope this is good constructive feedback. As you can see from the list,
there is quite a bit of interest in this driver, so to make it more generic
is in everyone's interest.
On a different topic, is anyone else considering running this driver through
the usb test suite? Just exercising a HCD with devices won't easily flush
out all the corner cases and bugs, so I'm considering to use the usb test
suite with a keyspan usb to serial device to test this more thoroughly. Any
experience / comments with this approach?
Thanks,
-Philipp
----------------------------------------------------------------------------
------------
Change log:
Based on Lothar's second patch Nov 17 (applied to 2.6.8.1) - isp1362 files
only
Changes required to make patch compile
--------------------------------------
(set device option to ISP1362)
(1) asm/arch/bitfield.h not part of PPC headers. Seems generic enough, so
I've copied it from PXA.
Could we eventually incorporate it into the driver, to make it more generic?
(2) ohci-isp1362.h requires header guards to avoid duplicate definitions of
...
(3) ohci-isp1362_regs.h requires header guards to avoid duplicate
definitions of ...
(4) As Expected: define platform specific HCHWCFG_INIT, HCDMACFG_INIT,
CHIP_TYPE, ... macros (HCCHIPID_MAGIC needs to be defined specific to 1160
or 1362 - inlcuded in Olav's patch)
(5) Debug macros: MEMDUMP undefined added empty macro definition, if not
previously defined. Added DDPRINTK and DPRINTK macros
(6) ohci_chip_info struct is now missing. Couldn't find it in
ohci_isp1362.h where it was before => added it back in where it was before.
The actual object is instantiated in platform specific code, so maybe the
structure definition should be too, but where?
(7) Renamed drop_all_ptds to clear_ptd_queue in ohci-isp1362.h. Added
clear_ptd_queue declaration.
(8) asm/hardware.h and asm/mach-types.h : No such file or directory
=> Assuming this platform specific. Added ifdef CONFIG_ARCH_KARO. Doesn't
seem to impact anything.
(9) dma_to_virt is undefined. I have not yet found a solution to this
problem. The driver no has a NO_DMA_TO_VIRT option, but it seems it is
using PXA specific harddware support, which won't work for me. So for now
I've redfined this macro to phys_to_virt and I am changing the ohci layer
to pass me the physical address instead of the dma address. This change
probably should not propagate, but its in there for now. Definition is
added in ohci-isp1362.h
(10) I could find no reference to __test_bit anywhere. So I redefined this
macro to test_bit, which does exist. Should probably have a closer look
for portability.
(11) NO_IRQ is not a standard identifier across all platforms and does not
exist at all for me. So I had to redefine it to -1 as in the PPC64
architecture.
(12) IRQT_FALLING and set_irq_type are architecture specific. Excluded from
my compile. Should probably be in a platform specific setup file.
(13) platform_get_resource and platform_get_irq introduced between kernel
2.6.6 and 2.6.8.1, so I've added these functions to the driver based on a
compile directive.
(14) HCD_STATE_SUSPENDED does not exist in kernel 2.6.6. There seems to be
an equivalent definition called USB_STATE_SUSPENDED, so I've replaced these
two with a version ifdef.
(15) Member not found in dev->dev.power.power_state. In my kernel tree
power-state is covered by the CONFIG_PM define, in 2.6.8.1 it is not
covered by it anymore. Since usb_hcd_isp1362_suspend/resume is only used
with CONFIG_PM defined, I've added an additional ifdef around these two
functions bypassing this problem.
Changes to remove additional compile time warnings
--------------------------------------------------
(1) Removed 2 print format warnings in BUF_LEN
(2) Unused variable ohci in retire_td()
(3) Removed print format warning in info() printout in update_cbp.
(4) hc_isp1362_dump_all_regs not used, added COMPILE switch.
(5) discard_ptds_for_td doesn't seem to be used - should it be? Placed
ifdef around it for now to keep it clean.
(6) EXPORT_SYMBOL for usp1362_bus_remove, usp1362_bus_probe and
ohci_hcd_isp1362_driver as I call these specifically to start the HCD.
Applying Big Endian Changes
---------------------------
These are hard to find and stem from the fact that OHCI is little endian.
So the emulation layer needs to be extremely careful when accessing
everything including endpoint descriptors, transfer descriptors, HCCA, and
using constants for these purposes. On little endian archs these bugs are
hidden.
(1) These functions needed to be changed to access 16-bit registers
correctly as the isp is LE:
- HC_ISP116x_WRITE_ADDR
- HC_ISP116x_WRITE_DATA16
- HC_ISP116x_READ_DATA16
- HC_ISP116x_WRITE_DATA32
- HC_ISP116x_READ_DATA32
All of them have le16_to_cpu or cpu_to_le16 call as for the respective data
value. Should have no effect on a little endian systems.
(2) Macros ed_pid uses ED_IN and ED_OUT, which explicitly are declared as
__constant_cpu_to_le32. When the macros is used with ed_flags, which gets
swapped to CPU, incorrect data is produced by it overrrunning the index in
the 2-D ed_td_to_pid array. That one took a while to find, no seg fault,
but packet directions didn't match up.
=>#define ed_pid(info) (((info) & le32_to_cpu(ED_IN | ED_OUT)) >> 11)
(3) The following 10-bit PTD access macros need to be changed, as it indexes
16-bit fields inside the PTD structure.
- PTD_GET_COUNT
- PTD_GET_MPS
- PTD_GET_LEN
Note: only the extracts seem to need the byte swapping, as they do the funky
pointer conversion.
(4) ED_LOWSPEEED again is LE32, so in process_td it has to be swapped back
to CPU. That made my low speed devices work ;)
(5) The HCCA area is LE. So when the HCCAFrameNumber is written in the ISR
it has to swap it to LE. This caused OHCI to stall sending frames ... this
change made them flow again ;)
(6) find_td() uses dma_to_td, but needs to swap td->hwNextTD to CPU type,
else the hash operation won't work. I used the same method of accessing it
as found in ohci-dbg.c (le32_to_cpup(&td->hwNextTD))
Importing Olav's first patch
----------------------------
(1) Merged all ifdef'd statements
(2) Added supporting defines and register definitions
Other Changes
-------------
(1) From usb-devel posting:
The two lines:
- } else if (TD_CC_GET(td_flags) == TD_BUSY) {
- continue;
should be discarded.
=> done
(2) Added second write/read to HC_ISP1362_WRITE_DATA32 / READ_DATA32 (see
usb-devel)
(3) Fixed drop_all_ptds (see usb-devel)
-----Original Message-----
From: [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED] Behalf Of Olav
Kongas
Sent: Thursday, November 18, 2004 11:32 AM
To: Lothar Wassmann
Cc: [EMAIL PROTECTED]
Subject: Re: [linux-usb-devel] RE: yet more on ohci-isp1362
Hi Lothar,
> I added a patch to the download area at
> http://www.karo-electronics.de/support-public.html that reflects my
> latest changes that hopefully will help you to get things working.
> Unfortunately I'm working on something completely different now, so
> that I can't invest much time in this driver right now. :(
Thanks for the updated driver.
Until now I have made my 116x specific modifications so that
they apply on top of your driver without affecting the
1362-specific stuff. Yes, there are quite some #ifdefs
there, but still there is far more common code than chip
specific stuff. I looked at your new patch and saw that you
have removed the isp116x stuff from the ohci-isp1362.h file.
Does this mean that you prefer to keep the 1362 driver
separate or would you still consider adding the 116x support
if I am able to provide it as a cleaned up patch one day? I
would of course make the 116x patch apply to your then
latest driver version.
Olav
-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
