> >>
> >> struct tegra_usb_soc_info {
> >> unsigned long flags;
> >> + unsigned int txfifothresh;
> >> + enum usb_dr_mode dr_mode;
> >> +};
> >> +
> >> +static const struct tegra_usb_soc_info tegra20_ehci_soc_info = {
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL,
> >> + .dr_mode = USB_DR_MODE_HOST,
> >> + .txfifothresh = 10,
> >> +};
> >> +
> >> +static const struct tegra_usb_soc_info tegra30_ehci_soc_info = {
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL
> >> + .dr_mode = USB_DR_MODE_HOST,
> >> + .txfifothresh = 16,
> >> };
> >>
> >> static const struct tegra_usb_soc_info tegra_udc_soc_info = {
> >> - .flags = CI_HDRC_REQUIRES_ALIGNED_DMA,
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL,
> >> + .dr_mode = USB_DR_MODE_UNKNOWN,
> >> };
> >
> > What's the usage for this dr_mode? Does these three SoCs only supports
> > single mode, it usually sets dr_mode at board dts file to indicate USB
> > role for this board.
>
> All Tegra SoCs have three USB controllers. Only one of these three controllers
> supports peripheral / OTG modes, the other two controllers are fixed to the
> host mode and device trees do not specify the dr_mode for the host
> controllers.
>
> Hence we need to enforce the dr_mode=host for the host-mode controllers.
>
> For UDC the default mode is "unknown" because actual mode comes from a
> board's device tree.
>
You could add dr_mode property at usb node of soc.dtsi to fulfill that.
> >> static const struct of_device_id tegra_usb_of_match[] = {
> >> {
> >> + .compatible = "nvidia,tegra20-ehci",
> >> + .data = &tegra20_ehci_soc_info,
> >> + }, {
> >> + .compatible = "nvidia,tegra30-ehci",
> >> + .data = &tegra30_ehci_soc_info,
> >> + }, {
> >> .compatible = "nvidia,tegra20-udc",
> >> .data = &tegra_udc_soc_info,
> >
> > Your compatible indicates this controller is single USB role, is it on
> > purpose?
>
> Yes, the "tegra20/30-ehci" controllers are fixed to the host-mode in hardware.
>
> ...
> >> +static int tegra_usb_internal_port_reset(struct ehci_hcd *ehci,
> >> + u32 __iomem *portsc_reg,
> >> + unsigned long *flags)
> >> +{
> >> + u32 saved_usbintr, temp;
> >> + unsigned int i, tries;
> >> + int retval = 0;
> >> +
> >> + saved_usbintr = ehci_readl(ehci, &ehci->regs->intr_enable);
> >> + /* disable USB interrupt */
> >> + ehci_writel(ehci, 0, &ehci->regs->intr_enable);
> >> + spin_unlock_irqrestore(&ehci->lock, *flags);
> >> +
> >> + /*
> >> + * Here we have to do Port Reset at most twice for
> >> + * Port Enable bit to be set.
> >> + */
> >> + for (i = 0; i < 2; i++) {
> >> + temp = ehci_readl(ehci, portsc_reg);
> >> + temp |= PORT_RESET;
> >> + ehci_writel(ehci, temp, portsc_reg);
> >> + mdelay(10);
> >
> > Needs accurate delay? How about usleep_range?
>
> To be hones I don't know for sure whether hub_control() could be used from
> interrupt context. This mdelay is borrowed from the older ehci-tegra driver.
>
> Are you suggesting that it should be safe to sleep here?
>
I see msleep is called at tegra_ehci_hub_control at ehci-tegra.c.
.hub_control is not called at interrupt context.
> ...
> >> static int tegra_usb_probe(struct platform_device *pdev) {
> >> const struct tegra_usb_soc_info *soc; @@ -83,24 +292,49 @@
> static
> >> int tegra_usb_probe(struct platform_device *pdev)
> >> return err;
> >> }
> >>
> >> + if (device_property_present(&pdev->dev, "nvidia,needs-double-reset"))
> >> + usb->needs_double_reset = true;
> >> +
> >> + err = tegra_usb_reset_controller(&pdev->dev);
> >> + if (err) {
> >> + dev_err(&pdev->dev, "failed to reset controller: %d\n", err);
> >> + goto fail_power_off;
> >> + }
> >> +
> >> + /*
> >> + * USB controller registers shan't be touched before PHY is
> >
> > %s/shan't/shouldn't
>
> ok
>
> ...
> >> static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool
> >> enable) {
> >> struct ehci_hcd *ehci = hcd_to_ehci(hcd); @@ -160,14 +166,14 @@
> >> static int host_start(struct ci_hdrc *ci)
> >> pinctrl_select_state(ci->platdata->pctl,
> >> ci->platdata->pins_host);
> >>
> >> + ci->hcd = hcd;
> >> +
> >> ret = usb_add_hcd(hcd, 0, 0);
> >> if (ret) {
> >> goto disable_reg;
> >> } else {
> >> struct usb_otg *otg = &ci->otg;
> >>
> >> - ci->hcd = hcd;
> >> -
> >
> > Why this changed?
>
> The ci->hcd is used by tegra_usb_notify_event() to handle reset event and the
> reset happens once usb_add_hcd() is invoked. Hence we need to pre-assign
> the hcd pointer, otherwise there will be a NULL dereference.
Get it, please set ci->hcd as NULL at error path.
Peter
