Re: [PATCH 2/5 v11] arm: omap: usb: ehci and ohci hwmod structuresfor omap3

[Munegowda, Keshava]

On Sat, Sep 24, 2011 at 12:45 PM, Paul Walmsley <paul@xxxxxxxxx> wrote:
> On Fri, 23 Sep 2011, Munegowda, Keshava wrote:
>
>> On Thu, Sep 22, 2011 at 11:31 PM, Paul Walmsley <paul@xxxxxxxxx> 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@xxxxxx>
>> >> Reviewed-by: Partha Basak <parthab@xxxxxxxxxxxx>
>> >> ---
>> >>  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?


it is usbhost_48m_fck ;
so,
main clock: usbhost_48m_fck
optional clock : usbhost_120m_fck

do you agree?

>
>> 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.

yes , you are right ! its not an interface clock.


>
>> >> +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?

yes, I will do this change.


keshava
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/