Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/2/21 2:15 AM, Tim Harvey wrote:
> > There is no need to set and/or detect mode in of_to_plat
>
> Are you absolutely sure of that ?
>
> It used to be necessary to know the core mode early to determine which
> driver (the gadget or host) to probe, that's why this code was in
> of_to_plat.
>

Marek,

I can't find a reason for the core mode to be known early and have
tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
tested this change also (from another thread) with SPL SDP on IMX8MM
(granted the SPL is not using DM_USB for that).

It appears I accidentally forgot to cc the uboot list so I'll need to
resend this at any rate.

Best regards,

Tim

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Marek Vasut]

On 7/2/21 9:43 PM, Tim Harvey wrote:
> On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/2/21 2:15 AM, Tim Harvey wrote:
>>> There is no need to set and/or detect mode in of_to_plat
>>
>> Are you absolutely sure of that ?
>>
>> It used to be necessary to know the core mode early to determine which
>> driver (the gadget or host) to probe, that's why this code was in
>> of_to_plat.
>>
> 
> Marek,
> 
> I can't find a reason for the core mode to be known early and have
> tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
> tested this change also (from another thread) with SPL SDP on IMX8MM
> (granted the SPL is not using DM_USB for that).
> 
> It appears I accidentally forgot to cc the uboot list so I'll need to
> resend this at any rate.

If I recall it correctly, the mode determination happened early because 
it was necessary to find out which driver to probe (this one, or the 
samsung gadget one) before it actually probed. I'm not sure myself if 
this is still even applicable.

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Fri, Jul 2, 2021 at 12:53 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/2/21 9:43 PM, Tim Harvey wrote:
> > On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/2/21 2:15 AM, Tim Harvey wrote:
> >>> There is no need to set and/or detect mode in of_to_plat
> >>
> >> Are you absolutely sure of that ?
> >>
> >> It used to be necessary to know the core mode early to determine which
> >> driver (the gadget or host) to probe, that's why this code was in
> >> of_to_plat.
> >>
> >
> > Marek,
> >
> > I can't find a reason for the core mode to be known early and have
> > tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
> > tested this change also (from another thread) with SPL SDP on IMX8MM
> > (granted the SPL is not using DM_USB for that).
> >
> > It appears I accidentally forgot to cc the uboot list so I'll need to
> > resend this at any rate.
>
> If I recall it correctly, the mode determination happened early because
> it was necessary to find out which driver to probe (this one, or the
> samsung gadget one) before it actually probed. I'm not sure myself if
> this is still even applicable.

Marek,

When I tested UMS with this series on imx6 based gwventana I had to
troubleshoot a regression caused when I switched to DM_USB and found
that the gadget driver isn't probed until you issue the ums command. I
don't know if this helps reduce your hesitation here.

I'm not sure what else you want me to do with this - personally I
would like to see some more maintainers of boards using ehci-mx6 chime
in (mx7?) with testing results showing host and device un-affected. I
only have imx6q/imx6dl/imx8mm/imx8mn to test with here.

Tim

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Marek Vasut]

On 7/2/21 11:44 PM, Tim Harvey wrote:
> On Fri, Jul 2, 2021 at 12:53 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/2/21 9:43 PM, Tim Harvey wrote:
>>> On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/2/21 2:15 AM, Tim Harvey wrote:
>>>>> There is no need to set and/or detect mode in of_to_plat
>>>>
>>>> Are you absolutely sure of that ?
>>>>
>>>> It used to be necessary to know the core mode early to determine which
>>>> driver (the gadget or host) to probe, that's why this code was in
>>>> of_to_plat.
>>>>
>>>
>>> Marek,
>>>
>>> I can't find a reason for the core mode to be known early and have
>>> tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
>>> tested this change also (from another thread) with SPL SDP on IMX8MM
>>> (granted the SPL is not using DM_USB for that).
>>>
>>> It appears I accidentally forgot to cc the uboot list so I'll need to
>>> resend this at any rate.
>>
>> If I recall it correctly, the mode determination happened early because
>> it was necessary to find out which driver to probe (this one, or the
>> samsung gadget one) before it actually probed. I'm not sure myself if
>> this is still even applicable.
> 
> Marek,
> 
> When I tested UMS with this series on imx6 based gwventana I had to
> troubleshoot a regression caused when I switched to DM_USB and found
> that the gadget driver isn't probed until you issue the ums command. I
> don't know if this helps reduce your hesitation here.

