[PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
source. The 2 available options are the differential input clock pair
supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
clock source supplied through USB_XTAL/USB_EXTAL.

The device can be configured to use the on-chip source by adding the
"renesas,use-on-chip-clk" option in the corresponding device tree node.

Signed-off-by: Petre Pircalabu <Petre_Pircalabu@mentor.com>
---
 .../devicetree/bindings/phy/rcar-gen3-phy-usb3.txt |  22 +++
 arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi       |   8 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/phy/Kconfig                                |   8 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-rcar-gen3-usb3.c                   | 166 +++++++++++++++++++++
 6 files changed, 206 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
 create mode 100644 drivers/phy/phy-rcar-gen3-usb3.c

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
new file mode 100644
index 0000000..aa9657c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
@@ -0,0 +1,22 @@
+* Renesas R-Car generation 3 USB 3.0 PHY
+
+This file provides information on what the device node for the R-Car generation
+3 USB 3.0 PHY contains.
+
+Required properties:
+- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible device.
+- reg: offset and length of the partial USB 3.0 Host PHY register block.
+- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
+
+Optional properties:
+
+Example (R-Car H3):
+
+	usb3_phy0: usb-phy@e65ee000 {
+		compatible = "renesas,rcar-gen3-usb3-phy";
+		reg = <0 0xe65ee000 0 0x100>;
+		power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		#phy-cells = <0>;
+		renesas,use-on-chip-clk;
+		status = "okay";
+	};
diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi
index 2bf5911..33fe114 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi
@@ -2499,6 +2499,14 @@
 			status = "disabled";
 		};
 
+		usb3_phy0: usb-phy@e65ee000 {
+			compatible = "renesas,rcar-gen3-usb3-phy";
+			reg = <0 0xe65ee000 0 0x100>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
 		ehci0: usb@ee080100 {
 			compatible = "generic-ehci";
 			reg = <0 0xee080100 0 0x100>;
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 4e030c8..df9ca01 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -441,6 +441,7 @@ CONFIG_PWM=y
 CONFIG_PWM_TEGRA=m
 CONFIG_COMMON_RESET_HI6220=y
 CONFIG_PHY_RCAR_GEN3_USB2=y
+CONFIG_PHY_RCAR_GEN3_USB3=y
 CONFIG_PHY_HI6220_USB=y
 CONFIG_PHY_XGENE=y
 CONFIG_PHY_TEGRA_XUSB=y
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index fe00f91..4c8a207 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -154,6 +154,14 @@ config PHY_RCAR_GEN3_USB2
 	help
 	  Support for USB 2.0 PHY found on Renesas R-Car generation 3 SoCs.
 
+config PHY_RCAR_GEN3_USB3
+	tristate "Renesas R-Car generation 3 USB 3.0 PHY driver"
+	depends on ARCH_RENESAS
+	select GENERIC_PHY
+	help
+	  Enable this to add support for the USB 3.0 PHY found on
+	  Renesas R-Car generation 3 SoCs.
+
 config OMAP_CONTROL_PHY
 	tristate "OMAP CONTROL PHY Driver"
 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index a534cf5..aeda949 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
 obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
 obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
 obj-$(CONFIG_PHY_RCAR_GEN3_USB2)	+= phy-rcar-gen3-usb2.o
+obj-$(CONFIG_PHY_RCAR_GEN3_USB3)	+= phy-rcar-gen3-usb3.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
diff --git a/drivers/phy/phy-rcar-gen3-usb3.c b/drivers/phy/phy-rcar-gen3-usb3.c
new file mode 100644
index 0000000..87fa24d
--- /dev/null
+++ b/drivers/phy/phy-rcar-gen3-usb3.c
@@ -0,0 +1,166 @@
+/*
+ * Renesas R-Car Gen3 for USB3.0 PHY driver
+ *
+ * Copyright (C) 2017 Mentor Graphics Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define USB30_CLKSET0 0x34
+#define USB30_CLKSET1 0x36
+
+#define USB30_CLKSET0_FSEL_MASK                 0x003F
+#define USB30_CLKSET0_FSEL_USB_XTAL             0x0002
+#define USB30_CLKSET0_FSEL_USB3S0_CLK           0x0027
+
+#define USB30_CLKSET1_PLL_MULTI_MASK            0x1FC0
+#define USB30_CLKSET1_PLL_MULTI_USB_XTAL        (0x64 << 6)
+#define USB30_CLKSET1_PLL_MULTI_USB3S0_CLK      (0x00 << 6)
+#define USB30_CLKSET1_REF_CLKDIV                BIT(3)
+#define USB30_CLKSET1_USB_SEL                   BIT(0)
+
+struct rcar_gen3_usb3_phy {
+	void __iomem *base;
+	struct phy *phy;
+	u8 use_on_chip_clk;
+};
+
+static int rcar_gen3_phy_usb3_init(struct phy *p)
+{
+	struct rcar_gen3_usb3_phy *phy_dev = phy_get_drvdata(p);
+	void __iomem *usb3_base = phy_dev->base;
+
+	u16 clkset0, clkset1;
+
+	clkset0 = readw(usb3_base + USB30_CLKSET0);
+	clkset1 = readw(usb3_base + USB30_CLKSET1);
+
+	dev_dbg(&p->dev, "USB30_CLKSET0 initial value = 0x%04X\n", clkset0);
+	dev_dbg(&p->dev, "USB30_CLKSET1 initial value = 0x%04X\n", clkset1);
+
+	clkset0 &= ~USB30_CLKSET0_FSEL_MASK;
+	clkset1 &= ~(USB30_CLKSET1_PLL_MULTI_MASK | USB30_CLKSET1_REF_CLKDIV |
+			USB30_CLKSET1_USB_SEL);
+
+	if (phy_dev->use_on_chip_clk) {
+		/* Select 50MHz clock */
+		dev_info(&p->dev, "USE USB_XTAL clock (50MHz)\n");
+		clkset0 |= USB30_CLKSET0_FSEL_USB_XTAL;
+		clkset1 |= USB30_CLKSET1_PLL_MULTI_USB_XTAL |
+			USB30_CLKSET1_REF_CLKDIV;
+		clkset1 &= ~USB30_CLKSET1_USB_SEL;
+	} else {
+		/* Select 100MHz clock */
+		dev_info(&p->dev, "USE USB3S0_CLK reference (100MHz)\n");
+		clkset0 |= USB30_CLKSET0_FSEL_USB3S0_CLK;
+		clkset1 |= USB30_CLKSET1_PLL_MULTI_USB3S0_CLK |
+			USB30_CLKSET1_USB_SEL;
+		clkset1 &= ~USB30_CLKSET1_REF_CLKDIV;
+	}
+
+	dev_dbg(&p->dev, "USB30_CLKSET0 new value = 0x%04X\n", clkset0);
+	dev_dbg(&p->dev, "USB30_CLKSET1 new value = 0x%04X\n", clkset1);
+
+	writew(clkset0, usb3_base + USB30_CLKSET0);
+	writew(clkset1, usb3_base + USB30_CLKSET1);
+
+	return 0;
+}
+
+static int rcar_gen3_phy_usb3_exit(struct phy *p)
+{
+	return 0;
+}
+
+
+static struct phy_ops rcar_gen3_phy_usb3_ops = {
+	.init		= rcar_gen3_phy_usb3_init,
+	.exit		= rcar_gen3_phy_usb3_exit,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id rcar_gen3_phy_usb3_match_table[] = {
+	{ .compatible = "renesas,rcar-gen3-usb3-phy" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rcar_gen3_phy_usb3_match_table);
+
+static int rcar_gen3_phy_usb3_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rcar_gen3_usb3_phy *phy_dev;
+	struct phy_provider *provider;
+	struct resource *res;
+
+	if (!dev->of_node) {
+		dev_err(dev, "This driver needs a device tree node\n");
+		return -EINVAL;
+	}
+
+	phy_dev = devm_kzalloc(dev, sizeof(*phy_dev), GFP_KERNEL);
+	if (!phy_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	phy_dev->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(phy_dev->base))
+		return PTR_ERR(phy_dev->base);
+
+	phy_dev->use_on_chip_clk = device_property_read_bool(dev,
+			"renesas,use-on-chip-clk");
+
+	pm_runtime_enable(dev);
+	phy_dev->phy = devm_phy_create(dev, NULL, &rcar_gen3_phy_usb3_ops);
+	if (IS_ERR(phy_dev->phy)) {
+		dev_err(dev, "Failed to create Rcar Gen3 USB3 PHY\n");
+		return PTR_ERR(phy_dev->phy);
+	}
+
+	platform_set_drvdata(pdev, phy_dev);
+	phy_set_drvdata(phy_dev->phy, phy_dev);
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "Failed to register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_info(&pdev->dev, "Initialized RCAR Gen3 USB3 PHY module\n");
+	return 0;
+}
+
+static int rcar_gen3_phy_usb3_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver rcar_gen3_phy_usb3_driver = {
+	.driver = {
+		.name		= "phy_rcar_gen3_usb3",
+		.of_match_table	= rcar_gen3_phy_usb3_match_table,
+	},
+	.probe	= rcar_gen3_phy_usb3_probe,
+	.remove	= rcar_gen3_phy_usb3_remove,
+};
+module_platform_driver(rcar_gen3_phy_usb3_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Renesas R-Car Gen3 USB 3.0 PHY");
+MODULE_AUTHOR("Petre Pircalabu <petre_pircalabu@mentor.com>");
-- 
1.9.1

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Petre,

Thank you for the patch!

I'm also developing a similar driver now :)
(I don't submit it though...)
My developing driver has SSC and VBUS_EN setting.

Anyway, I have some comments about your patch.

> -----Original Message-----
> From: Petre Pircalabu
> Sent: Thursday, May 18, 2017 2:58 AM
> 
> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
> source. The 2 available options are the differential input clock pair
> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
> clock source supplied through USB_XTAL/USB_EXTAL.
> 
> The device can be configured to use the on-chip source by adding the
> "renesas,use-on-chip-clk" option in the corresponding device tree node.
> 
> Signed-off-by: Petre Pircalabu <Petre_Pircalabu@mentor.com>
> ---
>  .../devicetree/bindings/phy/rcar-gen3-phy-usb3.txt |  22 +++
>  arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi       |   8 +
>  arch/arm64/configs/defconfig                       |   1 +
>  drivers/phy/Kconfig                                |   8 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/phy-rcar-gen3-usb3.c                   | 166 +++++++++++++++++++++

I think you should separate 3 patches as below:

1) Add phy-rcar-gen3-usb3 driver
2) Add a device node to r8a7795-es1.dtsi
3) Enable the phy driver on defconfig

