[2/2] usb: dwc3: Add driver for Xilinx platforms

Commit Message

Manish Narani Aug. 26, 2020, 6:44 p.m. UTC

Add a new driver for supporting Xilinx platforms. This driver handles
the USB 3.0 PHY initialization and PIPE control & reset operations for
ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
reset operations for Versal platforms.

Signed-off-by: Manish Narani <[e-mail]>
---
 drivers/usb/dwc3/Kconfig          |   8 +
 drivers/usb/dwc3/Makefile         |   1 +
 drivers/usb/dwc3/dwc3-of-simple.c |   1 -
 drivers/usb/dwc3/dwc3-xilinx.c    | 416 ++++++++++++++++++++++++++++++
 4 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c

Comments

Randy Dunlap Aug. 26, 2020, 7 p.m. UTC | #1

On 8/26/20 11:44 AM, Manish Narani wrote:
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 7a2304565a73..416063ee9d05 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -139,4 +139,12 @@ config USB_DWC3_QCOM
>  	  for peripheral mode support.
>  	  Say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_XILINX
> +       tristate "Xilinx Platforms"
> +       depends on (ARCH_ZYNQMP || ARCH_VERSAL) && OF
> +       default USB_DWC3
> +       help
> +         Support Xilinx SoCs with DesignWare Core USB3 IP.
> +	 Say 'Y' or 'M' if you have one such device.
> +
>  endif

Indent help text (2 lines) with one tab + 2 spaces, please,
according to Documentation/process/coding-style.rst.

thanks.

Felipe Balbi Aug. 27, 2020, 6:31 a.m. UTC | #2

Manish Narani <[e-mail]> writes:

> Add a new driver for supporting Xilinx platforms. This driver handles
> the USB 3.0 PHY initialization and PIPE control & reset operations for

PHY initialization should be done as part of a drivers/phy driver.

> ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
> reset operations for Versal platforms.

similarly for USB2 PHYs

> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> new file mode 100644
> index 000000000000..272906797a7a
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -0,0 +1,416 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * dwc3-xilinx.c - Xilinx DWC3 controller specific glue driver
> + *
> + * Authors: Anurag Kumar Vulisha <[e-mail]>
> + *          Manish Narani <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/of_address.h>
> +#include <linux/delay.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/io.h>
> +
> +#include <linux/phy/phy.h>
> +
> +/* USB phy reset mask register */
> +#define XLNX_USB_PHY_RST		0x001C
> +#define XLNX_PHY_RST_MASK		0x1
> +
> +/* Xilinx USB 3.0 IP Register */
> +#define XLNX_USB_COHERENCY		0x005C
> +#define XLNX_USB_COHERENCY_ENABLE	0x1
> +
> +/* Versal USB Reset ID */
> +#define VERSAL_USB_RESET_ID		0xC104036
> +
> +#define PIPE_CLK_OFFSET			0x7c
> +#define PIPE_CLK_ON			1
> +#define PIPE_CLK_OFF			0
> +#define PIPE_POWER_OFFSET		0x80
> +#define PIPE_POWER_ON			1
> +#define PIPE_POWER_OFF			0
> +
> +#define RST_TIMEOUT			1000
> +
> +struct dwc3_xlnx {
> +	int				num_clocks;
> +	struct clk_bulk_data		*clks;
> +	struct device			*dev;
> +	void __iomem			*regs;
> +	struct dwc3			*dwc;
> +	struct phy			*phy;
> +	struct phy			*usb3_phy;
> +	struct reset_control		*crst;
> +	struct reset_control		*hibrst;
> +	struct reset_control		*apbrst;
> +};
> +
> +static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
> +{
> +	u32 reg;
> +
> +	reg = readl(priv_data->regs + XLNX_USB_PHY_RST);
> +
> +	if (mask)
> +		/*
> +		 * Mask the phy reset signal from comtroller

s/comtroller/controller

But really, the comments don't bring any extra information. I'd say
remove the comments as the code speaks for itself very clearly in this
case.

> +static int dwc3_xlnx_rst_assert(struct reset_control *rstc)

this looks like it should be an actual reset controller driver.

> +static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
> +{
> +	struct device *dev = priv_data->dev;
> +	int ret;
> +
> +	dwc3_xlnx_mask_phy_rst(priv_data, false);
> +
> +	/* Assert and De-assert reset */
> +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> +				     PM_RESET_ACTION_ASSERT);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to assert Reset\n");
> +		return ret;
> +	}
> +
> +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> +				     PM_RESET_ACTION_RELEASE);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to De-assert Reset\n");
> +		return ret;
> +	}
> +
> +	dwc3_xlnx_mask_phy_rst(priv_data, true);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> +{
> +	struct device *dev = priv_data->dev;
> +	int ret;
> +	u32 reg;
> +
> +	priv_data->crst = devm_reset_control_get(dev, "usb_crst");
> +	if (IS_ERR(priv_data->crst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_crst");
> +		ret = PTR_ERR(priv_data->crst);
> +		goto err;
> +	}
> +
> +	priv_data->hibrst = devm_reset_control_get(dev, "usb_hibrst");
> +	if (IS_ERR(priv_data->hibrst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_hibrst");
> +		ret = PTR_ERR(priv_data->hibrst);
> +		goto err;
> +	}
> +
> +	priv_data->apbrst = devm_reset_control_get(dev, "usb_apbrst");
> +	if (IS_ERR(priv_data->apbrst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_apbrst");
> +		ret = PTR_ERR(priv_data->apbrst);
> +		goto err;
> +	}
> +
> +	priv_data->usb3_phy = devm_phy_get(dev, "usb3-phy");
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->crst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);

you don't need the function name or the line number here. Just improve
your error message a bit:

		dev_err(dev, "Failed to assert usb3-phy reset\n");

> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->hibrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);

		dev_err(dev, "Failed to assert hibernation reset\n");

> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);

		dev_err(dev, "Failed to assert APB reset\n");

> +		goto err;
> +	}
> +
> +	ret = phy_init(priv_data->usb3_phy);

dwc3 core should be handling this already

