Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-30 Thread Rob Herring 
On Tue, Jun 28, 2016 at 03:09:21PM -0700, Stephen Boyd wrote:
> Quoting Rob Herring (2016-06-28 13:56:42)
> > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
> > > product id ulpi registers. This makes it impossible to make a
> > > ulpi driver match against the id registers. Add support to
> > > discover the ulpi phys via DT to help alleviate this problem.
> > > 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 DT
> > > 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.
> > > 
> > > 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  | 56 
> > > +-
> > >  2 files changed, 74 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.
> > 
> > This needs to co-exist with the USB bus binding which has the controller 
> > ports for the child nodes. Maybe use the phy binding?
> 
> Which binding is that? bindings/usb/usb-device.txt? 

Yes.

> This ulpi binding is
> to describe phys that are accessed through the ulpi "viewport" in the
> usb controller. So controller ports don't come into the picture here.

You just need to confirm that there's no collision with child nodes like 
it assumes all children are ports.

Rob

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-30 Thread Rob Herring 
On Tue, Jun 28, 2016 at 03:09:21PM -0700, Stephen Boyd wrote:
> Quoting Rob Herring (2016-06-28 13:56:42)
> > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
> > > product id ulpi registers. This makes it impossible to make a
> > > ulpi driver match against the id registers. Add support to
> > > discover the ulpi phys via DT to help alleviate this problem.
> > > 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 DT
> > > 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.
> > > 
> > > 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  | 56 
> > > +-
> > >  2 files changed, 74 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.
> > 
> > This needs to co-exist with the USB bus binding which has the controller 
> > ports for the child nodes. Maybe use the phy binding?
> 
> Which binding is that? bindings/usb/usb-device.txt? 

Yes.

> This ulpi binding is
> to describe phys that are accessed through the ulpi "viewport" in the
> usb controller. So controller ports don't come into the picture here.

You just need to confirm that there's no collision with child nodes like 
it assumes all children are ports.

Rob

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Peter Chen 
On Tue, Jun 28, 2016 at 02:42:05PM +0300, Heikki Krogerus wrote:
> On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote:
> > Quoting Heikki Krogerus (2016-06-27 07:34:22)
> > > Hi,
> > > 
> > > I'm fine with most of the patch, except..
> > > 
> > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > > @@ -39,7 +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;
> > > >  
> > > > - for (id = drv->id_table; id->vendor; id++)
> > > > + if (of_driver_match_device(dev, driver))
> > > > + return 1;
> > > 
> > > I don't like this part. We should match separately like that only
> > > if the bus does not support native enumeration, and of course ULPI
> > > with its vendor and product IDs does. There really should always be
> > > IDs to match with here. So exceptions have to be solved before we
> > > attempt matching.
> > > 
> > > Since we also have to support platforms where the PHY is initially
> > > powered off and reading the IDs from the registers is not possible
> > > because of that, I think we should consider getting the product and
> > > vendor IDs optionally from device properties. Something like this:
> > 
> > Ok, I'm a little worried about conflating the powered off problem with
> > this product/vendor ID missing problem. But if you're ok with that I'll
> > combine the two patches into one using your approach below.
> > 
> > > 
> > > 
> > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > index 01c0c04..6228a85 100644
> > > --- a/drivers/usb/common/ulpi.c
> > > +++ b/drivers/usb/common/ulpi.c
> > > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
> > >  
> > >  /* 
> > > --
> > >  */
> > >  
> > > -static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > +static int ulpi_read_id(struct ulpi *ulpi)
> > >  {
> > > int ret;
> > >  
> > > @@ -174,6 +174,21 @@ 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;
> > > +
> > > +   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;
> > > +   }
> > > +
> > > ulpi->dev.parent = dev;
> > > ulpi->dev.bus = _bus;
> > > ulpi->dev.type = _dev_type;
> > > 
> > > 
> > > That should cover both cases. You would just have to create the IDs
> > > yourself in this case.
> > > 
> > 
> > Right, I would have to make up some IDs in this case. I suppose I can
> > use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy
> > and HSIC phy? That doesn't make me feel great because it's all made up,
> > but I guess there's no other option. I hope they don't decide to start
> > populating these ids in the future though and then we may have
> > conflicting product ids. If that happens I suppose we can do a
> > workaround based on compatible strings in the DT node. Fun!
> > 
> > Nice side effect of all that is I can drop requesting the module by DT
> > aliases and things become simpler. I'll try this out.
> 
> I was hoping that we could manage with product id 0 as an exception (I
> failed to consider that you have multiple PHYs to deal with). I don't
> think we can just come up with product id > 0.
> 
> I guess we should have the of_driver_match_device() call after all.
> Let's just call it conditionally, only in cases where there is no
> product ID, to make me feel a bit more better. I don't want to make it
> too easy to use.
> 
> The properties for the vendor and product ID are still something that
> we need to introduce in any case. We have the powered off problem on
> all kinds of platforms, and not all of them use DT.

I am thinking power sequence framework, how power sequence elements
(eg, clock, reset-gpios) can get for non-DT platform? Does ACPI does power
sequence for x86 platforms?

-- 

Best Regards,
Peter Chen

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Peter Chen 
On Tue, Jun 28, 2016 at 02:42:05PM +0300, Heikki Krogerus wrote:
> On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote:
> > Quoting Heikki Krogerus (2016-06-27 07:34:22)
> > > Hi,
> > > 
> > > I'm fine with most of the patch, except..
> > > 
> > > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > > @@ -39,7 +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;
> > > >  
> > > > - for (id = drv->id_table; id->vendor; id++)
> > > > + if (of_driver_match_device(dev, driver))
> > > > + return 1;
> > > 
> > > I don't like this part. We should match separately like that only
> > > if the bus does not support native enumeration, and of course ULPI
> > > with its vendor and product IDs does. There really should always be
> > > IDs to match with here. So exceptions have to be solved before we
> > > attempt matching.
> > > 
> > > Since we also have to support platforms where the PHY is initially
> > > powered off and reading the IDs from the registers is not possible
> > > because of that, I think we should consider getting the product and
> > > vendor IDs optionally from device properties. Something like this:
> > 
> > Ok, I'm a little worried about conflating the powered off problem with
> > this product/vendor ID missing problem. But if you're ok with that I'll
> > combine the two patches into one using your approach below.
> > 
> > > 
> > > 
> > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > index 01c0c04..6228a85 100644
> > > --- a/drivers/usb/common/ulpi.c
> > > +++ b/drivers/usb/common/ulpi.c
> > > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
> > >  
> > >  /* 
> > > --
> > >  */
> > >  
> > > -static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > +static int ulpi_read_id(struct ulpi *ulpi)
> > >  {
> > > int ret;
> > >  
> > > @@ -174,6 +174,21 @@ 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;
> > > +
> > > +   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;
> > > +   }
> > > +
> > > ulpi->dev.parent = dev;
> > > ulpi->dev.bus = _bus;
> > > ulpi->dev.type = _dev_type;
> > > 
> > > 
> > > That should cover both cases. You would just have to create the IDs
> > > yourself in this case.
> > > 
> > 
> > Right, I would have to make up some IDs in this case. I suppose I can
> > use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy
> > and HSIC phy? That doesn't make me feel great because it's all made up,
> > but I guess there's no other option. I hope they don't decide to start
> > populating these ids in the future though and then we may have
> > conflicting product ids. If that happens I suppose we can do a
> > workaround based on compatible strings in the DT node. Fun!
> > 
> > Nice side effect of all that is I can drop requesting the module by DT
> > aliases and things become simpler. I'll try this out.
> 
> I was hoping that we could manage with product id 0 as an exception (I
> failed to consider that you have multiple PHYs to deal with). I don't
> think we can just come up with product id > 0.
> 
> I guess we should have the of_driver_match_device() call after all.
> Let's just call it conditionally, only in cases where there is no
> product ID, to make me feel a bit more better. I don't want to make it
> too easy to use.
> 
> The properties for the vendor and product ID are still something that
> we need to introduce in any case. We have the powered off problem on
> all kinds of platforms, and not all of them use DT.

I am thinking power sequence framework, how power sequence elements
(eg, clock, reset-gpios) can get for non-DT platform? Does ACPI does power
sequence for x86 platforms?

-- 

Best Regards,
Peter Chen

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Rob Herring 
On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
> product id ulpi registers. This makes it impossible to make a
> ulpi driver match against the id registers. Add support to
> discover the ulpi phys via DT to help alleviate this problem.
> 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 DT
> 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.
> 
> 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  | 56 
> +-
>  2 files changed, 74 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.

This needs to co-exist with the USB bus binding which has the controller 
ports for the child nodes. Maybe use the phy binding?

> +
> +EXAMPLE
> +---
> +
> +usb {
> + compatible = "vendor,usb-controller";
> +
> + ulpi {
> + phy {
> + compatible = "vendor,phy";
> + };
> + };
> +};

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Rob Herring 
On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
> product id ulpi registers. This makes it impossible to make a
> ulpi driver match against the id registers. Add support to
> discover the ulpi phys via DT to help alleviate this problem.
> 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 DT
> 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.
> 
> 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  | 56 
> +-
>  2 files changed, 74 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.

This needs to co-exist with the USB bus binding which has the controller 
ports for the child nodes. Maybe use the phy binding?

> +
> +EXAMPLE
> +---
> +
> +usb {
> + compatible = "vendor,usb-controller";
> +
> + ulpi {
> + phy {
> + compatible = "vendor,phy";
> + };
> + };
> +};

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Heikki Krogerus 
On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote:
> Quoting Heikki Krogerus (2016-06-27 07:34:22)
> > Hi,
> > 
> > I'm fine with most of the patch, except..
> > 
> > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > @@ -39,7 +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;
> > >  
> > > - for (id = drv->id_table; id->vendor; id++)
> > > + if (of_driver_match_device(dev, driver))
> > > + return 1;
> > 
> > I don't like this part. We should match separately like that only
> > if the bus does not support native enumeration, and of course ULPI
> > with its vendor and product IDs does. There really should always be
> > IDs to match with here. So exceptions have to be solved before we
> > attempt matching.
> > 
> > Since we also have to support platforms where the PHY is initially
> > powered off and reading the IDs from the registers is not possible
> > because of that, I think we should consider getting the product and
> > vendor IDs optionally from device properties. Something like this:
> 
> Ok, I'm a little worried about conflating the powered off problem with
> this product/vendor ID missing problem. But if you're ok with that I'll
> combine the two patches into one using your approach below.
> 
> > 
> > 
> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > index 01c0c04..6228a85 100644
> > --- a/drivers/usb/common/ulpi.c
> > +++ b/drivers/usb/common/ulpi.c
> > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
> >  
> >  /* 
> > -- 
> > */
> >  
> > -static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > +static int ulpi_read_id(struct ulpi *ulpi)
> >  {
> > int ret;
> >  
> > @@ -174,6 +174,21 @@ 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;
> > +
> > +   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;
> > +   }
> > +
> > ulpi->dev.parent = dev;
> > ulpi->dev.bus = _bus;
> > ulpi->dev.type = _dev_type;
> > 
> > 
> > That should cover both cases. You would just have to create the IDs
> > yourself in this case.
> > 
> 
> Right, I would have to make up some IDs in this case. I suppose I can
> use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy
> and HSIC phy? That doesn't make me feel great because it's all made up,
> but I guess there's no other option. I hope they don't decide to start
> populating these ids in the future though and then we may have
> conflicting product ids. If that happens I suppose we can do a
> workaround based on compatible strings in the DT node. Fun!
> 
> Nice side effect of all that is I can drop requesting the module by DT
> aliases and things become simpler. I'll try this out.

I was hoping that we could manage with product id 0 as an exception (I
failed to consider that you have multiple PHYs to deal with). I don't
think we can just come up with product id > 0.

I guess we should have the of_driver_match_device() call after all.
Let's just call it conditionally, only in cases where there is no
product ID, to make me feel a bit more better. I don't want to make it
too easy to use.

The properties for the vendor and product ID are still something that
we need to introduce in any case. We have the powered off problem on
all kinds of platforms, and not all of them use DT. Please feel free
to incorporate the diff into the patch you had for the powered off
case if you are OK with it. So I think in your case you would just
need to addthe correct ulpi-vendor id 0x05c6 and ulpi-product id 0 to
the chipidea device node, and I think this would work.

Sorry about the hassle.


Thanks,

-- 
heikki

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-28 Thread Heikki Krogerus 
On Mon, Jun 27, 2016 at 03:10:40PM -0700, Stephen Boyd wrote:
> Quoting Heikki Krogerus (2016-06-27 07:34:22)
> > Hi,
> > 
> > I'm fine with most of the patch, except..
> > 
> > On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> > > @@ -39,7 +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;
> > >  
> > > - for (id = drv->id_table; id->vendor; id++)
> > > + if (of_driver_match_device(dev, driver))
> > > + return 1;
> > 
> > I don't like this part. We should match separately like that only
> > if the bus does not support native enumeration, and of course ULPI
> > with its vendor and product IDs does. There really should always be
> > IDs to match with here. So exceptions have to be solved before we
> > attempt matching.
> > 
> > Since we also have to support platforms where the PHY is initially
> > powered off and reading the IDs from the registers is not possible
> > because of that, I think we should consider getting the product and
> > vendor IDs optionally from device properties. Something like this:
> 
> Ok, I'm a little worried about conflating the powered off problem with
> this product/vendor ID missing problem. But if you're ok with that I'll
> combine the two patches into one using your approach below.
> 
> > 
> > 
> > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > index 01c0c04..6228a85 100644
> > --- a/drivers/usb/common/ulpi.c
> > +++ b/drivers/usb/common/ulpi.c
> > @@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
> >  
> >  /* 
> > -- 
> > */
> >  
> > -static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > +static int ulpi_read_id(struct ulpi *ulpi)
> >  {
> > int ret;
> >  
> > @@ -174,6 +174,21 @@ 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;
> > +
> > +   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;
> > +   }
> > +
> > ulpi->dev.parent = dev;
> > ulpi->dev.bus = _bus;
> > ulpi->dev.type = _dev_type;
> > 
> > 
> > That should cover both cases. You would just have to create the IDs
> > yourself in this case.
> > 
> 
> Right, I would have to make up some IDs in this case. I suppose I can
> use the qcom vendor ID 0x05c6 and then product ids 0 and 1 for HS phy
> and HSIC phy? That doesn't make me feel great because it's all made up,
> but I guess there's no other option. I hope they don't decide to start
> populating these ids in the future though and then we may have
> conflicting product ids. If that happens I suppose we can do a
> workaround based on compatible strings in the DT node. Fun!
> 
> Nice side effect of all that is I can drop requesting the module by DT
> aliases and things become simpler. I'll try this out.

I was hoping that we could manage with product id 0 as an exception (I
failed to consider that you have multiple PHYs to deal with). I don't
think we can just come up with product id > 0.

I guess we should have the of_driver_match_device() call after all.
Let's just call it conditionally, only in cases where there is no
product ID, to make me feel a bit more better. I don't want to make it
too easy to use.

The properties for the vendor and product ID are still something that
we need to introduce in any case. We have the powered off problem on
all kinds of platforms, and not all of them use DT. Please feel free
to incorporate the diff into the patch you had for the powered off
case if you are OK with it. So I think in your case you would just
need to addthe correct ulpi-vendor id 0x05c6 and ulpi-product id 0 to
the chipidea device node, and I think this would work.

Sorry about the hassle.


Thanks,

-- 
heikki

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-27 Thread Heikki Krogerus 
Hi,

I'm fine with most of the patch, except..

On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> @@ -39,7 +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;
>  
> - for (id = drv->id_table; id->vendor; id++)
> + if (of_driver_match_device(dev, driver))
> + return 1;

I don't like this part. We should match separately like that only
if the bus does not support native enumeration, and of course ULPI
with its vendor and product IDs does. There really should always be
IDs to match with here. So exceptions have to be solved before we
attempt matching.

Since we also have to support platforms where the PHY is initially
powered off and reading the IDs from the registers is not possible
because of that, I think we should consider getting the product and
vendor IDs optionally from device properties. Something like this:


diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 01c0c04..6228a85 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
 
 /* -- 
*/
 
-static int ulpi_register(struct device *dev, struct ulpi *ulpi)
+static int ulpi_read_id(struct ulpi *ulpi)
 {
int ret;
 
@@ -174,6 +174,21 @@ 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;
+
+   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;
+   }
+
ulpi->dev.parent = dev;
ulpi->dev.bus = _bus;
ulpi->dev.type = _dev_type;


That should cover both cases. You would just have to create the IDs
yourself in this case.


Thanks,

-- 
heikki

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-27 Thread Heikki Krogerus 
Hi,

I'm fine with most of the patch, except..

