Hi Johan,

Thanks for the review.

On Fri, 2021-07-30 at 21:38 +0200, Johan Jonker wrote:

Hi Ezequiel,

Some comments. Have a look if it's useful.

On 7/30/21 6:45 PM, Ezequiel Garcia wrote:

quoted

Rockchip SoCs RK3566 and RK3568 have a Mali Gondul core
which is based on the Bifrost architecture. It has
one shader core and two execution engines.

Quoting the datasheet:

Mali-G52 1-Core-2EE
* Support 1600Mpix/s fill rate when 800MHz clock frequency
* Support 38.4GLOPs when 800MHz clock frequency

Signed-off-by: Ezequiel Garcia <redacted>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 47 ++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index bef747fb1fe2..f4f400792659 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi

@@ -121,6 +121,35 @@ opp-1800000000 {

                };
        };
 

quoted

+       gpu_opp_table: opp-table2 {

        gpu_opp_table: gpu-opp-table {

Makes sense.

quoted

+               compatible = "operating-points-v2";
+
+               opp-200000000 {
+                       opp-hz = /bits/ 64 <200000000>;
+                       opp-microvolt = <825000>;
+               };

Similar to cpu0_opp_table keep the same style and add an empty line
between nodes.

Makes sense.

quoted

+               opp-300000000 {
+                       opp-hz = /bits/ 64 <300000000>;
+                       opp-microvolt = <825000>;
+               };
+               opp-400000000 {
+                       opp-hz = /bits/ 64 <400000000>;
+                       opp-microvolt = <825000>;
+               };
+               opp-600000000 {
+                       opp-hz = /bits/ 64 <600000000>;
+                       opp-microvolt = <825000>;
+               };
+               opp-700000000 {
+                       opp-hz = /bits/ 64 <700000000>;
+                       opp-microvolt = <900000>;
+               };
+               opp-800000000 {
+                       opp-hz = /bits/ 64 <800000000>;
+                       opp-microvolt = <1000000>;
+               };
+       };
+
        firmware {
                scmi: scmi {
                        compatible = "arm,scmi-smc";

@@ -332,6 +361,24 @@ power-domain@RK3568_PD_RKVENC {

                };
        };
 
+       gpu: gpu@fde60000 {
+               compatible = "rockchip,rk3568-mali", "arm,mali-bifrost";
+               reg = <0x0 0xfde60000 0x0 0x4000>;

quoted

+

remove empty lines

Makes sense.

quoted

+               interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
+                            <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+                            <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+               interrupt-names = "job", "mmu", "gpu";

quoted

+

dito

quoted

+               clocks = <&scmi_clk 1>, <&cru CLK_GPU>;
+               clock-names = "core", "bus";

Not sure if it's possible, but could you keep them all a little bit in
the same order/style as arm,mali-400?

From arm,mali-utgard.yaml:

  clock-names:
    items:
      - const: bus
      - const: core

Don't think we can do that in this case, as the first clock
is already expected as the "core" one.

These clocks might need some cleaner specification in the
driver or the dt-binding (or both), but I'd like to defer
that to the driver maintainers.


-- 
Kindly,
Ezequiel