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

From: Philipp Rossak
Date: Sat Apr 18 2020 - 19:03:32 EST


Hi Nikolaus,
Hi Maxime,

TI SoC seem to be the broadest number of available users
of sgx5xx in the past and nowadays. Others are more the exception.

And maybe TI has some complicated stuff around the GPU that others don't have?

Looks so.

I can only agree on this.



What I also assume is that developers of DTS know what they do.
So the risk that there is different semantics is IMHO very low.

Well, they know what they do if you document the binding. Let's say I have two
clocks now on my SoC, and you just document that you want a clocks property,
with a generic name in clock-names like "gpu".

Yes, that is what I want to propose for v7:

clocks:
maxItems: 1

clock-names:
maxItems: 1
items:
- const: gpu


If you agree I can add the clocks/clock-names property as an
optional property. This should solve omap and all others.

With the above example, what clock should I put in there? In which order? This
isn't some random example pulled out of nowhere. The Allwinner A31 has (at
least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
that the GPU actually needs at least that amount to be properly integrated into
an SoC.

Ah, now I understand your motivation: you have access and experience with
the A31 and you know that our proposal doesn't fit to it.

From what I know from your description is that the A31 is quite special with
4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
logic (like on OMAP which separates between "functional clocks" and "interface
clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?

So what would be your proposal for the A31 DT?

Then I get a chance to compare DT snippets and try to make a mixture for
the bindings.


This is my current binding for the A83T, the A31 looks similar but there is still something missing, since some parts are not mainlined yet:

sun8i-a83t.dtsi:
gpu: gpu@1c400000 {
compatible = "allwinner,sun8i-a83t-sgx544-115",
"img,sgx544-115", "img,sgx544";
reg = <0x01c40000 0x10000>;
interrupts = <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&ccu CLK_BUS_GPU>, <&ccu CLK_GPU_CORE>,
<&ccu CLK_GPU_MEMORY>, <&ccu CLK_GPU_HYD>;
clock-names = "bus", "core", "memory", "hyd";

resets = <&ccu RST_BUS_GPU>;
};

sun8i-a83t-bananapi-m3.dts:
&gpu {
gpu-supply = <&reg_dcdc4>;
};



But given that the current state on the Allwinner SoCs (at least) is that you
can't even read a register, it might be a good idea to delay the introduction of
that binding until you have something that works to avoid drowning under the
number of special cases to deal with backward compatibility.


Maxime is right. Even if you enable the regulator, write 0x0 to the GPU Power Off Gating Register, deassert the reset and enable the clocks you are not able to read the register.
You must first run: pvrsrvctl --no-module --start (user space binaries) to access registers otherwise the system will stuck with the following message when accessing them:

./devmem2 0x01C40024
/dev/mem opened.

Philipp has something minimal working on the A83 which works with the proposed
binding and enabled him to read out the sgx revision register.

This is not correct. In the other mail I talked about my reference system. This is an old 3.4.39 kernel, modified by allwinner to run on their SOC's which don't use the common kernel techniques. So it's very hacky, but they got the gpu running. I'm using this system for comparison, to read out registers and for reverse engineering.

My current kernel module behaves similar like the reference design, but right now I'm not able to run "pvrsrvctl --no-module --start" without errors. So the initialization never get's finalized and I see the issue described above.

So if you are a specialist for the A31 SGX, please make a proposal for a binding
that covers all the A31 needs and all other SoC we know. Or define a separate
bindings for the A31.

The A31 and the A31s have some additional clocks mentioned in their datasheet (@ System Control Register/SRAM Controler). Those are not available in the A83T datasheet, but might be there since the memory map looks similar and allwinner might use the same userspace binaries for their devices.

From the knowledge I gained the last 3 days, we should delay the patches for the A83T, A31 and A31s.
The idea of the placeholder patches was to show that from this binding also other devices could benefit.

Cheers,
Philipp