On Sun, Jun 26, 2016 at 12:28:19AM -0700, Stephen Boyd wrote:
> @@ -39,7 +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;
>  
> - for (id = drv->id_table; id->vendor; id++)
> + if (of_driver_match_device(dev, driver))
> + return 1;

I don't like this part. We should match separately like that only
if the bus does not support native enumeration, and of course ULPI
with its vendor and product IDs does. There really should always be
IDs to match with here. So exceptions have to be solved before we
attempt matching.

Since we also have to support platforms where the PHY is initially
powered off and reading the IDs from the registers is not possible
because of that, I think we should consider getting the product and
vendor IDs optionally from device properties. Something like this:


diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 01c0c04..6228a85 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -152,7 +152,7 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
 
 /* -- 
*/
 
-static int ulpi_register(struct device *dev, struct ulpi *ulpi)
+static int ulpi_read_id(struct ulpi *ulpi)
 {
int ret;
 
@@ -174,6 +174,21 @@ 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;
+
+   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;
+   }
+
ulpi->dev.parent = dev;
ulpi->dev.bus = _bus;
ulpi->dev.type = _dev_type;


That should cover both cases. You would just have to create the IDs
yourself in this case.


Thanks,

-- 
heikki

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-26 Thread kbuild test robot 
Hi,

[auto build test ERROR on peter.chen-usb/ci-for-usb-next]
[also build test ERROR on v4.7-rc5 next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Stephen-Boyd/Support-qcom-s-HSIC-USB-and-rewrite-USB2-HS-phy-support/20160627-102637
base:   https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb 
ci-for-usb-next
config: x86_64-randconfig-h0-06270614 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "of_device_request_module" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_get_modalias" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_uevent_modalias" [drivers/usb/common/ulpi.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-26 Thread kbuild test robot 
Hi,

[auto build test ERROR on peter.chen-usb/ci-for-usb-next]
[also build test ERROR on v4.7-rc5 next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Stephen-Boyd/Support-qcom-s-HSIC-USB-and-rewrite-USB2-HS-phy-support/20160627-102637
base:   https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb 
ci-for-usb-next
config: x86_64-randconfig-h0-06270614 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "of_device_request_module" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_get_modalias" [drivers/usb/common/ulpi.ko] undefined!
>> ERROR: "of_device_uevent_modalias" [drivers/usb/common/ulpi.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

[PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-26 Thread Stephen Boyd 
The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
product id ulpi registers. This makes it impossible to make a
ulpi driver match against the id registers. Add support to
discover the ulpi phys via DT to help alleviate this problem.
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 DT
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.

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  | 56 +-
 2 files changed, 74 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..980af672bfe3 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -16,6 +16,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 /* -- 
*/
 
@@ -39,7 +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;
 
-   for (id = drv->id_table; id->vendor; id++)
+   if (of_driver_match_device(dev, driver))
+   return 1;
+
+   for (id = drv->id_table; id && id->vendor; id++)
if (id->vendor == ulpi->id.vendor &&
id->product == ulpi->id.product)
return 1;
@@ -50,6 +56,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 +71,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 +103,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,6 +173,28 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
 
 /* -- 
*/
 
+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;
+
+   ulpi->dev.of_node = child;
+
+   return 0;
+}
+

[PATCH 02/21] usb: ulpi: Support device discovery via DT

2016-06-26 Thread Stephen Boyd 
The qcom HSIC ulpi phy doesn't have any bits set in the vendor or
product id ulpi registers. This makes it impossible to make a
ulpi driver match against the id registers. Add support to
discover the ulpi phys via DT to help alleviate this problem.
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 DT
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.

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  | 56 +-
 2 files changed, 74 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..980af672bfe3 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -16,6 +16,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 /* -- 
*/
 
@@ -39,7 +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;
 
-   for (id = drv->id_table; id->vendor; id++)
+   if (of_driver_match_device(dev, driver))
+   return 1;
+
+   for (id = drv->id_table; id && id->vendor; id++)
if (id->vendor == ulpi->id.vendor &&
id->product == ulpi->id.product)
return 1;
@@ -50,6 +56,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 +71,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 +103,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,6 +173,28 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
 
 /* -- 
*/
 
+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;
+
+   ulpi->dev.of_node = child;
+
+   return 0;
+}
+
 static int ulpi_register(struct device *dev, struct ulpi *ulpi)
 {
int ret;
@@ -181,7 +224,15 @@ static int