Hi Marek,
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@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@denx.de] >>> Sent: 2019年6月21日 4:54 >>> To: u-boot@lists.denx.de >>> Cc: Marek Vasut marex@denx.de; Abel Vesa >>> abel.vesa@nxp.com; > Adam >>> Ford aford173@gmail.com; Fabio Estevam >>> festevam@gmail.com; > Ludwig >>> Zenz lzenz@dh-electronics.com; Peng Fan >>> peng.fan@nxp.com; > Stefano >>> Babic sbabic@denx.de; Vagrant Cascadian >>> vagrant@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@denx.de >>> Cc: Abel Vesa abel.vesa@nxp.com >>> Cc: Adam Ford aford173@gmail.com >>> Cc: Fabio Estevam festevam@gmail.com >>> Cc: Ludwig Zenz lzenz@dh-electronics.com >>> Cc: Peng Fan peng.fan@nxp.com >>> Cc: Stefano Babic sbabic@denx.de >>> Cc: Vagrant Cascadian vagrant@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 ?
I've just tested it, without delving into details.
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@2184200:
Working one:
=> usb start starting USB... Bus usb@2184200: USB EHCI 1.00 scanning bus usb@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.
The board is being converted.
I presume you have some additional extra patches on top, right ?
Yes, the conversion patches.
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.
Thanks for the suggestion.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de