[PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[achew]

From: Andrew Chew <achew@nvidia.com>

Add code to try to get platform data information (register base, irq,
modes, various tuning parameters) from device tree, if not present in board
files.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 .../devicetree/bindings/usb/tegra20-ehci.txt       |   28 +++
 drivers/usb/host/ehci-tegra.c                      |  221 ++++++++++++++++++++
 2 files changed, 249 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/tegra20-ehci.txt

diff --git a/Documentation/devicetree/bindings/usb/tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
new file mode 100644
index 0000000..5a73a96
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
@@ -0,0 +1,28 @@
+NVIDIA Tegra20 SOC USB controllers
+
+The device node for a USB controller that is part of a Tegra20
+SOC is as described in the document "Open Firmware Recommended
+Practice: Universal Serial Bus" with the following modifications
+and additions:
+
+Required properties:
+ - compatible: Should be "nvidia,tegra20-ehci".
+ - mode: Should be one of "device", "host", or "otg".
+ - power_down_on_bus_suspend: For host mode only, should be <1> if you
+   want the USB phy to power down when the host is suspended.  Else, it
+   should be <0>.
+ - type: Should be one of "utmi" or "ulpi".
+
+Required properties for type = "utmi".  These values are derived from
+characterization by system engineering.
+ - hssync_start_delay
+ - idle_wait_delay
+ - elastic_limit
+ - term_range_adj
+ - xcvr_setup
+ - xcvr_lsfslew
+ - xcvr_lsrslew
+
+Required properties for type = "ulpi":
+ - reset_gpio: The GPIO used to drive reset
+ - clk
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 02b2bfd..89144d5 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -21,10 +21,20 @@
 #include <linux/platform_data/tegra_usb.h>
 #include <linux/irq.h>
 #include <linux/usb/otg.h>
+
+#if defined(CONFIG_OF)
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#endif
+
 #include <mach/usb_phy.h>
 
 #define TEGRA_USB_DMA_ALIGN 32
 
+static u64 tegra_ehci_dmamask = DMA_BIT_MASK(TEGRA_USB_DMA_ALIGN);
+
 struct tegra_ehci_hcd {
 	struct ehci_hcd *ehci;
 	struct tegra_usb_phy *phy;
@@ -574,9 +584,181 @@ static const struct hc_driver tegra_ehci_hc_driver = {
 	.port_handed_over	= ehci_port_handed_over,
 };
 
+#if defined(CONFIG_OF)
+static int tegra_ehci_parse_dt_node_utmi(struct device_node *dn,
+					 struct platform_device *pdev,
+					 struct tegra_ehci_platform_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	struct tegra_utmip_config *phy_config;
+	int retval;
+
+	phy_config = devm_kzalloc(dev, sizeof(struct tegra_utmip_config),
+				  GFP_KERNEL);
+	if (!phy_config)
+		return -ENOMEM;
+
+	retval = of_property_read_u32(dn, "hssync_start_delay",
+				      (u32 *)&phy_config->hssync_start_delay);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"hssync_start_delay\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "elastic_limit",
+				      (u32 *)&phy_config->elastic_limit);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"elastic_limit\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "idle_wait_delay",
+				      (u32 *)&phy_config->idle_wait_delay);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"idle_wait_delay\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "term_range_adj",
+				      (u32 *)&phy_config->term_range_adj);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"term_range_adj\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "xcvr_setup",
+				      (u32 *)&phy_config->xcvr_setup);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"xcvr_setup\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "xcvr_lsfslew",
+				      (u32 *)&phy_config->xcvr_lsfslew);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"xcvr_lsfslew\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_u32(dn, "xcvr_lsrslew",
+				      (u32 *)&phy_config->xcvr_lsrslew);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"xcvr_lsrslew\"\n");
+		return retval;
+	}
+
+	pdata->phy_config = phy_config;
+
+	return 0;
+}
+
+static int tegra_ehci_parse_dt_node_ulpi(struct device_node *dn,
+					 struct platform_device *pdev,
+					 struct tegra_ehci_platform_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	struct tegra_ulpi_config *phy_config;
+	int retval;
+
+	phy_config = devm_kzalloc(dev, sizeof(struct tegra_ulpi_config),
+				  GFP_KERNEL);
+	if (!phy_config)
+		return -ENOMEM;
+
+	retval = of_property_read_u32(dn, "reset_gpio",
+				      &phy_config->reset_gpio);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"reset_gpio\"\n");
+		return retval;
+	}
+
+	retval = of_property_read_string(dn, "clk",
+					 (char **)&phy_config->clk);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"clk\"\n");
+		return retval;
+	}
+
+	pdata->phy_config = phy_config;
+
+	return 0;
+}
+
+static struct tegra_ehci_platform_data *
+tegra_ehci_parse_dt_node(struct device_node *dn,
+			 struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tegra_ehci_platform_data *pdata;
+	char *mode;
+	char *type;
+	int retval;
+
+	pdata = devm_kzalloc(dev, sizeof(struct tegra_ehci_platform_data),
+			     GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	retval = of_property_read_string(dn, "mode", &mode);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"mode\"\n");
+		goto fail;
+	}
+
+	if (strcmp(mode, "device") == 0)
+		pdata->operating_mode = TEGRA_USB_DEVICE;
+	else if (strcmp(mode, "host") == 0)
+		pdata->operating_mode = TEGRA_USB_HOST;
+	else if (strcmp(mode, "otg") == 0)
+		pdata->operating_mode = TEGRA_USB_OTG;
+	else {
+		dev_err(dev, "Invalid dt property \"mode\" value %s\n", mode);
+		goto fail;
+	}
+
+	retval = of_property_read_u32(dn, "power_down_on_bus_suspend",
+				      &pdata->power_down_on_bus_suspend);
+	if (retval) {
+		dev_err(dev, "Missing dt property "
+			     "\"power_down_on_bus_suspend\"\n");
+		goto fail;
+	}
+
+	retval = of_property_read_string(dn, "type", &type);
+	if (retval) {
+		dev_err(dev, "Missing dt property \"type\"\n");
+		goto fail;
+	}
+
+	if (strcmp(type, "utmi") == 0) {
+		retval = tegra_ehci_parse_dt_node_utmi(dn, pdev, pdata);
+		if (retval)
+			goto fail;
+	} else if (strcmp(type, "ulpi") == 0) {
+		retval = tegra_ehci_parse_dt_node_ulpi(dn, pdev, pdata);
+		if (retval)
+			goto fail;
+	} else {
+		dev_err(dev, "Invalid dt property \"type\" value %s\n", type);
+		goto fail;
+	}
+
+	return pdata;
+
+fail:
+	devm_kfree(dev, pdata);
+
+	return NULL;
+}
+#endif
+
 static int tegra_ehci_probe(struct platform_device *pdev)
 {
 	struct resource *res;
+#if defined(CONFIG_OF)
+	struct device_node *dn = pdev->dev.of_node;
+	struct resource of_res;
+#endif
 	struct usb_hcd *hcd;
 	struct tegra_ehci_hcd *tegra;
 	struct tegra_ehci_platform_data *pdata;
@@ -584,7 +766,20 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	int irq;
 	int instance = pdev->id;
 
+	/*
+	 * See if there's any platform data passed via board files.
+	 * If there isn't, then allocate one and fill it by parsing
+	 * device tree node.
+	 */
 	pdata = pdev->dev.platform_data;
+#if defined(CONFIG_OF)
+	if (!pdata) {
+		pdata = tegra_ehci_parse_dt_node(dn, pdev);
+
+		pdev->dev.dma_mask = &tegra_ehci_dmamask;
+		pdev->dev.coherent_dma_mask = tegra_ehci_dmamask;
+	}
+#endif
 	if (!pdata) {
 		dev_err(&pdev->dev, "Platform data missing\n");
 		return -EINVAL;
@@ -625,7 +820,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	clk_enable(tegra->emc_clk);
 	clk_set_rate(tegra->emc_clk, 400000000);
 
+	/*
+	 * If there isn't an IORESOURCE_MEM defined in the board file,
+	 * then try to get that information from the device tree node.
+	 */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+#if defined(CONFIG_OF)
+	if (!res && (of_address_to_resource(dn, 0, &of_res) == 0))
+		res = &of_res;
+#endif
 	if (!res) {
 		dev_err(&pdev->dev, "Failed to get I/O memory\n");
 		err = -ENXIO;
@@ -658,7 +861,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	tegra->power_down_on_bus_suspend = pdata->power_down_on_bus_suspend;
 	tegra->ehci = hcd_to_ehci(hcd);
 
+	/*
+	 * If there isn't an irq defined in the board file, then try to get
+	 * that information from the device tree node.
+	 */
 	irq = platform_get_irq(pdev, 0);
+#if defined(CONFIG_OF)
+	if (!irq)
+		irq = irq_of_parse_and_map(dn, 0);
+#endif
 	if (!irq) {
 		dev_err(&pdev->dev, "Failed to get IRQ\n");
 		err = -ENODEV;
@@ -773,6 +984,14 @@ static void tegra_ehci_hcd_shutdown(struct platform_device *pdev)
 		hcd->driver->shutdown(hcd);
 }
 
+static const struct of_device_id tegra_ehci_of_match[] = {
+	{
+		.compatible = "nvidia,tegra20-ehci",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, tegra_ehci_of_match);
+
 static struct platform_driver tegra_ehci_driver = {
 	.probe		= tegra_ehci_probe,
 	.remove		= tegra_ehci_remove,
@@ -783,5 +1002,7 @@ static struct platform_driver tegra_ehci_driver = {
 	.shutdown	= tegra_ehci_hcd_shutdown,
 	.driver		= {
 		.name	= "tegra-ehci",
+		.owner	= THIS_MODULE,
+		.of_match_table = tegra_ehci_of_match,
 	}
 };
-- 
1.7.6

[PATCH 2/3 v2] dt: tegra20: Add ehci host controller nodes.

[achew]

From: Andrew Chew <achew@nvidia.com>

These values were derived from various headers in arch/arm/mach-tegra.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 arch/arm/boot/dts/tegra20.dtsi |   43 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 83fedf3..d4ab1ab 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -160,5 +160,48 @@
 		interrupts = < 63 >;
 		status = "disabled";
 	};
+
+	ehci@c5000000 {
+		compatible = "nvidia,tegra20-ehci";
+		reg = <0xc5000000 0x4000>;
+		interrupts = < 52 >;
+		mode = "host";
+		power_down_on_bus_suspend = < 1 >;
+		type = "utmi";
+		hssync_start_delay = < 9 >;
+		idle_wait_delay = < 17 >;
+		elastic_limit = < 16 >;
+		term_range_adj = < 6 >;
+		xcvr_setup = < 9 >;
+		xcvr_lsfslew = < 1 >;
+		xcvr_lsrslew = < 1 >;
+	};
+
+	ehci@c5004000 {
+		compatible = "nvidia,tegra20-ehci";
+		reg = <0xc5004000 0x4000>;
+		interrupts = < 53 >;
+		mode = "host";
+		power_down_on_bus_suspend = < 1 >;
+		type = "ulpi";
+		reset_gpio = < 169 >;
+		clk = "cdev2";
+	};
+
+	ehci@c5008000 {
+		compatible = "nvidia,tegra20-ehci";
+		reg = <0xc5008000 0x4000>;
+		interrupts = < 129 >;
+		mode = "host";
+		power_down_on_bus_suspend = < 1 >;
+		type = "utmi";
+		hssync_start_delay = < 9 >;
+		idle_wait_delay = < 17 >;
+		elastic_limit = < 16 >;
+		term_range_adj = < 6 >;
+		xcvr_setup = < 9 >;
+		xcvr_lsfslew = < 2 >;
+		xcvr_lsrslew = < 2 >;
+	};
 };
 
-- 
1.7.6

[PATCH 3/3 v2] dt: tegra20: Add ehci overrides to Seaboard.

[achew]

From: Andrew Chew <achew@nvidia.com>

Seaboard has different values for some of the utmi properties.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 arch/arm/boot/dts/tegra-seaboard.dts |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
index 5a99b4c..f8737ed 100644
--- a/arch/arm/boot/dts/tegra-seaboard.dts
+++ b/arch/arm/boot/dts/tegra-seaboard.dts
@@ -31,4 +31,16 @@
 	sdhci@c8000600 {
 		status = "ok";
 	};
+
+	ehci@c5000000 {
+		hssync_start_delay = < 0 >;
+		xcvr_setup = < 15 >;
+		xcvr_lsfslew = < 2 >;
+		xcvr_lsrslew = < 2 >;
+	};
+
+	ehci@c5008000 {
+		hssync_start_delay = < 0 >;
+		xcvr_setup = < 8 >;
+	};
 };
-- 
1.7.6

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Stephen Warren]

achew@nvidia.com wrote at Wednesday, July 20, 2011 1:38 PM:
> From: Andrew Chew <achew@nvidia.com>
> 
> Add code to try to get platform data information (register base, irq,
> modes, various tuning parameters) from device tree, if not present in board
> files.
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>

> +++ b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
...
> +Required properties for type = "ulpi":
> + - reset_gpio: The GPIO used to drive reset
> + - clk

We should probably document what this is. I know you're finding out more
about this internally.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> +	retval = of_property_read_u32(dn, "power_down_on_bus_suspend",
> +				      &pdata->power_down_on_bus_suspend);

Sorry I missed this before, but perhaps we should skip reading that
property unless mode=="host"?

Otherwise,
Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

RE: [PATCH 2/3 v2] dt: tegra20: Add ehci host controller nodes.

[Stephen Warren]

achew@nvidia.com wrote at Wednesday, July 20, 2011 1:38 PM:
> These values were derived from various headers in arch/arm/mach-tegra.
> 
> Signed-off-by: Andrew Chew <achew@nvidia.com>

Patches 2 and 3:

Acked-by: Stephen Warren <swarren@nvidia.com>

I know Olof had suggested splitting the phy properties into a sub-node
of the EHCI controller. I'll let you and him discuss that; I don't
really have an opinion on that.

-- 
nvpublic

Re: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Grant Likely]

