All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-23 22:06 ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-05-23 22:06 UTC (permalink / raw)
  To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

This PHY, though formally being a part of Renesas USBHS controller, contains the
UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
channels) to the different USB controllers: channel 0 can be connected to either
PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
or xHCI controllers.

This is a new driver for this USB PHY currently already supported under drivers/
usb/phy/. The reason for writing the new driver was the requirement that the
multiplexing of USB channels to the controller be dynamic, depending on what
USB drivers are loaded,  rather than static as provided by the old driver.
The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
ideally. The new driver only supports device tree probing for now.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.

Changes in version 4:
- added subnodes to the USB PHY node, described their properties in the binding
  document; rewrote the probe() method to parse out PHY selection and UGCTRL2
  mask values out of these child nodes;
- changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
  don't have control over it anyway;
- refreshed the patch.

Changes in version 3:
- removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
  'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
  methods;
- renamed the power_{on|off}() methods;
- replaced io{read|write}16() calls with {read|write}w();
- removed the comment to '#define USBHS_UGSTS_LOCK';
- broke the line in the dev_err() call in the probe() method;
- moved the driver's line in the 'Makefile' before OMAP drivers.

Changes in version 2:
- rebased the patch, resolving reject.

 Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
 drivers/phy/Kconfig                                     |    7 
 drivers/phy/Makefile                                    |    1 
 drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++++++
 4 files changed, 356 insertions(+)

Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
=================================--- /dev/null
+++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
@@ -0,0 +1,60 @@
+* Renesas R-Car generation 2 USB PHY
+
+This file provides information on what the device node for the R-Car generation
+2 USB PHY contains.
+
+Required properties:
+- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
+	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
+- reg: offset and length of the register block.
+- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
+- clocks: clock phandle and specifier pair.
+- clock-names: string, clock input name, must be "usbhs".
+
+The phandle's first argument in the PHY specifier identifies the USB channel,
+the second one is the USB controller selector and depends on the first:
+
++-----------+---------------+---------------+
+|\ Selector |               |               |
++ --------- +       0       |       1       |
+| Channel  \|               |               |
++-----------+---------------+---------------+
+| 0         | PCI EHCI/OHCI | HS-USB        |
+| 1         | PCI EHCI/OHCI | xHCI          |
++-----------+---------------+---------------+
+
+The USB PHY device tree node should be the subnodes corresponding to the USB
+channels and the controllers connected to them. These subnodes must contain the
+following properties:
+- renesas,phy-select: the first cell identifies the USB channel, the second cell
+  is the USB controller selector; see the table above for the values.
+- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
+  the USB channel, the second cell is the UGCTRL2 value corresponding to the
+  USB controller selected for that channel.
+
+Example (Lager board):
+
+	usb-phy@e6590100 {
+		compatible = "renesas,usb-phy-r8a7790";
+		reg = <0 0xe6590100 0 0x100>;
+		#phy-cells = <2>;
+		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
+		clock-names = "usbhs";
+
+		usb-phy@0,0 {
+			renesas,phy-select = <0 0>;
+			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
+		};
+		usb-phy@0,1 {
+			renesas,phy-select = <0 1>;
+			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
+		};
+		usb-phy@1,0 {
+			renesas,phy-select = <1 0>;
+			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
+		};
+		usb-phy@1,1 {
+			renesas,phy-select = <1 1>;
+			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
+		};
+	};
Index: linux-phy/drivers/phy/Kconfig
=================================--- linux-phy.orig/drivers/phy/Kconfig
+++ linux-phy/drivers/phy/Kconfig
@@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
 	depends on OF
 	select GENERIC_PHY
 
+config PHY_RCAR_GEN2
+	tristate "Renesas R-Car generation 2 USB PHY driver"
+	depends on ARCH_SHMOBILE
+	depends on GENERIC_PHY
+	help
+	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
+
 config OMAP_CONTROL_PHY
 	tristate "OMAP CONTROL PHY Driver"
 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
Index: linux-phy/drivers/phy/Makefile
=================================--- linux-phy.orig/drivers/phy/Makefile
+++ linux-phy/drivers/phy/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
 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_MVEBU_SATA)		+= phy-mvebu-sata.o
+obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
 obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
 obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
Index: linux-phy/drivers/phy/phy-rcar-gen2.c
=================================--- /dev/null
+++ linux-phy/drivers/phy/phy-rcar-gen2.c
@@ -0,0 +1,288 @@
+/*
+ * Renesas R-Car Gen2 PHY driver
+ *
+ * Copyright (C) 2014 Renesas Solutions Corp.
+ * Copyright (C) 2014 Cogent Embedded, Inc.
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define USBHS_LPSTS			0x02
+#define USBHS_UGCTRL			0x80
+#define USBHS_UGCTRL2			0x84
+#define USBHS_UGSTS			0x88
+
+/* Low Power Status register (LPSTS) */
+#define USBHS_LPSTS_SUSPM		0x4000
+
+/* USB General control register (UGCTRL) */
+#define USBHS_UGCTRL_CONNECT		0x00000004
+#define USBHS_UGCTRL_PLLRESET		0x00000001
+
+/* USB General control register 2 (UGCTRL2) */
+#define USBHS_UGCTRL2_USB2SEL		0x80000000
+#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
+#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
+#define USBHS_UGCTRL2_USB0SEL		0x00000030
+#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
+#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
+
+/* USB General status register (UGSTS) */
+#define USBHS_UGSTS_LOCK		0x00000300
+
+#define	NUM_USB_CHANNELS	2
+
+struct rcar_gen2_phy {
+	struct phy *phy;
+	struct rcar_gen2_phy_driver *drv;
+	u32 select_mask;
+	u32 select_value;
+};
+
+struct rcar_gen2_phy_driver {
+	void __iomem *base;
+	struct clk *clk;
+	spinlock_t lock;
+	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
+};
+
+static int rcar_gen2_phy_init(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	unsigned long flags;
+	u32 ugctrl2;
+
+	clk_prepare_enable(drv->clk);
+
+	spin_lock_irqsave(&drv->lock, flags);
+	ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
+	ugctrl2 &= ~phy->select_mask;
+	ugctrl2 |= phy->select_value;
+	writel(ugctrl2, drv->base + USBHS_UGCTRL2);
+	spin_unlock_irqrestore(&drv->lock, flags);
+	return 0;
+}
+
+static int rcar_gen2_phy_exit(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+
+	clk_disable_unprepare(phy->drv->clk);
+
+	return 0;
+}
+
+static int rcar_gen2_phy_power_on(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	void __iomem *base = drv->base;
+	unsigned long flags;
+	u32 value;
+	int err = 0, i;
+
+	/* Skip if it's not USBHS */
+	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
+		return 0;
+
+	spin_lock_irqsave(&drv->lock, flags);
+
+	/* Power on USBHS PHY */
+	value = readl(base + USBHS_UGCTRL);
+	value &= ~USBHS_UGCTRL_PLLRESET;
+	writel(value, base + USBHS_UGCTRL);
+
+	value = readw(base + USBHS_LPSTS);
+	value |= USBHS_LPSTS_SUSPM;
+	writew(value, base + USBHS_LPSTS);
+
+	for (i = 0; i < 20; i++) {
+		value = readl(base + USBHS_UGSTS);
+		if ((value & USBHS_UGSTS_LOCK) = USBHS_UGSTS_LOCK) {
+			value = readl(base + USBHS_UGCTRL);
+			value |= USBHS_UGCTRL_CONNECT;
+			writel(value, base + USBHS_UGCTRL);
+			goto out;
+		}
+		udelay(1);
+	}
+
+	/* Timed out waiting for the PLL lock */
+	err = -ETIMEDOUT;
+
+out:
+	spin_unlock_irqrestore(&drv->lock, flags);
+
+	return err;
+}
+
+static int rcar_gen2_phy_power_off(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	void __iomem *base = drv->base;
+	unsigned long flags;
+	u32 value;
+
+	/* Skip if it's not USBHS */
+	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
+		return 0;
+
+	spin_lock_irqsave(&drv->lock, flags);
+
+	/* Power off USBHS PHY */
+	value = readl(base + USBHS_UGCTRL);
+	value &= ~USBHS_UGCTRL_CONNECT;
+	writel(value, base + USBHS_UGCTRL);
+
+	value = readw(base + USBHS_LPSTS);
+	value &= ~USBHS_LPSTS_SUSPM;
+	writew(value, base + USBHS_LPSTS);
+
+	value = readl(base + USBHS_UGCTRL);
+	value |= USBHS_UGCTRL_PLLRESET;
+	writel(value, base + USBHS_UGCTRL);
+
+	spin_unlock_irqrestore(&drv->lock, flags);
+
+	return 0;
+}
+
+static struct phy_ops rcar_gen2_phy_ops = {
+	.init		= rcar_gen2_phy_init,
+	.exit		= rcar_gen2_phy_exit,
+	.power_on	= rcar_gen2_phy_power_on,
+	.power_off	= rcar_gen2_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id rcar_gen2_phy_match_table[] = {
+	{ .compatible = "renesas,usb-phy-r8a7790" },
+	{ .compatible = "renesas,usb-phy-r8a7791" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rcar_gen2_phy_match_table);
+
+static struct phy *rcar_gen2_phy_xlate(struct device *dev,
+				       struct of_phandle_args *args)
+{
+	struct rcar_gen2_phy_driver *drv;
+
+	drv = dev_get_drvdata(dev);
+	if (!drv)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= NUM_USB_CHANNELS || args->args[1] >= 2)
+		return ERR_PTR(-ENODEV);
+
+	return drv->phys[args->args[0]][args->args[1]].phy;
+}
+
+static int rcar_gen2_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rcar_gen2_phy_driver *drv;
+	struct phy_provider *provider;
+	struct device_node *np;
+	struct resource *res;
+	void __iomem *base;
+	struct clk *clk;
+
+	if (!dev->of_node) {
+		dev_err(dev,
+			"This driver is required to be instantiated from device tree\n");
+		return -EINVAL;
+	}
+
+	clk = devm_clk_get(dev, "usbhs");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "Can't get USBHS clock\n");
+		return PTR_ERR(clk);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	spin_lock_init(&drv->lock);
+
+	drv->clk = clk;
+	drv->base = base;
+
+	for_each_child_of_node(dev->of_node, np) {
+  		struct rcar_gen2_phy *phy;
+		u32 values[2];
+		int error;
+
+		error = of_property_read_u32_array(np, "renesas,phy-select",
+						   values, 2);
+		if (error) {
+			dev_err(dev, "Failed to read \"renesas,phy-select\" property\n");
+			return error;
+		}
+		if (values[0] >= NUM_USB_CHANNELS || values[1] >= 2) {
+			dev_err(dev, "Values out of range in \"renesas,phy-select\" property\n");
+			return error;
+		}
+		phy = &drv->phys[values[0]][values[1]];
+
+		error = of_property_read_u32_array(np, "renesas,ugctrl2-masks",
+						   values, 2);
+		if (error) {
+			dev_err(dev, "Failed to read \"renesas,ugctrl2-masks\" property\n");
+			return error;
+		}
+		phy->select_mask  = values[0];
+		phy->select_value = values[1];
+
+		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
+		if (IS_ERR(phy->phy)) {
+			dev_err(dev, "Failed to create PHY\n");
+			return PTR_ERR(phy->phy);
+		}
+
+		phy->drv = drv;
+		phy_set_drvdata(phy->phy, phy);
+	}
+
+	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "Failed to register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_set_drvdata(dev, drv);
+
+	return 0;
+}
+
+static struct platform_driver rcar_gen2_phy_driver = {
+	.driver = {
+		.name		= "phy_rcar_gen2",
+		.of_match_table	= rcar_gen2_phy_match_table,
+	},
+	.probe	= rcar_gen2_phy_probe,
+};
+
+module_platform_driver(rcar_gen2_phy_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Renesas R-Car Gen2 PHY");
+MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>");

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-23 22:06 ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-05-23 22:06 UTC (permalink / raw)
  To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

This PHY, though formally being a part of Renesas USBHS controller, contains the
UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
channels) to the different USB controllers: channel 0 can be connected to either
PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
or xHCI controllers.

This is a new driver for this USB PHY currently already supported under drivers/
usb/phy/. The reason for writing the new driver was the requirement that the
multiplexing of USB channels to the controller be dynamic, depending on what
USB drivers are loaded,  rather than static as provided by the old driver.
The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
ideally. The new driver only supports device tree probing for now.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.

Changes in version 4:
- added subnodes to the USB PHY node, described their properties in the binding
  document; rewrote the probe() method to parse out PHY selection and UGCTRL2
  mask values out of these child nodes;
- changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
  don't have control over it anyway;
- refreshed the patch.

Changes in version 3:
- removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
  'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
  methods;
- renamed the power_{on|off}() methods;
- replaced io{read|write}16() calls with {read|write}w();
- removed the comment to '#define USBHS_UGSTS_LOCK';
- broke the line in the dev_err() call in the probe() method;
- moved the driver's line in the 'Makefile' before OMAP drivers.

Changes in version 2:
- rebased the patch, resolving reject.

 Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
 drivers/phy/Kconfig                                     |    7 
 drivers/phy/Makefile                                    |    1 
 drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++++++
 4 files changed, 356 insertions(+)

Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
===================================================================
--- /dev/null
+++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
@@ -0,0 +1,60 @@
+* Renesas R-Car generation 2 USB PHY
+
+This file provides information on what the device node for the R-Car generation
+2 USB PHY contains.
+
+Required properties:
+- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
+	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
+- reg: offset and length of the register block.
+- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
+- clocks: clock phandle and specifier pair.
+- clock-names: string, clock input name, must be "usbhs".
+
+The phandle's first argument in the PHY specifier identifies the USB channel,
+the second one is the USB controller selector and depends on the first:
+
++-----------+---------------+---------------+
+|\ Selector |               |               |
++ --------- +       0       |       1       |
+| Channel  \|               |               |
++-----------+---------------+---------------+
+| 0         | PCI EHCI/OHCI | HS-USB        |
+| 1         | PCI EHCI/OHCI | xHCI          |
++-----------+---------------+---------------+
+
+The USB PHY device tree node should be the subnodes corresponding to the USB
+channels and the controllers connected to them. These subnodes must contain the
+following properties:
+- renesas,phy-select: the first cell identifies the USB channel, the second cell
+  is the USB controller selector; see the table above for the values.
+- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
+  the USB channel, the second cell is the UGCTRL2 value corresponding to the
+  USB controller selected for that channel.
+
+Example (Lager board):
+
+	usb-phy@e6590100 {
+		compatible = "renesas,usb-phy-r8a7790";
+		reg = <0 0xe6590100 0 0x100>;
+		#phy-cells = <2>;
+		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
+		clock-names = "usbhs";
+
+		usb-phy@0,0 {
+			renesas,phy-select = <0 0>;
+			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
+		};
+		usb-phy@0,1 {
+			renesas,phy-select = <0 1>;
+			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
+		};
+		usb-phy@1,0 {
+			renesas,phy-select = <1 0>;
+			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
+		};
+		usb-phy@1,1 {
+			renesas,phy-select = <1 1>;
+			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
+		};
+	};
Index: linux-phy/drivers/phy/Kconfig
===================================================================
--- linux-phy.orig/drivers/phy/Kconfig
+++ linux-phy/drivers/phy/Kconfig
@@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
 	depends on OF
 	select GENERIC_PHY
 
+config PHY_RCAR_GEN2
+	tristate "Renesas R-Car generation 2 USB PHY driver"
+	depends on ARCH_SHMOBILE
+	depends on GENERIC_PHY
+	help
+	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
+
 config OMAP_CONTROL_PHY
 	tristate "OMAP CONTROL PHY Driver"
 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
Index: linux-phy/drivers/phy/Makefile
===================================================================
--- linux-phy.orig/drivers/phy/Makefile
+++ linux-phy/drivers/phy/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
 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_MVEBU_SATA)		+= phy-mvebu-sata.o
+obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
 obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
 obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
