[PATCH 0/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[PATCH 0/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Sriram Dash]

Adds qoriq usb 3.0 phy driver support for LS1043A platform.
Describes the qoriq usb 2.0 phy driver binding, currently used
for LS1043A platform.
Adds entries in dts to enable USB 3.0 phy driver.

Sriram Dash (2):
  drivers: usb: phy: Add qoriq usb 3.0 phy driver support
  arm64: dts: ls1043a: Enable USB 3.0 phy driver

 .../devicetree/bindings/phy/phy-qoriq-usb3.txt     |  38 ++++
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi     |  33 ++++
 drivers/phy/Kconfig                                |   8 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-qoriq-usb3.c                       | 202 +++++++++++++++++++++
 5 files changed, 282 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
 create mode 100644 drivers/phy/phy-qoriq-usb3.c

-- 
2.1.0

[PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Sriram Dash]

Adds qoriq usb 3.0 phy driver support for LS1043A platform.
Describes the qoriq usb 2.0 phy driver binding, currently used
for LS1043A platform.

Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
---
 .../devicetree/bindings/phy/phy-qoriq-usb3.txt     |  36 ++++
 drivers/phy/Kconfig                                |   8 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-qoriq-usb3.c                       | 202 +++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
 create mode 100644 drivers/phy/phy-qoriq-usb3.c

diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
new file mode 100644
index 0000000..d934c80
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
@@ -0,0 +1,36 @@
+Driver for Freescale USB 3.0 PHY
+
+Required properties:
+
+- compatible :	fsl,qoriq-usb3-phy
+- reg :		register mappings for Parameter Configuration Register
+		and Phy base offset.
+- reg-names :	"param_ctrl" and "phy_base"
+- phy_type :	For multi port host USB controllers, should be one of
+		"ulpi", or "serial". For dual role USB controllers,
+		should be one of "ulpi", "utmi", "utmi_wide", or "serial".
+
+Example:
+		usbphy0: usb3-phy@084F0000 {
+                        compatible = "fsl,qoriq-usb3-phy";
+                        reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F0000 0x0 0x5000>;
+                        reg-names = "param_ctrl", "phy_base";
+                        #phy-cells = <0>;
+                        phy_type = "utmi";
+                };
+
+                usbphy1: usb3-phy@08500000 {
+                        compatible = "fsl,qoriq-usb3-phy";
+                        reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x08500000 0x0 0x5000>;
+                        reg-names = "param_ctrl", "phy_base";
+                        #phy-cells = <0>;
+                        phy_type = "utmi";
+                };
+
+                usbphy2: usb3-phy@08510000 {
+                        compatible = "fsl,qoriq-usb3-phy";
+                        reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x08510000 0x0 0x5000>;
+                        reg-names = "param_ctrl", "phy_base";
+                        #phy-cells = <0>;
+                        phy_type = "utmi";
+                };
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index fe00f91..4caa91c 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -489,4 +489,12 @@ config PHY_NS2_PCIE
 	help
 	  Enable this to support the Broadcom Northstar2 PCIe PHY.
 	  If unsure, say N.
+
+config PHY_QORIQ_USB3
+	tristate "Freescale QorIQ USB 3.0 PHY driver"
+	depends on ARCH_LAYERSCAPE
+	depends on OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the USB3.0 PHY on the QorIQ SoC.
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index a534cf5..a47ee36b 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)		+= phy-bcm-cygnus-pcie.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE)		+= phy-bcm-ns2-pcie.o
+obj-$(CONFIG_PHY_QORIQ_USB3)            += phy-qoriq-usb3.o
diff --git a/drivers/phy/phy-qoriq-usb3.c b/drivers/phy/phy-qoriq-usb3.c
new file mode 100644
index 0000000..5255089
--- /dev/null
+++ b/drivers/phy/phy-qoriq-usb3.c
@@ -0,0 +1,202 @@
+/*
+ * Freescale QorIQ USB3 phy driver
+ *
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Author: Sriram Dash <sriram.dash@nxp.com>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/phy/phy.h>
+#include <linux/usb/phy.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/usb/of.h>
+#include <linux/regmap.h>
+
+
+/* Parameter control */
+#define USB3PRM1CR		0x000
+#define USB3PRM1CR_VAL		0x27672b2a
+
+/*
+ * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
+ * @dev: pointer to device instance of this platform device
+ * @param_ctrl: usb3 phy parameter control register base
+ * @phy_base: usb3 phy register memory base
+ * @has_erratum_flag: keeps track of erratum applicable on device
+ */
+struct qoriq_usb3_phy {
+	struct device *dev;
+	void __iomem *param_ctrl;
+	void __iomem *phy_base;
+	u32 has_erratum_flag;
+};
+
+static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32 offset)
+{
+	return __raw_readl(addr + offset);
+}
+
+static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
+	u32 data)
+{
+	__raw_writel(data, addr + offset);
+}
+
+/*
+ * Erratum A008751
+ * SCFG USB3PRM1CR has incorrect default value
+ * SCFG USB3PRM1CR reset value should be 32'h27672B2A instead of 32'h25E72B2A.
+ */
+static void erratum_a008751(struct qoriq_usb3_phy *phy)
+{
+	qoriq_usb3_phy_writel(phy->param_ctrl, USB3PRM1CR,
+				USB3PRM1CR_VAL);
+}
+
+/*
+ * qoriq_usb3_phy_erratum - List of phy erratum
+ * @qoriq_phy_erratum - erratum application
+ * @compat - comapt string for erratum
+ */
+
+struct qoriq_usb3_phy_erratum {
+	void (*qoriq_phy_erratum)(struct qoriq_usb3_phy *phy);
+	char *compat;
+};
+
+/* Erratum list */
+struct qoriq_usb3_phy_erratum  phy_erratum_tbl[] = {
+	{&erratum_a008751, "fsl,usb-erratum-a008751"},
+	/* Add init time erratum here */
+};
+
+static int qoriq_usb3_phy_init(struct phy *x)
+{
+	struct qoriq_usb3_phy *phy = phy_get_drvdata(x);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
+		if (phy->has_erratum_flag & 1 << i)
+			phy_erratum_tbl[i].qoriq_phy_erratum(phy);
+	return 0;
+}
+
+static const struct phy_ops ops = {
+	.init		= qoriq_usb3_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static int qoriq_usb3_phy_probe(struct platform_device *pdev)
+{
+	struct qoriq_usb3_phy *phy;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	const struct of_device_id *of_id;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int i, ret;
+
+	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+	phy->dev = dev;
+
+	of_id = of_match_device(dev->driver->of_match_table, dev);
+	if (!of_id) {
+		dev_err(dev, "failed to get device match\n");
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "param_ctrl");
+	if (!res) {
+		dev_err(dev, "failed to get param_ctrl memory\n");
+		ret = -ENOENT;
+		goto err_out;
+	}
+
+	phy->param_ctrl = devm_ioremap_resource(dev, res);
+	if (!phy->param_ctrl) {
+		dev_err(dev, "failed to remap param_ctrl memory\n");
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_base");
+	if (!res) {
+		dev_err(dev, "failed to get phy_base memory\n");
+		ret = -ENOENT;
+		goto err_out;
+	}
+
+	phy->phy_base = devm_ioremap_resource(dev, res);
+	if (!phy->phy_base) {
+		dev_err(dev, "failed to remap phy_base memory\n");
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	phy->has_erratum_flag = 0;
+	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
+		phy->has_erratum_flag |= device_property_read_bool(dev,
+						phy_erratum_tbl[i].compat) << i;
+
+	platform_set_drvdata(pdev, phy);
+
+	generic_phy = devm_phy_create(dev, NULL, &ops);
+	if (IS_ERR(generic_phy))
+		return PTR_ERR(generic_phy);
+
+	phy_set_drvdata(generic_phy, phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+err_out:
+	return ret;
+}
+
+static const struct of_device_id qoriq_usb3_phy_dt_ids[] = {
+	{
+		.compatible = "fsl,qoriq-usb3-phy"
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, qoriq_usb3_phy_dt_ids);
+
+static struct platform_driver qoriq_usb3_phy_driver = {
+	.probe		= qoriq_usb3_phy_probe,
+	.driver		= {
+		.name	= "qoriq_usb3_phy",
+		.of_match_table = qoriq_usb3_phy_dt_ids,
+	},
+};
+
+module_platform_driver(qoriq_usb3_phy_driver);
+
+MODULE_ALIAS("platform:qoriq_usb3_phy");
+MODULE_AUTHOR("Sriram Dash <sriram.dash@nxp.com>");
+MODULE_DESCRIPTION("Freescale QorIQ USB3 phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[PATCH 2/2] arm64: dts: ls1043a: Enable USB 3.0 phy driver

[Sriram Dash]

This patch adds entries in dts to enable USB 3.0 PHY driver.

Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 33 ++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 97d331e..c87fc16 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -475,11 +475,40 @@
 				 <&clockgen 4 0>;
 		};
 
+		usbphy0: usb3-phy@084f0000 {
+			compatible = "fsl,qoriq-usb3-phy";
+			reg = <0x0 0x01570070 0x0 0xC>,
+			      <0x0 0x084F0000 0x0 0x5000>;
+			reg-names = "param_ctrl", "phy_base";
+			#phy-cells = <0>;
+			phy_type = "utmi";
+		};
+
+		usbphy1: usb3-phy@08500000 {
+			compatible = "fsl,qoriq-usb3-phy";
+			reg = <0x0 0x0157007C 0x0 0xC>,
+			      <0x0 0x08500000 0x0 0x5000>;
+			reg-names = "param_ctrl", "phy_base";
+			#phy-cells = <0>;
+			phy_type = "utmi";
+		};
+
+		usbphy2: usb3-phy@08510000 {
+			compatible = "fsl,qoriq-usb3-phy";
+			reg = <0x0 0x01570088 0x0 0xC>,
+			      <0x0 0x08510000 0x0 0x5000>;
+			reg-names = "param_ctrl", "phy_base";
+			#phy-cells = <0>;
+			phy_type = "utmi";
+		};
+
 		usb0: usb3@2f00000 {
 			compatible = "snps,dwc3";
 			reg = <0x0 0x2f00000 0x0 0x10000>;
 			interrupts = <0 60 0x4>;
 			dr_mode = "host";
+			phys = <&usbphy0>;
+			phy-names = "usb3-phy";
 			snps,quirk-frame-length-adjustment = <0x20>;
 			snps,dis_rxdet_inp3_quirk;
 		};
@@ -489,6 +518,8 @@
 			reg = <0x0 0x3000000 0x0 0x10000>;
 			interrupts = <0 61 0x4>;
 			dr_mode = "host";
+			phys = <&usbphy1>;
+			phy-names = "usb3-phy";
 			snps,quirk-frame-length-adjustment = <0x20>;
 			snps,dis_rxdet_inp3_quirk;
 		};
@@ -498,6 +529,8 @@
 			reg = <0x0 0x3100000 0x0 0x10000>;
 			interrupts = <0 63 0x4>;
 			dr_mode = "host";
+			phys = <&usbphy2>;
+			phy-names = "usb3-phy";
 			snps,quirk-frame-length-adjustment = <0x20>;
 			snps,dis_rxdet_inp3_quirk;
 		};
-- 
2.1.0

Re: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Scott Wood]

On 11/13/2016 11:27 PM, Sriram Dash wrote:
> diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
> new file mode 100644
> index 0000000..d934c80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
> @@ -0,0 +1,36 @@
> +Driver for Freescale USB 3.0 PHY
> +
> +Required properties:
> +
> +- compatible :	fsl,qoriq-usb3-phy

This is a very vague compatible.  Are there versioning registers within
this register block?

> +/* Parameter control */
> +#define USB3PRM1CR		0x000
> +#define USB3PRM1CR_VAL		0x27672b2a
> +
> +/*
> + * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
> + * @dev: pointer to device instance of this platform device
> + * @param_ctrl: usb3 phy parameter control register base
> + * @phy_base: usb3 phy register memory base
> + * @has_erratum_flag: keeps track of erratum applicable on device
> + */
> +struct qoriq_usb3_phy {
> +	struct device *dev;
> +	void __iomem *param_ctrl;
> +	void __iomem *phy_base;
> +	u32 has_erratum_flag;
> +};
> +
> +static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32 offset)
> +{
> +	return __raw_readl(addr + offset);
> +}
> +
> +static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
> +	u32 data)
> +{
> +	__raw_writel(data, addr + offset);
> +}

