[U-Boot-Users] USB OHCI drivers unification

[U-Boot-Users] USB OHCI drivers unification

[Rodolfo Giometti]

Hello,

I'm trying to unify the several USB OHCI drivers. First of all I
created two new files called "common/usb_ohci.c" and
"include/usb_ohci.h".

Latter file is just a copy of several OHCI headers (for instance I
copy the file cpu/mips/au1x00_usb_ohci.h) and the former is an OHCI
driver modified as follow:

Functions "usb_lowlevel_init()" and "usb_lowlevel_stop()" have the
form:

   int usb_lowlevel_init(void)
   {
           /* Specific board OHCI init */
           usb_ohci_lowlevel_init();

           dbg("OHCI board setup complete\n");

           memset (&gohci, 0, sizeof (ohci_t));
           memset (&urb_priv, 0, sizeof (urb_priv_t));

           ... bla bla ...

           dbg("OHCI revision: 0x%08x\nRH: a: 0x%08x b: 0x%08x\n",
                   readl(&gohci.regs->revision),
                   readl(&gohci.regs->roothub.a), readl(&gohci.regs->roothub.b));

           /* enable host controller */
           if (usb_ohci_lowlevel_enable() < 0)
                   goto errout;

           dbg("OHCI clock running\n");

           if (hc_reset (&gohci) < 0)
                   goto errout;

   	... bla	bla ...

   errout:
           err("OHCI initialization error\n");
           hc_release_ohci (&gohci);

           /* Initialization failed */
	   usb_ohci_lowlevel_disable();

           return -1;
   }

   int usb_lowlevel_stop(void)
   {
           /* this gets called really early - before the controller has */
           /* even been initialized! */
           if (!ohci_inited)
                   return 0;

           /* TODO release any interrupts, etc. */
           /* call hc_release_ohci() here ? */
           hc_reset (&gohci);

           /* disable host controller */
           usb_ohci_lowlevel_disable();

           return 0;
   }

Where the new functions "usb_ohci_lowlevel_init()",
"usb_ohci_lowlevel_enable()" and "usb_ohci_lowlevel_disable()" will be
defined per CPUs into their own directories.

I'd like to know if this modification can be acceptable and who can
help me in testing and doing the patch for each platform using USB
OHCI. I can test and patch my board with MIPS au1100.

Also, I'd like to know how I should build my patch since I'm quite
sure that it could be greater than 40KB...

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[U-Boot-Users] USB OHCI drivers unification

[Wolfgang Denk]

Dear Rodolfo,

in message <20060530140405.GB21734@enneenne.com> you wrote:
> 
> I'm trying to unify the several USB OHCI drivers. First of all I
> created two new files called "common/usb_ohci.c" and
> "include/usb_ohci.h".

Please negotiate your efforts with Markus Klotzb?cher <mk@denx.de>
(on Cc:) who is also working on this.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
As far as we know, our computer has never had an undetected error.
		                                           -- Weisert

[U-Boot-Users] USB OHCI drivers unification

[Markus Klotzbücher]

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

[U-Boot-Users] USB OHCI drivers unification

[Rodolfo Giometti]

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

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060531/121cfa44/attachment.pgp 

[U-Boot-Users] USB OHCI drivers unification

[Markus Klotzbücher]

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

[U-Boot-Users] USB OHCI drivers unification

[Rodolfo Giometti]

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

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060531/952271c0/attachment.pgp 

[U-Boot-Users] USB OHCI drivers unification

[Markus Klotzbücher]

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