diff mbox

[08/12] doc: binding: pwrseq-usb-generic: add binding doc for generic usb power sequence driver

Message ID 1466158165-9380-9-git-send-email-peter.chen@nxp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Chen June 17, 2016, 10:09 a.m. UTC
Add binding doc for generic usb power sequence driver, and update
generic usb device binding-doc accordingly.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
 .../devicetree/bindings/usb/usb-device.txt         |  2 ++
 2 files changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt

Comments

Rob Herring June 17, 2016, 5:16 p.m. UTC | #1
On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> Add binding doc for generic usb power sequence driver, and update
> generic usb device binding-doc accordingly.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
>  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
>  2 files changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
>
> diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> new file mode 100644
> index 0000000..8ad98382
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> @@ -0,0 +1,31 @@
> +The power sequence for generic USB Devices
> +
> +Some hard-wired USB devices need to do power sequence to let the
> +device work normally, the typical power sequence like: enable USB
> +PHY clock, toggle reset pin, etc. But current Linux USB driver
> +lacks of such code to do it, it may cause some hard-wired USB devices
> +works abnormal or can't be recognized by controller at all. The
> +power sequence will be done before this device can be found at USB
> +bus.
> +
> +Required properties:
> +- compatible : contains "usb-pwrseq-generic".

In case I have not been clear, no.

I am not going to accept anything along the lines of the current mmc
pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
an added property and not a duplication of information. I'd suggest
you figure out how to make the kernel work with that rather than
trying to work-around whatever kernel limitations there are.

Rob
Peter Chen June 20, 2016, 11:26 a.m. UTC | #2
On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > Add binding doc for generic usb power sequence driver, and update
> > generic usb device binding-doc accordingly.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
> >  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
> >  2 files changed, 33 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> >
> > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > new file mode 100644
> > index 0000000..8ad98382
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > @@ -0,0 +1,31 @@
> > +The power sequence for generic USB Devices
> > +
> > +Some hard-wired USB devices need to do power sequence to let the
> > +device work normally, the typical power sequence like: enable USB
> > +PHY clock, toggle reset pin, etc. But current Linux USB driver
> > +lacks of such code to do it, it may cause some hard-wired USB devices
> > +works abnormal or can't be recognized by controller at all. The
> > +power sequence will be done before this device can be found at USB
> > +bus.
> > +
> > +Required properties:
> > +- compatible : contains "usb-pwrseq-generic".
> 
> In case I have not been clear, no.
> 
> I am not going to accept anything along the lines of the current mmc
> pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
> an added property and not a duplication of information. I'd suggest
> you figure out how to make the kernel work with that rather than
> trying to work-around whatever kernel limitations there are.
> 

I see.

Would you agree with below:

&usbotg1 {
	vbus-supply = <&reg_usb_otg1_vbus>;
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_usb_otg1_id>;
	status = "okay";

	#address-cells = <1>;
	#size-cells = <0>;
	hub: genesys@1 {
		compatible = "usb5e3,608";
		reg = <1>;

		power-sequence;
		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
		reset-duration-us = <10>;
		clocks = <&clks IMX6SX_CLK_CKO>;

		#address-cells = <1>;
		#size-cells = <0>;
		ethernet: asix@1 {
			compatible = "usbb95,1708";
			reg = <1>;

			power-sequence;
			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
			reset-duration-us = <15>;
			clocks = <&clks IMX6SX_CLK_IPG>;
		};
	};
};

If the node has property "power-sequence", the pwrseq core will create
related platform device, and the driver under pwrseq driver will handle
power sequence stuffs. The property below "power-sequence" will be
handled at pwrseq driver.
Chen-Yu Tsai June 20, 2016, 12:29 p.m. UTC | #3
Hi,