On Wed, Jul 20, 2011 at 1:38 PM,  <achew@nvidia.com> wrote:
> From: Andrew Chew <achew@nvidia.com>
>
> Add code to try to get platform data information (register base, irq,
> modes, various tuning parameters) from device tree, if not present in board
> files.
>
> Signed-off-by: Andrew Chew <achew@nvidia.com>
> ---
>  .../devicetree/bindings/usb/tegra20-ehci.txt       |   28 +++
>  drivers/usb/host/ehci-tegra.c                      |  221 ++++++++++++++++++++
>  2 files changed, 249 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/tegra20-ehci.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
> new file mode 100644
> index 0000000..5a73a96
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/tegra20-ehci.txt
> @@ -0,0 +1,28 @@
> +NVIDIA Tegra20 SOC USB controllers
> +
> +The device node for a USB controller that is part of a Tegra20
> +SOC is as described in the document "Open Firmware Recommended
> +Practice: Universal Serial Bus" with the following modifications
> +and additions:
> +
> +Required properties:
> + - compatible: Should be "nvidia,tegra20-ehci".
> + - mode: Should be one of "device", "host", or "otg".
> + - power_down_on_bus_suspend: For host mode only, should be <1> if you
> +   want the USB phy to power down when the host is suspended.  Else, it
> +   should be <0>.

We use '-' in property names, not '_'

Boolean values like this should be based on whether or not the
property is present.  ie. default to not powering down the phy unless
an empty "power-down-on-suspend" property is present.

> + - type: Should be one of "utmi" or "ulpi".

phy-type perhaps?

> +
> +Required properties for type = "utmi".  These values are derived from
> +characterization by system engineering.
> + - hssync_start_delay
> + - idle_wait_delay
> + - elastic_limit
> + - term_range_adj
> + - xcvr_setup
> + - xcvr_lsfslew
> + - xcvr_lsrslew

All of these custom properties should be prefixed with "nvidia," to
avoid namespace collisions.

> +
> +Required properties for type = "ulpi":
> + - reset_gpio: The GPIO used to drive reset
> + - clk
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 02b2bfd..89144d5 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -21,10 +21,20 @@
>  #include <linux/platform_data/tegra_usb.h>
>  #include <linux/irq.h>
>  #include <linux/usb/otg.h>
> +
> +#if defined(CONFIG_OF)
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#endif
> +

linux/of* headers are always safe to include.  Don't separate them
from the rest of the #includes.

>  #include <mach/usb_phy.h>
>
>  #define TEGRA_USB_DMA_ALIGN 32
>
> +static u64 tegra_ehci_dmamask = DMA_BIT_MASK(TEGRA_USB_DMA_ALIGN);
> +
>  struct tegra_ehci_hcd {
>        struct ehci_hcd *ehci;
>        struct tegra_usb_phy *phy;
> @@ -574,9 +584,181 @@ static const struct hc_driver tegra_ehci_hc_driver = {
>        .port_handed_over       = ehci_port_handed_over,
>  };
>
> +#if defined(CONFIG_OF)
> +static int tegra_ehci_parse_dt_node_utmi(struct device_node *dn,
> +                                        struct platform_device *pdev,
> +                                        struct tegra_ehci_platform_data *pdata)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_utmip_config *phy_config;
> +       int retval;
> +
> +       phy_config = devm_kzalloc(dev, sizeof(struct tegra_utmip_config),
> +                                 GFP_KERNEL);
> +       if (!phy_config)
> +               return -ENOMEM;
> +
> +       retval = of_property_read_u32(dn, "hssync_start_delay",
> +                                     (u32 *)&phy_config->hssync_start_delay);

Gah!  Don't cast pointers that way.  It will break badly if the sizes
don't match.  The variable *must* be a u32.

> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"hssync_start_delay\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "elastic_limit",
> +                                     (u32 *)&phy_config->elastic_limit);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"elastic_limit\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "idle_wait_delay",
> +                                     (u32 *)&phy_config->idle_wait_delay);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"idle_wait_delay\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "term_range_adj",
> +                                     (u32 *)&phy_config->term_range_adj);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"term_range_adj\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_setup",
> +                                     (u32 *)&phy_config->xcvr_setup);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_setup\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_lsfslew",
> +                                     (u32 *)&phy_config->xcvr_lsfslew);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_lsfslew\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_u32(dn, "xcvr_lsrslew",
> +                                     (u32 *)&phy_config->xcvr_lsrslew);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"xcvr_lsrslew\"\n");
> +               return retval;
> +       }

