Re: [PATCH 9/9] arm64: dts: imx95: Describe Mali G310 GPU

Marek Vasut
2025-02-28T18:27:32Z

On 2/28/25 11:36 AM, Alexander Stein wrote:
> Hi Marek,

Hi,

>> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
>> index 3af13173de4bd..36bad211e5558 100644
>> --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
>> @@ -249,6 +249,37 @@ dummy: clock-dummy {
>> clock-output-names = "dummy";
>> };
>>
>> + gpu_fixed_reg: fixed-gpu-reg {
>> + compatible = "regulator-fixed";
>> + regulator-min-microvolt = <920000>;
>> + regulator-max-microvolt = <920000>;
>> + regulator-name = "vdd_gpu";
>> + regulator-always-on;
>> + regulator-boot-on;
>> + };
>
> Is this an internal voltage?

I think so.

>> +
>> + gpu_opp_table: opp_table {
>
> Node-Names use dash instead of underscore.

Fixed, thanks.

[...]

>> @@ -1846,6 +1877,37 @@ netc_emdio: mdio@0,0 {
>> };
>> };
>>
>> + gpu_blk_ctrl: reset-controller@4d810000 {
>> + compatible = "fsl,imx95-gpu-blk-ctrl";
>> + reg = <0x0 0x4d810000 0x0 0xc>;
>
> Mh, GPU_BLK_CTRL is /just a bit) more than the GPU reset. Does it make sense
> to make this an gpu-reset-only node, located at 0x4d810008?

The block controller itself is larger, it spans 3 or 4 registers, so
this should describe the entire block controller here.

>> + #reset-cells = <1>;
>> + clocks = <&scmi_clk IMX95_CLK_GPUAPB>;
>> + assigned-clocks = <&scmi_clk IMX95_CLK_GPUAPB>;
>> + assigned-clock-parents = <&scmi_clk IMX95_CLK_SYSPLL1_PFD1_DIV2>;
>> + assigned-clock-rates = <133333333>;
>> + power-domains = <&scmi_devpd IMX95_PD_GPU>;
>> + status = "disabled";
>> + };
>> +
>> + gpu: gpu@4d900000 {
>> + compatible = "fsl,imx95-mali", "arm,mali-valhall-csf";
>> + reg = <0 0x4d900000 0 0x480000>;
>> + clocks = <&scmi_clk IMX95_CLK_GPU>;
>
> There is also IMX95_CLK_GPUAPB. Is this only required for the rese control above?

I think I have to describe those clock here too, possibly as 'coregroup'
clock ?

>> + clock-names = "core";
>> + interrupts = <GIC_SPI 288 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 289 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "gpu", "job", "mmu";
>
> DT bindings say this order: job, mmu, gpu
Yes, currently it is sorted by IRQ number, fixed.