diff mbox series

[10/12] arm64: dts: rockchip: rk3568-evb: Enable VOP2 and hdmi

Message ID 20211117143347.314294-11-s.hauer@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/rockchip: RK356x VOP2 support | expand

Commit Message

Sascha Hauer Nov. 17, 2021, 2:33 p.m. UTC
This enabled the VOP2 display controller along with hdmi and the
required port routes which is enough to get a picture out of the
hdmi port of the board.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Rob Herring (Arm) Nov. 17, 2021, 3:19 p.m. UTC | #1
On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> This enabled the VOP2 display controller along with hdmi and the
> required port routes which is enough to get a picture out of the
> hdmi port of the board.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> index 184e2aa2416af..156e001492173 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk
>         status = "okay";
>  };
>
> +&hdmi {
> +       status = "okay";
> +       avdd-0v9-supply = <&vdda0v9_image>;
> +       avdd-1v8-supply = <&vcca1v8_image>;
> +};
> +
>  &i2c0 {
>         status = "okay";
>
> @@ -390,3 +396,21 @@ &sdmmc0 {
>  &uart2 {
>         status = "okay";
>  };
> +
> +&vop {
> +       status = "okay";
> +       assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> +       assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> +};
> +
> +&vop_mmu {
> +       status = "okay";
> +};
> +
> +&hdmi_in_vp0 {
> +       status = "okay";
> +};
> +
> +&vp0_out_hdmi {
> +       status = "okay";
> +};

You can accomplish the same thing already with:

&vp0_out_hdmi {
  remote-endpoint = <&hdmi_in_vp0>;
};

or:

&vp0_out_hdmi {
  /delete-property/ remote-endpoint;
};
Michael Riesch Nov. 17, 2021, 3:20 p.m. UTC | #2
Hi Sascha,

On 11/17/21 3:33 PM, Sascha Hauer wrote:
> This enabled the VOP2 display controller along with hdmi and the
> required port routes which is enough to get a picture out of the
> hdmi port of the board.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> index 184e2aa2416af..156e001492173 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk
>   	status = "okay";
>   };
>   
> +&hdmi {
> +	status = "okay";
> +	avdd-0v9-supply = <&vdda0v9_image>;
> +	avdd-1v8-supply = <&vcca1v8_image>;
> +};
> +
>   &i2c0 {
>   	status = "okay";
>   
> @@ -390,3 +396,21 @@ &sdmmc0 {
>   &uart2 {
>   	status = "okay";
>   };
> +
> +&vop {
> +	status = "okay";
> +	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> +	assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> +};
> +
> +&vop_mmu {
> +	status = "okay";
> +};
> +
> +&hdmi_in_vp0 {
> +	status = "okay";
> +};

Minor nitpick: This should probably be sorted alphabetically.

Best regards,
Michael

> +
> +&vp0_out_hdmi {
> +	status = "okay";
> +};
>
Sascha Hauer Dec. 2, 2021, 3:34 p.m. UTC | #3
On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote:
> On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > This enabled the VOP2 display controller along with hdmi and the
> > required port routes which is enough to get a picture out of the
> > hdmi port of the board.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > index 184e2aa2416af..156e001492173 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk
> >         status = "okay";
> >  };
> >
> > +&hdmi {
> > +       status = "okay";
> > +       avdd-0v9-supply = <&vdda0v9_image>;
> > +       avdd-1v8-supply = <&vcca1v8_image>;
> > +};
> > +
> >  &i2c0 {
> >         status = "okay";
> >
> > @@ -390,3 +396,21 @@ &sdmmc0 {
> >  &uart2 {
> >         status = "okay";
> >  };
> > +
> > +&vop {
> > +       status = "okay";
> > +       assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> > +       assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> > +};
> > +
> > +&vop_mmu {
> > +       status = "okay";
> > +};
> > +
> > +&hdmi_in_vp0 {
> > +       status = "okay";
> > +};
> > +
> > +&vp0_out_hdmi {
> > +       status = "okay";
> > +};
> 
> You can accomplish the same thing already with:
> 
> &vp0_out_hdmi {
>   remote-endpoint = <&hdmi_in_vp0>;
> };

My idea was to describe all possible connections in the dtsi file and
let the board dts writer only en/disable the needed connections. When
the connections are specified in the dts file then writing it is more
difficult and error prone.

> 
> or:
> 
> &vp0_out_hdmi {
>   /delete-property/ remote-endpoint;
> };

With this I have to change all connections that I don't need. With
status = "okay" I have to change all connections that I actually do
need, which will be much easier to read and write.

I'll stick to the status = "okay" method for the next round, maybe I can
still convince you ;)

If it's the 'status' property you don't like being used when it's not a
device that is enabled/disabled, then every other name would be fine
with me as well.

Sascha
Heiko Stübner Dec. 2, 2021, 3:41 p.m. UTC | #4
Hi Sascha,

Am Donnerstag, 2. Dezember 2021, 16:34:49 CET schrieb Sascha Hauer:
> On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote:
> > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > This enabled the VOP2 display controller along with hdmi and the
> > > required port routes which is enough to get a picture out of the
> > > hdmi port of the board.
> > >
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > index 184e2aa2416af..156e001492173 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk
> > >         status = "okay";
> > >  };
> > >
> > > +&hdmi {
> > > +       status = "okay";
> > > +       avdd-0v9-supply = <&vdda0v9_image>;
> > > +       avdd-1v8-supply = <&vcca1v8_image>;
> > > +};
> > > +
> > >  &i2c0 {
> > >         status = "okay";
> > >
> > > @@ -390,3 +396,21 @@ &sdmmc0 {
> > >  &uart2 {
> > >         status = "okay";
> > >  };
> > > +
> > > +&vop {
> > > +       status = "okay";
> > > +       assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> > > +       assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> > > +};
> > > +
> > > +&vop_mmu {
> > > +       status = "okay";
> > > +};
> > > +
> > > +&hdmi_in_vp0 {
> > > +       status = "okay";
> > > +};
> > > +
> > > +&vp0_out_hdmi {
> > > +       status = "okay";
> > > +};
> > 
> > You can accomplish the same thing already with:
> > 
> > &vp0_out_hdmi {
> >   remote-endpoint = <&hdmi_in_vp0>;
> > };
> 
> My idea was to describe all possible connections in the dtsi file and
> let the board dts writer only en/disable the needed connections. When
> the connections are specified in the dts file then writing it is more
> difficult and error prone.
> 
> > 
> > or:
> > 
> > &vp0_out_hdmi {
> >   /delete-property/ remote-endpoint;
> > };
> 
> With this I have to change all connections that I don't need. With
> status = "okay" I have to change all connections that I actually do
> need, which will be much easier to read and write.
> 
> I'll stick to the status = "okay" method for the next round, maybe I can
> still convince you ;)
> 
> If it's the 'status' property you don't like being used when it's not a
> device that is enabled/disabled, then every other name would be fine
> with me as well.

hmm, we do have code in the rockchip drm-driver to find out
if the device at the end of a graph-connection is disabled or not [0] ,
So on previous Rockchip socs, there are already all connections
established, and the driver weeds out the disabled ones.

So I'm wondering what is missing to use that in a vop2 context?


Heiko

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/rockchip_drm_drv.c#n274
Sascha Hauer Dec. 2, 2021, 4:09 p.m. UTC | #5
Hi Heiko,

On Thu, Dec 02, 2021 at 04:41:17PM +0100, Heiko Stübner wrote:
> Hi Sascha,
> 
> Am Donnerstag, 2. Dezember 2021, 16:34:49 CET schrieb Sascha Hauer:
> > On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote:
> > > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >
> > > > This enabled the VOP2 display controller along with hdmi and the
> > > > required port routes which is enough to get a picture out of the
> > > > hdmi port of the board.
> > > >
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  .../boot/dts/rockchip/rk3568-evb1-v10.dts     | 24 +++++++++++++++++++
> > > >  1 file changed, 24 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > > index 184e2aa2416af..156e001492173 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
> > > > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk
> > > >         status = "okay";
> > > >  };
> > > >
> > > > +&hdmi {
> > > > +       status = "okay";
> > > > +       avdd-0v9-supply = <&vdda0v9_image>;
> > > > +       avdd-1v8-supply = <&vcca1v8_image>;
> > > > +};
> > > > +
> > > >  &i2c0 {
> > > >         status = "okay";
> > > >
> > > > @@ -390,3 +396,21 @@ &sdmmc0 {
> > > >  &uart2 {
> > > >         status = "okay";
> > > >  };
> > > > +
> > > > +&vop {
> > > > +       status = "okay";
> > > > +       assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> > > > +       assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> > > > +};
> > > > +
> > > > +&vop_mmu {
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&hdmi_in_vp0 {
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&vp0_out_hdmi {
> > > > +       status = "okay";
> > > > +};
> > > 
> > > You can accomplish the same thing already with:
> > > 
> > > &vp0_out_hdmi {
> > >   remote-endpoint = <&hdmi_in_vp0>;
> > > };
> > 
> > My idea was to describe all possible connections in the dtsi file and
> > let the board dts writer only en/disable the needed connections. When
> > the connections are specified in the dts file then writing it is more
> > difficult and error prone.
> > 
> > > 
> > > or:
> > > 
> > > &vp0_out_hdmi {
> > >   /delete-property/ remote-endpoint;
> > > };
> > 
> > With this I have to change all connections that I don't need. With
> > status = "okay" I have to change all connections that I actually do
> > need, which will be much easier to read and write.
> > 
> > I'll stick to the status = "okay" method for the next round, maybe I can
> > still convince you ;)
> > 
> > If it's the 'status' property you don't like being used when it's not a
> > device that is enabled/disabled, then every other name would be fine
> > with me as well.
> 
> hmm, we do have code in the rockchip drm-driver to find out
> if the device at the end of a graph-connection is disabled or not [0] ,
> So on previous Rockchip socs, there are already all connections
> established, and the driver weeds out the disabled ones.
> 
> So I'm wondering what is missing to use that in a vop2 context?

The vop2 has three video ports (crtcs) instead of only one. All three are
described in the device tree and each of them has a of_graph connection
to the different encoders, so something like:

vp0 <-> hdmi
vp0 <-> mipi
vp1 <-> hdmi
vp1 <-> mipi
vp2 <-> hdmi
vp2 <-> mipi

Enabling just vp0 <-> hdmi means only the first video port is can do
hdmi. Different constraints in the clock tree (hdmi reference clock is
hardwired to hpll, not enough PLLs to put all video ports on independent
ones) prevent us from just allowing all connections.

Sascha
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
index 184e2aa2416af..156e001492173 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts
@@ -106,6 +106,12 @@  &gmac1m1_rgmii_clk
 	status = "okay";
 };
 
+&hdmi {
+	status = "okay";
+	avdd-0v9-supply = <&vdda0v9_image>;
+	avdd-1v8-supply = <&vcca1v8_image>;
+};
+
 &i2c0 {
 	status = "okay";
 
@@ -390,3 +396,21 @@  &sdmmc0 {
 &uart2 {
 	status = "okay";
 };
+
+&vop {
+	status = "okay";
+	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
+	assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
+};
+
+&vop_mmu {
+	status = "okay";
+};
+
+&hdmi_in_vp0 {
+	status = "okay";
+};
+
+&vp0_out_hdmi {
+	status = "okay";
+};