| Message ID | 1359993540-20780-10-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 04 February 2013 09:28 PM, Roger Quadros wrote: > Allows the OMAP HS USB host controller to be specified > via device tree. > > CC: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ > drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- > 2 files changed, 145 insertions(+), 6 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt > > diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > new file mode 100644 > index 0000000..2196893 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > @@ -0,0 +1,68 @@ > +OMAP HS USB Host > + > +Required properties: > + > +- compatible: should be "ti,usbhs-host" > +- reg: should contain one register range i.e. start and length > +- ti,hwmods: must contain "usb_host_hs" > + > +Optional properties: > + > +- nports: number of USB ports. Usually this is automatically detected > + from the IP's revision register but can be overridden by specifying > + this property. > + > +- portN_mode: Integer specifying the port mode for port N, where N can be > + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode > + in include/linux/platform_data/usb-omap.h > + If the port mode is not specified, that port is treated as unused. > + > +- single_ulpi_bypass: Must be present if the controller contains a single > + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 > + > +Required properties if child node exists: > + > +- #address-cells: Must be 1 > +- #size-cells: Must be 1 > +- ranges: must be present > + > +Properties for children: > + > +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. > +See Documentation/devicetree/bindings/usb/omap-ehci.txt and > +omap3-ohci.txt > + > +Example for OMAP4: > + > +usbhshost: usbhshost@0x4a064000 { > + compatible = "ti,usbhs-host"; > + reg = <0x4a064000 0x800>; > + ti,hwmods = "usb_host_hs"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + usbhsohci: ohci@0x4a064800 { > + compatible = "ti,omap3-ohci", "usb-ohci"; > + reg = <0x4a064800 0x400>; > + interrupt-parent = <&gic>; > + interrupts = <0 76 0x4>; > + }; > + > + usbhsehci: ehci@0x4a064c00 { > + compatible = "ti,omap-ehci", "usb-ehci"; > + reg = <0x4a064c00 0x400>; > + interrupt-parent = <&gic>; > + interrupts = <0 77 0x4>; > + }; > +}; > + > +&usbhshost { > + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ > + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ > + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ > +}; > + > +&usbhsehci { > + phy = <&hsusb1_phy 0 &hsusb3_phy>; > +}; > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index f8ed08e..0f67856 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -1,8 +1,9 @@ > /** > * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI > * > - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com > + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com > * Author: Keshava Munegowda <keshava_mgowda@ti.com> > + * Author: Roger Quadros <rogerq@ti.com> > * > * This program is free software: you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 of > @@ -27,6 +28,8 @@ > #include <linux/platform_device.h> > #include <linux/platform_data/usb-omap.h> > #include <linux/pm_runtime.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > > #include "omap-usb.h" > > @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) > pm_runtime_put_sync(dev); > } > > +static int usbhs_omap_get_dt_pdata(struct device_node *node, > + struct usbhs_omap_platform_data *pdata) > +{ > + int ret, i; > + > + ret = of_property_read_u32(node, "nports", &pdata->nports); > + if (ret) > + pdata->nports = 0; > + > + /* get port modes */ > + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { > + char prop[11]; > + > + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); > + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); > + if (ret) > + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; > + } > + > + /* get flags */ > + pdata->single_ulpi_bypass = of_property_read_bool(node, > + "single_ulpi_bypass"); > + return 0; > +} > + > +static struct of_device_id usbhs_child_match_table[] __initdata = { > + { .compatible = "ti,omap-ehci", }, > + { .compatible = "ti,omap-ohci", }, > + { } > +}; > + > /** > * usbhs_omap_probe - initialize TI-based HCDs > * > @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) > int i; > bool need_logic_fck; > > + if (dev->of_node) { > + /* For DT boot we populate platform data from OF node */ > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { > + dev_err(dev, > + "Error getting platform data from DT node\n"); > + return -ENODEV; > + } > + > + dev->platform_data = pdata; > + } > + > if (!pdata) { > dev_err(dev, "Missing platform data\n"); > return -ENODEV; > @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) > return -ENOMEM; > } > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > omap->uhh_base = devm_request_and_ioremap(dev, res); > if (!omap->uhh_base) { > dev_err(dev, "Resource request/ioremap failed\n"); > @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) > } > > omap_usbhs_init(dev); > - ret = omap_usbhs_alloc_children(pdev); > - if (ret) { > - dev_err(dev, "omap_usbhs_alloc_children failed\n"); > - goto err_alloc; > + > + if (dev->of_node) { > + ret = of_platform_populate(dev->of_node, > + usbhs_child_match_table, NULL, dev); > + > + if (ret) { > + dev_err(dev, "Failed to create DT children: %d\n", ret); > + goto err_alloc; > + } > + > + } else { > + ret = omap_usbhs_alloc_children(pdev); > + if (ret) { > + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", > + ret); > + goto err_alloc; > + } These child devices should be destroyed on driver remove..no? Thanks Kishon
On 02/05/2013 08:16 AM, kishon wrote: > On Monday 04 February 2013 09:28 PM, Roger Quadros wrote: >> Allows the OMAP HS USB host controller to be specified >> via device tree. >> >> CC: Samuel Ortiz <sameo@linux.intel.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ >> drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- >> 2 files changed, 145 insertions(+), 6 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> >> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> new file mode 100644 >> index 0000000..2196893 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> @@ -0,0 +1,68 @@ >> +OMAP HS USB Host >> + >> +Required properties: >> + >> +- compatible: should be "ti,usbhs-host" >> +- reg: should contain one register range i.e. start and length >> +- ti,hwmods: must contain "usb_host_hs" >> + >> +Optional properties: >> + >> +- nports: number of USB ports. Usually this is automatically detected >> + from the IP's revision register but can be overridden by specifying >> + this property. >> + >> +- portN_mode: Integer specifying the port mode for port N, where N can be >> + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode >> + in include/linux/platform_data/usb-omap.h >> + If the port mode is not specified, that port is treated as unused. >> + >> +- single_ulpi_bypass: Must be present if the controller contains a single >> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >> + >> +Required properties if child node exists: >> + >> +- #address-cells: Must be 1 >> +- #size-cells: Must be 1 >> +- ranges: must be present >> + >> +Properties for children: >> + >> +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. >> +See Documentation/devicetree/bindings/usb/omap-ehci.txt and >> +omap3-ohci.txt >> + >> +Example for OMAP4: >> + >> +usbhshost: usbhshost@0x4a064000 { >> + compatible = "ti,usbhs-host"; >> + reg = <0x4a064000 0x800>; >> + ti,hwmods = "usb_host_hs"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + usbhsohci: ohci@0x4a064800 { >> + compatible = "ti,omap3-ohci", "usb-ohci"; >> + reg = <0x4a064800 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 76 0x4>; >> + }; >> + >> + usbhsehci: ehci@0x4a064c00 { >> + compatible = "ti,omap-ehci", "usb-ehci"; >> + reg = <0x4a064c00 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 77 0x4>; >> + }; >> +}; >> + >> +&usbhshost { >> + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >> + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ >> + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >> +}; >> + >> +&usbhsehci { >> + phy = <&hsusb1_phy 0 &hsusb3_phy>; >> +}; >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index f8ed08e..0f67856 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -1,8 +1,9 @@ >> /** >> * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI >> * >> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com >> + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com >> * Author: Keshava Munegowda <keshava_mgowda@ti.com> >> + * Author: Roger Quadros <rogerq@ti.com> >> * >> * This program is free software: you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 of >> @@ -27,6 +28,8 @@ >> #include <linux/platform_device.h> >> #include <linux/platform_data/usb-omap.h> >> #include <linux/pm_runtime.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> >> #include "omap-usb.h" >> >> @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) >> pm_runtime_put_sync(dev); >> } >> >> +static int usbhs_omap_get_dt_pdata(struct device_node *node, >> + struct usbhs_omap_platform_data *pdata) >> +{ >> + int ret, i; >> + >> + ret = of_property_read_u32(node, "nports", &pdata->nports); >> + if (ret) >> + pdata->nports = 0; >> + >> + /* get port modes */ >> + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { >> + char prop[11]; >> + >> + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); >> + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); >> + if (ret) >> + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; >> + } >> + >> + /* get flags */ >> + pdata->single_ulpi_bypass = of_property_read_bool(node, >> + "single_ulpi_bypass"); >> + return 0; >> +} >> + >> +static struct of_device_id usbhs_child_match_table[] __initdata = { >> + { .compatible = "ti,omap-ehci", }, >> + { .compatible = "ti,omap-ohci", }, >> + { } >> +}; >> + >> /** >> * usbhs_omap_probe - initialize TI-based HCDs >> * >> @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> int i; >> bool need_logic_fck; >> >> + if (dev->of_node) { >> + /* For DT boot we populate platform data from OF node */ >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { >> + dev_err(dev, >> + "Error getting platform data from DT node\n"); >> + return -ENODEV; >> + } >> + >> + dev->platform_data = pdata; >> + } >> + >> if (!pdata) { >> dev_err(dev, "Missing platform data\n"); >> return -ENODEV; >> @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> return -ENOMEM; >> } >> >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> omap->uhh_base = devm_request_and_ioremap(dev, res); >> if (!omap->uhh_base) { >> dev_err(dev, "Resource request/ioremap failed\n"); >> @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> } >> >> omap_usbhs_init(dev); >> - ret = omap_usbhs_alloc_children(pdev); >> - if (ret) { >> - dev_err(dev, "omap_usbhs_alloc_children failed\n"); >> - goto err_alloc; >> + >> + if (dev->of_node) { >> + ret = of_platform_populate(dev->of_node, >> + usbhs_child_match_table, NULL, dev); >> + >> + if (ret) { >> + dev_err(dev, "Failed to create DT children: %d\n", ret); >> + goto err_alloc; >> + } >> + >> + } else { >> + ret = omap_usbhs_alloc_children(pdev); >> + if (ret) { >> + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", >> + ret); >> + goto err_alloc; >> + } > These child devices should be destroyed on driver remove..no? > Indeed. good catch :). cheers, -roger
On 02/05/2013 08:16 AM, kishon wrote: > On Monday 04 February 2013 09:28 PM, Roger Quadros wrote: >> Allows the OMAP HS USB host controller to be specified >> via device tree. >> >> CC: Samuel Ortiz <sameo@linux.intel.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ >> drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- >> 2 files changed, 145 insertions(+), 6 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> >> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> new file mode 100644 >> index 0000000..2196893 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> @@ -0,0 +1,68 @@ >> +OMAP HS USB Host >> + >> +Required properties: >> + >> +- compatible: should be "ti,usbhs-host" >> +- reg: should contain one register range i.e. start and length >> +- ti,hwmods: must contain "usb_host_hs" >> + >> +Optional properties: >> + >> +- nports: number of USB ports. Usually this is automatically detected >> + from the IP's revision register but can be overridden by specifying >> + this property. >> + >> +- portN_mode: Integer specifying the port mode for port N, where N can be >> + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode >> + in include/linux/platform_data/usb-omap.h >> + If the port mode is not specified, that port is treated as unused. >> + >> +- single_ulpi_bypass: Must be present if the controller contains a single >> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >> + >> +Required properties if child node exists: >> + >> +- #address-cells: Must be 1 >> +- #size-cells: Must be 1 >> +- ranges: must be present >> + >> +Properties for children: >> + >> +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. >> +See Documentation/devicetree/bindings/usb/omap-ehci.txt and >> +omap3-ohci.txt >> + >> +Example for OMAP4: >> + >> +usbhshost: usbhshost@0x4a064000 { >> + compatible = "ti,usbhs-host"; >> + reg = <0x4a064000 0x800>; >> + ti,hwmods = "usb_host_hs"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + usbhsohci: ohci@0x4a064800 { >> + compatible = "ti,omap3-ohci", "usb-ohci"; >> + reg = <0x4a064800 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 76 0x4>; >> + }; >> + >> + usbhsehci: ehci@0x4a064c00 { >> + compatible = "ti,omap-ehci", "usb-ehci"; >> + reg = <0x4a064c00 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 77 0x4>; >> + }; >> +}; >> + >> +&usbhshost { >> + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >> + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ >> + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >> +}; >> + >> +&usbhsehci { >> + phy = <&hsusb1_phy 0 &hsusb3_phy>; >> +}; >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index f8ed08e..0f67856 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -1,8 +1,9 @@ >> /** >> * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI >> * >> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com >> + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com >> * Author: Keshava Munegowda <keshava_mgowda@ti.com> >> + * Author: Roger Quadros <rogerq@ti.com> >> * >> * This program is free software: you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 of >> @@ -27,6 +28,8 @@ >> #include <linux/platform_device.h> >> #include <linux/platform_data/usb-omap.h> >> #include <linux/pm_runtime.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> >> #include "omap-usb.h" >> >> @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) >> pm_runtime_put_sync(dev); >> } >> >> +static int usbhs_omap_get_dt_pdata(struct device_node *node, >> + struct usbhs_omap_platform_data *pdata) >> +{ >> + int ret, i; >> + >> + ret = of_property_read_u32(node, "nports", &pdata->nports); >> + if (ret) >> + pdata->nports = 0; >> + >> + /* get port modes */ >> + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { >> + char prop[11]; >> + >> + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); >> + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); >> + if (ret) >> + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; >> + } >> + >> + /* get flags */ >> + pdata->single_ulpi_bypass = of_property_read_bool(node, >> + "single_ulpi_bypass"); >> + return 0; >> +} >> + >> +static struct of_device_id usbhs_child_match_table[] __initdata = { >> + { .compatible = "ti,omap-ehci", }, >> + { .compatible = "ti,omap-ohci", }, >> + { } >> +}; >> + >> /** >> * usbhs_omap_probe - initialize TI-based HCDs >> * >> @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> int i; >> bool need_logic_fck; >> >> + if (dev->of_node) { >> + /* For DT boot we populate platform data from OF node */ >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { >> + dev_err(dev, >> + "Error getting platform data from DT node\n"); >> + return -ENODEV; >> + } >> + >> + dev->platform_data = pdata; >> + } >> + >> if (!pdata) { >> dev_err(dev, "Missing platform data\n"); >> return -ENODEV; >> @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> return -ENOMEM; >> } >> >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> omap->uhh_base = devm_request_and_ioremap(dev, res); >> if (!omap->uhh_base) { >> dev_err(dev, "Resource request/ioremap failed\n"); >> @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> } >> >> omap_usbhs_init(dev); >> - ret = omap_usbhs_alloc_children(pdev); >> - if (ret) { >> - dev_err(dev, "omap_usbhs_alloc_children failed\n"); >> - goto err_alloc; >> + >> + if (dev->of_node) { >> + ret = of_platform_populate(dev->of_node, >> + usbhs_child_match_table, NULL, dev); >> + >> + if (ret) { >> + dev_err(dev, "Failed to create DT children: %d\n", ret); >> + goto err_alloc; >> + } >> + >> + } else { >> + ret = omap_usbhs_alloc_children(pdev); >> + if (ret) { >> + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", >> + ret); >> + goto err_alloc; >> + } > These child devices should be destroyed on driver remove..no? > I could not find a function that does the opposite of of_platform_populate() or of_platform_device_create_pdata(). It seems that platform devices created via device tree are never meant to be destroyed. It kind of makes sense for EHCI/OHCI, cause the devices are always present on the SoC. Also, this driver can't be built as a module so it can never be removed. So let's leave this patch the way it is except removing 0x from the device name in the example. cheers, -roger
Hi, On Tuesday 05 February 2013 04:28 PM, Roger Quadros wrote: > On 02/05/2013 08:16 AM, kishon wrote: >> On Monday 04 February 2013 09:28 PM, Roger Quadros wrote: >>> Allows the OMAP HS USB host controller to be specified >>> via device tree. >>> >>> CC: Samuel Ortiz <sameo@linux.intel.com> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ >>> drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- >>> 2 files changed, 145 insertions(+), 6 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>> new file mode 100644 >>> index 0000000..2196893 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>> @@ -0,0 +1,68 @@ >>> +OMAP HS USB Host >>> + >>> +Required properties: >>> + >>> +- compatible: should be "ti,usbhs-host" >>> +- reg: should contain one register range i.e. start and length >>> +- ti,hwmods: must contain "usb_host_hs" >>> + >>> +Optional properties: >>> + >>> +- nports: number of USB ports. Usually this is automatically detected >>> + from the IP's revision register but can be overridden by specifying >>> + this property. >>> + >>> +- portN_mode: Integer specifying the port mode for port N, where N can be >>> + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode >>> + in include/linux/platform_data/usb-omap.h >>> + If the port mode is not specified, that port is treated as unused. >>> + >>> +- single_ulpi_bypass: Must be present if the controller contains a single >>> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >>> + >>> +Required properties if child node exists: >>> + >>> +- #address-cells: Must be 1 >>> +- #size-cells: Must be 1 >>> +- ranges: must be present >>> + >>> +Properties for children: >>> + >>> +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. >>> +See Documentation/devicetree/bindings/usb/omap-ehci.txt and >>> +omap3-ohci.txt >>> + >>> +Example for OMAP4: >>> + >>> +usbhshost: usbhshost@0x4a064000 { >>> + compatible = "ti,usbhs-host"; >>> + reg = <0x4a064000 0x800>; >>> + ti,hwmods = "usb_host_hs"; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges; >>> + >>> + usbhsohci: ohci@0x4a064800 { >>> + compatible = "ti,omap3-ohci", "usb-ohci"; >>> + reg = <0x4a064800 0x400>; >>> + interrupt-parent = <&gic>; >>> + interrupts = <0 76 0x4>; >>> + }; >>> + >>> + usbhsehci: ehci@0x4a064c00 { >>> + compatible = "ti,omap-ehci", "usb-ehci"; >>> + reg = <0x4a064c00 0x400>; >>> + interrupt-parent = <&gic>; >>> + interrupts = <0 77 0x4>; >>> + }; >>> +}; >>> + >>> +&usbhshost { >>> + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >>> + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ >>> + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >>> +}; >>> + >>> +&usbhsehci { >>> + phy = <&hsusb1_phy 0 &hsusb3_phy>; >>> +}; >>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >>> index f8ed08e..0f67856 100644 >>> --- a/drivers/mfd/omap-usb-host.c >>> +++ b/drivers/mfd/omap-usb-host.c >>> @@ -1,8 +1,9 @@ >>> /** >>> * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI >>> * >>> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com >>> + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com >>> * Author: Keshava Munegowda <keshava_mgowda@ti.com> >>> + * Author: Roger Quadros <rogerq@ti.com> >>> * >>> * This program is free software: you can redistribute it and/or modify >>> * it under the terms of the GNU General Public License version 2 of >>> @@ -27,6 +28,8 @@ >>> #include <linux/platform_device.h> >>> #include <linux/platform_data/usb-omap.h> >>> #include <linux/pm_runtime.h> >>> +#include <linux/of.h> >>> +#include <linux/of_platform.h> >>> >>> #include "omap-usb.h" >>> >>> @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) >>> pm_runtime_put_sync(dev); >>> } >>> >>> +static int usbhs_omap_get_dt_pdata(struct device_node *node, >>> + struct usbhs_omap_platform_data *pdata) >>> +{ >>> + int ret, i; >>> + >>> + ret = of_property_read_u32(node, "nports", &pdata->nports); >>> + if (ret) >>> + pdata->nports = 0; >>> + >>> + /* get port modes */ >>> + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { >>> + char prop[11]; >>> + >>> + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); >>> + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); >>> + if (ret) >>> + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; >>> + } >>> + >>> + /* get flags */ >>> + pdata->single_ulpi_bypass = of_property_read_bool(node, >>> + "single_ulpi_bypass"); >>> + return 0; >>> +} >>> + >>> +static struct of_device_id usbhs_child_match_table[] __initdata = { >>> + { .compatible = "ti,omap-ehci", }, >>> + { .compatible = "ti,omap-ohci", }, >>> + { } >>> +}; >>> + >>> /** >>> * usbhs_omap_probe - initialize TI-based HCDs >>> * >>> @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>> int i; >>> bool need_logic_fck; >>> >>> + if (dev->of_node) { >>> + /* For DT boot we populate platform data from OF node */ >>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >>> + if (!pdata) >>> + return -ENOMEM; >>> + >>> + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { >>> + dev_err(dev, >>> + "Error getting platform data from DT node\n"); >>> + return -ENODEV; >>> + } >>> + >>> + dev->platform_data = pdata; >>> + } >>> + >>> if (!pdata) { >>> dev_err(dev, "Missing platform data\n"); >>> return -ENODEV; >>> @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>> return -ENOMEM; >>> } >>> >>> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> omap->uhh_base = devm_request_and_ioremap(dev, res); >>> if (!omap->uhh_base) { >>> dev_err(dev, "Resource request/ioremap failed\n"); >>> @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>> } >>> >>> omap_usbhs_init(dev); >>> - ret = omap_usbhs_alloc_children(pdev); >>> - if (ret) { >>> - dev_err(dev, "omap_usbhs_alloc_children failed\n"); >>> - goto err_alloc; >>> + >>> + if (dev->of_node) { >>> + ret = of_platform_populate(dev->of_node, >>> + usbhs_child_match_table, NULL, dev); >>> + >>> + if (ret) { >>> + dev_err(dev, "Failed to create DT children: %d\n", ret); >>> + goto err_alloc; >>> + } >>> + >>> + } else { >>> + ret = omap_usbhs_alloc_children(pdev); >>> + if (ret) { >>> + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", >>> + ret); >>> + goto err_alloc; >>> + } >> These child devices should be destroyed on driver remove..no? >> > I could not find a function that does the opposite of of_platform_populate() or > of_platform_device_create_pdata(). It seems that platform devices created via > device tree are never meant to be destroyed. No. I've done it for dwc3 in usb/dwc3/dwc3-omap.c. (you can check usb-next) > > It kind of makes sense for EHCI/OHCI, cause the devices are always present > on the SoC. Not true for devices created in drivers/ IMHO. It makes sense only if you create the device in some platform specific initialization file. > Also, this driver can't be built as a module so it can never be removed. Why is this restriction btw? Thanks Kishon
On 02/05/2013 02:11 PM, kishon wrote: > Hi, > > On Tuesday 05 February 2013 04:28 PM, Roger Quadros wrote: >> On 02/05/2013 08:16 AM, kishon wrote: >>> On Monday 04 February 2013 09:28 PM, Roger Quadros wrote: >>>> Allows the OMAP HS USB host controller to be specified >>>> via device tree. >>>> >>>> CC: Samuel Ortiz <sameo@linux.intel.com> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> --- >>>> .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ >>>> drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- >>>> 2 files changed, 145 insertions(+), 6 deletions(-) >>>> create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>>> new file mode 100644 >>>> index 0000000..2196893 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >>>> @@ -0,0 +1,68 @@ >>>> +OMAP HS USB Host >>>> + >>>> +Required properties: >>>> + >>>> +- compatible: should be "ti,usbhs-host" >>>> +- reg: should contain one register range i.e. start and length >>>> +- ti,hwmods: must contain "usb_host_hs" >>>> + >>>> +Optional properties: >>>> + >>>> +- nports: number of USB ports. Usually this is automatically detected >>>> + from the IP's revision register but can be overridden by specifying >>>> + this property. >>>> + >>>> +- portN_mode: Integer specifying the port mode for port N, where N can be >>>> + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode >>>> + in include/linux/platform_data/usb-omap.h >>>> + If the port mode is not specified, that port is treated as unused. >>>> + >>>> +- single_ulpi_bypass: Must be present if the controller contains a single >>>> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >>>> + >>>> +Required properties if child node exists: >>>> + >>>> +- #address-cells: Must be 1 >>>> +- #size-cells: Must be 1 >>>> +- ranges: must be present >>>> + >>>> +Properties for children: >>>> + >>>> +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. >>>> +See Documentation/devicetree/bindings/usb/omap-ehci.txt and >>>> +omap3-ohci.txt >>>> + >>>> +Example for OMAP4: >>>> + >>>> +usbhshost: usbhshost@0x4a064000 { >>>> + compatible = "ti,usbhs-host"; >>>> + reg = <0x4a064000 0x800>; >>>> + ti,hwmods = "usb_host_hs"; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + ranges; >>>> + >>>> + usbhsohci: ohci@0x4a064800 { >>>> + compatible = "ti,omap3-ohci", "usb-ohci"; >>>> + reg = <0x4a064800 0x400>; >>>> + interrupt-parent = <&gic>; >>>> + interrupts = <0 76 0x4>; >>>> + }; >>>> + >>>> + usbhsehci: ehci@0x4a064c00 { >>>> + compatible = "ti,omap-ehci", "usb-ehci"; >>>> + reg = <0x4a064c00 0x400>; >>>> + interrupt-parent = <&gic>; >>>> + interrupts = <0 77 0x4>; >>>> + }; >>>> +}; >>>> + >>>> +&usbhshost { >>>> + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >>>> + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ >>>> + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >>>> +}; >>>> + >>>> +&usbhsehci { >>>> + phy = <&hsusb1_phy 0 &hsusb3_phy>; >>>> +}; >>>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >>>> index f8ed08e..0f67856 100644 >>>> --- a/drivers/mfd/omap-usb-host.c >>>> +++ b/drivers/mfd/omap-usb-host.c >>>> @@ -1,8 +1,9 @@ >>>> /** >>>> * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI >>>> * >>>> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com >>>> + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com >>>> * Author: Keshava Munegowda <keshava_mgowda@ti.com> >>>> + * Author: Roger Quadros <rogerq@ti.com> >>>> * >>>> * This program is free software: you can redistribute it and/or modify >>>> * it under the terms of the GNU General Public License version 2 of >>>> @@ -27,6 +28,8 @@ >>>> #include <linux/platform_device.h> >>>> #include <linux/platform_data/usb-omap.h> >>>> #include <linux/pm_runtime.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_platform.h> >>>> >>>> #include "omap-usb.h" >>>> >>>> @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) >>>> pm_runtime_put_sync(dev); >>>> } >>>> >>>> +static int usbhs_omap_get_dt_pdata(struct device_node *node, >>>> + struct usbhs_omap_platform_data *pdata) >>>> +{ >>>> + int ret, i; >>>> + >>>> + ret = of_property_read_u32(node, "nports", &pdata->nports); >>>> + if (ret) >>>> + pdata->nports = 0; >>>> + >>>> + /* get port modes */ >>>> + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { >>>> + char prop[11]; >>>> + >>>> + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); >>>> + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); >>>> + if (ret) >>>> + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; >>>> + } >>>> + >>>> + /* get flags */ >>>> + pdata->single_ulpi_bypass = of_property_read_bool(node, >>>> + "single_ulpi_bypass"); >>>> + return 0; >>>> +} >>>> + >>>> +static struct of_device_id usbhs_child_match_table[] __initdata = { >>>> + { .compatible = "ti,omap-ehci", }, >>>> + { .compatible = "ti,omap-ohci", }, >>>> + { } >>>> +}; >>>> + >>>> /** >>>> * usbhs_omap_probe - initialize TI-based HCDs >>>> * >>>> @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>>> int i; >>>> bool need_logic_fck; >>>> >>>> + if (dev->of_node) { >>>> + /* For DT boot we populate platform data from OF node */ >>>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >>>> + if (!pdata) >>>> + return -ENOMEM; >>>> + >>>> + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { >>>> + dev_err(dev, >>>> + "Error getting platform data from DT node\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + dev->platform_data = pdata; >>>> + } >>>> + >>>> if (!pdata) { >>>> dev_err(dev, "Missing platform data\n"); >>>> return -ENODEV; >>>> @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>>> return -ENOMEM; >>>> } >>>> >>>> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> omap->uhh_base = devm_request_and_ioremap(dev, res); >>>> if (!omap->uhh_base) { >>>> dev_err(dev, "Resource request/ioremap failed\n"); >>>> @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) >>>> } >>>> >>>> omap_usbhs_init(dev); >>>> - ret = omap_usbhs_alloc_children(pdev); >>>> - if (ret) { >>>> - dev_err(dev, "omap_usbhs_alloc_children failed\n"); >>>> - goto err_alloc; >>>> + >>>> + if (dev->of_node) { >>>> + ret = of_platform_populate(dev->of_node, >>>> + usbhs_child_match_table, NULL, dev); >>>> + >>>> + if (ret) { >>>> + dev_err(dev, "Failed to create DT children: %d\n", ret); >>>> + goto err_alloc; >>>> + } >>>> + >>>> + } else { >>>> + ret = omap_usbhs_alloc_children(pdev); >>>> + if (ret) { >>>> + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", >>>> + ret); >>>> + goto err_alloc; >>>> + } >>> These child devices should be destroyed on driver remove..no? >>> >> I could not find a function that does the opposite of of_platform_populate() or >> of_platform_device_create_pdata(). It seems that platform devices created via >> device tree are never meant to be destroyed. > > No. I've done it for dwc3 in usb/dwc3/dwc3-omap.c. (you can check usb-next) OK, so platform_device_unregister() is sufficient it seems. Thanks for the hint. >> >> It kind of makes sense for EHCI/OHCI, cause the devices are always present >> on the SoC. > Not true for devices created in drivers/ IMHO. It makes sense only if you create the device in some platform specific initialization file. > >> Also, this driver can't be built as a module so it can never be removed. > Why is this restriction btw? I think it is because of the interdependency to load the omap-usb-tll driver before the omap-usb-host driver. Now that we have deferred probing mechanism, I don't think it should be a problem any more. cheers, -roger
Hi, I have a few comments on the binding and the way it's parsed. On Mon, Feb 04, 2013 at 03:58:56PM +0000, Roger Quadros wrote: > Allows the OMAP HS USB host controller to be specified > via device tree. > > CC: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ > drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- > 2 files changed, 145 insertions(+), 6 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt > > diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > new file mode 100644 > index 0000000..2196893 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt > @@ -0,0 +1,68 @@ > +OMAP HS USB Host > + > +Required properties: > + > +- compatible: should be "ti,usbhs-host" > +- reg: should contain one register range i.e. start and length > +- ti,hwmods: must contain "usb_host_hs" > + > +Optional properties: > + > +- nports: number of USB ports. Usually this is automatically detected > + from the IP's revision register but can be overridden by specifying > + this property. It would be nice if this were "num-ports", as atmel-usb is already using that, and it's clear that it's a number of ports rather than some other meaning of 'n'. From a quick grep of binding documents, out of "nTHING(s)", "nr-THINGs", and num-THINGs, num-THINGs seems to be the most common. It would be nice if new bindings could standardise this. > + > +- portN_mode: Integer specifying the port mode for port N, where N can be > + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode > + in include/linux/platform_data/usb-omap.h > + If the port mode is not specified, that port is treated as unused. I'm against devicetree bindings refering to Linux internals. It makes a poorly documented ABI that someone might change in future without realising the implications, and it makes it stupidly difficult to read a dts. Everything required should be specified in the binding document (or another linked binding document). It might be better to describe this with a string property that gets mapped by your dt parsing code to whatever internal representation you need. That way it's far easier for a human to verify the dts is correct, and you know by construction that the parsed value is something you can handle in the driver. It would be nicer is you used '-' rather than '_' for consistency with devicetree bindings in general. > + > +- single_ulpi_bypass: Must be present if the controller contains a single > + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 Again it would be nicer to have '-' rather than '_' here. It might be worth prefixing this "ti,". > + > +Required properties if child node exists: > + > +- #address-cells: Must be 1 > +- #size-cells: Must be 1 > +- ranges: must be present > + > +Properties for children: > + > +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. > +See Documentation/devicetree/bindings/usb/omap-ehci.txt and > +omap3-ohci.txt > + > +Example for OMAP4: > + > +usbhshost: usbhshost@0x4a064000 { > + compatible = "ti,usbhs-host"; > + reg = <0x4a064000 0x800>; > + ti,hwmods = "usb_host_hs"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + usbhsohci: ohci@0x4a064800 { > + compatible = "ti,omap3-ohci", "usb-ohci"; > + reg = <0x4a064800 0x400>; > + interrupt-parent = <&gic>; > + interrupts = <0 76 0x4>; > + }; > + > + usbhsehci: ehci@0x4a064c00 { > + compatible = "ti,omap-ehci", "usb-ehci"; > + reg = <0x4a064c00 0x400>; > + interrupt-parent = <&gic>; > + interrupts = <0 77 0x4>; > + }; > +}; > + > +&usbhshost { > + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ > + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ > + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ With a string property, these values would be self-documenting. > +}; > + > +&usbhsehci { > + phy = <&hsusb1_phy 0 &hsusb3_phy>; > +}; > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index f8ed08e..0f67856 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -1,8 +1,9 @@ > /** > * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI > * > - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com > + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com > * Author: Keshava Munegowda <keshava_mgowda@ti.com> > + * Author: Roger Quadros <rogerq@ti.com> > * > * This program is free software: you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 of > @@ -27,6 +28,8 @@ > #include <linux/platform_device.h> > #include <linux/platform_data/usb-omap.h> > #include <linux/pm_runtime.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > > #include "omap-usb.h" > > @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) > pm_runtime_put_sync(dev); > } > > +static int usbhs_omap_get_dt_pdata(struct device_node *node, > + struct usbhs_omap_platform_data *pdata) > +{ > + int ret, i; > + > + ret = of_property_read_u32(node, "nports", &pdata->nports); > + if (ret) > + pdata->nports = 0; Is there no upper bound on how many ports the controller can have lower than 4294967295? I see there are several places in the driver that assume you can only have at most OMAP3_HS_USB_PORTS (i.e. 3) ports. Is this expected to grow, or is the hardware design capped at 3? I don't seem to have usbhs_omap_platform_data::nports in my tree, and I couldn't see it addded in any of this series so far. Where can I find a tree with it present? Is it a u32? If not, you'll need to use a temporary when reading the dt. > + > + /* get port modes */ > + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { > + char prop[11]; > + > + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); > + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); > + if (ret) > + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; What if the port has an invalid mode value? What if something needs to be added to or removed from the enum in future? In my tree, pdata->port_mode[i] is an enum usbhs_omap_port_mode, not a u32. Assuming it's the same in your tree. depending on what size the compiler allocates the enum, you may clobber the other entries in the array (or data immediately beyond it). It'd at least be worth warning the user if there's a value the driver doesn't understand. > + } > + > + /* get flags */ > + pdata->single_ulpi_bypass = of_property_read_bool(node, > + "single_ulpi_bypass"); > + return 0; > +} > + > +static struct of_device_id usbhs_child_match_table[] __initdata = { > + { .compatible = "ti,omap-ehci", }, > + { .compatible = "ti,omap-ohci", }, > + { } > +}; > + > /** > * usbhs_omap_probe - initialize TI-based HCDs > * > @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) > int i; > bool need_logic_fck; > > + if (dev->of_node) { > + /* For DT boot we populate platform data from OF node */ > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { > + dev_err(dev, > + "Error getting platform data from DT node\n"); > + return -ENODEV; This is currently unnecessary, as usbhs_omap_get_dt_pdata always returns 0. It would be nicer if it error'd out on an invalid dt. > + } > + > + dev->platform_data = pdata; > + } > + > if (!pdata) { > dev_err(dev, "Missing platform data\n"); > return -ENODEV; [...] Thanks, Mark.
On 02/05/2013 04:20 PM, Mark Rutland wrote: > Hi, > > I have a few comments on the binding and the way it's parsed. > > On Mon, Feb 04, 2013 at 03:58:56PM +0000, Roger Quadros wrote: >> Allows the OMAP HS USB host controller to be specified >> via device tree. >> >> CC: Samuel Ortiz <sameo@linux.intel.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ >> drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- >> 2 files changed, 145 insertions(+), 6 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> >> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> new file mode 100644 >> index 0000000..2196893 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt >> @@ -0,0 +1,68 @@ >> +OMAP HS USB Host >> + >> +Required properties: >> + >> +- compatible: should be "ti,usbhs-host" >> +- reg: should contain one register range i.e. start and length >> +- ti,hwmods: must contain "usb_host_hs" >> + >> +Optional properties: >> + >> +- nports: number of USB ports. Usually this is automatically detected >> + from the IP's revision register but can be overridden by specifying >> + this property. > > It would be nice if this were "num-ports", as atmel-usb is already using that, > and it's clear that it's a number of ports rather than some other meaning of > 'n'. > > From a quick grep of binding documents, out of "nTHING(s)", "nr-THINGs", and > num-THINGs, num-THINGs seems to be the most common. It would be nice if new > bindings could standardise this. Agreed. > >> + >> +- portN_mode: Integer specifying the port mode for port N, where N can be >> + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode >> + in include/linux/platform_data/usb-omap.h >> + If the port mode is not specified, that port is treated as unused. > > I'm against devicetree bindings refering to Linux internals. It makes a poorly > documented ABI that someone might change in future without realising the > implications, and it makes it stupidly difficult to read a dts. > > Everything required should be specified in the binding document (or another > linked binding document). It might be better to describe this with a string > property that gets mapped by your dt parsing code to whatever internal > representation you need. That way it's far easier for a human to verify the dts > is correct, and you know by construction that the parsed value is something you > can handle in the driver. As string makes it self documenting, I'll convert it to a string and update the binding document. > > It would be nicer is you used '-' rather than '_' for consistency with > devicetree bindings in general. OK. > >> + >> +- single_ulpi_bypass: Must be present if the controller contains a single >> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 > > Again it would be nicer to have '-' rather than '_' here. It might be worth > prefixing this "ti,". Is prefixing with "ti" really required? how does it better? > >> + >> +Required properties if child node exists: >> + >> +- #address-cells: Must be 1 >> +- #size-cells: Must be 1 >> +- ranges: must be present >> + >> +Properties for children: >> + >> +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. >> +See Documentation/devicetree/bindings/usb/omap-ehci.txt and >> +omap3-ohci.txt >> + >> +Example for OMAP4: >> + >> +usbhshost: usbhshost@0x4a064000 { >> + compatible = "ti,usbhs-host"; >> + reg = <0x4a064000 0x800>; >> + ti,hwmods = "usb_host_hs"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + usbhsohci: ohci@0x4a064800 { >> + compatible = "ti,omap3-ohci", "usb-ohci"; >> + reg = <0x4a064800 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 76 0x4>; >> + }; >> + >> + usbhsehci: ehci@0x4a064c00 { >> + compatible = "ti,omap-ehci", "usb-ehci"; >> + reg = <0x4a064c00 0x400>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 77 0x4>; >> + }; >> +}; >> + >> +&usbhshost { >> + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ >> + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ >> + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ > > With a string property, these values would be self-documenting. > >> +}; >> + >> +&usbhsehci { >> + phy = <&hsusb1_phy 0 &hsusb3_phy>; >> +}; >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index f8ed08e..0f67856 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -1,8 +1,9 @@ >> /** >> * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI >> * >> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com >> + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com >> * Author: Keshava Munegowda <keshava_mgowda@ti.com> >> + * Author: Roger Quadros <rogerq@ti.com> >> * >> * This program is free software: you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 of >> @@ -27,6 +28,8 @@ >> #include <linux/platform_device.h> >> #include <linux/platform_data/usb-omap.h> >> #include <linux/pm_runtime.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> >> #include "omap-usb.h" >> >> @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) >> pm_runtime_put_sync(dev); >> } >> >> +static int usbhs_omap_get_dt_pdata(struct device_node *node, >> + struct usbhs_omap_platform_data *pdata) >> +{ >> + int ret, i; >> + >> + ret = of_property_read_u32(node, "nports", &pdata->nports); >> + if (ret) >> + pdata->nports = 0; > > Is there no upper bound on how many ports the controller can have lower than > 4294967295? > > I see there are several places in the driver that assume you can only have at > most OMAP3_HS_USB_PORTS (i.e. 3) ports. Is this expected to grow, or is the > hardware design capped at 3? AFAIK it is capped at 3. > > I don't seem to have usbhs_omap_platform_data::nports in my tree, and I > couldn't see it addded in any of this series so far. Where can I find a tree > with it present? It should be in linux-next. Alternatively you can pull the patchset from git://github.com/rogerq/linux.git linux-usbhost14-part > > Is it a u32? If not, you'll need to use a temporary when reading the dt. nports is int. > >> + >> + /* get port modes */ >> + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { >> + char prop[11]; >> + >> + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); >> + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); >> + if (ret) >> + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; > > What if the port has an invalid mode value? What if something needs to be added > to or removed from the enum in future? Right. I'll add checks for invalid modes and print a warning. > > In my tree, pdata->port_mode[i] is an enum usbhs_omap_port_mode, not a u32. > Assuming it's the same in your tree. depending on what size the compiler > allocates the enum, you may clobber the other entries in the array (or data > immediately beyond it). it pdata->port_mod[i] is still enum usbhs_omap_port_mode. So i'll have to fix this. > > It'd at least be worth warning the user if there's a value the driver doesn't > understand. > >> + } >> + >> + /* get flags */ >> + pdata->single_ulpi_bypass = of_property_read_bool(node, >> + "single_ulpi_bypass"); >> + return 0; >> +} >> + >> +static struct of_device_id usbhs_child_match_table[] __initdata = { >> + { .compatible = "ti,omap-ehci", }, >> + { .compatible = "ti,omap-ohci", }, >> + { } >> +}; >> + >> /** >> * usbhs_omap_probe - initialize TI-based HCDs >> * >> @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) >> int i; >> bool need_logic_fck; >> >> + if (dev->of_node) { >> + /* For DT boot we populate platform data from OF node */ >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { >> + dev_err(dev, >> + "Error getting platform data from DT node\n"); >> + return -ENODEV; > > This is currently unnecessary, as usbhs_omap_get_dt_pdata always returns 0. > > It would be nicer if it error'd out on an invalid dt. yes. > >> + } >> + >> + dev->platform_data = pdata; >> + } >> + >> if (!pdata) { >> dev_err(dev, "Missing platform data\n"); >> return -ENODEV; > Thanks for the in-depth review :). cheers, -roger
[...] > >> + > >> +- single_ulpi_bypass: Must be present if the controller contains a single > >> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 > > > > Again it would be nicer to have '-' rather than '_' here. It might be worth > > prefixing this "ti,". > > Is prefixing with "ti" really required? how does it better? I thought single-ulpi-bypass sounded rather generic, but it probably is specific enough as-is. I don't know enough about USB hardware to have strong feelings either way. [...] > Thanks for the in-depth review :). You're welcome. Thanks, Mark.
On 02/05/2013 06:11 PM, Mark Rutland wrote: > [...] > >>>> + >>>> +- single_ulpi_bypass: Must be present if the controller contains a single >>>> + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 >>> >>> Again it would be nicer to have '-' rather than '_' here. It might be worth >>> prefixing this "ti,". >> >> Is prefixing with "ti" really required? how does it better? > > I thought single-ulpi-bypass sounded rather generic, but it probably is > specific enough as-is. I don't know enough about USB hardware to have strong > feelings either way. > Yes, it is specific to the TI silicon. I'll leave it as it is then. cheers, -roger
diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt new file mode 100644 index 0000000..2196893 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt @@ -0,0 +1,68 @@ +OMAP HS USB Host + +Required properties: + +- compatible: should be "ti,usbhs-host" +- reg: should contain one register range i.e. start and length +- ti,hwmods: must contain "usb_host_hs" + +Optional properties: + +- nports: number of USB ports. Usually this is automatically detected + from the IP's revision register but can be overridden by specifying + this property. + +- portN_mode: Integer specifying the port mode for port N, where N can be + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode + in include/linux/platform_data/usb-omap.h + If the port mode is not specified, that port is treated as unused. + +- single_ulpi_bypass: Must be present if the controller contains a single + ULPI bypass control bit. e.g. OMAP3 silicon <= ES2.1 + +Required properties if child node exists: + +- #address-cells: Must be 1 +- #size-cells: Must be 1 +- ranges: must be present + +Properties for children: + +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. +See Documentation/devicetree/bindings/usb/omap-ehci.txt and +omap3-ohci.txt + +Example for OMAP4: + +usbhshost: usbhshost@0x4a064000 { + compatible = "ti,usbhs-host"; + reg = <0x4a064000 0x800>; + ti,hwmods = "usb_host_hs"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + usbhsohci: ohci@0x4a064800 { + compatible = "ti,omap3-ohci", "usb-ohci"; + reg = <0x4a064800 0x400>; + interrupt-parent = <&gic>; + interrupts = <0 76 0x4>; + }; + + usbhsehci: ehci@0x4a064c00 { + compatible = "ti,omap-ehci", "usb-ehci"; + reg = <0x4a064c00 0x400>; + interrupt-parent = <&gic>; + interrupts = <0 77 0x4>; + }; +}; + +&usbhshost { + port1_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ + port2_mode = <2>; /* OMAP_EHCI_PORT_MODE_TLL */ + port3_mode = <1>; /* OMAP_EHCI_PORT_MODE_PHY */ +}; + +&usbhsehci { + phy = <&hsusb1_phy 0 &hsusb3_phy>; +}; diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index f8ed08e..0f67856 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -1,8 +1,9 @@ /** * omap-usb-host.c - The USBHS core driver for OMAP EHCI & OHCI * - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com * Author: Keshava Munegowda <keshava_mgowda@ti.com> + * Author: Roger Quadros <rogerq@ti.com> * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 of @@ -27,6 +28,8 @@ #include <linux/platform_device.h> #include <linux/platform_data/usb-omap.h> #include <linux/pm_runtime.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include "omap-usb.h" @@ -464,6 +467,37 @@ static void omap_usbhs_init(struct device *dev) pm_runtime_put_sync(dev); } +static int usbhs_omap_get_dt_pdata(struct device_node *node, + struct usbhs_omap_platform_data *pdata) +{ + int ret, i; + + ret = of_property_read_u32(node, "nports", &pdata->nports); + if (ret) + pdata->nports = 0; + + /* get port modes */ + for (i = 0; i < OMAP3_HS_USB_PORTS; i++) { + char prop[11]; + + snprintf(prop, sizeof(prop), "port%d_mode", i + 1); + ret = of_property_read_u32(node, prop, &pdata->port_mode[i]); + if (ret) + pdata->port_mode[i] = OMAP_USBHS_PORT_MODE_UNUSED; + } + + /* get flags */ + pdata->single_ulpi_bypass = of_property_read_bool(node, + "single_ulpi_bypass"); + return 0; +} + +static struct of_device_id usbhs_child_match_table[] __initdata = { + { .compatible = "ti,omap-ehci", }, + { .compatible = "ti,omap-ohci", }, + { } +}; + /** * usbhs_omap_probe - initialize TI-based HCDs * @@ -479,6 +513,21 @@ static int usbhs_omap_probe(struct platform_device *pdev) int i; bool need_logic_fck; + if (dev->of_node) { + /* For DT boot we populate platform data from OF node */ + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return -ENOMEM; + + if (usbhs_omap_get_dt_pdata(dev->of_node, pdata)) { + dev_err(dev, + "Error getting platform data from DT node\n"); + return -ENODEV; + } + + dev->platform_data = pdata; + } + if (!pdata) { dev_err(dev, "Missing platform data\n"); return -ENODEV; @@ -490,7 +539,7 @@ static int usbhs_omap_probe(struct platform_device *pdev) return -ENOMEM; } - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "uhh"); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); omap->uhh_base = devm_request_and_ioremap(dev, res); if (!omap->uhh_base) { dev_err(dev, "Resource request/ioremap failed\n"); @@ -661,10 +710,23 @@ static int usbhs_omap_probe(struct platform_device *pdev) } omap_usbhs_init(dev); - ret = omap_usbhs_alloc_children(pdev); - if (ret) { - dev_err(dev, "omap_usbhs_alloc_children failed\n"); - goto err_alloc; + + if (dev->of_node) { + ret = of_platform_populate(dev->of_node, + usbhs_child_match_table, NULL, dev); + + if (ret) { + dev_err(dev, "Failed to create DT children: %d\n", ret); + goto err_alloc; + } + + } else { + ret = omap_usbhs_alloc_children(pdev); + if (ret) { + dev_err(dev, "omap_usbhs_alloc_children failed: %d\n", + ret); + goto err_alloc; + } } return 0; @@ -742,11 +804,20 @@ static const struct dev_pm_ops usbhsomap_dev_pm_ops = { .runtime_resume = usbhs_runtime_resume, }; +static const struct of_device_id usbhs_omap_dt_ids[] = { + { .compatible = "ti,usbhs-host" }, + { } +}; + +MODULE_DEVICE_TABLE(of, usbhs_omap_dt_ids); + + static struct platform_driver usbhs_omap_driver = { .driver = { .name = (char *)usbhs_driver_name, .owner = THIS_MODULE, .pm = &usbhsomap_dev_pm_ops, + .of_match_table = of_match_ptr(usbhs_omap_dt_ids), }, .remove = usbhs_omap_remove, };
Allows the OMAP HS USB host controller to be specified via device tree. CC: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Roger Quadros <rogerq@ti.com> --- .../devicetree/bindings/mfd/omap-usb-host.txt | 68 ++++++++++++++++ drivers/mfd/omap-usb-host.c | 83 ++++++++++++++++++-- 2 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt