Re: [PATCH v3 2/4] dt-bindings: Add TI SCI PM Domains

[Rob Herring]

On Tue, Jan 17, 2017 at 1:48 AM, Tero Kristo <t-kristo@xxxxxx> wrote:
> On 17/01/17 00:12, Dave Gerlach wrote:
>>
>> On 01/13/2017 08:40 PM, Rob Herring wrote:
>>>
>>> On Fri, Jan 13, 2017 at 2:28 PM, Dave Gerlach <d-gerlach@xxxxxx> wrote:
>>>>
>>>> On 01/13/2017 01:25 PM, Rob Herring wrote:
>>>>>
>>>>>
>>>>> On Thu, Jan 12, 2017 at 9:27 AM, Dave Gerlach <d-gerlach@xxxxxx> wrote:
>>>>>>
>>>>>>
>>>>>> Rob,
>>>>>>
>>>>>> On 01/11/2017 03:34 PM, Rob Herring wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jan 9, 2017 at 11:57 AM, Dave Gerlach <d-gerlach@xxxxxx>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Rob,
>>>>>>>>
>>>>>>>> On 01/09/2017 11:50 AM, Rob Herring wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Jan 04, 2017 at 02:55:34PM -0600, Dave Gerlach wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>>>>>>> will hook into the genpd framework and allow the TI SCI
>>>>>>>>>> protocol to
>>>>>>>>>> control device power states.
>>>>>>>>>>
>>>>>>>>>> Also, provide macros representing each device index as understood
>>>>>>>>>> by TI SCI to be used in the device node power-domain references.
>>>>>>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Nishanth Menon <nm@xxxxxx>
>>>>>>>>>> Signed-off-by: Dave Gerlach <d-gerlach@xxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> v2->v3:
>>>>>>>>>>         Update k2g_pds node docs to show it should be a child
>>>>>>>>>> of pmmc
>>>>>>>>>> node.
>>>>>>>>>>         In early versions a phandle was used to point to pmmc and
>>>>>>>>>> docs
>>>>>>>>>> still
>>>>>>>>>>         incorrectly showed this.
>>>>>>>>>>
>>>>>>>>>>  .../devicetree/bindings/soc/ti/sci-pm-domain.txt   | 59
>>>>>>>>>> ++++++++++++++
>>>>>>>>>>  MAINTAINERS                                        |  2 +
>>>>>>>>>>  include/dt-bindings/genpd/k2g.h                    | 90
>>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>>  3 files changed, 151 insertions(+)
>>>>>>>>>>  create mode 100644
>>>>>>>>>> Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>>>>  create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>>>>>>
>>>>>>>>>> diff --git
>>>>>>>>>> a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..4c9064e512cb
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>>>>> @@ -0,0 +1,59 @@
>>>>>>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>>>>>>> +---------------------------------------------
>>>>>>>>>> +
>>>>>>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...)
>>>>>>>>>> that
>>>>>>>>>> is
>>>>>>>>>> +responsible for controlling the state of the IPs that are
>>>>>>>>>> present.
>>>>>>>>>> +Communication between the host processor running an OS and the
>>>>>>>>>> system
>>>>>>>>>> +controller happens through a protocol known as TI-SCI [1].
>>>>>>>>>> This pm
>>>>>>>>>> domain
>>>>>>>>>> +implementation plugs into the generic pm domain framework and
>>>>>>>>>> makes
>>>>>>>>>> use
>>>>>>>>>> of
>>>>>>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>>>>>>> +
>>>>>>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>>>>>>> +
>>>>>>>>>> +PM Domain Node
>>>>>>>>>> +==============
>>>>>>>>>> +The PM domain node represents the global PM domain managed by the
>>>>>>>>>> PMMC,
>>>>>>>>>> +which in this case is the single implementation as documented
>>>>>>>>>> by the
>>>>>>>>>> generic
>>>>>>>>>> +PM domain bindings in
>>>>>>>>>> Documentation/devicetree/bindings/power/power_domain.txt.
>>>>>>>>>> +Because this relies on the TI SCI protocol to communicate with
>>>>>>>>>> the
>>>>>>>>>> PMMC
>>>>>>>>>> it
>>>>>>>>>> +must be a child of the pmmc node.
>>>>>>>>>> +
>>>>>>>>>> +Required Properties:
>>>>>>>>>> +--------------------
>>>>>>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>>>>>>> +- #power-domain-cells: Must be 0.
>>>>>>>>>> +
>>>>>>>>>> +Example (K2G):
>>>>>>>>>> +-------------
>>>>>>>>>> +       pmmc: pmmc {
>>>>>>>>>> +               compatible = "ti,k2g-sci";
>>>>>>>>>> +               ...
>>>>>>>>>> +
>>>>>>>>>> +               k2g_pds: k2g_pds {
>>>>>>>>>> +                       compatible = "ti,sci-pm-domain";
>>>>>>>>>> +                       #power-domain-cells = <0>;
>>>>>>>>>> +               };
>>>>>>>>>> +       };
>>>>>>>>>> +
>>>>>>>>>> +PM Domain Consumers
>>>>>>>>>> +===================
>>>>>>>>>> +Hardware blocks that require SCI control over their state must
>>>>>>>>>> provide
>>>>>>>>>> +a reference to the sci-pm-domain they are part of and a unique
>>>>>>>>>> device
>>>>>>>>>> +specific ID that identifies the device.
>>>>>>>>>> +
>>>>>>>>>> +Required Properties:
>>>>>>>>>> +--------------------
>>>>>>>>>> +- power-domains: phandle pointing to the corresponding PM domain
>>>>>>>>>> node.
>>>>>>>>>> +- ti,sci-id: index representing the device id to be passed
>>>>>>>>>> oevr SCI
>>>>>>>>>> to
>>>>>>>>>> +            be used for device control.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> As I've already stated before, this goes in power-domain cells.
>>>>>>>>> When
>>>>>>>>> you
>>>>>>>>> have a single thing (i.e. node) that controls multiple things, then
>>>>>>>>> you
>>>>>>>>> you need to specify the ID for each of them in phandle args.
>>>>>>>>> This is
>>>>>>>>> how
>>>>>>>>> irqs, gpio, clocks, *everything* in DT works.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> You think the reasoning for doing it this way provided by both
>>>>>>>> Ulf and
>>>>>>>> myself on v2 [1] is not valid then?
>>>>>>>>
>>>>>>>> From Ulf:
>>>>>>>>
>>>>>>>> To me, the TI SCI ID, is similar to a "conid" for any another
>>>>>>>> "device
>>>>>>>> resource" (like clock, pinctrl, regulator etc) which we can describe
>>>>>>>> in DT and assign to a device node. The only difference here, is that
>>>>>>>> we don't have common API to fetch the resource (like clk_get(),
>>>>>>>> regulator_get()), but instead we fetches the device's resource from
>>>>>>>> SoC specific code, via genpd's device ->attach() callback.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Sorry, but that sounds like a kernel problem to me and has nothing to
>>>>>>> do with DT bindings.
>>>>>>>
>>>>>>>> From me:
>>>>>>>>
>>>>>>>> Yes, you've pretty much hit it on the head. It is not an index
>>>>>>>> into a
>>>>>>>> list
>>>>>>>> of genpds but rather identifies the device *within* a single
>>>>>>>> genpd. It
>>>>>>>> is
>>>>>>>> a
>>>>>>>> property specific to each device that resides in a ti-sci-genpd,
>>>>>>>> not a
>>>>>>>> mapping describing which genpd the device belongs to. The generic
>>>>>>>> power
>>>>>>>> domain binding is concerned with mapping the device to a specific
>>>>>>>> genpd,
>>>>>>>> which is does fine for us, but we have a sub mapping for devices
>>>>>>>> that
>>>>>>>> exist
>>>>>>>> inside a genpd which, we must describe as well, hence the ti,sci-id.
>>>>>>>>
>>>>>>>>
>>>>>>>> So to summarize, the genpd framework does interpret the phandle
>>>>>>>> arg as
>>>>>>>> an
>>>>>>>> index into multiple genpds, just as you've said other frameworks do,
>>>>>>>> but
>>>>>>>> this is not what I am trying to do, we have multiple devices within
>>>>>>>> this
>>>>>>>> *single* genpd, hence the need for the ti,sci-id property.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Fix the genpd framework rather than work around it in DT.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I still disagree that this has nothing to do with DT bindings, as the
>>>>>> current DT binding represents something different already. I am
>>>>>> trying to
>>>>>> extend it to give me additional information needed for our
>>>>>> platforms. Are
>>>>>> you saying that we should break what the current DT binding already
>>>>>> represents to mean something else?
>>>>>
>>>>>
>>>>>
>>>>> No idea because what's the current binding? From the patch, looks like
>>>>> a new binding to me.
>>>>
>>>>
>>>>
>>>> Yes, ti,sci-id is a new binding. I am referring to the current
>>>> meaning of
>>>> the "power-domains" binding, which is where you are asking this
>>>> property to
>>>> be added, in "power-domains" cells. This is documented here [1] in the
>>>> kernel, although looking at it I must admit it is not very clear.
>>>>
>>>> The power-domains cell represents an offset into an array of power
>>>> domains,
>>>> if you choose to use it. That's what the genpd framework is hard
>>>> coded to
>>>> interpret it as. This is correct, as it is an index into a static
>>>> list of
>>>> power domains, used to identify which power domain a device belongs to,
>>>> which is exactly what the genpd framework itself is concerned with.
>>>> This is
>>>> already how it is used in the kernel today.
>>>
>>>
>>> Strictly speaking, the cells are purely for the interpretation of the
>>> phandle they are associated with. If some controller wants to have 20
>>> cells, then it could assuming a good reason. The reality is we tend to
>>> align the meaning of the cells. If genpd is interpreting the cells and
>>> not letting the driver for the power domain controller interpret them,
>>> then still, genpd needs to be fixed.
>>
>>
>> Ok, perhaps the genpd folks on the thread can jump in here with any
>> thoughts that they have.
>>
>>>
>>> IIRC, initially it was said genpd required 0 cells, hence my confusion.
>>>
>>>> My ti,sci-id is not an index into a list of power domains, so it
>>>> should not
>>>> go in the power-domains cells and go against what the power-domains
>>>> binding
>>>> says that the cell expects. We have one single power domain, and the new
>>>> ti,sci-id binding is not something the genpd framework itself is
>>>> concerned
>>>> with as it's our property to identify a device inside a power domain,
>>>> not to
>>>> identify which power domain it is associated with.
>>>
>>>
>>> What is the id used for? I can understand why you need to know what
>>> power domain a device is in (as power-domains identifies), but not
>>> what devices are in a power domain.
>>
>>
>> We have a system control processor that provides power management
>> services to the OS and it responsible for handling the power state of
>> each device. This control happens over a communication interface we have
>> called TI SCI (implemented at drivers/firmware/ti-sci.c). The
>> communication protocol uses these ids to identify each device within the
>> power domain so that the control processor can do what is necessary to
>> enable that device.
>
>
> I think a minor detail here that Rob might be missing right now is, that the
> ti,sci-id is only controlling the PM runtime handling, and providing the ID
> per-device for this purpose only. AFAIK, it is not really connected to the
> power domain anymore as such, as we don't have power-domains / per device
> anymore as was the case in some earlier revision of this work.

So you used to have multiple power domains and now you don't? Did the
h/w change?

> One could argue though that the whole usage of power-domains is now moot, as
> we basically only have implemented one genpd in the whole SoC, which doesn't
> really reflect the reality. I wonder if better approach would be to have
> this replaced with proper power domains at some point (if needed), and just
> have a runtime-pm implementation in place for the devices that require it.

We're talking about bindings here. Any explanation in terms of
runtime-pm and genpd a) goes over my head and b) isn't relevant to
describing the hardware.

I'm still confused with how many power domains (as defined by the h/w
design) there are controlled by the SCI? Is it 1 or multiple? If 1,
then why do you need the sci-id? For other things that are not the
power domain?

Or perhaps the SCI abstracts things such that you don't really know
what the relationship between devices and power domains is? IOW, you
can't tell from the SCI interface how many power domains there are.

>
> So, as an example in DT, we would only have:
>
> uart0: serial@02530c00 {
>   compatible = "xyz";
>   ...
>   ti,sci-id = <K2G_DEV_UART0>;
> };
>
> This is somewhat analogous to what OMAP family of SoCs have in place now,
> under "ti,hwmods" property. I also wonder if the "ti,sci-id" should be
> replaced with something like "ti,sci-dev-id" to make its purpose clearer.

Describing in terms of hwmods doesn't help me either. Never understood
that either. Sorry.

Rob