Date:   Mon, 12 Jun 2017 18:39:18 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Matt Sealey <neko@...uhatsu.net>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Roy Franz <roy.franz@...ium.com>,
        Harb Abdulhamid <harba@...eaurora.org>,
        Nishanth Menon <nm@...com>, Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [RFC PATCH 1/8] Documentation: add DT binding for ARM System
 Control and Management Interface(SCMI) protocol

Hi Matt,


Thanks for starting this discussion on the list.

On Fri, Jun 09, 2017 at 01:12:50PM -0500, Matt Sealey wrote:
> Hullo all,
> 
> This is a long one.. apologies for odd linefeeds and so on.
> 
> On Wed, Jun 7, 2017 at 11:10 AM, Sudeep Holla <sudeep.holla@....com> wrote:
> 
> > +Clock/Performance bindings for the clocks/OPPs based on SCMI Message
> Protocol
> > +------------------------------------------------------------
> > +
> > +This binding uses the common clock binding[1].
> > +
> > +Required properties:
> > +- compatible : shall be "arm,scmi-clocks" or "arm,scmi-perf-domains"
> 
> After a little thought, there are a couple objections to be made here.
> Firstly, the SCMI protocol families are  discoverable - you only really need
> to know that it is usable (and where to use it, mboxes etc.) - whether it
> supports the clock management, performance domains, power domains et al.
> protocols is a function of querying the base protocol for a list.
>
> These protocols are identified by a value, several of which are
> standardized, some being vendor extension numbers. All protocols must be
> able to be queried for information.
>

Agreed on most the above.

> As such, defining compatible properties for each protocol is treading that
> fine line of tying device trees to particular driver subsystems and giving
> operating systems an ability to ignore any discovery procedure. While I
> can't make a case for clock management (which should obviously conform with
> a particular clock management definition in DT, as already defined), there
> is plenty of past evidence of bindings for particular devices being mis-used
> or used in non-intended ways (regulators as reset GPIOs is the one that
> immediately came to mind) in lieu of a more fleshed out way of defining a
> particular class of device for a binding. The same would be true of tying a
> 'performance domain' to the concept of clock management.
>

As I mentioned in private, I reused clock for simplicity and most of the
platforms already do that. But yes, that's no valid argument to just
continue the legacy. I am fine to break clock and performance domains.
The main problem IMO is that it's not well defined either in theis
specification or architecture to an extent that we can define a standard
binding and live with it. We are/already have seen lost of churn around
this bindings and that's one of the reason I chose to reuse existing
bindings.

> From the point of view of being able to specify things against a particular
> binding (whatever that might be), one could imagine something that mapped
> protocols to those bindings without introducing compatible names. SCMI ids
> would be verbatim, and per-protocol. Things like clock-indices are therefore
> not relevant and defining which indices go with which protocol at the SCMI
> level isn't needed anymore. It is really up to the protocol how many cells
> it would need to define it's protocol behavior but for the purpose of some
> standardization, we could imagine a binding that defined protocols as such:
> 
> scmi: arm_scmi {
>         compatible = "arm,scmi,1.0";

I prefer v1.0 dropped based on the same argument that it's discoverable.
If we don't agree on that then the whole discussion falls apart. I
assume you agree on dropping versioning just to continue the discussion
here.

>         mboxes = <y>;
>         shmem = <x>;
>         protocols {
>               scmi_clocks: protocol@...4 {
>                         #whatever-cells = 3;
>                };
>                foo_smic: protocol@...9 {
>                         #foo-cells = 4;
>                         #bar-cells = 5;
>                 };
>         };
> };
> 
> uart: myuart@...00000 {
>         compatible = "arm,pl011";
>         clocks = <&foo_smic 3>;
> };
> 
> If you manage to get a device tree that specifies a clock but there is no
> protocol 0x89 then you're just as hosed as if you specified an
> arm,scmi-clocks node when the protocol was not supported by SCMI itself, so
> we don't gain any new dangers, but we do gain the ability to instantiate
> SCMI, discover protocols, and then load drivers against those protocols,
> without duplicating the discovery process with a hardcoded tree. Device
> trees, from my point of view, are a contract between the SoC & board
> designer and the OS (helped along by firmware, hopefully). They shouldn't be
> dictating the driver behavior to be applied at this kind of level. 

Agreed.

> Device trees need to be rock solid - agile development is fine but as soon
> as you ship, changing the device tree means cutting off support for existing
> software, or only working with augmented features on new software and
> severely reduced functionality on old software. That can be as simple as not
> being able to go to Turbo mode, or as bad as an inability to apply thermal
> limits and burning someone's board. If we define a specific binding of a
> specific protocol to a specific way of interacting with that device which is
> a purely software construct (like treating performance domain as an abstract
> clock interface with a particular number of cells or clock-like behavior),
> then you lock down protocols to *existing* OS subsystems. That means
> maintaining that subsystem and device tree specification to retain behavior,
> which is a lot of maintaining.

In total agreement with you.

> It shouldn't be necessary to actually define which protocols exist and what
> number of cells they use in the device tree binding, because this is
> actually documented elsewhere. Just as we do not create compatible
> properties for new devices which work the same as old ones, or redefine
> features which would otherwise be ascertained by some kind of discovery (PCI
> devices, USB devices, but even as simple just reading out an ID register
> from a device to determine if it has a particular feature), we shouldn't do
> this to lock down a further discovery model for activity types or
> programmers' models under those protocols.

Reasonable.

> Here's where I fall down: with a variable number of cells per protocol
> required (potentially) and no method to be able to assign a particular
> protocol's device or functionality to something else, discovery of what maps
> where has to be done as well. There's little of that in SCMI, that is to say
> it wouldn't be possible to infer that clock_id 20 in protocol 0x14 is the
> clock that goes with cpu@0. It is also, per the SCMI spec, a firmware table
> responsibility to define any dependencies on other parts of the protocol
> (for instance, clock trees). The best I can think of right now is that this
> would just have to be done on a global SCMI level:

> 
> scmi: arm_scmi {
>         compatible = "arm,scmi,1.0";
>         mboxes = <y>;
>         shmem = <x>;
>         disable-protocols = <0x87, 0x88>;
>	  // these are buggy, don't load drivers even if they're implemented
> 
>         #whatever-cells = 3;
>         required-whatever-prop;
> 
>         #foo-cells = 4;
> 
>         #bar-cells = 5;
>         optional-bar-prop = <5, 10, 15>;
> 
>         #clock-cells = 10;
> };
> 
> #define SCMI_PROTO_CLOCKMGMT 0x14
> #define SCMI_PROTO_VENDOR_CLOCKS 0x89
> uart: myuart {
>         compatible = "arm,pl011";
>         clocks = <&scmi SCMI_PROTO_CLOCKMGMT 3 0 0 0 0 0 0 0 0>, <&scmi
> SCMIU_PROTO_VENDOR_CLOCKS 3 4 7 99 0 0 0x9000 3>;
> };
> 

OK, I really don't like this. But if other's think this is better, then
I am fine with that. I prefer the first option you have present.

IMO we might collide with the property name soon i.e. 2 different
existing bindings having same property name.

> .. it is up to the driver to figure out what exactly this does in real
> life, without having to lock it to the clock et al. binding, but at least
> all drivers using protocols must be able to parse the number of cells
> defined. If a protocol only needs 3 cells, but another needs 10 cells for
> the same thing, then both protocols will by definition be defined as 10-cell
> groups. It implies hat any device that can be controlled or affected by SCMI
> can list the devices, and the protocol driver will be required to parse the
> remaining cells and ignore them. As long as the device tree can cover all
> cases in it's #foo-cells or other binding properties, it would be most
> flexible here.
>
> I don't like the lockdown of having to cover every binding that gets used
> whether it's truly for that device type or not, but it would be the most
> flexible within the current framework, without defeating discovery.

Sounds good, but as I said I am worried if we collide.

> We would do well to come up with a way of defining abstract interfaces to
> firmware or other processors that don't rely on there being a fixed binding
> that dictates what kind of device it is, where there isn't a way of defining
> that device type in a generic manner. There's no way of doing this right now
> and letting the driver in the OS know what's necessary - well, there is,
> things like RTAS support on CHRP got away with this in the old days, and
> PSCI does something very similar (which covers quite a lot of CPU management
> and also system domain activity), so it's not like we've come up against the
> issue before. But it's not been resolved when a device node would need to
> refer back to those abstraction interface nodes where there's not a
> reasonable way of binding the definition of the reference.

Exactly, I am trying to reuse existing bindings with minimum change to
provide the glue to SCMI interface. But I am open for any suggestions.

> RTAS had a property in every device node that depended on it and would be
> affected by it, "used-by-rtas". Perhaps we could augment devices with an
> "managed-by" property likewise to point to the protocol node.
> #protocol-cells might be a nice way of defining cell sizes generically
> (where protocol would be the name of the protocol - #scmi-cells perhaps -
> and the format of #scmi-cells would need to include the protocol number).
> Wrapping that up in a generic binding means each protocol then gets control
> of it's own world, and each device that is affected by that protocol would
> be able to realize this.

Interesting, I need more opinions here.

> If anyone comes up with a particular PSCI-like protocol for anything here
> that kind of adaptable binding for device/platform abstraction frameworks
> with non-discrete methods of working (i.e. it might not be able to be
> defined as "just a clock" or "just a power domain" or "just a thermal
> framework" or any combination without reducing functionality that would
> otherwise come from some built-in discovery system) would come in very
> handy.
>
> Just thoughts right now, though. I definitely don't have the whole story
> down, but it is definitely something to think about.

Sure, thanks again for such a detailed mail. Hope to see more
discussions if we need to make such drastic changes.

-- 
Regards,
Sudeep

Powered by blists - more mailing lists