[PATCH v4 0/2] usb: cdns3: Add TI wrapper

[PATCH v4 0/2] usb: cdns3: Add TI wrapper

[Roger Quadros]

Hi,

Texas Instruments SoCs have a wrapper module around the Cadence
USB3 core. It takes care of clocking and powering the core and providing
initial configuration to the core.

This series adds the driver for the TI wrapper and associated DT binding
document. This is for -next kernel. Thanks.

cheers,
-roger

Changelog:
v4:
- use devm_platform_ioremap_resource()

v3:
- switch to yaml DT schema
- fix typo in driver header

v2:
- dt-binding fixes to address Rob's comments
	- convert dt-binding document name to DT schema
	- get rid of bootstrap properties
	- fix DT example

Roger Quadros (2):
  dt-bindings: usb: Add binding for the TI wrapper for Cadence USB3
    controller
  usb: cdns3: Add TI specific wrapper driver

 .../devicetree/bindings/usb/ti,j721e-usb.yaml |  86 +++++++
 drivers/usb/cdns3/Kconfig                     |  10 +
 drivers/usb/cdns3/Makefile                    |   1 +
 drivers/usb/cdns3/cdns3-ti.c                  | 236 ++++++++++++++++++
 4 files changed, 333 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
 create mode 100644 drivers/usb/cdns3/cdns3-ti.c

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v4 1/2] dt-bindings: usb: Add binding for the TI wrapper for Cadence USB3 controller

[Roger Quadros]

TI platforms have a wrapper module around the Cadence USB3
controller. Add binding information for that.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Cc: Rob Herring <robh@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/usb/ti,j721e-usb.yaml | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
new file mode 100644
index 000000000000..5f5264b2e9ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/ti,j721e-usb.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Bindings for the TI wrapper module for the Cadence USBSS-DRD controller
+
+maintainers:
+  - Roger Quadros <rogerq@ti.com>
+
+properties:
+  compatible:
+    items:
+      - const: ti,j721e-usb
+
+  reg:
+    description: module registers
+
+  power-domains:
+    description:
+       PM domain provider node and an args specifier containing
+       the USB device id value. See,
+       Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
+
+  clocks:
+    description: Clock phandles to usb2_refclk and lpm_clk
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: ref
+      - const: lpm
+
+  ti,usb2-only:
+    description:
+      If present, it restricts the controller to USB2.0 mode of
+      operation. Must be present if USB3 PHY is not available
+      for USB.
+    type: boolean
+
+  ti,vbus-divider:
+    description:
+      Should be present if USB VBUS line is connected to the
+      VBUS pin of the SoC via a 1/3 voltage divider.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    cdns_usb@4104000 {
+          compatible = "ti,j721e-usb";
+          reg = <0x00 0x4104000 0x00 0x100>;
+          power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
+          clocks = <&k3_clks 288 15>, <&k3_clks 288 3>;
+          clock-names = "ref", "lpm";
+          assigned-clocks = <&k3_clks 288 15>;	/* USB2_REFCLK */
+          assigned-clock-parents = <&k3_clks 288 16>; /* HFOSC0 */
+          #address-cells = <2>;
+          #size-cells = <2>;
+
+          usb@6000000 {
+                compatible = "cdns,usb3";
+                reg = <0x00 0x6000000 0x00 0x10000>,
+                      <0x00 0x6010000 0x00 0x10000>,
+                      <0x00 0x6020000 0x00 0x10000>;
+                reg-names = "otg", "xhci", "dev";
+                interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,	/* irq.0 */
+                             <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,	/* irq.6 */
+                             <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;	/* otgirq.0 */
+                interrupt-names = "host",
+                                  "peripheral",
+                                  "otg";
+                maximum-speed = "super-speed";
+                dr_mode = "otg";
+        };
+    };
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v4 2/2] usb: cdns3: Add TI specific wrapper driver

[Roger Quadros]

The J721e platform comes with 2 Cadence USB3 controller
instances. This driver supports the TI specific wrapper
on this platform.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Reviewed-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/Kconfig    |  10 ++
 drivers/usb/cdns3/Makefile   |   1 +
 drivers/usb/cdns3/cdns3-ti.c | 236 +++++++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 drivers/usb/cdns3/cdns3-ti.c

diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
index d0331613a355..2a1e89d12ed9 100644
--- a/drivers/usb/cdns3/Kconfig
+++ b/drivers/usb/cdns3/Kconfig
@@ -43,4 +43,14 @@ config USB_CDNS3_PCI_WRAP
 	  If you choose to build this driver as module it will
 	  be dynamically linked and module will be called cdns3-pci.ko
 
+config USB_CDNS3_TI
+	tristate "Cadence USB3 support on TI platforms"
+	depends on ARCH_K3 || COMPILE_TEST
+	default USB_CDNS3
+	help
+	  Say 'Y' or 'M' here if you are building for Texas Instruments
+	  platforms that contain Cadence USB3 controller core.
+
+	  e.g. J721e.
+
 endif
diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
index a703547350bb..948e6b88d1a9 100644
--- a/drivers/usb/cdns3/Makefile
+++ b/drivers/usb/cdns3/Makefile
@@ -14,3 +14,4 @@ endif
 cdns3-$(CONFIG_USB_CDNS3_HOST)		+= host.o
 
 obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci-wrap.o
+obj-$(CONFIG_USB_CDNS3_TI)		+= cdns3-ti.o
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
new file mode 100644
index 000000000000..c6a79ca15858
--- /dev/null
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * cdns3-ti.c - TI specific Glue layer for Cadence USB Controller
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+
+/* USB Wrapper register offsets */
+#define USBSS_PID		0x0
+#define	USBSS_W1		0x4
+#define USBSS_STATIC_CONFIG	0x8
+#define USBSS_PHY_TEST		0xc
+#define	USBSS_DEBUG_CTRL	0x10
+#define	USBSS_DEBUG_INFO	0x14
+#define	USBSS_DEBUG_LINK_STATE	0x18
+#define	USBSS_DEVICE_CTRL	0x1c
+
+/* Wrapper 1 register bits */
+#define USBSS_W1_PWRUP_RST		BIT(0)
+#define USBSS_W1_OVERCURRENT_SEL	BIT(8)
+#define USBSS_W1_MODESTRAP_SEL		BIT(9)
+#define USBSS_W1_OVERCURRENT		BIT(16)
+#define USBSS_W1_MODESTRAP_MASK		GENMASK(18, 17)
+#define USBSS_W1_MODESTRAP_SHIFT	17
+#define USBSS_W1_USB2_ONLY		BIT(19)
+
+/* Static config register bits */
+#define USBSS1_STATIC_PLL_REF_SEL_MASK	GENMASK(8, 5)
+#define USBSS1_STATIC_PLL_REF_SEL_SHIFT	5
+#define USBSS1_STATIC_LOOPBACK_MODE_MASK	GENMASK(4, 3)
+#define USBSS1_STATIC_LOOPBACK_MODE_SHIFT	3
+#define USBSS1_STATIC_VBUS_SEL_MASK	GENMASK(2, 1)
+#define USBSS1_STATIC_VBUS_SEL_SHIFT	1
+#define USBSS1_STATIC_LANE_REVERSE	BIT(0)
+
+/* Modestrap modes */
+enum modestrap_mode { USBSS_MODESTRAP_MODE_NONE,
+		      USBSS_MODESTRAP_MODE_HOST,
+		      USBSS_MODESTRAP_MODE_PERIPHERAL};
+
+struct cdns_ti {
+	struct device *dev;
+	void __iomem *usbss;
+	int usb2_only:1;
+	int vbus_divider:1;
+	struct clk *usb2_refclk;
+	struct clk *lpm_clk;
+};
+
+static const int cdns_ti_rate_table[] = {	/* in KHZ */
+	9600,
+	10000,
+	12000,
+	19200,
+	20000,
+	24000,
+	25000,
+	26000,
+	38400,
+	40000,
+	58000,
+	50000,
+	52000,
+};
+
+static inline u32 cdns_ti_readl(struct cdns_ti *data, u32 offset)
+{
+	return readl(data->usbss + offset);
+}
+
+static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
+{
+	writel(value, data->usbss + offset);
+}
+
+static int cdns_ti_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct cdns_ti *data;
+	int error;
+	u32 reg;
+	int rate_code, i;
+	unsigned long rate;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+
+	data->dev = dev;
+
+	data->usbss = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->usbss)) {
+		dev_err(dev, "can't map IOMEM resource\n");
+		return PTR_ERR(data->usbss);
+	}
+
+	data->usb2_refclk = devm_clk_get(dev, "ref");
+	if (IS_ERR(data->usb2_refclk)) {
+		dev_err(dev, "can't get usb2_refclk\n");
+		return PTR_ERR(data->usb2_refclk);
+	}
+
+	data->lpm_clk = devm_clk_get(dev, "lpm");
+	if (IS_ERR(data->lpm_clk)) {
+		dev_err(dev, "can't get lpm_clk\n");
+		return PTR_ERR(data->lpm_clk);
+	}
+
+	rate = clk_get_rate(data->usb2_refclk);
+	rate /= 1000;	/* To KHz */
+	for (i = 0; i < ARRAY_SIZE(cdns_ti_rate_table); i++) {
+		if (cdns_ti_rate_table[i] == rate)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(cdns_ti_rate_table)) {
+		dev_err(dev, "unsupported usb2_refclk rate: %lu KHz\n", rate);
+		return -EINVAL;
+	}
+
+	rate_code = i;
+
+	pm_runtime_enable(dev);
+	error = pm_runtime_get_sync(dev);
+	if (error < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed: %d\n", error);
+		goto err_get;
+	}
+
+	/* assert RESET */
+	reg = cdns_ti_readl(data, USBSS_W1);
+	reg &= ~USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* set static config */
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
+	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
+
+	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
+	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
+	if (data->vbus_divider)
+		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
+
+	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+
+	/* set USB2_ONLY mode if requested */
+	reg = cdns_ti_readl(data, USBSS_W1);
+	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+	if (data->usb2_only)
+		reg |= USBSS_W1_USB2_ONLY;
+
+	/* set default modestrap */
+	reg |= USBSS_W1_MODESTRAP_SEL;
+	reg &= ~USBSS_W1_MODESTRAP_MASK;
+	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* de-assert RESET */
+	reg |= USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	error = of_platform_populate(node, NULL, NULL, dev);
+	if (error) {
+		dev_err(dev, "failed to create children: %d\n", error);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	pm_runtime_put_sync(data->dev);
+err_get:
+	pm_runtime_disable(data->dev);
+
+	return error;
+}
+
+static int cdns_ti_remove_core(struct device *dev, void *c)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	platform_device_unregister(pdev);
+
+	return 0;
+}
+
+static int cdns_ti_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	device_for_each_child(dev, NULL, cdns_ti_remove_core);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id cdns_ti_of_match[] = {
+	{ .compatible = "ti,j721e-usb", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cdns_ti_of_match);
+
+static struct platform_driver cdns_ti_driver = {
+	.probe		= cdns_ti_probe,
+	.remove		= cdns_ti_remove,
+	.driver		= {
+		.name	= "cdns3-ti",
+		.of_match_table	= cdns_ti_of_match,
+	},
+};
+
+module_platform_driver(cdns_ti_driver);
+
+MODULE_ALIAS("platform:cdns3-ti");
+MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Cadence USB3 TI Glue Layer");
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v4 2/2] usb: cdns3: Add TI specific wrapper driver

[Greg KH]

On Mon, Oct 28, 2019 at 11:32:49AM +0200, Roger Quadros wrote:
> The J721e platform comes with 2 Cadence USB3 controller
> instances. This driver supports the TI specific wrapper
> on this platform.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/Kconfig    |  10 ++
>  drivers/usb/cdns3/Makefile   |   1 +
>  drivers/usb/cdns3/cdns3-ti.c | 236 +++++++++++++++++++++++++++++++++++
>  3 files changed, 247 insertions(+)
>  create mode 100644 drivers/usb/cdns3/cdns3-ti.c
> 
> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
> index d0331613a355..2a1e89d12ed9 100644
> --- a/drivers/usb/cdns3/Kconfig
> +++ b/drivers/usb/cdns3/Kconfig
> @@ -43,4 +43,14 @@ config USB_CDNS3_PCI_WRAP
>  	  If you choose to build this driver as module it will
>  	  be dynamically linked and module will be called cdns3-pci.ko
>  
> +config USB_CDNS3_TI
> +	tristate "Cadence USB3 support on TI platforms"
> +	depends on ARCH_K3 || COMPILE_TEST
> +	default USB_CDNS3
> +	help
> +	  Say 'Y' or 'M' here if you are building for Texas Instruments
> +	  platforms that contain Cadence USB3 controller core.
> +
> +	  e.g. J721e.
> +
>  endif
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index a703547350bb..948e6b88d1a9 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -14,3 +14,4 @@ endif
>  cdns3-$(CONFIG_USB_CDNS3_HOST)		+= host.o
>  
>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci-wrap.o
> +obj-$(CONFIG_USB_CDNS3_TI)		+= cdns3-ti.o
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> new file mode 100644
> index 000000000000..c6a79ca15858
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * cdns3-ti.c - TI specific Glue layer for Cadence USB Controller
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +
> +/* USB Wrapper register offsets */
> +#define USBSS_PID		0x0
> +#define	USBSS_W1		0x4
> +#define USBSS_STATIC_CONFIG	0x8
> +#define USBSS_PHY_TEST		0xc
> +#define	USBSS_DEBUG_CTRL	0x10
> +#define	USBSS_DEBUG_INFO	0x14
> +#define	USBSS_DEBUG_LINK_STATE	0x18
> +#define	USBSS_DEVICE_CTRL	0x1c
> +
> +/* Wrapper 1 register bits */
> +#define USBSS_W1_PWRUP_RST		BIT(0)
> +#define USBSS_W1_OVERCURRENT_SEL	BIT(8)
> +#define USBSS_W1_MODESTRAP_SEL		BIT(9)
> +#define USBSS_W1_OVERCURRENT		BIT(16)
> +#define USBSS_W1_MODESTRAP_MASK		GENMASK(18, 17)
> +#define USBSS_W1_MODESTRAP_SHIFT	17
> +#define USBSS_W1_USB2_ONLY		BIT(19)
> +
> +/* Static config register bits */
> +#define USBSS1_STATIC_PLL_REF_SEL_MASK	GENMASK(8, 5)
> +#define USBSS1_STATIC_PLL_REF_SEL_SHIFT	5
> +#define USBSS1_STATIC_LOOPBACK_MODE_MASK	GENMASK(4, 3)
> +#define USBSS1_STATIC_LOOPBACK_MODE_SHIFT	3
> +#define USBSS1_STATIC_VBUS_SEL_MASK	GENMASK(2, 1)
> +#define USBSS1_STATIC_VBUS_SEL_SHIFT	1
> +#define USBSS1_STATIC_LANE_REVERSE	BIT(0)
> +
> +/* Modestrap modes */
> +enum modestrap_mode { USBSS_MODESTRAP_MODE_NONE,
> +		      USBSS_MODESTRAP_MODE_HOST,
> +		      USBSS_MODESTRAP_MODE_PERIPHERAL};
> +
> +struct cdns_ti {
> +	struct device *dev;
> +	void __iomem *usbss;
> +	int usb2_only:1;
> +	int vbus_divider:1;

'bool' instead of bitfields?  Makes it more obvious, right?

> +	struct clk *usb2_refclk;
> +	struct clk *lpm_clk;
> +};
> +
> +static const int cdns_ti_rate_table[] = {	/* in KHZ */
> +	9600,
> +	10000,
> +	12000,
> +	19200,
> +	20000,
> +	24000,
> +	25000,
> +	26000,
> +	38400,
> +	40000,
> +	58000,
> +	50000,
> +	52000,
> +};
> +
> +static inline u32 cdns_ti_readl(struct cdns_ti *data, u32 offset)
> +{
> +	return readl(data->usbss + offset);
> +}

