Re: [PATCH v4 0/2] dt-bindings: Intorduce domain-controller

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Oleksii,

On 18.08.22 11:05, Oleksii Moisieiev wrote:
> On Mon, Aug 15, 2022 at 06:37:23PM +0200, Ahmad Fatoum wrote:
>> Hello Oleksii,
>>
>> On 07.07.22 12:25, Oleksii Moisieiev wrote:
>>> Introducing the domain controller provider/consumenr bindngs which allow to
>>> divided system on chip into multiple domains that can be used to select
>>> by who hardware blocks could be accessed.
>>> A domain could be a cluster of CPUs, a group of hardware blocks or the
>>> set of devices, passed-through to the Guest in the virtualized systems.
>>>
>>> Device controllers are typically used to set the permissions of the hardware
>>> block. The contents of the domain configuration properties are defined by the
>>> binding for the individual domain controller device.
>>>
>>> The device controller conception in the virtualized systems is to set
>>> the device configuration for SCMI (System Control and Management
>>> Interface) which controls clocks/power-domains/resets etc from the
>>> Firmware. This configuratio sets the device_id to set the device permissions
>>> for the Fimware using BASE_SET_DEVICE_PERMISSIONS message (see 4.2.2.10 of [0]).
>>> There is no BASE_GET_DEVICE_PERMISSIONS call in SCMI and the way to
>>> determine device_id is not covered by the specification.
>>> Device permissions management described in DEN 0056, Section 4.2.2.10 [0].
>>> Given parameter should set the device_id, needed to set device
>>> permissions in the Firmware.
>>> This property is used by trusted Agent (which is hypervisor in our case)
>>> to set permissions for the devices, passed-through to the non-trusted
>>> Agents. Trusted Agent will use device-perms to set the Device
>>> permissions for the Firmware (See Section 4.2.2.10 [0] for details).
>>> Agents concept is described in Section 4.2.1 [0].
>>>
>>> Domains in Device-tree node example:
>>> usb@e6590000
>>> {
>>>     domain-0 = <&scmi 19>; //Set domain id 19 to usb node
>>>     clocks = <&scmi_clock 3>, <&scmi_clock 2>;
>>>     resets = <&scmi_reset 10>, <&scmi_reset 9>;
>>>     power-domains = <&scmi_power 0>;
>>> };
>>>
>>> &scmi {
>>>     #domain-cells = <1>;
>>> }
>>>
>>> All mentioned bindings are going to be processed by XEN SCMI mediator
>>> feature, which is responsible to redirect SCMI calls from guests to the
>>> firmware, and not going be passed to the guests.
>>>
>>> Domain-controller provider/consumenr concept was taken from the bus
>>> controller framework patch series, provided in the following thread:
>>> [1].
>>
>> I also was inspired by Benjamin's series to draft up a binding, but for a slightly
>> different problem: Some SoCs like the i.MX8MP have a great deal of variation
>> in which IPs are actually available. After factory testing, fuses are burnt
>> to describe which IPs are available and as the upstream DT only describes
>> the full featured SoCs, either board DT or bootloader is expected to turn
>> off the device that are unavailable.
>>
>> What I came up with as a binding for the bootloader to guide its fixup
>> looks very similar to what you have:
>>
>> feat: &ocotp { /* This is the efuse (On-Chip OTP) device */
>>     feature-controller;
>>     feature-cells = <1>;
>> };
>>
>> &vpu_g1 {
>>     features-gates = <&feat IMX8MP_VPU>;
>> };
>>
>> The OCOTP driver would see that it has a feature-controller property and register
>> a callback with a feature controller framework that checks whether a device
>> is available. barebox, that I implemented this binding for, would walk
>> the kernel device tree on boot looking for the feature-gates property and then
>> disable/delete nodes as indicated without having to write any SoC specific code
>> and especially without hardcoding node names and hierarchies, which is quite brittle.
>>
>> There was a previous attempt at defining a binding for this, but Rob's NAK
>> mentioned that a solution should cover both cases:
>>
>>  https://urldefense.com/v3/__https://lore.kernel.org/all/20220324042024.26813-1-peng.fan@oss.nxp.com/__;!!GF_29dbcQIUBPA!2j_vN6Jc1k2XI3EegAC2yzTLgJ1Rw1DhDrjGF03a5tDtOGpm_qp9B0zHJeAJzw-fWOeJp5HtnzYmOJZ0XPJxHzHFDBt_$  [lore[.]kernel[.]org]
>>
>> Having implemented nearly the same binding as what you describe, I obviously like your
>> patch. Only thing I think that should be changed is the naming. A domain doesn't
>> really describe this gated-by-fuses scenario I have. Calling it feature-gates
>> instead OTOH makes sense for both your and my use case. Same goes for the documentation
>> that could be worded more generically. I am open to other suggestions of course. :-)
>>
>> Also a general gpio-controller like property would be nice. It would allow drivers
>> to easily check whether they are supposed to register a domain/feature controller.
>> For devices like yours where a dedicated device node represents the domain controller,
>> it's redundant, but for a fuse bank, it's useful. #feature-cells could be used for
>> that, but I think a dedicated property may be better.
>>
>> Let me know what you think and thanks for working on this!
>>
>> Cheers,
>> Ahmad
>>
> 
> Hello Ahmad,
> 
> I'm very happy that you are interested in my proposal. It will be great
> if we produce common binding to suite both our requirements.
> I agree that binding should be renamed, but I don't think feature-gates
> name would fit my case.
> IIUC both our cases requires different devices across the system to
> provide some information to the controller device. This information
> could be used to identify the devices later or to make some
> controller-specific configuration. In this case I would prefer name
> "device-feature" or "bus-domain", suggested by Linus Walleij.
> Also I like your idea to add dedicated property. This will make bindings
> more clear.
> Summarizing all above, I would suggest the following names:
> 
>  feat: &ocotp { /* This is the efuse (On-Chip OTP) device */
>      device-feature-controller;
>      device-feature-cells = <1>;
>  };
> 
>  &vpu_g1 {
>      device-features = <&feat IMX8MP_VPU>;
>  };
> 
> What do you think about this?

Sorry for the late answer. Full plate before vacation :)

A device- prefix for device properties is kind of redundant IMO.
And [device-]features is somewhat ambiguous (it's not
a list of features of the device, but a list of features that
control the device). I see that gates might sounds a bit odd, how about
feature-domains, feature-domain-controller, #feature-domain-cells?

Cheers,
Ahmad

> 
> Best regards,
> Oleksii.
> 
>>
>>>
>>> I think we can cooperate with the bus controller framework developers
>>> and produce the common binding, which will fit the requirements of both
>>> features
>>>
>>> Also, I think that binding can also be used for STM32 ETZPC bus
>>> controller feature, proposed in the following thread: [2].
>>>
>>> Looking forward for your thoughts and ideas.
>>>
>>> [0] https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!2j_vN6Jc1k2XI3EegAC2yzTLgJ1Rw1DhDrjGF03a5tDtOGpm_qp9B0zHJeAJzw-fWOeJp5HtnzYmOJZ0XPJxH59KKjhc$  [developer[.]arm[.]com]
>>> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20190318100605.29120-1-benjamin.gaignard@st.com/__;!!GF_29dbcQIUBPA!2j_vN6Jc1k2XI3EegAC2yzTLgJ1Rw1DhDrjGF03a5tDtOGpm_qp9B0zHJeAJzw-fWOeJp5HtnzYmOJZ0XPJxHy1kyyWZ$  [lore[.]kernel[.]org]
>>> [2] https://urldefense.com/v3/__https://lore.kernel.org/all/20200701132523.32533-1-benjamin.gaignard@st.com/__;!!GF_29dbcQIUBPA!2j_vN6Jc1k2XI3EegAC2yzTLgJ1Rw1DhDrjGF03a5tDtOGpm_qp9B0zHJeAJzw-fWOeJp5HtnzYmOJZ0XPJxHzVdVT4B$  [lore[.]kernel[.]org]
>>>
>>> ---
>>> Changes v1 -> V2:
>>>    - update parameter name, made it xen-specific
>>>    - add xen vendor bindings
>>>
>>> Changes V2 -> V3:
>>>    - update parameter name, make it generic
>>>    - update parameter format, add link to controller
>>>    - do not include xen vendor bindings as already upstreamed
>>>
>>> Changes V3 -> V4:
>>>    - introduce domain controller provider/consumer device tree bindings
>>>    - making scmi node to act as domain controller provider when the
>>>      device permissions should be configured
>>> ---
>>>
>>> Oleksii Moisieiev (2):
>>>   dt-bindings: Document common device controller bindings
>>>   dt-bindings: Update scmi node description
>>>
>>>  .../bindings/domains/domain-controller.yaml   | 80 +++++++++++++++++++
>>>  .../bindings/firmware/arm,scmi.yaml           | 25 ++++++
>>>  2 files changed, 105 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/domains/domain-controller.yaml
>>>
>>
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | https://urldefense.com/v3/__http://www.pengutronix.de/__;!!GF_29dbcQIUBPA!2j_vN6Jc1k2XI3EegAC2yzTLgJ1Rw1DhDrjGF03a5tDtOGpm_qp9B0zHJeAJzw-fWOeJp5HtnzYmOJZ0XPJxH_HqFmwM$  [pengutronix[.]de]  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |