* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-26 18:44 ` [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms Manish Narani
@ 2020-08-26 19:00 ` Randy Dunlap
2020-08-27 6:31 ` Felipe Balbi
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Randy Dunlap @ 2020-08-26 19:00 UTC (permalink / raw)
To: Manish Narani, gregkh, robh+dt, michal.simek, balbi, p.zabel
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel, git
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-26 18:44 ` [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms Manish Narani
2020-08-26 19:00 ` Randy Dunlap
@ 2020-08-27 6:31 ` Felipe Balbi
2020-08-28 11:41 ` Manish Narani
2020-08-27 7:42 ` Chunfeng Yun
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2020-08-27 6:31 UTC (permalink / raw)
To: Manish Narani, gregkh, robh+dt, michal.simek, p.zabel
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel, git,
Manish Narani
[-- Attachment #1: Type: text/plain, Size: 11302 bytes --]
Manish Narani <manish.narani@xilinx.com> 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 <anurag.kumar.vulisha@xilinx.com>
> + * Manish Narani <manish.narani@xilinx.com>
> + */
> +
> +#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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-27 6:31 ` Felipe Balbi
@ 2020-08-28 11:41 ` Manish Narani
2020-09-01 12:15 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Manish Narani @ 2020-08-28 11:41 UTC (permalink / raw)
To: Felipe Balbi, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, p.zabel@pengutronix.de
Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git
Hi Felipe,
Thanks for the review. Please find my comments below inline.
> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Thursday, August 27, 2020 12:02 PM
> To: Manish Narani <MNARANI@xilinx.com>; gregkh@linuxfoundation.org;
> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>;
> p.zabel@pengutronix.de
> Cc: linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git <git@xilinx.com>;
> Manish Narani <MNARANI@xilinx.com>
> Subject: Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
>
> Manish Narani <manish.narani@xilinx.com> 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 <anurag.kumar.vulisha@xilinx.com>
> > + * Manish Narani <manish.narani@xilinx.com>
> > + */
> > +
> > +#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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-28 11:41 ` Manish Narani
@ 2020-09-01 12:15 ` Felipe Balbi
2020-09-09 9:14 ` Manish Narani
0 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2020-09-01 12:15 UTC (permalink / raw)
To: Manish Narani, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, p.zabel@pengutronix.de
Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git
[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]
Hi,
(remember to break your lines at 80-columns)
Manish Narani <MNARANI@xilinx.com> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-09-01 12:15 ` Felipe Balbi
@ 2020-09-09 9:14 ` Manish Narani
2020-09-09 10:26 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Manish Narani @ 2020-09-09 9:14 UTC (permalink / raw)
To: Felipe Balbi, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, p.zabel@pengutronix.de
Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git
Hi Felipe,
Thanks for the response.
> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-09-09 9:14 ` Manish Narani
@ 2020-09-09 10:26 ` Felipe Balbi
0 siblings, 0 replies; 17+ messages in thread
From: Felipe Balbi @ 2020-09-09 10:26 UTC (permalink / raw)
To: Manish Narani, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, p.zabel@pengutronix.de
Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git
[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]
Hi,
Manish Narani <MNARANI@xilinx.com> writes:
>> -----Original Message-----
>> From: Felipe Balbi <balbi@kernel.org>
>> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-26 18:44 ` [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms Manish Narani
2020-08-26 19:00 ` Randy Dunlap
2020-08-27 6:31 ` Felipe Balbi
@ 2020-08-27 7:42 ` Chunfeng Yun
2020-08-27 9:02 ` Philipp Zabel
2020-08-27 18:46 ` Robin Murphy
4 siblings, 0 replies; 17+ messages in thread
From: Chunfeng Yun @ 2020-08-27 7:42 UTC (permalink / raw)
To: Manish Narani
Cc: gregkh, robh+dt, michal.simek, balbi, p.zabel, linux-usb,
devicetree, linux-arm-kernel, linux-kernel, git
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 <manish.narani@xilinx.com>
> ---
> 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 <anurag.kumar.vulisha@xilinx.com>");
> +MODULE_AUTHOR("Manish Narani <manish.narani@xilinx.com>");
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-26 18:44 ` [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms Manish Narani
` (2 preceding siblings ...)
2020-08-27 7:42 ` Chunfeng Yun
@ 2020-08-27 9:02 ` Philipp Zabel
2020-08-27 18:46 ` Robin Murphy
4 siblings, 0 replies; 17+ messages in thread
From: Philipp Zabel @ 2020-08-27 9:02 UTC (permalink / raw)
To: Manish Narani, gregkh, robh+dt, michal.simek, balbi
Cc: linux-usb, devicetree, linux-arm-kernel, linux-kernel, git
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 <manish.narani@xilinx.com>
> ---
[...]
> +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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-26 18:44 ` [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms Manish Narani
` (3 preceding siblings ...)
2020-08-27 9:02 ` Philipp Zabel
@ 2020-08-27 18:46 ` Robin Murphy
2020-08-28 17:53 ` Manish Narani
4 siblings, 1 reply; 17+ messages in thread
From: Robin Murphy @ 2020-08-27 18:46 UTC (permalink / raw)
To: Manish Narani, gregkh, robh+dt, michal.simek, balbi, p.zabel
Cc: devicetree, linux-usb, linux-kernel, git, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-27 18:46 ` Robin Murphy
@ 2020-08-28 17:53 ` Manish Narani
2020-09-01 12:00 ` Robin Murphy
0 siblings, 1 reply; 17+ messages in thread
From: Manish Narani @ 2020-08-28 17:53 UTC (permalink / raw)
To: Robin Murphy, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, balbi@kernel.org, p.zabel@pengutronix.de
Cc: devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, git,
linux-arm-kernel@lists.infradead.org
Hi Robin,
Thanks for the review. Please find my comment below inline.
> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, August 28, 2020 12:17 AM
> To: Manish Narani <MNARANI@xilinx.com>; gregkh@linuxfoundation.org;
> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; balbi@kernel.org;
> p.zabel@pengutronix.de
> Cc: devicetree@vger.kernel.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; git <git@xilinx.com>; linux-arm-
> kernel@lists.infradead.org
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
2020-08-28 17:53 ` Manish Narani
@ 2020-09-01 12:00 ` Robin Murphy
0 siblings, 0 replies; 17+ messages in thread
From: Robin Murphy @ 2020-09-01 12:00 UTC (permalink / raw)
To: Manish Narani, gregkh@linuxfoundation.org, robh+dt@kernel.org,
Michal Simek, balbi@kernel.org, p.zabel@pengutronix.de
Cc: devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, git,
linux-arm-kernel@lists.infradead.org
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 <robin.murphy@arm.com>
>> Sent: Friday, August 28, 2020 12:17 AM
>> To: Manish Narani <MNARANI@xilinx.com>; gregkh@linuxfoundation.org;
>> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; balbi@kernel.org;
>> p.zabel@pengutronix.de
>> Cc: devicetree@vger.kernel.org; linux-usb@vger.kernel.org; linux-
>> kernel@vger.kernel.org; git <git@xilinx.com>; linux-arm-
>> kernel@lists.infradead.org
>> 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.
^ permalink raw reply [flat|nested] 17+ messages in thread