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@xxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> .../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:
signature.asc
Description: This is a digitally signed message part.