Do I understand it correctly that, because you had to debug a 
regression, this patch should be OK ? That implication there is a bit odd.

What I am asking is whether you considered all the things which can go 
wrong with this still messy driver, that's all.

> I'm not sure what else you want me to do with this - personally I
> would like to see some more maintainers of boards using ehci-mx6 chime
> in (mx7?) with testing results showing host and device un-affected. I
> only have imx6q/imx6dl/imx8mm/imx8mn to test with here.

The other option is to put it into next and see whether someone 
complains. I am banking toward that option.

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Fabio Estevam]

Hi Tim,

On Fri, Jul 2, 2021 at 6:44 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Marek,
>
> When I tested UMS with this series on imx6 based gwventana I had to
> troubleshoot a regression caused when I switched to DM_USB and found
> that the gadget driver isn't probed until you issue the ums command. I
> don't know if this helps reduce your hesitation here.
>
> I'm not sure what else you want me to do with this - personally I
> would like to see some more maintainers of boards using ehci-mx6 chime
> in (mx7?) with testing results showing host and device un-affected. I
> only have imx6q/imx6dl/imx8mm/imx8mn to test with here.

I have successfully tested your series on a imx7s-warp board too
(besides imx8mm-evk).

Thanks

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Tim Harvey]

On Fri, Jul 2, 2021 at 3:18 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/2/21 11:44 PM, Tim Harvey wrote:
> > On Fri, Jul 2, 2021 at 12:53 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 7/2/21 9:43 PM, Tim Harvey wrote:
> >>> On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 7/2/21 2:15 AM, Tim Harvey wrote:
> >>>>> There is no need to set and/or detect mode in of_to_plat
> >>>>
> >>>> Are you absolutely sure of that ?
> >>>>
> >>>> It used to be necessary to know the core mode early to determine which
> >>>> driver (the gadget or host) to probe, that's why this code was in
> >>>> of_to_plat.
> >>>>
> >>>
> >>> Marek,
> >>>
> >>> I can't find a reason for the core mode to be known early and have
> >>> tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
> >>> tested this change also (from another thread) with SPL SDP on IMX8MM
> >>> (granted the SPL is not using DM_USB for that).
> >>>
> >>> It appears I accidentally forgot to cc the uboot list so I'll need to
> >>> resend this at any rate.
> >>
> >> If I recall it correctly, the mode determination happened early because
> >> it was necessary to find out which driver to probe (this one, or the
> >> samsung gadget one) before it actually probed. I'm not sure myself if
> >> this is still even applicable.
> >
> > Marek,
> >
> > When I tested UMS with this series on imx6 based gwventana I had to
> > troubleshoot a regression caused when I switched to DM_USB and found
> > that the gadget driver isn't probed until you issue the ums command. I
> > don't know if this helps reduce your hesitation here.
>
> Do I understand it correctly that, because you had to debug a
> regression, this patch should be OK ? That implication there is a bit odd.
>

Not at all... My regression had to do with the fact that DM_USB uses
the 'usb0' alias to get to the USB OTG controller (which I didn't have
defined right). I merely pointed out that I had to poke around at the
code enough to realize the device controller didn't get probed until
you use the ums command. I didn't realize my regression until I set
out to test gadget mode so that I could resubmit this series and let
you know that I had tested both host and gadget mode on the boards I
have.

> What I am asking is whether you considered all the things which can go
> wrong with this still messy driver, that's all.

Oh I most definitely will never be able to consider everything as you
point out the driver is a complete mess and it supports a large
variety of SoC's and configurations. All I can do is make my best
effort to look for possible issues (which I've done), test (which I've
done), and post for review.

