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 <redacted>
---

[...]

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel