[PATCH v2] phy-rcar-gen2-usb: add device tree support

[PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
documenting the device tree binding as necessary.

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

---
This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.

Changes in version 2:
- restored devm_clk_get() call and the error handling logic in the probe()
  method, removed clk_put() call in the remove() method.

 Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
 drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
 2 files changed, 68 insertions(+), 3 deletions(-)

Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
===================================================================
--- /dev/null
+++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
@@ -0,0 +1,29 @@
+* 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.
+- clocks: clock phandle and specifier pair.
+- clock-names: string, clock input name, must be "usbhs".
+
+Optional properties:
+- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
+			to PCI EHCI/OHCI; otherwise, it will be connected to the
+			USBHS controller.
+- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
+			to PCI EHCI/OHCI; otherwise, it will be connected to the
+			USBSS controller (xHCI).
+
+Example (Lager board):
+
+	usb-phy@e6590100 {
+		compatible = "renesas,usb-phy-r8a7790";
+		reg = <0 0xe6590100 0 0x100>;
+		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
+		clock-names = "usbhs";
+		renesas,channel2-pci;
+	};
Index: usb/drivers/usb/phy/phy-rcar-gen2-usb.c
===================================================================
--- usb.orig/drivers/usb/phy/phy-rcar-gen2-usb.c
+++ usb/drivers/usb/phy/phy-rcar-gen2-usb.c
@@ -1,8 +1,8 @@
 /*
  * Renesas R-Car Gen2 USB phy driver
  *
- * Copyright (C) 2013 Renesas Solutions Corp.
- * Copyright (C) 2013 Cogent Embedded, Inc.
+ * Copyright (C) 2013-2014 Renesas Solutions Corp.
+ * Copyright (C) 2013-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
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_data/usb-rcar-gen2-phy.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
@@ -167,6 +168,37 @@ out:
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
+#ifdef CONFIG_OF
+static struct rcar_gen2_phy_platform_data *
+rcar_gen2_usb_phy_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct rcar_gen2_phy_platform_data *pdata;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->chan0_pci = of_property_read_bool(np, "renesas,channel0-pci");
+	pdata->chan2_pci = of_property_read_bool(np, "renesas,channel2-pci");
+
+	return pdata;
+}
+
+static const struct of_device_id rcar_gen2_usb_phy_match_table[] = {
+	{ .compatible = "renesas,usb-phy-r8a7790" },
+	{ .compatible = "renesas,usb-phy-r8a7791" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rcar_gen2_usb_phy_match_table);
+#else
+static inline struct rcar_gen2_phy_platform_data *
+rcar_gen2_usb_phy_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int rcar_gen2_usb_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -177,7 +209,10 @@ static int rcar_gen2_usb_phy_probe(struc
 	struct clk *clk;
 	int retval;
 
-	pdata = dev_get_platdata(dev);
+	if (dev->of_node)
+		pdata = rcar_gen2_usb_phy_parse_dt(dev);
+	else
+		pdata = dev_get_platdata(dev);
 	if (!pdata) {
 		dev_err(dev, "No platform data\n");
 		return -EINVAL;
@@ -236,6 +271,7 @@ static int rcar_gen2_usb_phy_remove(stru
 static struct platform_driver rcar_gen2_usb_phy_driver = {
 	.driver = {
 		.name = "usb_phy_rcar_gen2",
+		.of_match_table = of_match_ptr(rcar_gen2_usb_phy_match_table),
 	},
 	.probe = rcar_gen2_usb_phy_probe,
 	.remove = rcar_gen2_usb_phy_remove,

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5083 bytes --]

Hi,

On Sat, Mar 01, 2014 at 04:07:53AM +0300, Sergei Shtylyov wrote:
> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
> documenting the device tree binding as necessary.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Unless someone on devicetree@vger gives me an ACK pretty soon, I'm
afraid this patch will miss v3.15.

> 
> ---
> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.
> 
> Changes in version 2:
> - restored devm_clk_get() call and the error handling logic in the probe()
>   method, removed clk_put() call in the remove() method.
> 
>  Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
>  drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
>  2 files changed, 68 insertions(+), 3 deletions(-)
> 
> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> ===================================================================
> --- /dev/null
> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> @@ -0,0 +1,29 @@
> +* 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.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".
> +
> +Optional properties:
> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
> +			to PCI EHCI/OHCI; otherwise, it will be connected to the
> +			USBHS controller.
> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
> +			to PCI EHCI/OHCI; otherwise, it will be connected to the
> +			USBSS controller (xHCI).

I wonder if these two properties should be taken care by pinctrl
framework instead.

> +
> +Example (Lager board):
> +
> +	usb-phy@e6590100 {
> +		compatible = "renesas,usb-phy-r8a7790";
> +		reg = <0 0xe6590100 0 0x100>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		clock-names = "usbhs";
> +		renesas,channel2-pci;
> +	};
> Index: usb/drivers/usb/phy/phy-rcar-gen2-usb.c
> ===================================================================
> --- usb.orig/drivers/usb/phy/phy-rcar-gen2-usb.c
> +++ usb/drivers/usb/phy/phy-rcar-gen2-usb.c
> @@ -1,8 +1,8 @@
>  /*
>   * Renesas R-Car Gen2 USB phy driver
>   *
> - * Copyright (C) 2013 Renesas Solutions Corp.
> - * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013-2014 Renesas Solutions Corp.
> + * Copyright (C) 2013-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
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/platform_data/usb-rcar-gen2-phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> @@ -167,6 +168,37 @@ out:
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  }
>  
> +#ifdef CONFIG_OF
> +static struct rcar_gen2_phy_platform_data *
> +rcar_gen2_usb_phy_parse_dt(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct rcar_gen2_phy_platform_data *pdata;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	pdata->chan0_pci = of_property_read_bool(np, "renesas,channel0-pci");
> +	pdata->chan2_pci = of_property_read_bool(np, "renesas,channel2-pci");
> +
> +	return pdata;
> +}
> +
> +static const struct of_device_id rcar_gen2_usb_phy_match_table[] = {
> +	{ .compatible = "renesas,usb-phy-r8a7790" },
> +	{ .compatible = "renesas,usb-phy-r8a7791" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen2_usb_phy_match_table);
> +#else
> +static inline struct rcar_gen2_phy_platform_data *
> +rcar_gen2_usb_phy_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int rcar_gen2_usb_phy_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -177,7 +209,10 @@ static int rcar_gen2_usb_phy_probe(struc
>  	struct clk *clk;
>  	int retval;
>  
> -	pdata = dev_get_platdata(dev);
> +	if (dev->of_node)
> +		pdata = rcar_gen2_usb_phy_parse_dt(dev);
> +	else
> +		pdata = dev_get_platdata(dev);
>  	if (!pdata) {
>  		dev_err(dev, "No platform data\n");
>  		return -EINVAL;
> @@ -236,6 +271,7 @@ static int rcar_gen2_usb_phy_remove(stru
>  static struct platform_driver rcar_gen2_usb_phy_driver = {
>  	.driver = {
>  		.name = "usb_phy_rcar_gen2",
> +		.of_match_table = of_match_ptr(rcar_gen2_usb_phy_match_table),
>  	},
>  	.probe = rcar_gen2_usb_phy_probe,
>  	.remove = rcar_gen2_usb_phy_remove,

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

Hi,

On Mon, Mar 03, 2014 at 08:50:33PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 03/03/2014 07:44 PM, Felipe Balbi wrote:
> 
> >>Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
> >>documenting the device tree binding as necessary.
> 
> >>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> >Unless someone on devicetree@vger gives me an ACK pretty soon, I'm
> >afraid this patch will miss v3.15.
> 
>    Ugh, the USB acceptance window is getting very short...

well, -rc5 is already out... I want to have at least 2 weeks of
"linux-next soakage" (yes, I just coined a new term heh). Since most
releases go up to -rc7, it's about time to close the tree ;-)

> >>---
> >>This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.
> >>
> >>Changes in version 2:
> >>- restored devm_clk_get() call and the error handling logic in the probe()
> >>   method, removed clk_put() call in the remove() method.
> 
> >>  Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
> >>  drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
> >>  2 files changed, 68 insertions(+), 3 deletions(-)
> 
> >>Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> >>===================================================================
> >>--- /dev/null
> >>+++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> >>@@ -0,0 +1,29 @@
> >>+* 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.
> >>+- clocks: clock phandle and specifier pair.
> >>+- clock-names: string, clock input name, must be "usbhs".
> >>+
> >>+Optional properties:
> >>+- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
> >>+			to PCI EHCI/OHCI; otherwise, it will be connected to the
> >>+			USBHS controller.
> >>+- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
> >>+			to PCI EHCI/OHCI; otherwise, it will be connected to the
> >>+			USBSS controller (xHCI).
> 
> >I wonder if these two properties should be taken care by pinctrl
> >framework instead.
> 
>    No, the internal port multiplexing is controlled by the PHY
> itself, see the driver source. The same way it was with USB port 1 of
> the generation 1 R-Car SoCs.

ok, fair enough.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Hello.

On 03/03/2014 07:44 PM, Felipe Balbi wrote:

>> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
>> documenting the device tree binding as necessary.

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

> Unless someone on devicetree@vger gives me an ACK pretty soon, I'm
> afraid this patch will miss v3.15.

    Ugh, the USB acceptance window is getting very short...

>> ---
>> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.
>>
>> Changes in version 2:
>> - restored devm_clk_get() call and the error handling logic in the probe()
>>    method, removed clk_put() call in the remove() method.

>>   Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
>>   drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
>>   2 files changed, 68 insertions(+), 3 deletions(-)

>> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>> ===================================================================
>> --- /dev/null
>> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>> @@ -0,0 +1,29 @@
>> +* 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.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".
>> +
>> +Optional properties:
>> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
>> +			to PCI EHCI/OHCI; otherwise, it will be connected to the
>> +			USBHS controller.
>> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
>> +			to PCI EHCI/OHCI; otherwise, it will be connected to the
>> +			USBSS controller (xHCI).

> I wonder if these two properties should be taken care by pinctrl
> framework instead.

    No, the internal port multiplexing is controlled by the PHY itself, see 
the driver source. The same way it was with USB port 1 of the generation 1 
R-Car SoCs.

WBR, Sergei

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Ben Dooks]

On 06/03/14 19:39, Sergei Shtylyov wrote:
> Hello.
>
> On 03/03/2014 07:44 PM, Felipe Balbi wrote:
>
>>> Add support of the device tree probing for the Renesas R-Car
>>> generation 2 SoCs
>>> documenting the device tree binding as necessary.
>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
>> Unless someone on devicetree@vger gives me an ACK pretty soon, I'm
>> afraid this patch will miss v3.15.
>
>     Mark, you've commented on v1 of the patch, I hope I cleared things
> up for you in my reply, how about an ACK (or at least new portion of
> comments)?

And whose patches are we going to use? :-p


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Hello.

On 03/03/2014 07:44 PM, Felipe Balbi wrote:

>> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
>> documenting the device tree binding as necessary.

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

> Unless someone on devicetree@vger gives me an ACK pretty soon, I'm
> afraid this patch will miss v3.15.

    Mark, you've commented on v1 of the patch, I hope I cleared things up for 
you in my reply, how about an ACK (or at least new portion of comments)?

WBR, Sergei

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Magnus Damm]

On Sat, Mar 1, 2014 at 10:07 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
> documenting the device tree binding as necessary.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.
>
> Changes in version 2:
> - restored devm_clk_get() call and the error handling logic in the probe()
>   method, removed clk_put() call in the remove() method.
>
>  Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
>  drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
>  2 files changed, 68 insertions(+), 3 deletions(-)
>
> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> ===================================================================
> --- /dev/null
> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
> @@ -0,0 +1,29 @@
> +* 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.
> +- clocks: clock phandle and specifier pair.
> +- clock-names: string, clock input name, must be "usbhs".
> +
> +Optional properties:
> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
> +                       to PCI EHCI/OHCI; otherwise, it will be connected to the
> +                       USBHS controller.
> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
> +                       to PCI EHCI/OHCI; otherwise, it will be connected to the
> +                       USBSS controller (xHCI).

Thanks for your efforts here, but this DT binding looks like a mix of
software configuration and hardware description. I would like to omit
the software configuration portion if possible and only describe the
hardware.

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

As an example, instead of relying on "renesas,channel0-pci" or
"renesas,channel2-pci" to specify which hard coded software
configuration you want please rework this to expose 3 separate
channels and use DT to point each host controller to the right
channel.

That will give us the opportunity to perform any kind of run time
selection and not be limited by software policy set by DT.

So this is a NAK on my side. I expect better.

Thanks,

/ magnus

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Hello.

On 07-03-2014 9:46, Magnus Damm wrote:

>> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
>> documenting the device tree binding as necessary.

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

>> ---
>> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.

>> Changes in version 2:
>> - restored devm_clk_get() call and the error handling logic in the probe()
>>    method, removed clk_put() call in the remove() method.

>>   Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
>>   drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42 ++++++++++++++--
>>   2 files changed, 68 insertions(+), 3 deletions(-)

>> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>> ===================================================================
>> --- /dev/null
>> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>> @@ -0,0 +1,29 @@
>> +* 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.
>> +- clocks: clock phandle and specifier pair.
>> +- clock-names: string, clock input name, must be "usbhs".
>> +
>> +Optional properties:
>> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected
>> +                       to PCI EHCI/OHCI; otherwise, it will be connected to the
>> +                       USBHS controller.
>> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected
>> +                       to PCI EHCI/OHCI; otherwise, it will be connected to the
>> +                       USBSS controller (xHCI).

> Thanks for your efforts here, but this DT binding looks like a mix of
> software configuration and hardware description.

    I disagree. I have already explained to Mark Rutland how selecting a 
particular controller for a particular USB port should depend on the kind of 
the USB connector used, i.e. the board hardware.

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

> As an example, instead of relying on "renesas,channel0-pci" or
> "renesas,channel2-pci" to specify which hard coded software
> configuration you want please rework this to expose 3 separate
> channels and use DT to point each host controller to the right
> channel.

    So you want to expose the channels as "virtual devices"? Or what do you 
want? Also, it seems almost impossible to represent some USB controllers in DT 
-- such as USBHS in particular.

> That will give us the opportunity to perform any kind of run time
> selection and not be limited by software policy set by DT.

    Sorry, but I don't see how using DT could help with run-time selection.
Could you please explain?

> So this is a NAK on my side. I expect better.

    Thanks for your long awaited feedback.

> Thanks,

> / magnus

WBR, Sergei

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Hello.

On 03/07/2014 05:27 PM, Sergei Shtylyov wrote:

>>> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
>>> documenting the device tree binding as necessary.

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

>>> ---
>>> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.

>>> Changes in version 2:
>>> - restored devm_clk_get() call and the error handling logic in the probe()
>>>    method, removed clk_put() call in the remove() method.

>>>   Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++++++++++
>>>   drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42
>>> ++++++++++++++--
>>>   2 files changed, 68 insertions(+), 3 deletions(-)

>>> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>> ===================================================================
>>> --- /dev/null
>>> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>> @@ -0,0 +1,29 @@
>>> +* 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.
>>> +- clocks: clock phandle and specifier pair.
>>> +- clock-names: string, clock input name, must be "usbhs".
>>> +
>>> +Optional properties:
>>> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be
>>> connected
>>> +                       to PCI EHCI/OHCI; otherwise, it will be connected
>>> to the
>>> +                       USBHS controller.
>>> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be
>>> connected
>>> +                       to PCI EHCI/OHCI; otherwise, it will be connected
>>> to the
>>> +                       USBSS controller (xHCI).

>> Thanks for your efforts here, but this DT binding looks like a mix of
>> software configuration and hardware description.

>     I disagree. I have already explained to Mark Rutland how selecting a
> particular controller for a particular USB port should depend on the kind of
> the USB connector used, i.e. the board hardware.

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

>> As an example, instead of relying on "renesas,channel0-pci" or
>> "renesas,channel2-pci" to specify which hard coded software
>> configuration you want please rework this to expose 3 separate
>> channels and use DT to point each host controller to the right
>> channel.

>     So you want to expose the channels as "virtual devices"? Or what do you
> want?

    OK, let's assume you want either "virtual" PHYs or channel subnodes in the 
PHY node we have now. Now what the PHY driver should do with all these 
phandles pointing from the controller nodes to the PHY nodes or the channel 
subnodes? Scan all device tree in search of the nodes having certain prop and 
get info from such nodes on how to program the channel MUXing register using 
their "compatible" props? Sorry, that doesn't appeal to me at all.
    Now if we got the phandles pointing from the PHY node back to the selected 
controllers, things would look somewhat better. Yet it still looks like an 
unnecessary complication of what I'm proposing and so doesn't appeal to me 
much either.
    So, what's your intent here (other than to make my life harder :-)?

> Also, it seems almost impossible to represent some USB controllers in DT
> -- such as USBHS in particular.

    I've spent several days staring at the various USBHS code and I'm going to 
send you a more detailed report on USBHS vs DT issues.

[...]

>> So this is a NAK on my side. I expect better.

>     Thanks for your long awaited feedback.

    I'm not convinced that what you're proposing is indeed better though.

>> Thanks,

>> / magnus

WBR, Sergei

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Magnus Damm]

Hi Sergei,

On Sat, Mar 8, 2014 at 8:57 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 03/07/2014 05:27 PM, Sergei Shtylyov wrote:
>
>>>> Add support of the device tree probing for the Renesas R-Car generation
>>>> 2 SoCs
>>>> documenting the device tree binding as necessary.
>
>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
>
>>>> ---
>>>> This patch is against the 'next' branch of Felipe Balbi's 'usb.git'
>>>> repo.
>
>
>>>> Changes in version 2:
>>>> - restored devm_clk_get() call and the error handling logic in the
>>>> probe()
>>>>    method, removed clk_put() call in the remove() method.
>
>
>>>>   Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29
>>>> +++++++++++
>>>>   drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42
>>>> ++++++++++++++--
>>>>   2 files changed, 68 insertions(+), 3 deletions(-)
>
>
>>>> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>>> @@ -0,0 +1,29 @@
>>>> +* 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.
>>>> +- clocks: clock phandle and specifier pair.
>>>> +- clock-names: string, clock input name, must be "usbhs".
>>>> +
>>>> +Optional properties:
>>>> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be
>>>> connected
>>>> +                       to PCI EHCI/OHCI; otherwise, it will be
>>>> connected
>>>> to the
>>>> +                       USBHS controller.
>>>> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be
>>>> connected
>>>> +                       to PCI EHCI/OHCI; otherwise, it will be
>>>> connected
>>>> to the
>>>> +                       USBSS controller (xHCI).
>
>
>>> Thanks for your efforts here, but this DT binding looks like a mix of
>>> software configuration and hardware description.
>
>
>>     I disagree. I have already explained to Mark Rutland how selecting a
>> particular controller for a particular USB port should depend on the kind
>> of
>> the USB connector used, i.e. the board hardware.

Just to clarify, I'm talking about the selection of USB IP that is
happening inside the SoC. You are right that there are also board
specific dependencies, but that's a different issue. As an example, on
Lager it is possible to use either USB 2.0 Host device functionality
or USB 3.0 Host functionality using the same USB 3.0-compatible
connector that is being exposed by the Lager board. In this case the
board needs to describe that it has a USB3.0-compatbile connector on
r8a7790 USB2, while the r8a7790 PHY driver and USB Host controller
glue needs to represent which internal devices that are mapped to
USB2.

I imagine that there are board-specific DT bits for Lager that
describe USB2 as USB3.0 compatible. Then there are r8a7790 specific
bits that show how the USB2.0 host controller and the USB3.0 host
controller both point to the USB2 channel included in the USB PHY. The
last portion is not modified by the board-level integrator, only the
board specific bits.

>>>> +Example (Lager board):
>>>> +
>>>> +       usb-phy@e6590100 {
>>>> +               compatible = "renesas,usb-phy-r8a7790";
>>>> +               reg = <0 0xe6590100 0 0x100>;
>>>> +               clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>>>> +               clock-names = "usbhs";
>>>> +               renesas,channel2-pci;
>>>> +       };
>
>
>>> As an example, instead of relying on "renesas,channel0-pci" or
>>> "renesas,channel2-pci" to specify which hard coded software
>>> configuration you want please rework this to expose 3 separate
>>> channels and use DT to point each host controller to the right
>>> channel.
>
>
>>     So you want to expose the channels as "virtual devices"? Or what do
>> you
>> want?

I'm not sure what "virtual devices" would be, but channel sub nodes
sounds like one possibility.

>    OK, let's assume you want either "virtual" PHYs or channel subnodes in
> the PHY node we have now. Now what the PHY driver should do with all these
> phandles pointing from the controller nodes to the PHY nodes or the channel
> subnodes? Scan all device tree in search of the nodes having certain prop
> and get info from such nodes on how to program the channel MUXing register
> using their "compatible" props? Sorry, that doesn't appeal to me at all.

I can't say that I'm 100% sure what is the best solution for this, but
I'm quite OK with basically anything that describes the hardware
mapping between PHY ports and host controllers in a sane way.

Exactly how to represent this in a sane way is however not trivial.
But how fun would that be? =)

One example from my side (which may be bad, not sure) is to point from
each USB Host controller to a subnode in the USB PHY device. But then
you will have to scan all though devices to find the link which may
not be what you want.

>    Now if we got the phandles pointing from the PHY node back to the
> selected controllers, things would look somewhat better. Yet it still looks
> like an unnecessary complication of what I'm proposing and so doesn't appeal
> to me much either.

So my other example is probably same as your proposal above, which
would be to point all USB Host controllers to the same USB PHY, but as
DT properties in the PHY device have one list per USB port that points
back to all possible USB Host controllers. That would make scanning
easier.

>    So, what's your intent here (other than to make my life harder :-)?

Of course, only to make your life harder. =)

My goal is to describe the hardware connections between the PHY ports
and the USB Host controllers so we eventually in software would be
able to switch between USBHS for Function (or maybe Host) and USB PCI
for USB Host on a board where the hardware allows us to do so. I want
to be able to over time improve the kernel and do this without having
to modify the DTS. Actually, I'd like the DTS to show us how we can
switch the hardware.

This is quite different from hard coding one connection in the DTS.

>> Also, it seems almost impossible to represent some USB controllers in DT
>> -- such as USBHS in particular.

I'm not sure if impossible is the word I would pick, but it is indeed
difficult. My opinion is that we cannot rush this kind of development.
It is fine to take shortcuts with platform devices, but with DT we
want to get it right.

>    I've spent several days staring at the various USBHS code and I'm going
> to send you a more detailed report on USBHS vs DT issues.

Sure, if that makes you happy, but I'm more interested in patches myself. =)

>>> So this is a NAK on my side. I expect better.
>
>>     Thanks for your long awaited feedback.
>
>    I'm not convinced that what you're proposing is indeed better though.

Well, I'm looking forward to a better proposal then! =)

Thanks,

/ magnus

Re: [PATCH v2] phy-rcar-gen2-usb: add device tree support

[Sergei Shtylyov]

Hello.

On 03/10/2014 02:00 PM, Magnus Damm wrote:

>>>>> Add support of the device tree probing for the Renesas R-Car generation
>>>>> 2 SoCs
>>>>> documenting the device tree binding as necessary.

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

>>>>> ---
>>>>> This patch is against the 'next' branch of Felipe Balbi's 'usb.git'
>>>>> repo.

>>>>> Changes in version 2:
>>>>> - restored devm_clk_get() call and the error handling logic in the
>>>>> probe()
>>>>>     method, removed clk_put() call in the remove() method.

>>>>>    Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29
>>>>> +++++++++++
>>>>>    drivers/usb/phy/phy-rcar-gen2-usb.c                     |   42
>>>>> ++++++++++++++--
>>>>>    2 files changed, 68 insertions(+), 3 deletions(-)

>>>>> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>>>> ===================================================================
>>>>> --- /dev/null
>>>>> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
>>>>> @@ -0,0 +1,29 @@
>>>>> +* 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.
>>>>> +- clocks: clock phandle and specifier pair.
>>>>> +- clock-names: string, clock input name, must be "usbhs".
>>>>> +
>>>>> +Optional properties:
>>>>> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be
>>>>> connected
>>>>> +                       to PCI EHCI/OHCI; otherwise, it will be
>>>>> connected
>>>>> to the
>>>>> +                       USBHS controller.
>>>>> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be
>>>>> connected
>>>>> +                       to PCI EHCI/OHCI; otherwise, it will be
>>>>> connected
>>>>> to the
>>>>> +                       USBSS controller (xHCI).

>>>> Thanks for your efforts here, but this DT binding looks like a mix of
>>>> software configuration and hardware description.

>>>      I disagree. I have already explained to Mark Rutland how selecting a
>>> particular controller for a particular USB port should depend on the kind
>>> of
>>> the USB connector used, i.e. the board hardware.

> Just to clarify, I'm talking about the selection of USB IP that is
> happening inside the SoC.

    I'm talking about the same thing.

> You are right that there are also board
> specific dependencies, but that's a different issue.

    I don't really think so.

> As an example, on
> Lager it is possible to use either USB 2.0 Host device functionality
> or USB 3.0 Host functionality using the same USB 3.0-compatible
> connector that is being exposed by the Lager board.

    Note that USB 3.0 controller (xHCI) knows how to talk USB 2.0 and 1.1 
protocols, and I see little sense enabling EHCI/OHCI for USB 3.0 port (as it's 
done now), unless the xHCI hardware/driver is very buggy.

> In this case the
> board needs to describe that it has a USB3.0-compatbile connector on

    Hm, I think we'll then be the first trying to describe the board connector 
types in the device tree, at least I can't remember any example off the top of 
my head.

> r8a7790 USB2, while the r8a7790 PHY driver and USB Host controller
> glue needs to represent which internal devices that are mapped to
> USB2.

> I imagine that there are board-specific DT bits for Lager that
> describe USB2 as USB3.0 compatible. Then there are r8a7790 specific
> bits that show how the USB2.0 host controller and the USB3.0 host
> controller both point to the USB2 channel included in the USB PHY. The
> last portion is not modified by the board-level integrator, only the
> board specific bits.

    So, those "board specific bits" would be considered the ultimate source of 
information of how to program USB channel MUXes, right? Or you mean to only 
provide the USB connector info there?
    My main concern is where the data to program MUXes would be placed and, 
maybe more importantly, in what way they would be conveyed to the PHY driver, 
while you seem mainly concerning about the topology representation.

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

>>>> As an example, instead of relying on "renesas,channel0-pci" or
>>>> "renesas,channel2-pci" to specify which hard coded software
>>>> configuration you want please rework this to expose 3 separate
>>>> channels and use DT to point each host controller to the right
>>>> channel.

>>>      So you want to expose the channels as "virtual devices"? Or what do
>>> you want?

> I'm not sure what "virtual devices" would be,

    Devices without the "reg" property. By the way, from the manuals it didn't 
became crystal clear to me whether USBHS internal PHY (that is programmable 
and controls USB0/2 channel MUX among other things) and USB 2.0/3.0 PHY (that 
is not programmable but USB 3.0 part of which can be used alternatively as 
SATA0 PHY) is even one and the same thing. The PHY registers we control thru 
the dedicated PHY driver are documented along with other USBHS registers, so 
one might have an impression this PHY is dedicated to USBHS only... If this is 
not so, the USB PHY design and the way it's documented seems quite 
questionable to me.

> but channel sub nodes sounds like one possibility.

    OK.

>>     OK, let's assume you want either "virtual" PHYs or channel subnodes in
>> the PHY node we have now. Now what the PHY driver should do with all these
>> phandles pointing from the controller nodes to the PHY nodes or the channel
>> subnodes? Scan all device tree in search of the nodes having certain prop
>> and get info from such nodes on how to program the channel MUXing register
>> using their "compatible" props? Sorry, that doesn't appeal to me at all.

> I can't say that I'm 100% sure what is the best solution for this, but
> I'm quite OK with basically anything that describes the hardware
> mapping between PHY ports and host controllers in a sane way.

> Exactly how to represent this in a sane way is however not trivial.
> But how fun would that be? =)

    I'm afraid I'm not a huge fan of device trees (although I had to acquaint 
myself with them back in 2006, my first experience wasn't quite successful), 
so the fun for me seems somewhat dubious... I'm of the opinion that plain old 
platform devices are far more flexible than DT, and seeing DT more as a 
necessary evil used to bring the ARM kernel to the more reasonable sizes
and follow the x86 suit with the multiplatform builds...

> One example from my side (which may be bad, not sure) is to point from
> each USB Host controller to a subnode in the USB PHY device. But then
> you will have to scan all though devices to find the link which may
> not be what you want.

    Yes, I would like to avoid that.

>>     Now if we got the phandles pointing from the PHY node back to the
>> selected controllers, things would look somewhat better. Yet it still looks
>> like an unnecessary complication of what I'm proposing and so doesn't appeal
>> to me much either.

> So my other example is probably same as your proposal above, which
> would be to point all USB Host controllers to the same USB PHY, but as
> DT properties in the PHY device have one list per USB port that points
> back to all possible USB Host controllers. That would make scanning
> easier.

    OK...

>>     So, what's your intent here (other than to make my life harder :-)?

> Of course, only to make your life harder. =)

    Well, so far you seem to succeed. :-)

> My goal is to describe the hardware connections between the PHY ports
> and the USB Host controllers so we eventually in software would be
> able to switch between USBHS for Function (or maybe Host) and USB PCI
> for USB Host on a board where the hardware allows us to do so. I want
> to be able to over time improve the kernel and do this without having
> to modify the DTS.

    Not sure I understand what you're imagining here...

> Actually, I'd like the DTS to show us how we can
> switch the hardware.

    That's clear. I'm only not sure we really need that information in the 
device tree in order to switch the hardware. Oh well...

> This is quite different from hard coding one connection in the DTS.

    Well, having such limited platform data, I had little choice but mimic it 
in DT as it's usually done, at least for a first try...

>>> Also, it seems almost impossible to represent some USB controllers in DT
>>> -- such as USBHS in particular.

> I'm not sure if impossible is the word I would pick, but it is indeed
> difficult.

    Let's agree on "very difficult". :-)

> My opinion is that we cannot rush this kind of development.
> It is fine to take shortcuts with platform devices, but with DT we
> want to get it right.

    OK... but that makes the PHY DT work a matter of quite distant future, 
doesn't it? I mean unless we use much avoided so far auxdata thing for USBHS.

>>     I've spent several days staring at the various USBHS code and I'm going
>> to send you a more detailed report on USBHS vs DT issues.

> Sure, if that makes you happy, but I'm more interested in patches myself. =)

    No, it won't, so let's skip it.

>>>> So this is a NAK on my side. I expect better.

>>>      Thanks for your long awaited feedback.

>>     I'm not convinced that what you're proposing is indeed better though.

> Well, I'm looking forward to a better proposal then! =)

    Unfortunately, I'm lacking your imagination here. :-)

> Thanks,

> / magnus

WBR, Sergei