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

[Igor Grinberg]

Hi Govindraj,

I'm sorry it took me so much time, I just was very busy these days...
I've looked in the TRMs of both OMAP3 and OMAP4, please see below.

On 01/12/12 12:52, Govindraj 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.

Why do you keep saying that it needs fixing?
Can you point to a specific problem?

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

Yes, it is OMAP specific, but only the INSNREG05_ULPI register part...

>>>>>>>>>>>>
>>>>>>>>>>>> stuffs done are like soft-reset, port mode to be used and putting
>>>>>>>>>>>> port in no -idle mode(omap specific pm implementation) etc.

Well, the soft-reset, does not fit here...
The soft reset is defined by the ULPI spec and there is already an
implementation for this (as defined by the ULPI spec.)

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

By writing to the INSNREG05_ULPI register, you do not configure it,
but rather initiate the ULPI transaction to the ULPI PHY.

>>>>>>>>>>
>>>>>>>>>> looking into struct ulpi_regs {..}
>>>>>>>>>> then it doesn't map this configuration.

ulpi_regs {...} provides a kind of virtual mapping (to conform to U-Boot
way of accessing the registers) for the ULPI registers inside the ULPI PHY,
which are accessible in a platform specific manner which is sometimes called
a ULPI viewport.
In case of OMAP, the ULPI viewport is the INSNREG05_ULPI register.

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

No, Marek, there is another thing inside OMAP, which is called TLL.
It provides a kind of virtual ULPI interface and can be used instead of ULPI
PHY for the on-board connected devices.
TLL is not related to this discussion.

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

Marek, OMAP has a different ULPI viewport register structure, so
to reuse the ULPI layer, the omap-ulpi-viewport.c should be implemented
with its own ULPI accessors (e.g. ulpi_{read|write}()).

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

This is not exactly true...
Indeed, the INSNREG05_ULPI register is in EHCI registers address space.
In fact it does not meter where that register is located.

After those macros was written to the INSNREG05_ULPI register,
the ULPI transaction is issued to write the value inside the ULPI register.

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

Almost... TLL is exactly the opposite... It is a PHY-less configuration.
It is the TLL ULPI pseudo registers and it is not related to the discussion.

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

right, but it is the ULPI PHY access register and you do not
configure it, but access the ULPI PHY registers through this register.

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

Ok, now I've done that, and commented above.
So, IMO, yes the ULPI layer should be reused for the ULPI access, but
we have here a problem which needs a solution:
Current ULPI implementation does not support the multi-port mode of the
ULPI viewport register (PORTSEL bit on OMAP or ULPIPORT bit on iMX35).
Apparently, on iMX (e.g. efikamx) it is used in non-multi port mode
or the port is always 0.
Currently, there is no way to pass this parameter to the ULPI
viewport implementation.

I will send some more feedback on your latest patch tomorrow.

-- 
Regards,
Igor.