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

[Stephen Boyd]

On Fri, Sep 2, 2016 at 7:09 AM, Heikki Krogerus
 wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 05:40:17PM -0700, Stephen Boyd wrote:
>> @@ -174,14 +219,37 @@ 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;
>> +}
>> +
>> +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)) {
>
> I don't think you need to check that in this case.
>
>> + ret = ulpi_of_register(ulpi);
>> + if (ret)
>> + return ret;
>> + }
>> +
>>   ACPI_COMPANION_SET(>dev, ACPI_COMPANION(dev));
>
> ACPI_COMPANION_SET will overwrite the primary fwnode unconditionally,
> so just to play it safe, do this before you call ulpi_of_register().

Ok.

>
>> - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
>> + ret = ulpi_read_id(ulpi);
>> + /*
>> +  * Ignore failure in case of DT node because the device may
>> +  * not be powered up yet but we can still match by compatible
>> +  */
>> + if (ret && !ulpi->dev.of_node)
>> + return ret;
>> +
>> + if (of_device_request_module(>dev))
>> + request_module("ulpi:v%04xp%04x", ulpi->id.vendor,
>> +ulpi->id.product);
>
> I don't think this works in all cases. If of_device_request_module()
> fails and we don't have the id.vendor/product set, we should not
> register the device. It also looks a bit messy.
>
> How about just using of_device_request_module() call as fallback in
> ulpi_read_id() and moving also request_module() call there:

Sure I'll fold it in and test. Should we "goto err" if we can't read
the scratch register though? I would think that's a "real" failure and
we shouldn't try to support DT in that case.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Heikki Krogerus]

Hi,

On Wed, Aug 31, 2016 at 05:40:17PM -0700, Stephen Boyd wrote:
> @@ -174,14 +219,37 @@ 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;
> +}
> +
> +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)) {

I don't think you need to check that in this case.

> + ret = ulpi_of_register(ulpi);
> + if (ret)
> + return ret;
> + }
> +
>   ACPI_COMPANION_SET(>dev, ACPI_COMPANION(dev));

ACPI_COMPANION_SET will overwrite the primary fwnode unconditionally,
so just to play it safe, do this before you call ulpi_of_register().

> - request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> + ret = ulpi_read_id(ulpi);
> + /*
> +  * Ignore failure in case of DT node because the device may
> +  * not be powered up yet but we can still match by compatible
> +  */
> + if (ret && !ulpi->dev.of_node)
> + return ret;
> +
> + if (of_device_request_module(>dev))
> + request_module("ulpi:v%04xp%04x", ulpi->id.vendor,
> +ulpi->id.product);

I don't think this works in all cases. If of_device_request_module()
fails and we don't have the id.vendor/product set, we should not
register the device. It also looks a bit messy.

How about just using of_device_request_module() call as fallback in
ulpi_read_id() and moving also request_module() call there:

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 30ea770..667246c 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -205,14 +205,14 @@ static int ulpi_read_id(struct ulpi *ulpi)
/* 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;
@@ -220,7 +220,11 @@ static int ulpi_read_id(struct ulpi *ulpi)
ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW);
ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8;
 
+   request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
+
return 0;
+err:
+   return of_device_request_module(>dev);
 }
 
 static int ulpi_register(struct device *dev, struct ulpi *ulpi)
@@ -232,25 +236,15 @@ static int ulpi_register(struct device *dev, struct ulpi 
*ulpi)
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));
 
-   ret = ulpi_read_id(ulpi);
-   /*
-* Ignore failure in case of DT node because the device may
-* not be powered up yet but we can still match by compatible
-*/
-   if (ret && !ulpi->dev.of_node)
+   ret = ulpi_of_register(ulpi);
+   if (ret)
return ret;
 
-   if (of_device_request_module(>dev))
-   request_module("ulpi:v%04xp%04x", ulpi->id.vendor,
-  ulpi->id.product);
+   ret = ulpi_read_id(ulpi);
+   if (ret)
+   return ret;
 
ret = device_register(>dev);
if (ret)


Cheers,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 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 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 | 20 +++
 drivers/usb/common/ulpi.c  | 73 +-
 2 files changed, 91 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 ..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..33039a374847 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 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,7 +174,30 @@