Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
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 > Cc: Heikki Krogerus > Cc: > Cc: Rob Herring > Signed-off-by: Stephen Boyd > --- > 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 ..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: > +Definition: The USB-IF assigned vendor id for this device > + > +- ulpi-product: > +Usage: required if ulpi-vendor is present > +Value type: > +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
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
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); [...] -- Best Regards, Peter Chen
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd 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?
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd 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?
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd wrote: > On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd 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
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
On Tue, Aug 23, 2016 at 4:06 PM, Rob Herring wrote: > On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd wrote: >> On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd 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.
Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties
On Tue, Aug 23, 2016 at 12:58:07PM -0700, Stephen Boyd wrote: > On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd 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, -- heikki