RE: [PATCH v1 5/8] usb: chipidea: tegra: Support host mode

[Peter Chen]

> >>
> >>  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