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 <mkuo@xxxxxxxxxx>
> Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>
> Signed-off-by: Nagarjuna Kristam <nkristam@xxxxxxxxxx>
> ---
> 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 <abrestic@xxxxxxxxxxxx>");
> +MODULE_AUTHOR("Hui Fu");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
Attachment:
signature.asc
Description: PGP signature
