Re: [PATCH 1/5 v8] arm: omap: usb: ehci and ohci hwmod structures for omap4

[Munegowda, Keshava]

On Tue, Sep 20, 2011 at 2:03 AM, Cousson, Benoit <b-cousson@xxxxxx> wrote:
> OK, it looks like the second half of the answer was in a second email...
> that makes sense:-)
>
> On 9/15/2011 9:22 AM, Munegowda, Keshava wrote:
>>
>> On Thu, Sep 15, 2011 at 11:25 AM, Munegowda, Keshava
>> <keshava_mgowda@xxxxxx>  wrote:
>>>
>>> On Wed, Sep 14, 2011 at 10:20 PM, Cousson, Benoit<b-cousson@xxxxxx>
>>>  wrote:
>>>>
>>>> Hi Keshava,
>>>>
>>>> On 8/25/2011 9:01 AM, Munegowda, Keshava wrote:
>>>>>
>>>>> From: Benoit Cousson<b-cousson@xxxxxx>
>>>>>
>>>>> Following 4 hwmod structures are added:
>>>>> UHH hwmod of usbhs with uhh base address and functional clock,
>>>>> EHCI hwmod with irq and base address,
>>>>> OHCI hwmod with irq and base address,
>>>>> TLL hwmod of usbhs with the TLL base address and irq.
>>>>>
>>>>> Signed-off-by: Benoit Cousson<b-cousson@xxxxxx>
>>>>
>>>> That version is really different compared to my original patch, so you
>>>> should highlight the diff you introduced.
>>
>> Since there are too many changes are done compare to your original
>> patch; i prefer keep a single patch,rather
>> keeping your original patch and my changes are another patch.
>
> This is not really what I was asking for. You changed at least 30% of the
> original patch without mentioning anything in the changelog.
> You should at least add a history to clarify what part you edited / added
> compared to the original.
>
>>>>> Signed-off-by: Keshava Munegowda<keshava_mgowda@xxxxxx>
>>>>> ---
>>>>>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  247
>>>>> ++++++++++++++++++++++++++++
>>>>>   1 files changed, 247 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>>>> b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>>>> index 6201422..0bc01dd 100644
>>>>> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>>>> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>>>> @@ -68,6 +68,10 @@ static struct omap_hwmod omap44xx_mmc2_hwmod;
>>>>>   static struct omap_hwmod omap44xx_mpu_hwmod;
>>>>>   static struct omap_hwmod omap44xx_mpu_private_hwmod;
>>>>>   static struct omap_hwmod omap44xx_usb_otg_hs_hwmod;
>>>>> +static struct omap_hwmod omap44xx_usb_host_hs_hwmod;
>>>>> +static struct omap_hwmod omap44xx_usbhs_ohci_hwmod;
>>>>> +static struct omap_hwmod omap44xx_usbhs_ehci_hwmod;
>>>>> +static struct omap_hwmod omap44xx_usb_tll_hs_hwmod;
>>>>
>>>> None of the 3 last entries are master, and thus should not need any
>>>> backward declaration.
>>
>> yes, I will make this change.
>
> But you didn't...

If I remove these backward declaration It is causing compilation error;
This is because omap44xx_l4_cfg__usbhs_ohci structures includes
omap44xx_usbhs_ohci_hwmod structure and then this structure
omap44xx_usbhs_ohci_hwmod
is defined; you need backward declaration;
please let me know if i am missing anything here.


