On Mon, Mar 11, 2019 at 04:41:55PM +0530, Nagarjuna Kristam wrote: > This patch adds UDC driver for tegra XUSB 3.0 device mode controller.
s/tegra/Tegra/ > XUSB device mode controller support SS, HS and FS modes s/support/supports/ and terminate the sentence with a full-stop. > > Based on work by: > Mark Kuo <[email protected]> > Andrew Bresticker <[email protected]> > > Signed-off-by: Nagarjuna Kristam <[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 > > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig > index 0a16cbd..f6f469c 100644 > --- a/drivers/usb/gadget/udc/Kconfig > +++ b/drivers/usb/gadget/udc/Kconfig > @@ -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. > + depends on ARCH_TEGRA > + help > + Enables TEGRA USB 3.0 device mode controller driver. NVIDIA Tegra here, too. > + > + 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" > > # > diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile > index 897f648..1c55c96 100644 > --- a/drivers/usb/gadget/udc/Makefile > +++ b/drivers/usb/gadget/udc/Makefile > @@ -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 > diff --git a/drivers/usb/gadget/udc/tegra_xudc.c > b/drivers/usb/gadget/udc/tegra_xudc.c > new file mode 100644 > index 0000000..70beda0 > --- /dev/null > +++ b/drivers/usb/gadget/udc/tegra_xudc.c > @@ -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. [...] > +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; > + > + 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. > + > + 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); > + 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. > +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(). > + 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; > + 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. > + 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); > + > + 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 > + > +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"); > -- > 2.7.4 >
signature.asc
Description: PGP signature