And when we submit a generic phy driver's patch (e.g. 1) above), we should send to the phy maintainer(s).
Also, when we submit a patch that is related to device tree(e.g. both 1) and 2) above),
we should send to the device tree maintainer(s).

> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> new file mode 100644
> index 0000000..aa9657c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> @@ -0,0 +1,22 @@
> +* Renesas R-Car generation 3 USB 3.0 PHY
> +
> +This file provides information on what the device node for the R-Car generation
> +3 USB 3.0 PHY contains.
> +
> +Required properties:
> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible device.

I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796".

> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.

I think we should add "clocks" property as required.

> +Optional properties:

You should add "renesas,use-on-chip-clk" here.
And, the name of "use-on-chip-clk" is not good to me.
FYI, my developing patch names "renesas,usb-extal".

> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index a534cf5..aeda949 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>  obj-$(CONFIG_PHY_MIPHY365X)		+= phy-miphy365x.o
>  obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
>  obj-$(CONFIG_PHY_RCAR_GEN3_USB2)	+= phy-rcar-gen3-usb2.o
> +obj-$(CONFIG_PHY_RCAR_GEN3_USB3)	+= phy-rcar-gen3-usb3.o

The latest linux-phy.git / next branch doesn't update yet though,
the directory will be changed to ./drivers/phy/renesas.

http://www.spinics.net/lists/linux-usb/msg156875.html

