Skip to contentSkip to conversation

usb: gadget: Add UDC driver for tegra XUSB device mode controller

PATCH New Thierry Reding 1 participant 1 message May 3, 2019 USB

lore
Share & tools
lore
#0 lore raw

PATCH v2 7/8 usb: gadget: Add UDC driver for tegra XUSB device mode controller

Quoted 1 line · Nagarjuna Kristam

This patch adds UDC driver for tegra XUSB 3.0 device mode controller.

s/tegra/Tegra/

Quoted 1 line

XUSB device mode controller support SS, HS and FS modes

s/support/supports/ and terminate the sentence with a full-stop.

Quoted 18 lines

Based on work by:
Mark Kuo <[email protected]>
Andrew Bresticker <[email protected]>

---
 drivers/usb/gadget/udc/Kconfig      |   10 +
 drivers/usb/gadget/udc/Makefile     |    1 +
 drivers/usb/gadget/udc/tegra_xudc.c | 3702 +++++++++++++++++++++++++++++++++++
 3 files changed, 3713 insertions(+)
 create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c
drivers/usb/gadget/udc/Kconfig +0 -0
index 0a16cbd..f6f469c 100644

drivers/usb/gadget/udc/Kconfig config USB_GADGET_XILINX · line 443

Quoted 5 lines
drivers/usb/gadget/udc/Kconfig +2 -0
@@ -439,6 +439,16 @@ config USB_GADGET_XILINX
 	  dynamically linked module called "udc-xilinx" and force all
 	  gadget drivers to also be dynamically linked.
 
+config USB_TEGRA_XUDC
+	tristate "NVIDIA Superspeed USB 3.0 Device Controller"

NVIDIA Tegra? Not sure if this is available anywhere else.

drivers/usb/gadget/udc/Kconfig config USB_GADGET_XILINX · line 446

Quoted 3 lines
drivers/usb/gadget/udc/Kconfig +3 -0
+	depends on ARCH_TEGRA
+	help
+	 Enables TEGRA USB 3.0 device mode controller driver.

NVIDIA Tegra here, too.

Quoted 26 lines
drivers/usb/gadget/udc/Kconfig +5 -0
+
+	 Say "y" to link the driver statically, or "m" to build a
+	 dynamically linked module called "tegra_xudc" and force all
+	 gadget drivers to also be dynamically linked.
+
 source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig"
 
 #
drivers/usb/gadget/udc/Makefile +1 -0
index 897f648..1c55c96 100644
@@ -24,6 +24,7 @@ obj-$(CONFIG_USB_BCM63XX_UDC)	+= bcm63xx_udc.o
 obj-$(CONFIG_USB_FSL_USB2)	+= fsl_usb2_udc.o
 fsl_usb2_udc-y			:= fsl_udc_core.o
 fsl_usb2_udc-$(CONFIG_ARCH_MXC)	+= fsl_mxc_udc.o
+obj-$(CONFIG_USB_TEGRA_XUDC)	+= tegra_xudc.o
 obj-$(CONFIG_USB_M66592)	+= m66592-udc.o
 obj-$(CONFIG_USB_R8A66597)	+= r8a66597-udc.o
 obj-$(CONFIG_USB_RENESAS_USB3)	+= renesas_usb3.o
drivers/usb/gadget/udc/tegra_xudc.c new file +0 -0
new file mode 100644
index 0000000..70beda0

drivers/usb/gadget/udc/tegra_xudc.c line 3

Quoted 3 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +3 -0
@@ -0,0 +1,3702 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NVIDIA XUSB device mode controller

Again, perhaps mention that this is for Tegra.

I didn't find anything that stood out in most of the driver. Below are a couple of things towards the end that I think you should look at.

Generally I thought it was fairly difficult to read. You may want to see if you can improve readability by adding a bit of whitespace where appropriate. For example, try to leave a blank line above and below block elements, such as conditionals or loops. I find that that helps a lot in making the code easier to read.

See below for an example.

[...]

drivers/usb/gadget/udc/tegra_xudc.c line 15

Quoted 12 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +12 -0
+static int tegra_xudc_probe(struct platform_device *pdev)
+{
+	struct tegra_xudc *xudc;
+	struct resource *res;
+	unsigned int i;
+	int err;
+
+	xudc = devm_kzalloc(&pdev->dev, sizeof(*xudc), GFP_ATOMIC);
+	if (!xudc)
+		return -ENOMEM;
+	xudc->dev = &pdev->dev;
+	platform_set_drvdata(pdev, xudc);

This, for example, would be easier to read as:

	xudc = devm_kzalloc(&pdev->dev, sizeof(*xudc), GFP_ATOMIC);
	if (!xudc)
		return -ENOMEM;

	platform_set_drvdata(pdev, xudc);
	xudc->dev = &pdev->dev;

drivers/usb/gadget/udc/tegra_xudc.c line 19

Quoted 4 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +4 -0
+
+	xudc->soc = of_device_get_match_data(&pdev->dev);
+	if (!xudc->soc)
+		return -ENODEV;

This situation can never happen. The driver is only ever instantiated from device tree, in which case xudc->soc will be a valid pointer to the correct SoC data structure.

Quoted 173 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +173 -0
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	xudc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xudc->base))
+		return PTR_ERR(xudc->base);
+	xudc->phys_base = res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	xudc->fpci = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xudc->fpci))
+		return PTR_ERR(xudc->fpci);
+
+	if (xudc->soc->has_ipfs) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+		xudc->ipfs = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(xudc->ipfs))
+			return PTR_ERR(xudc->ipfs);
+	}
+
+	xudc->irq = platform_get_irq(pdev, 0);
+	if (xudc->irq < 0) {
+		dev_err(xudc->dev, "failed to get IRQ: %d\n",
+				xudc->irq);
+		return xudc->irq;
+	}
+	err = devm_request_irq(&pdev->dev, xudc->irq, tegra_xudc_irq, 0,
+			       dev_name(&pdev->dev), xudc);
+	if (err < 0) {
+		dev_err(xudc->dev, "failed to claim IRQ#%u: %d\n", xudc->irq,
+			err);
+		return err;
+	}
+
+	xudc->clks = devm_kcalloc(&pdev->dev, xudc->soc->num_clks,
+				      sizeof(*xudc->clks), GFP_KERNEL);
+	if (!xudc->clks)
+		return -ENOMEM;
+	for (i = 0; i < xudc->soc->num_clks; i++)
+		xudc->clks[i].id = xudc->soc->clock_names[i];
+	err = devm_clk_bulk_get(&pdev->dev, xudc->soc->num_clks,
+				      xudc->clks);
+	if (err) {
+		dev_err(xudc->dev, "failed to request clks %d\n", err);
+		return err;
+	}
+
+	xudc->supplies = devm_kcalloc(&pdev->dev, xudc->soc->num_supplies,
+				      sizeof(*xudc->supplies), GFP_KERNEL);
+	if (!xudc->supplies)
+		return -ENOMEM;
+	for (i = 0; i < xudc->soc->num_supplies; i++)
+		xudc->supplies[i].supply = xudc->soc->supply_names[i];
+	err = devm_regulator_bulk_get(&pdev->dev, xudc->soc->num_supplies,
+				      xudc->supplies);
+	if (err) {
+		dev_err(xudc->dev, "failed to request regulators %d\n", err);
+		return err;
+	}
+
+	xudc->padctl = tegra_xusb_padctl_get(&pdev->dev);
+	if (IS_ERR(xudc->padctl))
+		return PTR_ERR(xudc->padctl);
+
+	err = regulator_bulk_enable(xudc->soc->num_supplies, xudc->supplies);
+	if (err) {
+		dev_err(xudc->dev, "failed to enable regulators %d\n", err);
+		goto put_padctl;
+	}
+
+	xudc->usb3_phy = devm_phy_optional_get(&pdev->dev, "usb3");
+	if (IS_ERR(xudc->usb3_phy)) {
+		err = PTR_ERR(xudc->usb3_phy);
+		dev_err(xudc->dev, "failed to get usb3 phy: %d\n", err);
+		goto disable_regulator;
+	}
+	xudc->utmi_phy = devm_phy_optional_get(&pdev->dev, "usb2");
+	if (IS_ERR(xudc->utmi_phy)) {
+		err = PTR_ERR(xudc->utmi_phy);
+		dev_err(xudc->dev, "failed to get usb2 phy: %d\n", err);
+		goto disable_regulator;
+	}
+
+	xudc->data_role_extcon = extcon_get_edev_by_phandle(&pdev->dev, 0);
+	if (IS_ERR(xudc->data_role_extcon)) {
+		err = PTR_ERR(xudc->data_role_extcon);
+		dev_err(xudc->dev, "extcon_get_edev_by_phandle failed %d\n",
+				err);
+		goto disable_regulator;
+	}
+
+	err = tegra_xudc_powerdomain_init(xudc);
+	if (err)
+		goto put_powerdomains;
+
+	err = tegra_xudc_phy_init(xudc);
+	if (err)
+		goto put_powerdomains;
+
+	err = tegra_xudc_alloc_event_ring(xudc);
+	if (err)
+		goto disable_phy;
+
+	err = tegra_xudc_alloc_eps(xudc);
+	if (err)
+		goto free_event_ring;
+
+	spin_lock_init(&xudc->lock);
+	init_completion(&xudc->disconnect_complete);
+
+	INIT_WORK(&xudc->data_role_work, tegra_xudc_data_role_work);
+	xudc->data_role_nb.notifier_call = tegra_xudc_data_role_notifier;
+	extcon_register_notifier(xudc->data_role_extcon, EXTCON_USB,
+				 &xudc->data_role_nb);
+
+	INIT_DELAYED_WORK(&xudc->plc_reset_work, tegra_xudc_plc_reset_work);
+
+	INIT_DELAYED_WORK(&xudc->port_reset_war_work,
+				tegra_xudc_port_reset_war_work);
+
+	pm_runtime_enable(&pdev->dev);
+
+	xudc->gadget.ops = &tegra_xudc_gadget_ops;
+	xudc->gadget.ep0 = &xudc->ep[0].usb_ep;
+	xudc->gadget.name = "tegra-xudc";
+	xudc->gadget.max_speed = USB_SPEED_SUPER;
+
+	err = usb_add_gadget_udc(&pdev->dev, &xudc->gadget);
+	if (err) {
+		dev_err(&pdev->dev, "failed to add USB gadget: %d\n", err);
+		goto free_eps;
+	}
+
+	tegra_xudc_update_data_role(xudc);
+
+	return 0;
+
+free_eps:
+	tegra_xudc_free_eps(xudc);
+free_event_ring:
+	tegra_xudc_free_event_ring(xudc);
+disable_phy:
+	tegra_xudc_phy_exit(xudc);
+put_powerdomains:
+	tegra_xudc_powerdomain_remove(xudc);
+disable_regulator:
+	regulator_bulk_disable(xudc->soc->num_supplies, xudc->supplies);
+put_padctl:
+	tegra_xusb_padctl_put(xudc->padctl);
+
+	return err;
+}
+
+static int tegra_xudc_remove(struct platform_device *pdev)
+{
+	struct tegra_xudc *xudc = platform_get_drvdata(pdev);
+
+	pm_runtime_get_sync(xudc->dev);
+
+	cancel_delayed_work(&xudc->plc_reset_work);
+	if (xudc->data_role_extcon) {
+		extcon_unregister_notifier(xudc->data_role_extcon, EXTCON_USB,
+				&xudc->data_role_nb);
+		cancel_work_sync(&xudc->data_role_work);
+	}
+	usb_del_gadget_udc(&xudc->gadget);
+	tegra_xudc_free_eps(xudc);
+	tegra_xudc_free_event_ring(xudc);
+	tegra_xudc_powerdomain_remove(xudc);
+
+	regulator_bulk_disable(xudc->soc->num_supplies, xudc->supplies);
+
+	phy_power_off(xudc->utmi_phy);
+	phy_power_off(xudc->usb3_phy);

drivers/usb/gadget/udc/tegra_xudc.c line 202

Quoted 10 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +10 -0
+	tegra_xudc_phy_exit(xudc);
+	pm_runtime_disable(xudc->dev);
+	pm_runtime_put(xudc->dev);
+
+	tegra_xusb_padctl_put(xudc->padctl);
+
+	return 0;
+}
+
+#if IS_ENABLED(CONFIG_PM_SLEEP) || IS_ENABLED(CONFIG_PM)

I'd drop these and instead annotate with __maybe_unused. We no longer support building Tegra without PM, so this is mostly academic anyway.

drivers/usb/gadget/udc/tegra_xudc.c line 207

Quoted 5 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +5 -0
+static int tegra_xudc_powergate(struct tegra_xudc *xudc)
+{
+	unsigned long flags;
+
+	dev_info(xudc->dev, "entering ELPG\n");

There's a couple of dev_info() calls throughout the driver that I think are too noisy. I think most of those should be dev_dbg() so that users aren't bothered with them. In cases where you really want to highlight an error or something, make them dev_err() or dev_warn().

Quoted 17 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +17 -0
+	spin_lock_irqsave(&xudc->lock, flags);
+	xudc->powergated = true;
+	xudc->saved_regs.ctrl = xudc_readl(xudc, CTRL);
+	xudc->saved_regs.portpm = xudc_readl(xudc, PORTPM);
+	xudc_writel(xudc, 0, CTRL);
+	spin_unlock_irqrestore(&xudc->lock, flags);
+
+	clk_bulk_disable_unprepare(xudc->soc->num_clks, xudc->clks);
+	regulator_bulk_disable(xudc->soc->num_supplies, xudc->supplies);
+
+	dev_info(xudc->dev, "entering ELPG done\n");
+	return 0;
+}
+
+static int tegra_xudc_unpowergate(struct tegra_xudc *xudc)
+{
+	unsigned long flags;

drivers/usb/gadget/udc/tegra_xudc.c line 234

Quoted 10 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +10 -0
+	int err;
+
+	dev_info(xudc->dev, "exiting ELPG\n");
+
+	err = regulator_bulk_enable(xudc->soc->num_supplies,
+			xudc->supplies);
+	if (err < 0)
+		return err;
+
+

Gratuituous blank line.

Quoted 73 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +73 -0
+	err = clk_bulk_prepare_enable(xudc->soc->num_clks, xudc->clks);
+	if (err < 0)
+		return err;
+
+	tegra_xudc_fpci_ipfs_init(xudc);
+	tegra_xudc_device_params_init(xudc);
+
+	tegra_xudc_init_event_ring(xudc);
+	tegra_xudc_init_eps(xudc);
+
+	xudc_writel(xudc, xudc->saved_regs.portpm, PORTPM);
+	xudc_writel(xudc, xudc->saved_regs.ctrl, CTRL);
+
+	spin_lock_irqsave(&xudc->lock, flags);
+	xudc->powergated = false;
+	spin_unlock_irqrestore(&xudc->lock, flags);
+
+	dev_info(xudc->dev, "exiting ELPG done\n");
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int tegra_xudc_suspend(struct device *dev)
+{
+	struct tegra_xudc *xudc = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&xudc->lock, flags);
+	xudc->suspended = true;
+	spin_unlock_irqrestore(&xudc->lock, flags);
+
+	if (xudc->data_role_extcon)
+		flush_work(&xudc->data_role_work);
+
+	/* Forcibly disconnect before powergating. */
+	tegra_xudc_device_mode_off(xudc);
+
+	if (!pm_runtime_status_suspended(dev))
+		tegra_xudc_powergate(xudc);
+
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static int tegra_xudc_resume(struct device *dev)
+{
+	struct tegra_xudc *xudc = dev_get_drvdata(dev);
+	unsigned long flags;
+	int err;
+
+	err = tegra_xudc_unpowergate(xudc);
+	if (err < 0)
+		return err;
+
+	spin_lock_irqsave(&xudc->lock, flags);
+	xudc->suspended = false;
+	spin_unlock_irqrestore(&xudc->lock, flags);
+
+	tegra_xudc_update_data_role(xudc);
+
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM
+static int tegra_xudc_runtime_suspend(struct device *dev)
+{
+	struct tegra_xudc *xudc = dev_get_drvdata(dev);
+

drivers/usb/gadget/udc/tegra_xudc.c line 317

Quoted 10 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +10 -0
+	return tegra_xudc_powergate(xudc);
+}
+
+static int tegra_xudc_runtime_resume(struct device *dev)
+{
+	struct tegra_xudc *xudc = dev_get_drvdata(dev);
+
+	return tegra_xudc_unpowergate(xudc);
+}
+#endif

__maybe_unused for these as well.

Thierry

Quoted 25 lines
drivers/usb/gadget/udc/tegra_xudc.c new file +22 -0
+
+static const struct dev_pm_ops tegra_xudc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_xudc_suspend, tegra_xudc_resume)
+	SET_RUNTIME_PM_OPS(tegra_xudc_runtime_suspend,
+			   tegra_xudc_runtime_resume, NULL)
+};
+
+static struct platform_driver tegra_xudc_driver = {
+	.probe = tegra_xudc_probe,
+	.remove = tegra_xudc_remove,
+	.driver = {
+		.name = "tegra-xudc",
+		.pm = &tegra_xudc_pm_ops,
+		.of_match_table = tegra_xudc_of_match,
+	},
+};
+module_platform_driver(tegra_xudc_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra XUSB Device Controller");
+MODULE_AUTHOR("Andrew Bresticker <[email protected]>");
+MODULE_AUTHOR("Hui Fu");
+MODULE_LICENSE("GPL v2");
Signature · 1 line
2.7.4