[U-Boot] [PATCH v2 1/4] ehci-omap: Clean up added ehci-omap.c

[Govindraj]

On Thu, Jan 12, 2012 at 4:22 PM, Govindraj <govindraj.ti at gmail.com> wrote:
> On Thu, Jan 12, 2012 at 2:53 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>> Hi Govindraj,
>>
>> On 01/12/12 07:45, Govindraj wrote:
>>> Hi Igor,
>>>
>>> On Wed, Jan 11, 2012 at 8:33 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>>> Hi Guys,
>>>>
>>>> On 01/11/12 16:34, Marek Vasut wrote:
>>>>>> On Wed, Jan 11, 2012 at 6:58 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>> On Wed, Jan 11, 2012 at 6:16 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>> On Wed, Jan 11, 2012 at 4:22 PM, Marek Vasut <marek.vasut at gmail.com>
>>>>> wrote:
>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for you review.
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Jan 10, 2012 at 9:37 PM, Marek Vasut
>>>>>>>>>>>> <marek.vasut at gmail.com>
>>>>>>>
>>>>>>> wrote:
>>>>>>>>>>>>>> From: "Govindraj.R" <govindraj.raja at ti.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Clean up added ehci-omap.c and make it generic for re-use
>>>>>>>>>>>>>> across soc having same ehci ip block. Also pass the modes to
>>>>>>>>>>>>>> be configured and configure the ports accordingly. All usb
>>>>>>>>>>>>>> layers are not cache aligned till then keep cache off for usb
>>>>>>>>>>>>>> ops as ehci will use internally dma for all usb ops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> * Add a generic common header ehci-omap.h having common ip
>>>>>>>>>>>>>> block data and reg shifts.
>>>>>>>>>>>>>> * Rename and modify ehci-omap3 to ehci.h retain only
>>>>>>>>>>>>>> conflicting sysc reg shifts remove others and move to common
>>>>>>>>>>>>>> header file.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Don't reimplement the ulpi stuff ... there's already some ulpi
>>>>>>>>>>>>> stuff in uboot that needs fixing, so fix it and use it.
>>>>>>>>>>>>
>>>>>>>>>>>> I am not implementing any ulpi stuff I am just configuring OMAP on
>>>>>>>>>>>> soc usb host controller (ehci). All the configuration stuff
>>>>>>>>>>>> is OMAP specific things which are done in ehci-omap.c file
>>>>>>>>>>>>
>>>>>>>>>>>> stuffs done are like soft-reset, port mode to be used and putting
>>>>>>>>>>>> port in no -idle mode(omap specific pm implementation) etc.
>>>>>>>>>>>
>>>>>>>>>>> This stuff:
>>>>>>>>>>>
>>>>>>>>>>> +/* ULPI */
>>>>>>>>>>> +#define ULPI_SET(a)                                    (a + 1)
>>>>>>>>>>> +#define ULPI_CLR(a)                                    (a + 2)
>>>>>>>>>>> +#define ULPI_FUNC_CTRL                                 0x04
>>>>>>>>>>> +#define ULPI_FUNC_CTRL_RESET                           (1 << 5)
>>>>>>>>>>>
>>>>>>>>>>> is just accidentally conforming to ULPI spec?
>>>>>>>>>>
>>>>>>>>>> These are for configuring INSNREG05_ULPI reg in EHCI reg map
>>>>>>>>>> of omap while configuring in ulpi-phy mode.
>>>>>>>>>>
>>>>>>>>>> looking into struct ulpi_regs {..}
>>>>>>>>>> then it doesn't map this configuration.
>>>>>>>>>
>>>>>>>>> Can you point me to some documentation about this please? It's not
>>>>>>>>> that I don't trust you, I'd rather prefer to avoid unnecessary
>>>>>>>>> duplication.
>>>>>>>>
>>>>>>>> Yes that would be fine.
>>>>>>>>
>>>>>>>> You can download the omap4460 public trm from here:
>>>>>>>>
>>>>>>>> http://www.ti.com/pdfs/wtbu/OMAP4460_ES.1x_PUBLIC_TRM_vM.zip
>>>>>>>>
>>>>>>>> Go to chapter 23.11.6.6.1 EHCI Register Summary
>>>>>>>> (page number 5171 and 5186/87)
>>>>>>>
>>>>>>> Sure, but the macro above looks more like 23.11.6.3, doesn't it ? And for
>>>>>>> that purpose, the struct ulpi_regs is fitting ok.
>>>>>>>
>>>>>>> Actually ... can you check the ulpi_read and ulpi_write stuff that's
>>>>>>> already in u-boot and explain why they can not be used with this port?
>>>>>>
>>>>>> echi-omap.c is no where writing to those registers
>>>>>> and the macro was used only to configure INSNREG05_ULPI reg in EHCI reg map
>>>>>>
>>>>>> reg map in 23.11.6.3 used only for a external ulpi-phy communication.
>>>>>> and debug purpose(to view vid, pid etc) and to hack external phy
>>>>>> configuration through ulpi commands
>>>>>> from omap - usb host controller point of view only needs
>>>>>> INSNREG05_ULPI reg in EHCI reg configuration
>>>>>> rest on soc host controller takes care of it.
>>>>>
>>>>> Can someone else comment on this? I think I don't understand well (as I'm not
>>>>> OMAP guy).
>>>>
>>>> Well, it is on my list, actually,
>>>> but I will be able to get to it only in a couple of days.
>>>> (I'm really busy right now).
>>>>
>>>
>>> Could you please let me know what exactly that you will be
>>> updating?
>>>
>>> So that I can accordingly post my v3 of this patch fixing comments
>>> from Marek Vasut <marek.vasut at gmail.com>
>>
>> Well, I did not say, I'm going to update anything.
>> What I meant is that I'm going to look into TI's documentation
>> regarding EHCI and the ULPI to understand the dependencies and see
>> how your code meets those and if the generic ULPI layer can be used
>> for that.
>>
>
> okay, Thanks,
>
>> So to lower the work load from from you, I'd suggest you to wait
>> till Monday (if you can of course) to let me look into this.
>

Gentle Ping.

And just to clarify further there is no code duplication for ulpi read writes
in ehci-omap.c done with this patch.
Patch intends only in configuring ehci to right modes as specified by
board file.  Modes possible are hsic_mode, tll_mode, ulpi_phy mode.

This patch is derived with reference to linux kernel and even there
we can see that no ulpi_reg map registers are used and only ehci
is configured is respective mode as passed by board data.

Here [1] is the patch fixing Marek Vasut <marek.vasut at gmail.com>
comments.

This corrected patch along with dependent patch from
Ilya Yanok <yanok at emcraft.com>
[PATCH V4 1/2] ehci-omap: driver for EHCI host on OMAP3

Is available here:
git://gitorious.org/denx_u-boot/denx_uboot_omap.git v2_ehci_omap

--
Thanks,
Govindraj.R

[1]: