[U-Boot] [PATCH] usb: ehci-mx6: Fix bus enumeration for DM case
Lukasz Majewski
lukma at denx.de
Mon Jun 24 13:09:30 UTC 2019
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 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 ?
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 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.
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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190624/f3803a0d/attachment.sig>
More information about the U-Boot
mailing list