> +	if (ret < 0) {
> +		phy_exit(priv_data->usb3_phy);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_deassert(priv_data->apbrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	/* Set PIPE power present signal */
> +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
> +
> +	/* Clear PIPE CLK signal */
> +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);

shouldn't this be hidden under clk_enable()?

> +	ret = dwc3_xlnx_rst_deassert(priv_data->crst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_deassert(priv_data->hibrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = phy_power_on(priv_data->usb3_phy);
> +	if (ret < 0) {
> +		phy_exit(priv_data->usb3_phy);
> +		goto err;
> +	}
> +
> +	/*
> +	 * This routes the usb dma traffic to go through CCI path instead
> +	 * of reaching DDR directly. This traffic routing is needed to
> +	 * make SMMU and CCI work with USB dma.
> +	 */
> +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
> +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
> +		reg |= XLNX_USB_COHERENCY_ENABLE;
> +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static int dwc3_xlnx_probe(struct platform_device *pdev)
> +{
> +	struct dwc3_xlnx	*priv_data;
> +	struct device		*dev = &pdev->dev;
> +	struct device_node	*np = dev->of_node;
> +	struct resource		*res;
> +	void __iomem		*regs;
> +	int			ret;
> +
> +	priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "missing memory resource\n");
> +		return -ENODEV;
> +	}

you don't need to check res here. devm_ioremap_resource() already does
that for you.

> +	regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);

this looks like it could use an error message.

> +	/* Store the usb control regs into priv_data for further usage */

pointless comment.

> +	priv_data->regs = regs;
> +

unnecessary blank line.

> +	priv_data->dev = dev;
> +
> +	platform_set_drvdata(pdev, priv_data);
> +
> +	ret = clk_bulk_get_all(priv_data->dev, &priv_data->clks);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv_data->num_clocks = ret;
> +
> +	ret = clk_bulk_prepare_enable(priv_data->num_clocks, priv_data->clks);
> +	if (ret)
> +		return ret;
> +
> +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-dwc3")) {
> +		ret = dwc3_xlnx_init_zynqmp(priv_data);
> +		if (ret)
> +			goto err_clk_put;
> +	}

instead, you could pass a pointer to dwc3_xlnx_init_zynqmp() as driver
data and just call it unconditionally. It would avoid the compatible
check here.

> +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,versal-dwc3")) {
> +		ret = dwc3_xlnx_init_versal(priv_data);
> +		if (ret)
> +			goto err_clk_put;
> +	}

same as above

> +	ret = of_platform_populate(np, NULL, NULL, dev);
> +	if (ret)
> +		goto err_clk_put;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_suspend_ignore_children(dev, false);
> +	pm_runtime_get_sync(dev);
> +
> +	pm_runtime_forbid(dev);

why forbid? You already have a get_sync()

> +	return 0;
> +
> +err_clk_put:
> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> +
> +	return ret;
> +}
> +
> +static int dwc3_xlnx_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);
> +	struct device		*dev = &pdev->dev;
> +
> +	of_platform_depopulate(dev);
> +
> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> +	priv_data->num_clocks = 0;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int dwc3_xlnx_runtime_suspend(struct device *dev)
> +{
> +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_runtime_idle(struct device *dev)
> +{
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_autosuspend(dev);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_runtime_resume(struct device *dev)
> +{
> +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> +}
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int dwc3_xlnx_suspend(struct device *dev)
> +{
> +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> +
> +	/* Disable the clocks */
> +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_resume(struct device *dev)
> +{
> +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> +}

you have the same implementation for both types of suspend/resume. Maybe
extract dwc3_xlnx_{suspend,resume}_common() and just call it from both
callbacks?

> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwc3_xlnx_suspend, dwc3_xlnx_resume)
> +	SET_RUNTIME_PM_OPS(dwc3_xlnx_runtime_suspend, dwc3_xlnx_runtime_resume,
> +			   dwc3_xlnx_runtime_idle)

seems like it could be replaced with UNIVERSAL_PM_OPS with minor
modifications.

Chunfeng Yun Aug. 27, 2020, 7:42 a.m. UTC | #3

On Thu, 2020-08-27 at 00:14 +0530, Manish Narani wrote:
> Add a new driver for supporting Xilinx platforms. This driver handles
> the USB 3.0 PHY initialization and PIPE control & reset operations for
> ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
> reset operations for Versal platforms.
> 
> Signed-off-by: Manish Narani <[e-mail]>
> ---
>  drivers/usb/dwc3/Kconfig          |   8 +
>  drivers/usb/dwc3/Makefile         |   1 +
>  drivers/usb/dwc3/dwc3-of-simple.c |   1 -
>  drivers/usb/dwc3/dwc3-xilinx.c    | 416 ++++++++++++++++++++++++++++++
>  4 files changed, 425 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c
> 
[...]
> +static int dwc3_xlnx_probe(struct platform_device *pdev)
> +{
> +	struct dwc3_xlnx	*priv_data;
> +	struct device		*dev = &pdev->dev;
> +	struct device_node	*np = dev->of_node;
> +	struct resource		*res;
> +	void __iomem		*regs;
> +	int			ret;
> +
> +	priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "missing memory resource\n");
> +		return -ENODEV;
> +	}
> +
> +	regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
Use devm_platform_ioremap_resource()?

> +
> +	/* Store the usb control regs into priv_data for further usage */
> +	priv_data->regs = regs;
> +
> +	priv_data->dev = dev;
> +
> +	platform_set_drvdata(pdev, priv_data);
> +
> +	ret = clk_bulk_get_all(priv_data->dev, &priv_data->clks);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv_data->num_clocks = ret;
> +
> +	ret = clk_bulk_prepare_enable(priv_data->num_clocks, priv_data->clks);
> +	if (ret)
> +		return ret;
> +
> +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-dwc3")) {
> +		ret = dwc3_xlnx_init_zynqmp(priv_data);
> +		if (ret)
> +			goto err_clk_put;
> +	}
> +
> +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,versal-dwc3")) {
> +		ret = dwc3_xlnx_init_versal(priv_data);
> +		if (ret)
> +			goto err_clk_put;
> +	}
> +
> +	ret = of_platform_populate(np, NULL, NULL, dev);
> +	if (ret)
> +		goto err_clk_put;
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_suspend_ignore_children(dev, false);
> +	pm_runtime_get_sync(dev);
> +
> +	pm_runtime_forbid(dev);
> +
> +	return 0;
> +
> +err_clk_put:
> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> +
> +	return ret;
> +}
> +
> +static int dwc3_xlnx_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);
> +	struct device		*dev = &pdev->dev;
> +
> +	of_platform_depopulate(dev);
> +
> +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> +	priv_data->num_clocks = 0;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int dwc3_xlnx_runtime_suspend(struct device *dev)
> +{
> +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_runtime_idle(struct device *dev)
> +{
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_autosuspend(dev);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_runtime_resume(struct device *dev)
> +{
> +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> +}
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int dwc3_xlnx_suspend(struct device *dev)
> +{
> +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> +
> +	/* Disable the clocks */
> +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_resume(struct device *dev)
> +{
> +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwc3_xlnx_suspend, dwc3_xlnx_resume)
> +	SET_RUNTIME_PM_OPS(dwc3_xlnx_runtime_suspend, dwc3_xlnx_runtime_resume,
> +			   dwc3_xlnx_runtime_idle)
> +};
> +
> +static const struct of_device_id of_dwc3_xlnx_match[] = {
> +	{ .compatible = "xlnx,zynqmp-dwc3" },
> +	{ .compatible = "xlnx,versal-dwc3" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_dwc3_xlnx_match);
> +
> +static struct platform_driver dwc3_xlnx_driver = {
> +	.probe		= dwc3_xlnx_probe,
> +	.remove		= dwc3_xlnx_remove,
> +	.driver		= {
> +		.name	= "dwc3-xilinx",
> +		.of_match_table = of_dwc3_xlnx_match,
> +		.pm	= &dwc3_xlnx_dev_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(dwc3_xlnx_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Xilinx DWC3 controller specific glue driver");
> +MODULE_AUTHOR("Anurag Kumar Vulisha <[e-mail]>");
> +MODULE_AUTHOR("Manish Narani <[e-mail]>");

Philipp Zabel Aug. 27, 2020, 9:02 a.m. UTC | #4

Hi Manish,

On Thu, 2020-08-27 at 00:14 +0530, Manish Narani wrote:
> Add a new driver for supporting Xilinx platforms. This driver handles
> the USB 3.0 PHY initialization and PIPE control & reset operations for
> ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
> reset operations for Versal platforms.
> 
> Signed-off-by: Manish Narani <[e-mail]>
> ---
[...]
> +static int dwc3_xlnx_rst_assert(struct reset_control *rstc)
> +{
> +	unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
> +	unsigned long timeout;
> +
> +	reset_control_assert(rstc);
> +
> +	/* wait until reset is asserted or timeout */
> +	timeout = jiffies + loop_time;
> +
> +	while (!time_after_eq(jiffies, timeout)) {
> +		if (reset_control_status(rstc) > 0)
> +			return 0;
> +
> +		cpu_relax();
> +	}
> +
> +	return -ETIMEDOUT;
> +}

I think this should be done in the reset controller driver instead.

When reset_control_assert() is called with an exclusive reset control,
the reset line should be already asserted when the function returns.

> +
> +static int dwc3_xlnx_rst_deassert(struct reset_control *rstc)
> +{
> +	unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
> +	unsigned long timeout;
> +
> +	reset_control_deassert(rstc);
> +
> +	/* wait until reset is de-asserted or timeout */
> +	timeout = jiffies + loop_time;
> +	while (!time_after_eq(jiffies, timeout)) {
> +		if (!reset_control_status(rstc))
> +			return 0;
> +
> +		cpu_relax();
> +	}
> +
> +	return -ETIMEDOUT;
> +}

Same as above, this belongs in the reset controller driver.

> +static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
> +{
> +	struct device *dev = priv_data->dev;
> +	int ret;
> +
> +	dwc3_xlnx_mask_phy_rst(priv_data, false);
> +
> +	/* Assert and De-assert reset */
> +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> +				     PM_RESET_ACTION_ASSERT);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to assert Reset\n");
> +		return ret;
> +	}
> +
> +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> +				     PM_RESET_ACTION_RELEASE);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to De-assert Reset\n");
> +		return ret;
> +	}
> +
> +	dwc3_xlnx_mask_phy_rst(priv_data, true);
> +
> +	return 0;
> +}
> +
> +static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> +{
> +	struct device *dev = priv_data->dev;
> +	int ret;
> +	u32 reg;
> +
> +	priv_data->crst = devm_reset_control_get(dev, "usb_crst");

Please use devm_reset_control_get_exclusive() instead.

> +	if (IS_ERR(priv_data->crst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_crst");

Consider using dev_err_probe() to hide -EPROBE_DEFER error values.

> +		ret = PTR_ERR(priv_data->crst);
> +		goto err;
> +	}
> +
> +	priv_data->hibrst = devm_reset_control_get(dev, "usb_hibrst");
> +	if (IS_ERR(priv_data->hibrst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_hibrst");
> +		ret = PTR_ERR(priv_data->hibrst);
> +		goto err;
> +	}
> +
> +	priv_data->apbrst = devm_reset_control_get(dev, "usb_apbrst");
> +	if (IS_ERR(priv_data->apbrst)) {
> +		dev_err(dev, "failed to get %s reset signal\n", "usb_apbrst");
> +		ret = PTR_ERR(priv_data->apbrst);
> +		goto err;
> +	}

Same as above for the other two reset controls.

> +	priv_data->usb3_phy = devm_phy_get(dev, "usb3-phy");
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->crst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->hibrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = phy_init(priv_data->usb3_phy);
> +	if (ret < 0) {
> +		phy_exit(priv_data->usb3_phy);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_deassert(priv_data->apbrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	/* Set PIPE power present signal */
> +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
> +
> +	/* Clear PIPE CLK signal */
> +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
> +
> +	ret = dwc3_xlnx_rst_deassert(priv_data->crst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = dwc3_xlnx_rst_deassert(priv_data->hibrst);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: %d: Failed to release reset\n",
> +			__func__, __LINE__);
> +		goto err;
> +	}
> +
> +	ret = phy_power_on(priv_data->usb3_phy);
> +	if (ret < 0) {
> +		phy_exit(priv_data->usb3_phy);
> +		goto err;
> +	}
> +
> +	/*
> +	 * This routes the usb dma traffic to go through CCI path instead
> +	 * of reaching DDR directly. This traffic routing is needed to
> +	 * make SMMU and CCI work with USB dma.
> +	 */
> +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
> +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
> +		reg |= XLNX_USB_COHERENCY_ENABLE;
> +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static int dwc3_xlnx_probe(struct platform_device *pdev)
> +{
> +	struct dwc3_xlnx	*priv_data;
> +	struct device		*dev = &pdev->dev;
> +	struct device_node	*np = dev->of_node;
> +	struct resource		*res;
> +	void __iomem		*regs;
> +	int			ret;
> +
> +	priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "missing memory resource\n");
> +		return -ENODEV;
> +	}
> +
> +	regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	/* Store the usb control regs into priv_data for further usage */
> +	priv_data->regs = regs;
> +
> +	priv_data->dev = dev;
> +
> +	platform_set_drvdata(pdev, priv_data);
> +
> +	ret = clk_bulk_get_all(priv_data->dev, &priv_data->clks);

Why not use devm_clk_bulk_get_all()?

regards
Philipp

Robin Murphy Aug. 27, 2020, 6:46 p.m. UTC | #5

On 2020-08-26 19:44, Manish Narani wrote:
[...]
> +	/*
> +	 * This routes the usb dma traffic to go through CCI path instead
> +	 * of reaching DDR directly. This traffic routing is needed to
> +	 * make SMMU and CCI work with USB dma.
> +	 */
> +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
> +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
> +		reg |= XLNX_USB_COHERENCY_ENABLE;
> +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
> +	}

This looks rather suspect - coherency should be based on coherency, not 
on whether an IOMMU group is present. If the device isn't described as 
coherent in the DT, then any SMMU mappings will end up using attributes 
that will downgrade traffic to be non-snooping anyway. And if the SMMU 
is enabled but not translating (e.g. "iommu.passthrough=1") then 
enabling hardware coherency when the DMA layer hasn't been told about it 
can potentially lead to nasty subtle problems and data loss.

Robin.

Manish Narani Aug. 28, 2020, 11:41 a.m. UTC | #6

Hi Felipe,

Thanks for the review. Please find my comments below inline.

> -----Original Message-----
> From: Felipe Balbi <[e-mail]>
> Sent: Thursday, August 27, 2020 12:02 PM
> To: Manish Narani <[e-mail]>; [e-mail];
> [e-mail]; Michal Simek <[e-mail]>;
> [e-mail]
> Cc: [e-mail]; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; git <[e-mail]>;
> Manish Narani <[e-mail]>
> Subject: Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
> 
> Manish Narani <[e-mail]> writes:
> 
> > Add a new driver for supporting Xilinx platforms. This driver handles
> > the USB 3.0 PHY initialization and PIPE control & reset operations for
> 
> PHY initialization should be done as part of a drivers/phy driver.

Yes. This driver calls those APIs present in the Xilinx PHY driver (drivers/phy/xilinx/phy-zynqmp.c) for initializing the PHY.
May be I can optimize this commit message a bit.

> 
> > ZynqMP platforms. This also handles the USB 2.0 PHY initialization and
> > reset operations for Versal platforms.
> 
> similarly for USB2 PHYs
> 
> > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> > new file mode 100644
> > index 000000000000..272906797a7a
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> > @@ -0,0 +1,416 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * dwc3-xilinx.c - Xilinx DWC3 controller specific glue driver
> > + *
> > + * Authors: Anurag Kumar Vulisha <[e-mail]>
> > + *          Manish Narani <[e-mail]>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +#include <linux/of_address.h>
> > +#include <linux/delay.h>
> > +#include <linux/firmware/xlnx-zynqmp.h>
> > +#include <linux/io.h>
> > +
> > +#include <linux/phy/phy.h>
> > +
> > +/* USB phy reset mask register */
> > +#define XLNX_USB_PHY_RST		0x001C
> > +#define XLNX_PHY_RST_MASK		0x1
> > +
> > +/* Xilinx USB 3.0 IP Register */
> > +#define XLNX_USB_COHERENCY		0x005C
> > +#define XLNX_USB_COHERENCY_ENABLE	0x1
> > +
> > +/* Versal USB Reset ID */
> > +#define VERSAL_USB_RESET_ID		0xC104036
> > +
> > +#define PIPE_CLK_OFFSET			0x7c
> > +#define PIPE_CLK_ON			1
> > +#define PIPE_CLK_OFF			0
> > +#define PIPE_POWER_OFFSET		0x80
> > +#define PIPE_POWER_ON			1
> > +#define PIPE_POWER_OFF			0
> > +
> > +#define RST_TIMEOUT			1000
> > +
> > +struct dwc3_xlnx {
> > +	int				num_clocks;
> > +	struct clk_bulk_data		*clks;
> > +	struct device			*dev;
> > +	void __iomem			*regs;
> > +	struct dwc3			*dwc;
> > +	struct phy			*phy;
> > +	struct phy			*usb3_phy;
> > +	struct reset_control		*crst;
> > +	struct reset_control		*hibrst;
> > +	struct reset_control		*apbrst;
> > +};
> > +
> > +static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool
> mask)
> > +{
> > +	u32 reg;
> > +
> > +	reg = readl(priv_data->regs + XLNX_USB_PHY_RST);
> > +
> > +	if (mask)
> > +		/*
> > +		 * Mask the phy reset signal from comtroller
> 
> s/comtroller/controller
> 
> But really, the comments don't bring any extra information. I'd say
> remove the comments as the code speaks for itself very clearly in this
> case.
> 
> > +static int dwc3_xlnx_rst_assert(struct reset_control *rstc)
> 
> this looks like it should be an actual reset controller driver.
> 
> > +static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
> > +{
> > +	struct device *dev = priv_data->dev;
> > +	int ret;
> > +
> > +	dwc3_xlnx_mask_phy_rst(priv_data, false);
> > +
> > +	/* Assert and De-assert reset */
> > +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> > +				     PM_RESET_ACTION_ASSERT);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to assert Reset\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
> > +				     PM_RESET_ACTION_RELEASE);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to De-assert Reset\n");
> > +		return ret;
> > +	}
> > +
> > +	dwc3_xlnx_mask_phy_rst(priv_data, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> > +{
> > +	struct device *dev = priv_data->dev;
> > +	int ret;
> > +	u32 reg;
> > +
> > +	priv_data->crst = devm_reset_control_get(dev, "usb_crst");
> > +	if (IS_ERR(priv_data->crst)) {
> > +		dev_err(dev, "failed to get %s reset signal\n", "usb_crst");
> > +		ret = PTR_ERR(priv_data->crst);
> > +		goto err;
> > +	}
> > +
> > +	priv_data->hibrst = devm_reset_control_get(dev, "usb_hibrst");
> > +	if (IS_ERR(priv_data->hibrst)) {
> > +		dev_err(dev, "failed to get %s reset signal\n", "usb_hibrst");
> > +		ret = PTR_ERR(priv_data->hibrst);
> > +		goto err;
> > +	}
> > +
> > +	priv_data->apbrst = devm_reset_control_get(dev, "usb_apbrst");
> > +	if (IS_ERR(priv_data->apbrst)) {
> > +		dev_err(dev, "failed to get %s reset signal\n", "usb_apbrst");
> > +		ret = PTR_ERR(priv_data->apbrst);
> > +		goto err;
> > +	}
> > +
> > +	priv_data->usb3_phy = devm_phy_get(dev, "usb3-phy");
> > +
> > +	ret = dwc3_xlnx_rst_assert(priv_data->crst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> > +			__func__, __LINE__);
> 
> you don't need the function name or the line number here. Just improve
> your error message a bit:
> 
> 		dev_err(dev, "Failed to assert usb3-phy reset\n");
> 
> > +		goto err;
> > +	}
> > +
> > +	ret = dwc3_xlnx_rst_assert(priv_data->hibrst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> > +			__func__, __LINE__);
> 
> 		dev_err(dev, "Failed to assert hibernation reset\n");
> 
> > +		goto err;
> > +	}
> > +
> > +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> > +			__func__, __LINE__);
> 
> 		dev_err(dev, "Failed to assert APB reset\n");
> 
> > +		goto err;
> > +	}
> > +
> > +	ret = phy_init(priv_data->usb3_phy);
> 
> dwc3 core should be handling this already

The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy which has 4 GT lanes and can used by 4 peripherals at a time.
This USB controller uses 1 GT phy lane among the 4 GT lanes. To configure the GT lane for USB controller, the below sequence is expected.
1. Assert the USB controller resets.
2. Configure the Xilinx GT phy lane for USB controller (phy_init).
3. De-assert the USB controller resets and configure PIPE.
4. Wait for PLL of the GT lane used by USB to be locked (phy_power_on).
The dwc3 core by default does the phy_init() and phy_power_on() but the default sequence doesn't work with Xilinx platforms. Because of this reason, we have introduced this new driver to support the new sequence.

> 
> > +	if (ret < 0) {
> > +		phy_exit(priv_data->usb3_phy);
> > +		goto err;
> > +	}
> > +
> > +	ret = dwc3_xlnx_rst_deassert(priv_data->apbrst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
> > +			__func__, __LINE__);
> > +		goto err;
> > +	}
> > +
> > +	/* Set PIPE power present signal */
> > +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
> > +
> > +	/* Clear PIPE CLK signal */
> > +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
> 
> shouldn't this be hidden under clk_enable()?

Though its naming suggests something related to clock framework, it is a register in the Xilinx USB controller space which configures the PIPE clock coming from Serdes.

> 
> > +	ret = dwc3_xlnx_rst_deassert(priv_data->crst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
> > +			__func__, __LINE__);
> > +		goto err;
> > +	}
> > +
> > +	ret = dwc3_xlnx_rst_deassert(priv_data->hibrst);
> > +	if (ret < 0) {
> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
> > +			__func__, __LINE__);
> > +		goto err;
> > +	}
> > +
> > +	ret = phy_power_on(priv_data->usb3_phy);
> > +	if (ret < 0) {
> > +		phy_exit(priv_data->usb3_phy);
> > +		goto err;
> > +	}
> > +
> > +	/*
> > +	 * This routes the usb dma traffic to go through CCI path instead
> > +	 * of reaching DDR directly. This traffic routing is needed to
> > +	 * make SMMU and CCI work with USB dma.
> > +	 */
> > +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
> > +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
> > +		reg |= XLNX_USB_COHERENCY_ENABLE;
> > +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
> > +	}
> > +
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int dwc3_xlnx_probe(struct platform_device *pdev)
> > +{
> > +	struct dwc3_xlnx	*priv_data;
> > +	struct device		*dev = &pdev->dev;
> > +	struct device_node	*np = dev->of_node;
> > +	struct resource		*res;
> > +	void __iomem		*regs;
> > +	int			ret;
> > +
> > +	priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
> > +	if (!priv_data)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "missing memory resource\n");
> > +		return -ENODEV;
> > +	}
> 
> you don't need to check res here. devm_ioremap_resource() already does
> that for you.
> 
> > +	regs = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> 
> this looks like it could use an error message.
> 
> > +	/* Store the usb control regs into priv_data for further usage */
> 
> pointless comment.
> 
> > +	priv_data->regs = regs;
> > +
> 
> unnecessary blank line.
> 
> > +	priv_data->dev = dev;
> > +
> > +	platform_set_drvdata(pdev, priv_data);
> > +
> > +	ret = clk_bulk_get_all(priv_data->dev, &priv_data->clks);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	priv_data->num_clocks = ret;
> > +
> > +	ret = clk_bulk_prepare_enable(priv_data->num_clocks, priv_data-
> >clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-dwc3"))
> {
> > +		ret = dwc3_xlnx_init_zynqmp(priv_data);
> > +		if (ret)
> > +			goto err_clk_put;
> > +	}
> 
> instead, you could pass a pointer to dwc3_xlnx_init_zynqmp() as driver
> data and just call it unconditionally. It would avoid the compatible
> check here.
> 
> > +	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,versal-dwc3")) {
> > +		ret = dwc3_xlnx_init_versal(priv_data);
> > +		if (ret)
> > +			goto err_clk_put;
> > +	}
> 
> same as above
> 
> > +	ret = of_platform_populate(np, NULL, NULL, dev);
> > +	if (ret)
> > +		goto err_clk_put;
> > +
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +	pm_suspend_ignore_children(dev, false);
> > +	pm_runtime_get_sync(dev);
> > +
> > +	pm_runtime_forbid(dev);
> 
> why forbid? You already have a get_sync()
> 
> > +	return 0;
> > +
> > +err_clk_put:
> > +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> > +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> > +
> > +	return ret;
> > +}
> > +
> > +static int dwc3_xlnx_remove(struct platform_device *pdev)
> > +{
> > +	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);
> > +	struct device		*dev = &pdev->dev;
> > +
> > +	of_platform_depopulate(dev);
> > +
> > +	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
> > +	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
> > +	priv_data->num_clocks = 0;
> > +
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> > +	pm_runtime_set_suspended(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int dwc3_xlnx_runtime_suspend(struct device *dev)
> > +{
> > +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> > +
> > +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_xlnx_runtime_idle(struct device *dev)
> > +{
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_autosuspend(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_xlnx_runtime_resume(struct device *dev)
> > +{
> > +	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
> > +
> > +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int dwc3_xlnx_suspend(struct device *dev)
> > +{
> > +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> > +
> > +	/* Disable the clocks */
> > +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_xlnx_resume(struct device *dev)
> > +{
> > +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> > +
> > +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
> > +}
> 
> you have the same implementation for both types of suspend/resume. Maybe
> extract dwc3_xlnx_{suspend,resume}_common() and just call it from both
> callbacks?

Going forward we have a plan to add Hibernation handling calls in dwc3_xlnx_suspend/resume functions.
For that reason, these APIs are kept separate. If you insist, I can make them common for now and separate them later when I add hibernation code.

> 
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(dwc3_xlnx_suspend, dwc3_xlnx_resume)
> > +	SET_RUNTIME_PM_OPS(dwc3_xlnx_runtime_suspend,
> dwc3_xlnx_runtime_resume,
> > +			   dwc3_xlnx_runtime_idle)
> 
> seems like it could be replaced with UNIVERSAL_PM_OPS with minor
> modifications.
> 
> --
> balbi

Thanks,
Manish

Manish Narani Aug. 28, 2020, 5:53 p.m. UTC | #7

Hi Robin,

Thanks for the review. Please find my comment below inline.

> -----Original Message-----
> From: Robin Murphy <[e-mail]>
> Sent: Friday, August 28, 2020 12:17 AM
> To: Manish Narani <[e-mail]>; [e-mail];
> [e-mail]; Michal Simek <[e-mail]>; [e-mail];
> [e-mail]
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; git <[e-mail]>; linux-arm-
> [e-mail]
> Subject: Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
> 
> On 2020-08-26 19:44, Manish Narani wrote:
> [...]
> > +	/*
> > +	 * This routes the usb dma traffic to go through CCI path instead
> > +	 * of reaching DDR directly. This traffic routing is needed to
> > +	 * make SMMU and CCI work with USB dma.
> > +	 */
> > +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
> > +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
> > +		reg |= XLNX_USB_COHERENCY_ENABLE;
> > +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
> > +	}
> 
> This looks rather suspect - coherency should be based on coherency, not
> on whether an IOMMU group is present. If the device isn't described as
> coherent in the DT, then any SMMU mappings will end up using attributes
> that will downgrade traffic to be non-snooping anyway. And if the SMMU
> is enabled but not translating (e.g. "iommu.passthrough=1") then
> enabling hardware coherency when the DMA layer hasn't been told about it
> can potentially lead to nasty subtle problems and data loss.

May be the description needs to be updated in this. This is not the actual coherency enabling bit, but this is needed when coherency is enabled.
This is a register inside Xilinx USB controller which handles USB (which is in LPD) traffic route switching from LPD (Low Power Domain) to FPD (Full Power Domain)  path in the Xilinx SoC in either of the below scenarios:
1. Device is described coherent in  DT.
2. SMMU is enabled.

I will update the same in v2.

Thanks,
Manish

Robin Murphy Sept. 1, 2020, noon UTC | #8

On 2020-08-28 18:53, Manish Narani wrote:
> Hi Robin,
> 
> Thanks for the review. Please find my comment below inline.
> 
>> -----Original Message-----
>> From: Robin Murphy <[e-mail]>
>> Sent: Friday, August 28, 2020 12:17 AM
>> To: Manish Narani <[e-mail]>; [e-mail];
>> [e-mail]; Michal Simek <[e-mail]>; [e-mail];
>> [e-mail]
>> Cc: [e-mail]; [e-mail]; linux-
>> [e-mail]; git <[e-mail]>; linux-arm-
>> [e-mail]
>> Subject: Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
>>
>> On 2020-08-26 19:44, Manish Narani wrote:
>> [...]
>>> +	/*
>>> +	 * This routes the usb dma traffic to go through CCI path instead
>>> +	 * of reaching DDR directly. This traffic routing is needed to
>>> +	 * make SMMU and CCI work with USB dma.
>>> +	 */
>>> +	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
>>> +		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
>>> +		reg |= XLNX_USB_COHERENCY_ENABLE;
>>> +		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
>>> +	}
>>
>> This looks rather suspect - coherency should be based on coherency, not
>> on whether an IOMMU group is present. If the device isn't described as
>> coherent in the DT, then any SMMU mappings will end up using attributes
>> that will downgrade traffic to be non-snooping anyway. And if the SMMU
>> is enabled but not translating (e.g. "iommu.passthrough=1") then
>> enabling hardware coherency when the DMA layer hasn't been told about it
>> can potentially lead to nasty subtle problems and data loss.
> 
> May be the description needs to be updated in this. This is not the actual coherency enabling bit, but this is needed when coherency is enabled.
> This is a register inside Xilinx USB controller which handles USB (which is in LPD) traffic route switching from LPD (Low Power Domain) to FPD (Full Power Domain)  path in the Xilinx SoC in either of the below scenarios:
> 1. Device is described coherent in  DT.
> 2. SMMU is enabled.
> 
> I will update the same in v2.

Ah, OK, so it's just that the control bit itself has a terrible name :)

 From the available information I had assumed that this controlled the 
output attributes, and that the interconnect might then steer traffic 
based on those. Explaining a bit more clearly in the comment probably 
would be a good idea. In that case, I'd concur that the current logic is 
in fact appropriate, but please use the device_iommu_mapped() helper for 
cleanliness.

Cheers,
Robin.

Felipe Balbi Sept. 1, 2020, 12:15 p.m. UTC | #9

Hi,

(remember to break your lines at 80-columns)

Manish Narani <[e-mail]> writes:

<snip>

>> > +		goto err;
>> > +	}
>> > +
>> > +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
>> > +	if (ret < 0) {
>> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
>> > +			__func__, __LINE__);
>> 
>> 		dev_err(dev, "Failed to assert APB reset\n");
>> 
>> > +		goto err;
>> > +	}
>> > +
>> > +	ret = phy_init(priv_data->usb3_phy);
>> 
>> dwc3 core should be handling this already
>
> The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
> which has 4 GT lanes and can used by 4 peripherals at a time.

At the same time or are they mutually exclusive?

> This USB controller uses 1 GT phy lane among the 4 GT lanes. To
> configure the GT lane for USB controller, the below sequence is
> expected.
>
> 1. Assert the USB controller resets.
> 2. Configure the Xilinx GT phy lane for USB controller (phy_init).
> 3. De-assert the USB controller resets and configure PIPE.
> 4. Wait for PLL of the GT lane used by USB to be locked
>    (phy_power_on).

it seems like you need to extend the PHY framework and teach it about
lane configuration.

> The dwc3 core by default does the phy_init() and phy_power_on() but
> the default sequence doesn't work with Xilinx platforms. Because of
> this reason, we have introduced this new driver to support the new
> sequence.

Instead of teaching the relevant framework about your new requirements
;-)

>> > +	if (ret < 0) {
>> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
>> > +			__func__, __LINE__);
>> > +		goto err;
>> > +	}
>> > +
>> > +	/* Set PIPE power present signal */
>> > +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
>> > +
>> > +	/* Clear PIPE CLK signal */
>> > +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
>> 
>> shouldn't this be hidden under clk_enable()?
>
> Though its naming suggests something related to clock framework, it is
> a register in the Xilinx USB controller space which configures the
> PIPE clock coming from Serdes.

