| Message ID | 20160626072838.28082-3-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
[auto build test ERROR on peter.chen-usb/ci-for-usb-next]
[also build test ERROR on v4.7-rc5 next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Stephen-Boyd/Support-qcom-s-HSIC-USB-and-rewrite-USB2-HS-phy-support/20160627-102637
base: https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb ci-for-usb-next
config: x86_64-randconfig-h0-06270614 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "of_device_request_module" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_get_modalias" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_uevent_modalias" [drivers/usb/common/ulpi.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or > product id ulpi registers. This makes it impossible to make a > ulpi driver match against the id registers. Add support to > discover the ulpi phys via DT to help alleviate this problem. > 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 DT > 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. > > 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 | 56 +++++++++++++++++++++++++- > 2 files changed, 74 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..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. This needs to co-exist with the USB bus binding which has the controller ports for the child nodes. Maybe use the phy binding? > + > +EXAMPLE > +------- > + > +usb { > + compatible = "vendor,usb-controller"; > + > + ulpi { > + phy { > + compatible = "vendor,phy"; > + }; > + }; > +};
Quoting Rob Herring (2016-06-28 13:56:42) > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or > > product id ulpi registers. This makes it impossible to make a > > ulpi driver match against the id registers. Add support to > > discover the ulpi phys via DT to help alleviate this problem. > > 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 DT > > 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. > > > > 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 | 56 +++++++++++++++++++++++++- > > 2 files changed, 74 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..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. > > This needs to co-exist with the USB bus binding which has the controller > ports for the child nodes. Maybe use the phy binding? Which binding is that? bindings/usb/usb-device.txt? This ulpi binding is to describe phys that are accessed through the ulpi "viewport" in the usb controller. So controller ports don't come into the picture here.
On Tue, Jun 28, 2016 at 03:09:21PM -0700, Stephen Boyd wrote: > Quoting Rob Herring (2016-06-28 13:56:42) > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > > > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or > > > product id ulpi registers. This makes it impossible to make a > > > ulpi driver match against the id registers. Add support to > > > discover the ulpi phys via DT to help alleviate this problem. > > > 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 DT > > > 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. > > > > > > 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 | 56 +++++++++++++++++++++++++- > > > 2 files changed, 74 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..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. > > > > This needs to co-exist with the USB bus binding which has the controller > > ports for the child nodes. Maybe use the phy binding? > > Which binding is that? bindings/usb/usb-device.txt? Yes. > This ulpi binding is > to describe phys that are accessed through the ulpi "viewport" in the > usb controller. So controller ports don't come into the picture here. You just need to confirm that there's no collision with child nodes like it assumes all children are ports. Rob
Quoting Rob Herring (2016-06-30 17:59:15) > On Tue, Jun 28, 2016 at 03:09:21PM -0700, Stephen Boyd wrote: > > Quoting Rob Herring (2016-06-28 13:56:42) > > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote: > > > > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or > > > > product id ulpi registers. This makes it impossible to make a > > > > ulpi driver match against the id registers. Add support to > > > > discover the ulpi phys via DT to help alleviate this problem. > > > > 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 DT > > > > 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. > > > > > > > > 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 | 56 +++++++++++++++++++++++++- > > > > 2 files changed, 74 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..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. > > > > > > This needs to co-exist with the USB bus binding which has the controller > > > ports for the child nodes. Maybe use the phy binding? > > > > Which binding is that? bindings/usb/usb-device.txt? > > Yes. > > > This ulpi binding is > > to describe phys that are accessed through the ulpi "viewport" in the > > usb controller. So controller ports don't come into the picture here. > > You just need to confirm that there's no collision with child nodes like > it assumes all children are ports. > Ok. It will work with the way usb_of_get_child_node() is written but it doesn't seem like great DT design if we want to support usb-device binding and ulpi bus at the same time, because we treat the controller node as the usb bus. &usb1 { status = "okay"; #address-cells = <1>; #size-cells = <0>; phys = <&uphy>; phy-names = "usb-phy"; hub: genesys@1 { compatible = "usb5e3,608"; reg = <1>; }; ulpi { uphy: phy { compatible = "phy"; }; }; } In this example, the ulpi bus doesn't have a reg property because it isn't a port. Perhaps we can move the usb ports to a subnode like 'usb' so that we can have two logical busses underneath the controller, one for usb and one for ulpi? We would need backwards compatibility code that looks for a ports directly underneath the usb controller node when #address-cells and #size-cells are present, otherwise it should fall down into a 'usb' subnode.
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..980af672bfe3 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,7 +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; - for (id = drv->id_table; id->vendor; id++) + if (of_driver_match_device(dev, driver)) + return 1; + + for (id = drv->id_table; id && id->vendor; id++) if (id->vendor == ulpi->id.vendor && id->product == ulpi->id.product) return 1; @@ -50,6 +56,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 +71,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 +103,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,6 +173,28 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver); /* -------------------------------------------------------------------------- */ +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_register(struct device *dev, struct ulpi *ulpi) { int ret; @@ -181,7 +224,15 @@ 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); + if (IS_ENABLED(CONFIG_OF)) { + ret = ulpi_of_register(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 +283,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 ulpi registers. This makes it impossible to make a ulpi driver match against the id registers. Add support to discover the ulpi phys via DT to help alleviate this problem. 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 DT 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. 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 | 56 +++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/ulpi.txt