Re: [PATCH v13 4/6] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Oleksii,

Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:

> On 23/09/2026 04:02, Volodymyr Babchuk wrote:
>> Hi Oleksii,
>>
>> Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx> writes:
>>
>> This is a great piece of documentation. It is larger than the text you
>> added to the 'doc' directory. So I believe it is better to put into a
>> design document, so it is will not be lost in the git commit messages.
>
> Hi Volodymyr,
>
> Thank you for a quick response.
>
> I've just recheked document in patch 6: 
> docs/hypervisor-guide/arm/firmware/arm-scmi.rst and
>
> compared it with the information provided in the commit description.
>
> As I can see all information provided in the description (add schemes 
> and dts examples at least)
>
> are present in the documentation. Also doc itself is more detailed then 
> the commit description.
>
> Maybe I missed something?

Ah, so you put this information in the last patch in the series. Okay,
so I missed it.

>>> This patch introduces SCI driver to support for ARM EL3 Trusted Firmware-A
>>> (TF-A) which provides SCMI interface with multi-agent support, as shown
>>> below.
>>>
>>>    +-----------------------------------------+
>>>    |                                         |
>>>    | EL3 TF-A SCMI                           |
>>>    +-------+--+-------+--+-------+--+-------++
>>>    |shmem1 |  |shmem0 |  |shmem2 |  |shmemX |
>>>    +-----+-+  +---+---+  +--+----+  +---+---+
>>> smc-id1 |        |         |           |
>>> agent1  |        |         |           |
>>>    +-----v--------+---------+-----------+----+
>>>    |              |         |           |    |
>>>    |              |         |           |    |
>>>    +--------------+---------+-----------+----+
>>>           smc-id0 |  smc-id2|    smc-idX|
>>>           agent0  |  agent2 |    agentX |
>>>                   |         |           |
>>>              +----v---+  +--v-----+  +--v-----+
>>>              |        |  |        |  |        |
>>>              | Dom0   |  | Dom1   |  | DomX   |
>>>              |        |  |        |  |        |
>>>              |        |  |        |  |        |
>>>              +--------+  +--------+  +--------+
>>>
>>> The EL3 SCMI multi-agent firmware is expected to provide SCMI SMC shared
>>> memory transport for every Agent in the system.
>>>
>>> The SCMI Agent transport channel defined by pair:
>>>   - smc-id: SMC id used for Doorbell
>>>   - shmem: shared memory for messages transfer, Xen page
>>>   aligned. Shared memory is mapped with the following flags:
>>>   MT_DEVICE_nGnRE.
>>>
>>> The follwoing SCMI Agents are expected to be defined by SCMI FW to enable 
>>> SCMI
>>> multi-agent functionality under Xen:
>>> - Xen management agent: trusted agents that accesses to the Base Protocol
>>> commands to configure agent specific permissions
>>> - OSPM VM agents: non-trusted agent, one for each Guest domain which is
>>>    allowed direct HW access. At least one OSPM VM agent has to be provided
>>>    by FW if HW is handled only by Dom0 or Driver Domain.
>>>
>>> The EL3 SCMI FW is expected to implement following Base protocol messages:
>>> - BASE_DISCOVER_AGENT (optional if agent_id was provided)
>>> - BASE_RESET_AGENT_CONFIGURATION (optional)
>>> - BASE_SET_DEVICE_PERMISSIONS (optional)
>>>
>>> The SCI SCMI SMC multi-agent driver implements following
>>> functionality:
>>> - The driver is initialized from the Xen SCMI container ``xen_scmi_config``
>>>    (compatible ``xen,sci``) placed under ``/chosen/xen``. Only the
>>>    ``arm,scmi-smc`` node that is a child of this container will bind to Xen;
>>>    other SCMI nodes (for example under ``/firmware``) are ignored to avoid
>>>    stealing the host OSPM instance.
>>>
>>> scmi_shm_1: sram@47ff1000 {
>>>            compatible = "arm,scmi-shmem";
>>>            reg = <0x0 0x47ff1000 0x0 0x1000>;
>>> };
>>> scmi_xen: scmi {
>>>          compatible = "arm,scmi-smc";
>>>          arm,smc-id = <0x82000003>; <--- Xen management agent smc-id
>>>          #address-cells = < 1>;
>>>          #size-cells = < 0>;
>>>          #access-controller-cells = < 1>;
>>>          shmem = <&scmi_shm_1>; <--- Xen management agent shmem
>>> };
>>>
>>> - The driver obtains Xen specific SCMI Agent's configuration from the
>>>    Host DT, probes Agents and builds SCMI Agents list. The Agents
>>>    configuration is taken from "scmi-secondary-agents" property where
>>>    first item is "arm,smc-id", second - "arm,scmi-shmem" phandle and
>>>    third is optional "agent_id":
>>>
>>> / {
>>>    chosen {
>>>      xen {
>>>        ranges;
>>>        xen_scmi_config {
>>>          compatible = "xen,sci";
>>>          #address-cells = <2>;
>>>          #size-cells = <2>;
>>>          ranges;
>>>
>>>     scmi-secondary-agents = <
>>>            0x82000002 &scmi_shm_0 0
>>>            0x82000004 &scmi_shm_2 2
>>>            0x82000005 &scmi_shm_3 3>; <--- func_id, shmem, agent_id
>>>          #scmi-secondary-agents-cells = <3>;
>> I don't think that this is the correct way of using -cells property.
>>
>> These properties are used either to provide information for **child** nodes
>> (like #address-cells or #size-cells) or to provide a number of cells to
>> encode a specifier for a domain (like #interrupt-cells).
>>
>> Here you are doing neither of these. I think the proper way is to define
>> secondary agents as children:
>>
>> agents {
>>          #address-cells = 1;
>>          #size-cells = 0;
>>           scmi_agent_2: scmi_agent@2 {
>>             compatible = "arm,scmi-agent";  // Actually, I am not sure if
>>                                             // we allowed to use 'arm' 
>> namespace
>>
>>             reg = <2>;                      // Agent id goes here
>>             shmmem = &scmi_shm_2;
>>             arm,smc-id = 0x82000004;
>>           };
>> }
>>
>> With this approach you don't need #scmi-secondary-agents-cells at all
>> and the whole structure becomes more device-tree-ish.
> Thank you for looking at this. I agree that the two cases you list are
> the most common ones, but they are not the only sanctioned ones: there is a
> third, long-standing pattern where a "#<name>-cells" property in a node
> declares the cell stride of a *non-phandle list property in that very same
> node*. That is exactly what "#scmi-secondary-agents-cells" does for
> "scmi-secondary-agents". Some evidence from the Devicetree Specification and
> from Linux:
>
> 1) "ranges" and "interrupt-map" are parsed with the cell counts of the node
>     that carries them, not of a child or of a phandle target.
>
>     dtc enforces this itself:
>
>     - scripts/dtc/checks.c:785 check_ranges_format() computes the entry 
> length
>       as (parent #address-cells + this node's #address-cells + this node's
>       #size-cells) and validates the length of "ranges" in this node 
> against it.
>
>     - scripts/dtc/checks.c:1601 check_interrupt_map():
>
>           cellsize = node_addr_cells(node);
>           cellsize += propval_cell(get_property(node, "#interrupt-cells"));
>
>       i.e. the stride of the "interrupt-map" entries in a node is taken from
>       "#address-cells" and "#interrupt-cells" *of that same node*. Linux 
> does
>       the same in drivers/of/irq.c.
>
>     So "a #*-cells property in node X describing the layout of a property in
>     node X" is not an abuse of the convention, it is how two of the core
>     Devicetree Specification properties are defined.
>
> 2) There is a precedent whose shape is identical to ours - a vendor property
>     holding a list, plus cells properties in the same node that describe 
> how to
>     decode it:
>
>     Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:36
>
>         ibm,#dma-address-cells:
>           description:
>             number of cells that are used to encode the physical address 
> field
>             of dma-window properties
>         ibm,#dma-size-cells:
>           description:
>             number of cells that are used to encode the size field of
>             dma-window properties
>         ibm,my-dma-window:
>           description: DMA window associated with this virtual I/O Adapter
>
>     and the example (same file, lines 96-98):
>
>         ibm,#dma-address-cells = <0x2>;
>         ibm,#dma-size-cells = <0x2>;
>         ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
>
>     Both cells properties are "required". The parser is
>     arch/powerpc/kernel/prom_parse.c:11 of_parse_dma_window(), which reads
>     "ibm,#dma-address-cells" and "ibm,#dma-size-cells" from the same 
> node "dn"
>     to walk "ibm,my-dma-window". There is no phandle and no child node
>     involved; this is exactly the "self-describing list property" pattern.
>
> 3) A "#*-cells" property does not have to describe a phandle specifier 
> at all:
>
>     - "#pinctrl-cells" (Documentation/devicetree/bindings/pinctrl/
>       pinctrl-single.yaml:55) gives the number of cells per entry of the 
> plain
>       "pinctrl-single,pins" property; drivers/pinctrl/devicetree.c:297
>       pinctrl_find_cells_size() looks it up in the parent/grandparent node.
>       No phandle specifier is involved.
>
>     - "#index-cells" 
> (Documentation/devicetree/bindings/usb/fsl,usbmisc.yaml:56)
>       is not a specifier for any domain either.
>
> So the convention in practice is "a #<foo>-cells property states how 
> many cells
> one <foo> entry occupies", and the entries may live in a child node's 
> property
> (#address-cells), in a consumer's phandle specifier (#interrupt-cells,
> #clock-cells), or in a property of the declaring node itself (ranges,
> interrupt-map, ibm,my-dma-window). Our usage falls into the third group.
>
> This bring us to the following conclusion that 
> #scmi-secondary-agents-cells usage
> doesn't technically break the device-tree convention with 2 exceptions:
>
> a) Vendor prefix. Documentation/devicetree/bindings/writing-bindings.rst:68
>     says "DO use a vendor prefix on device-specific property names", and all
>     the same-node precedents above are vendor-prefixed 
> ("ibm,#dma-address-cells").
>     Both of our properties are Xen-specific and live under /chosen/xen, 
> so if we
>     keep the flat form they should become "xen,scmi-secondary-agents" and
>     "xen,#scmi-secondary-agents-cells".
>
> b) Entry layout. In our list the phandle is the second cell
>     (<smc-id &shmem agent-id>), which prevents the use of the standard
>     phandle-array helpers - they all expect the phandle first. If we 
> keep the
>     flat form, reordering to <&shmem smc-id agent-id> would let the 
> property be
>     parsed as an ordinary phandle-array.
>
> As for your proposal: the child-node form you suggest is certainly
> more idiomatic, it makes the agent id an addressable "reg" and it 
> removes the
> optional 2-vs-3-cells variant altogether. My only reservations are that 
> it grows
> the Xen DT parser (node iteration instead of a single property walk) and 
> that the
> compatible string would need a namespace we own ("xen,scmi-agent" rather 
> than
> "arm,scmi-agent", as you correctly suspected).
>
> So I do not think the current form is invalid, but I have no objection to
> moving to child nodes if you prefer it. Please let me know which way you 
> would
> like me to go for v14 and I will respin accordingly.

Yes, I think more idiomatic format is better. Also, you need to write a
parser once, but users will write device tree more often.

The second thing that bothers me is the shared memory nodes inside the
xen_scmi_config node. I believe they should belong to reserved_memory, no?


>>>          xen,dom0-sci-agent-id = <0>;
>>>
>>>          scmi_shm_0: sram@47ff0000 {
>>>            compatible = "arm,scmi-shmem";
>>>            reg = <0x0 0x47ff0000 0x0 0x1000>;
>>>          };
>>>

[...]

-- 
WBR, Volodymyr