| Message ID | 20191002014153.29831-6-pgwipeout@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | enable tegra-udc host mode | expand |
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 >
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 > >
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.
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?
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; }
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(+)