> diff --git a/drivers/phy/phy-rcar-gen3-usb3.c b/drivers/phy/phy-rcar-gen3-usb3.c
> new file mode 100644
> index 0000000..87fa24d
> --- /dev/null
> +++ b/drivers/phy/phy-rcar-gen3-usb3.c
> @@ -0,0 +1,166 @@
> +/*
> + * Renesas R-Car Gen3 for USB3.0 PHY driver
> + *
> + * Copyright (C) 2017 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> + * for more details.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>

We can remove this "linux/of_address.h".

> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define USB30_CLKSET0 0x34
> +#define USB30_CLKSET1 0x36
> +
> +#define USB30_CLKSET0_FSEL_MASK                 0x003F
> +#define USB30_CLKSET0_FSEL_USB_XTAL             0x0002
> +#define USB30_CLKSET0_FSEL_USB3S0_CLK           0x0027
> +
> +#define USB30_CLKSET1_PLL_MULTI_MASK            0x1FC0
> +#define USB30_CLKSET1_PLL_MULTI_USB_XTAL        (0x64 << 6)
> +#define USB30_CLKSET1_PLL_MULTI_USB3S0_CLK      (0x00 << 6)

I prefer the "6" becomes a definition.

> +#define USB30_CLKSET1_REF_CLKDIV                BIT(3)
> +#define USB30_CLKSET1_USB_SEL                   BIT(0)
> +
> +struct rcar_gen3_usb3_phy {
> +	void __iomem *base;
> +	struct phy *phy;
> +	u8 use_on_chip_clk;
> +};
> +
> +static int rcar_gen3_phy_usb3_init(struct phy *p)
> +{
> +	struct rcar_gen3_usb3_phy *phy_dev = phy_get_drvdata(p);
> +	void __iomem *usb3_base = phy_dev->base;
> +
> +	u16 clkset0, clkset1;
> +
> +	clkset0 = readw(usb3_base + USB30_CLKSET0);
> +	clkset1 = readw(usb3_base + USB30_CLKSET1);
> +
> +	dev_dbg(&p->dev, "USB30_CLKSET0 initial value = 0x%04X\n", clkset0);
> +	dev_dbg(&p->dev, "USB30_CLKSET1 initial value = 0x%04X\n", clkset1);

I guess the generic phy maintainer prefer dev_vdbg() (like phy-rcar-gen3-usb2.c).

> +	clkset0 &= ~USB30_CLKSET0_FSEL_MASK;
> +	clkset1 &= ~(USB30_CLKSET1_PLL_MULTI_MASK | USB30_CLKSET1_REF_CLKDIV |
> +			USB30_CLKSET1_USB_SEL);
> +
> +	if (phy_dev->use_on_chip_clk) {
> +		/* Select 50MHz clock */
> +		dev_info(&p->dev, "USE USB_XTAL clock (50MHz)\n");
> +		clkset0 |= USB30_CLKSET0_FSEL_USB_XTAL;
> +		clkset1 |= USB30_CLKSET1_PLL_MULTI_USB_XTAL |
> +			USB30_CLKSET1_REF_CLKDIV;
> +		clkset1 &= ~USB30_CLKSET1_USB_SEL;

Since USB30_CLKSET1_USB_SEL bit is cleared before, I don't think this code needs.

> +	} else {
> +		/* Select 100MHz clock */
> +		dev_info(&p->dev, "USE USB3S0_CLK reference (100MHz)\n");
> +		clkset0 |= USB30_CLKSET0_FSEL_USB3S0_CLK;
> +		clkset1 |= USB30_CLKSET1_PLL_MULTI_USB3S0_CLK |
> +			USB30_CLKSET1_USB_SEL;
> +		clkset1 &= ~USB30_CLKSET1_REF_CLKDIV;

Since USB30_CLKSET1_REF_CLKDIV bit is cleared before, I don't think this code needs.

> +	}
> +
> +	dev_dbg(&p->dev, "USB30_CLKSET0 new value = 0x%04X\n", clkset0);
> +	dev_dbg(&p->dev, "USB30_CLKSET1 new value = 0x%04X\n", clkset1);
> +
> +	writew(clkset0, usb3_base + USB30_CLKSET0);
> +	writew(clkset1, usb3_base + USB30_CLKSET1);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_phy_usb3_exit(struct phy *p)
> +{
> +	return 0;
> +}

We can remove this function.

