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

[Peter Chen <hzpeterchen@gmail.com>]

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.

-- 

Best Regards,
Peter Chen