Can the driver use sane defaults for any of these values?  This patch
will be a lot smaller if there isn't the need to check all the return
values each time.

> +
> +       pdata->phy_config = phy_config;
> +
> +       return 0;
> +}
> +
> +static int tegra_ehci_parse_dt_node_ulpi(struct device_node *dn,
> +                                        struct platform_device *pdev,
> +                                        struct tegra_ehci_platform_data *pdata)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_ulpi_config *phy_config;
> +       int retval;
> +
> +       phy_config = devm_kzalloc(dev, sizeof(struct tegra_ulpi_config),
> +                                 GFP_KERNEL);
> +       if (!phy_config)
> +               return -ENOMEM;
> +
> +       retval = of_property_read_u32(dn, "reset_gpio",
> +                                     &phy_config->reset_gpio);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"reset_gpio\"\n");
> +               return retval;
> +       }
> +
> +       retval = of_property_read_string(dn, "clk",
> +                                        (char **)&phy_config->clk);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"clk\"\n");
> +               return retval;
> +       }
> +
> +       pdata->phy_config = phy_config;
> +
> +       return 0;
> +}
> +
> +static struct tegra_ehci_platform_data *
> +tegra_ehci_parse_dt_node(struct device_node *dn,
> +                        struct platform_device *pdev)

Nit: to be friendly for grepping, function name should remain on same
line as return value and annotation.  Keep line breaks in the
parameter list.

> +{
> +       struct device *dev = &pdev->dev;
> +       struct tegra_ehci_platform_data *pdata;
> +       char *mode;
> +       char *type;
> +       int retval;
> +
> +       pdata = devm_kzalloc(dev, sizeof(struct tegra_ehci_platform_data),
> +                            GFP_KERNEL);
> +       if (!pdata)
> +               return NULL;
> +
> +       retval = of_property_read_string(dn, "mode", &mode);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"mode\"\n");
> +               goto fail;
> +       }
> +
> +       if (strcmp(mode, "device") == 0)
> +               pdata->operating_mode = TEGRA_USB_DEVICE;
> +       else if (strcmp(mode, "host") == 0)
> +               pdata->operating_mode = TEGRA_USB_HOST;
> +       else if (strcmp(mode, "otg") == 0)
> +               pdata->operating_mode = TEGRA_USB_OTG;
> +       else {
> +               dev_err(dev, "Invalid dt property \"mode\" value %s\n", mode);
> +               goto fail;
> +       }
> +
> +       retval = of_property_read_u32(dn, "power_down_on_bus_suspend",
> +                                     &pdata->power_down_on_bus_suspend);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property "
> +                            "\"power_down_on_bus_suspend\"\n");
> +               goto fail;
> +       }
> +
> +       retval = of_property_read_string(dn, "type", &type);
> +       if (retval) {
> +               dev_err(dev, "Missing dt property \"type\"\n");
> +               goto fail;
> +       }
> +
> +       if (strcmp(type, "utmi") == 0) {
> +               retval = tegra_ehci_parse_dt_node_utmi(dn, pdev, pdata);
> +               if (retval)
> +                       goto fail;
> +       } else if (strcmp(type, "ulpi") == 0) {
> +               retval = tegra_ehci_parse_dt_node_ulpi(dn, pdev, pdata);
> +               if (retval)
> +                       goto fail;
> +       } else {
> +               dev_err(dev, "Invalid dt property \"type\" value %s\n", type);
> +               goto fail;
> +       }
> +
> +       return pdata;
> +
> +fail:
> +       devm_kfree(dev, pdata);
> +
> +       return NULL;
> +}
> +#endif
> +
>  static int tegra_ehci_probe(struct platform_device *pdev)
>  {
>        struct resource *res;
> +#if defined(CONFIG_OF)
> +       struct device_node *dn = pdev->dev.of_node;
> +       struct resource of_res;
> +#endif
>        struct usb_hcd *hcd;
>        struct tegra_ehci_hcd *tegra;
>        struct tegra_ehci_platform_data *pdata;
> @@ -584,7 +766,20 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        int irq;
>        int instance = pdev->id;
>
> +       /*
> +        * See if there's any platform data passed via board files.
> +        * If there isn't, then allocate one and fill it by parsing
> +        * device tree node.
> +        */
>        pdata = pdev->dev.platform_data;
> +#if defined(CONFIG_OF)
> +       if (!pdata) {
> +               pdata = tegra_ehci_parse_dt_node(dn, pdev);
> +
> +               pdev->dev.dma_mask = &tegra_ehci_dmamask;
> +               pdev->dev.coherent_dma_mask = tegra_ehci_dmamask;
> +       }
> +#endif

Drop the #if/#endif protection, and create an empty version of
tegra_ehci_parse_dt_node() when CONFIG_OF is not selected.  It will
make the code cleaner.

>        if (!pdata) {
>                dev_err(&pdev->dev, "Platform data missing\n");
>                return -EINVAL;
> @@ -625,7 +820,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        clk_enable(tegra->emc_clk);
>        clk_set_rate(tegra->emc_clk, 400000000);
>
> +       /*
> +        * If there isn't an IORESOURCE_MEM defined in the board file,
> +        * then try to get that information from the device tree node.
> +        */
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +#if defined(CONFIG_OF)
> +       if (!res && (of_address_to_resource(dn, 0, &of_res) == 0))
> +               res = &of_res;
> +#endif

Unnecessary.  The register ranges are populated into the resource
table by default.

>        if (!res) {
>                dev_err(&pdev->dev, "Failed to get I/O memory\n");
>                err = -ENXIO;
> @@ -658,7 +861,15 @@ static int tegra_ehci_probe(struct platform_device *pdev)
>        tegra->power_down_on_bus_suspend = pdata->power_down_on_bus_suspend;
>        tegra->ehci = hcd_to_ehci(hcd);
>
> +       /*
> +        * If there isn't an irq defined in the board file, then try to get
> +        * that information from the device tree node.
> +        */
>        irq = platform_get_irq(pdev, 0);
> +#if defined(CONFIG_OF)
> +       if (!irq)
> +               irq = irq_of_parse_and_map(dn, 0);
> +#endif

Same here; unnecessary.

>        if (!irq) {
>                dev_err(&pdev->dev, "Failed to get IRQ\n");
>                err = -ENODEV;
> @@ -773,6 +984,14 @@ static void tegra_ehci_hcd_shutdown(struct platform_device *pdev)
>                hcd->driver->shutdown(hcd);
>  }
>
> +static const struct of_device_id tegra_ehci_of_match[] = {
> +       {
> +               .compatible = "nvidia,tegra20-ehci",
> +       },

The following form is more concise and preferred for match tables:

        { .compatible = "nvidia,tegra20-ehci", },
        {},

> +};
> +MODULE_DEVICE_TABLE(of, tegra_ehci_of_match);
> +
>  static struct platform_driver tegra_ehci_driver = {
>        .probe          = tegra_ehci_probe,
>        .remove         = tegra_ehci_remove,
> @@ -783,5 +1002,7 @@ static struct platform_driver tegra_ehci_driver = {
>        .shutdown       = tegra_ehci_hcd_shutdown,
>        .driver         = {
>                .name   = "tegra-ehci",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = tegra_ehci_of_match,
>        }
>  };
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/3 v2] dt: tegra20: Add ehci host controller nodes.

[Grant Likely]

On Wed, Jul 20, 2011 at 1:52 PM, Stephen Warren <swarren@nvidia.com> wrote:
> achew@nvidia.com wrote at Wednesday, July 20, 2011 1:38 PM:
>> These values were derived from various headers in arch/arm/mach-tegra.
>>
>> Signed-off-by: Andrew Chew <achew@nvidia.com>
>
> Patches 2 and 3:
>
> Acked-by: Stephen Warren <swarren@nvidia.com>
>
> I know Olof had suggested splitting the phy properties into a sub-node
> of the EHCI controller. I'll let you and him discuss that; I don't
> really have an opinion on that.

My comments on the property names will need to be resolved.

g.

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Andrew Chew]

> Can the driver use sane defaults for any of these values?  This patch
> will be a lot smaller if there isn't the need to check all the return
> values each time.

The defaults are in the dt ehci node.  I'd really hate to duplicate these defaults in C code as well, if that's what you're suggesting (they're already duplicated privately elsewhere, sadly, in mach-tegra, but I'm hoping those defaults can go away once we move over to dt for good).  By failing probe if these defaults aren't present, I think it helps make sure the dt ehci node stays in sync with the code.

Or am I misunderstanding your comment?

Re: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Grant Likely]

On Wed, Jul 20, 2011 at 2:16 PM, Andrew Chew <AChew@nvidia.com> wrote:
>> Can the driver use sane defaults for any of these values?  This patch
>> will be a lot smaller if there isn't the need to check all the return
>> values each time.
>
> The defaults are in the dt ehci node.  I'd really hate to duplicate these defaults in C code as well, if that's what you're suggesting (they're already duplicated privately elsewhere, sadly, in mach-tegra, but I'm hoping those defaults can go away once we move over to dt for good).  By failing probe if these defaults aren't present, I think it helps make sure the dt ehci node stays in sync with the code.

I'm saying that if the binding and the driver encode sane defaults for
those parameters, then the properties don't need to appear in the .dts
at all unless they are being overridden.  It is by no means a critical
issue though.  Do what makes the most sense to you.

g.

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Andrew Chew]

