Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties

2016-08-24 Thread Heikki Krogerus 
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

Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties

2016-08-24 Thread Heikki Krogerus 
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

Re: [PATCH v2 03/22] usb: ulpi: Support device discovery via device properties

2016-08-23 Thread Stephen Boyd 
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

2016-08-23 Thread Stephen Boyd 
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

2016-08-23 Thread Rob Herring 
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

2016-08-23 Thread Rob Herring 
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

2016-08-23 Thread Stephen Boyd 
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

2016-08-23 Thread Stephen Boyd 
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

2016-08-23 Thread Stephen Boyd 
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

2016-08-23 Thread Stephen Boyd 
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

2016-07-17 Thread Rob Herring 
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

2016-07-17 Thread Rob Herring 
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

2016-07-08 Thread Peter Chen 
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 = _bus;
>   ulpi->dev.type = _dev_type;
>   dev_set_name(>dev, "%s.ulpi", dev_name(dev));
>  
> + if (IS_ENABLED(CONFIG_OF)) {
> + ret = ulpi_of_register(ulpi);
> + if (ret)
> + return ret;
> + }
> +
>   ACPI_COMPANION_SET(>dev, ACPI_COMPANION(dev));
>  
> - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> + ret = device_property_read_u16(>dev, "ulpi-vendor",
> +>id.vendor);
> + ret |= device_property_read_u16(>dev, "ulpi-product",
> + >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

2016-07-08 Thread Peter Chen 
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 = _bus;
>   ulpi->dev.type = _dev_type;
>   dev_set_name(>dev, "%s.ulpi", dev_name(dev));
>  
> + if (IS_ENABLED(CONFIG_OF)) {
> + ret = ulpi_of_register(ulpi);
> + if (ret)
> + return ret;
> + }
> +
>   ACPI_COMPANION_SET(>dev, ACPI_COMPANION(dev));
>  
> - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> + ret = device_property_read_u16(>dev, "ulpi-vendor",
> +>id.vendor);
> + ret |= device_property_read_u16(>dev, "ulpi-product",
> + >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

[PATCH v2 03/22] usb: ulpi: Support device discovery via device properties

2016-07-07 Thread Stephen Boyd 
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>;
+   };
+   };
+};
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 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 /* -- 
*/
 
@@ -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,

[PATCH v2 03/22] usb: ulpi: Support device discovery via device properties

2016-07-07 Thread Stephen Boyd 
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>;
+   };
+   };
+};
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 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 /* -- 
*/
 
@@ -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 =