Re: [PATCH v2 1/5] dt-bindings: remoteproc: sse710: Add the External Systems remote processors

Krzysztof Kozlowski
2024-09-27T09:01:14Z

On 23/09/2024 19:19, Abdellatif El Khlifi wrote:

>>>
>>> The Host Base System Control [3] is the big block containing various functionalities (MMIO registers).
>>> Among the functionalities, the two remote cores registers (aka External system 0 and 1).
>>> The remote cores have two registers each.
>>>
>>> 1/ In the v1 patchset, a valid point was made by the community:
>>>
>>> Right now it seems somewhat tenuous to describe two consecutive
>>> 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's
>>
>> ARM is not special, neither this hardware is. Therefore:
>> 1. Each register as reg: nope, for obvious reasons.
>> 2. One device for entire syscon: quite common, why do you think it is
>> somehow odd?
>> 3. If you quote other person, please provide the lore link, so I won't
>> spend useless 5 minutes to find who said that or if it was even said...
>
> Please have a look at this lore link [1]. The idea is to add syscon
> and regmap support which I did in the v2 patchset.
>
> [1]: https://lore.kernel.org/all/ZfMVcQsmgQUXXcef@bogus/

There is nothing there about DT bindings. We do not talk here about
drivers. MFD and regmap are Linux driver stuff, not bindings.

I said nothing about not using MFD, regmap or whatever driver stuff you
want. We talk *only* about bindings. Syscon is mentioned there but I am
sorry - that quite a stretch to call a block syscon just because you
want regmap.

>
>>
>>> all there ever is. However if it's actually going to end up needing several
>>> more additional MMIO and/or memory regions for other functionality, then
>>> describing each register and location individually is liable to get
>>> unmanageable really fast, and a higher-level functional grouping (e.g. these
>>> reset-related registers together as a single 8-byte region) would likely be
>>> a better design.
>>>
>>> The Exernal system registers are part of a bigger block with other functionality in place.
>>> MFD/syscon might be better way to use these registers. You never know in
>>> future you might want to use another set of 2-4 registers with a different
>>> functionality in another driver.
>>>
>>> I would see if it makes sense to put together a single binding for
>>> this "Host Base System Control" register (not sure what exactly that means).
>>> Use MFD/regmap you access parts of this block. The remoteproc driver can
>>> then be semi-generic (meaning applicable to group of similar platforms)
>>> based on the platform compatible and use this regmap to provide the
>>> functionality needed.
>>
>> I don't understand how this lengthy semi-quote answers my concerns.
>> Please write concise points as arguments to my questions.
>>
>> For example, I don't care what your remote proc driver does and it
>> should not matter in the terms of this binding.
>
> I just wanted to show why we are using syscon based on the arguments
> of other reviewers.
>
>>
>>>
>>> 2/ There are many examples in the kernel that use syscon as a parent node of
>>> child nodes for devices located at an offset from the syscon base address.
>>> Please see these two examples [1][2]. I'm trying to follow a similar design if that
>>> makes sense.
>>
>> Yeah, for separate devices. If you have two words without any resources,
>> I claim you might not have here any separate devices or "not separate
>> enough", because all this is somehow fluid. Remote core sounds like
>> separate device, but all your arguments about need of extid which cannot
>> be used in reg does not support this case.
>>
>> The example in the binding is also not complete - missing rest of
>> devices - which does not help.
>
> Here I would like to explain the current suggestion and suggest an alternative solution.
>
> 1/ For more clarity, here is a complete example of use of both remote cores
> at the same time:
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> #address-cells = <1>;
> #size-cells = <1>;
>
> remoteproc@310 {
> compatible = "arm,sse710-extsys0";
> reg = <0x310 8>;
> firmware-name = "es0_flashfw.elf";
> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
> };
>
> remoteproc@318 {
> compatible = "arm,sse710-extsys1";
> reg = <0x318 8>;
> firmware-name = "es1_flashfw.elf";
> mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
> mbox-names = "txes0", "rxes0";
> memory-region = <&extsys1_vring0>, <&extsys1_vring1>;
> };
> };
>
> Here we have 2 cores, each one have 2 registers mapped respectively
> at 0x1a010310 and 0x1a010318.

All this looks fine, resources are indeed reasonable, except that I
still do not understand why do you need to call them 0 and 1 (now via
compatible).

Your driver code shows this nicely - it is entirely redundant! The 'reg'
- so the base - is already there! You just duplicate it with the
extsys_id, instead of relying on the base. So think what is the point of
'reg' property if you do not use it?

>
> Definetly these cores have seperate HW resources associated with them
> which are the MHUs (mailboxes HW). There are 2 pairs of MHUs associated
> with each core. These mailbox peripherals are obviously seperate.
> Furthermore, the vring buffers used for communication are seperate.
>
> Moreover, the remote cores are independent. They are not SMP cores of one processor.
>
> They can have different default firmware to use. In this example es0_flashfw.elf
> and es1_flashfw.elf
>
> The current HW implementation (Corstone-1000) provides one remote core only.
> However, the second core control registers are at 0x1a010318 do exist.
>
> Support for a second core is taken into consideration in this work to help
> end users who want to add a second core to their HW.
>
> 2/ Here I'm suggesting an alternative solution by using one remoteproc node for both cores as
> follows:
>
> syscon@1a010000 {
> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
> reg = <0x1a010000 0x1000>;
>
> remoteproc {
> compatible = "arm,sse710-extsys";
> firmware-name = "es0_flashfw.elf";
> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>, <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>;
> mbox-names = "txes0", "rxes0", "txes1", "rxes1";
> memory-region = <&extsys0_vring0>, <&extsys0_vring1>, <&extsys1_vring0>, <&extsys1_vring1>;
> };
> };
>
> Does this meet your expectations please ?
>
>>
>>>
>>> 3/ Since there are two registers for each remote core. I'm suggesting to set the
>>> size in the reg property to 8.
>>
>> How is this related?
>>
>>> The driver will read the match data to get the right
>>> offset to be used with regmap APIs.
>>
>> Sorry, no talks about driver. Don't care, at least in this context.
>>
>> You can completely omit address space from children in DT and everything
>> will work fine and look fine from bindings point of view.
>>
>>>
>>> Suggested nodes:
>>>
>>>
>>> syscon@1a010000 {
>>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon";
>>> reg = <0x1a010000 0x1000>;
>>>
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>>> remoteproc@310 {
>>> compatible = "arm,sse710-extsys0";
>>> reg = <0x310 8>;
>>> firmware-name = "es_flashfw.elf";
>>> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>;
>>> mbox-names = "txes0", "rxes0";
>>> memory-region = <&extsys0_vring0>, <&extsys0_vring1>;
>>> };
>>>
>>> remoteproc@318 {
>>> compatible = "arm,sse710-extsys1";
>>> reg = <0x318 8>;
>>> firmware-name = "es_flashfw.elf";
>>
>> Same firmware? Always or only depends?
>
> The firmware of the second core depends on the end user choice.
> Currently the second core is not implemented in the HW.
> Users who want to tweak Corstone-1000 HW can add
> a second core.

Two cores make more sense in such case.

Best regards,
Krzysztof