Why raw?  Besides missing barriers, this will cause the accesses to be
native-endian which is not correct.

> +/*
> + * Erratum A008751
> + * SCFG USB3PRM1CR has incorrect default value
> + * SCFG USB3PRM1CR reset value should be 32'h27672B2A instead of 32'h25E72B2A.

When documenting C code, can you stick with C-style numeric constants?

For that matter, just put the constant in the code instead of hiding it
in an overly-generically-named USB3PRM1CR_VAL and then you won't need to
redundantly state the value in a comment.  Normally putting magic
numbers in symbolic constants is a good thing, but in this case it's not
actually describing anything and the number is of no meaning outside of
this one erratum workaround (it might even be a different value if
another chip has a similar erratum).

> + */
> +static void erratum_a008751(struct qoriq_usb3_phy *phy)
> +{
> +	qoriq_usb3_phy_writel(phy->param_ctrl, USB3PRM1CR,
> +				USB3PRM1CR_VAL);
> +}
> +
> +/*
> + * qoriq_usb3_phy_erratum - List of phy erratum
> + * @qoriq_phy_erratum - erratum application
> + * @compat - comapt string for erratum
> + */
> +
> +struct qoriq_usb3_phy_erratum {
> +	void (*qoriq_phy_erratum)(struct qoriq_usb3_phy *phy);
> +	char *compat;
> +};
> +
> +/* Erratum list */
> +struct qoriq_usb3_phy_erratum  phy_erratum_tbl[] = {
> +	{&erratum_a008751, "fsl,usb-erratum-a008751"},
> +	/* Add init time erratum here */
> +};

