[PATCH 0/5] Marvell PXA1928 USB support

[PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928 
SOC.

The OTG block is based on ChipIdea and works with "chipidea,usb2" 
compatible driver as is just by adding the PHY driver. Yay!

Rob

Rob Herring (5):
  dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
  dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
  phy: Add Marvell USB 2.0 OTG 28nm PHY
  phy: add Marvell HSIC 28nm PHY
  usb: add pxa1928 ehci support

 .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
 .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
 drivers/phy/Kconfig                                |  20 ++
 drivers/phy/Makefile                               |   2 +
 drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
 drivers/phy/phy-mv-usb2.c                          | 329 +++++++++++++++++++++
 drivers/usb/host/Kconfig                           |  15 +-
 drivers/usb/host/Makefile                          |   1 +
 drivers/usb/host/ehci-mv-of.c                      | 243 +++++++++++++++
 9 files changed, 854 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
 create mode 100644 drivers/phy/phy-mv-hsic.c
 create mode 100644 drivers/phy/phy-mv-usb2.c
 create mode 100644 drivers/usb/host/ehci-mv-of.c

-- 
2.1.0

[PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928 
SOC.

The OTG block is based on ChipIdea and works with "chipidea,usb2" 
compatible driver as is just by adding the PHY driver. Yay!

Rob

Rob Herring (5):
  dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
  dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
  phy: Add Marvell USB 2.0 OTG 28nm PHY
  phy: add Marvell HSIC 28nm PHY
  usb: add pxa1928 ehci support

 .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
 .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
 drivers/phy/Kconfig                                |  20 ++
 drivers/phy/Makefile                               |   2 +
 drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
 drivers/phy/phy-mv-usb2.c                          | 329 +++++++++++++++++++++
 drivers/usb/host/Kconfig                           |  15 +-
 drivers/usb/host/Makefile                          |   1 +
 drivers/usb/host/ehci-mv-of.c                      | 243 +++++++++++++++
 9 files changed, 854 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
 create mode 100644 drivers/phy/phy-mv-hsic.c
 create mode 100644 drivers/phy/phy-mv-usb2.c
 create mode 100644 drivers/usb/host/ehci-mv-of.c

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings

[Rob Herring]

Add PHY binding for Marvell PXA1928 SOC's USB and HSIC PHYs.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/phy/pxa1928-usb-phy.txt        | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt b/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
new file mode 100644
index 0000000..660a13c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
@@ -0,0 +1,18 @@
+* Marvell PXA1928 USB and HSIC PHYs
+
+Required properties:
+- compatible: "marvell,pxa1928-usb-phy" or "marvell,pxa1928-hsic-phy"
+- reg: base address and length of the registers
+- clocks - A single clock. From common clock binding.
+- #phys-cells: should be 0. From commmon phy binding.
+- resets: reference to the reset controller
+
+Example:
+
+	usbphy: phy@7000 {
+		compatible = "marvell,pxa1928-usb-phy";
+		reg = <0x7000 0xe0>;
+		clocks = <&apmu_clocks PXA1928_CLK_USB>;
+		#phy-cells = <0>;
+	};
+
-- 
2.1.0

[PATCH 1/5] dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings

[Rob Herring]

Add PHY binding for Marvell PXA1928 SOC's USB and HSIC PHYs.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../devicetree/bindings/phy/pxa1928-usb-phy.txt        | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt b/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
new file mode 100644
index 0000000..660a13c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
@@ -0,0 +1,18 @@
+* Marvell PXA1928 USB and HSIC PHYs
+
+Required properties:
+- compatible: "marvell,pxa1928-usb-phy" or "marvell,pxa1928-hsic-phy"
+- reg: base address and length of the registers
+- clocks - A single clock. From common clock binding.
+- #phys-cells: should be 0. From commmon phy binding.
+- resets: reference to the reset controller
+
+Example:
+
+	usbphy: phy@7000 {
+		compatible = "marvell,pxa1928-usb-phy";
+		reg = <0x7000 0xe0>;
+		clocks = <&apmu_clocks PXA1928_CLK_USB>;
+		#phy-cells = <0>;
+	};
+
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] dt-bindings: Add Marvell PXA1928 USB EHCI controller binding

[Rob Herring]

Add binding for Marvell PXA1928 SOC's USB EHCI host controller.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/usb/ehci-pxa1928.txt          | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-pxa1928.txt

diff --git a/Documentation/devicetree/bindings/usb/ehci-pxa1928.txt b/Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
new file mode 100644
index 0000000..25881be
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
@@ -0,0 +1,19 @@
+* EHCI controller, Marvell PXA1928
+
+Required properties:
+- compatible: must be "marvell,pxa1928-ehci"
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- interrupts: The EHCI interrupt
+- clocks: reference to the clock
+- phys: phandle reference to the USB PHY
+
+Example:
+
+	hsic: usb@601100 {
+		compatible = "marvell,pxa1928-ehci";
+		reg = <0x601100 0x200>;
+		interrupts = <0 22 0x4>;
+		clocks = <&apmu_clocks PXA1928_CLK_HSIC>;
+		phys = <&hsicphy>;
+	};
-- 
2.1.0

[PATCH 3/5] phy: Add Marvell USB 2.0 OTG 28nm PHY

[Rob Herring]

Add driver for USB PHY found in Marvell PXA1928 SOC.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/Kconfig       |  10 ++
 drivers/phy/Makefile      |   1 +
 drivers/phy/phy-mv-usb2.c | 329 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 340 insertions(+)
 create mode 100644 drivers/phy/phy-mv-usb2.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index a53bd5b..ef7634f 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
 	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
 	  and EXYNOS SoCs.
 
+config PHY_MV_USB2
+	tristate "Marvell USB 2.0 28nm PHY Driver"
+	select GENERIC_PHY
+	help
+	  Enable this to support Marvell USB 2.0 PHY driver for Marvell
+	  SoC. This driver will do the PHY initialization and shutdown.
+	  The PHY driver will be used by Marvell udc/ehci/otg driver.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_MVEBU_SATA
 	def_bool y
 	depends on ARCH_DOVE || MACH_DOVE || MACH_KIRKWOOD
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index f126251..768e55a 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
 obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
 obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
 obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
+obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
 obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
 obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
 obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
diff --git a/drivers/phy/phy-mv-usb2.c b/drivers/phy/phy-mv-usb2.c
new file mode 100644
index 0000000..c48d111
--- /dev/null
+++ b/drivers/phy/phy-mv-usb2.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright (C) 2015 Linaro, Ltd.
+ * Rob Herring <robh@kernel.org>
+ *
+ * Based on vendor driver:
+ * Copyright (C) 2013 Marvell Inc.
+ * Author: Chao Xie <xiechao.mail@gmail.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/delay.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+
+/* USB PXA1928 PHY mapping */
+#define PHY_28NM_PLL_REG0			0x0
+#define PHY_28NM_PLL_REG1			0x4
+#define PHY_28NM_CAL_REG			0x8
+#define PHY_28NM_TX_REG0			0x0c
+#define PHY_28NM_TX_REG1			0x10
+#define PHY_28NM_RX_REG0			0x14
+#define PHY_28NM_RX_REG1			0x18
+#define PHY_28NM_DIG_REG0			0x1c
+#define PHY_28NM_DIG_REG1			0x20
+#define PHY_28NM_TEST_REG0			0x24
+#define PHY_28NM_TEST_REG1			0x28
+#define PHY_28NM_MOC_REG			0x2c
+#define PHY_28NM_PHY_RESERVE			0x30
+#define PHY_28NM_OTG_REG			0x34
+#define PHY_28NM_CHRG_DET			0x38
+#define PHY_28NM_CTRL_REG0			0xc4
+#define PHY_28NM_CTRL_REG1			0xc8
+#define PHY_28NM_CTRL_REG2			0xd4
+#define PHY_28NM_CTRL_REG3			0xdc
+
+/* PHY_28NM_PLL_REG0 */
+#define PHY_28NM_PLL_READY			BIT(31)
+
+#define PHY_28NM_PLL_SELLPFR_SHIFT		28
+#define PHY_28NM_PLL_SELLPFR_MASK		(0x3 << 28)
+
+#define PHY_28NM_PLL_FBDIV_SHIFT		16
+#define PHY_28NM_PLL_FBDIV_MASK			(0x1ff << 16)
+
+#define PHY_28NM_PLL_ICP_SHIFT			8
+#define PHY_28NM_PLL_ICP_MASK			(0x7 << 8)
+
+#define PHY_28NM_PLL_REFDIV_SHIFT		0
+#define PHY_28NM_PLL_REFDIV_MASK		0x7f
+
+/* PHY_28NM_PLL_REG1 */
+#define PHY_28NM_PLL_PU_BY_REG			BIT(1)
+
+#define PHY_28NM_PLL_PU_PLL			BIT(0)
+
+/* PHY_28NM_CAL_REG */
+#define PHY_28NM_PLL_PLLCAL_DONE		BIT(31)
+
+#define PHY_28NM_PLL_IMPCAL_DONE		BIT(23)
+
+#define PHY_28NM_PLL_KVCO_SHIFT			16
+#define PHY_28NM_PLL_KVCO_MASK			(0x7 << 16)
+
+#define PHY_28NM_PLL_CAL12_SHIFT		20
+#define PHY_28NM_PLL_CAL12_MASK			(0x3 << 20)
+
+#define PHY_28NM_IMPCAL_VTH_SHIFT		8
+#define PHY_28NM_IMPCAL_VTH_MASK		(0x7 << 8)
+
+#define PHY_28NM_PLLCAL_START_SHIFT		22
+#define PHY_28NM_IMPCAL_START_SHIFT		13
+
+/* PHY_28NM_TX_REG0 */
+#define PHY_28NM_TX_PU_BY_REG			BIT(25)
+
+#define PHY_28NM_TX_PU_ANA			BIT(24)
+
+#define PHY_28NM_TX_AMP_SHIFT			20
+#define PHY_28NM_TX_AMP_MASK			(0x7 << 20)
+
+/* PHY_28NM_RX_REG0 */
+#define PHY_28NM_RX_SQ_THRESH_SHIFT		0
+#define PHY_28NM_RX_SQ_THRESH_MASK		(0xf << 0)
+
+/* PHY_28NM_RX_REG1 */
+#define PHY_28NM_RX_SQCAL_DONE			BIT(31)
+
+/* PHY_28NM_DIG_REG0 */
+#define PHY_28NM_DIG_BITSTAFFING_ERR		BIT(31)
+#define PHY_28NM_DIG_SYNC_ERR			BIT(30)
+
+#define PHY_28NM_DIG_SQ_FILT_SHIFT		16
+#define PHY_28NM_DIG_SQ_FILT_MASK		(0x7 << 16)
+
+#define PHY_28NM_DIG_SQ_BLK_SHIFT		12
+#define PHY_28NM_DIG_SQ_BLK_MASK		(0x7 << 12)
+
+#define PHY_28NM_DIG_SYNC_NUM_SHIFT		0
+#define PHY_28NM_DIG_SYNC_NUM_MASK		(0x3 << 0)
+
+#define PHY_28NM_PLL_LOCK_BYPASS		BIT(7)
+
+/* PHY_28NM_OTG_REG */
+#define PHY_28NM_OTG_CONTROL_BY_PIN		BIT(5)
+#define PHY_28NM_OTG_PU_OTG			BIT(4)
+
+#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DM_SHIFT_28 13
+#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DP_SHIFT_28 12
+#define PHY_28NM_CHGDTC_VSRC_CHARGE_SHIFT_28	10
+#define PHY_28NM_CHGDTC_VDAT_CHARGE_SHIFT_28	8
+#define PHY_28NM_CHGDTC_CDP_DM_AUTO_SWITCH_SHIFT_28 7
+#define PHY_28NM_CHGDTC_DP_DM_SWAP_SHIFT_28	6
+#define PHY_28NM_CHGDTC_PU_CHRG_DTC_SHIFT_28	5
+#define PHY_28NM_CHGDTC_PD_EN_SHIFT_28		4
+#define PHY_28NM_CHGDTC_DCP_EN_SHIFT_28		3
+#define PHY_28NM_CHGDTC_CDP_EN_SHIFT_28		2
+#define PHY_28NM_CHGDTC_TESTMON_CHRGDTC_SHIFT_28 0
+
+#define PHY_28NM_CTRL1_CHRG_DTC_OUT_SHIFT_28	4
+#define PHY_28NM_CTRL1_VBUSDTC_OUT_SHIFT_28	2
+
+#define PHY_28NM_CTRL3_OVERWRITE		BIT(0)
+#define PHY_28NM_CTRL3_VBUS_VALID		BIT(4)
+#define PHY_28NM_CTRL3_AVALID			BIT(5)
+#define PHY_28NM_CTRL3_BVALID			BIT(6)
+
+struct mv_usb2_phy {
+	struct phy		*phy;
+	struct platform_device	*pdev;
+	void __iomem		*base;
+	struct clk		*clk;
+};
+
+static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
+{
+	timeout += jiffies;
+	while (time_is_after_eq_jiffies(timeout)) {
+		if ((readl(reg) & mask) == mask)
+			return true;
+		msleep(1);
+	}
+	return false;
+}
+
+static int mv_usb2_phy_28nm_init(struct phy *phy)
+{
+	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
+	struct platform_device *pdev = mv_phy->pdev;
+	void __iomem *base = mv_phy->base;
+	u32 reg;
+
+	clk_prepare_enable(mv_phy->clk);
+
+	/* PHY_28NM_PLL_REG0 */
+	reg = readl(base + PHY_28NM_PLL_REG0) &
+		~(PHY_28NM_PLL_SELLPFR_MASK | PHY_28NM_PLL_FBDIV_MASK
+		| PHY_28NM_PLL_ICP_MASK	| PHY_28NM_PLL_REFDIV_MASK);
+	writel(reg | (0x1 << PHY_28NM_PLL_SELLPFR_SHIFT
+		| 0xf0 << PHY_28NM_PLL_FBDIV_SHIFT
+		| 0x3 << PHY_28NM_PLL_ICP_SHIFT
+		| 0xd << PHY_28NM_PLL_REFDIV_SHIFT),
+		base + PHY_28NM_PLL_REG0);
+
+	/* PHY_28NM_PLL_REG1 */
+	reg = readl(base + PHY_28NM_PLL_REG1);
+	writel(reg | PHY_28NM_PLL_PU_PLL | PHY_28NM_PLL_PU_BY_REG,
+		base + PHY_28NM_PLL_REG1);
+
+	/* PHY_28NM_TX_REG0 */
+	reg = readl(base + PHY_28NM_TX_REG0) & ~PHY_28NM_TX_AMP_MASK;
+	writel(reg | PHY_28NM_TX_PU_BY_REG | 0x3 << PHY_28NM_TX_AMP_SHIFT |
+		PHY_28NM_TX_PU_ANA,
+		base + PHY_28NM_TX_REG0);
+
+	/* PHY_28NM_RX_REG0 */
+	reg = readl(base + PHY_28NM_RX_REG0) & ~PHY_28NM_RX_SQ_THRESH_MASK;
+	writel(reg | 0xa << PHY_28NM_RX_SQ_THRESH_SHIFT,
+		base + PHY_28NM_RX_REG0);
+
+	/* PHY_28NM_DIG_REG0 */
+	reg = readl(base + PHY_28NM_DIG_REG0) &
+		~(PHY_28NM_DIG_BITSTAFFING_ERR | PHY_28NM_DIG_SYNC_ERR |
+		PHY_28NM_DIG_SQ_FILT_MASK | PHY_28NM_DIG_SQ_BLK_MASK |
+		PHY_28NM_DIG_SYNC_NUM_MASK);
+	writel(reg | (0x1 << PHY_28NM_DIG_SYNC_NUM_SHIFT |
+		PHY_28NM_PLL_LOCK_BYPASS),
+		base + PHY_28NM_DIG_REG0);
+
+	/* PHY_28NM_OTG_REG */
+	reg = readl(base + PHY_28NM_OTG_REG) | PHY_28NM_OTG_PU_OTG;
+	writel(reg & ~PHY_28NM_OTG_CONTROL_BY_PIN, base + PHY_28NM_OTG_REG);
+
+	/*  Calibration Timing
+	*		   ____________________________
+	*  CAL START   ___|
+	*			   ____________________
+	*  CAL_DONE    ___________|
+	*		  | 400us |
+	*/
+
+	/* Make sure PHY Calibration is ready */
+	if (!wait_for_reg(base + PHY_28NM_CAL_REG,
+	    PHY_28NM_PLL_PLLCAL_DONE | PHY_28NM_PLL_IMPCAL_DONE,
+	    HZ / 10)) {
+		dev_warn(&pdev->dev, "USB PHY PLL calibrate not done after 100mS.");
+		return -ETIMEDOUT;
+	}
+	if (!wait_for_reg(base + PHY_28NM_RX_REG1,
+	    PHY_28NM_RX_SQCAL_DONE, HZ / 10)) {
+		dev_warn(&pdev->dev, "USB PHY RX SQ calibrate not done after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	/* Make sure PHY PLL is ready */
+	if (!wait_for_reg(base + PHY_28NM_PLL_REG0,
+	    PHY_28NM_PLL_READY, HZ / 10)) {
+		dev_warn(&pdev->dev, "PLL_READY not set after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	writel(readl(base + PHY_28NM_CTRL_REG3) |
+		(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
+		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
+		base + PHY_28NM_CTRL_REG3);
+
+	return 0;
+}
+
+static int mv_usb2_phy_28nm_power_off(struct phy *phy)
+{
+	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
+	void __iomem *base = mv_phy->base;
+	unsigned int val;
+
+	writel(readl(base + PHY_28NM_CTRL_REG3) |
+		~(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
+		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
+		base + PHY_28NM_CTRL_REG3);
+
+	val = readw(base + PHY_28NM_PLL_REG1);
+	val &= ~PHY_28NM_PLL_PU_PLL;
+	writew(val, base + PHY_28NM_PLL_REG1);
+
+	/* power down PHY Analog part */
+	val = readw(base + PHY_28NM_TX_REG0);
+	val &= ~PHY_28NM_TX_PU_ANA;
+	writew(val, base + PHY_28NM_TX_REG0);
+
+	/* power down PHY OTG part */
+	val = readw(base + PHY_28NM_OTG_REG);
+	val &= ~PHY_28NM_OTG_PU_OTG;
+	writew(val, base + PHY_28NM_OTG_REG);
+
+	clk_disable_unprepare(mv_phy->clk);
+	return 0;
+}
+
+static const struct phy_ops usb_ops = {
+	.init		= mv_usb2_phy_28nm_init,
+	.power_off	= mv_usb2_phy_28nm_power_off,
+};
+
+static int mv_usb2_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct mv_usb2_phy *mv_phy;
+	struct resource *r;
+
+	mv_phy = devm_kzalloc(&pdev->dev, sizeof(*mv_phy), GFP_KERNEL);
+	if (!mv_phy)
+		return -ENOMEM;
+
+	mv_phy->pdev = pdev;
+
+	mv_phy->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mv_phy->clk)) {
+		dev_err(&pdev->dev, "failed to get clock.\n");
+		return PTR_ERR(mv_phy->clk);
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mv_phy->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(mv_phy->base))
+		return PTR_ERR(mv_phy->base);
+
+	mv_phy->phy = devm_phy_create(&pdev->dev, pdev->dev.of_node, &usb_ops);
+	if (IS_ERR(mv_phy->phy))
+		return PTR_ERR(mv_phy->phy);
+
+	phy_set_drvdata(mv_phy->phy, mv_phy);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mv_usbphy_dt_match[] = {
+	{ .compatible = "marvell,pxa1928-usb-phy", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mv_usbphy_dt_match);
+
+static struct platform_driver mv_usb2_phy_driver = {
+	.probe	= mv_usb2_phy_probe,
+	.driver = {
+		.name   = "mv-usb2-phy",
+		.of_match_table = of_match_ptr(mv_usbphy_dt_match),
+	},
+};
+module_platform_driver(mv_usb2_phy_driver);
+
+MODULE_AUTHOR("Rob Herring <robh@kernel.org>");
+MODULE_DESCRIPTION("Marvell USB2 phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[PATCH 3/5] phy: Add Marvell USB 2.0 OTG 28nm PHY

[Rob Herring]

Add driver for USB PHY found in Marvell PXA1928 SOC.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
---
 drivers/phy/Kconfig       |  10 ++
 drivers/phy/Makefile      |   1 +
 drivers/phy/phy-mv-usb2.c | 329 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 340 insertions(+)
 create mode 100644 drivers/phy/phy-mv-usb2.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index a53bd5b..ef7634f 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
 	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
 	  and EXYNOS SoCs.
 
+config PHY_MV_USB2
+	tristate "Marvell USB 2.0 28nm PHY Driver"
+	select GENERIC_PHY
+	help
+	  Enable this to support Marvell USB 2.0 PHY driver for Marvell
+	  SoC. This driver will do the PHY initialization and shutdown.
+	  The PHY driver will be used by Marvell udc/ehci/otg driver.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_MVEBU_SATA
 	def_bool y
 	depends on ARCH_DOVE || MACH_DOVE || MACH_KIRKWOOD
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index f126251..768e55a 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
 obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
 obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
 obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
+obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
 obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
 obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
 obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
diff --git a/drivers/phy/phy-mv-usb2.c b/drivers/phy/phy-mv-usb2.c
new file mode 100644
index 0000000..c48d111
--- /dev/null
+++ b/drivers/phy/phy-mv-usb2.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright (C) 2015 Linaro, Ltd.
+ * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
+ *
+ * Based on vendor driver:
+ * Copyright (C) 2013 Marvell Inc.
+ * Author: Chao Xie <xiechao.mail-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/delay.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+
+/* USB PXA1928 PHY mapping */
+#define PHY_28NM_PLL_REG0			0x0
+#define PHY_28NM_PLL_REG1			0x4
+#define PHY_28NM_CAL_REG			0x8
+#define PHY_28NM_TX_REG0			0x0c
+#define PHY_28NM_TX_REG1			0x10
+#define PHY_28NM_RX_REG0			0x14
+#define PHY_28NM_RX_REG1			0x18
+#define PHY_28NM_DIG_REG0			0x1c
+#define PHY_28NM_DIG_REG1			0x20
+#define PHY_28NM_TEST_REG0			0x24
+#define PHY_28NM_TEST_REG1			0x28
+#define PHY_28NM_MOC_REG			0x2c
+#define PHY_28NM_PHY_RESERVE			0x30
+#define PHY_28NM_OTG_REG			0x34
+#define PHY_28NM_CHRG_DET			0x38
+#define PHY_28NM_CTRL_REG0			0xc4
+#define PHY_28NM_CTRL_REG1			0xc8
+#define PHY_28NM_CTRL_REG2			0xd4
+#define PHY_28NM_CTRL_REG3			0xdc
+
+/* PHY_28NM_PLL_REG0 */
+#define PHY_28NM_PLL_READY			BIT(31)
+
+#define PHY_28NM_PLL_SELLPFR_SHIFT		28
+#define PHY_28NM_PLL_SELLPFR_MASK		(0x3 << 28)
+
+#define PHY_28NM_PLL_FBDIV_SHIFT		16
+#define PHY_28NM_PLL_FBDIV_MASK			(0x1ff << 16)
+
+#define PHY_28NM_PLL_ICP_SHIFT			8
+#define PHY_28NM_PLL_ICP_MASK			(0x7 << 8)
+
+#define PHY_28NM_PLL_REFDIV_SHIFT		0
+#define PHY_28NM_PLL_REFDIV_MASK		0x7f
+
+/* PHY_28NM_PLL_REG1 */
+#define PHY_28NM_PLL_PU_BY_REG			BIT(1)
+
+#define PHY_28NM_PLL_PU_PLL			BIT(0)
+
+/* PHY_28NM_CAL_REG */
+#define PHY_28NM_PLL_PLLCAL_DONE		BIT(31)
+
+#define PHY_28NM_PLL_IMPCAL_DONE		BIT(23)
+
+#define PHY_28NM_PLL_KVCO_SHIFT			16
+#define PHY_28NM_PLL_KVCO_MASK			(0x7 << 16)
+
+#define PHY_28NM_PLL_CAL12_SHIFT		20
+#define PHY_28NM_PLL_CAL12_MASK			(0x3 << 20)
+
+#define PHY_28NM_IMPCAL_VTH_SHIFT		8
+#define PHY_28NM_IMPCAL_VTH_MASK		(0x7 << 8)
+
+#define PHY_28NM_PLLCAL_START_SHIFT		22
+#define PHY_28NM_IMPCAL_START_SHIFT		13
+
+/* PHY_28NM_TX_REG0 */
+#define PHY_28NM_TX_PU_BY_REG			BIT(25)
+
+#define PHY_28NM_TX_PU_ANA			BIT(24)
+
+#define PHY_28NM_TX_AMP_SHIFT			20
+#define PHY_28NM_TX_AMP_MASK			(0x7 << 20)
+
+/* PHY_28NM_RX_REG0 */
+#define PHY_28NM_RX_SQ_THRESH_SHIFT		0
+#define PHY_28NM_RX_SQ_THRESH_MASK		(0xf << 0)
+
+/* PHY_28NM_RX_REG1 */
+#define PHY_28NM_RX_SQCAL_DONE			BIT(31)
+
+/* PHY_28NM_DIG_REG0 */
+#define PHY_28NM_DIG_BITSTAFFING_ERR		BIT(31)
+#define PHY_28NM_DIG_SYNC_ERR			BIT(30)
+
+#define PHY_28NM_DIG_SQ_FILT_SHIFT		16
+#define PHY_28NM_DIG_SQ_FILT_MASK		(0x7 << 16)
+
+#define PHY_28NM_DIG_SQ_BLK_SHIFT		12
+#define PHY_28NM_DIG_SQ_BLK_MASK		(0x7 << 12)
+
+#define PHY_28NM_DIG_SYNC_NUM_SHIFT		0
+#define PHY_28NM_DIG_SYNC_NUM_MASK		(0x3 << 0)
+
+#define PHY_28NM_PLL_LOCK_BYPASS		BIT(7)
+
+/* PHY_28NM_OTG_REG */
+#define PHY_28NM_OTG_CONTROL_BY_PIN		BIT(5)
+#define PHY_28NM_OTG_PU_OTG			BIT(4)
+
+#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DM_SHIFT_28 13
+#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DP_SHIFT_28 12
+#define PHY_28NM_CHGDTC_VSRC_CHARGE_SHIFT_28	10
+#define PHY_28NM_CHGDTC_VDAT_CHARGE_SHIFT_28	8
+#define PHY_28NM_CHGDTC_CDP_DM_AUTO_SWITCH_SHIFT_28 7
+#define PHY_28NM_CHGDTC_DP_DM_SWAP_SHIFT_28	6
+#define PHY_28NM_CHGDTC_PU_CHRG_DTC_SHIFT_28	5
+#define PHY_28NM_CHGDTC_PD_EN_SHIFT_28		4
+#define PHY_28NM_CHGDTC_DCP_EN_SHIFT_28		3
+#define PHY_28NM_CHGDTC_CDP_EN_SHIFT_28		2
+#define PHY_28NM_CHGDTC_TESTMON_CHRGDTC_SHIFT_28 0
+
+#define PHY_28NM_CTRL1_CHRG_DTC_OUT_SHIFT_28	4
+#define PHY_28NM_CTRL1_VBUSDTC_OUT_SHIFT_28	2
+
+#define PHY_28NM_CTRL3_OVERWRITE		BIT(0)
+#define PHY_28NM_CTRL3_VBUS_VALID		BIT(4)
+#define PHY_28NM_CTRL3_AVALID			BIT(5)
+#define PHY_28NM_CTRL3_BVALID			BIT(6)
+
+struct mv_usb2_phy {
+	struct phy		*phy;
+	struct platform_device	*pdev;
+	void __iomem		*base;
+	struct clk		*clk;
+};
+
+static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
+{
+	timeout += jiffies;
+	while (time_is_after_eq_jiffies(timeout)) {
+		if ((readl(reg) & mask) == mask)
+			return true;
+		msleep(1);
+	}
+	return false;
+}
+
+static int mv_usb2_phy_28nm_init(struct phy *phy)
+{
+	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
+	struct platform_device *pdev = mv_phy->pdev;
+	void __iomem *base = mv_phy->base;
+	u32 reg;
+
+	clk_prepare_enable(mv_phy->clk);
+
+	/* PHY_28NM_PLL_REG0 */
+	reg = readl(base + PHY_28NM_PLL_REG0) &
+		~(PHY_28NM_PLL_SELLPFR_MASK | PHY_28NM_PLL_FBDIV_MASK
+		| PHY_28NM_PLL_ICP_MASK	| PHY_28NM_PLL_REFDIV_MASK);
+	writel(reg | (0x1 << PHY_28NM_PLL_SELLPFR_SHIFT
+		| 0xf0 << PHY_28NM_PLL_FBDIV_SHIFT
+		| 0x3 << PHY_28NM_PLL_ICP_SHIFT
+		| 0xd << PHY_28NM_PLL_REFDIV_SHIFT),
+		base + PHY_28NM_PLL_REG0);
+
+	/* PHY_28NM_PLL_REG1 */
+	reg = readl(base + PHY_28NM_PLL_REG1);
+	writel(reg | PHY_28NM_PLL_PU_PLL | PHY_28NM_PLL_PU_BY_REG,
+		base + PHY_28NM_PLL_REG1);
+
+	/* PHY_28NM_TX_REG0 */
+	reg = readl(base + PHY_28NM_TX_REG0) & ~PHY_28NM_TX_AMP_MASK;
+	writel(reg | PHY_28NM_TX_PU_BY_REG | 0x3 << PHY_28NM_TX_AMP_SHIFT |
+		PHY_28NM_TX_PU_ANA,
+		base + PHY_28NM_TX_REG0);
+
+	/* PHY_28NM_RX_REG0 */
+	reg = readl(base + PHY_28NM_RX_REG0) & ~PHY_28NM_RX_SQ_THRESH_MASK;
+	writel(reg | 0xa << PHY_28NM_RX_SQ_THRESH_SHIFT,
+		base + PHY_28NM_RX_REG0);
+
+	/* PHY_28NM_DIG_REG0 */
+	reg = readl(base + PHY_28NM_DIG_REG0) &
+		~(PHY_28NM_DIG_BITSTAFFING_ERR | PHY_28NM_DIG_SYNC_ERR |
+		PHY_28NM_DIG_SQ_FILT_MASK | PHY_28NM_DIG_SQ_BLK_MASK |
+		PHY_28NM_DIG_SYNC_NUM_MASK);
+	writel(reg | (0x1 << PHY_28NM_DIG_SYNC_NUM_SHIFT |
+		PHY_28NM_PLL_LOCK_BYPASS),
+		base + PHY_28NM_DIG_REG0);
+
+	/* PHY_28NM_OTG_REG */
+	reg = readl(base + PHY_28NM_OTG_REG) | PHY_28NM_OTG_PU_OTG;
+	writel(reg & ~PHY_28NM_OTG_CONTROL_BY_PIN, base + PHY_28NM_OTG_REG);
+
+	/*  Calibration Timing
+	*		   ____________________________
+	*  CAL START   ___|
+	*			   ____________________
+	*  CAL_DONE    ___________|
+	*		  | 400us |
+	*/
+
+	/* Make sure PHY Calibration is ready */
+	if (!wait_for_reg(base + PHY_28NM_CAL_REG,
+	    PHY_28NM_PLL_PLLCAL_DONE | PHY_28NM_PLL_IMPCAL_DONE,
+	    HZ / 10)) {
+		dev_warn(&pdev->dev, "USB PHY PLL calibrate not done after 100mS.");
+		return -ETIMEDOUT;
+	}
+	if (!wait_for_reg(base + PHY_28NM_RX_REG1,
+	    PHY_28NM_RX_SQCAL_DONE, HZ / 10)) {
+		dev_warn(&pdev->dev, "USB PHY RX SQ calibrate not done after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	/* Make sure PHY PLL is ready */
+	if (!wait_for_reg(base + PHY_28NM_PLL_REG0,
+	    PHY_28NM_PLL_READY, HZ / 10)) {
+		dev_warn(&pdev->dev, "PLL_READY not set after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	writel(readl(base + PHY_28NM_CTRL_REG3) |
+		(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
+		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
+		base + PHY_28NM_CTRL_REG3);
+
+	return 0;
+}
+
+static int mv_usb2_phy_28nm_power_off(struct phy *phy)
+{
+	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
+	void __iomem *base = mv_phy->base;
+	unsigned int val;
+
+	writel(readl(base + PHY_28NM_CTRL_REG3) |
+		~(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
+		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
+		base + PHY_28NM_CTRL_REG3);
+
+	val = readw(base + PHY_28NM_PLL_REG1);
+	val &= ~PHY_28NM_PLL_PU_PLL;
+	writew(val, base + PHY_28NM_PLL_REG1);
+
+	/* power down PHY Analog part */
+	val = readw(base + PHY_28NM_TX_REG0);
+	val &= ~PHY_28NM_TX_PU_ANA;
+	writew(val, base + PHY_28NM_TX_REG0);
+
+	/* power down PHY OTG part */
+	val = readw(base + PHY_28NM_OTG_REG);
+	val &= ~PHY_28NM_OTG_PU_OTG;
+	writew(val, base + PHY_28NM_OTG_REG);
+
+	clk_disable_unprepare(mv_phy->clk);
+	return 0;
+}
+
+static const struct phy_ops usb_ops = {
+	.init		= mv_usb2_phy_28nm_init,
+	.power_off	= mv_usb2_phy_28nm_power_off,
+};
+
+static int mv_usb2_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct mv_usb2_phy *mv_phy;
+	struct resource *r;
+
+	mv_phy = devm_kzalloc(&pdev->dev, sizeof(*mv_phy), GFP_KERNEL);
+	if (!mv_phy)
+		return -ENOMEM;
+
+	mv_phy->pdev = pdev;
+
+	mv_phy->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mv_phy->clk)) {
+		dev_err(&pdev->dev, "failed to get clock.\n");
+		return PTR_ERR(mv_phy->clk);
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mv_phy->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(mv_phy->base))
+		return PTR_ERR(mv_phy->base);
+
+	mv_phy->phy = devm_phy_create(&pdev->dev, pdev->dev.of_node, &usb_ops);
+	if (IS_ERR(mv_phy->phy))
+		return PTR_ERR(mv_phy->phy);
+
+	phy_set_drvdata(mv_phy->phy, mv_phy);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mv_usbphy_dt_match[] = {
+	{ .compatible = "marvell,pxa1928-usb-phy", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mv_usbphy_dt_match);
+
+static struct platform_driver mv_usb2_phy_driver = {
+	.probe	= mv_usb2_phy_probe,
+	.driver = {
+		.name   = "mv-usb2-phy",
+		.of_match_table = of_match_ptr(mv_usbphy_dt_match),
+	},
+};
+module_platform_driver(mv_usb2_phy_driver);
+
+MODULE_AUTHOR("Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>");
+MODULE_DESCRIPTION("Marvell USB2 phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Rob Herring]

Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in PXA1928
SOC.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/Kconfig       |  10 +++
 drivers/phy/Makefile      |   1 +
 drivers/phy/phy-mv-hsic.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/phy/phy-mv-hsic.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ef7634f..cfcae72 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
 	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
 	  and EXYNOS SoCs.
 
+config PHY_MV_HSIC
+	tristate "Marvell USB HSIC 28nm PHY Driver"
+	select GENERIC_PHY
+	help
+	  Enable this to support Marvell USB HSIC PHY driver for Marvell
+	  SoC. This driver will do the PHY initialization and shutdown.
+	  The PHY driver will be used by Marvell ehci driver.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_MV_USB2
 	tristate "Marvell USB 2.0 28nm PHY Driver"
 	select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 768e55a..472bf0a 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
 obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
 obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
 obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
+obj-$(CONFIG_PHY_MV_HSIC)		+= phy-mv-hsic.o
 obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
 obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
 obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
diff --git a/drivers/phy/phy-mv-hsic.c b/drivers/phy/phy-mv-hsic.c
new file mode 100644
index 0000000..aa6cccd
--- /dev/null
+++ b/drivers/phy/phy-mv-hsic.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright (C) 2015 Linaro, Ltd.
+ * Rob Herring <robh@kernel.org>
+ *
+ * Based on vendor driver:
+ * Copyright (C) 2013 Marvell Inc.
+ * Author: Chao Xie <xiechao.mail@gmail.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/delay.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+
+#define PHY_28NM_HSIC_CTRL			0x08
+#define PHY_28NM_HSIC_IMPCAL_CAL		0x18
+#define PHY_28NM_HSIC_PLL_CTRL01		0x1c
+#define PHY_28NM_HSIC_PLL_CTRL2			0x20
+#define PHY_28NM_HSIC_INT			0x28
+
+#define PHY_28NM_HSIC_PLL_SELLPFR_SHIFT		26
+#define PHY_28NM_HSIC_PLL_FBDIV_SHIFT		0
+#define PHY_28NM_HSIC_PLL_REFDIV_SHIFT		9
+
+#define PHY_28NM_HSIC_S2H_PU_PLL		BIT(10)
+#define PHY_28NM_HSIC_H2S_PLL_LOCK		BIT(15)
+#define PHY_28NM_HSIC_S2H_HSIC_EN		BIT(7)
+#define S2H_DRV_SE0_4RESUME			BIT(14)
+#define PHY_28NM_HSIC_H2S_IMPCAL_DONE		BIT(27)
+
+#define PHY_28NM_HSIC_CONNECT_INT		BIT(1)
+#define PHY_28NM_HSIC_HS_READY_INT		BIT(2)
+
+struct mv_hsic_phy {
+	struct phy		*phy;
+	struct platform_device	*pdev;
+	void __iomem		*base;
+	struct clk		*clk;
+};
+
+static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
+{
+	timeout += jiffies;
+	while (time_is_after_eq_jiffies(timeout)) {
+		if ((readl(reg) & mask) == mask)
+			return true;
+		msleep(1);
+	}
+	return false;
+}
+
+static int mv_hsic_phy_init(struct phy *phy)
+{
+	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
+	struct platform_device *pdev = mv_phy->pdev;
+	void __iomem *base = mv_phy->base;
+
+	clk_prepare_enable(mv_phy->clk);
+
+	/* Set reference clock */
+	writel(0x1 << PHY_28NM_HSIC_PLL_SELLPFR_SHIFT |
+		0xf0 << PHY_28NM_HSIC_PLL_FBDIV_SHIFT |
+		0xd << PHY_28NM_HSIC_PLL_REFDIV_SHIFT,
+		base + PHY_28NM_HSIC_PLL_CTRL01);
+
+	/* Turn on PLL */
+	writel(readl(base + PHY_28NM_HSIC_PLL_CTRL2) |
+		PHY_28NM_HSIC_S2H_PU_PLL,
+		base + PHY_28NM_HSIC_PLL_CTRL2);
+
+	/* Make sure PHY PLL is locked */
+	if (!wait_for_reg(base + PHY_28NM_HSIC_PLL_CTRL2,
+	    PHY_28NM_HSIC_H2S_PLL_LOCK, HZ / 10)) {
+		dev_err(&pdev->dev, "HSIC PHY PLL not locked after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	/* Avoid SE0 state when resume for some device will take it as reset */
+	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~S2H_DRV_SE0_4RESUME,
+		base + PHY_28NM_HSIC_CTRL);
+
+	/* Enable HSIC PHY */
+	writel(readl(base + PHY_28NM_HSIC_CTRL) | PHY_28NM_HSIC_S2H_HSIC_EN,
+		base + PHY_28NM_HSIC_CTRL);
+
+	/*  Calibration Timing
+	*		   ____________________________
+	*  CAL START   ___|
+	*			   ____________________
+	*  CAL_DONE    ___________|
+	*		  | 400us |
+	*/
+
+	/* Make sure PHY Calibration is ready */
+	if (!wait_for_reg(base + PHY_28NM_HSIC_IMPCAL_CAL,
+	    PHY_28NM_HSIC_H2S_IMPCAL_DONE, HZ / 10)) {
+		dev_warn(&pdev->dev, "HSIC PHY READY not set after 100mS.");
+		return -ETIMEDOUT;
+	}
+
+	/* Waiting for HSIC connect int*/
+	if (!wait_for_reg(base + PHY_28NM_HSIC_INT,
+	    PHY_28NM_HSIC_CONNECT_INT, HZ / 5)) {
+		dev_warn(&pdev->dev, "HSIC wait for connect interrupt timeout.");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int mv_hsic_phy_power_on(struct phy *phy)
+{
+	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
+	struct platform_device *pdev = mv_phy->pdev;
+
+	/* check HS ready */
+	if (!wait_for_reg(mv_phy->base + PHY_28NM_HSIC_INT,
+	    PHY_28NM_HSIC_HS_READY_INT, HZ / 10)) {
+		dev_err(&pdev->dev, "HSIC HS_READY not set\n");
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mv_hsic_phy_power_off(struct phy *phy)
+{
+	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
+	void __iomem *base = mv_phy->base;
+
+	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~PHY_28NM_HSIC_S2H_HSIC_EN,
+		base + PHY_28NM_HSIC_CTRL);
+
+	clk_disable_unprepare(mv_phy->clk);
+	return 0;
+}
+
+static const struct phy_ops hsic_ops = {
+	.init		= mv_hsic_phy_init,
+	.power_on	= mv_hsic_phy_power_on,
+	.power_off	= mv_hsic_phy_power_off,
+};
+
+static int mv_hsic_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct mv_hsic_phy *mv_phy;
+	struct resource *r;
+
+	mv_phy = devm_kzalloc(&pdev->dev, sizeof(*mv_phy), GFP_KERNEL);
+	if (!mv_phy)
+		return -ENOMEM;
+
+	mv_phy->pdev = pdev;
+
+	mv_phy->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mv_phy->clk)) {
+		dev_err(&pdev->dev, "failed to get clock.\n");
+		return PTR_ERR(mv_phy->clk);
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mv_phy->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(mv_phy->base))
+		return PTR_ERR(mv_phy->base);
+
+	mv_phy->phy = devm_phy_create(&pdev->dev, pdev->dev.of_node, &hsic_ops);
+	if (IS_ERR(mv_phy->phy))
+		return PTR_ERR(mv_phy->phy);
+
+	phy_set_drvdata(mv_phy->phy, mv_phy);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mv_hsic_phy_dt_match[] = {
+	{ .compatible = "marvell,pxa1928-hsic-phy", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mv_hsic_phy_dt_match);
+
+static struct platform_driver mv_hsic_phy_driver = {
+	.probe	= mv_hsic_phy_probe,
+	.driver = {
+		.name   = "mv-hsic-phy",
+		.of_match_table = of_match_ptr(mv_hsic_phy_dt_match),
+	},
+};
+module_platform_driver(mv_hsic_phy_driver);
+
+MODULE_AUTHOR("Rob Herring <robh@kernel.org>");
+MODULE_DESCRIPTION("Marvell HSIC phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[PATCH 5/5] usb: add pxa1928 ehci support

[Rob Herring]

Add platform driver for Marvell PXA1928 SOC. This is different from the
mv-ehci driver in that it uses the generic phy framework, uses DT, does
not use platform_data, is host only, and has a specific HSIC PHY and
controller initialization handshake.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
---
 drivers/usb/host/Kconfig      |  15 ++-
 drivers/usb/host/Makefile     |   1 +
 drivers/usb/host/ehci-mv-of.c | 243 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 258 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/host/ehci-mv-of.c

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 197a6a3..4d8cfbc 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -251,6 +251,19 @@ config USB_EHCI_MV
 	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
 	  on-chip EHCI USB controller" for those.
 
+config USB_EHCI_MV_OF
+	bool "EHCI OF support for Marvell PXA/MMP USB controller"
+	depends on (ARCH_PXA || ARCH_MMP)
+	select USB_EHCI_ROOT_HUB_TT
+	---help---
+	  Enables support for Marvell (including PXA and MMP series) on-chip
+	  USB SPH and OTG controller. SPH is a single port host, and it can
+	  only be EHCI host. OTG is controller that can switch to host mode.
+	  Note that this driver will not work on Marvell's other EHCI
+	  controller used by the EBU-type SoCs including Orion, Kirkwood,
+	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
+	  on-chip EHCI USB controller" for those.
+
 config USB_W90X900_EHCI
 	tristate "W90X900(W90P910) EHCI support"
 	depends on ARCH_W90X900
@@ -663,7 +676,7 @@ config USB_SL811_HCD
 	help
 	  The SL811HS is a single-port USB controller that supports either
 	  host side or peripheral side roles.  Enable this option if your
-	  board has this chip, and you want to use it as a host controller. 
+	  board has this chip, and you want to use it as a host controller.
 	  If unsure, say N.
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 65b0b6a..5ed93ff 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
 obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
+obj-$(CONFIG_USB_EHCI_MV_OF)	+= ehci-mv-of.o
 obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
 obj-$(CONFIG_USB_EHCI_HCD_OMAP)	+= ehci-omap.o
 obj-$(CONFIG_USB_EHCI_HCD_ORION)	+= ehci-orion.o
diff --git a/drivers/usb/host/ehci-mv-of.c b/drivers/usb/host/ehci-mv-of.c
new file mode 100644
index 0000000..3783299
--- /dev/null
+++ b/drivers/usb/host/ehci-mv-of.c
@@ -0,0 +1,243 @@
+/*
+ * Copyright (C) 2015 Linaro, Ltd.
+ * Rob Herring <robh@kernel.org>
+ *
+ * Based on vendor driver modifications to ehci-mv.c:
+ * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
+ * Author: Chao Xie <chao.xie@marvell.com>
+ *        Neil Zhang <zhangwm@marvell.com>
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/err.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+struct ehci_hcd_mv {
+	struct usb_hcd *hcd;
+	struct phy *phy;
+	struct clk *clk;
+};
+
+#define hcd_to_mv_priv(h) ((struct ehci_hcd_mv *)hcd_to_ehci(h)->priv)
+
+static struct hc_driver ehci_mv_hc_driver;
+
+static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
+{
+	timeout += jiffies;
+	while (time_is_after_eq_jiffies(timeout)) {
+		if ((readl(reg) & mask) == mask)
+			return true;
+		msleep(1);
+	}
+	return false;
+}
+
+static int mv_ehci_enable(struct ehci_hcd_mv *ehci_mv)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(ehci_mv->hcd);
+	struct ehci_regs *ehci_regs = ehci->regs;
+	int retval;
+	u32 status;
+
+	/* enable port power and reserved bit 25 */
+	status = __raw_readl(&ehci_regs->port_status[0]);
+	status |= (PORT_POWER) | (1 << 25);
+	/* Clear bits 30:31 for HSIC to be enabled */
+	status &= ~(0x3 << 30);
+	__raw_writel(status, &ehci_regs->port_status[0]);
+
+	/* test mode: force enable hs */
+	status = __raw_readl(&ehci_regs->port_status[0]);
+	status &= ~(0xf << 16);
+	status |= (0x5 << 16);
+	__raw_writel(status, &ehci_regs->port_status[0]);
+
+	/* disable test mode */
+	status = __raw_readl(&ehci_regs->port_status[0]);
+	status &= ~(0xf << 16);
+	__raw_writel(status, &ehci_regs->port_status[0]);
+
+	retval =  phy_power_on(ehci_mv->hcd->phy);
+	if (retval)
+		return retval;
+
+	/* issue port reset */
+	status = __raw_readl(&ehci_regs->port_status[0]);
+	status |= PORT_RESET;
+	status &= ~PORT_PE;
+	__raw_writel(status, &ehci_regs->port_status[0]);
+
+	/* check reset done */
+	if (!wait_for_reg(&ehci_regs->port_status[0], PORT_RESET, HZ / 10)) {
+		pr_err("HSIC port reset not done: port_status 0x%x\n", status);
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mv_ehci_probe(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd;
+	struct ehci_hcd *ehci;
+	struct ehci_hcd_mv *ehci_mv;
+	struct resource *r;
+	int retval = -ENODEV;
+
+	hcd = usb_create_hcd(&ehci_mv_hc_driver, &pdev->dev, "mv ehci");
+	if (!hcd)
+		return -ENOMEM;
+	ehci = hcd_to_ehci(hcd);
+	ehci_mv = hcd_to_mv_priv(hcd);
+	ehci_mv->hcd = hcd;
+
+	platform_set_drvdata(pdev, ehci_mv);
+
+	ehci_mv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ehci_mv->clk)) {
+		dev_err(&pdev->dev, "error getting clock\n");
+		retval = PTR_ERR(ehci_mv->clk);
+		goto err_put_hcd;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ehci->caps = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(ehci->caps)) {
+		retval = PTR_ERR(ehci->caps);
+		goto err_put_hcd;
+	}
+
+	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
+	if (IS_ERR_OR_NULL(hcd->phy)) {
+		retval = PTR_ERR(hcd->phy);
+		if (retval != -EPROBE_DEFER && retval != -ENODEV)
+			dev_err(&pdev->dev, "failed to get the phy\n");
+		else
+			return -EPROBE_DEFER;
+		goto err_put_hcd;
+	}
+
+	hcd->rsrc_start = r->start;
+	hcd->rsrc_len = resource_size(r);
+
+	hcd->irq = platform_get_irq(pdev, 0);
+	if (!hcd->irq) {
+		dev_err(&pdev->dev, "Cannot get irq.");
+		retval = -ENODEV;
+		goto err_put_hcd;
+	}
+
+	retval = phy_init(hcd->phy);
+	if (retval)
+		goto err_put_hcd;
+
+	clk_prepare(ehci_mv->clk);
+	clk_enable(ehci_mv->clk);
+
+	retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
+	if (retval) {
+		dev_err(&pdev->dev,
+			"failed to add hcd with err %d\n", retval);
+		goto err_disable_clk;
+	}
+
+	retval = mv_ehci_enable(ehci_mv);
+	if (retval) {
+		dev_err(&pdev->dev,
+			"EHCI: private init failed with err %d\n", retval);
+		goto err_disable_clk;
+	}
+
+	device_wakeup_enable(hcd->self.controller);
+
+	return 0;
+
+err_disable_clk:
+	clk_disable(ehci_mv->clk);
+	clk_unprepare(ehci_mv->clk);
+	phy_exit(hcd->phy);
+err_put_hcd:
+	usb_put_hcd(hcd);
+	return retval;
+}
+
+static int mv_ehci_remove(struct platform_device *pdev)
+{
+	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = ehci_mv->hcd;
+
+	phy_power_off(hcd->phy);
+	phy_exit(hcd->phy);
+	clk_disable(ehci_mv->clk);
+	clk_unprepare(ehci_mv->clk);
+
+	usb_remove_hcd(hcd);
+	usb_put_hcd(hcd);
+
+	return 0;
+}
+
+static const struct of_device_id mv_ehci_dt_match[] = {
+	{.compatible = "marvell,pxa1928-ehci"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mv_ehci_dt_match);
+
+static void mv_ehci_shutdown(struct platform_device *pdev)
+{
+	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
+	struct usb_hcd *hcd = ehci_mv->hcd;
+
+	if (!hcd->rh_registered)
+		return;
+
+	if (hcd->driver->shutdown)
+		hcd->driver->shutdown(hcd);
+}
+
+static const struct ehci_driver_overrides ehci_mv_overrides __initconst = {
+	.extra_priv_size =	sizeof(struct ehci_hcd_mv),
+};
+
+static struct platform_driver ehci_mv_driver = {
+	.probe = mv_ehci_probe,
+	.remove = mv_ehci_remove,
+	.shutdown = mv_ehci_shutdown,
+	.driver = {
+		   .name = "mv-ehci-of",
+		   .of_match_table = of_match_ptr(mv_ehci_dt_match),
+		   },
+};
+
+static int __init ehci_mv_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	ehci_init_driver(&ehci_mv_hc_driver, &ehci_mv_overrides);
+	return platform_driver_register(&ehci_mv_driver);
+}
+module_init(ehci_mv_init);
+
+static void __exit ehci_mv_cleanup(void)
+{
+	platform_driver_unregister(&ehci_mv_driver);
+}
+module_exit(ehci_mv_cleanup);
+
+MODULE_AUTHOR("Rob Herring <robh@kernel.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Paul Bolle]

On Wed, 2015-05-13 at 17:49 -0500, Rob Herring wrote:
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig

> +config USB_EHCI_MV_OF
> +	bool "EHCI OF support for Marvell PXA/MMP USB controller"
> +	depends on (ARCH_PXA || ARCH_MMP)
> +	select USB_EHCI_ROOT_HUB_TT
> +	---help---
> +	  Enables support for Marvell (including PXA and MMP series) on-chip
> +	  USB SPH and OTG controller. SPH is a single port host, and it can
> +	  only be EHCI host. OTG is controller that can switch to host mode.
> +	  Note that this driver will not work on Marvell's other EHCI
> +	  controller used by the EBU-type SoCs including Orion, Kirkwood,
> +	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> +	  on-chip EHCI USB controller" for those.

> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile

> +obj-$(CONFIG_USB_EHCI_MV_OF)	+= ehci-mv-of.o

USB_EHCI_MV_OF is a bool symbol so ehci-mv-of.o will never be part of a
module, correct?

> --- /dev/null
> +++ b/drivers/usb/host/ehci-mv-of.c

> +MODULE_DEVICE_TABLE(of, mv_ehci_dt_match);

> +module_init(ehci_mv_init);
> +
> +static void __exit ehci_mv_cleanup(void)
> +{
> +	platform_driver_unregister(&ehci_mv_driver);
> +}
> +module_exit(ehci_mv_cleanup);
> +
> +MODULE_AUTHOR("Rob Herring <robh@kernel.org>");
> +MODULE_LICENSE("GPL v2");

The code contains a few module-specific constructs. (These will be
preprocessed away, replaced with a built-in equivalent, etc.) Was it
your intention to make USB_EHCI_MV_OF tristate?


Paul  Bolle

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Arnd Bergmann]

On Thursday 14 May 2015 10:24:38 Paul Bolle wrote:
> 
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> 
> > +obj-$(CONFIG_USB_EHCI_MV_OF) += ehci-mv-of.o
> 
> USB_EHCI_MV_OF is a bool symbol so ehci-mv-of.o will never be part of a
> module, correct?
> 
> 

I think that's a bug.

	Arnd

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Arnd Bergmann]

On Wednesday 13 May 2015 17:49:00 Rob Herring wrote:
> +
> +	/* enable port power and reserved bit 25 */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status |= (PORT_POWER) | (1 << 25);
> +	/* Clear bits 30:31 for HSIC to be enabled */
> +	status &= ~(0x3 << 30);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* test mode: force enable hs */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	status |= (0x5 << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* disable test mode */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);

Please make this endian-safe.
> +
> +static const struct of_device_id mv_ehci_dt_match[] = {
> +	{.compatible = "marvell,pxa1928-ehci"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mv_ehci_dt_match);

> +static struct platform_driver ehci_mv_driver = {
> +	.probe = mv_ehci_probe,
> +	.remove = mv_ehci_remove,
> +	.shutdown = mv_ehci_shutdown,
> +	.driver = {
> +		   .name = "mv-ehci-of",
> +		   .of_match_table = of_match_ptr(mv_ehci_dt_match),
> +		   },
> +};

Warning: unused symbol 'mv_ehci_dt_match'.

Just remove the of_match_ptr().

	Arnd

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Rob Herring]

On Thu, May 14, 2015 at 5:42 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 14 May 2015 10:24:38 Paul Bolle wrote:
>>
>> > --- a/drivers/usb/host/Makefile
>> > +++ b/drivers/usb/host/Makefile
>>
>> > +obj-$(CONFIG_USB_EHCI_MV_OF) += ehci-mv-of.o
>>
>> USB_EHCI_MV_OF is a bool symbol so ehci-mv-of.o will never be part of a
>> module, correct?
>>
>>
>
> I think that's a bug.

Indeed.

Rob

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Rob Herring]

On Thu, May 14, 2015 at 5:42 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 14 May 2015 10:24:38 Paul Bolle wrote:
>>
>> > --- a/drivers/usb/host/Makefile
>> > +++ b/drivers/usb/host/Makefile
>>
>> > +obj-$(CONFIG_USB_EHCI_MV_OF) += ehci-mv-of.o
>>
>> USB_EHCI_MV_OF is a bool symbol so ehci-mv-of.o will never be part of a
>> module, correct?
>>
>>
>
> I think that's a bug.

Indeed.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Wed, 13 May 2015, Rob Herring wrote:

> Add platform driver for Marvell PXA1928 SOC. This is different from the
> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> not use platform_data, is host only, and has a specific HSIC PHY and
> controller initialization handshake.

Are those differences sufficient to make a separate source file 
necessary?  There are plenty of other drivers that work for both DT and 
non-DT, for example.

> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> ---
>  drivers/usb/host/Kconfig      |  15 ++-
>  drivers/usb/host/Makefile     |   1 +
>  drivers/usb/host/ehci-mv-of.c | 243 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/host/ehci-mv-of.c
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 197a6a3..4d8cfbc 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -251,6 +251,19 @@ config USB_EHCI_MV
>  	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
>  	  on-chip EHCI USB controller" for those.
>  
> +config USB_EHCI_MV_OF
> +	bool "EHCI OF support for Marvell PXA/MMP USB controller"
> +	depends on (ARCH_PXA || ARCH_MMP)
> +	select USB_EHCI_ROOT_HUB_TT
> +	---help---
> +	  Enables support for Marvell (including PXA and MMP series) on-chip
> +	  USB SPH and OTG controller. SPH is a single port host, and it can
> +	  only be EHCI host. OTG is controller that can switch to host mode.
> +	  Note that this driver will not work on Marvell's other EHCI
> +	  controller used by the EBU-type SoCs including Orion, Kirkwood,
> +	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> +	  on-chip EHCI USB controller" for those.

This is identical to the help text for USB_EHCI_MV.  How is a user 
supposed to know which option to enable?

> +
>  config USB_W90X900_EHCI
>  	tristate "W90X900(W90P910) EHCI support"
>  	depends on ARCH_W90X900
> @@ -663,7 +676,7 @@ config USB_SL811_HCD
>  	help
>  	  The SL811HS is a single-port USB controller that supports either
>  	  host side or peripheral side roles.  Enable this option if your
> -	  board has this chip, and you want to use it as a host controller. 
> +	  board has this chip, and you want to use it as a host controller.

This doesn't belong in the patch.

>  	  If unsure, say N.
>  
>  	  To compile this driver as a module, choose M here: the
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 65b0b6a..5ed93ff 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>  obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
>  obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
>  obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
> +obj-$(CONFIG_USB_EHCI_MV_OF)	+= ehci-mv-of.o
>  obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
>  obj-$(CONFIG_USB_EHCI_HCD_OMAP)	+= ehci-omap.o
>  obj-$(CONFIG_USB_EHCI_HCD_ORION)	+= ehci-orion.o
> diff --git a/drivers/usb/host/ehci-mv-of.c b/drivers/usb/host/ehci-mv-of.c
> new file mode 100644
> index 0000000..3783299
> --- /dev/null
> +++ b/drivers/usb/host/ehci-mv-of.c
> @@ -0,0 +1,243 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh@kernel.org>
> + *
> + * Based on vendor driver modifications to ehci-mv.c:
> + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
> + * Author: Chao Xie <chao.xie@marvell.com>
> + *        Neil Zhang <zhangwm@marvell.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/err.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +#include "ehci.h"
> +
> +struct ehci_hcd_mv {
> +	struct usb_hcd *hcd;
> +	struct phy *phy;
> +	struct clk *clk;
> +};

Where does the .phy member get used?

> +
> +#define hcd_to_mv_priv(h) ((struct ehci_hcd_mv *)hcd_to_ehci(h)->priv)
> +
> +static struct hc_driver ehci_mv_hc_driver;
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_ehci_enable(struct ehci_hcd_mv *ehci_mv)
> +{
> +	struct ehci_hcd *ehci = hcd_to_ehci(ehci_mv->hcd);
> +	struct ehci_regs *ehci_regs = ehci->regs;
> +	int retval;
> +	u32 status;
> +
> +	/* enable port power and reserved bit 25 */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status |= (PORT_POWER) | (1 << 25);
> +	/* Clear bits 30:31 for HSIC to be enabled */
> +	status &= ~(0x3 << 30);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* test mode: force enable hs */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	status |= (0x5 << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* disable test mode */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	retval =  phy_power_on(ehci_mv->hcd->phy);
> +	if (retval)
> +		return retval;
> +
> +	/* issue port reset */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status |= PORT_RESET;
> +	status &= ~PORT_PE;
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* check reset done */
> +	if (!wait_for_reg(&ehci_regs->port_status[0], PORT_RESET, HZ / 10)) {
> +		pr_err("HSIC port reset not done: port_status 0x%x\n", status);
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int mv_ehci_probe(struct platform_device *pdev)
> +{
> +	struct usb_hcd *hcd;
> +	struct ehci_hcd *ehci;
> +	struct ehci_hcd_mv *ehci_mv;
> +	struct resource *r;
> +	int retval = -ENODEV;
> +
> +	hcd = usb_create_hcd(&ehci_mv_hc_driver, &pdev->dev, "mv ehci");
> +	if (!hcd)
> +		return -ENOMEM;
> +	ehci = hcd_to_ehci(hcd);
> +	ehci_mv = hcd_to_mv_priv(hcd);
> +	ehci_mv->hcd = hcd;
> +
> +	platform_set_drvdata(pdev, ehci_mv);
> +
> +	ehci_mv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ehci_mv->clk)) {
> +		dev_err(&pdev->dev, "error getting clock\n");
> +		retval = PTR_ERR(ehci_mv->clk);
> +		goto err_put_hcd;
> +	}
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ehci->caps = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(ehci->caps)) {
> +		retval = PTR_ERR(ehci->caps);
> +		goto err_put_hcd;
> +	}
> +
> +	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
> +	if (IS_ERR_OR_NULL(hcd->phy)) {
> +		retval = PTR_ERR(hcd->phy);
> +		if (retval != -EPROBE_DEFER && retval != -ENODEV)
> +			dev_err(&pdev->dev, "failed to get the phy\n");
> +		else
> +			return -EPROBE_DEFER;
> +		goto err_put_hcd;
> +	}
> +
> +	hcd->rsrc_start = r->start;
> +	hcd->rsrc_len = resource_size(r);
> +
> +	hcd->irq = platform_get_irq(pdev, 0);
> +	if (!hcd->irq) {
> +		dev_err(&pdev->dev, "Cannot get irq.");
> +		retval = -ENODEV;
> +		goto err_put_hcd;
> +	}
> +
> +	retval = phy_init(hcd->phy);
> +	if (retval)
> +		goto err_put_hcd;
> +
> +	clk_prepare(ehci_mv->clk);
> +	clk_enable(ehci_mv->clk);
> +
> +	retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> +	if (retval) {
> +		dev_err(&pdev->dev,
> +			"failed to add hcd with err %d\n", retval);
> +		goto err_disable_clk;
> +	}
> +
> +	retval = mv_ehci_enable(ehci_mv);

Is this call in the right place?  I doubt it.  The rest of the system
expects the controller to be running by the time usb_add_hcd() returns.
Probably you ought to put mv_ehci_enable in the ehci_mv_overrides
structure as the .reset entry.

> +	if (retval) {
> +		dev_err(&pdev->dev,
> +			"EHCI: private init failed with err %d\n", retval);
> +		goto err_disable_clk;
> +	}
> +
> +	device_wakeup_enable(hcd->self.controller);
> +
> +	return 0;
> +
> +err_disable_clk:
> +	clk_disable(ehci_mv->clk);
> +	clk_unprepare(ehci_mv->clk);
> +	phy_exit(hcd->phy);
> +err_put_hcd:
> +	usb_put_hcd(hcd);
> +	return retval;
> +}

This doesn't set hcd->has_tt anywhere.  So why bother selecting 
USB_EHCI_ROOT_HUB_TT?

> +
> +static int mv_ehci_remove(struct platform_device *pdev)
> +{
> +	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
> +	struct usb_hcd *hcd = ehci_mv->hcd;
> +
> +	phy_power_off(hcd->phy);
> +	phy_exit(hcd->phy);
> +	clk_disable(ehci_mv->clk);
> +	clk_unprepare(ehci_mv->clk);
> +
> +	usb_remove_hcd(hcd);
> +	usb_put_hcd(hcd);

The remove actions should be carried out in reverse order of the probe
actions.  As it is, this code briefly leaves the controller running but
with the clock turned off.  That doesn't seem like a good idea.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mv_ehci_dt_match[] = {
> +	{.compatible = "marvell,pxa1928-ehci"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mv_ehci_dt_match);
> +
> +static void mv_ehci_shutdown(struct platform_device *pdev)
> +{
> +	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
> +	struct usb_hcd *hcd = ehci_mv->hcd;
> +
> +	if (!hcd->rh_registered)
> +		return;
> +
> +	if (hcd->driver->shutdown)
> +		hcd->driver->shutdown(hcd);
> +}

This is kind of strange.  It's the same as usb_hcd_platform_shutdown()  
except for the hcd->rh_registered test.  Is there some reason why that
test is needed here but not in the generic routine?  If not, can the
test be removed?  Or should it be added to the generic routine?

> +
> +static const struct ehci_driver_overrides ehci_mv_overrides __initconst = {
> +	.extra_priv_size =	sizeof(struct ehci_hcd_mv),
> +};
> +
> +static struct platform_driver ehci_mv_driver = {
> +	.probe = mv_ehci_probe,
> +	.remove = mv_ehci_remove,
> +	.shutdown = mv_ehci_shutdown,
> +	.driver = {
> +		   .name = "mv-ehci-of",
> +		   .of_match_table = of_match_ptr(mv_ehci_dt_match),
> +		   },
> +};
> +
> +static int __init ehci_mv_init(void)
> +{
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	ehci_init_driver(&ehci_mv_hc_driver, &ehci_mv_overrides);
> +	return platform_driver_register(&ehci_mv_driver);
> +}
> +module_init(ehci_mv_init);
> +
> +static void __exit ehci_mv_cleanup(void)
> +{
> +	platform_driver_unregister(&ehci_mv_driver);
> +}
> +module_exit(ehci_mv_cleanup);
> +
> +MODULE_AUTHOR("Rob Herring <robh@kernel.org>");
> +MODULE_LICENSE("GPL v2");

Alan Stern

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Wed, 13 May 2015, Rob Herring wrote:

> Add platform driver for Marvell PXA1928 SOC. This is different from the
> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> not use platform_data, is host only, and has a specific HSIC PHY and
> controller initialization handshake.

Are those differences sufficient to make a separate source file 
necessary?  There are plenty of other drivers that work for both DT and 
non-DT, for example.

> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/usb/host/Kconfig      |  15 ++-
>  drivers/usb/host/Makefile     |   1 +
>  drivers/usb/host/ehci-mv-of.c | 243 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 258 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/host/ehci-mv-of.c
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 197a6a3..4d8cfbc 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -251,6 +251,19 @@ config USB_EHCI_MV
>  	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
>  	  on-chip EHCI USB controller" for those.
>  
> +config USB_EHCI_MV_OF
> +	bool "EHCI OF support for Marvell PXA/MMP USB controller"
> +	depends on (ARCH_PXA || ARCH_MMP)
> +	select USB_EHCI_ROOT_HUB_TT
> +	---help---
> +	  Enables support for Marvell (including PXA and MMP series) on-chip
> +	  USB SPH and OTG controller. SPH is a single port host, and it can
> +	  only be EHCI host. OTG is controller that can switch to host mode.
> +	  Note that this driver will not work on Marvell's other EHCI
> +	  controller used by the EBU-type SoCs including Orion, Kirkwood,
> +	  Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> +	  on-chip EHCI USB controller" for those.

This is identical to the help text for USB_EHCI_MV.  How is a user 
supposed to know which option to enable?

> +
>  config USB_W90X900_EHCI
>  	tristate "W90X900(W90P910) EHCI support"
>  	depends on ARCH_W90X900
> @@ -663,7 +676,7 @@ config USB_SL811_HCD
>  	help
>  	  The SL811HS is a single-port USB controller that supports either
>  	  host side or peripheral side roles.  Enable this option if your
> -	  board has this chip, and you want to use it as a host controller. 
> +	  board has this chip, and you want to use it as a host controller.

This doesn't belong in the patch.

>  	  If unsure, say N.
>  
>  	  To compile this driver as a module, choose M here: the
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 65b0b6a..5ed93ff 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>  obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
>  obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
>  obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
> +obj-$(CONFIG_USB_EHCI_MV_OF)	+= ehci-mv-of.o
>  obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
>  obj-$(CONFIG_USB_EHCI_HCD_OMAP)	+= ehci-omap.o
>  obj-$(CONFIG_USB_EHCI_HCD_ORION)	+= ehci-orion.o
> diff --git a/drivers/usb/host/ehci-mv-of.c b/drivers/usb/host/ehci-mv-of.c
> new file mode 100644
> index 0000000..3783299
> --- /dev/null
> +++ b/drivers/usb/host/ehci-mv-of.c
> @@ -0,0 +1,243 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> + *
> + * Based on vendor driver modifications to ehci-mv.c:
> + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
> + * Author: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + *        Neil Zhang <zhangwm-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/err.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +#include "ehci.h"
> +
> +struct ehci_hcd_mv {
> +	struct usb_hcd *hcd;
> +	struct phy *phy;
> +	struct clk *clk;
> +};

Where does the .phy member get used?

> +
> +#define hcd_to_mv_priv(h) ((struct ehci_hcd_mv *)hcd_to_ehci(h)->priv)
> +
> +static struct hc_driver ehci_mv_hc_driver;
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_ehci_enable(struct ehci_hcd_mv *ehci_mv)
> +{
> +	struct ehci_hcd *ehci = hcd_to_ehci(ehci_mv->hcd);
> +	struct ehci_regs *ehci_regs = ehci->regs;
> +	int retval;
> +	u32 status;
> +
> +	/* enable port power and reserved bit 25 */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status |= (PORT_POWER) | (1 << 25);
> +	/* Clear bits 30:31 for HSIC to be enabled */
> +	status &= ~(0x3 << 30);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* test mode: force enable hs */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	status |= (0x5 << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* disable test mode */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status &= ~(0xf << 16);
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	retval =  phy_power_on(ehci_mv->hcd->phy);
> +	if (retval)
> +		return retval;
> +
> +	/* issue port reset */
> +	status = __raw_readl(&ehci_regs->port_status[0]);
> +	status |= PORT_RESET;
> +	status &= ~PORT_PE;
> +	__raw_writel(status, &ehci_regs->port_status[0]);
> +
> +	/* check reset done */
> +	if (!wait_for_reg(&ehci_regs->port_status[0], PORT_RESET, HZ / 10)) {
> +		pr_err("HSIC port reset not done: port_status 0x%x\n", status);
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int mv_ehci_probe(struct platform_device *pdev)
> +{
> +	struct usb_hcd *hcd;
> +	struct ehci_hcd *ehci;
> +	struct ehci_hcd_mv *ehci_mv;
> +	struct resource *r;
> +	int retval = -ENODEV;
> +
> +	hcd = usb_create_hcd(&ehci_mv_hc_driver, &pdev->dev, "mv ehci");
> +	if (!hcd)
> +		return -ENOMEM;
> +	ehci = hcd_to_ehci(hcd);
> +	ehci_mv = hcd_to_mv_priv(hcd);
> +	ehci_mv->hcd = hcd;
> +
> +	platform_set_drvdata(pdev, ehci_mv);
> +
> +	ehci_mv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ehci_mv->clk)) {
> +		dev_err(&pdev->dev, "error getting clock\n");
> +		retval = PTR_ERR(ehci_mv->clk);
> +		goto err_put_hcd;
> +	}
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ehci->caps = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(ehci->caps)) {
> +		retval = PTR_ERR(ehci->caps);
> +		goto err_put_hcd;
> +	}
> +
> +	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
> +	if (IS_ERR_OR_NULL(hcd->phy)) {
> +		retval = PTR_ERR(hcd->phy);
> +		if (retval != -EPROBE_DEFER && retval != -ENODEV)
> +			dev_err(&pdev->dev, "failed to get the phy\n");
> +		else
> +			return -EPROBE_DEFER;
> +		goto err_put_hcd;
> +	}
> +
> +	hcd->rsrc_start = r->start;
> +	hcd->rsrc_len = resource_size(r);
> +
> +	hcd->irq = platform_get_irq(pdev, 0);
> +	if (!hcd->irq) {
> +		dev_err(&pdev->dev, "Cannot get irq.");
> +		retval = -ENODEV;
> +		goto err_put_hcd;
> +	}
> +
> +	retval = phy_init(hcd->phy);
> +	if (retval)
> +		goto err_put_hcd;
> +
> +	clk_prepare(ehci_mv->clk);
> +	clk_enable(ehci_mv->clk);
> +
> +	retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> +	if (retval) {
> +		dev_err(&pdev->dev,
> +			"failed to add hcd with err %d\n", retval);
> +		goto err_disable_clk;
> +	}
> +
> +	retval = mv_ehci_enable(ehci_mv);

Is this call in the right place?  I doubt it.  The rest of the system
expects the controller to be running by the time usb_add_hcd() returns.
Probably you ought to put mv_ehci_enable in the ehci_mv_overrides
structure as the .reset entry.

> +	if (retval) {
> +		dev_err(&pdev->dev,
> +			"EHCI: private init failed with err %d\n", retval);
> +		goto err_disable_clk;
> +	}
> +
> +	device_wakeup_enable(hcd->self.controller);
> +
> +	return 0;
> +
> +err_disable_clk:
> +	clk_disable(ehci_mv->clk);
> +	clk_unprepare(ehci_mv->clk);
> +	phy_exit(hcd->phy);
> +err_put_hcd:
> +	usb_put_hcd(hcd);
> +	return retval;
> +}

This doesn't set hcd->has_tt anywhere.  So why bother selecting 
USB_EHCI_ROOT_HUB_TT?

> +
> +static int mv_ehci_remove(struct platform_device *pdev)
> +{
> +	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
> +	struct usb_hcd *hcd = ehci_mv->hcd;
> +
> +	phy_power_off(hcd->phy);
> +	phy_exit(hcd->phy);
> +	clk_disable(ehci_mv->clk);
> +	clk_unprepare(ehci_mv->clk);
> +
> +	usb_remove_hcd(hcd);
> +	usb_put_hcd(hcd);

The remove actions should be carried out in reverse order of the probe
actions.  As it is, this code briefly leaves the controller running but
with the clock turned off.  That doesn't seem like a good idea.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mv_ehci_dt_match[] = {
> +	{.compatible = "marvell,pxa1928-ehci"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mv_ehci_dt_match);
> +
> +static void mv_ehci_shutdown(struct platform_device *pdev)
> +{
> +	struct ehci_hcd_mv *ehci_mv = platform_get_drvdata(pdev);
> +	struct usb_hcd *hcd = ehci_mv->hcd;
> +
> +	if (!hcd->rh_registered)
> +		return;
> +
> +	if (hcd->driver->shutdown)
> +		hcd->driver->shutdown(hcd);
> +}

This is kind of strange.  It's the same as usb_hcd_platform_shutdown()  
except for the hcd->rh_registered test.  Is there some reason why that
test is needed here but not in the generic routine?  If not, can the
test be removed?  Or should it be added to the generic routine?

> +
> +static const struct ehci_driver_overrides ehci_mv_overrides __initconst = {
> +	.extra_priv_size =	sizeof(struct ehci_hcd_mv),
> +};
> +
> +static struct platform_driver ehci_mv_driver = {
> +	.probe = mv_ehci_probe,
> +	.remove = mv_ehci_remove,
> +	.shutdown = mv_ehci_shutdown,
> +	.driver = {
> +		   .name = "mv-ehci-of",
> +		   .of_match_table = of_match_ptr(mv_ehci_dt_match),
> +		   },
> +};
> +
> +static int __init ehci_mv_init(void)
> +{
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	ehci_init_driver(&ehci_mv_hc_driver, &ehci_mv_overrides);
> +	return platform_driver_register(&ehci_mv_driver);
> +}
> +module_init(ehci_mv_init);
> +
> +static void __exit ehci_mv_cleanup(void)
> +{
> +	platform_driver_unregister(&ehci_mv_driver);
> +}
> +module_exit(ehci_mv_cleanup);
> +
> +MODULE_AUTHOR("Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Wed, 13 May 2015, Rob Herring wrote:

> Add platform driver for Marvell PXA1928 SOC. This is different from the
> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> not use platform_data, is host only, and has a specific HSIC PHY and
> controller initialization handshake.

One more thing, which I had intended to include in my earlier review 
but then forgot about...

> +static int mv_ehci_probe(struct platform_device *pdev)
> +{

...

> +	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
> +	if (IS_ERR_OR_NULL(hcd->phy)) {
> +		retval = PTR_ERR(hcd->phy);
> +		if (retval != -EPROBE_DEFER && retval != -ENODEV)
> +			dev_err(&pdev->dev, "failed to get the phy\n");
> +		else
> +			return -EPROBE_DEFER;
> +		goto err_put_hcd;
> +	}

Please straighten out this convoluted logic.  It should go like this:

	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
	if (IS_ERR_OR_NULL(hcd->phy)) {
		retval = PTR_ERR(hcd->phy);
		if (retval == -EPROBE_DEFER || retval == -ENODEV)
			return -EPROBE_DEFER;
		dev_err(&pdev->dev, "failed to get the phy\n");
		goto err_put_hcd;
	}

Alan Stern

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Wed, 13 May 2015, Rob Herring wrote:

> Add platform driver for Marvell PXA1928 SOC. This is different from the
> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> not use platform_data, is host only, and has a specific HSIC PHY and
> controller initialization handshake.

One more thing, which I had intended to include in my earlier review 
but then forgot about...

> +static int mv_ehci_probe(struct platform_device *pdev)
> +{

...

> +	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
> +	if (IS_ERR_OR_NULL(hcd->phy)) {
> +		retval = PTR_ERR(hcd->phy);
> +		if (retval != -EPROBE_DEFER && retval != -ENODEV)
> +			dev_err(&pdev->dev, "failed to get the phy\n");
> +		else
> +			return -EPROBE_DEFER;
> +		goto err_put_hcd;
> +	}

Please straighten out this convoluted logic.  It should go like this:

	hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
	if (IS_ERR_OR_NULL(hcd->phy)) {
		retval = PTR_ERR(hcd->phy);
		if (retval == -EPROBE_DEFER || retval == -ENODEV)
			return -EPROBE_DEFER;
		dev_err(&pdev->dev, "failed to get the phy\n");
		goto err_put_hcd;
	}

Alan Stern

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Rob Herring]

On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 13 May 2015, Rob Herring wrote:
>
>> Add platform driver for Marvell PXA1928 SOC. This is different from the
>> mv-ehci driver in that it uses the generic phy framework, uses DT, does
>> not use platform_data, is host only, and has a specific HSIC PHY and
>> controller initialization handshake.
>
> Are those differences sufficient to make a separate source file
> necessary?  There are plenty of other drivers that work for both DT and
> non-DT, for example.

IMO, yes. If it were only DT support, then no it would not make sense.
I don't disagree there are too many ehci drivers mostly varying in phy
control.

Actually, ehci-platform is a closer match to this driver. I could use
it perhaps and either add my custom power_on function and a match
table entry to it or export ehci-platform functions (probe,
power_{on,off}, and use them here. Opinion on which direction you
prefer?

There's also a resume issue I have to figure out how support too. This
is what the vendor driver does:

@@ -469,6 +470,40 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
                        set_bit(i, &resume_needed);
                }
                ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
+
+               /* for HSIC phy, we have already disable SE0 state when
+                * resume back. we need do following step to pull USB
+                * controller back. */
+#ifdef CONFIG_USB_EHCI_MV_U2H_HSIC
+#define PHY_TEST_GRP_0 0x10
+
+#define S2H_TEST_UTMI_SEL (0x1 << 7)
+#define S2H_TEST_SUSPENDM (0x1 << 14)
+#define S2H_TEST_TX_BITSTUFF_EN (0x1 << 29)
+
+               if (hcd->usb_phy->is_hsic) {
+                       void __iomem *phy_base = hcd->usb_phy->io_priv;
+                       unsigned int count;
+                       mdelay(10);
+
+                       temp = readl(phy_base + PHY_TEST_GRP_0);
+                       writel(temp | (S2H_TEST_UTMI_SEL
+                                       | S2H_TEST_SUSPENDM
+                                       | S2H_TEST_TX_BITSTUFF_EN),
+                                       phy_base + PHY_TEST_GRP_0);
+
+                       count = 1000;
+                       while (--count && (ehci_readl(ehci,
+                               &ehci->regs->port_status[i]) & PORT_RESUME))
+                               udelay(50);
+                       if (count <= 0)
+                               ehci_dbg(ehci, "resume time out\n");
+                       writel(readl(phy_base + PHY_TEST_GRP_0)
+                               & ~S2H_TEST_UTMI_SEL,
+                               phy_base + PHY_TEST_GRP_0);
+               }
+#endif
+
        }

        /*


>> +config USB_EHCI_MV_OF
>> +     bool "EHCI OF support for Marvell PXA/MMP USB controller"
>> +     depends on (ARCH_PXA || ARCH_MMP)
>> +     select USB_EHCI_ROOT_HUB_TT
>> +     ---help---
>> +       Enables support for Marvell (including PXA and MMP series) on-chip
>> +       USB SPH and OTG controller. SPH is a single port host, and it can
>> +       only be EHCI host. OTG is controller that can switch to host mode.
>> +       Note that this driver will not work on Marvell's other EHCI
>> +       controller used by the EBU-type SoCs including Orion, Kirkwood,
>> +       Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
>> +       on-chip EHCI USB controller" for those.
>
> This is identical to the help text for USB_EHCI_MV.  How is a user
> supposed to know which option to enable?

The OTG part needs to go. Perhaps I need to make it PXA1928 specific.
All I know about Marvell IP is it is scattered all over the place, so
determining which SOCs are the same IP is difficult. The PHYs for sure
seem to be different on every chip as I did not find any existing
Marvell phy drivers that match.

Rob

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Rob Herring]

On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 13 May 2015, Rob Herring wrote:
>
>> Add platform driver for Marvell PXA1928 SOC. This is different from the
>> mv-ehci driver in that it uses the generic phy framework, uses DT, does
>> not use platform_data, is host only, and has a specific HSIC PHY and
>> controller initialization handshake.
>
> Are those differences sufficient to make a separate source file
> necessary?  There are plenty of other drivers that work for both DT and
> non-DT, for example.

IMO, yes. If it were only DT support, then no it would not make sense.
I don't disagree there are too many ehci drivers mostly varying in phy
control.

Actually, ehci-platform is a closer match to this driver. I could use
it perhaps and either add my custom power_on function and a match
table entry to it or export ehci-platform functions (probe,
power_{on,off}, and use them here. Opinion on which direction you
prefer?

There's also a resume issue I have to figure out how support too. This
is what the vendor driver does:

@@ -469,6 +470,40 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
                        set_bit(i, &resume_needed);
                }
                ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
+
+               /* for HSIC phy, we have already disable SE0 state when
+                * resume back. we need do following step to pull USB
+                * controller back. */
+#ifdef CONFIG_USB_EHCI_MV_U2H_HSIC
+#define PHY_TEST_GRP_0 0x10
+
+#define S2H_TEST_UTMI_SEL (0x1 << 7)
+#define S2H_TEST_SUSPENDM (0x1 << 14)
+#define S2H_TEST_TX_BITSTUFF_EN (0x1 << 29)
+
+               if (hcd->usb_phy->is_hsic) {
+                       void __iomem *phy_base = hcd->usb_phy->io_priv;
+                       unsigned int count;
+                       mdelay(10);
+
+                       temp = readl(phy_base + PHY_TEST_GRP_0);
+                       writel(temp | (S2H_TEST_UTMI_SEL
+                                       | S2H_TEST_SUSPENDM
+                                       | S2H_TEST_TX_BITSTUFF_EN),
+                                       phy_base + PHY_TEST_GRP_0);
+
+                       count = 1000;
+                       while (--count && (ehci_readl(ehci,
+                               &ehci->regs->port_status[i]) & PORT_RESUME))
+                               udelay(50);
+                       if (count <= 0)
+                               ehci_dbg(ehci, "resume time out\n");
+                       writel(readl(phy_base + PHY_TEST_GRP_0)
+                               & ~S2H_TEST_UTMI_SEL,
+                               phy_base + PHY_TEST_GRP_0);
+               }
+#endif
+
        }

        /*


>> +config USB_EHCI_MV_OF
>> +     bool "EHCI OF support for Marvell PXA/MMP USB controller"
>> +     depends on (ARCH_PXA || ARCH_MMP)
>> +     select USB_EHCI_ROOT_HUB_TT
>> +     ---help---
>> +       Enables support for Marvell (including PXA and MMP series) on-chip
>> +       USB SPH and OTG controller. SPH is a single port host, and it can
>> +       only be EHCI host. OTG is controller that can switch to host mode.
>> +       Note that this driver will not work on Marvell's other EHCI
>> +       controller used by the EBU-type SoCs including Orion, Kirkwood,
>> +       Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
>> +       on-chip EHCI USB controller" for those.
>
> This is identical to the help text for USB_EHCI_MV.  How is a user
> supposed to know which option to enable?

The OTG part needs to go. Perhaps I need to make it PXA1928 specific.
All I know about Marvell IP is it is scattered all over the place, so
determining which SOCs are the same IP is difficult. The PHYs for sure
seem to be different on every chip as I did not find any existing
Marvell phy drivers that match.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Thu, 14 May 2015, Rob Herring wrote:

> On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 13 May 2015, Rob Herring wrote:
> >
> >> Add platform driver for Marvell PXA1928 SOC. This is different from the
> >> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> >> not use platform_data, is host only, and has a specific HSIC PHY and
> >> controller initialization handshake.
> >
> > Are those differences sufficient to make a separate source file
> > necessary?  There are plenty of other drivers that work for both DT and
> > non-DT, for example.
> 
> IMO, yes. If it were only DT support, then no it would not make sense.
> I don't disagree there are too many ehci drivers mostly varying in phy
> control.
> 
> Actually, ehci-platform is a closer match to this driver. I could use
> it perhaps and either add my custom power_on function and a match
> table entry to it or export ehci-platform functions (probe,
> power_{on,off}, and use them here. Opinion on which direction you
> prefer?

If you can use ehci-platform in place of a more specific driver, that 
would be great.  Custom power-on, power-suspend, and power-off routines 
aren't a problem.

> There's also a resume issue I have to figure out how support too. This
> is what the vendor driver does:
> 
> @@ -469,6 +470,40 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
>                         set_bit(i, &resume_needed);
>                 }
>                 ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
> +
> +               /* for HSIC phy, we have already disable SE0 state when
> +                * resume back. we need do following step to pull USB
> +                * controller back. */
> +#ifdef CONFIG_USB_EHCI_MV_U2H_HSIC
> +#define PHY_TEST_GRP_0 0x10
> +
> +#define S2H_TEST_UTMI_SEL (0x1 << 7)
> +#define S2H_TEST_SUSPENDM (0x1 << 14)
> +#define S2H_TEST_TX_BITSTUFF_EN (0x1 << 29)
> +
> +               if (hcd->usb_phy->is_hsic) {
> +                       void __iomem *phy_base = hcd->usb_phy->io_priv;
> +                       unsigned int count;
> +                       mdelay(10);
> +
> +                       temp = readl(phy_base + PHY_TEST_GRP_0);
> +                       writel(temp | (S2H_TEST_UTMI_SEL
> +                                       | S2H_TEST_SUSPENDM
> +                                       | S2H_TEST_TX_BITSTUFF_EN),
> +                                       phy_base + PHY_TEST_GRP_0);
> +
> +                       count = 1000;
> +                       while (--count && (ehci_readl(ehci,
> +                               &ehci->regs->port_status[i]) & PORT_RESUME))
> +                               udelay(50);
> +                       if (count <= 0)
> +                               ehci_dbg(ehci, "resume time out\n");
> +                       writel(readl(phy_base + PHY_TEST_GRP_0)
> +                               & ~S2H_TEST_UTMI_SEL,
> +                               phy_base + PHY_TEST_GRP_0);
> +               }
> +#endif

It's not entirely clear why this is needed.  For example, why wait for 
PORT_RESUME to turn off if you never turned it on in the first place?

Anyway, it looks like this won't be so easy to integrate with 
ehci-platform.  Perhaps this could be done in the custom power-on 
routine; I don't know if that's feasible.

> >> +config USB_EHCI_MV_OF
> >> +     bool "EHCI OF support for Marvell PXA/MMP USB controller"
> >> +     depends on (ARCH_PXA || ARCH_MMP)
> >> +     select USB_EHCI_ROOT_HUB_TT
> >> +     ---help---
> >> +       Enables support for Marvell (including PXA and MMP series) on-chip
> >> +       USB SPH and OTG controller. SPH is a single port host, and it can
> >> +       only be EHCI host. OTG is controller that can switch to host mode.
> >> +       Note that this driver will not work on Marvell's other EHCI
> >> +       controller used by the EBU-type SoCs including Orion, Kirkwood,
> >> +       Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> >> +       on-chip EHCI USB controller" for those.
> >
> > This is identical to the help text for USB_EHCI_MV.  How is a user
> > supposed to know which option to enable?
> 
> The OTG part needs to go. Perhaps I need to make it PXA1928 specific.
> All I know about Marvell IP is it is scattered all over the place, so
> determining which SOCs are the same IP is difficult. The PHYs for sure
> seem to be different on every chip as I did not find any existing
> Marvell phy drivers that match.

Obviously this is the kind of thing that a developer is supposed to
sort out, rather then making users responsible for finding the right
combination of settings for their hardware.

Alan Stern

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Alan Stern]

On Thu, 14 May 2015, Rob Herring wrote:

> On Thu, May 14, 2015 at 9:56 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > On Wed, 13 May 2015, Rob Herring wrote:
> >
> >> Add platform driver for Marvell PXA1928 SOC. This is different from the
> >> mv-ehci driver in that it uses the generic phy framework, uses DT, does
> >> not use platform_data, is host only, and has a specific HSIC PHY and
> >> controller initialization handshake.
> >
> > Are those differences sufficient to make a separate source file
> > necessary?  There are plenty of other drivers that work for both DT and
> > non-DT, for example.
> 
> IMO, yes. If it were only DT support, then no it would not make sense.
> I don't disagree there are too many ehci drivers mostly varying in phy
> control.
> 
> Actually, ehci-platform is a closer match to this driver. I could use
> it perhaps and either add my custom power_on function and a match
> table entry to it or export ehci-platform functions (probe,
> power_{on,off}, and use them here. Opinion on which direction you
> prefer?

If you can use ehci-platform in place of a more specific driver, that 
would be great.  Custom power-on, power-suspend, and power-off routines 
aren't a problem.

> There's also a resume issue I have to figure out how support too. This
> is what the vendor driver does:
> 
> @@ -469,6 +470,40 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
>                         set_bit(i, &resume_needed);
>                 }
>                 ehci_writel(ehci, temp, &ehci->regs->port_status [i]);
> +
> +               /* for HSIC phy, we have already disable SE0 state when
> +                * resume back. we need do following step to pull USB
> +                * controller back. */
> +#ifdef CONFIG_USB_EHCI_MV_U2H_HSIC
> +#define PHY_TEST_GRP_0 0x10
> +
> +#define S2H_TEST_UTMI_SEL (0x1 << 7)
> +#define S2H_TEST_SUSPENDM (0x1 << 14)
> +#define S2H_TEST_TX_BITSTUFF_EN (0x1 << 29)
> +
> +               if (hcd->usb_phy->is_hsic) {
> +                       void __iomem *phy_base = hcd->usb_phy->io_priv;
> +                       unsigned int count;
> +                       mdelay(10);
> +
> +                       temp = readl(phy_base + PHY_TEST_GRP_0);
> +                       writel(temp | (S2H_TEST_UTMI_SEL
> +                                       | S2H_TEST_SUSPENDM
> +                                       | S2H_TEST_TX_BITSTUFF_EN),
> +                                       phy_base + PHY_TEST_GRP_0);
> +
> +                       count = 1000;
> +                       while (--count && (ehci_readl(ehci,
> +                               &ehci->regs->port_status[i]) & PORT_RESUME))
> +                               udelay(50);
> +                       if (count <= 0)
> +                               ehci_dbg(ehci, "resume time out\n");
> +                       writel(readl(phy_base + PHY_TEST_GRP_0)
> +                               & ~S2H_TEST_UTMI_SEL,
> +                               phy_base + PHY_TEST_GRP_0);
> +               }
> +#endif

It's not entirely clear why this is needed.  For example, why wait for 
PORT_RESUME to turn off if you never turned it on in the first place?

Anyway, it looks like this won't be so easy to integrate with 
ehci-platform.  Perhaps this could be done in the custom power-on 
routine; I don't know if that's feasible.

> >> +config USB_EHCI_MV_OF
> >> +     bool "EHCI OF support for Marvell PXA/MMP USB controller"
> >> +     depends on (ARCH_PXA || ARCH_MMP)
> >> +     select USB_EHCI_ROOT_HUB_TT
> >> +     ---help---
> >> +       Enables support for Marvell (including PXA and MMP series) on-chip
> >> +       USB SPH and OTG controller. SPH is a single port host, and it can
> >> +       only be EHCI host. OTG is controller that can switch to host mode.
> >> +       Note that this driver will not work on Marvell's other EHCI
> >> +       controller used by the EBU-type SoCs including Orion, Kirkwood,
> >> +       Dova, Armada 370 and Armada XP. See "Support for Marvell EBU
> >> +       on-chip EHCI USB controller" for those.
> >
> > This is identical to the help text for USB_EHCI_MV.  How is a user
> > supposed to know which option to enable?
> 
> The OTG part needs to go. Perhaps I need to make it PXA1928 specific.
> All I know about Marvell IP is it is scattered all over the place, so
> determining which SOCs are the same IP is difficult. The PHYs for sure
> seem to be different on every chip as I did not find any existing
> Marvell phy drivers that match.

Obviously this is the kind of thing that a developer is supposed to
sort out, rather then making users responsible for finding the right
combination of settings for their hardware.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: add pxa1928 ehci support

[Arnd Bergmann]

On Thursday 14 May 2015 11:55:36 Alan Stern wrote:
> 
> > +     hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
> > +     if (IS_ERR_OR_NULL(hcd->phy)) {
> > +             retval = PTR_ERR(hcd->phy);
> > +             if (retval != -EPROBE_DEFER && retval != -ENODEV)
> > +                     dev_err(&pdev->dev, "failed to get the phy\n");
> > +             else
> > +                     return -EPROBE_DEFER;
> > +             goto err_put_hcd;
> > +     }
> 
> Please straighten out this convoluted logic.  It should go like this:
> 
>         hcd->phy = devm_of_phy_get(&pdev->dev, pdev->dev.of_node, NULL);
>         if (IS_ERR_OR_NULL(hcd->phy)) {
>                 retval = PTR_ERR(hcd->phy);
>                 if (retval == -EPROBE_DEFER || retval == -ENODEV)
>                         return -EPROBE_DEFER;
>                 dev_err(&pdev->dev, "failed to get the phy\n");
>                 goto err_put_hcd;
>         }
> 

IS_ERR_OR_NULL is almost always wrong. Kernel interfaces that can return
an error pointer should never return a NULL pointer, and I'm pretty sure
that is also the case for the phy subsystem (or else it should be fixed).

In some cases, we have interfaces that return NULL pointers or error pointers,
but those are designed to treat NULL as success.

	Arnd

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Sebastian Hesselbarth]

On 14.05.2015 00:48, Rob Herring wrote:
> This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928
> SOC.
>
> The OTG block is based on ChipIdea and works with "chipidea,usb2"
> compatible driver as is just by adding the PHY driver. Yay!
>
> Rob
>
> Rob Herring (5):
>    dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
>    dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
>    phy: Add Marvell USB 2.0 OTG 28nm PHY
>    phy: add Marvell HSIC 28nm PHY
>    usb: add pxa1928 ehci support
>
>   .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
>   .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
>   drivers/phy/Kconfig                                |  20 ++
>   drivers/phy/Makefile                               |   2 +
>   drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
>   drivers/phy/phy-mv-usb2.c                          | 329 +++++++++++++++++++++

[Adding some MVEBU guys]

Rob,

I had a look at the USB PHYs of some of the other Marvell SoCs a while
ago for the barebox bootloader [1]. Marvell seems to distinguish the
USB PHY type by technology node, e.g. 28nm like the one above. For the
most used Marvell SoCs, i.e. Kirkwood, Dove, and Armada 370/XP, they all
use a different technology node and we could either use the SoC name
or the technology node as compatible.

Anyway, if you are introducing new PHY drivers with _that_ generic
names, it will either clash with every other Marvell USB PHYs -
or we'll have to add the PHY code into the drivers above.

[1] http://lists.infradead.org/pipermail/barebox/2014-June/019600.html

>   drivers/usb/host/Kconfig                           |  15 +-
>   drivers/usb/host/Makefile                          |   1 +
>   drivers/usb/host/ehci-mv-of.c                      | 243 +++++++++++++++
>   9 files changed, 854 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
>   create mode 100644 Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
>   create mode 100644 drivers/phy/phy-mv-hsic.c
>   create mode 100644 drivers/phy/phy-mv-usb2.c
>   create mode 100644 drivers/usb/host/ehci-mv-of.c
>

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Sebastian Hesselbarth]

On 14.05.2015 00:48, Rob Herring wrote:
> This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928
> SOC.
>
> The OTG block is based on ChipIdea and works with "chipidea,usb2"
> compatible driver as is just by adding the PHY driver. Yay!
>
> Rob
>
> Rob Herring (5):
>    dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
>    dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
>    phy: Add Marvell USB 2.0 OTG 28nm PHY
>    phy: add Marvell HSIC 28nm PHY
>    usb: add pxa1928 ehci support
>
>   .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
>   .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
>   drivers/phy/Kconfig                                |  20 ++
>   drivers/phy/Makefile                               |   2 +
>   drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
>   drivers/phy/phy-mv-usb2.c                          | 329 +++++++++++++++++++++

[Adding some MVEBU guys]

Rob,

I had a look at the USB PHYs of some of the other Marvell SoCs a while
ago for the barebox bootloader [1]. Marvell seems to distinguish the
USB PHY type by technology node, e.g. 28nm like the one above. For the
most used Marvell SoCs, i.e. Kirkwood, Dove, and Armada 370/XP, they all
use a different technology node and we could either use the SoC name
or the technology node as compatible.

Anyway, if you are introducing new PHY drivers with _that_ generic
names, it will either clash with every other Marvell USB PHYs -
or we'll have to add the PHY code into the drivers above.

[1] http://lists.infradead.org/pipermail/barebox/2014-June/019600.html

>   drivers/usb/host/Kconfig                           |  15 +-
>   drivers/usb/host/Makefile                          |   1 +
>   drivers/usb/host/ehci-mv-of.c                      | 243 +++++++++++++++
>   9 files changed, 854 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/devicetree/bindings/phy/pxa1928-usb-phy.txt
>   create mode 100644 Documentation/devicetree/bindings/usb/ehci-pxa1928.txt
>   create mode 100644 drivers/phy/phy-mv-hsic.c
>   create mode 100644 drivers/phy/phy-mv-usb2.c
>   create mode 100644 drivers/usb/host/ehci-mv-of.c
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

On Fri, May 15, 2015 at 4:55 AM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 14.05.2015 00:48, Rob Herring wrote:
>>
>> This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928
>> SOC.
>>
>> The OTG block is based on ChipIdea and works with "chipidea,usb2"
>> compatible driver as is just by adding the PHY driver. Yay!
>>
>> Rob
>>
>> Rob Herring (5):
>>    dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
>>    dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
>>    phy: Add Marvell USB 2.0 OTG 28nm PHY
>>    phy: add Marvell HSIC 28nm PHY
>>    usb: add pxa1928 ehci support
>>
>>   .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
>>   .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
>>   drivers/phy/Kconfig                                |  20 ++
>>   drivers/phy/Makefile                               |   2 +
>>   drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
>>   drivers/phy/phy-mv-usb2.c                          | 329
>> +++++++++++++++++++++
>
>
> [Adding some MVEBU guys]
>
> Rob,
>
> I had a look at the USB PHYs of some of the other Marvell SoCs a while
> ago for the barebox bootloader [1]. Marvell seems to distinguish the
> USB PHY type by technology node, e.g. 28nm like the one above. For the
> most used Marvell SoCs, i.e. Kirkwood, Dove, and Armada 370/XP, they all
> use a different technology node and we could either use the SoC name
> or the technology node as compatible.

It also seems to be by business unit. The registers in the 40nm
version I have look nothing like what are in Armada. It could be the
same IP, but PHY IP tends to just be signals and they leave the
register interface up the to licensee's imagination. Any idea if there
are 28nm Armada parts which have the same phy?

It's a good idea to always include the SOC name in the compatible.
Every new chip is an opportunity for changes or bugs.

> Anyway, if you are introducing new PHY drivers with _that_ generic
> names, it will either clash with every other Marvell USB PHYs -
> or we'll have to add the PHY code into the drivers above.

How about phy-pxa-28nm-{usb2,hsic}?

Rob

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

On Fri, May 15, 2015 at 4:55 AM, Sebastian Hesselbarth
<sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 14.05.2015 00:48, Rob Herring wrote:
>>
>> This series adds USB PHYs and EHCI host drivers for the Marvell PXA1928
>> SOC.
>>
>> The OTG block is based on ChipIdea and works with "chipidea,usb2"
>> compatible driver as is just by adding the PHY driver. Yay!
>>
>> Rob
>>
>> Rob Herring (5):
>>    dt-bindings: Add Marvell PXA1928 USB and HSIC PHY bindings
>>    dt-bindings: Add Marvell PXA1928 USB EHCI controller binding
>>    phy: Add Marvell USB 2.0 OTG 28nm PHY
>>    phy: add Marvell HSIC 28nm PHY
>>    usb: add pxa1928 ehci support
>>
>>   .../devicetree/bindings/phy/pxa1928-usb-phy.txt    |  18 ++
>>   .../devicetree/bindings/usb/ehci-pxa1928.txt       |  19 ++
>>   drivers/phy/Kconfig                                |  20 ++
>>   drivers/phy/Makefile                               |   2 +
>>   drivers/phy/phy-mv-hsic.c                          | 208 +++++++++++++
>>   drivers/phy/phy-mv-usb2.c                          | 329
>> +++++++++++++++++++++
>
>
> [Adding some MVEBU guys]
>
> Rob,
>
> I had a look at the USB PHYs of some of the other Marvell SoCs a while
> ago for the barebox bootloader [1]. Marvell seems to distinguish the
> USB PHY type by technology node, e.g. 28nm like the one above. For the
> most used Marvell SoCs, i.e. Kirkwood, Dove, and Armada 370/XP, they all
> use a different technology node and we could either use the SoC name
> or the technology node as compatible.

It also seems to be by business unit. The registers in the 40nm
version I have look nothing like what are in Armada. It could be the
same IP, but PHY IP tends to just be signals and they leave the
register interface up the to licensee's imagination. Any idea if there
are 28nm Armada parts which have the same phy?

It's a good idea to always include the SOC name in the compatible.
Every new chip is an opportunity for changes or bugs.

> Anyway, if you are introducing new PHY drivers with _that_ generic
> names, it will either clash with every other Marvell USB PHYs -
> or we'll have to add the PHY code into the drivers above.

How about phy-pxa-28nm-{usb2,hsic}?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Arnd Bergmann]

On Friday 15 May 2015 07:48:24 Rob Herring wrote:
> 
> > Anyway, if you are introducing new PHY drivers with _that_ generic
> > names, it will either clash with every other Marvell USB PHYs -
> > or we'll have to add the PHY code into the drivers above.
> 
> How about phy-pxa-28nm-{usb2,hsic}?
> 

Seems ok to me, but please verify that these are actually usb2 and hsic
specific devices. There are a lot of PHY implementations that share the
basic layout between devices for different purposes, e.g. USB3 and PCIe
and they should have a name that is not too specific for one connection
when the same driver gets reused by another part.

	Arnd

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

On Fri, May 15, 2015 at 9:11 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 May 2015 07:48:24 Rob Herring wrote:
>>
>> > Anyway, if you are introducing new PHY drivers with _that_ generic
>> > names, it will either clash with every other Marvell USB PHYs -
>> > or we'll have to add the PHY code into the drivers above.
>>
>> How about phy-pxa-28nm-{usb2,hsic}?
>>
>
> Seems ok to me, but please verify that these are actually usb2 and hsic
> specific devices. There are a lot of PHY implementations that share the
> basic layout between devices for different purposes, e.g. USB3 and PCIe
> and they should have a name that is not too specific for one connection
> when the same driver gets reused by another part.

Yes, I'm aware of that as Calxeda "comboPHYs" did 10G XAUI, PCIe and
SATA. Those are cases are supporting similar electrical specs. USB3 is
similar to those. I've not seen USB 2.0 PHYs shared with anything
else, and HSIC is also very unique.

Rob

>
>         Arnd

Re: [PATCH 0/5] Marvell PXA1928 USB support

[Rob Herring]

On Fri, May 15, 2015 at 9:11 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 15 May 2015 07:48:24 Rob Herring wrote:
>>
>> > Anyway, if you are introducing new PHY drivers with _that_ generic
>> > names, it will either clash with every other Marvell USB PHYs -
>> > or we'll have to add the PHY code into the drivers above.
>>
>> How about phy-pxa-28nm-{usb2,hsic}?
>>
>
> Seems ok to me, but please verify that these are actually usb2 and hsic
> specific devices. There are a lot of PHY implementations that share the
> basic layout between devices for different purposes, e.g. USB3 and PCIe
> and they should have a name that is not too specific for one connection
> when the same driver gets reused by another part.

Yes, I'm aware of that as Calxeda "comboPHYs" did 10G XAUI, PCIe and
SATA. Those are cases are supporting similar electrical specs. USB3 is
similar to those. I've not seen USB 2.0 PHYs shared with anything
else, and HSIC is also very unique.

Rob

>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] phy: Add Marvell USB 2.0 OTG 28nm PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
> Add driver for USB PHY found in Marvell PXA1928 SOC.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>   drivers/phy/Kconfig       |  10 ++
>   drivers/phy/Makefile      |   1 +
>   drivers/phy/phy-mv-usb2.c | 329 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 340 insertions(+)
>   create mode 100644 drivers/phy/phy-mv-usb2.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index a53bd5b..ef7634f 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>   	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>   	  and EXYNOS SoCs.
>
> +config PHY_MV_USB2
> +	tristate "Marvell USB 2.0 28nm PHY Driver"
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support Marvell USB 2.0 PHY driver for Marvell
> +	  SoC. This driver will do the PHY initialization and shutdown.
> +	  The PHY driver will be used by Marvell udc/ehci/otg driver.
> +
> +	  To compile this driver as a module, choose M here.
> +
>   config PHY_MVEBU_SATA
>   	def_bool y
>   	depends on ARCH_DOVE || MACH_DOVE || MACH_KIRKWOOD
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index f126251..768e55a 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>   obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>   obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>   obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
> +obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>   obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>   obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>   obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
> diff --git a/drivers/phy/phy-mv-usb2.c b/drivers/phy/phy-mv-usb2.c
> new file mode 100644
> index 0000000..c48d111
> --- /dev/null
> +++ b/drivers/phy/phy-mv-usb2.c
> @@ -0,0 +1,329 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh@kernel.org>
> + *
> + * Based on vendor driver:
> + * Copyright (C) 2013 Marvell Inc.
> + * Author: Chao Xie <xiechao.mail@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/delay.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +
> +/* USB PXA1928 PHY mapping */
> +#define PHY_28NM_PLL_REG0			0x0
> +#define PHY_28NM_PLL_REG1			0x4
> +#define PHY_28NM_CAL_REG			0x8
> +#define PHY_28NM_TX_REG0			0x0c
> +#define PHY_28NM_TX_REG1			0x10
> +#define PHY_28NM_RX_REG0			0x14
> +#define PHY_28NM_RX_REG1			0x18
> +#define PHY_28NM_DIG_REG0			0x1c
> +#define PHY_28NM_DIG_REG1			0x20
> +#define PHY_28NM_TEST_REG0			0x24
> +#define PHY_28NM_TEST_REG1			0x28
> +#define PHY_28NM_MOC_REG			0x2c
> +#define PHY_28NM_PHY_RESERVE			0x30
> +#define PHY_28NM_OTG_REG			0x34
> +#define PHY_28NM_CHRG_DET			0x38
> +#define PHY_28NM_CTRL_REG0			0xc4
> +#define PHY_28NM_CTRL_REG1			0xc8
> +#define PHY_28NM_CTRL_REG2			0xd4
> +#define PHY_28NM_CTRL_REG3			0xdc
> +
> +/* PHY_28NM_PLL_REG0 */
> +#define PHY_28NM_PLL_READY			BIT(31)
> +
> +#define PHY_28NM_PLL_SELLPFR_SHIFT		28
> +#define PHY_28NM_PLL_SELLPFR_MASK		(0x3 << 28)
> +
> +#define PHY_28NM_PLL_FBDIV_SHIFT		16
> +#define PHY_28NM_PLL_FBDIV_MASK			(0x1ff << 16)
> +
> +#define PHY_28NM_PLL_ICP_SHIFT			8
> +#define PHY_28NM_PLL_ICP_MASK			(0x7 << 8)
> +
> +#define PHY_28NM_PLL_REFDIV_SHIFT		0
> +#define PHY_28NM_PLL_REFDIV_MASK		0x7f
> +
> +/* PHY_28NM_PLL_REG1 */
> +#define PHY_28NM_PLL_PU_BY_REG			BIT(1)
> +
> +#define PHY_28NM_PLL_PU_PLL			BIT(0)
> +
> +/* PHY_28NM_CAL_REG */
> +#define PHY_28NM_PLL_PLLCAL_DONE		BIT(31)
> +
> +#define PHY_28NM_PLL_IMPCAL_DONE		BIT(23)
> +
> +#define PHY_28NM_PLL_KVCO_SHIFT			16
> +#define PHY_28NM_PLL_KVCO_MASK			(0x7 << 16)
> +
> +#define PHY_28NM_PLL_CAL12_SHIFT		20
> +#define PHY_28NM_PLL_CAL12_MASK			(0x3 << 20)
> +
> +#define PHY_28NM_IMPCAL_VTH_SHIFT		8
> +#define PHY_28NM_IMPCAL_VTH_MASK		(0x7 << 8)
> +
> +#define PHY_28NM_PLLCAL_START_SHIFT		22
> +#define PHY_28NM_IMPCAL_START_SHIFT		13
> +
> +/* PHY_28NM_TX_REG0 */
> +#define PHY_28NM_TX_PU_BY_REG			BIT(25)
> +
> +#define PHY_28NM_TX_PU_ANA			BIT(24)
> +
> +#define PHY_28NM_TX_AMP_SHIFT			20
> +#define PHY_28NM_TX_AMP_MASK			(0x7 << 20)
> +
> +/* PHY_28NM_RX_REG0 */
> +#define PHY_28NM_RX_SQ_THRESH_SHIFT		0
> +#define PHY_28NM_RX_SQ_THRESH_MASK		(0xf << 0)
> +
> +/* PHY_28NM_RX_REG1 */
> +#define PHY_28NM_RX_SQCAL_DONE			BIT(31)
> +
> +/* PHY_28NM_DIG_REG0 */
> +#define PHY_28NM_DIG_BITSTAFFING_ERR		BIT(31)
> +#define PHY_28NM_DIG_SYNC_ERR			BIT(30)
> +
> +#define PHY_28NM_DIG_SQ_FILT_SHIFT		16
> +#define PHY_28NM_DIG_SQ_FILT_MASK		(0x7 << 16)
> +
> +#define PHY_28NM_DIG_SQ_BLK_SHIFT		12
> +#define PHY_28NM_DIG_SQ_BLK_MASK		(0x7 << 12)
> +
> +#define PHY_28NM_DIG_SYNC_NUM_SHIFT		0
> +#define PHY_28NM_DIG_SYNC_NUM_MASK		(0x3 << 0)
> +
> +#define PHY_28NM_PLL_LOCK_BYPASS		BIT(7)
> +
> +/* PHY_28NM_OTG_REG */
> +#define PHY_28NM_OTG_CONTROL_BY_PIN		BIT(5)
> +#define PHY_28NM_OTG_PU_OTG			BIT(4)
> +
> +#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DM_SHIFT_28 13
> +#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DP_SHIFT_28 12
> +#define PHY_28NM_CHGDTC_VSRC_CHARGE_SHIFT_28	10
> +#define PHY_28NM_CHGDTC_VDAT_CHARGE_SHIFT_28	8
> +#define PHY_28NM_CHGDTC_CDP_DM_AUTO_SWITCH_SHIFT_28 7
> +#define PHY_28NM_CHGDTC_DP_DM_SWAP_SHIFT_28	6
> +#define PHY_28NM_CHGDTC_PU_CHRG_DTC_SHIFT_28	5
> +#define PHY_28NM_CHGDTC_PD_EN_SHIFT_28		4
> +#define PHY_28NM_CHGDTC_DCP_EN_SHIFT_28		3
> +#define PHY_28NM_CHGDTC_CDP_EN_SHIFT_28		2
> +#define PHY_28NM_CHGDTC_TESTMON_CHRGDTC_SHIFT_28 0
> +
> +#define PHY_28NM_CTRL1_CHRG_DTC_OUT_SHIFT_28	4
> +#define PHY_28NM_CTRL1_VBUSDTC_OUT_SHIFT_28	2
> +
> +#define PHY_28NM_CTRL3_OVERWRITE		BIT(0)
> +#define PHY_28NM_CTRL3_VBUS_VALID		BIT(4)
> +#define PHY_28NM_CTRL3_AVALID			BIT(5)
> +#define PHY_28NM_CTRL3_BVALID			BIT(6)
> +
> +struct mv_usb2_phy {
> +	struct phy		*phy;
> +	struct platform_device	*pdev;
> +	void __iomem		*base;
> +	struct clk		*clk;
> +};
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_usb2_phy_28nm_init(struct phy *phy)
> +{
> +	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +	void __iomem *base = mv_phy->base;
> +	u32 reg;
> +
> +	clk_prepare_enable(mv_phy->clk);
> +
> +	/* PHY_28NM_PLL_REG0 */
> +	reg = readl(base + PHY_28NM_PLL_REG0) &
> +		~(PHY_28NM_PLL_SELLPFR_MASK | PHY_28NM_PLL_FBDIV_MASK
> +		| PHY_28NM_PLL_ICP_MASK	| PHY_28NM_PLL_REFDIV_MASK);
> +	writel(reg | (0x1 << PHY_28NM_PLL_SELLPFR_SHIFT
> +		| 0xf0 << PHY_28NM_PLL_FBDIV_SHIFT
> +		| 0x3 << PHY_28NM_PLL_ICP_SHIFT
> +		| 0xd << PHY_28NM_PLL_REFDIV_SHIFT),
> +		base + PHY_28NM_PLL_REG0);
> +
> +	/* PHY_28NM_PLL_REG1 */
> +	reg = readl(base + PHY_28NM_PLL_REG1);
> +	writel(reg | PHY_28NM_PLL_PU_PLL | PHY_28NM_PLL_PU_BY_REG,
> +		base + PHY_28NM_PLL_REG1);
> +
> +	/* PHY_28NM_TX_REG0 */
> +	reg = readl(base + PHY_28NM_TX_REG0) & ~PHY_28NM_TX_AMP_MASK;
> +	writel(reg | PHY_28NM_TX_PU_BY_REG | 0x3 << PHY_28NM_TX_AMP_SHIFT |
> +		PHY_28NM_TX_PU_ANA,
> +		base + PHY_28NM_TX_REG0);
> +
> +	/* PHY_28NM_RX_REG0 */
> +	reg = readl(base + PHY_28NM_RX_REG0) & ~PHY_28NM_RX_SQ_THRESH_MASK;
> +	writel(reg | 0xa << PHY_28NM_RX_SQ_THRESH_SHIFT,
> +		base + PHY_28NM_RX_REG0);
> +
> +	/* PHY_28NM_DIG_REG0 */
> +	reg = readl(base + PHY_28NM_DIG_REG0) &
> +		~(PHY_28NM_DIG_BITSTAFFING_ERR | PHY_28NM_DIG_SYNC_ERR |
> +		PHY_28NM_DIG_SQ_FILT_MASK | PHY_28NM_DIG_SQ_BLK_MASK |
> +		PHY_28NM_DIG_SYNC_NUM_MASK);
> +	writel(reg | (0x1 << PHY_28NM_DIG_SYNC_NUM_SHIFT |
> +		PHY_28NM_PLL_LOCK_BYPASS),
> +		base + PHY_28NM_DIG_REG0);
> +
> +	/* PHY_28NM_OTG_REG */
> +	reg = readl(base + PHY_28NM_OTG_REG) | PHY_28NM_OTG_PU_OTG;
> +	writel(reg & ~PHY_28NM_OTG_CONTROL_BY_PIN, base + PHY_28NM_OTG_REG);
> +
> +	/*  Calibration Timing
> +	*		   ____________________________
> +	*  CAL START   ___|
> +	*			   ____________________
> +	*  CAL_DONE    ___________|
> +	*		  | 400us |
> +	*/
> +
> +	/* Make sure PHY Calibration is ready */
> +	if (!wait_for_reg(base + PHY_28NM_CAL_REG,
> +	    PHY_28NM_PLL_PLLCAL_DONE | PHY_28NM_PLL_IMPCAL_DONE,
> +	    HZ / 10)) {
> +		dev_warn(&pdev->dev, "USB PHY PLL calibrate not done after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +	if (!wait_for_reg(base + PHY_28NM_RX_REG1,
> +	    PHY_28NM_RX_SQCAL_DONE, HZ / 10)) {
> +		dev_warn(&pdev->dev, "USB PHY RX SQ calibrate not done after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Make sure PHY PLL is ready */
> +	if (!wait_for_reg(base + PHY_28NM_PLL_REG0,
> +	    PHY_28NM_PLL_READY, HZ / 10)) {
> +		dev_warn(&pdev->dev, "PLL_READY not set after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	writel(readl(base + PHY_28NM_CTRL_REG3) |
> +		(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
> +		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
> +		base + PHY_28NM_CTRL_REG3);

IMO only the PLL programming should be done in init. The VBUS_VALID etc.. 
should be done as part of power_on callback.

Thanks
Kishon

Re: [PATCH 3/5] phy: Add Marvell USB 2.0 OTG 28nm PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
> Add driver for USB PHY found in Marvell PXA1928 SOC.
>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
> ---
>   drivers/phy/Kconfig       |  10 ++
>   drivers/phy/Makefile      |   1 +
>   drivers/phy/phy-mv-usb2.c | 329 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 340 insertions(+)
>   create mode 100644 drivers/phy/phy-mv-usb2.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index a53bd5b..ef7634f 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>   	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>   	  and EXYNOS SoCs.
>
> +config PHY_MV_USB2
> +	tristate "Marvell USB 2.0 28nm PHY Driver"
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support Marvell USB 2.0 PHY driver for Marvell
> +	  SoC. This driver will do the PHY initialization and shutdown.
> +	  The PHY driver will be used by Marvell udc/ehci/otg driver.
> +
> +	  To compile this driver as a module, choose M here.
> +
>   config PHY_MVEBU_SATA
>   	def_bool y
>   	depends on ARCH_DOVE || MACH_DOVE || MACH_KIRKWOOD
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index f126251..768e55a 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>   obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>   obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>   obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
> +obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>   obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>   obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>   obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
> diff --git a/drivers/phy/phy-mv-usb2.c b/drivers/phy/phy-mv-usb2.c
> new file mode 100644
> index 0000000..c48d111
> --- /dev/null
> +++ b/drivers/phy/phy-mv-usb2.c
> @@ -0,0 +1,329 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> + *
> + * Based on vendor driver:
> + * Copyright (C) 2013 Marvell Inc.
> + * Author: Chao Xie <xiechao.mail-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/delay.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +
> +/* USB PXA1928 PHY mapping */
> +#define PHY_28NM_PLL_REG0			0x0
> +#define PHY_28NM_PLL_REG1			0x4
> +#define PHY_28NM_CAL_REG			0x8
> +#define PHY_28NM_TX_REG0			0x0c
> +#define PHY_28NM_TX_REG1			0x10
> +#define PHY_28NM_RX_REG0			0x14
> +#define PHY_28NM_RX_REG1			0x18
> +#define PHY_28NM_DIG_REG0			0x1c
> +#define PHY_28NM_DIG_REG1			0x20
> +#define PHY_28NM_TEST_REG0			0x24
> +#define PHY_28NM_TEST_REG1			0x28
> +#define PHY_28NM_MOC_REG			0x2c
> +#define PHY_28NM_PHY_RESERVE			0x30
> +#define PHY_28NM_OTG_REG			0x34
> +#define PHY_28NM_CHRG_DET			0x38
> +#define PHY_28NM_CTRL_REG0			0xc4
> +#define PHY_28NM_CTRL_REG1			0xc8
> +#define PHY_28NM_CTRL_REG2			0xd4
> +#define PHY_28NM_CTRL_REG3			0xdc
> +
> +/* PHY_28NM_PLL_REG0 */
> +#define PHY_28NM_PLL_READY			BIT(31)
> +
> +#define PHY_28NM_PLL_SELLPFR_SHIFT		28
> +#define PHY_28NM_PLL_SELLPFR_MASK		(0x3 << 28)
> +
> +#define PHY_28NM_PLL_FBDIV_SHIFT		16
> +#define PHY_28NM_PLL_FBDIV_MASK			(0x1ff << 16)
> +
> +#define PHY_28NM_PLL_ICP_SHIFT			8
> +#define PHY_28NM_PLL_ICP_MASK			(0x7 << 8)
> +
> +#define PHY_28NM_PLL_REFDIV_SHIFT		0
> +#define PHY_28NM_PLL_REFDIV_MASK		0x7f
> +
> +/* PHY_28NM_PLL_REG1 */
> +#define PHY_28NM_PLL_PU_BY_REG			BIT(1)
> +
> +#define PHY_28NM_PLL_PU_PLL			BIT(0)
> +
> +/* PHY_28NM_CAL_REG */
> +#define PHY_28NM_PLL_PLLCAL_DONE		BIT(31)
> +
> +#define PHY_28NM_PLL_IMPCAL_DONE		BIT(23)
> +
> +#define PHY_28NM_PLL_KVCO_SHIFT			16
> +#define PHY_28NM_PLL_KVCO_MASK			(0x7 << 16)
> +
> +#define PHY_28NM_PLL_CAL12_SHIFT		20
> +#define PHY_28NM_PLL_CAL12_MASK			(0x3 << 20)
> +
> +#define PHY_28NM_IMPCAL_VTH_SHIFT		8
> +#define PHY_28NM_IMPCAL_VTH_MASK		(0x7 << 8)
> +
> +#define PHY_28NM_PLLCAL_START_SHIFT		22
> +#define PHY_28NM_IMPCAL_START_SHIFT		13
> +
> +/* PHY_28NM_TX_REG0 */
> +#define PHY_28NM_TX_PU_BY_REG			BIT(25)
> +
> +#define PHY_28NM_TX_PU_ANA			BIT(24)
> +
> +#define PHY_28NM_TX_AMP_SHIFT			20
> +#define PHY_28NM_TX_AMP_MASK			(0x7 << 20)
> +
> +/* PHY_28NM_RX_REG0 */
> +#define PHY_28NM_RX_SQ_THRESH_SHIFT		0
> +#define PHY_28NM_RX_SQ_THRESH_MASK		(0xf << 0)
> +
> +/* PHY_28NM_RX_REG1 */
> +#define PHY_28NM_RX_SQCAL_DONE			BIT(31)
> +
> +/* PHY_28NM_DIG_REG0 */
> +#define PHY_28NM_DIG_BITSTAFFING_ERR		BIT(31)
> +#define PHY_28NM_DIG_SYNC_ERR			BIT(30)
> +
> +#define PHY_28NM_DIG_SQ_FILT_SHIFT		16
> +#define PHY_28NM_DIG_SQ_FILT_MASK		(0x7 << 16)
> +
> +#define PHY_28NM_DIG_SQ_BLK_SHIFT		12
> +#define PHY_28NM_DIG_SQ_BLK_MASK		(0x7 << 12)
> +
> +#define PHY_28NM_DIG_SYNC_NUM_SHIFT		0
> +#define PHY_28NM_DIG_SYNC_NUM_MASK		(0x3 << 0)
> +
> +#define PHY_28NM_PLL_LOCK_BYPASS		BIT(7)
> +
> +/* PHY_28NM_OTG_REG */
> +#define PHY_28NM_OTG_CONTROL_BY_PIN		BIT(5)
> +#define PHY_28NM_OTG_PU_OTG			BIT(4)
> +
> +#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DM_SHIFT_28 13
> +#define PHY_28NM_CHGDTC_ENABLE_SWITCH_DP_SHIFT_28 12
> +#define PHY_28NM_CHGDTC_VSRC_CHARGE_SHIFT_28	10
> +#define PHY_28NM_CHGDTC_VDAT_CHARGE_SHIFT_28	8
> +#define PHY_28NM_CHGDTC_CDP_DM_AUTO_SWITCH_SHIFT_28 7
> +#define PHY_28NM_CHGDTC_DP_DM_SWAP_SHIFT_28	6
> +#define PHY_28NM_CHGDTC_PU_CHRG_DTC_SHIFT_28	5
> +#define PHY_28NM_CHGDTC_PD_EN_SHIFT_28		4
> +#define PHY_28NM_CHGDTC_DCP_EN_SHIFT_28		3
> +#define PHY_28NM_CHGDTC_CDP_EN_SHIFT_28		2
> +#define PHY_28NM_CHGDTC_TESTMON_CHRGDTC_SHIFT_28 0
> +
> +#define PHY_28NM_CTRL1_CHRG_DTC_OUT_SHIFT_28	4
> +#define PHY_28NM_CTRL1_VBUSDTC_OUT_SHIFT_28	2
> +
> +#define PHY_28NM_CTRL3_OVERWRITE		BIT(0)
> +#define PHY_28NM_CTRL3_VBUS_VALID		BIT(4)
> +#define PHY_28NM_CTRL3_AVALID			BIT(5)
> +#define PHY_28NM_CTRL3_BVALID			BIT(6)
> +
> +struct mv_usb2_phy {
> +	struct phy		*phy;
> +	struct platform_device	*pdev;
> +	void __iomem		*base;
> +	struct clk		*clk;
> +};
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_usb2_phy_28nm_init(struct phy *phy)
> +{
> +	struct mv_usb2_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +	void __iomem *base = mv_phy->base;
> +	u32 reg;
> +
> +	clk_prepare_enable(mv_phy->clk);
> +
> +	/* PHY_28NM_PLL_REG0 */
> +	reg = readl(base + PHY_28NM_PLL_REG0) &
> +		~(PHY_28NM_PLL_SELLPFR_MASK | PHY_28NM_PLL_FBDIV_MASK
> +		| PHY_28NM_PLL_ICP_MASK	| PHY_28NM_PLL_REFDIV_MASK);
> +	writel(reg | (0x1 << PHY_28NM_PLL_SELLPFR_SHIFT
> +		| 0xf0 << PHY_28NM_PLL_FBDIV_SHIFT
> +		| 0x3 << PHY_28NM_PLL_ICP_SHIFT
> +		| 0xd << PHY_28NM_PLL_REFDIV_SHIFT),
> +		base + PHY_28NM_PLL_REG0);
> +
> +	/* PHY_28NM_PLL_REG1 */
> +	reg = readl(base + PHY_28NM_PLL_REG1);
> +	writel(reg | PHY_28NM_PLL_PU_PLL | PHY_28NM_PLL_PU_BY_REG,
> +		base + PHY_28NM_PLL_REG1);
> +
> +	/* PHY_28NM_TX_REG0 */
> +	reg = readl(base + PHY_28NM_TX_REG0) & ~PHY_28NM_TX_AMP_MASK;
> +	writel(reg | PHY_28NM_TX_PU_BY_REG | 0x3 << PHY_28NM_TX_AMP_SHIFT |
> +		PHY_28NM_TX_PU_ANA,
> +		base + PHY_28NM_TX_REG0);
> +
> +	/* PHY_28NM_RX_REG0 */
> +	reg = readl(base + PHY_28NM_RX_REG0) & ~PHY_28NM_RX_SQ_THRESH_MASK;
> +	writel(reg | 0xa << PHY_28NM_RX_SQ_THRESH_SHIFT,
> +		base + PHY_28NM_RX_REG0);
> +
> +	/* PHY_28NM_DIG_REG0 */
> +	reg = readl(base + PHY_28NM_DIG_REG0) &
> +		~(PHY_28NM_DIG_BITSTAFFING_ERR | PHY_28NM_DIG_SYNC_ERR |
> +		PHY_28NM_DIG_SQ_FILT_MASK | PHY_28NM_DIG_SQ_BLK_MASK |
> +		PHY_28NM_DIG_SYNC_NUM_MASK);
> +	writel(reg | (0x1 << PHY_28NM_DIG_SYNC_NUM_SHIFT |
> +		PHY_28NM_PLL_LOCK_BYPASS),
> +		base + PHY_28NM_DIG_REG0);
> +
> +	/* PHY_28NM_OTG_REG */
> +	reg = readl(base + PHY_28NM_OTG_REG) | PHY_28NM_OTG_PU_OTG;
> +	writel(reg & ~PHY_28NM_OTG_CONTROL_BY_PIN, base + PHY_28NM_OTG_REG);
> +
> +	/*  Calibration Timing
> +	*		   ____________________________
> +	*  CAL START   ___|
> +	*			   ____________________
> +	*  CAL_DONE    ___________|
> +	*		  | 400us |
> +	*/
> +
> +	/* Make sure PHY Calibration is ready */
> +	if (!wait_for_reg(base + PHY_28NM_CAL_REG,
> +	    PHY_28NM_PLL_PLLCAL_DONE | PHY_28NM_PLL_IMPCAL_DONE,
> +	    HZ / 10)) {
> +		dev_warn(&pdev->dev, "USB PHY PLL calibrate not done after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +	if (!wait_for_reg(base + PHY_28NM_RX_REG1,
> +	    PHY_28NM_RX_SQCAL_DONE, HZ / 10)) {
> +		dev_warn(&pdev->dev, "USB PHY RX SQ calibrate not done after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Make sure PHY PLL is ready */
> +	if (!wait_for_reg(base + PHY_28NM_PLL_REG0,
> +	    PHY_28NM_PLL_READY, HZ / 10)) {
> +		dev_warn(&pdev->dev, "PLL_READY not set after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	writel(readl(base + PHY_28NM_CTRL_REG3) |
> +		(PHY_28NM_CTRL3_OVERWRITE | PHY_28NM_CTRL3_VBUS_VALID
> +		| PHY_28NM_CTRL3_AVALID	| PHY_28NM_CTRL3_BVALID),
> +		base + PHY_28NM_CTRL_REG3);

IMO only the PLL programming should be done in init. The VBUS_VALID etc.. 
should be done as part of power_on callback.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in PXA1928
> SOC.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>   drivers/phy/Kconfig       |  10 +++
>   drivers/phy/Makefile      |   1 +
>   drivers/phy/phy-mv-hsic.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 219 insertions(+)
>   create mode 100644 drivers/phy/phy-mv-hsic.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ef7634f..cfcae72 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>   	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>   	  and EXYNOS SoCs.
>
> +config PHY_MV_HSIC
> +	tristate "Marvell USB HSIC 28nm PHY Driver"
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support Marvell USB HSIC PHY driver for Marvell
> +	  SoC. This driver will do the PHY initialization and shutdown.
> +	  The PHY driver will be used by Marvell ehci driver.
> +
> +	  To compile this driver as a module, choose M here.
> +
>   config PHY_MV_USB2
>   	tristate "Marvell USB 2.0 28nm PHY Driver"
>   	select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 768e55a..472bf0a 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>   obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>   obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>   obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
> +obj-$(CONFIG_PHY_MV_HSIC)		+= phy-mv-hsic.o
>   obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>   obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>   obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
> diff --git a/drivers/phy/phy-mv-hsic.c b/drivers/phy/phy-mv-hsic.c
> new file mode 100644
> index 0000000..aa6cccd
> --- /dev/null
> +++ b/drivers/phy/phy-mv-hsic.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh@kernel.org>
> + *
> + * Based on vendor driver:
> + * Copyright (C) 2013 Marvell Inc.
> + * Author: Chao Xie <xiechao.mail@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/delay.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +
> +#define PHY_28NM_HSIC_CTRL			0x08
> +#define PHY_28NM_HSIC_IMPCAL_CAL		0x18
> +#define PHY_28NM_HSIC_PLL_CTRL01		0x1c
> +#define PHY_28NM_HSIC_PLL_CTRL2			0x20
> +#define PHY_28NM_HSIC_INT			0x28
> +
> +#define PHY_28NM_HSIC_PLL_SELLPFR_SHIFT		26
> +#define PHY_28NM_HSIC_PLL_FBDIV_SHIFT		0
> +#define PHY_28NM_HSIC_PLL_REFDIV_SHIFT		9
> +
> +#define PHY_28NM_HSIC_S2H_PU_PLL		BIT(10)
> +#define PHY_28NM_HSIC_H2S_PLL_LOCK		BIT(15)
> +#define PHY_28NM_HSIC_S2H_HSIC_EN		BIT(7)
> +#define S2H_DRV_SE0_4RESUME			BIT(14)
> +#define PHY_28NM_HSIC_H2S_IMPCAL_DONE		BIT(27)
> +
> +#define PHY_28NM_HSIC_CONNECT_INT		BIT(1)
> +#define PHY_28NM_HSIC_HS_READY_INT		BIT(2)
> +
> +struct mv_hsic_phy {
> +	struct phy		*phy;
> +	struct platform_device	*pdev;
> +	void __iomem		*base;
> +	struct clk		*clk;
> +};
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_hsic_phy_init(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +	void __iomem *base = mv_phy->base;
> +
> +	clk_prepare_enable(mv_phy->clk);
> +
> +	/* Set reference clock */
> +	writel(0x1 << PHY_28NM_HSIC_PLL_SELLPFR_SHIFT |
> +		0xf0 << PHY_28NM_HSIC_PLL_FBDIV_SHIFT |
> +		0xd << PHY_28NM_HSIC_PLL_REFDIV_SHIFT,
> +		base + PHY_28NM_HSIC_PLL_CTRL01);
> +
> +	/* Turn on PLL */
> +	writel(readl(base + PHY_28NM_HSIC_PLL_CTRL2) |
> +		PHY_28NM_HSIC_S2H_PU_PLL,
> +		base + PHY_28NM_HSIC_PLL_CTRL2);
> +
> +	/* Make sure PHY PLL is locked */
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_PLL_CTRL2,
> +	    PHY_28NM_HSIC_H2S_PLL_LOCK, HZ / 10)) {
> +		dev_err(&pdev->dev, "HSIC PHY PLL not locked after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Avoid SE0 state when resume for some device will take it as reset */
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~S2H_DRV_SE0_4RESUME,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	/* Enable HSIC PHY */
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) | PHY_28NM_HSIC_S2H_HSIC_EN,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	/*  Calibration Timing
> +	*		   ____________________________
> +	*  CAL START   ___|
> +	*			   ____________________
> +	*  CAL_DONE    ___________|
> +	*		  | 400us |
> +	*/

Please use the comment style used here
http://lxr.free-electrons.com/source/Documentation/CodingStyle#L464

> +
> +	/* Make sure PHY Calibration is ready */
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_IMPCAL_CAL,
> +	    PHY_28NM_HSIC_H2S_IMPCAL_DONE, HZ / 10)) {
> +		dev_warn(&pdev->dev, "HSIC PHY READY not set after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Waiting for HSIC connect int*/
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_INT,
> +	    PHY_28NM_HSIC_CONNECT_INT, HZ / 5)) {
> +		dev_warn(&pdev->dev, "HSIC wait for connect interrupt timeout.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv_hsic_phy_power_on(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +
> +	/* check HS ready */
> +	if (!wait_for_reg(mv_phy->base + PHY_28NM_HSIC_INT,
> +	    PHY_28NM_HSIC_HS_READY_INT, HZ / 10)) {
> +		dev_err(&pdev->dev, "HSIC HS_READY not set\n");
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int mv_hsic_phy_power_off(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	void __iomem *base = mv_phy->base;
> +
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~PHY_28NM_HSIC_S2H_HSIC_EN,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	clk_disable_unprepare(mv_phy->clk);
> +	return 0;
> +}
> +
> +static const struct phy_ops hsic_ops = {
> +	.init		= mv_hsic_phy_init,
> +	.power_on	= mv_hsic_phy_power_on,
> +	.power_off	= mv_hsic_phy_power_off,

exit callback is missing? Shouldn't we turn off the PLLs in exit callback?

Thanks
Kishon

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in PXA1928
> SOC.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>   drivers/phy/Kconfig       |  10 +++
>   drivers/phy/Makefile      |   1 +
>   drivers/phy/phy-mv-hsic.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 219 insertions(+)
>   create mode 100644 drivers/phy/phy-mv-hsic.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ef7634f..cfcae72 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>   	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>   	  and EXYNOS SoCs.
>
> +config PHY_MV_HSIC
> +	tristate "Marvell USB HSIC 28nm PHY Driver"
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support Marvell USB HSIC PHY driver for Marvell
> +	  SoC. This driver will do the PHY initialization and shutdown.
> +	  The PHY driver will be used by Marvell ehci driver.
> +
> +	  To compile this driver as a module, choose M here.
> +
>   config PHY_MV_USB2
>   	tristate "Marvell USB 2.0 28nm PHY Driver"
>   	select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 768e55a..472bf0a 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>   obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>   obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>   obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
> +obj-$(CONFIG_PHY_MV_HSIC)		+= phy-mv-hsic.o
>   obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>   obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>   obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
> diff --git a/drivers/phy/phy-mv-hsic.c b/drivers/phy/phy-mv-hsic.c
> new file mode 100644
> index 0000000..aa6cccd
> --- /dev/null
> +++ b/drivers/phy/phy-mv-hsic.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (C) 2015 Linaro, Ltd.
> + * Rob Herring <robh@kernel.org>
> + *
> + * Based on vendor driver:
> + * Copyright (C) 2013 Marvell Inc.
> + * Author: Chao Xie <xiechao.mail@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/delay.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +
> +#define PHY_28NM_HSIC_CTRL			0x08
> +#define PHY_28NM_HSIC_IMPCAL_CAL		0x18
> +#define PHY_28NM_HSIC_PLL_CTRL01		0x1c
> +#define PHY_28NM_HSIC_PLL_CTRL2			0x20
> +#define PHY_28NM_HSIC_INT			0x28
> +
> +#define PHY_28NM_HSIC_PLL_SELLPFR_SHIFT		26
> +#define PHY_28NM_HSIC_PLL_FBDIV_SHIFT		0
> +#define PHY_28NM_HSIC_PLL_REFDIV_SHIFT		9
> +
> +#define PHY_28NM_HSIC_S2H_PU_PLL		BIT(10)
> +#define PHY_28NM_HSIC_H2S_PLL_LOCK		BIT(15)
> +#define PHY_28NM_HSIC_S2H_HSIC_EN		BIT(7)
> +#define S2H_DRV_SE0_4RESUME			BIT(14)
> +#define PHY_28NM_HSIC_H2S_IMPCAL_DONE		BIT(27)
> +
> +#define PHY_28NM_HSIC_CONNECT_INT		BIT(1)
> +#define PHY_28NM_HSIC_HS_READY_INT		BIT(2)
> +
> +struct mv_hsic_phy {
> +	struct phy		*phy;
> +	struct platform_device	*pdev;
> +	void __iomem		*base;
> +	struct clk		*clk;
> +};
> +
> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
> +{
> +	timeout += jiffies;
> +	while (time_is_after_eq_jiffies(timeout)) {
> +		if ((readl(reg) & mask) == mask)
> +			return true;
> +		msleep(1);
> +	}
> +	return false;
> +}
> +
> +static int mv_hsic_phy_init(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +	void __iomem *base = mv_phy->base;
> +
> +	clk_prepare_enable(mv_phy->clk);
> +
> +	/* Set reference clock */
> +	writel(0x1 << PHY_28NM_HSIC_PLL_SELLPFR_SHIFT |
> +		0xf0 << PHY_28NM_HSIC_PLL_FBDIV_SHIFT |
> +		0xd << PHY_28NM_HSIC_PLL_REFDIV_SHIFT,
> +		base + PHY_28NM_HSIC_PLL_CTRL01);
> +
> +	/* Turn on PLL */
> +	writel(readl(base + PHY_28NM_HSIC_PLL_CTRL2) |
> +		PHY_28NM_HSIC_S2H_PU_PLL,
> +		base + PHY_28NM_HSIC_PLL_CTRL2);
> +
> +	/* Make sure PHY PLL is locked */
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_PLL_CTRL2,
> +	    PHY_28NM_HSIC_H2S_PLL_LOCK, HZ / 10)) {
> +		dev_err(&pdev->dev, "HSIC PHY PLL not locked after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Avoid SE0 state when resume for some device will take it as reset */
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~S2H_DRV_SE0_4RESUME,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	/* Enable HSIC PHY */
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) | PHY_28NM_HSIC_S2H_HSIC_EN,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	/*  Calibration Timing
> +	*		   ____________________________
> +	*  CAL START   ___|
> +	*			   ____________________
> +	*  CAL_DONE    ___________|
> +	*		  | 400us |
> +	*/

Please use the comment style used here
http://lxr.free-electrons.com/source/Documentation/CodingStyle#L464

> +
> +	/* Make sure PHY Calibration is ready */
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_IMPCAL_CAL,
> +	    PHY_28NM_HSIC_H2S_IMPCAL_DONE, HZ / 10)) {
> +		dev_warn(&pdev->dev, "HSIC PHY READY not set after 100mS.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Waiting for HSIC connect int*/
> +	if (!wait_for_reg(base + PHY_28NM_HSIC_INT,
> +	    PHY_28NM_HSIC_CONNECT_INT, HZ / 5)) {
> +		dev_warn(&pdev->dev, "HSIC wait for connect interrupt timeout.");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv_hsic_phy_power_on(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	struct platform_device *pdev = mv_phy->pdev;
> +
> +	/* check HS ready */
> +	if (!wait_for_reg(mv_phy->base + PHY_28NM_HSIC_INT,
> +	    PHY_28NM_HSIC_HS_READY_INT, HZ / 10)) {
> +		dev_err(&pdev->dev, "HSIC HS_READY not set\n");
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int mv_hsic_phy_power_off(struct phy *phy)
> +{
> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
> +	void __iomem *base = mv_phy->base;
> +
> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~PHY_28NM_HSIC_S2H_HSIC_EN,
> +		base + PHY_28NM_HSIC_CTRL);
> +
> +	clk_disable_unprepare(mv_phy->clk);
> +	return 0;
> +}
> +
> +static const struct phy_ops hsic_ops = {
> +	.init		= mv_hsic_phy_init,
> +	.power_on	= mv_hsic_phy_power_on,
> +	.power_off	= mv_hsic_phy_power_off,

exit callback is missing? Shouldn't we turn off the PLLs in exit callback?

Thanks
Kishon

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Kishon Vijay Abraham I]

On Thursday 21 May 2015 06:15 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
>> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in PXA1928
>> SOC.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>    drivers/phy/Kconfig       |  10 +++
>>    drivers/phy/Makefile      |   1 +
>>    drivers/phy/phy-mv-hsic.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>>    3 files changed, 219 insertions(+)
>>    create mode 100644 drivers/phy/phy-mv-hsic.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index ef7634f..cfcae72 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>>    	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>>    	  and EXYNOS SoCs.
>>
>> +config PHY_MV_HSIC
>> +	tristate "Marvell USB HSIC 28nm PHY Driver"
>> +	select GENERIC_PHY
>> +	help
>> +	  Enable this to support Marvell USB HSIC PHY driver for Marvell
>> +	  SoC. This driver will do the PHY initialization and shutdown.
>> +	  The PHY driver will be used by Marvell ehci driver.
>> +
>> +	  To compile this driver as a module, choose M here.
>> +
>>    config PHY_MV_USB2
>>    	tristate "Marvell USB 2.0 28nm PHY Driver"
>>    	select GENERIC_PHY
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 768e55a..472bf0a 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>>    obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>>    obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>>    obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
>> +obj-$(CONFIG_PHY_MV_HSIC)		+= phy-mv-hsic.o
>>    obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>>    obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>>    obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>> diff --git a/drivers/phy/phy-mv-hsic.c b/drivers/phy/phy-mv-hsic.c
>> new file mode 100644
>> index 0000000..aa6cccd
>> --- /dev/null
>> +++ b/drivers/phy/phy-mv-hsic.c
>> @@ -0,0 +1,208 @@
>> +/*
>> + * Copyright (C) 2015 Linaro, Ltd.
>> + * Rob Herring <robh@kernel.org>
>> + *
>> + * Based on vendor driver:
>> + * Copyright (C) 2013 Marvell Inc.
>> + * Author: Chao Xie <xiechao.mail@gmail.com>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * 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/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#define PHY_28NM_HSIC_CTRL			0x08
>> +#define PHY_28NM_HSIC_IMPCAL_CAL		0x18
>> +#define PHY_28NM_HSIC_PLL_CTRL01		0x1c
>> +#define PHY_28NM_HSIC_PLL_CTRL2			0x20
>> +#define PHY_28NM_HSIC_INT			0x28
>> +
>> +#define PHY_28NM_HSIC_PLL_SELLPFR_SHIFT		26
>> +#define PHY_28NM_HSIC_PLL_FBDIV_SHIFT		0
>> +#define PHY_28NM_HSIC_PLL_REFDIV_SHIFT		9
>> +
>> +#define PHY_28NM_HSIC_S2H_PU_PLL		BIT(10)
>> +#define PHY_28NM_HSIC_H2S_PLL_LOCK		BIT(15)
>> +#define PHY_28NM_HSIC_S2H_HSIC_EN		BIT(7)
>> +#define S2H_DRV_SE0_4RESUME			BIT(14)
>> +#define PHY_28NM_HSIC_H2S_IMPCAL_DONE		BIT(27)
>> +
>> +#define PHY_28NM_HSIC_CONNECT_INT		BIT(1)
>> +#define PHY_28NM_HSIC_HS_READY_INT		BIT(2)
>> +
>> +struct mv_hsic_phy {
>> +	struct phy		*phy;
>> +	struct platform_device	*pdev;
>> +	void __iomem		*base;
>> +	struct clk		*clk;
>> +};
>> +
>> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
>> +{
>> +	timeout += jiffies;
>> +	while (time_is_after_eq_jiffies(timeout)) {
>> +		if ((readl(reg) & mask) == mask)
>> +			return true;
>> +		msleep(1);
>> +	}
>> +	return false;
>> +}
>> +
>> +static int mv_hsic_phy_init(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	struct platform_device *pdev = mv_phy->pdev;
>> +	void __iomem *base = mv_phy->base;
>> +
>> +	clk_prepare_enable(mv_phy->clk);
>> +
>> +	/* Set reference clock */
>> +	writel(0x1 << PHY_28NM_HSIC_PLL_SELLPFR_SHIFT |
>> +		0xf0 << PHY_28NM_HSIC_PLL_FBDIV_SHIFT |
>> +		0xd << PHY_28NM_HSIC_PLL_REFDIV_SHIFT,
>> +		base + PHY_28NM_HSIC_PLL_CTRL01);
>> +
>> +	/* Turn on PLL */
>> +	writel(readl(base + PHY_28NM_HSIC_PLL_CTRL2) |
>> +		PHY_28NM_HSIC_S2H_PU_PLL,
>> +		base + PHY_28NM_HSIC_PLL_CTRL2);
>> +
>> +	/* Make sure PHY PLL is locked */
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_PLL_CTRL2,
>> +	    PHY_28NM_HSIC_H2S_PLL_LOCK, HZ / 10)) {
>> +		dev_err(&pdev->dev, "HSIC PHY PLL not locked after 100mS.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	/* Avoid SE0 state when resume for some device will take it as reset */
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~S2H_DRV_SE0_4RESUME,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	/* Enable HSIC PHY */
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) | PHY_28NM_HSIC_S2H_HSIC_EN,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	/*  Calibration Timing
>> +	*		   ____________________________
>> +	*  CAL START   ___|
>> +	*			   ____________________
>> +	*  CAL_DONE    ___________|
>> +	*		  | 400us |
>> +	*/
>
> Please use the comment style used here
> http://lxr.free-electrons.com/source/Documentation/CodingStyle#L464
>
>> +
>> +	/* Make sure PHY Calibration is ready */
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_IMPCAL_CAL,
>> +	    PHY_28NM_HSIC_H2S_IMPCAL_DONE, HZ / 10)) {
>> +		dev_warn(&pdev->dev, "HSIC PHY READY not set after 100mS.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	/* Waiting for HSIC connect int*/
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_INT,
>> +	    PHY_28NM_HSIC_CONNECT_INT, HZ / 5)) {
>> +		dev_warn(&pdev->dev, "HSIC wait for connect interrupt timeout.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mv_hsic_phy_power_on(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	struct platform_device *pdev = mv_phy->pdev;
>> +
>> +	/* check HS ready */
>> +	if (!wait_for_reg(mv_phy->base + PHY_28NM_HSIC_INT,
>> +	    PHY_28NM_HSIC_HS_READY_INT, HZ / 10)) {
>> +		dev_err(&pdev->dev, "HSIC HS_READY not set\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int mv_hsic_phy_power_off(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	void __iomem *base = mv_phy->base;
>> +
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~PHY_28NM_HSIC_S2H_HSIC_EN,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	clk_disable_unprepare(mv_phy->clk);
>> +	return 0;
>> +}
>> +
>> +static const struct phy_ops hsic_ops = {
>> +	.init		= mv_hsic_phy_init,
>> +	.power_on	= mv_hsic_phy_power_on,
>> +	.power_off	= mv_hsic_phy_power_off,
>
> exit callback is missing? Shouldn't we turn off the PLLs in exit callback?
Also add the .owner member since this driver can be used as module.

Thanks
Kishon

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Kishon Vijay Abraham I]

On Thursday 21 May 2015 06:15 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
>> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in PXA1928
>> SOC.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>    drivers/phy/Kconfig       |  10 +++
>>    drivers/phy/Makefile      |   1 +
>>    drivers/phy/phy-mv-hsic.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>>    3 files changed, 219 insertions(+)
>>    create mode 100644 drivers/phy/phy-mv-hsic.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index ef7634f..cfcae72 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -52,6 +52,16 @@ config PHY_EXYNOS_MIPI_VIDEO
>>    	  Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung S5P
>>    	  and EXYNOS SoCs.
>>
>> +config PHY_MV_HSIC
>> +	tristate "Marvell USB HSIC 28nm PHY Driver"
>> +	select GENERIC_PHY
>> +	help
>> +	  Enable this to support Marvell USB HSIC PHY driver for Marvell
>> +	  SoC. This driver will do the PHY initialization and shutdown.
>> +	  The PHY driver will be used by Marvell ehci driver.
>> +
>> +	  To compile this driver as a module, choose M here.
>> +
>>    config PHY_MV_USB2
>>    	tristate "Marvell USB 2.0 28nm PHY Driver"
>>    	select GENERIC_PHY
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 768e55a..472bf0a 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)	+= phy-armada375-usb2.o
>>    obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>>    obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>>    obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
>> +obj-$(CONFIG_PHY_MV_HSIC)		+= phy-mv-hsic.o
>>    obj-$(CONFIG_PHY_MV_USB2)		+= phy-mv-usb2.o
>>    obj-$(CONFIG_PHY_MVEBU_SATA)		+= phy-mvebu-sata.o
>>    obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>> diff --git a/drivers/phy/phy-mv-hsic.c b/drivers/phy/phy-mv-hsic.c
>> new file mode 100644
>> index 0000000..aa6cccd
>> --- /dev/null
>> +++ b/drivers/phy/phy-mv-hsic.c
>> @@ -0,0 +1,208 @@
>> +/*
>> + * Copyright (C) 2015 Linaro, Ltd.
>> + * Rob Herring <robh@kernel.org>
>> + *
>> + * Based on vendor driver:
>> + * Copyright (C) 2013 Marvell Inc.
>> + * Author: Chao Xie <xiechao.mail@gmail.com>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * 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/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#define PHY_28NM_HSIC_CTRL			0x08
>> +#define PHY_28NM_HSIC_IMPCAL_CAL		0x18
>> +#define PHY_28NM_HSIC_PLL_CTRL01		0x1c
>> +#define PHY_28NM_HSIC_PLL_CTRL2			0x20
>> +#define PHY_28NM_HSIC_INT			0x28
>> +
>> +#define PHY_28NM_HSIC_PLL_SELLPFR_SHIFT		26
>> +#define PHY_28NM_HSIC_PLL_FBDIV_SHIFT		0
>> +#define PHY_28NM_HSIC_PLL_REFDIV_SHIFT		9
>> +
>> +#define PHY_28NM_HSIC_S2H_PU_PLL		BIT(10)
>> +#define PHY_28NM_HSIC_H2S_PLL_LOCK		BIT(15)
>> +#define PHY_28NM_HSIC_S2H_HSIC_EN		BIT(7)
>> +#define S2H_DRV_SE0_4RESUME			BIT(14)
>> +#define PHY_28NM_HSIC_H2S_IMPCAL_DONE		BIT(27)
>> +
>> +#define PHY_28NM_HSIC_CONNECT_INT		BIT(1)
>> +#define PHY_28NM_HSIC_HS_READY_INT		BIT(2)
>> +
>> +struct mv_hsic_phy {
>> +	struct phy		*phy;
>> +	struct platform_device	*pdev;
>> +	void __iomem		*base;
>> +	struct clk		*clk;
>> +};
>> +
>> +static bool wait_for_reg(void __iomem *reg, u32 mask, unsigned long timeout)
>> +{
>> +	timeout += jiffies;
>> +	while (time_is_after_eq_jiffies(timeout)) {
>> +		if ((readl(reg) & mask) == mask)
>> +			return true;
>> +		msleep(1);
>> +	}
>> +	return false;
>> +}
>> +
>> +static int mv_hsic_phy_init(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	struct platform_device *pdev = mv_phy->pdev;
>> +	void __iomem *base = mv_phy->base;
>> +
>> +	clk_prepare_enable(mv_phy->clk);
>> +
>> +	/* Set reference clock */
>> +	writel(0x1 << PHY_28NM_HSIC_PLL_SELLPFR_SHIFT |
>> +		0xf0 << PHY_28NM_HSIC_PLL_FBDIV_SHIFT |
>> +		0xd << PHY_28NM_HSIC_PLL_REFDIV_SHIFT,
>> +		base + PHY_28NM_HSIC_PLL_CTRL01);
>> +
>> +	/* Turn on PLL */
>> +	writel(readl(base + PHY_28NM_HSIC_PLL_CTRL2) |
>> +		PHY_28NM_HSIC_S2H_PU_PLL,
>> +		base + PHY_28NM_HSIC_PLL_CTRL2);
>> +
>> +	/* Make sure PHY PLL is locked */
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_PLL_CTRL2,
>> +	    PHY_28NM_HSIC_H2S_PLL_LOCK, HZ / 10)) {
>> +		dev_err(&pdev->dev, "HSIC PHY PLL not locked after 100mS.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	/* Avoid SE0 state when resume for some device will take it as reset */
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~S2H_DRV_SE0_4RESUME,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	/* Enable HSIC PHY */
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) | PHY_28NM_HSIC_S2H_HSIC_EN,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	/*  Calibration Timing
>> +	*		   ____________________________
>> +	*  CAL START   ___|
>> +	*			   ____________________
>> +	*  CAL_DONE    ___________|
>> +	*		  | 400us |
>> +	*/
>
> Please use the comment style used here
> http://lxr.free-electrons.com/source/Documentation/CodingStyle#L464
>
>> +
>> +	/* Make sure PHY Calibration is ready */
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_IMPCAL_CAL,
>> +	    PHY_28NM_HSIC_H2S_IMPCAL_DONE, HZ / 10)) {
>> +		dev_warn(&pdev->dev, "HSIC PHY READY not set after 100mS.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	/* Waiting for HSIC connect int*/
>> +	if (!wait_for_reg(base + PHY_28NM_HSIC_INT,
>> +	    PHY_28NM_HSIC_CONNECT_INT, HZ / 5)) {
>> +		dev_warn(&pdev->dev, "HSIC wait for connect interrupt timeout.");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mv_hsic_phy_power_on(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	struct platform_device *pdev = mv_phy->pdev;
>> +
>> +	/* check HS ready */
>> +	if (!wait_for_reg(mv_phy->base + PHY_28NM_HSIC_INT,
>> +	    PHY_28NM_HSIC_HS_READY_INT, HZ / 10)) {
>> +		dev_err(&pdev->dev, "HSIC HS_READY not set\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int mv_hsic_phy_power_off(struct phy *phy)
>> +{
>> +	struct mv_hsic_phy *mv_phy = phy_get_drvdata(phy);
>> +	void __iomem *base = mv_phy->base;
>> +
>> +	writel(readl(base + PHY_28NM_HSIC_CTRL) & ~PHY_28NM_HSIC_S2H_HSIC_EN,
>> +		base + PHY_28NM_HSIC_CTRL);
>> +
>> +	clk_disable_unprepare(mv_phy->clk);
>> +	return 0;
>> +}
>> +
>> +static const struct phy_ops hsic_ops = {
>> +	.init		= mv_hsic_phy_init,
>> +	.power_on	= mv_hsic_phy_power_on,
>> +	.power_off	= mv_hsic_phy_power_off,
>
> exit callback is missing? Shouldn't we turn off the PLLs in exit callback?
Also add the .owner member since this driver can be used as module.

Thanks
Kishon

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Rob Herring]

On Thu, May 21, 2015 at 7:51 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On Thursday 21 May 2015 06:15 PM, Kishon Vijay Abraham I wrote:
>>
>> Hi,
>>
>> On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
>>>
>>> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in
>>> PXA1928
>>> SOC.

[...]

>>> +       writel(readl(base + PHY_28NM_HSIC_CTRL) &
>>> ~PHY_28NM_HSIC_S2H_HSIC_EN,
>>> +               base + PHY_28NM_HSIC_CTRL);
>>> +
>>> +       clk_disable_unprepare(mv_phy->clk);
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct phy_ops hsic_ops = {
>>> +       .init           = mv_hsic_phy_init,
>>> +       .power_on       = mv_hsic_phy_power_on,
>>> +       .power_off      = mv_hsic_phy_power_off,
>>
>>
>> exit callback is missing? Shouldn't we turn off the PLLs in exit callback?

I really don't understand the division of the ops functions. It seems
backwards to me. Don't you need to power on the phy before you can
initialize it? Or init is supposed to be s/w init of some kind.
AFAICT, all the drivers just call init and power_on back to back.

>
> Also add the .owner member since this driver can be used as module.

Strange. Generally an ops struct just has ops.

Rob

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Rob Herring]

On Thu, May 21, 2015 at 7:51 AM, Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org> wrote:
> On Thursday 21 May 2015 06:15 PM, Kishon Vijay Abraham I wrote:
>>
>> Hi,
>>
>> On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
>>>
>>> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in
>>> PXA1928
>>> SOC.

[...]

>>> +       writel(readl(base + PHY_28NM_HSIC_CTRL) &
>>> ~PHY_28NM_HSIC_S2H_HSIC_EN,
>>> +               base + PHY_28NM_HSIC_CTRL);
>>> +
>>> +       clk_disable_unprepare(mv_phy->clk);
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct phy_ops hsic_ops = {
>>> +       .init           = mv_hsic_phy_init,
>>> +       .power_on       = mv_hsic_phy_power_on,
>>> +       .power_off      = mv_hsic_phy_power_off,
>>
>>
>> exit callback is missing? Shouldn't we turn off the PLLs in exit callback?

I really don't understand the division of the ops functions. It seems
backwards to me. Don't you need to power on the phy before you can
initialize it? Or init is supposed to be s/w init of some kind.
AFAICT, all the drivers just call init and power_on back to back.

>
> Also add the .owner member since this driver can be used as module.

Strange. Generally an ops struct just has ops.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] phy: add Marvell HSIC 28nm PHY

[Kishon Vijay Abraham I]

Hi,

On Saturday 23 May 2015 01:24 AM, Rob Herring wrote:
> On Thu, May 21, 2015 at 7:51 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> On Thursday 21 May 2015 06:15 PM, Kishon Vijay Abraham I wrote:
>>>
>>> Hi,
>>>
>>> On Thursday 14 May 2015 04:18 AM, Rob Herring wrote:
>>>>
>>>> Add PHY driver for the Marvell HSIC 28nm PHY. This PHY is found in
>>>> PXA1928
>>>> SOC.
>
> [...]
>
>>>> +       writel(readl(base + PHY_28NM_HSIC_CTRL) &
>>>> ~PHY_28NM_HSIC_S2H_HSIC_EN,
>>>> +               base + PHY_28NM_HSIC_CTRL);
>>>> +
>>>> +       clk_disable_unprepare(mv_phy->clk);
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static const struct phy_ops hsic_ops = {
>>>> +       .init           = mv_hsic_phy_init,
>>>> +       .power_on       = mv_hsic_phy_power_on,
>>>> +       .power_off      = mv_hsic_phy_power_off,
>>>
>>>
>>> exit callback is missing? Shouldn't we turn off the PLLs in exit callback?
>
> I really don't understand the division of the ops functions. It seems
> backwards to me. Don't you need to power on the phy before you can
> initialize it? Or init is supposed to be s/w init of some kind.

Generally the order is init and then power on. During init the driver can 
perform the various setting for initializing the PHY which includes PLL 
programming, calibration etc.. And then power on which actually starts the PHY 
so that PHY can now transfer and receive data.
> AFAICT, all the drivers just call init and power_on back to back.
>
>>
>> Also add the .owner member since this driver can be used as module.
>
> Strange. Generally an ops struct just has ops.

phy-core has a call to try_module_get with this .owner so that the module is 
not removed after phy_get. Maybe we can make phy-core to handle it in some 
other way. Need to check.

Thanks
Kishon