>
> > I'm not sure what else you want me to do with this - personally I
> > would like to see some more maintainers of boards using ehci-mx6 chime
> > in (mx7?) with testing results showing host and device un-affected. I
> > only have imx6q/imx6dl/imx8mm/imx8mn to test with here.
>
> The other option is to put it into next and see whether someone
> complains. I am banking toward that option.

By 'next' do you mean it sits in a branch that never gets merged or
are you saying it gets merged after v2021.07? I would agree it should
not be merged this late into v2021.07 but if it sits in a branch
that's never merged to master it will never get tested.

Tim

Re: [PATCH v2 1/2] usb: ehci-mx6: move mode set/detect to probe

[Marek Vasut]

On 7/3/21 12:36 AM, Tim Harvey wrote:
> On Fri, Jul 2, 2021 at 3:18 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/2/21 11:44 PM, Tim Harvey wrote:
>>> On Fri, Jul 2, 2021 at 12:53 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/2/21 9:43 PM, Tim Harvey wrote:
>>>>> On Fri, Jul 2, 2021 at 12:17 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 7/2/21 2:15 AM, Tim Harvey wrote:
>>>>>>> There is no need to set and/or detect mode in of_to_plat
>>>>>>
>>>>>> Are you absolutely sure of that ?
>>>>>>
>>>>>> It used to be necessary to know the core mode early to determine which
>>>>>> driver (the gadget or host) to probe, that's why this code was in
>>>>>> of_to_plat.
>>>>>>
>>>>>
>>>>> Marek,
>>>>>
>>>>> I can't find a reason for the core mode to be known early and have
>>>>> tested it both with host and gadget mode on IMX6 and IMX8MM. Fabio has
>>>>> tested this change also (from another thread) with SPL SDP on IMX8MM
>>>>> (granted the SPL is not using DM_USB for that).
>>>>>
>>>>> It appears I accidentally forgot to cc the uboot list so I'll need to
>>>>> resend this at any rate.
>>>>
>>>> If I recall it correctly, the mode determination happened early because
>>>> it was necessary to find out which driver to probe (this one, or the
>>>> samsung gadget one) before it actually probed. I'm not sure myself if
>>>> this is still even applicable.
>>>
>>> Marek,
>>>
>>> When I tested UMS with this series on imx6 based gwventana I had to
>>> troubleshoot a regression caused when I switched to DM_USB and found
>>> that the gadget driver isn't probed until you issue the ums command. I
>>> don't know if this helps reduce your hesitation here.
>>
>> Do I understand it correctly that, because you had to debug a
>> regression, this patch should be OK ? That implication there is a bit odd.
>>
> 
> Not at all... My regression had to do with the fact that DM_USB uses
> the 'usb0' alias to get to the USB OTG controller (which I didn't have
> defined right). I merely pointed out that I had to poke around at the
> code enough to realize the device controller didn't get probed until
> you use the ums command. I didn't realize my regression until I set
> out to test gadget mode so that I could resubmit this series and let
> you know that I had tested both host and gadget mode on the boards I
> have.
> 
>> What I am asking is whether you considered all the things which can go
>> wrong with this still messy driver, that's all.
> 
> Oh I most definitely will never be able to consider everything as you
> point out the driver is a complete mess and it supports a large
> variety of SoC's and configurations. All I can do is make my best
> effort to look for possible issues (which I've done), test (which I've
> done), and post for review.

All right

>>> I'm not sure what else you want me to do with this - personally I
>>> would like to see some more maintainers of boards using ehci-mx6 chime
>>> in (mx7?) with testing results showing host and device un-affected. I
>>> only have imx6q/imx6dl/imx8mm/imx8mn to test with here.
>>
>> The other option is to put it into next and see whether someone
>> complains. I am banking toward that option.
> 
> By 'next' do you mean it sits in a branch that never gets merged or
> are you saying it gets merged after v2021.07? I would agree it should
> not be merged this late into v2021.07 but if it sits in a branch
> that's never merged to master it will never get tested.

The later, i.e. scheduled for 2021.10 .