| Message ID | 20160907213519.27340-4-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 7, 2016 at 4:35 PM, Stephen Boyd <stephen.boyd@linaro.org> 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 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. > > The ULPI bus code 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 up. To avoid any problems with reading the ID > registers before the phy is powered we fallback to DT matching > when the ID reads fail. > > 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 device matched up with DT we can 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 | 20 +++++++ > drivers/usb/common/ulpi.c | 77 ++++++++++++++++++++++++-- > 2 files changed, 92 insertions(+), 5 deletions(-) > create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt Acked-by: Rob Herring <robh@kernel.org> But one concern below. > -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 */ parent of the parent is called the grandparent. > + 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, First setting "parent = ulpi->dev.parent" would make this a bit easier on the eyes. When is it valid to be the grandparent? The binding doesn't mention that. Rob
Quoting Rob Herring (2016-09-07 18:12:32) > On Wed, Sep 7, 2016 at 4:35 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > > --- > > Documentation/devicetree/bindings/usb/ulpi.txt | 20 +++++++ > > drivers/usb/common/ulpi.c | 77 ++++++++++++++++++++++++-- > > 2 files changed, 92 insertions(+), 5 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt > > Acked-by: Rob Herring <robh@kernel.org> > > But one concern below. > > > -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 */ > > parent of the parent is called the grandparent. Heh, I can reword it. > > > + 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, > > First setting "parent = ulpi->dev.parent" would make this a bit easier > on the eyes. Ok. > > When is it valid to be the grandparent? The binding doesn't mention that. This case arises when we're registering the ulpi device as a child of the device created by chipidea usb glue drivers (the "core" chipidea device called something like ci_hdrc.N). In that case we have DT like this: usb-controller { ulpi { phy { }; }; }; where the usb-controller node corresponds to a device that's probed by the glue driver. That glue driver creates a child platform device, ci_hdrc.0, that doesn't have any DT node associated with it, and then that device driver probes and creates the ulpi device that is associated with the phy node. The grandparent scenario will fall away once Peter's patch is merged to set the of_node of ci_hdrc.0 to be the same of_node as the glue device[1]. I can rebase on that patch, but it looked like the power sequence stuff was getting held up. Honestly, having the child device registered by the glue driver seems to cause some difficulties and I've been thinking about how we could get rid of it, but Peter seems fairly adamant about keeping this design. Also, I think dwc3 is done in a similar way, but in that case the child device is in DT and I think we could put the ulpi node inside of it. I haven't looked in too much detail though. [1] http://lkml.kernel.org/r/1471252398-957-6-git-send-email-peter.chen@nxp.com
Quoting Stephen Boyd (2016-09-07 14:35:00) > @@ -174,6 +219,21 @@ 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; > > + /* Some ULPI devices don't have a vendor id so rely on OF match */ > + if (ulpi->id.vendor == 0) > + goto err; > + > + request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product); > + > + return 0; > +err: > + return of_device_request_module(&ulpi->dev); This can't return the value of of_device_request_module() because that returns an error if the module is builtin or if module loading is disabled. I'll have to ignore the error here and just return success all the time.
diff --git a/Documentation/devicetree/bindings/usb/ulpi.txt b/Documentation/devicetree/bindings/usb/ulpi.txt new file mode 100644 index 000000000000..ca179dc4bd50 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/ulpi.txt @@ -0,0 +1,20 @@ +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. + +EXAMPLE +------- + +usb { + compatible = "vendor,usb-controller"; + + ulpi { + phy { + compatible = "vendor,phy"; + }; + }; +}; diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index 01c0c0477a9e..24689e05a3df 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 vendor id so rely on OF match */ + if (ulpi->id.vendor == 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,21 +174,44 @@ 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); + of_node_put(np); + if (!child) + return -EINVAL; + + ulpi->dev.of_node = child; + + return 0; +} + +static int ulpi_read_id(struct ulpi *ulpi) { int ret; /* Test the interface */ ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa); if (ret < 0) - return ret; + goto err; ret = ulpi_read(ulpi, ULPI_SCRATCH); if (ret < 0) - return ret; + goto err; if (ret != 0xaa) - return -ENODEV; + goto err; ulpi->id.vendor = ulpi_read(ulpi, ULPI_VENDOR_ID_LOW); ulpi->id.vendor |= ulpi_read(ulpi, ULPI_VENDOR_ID_HIGH) << 8; @@ -174,6 +219,21 @@ 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; + /* Some ULPI devices don't have a vendor id so rely on OF match */ + if (ulpi->id.vendor == 0) + goto err; + + request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product); + + return 0; +err: + return of_device_request_module(&ulpi->dev); +} + +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; @@ -181,7 +241,13 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ACPI_COMPANION_SET(&ulpi->dev, ACPI_COMPANION(dev)); - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product); + ret = ulpi_of_register(ulpi); + if (ret) + return ret; + + ret = ulpi_read_id(ulpi); + if (ret) + return ret; ret = device_register(&ulpi->dev); if (ret) @@ -232,6 +298,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 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. The ULPI bus code 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 up. To avoid any problems with reading the ID registers before the phy is powered we fallback to DT matching when the ID reads fail. 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 device matched up with DT we can 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 | 20 +++++++ drivers/usb/common/ulpi.c | 77 ++++++++++++++++++++++++-- 2 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt