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

[Ulf Hansson]

On 20 June 2014 10:14, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 06/20/2014 10:02 AM, Olof Johansson wrote:
>> 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.
>
> Right, it is an input to the sdio-module, not to the mmc-host, so its an
> input to a different piece of hardware (at different ends of the mmc bus),
> but since the mmc-bus normally is fully discoverable we've no node for the
> other end of the bus.
>
> So from the mmc-host pov, which is the one which needs to bind the pwrseq
> driver, as that needs to be done before it can probe its bus, this is
> a different piece of hardware, hence a subnode to the host makes perfect
> sense.  This is in no way part of the host, so certainly it does not belong
> inside the hosts subnode.

I fully agree with you Hans here.

If we were to put this information in the host's DT node, that would
be a wrong description of the hardware. Currently, I can't think of
anything better than a subnode, but I am open to suggestions.

Kind regards
Uffe

>
>> It probably makes sense to not use the standard names for the new
>> resources.
>
> I disagree, being able to use standard names is very useful, and actually
> is a must if we don't want to have to have special versions of devm_get_clk,
> and all other devm_get_xxx esp. for pwrseq stuff.
>
> Regards,
>
> Hans
--
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/