Re: [PATCH 3/3] arm64: dts: rockchip: Add rkvdec2 Video Decoder on rk3588(s)

[Detlev Casanova <detlev.casanova@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 6028 bytes --]

On Sunday, June 16, 2024 5:17:31 A.M. EDT Jonas Karlman wrote:
> Hi Detlev,
> 
> On 2024-06-15 21:55, Detlev Casanova wrote:
> > On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
> >> Hi Detlev,
> >> 
> >> On 2024-06-15 03:56, Detlev Casanova wrote:
> >>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> >>> 
> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>> ---
> >>> 
> >>>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
> >>>  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
> >>>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
> >>>  3 files changed, 27 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> >>> c551b676860c..965322c24a65 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> @@ -503,6 +503,10 @@ &pwm1 {
> >>> 
> >>>  	status = "okay";
> >>>  
> >>>  };
> >>> 
> >>> +&rkvdec0 {
> >>> +	status = "okay";
> >>> +};
> >> 
> >> Enable of rkvdec0 should probably be split out from the patch that adds
> >> the rkvdec0 node to soc dtsi.
> > 
> > Ack
> > 
> >> Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
> > 
> > I only could test on those two but I can enable it on all rk3588 devices.
> 
> Because the decoder is an integrated part of the SoC the default should
> probably be that the IP is enabled, i.e. no status prop required for the
> vdec and related mmu nodes in rk3588s.dtsi.
> 
> >>> +
> >>> 
> >>>  &saradc {
> >>>  
> >>>  	vref-supply = <&avcc_1v8_s0>;
> >>>  	status = "okay";
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
> >>> feea6b20a6bf..2828fb4c182a 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
> >>> 
> >>>  	};
> >>>  
> >>>  };
> >>> 
> >>> +&rkvdec0 {
> >>> +	status = "okay";
> >>> +};
> >>> +
> >>> 
> >>>  &saradc {
> >>>  
> >>>  	vref-supply = <&avcc_1v8_s0>;
> >>>  	status = "okay";
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> >>> 0fecbf46e127..09672636dcea 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
> >>> 
> >>>  		ranges = <0x0 0x0 0xff001000 0xef000>;
> >>>  		#address-cells = <1>;
> >>>  		#size-cells = <1>;
> >> 
> >> Blank line is missing.
> >> 
> >>> +		rkvdec0_sram: rkvdec-sram@0 {
> >>> +			reg = <0x0 0x78000>;
> >>> +		};
> >>> 
> >>>  	};
> >>>  	
> >>>  	pinctrl: pinctrl {
> >>> 
> >>> @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
> >>> 
> >>>  			#interrupt-cells = <2>;
> >>>  		
> >>>  		};
> >>>  	
> >>>  	};
> >>> 
> >>> +
> >>> +	rkvdec0: video-decoder@fdc38100 {
> 
> To match prior generations the symbol should probably be called vdec0.
> 
> >>> +		compatible = "rockchip,rk3588-vdec2";
> >>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> >>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>,
> > 
> > <&cru
> > 
> >>> CLK_RKVDEC0_CORE>, +			 <&cru
> > 
> > CLK_RKVDEC0_CA>, <&cru
> > 
> >>> CLK_RKVDEC0_HEVC_CA>;
> >>> +		clock-names = "axi", "ahb", "core",
> >>> +			      "cabac", "hevc_cabac";
> >>> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru
> > 
> > CLK_RKVDEC0_CORE>,
> > 
> >>> +				  <&cru CLK_RKVDEC0_CA>, <&cru
> > 
> > CLK_RKVDEC0_HEVC_CA>;
> > 
> >>> +		assigned-clock-rates = <800000000>, <600000000>,
> >>> +				       <600000000>, <1000000000>;
> >>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> >> 
> >> iommus and resets should probably be added.
> >> 
> >>> +		status = "disabled";
> >>> +	};
> >> 
> >> The iommu node for rkvdec0_mmu seem to be missing, is it not required to
> >> be able to use memory >4GiB as decoding buffers?
> > 
> > I need to check if the current rockchip iommu driver will work for this
> > decoder. I remember that the iommu code for AV1 was a bit different, not
> > sure about this rkvdec.
> 
> The device tree should describe the HW not what drivers are capable of.
> 
> If there are substantial differences in iommu IP a new compatible should
> probably be added for that iommu.
> 
> >> I would also consider adding the rkvdec1 node(s), if I am understanding
> >> correctly they can both be used in a cluster or completely independent.
> > 
> > They can be used independently, yes. I'll add rkvdec1 for rk3588 devices
> > (rk3588s only has  1 core)
> 
> I do not think that is true, the rk3588s variant should also include two
> decoder and two encoder cores.

You are right, I had HDMI ports in mind. There are 2 decoders available on 
rk3588s too.

> However, the rk3582/rk3583 variants (rk3588s with one or more bad cores)
> may have 0-2 cores working for the decoder and/or encoder.
> 
> E.g on my rk3582 boards I have following different ip-state in otp:
> - 1 bad cpu core (ip-state: 10 00 00)
> - 1 bad decoder core (ip-state: 00 80 00)
> - 1 bad encoder core (ip-state: 00 00 04)
> 
> The general idea is that bootloader will disable or delete the offending
> nodes in the device tree to correctly describe the HW for the OS.

I see, so I will add both cores, enabled, in rk3588s.dtsi and let the 
bootloader disable the ones that are bad.

Should I also add compatibles for rk3582/rk3583 then ?

> Regards,
> Jonas
> 
> > Regards,
> > Detlev.
> > 
> >> Also on RK3582/RK3583 one (or both) of the decoder cores may be marked
> >> as bad, yet the remaining one can still be used independently. The idea
> >> will be that bootloader fixup the DT and disabled/delete-node the bad
> >> core(s).
> >> 
> >> Regards,
> >> Jonas
> >> 
> >>>  };
> >>>  
> >>>  #include "rk3588s-pinctrl.dtsi"


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]