| Message ID | 1466158165-9380-9-git-send-email-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 = <®_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.
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 = <®_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
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 = <®_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
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.
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 = <®_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.
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 = <®_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.
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
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
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
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 --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 = <®_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>; }; }
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