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

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Govindraj,

On 01/12/12 07:45, Govindraj wrote:
> Hi Igor,
> 
> On Wed, Jan 11, 2012 at 8:33 PM, Igor Grinberg <grinberg@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@gmail.com> wrote:
>>>>>> On Wed, Jan 11, 2012 at 6:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> On Wed, Jan 11, 2012 at 4:22 PM, Marek Vasut <marek.vasut@gmail.com>
>>> wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> Thanks for you review.
>>>>>>>>>>
>>>>>>>>>> On Tue, Jan 10, 2012 at 9:37 PM, Marek Vasut
>>>>>>>>>> <marek.vasut@gmail.com>
>>>>>
>>>>> wrote:
>>>>>>>>>>>> From: "Govindraj.R" <govindraj.raja@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@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.

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.
It is also possible that I will be able to check the above today
or tomorrow, but I can't promise, as I'm really busy now.

Thanks.


-- 
Regards,
Igor.