On 21 June 2016 at 23:26, Rob Herring [off-list ref] wrote:

On Tue, Jun 21, 2016 at 10:11:17AM +0800, Peter Chen wrote:

quoted

On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:

quoted

On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:

quoted

On Fri, Jun 17, 2016 at 12:16:48PM -0500, Rob Herring wrote:

quoted

On Fri, Jun 17, 2016 at 5:09 AM, Peter Chen [off-list ref] wrote:

quoted

Add binding doc for generic usb power sequence driver, and update
generic usb device binding-doc accordingly.

[...]

quoted

quoted

quoted

          clocks = <&clks IMX6SX_CLK_CKO>;

          #address-cells = <1>;
          #size-cells = <0>;
          ethernet: asix at 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.

quoted

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