PIPE clock is a clock. It just so happens that the source is the PHY
itself.

>> > +static int dwc3_xlnx_resume(struct device *dev)
>> > +{
>> > +	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
>> > +
>> > +	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
>> > +}
>> 
>> you have the same implementation for both types of suspend/resume. Maybe
>> extract dwc3_xlnx_{suspend,resume}_common() and just call it from both
>> callbacks?
>
> Going forward we have a plan to add Hibernation handling calls in
> dwc3_xlnx_suspend/resume functions.

at that moment and only at that moment, should you be worried about
splitting them.

> For that reason, these APIs are kept separate. If you insist, I can
> make them common for now and separate them later when I add
> hibernation code.

It would be a little better, no?

cheers

Manish Narani Sept. 9, 2020, 9:14 a.m. UTC | #10

Hi Felipe,

Thanks for the response.

> -----Original Message-----
> From: Felipe Balbi <[e-mail]>
> Sent: Tuesday, September 1, 2020 5:45 PM
> 
> >> > +		goto err;
> >> > +	}
> >> > +
> >> > +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
> >> > +	if (ret < 0) {
> >> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
> >> > +			__func__, __LINE__);
> >>
> >> 		dev_err(dev, "Failed to assert APB reset\n");
> >>
> >> > +		goto err;
> >> > +	}
> >> > +
> >> > +	ret = phy_init(priv_data->usb3_phy);
> >>
> >> dwc3 core should be handling this already
> >
> > The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
> > which has 4 GT lanes and can used by 4 peripherals at a time.
> 
> At the same time or are they mutually exclusive?

The lanes are mutually exclusive.

 [...]
> >> > +	if (ret < 0) {
> >> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
> >> > +			__func__, __LINE__);
> >> > +		goto err;
> >> > +	}
> >> > +
> >> > +	/* Set PIPE power present signal */
> >> > +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
> >> > +
> >> > +	/* Clear PIPE CLK signal */
> >> > +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
> >>
> >> shouldn't this be hidden under clk_enable()?
> >
> > Though its naming suggests something related to clock framework, it is
> > a register in the Xilinx USB controller space which configures the
> > PIPE clock coming from Serdes.
> 
> PIPE clock is a clock. It just so happens that the source is the PHY
> itself.

This bit is used to choose between PIPE clock coming from SerDes
and the Suspend Clock. When the controller is out of reset, this bit
needs to be reset in order to make the USB controller work. This
register is added in Xilinx USB controller register space. I will
add more description about the same in v2.

Thanks,
Manish

Felipe Balbi Sept. 9, 2020, 10:26 a.m. UTC | #11

Hi,

Manish Narani <[e-mail]> writes:
>> -----Original Message-----
>> From: Felipe Balbi <[e-mail]>
>> Sent: Tuesday, September 1, 2020 5:45 PM
>> 
>> >> > +		goto err;
>> >> > +	}
>> >> > +
>> >> > +	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
>> >> > +	if (ret < 0) {
>> >> > +		dev_err(dev, "%s: %d: Failed to assert reset\n",
>> >> > +			__func__, __LINE__);
>> >>
>> >> 		dev_err(dev, "Failed to assert APB reset\n");
>> >>
>> >> > +		goto err;
>> >> > +	}
>> >> > +
>> >> > +	ret = phy_init(priv_data->usb3_phy);
>> >>
>> >> dwc3 core should be handling this already
>> >
>> > The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
>> > which has 4 GT lanes and can used by 4 peripherals at a time.
>> 
>> At the same time or are they mutually exclusive?
>
> The lanes are mutually exclusive.

