diff mbox series

[v6,13/13] arm64: dts: imx8mq: Add node to G2 hardware

Message ID 20210318082046.51546-14-benjamin.gaignard@collabora.com
State Superseded
Headers show
Series Add HANTRO G2/HEVC decoder support for IMX8MQ | expand

Commit Message

Benjamin Gaignard March 18, 2021, 8:20 a.m. UTC
Split VPU node in two: one for G1 and one for G2 since they are
different hardware blocks.
Add syscon for hardware control block.
Remove reg-names property that is useless.
Each VPU node only need one interrupt.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 5:
 - use syscon instead of VPU reset

 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++-----
 1 file changed, 34 insertions(+), 9 deletions(-)

Comments

Philipp Zabel March 26, 2021, 2:24 p.m. UTC | #1
On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:
> Split VPU node in two: one for G1 and one for G2 since they are
> different hardware blocks.
> Add syscon for hardware control block.
> Remove reg-names property that is useless.
> Each VPU node only need one interrupt.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> version 5:
>  - use syscon instead of VPU reset
> 
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 17c449e12c2e..b537d153ebbd 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {
>  			status = "disabled";
>  		};
>  
> -		vpu: video-codec@38300000 {
> +		vpu_ctrl: syscon@38320000 {
> +			compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> +			reg = <0x38320000 0x10000>;
> +		};
> +
> +		vpu_g1: video-codec@38300000 {
>  			compatible = "nxp,imx8mq-vpu";
> -			reg = <0x38300000 0x10000>,
> -			      <0x38310000 0x10000>,
> -			      <0x38320000 0x10000>;
> -			reg-names = "g1", "g2", "ctrl";
> -			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> -			interrupt-names = "g1", "g2";
> +			reg = <0x38300000 0x10000>;
> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "g1";
>  			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
>  				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
>  				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 {
>  						 <&clk IMX8MQ_VPU_PLL_OUT>,
>  						 <&clk IMX8MQ_SYS1_PLL_800M>,
>  						 <&clk IMX8MQ_VPU_PLL>;
> -			assigned-clock-rates = <600000000>, <600000000>,
> +			assigned-clock-rates = <600000000>, <300000000>,

I'd like to see this mentioned in the commit message.

> +					       <800000000>, <0>;
> +			power-domains = <&pgc_vpu>;
> +			nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;
> +		};
> +
> +		vpu_g2: video-codec@38310000 {
> +			compatible = "nxp,imx8mq-vpu-g2";
> +			reg = <0x38310000 0x10000>;
> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "g2";
> +			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> +				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> +				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> +			clock-names = "g1", "g2",  "bus";
> +			assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,

Can the G1 clock configuration be dropped from the G2 device node and
the G2 clock configuration from the G1 device node? It looks weird that
these devices configure each other's clocks.

regards
Philipp
Benjamin Gaignard March 26, 2021, 2:33 p.m. UTC | #2
Le 26/03/2021 à 15:24, Philipp Zabel a écrit :
> On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:

>> Split VPU node in two: one for G1 and one for G2 since they are

>> different hardware blocks.

>> Add syscon for hardware control block.

>> Remove reg-names property that is useless.

>> Each VPU node only need one interrupt.

>>

>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>

>> ---

>> version 5:

>>   - use syscon instead of VPU reset

>>

>>   arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++-----

>>   1 file changed, 34 insertions(+), 9 deletions(-)

>>

>> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi

>> index 17c449e12c2e..b537d153ebbd 100644

>> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi

>> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi

>> @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {

>>   			status = "disabled";

>>   		};

>>   

>> -		vpu: video-codec@38300000 {

>> +		vpu_ctrl: syscon@38320000 {

>> +			compatible = "nxp,imx8mq-vpu-ctrl", "syscon";

>> +			reg = <0x38320000 0x10000>;

>> +		};

>> +

>> +		vpu_g1: video-codec@38300000 {

>>   			compatible = "nxp,imx8mq-vpu";

>> -			reg = <0x38300000 0x10000>,

>> -			      <0x38310000 0x10000>,

>> -			      <0x38320000 0x10000>;

>> -			reg-names = "g1", "g2", "ctrl";

>> -			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,

>> -				     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;

>> -			interrupt-names = "g1", "g2";

>> +			reg = <0x38300000 0x10000>;

>> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;

>> +			interrupt-names = "g1";

>>   			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,

>>   				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,

>>   				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;

>> @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 {

>>   						 <&clk IMX8MQ_VPU_PLL_OUT>,

>>   						 <&clk IMX8MQ_SYS1_PLL_800M>,

>>   						 <&clk IMX8MQ_VPU_PLL>;

>> -			assigned-clock-rates = <600000000>, <600000000>,

>> +			assigned-clock-rates = <600000000>, <300000000>,

> I'd like to see this mentioned in the commit message.


Yes I would do that.
The value comes from the datasheet.

>

>> +					       <800000000>, <0>;

>> +			power-domains = <&pgc_vpu>;

>> +			nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;

>> +		};

>> +

>> +		vpu_g2: video-codec@38310000 {

>> +			compatible = "nxp,imx8mq-vpu-g2";

>> +			reg = <0x38310000 0x10000>;

>> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;

>> +			interrupt-names = "g2";

>> +			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,

>> +				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,

>> +				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;

>> +			clock-names = "g1", "g2",  "bus";

>> +			assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,

> Can the G1 clock configuration be dropped from the G2 device node and

> the G2 clock configuration from the G1 device node? It looks weird that

> these devices configure each other's clocks.


No because if only one device node is enabled we need to configure the both
clocks anyway.

Benjamin

>

> regards

> Philipp

>
Ezequiel Garcia March 26, 2021, 3:28 p.m. UTC | #3
On Fri, 2021-03-26 at 15:33 +0100, Benjamin Gaignard wrote:
> 
> Le 26/03/2021 à 15:24, Philipp Zabel a écrit :
> > On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:
> > > Split VPU node in two: one for G1 and one for G2 since they are
> > > different hardware blocks.
> > > Add syscon for hardware control block.
> > > Remove reg-names property that is useless.
> > > Each VPU node only need one interrupt.
> > > 
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > ---
> > > version 5:
> > >   - use syscon instead of VPU reset
> > > 
> > >   arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++-----
> > >   1 file changed, 34 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > index 17c449e12c2e..b537d153ebbd 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {
> > >                         status = "disabled";
> > >                 };
> > >   
> > > -               vpu: video-codec@38300000 {
> > > +               vpu_ctrl: syscon@38320000 {
> > > +                       compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> > > +                       reg = <0x38320000 0x10000>;
> > > +               };
> > > +
> > > +               vpu_g1: video-codec@38300000 {
> > >                         compatible = "nxp,imx8mq-vpu";
> > > -                       reg = <0x38300000 0x10000>,
> > > -                             <0x38310000 0x10000>,
> > > -                             <0x38320000 0x10000>;
> > > -                       reg-names = "g1", "g2", "ctrl";
> > > -                       interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> > > -                                    <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > > -                       interrupt-names = "g1", "g2";
> > > +                       reg = <0x38300000 0x10000>;
> > > +                       interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > > +                       interrupt-names = "g1";
> > >                         clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> > >                                  <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> > >                                  <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> > > @@ -1350,9 +1351,33 @@ vpu: video-codec@38300000 {
> > >                                                  <&clk IMX8MQ_VPU_PLL_OUT>,
> > >                                                  <&clk IMX8MQ_SYS1_PLL_800M>,
> > >                                                  <&clk IMX8MQ_VPU_PLL>;
> > > -                       assigned-clock-rates = <600000000>, <600000000>,
> > > +                       assigned-clock-rates = <600000000>, <300000000>,
> > I'd like to see this mentioned in the commit message.
> 
> Yes I would do that.
> The value comes from the datasheet.
> 
> > 
> > > +                                              <800000000>, <0>;
> > > +                       power-domains = <&pgc_vpu>;
> > > +                       nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;
> > > +               };
> > > +
> > > +               vpu_g2: video-codec@38310000 {
> > > +                       compatible = "nxp,imx8mq-vpu-g2";
> > > +                       reg = <0x38310000 0x10000>;
> > > +                       interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > > +                       interrupt-names = "g2";
> > > +                       clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> > > +                                <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> > > +                                <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> > > +                       clock-names = "g1", "g2",  "bus";
> > > +                       assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
> > Can the G1 clock configuration be dropped from the G2 device node and
> > the G2 clock configuration from the G1 device node? It looks weird that
> > these devices configure each other's clocks.
> 
> No because if only one device node is enabled we need to configure the both
> clocks anyway.
> 

Since this is akward, how about adding a comment here in the dtsi to clarify it?

Thanks,
Ezequiel
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 17c449e12c2e..b537d153ebbd 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1329,15 +1329,16 @@  usb3_phy1: usb-phy@382f0040 {
 			status = "disabled";
 		};
 
-		vpu: video-codec@38300000 {
+		vpu_ctrl: syscon@38320000 {
+			compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
+			reg = <0x38320000 0x10000>;
+		};
+
+		vpu_g1: video-codec@38300000 {
 			compatible = "nxp,imx8mq-vpu";
-			reg = <0x38300000 0x10000>,
-			      <0x38310000 0x10000>,
-			      <0x38320000 0x10000>;
-			reg-names = "g1", "g2", "ctrl";
-			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "g1", "g2";
+			reg = <0x38300000 0x10000>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g1";
 			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
 				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
 				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
@@ -1350,9 +1351,33 @@  vpu: video-codec@38300000 {
 						 <&clk IMX8MQ_VPU_PLL_OUT>,
 						 <&clk IMX8MQ_SYS1_PLL_800M>,
 						 <&clk IMX8MQ_VPU_PLL>;
-			assigned-clock-rates = <600000000>, <600000000>,
+			assigned-clock-rates = <600000000>, <300000000>,
+					       <800000000>, <0>;
+			power-domains = <&pgc_vpu>;
+			nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;
+		};
+
+		vpu_g2: video-codec@38310000 {
+			compatible = "nxp,imx8mq-vpu-g2";
+			reg = <0x38310000 0x10000>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g2";
+			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
+				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
+				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
+			clock-names = "g1", "g2",  "bus";
+			assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
+					  <&clk IMX8MQ_CLK_VPU_G2>,
+					  <&clk IMX8MQ_CLK_VPU_BUS>,
+					  <&clk IMX8MQ_VPU_PLL_BYPASS>;
+			assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>,
+						 <&clk IMX8MQ_VPU_PLL_OUT>,
+						 <&clk IMX8MQ_SYS1_PLL_800M>,
+						 <&clk IMX8MQ_VPU_PLL>;
+			assigned-clock-rates = <600000000>, <300000000>,
 					       <800000000>, <0>;
 			power-domains = <&pgc_vpu>;
+			nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;
 		};
 
 		pcie0: pcie@33800000 {