> +
> +static struct phy_ops rcar_gen3_phy_usb3_ops = {
> +	.init		= rcar_gen3_phy_usb3_init,
> +	.exit		= rcar_gen3_phy_usb3_exit,

We can remove this .exit.

> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id rcar_gen3_phy_usb3_match_table[] = {
> +	{ .compatible = "renesas,rcar-gen3-usb3-phy" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen3_phy_usb3_match_table);
> +
> +static int rcar_gen3_phy_usb3_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rcar_gen3_usb3_phy *phy_dev;
> +	struct phy_provider *provider;
> +	struct resource *res;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "This driver needs a device tree node\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_dev = devm_kzalloc(dev, sizeof(*phy_dev), GFP_KERNEL);
> +	if (!phy_dev)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	phy_dev->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(phy_dev->base))
> +		return PTR_ERR(phy_dev->base);
> +
> +	phy_dev->use_on_chip_clk = device_property_read_bool(dev,
> +			"renesas,use-on-chip-clk");
> +
> +	pm_runtime_enable(dev);
> +	phy_dev->phy = devm_phy_create(dev, NULL, &rcar_gen3_phy_usb3_ops);
> +	if (IS_ERR(phy_dev->phy)) {

You should call pm_runtime_disable(dev); here.
I prefer using "goto" and though :)

> +		dev_err(dev, "Failed to create Rcar Gen3 USB3 PHY\n");
> +		return PTR_ERR(phy_dev->phy);
> +	}
> +
> +	platform_set_drvdata(pdev, phy_dev);
> +	phy_set_drvdata(phy_dev->phy, phy_dev);
> +
> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider)) {

You should call pm_runtime_disable(dev); here.

> +		dev_err(dev, "Failed to register PHY provider\n");
> +		return PTR_ERR(provider);
> +	}
> +
> +	dev_info(&pdev->dev, "Initialized RCAR Gen3 USB3 PHY module\n");
> +	return 0;
> +}
> +
> +static int rcar_gen3_phy_usb3_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
> +static struct platform_driver rcar_gen3_phy_usb3_driver = {
> +	.driver = {
> +		.name		= "phy_rcar_gen3_usb3",
> +		.of_match_table	= rcar_gen3_phy_usb3_match_table,
> +	},
> +	.probe	= rcar_gen3_phy_usb3_probe,
> +	.remove	= rcar_gen3_phy_usb3_remove,
> +};
> +module_platform_driver(rcar_gen3_phy_usb3_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Renesas R-Car Gen3 USB 3.0 PHY");
> +MODULE_AUTHOR("Petre Pircalabu <petre_pircalabu@mentor.com>");
> --
> 1.9.1

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

On Thu, May 18, 2017 at 4:53 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Petre Pircalabu
>> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
>> source. The 2 available options are the differential input clock pair
>> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
>> clock source supplied through USB_XTAL/USB_EXTAL.
>>
>> The device can be configured to use the on-chip source by adding the
>> "renesas,use-on-chip-clk" option in the corresponding device tree node.
>>
>> Signed-off-by: Petre Pircalabu <Petre_Pircalabu@mentor.com>

>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
>> @@ -0,0 +1,22 @@
>> +* Renesas R-Car generation 3 USB 3.0 PHY
>> +
>> +This file provides information on what the device node for the R-Car generation
>> +3 USB 3.0 PHY contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible device.
>
> I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796".

"renesas,r8a7795-usb3-phy" etc.?

(I know all other Renesas PHY drivers use the old scheme)

>> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>
> I think we should add "clocks" property as required.
>
>> +Optional properties:
>
> You should add "renesas,use-on-chip-clk" here.
> And, the name of "use-on-chip-clk" is not good to me.
> FYI, my developing patch names "renesas,usb-extal".

Can this be decided at runtime, e.g. by looking at the rates of the clocks
to see which one is available/best suited?

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

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> -----Original Message-----
> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 5:46 PM
> 
> On Thu, May 18, 2017 at 4:53 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> From: Petre Pircalabu
> >> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
> >> source. The 2 available options are the differential input clock pair
> >> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
> >> clock source supplied through USB_XTAL/USB_EXTAL.
> >>
> >> The device can be configured to use the on-chip source by adding the
> >> "renesas,use-on-chip-clk" option in the corresponding device tree node.
> >>
> >> Signed-off-by: Petre Pircalabu <Petre_Pircalabu@mentor.com>
> 
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> >> @@ -0,0 +1,22 @@
> >> +* Renesas R-Car generation 3 USB 3.0 PHY
> >> +
> >> +This file provides information on what the device node for the R-Car generation
> >> +3 USB 3.0 PHY contains.
> >> +
> >> +Required properties:
> >> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible device.
> >
> > I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796".
> 
> "renesas,r8a7795-usb3-phy" etc.?
> 
> (I know all other Renesas PHY drivers use the old scheme)

Oops! Yes, we should be "renesas,<soc>-<driver name>".
(However, "renesas,rcar-gen3-usb3-phy" is OK.)

> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> >
> > I think we should add "clocks" property as required.
> >
> >> +Optional properties:
> >
> > You should add "renesas,use-on-chip-clk" here.
> > And, the name of "use-on-chip-clk" is not good to me.
> > FYI, my developing patch names "renesas,usb-extal".
> 
> Can this be decided at runtime, e.g. by looking at the rates of the clocks
> to see which one is available/best suited?

According to the HW manual, this module cannot see which one is available/best suited.
So, I don't think this can be decided at runtime.

Best regards,
Yoshihiro Shimoda

> 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

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>> >
>> > I think we should add "clocks" property as required.
>> >
>> >> +Optional properties:
>> >
>> > You should add "renesas,use-on-chip-clk" here.
>> > And, the name of "use-on-chip-clk" is not good to me.
>> > FYI, my developing patch names "renesas,usb-extal".
>>
>> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>> to see which one is available/best suited?
>
> According to the HW manual, this module cannot see which one is available/best suited.
> So, I don't think this can be decided at runtime.

I mean, can't Linux look at the rates of the two clocks, and if one is zero,
use the other?

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

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 8:41 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> >> >
> >> > I think we should add "clocks" property as required.
> >> >
> >> >> +Optional properties:
> >> >
> >> > You should add "renesas,use-on-chip-clk" here.
> >> > And, the name of "use-on-chip-clk" is not good to me.
> >> > FYI, my developing patch names "renesas,usb-extal".

I'm sorry for this. I missed description "on-chip clock source is supplied though
USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.

> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
> >> to see which one is available/best suited?
> >
> > According to the HW manual, this module cannot see which one is available/best suited.
> > So, I don't think this can be decided at runtime.
> 
> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
> use the other?

I still misunderstand your question though, but software cannot look at the rates
of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
for indicating the rates.

Best regards,
Yoshihiro Shimoda

> 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

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Geert Uytterhoeven
>> Sent: Thursday, May 18, 2017 8:41 PM
>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>> >> >
>> >> > I think we should add "clocks" property as required.
>> >> >
>> >> >> +Optional properties:
>> >> >
>> >> > You should add "renesas,use-on-chip-clk" here.
>> >> > And, the name of "use-on-chip-clk" is not good to me.
>> >> > FYI, my developing patch names "renesas,usb-extal".
>
> I'm sorry for this. I missed description "on-chip clock source is supplied though
> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.
>
>> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>> >> to see which one is available/best suited?
>> >
>> > According to the HW manual, this module cannot see which one is available/best suited.
>> > So, I don't think this can be decided at runtime.
>>
>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>> use the other?
>
> I still misunderstand your question though, but software cannot look at the rates
> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
> for indicating the rates.

If we have "clocks" properties, as you suggested, both clocks will have to
be described in DT.  Hence Linux will create clock objects for them, and the
USB PHY driver can call clk_get_rate() on those objects.

If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
with zero rate should be described in DT.

Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.

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

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Geert, Shimoda-san,

First, thank you very much for your comments. I will refactor the
patch and send a v2 patchset as soon as possible.

To my understanding, although the RCAR Gen3 HW manual references the
clk frequencies (50MHz for XTAL and 100MHz for external clock) the
actual register description only allows choosing between between an
on-chip clock and an external reference signal (no clock rate /
divisor is specified in the PHY registers ).
In my opinion, the "use-on-chip-clk" parameter was meant to
differentiate between the internal/external references, and not to by
a specific clock rate. My only concern here is that the device tree
description of the hardware might be a little difficult to understand
when compared to the actual description from SOC's HW manual.

Many thanks for your support,
Petre



On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Shimoda-san,
>
> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>> From: Geert Uytterhoeven
>>> Sent: Thursday, May 18, 2017 8:41 PM
>>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>>> >> >
>>> >> > I think we should add "clocks" property as required.
>>> >> >
>>> >> >> +Optional properties:
>>> >> >
>>> >> > You should add "renesas,use-on-chip-clk" here.
>>> >> > And, the name of "use-on-chip-clk" is not good to me.
>>> >> > FYI, my developing patch names "renesas,usb-extal".
>>
>> I'm sorry for this. I missed description "on-chip clock source is supplied though
>> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.
>>
>>> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>>> >> to see which one is available/best suited?
>>> >
>>> > According to the HW manual, this module cannot see which one is available/best suited.
>>> > So, I don't think this can be decided at runtime.
>>>
>>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>>> use the other?
>>
>> I still misunderstand your question though, but software cannot look at the rates
>> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
>> for indicating the rates.
>
> If we have "clocks" properties, as you suggested, both clocks will have to
> be described in DT.  Hence Linux will create clock objects for them, and the
> USB PHY driver can call clk_get_rate() on those objects.
>
> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
> with zero rate should be described in DT.
>
> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.
>
> 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

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Geert, Shimoda-san,

I have also used initialy the name "use-xtal-clk" to describe the
flag, but I've changed it to "use-on-chip-clk" to make a clear
distinction between the on-chip and the external clocks. If you think
"usb-extal" is more descriptive I can change he name to this value.

Regarding the clock, I suggest the distinction between them could be
made using predefined names.
E.g.:
..
clocks = <&clk1>, <&clk2>
clock-names = "external', "on-chip"
...

The device node can define none, one of both the clocks:
- If no clock is defined (e.g. backup compatibility with the
Salvator-X  board), the driver should not set any of the phy registers
and preserve the hw defaults (selects external)
- If only one clock is defined the PHY driver should be set-up
according to the name ("external" and "on-chip")
- if both clocks are defined (and valid), the driver should choose one
of them according to the "use-on-chip-clk" parameter (if present or
not). (the scenario of having 2 valid clocks and selecting the on-chip
one is possible, although I see no real application).

The default should be "no clocks defined" in order not to break any
existing (and not yet published) platforms.

Do you think this proposal is feasible?

Also, theoretically the driver should work also with r8a7795 ES2.0 and
r8a7796, but unfortunately I don't have access to neither a H3 ES2.0
nor a M3 platform. I can create a patch for these platforms, but I
would require your assistance in order to test it (I'm feeling a
little bit queasy in submitting an untested patch).

Best regards,
Petre

On Thu, May 18, 2017 at 11:21 PM, Petre Pircalabu
<petre.pircalabu@gmail.com> wrote:
> Hi Geert, Shimoda-san,
>
> First, thank you very much for your comments. I will refactor the
> patch and send a v2 patchset as soon as possible.
>
> To my understanding, although the RCAR Gen3 HW manual references the
> clk frequencies (50MHz for XTAL and 100MHz for external clock) the
> actual register description only allows choosing between between an
> on-chip clock and an external reference signal (no clock rate /
> divisor is specified in the PHY registers ).
> In my opinion, the "use-on-chip-clk" parameter was meant to
> differentiate between the internal/external references, and not to by
> a specific clock rate. My only concern here is that the device tree
> description of the hardware might be a little difficult to understand
> when compared to the actual description from SOC's HW manual.
>
> Many thanks for your support,
> Petre
>
>
>
> On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> Hi Shimoda-san,
>>
>> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>> From: Geert Uytterhoeven
>>>> Sent: Thursday, May 18, 2017 8:41 PM
>>>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>>>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>>>> >> >
>>>> >> > I think we should add "clocks" property as required.
>>>> >> >
>>>> >> >> +Optional properties:
>>>> >> >
>>>> >> > You should add "renesas,use-on-chip-clk" here.
>>>> >> > And, the name of "use-on-chip-clk" is not good to me.
>>>> >> > FYI, my developing patch names "renesas,usb-extal".
>>>
>>> I'm sorry for this. I missed description "on-chip clock source is supplied though
>>> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.
>>>
>>>> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>>>> >> to see which one is available/best suited?
>>>> >
>>>> > According to the HW manual, this module cannot see which one is available/best suited.
>>>> > So, I don't think this can be decided at runtime.
>>>>
>>>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>>>> use the other?
>>>
>>> I still misunderstand your question though, but software cannot look at the rates
>>> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
>>> for indicating the rates.
>>
>> If we have "clocks" properties, as you suggested, both clocks will have to
>> be described in DT.  Hence Linux will create clock objects for them, and the
>> USB PHY driver can call clk_get_rate() on those objects.
>>
>> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
>> with zero rate should be described in DT.
>>
>> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.
>>
>> 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

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Shimoda-san,

I just saw that you patch predates mine. I will drop my version and
integrate yours.

Many thanks for for your support,
Petre

On Fri, May 19, 2017 at 12:50 AM, Petre Pircalabu
<petre.pircalabu@gmail.com> wrote:
> Hi Geert, Shimoda-san,
>
> I have also used initialy the name "use-xtal-clk" to describe the
> flag, but I've changed it to "use-on-chip-clk" to make a clear
> distinction between the on-chip and the external clocks. If you think
> "usb-extal" is more descriptive I can change he name to this value.
>
> Regarding the clock, I suggest the distinction between them could be
> made using predefined names.
> E.g.:
> ..
> clocks = <&clk1>, <&clk2>
> clock-names = "external', "on-chip"
> ...
>
> The device node can define none, one of both the clocks:
> - If no clock is defined (e.g. backup compatibility with the
> Salvator-X  board), the driver should not set any of the phy registers
> and preserve the hw defaults (selects external)
> - If only one clock is defined the PHY driver should be set-up
> according to the name ("external" and "on-chip")
> - if both clocks are defined (and valid), the driver should choose one
> of them according to the "use-on-chip-clk" parameter (if present or
> not). (the scenario of having 2 valid clocks and selecting the on-chip
> one is possible, although I see no real application).
>
> The default should be "no clocks defined" in order not to break any
> existing (and not yet published) platforms.
>
> Do you think this proposal is feasible?
>
> Also, theoretically the driver should work also with r8a7795 ES2.0 and
> r8a7796, but unfortunately I don't have access to neither a H3 ES2.0
> nor a M3 platform. I can create a patch for these platforms, but I
> would require your assistance in order to test it (I'm feeling a
> little bit queasy in submitting an untested patch).
>
> Best regards,
> Petre
>
> On Thu, May 18, 2017 at 11:21 PM, Petre Pircalabu
> <petre.pircalabu@gmail.com> wrote:
>> Hi Geert, Shimoda-san,
>>
>> First, thank you very much for your comments. I will refactor the
>> patch and send a v2 patchset as soon as possible.
>>
>> To my understanding, although the RCAR Gen3 HW manual references the
>> clk frequencies (50MHz for XTAL and 100MHz for external clock) the
>> actual register description only allows choosing between between an
>> on-chip clock and an external reference signal (no clock rate /
>> divisor is specified in the PHY registers ).
>> In my opinion, the "use-on-chip-clk" parameter was meant to
>> differentiate between the internal/external references, and not to by
>> a specific clock rate. My only concern here is that the device tree
>> description of the hardware might be a little difficult to understand
>> when compared to the actual description from SOC's HW manual.
>>
>> Many thanks for your support,
>> Petre
>>
>>
>>
>> On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> Hi Shimoda-san,
>>>
>>> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>>> From: Geert Uytterhoeven
>>>>> Sent: Thursday, May 18, 2017 8:41 PM
>>>>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>>>>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>>>>> >> >
>>>>> >> > I think we should add "clocks" property as required.
>>>>> >> >
>>>>> >> >> +Optional properties:
>>>>> >> >
>>>>> >> > You should add "renesas,use-on-chip-clk" here.
>>>>> >> > And, the name of "use-on-chip-clk" is not good to me.
>>>>> >> > FYI, my developing patch names "renesas,usb-extal".
>>>>
>>>> I'm sorry for this. I missed description "on-chip clock source is supplied though
>>>> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.
>>>>
>>>>> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>>>>> >> to see which one is available/best suited?
>>>>> >
>>>>> > According to the HW manual, this module cannot see which one is available/best suited.
>>>>> > So, I don't think this can be decided at runtime.
>>>>>
>>>>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>>>>> use the other?
>>>>
>>>> I still misunderstand your question though, but software cannot look at the rates
>>>> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
>>>> for indicating the rates.
>>>
>>> If we have "clocks" properties, as you suggested, both clocks will have to
>>> be described in DT.  Hence Linux will create clock objects for them, and the
>>> USB PHY driver can call clk_get_rate() on those objects.
>>>
>>> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
>>> with zero rate should be described in DT.
>>>
>>> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.
>>>
>>> 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

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Petre-san,

> From: Petre Pircalabu
> Sent: Friday, May 19, 2017 8:55 AM
> 
> Hi Shimoda-san,
> 
> I just saw that you patch predates mine. I will drop my version and
> integrate yours.

I got it!

Best regards,
Yoshihiro Shimoda

> Many thanks for for your support,
> Petre

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 9:43 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> From: Geert Uytterhoeven
> >> Sent: Thursday, May 18, 2017 8:41 PM
> >> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> >> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> >> >> >
> >> >> > I think we should add "clocks" property as required.
> >> >> >
> >> >> >> +Optional properties:
> >> >> >
> >> >> > You should add "renesas,use-on-chip-clk" here.
> >> >> > And, the name of "use-on-chip-clk" is not good to me.
> >> >> > FYI, my developing patch names "renesas,usb-extal".
> >
> > I'm sorry for this. I missed description "on-chip clock source is supplied though
> > USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is reasonable.
> >
> >> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
> >> >> to see which one is available/best suited?
> >> >
> >> > According to the HW manual, this module cannot see which one is available/best suited.
> >> > So, I don't think this can be decided at runtime.
> >>
> >> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
> >> use the other?
> >
> > I still misunderstand your question though, but software cannot look at the rates
> > of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have registers
> > for indicating the rates.
> 
> If we have "clocks" properties, as you suggested, both clocks will have to
> be described in DT.  Hence Linux will create clock objects for them, and the
> USB PHY driver can call clk_get_rate() on those objects.
> 
> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
> with zero rate should be described in DT.
> 
> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.

Thank you for the detailed explanation!
I understood it. I will try to use such properties.

Best regards,
Yoshihiro Shimoda

> 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