This needs to be static.

Unnecessary & on the function pointer.

> +static int qoriq_usb3_phy_init(struct phy *x)
> +{
> +	struct qoriq_usb3_phy *phy = phy_get_drvdata(x);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
> +		if (phy->has_erratum_flag & 1 << i)
> +			phy_erratum_tbl[i].qoriq_phy_erratum(phy);
> +	return 0;
> +}
> +
> +static const struct phy_ops ops = {
> +	.init		= qoriq_usb3_phy_init,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int qoriq_usb3_phy_probe(struct platform_device *pdev)
> +{
> +	struct qoriq_usb3_phy *phy;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	const struct of_device_id *of_id;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int i, ret;
> +
> +	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +	phy->dev = dev;
> +
> +	of_id = of_match_device(dev->driver->of_match_table, dev);
> +	if (!of_id) {
> +		dev_err(dev, "failed to get device match\n");
> +		ret = -EINVAL;
> +		goto err_out;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "param_ctrl");
> +	if (!res) {
> +		dev_err(dev, "failed to get param_ctrl memory\n");
> +		ret = -ENOENT;
> +		goto err_out;
> +	}
> +
> +	phy->param_ctrl = devm_ioremap_resource(dev, res);
> +	if (!phy->param_ctrl) {
> +		dev_err(dev, "failed to remap param_ctrl memory\n");
> +		ret = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_base");
> +	if (!res) {
> +		dev_err(dev, "failed to get phy_base memory\n");
> +		ret = -ENOENT;
> +		goto err_out;
> +	}
> +
> +	phy->phy_base = devm_ioremap_resource(dev, res);
> +	if (!phy->phy_base) {
> +		dev_err(dev, "failed to remap phy_base memory\n");
> +		ret = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	phy->has_erratum_flag = 0;
> +	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
> +		phy->has_erratum_flag |= device_property_read_bool(dev,
> +						phy_erratum_tbl[i].compat) << i;

I don't see the erratum property in either the binding or the device
tree.  Also, a property name is not a "compat".

Is there a reason why this flag and array mechanism is needed, rather
than just checking the erratum properties from the init function -- or,
if you have a good reason to not want to do device tree accesses from
init, just using a bool per erratum?  How many errata are you expecting?

-Scott

RE: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Sriram Dash]

>From: Scott Wood
>On 11/13/2016 11:27 PM, Sriram Dash wrote:
>> diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> new file mode 100644
>> index 0000000..d934c80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> @@ -0,0 +1,36 @@
>> +Driver for Freescale USB 3.0 PHY
>> +
>> +Required properties:
>> +
>> +- compatible :	fsl,qoriq-usb3-phy
>

Hi Scott,

>This is a very vague compatible.  Are there versioning registers within this register
>block?
>

There are versioning registers for the phy (1.0 and 1.1). But the current erratum
A008751 does not require the mentioning of the version numbers. Was planning
to take care of the versioning when there is code diversity on the basis of the
version number.
Shall I include the versioning in the documentation now for 1.0 and 1.1?

>> +/* Parameter control */
>> +#define USB3PRM1CR		0x000
>> +#define USB3PRM1CR_VAL		0x27672b2a
>> +
>> +/*
>> + * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
>> + * @dev: pointer to device instance of this platform device
>> + * @param_ctrl: usb3 phy parameter control register base
>> + * @phy_base: usb3 phy register memory base
>> + * @has_erratum_flag: keeps track of erratum applicable on device  */
>> +struct qoriq_usb3_phy {
>> +	struct device *dev;
>> +	void __iomem *param_ctrl;
>> +	void __iomem *phy_base;
>> +	u32 has_erratum_flag;
>> +};
>> +
>> +static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32
>> +offset) {
>> +	return __raw_readl(addr + offset);
>> +}
>> +
>> +static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
>> +	u32 data)
>> +{
>> +	__raw_writel(data, addr + offset);
>> +}
>
>Why raw?  Besides missing barriers, this will cause the accesses to be native-endian
>which is not correct.
>

The only reason for __raw_writel is to make the code faster. However, shall I use
writel(with both barriers and byte swap) instead and then make appropriate changes
in the value 32'h27672B2A?

>> +/*
>> + * Erratum A008751
>> + * SCFG USB3PRM1CR has incorrect default value
>> + * SCFG USB3PRM1CR reset value should be 32'h27672B2A instead of
>32'h25E72B2A.
>
>When documenting C code, can you stick with C-style numeric constants?
>
>For that matter, just put the constant in the code instead of hiding it in an overly-
>generically-named USB3PRM1CR_VAL and then you won't need to redundantly
>state the value in a comment.  Normally putting magic numbers in symbolic
>constants is a good thing, but in this case it's not actually describing anything and
>the number is of no meaning outside of this one erratum workaround (it might even
>be a different value if another chip has a similar erratum).
>

The POR value of the USB3PRM1CR  should be 32'h27672B2A. So, I had named it as
such. Thanks for the info. Will remove the USB3PRM1CR_VAL and during writel,
will use 0x27672b2a directly. I will take care the next time.

>> + */
>> +static void erratum_a008751(struct qoriq_usb3_phy *phy) {
>> +	qoriq_usb3_phy_writel(phy->param_ctrl, USB3PRM1CR,
>> +				USB3PRM1CR_VAL);
>> +}
>> +
>> +/*
>> + * qoriq_usb3_phy_erratum - List of phy erratum
>> + * @qoriq_phy_erratum - erratum application
>> + * @compat - comapt string for erratum  */
>> +
>> +struct qoriq_usb3_phy_erratum {
>> +	void (*qoriq_phy_erratum)(struct qoriq_usb3_phy *phy);
>> +	char *compat;
>> +};
>> +
>> +/* Erratum list */
>> +struct qoriq_usb3_phy_erratum  phy_erratum_tbl[] = {
>> +	{&erratum_a008751, "fsl,usb-erratum-a008751"},
>> +	/* Add init time erratum here */
>> +};
>
>This needs to be static.
>

Ok. Will take care next time.

>Unnecessary & on the function pointer.
>

Ok. Will change in next version.

>> +static int qoriq_usb3_phy_init(struct phy *x) {
>> +	struct qoriq_usb3_phy *phy = phy_get_drvdata(x);
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
>> +		if (phy->has_erratum_flag & 1 << i)
>> +			phy_erratum_tbl[i].qoriq_phy_erratum(phy);
>> +	return 0;
>> +}
>> +
>> +static const struct phy_ops ops = {
>> +	.init		= qoriq_usb3_phy_init,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static int qoriq_usb3_phy_probe(struct platform_device *pdev) {
>> +	struct qoriq_usb3_phy *phy;
>> +	struct phy *generic_phy;
>> +	struct phy_provider *phy_provider;
>> +	const struct of_device_id *of_id;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	int i, ret;
>> +
>> +	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
>> +	if (!phy)
>> +		return -ENOMEM;
>> +	phy->dev = dev;
>> +
>> +	of_id = of_match_device(dev->driver->of_match_table, dev);
>> +	if (!of_id) {
>> +		dev_err(dev, "failed to get device match\n");
>> +		ret = -EINVAL;
>> +		goto err_out;
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>"param_ctrl");
>> +	if (!res) {
>> +		dev_err(dev, "failed to get param_ctrl memory\n");
>> +		ret = -ENOENT;
>> +		goto err_out;
>> +	}
>> +
>> +	phy->param_ctrl = devm_ioremap_resource(dev, res);
>> +	if (!phy->param_ctrl) {
>> +		dev_err(dev, "failed to remap param_ctrl memory\n");
>> +		ret = -ENOMEM;
>> +		goto err_out;
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>"phy_base");
>> +	if (!res) {
>> +		dev_err(dev, "failed to get phy_base memory\n");
>> +		ret = -ENOENT;
>> +		goto err_out;
>> +	}
>> +
>> +	phy->phy_base = devm_ioremap_resource(dev, res);
>> +	if (!phy->phy_base) {
>> +		dev_err(dev, "failed to remap phy_base memory\n");
>> +		ret = -ENOMEM;
>> +		goto err_out;
>> +	}
>> +
>> +	phy->has_erratum_flag = 0;
>> +	for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
>> +		phy->has_erratum_flag |= device_property_read_bool(dev,
>> +						phy_erratum_tbl[i].compat) << i;
>
>I don't see the erratum property in either the binding or the device tree.  Also, a
>property name is not a "compat".
>

Ok. Will rename "char *compat" into "char *prop".

>Is there a reason why this flag and array mechanism is needed, rather than just
>checking the erratum properties from the init function -- or, if you have a good
>reason to not want to do device tree accesses from init, just using a bool per
>erratum?  How many errata are you expecting?
>

The erratum prop will be specified via the dt and parsed in the init. The idea
behind the table is that it will accommodate more errata and every time we
are adding a new erratum, and the code changes would be minimal(with this
approach, we don't have to add new variable in qoriq_usb3_phy struct
corresponding to every new erratum being added).

In my knowledge, there are more than 5 errata in pipeline, and all of them
have to be applied in the init time. So, this table will make the code more
readable and less cumbersome. 

However, in future, if any other erratum comes up, and it has to be applied
at any point other than during init, then the variable has to be added in
qoriq_usb3_phy struct and the property has to be read separately.

>-Scott

Re: [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Rob Herring]

On Mon, Nov 14, 2016 at 10:56:54AM +0530, Sriram Dash wrote:
> Adds qoriq usb 3.0 phy driver support for LS1043A platform.
> Describes the qoriq usb 2.0 phy driver binding, currently used
> for LS1043A platform.
> 
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> ---
>  .../devicetree/bindings/phy/phy-qoriq-usb3.txt     |  36 ++++
>  drivers/phy/Kconfig                                |   8 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/phy-qoriq-usb3.c                       | 202 +++++++++++++++++++++
>  4 files changed, 247 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>  create mode 100644 drivers/phy/phy-qoriq-usb3.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
> new file mode 100644
> index 0000000..d934c80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
> @@ -0,0 +1,36 @@
> +Driver for Freescale USB 3.0 PHY
> +
> +Required properties:
> +
> +- compatible :	fsl,qoriq-usb3-phy
> +- reg :		register mappings for Parameter Configuration Register
> +		and Phy base offset.
> +- reg-names :	"param_ctrl" and "phy_base"
> +- phy_type :	For multi port host USB controllers, should be one of
> +		"ulpi", or "serial". For dual role USB controllers,
> +		should be one of "ulpi", "utmi", "utmi_wide", or "serial".

Do any of these really apply to a USB3 PHY?

Rob

> +
> +Example:
> +		usbphy0: usb3-phy@084F0000 {

usb-phy@...

> +                        compatible = "fsl,qoriq-usb3-phy";
> +                        reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F0000 0x0 0x5000>;
> +                        reg-names = "param_ctrl", "phy_base";
> +                        #phy-cells = <0>;
> +                        phy_type = "utmi";
> +                };
> +
> +                usbphy1: usb3-phy@08500000 {
> +                        compatible = "fsl,qoriq-usb3-phy";
> +                        reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x08500000 0x0 0x5000>;
> +                        reg-names = "param_ctrl", "phy_base";
> +                        #phy-cells = <0>;
> +                        phy_type = "utmi";
> +                };
> +
> +                usbphy2: usb3-phy@08510000 {
> +                        compatible = "fsl,qoriq-usb3-phy";
> +                        reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x08510000 0x0 0x5000>;
> +                        reg-names = "param_ctrl", "phy_base";
> +                        #phy-cells = <0>;
> +                        phy_type = "utmi";
> +                };

RE: [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Sriram Dash]

>From: Rob Herring [mailto:robh@kernel.org]
>On Mon, Nov 14, 2016 at 10:56:54AM +0530, Sriram Dash wrote:
>> Adds qoriq usb 3.0 phy driver support for LS1043A platform.
>> Describes the qoriq usb 2.0 phy driver binding, currently used for
>> LS1043A platform.
>>
>> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
>> ---
>>  .../devicetree/bindings/phy/phy-qoriq-usb3.txt     |  36 ++++
>>  drivers/phy/Kconfig                                |   8 +
>>  drivers/phy/Makefile                               |   1 +
>>  drivers/phy/phy-qoriq-usb3.c                       | 202 +++++++++++++++++++++
>>  4 files changed, 247 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>  create mode 100644 drivers/phy/phy-qoriq-usb3.c
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> new file mode 100644
>> index 0000000..d934c80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>> @@ -0,0 +1,36 @@
>> +Driver for Freescale USB 3.0 PHY
>> +
>> +Required properties:
>> +
>> +- compatible :	fsl,qoriq-usb3-phy
>> +- reg :		register mappings for Parameter Configuration Register
>> +		and Phy base offset.
>> +- reg-names :	"param_ctrl" and "phy_base"
>> +- phy_type :	For multi port host USB controllers, should be one of
>> +		"ulpi", or "serial". For dual role USB controllers,
>> +		should be one of "ulpi", "utmi", "utmi_wide", or "serial".

Hi Rob,

>
>Do any of these really apply to a USB3 PHY?
>

The concerned USB3 phy used is UTMI. I agree to your point somewhat that
all the types are not required now. Anyway, shall I make it an optional
property, with the mention of only UTMI and ULPI?

>Rob
>
>> +
>> +Example:
>> +		usbphy0: usb3-phy@084F0000 {
>
>usb-phy@...
>

Ok. Will change in the next rev for Documentation and dts (patch 2/2)

>> +                        compatible = "fsl,qoriq-usb3-phy";
>> +                        reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F0000 0x0 0x5000>;
>> +                        reg-names = "param_ctrl", "phy_base";
>> +                        #phy-cells = <0>;
>> +                        phy_type = "utmi";
>> +                };
>> +
>> +                usbphy1: usb3-phy@08500000 {
>> +                        compatible = "fsl,qoriq-usb3-phy";
>> +                        reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x08500000 0x0 0x5000>;
>> +                        reg-names = "param_ctrl", "phy_base";
>> +                        #phy-cells = <0>;
>> +                        phy_type = "utmi";
>> +                };
>> +
>> +                usbphy2: usb3-phy@08510000 {
>> +                        compatible = "fsl,qoriq-usb3-phy";
>> +                        reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x08510000 0x0 0x5000>;
>> +                        reg-names = "param_ctrl", "phy_base";
>> +                        #phy-cells = <0>;
>> +                        phy_type = "utmi";
>> +                };

Re: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Scott Wood]

On 11/15/2016 06:39 AM, Sriram Dash wrote:
>> From: Scott Wood
>> On 11/13/2016 11:27 PM, Sriram Dash wrote:
>>> diff --git a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>> new file mode 100644
>>> index 0000000..d934c80
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>> @@ -0,0 +1,36 @@
>>> +Driver for Freescale USB 3.0 PHY
>>> +
>>> +Required properties:
>>> +
>>> +- compatible :	fsl,qoriq-usb3-phy
>>
> 
> Hi Scott,
> 
>> This is a very vague compatible.  Are there versioning registers within this register
>> block?
>>
> 
> There are versioning registers for the phy (1.0 and 1.1). But the current erratum
> A008751 does not require the mentioning of the version numbers. Was planning
> to take care of the versioning when there is code diversity on the basis of the
> version number.

That is not how device tree bindings work.  The describe the hardware,
not the driver.

That said, is the block version sufficient to tell whether a given chip
has this erratum?  If so, you don't need a special property for the
erratum.  If not, what is different about the PHY that is not described
by the versioning?

In any case, it would be nice to mention the version register and its
offset in the binding, just so that it becomes part of the definition of
this compatible string, and if we come out with some QorIQ chip with a
USB3 PHY that is totally different and doesn't have that version
register, it'll be clear that it needs a different compatible.

>>> +static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32
>>> +offset) {
>>> +	return __raw_readl(addr + offset);
>>> +}
>>> +
>>> +static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
>>> +	u32 data)
>>> +{
>>> +	__raw_writel(data, addr + offset);
>>> +}
>>
>> Why raw?  Besides missing barriers, this will cause the accesses to be native-endian
>> which is not correct.
>>
> 
> The only reason for __raw_writel is to make the code faster.

Does that really matter here?

> However, shall I use writel(with both barriers and byte swap) instead 

Yes, if the registers are little-endian on all chips.

> and then make appropriate changes in the value 32'h27672B2A?

Not sure what you mean here.

> In my knowledge, there are more than 5 errata in pipeline,

Then please get all of these errata described in the device tree ASAP
(unless their presence can be reliably inferred from the block version,
as discussed above).

> However, in future, if any other erratum comes up, and it has to be applied
> at any point other than during init, then the variable has to be added in
> qoriq_usb3_phy struct and the property has to be read separately.

Or if the erratum is detected by some means other than a device tree
property...

-Scott

RE: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Sriram Dash]

>From: Scott Wood
>On 11/15/2016 06:39 AM, Sriram Dash wrote:
>>> From: Scott Wood
>>> On 11/13/2016 11:27 PM, Sriram Dash wrote:
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> new file mode 100644
>>>> index 0000000..d934c80
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> @@ -0,0 +1,36 @@
>>>> +Driver for Freescale USB 3.0 PHY
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible :	fsl,qoriq-usb3-phy
>>>
>>
>> Hi Scott,
>>
>>> This is a very vague compatible.  Are there versioning registers
>>> within this register block?
>>>
>>
>> There are versioning registers for the phy (1.0 and 1.1). But the
>> current erratum
>> A008751 does not require the mentioning of the version numbers. Was
>> planning to take care of the versioning when there is code diversity
>> on the basis of the version number.
>
>That is not how device tree bindings work.  The describe the hardware, not the
>driver.
>
>That said, is the block version sufficient to tell whether a given chip has this
>erratum?  If so, you don't need a special property for the erratum.  If not, what is
>different about the PHY that is not described by the versioning?
>
>In any case, it would be nice to mention the version register and its offset in the
>binding, just so that it becomes part of the definition of this compatible string, and
>if we come out with some QorIQ chip with a
>USB3 PHY that is totally different and doesn't have that version register, it'll be
>clear that it needs a different compatible.
>

Okay. Will include version number in the next rev for Documentation and dt.

>>>> +static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32
>>>> +offset) {
>>>> +	return __raw_readl(addr + offset); }
>>>> +
>>>> +static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
>>>> +	u32 data)
>>>> +{
>>>> +	__raw_writel(data, addr + offset); }
>>>
>>> Why raw?  Besides missing barriers, this will cause the accesses to
>>> be native-endian which is not correct.
>>>
>>
>> The only reason for __raw_writel is to make the code faster.
>
>Does that really matter here?
>
>> However, shall I use writel(with both barriers and byte swap) instead
>
>Yes, if the registers are little-endian on all chips.
>

The endianness is not same for all Socs. But for most Socs, it is big-endian.
Is "iowrite32be" better instead? 

>> and then make appropriate changes in the value 32'h27672B2A?
>
>Not sure what you mean here.
>
>> In my knowledge, there are more than 5 errata in pipeline,
>
>Then please get all of these errata described in the device tree ASAP (unless their
>presence can be reliably inferred from the block version, as discussed above).
>

Yes. We will push the errata asap.

>> However, in future, if any other erratum comes up, and it has to be
>> applied at any point other than during init, then the variable has to
>> be added in qoriq_usb3_phy struct and the property has to be read separately.
>
>Or if the erratum is detected by some means other than a device tree property...
>

