This patch series documents the Xilinx Versal DWC3 controller. This also
adds a new Xilinx specific driver for adding new features in the future.
Manish Narani (2):
dt-bindings: usb: dwc3-xilinx: Add documentation for Versal DWC3
Controller
usb: dwc3: Add driver for Xilinx platforms
.../devicetree/bindings/usb/dwc3-xilinx.txt | 12 +-
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 ++++++++++++++++++
5 files changed, 436 insertions(+), 2 deletions(-)
create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c
--
2.17.1
Add documentation for Versal DWC3 controller. Add required property
'reg' for the same. Also add optional properties for snps,dwc3.
Signed-off-by: Manish Narani <[email protected]>
---
.../devicetree/bindings/usb/dwc3-xilinx.txt | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
index 4aae5b2cef56..dd41ed831411 100644
--- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
@@ -1,7 +1,8 @@
Xilinx SuperSpeed DWC3 USB SoC controller
Required properties:
-- compatible: Should contain "xlnx,zynqmp-dwc3"
+- compatible: May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-dwc3"
+- reg: Base address and length of the register control block
- clocks: A list of phandles for the clocks listed in clock-names
- clock-names: Should contain the following:
"bus_clk" Master/Core clock, have to be >= 125 MHz for SS
@@ -13,12 +14,19 @@ Required child node:
A child node must exist to represent the core DWC3 IP block. The name of
the node is not important. The content of the node is defined in dwc3.txt.
+Optional properties for snps,dwc3:
+- dma-coherent: Enable this flag if CCI is enabled in design. Adding this
+ flag configures Global SoC bus Configuration Register and
+ Xilinx USB 3.0 IP - USB coherency register to enable CCI.
+- interrupt-names: This property provides the names of the interrupt ids used
+
Example device node:
usb@0 {
#address-cells = <0x2>;
#size-cells = <0x1>;
compatible = "xlnx,zynqmp-dwc3";
+ reg = <0x0 0xff9d0000 0x0 0x100>;
clock-names = "bus_clk" "ref_clk";
clocks = <&clk125>, <&clk125>;
ranges;
@@ -26,7 +34,9 @@ Example device node:
dwc3@fe200000 {
compatible = "snps,dwc3";
reg = <0x0 0xfe200000 0x40000>;
+ interrupt-name = "dwc_usb3";
interrupts = <0x0 0x41 0x4>;
dr_mode = "host";
+ dma-coherent;
};
};
--
2.17.1
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 <[email protected]>
---
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
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 <[email protected]>
+ * Manish Narani <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_AUTHOR("Manish Narani <[email protected]>");
--
2.17.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.
--
~Randy
Manish Narani <[email protected]> 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 <[email protected]>
> + * Manish Narani <[email protected]>
> + */
> +
> +#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.
--
balbi
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 <[email protected]>
> ---
> 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 <[email protected]>");
> +MODULE_AUTHOR("Manish Narani <[email protected]>");
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 <[email protected]>
> ---
[...]
> +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
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.
Hi Felipe,
Thanks for the review. Please find my comments below inline.
> -----Original Message-----
> From: Felipe Balbi <[email protected]>
> Sent: Thursday, August 27, 2020 12:02 PM
> To: Manish Narani <[email protected]>; [email protected];
> [email protected]; Michal Simek <[email protected]>;
> [email protected]
> Cc: [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; git <[email protected]>;
> Manish Narani <[email protected]>
> Subject: Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
>
> Manish Narani <[email protected]> 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 <[email protected]>
> > + * Manish Narani <[email protected]>
> > + */
> > +
> > +#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
Hi Robin,
Thanks for the review. Please find my comment below inline.
> -----Original Message-----
> From: Robin Murphy <[email protected]>
> Sent: Friday, August 28, 2020 12:17 AM
> To: Manish Narani <[email protected]>; [email protected];
> [email protected]; Michal Simek <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; git <[email protected]>; linux-arm-
> [email protected]
> 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
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 <[email protected]>
>> Sent: Friday, August 28, 2020 12:17 AM
>> To: Manish Narani <[email protected]>; [email protected];
>> [email protected]; Michal Simek <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; git <[email protected]>; linux-arm-
>> [email protected]
>> 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.
Hi,
(remember to break your lines at 80-columns)
Manish Narani <[email protected]> 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
--
balbi
On Thu, Aug 27, 2020 at 12:14:00AM +0530, Manish Narani wrote:
> Add documentation for Versal DWC3 controller. Add required property
> 'reg' for the same. Also add optional properties for snps,dwc3.
>
> Signed-off-by: Manish Narani <[email protected]>
> ---
> .../devicetree/bindings/usb/dwc3-xilinx.txt | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> index 4aae5b2cef56..dd41ed831411 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> @@ -1,7 +1,8 @@
> Xilinx SuperSpeed DWC3 USB SoC controller
>
> Required properties:
> -- compatible: Should contain "xlnx,zynqmp-dwc3"
> +- compatible: May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-dwc3"
> +- reg: Base address and length of the register control block
> - clocks: A list of phandles for the clocks listed in clock-names
> - clock-names: Should contain the following:
> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS
> @@ -13,12 +14,19 @@ Required child node:
> A child node must exist to represent the core DWC3 IP block. The name of
> the node is not important. The content of the node is defined in dwc3.txt.
>
> +Optional properties for snps,dwc3:
> +- dma-coherent: Enable this flag if CCI is enabled in design. Adding this
> + flag configures Global SoC bus Configuration Register and
> + Xilinx USB 3.0 IP - USB coherency register to enable CCI.
> +- interrupt-names: This property provides the names of the interrupt ids used
You have to define what the names are. 'dwc_usb3' seems pretty pointless
if only 1 name.
> +
> Example device node:
>
> usb@0 {
> #address-cells = <0x2>;
> #size-cells = <0x1>;
> compatible = "xlnx,zynqmp-dwc3";
> + reg = <0x0 0xff9d0000 0x0 0x100>;
> clock-names = "bus_clk" "ref_clk";
> clocks = <&clk125>, <&clk125>;
> ranges;
> @@ -26,7 +34,9 @@ Example device node:
> dwc3@fe200000 {
> compatible = "snps,dwc3";
> reg = <0x0 0xfe200000 0x40000>;
> + interrupt-name = "dwc_usb3";
> interrupts = <0x0 0x41 0x4>;
> dr_mode = "host";
> + dma-coherent;
> };
> };
> --
> 2.17.1
>
Hi Felipe,
Thanks for the response.
> -----Original Message-----
> From: Felipe Balbi <[email protected]>
> 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
Hi,
Manish Narani <[email protected]> writes:
>> -----Original Message-----
>> From: Felipe Balbi <[email protected]>
>> 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
--
balbi
On Wed, Sep 9, 2020 at 9:46 AM Manish Narani <[email protected]> wrote:
>
> Hi Rob,
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: Wednesday, September 9, 2020 4:35 AM
> > To: Manish Narani <[email protected]>
> > Cc: [email protected]; Michal Simek <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; git <[email protected]>
> > Subject: Re: [PATCH 1/2] dt-bindings: usb: dwc3-xilinx: Add documentation for
> > Versal DWC3 Controller
> >
> > On Thu, Aug 27, 2020 at 12:14:00AM +0530, Manish Narani wrote:
> > > Add documentation for Versal DWC3 controller. Add required property
> > > 'reg' for the same. Also add optional properties for snps,dwc3.
> > >
> > > Signed-off-by: Manish Narani <[email protected]>
> > > ---
> > > .../devicetree/bindings/usb/dwc3-xilinx.txt | 12 +++++++++++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > > index 4aae5b2cef56..dd41ed831411 100644
> > > --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > > +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > > @@ -1,7 +1,8 @@
> > > Xilinx SuperSpeed DWC3 USB SoC controller
> > >
> > > Required properties:
> > > -- compatible: Should contain "xlnx,zynqmp-dwc3"
> > > +- compatible: May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-
> > dwc3"
> > > +- reg: Base address and length of the register control block
> > > - clocks: A list of phandles for the clocks listed in clock-names
> > > - clock-names: Should contain the following:
> > > "bus_clk" Master/Core clock, have to be >= 125 MHz for SS
> > > @@ -13,12 +14,19 @@ Required child node:
> > > A child node must exist to represent the core DWC3 IP block. The name of
> > > the node is not important. The content of the node is defined in dwc3.txt.
> > >
> > > +Optional properties for snps,dwc3:
> > > +- dma-coherent: Enable this flag if CCI is enabled in design. Adding this
> > > + flag configures Global SoC bus Configuration Register and
> > > + Xilinx USB 3.0 IP - USB coherency register to enable CCI.
> > > +- interrupt-names: This property provides the names of the interrupt ids
> > used
> >
> > You have to define what the names are. 'dwc_usb3' seems pretty pointless
> > if only 1 name.
>
> OK. I am planning to add more interrupt ids going ahead. For now I will remove
> this interrupt name in v2. The interrupt name will be added along with other interrupt
> names.
Define all the interrupts you have. Bindings should be complete, not
what a driver for some OS happens to use at some point in time.
Rob
Hi Rob,
Thanks for the review.
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Wednesday, September 9, 2020 4:35 AM
> To: Manish Narani <[email protected]>
> Cc: [email protected]; Michal Simek <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; git <[email protected]>
> Subject: Re: [PATCH 1/2] dt-bindings: usb: dwc3-xilinx: Add documentation for
> Versal DWC3 Controller
>
> On Thu, Aug 27, 2020 at 12:14:00AM +0530, Manish Narani wrote:
> > Add documentation for Versal DWC3 controller. Add required property
> > 'reg' for the same. Also add optional properties for snps,dwc3.
> >
> > Signed-off-by: Manish Narani <[email protected]>
> > ---
> > .../devicetree/bindings/usb/dwc3-xilinx.txt | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > index 4aae5b2cef56..dd41ed831411 100644
> > --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> > @@ -1,7 +1,8 @@
> > Xilinx SuperSpeed DWC3 USB SoC controller
> >
> > Required properties:
> > -- compatible: Should contain "xlnx,zynqmp-dwc3"
> > +- compatible: May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-
> dwc3"
> > +- reg: Base address and length of the register control block
> > - clocks: A list of phandles for the clocks listed in clock-names
> > - clock-names: Should contain the following:
> > "bus_clk" Master/Core clock, have to be >= 125 MHz for SS
> > @@ -13,12 +14,19 @@ Required child node:
> > A child node must exist to represent the core DWC3 IP block. The name of
> > the node is not important. The content of the node is defined in dwc3.txt.
> >
> > +Optional properties for snps,dwc3:
> > +- dma-coherent: Enable this flag if CCI is enabled in design. Adding this
> > + flag configures Global SoC bus Configuration Register and
> > + Xilinx USB 3.0 IP - USB coherency register to enable CCI.
> > +- interrupt-names: This property provides the names of the interrupt ids
> used
>
> You have to define what the names are. 'dwc_usb3' seems pretty pointless
> if only 1 name.
OK. I am planning to add more interrupt ids going ahead. For now I will remove
this interrupt name in v2. The interrupt name will be added along with other interrupt
names.
Thanks,
Manish