USB support for Armada 38x and Armada 375

This commit updates the Device Tree binding documentation of ehci-orion to take into account the fact that we can now optionally pass a clock and a PHY reference.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Documentation/devicetree/bindings/usb/ehci-orion.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/usb/ehci-orion.txt +5 -0

index 6bc09ec..17c3bc8 100644
@@ -6,6 +6,11 @@ Required properties:
   region.
 - interrupts: The EHCI interrupt
 
+Optional properties:
+- clocks: reference to the clock
+- phys: reference to the USB PHY
+- phy-names: name of the USB PHY, should be "usb"
+
 Example:
 
 	ehci at 50000 {

Hi Alan,

Quoted 3 lines · Thomas Petazzoni

This commit updates the Device Tree binding documentation of ehci-orion to take into account the fact that we can now optionally pass a clock and a PHY reference.

Would you agree to take this patch with the 4 first ones?

For this kind of change in the device tree the agreement was that it can be directly taken by the maintainer of the subsystem. Indeed here we don't introduce a new kind of device tree node, we just add a optional resource already used in many place.

Quoted 22 lines

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Documentation/devicetree/bindings/usb/ehci-orion.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/usb/ehci-orion.txt +5 -0

index 6bc09ec..17c3bc8 100644
@@ -6,6 +6,11 @@ Required properties:
   region.
 - interrupts: The EHCI interrupt
 
+Optional properties:
+- clocks: reference to the clock
+- phys: reference to the USB PHY
+- phy-names: name of the USB PHY, should be "usb"
+
 Example:
 
 	ehci at 50000 {

Quoted 7 lines · Gregory CLEMENT

Hi Alan,

Quoted 3 lines · Thomas Petazzoni

This commit updates the Device Tree binding documentation of ehci-orion to take into account the fact that we can now optionally pass a clock and a PHY reference.

Quoted 6 lines

Would you agree to take this patch with the 4 first ones?

For this kind of change in the device tree the agreement was that it can be directly taken by the maintainer of the subsystem. Indeed here we don't introduce a new kind of device tree node, we just add a optional resource already used in many place.

Sorry, I missed this one. Is it okay with the DT people? If it is,

you can submit it directly to Greg KH and he'll merge it.

Alan Stern

Quoted 22 lines

Quoted 22 lines

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Documentation/devicetree/bindings/usb/ehci-orion.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/usb/ehci-orion.txt +5 -0

index 6bc09ec..17c3bc8 100644
@@ -6,6 +6,11 @@ Required properties:
   region.
 - interrupts: The EHCI interrupt
 
+Optional properties:
+- clocks: reference to the clock
+- phys: reference to the USB PHY
+- phy-names: name of the USB PHY, should be "usb"
+
 Example:
 
 	ehci at 50000 {

From: Gregory CLEMENT <[email protected]>

Some platforms (such as the Armada 38x ones) can gate the clock of their USB controller. This patch adds the support for one clock in xhci-plat, by enabling it during probe and disabling it on remove.

To achieve this, it adds a 'struct clk *' member in xhci_hcd. While only used for now in xhci-plat, it might be used by other drivers in the future. Moreover, the xhci_hcd structure already holds other members such as msix_count and msix_entries, which are MSI-X specific, and therefore only used by xhci-pci.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/usb/host/xhci-plat.c | 24 +++++++++++++++++++++++-
 drivers/usb/host/xhci.h      |  2 ++
 2 files changed, 25 insertions(+), 1 deletion(-)

drivers/usb/host/xhci-plat.c +23 -1

index f5351af..8108e58 100644
@@ -11,6 +11,7 @@
  * version 2 as published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>

@@ -91,6 +92,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct xhci_hcd		*xhci;
 	struct resource         *res;
 	struct usb_hcd		*hcd;
+	struct clk              *clk;
 	int			ret;
 	int			irq;
 

@@ -137,14 +139,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		goto release_mem_region;
 	}
 
+	/*
+	 * Not all platforms have a clk so it is not an error if the
+	 * clock does not exists.
+	 */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(clk)) {
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			goto unmap_registers;
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
-		goto unmap_registers;
+		goto disable_clk;
+
 	device_wakeup_enable(hcd->self.controller);
 
 	/* USB 2.0 roothub is stored in the platform_device now. */
 	hcd = platform_get_drvdata(pdev);
 	xhci = hcd_to_xhci(hcd);
+	xhci->clk = clk;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {

@@ -173,6 +188,10 @@ put_usb3_hcd:
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
+disable_clk:
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
+
 unmap_registers:
 	iounmap(hcd->regs);
 

@@ -189,11 +208,14 @@ static int xhci_plat_remove(struct platform_device *dev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
 	usb_remove_hcd(hcd);
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);

drivers/usb/host/xhci.h +2 -0

index d280e92..003dc09 100644
@@ -1478,6 +1478,8 @@ struct xhci_hcd {
 	/* msi-x vectors */
 	int		msix_count;
 	struct msix_entry	*msix_entries;
+	/* optional clock */
+	struct clk		*clk;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;

Hi Mathias, Felipe,

Quoted 11 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

Some platforms (such as the Armada 38x ones) can gate the clock of their USB controller. This patch adds the support for one clock in xhci-plat, by enabling it during probe and disabling it on remove.

To achieve this, it adds a 'struct clk *' member in xhci_hcd. While only used for now in xhci-plat, it might be used by other drivers in the future. Moreover, the xhci_hcd structure already holds other members such as msix_count and msix_entries, which are MSI-X specific, and therefore only used by xhci-pci.

Felpie,

would you agree to add your acked-by on this patch. You gave it on the 3rd version however as pointed by Thomas there was some issues, what do you think of this version?

Mathias,

as the xHCI maintainer what is your opinion on this patch. Do you plan to take it? do you expect a pull request?

We submitted the first version of this series one month ago and we didn't receive any feedback from you. Maybe it was just because you were happy with the patch sent, but I would like to be sure that you noticed this series.

I have the same question for the patch before: "[PATCHv5 06/20] usb: host:
xhci-plat: sort the headers in alphabetic order" and the three following:
"[PATCHv5 07/20] usb: host: xhci-plat: add clock support", "[PATCHv5 08/20]
usb: host: xhci-plat: add support for the Armada 375/38x XHCI controllers"
and "[PATCHv5 09/20] Documentation: dt-bindings: update xhci-platform DT binding".

From: Gregory CLEMENT <[email protected]>

Some platforms (such as the Armada 38x ones) can gate the clock of their USB controller. This patch adds the support for one clock in xhci-plat, by enabling it during probe and disabling it on remove.

To achieve this, it adds a 'struct clk *' member in xhci_hcd. While only used for now in xhci-plat, it might be used by other drivers in the future. Moreover, the xhci_hcd structure already holds other members such as msix_count and msix_entries, which are MSI-X specific, and therefore only used by xhci-pci.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/usb/host/xhci-plat.c | 24 +++++++++++++++++++++++-
 drivers/usb/host/xhci.h      |  2 ++
 2 files changed, 25 insertions(+), 1 deletion(-)

drivers/usb/host/xhci-plat.c +23 -1

index f5351af..8108e58 100644
@@ -11,6 +11,7 @@
  * version 2 as published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>

@@ -91,6 +92,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct xhci_hcd		*xhci;
 	struct resource         *res;
 	struct usb_hcd		*hcd;
+	struct clk              *clk;
 	int			ret;
 	int			irq;
 

@@ -137,14 +139,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		goto release_mem_region;
 	}
 
+	/*
+	 * Not all platforms have a clk so it is not an error if the
+	 * clock does not exists.
+	 */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(clk)) {
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			goto unmap_registers;
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
-		goto unmap_registers;
+		goto disable_clk;
+
 	device_wakeup_enable(hcd->self.controller);
 
 	/* USB 2.0 roothub is stored in the platform_device now. */
 	hcd = platform_get_drvdata(pdev);
 	xhci = hcd_to_xhci(hcd);
+	xhci->clk = clk;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {

@@ -173,6 +188,10 @@ put_usb3_hcd:
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
+disable_clk:
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
+
 unmap_registers:
 	iounmap(hcd->regs);
 

@@ -189,11 +208,14 @@ static int xhci_plat_remove(struct platform_device *dev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
 	usb_remove_hcd(hcd);
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);

drivers/usb/host/xhci.h +2 -0

index d280e92..003dc09 100644
@@ -1478,6 +1478,8 @@ struct xhci_hcd {
 	/* msi-x vectors */
 	int		msix_count;
 	struct msix_entry	*msix_entries;
+	/* optional clock */
+	struct clk		*clk;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;

Signature · 1 line

1.9.2

From: Gregory CLEMENT <[email protected]>

The Armada 375 and 38x SoCs come with an XHCI controller that requires some specific initialization related to the MBus windows configuration. This patch adds the support for this special configuration as an XHCI quirk executed during probe.

Two new compatible strings are added to identify the Armada 375 and Armada 38x XHCI controllers, and therefore enable the relevant quirk.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
  drivers/usb/host/Kconfig      |  8 +++++
  drivers/usb/host/Makefile     |  3 ++
  drivers/usb/host/xhci-mvebu.c | 70 +++++++++++++++++++++++++++++++++++++++++++
  drivers/usb/host/xhci-mvebu.h | 21 +++++++++++++
  drivers/usb/host/xhci-plat.c  | 12 ++++++++
  5 files changed, 114 insertions(+)

create mode 100644 drivers/usb/host/xhci-mvebu.c create mode 100644 drivers/usb/host/xhci-mvebu.h

drivers/usb/host/Kconfig +8 -0

index 3d9e540..9247ad2 100644
@@ -29,6 +29,14 @@ if USB_XHCI_HCD
  config USB_XHCI_PLATFORM
  	tristate
 
+config USB_XHCI_MVEBU
+	tristate "xHCI support for Marvell Armada 375/38x"
+	select USB_XHCI_PLATFORM
+	depends on ARCH_MVEBU || COMPILE_TEST
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  found in Marvell Armada 375/38x ARM SOCs.
+
  endif # USB_XHCI_HCD
 
  config USB_EHCI_HCD

drivers/usb/host/Makefile +3 -0

index 7530468..7c0886a 100644
@@ -19,6 +19,9 @@ xhci-hcd-$(CONFIG_PCI)	+= xhci-pci.o
 
  ifneq ($(CONFIG_USB_XHCI_PLATFORM), )
  	xhci-hcd-y		+= xhci-plat.o
+ifneq ($(CONFIG_USB_XHCI_MVEBU), )
+	xhci-hcd-y		+= xhci-mvebu.o
+endif
  endif
 
  obj-$(CONFIG_USB_WHCI_HCD)	+= whci/

drivers/usb/host/xhci-mvebu.c new file +36 -0

new file mode 100644
index 0000000..fab9d6f
@@ -0,0 +1,70 @@
+/*
+ * Copyright (C) 2014 Marvell
+ * Author: Gregory CLEMENT <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/io.h>
+#include <linux/mbus.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define USB3_MAX_WINDOWS	4
+#define USB3_WIN_CTRL(w)	(0x0 + ((w) * 8))
+#define USB3_WIN_BASE(w)	(0x4 + ((w) * 8))
+
+static void xhci_mvebu_mbus_config(void __iomem *base,
+			const struct mbus_dram_target_info *dram)
+{
+	int win;
+
+	/* Clear all existing windows */
+	for (win = 0; win < USB3_MAX_WINDOWS; win++) {
+		writel(0, base + USB3_WIN_CTRL(win));
+		writel(0, base + USB3_WIN_BASE(win));
+	}
+
+	/* Program each DRAM CS in a seperate window */
+	for (win = 0; win < dram->num_cs; win++) {
+		const struct mbus_dram_window *cs = dram->cs + win;
+
+		writel(((cs->size - 1) & 0xffff0000) | (cs->mbus_attr << 8) |
+		       (dram->mbus_dram_target_id << 4) | 1,
+		       base + USB3_WIN_CTRL(win));

drivers/usb/host/xhci-mvebu.c line 46

Quoted 5 lines

drivers/usb/host/xhci-mvebu.c new file +5 -0

+int xhci_mvebu_mbus_init_quirk(struct platform_device *pdev)
+{
+	struct resource	*res;
+	void __iomem *base;
+	const struct mbus_dram_target_info *dram;

Hi

Sparse warns about this:

drivers/usb/host/xhci-mvebu.c:42:5: warning: symbol 'xhci_mvebu_mbus_init_quirk' was not declared. Should it be static?

Dear Mathias Nyman,

Quoted 8 lines · Mathias Nyman

drivers/usb/host/xhci-mvebu.c line 46

Quoted 5 lines

drivers/usb/host/xhci-mvebu.c new file +5 -0

+int xhci_mvebu_mbus_init_quirk(struct platform_device *pdev)
+{
+	struct resource	*res;
+	void __iomem *base;
+	const struct mbus_dram_target_info *dram;

Hi

Quoted 4 lines

Sparse warns about this:

drivers/usb/host/xhci-mvebu.c:42:5: warning: symbol 'xhci_mvebu_mbus_init_quirk' was not declared. Should it be static?

Not, it should not: it gets called from xhci-plat.c. However, xhci-mvebu.c should include its header xhci-mvebu.h so that sparse realize that the function is voluntarily exported, and that therefore not having the static qualifier is expected.

Should we resend a v6 with just this change?

From: Gregory CLEMENT <[email protected]>

This commit extends the compatible string list of the xhci-platform binding with the new "armada-375-xhci" and "armada-380-xhci" compatible strings. It is used to describe the XHCI controller which is available in the Armada 375 and 38x SoCs.

It also indicates that an optional 'clocks' property is now supported.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
  Documentation/devicetree/bindings/usb/usb-xhci.txt | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +157 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+
+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;
+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);
+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {
+		phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY n%d\n", i);
+			return PTR_ERR(phy);
+		}
+
+		usb_cluster_phy[i].phy = phy;
+		usb_cluster_phy[i].reg = usb_cluster_base;
+		usb_cluster_phy[i].enable = false;
+		phy_set_drvdata(phy, &usb_cluster_phy[i]);
+	}
+
+	usb_cluster_phy[PHY_USB2].use_usb3 = false;
+	usb_cluster_phy[PHY_USB3].use_usb3 = true;
+
+	/*
+	 * We can't use the first usb2 unit and usb3 at the same time
+	 * to manage a USB2 device, so let's disable usb2 if usb3 is
+	 * selected. In this case the USB2 device will be managed by
+	 * the xhci controller.
+	 */
+
+	xhci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,armada-375-xhci");
+
+	if (xhci_node && of_device_is_available(xhci_node)) {
+		usb_cluster_phy[PHY_USB3].enable = true;
+	} else {
+		struct device_node *ehci_node;
+		ehci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,orion-ehci");
+		if (ehci_node && of_device_is_available(ehci_node))
+			usb_cluster_phy[PHY_USB2].enable = true;
+		of_node_put(ehci_node);
+	}
+
+	of_node_put(xhci_node);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     armada375_usb_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id of_usb_cluster_table[] = {
+	{ .compatible = "marvell,armada-375-usb-cluster", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
+
+static struct platform_driver armada375_usb_phy_driver = {
+	.probe	= armada375_usb_phy_probe,
+	.driver = {
+		.of_match_table	= of_usb_cluster_table,
+		.name  = "armada-375-usb-cluster",
+		.owner = THIS_MODULE,
+	}
+};
+module_platform_driver(armada375_usb_phy_driver);
+
+MODULE_DESCRIPTION("Armada 375 USB cluster driver");
+MODULE_AUTHOR("Gregory CLEMENT <[email protected]>");
+MODULE_LICENSE("GPL");

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +38 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;

drivers/phy/phy-armada375-usb2.c new file +24 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;

drivers/phy/phy-armada375-usb2.c new file +14 -0

+		phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY n%d\n", i);
+			return PTR_ERR(phy);
+		}
+
+		usb_cluster_phy[i].phy = phy;
+		usb_cluster_phy[i].reg = usb_cluster_base;
+		usb_cluster_phy[i].enable = false;
+		phy_set_drvdata(phy, &usb_cluster_phy[i]);
+	}
+
+	usb_cluster_phy[PHY_USB2].use_usb3 = false;
+	usb_cluster_phy[PHY_USB3].use_usb3 = true;

drivers/phy/phy-armada375-usb2.c new file +31 -0

+
+	if (xhci_node && of_device_is_available(xhci_node)) {
+		usb_cluster_phy[PHY_USB3].enable = true;
+	} else {
+		struct device_node *ehci_node;
+		ehci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,orion-ehci");
+		if (ehci_node && of_device_is_available(ehci_node))
+			usb_cluster_phy[PHY_USB2].enable = true;
+		of_node_put(ehci_node);
+	}
+
+	of_node_put(xhci_node);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     armada375_usb_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id of_usb_cluster_table[] = {
+	{ .compatible = "marvell,armada-375-usb-cluster", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
+
+static struct platform_driver armada375_usb_phy_driver = {
+	.probe	= armada375_usb_phy_probe,
+	.driver = {

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

drivers/phy/phy-armada375-usb2.c line 58

Quoted 10 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Quoted 18 lines

drivers/phy/phy-armada375-usb2.c new file +18 -0

+		phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY n%d\n", i);
+			return PTR_ERR(phy);
+		}
+
+		usb_cluster_phy[i].phy = phy;
+		usb_cluster_phy[i].reg = usb_cluster_base;
+		usb_cluster_phy[i].enable = false;
+		phy_set_drvdata(phy, &usb_cluster_phy[i]);
+	}
+
+	usb_cluster_phy[PHY_USB2].use_usb3 = false;
+	usb_cluster_phy[PHY_USB3].use_usb3 = true;
+
+	/*
+	 * We can't use the first usb2 unit and usb3 at the same time
+	 * to manage a USB2 device, so let's disable usb2 if usb3 is

drivers/phy/phy-armada375-usb2.c line 116

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+	 * selected. In this case the USB2 device will be managed by
+	 * the xhci controller.
+	 */
+
+	xhci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,armada-375-xhci");

huh.. that's too much binding between the controller and the PHY.

Quoted 35 lines

drivers/phy/phy-armada375-usb2.c new file +35 -0

+
+	if (xhci_node && of_device_is_available(xhci_node)) {
+		usb_cluster_phy[PHY_USB3].enable = true;
+	} else {
+		struct device_node *ehci_node;
+		ehci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,orion-ehci");
+		if (ehci_node && of_device_is_available(ehci_node))
+			usb_cluster_phy[PHY_USB2].enable = true;
+		of_node_put(ehci_node);
+	}
+
+	of_node_put(xhci_node);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     armada375_usb_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id of_usb_cluster_table[] = {
+	{ .compatible = "marvell,armada-375-usb-cluster", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
+
+static struct platform_driver armada375_usb_phy_driver = {
+	.probe	= armada375_usb_phy_probe,
+	.driver = {
+		.of_match_table	= of_usb_cluster_table,
+		.name  = "armada-375-usb-cluster",
+		.owner = THIS_MODULE,
+	}

drivers/phy/phy-armada375-usb2.c line 157

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+};
+module_platform_driver(armada375_usb_phy_driver);
+
+MODULE_DESCRIPTION("Armada 375 USB cluster driver");
+MODULE_AUTHOR("Gregory CLEMENT <[email protected]>");
+MODULE_LICENSE("GPL");

GPL v2?

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Right

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 20 lines

Quoted 20 lines

drivers/phy/phy-armada375-usb2.c new file +18 -0

+		phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY n%d\n", i);
+			return PTR_ERR(phy);
+		}
+
+		usb_cluster_phy[i].phy = phy;
+		usb_cluster_phy[i].reg = usb_cluster_base;
+		usb_cluster_phy[i].enable = false;
+		phy_set_drvdata(phy, &usb_cluster_phy[i]);
+	}
+
+	usb_cluster_phy[PHY_USB2].use_usb3 = false;
+	usb_cluster_phy[PHY_USB3].use_usb3 = true;
+
+	/*
+	 * We can't use the first usb2 unit and usb3 at the same time
+	 * to manage a USB2 device, so let's disable usb2 if usb3 is

Quoted 9 lines

drivers/phy/phy-armada375-usb2.c line 116

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+	 * selected. In this case the USB2 device will be managed by
+	 * the xhci controller.
+	 */
+
+	xhci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,armada-375-xhci");

huh.. that's too much binding between the controller and the PHY.

That's why initially it was not part of the PHY framework. The USB cluster is really a part managing common feature between the USB controllers which are part of the Armada 375 SoC.

However the initial version was not really good, because this piece of code was located in the mach- directory whereas we are trying to move most of the files out of this directory now. The USB cluster is not a real PHY as it is related to the PHY management this framework remains the best place for it.

Quoted 35 lines

Quoted 35 lines

drivers/phy/phy-armada375-usb2.c new file +35 -0

+
+	if (xhci_node && of_device_is_available(xhci_node)) {
+		usb_cluster_phy[PHY_USB3].enable = true;
+	} else {
+		struct device_node *ehci_node;
+		ehci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,orion-ehci");
+		if (ehci_node && of_device_is_available(ehci_node))
+			usb_cluster_phy[PHY_USB2].enable = true;
+		of_node_put(ehci_node);
+	}
+
+	of_node_put(xhci_node);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     armada375_usb_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id of_usb_cluster_table[] = {
+	{ .compatible = "marvell,armada-375-usb-cluster", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
+
+static struct platform_driver armada375_usb_phy_driver = {
+	.probe	= armada375_usb_phy_probe,
+	.driver = {
+		.of_match_table	= of_usb_cluster_table,
+		.name  = "armada-375-usb-cluster",
+		.owner = THIS_MODULE,
+	}

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 157

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+};
+module_platform_driver(armada375_usb_phy_driver);
+
+MODULE_DESCRIPTION("Armada 375 USB cluster driver");
+MODULE_AUTHOR("Gregory CLEMENT <[email protected]>");
+MODULE_LICENSE("GPL");

GPL v2?

No it is really GPL v2 or latter as written at the top of this file

Thanks,

Gregory

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 47 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

Quoted 3 lines

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Quoted 91 lines

Quoted 20 lines

Quoted 20 lines

drivers/phy/phy-armada375-usb2.c new file +18 -0

+		phy = devm_phy_create(dev, &armada375_usb_phy_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY n%d\n", i);
+			return PTR_ERR(phy);
+		}
+
+		usb_cluster_phy[i].phy = phy;
+		usb_cluster_phy[i].reg = usb_cluster_base;
+		usb_cluster_phy[i].enable = false;
+		phy_set_drvdata(phy, &usb_cluster_phy[i]);
+	}
+
+	usb_cluster_phy[PHY_USB2].use_usb3 = false;
+	usb_cluster_phy[PHY_USB3].use_usb3 = true;
+
+	/*
+	 * We can't use the first usb2 unit and usb3 at the same time
+	 * to manage a USB2 device, so let's disable usb2 if usb3 is

Quoted 9 lines

drivers/phy/phy-armada375-usb2.c line 116

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+	 * selected. In this case the USB2 device will be managed by
+	 * the xhci controller.
+	 */
+
+	xhci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,armada-375-xhci");

huh.. that's too much binding between the controller and the PHY.

That's why initially it was not part of the PHY framework. The USB cluster is really a part managing common feature between the USB controllers which are part of the Armada 375 SoC.

However the initial version was not really good, because this piece of code was located in the mach- directory whereas we are trying to move most of the files out of this directory now. The USB cluster is not a real PHY as it is related to the PHY management this framework remains the best place for it.

Quoted 35 lines

Quoted 35 lines

drivers/phy/phy-armada375-usb2.c new file +35 -0

+
+	if (xhci_node && of_device_is_available(xhci_node)) {
+		usb_cluster_phy[PHY_USB3].enable = true;
+	} else {
+		struct device_node *ehci_node;
+		ehci_node = of_find_compatible_node(NULL, NULL,
+					"marvell,orion-ehci");
+		if (ehci_node && of_device_is_available(ehci_node))
+			usb_cluster_phy[PHY_USB2].enable = true;
+		of_node_put(ehci_node);
+	}
+
+	of_node_put(xhci_node);
+
+	phy_provider = devm_of_phy_provider_register(&pdev->dev,
+						     armada375_usb_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id of_usb_cluster_table[] = {
+	{ .compatible = "marvell,armada-375-usb-cluster", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, of_usb_cluster_table);
+
+static struct platform_driver armada375_usb_phy_driver = {
+	.probe	= armada375_usb_phy_probe,
+	.driver = {
+		.of_match_table	= of_usb_cluster_table,
+		.name  = "armada-375-usb-cluster",
+		.owner = THIS_MODULE,
+	}

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 157

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+};
+module_platform_driver(armada375_usb_phy_driver);
+
+MODULE_DESCRIPTION("Armada 375 USB cluster driver");
+MODULE_AUTHOR("Gregory CLEMENT <[email protected]>");
+MODULE_LICENSE("GPL");

GPL v2?

No it is really GPL v2 or latter as written at the top of this file

Thanks,

Gregory

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 47 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

Quoted 3 lines

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Hi,

Quoted 181 lines · Gregory CLEMENT

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 50 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 4 lines

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Alright, so you have a single PHY that can be used by either XHCI or EHCI? And the use of PHY is mutually exclusive? How should it behave if you have both XHCI and EHCI?

One way to configure the PHY to a particular mode is by passing it as phandle arguments. I think you can use that to enable or disable USB2_PHY_CONFIG_DISABLE?

Thanks
Kishon

Quoted 184 lines · Kishon Vijay Abraham I

Hi,

Quoted 181 lines · Gregory CLEMENT

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 50 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 8 lines

Quoted 4 lines

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Alright, so you have a single PHY that can be used by either XHCI or EHCI? And the use of PHY is mutually exclusive? How should it behave if you have both XHCI and EHCI?

if we have both XHCI and EHCI then it is the USB2_PHY_CONFIG_DISABLE which determine which one is used. By default we decide to select the XHCI.

Quoted 3 lines

One way to configure the PHY to a particular mode is by passing it as phandle arguments. I think you can use that to enable or disable USB2_PHY_CONFIG_DISABLE?

actually it was more or less what I do:
for the XHCI I use:
phys = <&usbcluster 1>;
which enable the USB2_PHY_CONFIG_DISABLE

for the EHCI I use phys = <&usbcluster 0>;
which disable the USB2_PHY_CONFIG_DISABLE

If I had to create two PHY it was because of the behavior of phy_init(). I need to be able to disable a controller if it can't use the PHY. For this purpose my ops->init() exits in error. However phy_init() will call ops->init() only one time, then the internal counter init_count will be incremented, and the next call to phy_init will skip the call to ops->init. And the behavior is the same for phy_power_on().

So given this I don't see how to do in an other way except by modifying the value of the counter in my ops.

Thanks,

Gregory

Quoted 4 lines

Thanks
Kishon

Hi Kishon,

Quoted 217 lines · Gregory CLEMENT

Quoted 184 lines · Kishon Vijay Abraham I

Hi,

Quoted 181 lines · Gregory CLEMENT

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 50 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 8 lines

Quoted 4 lines

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Alright, so you have a single PHY that can be used by either XHCI or EHCI? And the use of PHY is mutually exclusive? How should it behave if you have both XHCI and EHCI?

if we have both XHCI and EHCI then it is the USB2_PHY_CONFIG_DISABLE which determine which one is used. By default we decide to select the XHCI.

Quoted 3 lines

One way to configure the PHY to a particular mode is by passing it as phandle arguments. I think you can use that to enable or disable USB2_PHY_CONFIG_DISABLE?

actually it was more or less what I do:
for the XHCI I use:
phys = <&usbcluster 1>;
which enable the USB2_PHY_CONFIG_DISABLE

for the EHCI I use phys = <&usbcluster 0>;
which disable the USB2_PHY_CONFIG_DISABLE

If I had to create two PHY it was because of the behavior of phy_init(). I need to be able to disable a controller if it can't use the PHY. For this purpose my ops->init() exits in error. However phy_init() will call ops->init() only one time, then the internal counter init_count will be incremented, and the next call to phy_init will skip the call to ops->init. And the behavior is the same for phy_power_on().

Quoted 2 lines

So given this I don't see how to do in an other way except by modifying the value of the counter in my ops.

What do you prefer here? Keep the current implementation or using only one PHY by passing a argument through the phandle and in the same time hacking the init_count?

Hi,

Quoted 220 lines · Gregory CLEMENT

Hi Kishon,

Quoted 217 lines · Gregory CLEMENT

Quoted 184 lines · Kishon Vijay Abraham I

Hi,

Quoted 181 lines · Gregory CLEMENT

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 50 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 8 lines

Quoted 4 lines

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Alright, so you have a single PHY that can be used by either XHCI or EHCI? And the use of PHY is mutually exclusive? How should it behave if you have both XHCI and EHCI?

if we have both XHCI and EHCI then it is the USB2_PHY_CONFIG_DISABLE which determine which one is used. By default we decide to select the XHCI.

Quoted 3 lines

One way to configure the PHY to a particular mode is by passing it as phandle arguments. I think you can use that to enable or disable USB2_PHY_CONFIG_DISABLE?

actually it was more or less what I do:
for the XHCI I use:
phys = <&usbcluster 1>;
which enable the USB2_PHY_CONFIG_DISABLE

for the EHCI I use phys = <&usbcluster 0>;
which disable the USB2_PHY_CONFIG_DISABLE

If I had to create two PHY it was because of the behavior of phy_init(). I need to be able to disable a controller if it can't use the PHY. For this purpose my ops->init() exits in error. However phy_init() will call ops->init() only one time, then the internal counter init_count will be incremented, and the next call to phy_init will skip the call to ops->init. And the behavior is the same for phy_power_on().

Quoted 6 lines

Quoted 2 lines

So given this I don't see how to do in an other way except by modifying the value of the counter in my ops.

What do you prefer here? Keep the current implementation or using only one PHY by passing a argument through the phandle and in the same time hacking the init_count?

I clearly don't prefer having of_find_compatible_node in PHY driver code. If you can find something without using that, it would be good.

Maybe create an API in phy-core that invokes the call-backs without caring about the ref count and implement some sort of mutual exclusivity in your driver? However this needs to be reviewed and discussed.

Hi Kishon,

Quoted 230 lines · Kishon Vijay Abraham I

Hi,

Quoted 220 lines · Gregory CLEMENT

Hi Kishon,

Quoted 217 lines · Gregory CLEMENT

Quoted 184 lines · Kishon Vijay Abraham I

Hi,

Quoted 181 lines · Gregory CLEMENT

Quoted 108 lines · Gregory CLEMENT

Quoted 98 lines · Kishon Vijay Abraham I

Hi,

Quoted 95 lines · Thomas Petazzoni

From: Gregory CLEMENT <[email protected]>

The Armada 375 SoC comes with an USB2 host and device controller and an USB3 controller. The USB cluster control register allows to manage common features of both USB controllers.

This commit adds a driver integrated in the generic PHY framework to control this USB cluster feature.

Signed-off-by Gregory CLEMENT <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 drivers/phy/Kconfig              |   6 ++
 drivers/phy/Makefile             |   1 +
 drivers/phy/phy-armada375-usb2.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/phy/phy-armada375-usb2.c

drivers/phy/Kconfig +6 -0

index 3bb05f1..e63cf9d 100644
@@ -15,6 +15,12 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config ARMADA375_USBCLUSTER_PHY
+	def_bool y
+	depends on MACH_ARMADA_375 || COMPILE_TEST
+	depends on OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM

drivers/phy/Makefile +1 -0

index 2faf78e..47d5a86 100644
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+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

drivers/phy/phy-armada375-usb2.c new file +42 -0

new file mode 100644
index 0000000..a6f746d
@@ -0,0 +1,157 @@
+/*
+ * USB cluster support for Armada 375 platform.
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ *
+ * Armada 375 comes with an USB2 host and device controller and an
+ * USB3 controller. The USB cluster control register allows to manage
+ * common features of both USB controllers.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define USB2_PHY_CONFIG_DISABLE BIT(0)
+
+/* The USB cluster allows to choose between two PHYs */
+#define NB_PHY 2
+
+enum {
+	PHY_USB2 = 0,
+	PHY_USB3 = 1,
+};
+
+struct armada375_cluster_phy {
+	struct phy *phy;
+	void __iomem *reg;
+	bool enable;
+	bool use_usb3;
+};
+

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 48

Quoted 6 lines

drivers/phy/phy-armada375-usb2.c new file +6 -0

+struct armada375_cluster_phy usb_cluster_phy[NB_PHY];
+
+static int armada375_usb_phy_init(struct phy *phy)
+{
+	struct armada375_cluster_phy *cluster_phy = phy_get_drvdata(phy);
+	u32 reg;

This function should be protected since both your PHYs use this ops.

Quoted 1 line

Right

Actually only one PHY can access this register. See the probe function, cluster_phy->enable is only set to true for one PHY.

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-armada375-usb2.c line 58

Quoted 11 lines

drivers/phy/phy-armada375-usb2.c new file +10 -0

+
+	if (!cluster_phy->enable)
+		return -ENODEV;
+
+	reg = readl(cluster_phy->reg);
+	if (cluster_phy->use_usb3)
+		reg |= USB2_PHY_CONFIG_DISABLE;
+	else
+		reg &= ~USB2_PHY_CONFIG_DISABLE;
+	writel(reg, cluster_phy->reg);

This is confusing since both your PHYs control the same bit?

Same here at the end the bit is accessed by only one PHY.

Quoted 50 lines

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-armada375-usb2.c new file +29 -0

+
+	return 0;
+}
+
+static struct phy_ops armada375_usb_phy_ops = {
+	.init = armada375_usb_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *armada375_usb_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	if (WARN_ON(args->args[0] >= NB_PHY))
+		return ERR_PTR(-ENODEV);
+
+	return usb_cluster_phy[args->args[0]].phy;
+}
+
+static int armada375_usb_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	void __iomem *usb_cluster_base;
+	struct device_node *xhci_node;
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-armada375-usb2.c line 92

Quoted 5 lines

drivers/phy/phy-armada375-usb2.c new file +5 -0

+	usb_cluster_base = devm_ioremap_resource(&pdev->dev, res);
+	if (!usb_cluster_base)
+		return -ENOMEM;
+
+	for (i = 0; i < NB_PHY; i++) {

For devices which have multiple PHYs, each PHY should be modelled as the sub-node of the *PHY provider* device node.

Actually it is the opposite the same PHY is shared between the EHCI and the xHCI controllers. It is more a PHY muxer than a PHY itself.

I had to create 2 logical PHYs because once the phy_init() is called by a USB driver then the .init ops is not called anymore by the next call to phy_init(). One of the goal of this is to disable a port for the USB controller which can't use it due to the configuration of the USB cluster.

But I can see how to make this two "pseudo" PHYs sub-node of the *PHY provider* device node. It shouldn't change the internal logic of this driver.

Quoted 8 lines

Quoted 4 lines

I need to make a distinction when the PHY access by the xHCI or when it was access by the EHCI. If I create two new sub-node then I will also need to add a property to make this distinction. It seems a little overkill for the need.

Alright, so you have a single PHY that can be used by either XHCI or EHCI? And the use of PHY is mutually exclusive? How should it behave if you have both XHCI and EHCI?

if we have both XHCI and EHCI then it is the USB2_PHY_CONFIG_DISABLE which determine which one is used. By default we decide to select the XHCI.

Quoted 3 lines

One way to configure the PHY to a particular mode is by passing it as phandle arguments. I think you can use that to enable or disable USB2_PHY_CONFIG_DISABLE?

actually it was more or less what I do:
for the XHCI I use:
phys = <&usbcluster 1>;
which enable the USB2_PHY_CONFIG_DISABLE

for the EHCI I use phys = <&usbcluster 0>;
which disable the USB2_PHY_CONFIG_DISABLE

If I had to create two PHY it was because of the behavior of phy_init(). I need to be able to disable a controller if it can't use the PHY. For this purpose my ops->init() exits in error. However phy_init() will call ops->init() only one time, then the internal counter init_count will be incremented, and the next call to phy_init will skip the call to ops->init. And the behavior is the same for phy_power_on().

Quoted 6 lines

Quoted 2 lines

So given this I don't see how to do in an other way except by modifying the value of the counter in my ops.

What do you prefer here? Keep the current implementation or using only one PHY by passing a argument through the phandle and in the same time hacking the init_count?

Quoted 6 lines

I clearly don't prefer having of_find_compatible_node in PHY driver code. If you can find something without using that, it would be good.

Maybe create an API in phy-core that invokes the call-backs without caring about the ref count and implement some sort of mutual exclusivity in your driver? However this needs to be reviewed and discussed.

Thanks for your feedback. This feature (usb cluster) is a nice to have for the Armada 375 SoC, but is not mandatory for the USB support on this SoC.