Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Paul and Paul,

> Am 03.05.2020 um 16:18 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
> 
> 
> 
> Le dim. 3 mai 2020 à 15:31, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit :
>> Hi Paul,
>>> Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
>>>>> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.
>>>> To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.
>>>> This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.
>>> As we said before, the number of clocks is a property of the GPU and *not* its integration into the SoC.
>> Well, it is a not very well documented property of the GPU. We have no data sheet of the standalone GPU. Only several SoC data sheets which give some indications.
> 
> Maybe we can nicely ask them?

There is some (old) answer here:

https://github.com/MIPS/CI20_linux/blob/ci20-v3.18/arch/mips/boot/dts/jz4780.dtsi#L63

> I expect Paul Burton to have some contacts at ImgTec. Asking for a doc would be too much, but maybe they can help a bit with the DT bindings.

Good idea! It is definitively worth to try. Therefore I have moved him from CC: to To:

> 
>> It appears as if some sgx5xx versions have 3 clocks and some have 4. So you are right, the number of clocks depends on the sgx5xx version and that could be made dependent in the bindings (if necessary).
>>> So you would *not* have a number of clocks between 0 and 4. You get either 0, or 4, depending on whether or not you have a wrapper.
>> I think this is contradicting your previous sentence. If the number of clocks is a property of the GPU and not the integration it must also not depend on whether there is a wrapper. I.e. it must be a constant for any type of integration.
> 
> Well, I expected all SGX versions to have 4 clocks.
> 
> If some SGX versions have 3 clocks, and others have 4 clocks, it's still OK as long as the number of clocks is enforced, so that all implementations of a given SGX core will have to use the same number of clocks.

> 
>> The really correct variant would be to always make the SoC integration (wrapper) a separate subsystem (because it is never part of the SGX core but some interface bus) and clock provider and connect it explicitly to the clock inputs.
> 
> About the wrapper... I don't really know how it's done there. But you could very well pass all clocks unconditionally to the SGX node, even if it's inside a wrapper.
> The wrapper itself probably needs only one clock, the one that allows it to access its registers.
> 
>> To be clear: I am not at all against describing the clocks. I just doubt that the time we invest into discussing on this level of detail and adding conditional clock requirements is worth the result. IMHO the bindings and .dts do not become better by describing them in more detail than just "optional". It just takes our time from contributing to other subsystems.
> 
> You have a new SoC with a SGX, and you only need to enable one clock to get it to work. So you create a devicetree node which receives only one clock.
> 
> Turns out, that the bootloader was enabling the other 3 clocks,

Does it? I haven't seen such boot loaders. Usually they bring up only the core and e.g. mmc to be able to boot.

> and since the last release, it doesn't anymore. You're left with having to support a broken devicetree.

> 
> That's the kind of problem that can be easily avoided by enforcing the number of clocks that have to be provided.
>>>>> See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.
>>>> Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.
>>>> Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.
>>>> Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.
>>> If you add support for a new SoC, you'd still need to modify the binding to add the compatible string. So the argument of "more work" is moot.
>> Agreed, I forgot this aspect. Nevertheless, it is easier to review a new compatible string than a new clock number rule (question: how do you practically review this? By looking if it does match the DTS?).
>> We have to add the compatible string as long as we need to have the SoC name in the compatible string (which as said is my dream to get rid of in far future). If we could get rid of it, there won't be a change any more. By just taking "img,sgx544" into a new SoC. The change would be moved into SoC specific wrappers. In such an ideal world, we would explicitly describe the wrappers as separate DT nodes. Even if they have no explicit driver (e.g. by some simple-pm-bus).
> 
> What's wrong with having the SoC name in the compatible string?

I'd say it should be avoided if possible. But you give a good hint and a little research shows some examples having the SoC name in the compatible string: musb, dwc2, dwc3.

> 
> You cannot use just a "img,sgx544" compatible string, as then you would assume that the same SGX version in (e.g.) an Ingenic or a Omap SoC is the exact same. This may actually be true.

Yes. That is the assumption and I have not seen any hints for the opposite in the pvrsrvkm sources. They only differentiate the SoC integration (clocks, reset) but not in the SGX operation (memory mapping, communication with firmware) itself. So the differences could easily be factored out into a wrapper driver.

> But the moment you discover even a tiny thing that needs to be handled differently, you wouldn't have the possibility to do so.

You would still have the possibility. An SGX driver can instead of differentiating by its own compatible string table look for the wrapper or SoC compatible string to find out where the sgx is integrated to. It is just simpler to do if we have the combined soc+sgx versions. And at the moment we even compile separate kernel modules from the same source.

> 
>>                   PRCM,bus,
>> Processor <<---->> Wrapper <<----->> SGX
>> ti,...             ti,sysc           img,sgx530
>> img,...            simple-bus        img,sgx540
>> samsung,...        ...               img,sgx544
>> other,             other,gpu-wrapper img,...
>> But this IMHO correct proposal was already rejected.
>> So at the moment we are circling around several proposals because none can fulfill all requirements.
>> Therefore my attempt to solve the gordian knot is to make clocks generally optional. This keeps the bindings simple but not generally wrong. And since the DTS are not only tested against bindings.yaml but on real hardware, the omission to enforce a specific number of clocks doesn't harm anyone. As said it is impossible to get the SGX running without defining the correct clocks (whether they are enforced by bindings.yaml or not).
> 
> That's what I tried to explain above. You'd be able to get the SGX to work without a single clock in devicetree. That doesn't mean it should be allowed.
> 
> Cheers,
> -Paul

BR and thanks,
Nikolaus