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

[Arend van Spriel]

On 01-07-14 18:42, Ulf Hansson wrote:
> 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.

It could also be a property in the mmc-host that references a gpio
and/or clock for the simple sequences. For the complex sequence it could
similarly reference to a power-seq node somewhere else in the DT. There
may not be a functional difference. It just indicates that the power
sequence is not part of the mmc host, but an optional resource needed
for bus operation, ie. make sdio device discoverable.

Regards,
Arend

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