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
