[U-Boot] [PATCH] usb: ehci-mx6: Fix bus enumeration for DM case
Marek Vasut
marex at denx.de
Mon Jun 24 12:11:01 UTC 2019
On 6/24/19 1:57 PM, Lukasz Majewski wrote:
> Hi Marek,
>
>> On 6/24/19 12:26 PM, Lukasz Majewski wrote:
>>> On Mon, 24 Jun 2019 12:06:28 +0200
>>> Marek Vasut <marex at denx.de> wrote:
>>>
>>>> On 6/24/19 8:33 AM, Peng Fan wrote:
>>>>> Hi Marek,
>>>>>
>>>>>> Subject: Re: [PATCH] usb: ehci-mx6: Fix bus enumeration for DM
>>>>>> case
>>>>>>
>>>>>> On 6/21/19 3:45 AM, Peng Fan wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Marek Vasut [mailto:marex at denx.de]
>>>>>>>> Sent: 2019年6月21日 4:54
>>>>>>>> To: u-boot at lists.denx.de
>>>>>>>> Cc: Marek Vasut <marex at denx.de>; Abel Vesa
>>>>>>>> <abel.vesa at nxp.com>;
>>>>>> Adam
>>>>>>>> Ford <aford173 at gmail.com>; Fabio Estevam
>>>>>>>> <festevam at gmail.com>;
>>>>>> Ludwig
>>>>>>>> Zenz <lzenz at dh-electronics.com>; Peng Fan
>>>>>>>> <peng.fan at nxp.com>;
>>>>>> Stefano
>>>>>>>> Babic <sbabic at denx.de>; Vagrant Cascadian <vagrant at debian.org>
>>>>>>>> Subject: [PATCH] usb: ehci-mx6: Fix bus enumeration for DM case
>>>>>>>>
>>>>>>>> It is likely that the DM conversion of EHCI iMX6 driver was a
>>>>>>>> derivative of EHCI VF, however the conversion is incomplete and
>>>>>>>> is missing the bind workaround, which updates dev->seq number.
>>>>>>>> Without this, all controllers have dev->seq number 0 . Add this
>>>>>>>> bind workaround
>>>>>> into EHCI iMX6 driver as well.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>> Cc: Abel Vesa <abel.vesa at nxp.com>
>>>>>>>> Cc: Adam Ford <aford173 at gmail.com>
>>>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
>>>>>>>> Cc: Ludwig Zenz <lzenz at dh-electronics.com>
>>>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>>>> Cc: Stefano Babic <sbabic at denx.de>
>>>>>>>> Cc: Vagrant Cascadian <vagrant at debian.org>
>>>>>>>> ---
>>>>>>>> drivers/usb/host/ehci-mx6.c | 17 +++++++++++++++++
>>>>>>>> 1 file changed, 17 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/host/ehci-mx6.c
>>>>>>>> b/drivers/usb/host/ehci-mx6.c index 33abfeada0..109ed7ed4a
>>>>>>>> 100644 --- a/drivers/usb/host/ehci-mx6.c
>>>>>>>> +++ b/drivers/usb/host/ehci-mx6.c
>>>>>>>> @@ -503,6 +503,22 @@ static int
>>>>>>>> ehci_usb_ofdata_to_platdata(struct udevice *dev)
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static int ehci_usb_bind(struct udevice *dev) {
>>>>>>>> + static int num_controllers;
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * Without this hack, if we return ENODEV for USB
>>>>>>>> Controller 0, on
>>>>>>>> + * probe for the next controller, USB Controller 1
>>>>>>>> will be given a
>>>>>>>> + * sequence number of 0. This conflicts with our
>>>>>>>> requirement of
>>>>>>>> + * sequence numbers while initialising the
>>>>>>>> peripherals.
>>>>>>>> + */
>>>>>>>> + dev->req_seq = num_controllers;
>>>>>>>
>>>>>>> With alias in dts, no need this, right?
>>>>>>
>>>>>> There are no aliases in the DTs for the USB controllers, so that
>>>>>> doesn't help. I think for DM, the real solution would be to parse
>>>>>> the PHY phandle and pass around the PHY address instead of some
>>>>>> arbitrary index, but that's something to be done for next
>>>>>> release. For this release, it's only fixes.
>>>>>
>>>>> I think the better method is use alias in dts, introducing
>>>>> xx-u-boot.dtsi for such case.
>>>>
>>>> What good would that make if you can obtain the PHY address from
>>>> the DT, without the need for arbitrary error-prone indexes? The
>>>> indexes are remnants from when there was no DM or from the DM
>>>> conversion, so we should remove them altogether.
>>>>
>>>>>> Would you be interested in implementing the later for
>>>>>> -next ? ;-)
>>>>>
>>>>> In NXP vendor tree, we have ehci_usb_get_phy to parse phy from
>>>>> dtb, I could find some time send that to community.
>>>> What about implementing a proper USB PHY driver, which binds to a
>>>> DT entry, and then hooking it up to the EHCI MX6 ? IIRC the
>>>> infrastructure for this is already in place and that would be the
>>>> right (TM) solution.
>>>>
>>>
>>> Sorry, but I'm a bit confused.
>>>
>>> The patch that you introduced above, in the commit message, states
>>> clearly that it is a hack.
>>>
>>> Why shall we allow introducing hacks instead of implementing it in
>>> the "right way" from the outset ?
>>
>> Because we're in RC4 right now and this is the simplest possible fix
>> which works and is consistent with the other NXP EHCI drivers.
>> See the
>> commit message -- this fills in the missing bit which ehci-vf has, but
>> which was omitted from ehci-mx6 and ehci-mx5 during the conversion.
>>
>
> Apparently I was not causing the error which you have encountered.
>
>> The PHY driver and the migration to that infrastructure can be done in
>> the next cycle, but right now there's no time to develop a driver and
>> thoroughly test either the PHY driver or the changes to the EHCI
>> driver, no way.
>>
>>> As you mentioned the infrastructure is already in place, so why not
>>> do it in the correct way?
>>>
>>> And yes, as Peng noted up, till now many boards use aliases to fix
>>> this situation.
>>
>> Right, so this does not work with the USB stack, but feel free to try
>> it yourself. That's why the ehci-vf has this hack in place.
>
> That is why I'm asking as I do use usb on imx53 and imx6q boards. The
> imx53 uses usbh1 which has alias to usb1.
And the device sequence number is consistently 0, right ?
>> And I am in fact fine with this, because you don't end up patching DTs
>> with arbitrary aliases
>
> This patch causes regression (the board hang) when tested on TPC70
> i.MX6Q board.
>
> master branch
> SHA1: 77f6e2dd0551d8a825bab391a1bd6b838874bcd4
>
> Log from u-boot (broken):
>
> Booting update from usb ...
> starting USB...
> Bus usb at 2184200:
>
>
> Working one:
>
> => usb start
> starting USB...
> Bus usb at 2184200: USB EHCI 1.00
> scanning bus usb at 2184200 for devices... 2 USB Device(s) found
> scanning usb for storage devices... 1 Storage Device(s) found
>
>
>> AND because this will code go away once the PHY
>> driver is in place anyway and so would the aliases.
>>
>
> For now it causes working board to hang. Please fix the patch.
I wonder how you managed to trigger this on , presumably , kp_imx6q_tpc
. When I build this target on 77f6e2dd0551d8a825bab391a1bd6b838874bcd4 ,
I get:
===================== WARNING ======================
This board does not use CONFIG_DM_USB. Please update
the board to use CONFIG_DM_USB before the v2019.07 release.
Failure to update by the deadline may result in board removal.
See doc/driver-model/MIGRATION.txt for more info.
====================================================
So the code in this patch doesn't even get compiled.
I presume you have some additional extra patches on top, right ?
Anyway, I cannot debug a board I don't have. Take a look at
ehci_usb_probe() priv->portnr before and after this patch, does it
change ? That's the value from which the PHY index is derived ; if the
PHY index does not match the controller index, the probe hangs, I think
that's what you're hitting.
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list