Re: [U-Boot-Users] USB OHCI drivers unification
Hi Rodolfo,
Rodolfo Giometti giometti@linux.it writes:
may I know what you have already done on this topic?
Sure. I basically took the same approach as you suggested in your previous mail, but have split the the lowlevel functions into board and cpu dependant handling. I think this makes sense as boards for example can use the same cpu ohci controller but may require specific board dependant power settings. Either can be chosen by defining CFG_USB_OHCI_BOARD_INIT and CFG_USB_OHCI_CPU_INIT respectively.
I also discovered that the actions taken in case of failure sometimes differ from those to stop the controller, so I added the fail functions.
So I ended up with these hooks:
usb_cpu_init usb_cpu_stop usb_cpu_fail
usb_board_init usb_board_stop usb_board_fail
I have currently adapted the monahans, the at91rm9200, and the s3c24x0 cpus to use the generic driver. Please note that I used the cpu/arm920t/at91rm9200/usb_ohci.[hc] driver as a starting point for the generic driver.
We are currently starting a new USB testing branch for testing these and other USB related changes, which should be available in the git repo soon.
Please take a look at my patch to enable USB OHCI support on AU1x00 CPUs; in fact the problem was about virtual and physical addresses on MIPS platforms. For ARM platforms you may define as void the functions "virt_to_phys()" and phys_to_virt() but for MIPS are essential.
Your patch looks fine, but would you mind resubmitting it against the USB testing branch using the generic driver (drivers/usb_ohci.c) ?
Thank you!
Regards
Markus Klotzbücher
On Wed, May 31, 2006 at 10:43:13AM +0200, Markus Klotzbücher wrote:
Sure. I basically took the same approach as you suggested in your previous mail, but have split the the lowlevel functions into board and cpu dependant handling. I think this makes sense as boards for example can use the same cpu ohci controller but may require specific board dependant power settings. Either can be chosen by defining CFG_USB_OHCI_BOARD_INIT and CFG_USB_OHCI_CPU_INIT respectively.
I also discovered that the actions taken in case of failure sometimes differ from those to stop the controller, so I added the fail functions.
So I ended up with these hooks:
usb_cpu_init usb_cpu_stop usb_cpu_fail
usb_board_init usb_board_stop usb_board_fail
Ok.
I have currently adapted the monahans, the at91rm9200, and the s3c24x0 cpus to use the generic driver. Please note that I used the cpu/arm920t/at91rm9200/usb_ohci.[hc] driver as a starting point for the generic driver.
File usb_ohci.h is quite the same, but usb_ohci.c has some differences. I decided to start from "cpu/mpc5xxx/usb_ohci.c" since it seemed to have a better events handling. Please, see submit_common_msg() at comment "NOTE: since we are not interrupt driven in U-Boot..." or have a look at:
diff -Ebu cpu/arm920t/at91rm9200/usb_ohci.c cpu/mpc5xxx/usb_ohci.c
where you can better see the new variable "urb_finished".
However my mayor changes was about substitution of m16_swap() with proper ohci_cpu_to_le16() (and similar) and in adding virt_to_phys() and phys_to_virt() functions where needed (see my last patch for au1x00).
We are currently starting a new USB testing branch for testing these and other USB related changes, which should be available in the git repo soon.
Ok.
Your patch looks fine, but would you mind resubmitting it against the USB testing branch using the generic driver (drivers/usb_ohci.c) ?
I'll do it ASAP.
Ciao,
Rodolfo
Hi Rodolfo,
Rodolfo Giometti giometti@linux.it writes:
On Wed, May 31, 2006 at 10:43:13AM +0200, Markus Klotzbücher wrote:
I have currently adapted the monahans, the at91rm9200, and the s3c24x0 cpus to use the generic driver. Please note that I used the cpu/arm920t/at91rm9200/usb_ohci.[hc] driver as a starting point for the generic driver.
File usb_ohci.h is quite the same, but usb_ohci.c has some differences. I decided to start from "cpu/mpc5xxx/usb_ohci.c" since it seemed to have a better events handling. Please, see submit_common_msg() at comment "NOTE: since we are not interrupt driven in U-Boot..." or have a look at:
Yes, the s3c24x0 also uses this, and for now I included it (see S3C24X0_merge #define), although I'm not sure this is really necessary. At least the TRAB board worked fine without. Let's keep it for now.
diff -Ebu cpu/arm920t/at91rm9200/usb_ohci.c cpu/mpc5xxx/usb_ohci.c
where you can better see the new variable "urb_finished".
However my mayor changes was about substitution of m16_swap() with proper ohci_cpu_to_le16() (and similar) and in adding virt_to_phys() and phys_to_virt() functions where needed (see my last patch for au1x00).
I understand that the virt_to_phys() are required, but why do you need the ohci_cpu_to_le16 macros? They seem to be the same as the m16_swap, m32_swap macros?
Your patch looks fine, but would you mind resubmitting it against the USB testing branch using the generic driver (drivers/usb_ohci.c) ?
I'll do it ASAP.
Thanks!
Regards
Markus Klotzbuecher
On Wed, May 31, 2006 at 12:29:50PM +0200, Markus Klotzbücher wrote:
Yes, the s3c24x0 also uses this, and for now I included it (see S3C24X0_merge #define), although I'm not sure this is really necessary. At least the TRAB board worked fine without. Let's keep it for now.
Ok.
I understand that the virt_to_phys() are required, but why do you need the ohci_cpu_to_le16 macros? They seem to be the same as the m16_swap, m32_swap macros?
They are useful only for better reading the code since if I see m16_swap() I may think that the variable _must_ be swapped in any case, but if I read ohci_cpu_to_le16() I well understand that the variable _may_ be swapped according to CPU endianess.
Ciao,
Rodolfo
Rodolfo Giometti giometti@linux.it writes:
On Wed, May 31, 2006 at 12:29:50PM +0200, Markus Klotzbücher wrote:
I understand that the virt_to_phys() are required, but why do you need the ohci_cpu_to_le16 macros? They seem to be the same as the m16_swap, m32_swap macros?
They are useful only for better reading the code since if I see m16_swap() I may think that the variable _must_ be swapped in any case, but if I read ohci_cpu_to_le16() I well understand that the variable _may_ be swapped according to CPU endianess.
Well, then we should probably use the existing macros (include/asm/byteorder.h), but the downside is that this will grow the diffs between the remaining ohci drivers and the generic one which will make merging them more work. I'd prefer to leave cosmetic stuff for now, until the other drivers are merged.
Regards
Markus Klotzbuecher
participants (2)
-
Markus Klotzbücher -
Rodolfo Giometti