Index: linux-phy/drivers/phy/phy-rcar-gen2.c
===================================================================
--- /dev/null
+++ linux-phy/drivers/phy/phy-rcar-gen2.c
@@ -0,0 +1,288 @@
+/*
+ * Renesas R-Car Gen2 PHY driver
+ *
+ * Copyright (C) 2014 Renesas Solutions Corp.
+ * Copyright (C) 2014 Cogent Embedded, Inc.
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define USBHS_LPSTS			0x02
+#define USBHS_UGCTRL			0x80
+#define USBHS_UGCTRL2			0x84
+#define USBHS_UGSTS			0x88
+
+/* Low Power Status register (LPSTS) */
+#define USBHS_LPSTS_SUSPM		0x4000
+
+/* USB General control register (UGCTRL) */
+#define USBHS_UGCTRL_CONNECT		0x00000004
+#define USBHS_UGCTRL_PLLRESET		0x00000001
+
+/* USB General control register 2 (UGCTRL2) */
+#define USBHS_UGCTRL2_USB2SEL		0x80000000
+#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
+#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
+#define USBHS_UGCTRL2_USB0SEL		0x00000030
+#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
+#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
+
+/* USB General status register (UGSTS) */
+#define USBHS_UGSTS_LOCK		0x00000300
+
+#define	NUM_USB_CHANNELS	2
+
+struct rcar_gen2_phy {
+	struct phy *phy;
+	struct rcar_gen2_phy_driver *drv;
+	u32 select_mask;
+	u32 select_value;
+};
+
+struct rcar_gen2_phy_driver {
+	void __iomem *base;
+	struct clk *clk;
+	spinlock_t lock;
+	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
+};
+
+static int rcar_gen2_phy_init(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	unsigned long flags;
+	u32 ugctrl2;
+
+	clk_prepare_enable(drv->clk);
+
+	spin_lock_irqsave(&drv->lock, flags);
+	ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
+	ugctrl2 &= ~phy->select_mask;
+	ugctrl2 |= phy->select_value;
+	writel(ugctrl2, drv->base + USBHS_UGCTRL2);
+	spin_unlock_irqrestore(&drv->lock, flags);
+	return 0;
+}
+
+static int rcar_gen2_phy_exit(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+
+	clk_disable_unprepare(phy->drv->clk);
+
+	return 0;
+}
+
+static int rcar_gen2_phy_power_on(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	void __iomem *base = drv->base;
+	unsigned long flags;
+	u32 value;
+	int err = 0, i;
+
+	/* Skip if it's not USBHS */
+	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
+		return 0;
+
+	spin_lock_irqsave(&drv->lock, flags);
+
+	/* Power on USBHS PHY */
+	value = readl(base + USBHS_UGCTRL);
+	value &= ~USBHS_UGCTRL_PLLRESET;
+	writel(value, base + USBHS_UGCTRL);
+
+	value = readw(base + USBHS_LPSTS);
+	value |= USBHS_LPSTS_SUSPM;
+	writew(value, base + USBHS_LPSTS);
+
+	for (i = 0; i < 20; i++) {
+		value = readl(base + USBHS_UGSTS);
+		if ((value & USBHS_UGSTS_LOCK) == USBHS_UGSTS_LOCK) {
+			value = readl(base + USBHS_UGCTRL);
+			value |= USBHS_UGCTRL_CONNECT;
+			writel(value, base + USBHS_UGCTRL);
+			goto out;
+		}
+		udelay(1);
+	}
+
+	/* Timed out waiting for the PLL lock */
+	err = -ETIMEDOUT;
+
+out:
+	spin_unlock_irqrestore(&drv->lock, flags);
+
+	return err;
+}
+
+static int rcar_gen2_phy_power_off(struct phy *p)
+{
+	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
+	struct rcar_gen2_phy_driver *drv = phy->drv;
+	void __iomem *base = drv->base;
+	unsigned long flags;
+	u32 value;
+
+	/* Skip if it's not USBHS */
+	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
+		return 0;
+
+	spin_lock_irqsave(&drv->lock, flags);
+
+	/* Power off USBHS PHY */
+	value = readl(base + USBHS_UGCTRL);
+	value &= ~USBHS_UGCTRL_CONNECT;
+	writel(value, base + USBHS_UGCTRL);
+
+	value = readw(base + USBHS_LPSTS);
+	value &= ~USBHS_LPSTS_SUSPM;
+	writew(value, base + USBHS_LPSTS);
+
+	value = readl(base + USBHS_UGCTRL);
+	value |= USBHS_UGCTRL_PLLRESET;
+	writel(value, base + USBHS_UGCTRL);
+
+	spin_unlock_irqrestore(&drv->lock, flags);
+
+	return 0;
+}
+
+static struct phy_ops rcar_gen2_phy_ops = {
+	.init		= rcar_gen2_phy_init,
+	.exit		= rcar_gen2_phy_exit,
+	.power_on	= rcar_gen2_phy_power_on,
+	.power_off	= rcar_gen2_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id rcar_gen2_phy_match_table[] = {
+	{ .compatible = "renesas,usb-phy-r8a7790" },
+	{ .compatible = "renesas,usb-phy-r8a7791" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rcar_gen2_phy_match_table);
+
+static struct phy *rcar_gen2_phy_xlate(struct device *dev,
+				       struct of_phandle_args *args)
+{
+	struct rcar_gen2_phy_driver *drv;
+
+	drv = dev_get_drvdata(dev);
+	if (!drv)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= NUM_USB_CHANNELS || args->args[1] >= 2)
+		return ERR_PTR(-ENODEV);
+
+	return drv->phys[args->args[0]][args->args[1]].phy;
+}
+
+static int rcar_gen2_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rcar_gen2_phy_driver *drv;
+	struct phy_provider *provider;
+	struct device_node *np;
+	struct resource *res;
+	void __iomem *base;
+	struct clk *clk;
+
+	if (!dev->of_node) {
+		dev_err(dev,
+			"This driver is required to be instantiated from device tree\n");
+		return -EINVAL;
+	}
+
+	clk = devm_clk_get(dev, "usbhs");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "Can't get USBHS clock\n");
+		return PTR_ERR(clk);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	spin_lock_init(&drv->lock);
+
+	drv->clk = clk;
+	drv->base = base;
+
+	for_each_child_of_node(dev->of_node, np) {
+  		struct rcar_gen2_phy *phy;
+		u32 values[2];
+		int error;
+
+		error = of_property_read_u32_array(np, "renesas,phy-select",
+						   values, 2);
+		if (error) {
+			dev_err(dev, "Failed to read \"renesas,phy-select\" property\n");
+			return error;
+		}
+		if (values[0] >= NUM_USB_CHANNELS || values[1] >= 2) {
+			dev_err(dev, "Values out of range in \"renesas,phy-select\" property\n");
+			return error;
+		}
+		phy = &drv->phys[values[0]][values[1]];
+
+		error = of_property_read_u32_array(np, "renesas,ugctrl2-masks",
+						   values, 2);
+		if (error) {
+			dev_err(dev, "Failed to read \"renesas,ugctrl2-masks\" property\n");
+			return error;
+		}
+		phy->select_mask  = values[0];
+		phy->select_value = values[1];
+
+		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
+		if (IS_ERR(phy->phy)) {
+			dev_err(dev, "Failed to create PHY\n");
+			return PTR_ERR(phy->phy);
+		}
+
+		phy->drv = drv;
+		phy_set_drvdata(phy->phy, phy);
+	}
+
+	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "Failed to register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_set_drvdata(dev, drv);
+
+	return 0;
+}
+
+static struct platform_driver rcar_gen2_phy_driver = {
+	.driver = {
+		.name		= "phy_rcar_gen2",
+		.of_match_table	= rcar_gen2_phy_match_table,
+	},
+	.probe	= rcar_gen2_phy_probe,
+};
+
+module_platform_driver(rcar_gen2_phy_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Renesas R-Car Gen2 PHY");
+MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>");

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-23 22:06 ` Sergei Shtylyov
@ 2014-05-26  7:28   ` Geert Uytterhoeven
  -1 siblings, 0 replies; 38+ messages in thread
From: Geert Uytterhoeven @ 2014-05-26  7:28 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	kishon, Grant Likely, devicetree, Randy Dunlap, linux-doc,
	Linux-sh list

On Sat, May 24, 2014 at 12:06 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car generation
> +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.

As this is a new driver, I think you should switch to the recommended
vendor/soc/device ordering, which is also supported by checkpatch (incl.
wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-26  7:28   ` Geert Uytterhoeven
  0 siblings, 0 replies; 38+ messages in thread
From: Geert Uytterhoeven @ 2014-05-26  7:28 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	kishon, Grant Likely, devicetree, Randy Dunlap, linux-doc,
	Linux-sh list

On Sat, May 24, 2014 at 12:06 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car generation
> +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.

As this is a new driver, I think you should switch to the recommended
vendor/soc/device ordering, which is also supported by checkpatch (incl.
wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-26  7:28   ` Geert Uytterhoeven
@ 2014-05-26  7:48     ` Simon Horman
  -1 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2014-05-26  7:48 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Sergei Shtylyov, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, kishon, Grant Likely, devicetree,
	Randy Dunlap, linux-doc, Linux-sh list

On Mon, May 26, 2014 at 09:28:22AM +0200, Geert Uytterhoeven wrote:
> On Sat, May 24, 2014 at 12:06 AM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
> > --- /dev/null
> > +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> > @@ -0,0 +1,60 @@
> > +* Renesas R-Car generation 2 USB PHY
> > +
> > +This file provides information on what the device node for the R-Car generation
> > +2 USB PHY contains.
> > +
> > +Required properties:
> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> 
> As this is a new driver, I think you should switch to the recommended
> vendor/soc/device ordering, which is also supported by checkpatch (incl.
> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.

A while ago we were asked to consolidate our drivers around a consistent
scheme. At the time the <vendor>,<device>-<version> scheme, where version SoC for most Renesas parts, was agreed to be acceptable and we have
(hopefully) consistently been using that ever since.

So unless there has been a policy change I would prefer to keep using this
scheme (i.e. leave the bindings above as-is). And update checkpatch if that
is appropriate.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-26  7:48     ` Simon Horman
  0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2014-05-26  7:48 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Sergei Shtylyov, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, kishon, Grant Likely, devicetree,
	Randy Dunlap, linux-doc, Linux-sh list

On Mon, May 26, 2014 at 09:28:22AM +0200, Geert Uytterhoeven wrote:
> On Sat, May 24, 2014 at 12:06 AM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
> > --- /dev/null
> > +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> > @@ -0,0 +1,60 @@
> > +* Renesas R-Car generation 2 USB PHY
> > +
> > +This file provides information on what the device node for the R-Car generation
> > +2 USB PHY contains.
> > +
> > +Required properties:
> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> 
> As this is a new driver, I think you should switch to the recommended
> vendor/soc/device ordering, which is also supported by checkpatch (incl.
> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.

A while ago we were asked to consolidate our drivers around a consistent
scheme. At the time the <vendor>,<device>-<version> scheme, where version =
SoC for most Renesas parts, was agreed to be acceptable and we have
(hopefully) consistently been using that ever since.

So unless there has been a policy change I would prefer to keep using this
scheme (i.e. leave the bindings above as-is). And update checkpatch if that
is appropriate.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-26  7:48     ` Simon Horman
@ 2014-05-26  8:04       ` Geert Uytterhoeven
  -1 siblings, 0 replies; 38+ messages in thread
From: Geert Uytterhoeven @ 2014-05-26  8:04 UTC (permalink / raw)
  To: Simon Horman, Rob Herring
  Cc: Sergei Shtylyov, Pawel Moll, Mark Rutland, Ian Campbell,
	Kumar Gala, kishon, Grant Likely, devicetree, Randy Dunlap,
	linux-doc, Linux-sh list

Hi Simon, Rob,

On Mon, May 26, 2014 at 9:48 AM, Simon Horman <horms@verge.net.au> wrote:
>> > +Required properties:
>> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
>> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>
>> As this is a new driver, I think you should switch to the recommended
>> vendor/soc/device ordering, which is also supported by checkpatch (incl.
>> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.
>
> A while ago we were asked to consolidate our drivers around a consistent
> scheme. At the time the <vendor>,<device>-<version> scheme, where version 
(after talking around at ELC) It's my understanding this was a silly mistake.

> SoC for most Renesas parts, was agreed to be acceptable and we have
> (hopefully) consistently been using that ever since.

We did for everything except for clock drivers, sound, and thermal.

> So unless there has been a policy change I would prefer to keep using this
> scheme (i.e. leave the bindings above as-is). And update checkpatch if that
> is appropriate.

Rob, can we have the official policy documented in
Documentation/devicetree/bindings?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-26  8:04       ` Geert Uytterhoeven
  0 siblings, 0 replies; 38+ messages in thread
From: Geert Uytterhoeven @ 2014-05-26  8:04 UTC (permalink / raw)
  To: Simon Horman, Rob Herring
  Cc: Sergei Shtylyov, Pawel Moll, Mark Rutland, Ian Campbell,
	Kumar Gala, kishon, Grant Likely, devicetree, Randy Dunlap,
	linux-doc, Linux-sh list

Hi Simon, Rob,

On Mon, May 26, 2014 at 9:48 AM, Simon Horman <horms@verge.net.au> wrote:
>> > +Required properties:
>> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
>> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>
>> As this is a new driver, I think you should switch to the recommended
>> vendor/soc/device ordering, which is also supported by checkpatch (incl.
>> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.
>
> A while ago we were asked to consolidate our drivers around a consistent
> scheme. At the time the <vendor>,<device>-<version> scheme, where version =

(after talking around at ELC) It's my understanding this was a silly mistake.

> SoC for most Renesas parts, was agreed to be acceptable and we have
> (hopefully) consistently been using that ever since.

We did for everything except for clock drivers, sound, and thermal.

> So unless there has been a policy change I would prefer to keep using this
> scheme (i.e. leave the bindings above as-is). And update checkpatch if that
> is appropriate.

Rob, can we have the official policy documented in
Documentation/devicetree/bindings?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-26  8:04       ` Geert Uytterhoeven
@ 2014-05-26  9:00         ` Simon Horman
  -1 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2014-05-26  9:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Sergei Shtylyov, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, kishon, Grant Likely, devicetree,
	Randy Dunlap, linux-doc, Linux-sh list

On Mon, May 26, 2014 at 10:04:52AM +0200, Geert Uytterhoeven wrote:
> Hi Simon, Rob,
> 
> On Mon, May 26, 2014 at 9:48 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > +Required properties:
> >> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> >> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> >>
> >> As this is a new driver, I think you should switch to the recommended
> >> vendor/soc/device ordering, which is also supported by checkpatch (incl.
> >> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.
> >
> > A while ago we were asked to consolidate our drivers around a consistent
> > scheme. At the time the <vendor>,<device>-<version> scheme, where version > 
> (after talking around at ELC) It's my understanding this was a silly mistake.

That is a shame as we switched around the binding for at least one driver

> > SoC for most Renesas parts, was agreed to be acceptable and we have
> > (hopefully) consistently been using that ever since.
> 
> We did for everything except for clock drivers, sound, and thermal.
> 
> > So unless there has been a policy change I would prefer to keep using this
> > scheme (i.e. leave the bindings above as-is). And update checkpatch if that
> > is appropriate.
> 
> Rob, can we have the official policy documented in
> Documentation/devicetree/bindings?

Of course if there is a policy I'm have no objections to following it for
new drivers.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-26  9:00         ` Simon Horman
  0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2014-05-26  9:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Sergei Shtylyov, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, kishon, Grant Likely, devicetree,
	Randy Dunlap, linux-doc, Linux-sh list

On Mon, May 26, 2014 at 10:04:52AM +0200, Geert Uytterhoeven wrote:
> Hi Simon, Rob,
> 
> On Mon, May 26, 2014 at 9:48 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > +Required properties:
> >> > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> >> > +             "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> >>
> >> As this is a new driver, I think you should switch to the recommended
> >> vendor/soc/device ordering, which is also supported by checkpatch (incl.
> >> wildcard matching!), i.e. "renesas,r8a7790-usb-phy" etc.
> >
> > A while ago we were asked to consolidate our drivers around a consistent
> > scheme. At the time the <vendor>,<device>-<version> scheme, where version =
> 
> (after talking around at ELC) It's my understanding this was a silly mistake.

That is a shame as we switched around the binding for at least one driver

> > SoC for most Renesas parts, was agreed to be acceptable and we have
> > (hopefully) consistently been using that ever since.
> 
> We did for everything except for clock drivers, sound, and thermal.
> 
> > So unless there has been a policy change I would prefer to keep using this
> > scheme (i.e. leave the bindings above as-is). And update checkpatch if that
> > is appropriate.
> 
> Rob, can we have the official policy documented in
> Documentation/devicetree/bindings?

Of course if there is a policy I'm have no objections to following it for
new drivers.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-23 22:06 ` Sergei Shtylyov
@ 2014-05-27  9:29   ` Yoshihiro Shimoda
  -1 siblings, 0 replies; 38+ messages in thread
From: Yoshihiro Shimoda @ 2014-05-27  9:29 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

(2014/05/24 7:06), Sergei Shtylyov wrote:
< snip >
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";

I'm not sure about the clock-names string,
but the current clock-name of R8A7790_CLK_HSUSB is "hsusb" in r8a7790.dtsi.
So, before I tested this driver, I changed the "hsusb" to "usbhs".
Otherwise, the devm_clk_get() in the driver failed.

< snip >
> +	for_each_child_of_node(dev->of_node, np) {
> +  		struct rcar_gen2_phy *phy;

This line has two while spaces at the start of the line.

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-27  9:29   ` Yoshihiro Shimoda
  0 siblings, 0 replies; 38+ messages in thread
From: Yoshihiro Shimoda @ 2014-05-27  9:29 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

(2014/05/24 7:06), Sergei Shtylyov wrote:
< snip >
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";

I'm not sure about the clock-names string,
but the current clock-name of R8A7790_CLK_HSUSB is "hsusb" in r8a7790.dtsi.
So, before I tested this driver, I changed the "hsusb" to "usbhs".
Otherwise, the devm_clk_get() in the driver failed.

< snip >
> +	for_each_child_of_node(dev->of_node, np) {
> +  		struct rcar_gen2_phy *phy;

This line has two while spaces at the start of the line.

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-27  9:29   ` Yoshihiro Shimoda
@ 2014-05-27 19:38     ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-05-27 19:38 UTC (permalink / raw)
  To: Yoshihiro Shimoda, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 05/27/2014 01:29 PM, Yoshihiro Shimoda wrote:

>> +Example (Lager board):
>> +
>> +	usb-phy@e6590100 {
>> +		compatible = "renesas,usb-phy-r8a7790";
>> +		reg = <0 0xe6590100 0 0x100>;
>> +		#phy-cells = <2>;
>> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>> +		clock-names = "usbhs";

> I'm not sure about the clock-names string,
> but the current clock-name of R8A7790_CLK_HSUSB

    I guess you meant "clock-output-names" prop?

> is "hsusb" in r8a7790.dtsi.

    Yes. But it doesn't have to match to what's passed to devm_clk_get().

> So, before I tested this driver, I changed the "hsusb" to "usbhs".
> Otherwise, the devm_clk_get() in the driver failed.

    This is very strange, the driver works OK for me.

> < snip >
>> +	for_each_child_of_node(dev->of_node, np) {
>> +  		struct rcar_gen2_phy *phy;
>
> This line has two while spaces at the start of the line.

    Thank you, looks like I cut&pasted from a bad source and forgot to run the 
patch thru scripts/checkpatch.pl...

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-27 19:38     ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-05-27 19:38 UTC (permalink / raw)
  To: Yoshihiro Shimoda, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 05/27/2014 01:29 PM, Yoshihiro Shimoda wrote:

>> +Example (Lager board):
>> +
>> +	usb-phy@e6590100 {
>> +		compatible = "renesas,usb-phy-r8a7790";
>> +		reg = <0 0xe6590100 0 0x100>;
>> +		#phy-cells = <2>;
>> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>> +		clock-names = "usbhs";

> I'm not sure about the clock-names string,
> but the current clock-name of R8A7790_CLK_HSUSB

    I guess you meant "clock-output-names" prop?

> is "hsusb" in r8a7790.dtsi.

    Yes. But it doesn't have to match to what's passed to devm_clk_get().

> So, before I tested this driver, I changed the "hsusb" to "usbhs".
> Otherwise, the devm_clk_get() in the driver failed.

    This is very strange, the driver works OK for me.

> < snip >
>> +	for_each_child_of_node(dev->of_node, np) {
>> +  		struct rcar_gen2_phy *phy;
>
> This line has two while spaces at the start of the line.

    Thank you, looks like I cut&pasted from a bad source and forgot to run the 
patch thru scripts/checkpatch.pl...

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-27 19:38     ` Sergei Shtylyov
@ 2014-05-28  1:12       ` Yoshihiro Shimoda
  -1 siblings, 0 replies; 38+ messages in thread
From: Yoshihiro Shimoda @ 2014-05-28  1:12 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello,

(2014/05/28 4:38), Sergei Shtylyov wrote:
> Hello.
> 
> On 05/27/2014 01:29 PM, Yoshihiro Shimoda wrote:
< snip >
>> So, before I tested this driver, I changed the "hsusb" to "usbhs".
>> Otherwise, the devm_clk_get() in the driver failed.
> 
>     This is very strange, the driver works OK for me.
> 

I'm sorry, my report was wrong.
The driver works OK for me too, when I tested the driver using "usbhs" today.
And, I was not able to duplicate an issue I reported yesterday.
So, maybe my test environment was wrong...

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-05-28  1:12       ` Yoshihiro Shimoda
  0 siblings, 0 replies; 38+ messages in thread
From: Yoshihiro Shimoda @ 2014-05-28  1:12 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, kishon, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello,

(2014/05/28 4:38), Sergei Shtylyov wrote:
> Hello.
> 
> On 05/27/2014 01:29 PM, Yoshihiro Shimoda wrote:
< snip >
>> So, before I tested this driver, I changed the "hsusb" to "usbhs".
>> Otherwise, the devm_clk_get() in the driver failed.
> 
>     This is very strange, the driver works OK for me.
> 

I'm sorry, my report was wrong.
The driver works OK for me too, when I tested the driver using "usbhs" today.
And, I was not able to duplicate an issue I reported yesterday.
So, maybe my test environment was wrong...

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-23 22:06 ` Sergei Shtylyov
@ 2014-06-04 11:52   ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-06-04 11:40 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Saturday 24 May 2014 03:36 AM, Sergei Shtylyov wrote:
> This PHY, though formally being a part of Renesas USBHS controller, contains the
> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
> channels) to the different USB controllers: channel 0 can be connected to either
> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
> or xHCI controllers.
> 
> This is a new driver for this USB PHY currently already supported under drivers/
> usb/phy/. The reason for writing the new driver was the requirement that the
> multiplexing of USB channels to the controller be dynamic, depending on what
> USB drivers are loaded,  rather than static as provided by the old driver.
> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
> ideally. The new driver only supports device tree probing for now.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.
> 
> Changes in version 4:
> - added subnodes to the USB PHY node, described their properties in the binding
>   document; rewrote the probe() method to parse out PHY selection and UGCTRL2
>   mask values out of these child nodes;
> - changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
>   don't have control over it anyway;
> - refreshed the patch.
> 
> Changes in version 3:
> - removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
>   'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
>   methods;
> - renamed the power_{on|off}() methods;
> - replaced io{read|write}16() calls with {read|write}w();
> - removed the comment to '#define USBHS_UGSTS_LOCK';
> - broke the line in the dev_err() call in the probe() method;
> - moved the driver's line in the 'Makefile' before OMAP drivers.
> 
> Changes in version 2:
> - rebased the patch, resolving reject.
> 
>  Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
>  drivers/phy/Kconfig                                     |    7 
>  drivers/phy/Makefile                                    |    1 
>  drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++++++
>  4 files changed, 356 insertions(+)
> 
> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> ===================================================================
> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car generation
> +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> +- reg: offset and length of the register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".
> +
> +The phandle's first argument in the PHY specifier identifies the USB channel,
> +the second one is the USB controller selector and depends on the first:
> +
> ++-----------+---------------+---------------+
> +|\ Selector |               |               |
> ++ --------- +       0       |       1       |
> +| Channel  \|               |               |
> ++-----------+---------------+---------------+
> +| 0         | PCI EHCI/OHCI | HS-USB        |
> +| 1         | PCI EHCI/OHCI | xHCI          |
> ++-----------+---------------+---------------+
> +
> +The USB PHY device tree node should be the subnodes corresponding to the USB
> +channels and the controllers connected to them. These subnodes must contain the
> +following properties:
> +- renesas,phy-select: the first cell identifies the USB channel, the second cell
> +  is the USB controller selector; see the table above for the values.
> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
> +  USB controller selected for that channel.
> +
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";
> +
> +		usb-phy@0,0 {
> +			renesas,phy-select = <0 0>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> +		};
> +		usb-phy@0,1 {
> +			renesas,phy-select = <0 1>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> +		};
> +		usb-phy@1,0 {
> +			renesas,phy-select = <1 0>;
> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> +		};
> +		usb-phy@1,1 {
> +			renesas,phy-select = <1 1>;
> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> +		};
> +	};

IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
used for both. And channel 1 can be configured for either PCI EHCI/OHCI or
xHCI. right?

So ideally only two sub-nodes should be created for channel '0' and channel
'1'. You can configure a channel to a particular mode by passing the mode in
PHY specifier (The channel should be configured to a particualr mode in xlate).

Btw I'm not sure if it is recommended to pass register mask values from dt.
> Index: linux-phy/drivers/phy/Kconfig
> ===================================================================
> --- linux-phy.orig/drivers/phy/Kconfig
> +++ linux-phy/drivers/phy/Kconfig
> @@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
>  	depends on OF
>  	select GENERIC_PHY
>  
> +config PHY_RCAR_GEN2
> +	tristate "Renesas R-Car generation 2 USB PHY driver"
> +	depends on ARCH_SHMOBILE
> +	depends on GENERIC_PHY
> +	help
> +	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> Index: linux-phy/drivers/phy/Makefile
> ===================================================================
> --- linux-phy.orig/drivers/phy/Makefile
> +++ linux-phy/drivers/phy/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
>  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_MVEBU_SATA)		+= phy-mvebu-sata.o
> +obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
>  obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
>  obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> ===================================================================
> --- /dev/null
> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> @@ -0,0 +1,288 @@
> +/*
> + * Renesas R-Car Gen2 PHY driver
> + *
> + * Copyright (C) 2014 Renesas Solutions Corp.
> + * Copyright (C) 2014 Cogent Embedded, Inc.
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define USBHS_LPSTS			0x02
> +#define USBHS_UGCTRL			0x80
> +#define USBHS_UGCTRL2			0x84
> +#define USBHS_UGSTS			0x88
> +
> +/* Low Power Status register (LPSTS) */
> +#define USBHS_LPSTS_SUSPM		0x4000
> +
> +/* USB General control register (UGCTRL) */
> +#define USBHS_UGCTRL_CONNECT		0x00000004
> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> +
> +/* USB General control register 2 (UGCTRL2) */
> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> +
> +/* USB General status register (UGSTS) */
> +#define USBHS_UGSTS_LOCK		0x00000300
> +
> +#define	NUM_USB_CHANNELS	2
> +
> +struct rcar_gen2_phy {
> +	struct phy *phy;
> +	struct rcar_gen2_phy_driver *drv;
> +	u32 select_mask;
> +	u32 select_value;
> +};
> +
> +struct rcar_gen2_phy_driver {
> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t lock;
> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

This can be created dynamically based on the number of sub-nodes. In this case
it'll be only rcar_gen2_phy phys[2], one for each channel.
By this we need not hard code NUM_USB_CHANNELS.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-04 11:52   ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-06-04 11:52 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Saturday 24 May 2014 03:36 AM, Sergei Shtylyov wrote:
> This PHY, though formally being a part of Renesas USBHS controller, contains the
> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
> channels) to the different USB controllers: channel 0 can be connected to either
> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
> or xHCI controllers.
> 
> This is a new driver for this USB PHY currently already supported under drivers/
> usb/phy/. The reason for writing the new driver was the requirement that the
> multiplexing of USB channels to the controller be dynamic, depending on what
> USB drivers are loaded,  rather than static as provided by the old driver.
> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
> ideally. The new driver only supports device tree probing for now.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.
> 
> Changes in version 4:
> - added subnodes to the USB PHY node, described their properties in the binding
>   document; rewrote the probe() method to parse out PHY selection and UGCTRL2
>   mask values out of these child nodes;
> - changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
>   don't have control over it anyway;
> - refreshed the patch.
> 
> Changes in version 3:
> - removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
>   'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
>   methods;
> - renamed the power_{on|off}() methods;
> - replaced io{read|write}16() calls with {read|write}w();
> - removed the comment to '#define USBHS_UGSTS_LOCK';
> - broke the line in the dev_err() call in the probe() method;
> - moved the driver's line in the 'Makefile' before OMAP drivers.
> 
> Changes in version 2:
> - rebased the patch, resolving reject.
> 
>  Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
>  drivers/phy/Kconfig                                     |    7 
>  drivers/phy/Makefile                                    |    1 
>  drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++++++
>  4 files changed, 356 insertions(+)
> 
> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> =================================> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car generation
> +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
> +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
> +- reg: offset and length of the register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".
> +
> +The phandle's first argument in the PHY specifier identifies the USB channel,
> +the second one is the USB controller selector and depends on the first:
> +
> ++-----------+---------------+---------------+
> +|\ Selector |               |               |
> ++ --------- +       0       |       1       |
> +| Channel  \|               |               |
> ++-----------+---------------+---------------+
> +| 0         | PCI EHCI/OHCI | HS-USB        |
> +| 1         | PCI EHCI/OHCI | xHCI          |
> ++-----------+---------------+---------------+
> +
> +The USB PHY device tree node should be the subnodes corresponding to the USB
> +channels and the controllers connected to them. These subnodes must contain the
> +following properties:
> +- renesas,phy-select: the first cell identifies the USB channel, the second cell
> +  is the USB controller selector; see the table above for the values.
> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
> +  USB controller selected for that channel.
> +
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";
> +
> +		usb-phy@0,0 {
> +			renesas,phy-select = <0 0>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> +		};
> +		usb-phy@0,1 {
> +			renesas,phy-select = <0 1>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> +		};
> +		usb-phy@1,0 {
> +			renesas,phy-select = <1 0>;
> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> +		};
> +		usb-phy@1,1 {
> +			renesas,phy-select = <1 1>;
> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> +		};
> +	};

IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
used for both. And channel 1 can be configured for either PCI EHCI/OHCI or
xHCI. right?

So ideally only two sub-nodes should be created for channel '0' and channel
'1'. You can configure a channel to a particular mode by passing the mode in
PHY specifier (The channel should be configured to a particualr mode in xlate).

Btw I'm not sure if it is recommended to pass register mask values from dt.
> Index: linux-phy/drivers/phy/Kconfig
> =================================> --- linux-phy.orig/drivers/phy/Kconfig
> +++ linux-phy/drivers/phy/Kconfig
> @@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
>  	depends on OF
>  	select GENERIC_PHY
>  
> +config PHY_RCAR_GEN2
> +	tristate "Renesas R-Car generation 2 USB PHY driver"
> +	depends on ARCH_SHMOBILE
> +	depends on GENERIC_PHY
> +	help
> +	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> Index: linux-phy/drivers/phy/Makefile
> =================================> --- linux-phy.orig/drivers/phy/Makefile
> +++ linux-phy/drivers/phy/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
>  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_MVEBU_SATA)		+= phy-mvebu-sata.o
> +obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
>  obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
>  obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> =================================> --- /dev/null
> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> @@ -0,0 +1,288 @@
> +/*
> + * Renesas R-Car Gen2 PHY driver
> + *
> + * Copyright (C) 2014 Renesas Solutions Corp.
> + * Copyright (C) 2014 Cogent Embedded, Inc.
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define USBHS_LPSTS			0x02
> +#define USBHS_UGCTRL			0x80
> +#define USBHS_UGCTRL2			0x84
> +#define USBHS_UGSTS			0x88
> +
> +/* Low Power Status register (LPSTS) */
> +#define USBHS_LPSTS_SUSPM		0x4000
> +
> +/* USB General control register (UGCTRL) */
> +#define USBHS_UGCTRL_CONNECT		0x00000004
> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> +
> +/* USB General control register 2 (UGCTRL2) */
> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> +
> +/* USB General status register (UGSTS) */
> +#define USBHS_UGSTS_LOCK		0x00000300
> +
> +#define	NUM_USB_CHANNELS	2
> +
> +struct rcar_gen2_phy {
> +	struct phy *phy;
> +	struct rcar_gen2_phy_driver *drv;
> +	u32 select_mask;
> +	u32 select_value;
> +};
> +
> +struct rcar_gen2_phy_driver {
> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t lock;
> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

This can be created dynamically based on the number of sub-nodes. In this case
it'll be only rcar_gen2_phy phys[2], one for each channel.
By this we need not hard code NUM_USB_CHANNELS.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-04 11:52   ` Kishon Vijay Abraham I
@ 2014-06-04 21:54     ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-04 21:54 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:

>> This PHY, though formally being a part of Renesas USBHS controller, contains the
>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
>> channels) to the different USB controllers: channel 0 can be connected to either
>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>> or xHCI controllers.

>> This is a new driver for this USB PHY currently already supported under drivers/
>> usb/phy/. The reason for writing the new driver was the requirement that the
>> multiplexing of USB channels to the controller be dynamic, depending on what
>> USB drivers are loaded,  rather than static as provided by the old driver.
>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>> ideally. The new driver only supports device tree probing for now.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> =================================>> --- /dev/null
>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> @@ -0,0 +1,60 @@
>> +* Renesas R-Car generation 2 USB PHY
>> +
>> +This file provides information on what the device node for the R-Car generation
>> +2 USB PHY contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
>> +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>> +- reg: offset and length of the register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".
>> +
>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>> +the second one is the USB controller selector and depends on the first:
>> +
>> ++-----------+---------------+---------------+
>> +|\ Selector |               |               |
>> ++ --------- +       0       |       1       |
>> +| Channel  \|               |               |
>> ++-----------+---------------+---------------+
>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>> +| 1         | PCI EHCI/OHCI | xHCI          |
>> ++-----------+---------------+---------------+
>> +
>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>> +channels and the controllers connected to them. These subnodes must contain the
>> +following properties:
>> +- renesas,phy-select: the first cell identifies the USB channel, the second cell
>> +  is the USB controller selector; see the table above for the values.
>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>> +  USB controller selected for that channel.
>> +
>> +Example (Lager board):
>> +
>> +	usb-phy@e6590100 {
>> +		compatible = "renesas,usb-phy-r8a7790";
>> +		reg = <0 0xe6590100 0 0x100>;
>> +		#phy-cells = <2>;
>> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>> +		clock-names = "usbhs";
>> +
>> +		usb-phy@0,0 {
>> +			renesas,phy-select = <0 0>;
>> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>> +		};
>> +		usb-phy@0,1 {
>> +			renesas,phy-select = <0 1>;
>> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>> +		};
>> +		usb-phy@1,0 {
>> +			renesas,phy-select = <1 0>;
>> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>> +		};
>> +		usb-phy@1,1 {
>> +			renesas,phy-select = <1 1>;
>> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>> +		};
>> +	};

> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
> xHCI. right?

    Yes. However that depends on the driver load order: if e.g. xHCI driver is 
loaded later than PCI USB drivers,
it will override the channel routing.

> So ideally only two sub-nodes should be created for channel '0' and channel
> '1'.

    Hm, but I need to perform a special PHY power up sequence for the USBHS 
PHY itself (corresponding to channel #0, selector #1).

> You can configure a channel to a particular mode by passing the mode in
> PHY specifier

    I already have "#phy-cells" prop equal to 2.

> (The channel should be configured to a particualr mode in xlate).

    I have even considered using the of_xlate() method at first but then 
abandoned that idea for the phy_init() method...

> Btw I'm not sure if it is recommended to pass register mask values from dt.

    Neither am I. I did that because you requested the device tree 
representation for each of the sub-PHYs under that part of the driver which 
initialized the register masks, so that I thought that you wanted those masks 
to be represented in the device tree as well...

[...]
>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>> =================================>> --- /dev/null
>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>> @@ -0,0 +1,288 @@
[...]
>> +#define USBHS_LPSTS			0x02
>> +#define USBHS_UGCTRL			0x80
>> +#define USBHS_UGCTRL2			0x84
>> +#define USBHS_UGSTS			0x88
>> +
>> +/* Low Power Status register (LPSTS) */
>> +#define USBHS_LPSTS_SUSPM		0x4000
>> +
>> +/* USB General control register (UGCTRL) */
>> +#define USBHS_UGCTRL_CONNECT		0x00000004
>> +#define USBHS_UGCTRL_PLLRESET		0x00000001
>> +
>> +/* USB General control register 2 (UGCTRL2) */
>> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
>> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
>> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
>> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
>> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
>> +
>> +/* USB General status register (UGSTS) */
>> +#define USBHS_UGSTS_LOCK		0x00000300
>> +
>> +#define	NUM_USB_CHANNELS	2
>> +
>> +struct rcar_gen2_phy {
>> +	struct phy *phy;
>> +	struct rcar_gen2_phy_driver *drv;
>> +	u32 select_mask;
>> +	u32 select_value;
>> +};
>> +
>> +struct rcar_gen2_phy_driver {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	spinlock_t lock;
>> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

> This can be created dynamically based on the number of sub-nodes. In this case

    Hm, that sounds to me like another complication of the driver with no 
apparent win...

> it'll be only rcar_gen2_phy phys[2], one for each channel.
> By this we need not hard code NUM_USB_CHANNELS.

    I don't quite understand what's up with hard-coding it -- this constant is 
dictated by the UGCTRL2 register layout anyway.

> Thanks
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-04 21:54     ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-04 21:54 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:

>> This PHY, though formally being a part of Renesas USBHS controller, contains the
>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls them
>> channels) to the different USB controllers: channel 0 can be connected to either
>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>> or xHCI controllers.

>> This is a new driver for this USB PHY currently already supported under drivers/
>> usb/phy/. The reason for writing the new driver was the requirement that the
>> multiplexing of USB channels to the controller be dynamic, depending on what
>> USB drivers are loaded,  rather than static as provided by the old driver.
>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>> ideally. The new driver only supports device tree probing for now.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> ===================================================================
>> --- /dev/null
>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> @@ -0,0 +1,60 @@
>> +* Renesas R-Car generation 2 USB PHY
>> +
>> +This file provides information on what the device node for the R-Car generation
>> +2 USB PHY contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC.
>> +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>> +- reg: offset and length of the register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".
>> +
>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>> +the second one is the USB controller selector and depends on the first:
>> +
>> ++-----------+---------------+---------------+
>> +|\ Selector |               |               |
>> ++ --------- +       0       |       1       |
>> +| Channel  \|               |               |
>> ++-----------+---------------+---------------+
>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>> +| 1         | PCI EHCI/OHCI | xHCI          |
>> ++-----------+---------------+---------------+
>> +
>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>> +channels and the controllers connected to them. These subnodes must contain the
>> +following properties:
>> +- renesas,phy-select: the first cell identifies the USB channel, the second cell
>> +  is the USB controller selector; see the table above for the values.
>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>> +  USB controller selected for that channel.
>> +
>> +Example (Lager board):
>> +
>> +	usb-phy@e6590100 {
>> +		compatible = "renesas,usb-phy-r8a7790";
>> +		reg = <0 0xe6590100 0 0x100>;
>> +		#phy-cells = <2>;
>> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>> +		clock-names = "usbhs";
>> +
>> +		usb-phy@0,0 {
>> +			renesas,phy-select = <0 0>;
>> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>> +		};
>> +		usb-phy@0,1 {
>> +			renesas,phy-select = <0 1>;
>> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>> +		};
>> +		usb-phy@1,0 {
>> +			renesas,phy-select = <1 0>;
>> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>> +		};
>> +		usb-phy@1,1 {
>> +			renesas,phy-select = <1 1>;
>> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>> +		};
>> +	};

> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
> xHCI. right?

    Yes. However that depends on the driver load order: if e.g. xHCI driver is 
loaded later than PCI USB drivers,
it will override the channel routing.

> So ideally only two sub-nodes should be created for channel '0' and channel
> '1'.

    Hm, but I need to perform a special PHY power up sequence for the USBHS 
PHY itself (corresponding to channel #0, selector #1).

> You can configure a channel to a particular mode by passing the mode in
> PHY specifier

    I already have "#phy-cells" prop equal to 2.

> (The channel should be configured to a particualr mode in xlate).

    I have even considered using the of_xlate() method at first but then 
abandoned that idea for the phy_init() method...

> Btw I'm not sure if it is recommended to pass register mask values from dt.

    Neither am I. I did that because you requested the device tree 
representation for each of the sub-PHYs under that part of the driver which 
initialized the register masks, so that I thought that you wanted those masks 
to be represented in the device tree as well...

[...]
>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>> ===================================================================
>> --- /dev/null
>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>> @@ -0,0 +1,288 @@
[...]
>> +#define USBHS_LPSTS			0x02
>> +#define USBHS_UGCTRL			0x80
>> +#define USBHS_UGCTRL2			0x84
>> +#define USBHS_UGSTS			0x88
>> +
>> +/* Low Power Status register (LPSTS) */
>> +#define USBHS_LPSTS_SUSPM		0x4000
>> +
>> +/* USB General control register (UGCTRL) */
>> +#define USBHS_UGCTRL_CONNECT		0x00000004
>> +#define USBHS_UGCTRL_PLLRESET		0x00000001
>> +
>> +/* USB General control register 2 (UGCTRL2) */
>> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
>> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
>> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
>> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
>> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
>> +
>> +/* USB General status register (UGSTS) */
>> +#define USBHS_UGSTS_LOCK		0x00000300
>> +
>> +#define	NUM_USB_CHANNELS	2
>> +
>> +struct rcar_gen2_phy {
>> +	struct phy *phy;
>> +	struct rcar_gen2_phy_driver *drv;
>> +	u32 select_mask;
>> +	u32 select_value;
>> +};
>> +
>> +struct rcar_gen2_phy_driver {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	spinlock_t lock;
>> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

> This can be created dynamically based on the number of sub-nodes. In this case

    Hm, that sounds to me like another complication of the driver with no 
apparent win...

> it'll be only rcar_gen2_phy phys[2], one for each channel.
> By this we need not hard code NUM_USB_CHANNELS.

    I don't quite understand what's up with hard-coding it -- this constant is 
dictated by the UGCTRL2 register layout anyway.

> Thanks
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-04 21:54     ` Sergei Shtylyov
@ 2014-06-09 11:43       ` Laurent Pinchart
  -1 siblings, 0 replies; 38+ messages in thread
From: Laurent Pinchart @ 2014-06-09 11:43 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree, rdunlap,
	linux-doc, linux-sh

Hello,

On Thursday 05 June 2014 01:54:47 Sergei Shtylyov wrote:
> Hello.
> 
> On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:
> >> This PHY, though formally being a part of Renesas USBHS controller,
> >> contains the UGCTRL2 register that controls multiplexing of the USB
> >> ports (Renesas calls them channels) to the different USB controllers:
> >> channel 0 can be connected to either PCI EHCI/OHCI or USBHS controllers,
> >> channel 2 can be connected to PCI EHCI/OHCI or xHCI controllers.
> >> 
> >> This is a new driver for this USB PHY currently already supported under
> >> drivers/ usb/phy/. The reason for writing the new driver was the
> >> requirement that the multiplexing of USB channels to the controller be
> >> dynamic, depending on what USB drivers are loaded,  rather than static
> >> as provided by the old driver. The infrastructure provided by
> >> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
> >> only supports device tree probing for now.
> >> 
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
> >> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> >> =================================> >> --- /dev/null
> >> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> >> @@ -0,0 +1,60 @@
> >> +* Renesas R-Car generation 2 USB PHY
> >> +
> >> +This file provides information on what the device node for the R-Car
> >> generation +2 USB PHY contains.
> >> +
> >> +Required properties:
> >> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of
> >> R8A7790 SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part
> >> of R8A7791 SoC. +- reg: offset and length of the register block.
> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> >> +- clocks: clock phandle and specifier pair.
> >> +- clock-names: string, clock input name, must be "usbhs".
> >> +
> >> +The phandle's first argument in the PHY specifier identifies the USB
> >> channel, +the second one is the USB controller selector and depends on
> >> the first: +
> >> ++-----------+---------------+---------------+
> >> +|\ Selector |               |               |
> >> ++ --------- +       0       |       1       |
> >> +| Channel  \|               |               |
> >> ++-----------+---------------+---------------+
> >> +| 0         | PCI EHCI/OHCI | HS-USB        |
> >> +| 1         | PCI EHCI/OHCI | xHCI          |
> >> ++-----------+---------------+---------------+
> >> +
> >> +The USB PHY device tree node should be the subnodes corresponding to the
> >> USB
> >> +channels and the controllers connected to them. These subnodes must
> >> contain the
> >> +following properties:
> >> +- renesas,phy-select: the first cell identifies the USB channel, the
> >> second cell
> >> +  is the USB controller selector; see the table above for the values.
> >> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask
> >> corresponding to
> >> +  the USB channel, the second cell is the UGCTRL2 value corresponding to
> >> the
> >> +  USB controller selected for that channel.
> >> +
> >> +Example (Lager board):
> >> +
> >> +	usb-phy@e6590100 {
> >> +		compatible = "renesas,usb-phy-r8a7790";
> >> +		reg = <0 0xe6590100 0 0x100>;
> >> +		#phy-cells = <2>;
> >> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> >> +		clock-names = "usbhs";
> >> +
> >> +		usb-phy@0,0 {
> >> +			renesas,phy-select = <0 0>;
> >> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> >> +		};
> >> +		usb-phy@0,1 {
> >> +			renesas,phy-select = <0 1>;
> >> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> >> +		};
> >> +		usb-phy@1,0 {
> >> +			renesas,phy-select = <1 0>;
> >> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> >> +		};
> >> +		usb-phy@1,1 {
> >> +			renesas,phy-select = <1 1>;
> >> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> >> +		};
> >> +	};
> > 
> > IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't
> > be used for both.  And channel 1 can be configured for either PCI
> > EHCI/OHCI or xHCI. right?
> 
> Yes. However that depends on the driver load order: if e.g. xHCI driver
> is loaded later than PCI USB drivers,
> it will override the channel routing.
> 
> > So ideally only two sub-nodes should be created for channel '0' and
> > channel '1'.
> 
> Hm, but I need to perform a special PHY power up sequence for the USBHS
> PHY itself (corresponding to channel #0, selector #1).
> 
> > You can configure a channel to a particular mode by passing the mode in
> > PHY specifier
> 
> I already have "#phy-cells" prop equal to 2.
> 
> > (The channel should be configured to a particualr mode in xlate).
> 
> I have even considered using the of_xlate() method at first but then
> abandoned that idea for the phy_init() method...
> 
> > Btw I'm not sure if it is recommended to pass register mask values from
> > dt.
> 
> Neither am I. I did that because you requested the device tree
> representation for each of the sub-PHYs under that part of the driver which
> initialized the register masks, so that I thought that you wanted those
> masks to be represented in the device tree as well...

Given that this is a new driver, and that we can't know how future generations 
of the device will behave and what information will be required, wouldn't it 
make more sense to move the information represented by the subnodes to the 
driver ?

> [...]
> 
> >> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> >> =================================> >> --- /dev/null
> >> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> >> @@ -0,0 +1,288 @@
> 
> [...]
> 
> >> +#define USBHS_LPSTS			0x02
> >> +#define USBHS_UGCTRL			0x80
> >> +#define USBHS_UGCTRL2			0x84
> >> +#define USBHS_UGSTS			0x88
> >> +
> >> +/* Low Power Status register (LPSTS) */
> >> +#define USBHS_LPSTS_SUSPM		0x4000
> >> +
> >> +/* USB General control register (UGCTRL) */
> >> +#define USBHS_UGCTRL_CONNECT		0x00000004
> >> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> >> +
> >> +/* USB General control register 2 (UGCTRL2) */
> >> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> >> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> >> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> >> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> >> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> >> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> >> +
> >> +/* USB General status register (UGSTS) */
> >> +#define USBHS_UGSTS_LOCK		0x00000300
> >> +
> >> +#define	NUM_USB_CHANNELS	2
> >> +
> >> +struct rcar_gen2_phy {
> >> +	struct phy *phy;
> >> +	struct rcar_gen2_phy_driver *drv;
> >> +	u32 select_mask;
> >> +	u32 select_value;
> >> +};
> >> +
> >> +struct rcar_gen2_phy_driver {
> >> +	void __iomem *base;
> >> +	struct clk *clk;
> >> +	spinlock_t lock;
> >> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> > 
> > This can be created dynamically based on the number of sub-nodes. In this
> > case
>
> Hm, that sounds to me like another complication of the driver with no
> apparent win...
> 
> > it'll be only rcar_gen2_phy phys[2], one for each channel.
> > By this we need not hard code NUM_USB_CHANNELS.
> 
> I don't quite understand what's up with hard-coding it -- this constant is
> dictated by the UGCTRL2 register layout anyway.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-09 11:43       ` Laurent Pinchart
  0 siblings, 0 replies; 38+ messages in thread
From: Laurent Pinchart @ 2014-06-09 11:43 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree, rdunlap,
	linux-doc, linux-sh

Hello,

On Thursday 05 June 2014 01:54:47 Sergei Shtylyov wrote:
> Hello.
> 
> On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:
> >> This PHY, though formally being a part of Renesas USBHS controller,
> >> contains the UGCTRL2 register that controls multiplexing of the USB
> >> ports (Renesas calls them channels) to the different USB controllers:
> >> channel 0 can be connected to either PCI EHCI/OHCI or USBHS controllers,
> >> channel 2 can be connected to PCI EHCI/OHCI or xHCI controllers.
> >> 
> >> This is a new driver for this USB PHY currently already supported under
> >> drivers/ usb/phy/. The reason for writing the new driver was the
> >> requirement that the multiplexing of USB channels to the controller be
> >> dynamic, depending on what USB drivers are loaded,  rather than static
> >> as provided by the old driver. The infrastructure provided by
> >> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
> >> only supports device tree probing for now.
> >> 
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
> >> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> >> ===================================================================
> >> --- /dev/null
> >> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> >> @@ -0,0 +1,60 @@
> >> +* Renesas R-Car generation 2 USB PHY
> >> +
> >> +This file provides information on what the device node for the R-Car
> >> generation +2 USB PHY contains.
> >> +
> >> +Required properties:
> >> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of
> >> R8A7790 SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part
> >> of R8A7791 SoC. +- reg: offset and length of the register block.
> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> >> +- clocks: clock phandle and specifier pair.
> >> +- clock-names: string, clock input name, must be "usbhs".
> >> +
> >> +The phandle's first argument in the PHY specifier identifies the USB
> >> channel, +the second one is the USB controller selector and depends on
> >> the first: +
> >> ++-----------+---------------+---------------+
> >> +|\ Selector |               |               |
> >> ++ --------- +       0       |       1       |
> >> +| Channel  \|               |               |
> >> ++-----------+---------------+---------------+
> >> +| 0         | PCI EHCI/OHCI | HS-USB        |
> >> +| 1         | PCI EHCI/OHCI | xHCI          |
> >> ++-----------+---------------+---------------+
> >> +
> >> +The USB PHY device tree node should be the subnodes corresponding to the
> >> USB
> >> +channels and the controllers connected to them. These subnodes must
> >> contain the
> >> +following properties:
> >> +- renesas,phy-select: the first cell identifies the USB channel, the
> >> second cell
> >> +  is the USB controller selector; see the table above for the values.
> >> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask
> >> corresponding to
> >> +  the USB channel, the second cell is the UGCTRL2 value corresponding to
> >> the
> >> +  USB controller selected for that channel.
> >> +
> >> +Example (Lager board):
> >> +
> >> +	usb-phy@e6590100 {
> >> +		compatible = "renesas,usb-phy-r8a7790";
> >> +		reg = <0 0xe6590100 0 0x100>;
> >> +		#phy-cells = <2>;
> >> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> >> +		clock-names = "usbhs";
> >> +
> >> +		usb-phy@0,0 {
> >> +			renesas,phy-select = <0 0>;
> >> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> >> +		};
> >> +		usb-phy@0,1 {
> >> +			renesas,phy-select = <0 1>;
> >> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> >> +		};
> >> +		usb-phy@1,0 {
> >> +			renesas,phy-select = <1 0>;
> >> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> >> +		};
> >> +		usb-phy@1,1 {
> >> +			renesas,phy-select = <1 1>;
> >> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> >> +		};
> >> +	};
> > 
> > IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't
> > be used for both.  And channel 1 can be configured for either PCI
> > EHCI/OHCI or xHCI. right?
> 
> Yes. However that depends on the driver load order: if e.g. xHCI driver
> is loaded later than PCI USB drivers,
> it will override the channel routing.
> 
> > So ideally only two sub-nodes should be created for channel '0' and
> > channel '1'.
> 
> Hm, but I need to perform a special PHY power up sequence for the USBHS
> PHY itself (corresponding to channel #0, selector #1).
> 
> > You can configure a channel to a particular mode by passing the mode in
> > PHY specifier
> 
> I already have "#phy-cells" prop equal to 2.
> 
> > (The channel should be configured to a particualr mode in xlate).
> 
> I have even considered using the of_xlate() method at first but then
> abandoned that idea for the phy_init() method...
> 
> > Btw I'm not sure if it is recommended to pass register mask values from
> > dt.
> 
> Neither am I. I did that because you requested the device tree
> representation for each of the sub-PHYs under that part of the driver which
> initialized the register masks, so that I thought that you wanted those
> masks to be represented in the device tree as well...

Given that this is a new driver, and that we can't know how future generations 
of the device will behave and what information will be required, wouldn't it 
make more sense to move the information represented by the subnodes to the 
driver ?

> [...]
> 
> >> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> >> ===================================================================
> >> --- /dev/null
> >> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> >> @@ -0,0 +1,288 @@
> 
> [...]
> 
> >> +#define USBHS_LPSTS			0x02
> >> +#define USBHS_UGCTRL			0x80
> >> +#define USBHS_UGCTRL2			0x84
> >> +#define USBHS_UGSTS			0x88
> >> +
> >> +/* Low Power Status register (LPSTS) */
> >> +#define USBHS_LPSTS_SUSPM		0x4000
> >> +
> >> +/* USB General control register (UGCTRL) */
> >> +#define USBHS_UGCTRL_CONNECT		0x00000004
> >> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> >> +
> >> +/* USB General control register 2 (UGCTRL2) */
> >> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> >> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> >> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> >> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> >> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> >> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> >> +
> >> +/* USB General status register (UGSTS) */
> >> +#define USBHS_UGSTS_LOCK		0x00000300
> >> +
> >> +#define	NUM_USB_CHANNELS	2
> >> +
> >> +struct rcar_gen2_phy {
> >> +	struct phy *phy;
> >> +	struct rcar_gen2_phy_driver *drv;
> >> +	u32 select_mask;
> >> +	u32 select_value;
> >> +};
> >> +
> >> +struct rcar_gen2_phy_driver {
> >> +	void __iomem *base;
> >> +	struct clk *clk;
> >> +	spinlock_t lock;
> >> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> > 
> > This can be created dynamically based on the number of sub-nodes. In this
> > case
>
> Hm, that sounds to me like another complication of the driver with no
> apparent win...
> 
> > it'll be only rcar_gen2_phy phys[2], one for each channel.
> > By this we need not hard code NUM_USB_CHANNELS.
> 
> I don't quite understand what's up with hard-coding it -- this constant is
> dictated by the UGCTRL2 register layout anyway.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-05-23 22:06 ` Sergei Shtylyov
@ 2014-06-09 11:44   ` Laurent Pinchart
  -1 siblings, 0 replies; 38+ messages in thread
From: Laurent Pinchart @ 2014-06-09 11:44 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree, rdunlap, linux-doc, linux-sh

Hi Sergei,

Thank you for the patch.

On Saturday 24 May 2014 02:06:03 Sergei Shtylyov wrote:
> This PHY, though formally being a part of Renesas USBHS controller, contains
> the UGCTRL2 register that controls multiplexing of the USB ports (Renesas
> calls them channels) to the different USB controllers: channel 0 can be
> connected to either PCI EHCI/OHCI or USBHS controllers, channel 2 can be
> connected to PCI EHCI/OHCI or xHCI controllers.
> 
> This is a new driver for this USB PHY currently already supported under
> drivers/ usb/phy/. The reason for writing the new driver was the
> requirement that the multiplexing of USB channels to the controller be
> dynamic, depending on what USB drivers are loaded,  rather than static as
> provided by the old driver. The infrastructure provided by
> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
> only supports device tree probing for now.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.
> 
> Changes in version 4:
> - added subnodes to the USB PHY node, described their properties in the
> binding document; rewrote the probe() method to parse out PHY selection and
> UGCTRL2 mask values out of these child nodes;
> - changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
> don't have control over it anyway;
> - refreshed the patch.
> 
> Changes in version 3:
> - removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
>   'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
>   methods;
> - renamed the power_{on|off}() methods;
> - replaced io{read|write}16() calls with {read|write}w();
> - removed the comment to '#define USBHS_UGSTS_LOCK';
> - broke the line in the dev_err() call in the probe() method;
> - moved the driver's line in the 'Makefile' before OMAP drivers.
> 
> Changes in version 2:
> - rebased the patch, resolving reject.
> 
>  Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
>  drivers/phy/Kconfig                                     |    7
>  drivers/phy/Makefile                                    |    1
>  drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++
>  4 files changed, 356 insertions(+)
> 
> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> =================================> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car
> generation +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
> SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791
> SoC. +- reg: offset and length of the register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".

Is that the functional clock for the PHY ? If so, what about naming it "fck" 
(or something similar) ?

> +The phandle's first argument in the PHY specifier identifies the USB
> channel, +the second one is the USB controller selector and depends on the
> first: +
> ++-----------+---------------+---------------+
> +|\ Selector |               |               |
> ++ --------- +       0       |       1       |
> +| Channel  \|               |               |
> ++-----------+---------------+---------------+
> +| 0         | PCI EHCI/OHCI | HS-USB        |
> +| 1         | PCI EHCI/OHCI | xHCI          |
> ++-----------+---------------+---------------+
> +
> +The USB PHY device tree node should be the subnodes corresponding to the
> USB +channels and the controllers connected to them. These subnodes must
> contain the +following properties:
> +- renesas,phy-select: the first cell identifies the USB channel, the second
> cell +  is the USB controller selector; see the table above for the values.
> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding
> to +  the USB channel, the second cell is the UGCTRL2 value corresponding
> to the +  USB controller selected for that channel.
> +
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";
> +
> +		usb-phy@0,0 {
> +			renesas,phy-select = <0 0>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> +		};
> +		usb-phy@0,1 {
> +			renesas,phy-select = <0 1>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> +		};
> +		usb-phy@1,0 {
> +			renesas,phy-select = <1 0>;
> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> +		};
> +		usb-phy@1,1 {
> +			renesas,phy-select = <1 1>;
> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> +		};
> +	};
> Index: linux-phy/drivers/phy/Kconfig
> =================================> --- linux-phy.orig/drivers/phy/Kconfig
> +++ linux-phy/drivers/phy/Kconfig
> @@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
>  	depends on OF
>  	select GENERIC_PHY
> 
> +config PHY_RCAR_GEN2
> +	tristate "Renesas R-Car generation 2 USB PHY driver"
> +	depends on ARCH_SHMOBILE
> +	depends on GENERIC_PHY
> +	help
> +	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> Index: linux-phy/drivers/phy/Makefile
> =================================> --- linux-phy.orig/drivers/phy/Makefile
> +++ linux-phy/drivers/phy/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
>  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_MVEBU_SATA)		+= phy-mvebu-sata.o
> +obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
>  obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
>  obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> =================================> --- /dev/null
> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> @@ -0,0 +1,288 @@
> +/*
> + * Renesas R-Car Gen2 PHY driver
> + *
> + * Copyright (C) 2014 Renesas Solutions Corp.
> + * Copyright (C) 2014 Cogent Embedded, Inc.
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define USBHS_LPSTS			0x02
> +#define USBHS_UGCTRL			0x80
> +#define USBHS_UGCTRL2			0x84
> +#define USBHS_UGSTS			0x88
> +
> +/* Low Power Status register (LPSTS) */
> +#define USBHS_LPSTS_SUSPM		0x4000
> +
> +/* USB General control register (UGCTRL) */
> +#define USBHS_UGCTRL_CONNECT		0x00000004
> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> +
> +/* USB General control register 2 (UGCTRL2) */
> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> +
> +/* USB General status register (UGSTS) */
> +#define USBHS_UGSTS_LOCK		0x00000300
> +
> +#define	NUM_USB_CHANNELS	2
> +
> +struct rcar_gen2_phy {
> +	struct phy *phy;
> +	struct rcar_gen2_phy_driver *drv;
> +	u32 select_mask;
> +	u32 select_value;
> +};
> +
> +struct rcar_gen2_phy_driver {
> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t lock;
> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> +};
> +
> +static int rcar_gen2_phy_init(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	unsigned long flags;
> +	u32 ugctrl2;
> +
> +	clk_prepare_enable(drv->clk);
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +	ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
> +	ugctrl2 &= ~phy->select_mask;
> +	ugctrl2 |= phy->select_value;
> +	writel(ugctrl2, drv->base + USBHS_UGCTRL2);
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +	return 0;
> +}
> +
> +static int rcar_gen2_phy_exit(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +
> +	clk_disable_unprepare(phy->drv->clk);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen2_phy_power_on(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	void __iomem *base = drv->base;
> +	unsigned long flags;
> +	u32 value;
> +	int err = 0, i;
> +
> +	/* Skip if it's not USBHS */
> +	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
> +		return 0;
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +
> +	/* Power on USBHS PHY */
> +	value = readl(base + USBHS_UGCTRL);
> +	value &= ~USBHS_UGCTRL_PLLRESET;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	value = readw(base + USBHS_LPSTS);
> +	value |= USBHS_LPSTS_SUSPM;
> +	writew(value, base + USBHS_LPSTS);
> +
> +	for (i = 0; i < 20; i++) {
> +		value = readl(base + USBHS_UGSTS);
> +		if ((value & USBHS_UGSTS_LOCK) = USBHS_UGSTS_LOCK) {
> +			value = readl(base + USBHS_UGCTRL);
> +			value |= USBHS_UGCTRL_CONNECT;
> +			writel(value, base + USBHS_UGCTRL);
> +			goto out;
> +		}
> +		udelay(1);
> +	}
> +
> +	/* Timed out waiting for the PLL lock */
> +	err = -ETIMEDOUT;
> +
> +out:
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +
> +	return err;
> +}
> +
> +static int rcar_gen2_phy_power_off(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	void __iomem *base = drv->base;
> +	unsigned long flags;
> +	u32 value;
> +
> +	/* Skip if it's not USBHS */
> +	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
> +		return 0;
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +
> +	/* Power off USBHS PHY */
> +	value = readl(base + USBHS_UGCTRL);
> +	value &= ~USBHS_UGCTRL_CONNECT;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	value = readw(base + USBHS_LPSTS);
> +	value &= ~USBHS_LPSTS_SUSPM;
> +	writew(value, base + USBHS_LPSTS);
> +
> +	value = readl(base + USBHS_UGCTRL);
> +	value |= USBHS_UGCTRL_PLLRESET;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops rcar_gen2_phy_ops = {
> +	.init		= rcar_gen2_phy_init,
> +	.exit		= rcar_gen2_phy_exit,
> +	.power_on	= rcar_gen2_phy_power_on,
> +	.power_off	= rcar_gen2_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id rcar_gen2_phy_match_table[] = {
> +	{ .compatible = "renesas,usb-phy-r8a7790" },
> +	{ .compatible = "renesas,usb-phy-r8a7791" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen2_phy_match_table);
> +
> +static struct phy *rcar_gen2_phy_xlate(struct device *dev,
> +				       struct of_phandle_args *args)
> +{
> +	struct rcar_gen2_phy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (args->args[0] >= NUM_USB_CHANNELS || args->args[1] >= 2)
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->phys[args->args[0]][args->args[1]].phy;
> +}
> +
> +static int rcar_gen2_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rcar_gen2_phy_driver *drv;
> +	struct phy_provider *provider;
> +	struct device_node *np;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev,
> +			"This driver is required to be instantiated from device tree\n");
> +		return -EINVAL;
> +	}
> +
> +	clk = devm_clk_get(dev, "usbhs");
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "Can't get USBHS clock\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&drv->lock);
> +
> +	drv->clk = clk;
> +	drv->base = base;
> +
> +	for_each_child_of_node(dev->of_node, np) {
> +  		struct rcar_gen2_phy *phy;
> +		u32 values[2];
> +		int error;
> +
> +		error = of_property_read_u32_array(np, "renesas,phy-select",
> +						   values, 2);
> +		if (error) {
> +			dev_err(dev, "Failed to read \"renesas,phy-select\" property\n");
> +			return error;
> +		}
> +		if (values[0] >= NUM_USB_CHANNELS || values[1] >= 2) {
> +			dev_err(dev, "Values out of range in \"renesas,phy-select\"
> property\n"); +			return error;
> +		}
> +		phy = &drv->phys[values[0]][values[1]];
> +
> +		error = of_property_read_u32_array(np, "renesas,ugctrl2-masks",
> +						   values, 2);
> +		if (error) {
> +			dev_err(dev, "Failed to read \"renesas,ugctrl2-masks\" 
property\n");
> +			return error;
> +		}
> +		phy->select_mask  = values[0];
> +		phy->select_value = values[1];
> +
> +		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
> +		if (IS_ERR(phy->phy)) {
> +			dev_err(dev, "Failed to create PHY\n");
> +			return PTR_ERR(phy->phy);
> +		}
> +
> +		phy->drv = drv;
> +		phy_set_drvdata(phy->phy, phy);
> +	}
> +
> +	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(dev, "Failed to register PHY provider\n");
> +		return PTR_ERR(provider);
> +	}
> +
> +	dev_set_drvdata(dev, drv);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rcar_gen2_phy_driver = {
> +	.driver = {
> +		.name		= "phy_rcar_gen2",
> +		.of_match_table	= rcar_gen2_phy_match_table,
> +	},
> +	.probe	= rcar_gen2_phy_probe,
> +};
> +
> +module_platform_driver(rcar_gen2_phy_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Renesas R-Car Gen2 PHY");
> +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>");
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-09 11:44   ` Laurent Pinchart
  0 siblings, 0 replies; 38+ messages in thread
From: Laurent Pinchart @ 2014-06-09 11:44 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree, rdunlap, linux-doc, linux-sh

Hi Sergei,

Thank you for the patch.

On Saturday 24 May 2014 02:06:03 Sergei Shtylyov wrote:
> This PHY, though formally being a part of Renesas USBHS controller, contains
> the UGCTRL2 register that controls multiplexing of the USB ports (Renesas
> calls them channels) to the different USB controllers: channel 0 can be
> connected to either PCI EHCI/OHCI or USBHS controllers, channel 2 can be
> connected to PCI EHCI/OHCI or xHCI controllers.
> 
> This is a new driver for this USB PHY currently already supported under
> drivers/ usb/phy/. The reason for writing the new driver was the
> requirement that the multiplexing of USB channels to the controller be
> dynamic, depending on what USB drivers are loaded,  rather than static as
> provided by the old driver. The infrastructure provided by
> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
> only supports device tree probing for now.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'next' branch of Kishon's 'linux-phy.git' repo.
> 
> Changes in version 4:
> - added subnodes to the USB PHY node, described their properties in the
> binding document; rewrote the probe() method to parse out PHY selection and
> UGCTRL2 mask values out of these child nodes;
> - changed NUM_USB_CHANNELS to 2, removed the former channel #1 support as we
> don't have control over it anyway;
> - refreshed the patch.
> 
> Changes in version 3:
> - removed 'rcar_gen2_usbhs_phy_ops', moving 'power_{on|off}' intializers to
>   'rcar_gen2_phy_ops' and adding a check for USBHS PHY to power_{on|off}()
>   methods;
> - renamed the power_{on|off}() methods;
> - replaced io{read|write}16() calls with {read|write}w();
> - removed the comment to '#define USBHS_UGSTS_LOCK';
> - broke the line in the dev_err() call in the probe() method;
> - moved the driver's line in the 'Makefile' before OMAP drivers.
> 
> Changes in version 2:
> - rebased the patch, resolving reject.
> 
>  Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt |   60 +++
>  drivers/phy/Kconfig                                     |    7
>  drivers/phy/Makefile                                    |    1
>  drivers/phy/phy-rcar-gen2.c                             |  288 ++++++++++++
>  4 files changed, 356 insertions(+)
> 
> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> ===================================================================
> --- /dev/null
> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
> @@ -0,0 +1,60 @@
> +* Renesas R-Car generation 2 USB PHY
> +
> +This file provides information on what the device node for the R-Car
> generation +2 USB PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
> SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791
> SoC. +- reg: offset and length of the register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".

Is that the functional clock for the PHY ? If so, what about naming it "fck" 
(or something similar) ?

> +The phandle's first argument in the PHY specifier identifies the USB
> channel, +the second one is the USB controller selector and depends on the
> first: +
> ++-----------+---------------+---------------+
> +|\ Selector |               |               |
> ++ --------- +       0       |       1       |
> +| Channel  \|               |               |
> ++-----------+---------------+---------------+
> +| 0         | PCI EHCI/OHCI | HS-USB        |
> +| 1         | PCI EHCI/OHCI | xHCI          |
> ++-----------+---------------+---------------+
> +
> +The USB PHY device tree node should be the subnodes corresponding to the
> USB +channels and the controllers connected to them. These subnodes must
> contain the +following properties:
> +- renesas,phy-select: the first cell identifies the USB channel, the second
> cell +  is the USB controller selector; see the table above for the values.
> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding
> to +  the USB channel, the second cell is the UGCTRL2 value corresponding
> to the +  USB controller selected for that channel.
> +
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		#phy-cells = <2>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";
> +
> +		usb-phy@0,0 {
> +			renesas,phy-select = <0 0>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000010>;
> +		};
> +		usb-phy@0,1 {
> +			renesas,phy-select = <0 1>;
> +			renesas,ugctrl2-masks = <0x00000030 0x00000030>;
> +		};
> +		usb-phy@1,0 {
> +			renesas,phy-select = <1 0>;
> +			renesas,ugctrl2-masks = <0x80000000 0x00000000>;
> +		};
> +		usb-phy@1,1 {
> +			renesas,phy-select = <1 1>;
> +			renesas,ugctrl2-masks = <0x80000000 0x80000000>;
> +		};
> +	};
> Index: linux-phy/drivers/phy/Kconfig
> ===================================================================
> --- linux-phy.orig/drivers/phy/Kconfig
> +++ linux-phy/drivers/phy/Kconfig
> @@ -31,6 +31,13 @@ config PHY_MVEBU_SATA
>  	depends on OF
>  	select GENERIC_PHY
> 
> +config PHY_RCAR_GEN2
> +	tristate "Renesas R-Car generation 2 USB PHY driver"
> +	depends on ARCH_SHMOBILE
> +	depends on GENERIC_PHY
> +	help
> +	  Support for USB PHY found on Renesas R-Car generation 2 SoCs.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> Index: linux-phy/drivers/phy/Makefile
> ===================================================================
> --- linux-phy.orig/drivers/phy/Makefile
> +++ linux-phy/drivers/phy/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-
>  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_MVEBU_SATA)		+= phy-mvebu-sata.o
> +obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
>  obj-$(CONFIG_OMAP_CONTROL_PHY)		+= phy-omap-control.o
>  obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> ===================================================================
> --- /dev/null
> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> @@ -0,0 +1,288 @@
> +/*
> + * Renesas R-Car Gen2 PHY driver
> + *
> + * Copyright (C) 2014 Renesas Solutions Corp.
> + * Copyright (C) 2014 Cogent Embedded, Inc.
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define USBHS_LPSTS			0x02
> +#define USBHS_UGCTRL			0x80
> +#define USBHS_UGCTRL2			0x84
> +#define USBHS_UGSTS			0x88
> +
> +/* Low Power Status register (LPSTS) */
> +#define USBHS_LPSTS_SUSPM		0x4000
> +
> +/* USB General control register (UGCTRL) */
> +#define USBHS_UGCTRL_CONNECT		0x00000004
> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> +
> +/* USB General control register 2 (UGCTRL2) */
> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> +
> +/* USB General status register (UGSTS) */
> +#define USBHS_UGSTS_LOCK		0x00000300
> +
> +#define	NUM_USB_CHANNELS	2
> +
> +struct rcar_gen2_phy {
> +	struct phy *phy;
> +	struct rcar_gen2_phy_driver *drv;
> +	u32 select_mask;
> +	u32 select_value;
> +};
> +
> +struct rcar_gen2_phy_driver {
> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t lock;
> +	struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> +};
> +
> +static int rcar_gen2_phy_init(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	unsigned long flags;
> +	u32 ugctrl2;
> +
> +	clk_prepare_enable(drv->clk);
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +	ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
> +	ugctrl2 &= ~phy->select_mask;
> +	ugctrl2 |= phy->select_value;
> +	writel(ugctrl2, drv->base + USBHS_UGCTRL2);
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +	return 0;
> +}
> +
> +static int rcar_gen2_phy_exit(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +
> +	clk_disable_unprepare(phy->drv->clk);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen2_phy_power_on(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	void __iomem *base = drv->base;
> +	unsigned long flags;
> +	u32 value;
> +	int err = 0, i;
> +
> +	/* Skip if it's not USBHS */
> +	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
> +		return 0;
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +
> +	/* Power on USBHS PHY */
> +	value = readl(base + USBHS_UGCTRL);
> +	value &= ~USBHS_UGCTRL_PLLRESET;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	value = readw(base + USBHS_LPSTS);
> +	value |= USBHS_LPSTS_SUSPM;
> +	writew(value, base + USBHS_LPSTS);
> +
> +	for (i = 0; i < 20; i++) {
> +		value = readl(base + USBHS_UGSTS);
> +		if ((value & USBHS_UGSTS_LOCK) == USBHS_UGSTS_LOCK) {
> +			value = readl(base + USBHS_UGCTRL);
> +			value |= USBHS_UGCTRL_CONNECT;
> +			writel(value, base + USBHS_UGCTRL);
> +			goto out;
> +		}
> +		udelay(1);
> +	}
> +
> +	/* Timed out waiting for the PLL lock */
> +	err = -ETIMEDOUT;
> +
> +out:
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +
> +	return err;
> +}
> +
> +static int rcar_gen2_phy_power_off(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_phy_driver *drv = phy->drv;
> +	void __iomem *base = drv->base;
> +	unsigned long flags;
> +	u32 value;
> +
> +	/* Skip if it's not USBHS */
> +	if (phy->select_value != USBHS_UGCTRL2_USB0SEL_HS_USB)
> +		return 0;
> +
> +	spin_lock_irqsave(&drv->lock, flags);
> +
> +	/* Power off USBHS PHY */
> +	value = readl(base + USBHS_UGCTRL);
> +	value &= ~USBHS_UGCTRL_CONNECT;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	value = readw(base + USBHS_LPSTS);
> +	value &= ~USBHS_LPSTS_SUSPM;
> +	writew(value, base + USBHS_LPSTS);
> +
> +	value = readl(base + USBHS_UGCTRL);
> +	value |= USBHS_UGCTRL_PLLRESET;
> +	writel(value, base + USBHS_UGCTRL);
> +
> +	spin_unlock_irqrestore(&drv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops rcar_gen2_phy_ops = {
> +	.init		= rcar_gen2_phy_init,
> +	.exit		= rcar_gen2_phy_exit,
> +	.power_on	= rcar_gen2_phy_power_on,
> +	.power_off	= rcar_gen2_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id rcar_gen2_phy_match_table[] = {
> +	{ .compatible = "renesas,usb-phy-r8a7790" },
> +	{ .compatible = "renesas,usb-phy-r8a7791" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen2_phy_match_table);
> +
> +static struct phy *rcar_gen2_phy_xlate(struct device *dev,
> +				       struct of_phandle_args *args)
> +{
> +	struct rcar_gen2_phy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (args->args[0] >= NUM_USB_CHANNELS || args->args[1] >= 2)
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->phys[args->args[0]][args->args[1]].phy;
> +}
> +
> +static int rcar_gen2_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rcar_gen2_phy_driver *drv;
> +	struct phy_provider *provider;
> +	struct device_node *np;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev,
> +			"This driver is required to be instantiated from device tree\n");
> +		return -EINVAL;
> +	}
> +
> +	clk = devm_clk_get(dev, "usbhs");
> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "Can't get USBHS clock\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&drv->lock);
> +
> +	drv->clk = clk;
> +	drv->base = base;
> +
> +	for_each_child_of_node(dev->of_node, np) {
> +  		struct rcar_gen2_phy *phy;
> +		u32 values[2];
> +		int error;
> +
> +		error = of_property_read_u32_array(np, "renesas,phy-select",
> +						   values, 2);
> +		if (error) {
> +			dev_err(dev, "Failed to read \"renesas,phy-select\" property\n");
> +			return error;
> +		}
> +		if (values[0] >= NUM_USB_CHANNELS || values[1] >= 2) {
> +			dev_err(dev, "Values out of range in \"renesas,phy-select\"
> property\n"); +			return error;
> +		}
> +		phy = &drv->phys[values[0]][values[1]];
> +
> +		error = of_property_read_u32_array(np, "renesas,ugctrl2-masks",
> +						   values, 2);
> +		if (error) {
> +			dev_err(dev, "Failed to read \"renesas,ugctrl2-masks\" 
property\n");
> +			return error;
> +		}
> +		phy->select_mask  = values[0];
> +		phy->select_value = values[1];
> +
> +		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
> +		if (IS_ERR(phy->phy)) {
> +			dev_err(dev, "Failed to create PHY\n");
> +			return PTR_ERR(phy->phy);
> +		}
> +
> +		phy->drv = drv;
> +		phy_set_drvdata(phy->phy, phy);
> +	}
> +
> +	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(dev, "Failed to register PHY provider\n");
> +		return PTR_ERR(provider);
> +	}
> +
> +	dev_set_drvdata(dev, drv);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rcar_gen2_phy_driver = {
> +	.driver = {
> +		.name		= "phy_rcar_gen2",
> +		.of_match_table	= rcar_gen2_phy_match_table,
> +	},
> +	.probe	= rcar_gen2_phy_probe,
> +};
> +
> +module_platform_driver(rcar_gen2_phy_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Renesas R-Car Gen2 PHY");
> +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>");
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-09 11:44   ` Laurent Pinchart
@ 2014-06-09 12:34     ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-09 12:34 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree, rdunlap, linux-doc, linux-sh

Hello.

On 06/09/2014 03:44 PM, Laurent Pinchart wrote:

> Thank you for the patch.

    Could you please remove the parts of the patch to which you didn't reply? 
I've hardly found your comment.

> On Saturday 24 May 2014 02:06:03 Sergei Shtylyov wrote:
>> This PHY, though formally being a part of Renesas USBHS controller, contains
>> the UGCTRL2 register that controls multiplexing of the USB ports (Renesas
>> calls them channels) to the different USB controllers: channel 0 can be
>> connected to either PCI EHCI/OHCI or USBHS controllers, channel 2 can be
>> connected to PCI EHCI/OHCI or xHCI controllers.

>> This is a new driver for this USB PHY currently already supported under
>> drivers/ usb/phy/. The reason for writing the new driver was the
>> requirement that the multiplexing of USB channels to the controller be
>> dynamic, depending on what USB drivers are loaded,  rather than static as
>> provided by the old driver. The infrastructure provided by
>> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
>> only supports device tree probing for now.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> =================================>> --- /dev/null
>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> @@ -0,0 +1,60 @@
>> +* Renesas R-Car generation 2 USB PHY
>> +
>> +This file provides information on what the device node for the R-Car
>> generation +2 USB PHY contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>> SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791
>> SoC. +- reg: offset and length of the register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".

> Is that the functional clock for the PHY ? If so, what about naming it "fck"
> (or something similar) ?

    No, it's the clock for the USBHS controller to which this PHY belongs.

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-09 12:34     ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-09 12:34 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, kishon,
	grant.likely, devicetree, rdunlap, linux-doc, linux-sh

Hello.

On 06/09/2014 03:44 PM, Laurent Pinchart wrote:

> Thank you for the patch.

    Could you please remove the parts of the patch to which you didn't reply? 
I've hardly found your comment.

> On Saturday 24 May 2014 02:06:03 Sergei Shtylyov wrote:
>> This PHY, though formally being a part of Renesas USBHS controller, contains
>> the UGCTRL2 register that controls multiplexing of the USB ports (Renesas
>> calls them channels) to the different USB controllers: channel 0 can be
>> connected to either PCI EHCI/OHCI or USBHS controllers, channel 2 can be
>> connected to PCI EHCI/OHCI or xHCI controllers.

>> This is a new driver for this USB PHY currently already supported under
>> drivers/ usb/phy/. The reason for writing the new driver was the
>> requirement that the multiplexing of USB channels to the controller be
>> dynamic, depending on what USB drivers are loaded,  rather than static as
>> provided by the old driver. The infrastructure provided by
>> drivers/phy/phy-core.c seems to fit that purpose ideally. The new driver
>> only supports device tree probing for now.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> ===================================================================
>> --- /dev/null
>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>> @@ -0,0 +1,60 @@
>> +* Renesas R-Car generation 2 USB PHY
>> +
>> +This file provides information on what the device node for the R-Car
>> generation +2 USB PHY contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>> SoC. +	      "renesas,usb-phy-r8a7791" if the device is a part of R8A7791
>> SoC. +- reg: offset and length of the register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".

> Is that the functional clock for the PHY ? If so, what about naming it "fck"
> (or something similar) ?

    No, it's the clock for the USBHS controller to which this PHY belongs.

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-04 21:54     ` Sergei Shtylyov
@ 2014-06-10 10:55       ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-06-10 10:43 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Thursday 05 June 2014 03:24 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:
> 
>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>> the
>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>> them
>>> channels) to the different USB controllers: channel 0 can be connected to
>>> either
>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>>> or xHCI controllers.
> 
>>> This is a new driver for this USB PHY currently already supported under
>>> drivers/
>>> usb/phy/. The reason for writing the new driver was the requirement that the
>>> multiplexing of USB channels to the controller be dynamic, depending on what
>>> USB drivers are loaded,  rather than static as provided by the old driver.
>>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>>> ideally. The new driver only supports device tree probing for now.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
>>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>> @@ -0,0 +1,60 @@
>>> +* Renesas R-Car generation 2 USB PHY
>>> +
>>> +This file provides information on what the device node for the R-Car
>>> generation
>>> +2 USB PHY contains.
>>> +
>>> +Required properties:
>>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>>> SoC.
>>> +          "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>> +- reg: offset and length of the register block.
>>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>>> +- clocks: clock phandle and specifier pair.
>>> +- clock-names: string, clock input name, must be "usbhs".
>>> +
>>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>>> +the second one is the USB controller selector and depends on the first:
>>> +
>>> ++-----------+---------------+---------------+
>>> +|\ Selector |               |               |
>>> ++ --------- +       0       |       1       |
>>> +| Channel  \|               |               |
>>> ++-----------+---------------+---------------+
>>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>>> +| 1         | PCI EHCI/OHCI | xHCI          |
>>> ++-----------+---------------+---------------+
>>> +
>>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>>> +channels and the controllers connected to them. These subnodes must contain
>>> the
>>> +following properties:
>>> +- renesas,phy-select: the first cell identifies the USB channel, the second
>>> cell
>>> +  is the USB controller selector; see the table above for the values.
>>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>>> +  USB controller selected for that channel.
>>> +
>>> +Example (Lager board):
>>> +
>>> +    usb-phy@e6590100 {
>>> +        compatible = "renesas,usb-phy-r8a7790";
>>> +        reg = <0 0xe6590100 0 0x100>;
>>> +        #phy-cells = <2>;
>>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>>> +        clock-names = "usbhs";
>>> +
>>> +        usb-phy@0,0 {
>>> +            renesas,phy-select = <0 0>;
>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>>> +        };
>>> +        usb-phy@0,1 {
>>> +            renesas,phy-select = <0 1>;
>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>>> +        };
>>> +        usb-phy@1,0 {
>>> +            renesas,phy-select = <1 0>;
>>> +            renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>>> +        };
>>> +        usb-phy@1,1 {
>>> +            renesas,phy-select = <1 1>;
>>> +            renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>>> +        };
>>> +    };
> 
>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>> xHCI. right?
> 
>    Yes. However that depends on the driver load order: if e.g. xHCI driver is
> loaded later than PCI USB drivers,
> it will override the channel routing.

So will the PCI USB drivers will be notified of that?
> 
>> So ideally only two sub-nodes should be created for channel '0' and channel
>> '1'.
> 
>    Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
> itself (corresponding to channel #0, selector #1).
> 
>> You can configure a channel to a particular mode by passing the mode in
>> PHY specifier
> 
>    I already have "#phy-cells" prop equal to 2.
> 
>> (The channel should be configured to a particualr mode in xlate).
> 
>    I have even considered using the of_xlate() method at first but then
> abandoned that idea for the phy_init() method...

If you want to configure the PHY to a particular mode, xlate should be the best
place.
> 
>> Btw I'm not sure if it is recommended to pass register mask values from dt.
> 
>    Neither am I. I did that because you requested the device tree
> representation for each of the sub-PHYs under that part of the driver which
> initialized the register masks, so that I thought that you wanted those masks
> to be represented in the device tree as well...

No I didn't mean that. Maybe for now we can have a identifier in the sub-node
and select mask and value based on that.
> 
> [...]
>>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>>> @@ -0,0 +1,288 @@
> [...]
>>> +#define USBHS_LPSTS            0x02
>>> +#define USBHS_UGCTRL            0x80
>>> +#define USBHS_UGCTRL2            0x84
>>> +#define USBHS_UGSTS            0x88
>>> +
>>> +/* Low Power Status register (LPSTS) */
>>> +#define USBHS_LPSTS_SUSPM        0x4000
>>> +
>>> +/* USB General control register (UGCTRL) */
>>> +#define USBHS_UGCTRL_CONNECT        0x00000004
>>> +#define USBHS_UGCTRL_PLLRESET        0x00000001
>>> +
>>> +/* USB General control register 2 (UGCTRL2) */
>>> +#define USBHS_UGCTRL2_USB2SEL        0x80000000
>>> +#define USBHS_UGCTRL2_USB2SEL_PCI    0x00000000
>>> +#define USBHS_UGCTRL2_USB2SEL_USB30    0x80000000
>>> +#define USBHS_UGCTRL2_USB0SEL        0x00000030
>>> +#define USBHS_UGCTRL2_USB0SEL_PCI    0x00000010
>>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB    0x00000030
>>> +
>>> +/* USB General status register (UGSTS) */
>>> +#define USBHS_UGSTS_LOCK        0x00000300
>>> +
>>> +#define    NUM_USB_CHANNELS    2
>>> +
>>> +struct rcar_gen2_phy {
>>> +    struct phy *phy;
>>> +    struct rcar_gen2_phy_driver *drv;
>>> +    u32 select_mask;
>>> +    u32 select_value;
>>> +};
>>> +
>>> +struct rcar_gen2_phy_driver {
>>> +    void __iomem *base;
>>> +    struct clk *clk;
>>> +    spinlock_t lock;
>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>> This can be created dynamically based on the number of sub-nodes. In this case
> 
>    Hm, that sounds to me like another complication of the driver with no
> apparent win...

dynamic allocation is in no way going to complicate the driver.
> 
>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>> By this we need not hard code NUM_USB_CHANNELS.
> 
>    I don't quite understand what's up with hard-coding it -- this constant is
> dictated by the UGCTRL2 register layout anyway.

right but you don't want to change the driver a whole lot when they change the
no of channels in the next version or they use a slightly modified version of
this IP in a different SoC. And finding the number of channels dynamically is
not complicated anyway IMO.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-10 10:55       ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-06-10 10:55 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Thursday 05 June 2014 03:24 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/04/2014 03:40 PM, Kishon Vijay Abraham I wrote:
> 
>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>> the
>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>> them
>>> channels) to the different USB controllers: channel 0 can be connected to
>>> either
>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>>> or xHCI controllers.
> 
>>> This is a new driver for this USB PHY currently already supported under
>>> drivers/
>>> usb/phy/. The reason for writing the new driver was the requirement that the
>>> multiplexing of USB channels to the controller be dynamic, depending on what
>>> USB drivers are loaded,  rather than static as provided by the old driver.
>>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>>> ideally. The new driver only supports device tree probing for now.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
>>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>> =================================>>> --- /dev/null
>>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>> @@ -0,0 +1,60 @@
>>> +* Renesas R-Car generation 2 USB PHY
>>> +
>>> +This file provides information on what the device node for the R-Car
>>> generation
>>> +2 USB PHY contains.
>>> +
>>> +Required properties:
>>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>>> SoC.
>>> +          "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>> +- reg: offset and length of the register block.
>>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>>> +- clocks: clock phandle and specifier pair.
>>> +- clock-names: string, clock input name, must be "usbhs".
>>> +
>>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>>> +the second one is the USB controller selector and depends on the first:
>>> +
>>> ++-----------+---------------+---------------+
>>> +|\ Selector |               |               |
>>> ++ --------- +       0       |       1       |
>>> +| Channel  \|               |               |
>>> ++-----------+---------------+---------------+
>>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>>> +| 1         | PCI EHCI/OHCI | xHCI          |
>>> ++-----------+---------------+---------------+
>>> +
>>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>>> +channels and the controllers connected to them. These subnodes must contain
>>> the
>>> +following properties:
>>> +- renesas,phy-select: the first cell identifies the USB channel, the second
>>> cell
>>> +  is the USB controller selector; see the table above for the values.
>>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>>> +  USB controller selected for that channel.
>>> +
>>> +Example (Lager board):
>>> +
>>> +    usb-phy@e6590100 {
>>> +        compatible = "renesas,usb-phy-r8a7790";
>>> +        reg = <0 0xe6590100 0 0x100>;
>>> +        #phy-cells = <2>;
>>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>>> +        clock-names = "usbhs";
>>> +
>>> +        usb-phy@0,0 {
>>> +            renesas,phy-select = <0 0>;
>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>>> +        };
>>> +        usb-phy@0,1 {
>>> +            renesas,phy-select = <0 1>;
>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>>> +        };
>>> +        usb-phy@1,0 {
>>> +            renesas,phy-select = <1 0>;
>>> +            renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>>> +        };
>>> +        usb-phy@1,1 {
>>> +            renesas,phy-select = <1 1>;
>>> +            renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>>> +        };
>>> +    };
> 
>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>> xHCI. right?
> 
>    Yes. However that depends on the driver load order: if e.g. xHCI driver is
> loaded later than PCI USB drivers,
> it will override the channel routing.

So will the PCI USB drivers will be notified of that?
> 
>> So ideally only two sub-nodes should be created for channel '0' and channel
>> '1'.
> 
>    Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
> itself (corresponding to channel #0, selector #1).
> 
>> You can configure a channel to a particular mode by passing the mode in
>> PHY specifier
> 
>    I already have "#phy-cells" prop equal to 2.
> 
>> (The channel should be configured to a particualr mode in xlate).
> 
>    I have even considered using the of_xlate() method at first but then
> abandoned that idea for the phy_init() method...

If you want to configure the PHY to a particular mode, xlate should be the best
place.
> 
>> Btw I'm not sure if it is recommended to pass register mask values from dt.
> 
>    Neither am I. I did that because you requested the device tree
> representation for each of the sub-PHYs under that part of the driver which
> initialized the register masks, so that I thought that you wanted those masks
> to be represented in the device tree as well...

No I didn't mean that. Maybe for now we can have a identifier in the sub-node
and select mask and value based on that.
> 
> [...]
>>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>>> =================================>>> --- /dev/null
>>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>>> @@ -0,0 +1,288 @@
> [...]
>>> +#define USBHS_LPSTS            0x02
>>> +#define USBHS_UGCTRL            0x80
>>> +#define USBHS_UGCTRL2            0x84
>>> +#define USBHS_UGSTS            0x88
>>> +
>>> +/* Low Power Status register (LPSTS) */
>>> +#define USBHS_LPSTS_SUSPM        0x4000
>>> +
>>> +/* USB General control register (UGCTRL) */
>>> +#define USBHS_UGCTRL_CONNECT        0x00000004
>>> +#define USBHS_UGCTRL_PLLRESET        0x00000001
>>> +
>>> +/* USB General control register 2 (UGCTRL2) */
>>> +#define USBHS_UGCTRL2_USB2SEL        0x80000000
>>> +#define USBHS_UGCTRL2_USB2SEL_PCI    0x00000000
>>> +#define USBHS_UGCTRL2_USB2SEL_USB30    0x80000000
>>> +#define USBHS_UGCTRL2_USB0SEL        0x00000030
>>> +#define USBHS_UGCTRL2_USB0SEL_PCI    0x00000010
>>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB    0x00000030
>>> +
>>> +/* USB General status register (UGSTS) */
>>> +#define USBHS_UGSTS_LOCK        0x00000300
>>> +
>>> +#define    NUM_USB_CHANNELS    2
>>> +
>>> +struct rcar_gen2_phy {
>>> +    struct phy *phy;
>>> +    struct rcar_gen2_phy_driver *drv;
>>> +    u32 select_mask;
>>> +    u32 select_value;
>>> +};
>>> +
>>> +struct rcar_gen2_phy_driver {
>>> +    void __iomem *base;
>>> +    struct clk *clk;
>>> +    spinlock_t lock;
>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>> This can be created dynamically based on the number of sub-nodes. In this case
> 
>    Hm, that sounds to me like another complication of the driver with no
> apparent win...

dynamic allocation is in no way going to complicate the driver.
> 
>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>> By this we need not hard code NUM_USB_CHANNELS.
> 
>    I don't quite understand what's up with hard-coding it -- this constant is
> dictated by the UGCTRL2 register layout anyway.

right but you don't want to change the driver a whole lot when they change the
no of channels in the next version or they use a slightly modified version of
this IP in a different SoC. And finding the number of channels dynamically is
not complicated anyway IMO.

Thanks
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-10 10:55       ` Kishon Vijay Abraham I
@ 2014-06-25 22:16         ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-25 22:16 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 06/10/2014 02:43 PM, Kishon Vijay Abraham I wrote:

    Sorry for delayed reply, I was busy with other things. Now it's time to 
revisit the infamous USB PHY driver...

>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>> the
>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>> them
>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>> either
>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>>>> or xHCI controllers.

>>>> This is a new driver for this USB PHY currently already supported under
>>>> drivers/
>>>> usb/phy/. The reason for writing the new driver was the requirement that the
>>>> multiplexing of USB channels to the controller be dynamic, depending on what
>>>> USB drivers are loaded,  rather than static as provided by the old driver.
>>>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>>>> ideally. The new driver only supports device tree probing for now.

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> [...]

>>>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>>> =================================>>>> --- /dev/null
>>>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>>> @@ -0,0 +1,60 @@
>>>> +* Renesas R-Car generation 2 USB PHY
>>>> +
>>>> +This file provides information on what the device node for the R-Car
>>>> generation
>>>> +2 USB PHY contains.
>>>> +
>>>> +Required properties:
>>>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>>>> SoC.
>>>> +          "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>>> +- reg: offset and length of the register block.
>>>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>>>> +- clocks: clock phandle and specifier pair.
>>>> +- clock-names: string, clock input name, must be "usbhs".
>>>> +
>>>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>>>> +the second one is the USB controller selector and depends on the first:
>>>> +
>>>> ++-----------+---------------+---------------+
>>>> +|\ Selector |               |               |
>>>> ++ --------- +       0       |       1       |
>>>> +| Channel  \|               |               |
>>>> ++-----------+---------------+---------------+
>>>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>>>> +| 1         | PCI EHCI/OHCI | xHCI          |
>>>> ++-----------+---------------+---------------+
>>>> +
>>>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>>>> +channels and the controllers connected to them. These subnodes must contain
>>>> the
>>>> +following properties:
>>>> +- renesas,phy-select: the first cell identifies the USB channel, the second
>>>> cell
>>>> +  is the USB controller selector; see the table above for the values.
>>>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>>>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>>>> +  USB controller selected for that channel.
>>>> +
>>>> +Example (Lager board):
>>>> +
>>>> +    usb-phy@e6590100 {
>>>> +        compatible = "renesas,usb-phy-r8a7790";
>>>> +        reg = <0 0xe6590100 0 0x100>;
>>>> +        #phy-cells = <2>;
>>>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>>>> +        clock-names = "usbhs";
>>>> +
>>>> +        usb-phy@0,0 {
>>>> +            renesas,phy-select = <0 0>;
>>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>>>> +        };
>>>> +        usb-phy@0,1 {
>>>> +            renesas,phy-select = <0 1>;
>>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>>>> +        };
>>>> +        usb-phy@1,0 {
>>>> +            renesas,phy-select = <1 0>;
>>>> +            renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>>>> +        };
>>>> +        usb-phy@1,1 {
>>>> +            renesas,phy-select = <1 1>;
>>>> +            renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>>>> +        };
>>>> +    };

>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>> xHCI. right?

>>     Yes. However that depends on the driver load order: if e.g. xHCI driver is
>> loaded later than PCI USB drivers,
>> it will override the channel routing.

> So will the PCI USB drivers will be notified of that?

    Unfortunately, no. But this is also the case with the other multi-PHY 
drivers...

>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>> '1'.

>>     Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>> itself (corresponding to channel #0, selector #1).

>>> You can configure a channel to a particular mode by passing the mode in
>>> PHY specifier

>>     I already have "#phy-cells" prop equal to 2.

>>> (The channel should be configured to a particualr mode in xlate).

>>     I have even considered using the of_xlate() method at first but then
>> abandoned that idea for the phy_init() method...

> If you want to configure the PHY to a particular mode, xlate should be the best
> place.

    I tried to move the code there from the init() method but then I realized 
that I need to prepare/enable the USBHS clock before writing to the UGCTRL2 
register and there's no place I can disable/unprepare this clock if I do the 
channel routing in the xlate() method. So no, I don't agree here.

>>> Btw I'm not sure if it is recommended to pass register mask values from dt.

>>     Neither am I. I did that because you requested the device tree
>> representation for each of the sub-PHYs under that part of the driver which
>> initialized the register masks, so that I thought that you wanted those masks
>> to be represented in the device tree as well...

> No I didn't mean that. Maybe for now we can have a identifier in the sub-node
> and select mask and value based on that.

    Sigh. Yes, having some sort of selecting property in the subnodes is the 
only way...

>> [...]
>>>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>>>> =================================>>>> --- /dev/null
>>>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>>>> @@ -0,0 +1,288 @@
>> [...]
>>>> +#define USBHS_LPSTS            0x02
>>>> +#define USBHS_UGCTRL            0x80
>>>> +#define USBHS_UGCTRL2            0x84
>>>> +#define USBHS_UGSTS            0x88
>>>> +
>>>> +/* Low Power Status register (LPSTS) */
>>>> +#define USBHS_LPSTS_SUSPM        0x4000
>>>> +
>>>> +/* USB General control register (UGCTRL) */
>>>> +#define USBHS_UGCTRL_CONNECT        0x00000004
>>>> +#define USBHS_UGCTRL_PLLRESET        0x00000001
>>>> +
>>>> +/* USB General control register 2 (UGCTRL2) */
>>>> +#define USBHS_UGCTRL2_USB2SEL        0x80000000
>>>> +#define USBHS_UGCTRL2_USB2SEL_PCI    0x00000000
>>>> +#define USBHS_UGCTRL2_USB2SEL_USB30    0x80000000
>>>> +#define USBHS_UGCTRL2_USB0SEL        0x00000030
>>>> +#define USBHS_UGCTRL2_USB0SEL_PCI    0x00000010
>>>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB    0x00000030
>>>> +
>>>> +/* USB General status register (UGSTS) */
>>>> +#define USBHS_UGSTS_LOCK        0x00000300
>>>> +
>>>> +#define    NUM_USB_CHANNELS    2
>>>> +
>>>> +struct rcar_gen2_phy {
>>>> +    struct phy *phy;
>>>> +    struct rcar_gen2_phy_driver *drv;
>>>> +    u32 select_mask;
>>>> +    u32 select_value;
>>>> +};
>>>> +
>>>> +struct rcar_gen2_phy_driver {
>>>> +    void __iomem *base;
>>>> +    struct clk *clk;
>>>> +    spinlock_t lock;
>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>> This can be created dynamically based on the number of sub-nodes. In this case

    Did you mean that I'll need to use linked list here instead of an array?

>>     Hm, that sounds to me like another complication of the driver with no
>> apparent win...

> dynamic allocation is in no way going to complicate the driver.

    Since when the dynamic data allocation is as simple as the statically 
allocated data? :-O

>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>> By this we need not hard code NUM_USB_CHANNELS.

>>     I don't quite understand what's up with hard-coding it -- this constant is
>> dictated by the UGCTRL2 register layout anyway.

> right but you don't want to change the driver a whole lot when they change the
> no of channels in the next version

    They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
However, the number of controllable channels didn't change.

> or they use a slightly modified version of
> this IP in a different SoC. And finding the number of channels dynamically is
> not complicated anyway IMO.

    Sorry, but what you're saying here just doesn't make sense to me. I'd need 
to modify the driver for the different number of the controllable channels in 
any case since the UGCTRL2 masks/values have to be hard coded in the driver as 
you said. If they were read from the device tree, that would have made sense 
but you seem to be against that...

> Thanks
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-06-25 22:16         ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-06-25 22:16 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 06/10/2014 02:43 PM, Kishon Vijay Abraham I wrote:

    Sorry for delayed reply, I was busy with other things. Now it's time to 
revisit the infamous USB PHY driver...

>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>> the
>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>> them
>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>> either
>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>>>> or xHCI controllers.

>>>> This is a new driver for this USB PHY currently already supported under
>>>> drivers/
>>>> usb/phy/. The reason for writing the new driver was the requirement that the
>>>> multiplexing of USB channels to the controller be dynamic, depending on what
>>>> USB drivers are loaded,  rather than static as provided by the old driver.
>>>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>>>> ideally. The new driver only supports device tree probing for now.

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> [...]

>>>> Index: linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ linux-phy/Documentation/devicetree/bindings/phy/rcar-gen2-phy.txt
>>>> @@ -0,0 +1,60 @@
>>>> +* Renesas R-Car generation 2 USB PHY
>>>> +
>>>> +This file provides information on what the device node for the R-Car
>>>> generation
>>>> +2 USB PHY contains.
>>>> +
>>>> +Required properties:
>>>> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790
>>>> SoC.
>>>> +          "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC.
>>>> +- reg: offset and length of the register block.
>>>> +- #phy-cells: see phy-bindings.txt in the same directory, must be 2.
>>>> +- clocks: clock phandle and specifier pair.
>>>> +- clock-names: string, clock input name, must be "usbhs".
>>>> +
>>>> +The phandle's first argument in the PHY specifier identifies the USB channel,
>>>> +the second one is the USB controller selector and depends on the first:
>>>> +
>>>> ++-----------+---------------+---------------+
>>>> +|\ Selector |               |               |
>>>> ++ --------- +       0       |       1       |
>>>> +| Channel  \|               |               |
>>>> ++-----------+---------------+---------------+
>>>> +| 0         | PCI EHCI/OHCI | HS-USB        |
>>>> +| 1         | PCI EHCI/OHCI | xHCI          |
>>>> ++-----------+---------------+---------------+
>>>> +
>>>> +The USB PHY device tree node should be the subnodes corresponding to the USB
>>>> +channels and the controllers connected to them. These subnodes must contain
>>>> the
>>>> +following properties:
>>>> +- renesas,phy-select: the first cell identifies the USB channel, the second
>>>> cell
>>>> +  is the USB controller selector; see the table above for the values.
>>>> +- renesas,ugctrl2-masks: the first cell is the UGCTRL2 mask corresponding to
>>>> +  the USB channel, the second cell is the UGCTRL2 value corresponding to the
>>>> +  USB controller selected for that channel.
>>>> +
>>>> +Example (Lager board):
>>>> +
>>>> +    usb-phy@e6590100 {
>>>> +        compatible = "renesas,usb-phy-r8a7790";
>>>> +        reg = <0 0xe6590100 0 0x100>;
>>>> +        #phy-cells = <2>;
>>>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>>>> +        clock-names = "usbhs";
>>>> +
>>>> +        usb-phy@0,0 {
>>>> +            renesas,phy-select = <0 0>;
>>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000010>;
>>>> +        };
>>>> +        usb-phy@0,1 {
>>>> +            renesas,phy-select = <0 1>;
>>>> +            renesas,ugctrl2-masks = <0x00000030 0x00000030>;
>>>> +        };
>>>> +        usb-phy@1,0 {
>>>> +            renesas,phy-select = <1 0>;
>>>> +            renesas,ugctrl2-masks = <0x80000000 0x00000000>;
>>>> +        };
>>>> +        usb-phy@1,1 {
>>>> +            renesas,phy-select = <1 1>;
>>>> +            renesas,ugctrl2-masks = <0x80000000 0x80000000>;
>>>> +        };
>>>> +    };

>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>> xHCI. right?

>>     Yes. However that depends on the driver load order: if e.g. xHCI driver is
>> loaded later than PCI USB drivers,
>> it will override the channel routing.

> So will the PCI USB drivers will be notified of that?

    Unfortunately, no. But this is also the case with the other multi-PHY 
drivers...

>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>> '1'.

>>     Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>> itself (corresponding to channel #0, selector #1).

>>> You can configure a channel to a particular mode by passing the mode in
>>> PHY specifier

>>     I already have "#phy-cells" prop equal to 2.

>>> (The channel should be configured to a particualr mode in xlate).

>>     I have even considered using the of_xlate() method at first but then
>> abandoned that idea for the phy_init() method...

> If you want to configure the PHY to a particular mode, xlate should be the best
> place.

    I tried to move the code there from the init() method but then I realized 
that I need to prepare/enable the USBHS clock before writing to the UGCTRL2 
register and there's no place I can disable/unprepare this clock if I do the 
channel routing in the xlate() method. So no, I don't agree here.

>>> Btw I'm not sure if it is recommended to pass register mask values from dt.

>>     Neither am I. I did that because you requested the device tree
>> representation for each of the sub-PHYs under that part of the driver which
>> initialized the register masks, so that I thought that you wanted those masks
>> to be represented in the device tree as well...

> No I didn't mean that. Maybe for now we can have a identifier in the sub-node
> and select mask and value based on that.

    Sigh. Yes, having some sort of selecting property in the subnodes is the 
only way...

>> [...]
>>>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>>>> @@ -0,0 +1,288 @@
>> [...]
>>>> +#define USBHS_LPSTS            0x02
>>>> +#define USBHS_UGCTRL            0x80
>>>> +#define USBHS_UGCTRL2            0x84
>>>> +#define USBHS_UGSTS            0x88
>>>> +
>>>> +/* Low Power Status register (LPSTS) */
>>>> +#define USBHS_LPSTS_SUSPM        0x4000
>>>> +
>>>> +/* USB General control register (UGCTRL) */
>>>> +#define USBHS_UGCTRL_CONNECT        0x00000004
>>>> +#define USBHS_UGCTRL_PLLRESET        0x00000001
>>>> +
>>>> +/* USB General control register 2 (UGCTRL2) */
>>>> +#define USBHS_UGCTRL2_USB2SEL        0x80000000
>>>> +#define USBHS_UGCTRL2_USB2SEL_PCI    0x00000000
>>>> +#define USBHS_UGCTRL2_USB2SEL_USB30    0x80000000
>>>> +#define USBHS_UGCTRL2_USB0SEL        0x00000030
>>>> +#define USBHS_UGCTRL2_USB0SEL_PCI    0x00000010
>>>> +#define USBHS_UGCTRL2_USB0SEL_HS_USB    0x00000030
>>>> +
>>>> +/* USB General status register (UGSTS) */
>>>> +#define USBHS_UGSTS_LOCK        0x00000300
>>>> +
>>>> +#define    NUM_USB_CHANNELS    2
>>>> +
>>>> +struct rcar_gen2_phy {
>>>> +    struct phy *phy;
>>>> +    struct rcar_gen2_phy_driver *drv;
>>>> +    u32 select_mask;
>>>> +    u32 select_value;
>>>> +};
>>>> +
>>>> +struct rcar_gen2_phy_driver {
>>>> +    void __iomem *base;
>>>> +    struct clk *clk;
>>>> +    spinlock_t lock;
>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>> This can be created dynamically based on the number of sub-nodes. In this case

    Did you mean that I'll need to use linked list here instead of an array?

>>     Hm, that sounds to me like another complication of the driver with no
>> apparent win...

> dynamic allocation is in no way going to complicate the driver.

    Since when the dynamic data allocation is as simple as the statically 
allocated data? :-O

>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>> By this we need not hard code NUM_USB_CHANNELS.

>>     I don't quite understand what's up with hard-coding it -- this constant is
>> dictated by the UGCTRL2 register layout anyway.

> right but you don't want to change the driver a whole lot when they change the
> no of channels in the next version

    They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
However, the number of controllable channels didn't change.

> or they use a slightly modified version of
> this IP in a different SoC. And finding the number of channels dynamically is
> not complicated anyway IMO.

    Sorry, but what you're saying here just doesn't make sense to me. I'd need 
to modify the driver for the different number of the controllable channels in 
any case since the UGCTRL2 masks/values have to be hard coded in the driver as 
you said. If they were read from the device tree, that would have made sense 
but you seem to be against that...

> Thanks
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-06-25 22:16         ` Sergei Shtylyov
@ 2014-07-01 13:23           ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-07-01 13:11 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Thursday 26 June 2014 03:46 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/10/2014 02:43 PM, Kishon Vijay Abraham I wrote:
> 
>    Sorry for delayed reply, I was busy with other things. Now it's time to
> revisit the infamous USB PHY driver...
> 
>>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>>> the
>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>> them
>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>> either
>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>> EHCI/OHCI
>>>>> or xHCI controllers.
> 
.
.
<snip>
.
.

> 
>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>> xHCI. right?
> 
>>>     Yes. However that depends on the driver load order: if e.g. xHCI driver is
>>> loaded later than PCI USB drivers,
>>> it will override the channel routing.
> 
>> So will the PCI USB drivers will be notified of that?
> 
>    Unfortunately, no. But this is also the case with the other multi-PHY
> drivers...

IIRC, in the case of other existing multi-phy drivers, all the PHYs can
co-exist without actually overriding anything that was configured previously.

Anyway it is a good point and it will indeed create problems in cases where the
PHY is mutually exclusive like in your case where a channel can be configured
for either EHCI or USB but not both. The other driver where this will be a
problem is also floating in the list [1].

[1] -> https://lkml.org/lkml/2014/6/30/324
> 
>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>> '1'.
> 
>>>     Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>>> itself (corresponding to channel #0, selector #1).
> 
>>>> You can configure a channel to a particular mode by passing the mode in
>>>> PHY specifier
> 
>>>     I already have "#phy-cells" prop equal to 2.
> 
>>>> (The channel should be configured to a particualr mode in xlate).
> 
>>>     I have even considered using the of_xlate() method at first but then
>>> abandoned that idea for the phy_init() method...
> 
>> If you want to configure the PHY to a particular mode, xlate should be the best
>> place.
> 
>    I tried to move the code there from the init() method but then I realized
> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
> register and there's no place I can disable/unprepare this clock if I do the
> channel routing in the xlate() method. So no, I don't agree here.

enabling clock from init() seems correct to me. We need a better way to avoid
overriding the PHY to a particular mode.
> 
>>>> Btw I'm not sure if it is recommended to pass register mask values from dt.
> 
>>>     Neither am I. I did that because you requested the device tree
>>> representation for each of the sub-PHYs under that part of the driver which
>>> initialized the register masks, so that I thought that you wanted those masks
>>> to be represented in the device tree as well...
> 
>> No I didn't mean that. Maybe for now we can have a identifier in the sub-node
>> and select mask and value based on that.
> 
>    Sigh. Yes, having some sort of selecting property in the subnodes is the
> only way...
> 
>>> [...]
.
.
<snip>
.
.

>>>>> +struct rcar_gen2_phy_driver {
>>>>> +    void __iomem *base;
>>>>> +    struct clk *clk;
>>>>> +    spinlock_t lock;
>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>>>> This can be created dynamically based on the number of sub-nodes. In this case
> 
>    Did you mean that I'll need to use linked list here instead of an array?

Nope. I meant something like below.

struct rcar_gen2_phy_driver {
	.
	.
	struct rcar_gen2_phy **phys;
}

probe()
{
	<snip>
	int i = 0, channel_count;
	struct rcar_gen2_phy **phys;
	channel_count = of_get_child_count(np);
	phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);
	for_each_child_of_node(dev->of_node, np) {
		struct rcar_gen2_phy *phy;
		.
		.
		phy = kzalloc(sizeof(*phy), GFP_KERNEL);
		.
		.
		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
		phys[i++] = phy;
	}
	drv->phys = phys;
	<snip>
}

Then you can access 'phys' just like how you access an array.
> 
>>>     Hm, that sounds to me like another complication of the driver with no
>>> apparent win...
> 
>> dynamic allocation is in no way going to complicate the driver.
> 
>    Since when the dynamic data allocation is as simple as the statically
> allocated data? :-O
> 
>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>> By this we need not hard code NUM_USB_CHANNELS.
> 
>>>     I don't quite understand what's up with hard-coding it -- this constant is
>>> dictated by the UGCTRL2 register layout anyway.
> 
>> right but you don't want to change the driver a whole lot when they change the
>> no of channels in the next version
> 
>    They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
> However, the number of controllable channels didn't change.

right.. that's where I'd like to have status = "disabled" for that channel in
your dt node.
> 
>> or they use a slightly modified version of
>> this IP in a different SoC. And finding the number of channels dynamically is
>> not complicated anyway IMO.
> 
>    Sorry, but what you're saying here just doesn't make sense to me. I'd need
> to modify the driver for the different number of the controllable channels in
> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
> you said. If they were read from the device tree, that would have made sense
> but you seem to be against that...

R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
NUM_CHANNELS in this driver? Modifying the driver _can_ be adding macros for
registers, bit masks etc.. and maybe appropriately modifying of_device data.

Cheers
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-07-01 13:23           ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-07-01 13:23 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Thursday 26 June 2014 03:46 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/10/2014 02:43 PM, Kishon Vijay Abraham I wrote:
> 
>    Sorry for delayed reply, I was busy with other things. Now it's time to
> revisit the infamous USB PHY driver...
> 
>>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>>> the
>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>> them
>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>> either
>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>> EHCI/OHCI
>>>>> or xHCI controllers.
> 
.
.
<snip>
.
.

> 
>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>> xHCI. right?
> 
>>>     Yes. However that depends on the driver load order: if e.g. xHCI driver is
>>> loaded later than PCI USB drivers,
>>> it will override the channel routing.
> 
>> So will the PCI USB drivers will be notified of that?
> 
>    Unfortunately, no. But this is also the case with the other multi-PHY
> drivers...

IIRC, in the case of other existing multi-phy drivers, all the PHYs can
co-exist without actually overriding anything that was configured previously.

Anyway it is a good point and it will indeed create problems in cases where the
PHY is mutually exclusive like in your case where a channel can be configured
for either EHCI or USB but not both. The other driver where this will be a
problem is also floating in the list [1].

[1] -> https://lkml.org/lkml/2014/6/30/324
> 
>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>> '1'.
> 
>>>     Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>>> itself (corresponding to channel #0, selector #1).
> 
>>>> You can configure a channel to a particular mode by passing the mode in
>>>> PHY specifier
> 
>>>     I already have "#phy-cells" prop equal to 2.
> 
>>>> (The channel should be configured to a particualr mode in xlate).
> 
>>>     I have even considered using the of_xlate() method at first but then
>>> abandoned that idea for the phy_init() method...
> 
>> If you want to configure the PHY to a particular mode, xlate should be the best
>> place.
> 
>    I tried to move the code there from the init() method but then I realized
> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
> register and there's no place I can disable/unprepare this clock if I do the
> channel routing in the xlate() method. So no, I don't agree here.

enabling clock from init() seems correct to me. We need a better way to avoid
overriding the PHY to a particular mode.
> 
>>>> Btw I'm not sure if it is recommended to pass register mask values from dt.
> 
>>>     Neither am I. I did that because you requested the device tree
>>> representation for each of the sub-PHYs under that part of the driver which
>>> initialized the register masks, so that I thought that you wanted those masks
>>> to be represented in the device tree as well...
> 
>> No I didn't mean that. Maybe for now we can have a identifier in the sub-node
>> and select mask and value based on that.
> 
>    Sigh. Yes, having some sort of selecting property in the subnodes is the
> only way...
> 
>>> [...]
.
.
<snip>
.
.

>>>>> +struct rcar_gen2_phy_driver {
>>>>> +    void __iomem *base;
>>>>> +    struct clk *clk;
>>>>> +    spinlock_t lock;
>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>>>> This can be created dynamically based on the number of sub-nodes. In this case
> 
>    Did you mean that I'll need to use linked list here instead of an array?

Nope. I meant something like below.

struct rcar_gen2_phy_driver {
	.
	.
	struct rcar_gen2_phy **phys;
}

probe()
{
	<snip>
	int i = 0, channel_count;
	struct rcar_gen2_phy **phys;
	channel_count = of_get_child_count(np);
	phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);
	for_each_child_of_node(dev->of_node, np) {
		struct rcar_gen2_phy *phy;
		.
		.
		phy = kzalloc(sizeof(*phy), GFP_KERNEL);
		.
		.
		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
		phys[i++] = phy;
	}
	drv->phys = phys;
	<snip>
}

Then you can access 'phys' just like how you access an array.
> 
>>>     Hm, that sounds to me like another complication of the driver with no
>>> apparent win...
> 
>> dynamic allocation is in no way going to complicate the driver.
> 
>    Since when the dynamic data allocation is as simple as the statically
> allocated data? :-O
> 
>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>> By this we need not hard code NUM_USB_CHANNELS.
> 
>>>     I don't quite understand what's up with hard-coding it -- this constant is
>>> dictated by the UGCTRL2 register layout anyway.
> 
>> right but you don't want to change the driver a whole lot when they change the
>> no of channels in the next version
> 
>    They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
> However, the number of controllable channels didn't change.

right.. that's where I'd like to have status = "disabled" for that channel in
your dt node.
> 
>> or they use a slightly modified version of
>> this IP in a different SoC. And finding the number of channels dynamically is
>> not complicated anyway IMO.
> 
>    Sorry, but what you're saying here just doesn't make sense to me. I'd need
> to modify the driver for the different number of the controllable channels in
> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
> you said. If they were read from the device tree, that would have made sense
> but you seem to be against that...

R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
NUM_CHANNELS in this driver? Modifying the driver _can_ be adding macros for
registers, bit masks etc.. and maybe appropriately modifying of_device data.

Cheers
Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-07-01 13:23           ` Kishon Vijay Abraham I
@ 2014-07-04 20:53             ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-07-04 20:53 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 07/01/2014 05:11 PM, Kishon Vijay Abraham I wrote:

>>>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>>>> the
>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>> them
>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>> either
>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>> EHCI/OHCI
>>>>>> or xHCI controllers.

> .
> .
> <snip>
> .
> .

>>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>>> xHCI. right?

>>>>      Yes. However that depends on the driver load order: if e.g. xHCI driver is
>>>> loaded later than PCI USB drivers,
>>>> it will override the channel routing.

>>> So will the PCI USB drivers will be notified of that?

>>     Unfortunately, no. But this is also the case with the other multi-PHY
>> drivers...

> IIRC, in the case of other existing multi-phy drivers, all the PHYs can
> co-exist without actually overriding anything that was configured previously.

    'phy-exynos-mipi-video' driver looked somewhat suspicious in this respect 
(I didn't understand why they used "#phy-cells" of 1, having 2 channels with 
two PHYs each) but upon further scrutiny it appears that the PHYs on one 
channel function quite independently...

[...]

>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>> '1'.

>>>>      Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>>>> itself (corresponding to channel #0, selector #1).

>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>> PHY specifier

>>>>      I already have "#phy-cells" prop equal to 2.

>>>>> (The channel should be configured to a particualr mode in xlate).

>>>>      I have even considered using the of_xlate() method at first but then
>>>> abandoned that idea for the phy_init() method...

>>> If you want to configure the PHY to a particular mode, xlate should be the best
>>> place.

>>     I tried to move the code there from the init() method but then I realized
>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>> register and there's no place I can disable/unprepare this clock if I do the

    Unless I prepare/enable the clock when probing, and undo it on removal, 
that is.

>> channel routing in the xlate() method. So no, I don't agree here.

> enabling clock from init() seems correct to me. We need a better way to avoid
> overriding the PHY to a particular mode.

    In fact, in my case such override may be rather desirable.

[...]
> .
> .
> <snip>
> .
> .
>
>>>>>> +struct rcar_gen2_phy_driver {
>>>>>> +    void __iomem *base;
>>>>>> +    struct clk *clk;
>>>>>> +    spinlock_t lock;
>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>>>> This can be created dynamically based on the number of sub-nodes. In this case

>>     Did you mean that I'll need to use linked list here instead of an array?

> Nope. I meant something like below.

> struct rcar_gen2_phy_driver {
> 	.
> 	.
> 	struct rcar_gen2_phy **phys;
> }
>
> probe()
> {
> 	<snip>
> 	int i = 0, channel_count;
> 	struct rcar_gen2_phy **phys;
> 	channel_count = of_get_child_count(np);

    Didn't know of such function...

> 	phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);

    Rather kcalloc().

> 	for_each_child_of_node(dev->of_node, np) {
> 		struct rcar_gen2_phy *phy;
> 		.
> 		.
> 		phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> 		.
> 		.
> 		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
> 		phys[i++] = phy;
> 	}
> 	drv->phys = phys;
> 	<snip>
> }

> Then you can access 'phys' just like how you access an array.

    Aren't you over-engineering things? I'd rather have just an array of 
'struct rcar_gen2_phy' dynamically allocated at once, instead of an array of 
pointers to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

    Anyway, this means that I'll have to do linear search for the needed PHY 
in the xlate() method, just like it would have been with a linked list. 
Complication. :-)

[...]

>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>> By this we need not hard code NUM_USB_CHANNELS.

>>>>      I don't quite understand what's up with hard-coding it -- this constant is
>>>> dictated by the UGCTRL2 register layout anyway.

>>> right but you don't want to change the driver a whole lot when they change the
>>> no of channels in the next version

>>     They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>> However, the number of controllable channels didn't change.

> right.. that's where I'd like to have status = "disabled" for that channel in
> your dt node.

    I disagree here. First, channel #1 is not controllable anyway, so of no 
interest to us. Anyway, if more controllable channel appear, may point is that 
should be a matter of introducing and properly handling a new "compatible" 
property, not just adding/removing subnodes.

>>> or they use a slightly modified version of
>>> this IP in a different SoC. And finding the number of channels dynamically is
>>> not complicated anyway IMO.

>>     Sorry, but what you're saying here just doesn't make sense to me. I'd need
>> to modify the driver for the different number of the controllable channels in
>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>> you said. If they were read from the device tree, that would have made sense
>> but you seem to be against that...

> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
> NUM_CHANNELS in this driver?

    Two; we have only two controllable channels in any case.

> Modifying the driver _can_ be adding macros for
> registers, bit masks etc.. and maybe appropriately modifying of_device data.

    s/_can_/must/.

> Cheers
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-07-04 20:53             ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-07-04 20:53 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 07/01/2014 05:11 PM, Kishon Vijay Abraham I wrote:

>>>>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>>>>> the
>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>> them
>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>> either
>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>> EHCI/OHCI
>>>>>> or xHCI controllers.

> .
> .
> <snip>
> .
> .

>>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but can't be
>>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>>> xHCI. right?

>>>>      Yes. However that depends on the driver load order: if e.g. xHCI driver is
>>>> loaded later than PCI USB drivers,
>>>> it will override the channel routing.

>>> So will the PCI USB drivers will be notified of that?

>>     Unfortunately, no. But this is also the case with the other multi-PHY
>> drivers...

> IIRC, in the case of other existing multi-phy drivers, all the PHYs can
> co-exist without actually overriding anything that was configured previously.

    'phy-exynos-mipi-video' driver looked somewhat suspicious in this respect 
(I didn't understand why they used "#phy-cells" of 1, having 2 channels with 
two PHYs each) but upon further scrutiny it appears that the PHYs on one 
channel function quite independently...

[...]

>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>> '1'.

>>>>      Hm, but I need to perform a special PHY power up sequence for the USBHS PHY
>>>> itself (corresponding to channel #0, selector #1).

>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>> PHY specifier

>>>>      I already have "#phy-cells" prop equal to 2.

>>>>> (The channel should be configured to a particualr mode in xlate).

>>>>      I have even considered using the of_xlate() method at first but then
>>>> abandoned that idea for the phy_init() method...

>>> If you want to configure the PHY to a particular mode, xlate should be the best
>>> place.

>>     I tried to move the code there from the init() method but then I realized
>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>> register and there's no place I can disable/unprepare this clock if I do the

    Unless I prepare/enable the clock when probing, and undo it on removal, 
that is.

>> channel routing in the xlate() method. So no, I don't agree here.

> enabling clock from init() seems correct to me. We need a better way to avoid
> overriding the PHY to a particular mode.

    In fact, in my case such override may be rather desirable.

[...]
> .
> .
> <snip>
> .
> .
>
>>>>>> +struct rcar_gen2_phy_driver {
>>>>>> +    void __iomem *base;
>>>>>> +    struct clk *clk;
>>>>>> +    spinlock_t lock;
>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>>>> This can be created dynamically based on the number of sub-nodes. In this case

>>     Did you mean that I'll need to use linked list here instead of an array?

> Nope. I meant something like below.

> struct rcar_gen2_phy_driver {
> 	.
> 	.
> 	struct rcar_gen2_phy **phys;
> }
>
> probe()
> {
> 	<snip>
> 	int i = 0, channel_count;
> 	struct rcar_gen2_phy **phys;
> 	channel_count = of_get_child_count(np);

    Didn't know of such function...

> 	phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);

    Rather kcalloc().

> 	for_each_child_of_node(dev->of_node, np) {
> 		struct rcar_gen2_phy *phy;
> 		.
> 		.
> 		phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> 		.
> 		.
> 		phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
> 		phys[i++] = phy;
> 	}
> 	drv->phys = phys;
> 	<snip>
> }

> Then you can access 'phys' just like how you access an array.

    Aren't you over-engineering things? I'd rather have just an array of 
'struct rcar_gen2_phy' dynamically allocated at once, instead of an array of 
pointers to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

    Anyway, this means that I'll have to do linear search for the needed PHY 
in the xlate() method, just like it would have been with a linked list. 
Complication. :-)

[...]

>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>> By this we need not hard code NUM_USB_CHANNELS.

>>>>      I don't quite understand what's up with hard-coding it -- this constant is
>>>> dictated by the UGCTRL2 register layout anyway.

>>> right but you don't want to change the driver a whole lot when they change the
>>> no of channels in the next version

>>     They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>> However, the number of controllable channels didn't change.

> right.. that's where I'd like to have status = "disabled" for that channel in
> your dt node.

    I disagree here. First, channel #1 is not controllable anyway, so of no 
interest to us. Anyway, if more controllable channel appear, may point is that 
should be a matter of introducing and properly handling a new "compatible" 
property, not just adding/removing subnodes.

>>> or they use a slightly modified version of
>>> this IP in a different SoC. And finding the number of channels dynamically is
>>> not complicated anyway IMO.

>>     Sorry, but what you're saying here just doesn't make sense to me. I'd need
>> to modify the driver for the different number of the controllable channels in
>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>> you said. If they were read from the device tree, that would have made sense
>> but you seem to be against that...

> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
> NUM_CHANNELS in this driver?

    Two; we have only two controllable channels in any case.

> Modifying the driver _can_ be adding macros for
> registers, bit masks etc.. and maybe appropriately modifying of_device data.

    s/_can_/must/.

> Cheers
> Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-07-04 20:53             ` Sergei Shtylyov
@ 2014-07-08 12:32               ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-07-08 12:20 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Saturday 05 July 2014 02:23 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/01/2014 05:11 PM, Kishon Vijay Abraham I wrote:
> 
>>>>>>> This PHY, though formally being a part of Renesas USBHS controller,
>>>>>>> contains
>>>>>>> the
>>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>>> them
>>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>>> either
>>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>>> EHCI/OHCI
>>>>>>> or xHCI controllers.
> 
>> .
>> .
>> <snip>
>> .
>> .
> 
>>>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but
>>>>>> can't be
>>>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>>>> xHCI. right?
> 
>>>>>      Yes. However that depends on the driver load order: if e.g. xHCI
>>>>> driver is
>>>>> loaded later than PCI USB drivers,
>>>>> it will override the channel routing.
> 
>>>> So will the PCI USB drivers will be notified of that?
> 
>>>     Unfortunately, no. But this is also the case with the other multi-PHY
>>> drivers...
> 
>> IIRC, in the case of other existing multi-phy drivers, all the PHYs can
>> co-exist without actually overriding anything that was configured previously.
> 
>    'phy-exynos-mipi-video' driver looked somewhat suspicious in this respect (I
> didn't understand why they used "#phy-cells" of 1, having 2 channels with two
> PHYs each) but upon further scrutiny it appears that the PHYs on one channel
> function quite independently...
> 
> [...]
> 
>>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>>> '1'.
> 
>>>>>      Hm, but I need to perform a special PHY power up sequence for the
>>>>> USBHS PHY
>>>>> itself (corresponding to channel #0, selector #1).
> 
>>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>>> PHY specifier
> 
>>>>>      I already have "#phy-cells" prop equal to 2.
> 
>>>>>> (The channel should be configured to a particualr mode in xlate).
> 
>>>>>      I have even considered using the of_xlate() method at first but then
>>>>> abandoned that idea for the phy_init() method...
> 
>>>> If you want to configure the PHY to a particular mode, xlate should be the
>>>> best
>>>> place.
> 
>>>     I tried to move the code there from the init() method but then I realized
>>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>>> register and there's no place I can disable/unprepare this clock if I do the
> 
>    Unless I prepare/enable the clock when probing, and undo it on removal, that
> is.
> 
>>> channel routing in the xlate() method. So no, I don't agree here.
> 
>> enabling clock from init() seems correct to me. We need a better way to avoid
>> overriding the PHY to a particular mode.
> 
>    In fact, in my case such override may be rather desirable.

Don't understand how overriding is desirable. Won't it affect the first
controller that got the PHY?
> 
> [...]
>> .
>> .
>> <snip>
>> .
>> .
>>
>>>>>>> +struct rcar_gen2_phy_driver {
>>>>>>> +    void __iomem *base;
>>>>>>> +    struct clk *clk;
>>>>>>> +    spinlock_t lock;
>>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>>>>>> This can be created dynamically based on the number of sub-nodes. In this
>>>>>> case
> 
>>>     Did you mean that I'll need to use linked list here instead of an array?
> 
>> Nope. I meant something like below.
> 
>> struct rcar_gen2_phy_driver {
>>     .
>>     .
>>     struct rcar_gen2_phy **phys;
>> }
>>
>> probe()
>> {
>>     <snip>
>>     int i = 0, channel_count;
>>     struct rcar_gen2_phy **phys;
>>     channel_count = of_get_child_count(np);
> 
>    Didn't know of such function...
> 
>>     phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);
> 
>    Rather kcalloc().
> 
>>     for_each_child_of_node(dev->of_node, np) {
>>         struct rcar_gen2_phy *phy;
>>         .
>>         .
>>         phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>         .
>>         .
>>         phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
>>         phys[i++] = phy;
>>     }
>>     drv->phys = phys;
>>     <snip>
>> }
> 
>> Then you can access 'phys' just like how you access an array.
> 
>    Aren't you over-engineering things? I'd rather have just an array of 'struct
> rcar_gen2_phy' dynamically allocated at once, instead of an array of pointers
> to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

yeah.. that can be done.
> 
>    Anyway, this means that I'll have to do linear search for the needed PHY in
> the xlate() method, just like it would have been with a linked list.

indeed. Unless we directly pass the index in the phy specifier (from dt). But I
would prefer linear search instead.
> Complication. :-)
> 
> [...]
> 
>>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>>> By this we need not hard code NUM_USB_CHANNELS.
> 
>>>>>      I don't quite understand what's up with hard-coding it -- this
>>>>> constant is
>>>>> dictated by the UGCTRL2 register layout anyway.
> 
>>>> right but you don't want to change the driver a whole lot when they change the
>>>> no of channels in the next version
> 
>>>     They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>>> However, the number of controllable channels didn't change.
> 
>> right.. that's where I'd like to have status = "disabled" for that channel in
>> your dt node.
> 
>    I disagree here. First, channel #1 is not controllable anyway, so of no
> interest to us. Anyway, if more controllable channel appear, may point is that
> should be a matter of introducing and properly handling a new "compatible"
> property, not just adding/removing subnodes.

That will lead to broken dt data. I think we have to do both.
> 
>>>> or they use a slightly modified version of
>>>> this IP in a different SoC. And finding the number of channels dynamically is
>>>> not complicated anyway IMO.
> 
>>>     Sorry, but what you're saying here just doesn't make sense to me. I'd need
>>> to modify the driver for the different number of the controllable channels in
>>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>>> you said. If they were read from the device tree, that would have made sense
>>> but you seem to be against that...
> 
>> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
>> NUM_CHANNELS in this driver?
> 
>    Two; we have only two controllable channels in any case.

NAK.

-Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-07-08 12:32               ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 38+ messages in thread
From: Kishon Vijay Abraham I @ 2014-07-08 12:32 UTC (permalink / raw)
  To: Sergei Shtylyov, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hi,

On Saturday 05 July 2014 02:23 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/01/2014 05:11 PM, Kishon Vijay Abraham I wrote:
> 
>>>>>>> This PHY, though formally being a part of Renesas USBHS controller,
>>>>>>> contains
>>>>>>> the
>>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>>> them
>>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>>> either
>>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>>> EHCI/OHCI
>>>>>>> or xHCI controllers.
> 
>> .
>> .
>> <snip>
>> .
>> .
> 
>>>>>> IIUC, channel 0 can be configured for either EHCI/OHCI or HS-USB but
>>>>>> can't be
>>>>>> used for both.  And channel 1 can be configured for either PCI EHCI/OHCI or
>>>>>> xHCI. right?
> 
>>>>>      Yes. However that depends on the driver load order: if e.g. xHCI
>>>>> driver is
>>>>> loaded later than PCI USB drivers,
>>>>> it will override the channel routing.
> 
>>>> So will the PCI USB drivers will be notified of that?
> 
>>>     Unfortunately, no. But this is also the case with the other multi-PHY
>>> drivers...
> 
>> IIRC, in the case of other existing multi-phy drivers, all the PHYs can
>> co-exist without actually overriding anything that was configured previously.
> 
>    'phy-exynos-mipi-video' driver looked somewhat suspicious in this respect (I
> didn't understand why they used "#phy-cells" of 1, having 2 channels with two
> PHYs each) but upon further scrutiny it appears that the PHYs on one channel
> function quite independently...
> 
> [...]
> 
>>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>>> '1'.
> 
>>>>>      Hm, but I need to perform a special PHY power up sequence for the
>>>>> USBHS PHY
>>>>> itself (corresponding to channel #0, selector #1).
> 
>>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>>> PHY specifier
> 
>>>>>      I already have "#phy-cells" prop equal to 2.
> 
>>>>>> (The channel should be configured to a particualr mode in xlate).
> 
>>>>>      I have even considered using the of_xlate() method at first but then
>>>>> abandoned that idea for the phy_init() method...
> 
>>>> If you want to configure the PHY to a particular mode, xlate should be the
>>>> best
>>>> place.
> 
>>>     I tried to move the code there from the init() method but then I realized
>>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>>> register and there's no place I can disable/unprepare this clock if I do the
> 
>    Unless I prepare/enable the clock when probing, and undo it on removal, that
> is.
> 
>>> channel routing in the xlate() method. So no, I don't agree here.
> 
>> enabling clock from init() seems correct to me. We need a better way to avoid
>> overriding the PHY to a particular mode.
> 
>    In fact, in my case such override may be rather desirable.

Don't understand how overriding is desirable. Won't it affect the first
controller that got the PHY?
> 
> [...]
>> .
>> .
>> <snip>
>> .
>> .
>>
>>>>>>> +struct rcar_gen2_phy_driver {
>>>>>>> +    void __iomem *base;
>>>>>>> +    struct clk *clk;
>>>>>>> +    spinlock_t lock;
>>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];
> 
>>>>>> This can be created dynamically based on the number of sub-nodes. In this
>>>>>> case
> 
>>>     Did you mean that I'll need to use linked list here instead of an array?
> 
>> Nope. I meant something like below.
> 
>> struct rcar_gen2_phy_driver {
>>     .
>>     .
>>     struct rcar_gen2_phy **phys;
>> }
>>
>> probe()
>> {
>>     <snip>
>>     int i = 0, channel_count;
>>     struct rcar_gen2_phy **phys;
>>     channel_count = of_get_child_count(np);
> 
>    Didn't know of such function...
> 
>>     phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);
> 
>    Rather kcalloc().
> 
>>     for_each_child_of_node(dev->of_node, np) {
>>         struct rcar_gen2_phy *phy;
>>         .
>>         .
>>         phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>         .
>>         .
>>         phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
>>         phys[i++] = phy;
>>     }
>>     drv->phys = phys;
>>     <snip>
>> }
> 
>> Then you can access 'phys' just like how you access an array.
> 
>    Aren't you over-engineering things? I'd rather have just an array of 'struct
> rcar_gen2_phy' dynamically allocated at once, instead of an array of pointers
> to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

yeah.. that can be done.
> 
>    Anyway, this means that I'll have to do linear search for the needed PHY in
> the xlate() method, just like it would have been with a linked list.

indeed. Unless we directly pass the index in the phy specifier (from dt). But I
would prefer linear search instead.
> Complication. :-)
> 
> [...]
> 
>>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>>> By this we need not hard code NUM_USB_CHANNELS.
> 
>>>>>      I don't quite understand what's up with hard-coding it -- this
>>>>> constant is
>>>>> dictated by the UGCTRL2 register layout anyway.
> 
>>>> right but you don't want to change the driver a whole lot when they change the
>>>> no of channels in the next version
> 
>>>     They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>>> However, the number of controllable channels didn't change.
> 
>> right.. that's where I'd like to have status = "disabled" for that channel in
>> your dt node.
> 
>    I disagree here. First, channel #1 is not controllable anyway, so of no
> interest to us. Anyway, if more controllable channel appear, may point is that
> should be a matter of introducing and properly handling a new "compatible"
> property, not just adding/removing subnodes.

That will lead to broken dt data. I think we have to do both.
> 
>>>> or they use a slightly modified version of
>>>> this IP in a different SoC. And finding the number of channels dynamically is
>>>> not complicated anyway IMO.
> 
>>>     Sorry, but what you're saying here just doesn't make sense to me. I'd need
>>> to modify the driver for the different number of the controllable channels in
>>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>>> you said. If they were read from the device tree, that would have made sense
>>> but you seem to be against that...
> 
>> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
>> NUM_CHANNELS in this driver?
> 
>    Two; we have only two controllable channels in any case.

NAK.

-Kishon

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
  2014-07-08 12:32               ` Kishon Vijay Abraham I
@ 2014-07-08 21:27                 ` Sergei Shtylyov
  -1 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-07-08 21:27 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 07/08/2014 04:20 PM, Kishon Vijay Abraham I wrote:

>>>>>>>> This PHY, though formally being a part of Renesas USBHS controller,
>>>>>>>> contains
>>>>>>>> the
>>>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>>>> them
>>>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>>>> either
>>>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>>>> EHCI/OHCI
>>>>>>>> or xHCI controllers.

[...]

>>>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>>>> '1'.

>>>>>>       Hm, but I need to perform a special PHY power up sequence for the
>>>>>> USBHS PHY
>>>>>> itself (corresponding to channel #0, selector #1).

>>>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>>>> PHY specifier

>>>>>>       I already have "#phy-cells" prop equal to 2.

>>>>>>> (The channel should be configured to a particualr mode in xlate).

>>>>>>       I have even considered using the of_xlate() method at first but then
>>>>>> abandoned that idea for the phy_init() method...

>>>>> If you want to configure the PHY to a particular mode, xlate should be the
>>>>> best place.

>>>>      I tried to move the code there from the init() method but then I realized
>>>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>>>> register and there's no place I can disable/unprepare this clock if I do the

>>     Unless I prepare/enable the clock when probing, and undo it on removal, that
>> is.

>>>> channel routing in the xlate() method. So no, I don't agree here.

>>> enabling clock from init() seems correct to me. We need a better way to avoid
>>> overriding the PHY to a particular mode.

>>     In fact, in my case such override may be rather desirable.

> Don't understand how overriding is desirable.

    Consider the scenario where OHCI/EHCI drivers load first: they would claim 
all channels for themselves (despite channel #0 might have a host connected 
instead of a device). With the channel exclusion mechanism in place, the USBHS 
driver loaded later won't be able to take control of the channel #0, while 
it's actually desirable.

> Won't it affect the first controller that got the PHY?

    It will, though it might be desirable. But I'd probably agree that making 
the host/UDC driver hold into its PHY forbidding the other drivers to bind to 
the same channel would be more straight-forward way: just load the drivers 
that you want to control a given channel first, not second. I guess you want 
me to invent something here?

[...]

>>>>>>>> +struct rcar_gen2_phy_driver {
>>>>>>>> +    void __iomem *base;
>>>>>>>> +    struct clk *clk;
>>>>>>>> +    spinlock_t lock;
>>>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>>>>>> This can be created dynamically based on the number of sub-nodes. In this
>>>>>>> case

>>>>      Did you mean that I'll need to use linked list here instead of an array?

>>> Nope. I meant something like below.

>>> struct rcar_gen2_phy_driver {
>>>      .
>>>      .
>>>      struct rcar_gen2_phy **phys;
>>> }
>>>
>>> probe()
>>> {
>>>      <snip>
>>>      int i = 0, channel_count;
>>>      struct rcar_gen2_phy **phys;
>>>      channel_count = of_get_child_count(np);

>>     Didn't know of such function...

>>>      phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);

>>     Rather kcalloc().

>>>      for_each_child_of_node(dev->of_node, np) {
>>>          struct rcar_gen2_phy *phy;
>>>          .
>>>          .
>>>          phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>>          .
>>>          .
>>>          phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
>>>          phys[i++] = phy;
>>>      }
>>>      drv->phys = phys;
>>>      <snip>
>>> }

>>> Then you can access 'phys' just like how you access an array.

>>     Aren't you over-engineering things? I'd rather have just an array of 'struct
>> rcar_gen2_phy' dynamically allocated at once, instead of an array of pointers
>> to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

> yeah.. that can be done.

    That would a bit simpler and even faster, I think.

>>     Anyway, this means that I'll have to do linear search for the needed PHY in
>> the xlate() method, just like it would have been with a linked list.

> indeed. Unless we directly pass the index in the phy specifier (from dt). But I

    The DT subnodes are not guaranteed to be in a certain order or even have 
consecutive channel #'s, no?

> would prefer linear search instead.

    OK.

>> Complication. :-)

>> [...]

>>>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>>>> By this we need not hard code NUM_USB_CHANNELS.

>>>>>>       I don't quite understand what's up with hard-coding it -- this
>>>>>> constant is
>>>>>> dictated by the UGCTRL2 register layout anyway.

>>>>> right but you don't want to change the driver a whole lot when they change the
>>>>> no of channels in the next version

>>>>      They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>>>> However, the number of controllable channels didn't change.

>>> right.. that's where I'd like to have status = "disabled" for that channel in
>>> your dt node.

    Looking at the IEEE1275-1994 standard, I do not think "disabled" could 
mean that the device is not present:

“disabled”
	The device represented by this node is not operational, but it might
	become operational in the future (e.g., an external switch is turned
	off, or something isn’t plugged in.)

    So I would really prefer the non-existing channels to not be described in DT.

>>     I disagree here. First, channel #1 is not controllable anyway, so of no
>> interest to us. Anyway, if more controllable channel appear, may point is that
>> should be a matter of introducing and properly handling a new "compatible"
>> property, not just adding/removing subnodes.

> That will lead to broken dt data.

    Could you elaborate?

> I think we have to do both.

    Well, if we agree that we'd have subnodes, yes.

>>>>> or they use a slightly modified version of
>>>>> this IP in a different SoC. And finding the number of channels dynamically is
>>>>> not complicated anyway IMO.

>>>>      Sorry, but what you're saying here just doesn't make sense to me. I'd need
>>>> to modify the driver for the different number of the controllable channels in
>>>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>>>> you said. If they were read from the device tree, that would have made sense
>>>> but you seem to be against that...

>>> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
>>> NUM_CHANNELS in this driver?

    I have now read the info on all members of the R-Car generation 2 SoCs, 
most will have the same two channels as R8A7791, only R8A7790 has 3 channels.
There's still some hope that the future SoC family (a few years away from now) 
would have more sane PHY design...

>>     Two; we have only two controllable channels in any case.

> NAK.

    Seriously, I don't want to waste any memory on non-switchable channel, 
even when it exists.

> -Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v4] phy: Renesas R-Car Gen2 PHY driver
@ 2014-07-08 21:27                 ` Sergei Shtylyov
  0 siblings, 0 replies; 38+ messages in thread
From: Sergei Shtylyov @ 2014-07-08 21:27 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, grant.likely, devicetree
  Cc: rdunlap, linux-doc, linux-sh

Hello.

On 07/08/2014 04:20 PM, Kishon Vijay Abraham I wrote:

>>>>>>>> This PHY, though formally being a part of Renesas USBHS controller,
>>>>>>>> contains
>>>>>>>> the
>>>>>>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>>>>>>> them
>>>>>>>> channels) to the different USB controllers: channel 0 can be connected to
>>>>>>>> either
>>>>>>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI
>>>>>>>> EHCI/OHCI
>>>>>>>> or xHCI controllers.

[...]

>>>>>>> So ideally only two sub-nodes should be created for channel '0' and channel
>>>>>>> '1'.

>>>>>>       Hm, but I need to perform a special PHY power up sequence for the
>>>>>> USBHS PHY
>>>>>> itself (corresponding to channel #0, selector #1).

>>>>>>> You can configure a channel to a particular mode by passing the mode in
>>>>>>> PHY specifier

>>>>>>       I already have "#phy-cells" prop equal to 2.

>>>>>>> (The channel should be configured to a particualr mode in xlate).

>>>>>>       I have even considered using the of_xlate() method at first but then
>>>>>> abandoned that idea for the phy_init() method...

>>>>> If you want to configure the PHY to a particular mode, xlate should be the
>>>>> best place.

>>>>      I tried to move the code there from the init() method but then I realized
>>>> that I need to prepare/enable the USBHS clock before writing to the UGCTRL2
>>>> register and there's no place I can disable/unprepare this clock if I do the

>>     Unless I prepare/enable the clock when probing, and undo it on removal, that
>> is.

>>>> channel routing in the xlate() method. So no, I don't agree here.

>>> enabling clock from init() seems correct to me. We need a better way to avoid
>>> overriding the PHY to a particular mode.

>>     In fact, in my case such override may be rather desirable.

> Don't understand how overriding is desirable.

    Consider the scenario where OHCI/EHCI drivers load first: they would claim 
all channels for themselves (despite channel #0 might have a host connected 
instead of a device). With the channel exclusion mechanism in place, the USBHS 
driver loaded later won't be able to take control of the channel #0, while 
it's actually desirable.

> Won't it affect the first controller that got the PHY?

    It will, though it might be desirable. But I'd probably agree that making 
the host/UDC driver hold into its PHY forbidding the other drivers to bind to 
the same channel would be more straight-forward way: just load the drivers 
that you want to control a given channel first, not second. I guess you want 
me to invent something here?

[...]

>>>>>>>> +struct rcar_gen2_phy_driver {
>>>>>>>> +    void __iomem *base;
>>>>>>>> +    struct clk *clk;
>>>>>>>> +    spinlock_t lock;
>>>>>>>> +    struct rcar_gen2_phy phys[NUM_USB_CHANNELS][2];

>>>>>>> This can be created dynamically based on the number of sub-nodes. In this
>>>>>>> case

>>>>      Did you mean that I'll need to use linked list here instead of an array?

>>> Nope. I meant something like below.

>>> struct rcar_gen2_phy_driver {
>>>      .
>>>      .
>>>      struct rcar_gen2_phy **phys;
>>> }
>>>
>>> probe()
>>> {
>>>      <snip>
>>>      int i = 0, channel_count;
>>>      struct rcar_gen2_phy **phys;
>>>      channel_count = of_get_child_count(np);

>>     Didn't know of such function...

>>>      phys = kzalloc(sizeof(*phys) * channel_count, GFP_KERNEL);

>>     Rather kcalloc().

>>>      for_each_child_of_node(dev->of_node, np) {
>>>          struct rcar_gen2_phy *phy;
>>>          .
>>>          .
>>>          phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>>          .
>>>          .
>>>          phy->phy = devm_phy_create(dev, &rcar_gen2_phy_ops, NULL);
>>>          phys[i++] = phy;
>>>      }
>>>      drv->phys = phys;
>>>      <snip>
>>> }

>>> Then you can access 'phys' just like how you access an array.

>>     Aren't you over-engineering things? I'd rather have just an array of 'struct
>> rcar_gen2_phy' dynamically allocated at once, instead of an array of pointers
>> to struct rcar_gen2_phy' and then PHYs allocated piecemeal...

> yeah.. that can be done.

    That would a bit simpler and even faster, I think.

>>     Anyway, this means that I'll have to do linear search for the needed PHY in
>> the xlate() method, just like it would have been with a linked list.

> indeed. Unless we directly pass the index in the phy specifier (from dt). But I

    The DT subnodes are not guaranteed to be in a certain order or even have 
consecutive channel #'s, no?

> would prefer linear search instead.

    OK.

>> Complication. :-)

>> [...]

>>>>>>> it'll be only rcar_gen2_phy phys[2], one for each channel.
>>>>>>> By this we need not hard code NUM_USB_CHANNELS.

>>>>>>       I don't quite understand what's up with hard-coding it -- this
>>>>>> constant is
>>>>>> dictated by the UGCTRL2 register layout anyway.

>>>>> right but you don't want to change the driver a whole lot when they change the
>>>>> no of channels in the next version

>>>>      They have already done so: R8A7790 has 3 USB channels, R8A7791 has only 2.
>>>> However, the number of controllable channels didn't change.

>>> right.. that's where I'd like to have status = "disabled" for that channel in
>>> your dt node.

    Looking at the IEEE1275-1994 standard, I do not think "disabled" could 
mean that the device is not present:

“disabled”
	The device represented by this node is not operational, but it might
	become operational in the future (e.g., an external switch is turned
	off, or something isn’t plugged in.)

    So I would really prefer the non-existing channels to not be described in DT.

>>     I disagree here. First, channel #1 is not controllable anyway, so of no
>> interest to us. Anyway, if more controllable channel appear, may point is that
>> should be a matter of introducing and properly handling a new "compatible"
>> property, not just adding/removing subnodes.

> That will lead to broken dt data.

    Could you elaborate?

> I think we have to do both.

    Well, if we agree that we'd have subnodes, yes.

>>>>> or they use a slightly modified version of
>>>>> this IP in a different SoC. And finding the number of channels dynamically is
>>>>> not complicated anyway IMO.

>>>>      Sorry, but what you're saying here just doesn't make sense to me. I'd need
>>>> to modify the driver for the different number of the controllable channels in
>>>> any case since the UGCTRL2 masks/values have to be hard coded in the driver as
>>>> you said. If they were read from the device tree, that would have made sense
>>>> but you seem to be against that...

>>> R8A7790 has 3 USB channels and R8A7791 has only 2. So what should be the
>>> NUM_CHANNELS in this driver?

    I have now read the info on all members of the R-Car generation 2 SoCs, 
most will have the same two channels as R8A7791, only R8A7790 has 3 channels.
There's still some hope that the future SoC family (a few years away from now) 
would have more sane PHY design...

>>     Two; we have only two controllable channels in any case.

> NAK.

    Seriously, I don't want to waste any memory on non-switchable channel, 
even when it exists.

> -Kishon

WBR, Sergei


^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2014-07-08 21:27 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-23 22:06 [PATCH v4] phy: Renesas R-Car Gen2 PHY driver Sergei Shtylyov
2014-05-23 22:06 ` Sergei Shtylyov
2014-05-26  7:28 ` Geert Uytterhoeven
2014-05-26  7:28   ` Geert Uytterhoeven
2014-05-26  7:48   ` Simon Horman
2014-05-26  7:48     ` Simon Horman
2014-05-26  8:04     ` Geert Uytterhoeven
2014-05-26  8:04       ` Geert Uytterhoeven
2014-05-26  9:00       ` Simon Horman
2014-05-26  9:00         ` Simon Horman
2014-05-27  9:29 ` Yoshihiro Shimoda
2014-05-27  9:29   ` Yoshihiro Shimoda
2014-05-27 19:38   ` Sergei Shtylyov
2014-05-27 19:38     ` Sergei Shtylyov
2014-05-28  1:12     ` Yoshihiro Shimoda
2014-05-28  1:12       ` Yoshihiro Shimoda
2014-06-04 11:40 ` Kishon Vijay Abraham I
2014-06-04 11:52   ` Kishon Vijay Abraham I
2014-06-04 21:54   ` Sergei Shtylyov
2014-06-04 21:54     ` Sergei Shtylyov
2014-06-09 11:43     ` Laurent Pinchart
2014-06-09 11:43       ` Laurent Pinchart
2014-06-10 10:43     ` Kishon Vijay Abraham I
2014-06-10 10:55       ` Kishon Vijay Abraham I
2014-06-25 22:16       ` Sergei Shtylyov
2014-06-25 22:16         ` Sergei Shtylyov
2014-07-01 13:11         ` Kishon Vijay Abraham I
2014-07-01 13:23           ` Kishon Vijay Abraham I
2014-07-04 20:53           ` Sergei Shtylyov
2014-07-04 20:53             ` Sergei Shtylyov
2014-07-08 12:20             ` Kishon Vijay Abraham I
2014-07-08 12:32               ` Kishon Vijay Abraham I
2014-07-08 21:27               ` Sergei Shtylyov
2014-07-08 21:27                 ` Sergei Shtylyov
2014-06-09 11:44 ` Laurent Pinchart
2014-06-09 11:44   ` Laurent Pinchart
2014-06-09 12:34   ` Sergei Shtylyov
2014-06-09 12:34     ` Sergei Shtylyov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.