[U-Boot] [PATCH 2/3] ARM: Tegra: USB: Add driver support for Tegra30/Tegra114

[Marek Vasut]

Dear Tom Rini,

> On Wed, May 01, 2013 at 09:16:45AM -0700, Tom Warren wrote:
> > Tom,
> > 
> > On Tue, Apr 30, 2013 at 10:20 AM, Tom Rini <trini at ti.com> wrote:
> > > On Tue, Apr 30, 2013 at 09:21:18AM -0700, Tom Warren wrote:
> > > > Marek,
> > > > 
> > > > > -----Original Message-----
> > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > Sent: Monday, April 29, 2013 4:47 PM
> > > > > To: Jim Lin
> > > > > Cc: u-boot at lists.denx.de; Tom Warren; Stephen Warren
> > > > > Subject: Re: [PATCH 2/3] ARM: Tegra: USB: Add driver support for
> > > > > Tegra30/Tegra114
> > > > > 
> > > > > Dear Jim Lin,
> > > > > 
> > > > > > Tegra30 and Tegra114 are compatible except 1. T30 takes 55 ms to
> > > > > > finish Port Reset. T114 takes
> > > > > > 
> > > > > >    50 ms.
> > > > > > 
> > > > > > 2. PLL parameters
> > > > > > 
> > > > > > Tested on Tegra20 Harmony/Seaboard, Tegra30 Cardhu, and Tegra114
> > > > > > Dalmore platforms. All works well.
> > > > > > 
> > > > > > Signed-off-by: Jim Lin <jilin at nvidia.com>
> > > > > > ---
> > > > > > 
> > > > > >  arch/arm/include/asm/arch-tegra/clk_rst.h  |   10 +
> > > > > >  arch/arm/include/asm/arch-tegra/usb.h      |  249
> > > > > >  ------------------ arch/arm/include/asm/arch-tegra114/tegra.h |
> > > > > >     1 +
> > > > > >  arch/arm/include/asm/arch-tegra114/usb.h   |  287
> > > > > 
> > > > > +++++++++++++++++++++
> > > > > 
> > > > > >  arch/arm/include/asm/arch-tegra20/usb.h    |  279
> > > > > 
> > > > > +++++++++++++++++++++
> > > > > 
> > > > > >  arch/arm/include/asm/arch-tegra30/usb.h    |  294
> > > > > 
> > > > > ++++++++++++++++++++++
> > > > > 
> > > > > Do we now have three copies of the same stuff ?
> > > > 
> > > > When only T20 was supported (for USB), there was a common
> > > > (arch-tegra/usb.h) header. That's been moved to arch-tegra20/usb.h,
> > > > and (unfortunately) there are 2 new usb.h files due to the HW
> > > > differences in the registers between T20 and T30/T114.  I don't see
> > > > any easy way to have a common usb.h file for Tegra w/o adding ugly
> > > > #ifdefs to the USB register space struct(s).
> > > 
> > > Just how different are they?  Are all of the related defines and masks
> > > different too?  Do we have conflicts? Moved registers?  Just
> > > conflicting values?  A quick peek shows '30' and '114' are pretty
> > > similar, except for masks.  Maybe splitting the struct up so you can
> > > discard some of the reserved gaps, run-time checking to see if we can
> > > or cannot use a particular part of the struct?
> > 
> > This is really Jim's patchset (and his specialty), but here's what I know
> > about Tegra USB regs:
> > 
> > T20 had a gap in the registers @ offset 0x130. T30 (and T114) moved the
> > offset of the command/status/interrupt regs down to fill in this gap,
> > which dragged all the subsequent registers back 16 bytes. The two SoCs
> > 'families' sync up again at offset 0x400 and are pretty much equal from
> > there on out to 0x840.
> > 
> > The defines are probably 90% the same, with some weirdness for the first
> > USB controller (USB1) and its PTS/STS bits that differs in offset from
> > the other 2 controllers (again, no clue why the HW guys would do this).
> > 
> > So we could have the 3 different USB headers in the arch-tegraXX area
> > contain the register structs, and have a common arch-tegra/usb.h that has
> > the #defines that are the same, and is included in the arch-tegraxx/usb.h
> > files. That would reduce this down somewhat, without the ugliness of
> > #ifdefs in the structs.
> > 
> > What do you think?
> 
> Sounds like the best we can do then.  It's probable that trying to
> define USB_REGMAP_GAPSIZE1/2 or whatever to do it on the fly would just
> be uglier still.  Thanks!

This is a problem with the struct-based access indeed. I agree with Tom it'd be 
worth to at least try distilling the common part into header shared between 
those three CPUs.

btw you're also adding some kernel-doc-alike annotations to functions, why don't 
you follow kerneldoc style altogether?

Best regards,
Marek Vasut