Re: [PATCH 2/8] arm64: dts: allwinner: h616: add display engine, bus and mixer nodes

[Andre Przywara <andre.przywara@arm.com>]

On Sun, 16 Feb 2025 22:27:09 +1300
Ryan Walklin <ryan@testtoast.com> wrote:

Hi,

> From: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> The Allwinner H616 and variants (H618, H700 and T507) have a new display
> engine variant (DE33). Support has been added to the existing DE2/DE3
> sun4i driver in a previous patch series (x). The variant is selected via
> the appropriate mixer device tree compatible string.
> 
> Add the respective device-tree nodes for the DE, bus, clock and mixer to
> the H616 DTSI, and the matching SRAM section for the DE.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  .../arm64/boot/dts/allwinner/sun50i-h616.dtsi | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> index cdce3dcb8ec02..ab8b70ce7df89 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> @@ -94,6 +94,12 @@ l2_cache: l2-cache {
>  		};
>  	};
>  
> +	de: display-engine {
> +		compatible = "allwinner,sun50i-h6-display-engine";

That should either be sun50i-h616-display-engine, or it should use a
fallback. IIUC this "device" is just something more or less artificial
that ties things together? I don't see any differences between the
latest SoCs in the driver, but still we seem to use a separate compatible
for every SoC there, which I guess is intentional?

> +		allwinner,pipelines = <&mixer0>;
> +		status = "disabled";
> +	};
> +
>  	reserved-memory {
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> @@ -150,6 +156,51 @@ soc {
>  		#size-cells = <1>;
>  		ranges = <0x0 0x0 0x0 0x40000000>;
>  
> +		bus: bus@1000000 {
> +			compatible = "allwinner,sun50i-h616-de33",
> +				     "allwinner,sun50i-a64-de2";
> +			reg = <0x1000000 0x400000>;
> +			allwinner,sram = <&de3_sram 1>;

Should this label be de33_sram?

> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0 0x1000000 0x400000>;
> +
> +			display_clocks: clock@8000 {
> +				compatible = "allwinner,sun50i-h616-de33-clk";
> +				reg = <0x8000 0x100>;
> +				clocks = <&ccu CLK_DE>, <&ccu CLK_BUS_DE>;
> +				clock-names = "mod", "bus";
> +				resets = <&ccu RST_BUS_DE>;
> +				#clock-cells = <1>;
> +				#reset-cells = <1>;
> +			};
> +
> +			mixer0: mixer@100000 {
> +				compatible = "allwinner,sun50i-h616-de33-mixer-0";
> +				reg = <0x100000 0x100000>,
> +				      <0x8100 0x40>,
> +				      <0x280000 0x20000>;

As mentioned in the binding patch, I think having reg-names here would
help to make it clearer what those regions are for.

> +				clocks = <&display_clocks CLK_BUS_MIXER0>,
> +					 <&display_clocks CLK_MIXER0>;
> +				clock-names = "bus", "mod";
> +				resets = <&display_clocks RST_MIXER0>;
> +				iommus = <&iommu 0>;
> +
> +				ports {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					mixer0_out: port@1 {
> +						reg = <1>;
> +
> +						mixer0_out_tcon_top_mixer0: endpoint {
> +							remote-endpoint = <&tcon_top_mixer0_in_mixer0>;
> +						};
> +					};
> +				};
> +			};
> +		};
> +
>  		crypto: crypto@1904000 {
>  			compatible = "allwinner,sun50i-h616-crypto";
>  			reg = <0x01904000 0x800>;
> @@ -173,6 +224,11 @@ sram_c: sram@28000 {
>  				#address-cells = <1>;
>  				#size-cells = <1>;
>  				ranges = <0 0x00028000 0x30000>;
> +
> +				de3_sram: sram-section@0 {

de33_sram?

> +					compatible = "allwinner,sun50i-a64-sram-c";

I think we need a new compatible, with the A64 as a fallback. The H6 seems
to do this as well.

Cheers,
Andre

> +					reg = <0x0000 0x1e000>;
> +				};
>  			};
>  		};
>