> >> Can the driver use sane defaults for any of these values?  
> This patch
> >> will be a lot smaller if there isn't the need to check all 
> the return
> >> values each time.
> >
> > The defaults are in the dt ehci node.  I'd really hate to 
> duplicate these defaults in C code as well, if that's what 
> you're suggesting (they're already duplicated privately 
> elsewhere, sadly, in mach-tegra, but I'm hoping those 
> defaults can go away once we move over to dt for good).  By 
> failing probe if these defaults aren't present, I think it 
> helps make sure the dt ehci node stays in sync with the code.
> 
> I'm saying that if the binding and the driver encode sane defaults for
> those parameters, then the properties don't need to appear in the .dts
> at all unless they are being overridden.  It is by no means a critical
> issue though.  Do what makes the most sense to you.

I see.  I think that makes sense as well.  To be clear, you're suggesting to document the defaults in the bindings file, and have the defaults in the driver (C code).  Then omit the properties from the dts entirely unless they are different (i.e. the dts will no longer have any default values specified), right?  If that's more consistent with how the other drivers will work, I'll do that.

Re: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Grant Likely]

On Wed, Jul 20, 2011 at 2:27 PM, Andrew Chew <AChew@nvidia.com> wrote:
>> >> Can the driver use sane defaults for any of these values?
>> This patch
>> >> will be a lot smaller if there isn't the need to check all
>> the return
>> >> values each time.
>> >
>> > The defaults are in the dt ehci node.  I'd really hate to
>> duplicate these defaults in C code as well, if that's what
>> you're suggesting (they're already duplicated privately
>> elsewhere, sadly, in mach-tegra, but I'm hoping those
>> defaults can go away once we move over to dt for good).  By
>> failing probe if these defaults aren't present, I think it
>> helps make sure the dt ehci node stays in sync with the code.
>>
>> I'm saying that if the binding and the driver encode sane defaults for
>> those parameters, then the properties don't need to appear in the .dts
>> at all unless they are being overridden.  It is by no means a critical
>> issue though.  Do what makes the most sense to you.
>
> I see.  I think that makes sense as well.  To be clear, you're suggesting to document the defaults in the bindings file, and have the defaults in the driver (C code).  Then omit the properties from the dts entirely unless they are different (i.e. the dts will no longer have any default values specified), right?  If that's more consistent with how the other drivers will work, I'll do that.

Yes, that's what I'm saying.  Again, it's up to you on whatever makes
the most sense.

g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Andrew Chew]

> > +Required properties for type = "utmi".  These values are 
> derived from
> > +characterization by system engineering.
> > + - hssync_start_delay
> > + - idle_wait_delay
> > + - elastic_limit
> > + - term_range_adj
> > + - xcvr_setup
> > + - xcvr_lsfslew
> > + - xcvr_lsrslew
> 
> All of these custom properties should be prefixed with "nvidia," to
> avoid namespace collisions.

Are you saying we should prefix with "nvidia," on ALL of the properties I've defined for the tegra20-ehci node?  Or specifically those seven that you pointed out?  Because I don't see a difference in terms of "custom"-ality between say, hssync-start-delay, and the other stuff like phy-type, mode, power-down-on-bus-suspend, and the reset-gpio and clk from phy-type=ulpi...unless we're considering stuff like phy-type to be a standard USB host controller node property.

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Andrew Chew]

> > +       retval = of_property_read_u32(dn, "hssync_start_delay",
> > +                                     (u32 
> *)&phy_config->hssync_start_delay);
> 
> Gah!  Don't cast pointers that way.  It will break badly if the sizes
> don't match.  The variable *must* be a u32.

Can we have u16 and u8 versions of of_property_read, then?  It'll simplify code.  I'll be happy to submit a patch for that if that's what we want.