On Mon, Jun 20, 2016 at 7:26 PM, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
>> On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
>> > Add binding doc for generic usb power sequence driver, and update
>> > generic usb device binding-doc accordingly.
>> >
>> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
>> > ---
>> >  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
>> >  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
>> >  2 files changed, 33 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
>> > new file mode 100644
>> > index 0000000..8ad98382
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
>> > @@ -0,0 +1,31 @@
>> > +The power sequence for generic USB Devices
>> > +
>> > +Some hard-wired USB devices need to do power sequence to let the
>> > +device work normally, the typical power sequence like: enable USB
>> > +PHY clock, toggle reset pin, etc. But current Linux USB driver
>> > +lacks of such code to do it, it may cause some hard-wired USB devices
>> > +works abnormal or can't be recognized by controller at all. The
>> > +power sequence will be done before this device can be found at USB
>> > +bus.
>> > +
>> > +Required properties:
>> > +- compatible : contains "usb-pwrseq-generic".
>>
>> In case I have not been clear, no.
>>
>> I am not going to accept anything along the lines of the current mmc
>> pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
>> an added property and not a duplication of information. I'd suggest
>> you figure out how to make the kernel work with that rather than
>> trying to work-around whatever kernel limitations there are.
>>
>
> I see.
>
> Would you agree with below:
>
> &usbotg1 {
>         vbus-supply = <&reg_usb_otg1_vbus>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_usb_otg1_id>;
>         status = "okay";
>
>         #address-cells = <1>;
>         #size-cells = <0>;
>         hub: genesys@1 {
>                 compatible = "usb5e3,608";
>                 reg = <1>;
>
>                 power-sequence;
>                 reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
>                 reset-duration-us = <10>;
>                 clocks = <&clks IMX6SX_CLK_CKO>;
>
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 ethernet: asix@1 {
>                         compatible = "usbb95,1708";
>                         reg = <1>;
>
>                         power-sequence;
>                         reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
>                         reset-duration-us = <15>;
>                         clocks = <&clks IMX6SX_CLK_IPG>;
>                 };
>         };
> };
>
> If the node has property "power-sequence", the pwrseq core will create
> related platform device, and the driver under pwrseq driver will handle
> power sequence stuffs. The property below "power-sequence" will be
> handled at pwrseq driver.

Isn't this binding what Krzysztof proposed? His series also provides example
code for enabling USB device power sequencing. Does it not work for you?

Having 2 series by 2 separate people doing the same thing but slightly
different is very confusing.

Regards
ChenYu
Rob Herring June 20, 2016, 4:16 p.m. UTC | #4
On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > Add binding doc for generic usb power sequence driver, and update
> > > generic usb device binding-doc accordingly.
> > >
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
> > >  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
> > >  2 files changed, 33 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > > new file mode 100644
> > > index 0000000..8ad98382
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > > @@ -0,0 +1,31 @@
> > > +The power sequence for generic USB Devices
> > > +
> > > +Some hard-wired USB devices need to do power sequence to let the
> > > +device work normally, the typical power sequence like: enable USB
> > > +PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > +lacks of such code to do it, it may cause some hard-wired USB devices
> > > +works abnormal or can't be recognized by controller at all. The
> > > +power sequence will be done before this device can be found at USB
> > > +bus.
> > > +
> > > +Required properties:
> > > +- compatible : contains "usb-pwrseq-generic".
> > 
> > In case I have not been clear, no.
> > 
> > I am not going to accept anything along the lines of the current mmc
> > pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
> > an added property and not a duplication of information. I'd suggest
> > you figure out how to make the kernel work with that rather than
> > trying to work-around whatever kernel limitations there are.
> > 
> 
> I see.
> 
> Would you agree with below:

In general, yes. There are some points being discussed in the other 
thread.

> &usbotg1 {
> 	vbus-supply = <&reg_usb_otg1_vbus>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_usb_otg1_id>;
> 	status = "okay";
> 
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	hub: genesys@1 {
> 		compatible = "usb5e3,608";
> 		reg = <1>;
> 
> 		power-sequence;
> 		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
> 		reset-duration-us = <10>;

I wonder if this really needs to be specified. A sufficiently long 
default should be good enough for 90 something % of devices.

> 		clocks = <&clks IMX6SX_CLK_CKO>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		ethernet: asix@1 {
> 			compatible = "usbb95,1708";
> 			reg = <1>;
> 
> 			power-sequence;
> 			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> 			reset-duration-us = <15>;
> 			clocks = <&clks IMX6SX_CLK_IPG>;
> 		};
> 	};
> };
> 
> If the node has property "power-sequence", the pwrseq core will create
> related platform device, and the driver under pwrseq driver will handle
> power sequence stuffs. 