Yes. For any other case also, it will be handled differently.

>-Scott

Re: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

[Scott Wood]

On 11/16/2016 05:33 AM, Sriram Dash wrote:
>> From: Scott Wood
>> On 11/15/2016 06:39 AM, Sriram Dash wrote:
>>>> From: Scott Wood
>>>> On 11/13/2016 11:27 PM, Sriram Dash wrote:
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>>> new file mode 100644
>>>>> index 0000000..d934c80
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>>> @@ -0,0 +1,36 @@
>>>>> +Driver for Freescale USB 3.0 PHY
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +- compatible :	fsl,qoriq-usb3-phy
>>>>
>>>
>>> Hi Scott,
>>>
>>>> This is a very vague compatible.  Are there versioning registers
>>>> within this register block?
>>>>
>>>
>>> There are versioning registers for the phy (1.0 and 1.1). But the
>>> current erratum
>>> A008751 does not require the mentioning of the version numbers. Was
>>> planning to take care of the versioning when there is code diversity
>>> on the basis of the version number.
>>
>> That is not how device tree bindings work.  The describe the hardware, not the
>> driver.
>>
>> That said, is the block version sufficient to tell whether a given chip has this
>> erratum? If so, you don't need a special property for the erratum.  If not, what is
>> different about the PHY that is not described by the versioning?

