Date:   Thu, 19 Jan 2017 14:33:49 +0800
From:   Peter Chen <hzpeterchen@...il.com>
To:     Stephen Boyd <stephen.boyd@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Neil Armstrong <narmstrong@...libre.com>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        devicetree@...r.kernel.org, Peter Chen <peter.chen@....com>,
        Andy Gross <andy.gross@...aro.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 03/25] usb: ulpi: Support device discovery via DT

On Wed, Dec 28, 2016 at 02:56:49PM -0800, 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 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 device matched up with DT we can avoid this
> problem.
> 
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Acked-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Cc: <devicetree@...r.kernel.org>
> Acked-by: Rob Herring <robh@...nel.org>
> Signed-off-by: Stephen Boyd <stephen.boyd@...aro.org>

Greg, is it ok I pick up this patch, and send it with chipidea
changes together for 4.11-rc1 later?

Peter

> ---
>  Documentation/devicetree/bindings/usb/ulpi.txt | 20 +++++++
>  drivers/usb/common/ulpi.c                      | 79 ++++++++++++++++++++++++--
>  2 files changed, 93 insertions(+), 6 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.
> +
> +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 8b317702d761..c9480d77810c 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,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);
>  }
> @@ -153,23 +175,45 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
>  
>  /* -------------------------------------------------------------------------- */
>  
> -static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> +static int ulpi_of_register(struct ulpi *ulpi)
>  {
> -	int ret;
> +	struct device_node *np = NULL, *child;
> +	struct device *parent;
> +
> +	/* Find a ulpi bus underneath the parent or the grandparent */
> +	parent = ulpi->dev.parent;
> +	if (parent->of_node)
> +		np = of_find_node_by_name(parent->of_node, "ulpi");
> +	else if (parent->parent && parent->parent->of_node)
> +		np = of_find_node_by_name(parent->parent->of_node, "ulpi");
> +	if (!np)
> +		return 0;
> +
> +	child = of_get_next_available_child(np, NULL);
> +	of_node_put(np);
> +	if (!child)
> +		return -EINVAL;
>  
> -	ulpi->dev.parent = dev; /* needed early for ops */
> +	ulpi->dev.of_node = child;
> +
> +	return 0;
> +}
> +
> +static int ulpi_read_id(struct ulpi *ulpi)
> +{
> +	int ret;
>  
>  	/* 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;
>  
>  	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;
> @@ -177,13 +221,35 @@ 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;
>  
> +	/* Some ULPI devices don't have a vendor id so rely on OF match */
> +	if (ulpi->id.vendor == 0)
> +		goto err;
> +
> +	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> +	return 0;
> +err:
> +	of_device_request_module(&ulpi->dev);
> +	return 0;
> +}
> +
> +static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> +{
> +	int ret;
> +
> +	ulpi->dev.parent = dev; /* needed early for ops */
>  	ulpi->dev.bus = &ulpi_bus;
>  	ulpi->dev.type = &ulpi_dev_type;
>  	dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev));
>  
>  	ACPI_COMPANION_SET(&ulpi->dev, ACPI_COMPANION(dev));
>  
> -	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> +	ret = ulpi_of_register(ulpi);
> +	if (ret)
> +		return ret;
> +
> +	ret = ulpi_read_id(ulpi);
> +	if (ret)
> +		return ret;
>  
>  	ret = device_register(&ulpi->dev);
>  	if (ret)
> @@ -234,6 +300,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);
> -- 
> 2.10.0.297.gf6727b0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 

Best Regards,
Peter Chen

Powered by blists - more mailing lists