Re: [RFC 1/2] pwrseq: Add subsystem to handle complex power sequences

[Olof Johansson]

On Fri, Jun 20, 2014 at 12:27 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 06/19/2014 07:18 PM, Olof Johansson wrote:
>> Hi,
>>
>>
>>
>> On Thu, Jun 19, 2014 at 6:04 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>> The pwrseq subsystem handles complex power sequences, typically useful
>>> for subsystems that makes use of discoverable buses, like for example
>>> MMC and I2C.
>>>
>>> The pwrseq subsystem is dependant on CONFIG_OF to be able to parse a DT
>>> childnode to find out what power sequence method to bind for a device.
>>>
>>> From the DT childnode, the pwrseq DT parser tries to locate a
>>> "power-method" property, which string is matched towards the list of
>>> supported pwrseq methods.
>>>
>>> Each pwrseq method implements it's own power sequence and interfaces
>>> the pwrseq core through a few callback functions.
>>>
>>> To instantiate a pwrseq method, clients shall use the devm_pwrseq_get()
>>> API. If needed, clients can explicity drop the references to a pwrseq
>>> method using devm_pwrseq_put() API.
>>>
>>> Besides instantiation, the pwrseq API provides clients opportunity to
>>> select a certain power state. In this intial version, PWRSEQ_POWER_ON
>>> and PWRSEQ_POWER_OFF are supported. Those are also mandatory for each
>>> pwrseq method to support.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/pwrseq/pwrseq.txt          |   48 ++++++
>>>  drivers/Makefile                                   |    2 +-
>>>  drivers/pwrseq/Makefile                            |    2 +
>>>  drivers/pwrseq/core.c                              |  175 ++++++++++++++++++++
>>>  drivers/pwrseq/core.h                              |   37 +++++
>>>  drivers/pwrseq/method.c                            |   38 +++++
>>>  include/linux/pwrseq.h                             |   50 ++++++
>>>  7 files changed, 351 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>>>  create mode 100644 drivers/pwrseq/Makefile
>>>  create mode 100644 drivers/pwrseq/core.c
>>>  create mode 100644 drivers/pwrseq/core.h
>>>  create mode 100644 drivers/pwrseq/method.c
>>>  create mode 100644 include/linux/pwrseq.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwrseq/pwrseq.txt b/Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>>> new file mode 100644
>>> index 0000000..80848ae
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwrseq/pwrseq.txt
>>> @@ -0,0 +1,48 @@
>>> +Power sequence DT bindings
>>> +
>>> +Each power sequence method has a corresponding "power-method" property string.
>>> +This property shall be set in a subnode for a device. That subnode should also
>>> +describe resourses which are specific to that power method.
>>> +
>>> +Do note, power sequences as such isn't encoded through DT. Instead those are
>>> +implemented by each power method.
>>> +
>>> +Required subnode properties:
>>> +- power-method: should contain the string for the power method to bind.
>>> +
>>> +       Supported power methods: None.
>>> +
>>> +Example:
>>> +
>>> +Note, the "clock" power method in this example isn't actually supported, but
>>> +used to visualize how a childnode could be described.
>>> +
>>> +// WLAN SDIO channel
>>> +sdi1_per2@80118000 {
>>> +       compatible = "arm,pl18x", "arm,primecell";
>>> +       reg = <0x80118000 0x1000>;
>>> +       interrupts = <0 50 IRQ_TYPE_LEVEL_HIGH>;
>>> +
>>> +       dmas = <&dma 32 0 0x2>, /* Logical - DevToMem */
>>> +              <&dma 32 0 0x0>; /* Logical - MemToDev */
>>> +       dma-names = "rx", "tx";
>>> +
>>> +       clocks = <&prcc_kclk 2 4>, <&prcc_pclk 2 6>;
>>> +       clock-names = "sdi", "apb_pclk";
>>> +
>>> +       arm,primecell-periphid = <0x10480180>;
>>> +       max-frequency = <100000000>;
>>> +       bus-width = <4>;
>>> +       non-removable;
>>> +       pinctrl-names = "default", "sleep";
>>> +       pinctrl-0 = <&sdi1_default_mode>;
>>> +       pinctrl-1 = <&sdi1_sleep_mode>;
>>> +
>>> +       status = "okay";
>>> +
>>> +       pwrseq: pwrseq1 {
>>> +               power-method = "clock";
>>> +               clocks = <&someclk 1 2>, <&someclk 3 4>;
>>> +               clock-names = "pwrseq1", "pwrseq2";
>>> +       };
>>
>> I am strongly against the subnode approach as a general framework. We
>> don't have a subnode for interrupts, nor for clocks or pinctrl. So why
>> should we have it for the power sequencing?
>>
>> Sure, that fits the linux driver model better, but that's irrelevant
>> w.r.t. describing the hardware.
>
> Actually this is about describing the hardware, when you have e.g. an
> mmc device which needs pwrseq, there will be 2 sets of certain
> resources, ie clocks for the host controller and clocks going directly
> to the mmc device. I think putting those both in the same subnode is
> a BAD idea, so we really do need a subnode to group the pwrseq resources
> together.

I disagree.

The clock is the input to the module, and it is what needs to be
enabled for the module to work. It's not the input to some
power-sequence component on the module, or next to the module on the
bus.

It probably makes sense to not use the standard names for the new
resources. I.e. I'm OK with using new property names that are needed
for the device vs what's needed for the MMC controller, but not with a
subnode. I.e. we don't need to use "clocks" for the clocks, we can use
something else.

-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/