Can you find out the answer to this?

>> In any case, it would be nice to mention the version register and its offset in the
>> binding, just so that it becomes part of the definition of this compatible string, and
>> if we come out with some QorIQ chip with a
>> USB3 PHY that is totally different and doesn't have that version register, it'll be
>> clear that it needs a different compatible.
>>
> 
> Okay. Will include version number in the next rev for Documentation and dt.

I'm not asking you to put the version number in the dt if it can be read
from a register.  I'm asking you to have the binding describe the
version register, as part of the definition of what "fsl,qoriq-usb3-phy"
means.

>>> The only reason for __raw_writel is to make the code faster.
>>
>> Does that really matter here?
>>
>>> However, shall I use writel(with both barriers and byte swap) instead
>>
>> Yes, if the registers are little-endian on all chips.
>>
> 
> The endianness is not same for all Socs. But for most Socs, it is big-endian.
> Is "iowrite32be" better instead? 

Then the device tree node needs to say what endian it is, and you need
to choose the appropriate accessor at runtime.

But is it really big endian for most or even any SoCs?  This hardware
isn't present on our PPC chips, right (I checked a couple of the most
recent PPC RMs and it shows USB 2.0 only)?  So it'd just be the few ARM
chips that made almost everything big endian, and even there the couple
RMs I checked (ls1021a and ls1043a) have these registers described as
little-endian.

>>> and then make appropriate changes in the value 32'h27672B2A?
>>
>> Not sure what you mean here.
>>
>>> In my knowledge, there are more than 5 errata in pipeline,
>>
>> Then please get all of these errata described in the device tree ASAP (unless their
>> presence can be reliably inferred from the block version, as discussed above).
>>
> 
> Yes. We will push the errata asap.

My point is that the device tree node should be complete when you submit
it.  Don't wait for the implementation of a workaround to advertise the
existence of an erratum in the device tree.

-Scott