This I have issue with. If you are creating a platform device here, you 
are trying to work-around limitations in the linux driver model. Either 
we need some sort of pre-probe hook to the drivers to call or each 
parent node driver is responsible for checking and calling pwr-seq 
functions for child nodes. e.g. The host controller calls pwr-seq for 
the hub, the hub driver calls the power seq for the asix chip. Soon as 
we have a case too complex for the generic pwr-seq, we're going to need 
the pre-probe hook as I don't want to see a continual expansion of 
generic pwr-seq binding for ever more complex cases.

> The property below "power-sequence" will be
> handled at pwrseq driver.

You cannot rely on order other than for logical readability. There are 
no guarantees of either the dtb or kernel ordering of properties.

Rob
Mark Brown June 20, 2016, 5:06 p.m. UTC | #5
On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:

> > If the node has property "power-sequence", the pwrseq core will create
> > related platform device, and the driver under pwrseq driver will handle
> > power sequence stuffs. 

> This I have issue with. If you are creating a platform device here, you 
> are trying to work-around limitations in the linux driver model. Either 
> we need some sort of pre-probe hook to the drivers to call or each 
> parent node driver is responsible for checking and calling pwr-seq 
> functions for child nodes. e.g. The host controller calls pwr-seq for 
> the hub, the hub driver calls the power seq for the asix chip. Soon as 
> we have a case too complex for the generic pwr-seq, we're going to need 
> the pre-probe hook as I don't want to see a continual expansion of 
> generic pwr-seq binding for ever more complex cases.

I think it's fairly clear that we need one or both of these mechanisms
for enumerable buses in embedded contexts - it's something that keeps
croping up.
Peter Chen June 21, 2016, 2:11 a.m. UTC | #6
On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > Add binding doc for generic usb power sequence driver, and update
> > > > generic usb device binding-doc accordingly.
> > > >
> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > ---
> > > >  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
> > > >  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
> > > >  2 files changed, 33 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > > > new file mode 100644
> > > > index 0000000..8ad98382
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> > > > @@ -0,0 +1,31 @@
> > > > +The power sequence for generic USB Devices
> > > > +
> > > > +Some hard-wired USB devices need to do power sequence to let the
> > > > +device work normally, the typical power sequence like: enable USB
> > > > +PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > > +lacks of such code to do it, it may cause some hard-wired USB devices
> > > > +works abnormal or can't be recognized by controller at all. The
> > > > +power sequence will be done before this device can be found at USB
> > > > +bus.
> > > > +
> > > > +Required properties:
> > > > +- compatible : contains "usb-pwrseq-generic".
> > > 
> > > In case I have not been clear, no.
> > > 
> > > I am not going to accept anything along the lines of the current mmc
> > > pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
> > > an added property and not a duplication of information. I'd suggest
> > > you figure out how to make the kernel work with that rather than
> > > trying to work-around whatever kernel limitations there are.
> > > 
> > 
> > I see.
> > 
> > Would you agree with below:
> 
> In general, yes. There are some points being discussed in the other 
> thread.
> 
> > &usbotg1 {
> > 	vbus-supply = <&reg_usb_otg1_vbus>;
> > 	pinctrl-names = "default";
> > 	pinctrl-0 = <&pinctrl_usb_otg1_id>;
> > 	status = "okay";
> > 
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	hub: genesys@1 {
> > 		compatible = "usb5e3,608";
> > 		reg = <1>;
> > 
> > 		power-sequence;
> > 		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
> > 		reset-duration-us = <10>;
> 
> I wonder if this really needs to be specified. A sufficiently long 
> default should be good enough for 90 something % of devices.
> 

This property is optional, there is a default value in pwrseq driver.

> > 		clocks = <&clks IMX6SX_CLK_CKO>;
> > 
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 		ethernet: asix@1 {
> > 			compatible = "usbb95,1708";
> > 			reg = <1>;
> > 
> > 			power-sequence;
> > 			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> > 			reset-duration-us = <15>;
> > 			clocks = <&clks IMX6SX_CLK_IPG>;
> > 		};
> > 	};
> > };
> > 
> > If the node has property "power-sequence", the pwrseq core will create
> > related platform device, and the driver under pwrseq driver will handle
> > power sequence stuffs. 
> 
> This I have issue with. If you are creating a platform device here, you 
> are trying to work-around limitations in the linux driver model.

My current solution like below, but it seems you didn't agree with that.
I just double confirm here, if you don't, I give up the solution for
using generic power sequence framework.

In drivers/usb/core/hub.c

	for_each_child_of_node(parent->of_node, node) {
		hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
		if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
			pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
			if (!pwrseq_node) {
				ret = -ENOMEM;
				goto err1;
			}
			/* power on sequence */
			ret = pwrseq_pre_power_on(hdev_pwrseq);
			if (ret)
				goto err2;

			pwrseq_node->pwrseq_on = hdev_pwrseq;
			list_add(&pwrseq_node->list, &hub->pwrseq_list);
		} else if (IS_ERR(hdev_pwrseq)) {
			return PTR_ERR(hdev_pwrseq);
		}
	}

In drivers/power/pwrseq/core.c

struct pwrseq *pwrseq_alloc(struct device_node *np, const char *dev_name)
{
	struct pwrseq *p, *pwrseq = NULL;
	bool created;

	/* If there is no device is associated with this node, create it */
	if (!of_find_device_by_node(np)) {
		if (of_platform_device_create(np, dev_name, NULL))
			created = true;
		else
			return ERR_PTR(-ENODEV);
	}

	mutex_lock(&pwrseq_list_mutex);
	list_for_each_entry(p, &pwrseq_list, pwrseq_node) {
		if (p->dev->of_node == np) {
			if (!try_module_get(p->owner))
				dev_err(p->dev,
					"increasing module refcount failed\n");
			else
				pwrseq = p;
			break;
		}
	}

And there is a platform driver at drivers/power/pwrseq/pwrseq_usb_generic.c
to handle these pwrseq properties, see my patch 9/12.

> Either 
> we need some sort of pre-probe hook to the drivers to call or each 
> parent node driver is responsible for checking and calling pwr-seq 
> functions for child nodes. e.g. The host controller calls pwr-seq for 
> the hub, the hub driver calls the power seq for the asix chip. Soon as 
> we have a case too complex for the generic pwr-seq, we're going to need 
> the pre-probe hook as I don't want to see a continual expansion of 
> generic pwr-seq binding for ever more complex cases.
> 

How the driver know what it needs to handle (eg, gpio, clock) if there
is no device for it? The most important we need to consider is which
device owns there power sequence properties, then the corresponding
driver can handle it.
Peter Chen June 21, 2016, 2:14 a.m. UTC | #7
On Mon, Jun 20, 2016 at 08:29:55PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Mon, Jun 20, 2016 at 7:26 PM, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> >> On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> >> > Add binding doc for generic usb power sequence driver, and update
> >> > generic usb device binding-doc accordingly.
> >> >
> >> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> >> > ---
> >> >  .../bindings/power/pwrseq/pwrseq-usb-generic.txt   | 31 ++++++++++++++++++++++
> >> >  .../devicetree/bindings/usb/usb-device.txt         |  2 ++
> >> >  2 files changed, 33 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> >> > new file mode 100644
> >> > index 0000000..8ad98382
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
> >> > @@ -0,0 +1,31 @@
> >> > +The power sequence for generic USB Devices
> >> > +
> >> > +Some hard-wired USB devices need to do power sequence to let the
> >> > +device work normally, the typical power sequence like: enable USB
> >> > +PHY clock, toggle reset pin, etc. But current Linux USB driver
> >> > +lacks of such code to do it, it may cause some hard-wired USB devices
> >> > +works abnormal or can't be recognized by controller at all. The
> >> > +power sequence will be done before this device can be found at USB
> >> > +bus.
> >> > +
> >> > +Required properties:
> >> > +- compatible : contains "usb-pwrseq-generic".
> >>
> >> In case I have not been clear, no.
> >>
> >> I am not going to accept anything along the lines of the current mmc
> >> pwrseq. I am basically okay with Krzysztof's proposal as it is *only*
> >> an added property and not a duplication of information. I'd suggest
> >> you figure out how to make the kernel work with that rather than
> >> trying to work-around whatever kernel limitations there are.
> >>
> >
> > I see.
> >
> > Would you agree with below:
> >
> > &usbotg1 {
> >         vbus-supply = <&reg_usb_otg1_vbus>;
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_usb_otg1_id>;
> >         status = "okay";
> >
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >         hub: genesys@1 {
> >                 compatible = "usb5e3,608";
> >                 reg = <1>;
> >
> >                 power-sequence;
> >                 reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
> >                 reset-duration-us = <10>;
> >                 clocks = <&clks IMX6SX_CLK_CKO>;
> >
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 ethernet: asix@1 {
> >                         compatible = "usbb95,1708";
> >                         reg = <1>;
> >
> >                         power-sequence;
> >                         reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> >                         reset-duration-us = <15>;
> >                         clocks = <&clks IMX6SX_CLK_IPG>;
> >                 };
> >         };
> > };
> >
> > If the node has property "power-sequence", the pwrseq core will create
> > related platform device, and the driver under pwrseq driver will handle
> > power sequence stuffs. The property below "power-sequence" will be
> > handled at pwrseq driver.
> 
> Isn't this binding what Krzysztof proposed? His series also provides example
> code for enabling USB device power sequencing. Does it not work for you?
> 

In this series, I just change RFC to formal patch, and finished his
example code. USB power sequence should be described on USB bus, not
platform bus.
Rob Herring June 21, 2016, 9:26 p.m. UTC | #8
On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
> On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > Add binding doc for generic usb power sequence driver, and update
> > > > > generic usb device binding-doc accordingly.

[...]

> > > 		clocks = <&clks IMX6SX_CLK_CKO>;
> > > 
> > > 		#address-cells = <1>;
> > > 		#size-cells = <0>;
> > > 		ethernet: asix@1 {
> > > 			compatible = "usbb95,1708";
> > > 			reg = <1>;
> > > 
> > > 			power-sequence;
> > > 			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> > > 			reset-duration-us = <15>;
> > > 			clocks = <&clks IMX6SX_CLK_IPG>;
> > > 		};
> > > 	};
> > > };
> > > 
> > > If the node has property "power-sequence", the pwrseq core will create
> > > related platform device, and the driver under pwrseq driver will handle
> > > power sequence stuffs. 
> > 
> > This I have issue with. If you are creating a platform device here, you 
> > are trying to work-around limitations in the linux driver model.
> 
> My current solution like below, but it seems you didn't agree with that.
> I just double confirm here, if you don't, I give up the solution for
> using generic power sequence framework.
> 
> In drivers/usb/core/hub.c
> 
> 	for_each_child_of_node(parent->of_node, node) {
> 		hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
> 		if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
> 			pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
> 			if (!pwrseq_node) {
> 				ret = -ENOMEM;
> 				goto err1;
> 			}
> 			/* power on sequence */
> 			ret = pwrseq_pre_power_on(hdev_pwrseq);

Why does this function need to do anything more than:

- Check if the child has a "power-sequence" property
- Get the "reset-gpios" GPIO
- Assert reset for specified/default time
- Deassert reset

Then continue on as normal. That seems straight-forward to me.

There is no reason you need a platform device in the mix. Perhaps trying 
to move the MMC pwr-seq code is pointless as it adds needless 
complexity.

[...]

> > Either 
> > we need some sort of pre-probe hook to the drivers to call or each 
> > parent node driver is responsible for checking and calling pwr-seq 
> > functions for child nodes. e.g. The host controller calls pwr-seq for 
> > the hub, the hub driver calls the power seq for the asix chip. Soon as 
> > we have a case too complex for the generic pwr-seq, we're going to need 
> > the pre-probe hook as I don't want to see a continual expansion of 
> > generic pwr-seq binding for ever more complex cases.
> > 
> 
> How the driver know what it needs to handle (eg, gpio, clock) if there
> is no device for it? The most important we need to consider is which
> device owns there power sequence properties, then the corresponding
> driver can handle it.

What can be handled by is defined by presence of power-sequence 
property. There can be 1 driver for the device. That is the USB hub 
driver in this example. You should not have 2 "devices".

Rob
Peter Chen June 22, 2016, 1:14 a.m. UTC | #9
On Tue, Jun 21, 2016 at 04:26:53PM -0500, Rob Herring wrote:
> On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
> > On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> > > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
> > > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
> > > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > > Add binding doc for generic usb power sequence driver, and update
> > > > > > generic usb device binding-doc accordingly.
> 
> [...]
> 
> > > > 		clocks = <&clks IMX6SX_CLK_CKO>;
> > > > 
> > > > 		#address-cells = <1>;
> > > > 		#size-cells = <0>;
> > > > 		ethernet: asix@1 {
> > > > 			compatible = "usbb95,1708";
> > > > 			reg = <1>;
> > > > 
> > > > 			power-sequence;
> > > > 			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
> > > > 			reset-duration-us = <15>;
> > > > 			clocks = <&clks IMX6SX_CLK_IPG>;
> > > > 		};
> > > > 	};
> > > > };
> > > > 
> > > > If the node has property "power-sequence", the pwrseq core will create
> > > > related platform device, and the driver under pwrseq driver will handle
> > > > power sequence stuffs. 
> > > 
> > > This I have issue with. If you are creating a platform device here, you 
> > > are trying to work-around limitations in the linux driver model.
> > 
> > My current solution like below, but it seems you didn't agree with that.
> > I just double confirm here, if you don't, I give up the solution for
> > using generic power sequence framework.
> > 
> > In drivers/usb/core/hub.c
> > 
> > 	for_each_child_of_node(parent->of_node, node) {
> > 		hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
> > 		if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
> > 			pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
> > 			if (!pwrseq_node) {
> > 				ret = -ENOMEM;
> > 				goto err1;
> > 			}
> > 			/* power on sequence */
> > 			ret = pwrseq_pre_power_on(hdev_pwrseq);
> 
> Why does this function need to do anything more than:
> 
> - Check if the child has a "power-sequence" property
> - Get the "reset-gpios" GPIO
> - Assert reset for specified/default time
> - Deassert reset
> 

Besides gpios, it may exist clock and regulator operation, and the
operation may be failed. I thought these operations can be easy
done belongs to a device, but now, let me try this straight-forward
way, thanks.

Peter
> Then continue on as normal. That seems straight-forward to me.
> 
> There is no reason you need a platform device in the mix. Perhaps trying 
> to move the MMC pwr-seq code is pointless as it adds needless 
> complexity.
> 
> [...]
> 
> > > Either 
> > > we need some sort of pre-probe hook to the drivers to call or each 
> > > parent node driver is responsible for checking and calling pwr-seq 
> > > functions for child nodes. e.g. The host controller calls pwr-seq for 
> > > the hub, the hub driver calls the power seq for the asix chip. Soon as 
> > > we have a case too complex for the generic pwr-seq, we're going to need 
> > > the pre-probe hook as I don't want to see a continual expansion of 
> > > generic pwr-seq binding for ever more complex cases.
> > > 
> > 
> > How the driver know what it needs to handle (eg, gpio, clock) if there
> > is no device for it? The most important we need to consider is which
> > device owns there power sequence properties, then the corresponding
> > driver can handle it.
> 
> What can be handled by is defined by presence of power-sequence 
> property. There can be 1 driver for the device. That is the USB hub 
> driver in this example. You should not have 2 "devices".
> 
> Rob
Ulf Hansson June 22, 2016, 9:09 a.m. UTC | #10
On 21 June 2016 at 23:26, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
>> On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
>> > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
>> > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
>> > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
>> > > > > Add binding doc for generic usb power sequence driver, and update
>> > > > > generic usb device binding-doc accordingly.
>
> [...]
>
>> > >           clocks = <&clks IMX6SX_CLK_CKO>;
>> > >
>> > >           #address-cells = <1>;
>> > >           #size-cells = <0>;
>> > >           ethernet: asix@1 {
>> > >                   compatible = "usbb95,1708";
>> > >                   reg = <1>;
>> > >
>> > >                   power-sequence;
>> > >                   reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
>> > >                   reset-duration-us = <15>;
>> > >                   clocks = <&clks IMX6SX_CLK_IPG>;
>> > >           };
>> > >   };
>> > > };
>> > >
>> > > If the node has property "power-sequence", the pwrseq core will create
>> > > related platform device, and the driver under pwrseq driver will handle
>> > > power sequence stuffs.
>> >
>> > This I have issue with. If you are creating a platform device here, you
>> > are trying to work-around limitations in the linux driver model.

I somewhat understand your point.

Although, having the option to use a driver (which requires a device)
has turned out to be quite convenient from many aspects - at least in
the mmc case.

Certainly one can do without it, but in the end using a driver avoids
open coding.

>>
>> My current solution like below, but it seems you didn't agree with that.
>> I just double confirm here, if you don't, I give up the solution for
>> using generic power sequence framework.
>>
>> In drivers/usb/core/hub.c
>>
>>       for_each_child_of_node(parent->of_node, node) {
>>               hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
>>               if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
>>                       pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
>>                       if (!pwrseq_node) {
>>                               ret = -ENOMEM;
>>                               goto err1;
>>                       }
>>                       /* power on sequence */
>>                       ret = pwrseq_pre_power_on(hdev_pwrseq);
>
> Why does this function need to do anything more than:
>
> - Check if the child has a "power-sequence" property
> - Get the "reset-gpios" GPIO
> - Assert reset for specified/default time
> - Deassert reset
>
> Then continue on as normal. That seems straight-forward to me.
>
> There is no reason you need a platform device in the mix. Perhaps trying
> to move the MMC pwr-seq code is pointless as it adds needless
> complexity.

Complexity?

The problem we are tying to solve, is to make the various platform/SoC
specific power sequences to be able to live in generic drivers.

One could decide to encode the sequences inside the driver code
itself, but it will soon turn into a mess and more importantly, lots
of open coding as to support different platforms/SoCs. To most kernel
hackers I don't think this is an option to consider.

The MMC pwr-seq code/drivers tries to address these issues - in a
somewhat generic way.
Initially we have decided to start with only a few flavours of
supported sequences and so far these have been sufficient.

Finally, I am indeed concerned that it so hard to agree on a solution
to deal with generic power sequences. There have been many attempts
throughout the last ~4-5 years, but peoples strong opinions about
different approaches mad them all fail. Isn't it time to finally just
pick a solution and stick with it, even if it doesn't meet all peoples
expectations?

[...]

Kind regards
Uffe
Rob Herring June 24, 2016, 3:25 p.m. UTC | #11
On Wed, Jun 22, 2016 at 4:09 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 June 2016 at 23:26, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:
>>> On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
>>> > On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:
>>> > > On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:
>>> > > > On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen <peter.chen@nxp.com> wrote:
>>> > > > > Add binding doc for generic usb power sequence driver, and update
>>> > > > > generic usb device binding-doc accordingly.
>>
>> [...]
>>
>>> > >           clocks = <&clks IMX6SX_CLK_CKO>;
>>> > >
>>> > >           #address-cells = <1>;
>>> > >           #size-cells = <0>;
>>> > >           ethernet: asix@1 {
>>> > >                   compatible = "usbb95,1708";
>>> > >                   reg = <1>;
>>> > >
>>> > >                   power-sequence;
>>> > >                   reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>; /* ethernet_rst */
>>> > >                   reset-duration-us = <15>;
>>> > >                   clocks = <&clks IMX6SX_CLK_IPG>;
>>> > >           };
>>> > >   };
>>> > > };
>>> > >
>>> > > If the node has property "power-sequence", the pwrseq core will create
>>> > > related platform device, and the driver under pwrseq driver will handle
>>> > > power sequence stuffs.
>>> >
>>> > This I have issue with. If you are creating a platform device here, you
>>> > are trying to work-around limitations in the linux driver model.
>
> I somewhat understand your point.
>
> Although, having the option to use a driver (which requires a device)
> has turned out to be quite convenient from many aspects - at least in
> the mmc case.
>
> Certainly one can do without it, but in the end using a driver avoids
> open coding.

Why would it be open coded? Just create library functions to parse the
node and implement the generic pwr-seq steps.

>>> My current solution like below, but it seems you didn't agree with that.
>>> I just double confirm here, if you don't, I give up the solution for
>>> using generic power sequence framework.
>>>
>>> In drivers/usb/core/hub.c
>>>
>>>       for_each_child_of_node(parent->of_node, node) {
>>>               hdev_pwrseq = pwrseq_alloc(node, "usb_pwrseq_generic");
>>>               if (!IS_ERR_OR_NULL(hdev_pwrseq)) {
>>>                       pwrseq_node = kzalloc(sizeof(pwrseq_node), GFP_KERNEL);
>>>                       if (!pwrseq_node) {
>>>                               ret = -ENOMEM;
>>>                               goto err1;
>>>                       }
>>>                       /* power on sequence */
>>>                       ret = pwrseq_pre_power_on(hdev_pwrseq);
>>
>> Why does this function need to do anything more than:
>>
>> - Check if the child has a "power-sequence" property
>> - Get the "reset-gpios" GPIO
>> - Assert reset for specified/default time
>> - Deassert reset
>>
>> Then continue on as normal. That seems straight-forward to me.
>>
>> There is no reason you need a platform device in the mix. Perhaps trying
>> to move the MMC pwr-seq code is pointless as it adds needless
>> complexity.
>
> Complexity?
>
> The problem we are tying to solve, is to make the various platform/SoC
> specific power sequences to be able to live in generic drivers.
>
> One could decide to encode the sequences inside the driver code
> itself, but it will soon turn into a mess and more importantly, lots
> of open coding as to support different platforms/SoCs. To most kernel
> hackers I don't think this is an option to consider.

I'm not proposing open coding, but having generic library functions.
I'm not saying the mmc pwr-seq has to change from being a driver
either. Leave it as is. I'm only talking about Krzysztof's new
proposal.

> The MMC pwr-seq code/drivers tries to address these issues - in a
> somewhat generic way.
> Initially we have decided to start with only a few flavours of
> supported sequences and so far these have been sufficient.

Only a few because we've pushed back against defining state machines in DT.

> Finally, I am indeed concerned that it so hard to agree on a solution
> to deal with generic power sequences. There have been many attempts
> throughout the last ~4-5 years, but peoples strong opinions about
> different approaches mad them all fail. Isn't it time to finally just
> pick a solution and stick with it, even if it doesn't meet all peoples
> expectations?

No. I'm pretty that is not how kernel development works. Features get
merged when all are happy (or not paying attention).

From what I recall, all attempts have worked around the problem that
the driver model has no way to either force probe or provide a
pre-probe hook on probeable buses. This then leads to the work-around
defining the DT binding.

Rob
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
new file mode 100644
index 0000000..8ad98382
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-usb-generic.txt
@@ -0,0 +1,31 @@ 
+The power sequence for generic USB Devices
+
+Some hard-wired USB devices need to do power sequence to let the
+device work normally, the typical power sequence like: enable USB
+PHY clock, toggle reset pin, etc. But current Linux USB driver
+lacks of such code to do it, it may cause some hard-wired USB devices
+works abnormal or can't be recognized by controller at all. The
+power sequence will be done before this device can be found at USB
+bus.
+
+Required properties:
+- compatible : contains "usb-pwrseq-generic".
+
+Optional properties:
+- clocks: the input clock for USB device.
+- clock-frequency: the frequency for device's clock.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration in microsecond for assert reset signal.
+- enable-gpios: Should specify the GPIO for enable.
+- power-supply: The regulator of the power
+
+Example:
+
+usb2415_pwrseq: usb2415_pwrseq {
+	compatible = "usb-pwrseq-generic";
+	clocks = <&clks IMX6QDL_CLK_CKO>;
+	reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+	reset-duration-us = <3000>;
+	enable-gpios = <&gpio7 11 GPIO_ACTIVE_LOW>;
+	power-supply = <&reg_usb_power>;
+};
diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
index 1c35e7b..4c8a25e 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.txt
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -12,6 +12,7 @@  Required properties:
   for usbVID,PID.
 - reg: the port number which this device is connecting to, the range
   is 1-31.
+- usb-pwrseq: phandle for power sequence.
 
 Example:
 
@@ -24,5 +25,6 @@  Example:
 	hub: genesys@1 {
 		compatible = "usb5e3,608";
 		reg = <1>;
+		usb-pwrseq = <&usb_genesys_pwrseq>;
 	};
 }