Support Runtime PM and host mode by Tegra ChipIdea USB driver

The PHY hardware needs the delay of 2ms after power up, otherwise initial interrupt may be lost if USB controller is accessed before PHY is settled down. Previously this issue was masked by implicit delays, but now it pops up after squashing the older ehci-tegra driver into the ChipIdea driver.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/usb/phy/phy-tegra-usb.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/usb/phy/phy-tegra-usb.c +0 -0

index 03a333797382..cee9c9dbb775 100644

Hello!

Quoted 14 lines · Dmitry Osipenko

The PHY hardware needs the delay of 2ms after power up, otherwise initial interrupt may be lost if USB controller is accessed before PHY is settled down. Previously this issue was masked by implicit delays, but now it pops
up after squashing the older ehci-tegra driver into the ChipIdea driver.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/usb/phy/phy-tegra-usb.c | 3 +++

  1 file changed, 3 insertions(+)

Quoted 9 lines

drivers/usb/phy/phy-tegra-usb.c +0 -0

index 03a333797382..cee9c9dbb775 100644
@@ -784,6 +784,9 @@ static int tegra_usb_phy_power_on(struct
 tegra_usb_phy *phy)
         phy->powered_on = true;

  +    /* let PHY to settle down */

   Let the PHY settle down.

The module alias is provided by the OF core for the OF drivers, it overrides the alias set by the drivers. Hence remove the unneeded macro in order to keep the driver code cleaner.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 1 -
 1 file changed, 1 deletion(-)

drivers/usb/chipidea/ci_hdrc_tegra.c +0 -1

index 7455df0ede49..10eaaba2a3f0 100644
@@ -128,5 +128,4 @@ module_platform_driver(tegra_udc_driver);
 
 MODULE_DESCRIPTION("NVIDIA Tegra USB device mode driver");
 MODULE_AUTHOR("Thierry Reding <[email protected]>");
-MODULE_ALIAS("platform:tegra-udc");
 MODULE_LICENSE("GPL v2");

Signature · 1 line

2.29.2

Quoted 3 lines

The module alias is provided by the OF core for the OF drivers, it overrides the alias set by the drivers. Hence remove the unneeded macro in order to keep the driver code cleaner.

This is slightly misleading because that manual MODULE_ALIAS is there for legacy reasons and created that alias to bind against a platform device created from board files back in the day.

Depending on how I interpret the commit message, it also sounds like you're suggesting that the OF core will automatically add this alias. However, what the OF core adds is an alias based on the OF device ID table, which is completely different.

We don't support board files anymore and to my knowledge the aliases generated from the OF device ID table are enough to have the driver autoloaded, so this patch is correct. Perhaps make it clearer in the commit message why this is no longer needed. With that:

Acked-by Thierry Reding <[email protected]>

From: Peter Geis <[email protected]>

struct tegra_usb_soc_info {

drivers/usb/chipidea/ci_hdrc_tegra.c +14 -0

 	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

drivers/usb/chipidea/ci_hdrc_tegra.c +84 -0

 	}, {

@@ -47,6 +81,181 @@ static const struct of_device_id tegra_usb_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_usb_of_match);
 
+static int tegra_usb_reset_controller(struct device *dev)
+{
+	struct reset_control *rst, *rst_utmi;
+	struct device_node *phy_np;
+	int err;
+
+	rst = devm_reset_control_get_shared(dev, "usb");
+	if (IS_ERR(rst)) {
+		dev_err(dev, "can't get ehci reset: %pe\n", rst);
+		return PTR_ERR(rst);
+	}
+
+	phy_np = of_parse_phandle(dev->of_node, "nvidia,phy", 0);
+	if (!phy_np)
+		return -ENOENT;
+
+	/*
+	 * The 1st USB controller contains some UTMI pad registers that are
+	 * global for all the controllers on the chip. Those registers are
+	 * also cleared when reset is asserted to the 1st controller.
+	 */
+	rst_utmi = of_reset_control_get_shared(phy_np, "utmi-pads");
+	if (IS_ERR(rst_utmi)) {
+		dev_warn(dev, "can't get utmi-pads reset from the PHY\n");
+		dev_warn(dev, "continuing, but please update your DT\n");
+	} else {
+		/*
+		 * PHY driver performs UTMI-pads reset in a case of a
+		 * non-legacy DT.
+		 */
+		reset_control_put(rst_utmi);
+	}
+
+	of_node_put(phy_np);
+
+	/* reset control is shared, hence initialize it first */
+	err = reset_control_deassert(rst);
+	if (err)
+		return err;
+
+	err = reset_control_assert(rst);
+	if (err)
+		return err;
+
+	udelay(1);
+
+	err = reset_control_deassert(rst);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int tegra_usb_notify_event(struct ci_hdrc *ci, unsigned int event)
+{
+	struct tegra_usb *usb = dev_get_drvdata(ci->dev->parent);
+	struct ehci_hcd *ehci;
+
+	switch (event) {
+	case CI_HDRC_CONTROLLER_RESET_EVENT:
+		if (ci->hcd) {
+			ehci = hcd_to_ehci(ci->hcd);
+			ehci->has_tdi_phy_lpm = false;
+			ehci_writel(ehci, usb->soc->txfifothresh << 16,
+				    &ehci->regs->txfill_tuning);
+		}
+		break;
+	}
+
+	return 0;
+}
+
+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);

drivers/usb/chipidea/ci_hdrc_tegra.c +82 -0

+		temp &= ~PORT_RESET;
+		ehci_writel(ehci, temp, portsc_reg);
+		mdelay(1);
+		tries = 100;
+		do {
+			mdelay(1);
+			/*
+			 * Up to this point, Port Enable bit is
+			 * expected to be set after 2 ms waiting.
+			 * USB1 usually takes extra 45 ms, for safety,
+			 * we take 100 ms as timeout.
+			 */
+			temp = ehci_readl(ehci, portsc_reg);
+		} while (!(temp & PORT_PE) && tries--);
+		if (temp & PORT_PE)
+			break;
+	}
+	if (i == 2)
+		retval = -ETIMEDOUT;
+
+	/*
+	 * Clear Connect Status Change bit if it's set.
+	 * We can't clear PORT_PEC. It will also cause PORT_PE to be cleared.
+	 */
+	if (temp & PORT_CSC)
+		ehci_writel(ehci, PORT_CSC, portsc_reg);
+
+	/*
+	 * Write to clear any interrupt status bits that might be set
+	 * during port reset.
+	 */
+	temp = ehci_readl(ehci, &ehci->regs->status);
+	ehci_writel(ehci, temp, &ehci->regs->status);
+
+	/* restore original interrupt-enable bits */
+	spin_lock_irqsave(&ehci->lock, *flags);
+	ehci_writel(ehci, saved_usbintr, &ehci->regs->intr_enable);
+
+	return retval;
+}
+
+static int tegra_ehci_hub_control(struct ci_hdrc *ci, u16 typeReq, u16 wValue,
+				  u16 wIndex, char *buf, u16 wLength,
+				  bool *done, unsigned long *flags)
+{
+	struct tegra_usb *usb = dev_get_drvdata(ci->dev->parent);
+	struct ehci_hcd *ehci = hcd_to_ehci(ci->hcd);
+	u32 __iomem *status_reg;
+	int retval = 0;
+
+	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
+
+	switch (typeReq) {
+	case SetPortFeature:
+		if (wValue != USB_PORT_FEAT_RESET || !usb->needs_double_reset)
+			break;
+
+		/* for USB1 port we need to issue Port Reset twice internally */
+		retval = tegra_usb_internal_port_reset(ehci, status_reg, flags);
+		*done  = true;
+		break;
+	}
+
+	return retval;
+}
+
+static void tegra_usb_enter_lpm(struct ci_hdrc *ci, bool enable)
+{
+	/*
+	 * Touching any register which belongs to AHB clock domain will
+	 * hang CPU if USB controller is put into low power mode because
+	 * AHB USB clock is gated on Tegra in the LPM.
+	 *
+	 * Tegra PHY has a separate register for checking the clock status
+	 * and usb_phy_set_suspend() takes care of gating/ungating the clocks
+	 * and restoring the PHY state on Tegra. Hence DEVLC/PORTSC registers
+	 * shouldn't be touched directly by the CI driver.
+	 */
+	usb_phy_set_suspend(ci->usb_phy, enable);
+}
+
 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"))

drivers/usb/chipidea/ci_hdrc_tegra.c +18 -3

+	 * initialized, otherwise CPU will hang because clocks are gated.
+	 * PHY driver controls gating of internal USB clocks on Tegra.
+	 */
+	err = usb_phy_init(usb->phy);
+	if (err)
+		goto fail_power_off;
+
+	platform_set_drvdata(pdev, usb);
+
 	/* setup and register ChipIdea HDRC device */
+	usb->soc = soc;
 	usb->data.name = "tegra-usb";
 	usb->data.flags = soc->flags;
 	usb->data.usb_phy = usb->phy;
+	usb->data.dr_mode = soc->dr_mode;
 	usb->data.capoffset = DEF_CAPOFFSET;
+	usb->data.enter_lpm = tegra_usb_enter_lpm;
+	usb->data.hub_control = tegra_ehci_hub_control;
+	usb->data.notify_event = tegra_usb_notify_event;
 
 	usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
 				      pdev->num_resources, &usb->data);
 	if (IS_ERR(usb->dev)) {
 		err = PTR_ERR(usb->dev);
 		dev_err(&pdev->dev, "failed to add HDRC device: %d\n", err);
-		goto fail_power_off;
+		goto phy_shutdown;
 	}
 
-	platform_set_drvdata(pdev, usb);
-
 	return 0;
 
+phy_shutdown:
+	usb_phy_shutdown(usb->phy);
 fail_power_off:
 	clk_disable_unprepare(usb->clk);
 	return err;

@@ -111,6 +345,7 @@ static int tegra_usb_remove(struct platform_device *pdev)
 	struct tegra_usb *usb = platform_get_drvdata(pdev);
 
 	ci_hdrc_remove_device(usb->dev);
+	usb_phy_shutdown(usb->phy);
 	clk_disable_unprepare(usb->clk);
 
 	return 0;

drivers/usb/chipidea/core.c +9 -1

index aa40e510b806..3f6c21406dbd 100644
@@ -195,7 +195,7 @@ static void hw_wait_phy_stable(void)
 }
 
 /* The PHY enters/leaves low power mode */
-static void ci_hdrc_enter_lpm(struct ci_hdrc *ci, bool enable)
+static void ci_hdrc_enter_lpm_common(struct ci_hdrc *ci, bool enable)
 {
 	enum ci_hw_regs reg = ci->hw_bank.lpm ? OP_DEVLC : OP_PORTSC;
 	bool lpm = !!(hw_read(ci, reg, PORTSC_PHCD(ci->hw_bank.lpm)));

@@ -208,6 +208,11 @@ static void ci_hdrc_enter_lpm(struct ci_hdrc *ci, bool enable)
 				0);
 }
 
+static void ci_hdrc_enter_lpm(struct ci_hdrc *ci, bool enable)
+{
+	return ci->platdata->enter_lpm(ci, enable);
+}
+
 static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
 {
 	u32 reg;

@@ -790,6 +795,9 @@ static int ci_get_platdata(struct device *dev,
 			platdata->pins_device = p;
 	}
 
+	if (!platdata->enter_lpm)
+		platdata->enter_lpm = ci_hdrc_enter_lpm_common;
+
 	return 0;
 }
 

drivers/usb/chipidea/host.c +6 -0

index 48e4a5ca1835..b8a4c44ab580 100644
@@ -29,6 +29,12 @@ struct ehci_ci_priv {
 	bool enabled;
 };
 
+struct ci_hdrc_dma_aligned_buffer {
+	void *kmalloc_ptr;
+	void *old_xfer_buffer;
+	u8 data[0];
+};
+
 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);
 

drivers/usb/chipidea/host.c +93 -0

 		if (ci_otg_is_fsm_mode(ci)) {
 			otg->host = &hcd->self;
 			hcd->self.otg_port = 1;

@@ -237,6 +243,7 @@ static int ci_ehci_hub_control(
 	u32		temp;
 	unsigned long	flags;
 	int		retval = 0;
+	bool		done = false;
 	struct device *dev = hcd->self.controller;
 	struct ci_hdrc *ci = dev_get_drvdata(dev);
 

@@ -244,6 +251,13 @@ static int ci_ehci_hub_control(
 
 	spin_lock_irqsave(&ehci->lock, flags);
 
+	if (ci->platdata->hub_control) {
+		retval = ci->platdata->hub_control(ci, typeReq, wValue, wIndex,
+						   buf, wLength, &done, &flags);
+		if (done)
+			goto done;
+	}
+
 	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) {
 		temp = ehci_readl(ehci, status_reg);
 		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {

@@ -349,6 +363,86 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 	return 0;
 }
 
+static void ci_hdrc_free_dma_aligned_buffer(struct urb *urb)
+{
+	struct ci_hdrc_dma_aligned_buffer *temp;
+	size_t length;
+
+	if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
+		return;
+
+	temp = container_of(urb->transfer_buffer,
+			    struct ci_hdrc_dma_aligned_buffer, data);
+
+	if (usb_urb_dir_in(urb)) {
+		if (usb_pipeisoc(urb->pipe))
+			length = urb->transfer_buffer_length;
+		else
+			length = urb->actual_length;
+
+		memcpy(temp->old_xfer_buffer, temp->data, length);
+	}
+	urb->transfer_buffer = temp->old_xfer_buffer;
+	kfree(temp->kmalloc_ptr);
+
+	urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
+}
+
+static int ci_hdrc_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
+{
+	struct ci_hdrc_dma_aligned_buffer *temp, *kmalloc_ptr;
+	const unsigned int ci_hdrc_usb_dma_align = 32;
+	size_t kmalloc_size;
+
+	if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
+	    !((uintptr_t)urb->transfer_buffer & (ci_hdrc_usb_dma_align - 1)))
+		return 0;
+
+	/* Allocate a buffer with enough padding for alignment */
+	kmalloc_size = urb->transfer_buffer_length +
+		       sizeof(struct ci_hdrc_dma_aligned_buffer) +
+		       ci_hdrc_usb_dma_align - 1;
+
+	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
+	if (!kmalloc_ptr)
+		return -ENOMEM;
+
+	/* Position our struct dma_aligned_buffer such that data is aligned */
+	temp = PTR_ALIGN(kmalloc_ptr + 1, ci_hdrc_usb_dma_align) - 1;
+	temp->kmalloc_ptr = kmalloc_ptr;
+	temp->old_xfer_buffer = urb->transfer_buffer;
+	if (usb_urb_dir_out(urb))
+		memcpy(temp->data, urb->transfer_buffer,
+		       urb->transfer_buffer_length);
+	urb->transfer_buffer = temp->data;
+
+	urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
+
+	return 0;
+}
+
+static int ci_hdrc_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				   gfp_t mem_flags)
+{
+	int ret;
+
+	ret = ci_hdrc_alloc_dma_aligned_buffer(urb, mem_flags);
+	if (ret)
+		return ret;
+
+	ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+	if (ret)
+		ci_hdrc_free_dma_aligned_buffer(urb);
+
+	return ret;
+}
+
+static void ci_hdrc_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+	usb_hcd_unmap_urb_for_dma(hcd, urb);
+	ci_hdrc_free_dma_aligned_buffer(urb);
+}
+
 int ci_hdrc_host_init(struct ci_hdrc *ci)
 {
 	struct ci_role_driver *rdrv;

@@ -366,6 +460,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_REQUIRES_ALIGNED_DMA) {
+		ci_ehci_hc_driver.map_urb_for_dma = ci_hdrc_map_urb_for_dma;
+		ci_ehci_hc_driver.unmap_urb_for_dma = ci_hdrc_unmap_urb_for_dma;
+	}
+
 	return 0;
 }
 

include/linux/usb/chipidea.h +6 -0

index 025b41687ce9..edf3342507f1 100644
@@ -88,6 +88,12 @@ struct ci_hdrc_platform_data {
 	struct pinctrl_state *pins_default;
 	struct pinctrl_state *pins_host;
 	struct pinctrl_state *pins_device;
+
+	/* platform-specific hooks */
+	int (*hub_control)(struct ci_hdrc *ci, u16 typeReq, u16 wValue,
+			   u16 wIndex, char *buf, u16 wLength,
+			   bool *done, unsigned long *flags);
+	void (*enter_lpm)(struct ci_hdrc *ci, bool enable);
 };
 
 /* Default offset of capability registers */

Signature · 1 line

2.29.2

Quoted 22 lines

From: Peter Geis <[email protected]>

struct tegra_usb_soc_info {

drivers/usb/chipidea/ci_hdrc_tegra.c +16 -0

 	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,
 };

drivers/usb/chipidea/ci_hdrc_tegra.c struct tegra_usb { · line 57

Quoted 6 lines

static const struct tegra_usb_soc_info tegra_udc_soc_info = {

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -1

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

Quoted 5 lines

static const struct of_device_id tegra_usb_of_match[] = {

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -0

 	{
+		.compatible = "nvidia,tegra20-ehci",
+		.data = &tegra20_ehci_soc_info,
+	}, {

drivers/usb/chipidea/ci_hdrc_tegra.c struct tegra_usb { · line 68

Quoted 5 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -0

+		.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?

Quoted 16 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +16 -0

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

drivers/usb/chipidea/ci_hdrc_tegra.c static const struct of_device_id tegra_usb_of_match[] = { · line 177

Quoted 6 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +6 -0

+	 */
+	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?

Quoted 7 lines

static int tegra_usb_probe(struct platform_device *pdev) {

drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0

 	const struct tegra_usb_soc_info *soc;

@@ -83,24 +292,49 @@ static int tegra_usb_probe(struct platform_device *pdev)
 		return err;
 	}
 

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_usb_probe(struct platform_device *pdev) · line 305

Quoted 11 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +11 -0

+	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

Quoted 15 lines

drivers/usb/chipidea/host.c +2 -0

 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;
 

drivers/usb/chipidea/host.c static int host_start(struct ci_hdrc *ci) · line 170

Quoted 2 lines

drivers/usb/chipidea/host.c +0 -2

-		ci->hcd = hcd;
-

Why this changed?

Quoted 15 lines

Quoted 15 lines

static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable) {

drivers/usb/chipidea/host.c +2 -0

 	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;
 

Quoted 3 lines

drivers/usb/chipidea/host.c static int host_start(struct ci_hdrc *ci) · line 170

Quoted 2 lines

drivers/usb/chipidea/host.c +0 -2

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

Quoted 22 lines

Quoted 22 lines

struct tegra_usb_soc_info {

drivers/usb/chipidea/ci_hdrc_tegra.c +16 -0

 	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,
 };

Quoted 10 lines

drivers/usb/chipidea/ci_hdrc_tegra.c struct tegra_usb { · line 57

Quoted 6 lines

static const struct tegra_usb_soc_info tegra_udc_soc_info = {

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -1

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

Quoted 5 lines

Quoted 5 lines

static const struct of_device_id tegra_usb_of_match[] = {

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -0

 	{
+		.compatible = "nvidia,tegra20-ehci",
+		.data = &tegra20_ehci_soc_info,
+	}, {

Quoted 8 lines

drivers/usb/chipidea/ci_hdrc_tegra.c struct tegra_usb { · line 68

Quoted 5 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -0

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

...

Quoted 16 lines

Quoted 16 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +16 -0

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

Quoted 8 lines

drivers/usb/chipidea/ci_hdrc_tegra.c static const struct of_device_id tegra_usb_of_match[] = { · line 177

Quoted 6 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +6 -0

+	 */
+	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?

...

Quoted 2 lines

Quoted 2 lines

static int tegra_usb_probe(struct platform_device *pdev) {
const struct tegra_usb_soc_info *soc; @@ -83,24 +292,49 @@

static

Quoted 4 lines

Quoted 4 lines

int tegra_usb_probe(struct platform_device *pdev)

drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0

 		return err;
 	}

Quoted 13 lines

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_usb_probe(struct platform_device *pdev) · line 305

Quoted 11 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +11 -0

+	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

...

Quoted 15 lines

Quoted 15 lines

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)

drivers/usb/chipidea/host.c +2 -0

 		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;

Quoted 4 lines

drivers/usb/chipidea/host.c static int host_start(struct ci_hdrc *ci) · line 170

Quoted 2 lines

drivers/usb/chipidea/host.c +0 -2

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

Quoted 38 lines

Quoted 22 lines

Quoted 22 lines

struct tegra_usb_soc_info {

drivers/usb/chipidea/ci_hdrc_tegra.c +16 -0

 	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,
 };

Quoted 10 lines

drivers/usb/chipidea/ci_hdrc_tegra.c struct tegra_usb { · line 57

Quoted 6 lines

static const struct tegra_usb_soc_info tegra_udc_soc_info = {

drivers/usb/chipidea/ci_hdrc_tegra.c +3 -1

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

Quoted 5 lines

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.

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

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)

drivers/usb/chipidea/host.c +0 -0

 		pinctrl_select_state(ci->platdata->pctl,
 				     ci->platdata->pins_host);
 

drivers/usb/chipidea/host.c static int host_start(struct ci_hdrc *ci) · line 170

Quoted 10 lines

drivers/usb/chipidea/host.c +2 -2

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

Quoted 3 lines

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.

Quoted 24 lines

...

Quoted 23 lines

Quoted 12 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +12 -0

+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);

drivers/usb/chipidea/ci_hdrc_tegra.c static const struct of_device_id tegra_usb_of_match[] = { · line 177

Quoted 10 lines

drivers/usb/chipidea/ci_hdrc_tegra.c +10 -0

+
+	/*
+	 * 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?

Quoted 5 lines

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.

The ChipIdea driver now provides USB2 host mode support for NVIDIA Tegra SoCs. The ehci-tegra driver is obsolete now, remove it.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/usb/host/Kconfig      |   9 -
 drivers/usb/host/Makefile     |   1 -
 drivers/usb/host/ehci-tegra.c | 604 ----------------------------------
 3 files changed, 614 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-tegra.c

drivers/usb/host/Kconfig +0 -0

index 31e59309da1f..9c9e6ff9c43a 100644
@@ -266,15 +266,6 @@ config USB_EHCI_HCD_AT91
 	  Enables support for the on-chip EHCI controller on

Quoted 22 lines

The ChipIdea driver now provides USB2 host mode support for NVIDIA Tegra SoCs. The ehci-tegra driver is obsolete now, remove it.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/usb/host/Kconfig      |   9 -
 drivers/usb/host/Makefile     |   1 -
 drivers/usb/host/ehci-tegra.c | 604 ----------------------------------
 3 files changed, 614 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-tegra.c

drivers/usb/host/Kconfig +0 -0

index 31e59309da1f..9c9e6ff9c43a 100644
@@ -266,15 +266,6 @@ config USB_EHCI_HCD_AT91
 	  Enables support for the on-chip EHCI controller on
 	  Atmel chips.
 

drivers/usb/host/Kconfig config USB_EHCI_HCD_AT91 · line 276

Quoted 8 lines

drivers/usb/host/Kconfig +0 -8

-config USB_EHCI_TEGRA
-	tristate "NVIDIA Tegra HCD support"
-	depends on ARCH_TEGRA
-	select USB_EHCI_ROOT_HUB_TT
-	select USB_TEGRA_PHY
-	help
-	  This driver enables support for the internal USB Host Controllers
-	  found in NVIDIA Tegra SoCs. The controllers are EHCI compliant.

For people upgrading from earlier kernel versions, do you think it would help to add a pointer here telling them which Kconfig option they need to enable now in order to get this functionality?

16.12.2020 19:45, Alan Stern пишет:

Quoted 23 lines · Dmitry Osipenko

Quoted 22 lines

The ChipIdea driver now provides USB2 host mode support for NVIDIA Tegra SoCs. The ehci-tegra driver is obsolete now, remove it.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/usb/host/Kconfig      |   9 -
 drivers/usb/host/Makefile     |   1 -
 drivers/usb/host/ehci-tegra.c | 604 ----------------------------------
 3 files changed, 614 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-tegra.c

drivers/usb/host/Kconfig +0 -0

index 31e59309da1f..9c9e6ff9c43a 100644
@@ -266,15 +266,6 @@ config USB_EHCI_HCD_AT91
 	  Enables support for the on-chip EHCI controller on
 	  Atmel chips.
 

Quoted 12 lines

drivers/usb/host/Kconfig config USB_EHCI_HCD_AT91 · line 276

Quoted 8 lines

drivers/usb/host/Kconfig +0 -8

-config USB_EHCI_TEGRA
-	tristate "NVIDIA Tegra HCD support"
-	depends on ARCH_TEGRA
-	select USB_EHCI_ROOT_HUB_TT
-	select USB_TEGRA_PHY
-	help
-	  This driver enables support for the internal USB Host Controllers
-	  found in NVIDIA Tegra SoCs. The controllers are EHCI compliant.

For people upgrading from earlier kernel versions, do you think it would help to add a pointer here telling them which Kconfig option they need to enable now in order to get this functionality?

Could you please clarify what do you mean by the "pointer"?

Quoted 37 lines

16.12.2020 19:45, Alan Stern пишет:

Quoted 35 lines · Dmitry Osipenko

Quoted 20 lines

The ChipIdea driver now provides USB2 host mode support for NVIDIA Tegra SoCs. The ehci-tegra driver is obsolete now, remove it.

Tested-by Matt Merhar <[email protected]>

Tested-by Nicolas Chauvet <[email protected]>

Tested-by Peter Geis <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/usb/host/Kconfig      |   9 -
 drivers/usb/host/Makefile     |   1 -
 drivers/usb/host/ehci-tegra.c | 604 ----------------------------------
 3 files changed, 614 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-tegra.c

drivers/usb/host/Kconfig +0 -0

index 31e59309da1f..9c9e6ff9c43a 100644
@@ -266,15 +266,6 @@ config USB_EHCI_HCD_AT91
 	  Enables support for the on-chip EHCI controller on

drivers/usb/host/Kconfig config USB_EHCI_HCD_AT91 · line 276

Quoted 10 lines

drivers/usb/host/Kconfig +0 -8

 	  Atmel chips.
 
-config USB_EHCI_TEGRA
-	tristate "NVIDIA Tegra HCD support"
-	depends on ARCH_TEGRA
-	select USB_EHCI_ROOT_HUB_TT
-	select USB_TEGRA_PHY
-	help
-	  This driver enables support for the internal USB Host Controllers
-	  found in NVIDIA Tegra SoCs. The controllers are EHCI compliant.

For people upgrading from earlier kernel versions, do you think it would help to add a pointer here telling them which Kconfig option they need to enable now in order to get this functionality?

Quoted 1 line

Could you please clarify what do you mean by the "pointer"?

See the entries for USB_CNS3XXX_EHCI, USB_OCTEON_EHCI, USB_OHCI_HCD_OMAP3, and USB_OHCI_SH in that Kconfig file for examples.