>>>>>   /*
>>>>>    * Interconnects omap_hwmod structures
>>>>> @@ -5336,6 +5340,245 @@ static struct omap_hwmod
>>>>> omap44xx_wd_timer3_hwmod = {
>>>>>       .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
>>>>>   };
>>>>>
>>>>> +/*
>>>>> + * 'usb_host_hs' class
>>>>> + * high-speed multi-port usb host controller
>>>>> + */
>>>>> +static struct omap_hwmod_ocp_if omap44xx_usb_host_hs__l3_main_2 = {
>>>>> +     .master         =&omap44xx_usb_host_hs_hwmod,
>>>>> +     .slave          =&omap44xx_l3_main_2_hwmod,
>>>>> +     .clk            = "l3_div_ck",
>>>>> +     .user           = OCP_USER_MPU | OCP_USER_SDMA,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_class_sysconfig omap44xx_usb_host_hs_sysc = {
>>>>> +     .rev_offs       = 0x0000,
>>>>> +     .sysc_offs      = 0x0010,
>>>>> +     .syss_offs      = 0x0014,
>>>>> +     .sysc_flags     = (SYSC_HAS_MIDLEMODE | SYSC_HAS_SIDLEMODE),
>>>>> +     .idlemodes      = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART |
>>>>> +                             SIDLE_SMART_WKUP | MSTANDBY_FORCE |
>>>>> +                             MSTANDBY_NO | MSTANDBY_SMART |
>>>>> +                             MSTANDBY_SMART_WKUP),
>>>>
>>>> Minor, but it should be:
>>>> +       .idlemodes      = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART |
>>>> +                          SIDLE_SMART_WKUP | MSTANDBY_FORCE |
>>>> MSTANDBY_NO |
>>>> +                          MSTANDBY_SMART | MSTANDBY_SMART_WKUP),
>>>>
>>>>> +     .sysc_fields    =&omap_hwmod_sysc_type2,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_class omap44xx_usb_host_hs_hwmod_class = {
>>>>> +     .name = "usbhs_uhh",
>>>>> +     .sysc =&omap44xx_usb_host_hs_sysc,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_ocp_if *omap44xx_usb_host_hs_masters[] = {
>>>>> +&omap44xx_usb_host_hs__l3_main_2,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_addr_space omap44xx_usb_host_hs_addrs[] = {
>>>>> +     {
>>>>> +             .name           = "uhh",
>>>>
>>>> In general, there is no name for unique entry. And if you need a name,
>>>> you should find something relevant considering this is local to the hwmod.
>
> No answer and no change on that one.

The usbhs driver internally uses platform_get_resource_byname , hence
it is useful.


>>>>> +             .pa_start       = 0x4a064000,
>>>>> +             .pa_end         = 0x4a0647ff,
>>>>> +             .flags          = ADDR_TYPE_RT
>>>>> +     },
>>>>> +     {} /* Terminating Entry */
>>>>
>>>> That comment is useless. Paul added one space inside the terminator as
>>>> well.
>>>>
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_ocp_if omap44xx_l4_cfg__usb_host_hs = {
>>>>> +     .master         =&omap44xx_l4_cfg_hwmod,
>>>>> +     .slave          =&omap44xx_usb_host_hs_hwmod,
>>>>> +     .clk            = "l4_div_ck",
>>>>> +     .addr           = omap44xx_usb_host_hs_addrs,
>>>>> +     .user           = OCP_USER_MPU | OCP_USER_SDMA,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_ocp_if *omap44xx_usb_host_hs_slaves[] = {
>>>>> +&omap44xx_l4_cfg__usb_host_hs,
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod omap44xx_usb_host_hs_hwmod = {
>>>>> +     .name           = "usbhs_uhh",
>>>>> +     .class          =&omap44xx_usb_host_hs_hwmod_class,
>>>>> +     .clkdm_name     = "l3_init_clkdm",
>>>>> +     .main_clk       = "usb_host_hs_fck",
>>>>> +     .prcm = {
>>>>> +             .omap4 = {
>>>>> +                     .clkctrl_offs =
>>>>> OMAP4_CM_L3INIT_USB_HOST_CLKCTRL_OFFSET,
>>>>> +                     .context_offs =
>>>>> OMAP4_RM_L3INIT_USB_HOST_CONTEXT_OFFSET,
>>>>> +                     .modulemode   = MODULEMODE_SWCTRL,
>>>>> +             },
>>>>> +     },
>>>>> +     .slaves         = omap44xx_usb_host_hs_slaves,
>>>>> +     .slaves_cnt     = ARRAY_SIZE(omap44xx_usb_host_hs_slaves),
>>>>> +     .masters        = omap44xx_usb_host_hs_masters,
>>>>> +     .masters_cnt    = ARRAY_SIZE(omap44xx_usb_host_hs_masters),
>>>>> +     .flags          = HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY,
>>>>> +     .omap_chip      = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
>>>>> +};
>>>>> +
>>>>> +/* 'usbhs_ohci' class */
>>>>> +static struct omap_hwmod_class omap44xx_usbhs_ohci_hwmod_class = {
>>>>> +     .name = "usbhs_ohci",
>>>>
>>>> In the context of devicetree and considering what Felipe did for the
>>>> USB3, I'm wondering if you should define hwmods for ohci and ehci.
>>>> I'm not 100% sure it will apply here, but cannot you add the register
>>>> entries inside the usb_host_hs hwmod and create the devices using that?
>>>>
>>>> There is no PRCM entry for them so they do not need to be omap_device.
>>
>>
>> you need , ehci and ohci as a different hwmods, because later we can
>> add the mux to ehc and ochi independently.
>
> So what? Why do you need 2 hwmods for that? cannot you have regular
> platform_device for that purpose?
>
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_irq_info omap44xx_usbhs_ohci_irqs[] = {
>>>>> +     { .name = "ohci-irq", .irq = 76 + OMAP44XX_IRQ_GIC_START },
>>>>
>>>> Same comment that before about address space name.
>>>>
>>>>> +     { .irq = -1 } /* Terminating IRQ */
>>>>> +};
>>>>> +
>>>>> +static struct omap_hwmod_addr_space omap44xx_usbhs_ohci_addrs[] = {
>>>>> +     {
>>>>> +             .name           = "ohci",
>>>>
>>>> Same comment than before.
>>>>
>>>>> +             .pa_start       = 0x4A064800,
>>>>> +             .pa_end         = 0x4A064BFF,
>>>>> +             .flags          = ADDR_MAP_ON_INIT
>>>>
>>>> Why do you need that flag?
>>
>> This address space does not has sysconfig; so hwmod need not to map to
>> this address.
>> driver will to ioremap.
>
> I do agree for the first part, but that does not explain why you did add
> that flag? Have you check the behavior of that flag?
> No using the ADDR_TYPE_RT does not mean you have to add that flag.

In hwmod code, I am seeing that ioremap is done if ADDR_TYPE_RT is set.
But, for ehci and ohci address space , the usbhs driver does it
ioremap indirectly
by child drivers ehci and ohci; hence I feel ADDR_MAP_ON_INIT sufficient.

regards
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/