| Message ID | 20160707222114.1673-4-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > struct ulpi *ulpi = to_ulpi_dev(dev); > const struct ulpi_device_id *id; > > + /* Some ULPI devices don't have a product id so rely on OF match */ > + if (ulpi->id.product == 0) > + return of_driver_match_device(dev, driver); > + How about using vendor id? It can't be 0, but pid may be 0. See: http://www.linux-usb.org/usb.ids > +static int ulpi_of_register(struct ulpi *ulpi) > +{ > + struct device_node *np = NULL, *child; > + > + /* Find a ulpi bus underneath the parent or the parent of the parent */ > + if (ulpi->dev.parent->of_node) > + np = of_find_node_by_name(ulpi->dev.parent->of_node, "ulpi"); > + else if (ulpi->dev.parent->parent && ulpi->dev.parent->parent->of_node) > + np = of_find_node_by_name(ulpi->dev.parent->parent->of_node, > + "ulpi"); > + if (!np) > + return 0; > + > + child = of_get_next_available_child(np, NULL); > + if (!child) > + return -EINVAL; You may need to call of_node_put on parent (np), not on child node below. > + > + ulpi->dev.of_node = child; > + > + return 0; > +} > + > +static int ulpi_read_id(struct ulpi *ulpi) > { > int ret; > > @@ -174,14 +218,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); > ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; > > + return 0; > +} > + What does this API for? Why it still needs to be called after vid/pid gets from firmware? > +static int ulpi_register(struct device *dev, struct ulpi *ulpi) > +{ > + int ret; > + > ulpi->dev.parent = dev; > ulpi->dev.bus = &ulpi_bus; > ulpi->dev.type = &ulpi_dev_type; > dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev)); > > + if (IS_ENABLED(CONFIG_OF)) { > + ret = ulpi_of_register(ulpi); > + if (ret) > + return ret; > + } > + > ACPI_COMPANION_SET(&ulpi->dev, ACPI_COMPANION(dev)); > > - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product); > + ret = device_property_read_u16(&ulpi->dev, "ulpi-vendor", > + &ulpi->id.vendor); > + ret |= device_property_read_u16(&ulpi->dev, "ulpi-product", > + &ulpi->id.product); > + if (ret) { > + ret = ulpi_read_id(ulpi); > + if (ret) > + return ret; > + } > + [...] > void ulpi_unregister_interface(struct ulpi *ulpi) > { > + of_node_put(ulpi->dev.of_node); [...]
On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > The qcom HSIC ULPI phy doesn't have any bits set in the vendor or > product ID registers. This makes it impossible to make a ULPI > driver match against the ID registers. Add support to discover > the ULPI phys via DT/device properties to help alleviate this > problem. In the DT case, we'll look for a ULPI bus node > underneath the device registering the ULPI viewport (or the > parent of that device to support chipidea's device layout) and > then match up the phy node underneath that with the ULPI device > that's created. > > The side benefit of this is that we can use standard properties > in the phy node like clks, regulators, gpios, etc. because we > don't have firmware like ACPI to turn these things on for us. And > we can use the DT phy binding to point our phy consumer to the > phy provider. > > Furthermore, this avoids any problems with reading the ID > registers before the phy is powered up. The ULPI bus supports > native enumeration by reading the vendor ID and product ID > registers at device creation time, but we can't be certain that > those register reads will succeed if the phy is not powered. > > If the ULPI spec had some generic power sequencing for these > registers we could put that into the ULPI bus layer and power up > the device before reading the ID registers. Unfortunately this > doesn't exist and the power sequence is usually device specific. > By having the vendor and product ID properties in ACPI or DT, we > can match up devices with drivers without having to read the > hardware before it's powered up and avoid this problem. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: <devicetree@vger.kernel.org> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > Documentation/devicetree/bindings/usb/ulpi.txt | 35 ++++++++++++ > drivers/usb/common/ulpi.c | 74 +++++++++++++++++++++++++- > 2 files changed, 107 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt > > diff --git a/Documentation/devicetree/bindings/usb/ulpi.txt b/Documentation/devicetree/bindings/usb/ulpi.txt > new file mode 100644 > index 000000000000..c649ca5b0996 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ulpi.txt > @@ -0,0 +1,35 @@ > +ULPI bus binding > +---------------- > + > +Phys that are behind a ULPI connection can be described with the following > +binding. The host controller shall have a "ulpi" named node as a child, and > +that node shall have one enabled node underneath it representing the ulpi > +device on the bus. > + > +PROPERTIES > +---------- > + > +- ulpi-vendor: > + Usage: optional > + Value type: <u16> > + Definition: The USB-IF assigned vendor id for this device > + > +- ulpi-product: > + Usage: required if ulpi-vendor is present > + Value type: <u16> > + Definition: The vendor assigned product id for this device > + > +EXAMPLE > +------- > + > +usb { > + compatible = "vendor,usb-controller"; > + > + ulpi { > + phy { > + compatible = "vendor,phy"; > + ulpi-vendor = /bits/ 16 <0x1d6b>; > + ulpi-product = /bits/ 16 <0x0002>; > + }; > + }; I'm still having concerns about describing both phys and devices. If I have a controller with 2 ports and 2 devices attached, I'd have something like this under the USB controller: ulpi { phy@1 { }; phy@2 { }; }; dev@1 { ... }; dev@2 { ... }; That doesn't seem the best, but I don't have a better suggestion. Maybe the device nodes need to go under the phy nodes? Rob
Quoting Peter Chen (2016-07-08 02:04:58) > On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > > struct ulpi *ulpi = to_ulpi_dev(dev); > > const struct ulpi_device_id *id; > > > > + /* Some ULPI devices don't have a product id so rely on OF match */ > > + if (ulpi->id.product == 0) > > + return of_driver_match_device(dev, driver); > > + > > How about using vendor id? It can't be 0, but pid may be 0. > See: http://www.linux-usb.org/usb.ids Heikki suggested a product id of 0 would mean we need to use DT matching. Should it be changed to vendor id instead? > > > +static int ulpi_of_register(struct ulpi *ulpi) > > +{ > > + struct device_node *np = NULL, *child; > > + > > + /* Find a ulpi bus underneath the parent or the parent of the parent */ > > + if (ulpi->dev.parent->of_node) > > + np = of_find_node_by_name(ulpi->dev.parent->of_node, "ulpi"); > > + else if (ulpi->dev.parent->parent && ulpi->dev.parent->parent->of_node) > > + np = of_find_node_by_name(ulpi->dev.parent->parent->of_node, > > + "ulpi"); > > + if (!np) > > + return 0; > > + > > + child = of_get_next_available_child(np, NULL); > > + if (!child) > > + return -EINVAL; > > You may need to call of_node_put on parent (np), not on child node > below. Fixed, thanks. > > > + > > + ulpi->dev.of_node = child; > > + > > + return 0; > > +} > > + > > +static int ulpi_read_id(struct ulpi *ulpi) > > { > > int ret; > > > > @@ -174,14 +218,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > > ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); > > ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; > > > > + return 0; > > +} > > + > > What does this API for? Why it still needs to be called after > vid/pid gets from firmware? Ideally we get the vid/pid from the ULPI registers instead of from firmware. So we typically want to call ulpi_read_id() because most likely the ulpi-vendor and ulpi-product properties are missing.
Quoting Rob Herring (2016-07-17 19:23:55) > On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > > +------- > > + > > +usb { > > + compatible = "vendor,usb-controller"; > > + > > + ulpi { > > + phy { > > + compatible = "vendor,phy"; > > + ulpi-vendor = /bits/ 16 <0x1d6b>; > > + ulpi-product = /bits/ 16 <0x0002>; > > + }; > > + }; > > I'm still having concerns about describing both phys and devices. If I > have a controller with 2 ports and 2 devices attached, I'd have > something like this under the USB controller: > > ulpi { > phy@1 { > }; > phy@2 { > }; > }; My understanding is there would only be one status="ok" node on the ULPI bus for the single phy that a usb controller would have. At the least, the kernel's ULPI layer only seems to support one ULPI phy for a controller right now. So even if there are two ports, it doesn't mean there are two phys. > > dev@1 { > ... > }; > > dev@2 { > ... > }; > > > That doesn't seem the best, but I don't have a better suggestion. Maybe > the device nodes need to go under the phy nodes? > What if we moved the dev@1 and dev@2 to another sub node like "ports" or "usb-devices"? Legacy code can support having those devices directly underneath the usb controller, but future users would always need to put them in a different sub-node so that we can easily differentiate the different busses that a usb controller node may support? I'm not sure I see any need to relate the phy to the ports that are on the controller, but if that is needed then perhaps you're right and we should move the ports underneath the phy. USB core could be modified to go through the legacy path or through the phy, if it even exists, to find ports. Do we typically do this for other phy designs like sata or pci? The phy always seemed like a parallel thing to the logical bus that the phy is used for.
On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > Quoting Peter Chen (2016-07-08 02:04:58) >> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: >> > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) >> > struct ulpi *ulpi = to_ulpi_dev(dev); >> > const struct ulpi_device_id *id; >> > >> > + /* Some ULPI devices don't have a product id so rely on OF match */ >> > + if (ulpi->id.product == 0) >> > + return of_driver_match_device(dev, driver); >> > + >> >> How about using vendor id? It can't be 0, but pid may be 0. >> See: http://www.linux-usb.org/usb.ids > > Heikki suggested a product id of 0 would mean we need to use DT > matching. Should it be changed to vendor id instead? Any comments here?
On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > Quoting Rob Herring (2016-07-17 19:23:55) >> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: >> > +------- >> > + >> > +usb { >> > + compatible = "vendor,usb-controller"; >> > + >> > + ulpi { >> > + phy { >> > + compatible = "vendor,phy"; >> > + ulpi-vendor = /bits/ 16 <0x1d6b>; >> > + ulpi-product = /bits/ 16 <0x0002>; >> > + }; >> > + }; >> >> I'm still having concerns about describing both phys and devices. If I >> have a controller with 2 ports and 2 devices attached, I'd have >> something like this under the USB controller: >> >> ulpi { >> phy@1 { >> }; >> phy@2 { >> }; >> }; > > My understanding is there would only be one status="ok" node on the ULPI > bus for the single phy that a usb controller would have. At the least, > the kernel's ULPI layer only seems to support one ULPI phy for a > controller right now. So even if there are two ports, it doesn't mean > there are two phys. > >> >> dev@1 { >> ... >> }; >> >> dev@2 { >> ... >> }; >> >> >> That doesn't seem the best, but I don't have a better suggestion. Maybe >> the device nodes need to go under the phy nodes? >> > > What if we moved the dev@1 and dev@2 to another sub node like "ports" or > "usb-devices"? Legacy code can support having those devices directly > underneath the usb controller, but future users would always need to put > them in a different sub-node so that we can easily differentiate the > different busses that a usb controller node may support? > > I'm not sure I see any need to relate the phy to the ports that are on > the controller, but if that is needed then perhaps you're right and we > should move the ports underneath the phy. USB core could be modified to > go through the legacy path or through the phy, if it even exists, to > find ports. > > Do we typically do this for other phy designs like sata or pci? The phy > always seemed like a parallel thing to the logical bus that the phy is > used for. Rob does this sound ok to you?
On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: >> Quoting Rob Herring (2016-07-17 19:23:55) >>> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: >>> > +------- >>> > + >>> > +usb { >>> > + compatible = "vendor,usb-controller"; >>> > + >>> > + ulpi { >>> > + phy { >>> > + compatible = "vendor,phy"; >>> > + ulpi-vendor = /bits/ 16 <0x1d6b>; >>> > + ulpi-product = /bits/ 16 <0x0002>; >>> > + }; >>> > + }; >>> >>> I'm still having concerns about describing both phys and devices. If I >>> have a controller with 2 ports and 2 devices attached, I'd have >>> something like this under the USB controller: >>> >>> ulpi { >>> phy@1 { >>> }; >>> phy@2 { >>> }; >>> }; >> >> My understanding is there would only be one status="ok" node on the ULPI >> bus for the single phy that a usb controller would have. At the least, >> the kernel's ULPI layer only seems to support one ULPI phy for a >> controller right now. So even if there are two ports, it doesn't mean >> there are two phys. >> >>> >>> dev@1 { >>> ... >>> }; >>> >>> dev@2 { >>> ... >>> }; >>> >>> >>> That doesn't seem the best, but I don't have a better suggestion. Maybe >>> the device nodes need to go under the phy nodes? >>> >> >> What if we moved the dev@1 and dev@2 to another sub node like "ports" or >> "usb-devices"? Legacy code can support having those devices directly >> underneath the usb controller, but future users would always need to put >> them in a different sub-node so that we can easily differentiate the >> different busses that a usb controller node may support? >> >> I'm not sure I see any need to relate the phy to the ports that are on >> the controller, but if that is needed then perhaps you're right and we >> should move the ports underneath the phy. USB core could be modified to >> go through the legacy path or through the phy, if it even exists, to >> find ports. >> >> Do we typically do this for other phy designs like sata or pci? The phy >> always seemed like a parallel thing to the logical bus that the phy is >> used for. > > Rob does this sound ok to you? Well, if there's only ever 1 phy under the controller, then as you had it is fine. Rob
On Tue, Aug 23, 2016 at 4:06 PM, Rob Herring <robh@kernel.org> wrote: > On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: >> On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: >>> Quoting Rob Herring (2016-07-17 19:23:55) >>>> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: >>>> > +------- >>>> > + >>>> > +usb { >>>> > + compatible = "vendor,usb-controller"; >>>> > + >>>> > + ulpi { >>>> > + phy { >>>> > + compatible = "vendor,phy"; >>>> > + ulpi-vendor = /bits/ 16 <0x1d6b>; >>>> > + ulpi-product = /bits/ 16 <0x0002>; >>>> > + }; >>>> > + }; >>>> >>>> I'm still having concerns about describing both phys and devices. If I >>>> have a controller with 2 ports and 2 devices attached, I'd have >>>> something like this under the USB controller: >>>> >>>> ulpi { >>>> phy@1 { >>>> }; >>>> phy@2 { >>>> }; >>>> }; >>> >>> My understanding is there would only be one status="ok" node on the ULPI >>> bus for the single phy that a usb controller would have. At the least, >>> the kernel's ULPI layer only seems to support one ULPI phy for a >>> controller right now. So even if there are two ports, it doesn't mean >>> there are two phys. >>> >>>> >>>> dev@1 { >>>> ... >>>> }; >>>> >>>> dev@2 { >>>> ... >>>> }; >>>> >>>> >>>> That doesn't seem the best, but I don't have a better suggestion. Maybe >>>> the device nodes need to go under the phy nodes? >>>> >>> >>> What if we moved the dev@1 and dev@2 to another sub node like "ports" or >>> "usb-devices"? Legacy code can support having those devices directly >>> underneath the usb controller, but future users would always need to put >>> them in a different sub-node so that we can easily differentiate the >>> different busses that a usb controller node may support? >>> >>> I'm not sure I see any need to relate the phy to the ports that are on >>> the controller, but if that is needed then perhaps you're right and we >>> should move the ports underneath the phy. USB core could be modified to >>> go through the legacy path or through the phy, if it even exists, to >>> find ports. >>> >>> Do we typically do this for other phy designs like sata or pci? The phy >>> always seemed like a parallel thing to the logical bus that the phy is >>> used for. >> >> Rob does this sound ok to you? > > Well, if there's only ever 1 phy under the controller, then as you had > it is fine. > Ok. For ULPI I believe that's the case, but in general usb controllers can have more than one phy.
On Tue, Aug 23, 2016 at 12:58:07PM -0700, Stephen Boyd wrote: > On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > > Quoting Peter Chen (2016-07-08 02:04:58) > >> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > >> > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > >> > struct ulpi *ulpi = to_ulpi_dev(dev); > >> > const struct ulpi_device_id *id; > >> > > >> > + /* Some ULPI devices don't have a product id so rely on OF match */ > >> > + if (ulpi->id.product == 0) > >> > + return of_driver_match_device(dev, driver); > >> > + > >> > >> How about using vendor id? It can't be 0, but pid may be 0. > >> See: http://www.linux-usb.org/usb.ids > > > > Heikki suggested a product id of 0 would mean we need to use DT > > matching. Should it be changed to vendor id instead? > > Any comments here? It makes sense. I don't have any problem with that. Thanks,
Quoting Heikki Krogerus (2016-08-24 00:31:34) > On Tue, Aug 23, 2016 at 12:58:07PM -0700, Stephen Boyd wrote: > > On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > > > Quoting Peter Chen (2016-07-08 02:04:58) > > >> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote: > > >> > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) > > >> > struct ulpi *ulpi = to_ulpi_dev(dev); > > >> > const struct ulpi_device_id *id; > > >> > > > >> > + /* Some ULPI devices don't have a product id so rely on OF match */ > > >> > + if (ulpi->id.product == 0) > > >> > + return of_driver_match_device(dev, driver); > > >> > + > > >> > > >> How about using vendor id? It can't be 0, but pid may be 0. > > >> See: http://www.linux-usb.org/usb.ids > > > > > > Heikki suggested a product id of 0 would mean we need to use DT > > > matching. Should it be changed to vendor id instead? > > > > Any comments here? > > It makes sense. I don't have any problem with that. Ok. Having pid equal to 0 in DT/device properties seems wrong, so I think I'll be dropping those properties that this patch introduced and go back to always attempting to read the hardware when the device is populated. That will bring back the power sequencing problem though, so I have to think of some way to solve that. For now I'll just set the pid and vid to 0 when the power is off and we try to read and see how it goes.
diff --git a/Documentation/devicetree/bindings/usb/ulpi.txt b/Documentation/devicetree/bindings/usb/ulpi.txt new file mode 100644 index 000000000000..c649ca5b0996 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/ulpi.txt @@ -0,0 +1,35 @@ +ULPI bus binding +---------------- + +Phys that are behind a ULPI connection can be described with the following +binding. The host controller shall have a "ulpi" named node as a child, and +that node shall have one enabled node underneath it representing the ulpi +device on the bus. + +PROPERTIES +---------- + +- ulpi-vendor: + Usage: optional + Value type: <u16> + Definition: The USB-IF assigned vendor id for this device + +- ulpi-product: + Usage: required if ulpi-vendor is present + Value type: <u16> + Definition: The vendor assigned product id for this device + +EXAMPLE +------- + +usb { + compatible = "vendor,usb-controller"; + + ulpi { + phy { + compatible = "vendor,phy"; + ulpi-vendor = /bits/ 16 <0x1d6b>; + ulpi-product = /bits/ 16 <0x0002>; + }; + }; +}; diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index 01c0c0477a9e..3e8dd7b57aaf 100644 --- a/drivers/usb/common/ulpi.c +++ b/drivers/usb/common/ulpi.c @@ -16,6 +16,9 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/clk/clk-conf.h> /* -------------------------------------------------------------------------- */ @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) struct ulpi *ulpi = to_ulpi_dev(dev); const struct ulpi_device_id *id; + /* Some ULPI devices don't have a product id so rely on OF match */ + if (ulpi->id.product == 0) + return of_driver_match_device(dev, driver); + for (id = drv->id_table; id->vendor; id++) if (id->vendor == ulpi->id.vendor && id->product == ulpi->id.product) @@ -50,6 +57,11 @@ static int ulpi_match(struct device *dev, struct device_driver *driver) static int ulpi_uevent(struct device *dev, struct kobj_uevent_env *env) { struct ulpi *ulpi = to_ulpi_dev(dev); + int ret; + + ret = of_device_uevent_modalias(dev, env); + if (ret != -ENODEV) + return ret; if (add_uevent_var(env, "MODALIAS=ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product)) @@ -60,6 +72,11 @@ static int ulpi_uevent(struct device *dev, struct kobj_uevent_env *env) static int ulpi_probe(struct device *dev) { struct ulpi_driver *drv = to_ulpi_driver(dev->driver); + int ret; + + ret = of_clk_set_defaults(dev->of_node, false); + if (ret < 0) + return ret; return drv->probe(to_ulpi_dev(dev)); } @@ -87,8 +104,13 @@ static struct bus_type ulpi_bus = { static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, char *buf) { + int len; struct ulpi *ulpi = to_ulpi_dev(dev); + len = of_device_get_modalias(dev, buf, PAGE_SIZE - 1); + if (len != -ENODEV) + return len; + return sprintf(buf, "ulpi:v%04xp%04x\n", ulpi->id.vendor, ulpi->id.product); } @@ -152,7 +174,29 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver); /* -------------------------------------------------------------------------- */ -static int ulpi_register(struct device *dev, struct ulpi *ulpi) +static int ulpi_of_register(struct ulpi *ulpi) +{ + struct device_node *np = NULL, *child; + + /* Find a ulpi bus underneath the parent or the parent of the parent */ + if (ulpi->dev.parent->of_node) + np = of_find_node_by_name(ulpi->dev.parent->of_node, "ulpi"); + else if (ulpi->dev.parent->parent && ulpi->dev.parent->parent->of_node) + np = of_find_node_by_name(ulpi->dev.parent->parent->of_node, + "ulpi"); + if (!np) + return 0; + + child = of_get_next_available_child(np, NULL); + if (!child) + return -EINVAL; + + ulpi->dev.of_node = child; + + return 0; +} + +static int ulpi_read_id(struct ulpi *ulpi) { int ret; @@ -174,14 +218,39 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW); ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8; + return 0; +} + +static int ulpi_register(struct device *dev, struct ulpi *ulpi) +{ + int ret; + ulpi->dev.parent = dev; ulpi->dev.bus = &ulpi_bus; ulpi->dev.type = &ulpi_dev_type; dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev)); + if (IS_ENABLED(CONFIG_OF)) { + ret = ulpi_of_register(ulpi); + if (ret) + return ret; + } + ACPI_COMPANION_SET(&ulpi->dev, ACPI_COMPANION(dev)); - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product); + ret = device_property_read_u16(&ulpi->dev, "ulpi-vendor", + &ulpi->id.vendor); + ret |= device_property_read_u16(&ulpi->dev, "ulpi-product", + &ulpi->id.product); + if (ret) { + ret = ulpi_read_id(ulpi); + if (ret) + return ret; + } + + if (of_device_request_module(&ulpi->dev)) + request_module("ulpi:v%04xp%04x", ulpi->id.vendor, + ulpi->id.product); ret = device_register(&ulpi->dev); if (ret) @@ -232,6 +301,7 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface); */ void ulpi_unregister_interface(struct ulpi *ulpi) { + of_node_put(ulpi->dev.of_node); device_unregister(&ulpi->dev); } EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
The qcom HSIC ULPI phy doesn't have any bits set in the vendor or product ID registers. This makes it impossible to make a ULPI driver match against the ID registers. Add support to discover the ULPI phys via DT/device properties to help alleviate this problem. In the DT case, we'll look for a ULPI bus node underneath the device registering the ULPI viewport (or the parent of that device to support chipidea's device layout) and then match up the phy node underneath that with the ULPI device that's created. The side benefit of this is that we can use standard properties in the phy node like clks, regulators, gpios, etc. because we don't have firmware like ACPI to turn these things on for us. And we can use the DT phy binding to point our phy consumer to the phy provider. Furthermore, this avoids any problems with reading the ID registers before the phy is powered up. The ULPI bus supports native enumeration by reading the vendor ID and product ID registers at device creation time, but we can't be certain that those register reads will succeed if the phy is not powered. If the ULPI spec had some generic power sequencing for these registers we could put that into the ULPI bus layer and power up the device before reading the ID registers. Unfortunately this doesn't exist and the power sequence is usually device specific. By having the vendor and product ID properties in ACPI or DT, we can match up devices with drivers without having to read the hardware before it's powered up and avoid this problem. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: <devicetree@vger.kernel.org> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- Documentation/devicetree/bindings/usb/ulpi.txt | 35 ++++++++++++ drivers/usb/common/ulpi.c | 74 +++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt