From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Sat, 30 Jun 2012 06:14:44 +0200
Subject: [U-Boot] [PATCH v4 0/3] AM335x: Add USB support in u-boot.
In-Reply-To: <CAJ_ou-4J950SbHqHDjGn=3SprwEi_OUxVuKkviM79hMYx=yWZA@mail.gmail.com>
References: <1340969606-8448-1-git-send-email-harman_sohanpal@ti.com>
	<201206300558.29263.marex@denx.de>
	<CAJ_ou-4J950SbHqHDjGn=3SprwEi_OUxVuKkviM79hMYx=yWZA@mail.gmail.com>
Message-ID: <201206300614.44399.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Harman Sohanpal,

> On Sat, Jun 30, 2012 at 9:28 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Harman Sohanpal,
> > 
> >> On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut <marex@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