linux-kernel - Re: [PATCH 2/5 v11] arm: omap: usb: ehci and ohci hwmod structures for omap3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 24 Sep 2011 01:15:01 -0600 (MDT)
From:	Paul Walmsley <paul@...an.com>
To:	"Munegowda, Keshava" <keshava_mgowda@...com>
cc:	parthab@...ia.ti.com, linux-usb@...r.kernel.org,
	linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
	b-cousson@...com, balbi@...com, gadiyar@...com,
	sameo@...ux.intel.com, tony@...mide.com, khilman@...com,
	johnstul@...ibm.com, vishwanath.bs@...com
Subject: Re: [PATCH 2/5 v11] arm: omap: usb: ehci and ohci hwmod structures
 for omap3

On Fri, 23 Sep 2011, Munegowda, Keshava wrote:

> On Thu, Sep 22, 2011 at 11:31 PM, Paul Walmsley <paul@...an.com> wrote:
> >
> > On Thu, 22 Sep 2011, Keshava Munegowda wrote:
> >> 4. usb_tll_hs hwmod of usbhs with the TLL base address and irq.
> >>
> >> Signed-off-by: Keshava Munegowda <keshava_mgowda@...com>
> >> Reviewed-by: Partha Basak <parthab@...ia.ti.com>
> >> ---
> >>  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |  271 ++++++++++++++++++++++++++++
> >>  1 files changed, 271 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> >> index 59fdb9f..d79f728 100644
> >> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> >> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c

> >> +static struct omap_hwmod_ocp_if omap34xx_f128m_cfg__usb_host_hs = {
> >> +     .clk            = "usbhost_120m_fck",
> >
> > This doesn't look right.  This is an interface structure record, so it
> > should be associated with an interface clock.  Is the hardware really
> > using the functional clock as the interface clock?  Or, as seems more
> > likely...
> 
> 
> Agreed, how about:
> 
> main clock: usbhost_120m_fck
> optional f clock: usbhost_48m_fck

Assuming the interface clock is enabled, which one of these clocks is 
needed for UHH register accesses to complete successfully?

> interface clock: usbhost_ick.
> 
> 
> 
> >> +     .user           = OCP_USER_MPU,
> >> +     .flags          = OCPIF_SWSUP_IDLE,
> >
> > ... is this just a hack?  OCPIF_SWSUP_IDLE is intended to work around
> > hardware autoidle bugs only.  Are you sure this shouldn't be defined as an
> > optional clock instead?
> 
> yes ! it was causeing resets, hence you this flag.

usbhost_120m_fck is not an interface clock.  That is probably what was
causing the problem you describe.

> >> +static struct omap_hwmod omap34xx_usb_host_hs_hwmod = {
> >> +     .name           = "usb_host_hs",
> >> +     .class          = &omap34xx_usb_host_hs_hwmod_class,
> >> +     .main_clk       = "usbhost_ick",
> >
> > Is this really the main clock?  The main clock is the clock that drives
> > the register logic in the IP block.  Looks to me, based on the integration
> > document in the 34xx TRM vZR, that this module has a functional clock.  In
> > general, the only time that the main_clk should be an interface clock is
> > when the clock is a combined interface and functional clock.  The mailbox
> > IP block is a classic example.
> 
> As I mentioned above;  I can make
> 
> main clock: usbhost_120m_fck
> optional f clock: usbhost_48m_fck
> interface clock: usbhost_ick.
> 
> do you agree?

The interface clock should definitely be usbhost_ick, no doubt about it.

But, as I mentioned above, to determine which functional clock should be 
the main clock, you first need to know which of those two clocks need to 
be enabled for register accesses to that module to succeed.

I did this work a few years ago, by trial and error since it was largely 
undocumented.  It was needed since the OMAP3 clk_enable() code waits for 
the PRCM to request that the IP block exit idle before returning.  
You might want to take a look at arch/arm/mach-omap2/clock3xxx_data.c.

> >> +static struct omap_hwmod omap34xx_usb_tll_hs_hwmod = {
> >> +     .name           = "usb_tll_hs",
> >> +     .class          = &omap34xx_usb_tll_hs_hwmod_class,
> >> +     .mpu_irqs       = omap34xx_usb_tll_hs_irqs,
> >> +     .main_clk       = "usbtll_ick",
> >
> > Is this really the main clock?  The main clock is the clock that drives
> > the register logic in the IP block.  Looks to me, based on the integration
> > document in the 34xx TRM vZR, that this module has a functional clock.
> 
> I can make
> 
> main clock: usbtll_fck
> interface clock: usbtll_ick.
> 
> do you agree?

Doesn't that make more sense?


- Paul