On Wed, Oct 2, 2019 at 7:35 AM Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
