Re: [PATCH v4 16/17] arm64: dts: rockchip: Add vop on rk3588

[Andy Yan]

Hi Heiko:

On 12/9/23 00:26, Heiko Stübner wrote:
> Hi Andy,
>
> Am Donnerstag, 7. Dezember 2023, 09:02:47 CET schrieb Andy Yan:
> > From: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
> >
> > Add vop dt node for rk3588.
> >
> > Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
> > ---
> >
> > (no changes since v1)
> >
> >   arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 96 +++++++++++++++++++++++
> >   1 file changed, 96 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > index 7064c0e9179f..a9810ca78dc4 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -593,6 +608,87 @@ i2c0: i2c@fd880000 {
> >   		status = "disabled";
> >   	};
> >   
> > +	vop: vop@fdd90000 {
> > +		compatible = "rockchip,rk3588-vop";
> > +		reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
> > +		reg-names = "vop", "gamma_lut";
> > +		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
> > +		clocks = <&cru ACLK_VOP>,
> > +			 <&cru HCLK_VOP>,
> > +			 <&cru DCLK_VOP0>,
> > +			 <&cru DCLK_VOP1>,
> > +			 <&cru DCLK_VOP2>,
> > +			 <&cru DCLK_VOP3>,
> > +			 <&cru PCLK_VOP_ROOT>;
> > +		clock-names = "aclk",
> > +			      "hclk",
> > +			      "dclk_vp0",
> > +			      "dclk_vp1",
> > +			      "dclk_vp2",
> > +			      "dclk_vp3",
> > +			      "pclk_vop";
> > +		resets = <&cru SRST_A_VOP>,
> > +			 <&cru SRST_H_VOP>,
> > +			 <&cru SRST_D_VOP0>,
> > +			 <&cru SRST_D_VOP1>,
> > +			 <&cru SRST_D_VOP2>,
> > +			 <&cru SRST_D_VOP3>;
> > +		reset-names = "axi",
> > +			      "ahb",
> > +			      "dclk_vp0",
> > +			      "dclk_vp1",
> > +			      "dclk_vp2",
> > +			      "dclk_vp3";
>
> resets and reset-names do not seem to be part of the binding, so
> should probably be added there.

I will drop resets and reset-names in next version.
> > +		iommus = <&vop_mmu>;
> > +		power-domains = <&power RK3588_PD_VOP>;
> > +		rockchip,grf = <&sys_grf>;
> > +		rockchip,vop-grf = <&vop_grf>;
> > +		rockchip,vo1-grf = <&vo1_grf>;
> > +		rockchip,pmu = <&pmu>;
> > +
>
> move this blank line _below_ the status=disabled please.

ok, will do.
> > +		status = "disabled";
> > +		vop_out: ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			vp0: port@0 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <0>;
> > +			};
> > +
> > +			vp1: port@1 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <1>;
> > +			};
> > +
> > +			vp2: port@2 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <2>;
> > +			};
> > +
> > +			vp3: port@3 {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <3>;
> > +			};
> > +		};
> > +	};
> > +
> > +	vop_mmu: iommu@fdd97e00 {
> > +		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> > +		reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
> > +		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
> > +		interrupt-names = "vop_mmu";
>
> interrupt-names is not part of the mainline iommu binding, so should be dropped

will do.
> Thanks
> Heiko