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