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

[Ulf Hansson]

On 21 June 2016 at 23:26, Rob Herring <robh at 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 at 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 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.

>>
>> 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