[5/5] usb: chipidea: tegra: enable tegra-udc host mode
diff mbox series

Message ID 20191002014153.29831-6-pgwipeout@gmail.com
State New
Headers show
Series
  • enable tegra-udc host mode
Related show

Commit Message

Peter Geis Oct. 2, 2019, 1:41 a.m. UTC
Add the functions to the chipidea host driver to enable tegra specific
dma alignment and reset handlers.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c |  7 +++++++
 drivers/usb/chipidea/host.c          | 13 +++++++++++++
 2 files changed, 20 insertions(+)

Comments

Thierry Reding Oct. 2, 2019, 11:35 a.m. UTC | #1
On Tue, Oct 01, 2019 at 09:41:53PM -0400, Peter Geis wrote:
> Add the functions to the chipidea host driver to enable tegra specific
> dma alignment and reset handlers.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_tegra.c |  7 +++++++
>  drivers/usb/chipidea/host.c          | 13 +++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 29415c3a2f48..2f7d542d2273 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> @@ -118,6 +118,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  	udc->data.usb_phy = udc->phy;
>  	udc->data.capoffset = DEF_CAPOFFSET;
>  
> +	/* check the double reset flag */
> +	if (of_property_read_bool(pdev->dev.of_node,
> +				"nvidia,needs-double-reset")) {
> +		dev_dbg(&pdev->dev, "setting double reset flag\n");
> +		udc->data.flags |= CI_HDRC_TEGRA_DOUBLE_RESET;
> +	}

Like I said, I think it'd be better to put this into the same patch that
adds the flag.

> +
>  	udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
>  				      pdev->num_resources, &udc->data);
>  	if (IS_ERR(udc->dev)) {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index b45ceb91c735..e95b7cb8c54d 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -20,6 +20,7 @@
>  #include "ci.h"
>  #include "bits.h"
>  #include "host.h"
> +#include "tegra.h"
>  
>  static struct hc_driver __read_mostly ci_ehci_hc_driver;
>  static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> @@ -275,6 +276,13 @@ static int ci_ehci_hub_control(
>  		goto done;
>  	}
>  
> +	/* For Tegra USB1 port we need to issue Port Reset twice internally */
> +	if (ci->platdata->flags & CI_HDRC_TEGRA_DOUBLE_RESET &&
> +	(typeReq == SetPortFeature && wValue == USB_PORT_FEAT_RESET)) {
> +		spin_unlock_irqrestore(&ehci->lock, flags);
> +		return tegra_ehci_internal_port_reset(ehci, status_reg);
> +	}
> +
>  	/*
>  	 * After resume has finished, it needs do some post resume
>  	 * operation for some SoCs.
> @@ -364,6 +372,11 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
>  	rdrv->name	= "host";
>  	ci->roles[CI_ROLE_HOST] = rdrv;
>  
> +	if (ci->platdata->flags & CI_HDRC_TEGRA_HOST) {
> +		ci_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
> +		ci_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
> +	}

Same here.

That said, there are a few other bits in ehci-tegra.c that we may need.
For example, the tegra_ehci_reset() function sets different values for
the TX FIFO threshold, which we don't do for ChipIdea as far as I can
tell. We also differentiate between Tegra20 and later generations with
respect to whether or not they have the HOSTPC registers.

tegra_ehci_hub_control() also seems to have a number of other work-
arounds that are not yet ported as part of this series. And then there
is the matter of tegra_reset_usb_controller(). I recall that this has
caused severe headaches in the past, so we need to be very careful when
changing to the ChipIdea driver that we don't reintroduce old bugs
again.

Thierry

> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
>
Peter Geis Oct. 2, 2019, 12:15 p.m. UTC | #2
On Wed, Oct 2, 2019 at 7:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Oct 01, 2019 at 09:41:53PM -0400, Peter Geis wrote:
> > Add the functions to the chipidea host driver to enable tegra specific
> > dma alignment and reset handlers.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  drivers/usb/chipidea/ci_hdrc_tegra.c |  7 +++++++
> >  drivers/usb/chipidea/host.c          | 13 +++++++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> > index 29415c3a2f48..2f7d542d2273 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> > @@ -118,6 +118,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >       udc->data.usb_phy = udc->phy;
> >       udc->data.capoffset = DEF_CAPOFFSET;
> >
> > +     /* check the double reset flag */
> > +     if (of_property_read_bool(pdev->dev.of_node,
> > +                             "nvidia,needs-double-reset")) {
> > +             dev_dbg(&pdev->dev, "setting double reset flag\n");
> > +             udc->data.flags |= CI_HDRC_TEGRA_DOUBLE_RESET;
> > +     }
>
> Like I said, I think it'd be better to put this into the same patch that
> adds the flag.
>
> > +
> >       udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> >                                     pdev->num_resources, &udc->data);
> >       if (IS_ERR(udc->dev)) {
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index b45ceb91c735..e95b7cb8c54d 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -20,6 +20,7 @@
> >  #include "ci.h"
> >  #include "bits.h"
> >  #include "host.h"
> > +#include "tegra.h"
> >
> >  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> >  static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> > @@ -275,6 +276,13 @@ static int ci_ehci_hub_control(
> >               goto done;
> >       }
> >
> > +     /* For Tegra USB1 port we need to issue Port Reset twice internally */
> > +     if (ci->platdata->flags & CI_HDRC_TEGRA_DOUBLE_RESET &&
> > +     (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_RESET)) {
> > +             spin_unlock_irqrestore(&ehci->lock, flags);
> > +             return tegra_ehci_internal_port_reset(ehci, status_reg);
> > +     }
> > +
> >       /*
> >        * After resume has finished, it needs do some post resume
> >        * operation for some SoCs.
> > @@ -364,6 +372,11 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
> >       rdrv->name      = "host";
> >       ci->roles[CI_ROLE_HOST] = rdrv;
> >
> > +     if (ci->platdata->flags & CI_HDRC_TEGRA_HOST) {
> > +             ci_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
> > +             ci_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
> > +     }
>
> Same here.
>
> That said, there are a few other bits in ehci-tegra.c that we may need.
> For example, the tegra_ehci_reset() function sets different values for
> the TX FIFO threshold, which we don't do for ChipIdea as far as I can
> tell. We also differentiate between Tegra20 and later generations with
> respect to whether or not they have the HOSTPC registers.
>
> tegra_ehci_hub_control() also seems to have a number of other work-
> arounds that are not yet ported as part of this series. And then there
> is the matter of tegra_reset_usb_controller(). I recall that this has
> caused severe headaches in the past, so we need to be very careful when
> changing to the ChipIdea driver that we don't reintroduce old bugs
> again.
>
> Thierry

I saw the patch around Tegra20's FIFO pipeline, I have a Tegra20
device to test on so I'll look if that's still necessary.
The tegra_ehci driver appeared to implement only what was necessary to
make the controller work, as there's a lot of overlap with the
chipidea driver.
Since the tegra-udc driver worked with very little code, I figured the
chipidea driver handled most everything correctly already.

As such I looked mostly at the workarounds that were tegra specific.

This is also why I requested multiple eyes, as I don't have the
benefit of historical context beyond the commit messages.
>
> > +
> >       return 0;
> >  }
> >
> > --
> > 2.17.1
> >
Dmitry Osipenko Oct. 2, 2019, 7:34 p.m. UTC | #3
02.10.2019 15:15, Peter Geis пишет:
> On Wed, Oct 2, 2019 at 7:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>>
>> On Tue, Oct 01, 2019 at 09:41:53PM -0400, Peter Geis wrote:
>>> Add the functions to the chipidea host driver to enable tegra specific
>>> dma alignment and reset handlers.
>>>
>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>> ---
>>>  drivers/usb/chipidea/ci_hdrc_tegra.c |  7 +++++++
>>>  drivers/usb/chipidea/host.c          | 13 +++++++++++++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>> index 29415c3a2f48..2f7d542d2273 100644
>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>> @@ -118,6 +118,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>       udc->data.usb_phy = udc->phy;
>>>       udc->data.capoffset = DEF_CAPOFFSET;
>>>
>>> +     /* check the double reset flag */
>>> +     if (of_property_read_bool(pdev->dev.of_node,
>>> +                             "nvidia,needs-double-reset")) {
>>> +             dev_dbg(&pdev->dev, "setting double reset flag\n");
>>> +             udc->data.flags |= CI_HDRC_TEGRA_DOUBLE_RESET;
>>> +     }
>>
>> Like I said, I think it'd be better to put this into the same patch that
>> adds the flag.
>>
>>> +
>>>       udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
>>>                                     pdev->num_resources, &udc->data);
>>>       if (IS_ERR(udc->dev)) {
>>> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
>>> index b45ceb91c735..e95b7cb8c54d 100644
>>> --- a/drivers/usb/chipidea/host.c
>>> +++ b/drivers/usb/chipidea/host.c
>>> @@ -20,6 +20,7 @@
>>>  #include "ci.h"
>>>  #include "bits.h"
>>>  #include "host.h"
>>> +#include "tegra.h"
>>>
>>>  static struct hc_driver __read_mostly ci_ehci_hc_driver;
>>>  static int (*orig_bus_suspend)(struct usb_hcd *hcd);
>>> @@ -275,6 +276,13 @@ static int ci_ehci_hub_control(
>>>               goto done;
>>>       }
>>>
>>> +     /* For Tegra USB1 port we need to issue Port Reset twice internally */
>>> +     if (ci->platdata->flags & CI_HDRC_TEGRA_DOUBLE_RESET &&
>>> +     (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_RESET)) {
>>> +             spin_unlock_irqrestore(&ehci->lock, flags);
>>> +             return tegra_ehci_internal_port_reset(ehci, status_reg);
>>> +     }
>>> +
>>>       /*
>>>        * After resume has finished, it needs do some post resume
>>>        * operation for some SoCs.
>>> @@ -364,6 +372,11 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
>>>       rdrv->name      = "host";
>>>       ci->roles[CI_ROLE_HOST] = rdrv;
>>>
>>> +     if (ci->platdata->flags & CI_HDRC_TEGRA_HOST) {
>>> +             ci_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
>>> +             ci_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
>>> +     }
>>
>> Same here.
>>
>> That said, there are a few other bits in ehci-tegra.c that we may need.
>> For example, the tegra_ehci_reset() function sets different values for
>> the TX FIFO threshold, which we don't do for ChipIdea as far as I can
>> tell. We also differentiate between Tegra20 and later generations with
>> respect to whether or not they have the HOSTPC registers.
>>
>> tegra_ehci_hub_control() also seems to have a number of other work-
>> arounds that are not yet ported as part of this series. And then there
>> is the matter of tegra_reset_usb_controller(). I recall that this has
>> caused severe headaches in the past, so we need to be very careful when
>> changing to the ChipIdea driver that we don't reintroduce old bugs
>> again.
>>
>> Thierry
> 
> I saw the patch around Tegra20's FIFO pipeline, I have a Tegra20
> device to test on so I'll look if that's still necessary.
> The tegra_ehci driver appeared to implement only what was necessary to
> make the controller work, as there's a lot of overlap with the
> chipidea driver.
> Since the tegra-udc driver worked with very little code, I figured the
> chipidea driver handled most everything correctly already.
> 
> As such I looked mostly at the workarounds that were tegra specific.
> 
> This is also why I requested multiple eyes, as I don't have the
> benefit of historical context beyond the commit messages.

Ha! Host port works on my Tegra20 using CI driver and this series! At least mouse,
keyboard and WiFi dongle are working fine. Well done, looking forward to v2 :) We are
getting closer to fold tegra-ehci and move to the CI driver uniformly.
Peter Chen Oct. 8, 2019, 6:32 a.m. UTC | #4
On 19-10-02 22:34:21, Dmitry Osipenko wrote:
> 02.10.2019 15:15, Peter Geis пишет:
> > On Wed, Oct 2, 2019 at 7:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >>
> >> On Tue, Oct 01, 2019 at 09:41:53PM -0400, Peter Geis wrote:
> >>> Add the functions to the chipidea host driver to enable tegra specific
> >>> dma alignment and reset handlers.
> >>>
> >>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> >>> ---
> >>>  drivers/usb/chipidea/ci_hdrc_tegra.c |  7 +++++++
> >>>  drivers/usb/chipidea/host.c          | 13 +++++++++++++
> >>>  2 files changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>> index 29415c3a2f48..2f7d542d2273 100644
> >>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>> @@ -118,6 +118,13 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>>       udc->data.usb_phy = udc->phy;
> >>>       udc->data.capoffset = DEF_CAPOFFSET;
> >>>
> >>> +     /* check the double reset flag */
> >>> +     if (of_property_read_bool(pdev->dev.of_node,
> >>> +                             "nvidia,needs-double-reset")) {
> >>> +             dev_dbg(&pdev->dev, "setting double reset flag\n");
> >>> +             udc->data.flags |= CI_HDRC_TEGRA_DOUBLE_RESET;
> >>> +     }
> >>
> >> Like I said, I think it'd be better to put this into the same patch that
> >> adds the flag.
> >>
> >>> +
> >>>       udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> >>>                                     pdev->num_resources, &udc->data);
> >>>       if (IS_ERR(udc->dev)) {
> >>> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> >>> index b45ceb91c735..e95b7cb8c54d 100644
> >>> --- a/drivers/usb/chipidea/host.c
> >>> +++ b/drivers/usb/chipidea/host.c
> >>> @@ -20,6 +20,7 @@
> >>>  #include "ci.h"
> >>>  #include "bits.h"
> >>>  #include "host.h"
> >>> +#include "tegra.h"
> >>>
> >>>  static struct hc_driver __read_mostly ci_ehci_hc_driver;
> >>>  static int (*orig_bus_suspend)(struct usb_hcd *hcd);
> >>> @@ -275,6 +276,13 @@ static int ci_ehci_hub_control(
> >>>               goto done;
> >>>       }
> >>>
> >>> +     /* For Tegra USB1 port we need to issue Port Reset twice internally */
> >>> +     if (ci->platdata->flags & CI_HDRC_TEGRA_DOUBLE_RESET &&
> >>> +     (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_RESET)) {
> >>> +             spin_unlock_irqrestore(&ehci->lock, flags);
> >>> +             return tegra_ehci_internal_port_reset(ehci, status_reg);
> >>> +     }
> >>> +
> >>>       /*
> >>>        * After resume has finished, it needs do some post resume
> >>>        * operation for some SoCs.
> >>> @@ -364,6 +372,11 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
> >>>       rdrv->name      = "host";
> >>>       ci->roles[CI_ROLE_HOST] = rdrv;
> >>>
> >>> +     if (ci->platdata->flags & CI_HDRC_TEGRA_HOST) {
> >>> +             ci_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
> >>> +             ci_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
> >>> +     }
> >>
> >> Same here.
> >>
> >> That said, there are a few other bits in ehci-tegra.c that we may need.
> >> For example, the tegra_ehci_reset() function sets different values for
> >> the TX FIFO threshold, which we don't do for ChipIdea as far as I can
> >> tell. We also differentiate between Tegra20 and later generations with
> >> respect to whether or not they have the HOSTPC registers.
> >>
> >> tegra_ehci_hub_control() also seems to have a number of other work-
> >> arounds that are not yet ported as part of this series. And then there
> >> is the matter of tegra_reset_usb_controller(). I recall that this has
> >> caused severe headaches in the past, so we need to be very careful when
> >> changing to the ChipIdea driver that we don't reintroduce old bugs
> >> again.
> >>
> >> Thierry
> > 
> > I saw the patch around Tegra20's FIFO pipeline, I have a Tegra20
> > device to test on so I'll look if that's still necessary.
> > The tegra_ehci driver appeared to implement only what was necessary to
> > make the controller work, as there's a lot of overlap with the
> > chipidea driver.
> > Since the tegra-udc driver worked with very little code, I figured the
> > chipidea driver handled most everything correctly already.
> > 
> > As such I looked mostly at the workarounds that were tegra specific.
> > 
> > This is also why I requested multiple eyes, as I don't have the
> > benefit of historical context beyond the commit messages.
> 
> Ha! Host port works on my Tegra20 using CI driver and this series! At least mouse,
> keyboard and WiFi dongle are working fine. Well done, looking forward to v2 :) We are
> getting closer to fold tegra-ehci and move to the CI driver uniformly.

Great, would you please CC linux-usb ML for v2?

Patch
diff mbox series

diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
index 29415c3a2f48..2f7d542d2273 100644
--- a/drivers/usb/chipidea/ci_hdrc_tegra.c
+++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
@@ -118,6 +118,13 @@  static int tegra_udc_probe(struct platform_device *pdev)
 	udc->data.usb_phy = udc->phy;
 	udc->data.capoffset = DEF_CAPOFFSET;
 
+	/* check the double reset flag */
+	if (of_property_read_bool(pdev->dev.of_node,
+				"nvidia,needs-double-reset")) {
+		dev_dbg(&pdev->dev, "setting double reset flag\n");
+		udc->data.flags |= CI_HDRC_TEGRA_DOUBLE_RESET;
+	}
+
 	udc->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
 				      pdev->num_resources, &udc->data);
 	if (IS_ERR(udc->dev)) {
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index b45ceb91c735..e95b7cb8c54d 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -20,6 +20,7 @@ 
 #include "ci.h"
 #include "bits.h"
 #include "host.h"
+#include "tegra.h"
 
 static struct hc_driver __read_mostly ci_ehci_hc_driver;
 static int (*orig_bus_suspend)(struct usb_hcd *hcd);
@@ -275,6 +276,13 @@  static int ci_ehci_hub_control(
 		goto done;
 	}
 
+	/* For Tegra USB1 port we need to issue Port Reset twice internally */
+	if (ci->platdata->flags & CI_HDRC_TEGRA_DOUBLE_RESET &&
+	(typeReq == SetPortFeature && wValue == USB_PORT_FEAT_RESET)) {
+		spin_unlock_irqrestore(&ehci->lock, flags);
+		return tegra_ehci_internal_port_reset(ehci, status_reg);
+	}
+
 	/*
 	 * After resume has finished, it needs do some post resume
 	 * operation for some SoCs.
@@ -364,6 +372,11 @@  int ci_hdrc_host_init(struct ci_hdrc *ci)
 	rdrv->name	= "host";
 	ci->roles[CI_ROLE_HOST] = rdrv;
 
+	if (ci->platdata->flags & CI_HDRC_TEGRA_HOST) {
+		ci_ehci_hc_driver.map_urb_for_dma = tegra_ehci_map_urb_for_dma;
+		ci_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
+	}
+
 	return 0;
 }