Does sparse like this function?

> +
> +static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
> +{
> +	writel(value, data->usbss + offset);

Same here, have you run sparse on this code?

> +}
> +
> +static int cdns_ti_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct cdns_ti *data;
> +	int error;
> +	u32 reg;
> +	int rate_code, i;
> +	unsigned long rate;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->dev = dev;
> +
> +	data->usbss = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(data->usbss)) {
> +		dev_err(dev, "can't map IOMEM resource\n");

Doesn't the function print an error?

> +		return PTR_ERR(data->usbss);
> +	}
> +
> +	data->usb2_refclk = devm_clk_get(dev, "ref");
> +	if (IS_ERR(data->usb2_refclk)) {
> +		dev_err(dev, "can't get usb2_refclk\n");

Again, doesn't the function print an error?

> +		return PTR_ERR(data->usb2_refclk);
> +	}
> +
> +	data->lpm_clk = devm_clk_get(dev, "lpm");
> +	if (IS_ERR(data->lpm_clk)) {
> +		dev_err(dev, "can't get lpm_clk\n");

Same?

> +		return PTR_ERR(data->lpm_clk);
> +	}
> +
> +	rate = clk_get_rate(data->usb2_refclk);
> +	rate /= 1000;	/* To KHz */
> +	for (i = 0; i < ARRAY_SIZE(cdns_ti_rate_table); i++) {
> +		if (cdns_ti_rate_table[i] == rate)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(cdns_ti_rate_table)) {
> +		dev_err(dev, "unsupported usb2_refclk rate: %lu KHz\n", rate);

What can userspace do about this?

> +		return -EINVAL;
> +	}
> +
> +	rate_code = i;
> +
> +	pm_runtime_enable(dev);
> +	error = pm_runtime_get_sync(dev);
> +	if (error < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", error);

Again, the call should print the error, right?

> +		goto err_get;
> +	}
> +
> +	/* assert RESET */
> +	reg = cdns_ti_readl(data, USBSS_W1);
> +	reg &= ~USBSS_W1_PWRUP_RST;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +
> +	/* set static config */
> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> +	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
> +	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
> +
> +	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
> +	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> +	if (data->vbus_divider)
> +		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
> +
> +	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> +
> +	/* set USB2_ONLY mode if requested */
> +	reg = cdns_ti_readl(data, USBSS_W1);
> +	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> +	if (data->usb2_only)
> +		reg |= USBSS_W1_USB2_ONLY;
> +
> +	/* set default modestrap */
> +	reg |= USBSS_W1_MODESTRAP_SEL;
> +	reg &= ~USBSS_W1_MODESTRAP_MASK;
> +	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +
> +	/* de-assert RESET */
> +	reg |= USBSS_W1_PWRUP_RST;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +
> +	error = of_platform_populate(node, NULL, NULL, dev);
> +	if (error) {
> +		dev_err(dev, "failed to create children: %d\n", error);

Again, error in the caller?

thanks,

greg k-h

Re: [PATCH v4 2/2] usb: cdns3: Add TI specific wrapper driver

[Roger Quadros]

On 04/11/2019 16:47, Greg KH wrote:
> On Mon, Oct 28, 2019 at 11:32:49AM +0200, Roger Quadros wrote:
>> The J721e platform comes with 2 Cadence USB3 controller
>> instances. This driver supports the TI specific wrapper
>> on this platform.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>   drivers/usb/cdns3/Kconfig    |  10 ++
>>   drivers/usb/cdns3/Makefile   |   1 +
>>   drivers/usb/cdns3/cdns3-ti.c | 236 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 247 insertions(+)
>>   create mode 100644 drivers/usb/cdns3/cdns3-ti.c
>>
>> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
>> index d0331613a355..2a1e89d12ed9 100644
>> --- a/drivers/usb/cdns3/Kconfig
>> +++ b/drivers/usb/cdns3/Kconfig
>> @@ -43,4 +43,14 @@ config USB_CDNS3_PCI_WRAP
>>   	  If you choose to build this driver as module it will
>>   	  be dynamically linked and module will be called cdns3-pci.ko
>>   
>> +config USB_CDNS3_TI
>> +	tristate "Cadence USB3 support on TI platforms"
>> +	depends on ARCH_K3 || COMPILE_TEST
>> +	default USB_CDNS3
>> +	help
>> +	  Say 'Y' or 'M' here if you are building for Texas Instruments
>> +	  platforms that contain Cadence USB3 controller core.
>> +
>> +	  e.g. J721e.
>> +
>>   endif
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index a703547350bb..948e6b88d1a9 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -14,3 +14,4 @@ endif
>>   cdns3-$(CONFIG_USB_CDNS3_HOST)		+= host.o
>>   
>>   obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci-wrap.o
>> +obj-$(CONFIG_USB_CDNS3_TI)		+= cdns3-ti.o
>> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
>> new file mode 100644
>> index 000000000000..c6a79ca15858
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/cdns3-ti.c
>> @@ -0,0 +1,236 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * cdns3-ti.c - TI specific Glue layer for Cadence USB Controller
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/io.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +/* USB Wrapper register offsets */
>> +#define USBSS_PID		0x0
>> +#define	USBSS_W1		0x4
>> +#define USBSS_STATIC_CONFIG	0x8
>> +#define USBSS_PHY_TEST		0xc
>> +#define	USBSS_DEBUG_CTRL	0x10
>> +#define	USBSS_DEBUG_INFO	0x14
>> +#define	USBSS_DEBUG_LINK_STATE	0x18
>> +#define	USBSS_DEVICE_CTRL	0x1c
>> +
>> +/* Wrapper 1 register bits */
>> +#define USBSS_W1_PWRUP_RST		BIT(0)
>> +#define USBSS_W1_OVERCURRENT_SEL	BIT(8)
>> +#define USBSS_W1_MODESTRAP_SEL		BIT(9)
>> +#define USBSS_W1_OVERCURRENT		BIT(16)
>> +#define USBSS_W1_MODESTRAP_MASK		GENMASK(18, 17)
>> +#define USBSS_W1_MODESTRAP_SHIFT	17
>> +#define USBSS_W1_USB2_ONLY		BIT(19)
>> +
>> +/* Static config register bits */
>> +#define USBSS1_STATIC_PLL_REF_SEL_MASK	GENMASK(8, 5)
>> +#define USBSS1_STATIC_PLL_REF_SEL_SHIFT	5
>> +#define USBSS1_STATIC_LOOPBACK_MODE_MASK	GENMASK(4, 3)
>> +#define USBSS1_STATIC_LOOPBACK_MODE_SHIFT	3
>> +#define USBSS1_STATIC_VBUS_SEL_MASK	GENMASK(2, 1)
>> +#define USBSS1_STATIC_VBUS_SEL_SHIFT	1
>> +#define USBSS1_STATIC_LANE_REVERSE	BIT(0)
>> +
>> +/* Modestrap modes */
>> +enum modestrap_mode { USBSS_MODESTRAP_MODE_NONE,
>> +		      USBSS_MODESTRAP_MODE_HOST,
>> +		      USBSS_MODESTRAP_MODE_PERIPHERAL};
>> +
>> +struct cdns_ti {
>> +	struct device *dev;
>> +	void __iomem *usbss;
>> +	int usb2_only:1;
>> +	int vbus_divider:1;
> 
> 'bool' instead of bitfields?  Makes it more obvious, right?
> 

right.

>> +	struct clk *usb2_refclk;
>> +	struct clk *lpm_clk;
>> +};
>> +
>> +static const int cdns_ti_rate_table[] = {	/* in KHZ */
>> +	9600,
>> +	10000,
>> +	12000,
>> +	19200,
>> +	20000,
>> +	24000,
>> +	25000,
>> +	26000,
>> +	38400,
>> +	40000,
>> +	58000,
>> +	50000,
>> +	52000,
>> +};
>> +
>> +static inline u32 cdns_ti_readl(struct cdns_ti *data, u32 offset)
>> +{
>> +	return readl(data->usbss + offset);
>> +}
> 
> Does sparse like this function?
> 

It doesn't complain.

>> +
>> +static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
>> +{
>> +	writel(value, data->usbss + offset);
> 
> Same here, have you run sparse on this code?

I hadn't but now that I run i only see these errors.

   CHECK   drivers/usb/cdns3/cdns3-ti.c
drivers/usb/cdns3/cdns3-ti.c:55:24: error: dubious one-bit signed bitfield
drivers/usb/cdns3/cdns3-ti.c:56:27: error: dubious one-bit signed bitfield

> 
>> +}
>> +
>> +static int cdns_ti_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct cdns_ti *data;
>> +	int error;
>> +	u32 reg;
>> +	int rate_code, i;
>> +	unsigned long rate;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	data->dev = dev;
>> +
>> +	data->usbss = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(data->usbss)) {
>> +		dev_err(dev, "can't map IOMEM resource\n");
> 
> Doesn't the function print an error?

It does.
> 
>> +		return PTR_ERR(data->usbss);
>> +	}
>> +
>> +	data->usb2_refclk = devm_clk_get(dev, "ref");
>> +	if (IS_ERR(data->usb2_refclk)) {
>> +		dev_err(dev, "can't get usb2_refclk\n");
> 
> Again, doesn't the function print an error?

No.

> 
>> +		return PTR_ERR(data->usb2_refclk);
>> +	}
>> +
>> +	data->lpm_clk = devm_clk_get(dev, "lpm");
>> +	if (IS_ERR(data->lpm_clk)) {
>> +		dev_err(dev, "can't get lpm_clk\n");
> 
> Same?
> 
>> +		return PTR_ERR(data->lpm_clk);
>> +	}
>> +
>> +	rate = clk_get_rate(data->usb2_refclk);
>> +	rate /= 1000;	/* To KHz */
>> +	for (i = 0; i < ARRAY_SIZE(cdns_ti_rate_table); i++) {
>> +		if (cdns_ti_rate_table[i] == rate)
>> +			break;
>> +	}
>> +
>> +	if (i == ARRAY_SIZE(cdns_ti_rate_table)) {
>> +		dev_err(dev, "unsupported usb2_refclk rate: %lu KHz\n", rate);
> 
> What can userspace do about this?

Nothing, but it would help us identify the issue on customer boards.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	rate_code = i;
>> +
>> +	pm_runtime_enable(dev);
>> +	error = pm_runtime_get_sync(dev);
>> +	if (error < 0) {
>> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", error);
> 
> Again, the call should print the error, right?

It doesn't.
> 
>> +		goto err_get;
>> +	}
>> +
>> +	/* assert RESET */
>> +	reg = cdns_ti_readl(data, USBSS_W1);
>> +	reg &= ~USBSS_W1_PWRUP_RST;
>> +	cdns_ti_writel(data, USBSS_W1, reg);
>> +
>> +	/* set static config */
>> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
>> +	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
>> +	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
>> +
>> +	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
>> +	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
>> +	if (data->vbus_divider)
>> +		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
>> +
>> +	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
>> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
>> +
>> +	/* set USB2_ONLY mode if requested */
>> +	reg = cdns_ti_readl(data, USBSS_W1);
>> +	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
>> +	if (data->usb2_only)
>> +		reg |= USBSS_W1_USB2_ONLY;
>> +
>> +	/* set default modestrap */
>> +	reg |= USBSS_W1_MODESTRAP_SEL;
>> +	reg &= ~USBSS_W1_MODESTRAP_MASK;
>> +	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
>> +	cdns_ti_writel(data, USBSS_W1, reg);
>> +
>> +	/* de-assert RESET */
>> +	reg |= USBSS_W1_PWRUP_RST;
>> +	cdns_ti_writel(data, USBSS_W1, reg);
>> +
>> +	error = of_platform_populate(node, NULL, NULL, dev);
>> +	if (error) {
>> +		dev_err(dev, "failed to create children: %d\n", error);
> 
> Again, error in the caller?

doesn't seem like so.


Felipe,

Do you want me to send a diff with the changes or a revised patch?

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki