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

[Paul Cercueil <paul@crapouillou.net>]

Le dim. 3 mai 2020 à 15:31, H. Nikolaus Schaller <hns@goldelico.com> a 
écrit :
> Hi Paul,
> 
>>  Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>>>  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?

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.

> 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, 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?

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. But the moment you discover 
even a tiny thing that needs to be handled differently, you wouldn't 
have the possibility to do so.

>                    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