[U-Boot] [PATCH v3 2/7] arm: usb: dra7xx: xHCI registers based on USB port index

Mon Feb 20 08:28:23 UTC 2017

On 02/20/2017 08:47 AM, Igor Grinberg wrote:
> Hey Marek,

Hi,

> We haven't spoken for great while... I'm glad to hear from you!

yeah

> On 02/19/17 19:39, Marek Vasut wrote:
>> On 02/19/2017 05:26 PM, Igor Grinberg wrote:
>>> Hi guys,
>>>
>>> On 02/19/17 17:15, Tom Rini wrote:
>>>> On Sun, Feb 19, 2017 at 04:13:02PM +0100, Marek Vasut wrote:
>>>>> On 02/19/2017 03:55 PM, Uri Mashiach wrote:
>>>>>>
>>>>>>
>>>>>> On 02/19/2017 04:27 PM, Marek Vasut wrote:
>>>>>>> On 02/19/2017 02:27 PM, Uri Mashiach wrote:
>>>>>>>> Modify the determination of the base address of xHCI registers of DRA7XX
>>>>>>>> targets.
>>>>>>>> Before the commit: by the target.
>>>>>>>> After the commit: by the USB port index.
>>>>>>>>
>>>>>>>> Cc: Lokesh Vutla <lokeshvutla at ti.com>
>>>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>>>> Signed-off-by: Uri Mashiach <uri.mashiach at compulab.co.il>
>>>>>>>> ---
>>>>>>>> V1 -> V2: Replace the commit "fix XHCI registers base address".
>>>>>>>> V2 -> V3: Replace the commit "reintroduce the CONFIG_AM57XX symbol"
>>>>>>>>
>>>>>>>>  configs/dra7xx_evm_defconfig    |  1 +
>>>>>>>>  configs/dra7xx_hs_evm_defconfig |  1 +
>>>>>>>>  drivers/usb/host/Kconfig        | 16 ++++++++++++++++
>>>>>>>>  include/linux/usb/xhci-omap.h   |  6 ++++--
>>>>>>>>  4 files changed, 22 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/configs/dra7xx_evm_defconfig b/configs/dra7xx_evm_defconfig
>>>>>>>> index 26b26cc..1f47f92 100644
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>>>>>>>> index 5129a57..440fbcf 100644
>>>>>>>> --- a/drivers/usb/host/Kconfig
>>>>>>>> +++ b/drivers/usb/host/Kconfig
>>>>>>>> @@ -43,6 +43,22 @@ config USB_XHCI_ZYNQMP
>>>>>>>>      help
>>>>>>>>        Enables support for the on-chip xHCI controller on Xilinx
>>>>>>>> ZynqMP SoCs.
>>>>>>>>
>>>>>>>> +choice
>>>>>>>> +    prompt "DRA7XX xHCI USB index select"
>>>>>>>> +    depends on DRA7XX
>>>>>>>> +
>>>>>>>> +config USB_XHCI_DRA7XX_INDEX0
>>>>>>>> +    bool "USB0"
>>>>>>>> +    help
>>>>>>>> +      DRA7XX xHCI USB0.
>>>>>>>> +
>>>>>>>> +config USB_XHCI_DRA7XX_INDEX1
>>>>>>>> +    bool "USB1"
>>>>>>>> +    help
>>>>>>>> +      DRA7XX xHCI USB1.
>>>>>>>
>>>>>>> What is this all about ? Shouldn't this come from DT ? And what if I
>>>>>>> want to use both XHCI ? This looks totally bogus ...
>>>
>>> Right, both XHCIs cannot be used with current driver and we do not have
>>> the time to fix it by our own... may be TI has?
>>> Remember, you've accepted the driver and following patches, right?
>>
>> You mean this patch [1] which actually adds the board dependency ?
>> Actually , no , I DID NOT. It's been going on for a while that my
>> role as a USB custodian was actively circumvented and patches which
>> should've gone through the USB tree just did not.
> 
> Ahh.. Sorry, thought you are collecting all the USB patches or at least
> provide your Ack so patches can go through other trees..

So did I ...

>> So please, next time you start blaming someone, get your facts right.
> 
> Yeah, I haven't looked at it thoroughly... That's why the "right?" in the
> end of my sentence. Sorry once again, I did not mean to offend you.
> 
>>
>> I already have almost zero motivation to maintain USB in my free time,
>> being paid nothing for it, not ever hearing a single "thank you" and
>> just spending time I could use otherwise. Being circumvented and only
>> catching shit for problematic patches I did NOT even apply though, that
>> is completely off-putting .
> 
> Ahhhhh... First of all thank you Marek! You've done a great job AFAIR.
> I really mean it.
> And it is a pity, things have gone this way.
> May be this can still be fixed? Tom?

I already had a long discussion with Tom yesterday ... so let's see .

>> [1] https://patchwork.ozlabs.org/patch/700372/
>>
>>> Regarding DT, do we have a DT as a requirement to run USB in U-Boot?
>>
>> It is highly recommended.
> 
> Hmmm... I don't really like this approach as it does not help during the board
> production, where I'd like to see the bootloader detect as much a it can
> during runtime and then flash the board with correct DT...

You can enable all during production, plug USB sticks into every port,
check which controllers detect devices and then only keep those enabled
via the "status" DT prop. As a bonus, you get functional test.

Does the OMAP5 not use DT yet ?

>>> I don't remember this happening and I think it shouldn't be a requirement.
>>>
>>>>>>>
>>>>>>
>>>>>> The support for both XHCI is currently missing.
>>>>>> This could be a temporary solution until the DT solution.
>>>>>> The current situation is worse - selecting USB0 or USB1 based on the
>>>>>> target.
>>>>>
>>>>> So we're replacing it with equally bad solution ?
>>>
>>> I don't think equally applies here...
>>> This IS an improvement. Of course it is not like you would want it to be,
>>> but still it is from a platform POV.
>>
>> TBH, revering 042fdb7cabb8270eb86c45f11263fa91c12e3b65 might be the way
>> to go.
> 
> Ok. I'm fine with this approach.
> I'm also fine with if the driver gets fixed and both XHCI instances can be used.

Someone would have to put that effort in.

I wonder if instead of defining INDEX0, INDEX1 ... this could be turned
into integer ...

>>
>>>>> Hmmm , no.
>>>>> The MW will open mid-march, there's about a month to fix this,
>>>>> so please do.
>>>>
>>>> Do note that the relevant driver here is not yet using DM_USB.
>>>
>>> Yes, the driver should be fixed some day. We would really like to take
>>> this task, but unfortunately, we cannot, at least not right now.
>>> But we do need that USB working on our board and not only on TI EVMs...
>>
>> See above, I believe 042fdb7cabb8270eb86c45f11263fa91c12e3b65 is bogus.
> 
> Well, agreed.
> 
>>
>>> Tom,
>>> Should we fall back to v1 and have a worse solution for the base addresses?
>>
>> OK, I see me being USB custodian has exactly zero value now ... good to
>> know.
> 
> I believe we should start fixing things and probably the best time
> would be... now? I mean, we can't fix things in the past, right?
> 

-- 
Best regards,
Marek Vasut