RE: [PATCH 1/3 v2] usb: tegra20-ehci: Add devicetree support.

[Andrew Chew]

> > > +Required properties for type = "utmi".  These values are 
> > derived from
> > > +characterization by system engineering.
> > > + - hssync_start_delay
> > > + - idle_wait_delay
> > > + - elastic_limit
> > > + - term_range_adj
> > > + - xcvr_setup
> > > + - xcvr_lsfslew
> > > + - xcvr_lsrslew
> > 
> > All of these custom properties should be prefixed with "nvidia," to
> > avoid namespace collisions.
> 
> Are you saying we should prefix with "nvidia," on ALL of the 
> properties I've defined for the tegra20-ehci node?  Or 
> specifically those seven that you pointed out?  Because I 
> don't see a difference in terms of "custom"-ality between 
> say, hssync-start-delay, and the other stuff like phy-type, 
> mode, power-down-on-bus-suspend, and the reset-gpio and clk 
> from phy-type=ulpi...unless we're considering stuff like 
> phy-type to be a standard USB host controller node property.

Looking around for precedence, I see that fsl-usb.txt documents a phy-type as well as a dr-mode (though there's still underscores in that document...I assume they will be replaced by hyphens at some point).  So I think the tegra2 ehci host driver will mirror that, and use those property names, in which case I assume we don't have a vendor prefix on those.  Are "standard" usb host controller properties (those that shouldn't have a vendor prefix) documented anywhere?