Thank you for confirming :-)

>  [...]
>> >> > +	if (ret < 0) {
>> >> > +		dev_err(dev, "%s: %d: Failed to release reset\n",
>> >> > +			__func__, __LINE__);
>> >> > +		goto err;
>> >> > +	}
>> >> > +
>> >> > +	/* Set PIPE power present signal */
>> >> > +	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
>> >> > +
>> >> > +	/* Clear PIPE CLK signal */
>> >> > +	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
>> >>
>> >> shouldn't this be hidden under clk_enable()?
>> >
>> > Though its naming suggests something related to clock framework, it is
>> > a register in the Xilinx USB controller space which configures the
>> > PIPE clock coming from Serdes.
>> 
>> PIPE clock is a clock. It just so happens that the source is the PHY
>> itself.
>
> This bit is used to choose between PIPE clock coming from SerDes
> and the Suspend Clock. When the controller is out of reset, this bit
> needs to be reset in order to make the USB controller work. This
> register is added in Xilinx USB controller register space. I will
> add more description about the same in v2.

Aha! That clarifies. It's just a clock selection from clocks that are
generated elsewhere :-) I guess a clk driver would be overkill, indeed.

Thanks for explaining. Could you add some of this information to commit
log, then?

cheers

Patch

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 7a2304565a73..416063ee9d05 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -139,4 +139,12 @@  config USB_DWC3_QCOM
 	  for peripheral mode support.
 	  Say 'Y' or 'M' if you have one such device.
 
+config USB_DWC3_XILINX
+       tristate "Xilinx Platforms"
+       depends on (ARCH_ZYNQMP || ARCH_VERSAL) && OF
+       default USB_DWC3
+       help
+         Support Xilinx SoCs with DesignWare Core USB3 IP.
+	 Say 'Y' or 'M' if you have one such device.
+
 endif
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index ae86da0dc5bd..add567578b1f 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -51,3 +51,4 @@  obj-$(CONFIG_USB_DWC3_MESON_G12A)	+= dwc3-meson-g12a.o
 obj-$(CONFIG_USB_DWC3_OF_SIMPLE)	+= dwc3-of-simple.o
 obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
 obj-$(CONFIG_USB_DWC3_QCOM)		+= dwc3-qcom.o
+obj-$(CONFIG_USB_DWC3_XILINX)		+= dwc3-xilinx.o
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 7df115012935..e3a485b76818 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -172,7 +172,6 @@  static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops = {
 
 static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "rockchip,rk3399-dwc3" },
-	{ .compatible = "xlnx,zynqmp-dwc3" },
 	{ .compatible = "cavium,octeon-7130-usb-uctl" },
 	{ .compatible = "sprd,sc9860-dwc3" },
 	{ .compatible = "allwinner,sun50i-h6-dwc3" },
diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
new file mode 100644
index 000000000000..272906797a7a
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -0,0 +1,416 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * dwc3-xilinx.c - Xilinx DWC3 controller specific glue driver
+ *
+ * Authors: Anurag Kumar Vulisha <[e-mail]>
+ *          Manish Narani <[e-mail]>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/io.h>
+
+#include <linux/phy/phy.h>
+
+/* USB phy reset mask register */
+#define XLNX_USB_PHY_RST		0x001C
+#define XLNX_PHY_RST_MASK		0x1
+
+/* Xilinx USB 3.0 IP Register */
+#define XLNX_USB_COHERENCY		0x005C
+#define XLNX_USB_COHERENCY_ENABLE	0x1
+
+/* Versal USB Reset ID */
+#define VERSAL_USB_RESET_ID		0xC104036
+
+#define PIPE_CLK_OFFSET			0x7c
+#define PIPE_CLK_ON			1
+#define PIPE_CLK_OFF			0
+#define PIPE_POWER_OFFSET		0x80
+#define PIPE_POWER_ON			1
+#define PIPE_POWER_OFF			0
+
+#define RST_TIMEOUT			1000
+
+struct dwc3_xlnx {
+	int				num_clocks;
+	struct clk_bulk_data		*clks;
+	struct device			*dev;
+	void __iomem			*regs;
+	struct dwc3			*dwc;
+	struct phy			*phy;
+	struct phy			*usb3_phy;
+	struct reset_control		*crst;
+	struct reset_control		*hibrst;
+	struct reset_control		*apbrst;
+};
+
+static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
+{
+	u32 reg;
+
+	reg = readl(priv_data->regs + XLNX_USB_PHY_RST);
+
+	if (mask)
+		/*
+		 * Mask the phy reset signal from comtroller
+		 * reaching ULPI phy. This can be done by
+		 * writing 0 into usb2_phy_reset register
+		 */
+		reg &= ~XLNX_PHY_RST_MASK;
+	else
+		/*
+		 * Allow phy reset signal from controller to
+		 * reset ULPI phy. This can be done by writing
+		 * 0x1 into usb2_phy_reset register
+		 */
+		reg |= XLNX_PHY_RST_MASK;
+
+	writel(reg, priv_data->regs + XLNX_USB_PHY_RST);
+}
+
+static int dwc3_xlnx_rst_assert(struct reset_control *rstc)
+{
+	unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
+	unsigned long timeout;
+
+	reset_control_assert(rstc);
+
+	/* wait until reset is asserted or timeout */
+	timeout = jiffies + loop_time;
+
+	while (!time_after_eq(jiffies, timeout)) {
+		if (reset_control_status(rstc) > 0)
+			return 0;
+
+		cpu_relax();
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int dwc3_xlnx_rst_deassert(struct reset_control *rstc)
+{
+	unsigned long loop_time = msecs_to_jiffies(RST_TIMEOUT);
+	unsigned long timeout;
+
+	reset_control_deassert(rstc);
+
+	/* wait until reset is de-asserted or timeout */
+	timeout = jiffies + loop_time;
+	while (!time_after_eq(jiffies, timeout)) {
+		if (!reset_control_status(rstc))
+			return 0;
+
+		cpu_relax();
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
+{
+	struct device *dev = priv_data->dev;
+	int ret;
+
+	dwc3_xlnx_mask_phy_rst(priv_data, false);
+
+	/* Assert and De-assert reset */
+	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
+				     PM_RESET_ACTION_ASSERT);
+	if (ret < 0) {
+		dev_err(dev, "failed to assert Reset\n");
+		return ret;
+	}
+
+	ret = zynqmp_pm_reset_assert(VERSAL_USB_RESET_ID,
+				     PM_RESET_ACTION_RELEASE);
+	if (ret < 0) {
+		dev_err(dev, "failed to De-assert Reset\n");
+		return ret;
+	}
+
+	dwc3_xlnx_mask_phy_rst(priv_data, true);
+
+	return 0;
+}
+
+static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
+{
+	struct device *dev = priv_data->dev;
+	int ret;
+	u32 reg;
+
+	priv_data->crst = devm_reset_control_get(dev, "usb_crst");
+	if (IS_ERR(priv_data->crst)) {
+		dev_err(dev, "failed to get %s reset signal\n", "usb_crst");
+		ret = PTR_ERR(priv_data->crst);
+		goto err;
+	}
+
+	priv_data->hibrst = devm_reset_control_get(dev, "usb_hibrst");
+	if (IS_ERR(priv_data->hibrst)) {
+		dev_err(dev, "failed to get %s reset signal\n", "usb_hibrst");
+		ret = PTR_ERR(priv_data->hibrst);
+		goto err;
+	}
+
+	priv_data->apbrst = devm_reset_control_get(dev, "usb_apbrst");
+	if (IS_ERR(priv_data->apbrst)) {
+		dev_err(dev, "failed to get %s reset signal\n", "usb_apbrst");
+		ret = PTR_ERR(priv_data->apbrst);
+		goto err;
+	}
+
+	priv_data->usb3_phy = devm_phy_get(dev, "usb3-phy");
+
+	ret = dwc3_xlnx_rst_assert(priv_data->crst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to assert reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	ret = dwc3_xlnx_rst_assert(priv_data->hibrst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to assert reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to assert reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	ret = phy_init(priv_data->usb3_phy);
+	if (ret < 0) {
+		phy_exit(priv_data->usb3_phy);
+		goto err;
+	}
+
+	ret = dwc3_xlnx_rst_deassert(priv_data->apbrst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to release reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	/* Set PIPE power present signal */
+	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
+
+	/* Clear PIPE CLK signal */
+	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
+
+	ret = dwc3_xlnx_rst_deassert(priv_data->crst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to release reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	ret = dwc3_xlnx_rst_deassert(priv_data->hibrst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to release reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	ret = phy_power_on(priv_data->usb3_phy);
+	if (ret < 0) {
+		phy_exit(priv_data->usb3_phy);
+		goto err;
+	}
+
+	/*
+	 * This routes the usb dma traffic to go through CCI path instead
+	 * of reaching DDR directly. This traffic routing is needed to
+	 * make SMMU and CCI work with USB dma.
+	 */
+	if (of_dma_is_coherent(dev->of_node) || dev->iommu_group) {
+		reg = readl(priv_data->regs + XLNX_USB_COHERENCY);
+		reg |= XLNX_USB_COHERENCY_ENABLE;
+		writel(reg, priv_data->regs + XLNX_USB_COHERENCY);
+	}
+
+err:
+	return ret;
+}
+
+static int dwc3_xlnx_probe(struct platform_device *pdev)
+{
+	struct dwc3_xlnx	*priv_data;
+	struct device		*dev = &pdev->dev;
+	struct device_node	*np = dev->of_node;
+	struct resource		*res;
+	void __iomem		*regs;
+	int			ret;
+
+	priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
+	if (!priv_data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "missing memory resource\n");
+		return -ENODEV;
+	}
+
+	regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	/* Store the usb control regs into priv_data for further usage */
+	priv_data->regs = regs;
+
+	priv_data->dev = dev;
+
+	platform_set_drvdata(pdev, priv_data);
+
+	ret = clk_bulk_get_all(priv_data->dev, &priv_data->clks);
+	if (ret < 0)
+		return ret;
+
+	priv_data->num_clocks = ret;
+
+	ret = clk_bulk_prepare_enable(priv_data->num_clocks, priv_data->clks);
+	if (ret)
+		return ret;
+
+	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-dwc3")) {
+		ret = dwc3_xlnx_init_zynqmp(priv_data);
+		if (ret)
+			goto err_clk_put;
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node, "xlnx,versal-dwc3")) {
+		ret = dwc3_xlnx_init_versal(priv_data);
+		if (ret)
+			goto err_clk_put;
+	}
+
+	ret = of_platform_populate(np, NULL, NULL, dev);
+	if (ret)
+		goto err_clk_put;
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_suspend_ignore_children(dev, false);
+	pm_runtime_get_sync(dev);
+
+	pm_runtime_forbid(dev);
+
+	return 0;
+
+err_clk_put:
+	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
+	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
+
+	return ret;
+}
+
+static int dwc3_xlnx_remove(struct platform_device *pdev)
+{
+	struct dwc3_xlnx	*priv_data = platform_get_drvdata(pdev);
+	struct device		*dev = &pdev->dev;
+
+	of_platform_depopulate(dev);
+
+	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
+	clk_bulk_put_all(priv_data->num_clocks, priv_data->clks);
+	priv_data->num_clocks = 0;
+
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_set_suspended(dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int dwc3_xlnx_runtime_suspend(struct device *dev)
+{
+	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
+
+	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
+
+	return 0;
+}
+
+static int dwc3_xlnx_runtime_idle(struct device *dev)
+{
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_autosuspend(dev);
+
+	return 0;
+}
+
+static int dwc3_xlnx_runtime_resume(struct device *dev)
+{
+	struct dwc3_xlnx	*priv_data = dev_get_drvdata(dev);
+
+	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
+}
+#endif /* CONFIG_PM */
+
+#ifdef CONFIG_PM_SLEEP
+static int dwc3_xlnx_suspend(struct device *dev)
+{
+	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
+
+	/* Disable the clocks */
+	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
+
+	return 0;
+}
+
+static int dwc3_xlnx_resume(struct device *dev)
+{
+	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
+
+	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwc3_xlnx_suspend, dwc3_xlnx_resume)
+	SET_RUNTIME_PM_OPS(dwc3_xlnx_runtime_suspend, dwc3_xlnx_runtime_resume,
+			   dwc3_xlnx_runtime_idle)
+};
+
+static const struct of_device_id of_dwc3_xlnx_match[] = {
+	{ .compatible = "xlnx,zynqmp-dwc3" },
+	{ .compatible = "xlnx,versal-dwc3" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_dwc3_xlnx_match);
+
+static struct platform_driver dwc3_xlnx_driver = {
+	.probe		= dwc3_xlnx_probe,
+	.remove		= dwc3_xlnx_remove,
+	.driver		= {
+		.name	= "dwc3-xilinx",
+		.of_match_table = of_dwc3_xlnx_match,
+		.pm	= &dwc3_xlnx_dev_pm_ops,
+	},
+};
+
+module_platform_driver(dwc3_xlnx_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Xilinx DWC3 controller specific glue driver");
+MODULE_AUTHOR("Anurag Kumar Vulisha <[e-mail]>");
+MODULE_AUTHOR("Manish Narani <[e-mail]>");