[U-Boot] [PATCH v4 0/3] AM335x: Add USB support in u-boot.

[Marek Vasut]

Dear Harman Sohanpal,

> On Sat, Jun 30, 2012 at 9:28 AM, Marek Vasut <marex at denx.de> wrote:
> > Dear Harman Sohanpal,
> > 
> >> On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut <marex at denx.de> wrote:
> >> > Dear Harman Sohanpal,
> >> > 
> >> >> These patches add USB support in u-boot for AM335x.
> >> >> The support for host or device is selected
> >> >> depending on the config selected from boards.cfg file.
> >> >> Host mode is selected for USB1 and device mode is
> >> >> selected for USB0.
> >> >> Base addresses are selected accordingly.
> >> >> 
> >> >> Gene Zarkhin (1):
> >> >>   AM335x : Add USB support for AM335x in u-boot
> >> >> 
> >> >> Harman Sohanpal (2):
> >> >>   AM335x : Configs to add USB host support.
> >> >>   musb_udc : Fix compile warning.
> >> > 
> >> > Dumb question ... but, can this not be made part of am35x USB ?
> >> 
> >> Hi Marek,
> >> Well this can always be made part of am35x.c.
> >> But there would be a lot of changes required in the file.
> > 
> > Why? They use different IP block or something?
> > 
> >> And also I believe it would not make much sense.
> >> It would require ifdefs at a lot of places.
> >> Best example I can give to support what i said is
> >>  that the control register
> >> is at an offset of 4 in am35x and 14 in am335x.
> > 
> > So what, define two sets of register structures and pass them according
> > to CPU ID.
> > 
> >> I am sure adding an ifdef at that place would not seem
> >> good to you to change address from 4 to 14 acc to platform.
> > 
> > well ...
> > struct regs_a {
> >        uint32_t padding[?];
> >        uint32_t reg;
> > ...
> > };
> > 
> > struct regs_b {
> >        uint32_t reg;
> > ...
> > };
> > 
> > Create IO accessors ... like ... my_usb_writel() and my_usb_readl() to
> > read and write the registers. And those accessors will cover the
> > differences. Or is there more?
> > 
> >> Is there much pain to add these 2 files?
> > 
> > Yes, duplication of code is always bad.
> > 
> >> In my opinion we must need to have a separate file for this.
> > 
> > Why? If it's only about the registers, won't the approach above work?
> > 
> >> This is as per my understanding.
> >> It could also cause confusions to some due to name. maybe :)
> > 
> > I'm no omap guru, Tom is. Tom?
> > 
> >> Kindly give your thoughts.
> > 
> > Oh my brain is spinning from this :-)
> > 
> >> In case still some changes are required, we can think upon it :)
> > 
> > I'm really glad to hear that, let's do our best to find the best possible
> > solution :-)
> > 
> >> Thanks,
> >> Harman
> > 
> > Best regards,
> > Marek Vasut
> 
> Also we are adding the USB module to be selected dynamically
> from boards.cfg as suggested by Tom.
> If such is the case I believe the same should be done for am35x.
> Also from the previous mail of Tom, he seemed to be fine with
> separate files.
> The best approach would be to wait for Tom's reply.
> Thanks for the review :)

Hm, previous review? Was I not CCed on it maybe ? I'll check it.

> Harman

Best regards,
Marek Vasut