[PATCH 1/3] arm64: dts: ti: am654: Add DSS node

[PATCH 1/3] arm64: dts: ti: am654: Add DSS node

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Add DSS node to k3-am65-main.dtsi with labels for board specific
support and syscon node for oldi-io-ctrl.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 44 ++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index 11887c72f23a..7d1bc991708e 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -287,6 +287,11 @@
 			mux-reg-masks = <0x4080 0x3>, /* SERDES0 lane select */
 					<0x4090 0x3>; /* SERDES1 lane select */
 		};
+
+		dss_oldi_io_ctrl: dss_oldi_io_ctrl@41E0 {
+			compatible = "syscon";
+			reg = <0x0000041E0 0x14>;
+		};
 	};
 
 	dwc3_0: dwc3@4000000 {
@@ -746,4 +751,43 @@
 			};
 		};
 	};
+
+	dss: dss@04a00000 {
+		compatible = "ti,am65x-dss";
+		reg =	<0x0 0x04a00000 0x0 0x1000>, /* common */
+			<0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
+			<0x0 0x04a06000 0x0 0x1000>, /* vid */
+			<0x0 0x04a07000 0x0 0x1000>, /* ovr1 */
+			<0x0 0x04a08000 0x0 0x1000>, /* ovr2 */
+			<0x0 0x04a0a000 0x0 0x1000>, /* vp1 */
+			<0x0 0x04a0b000 0x0 0x1000>; /* vp2 */
+		reg-names = "common", "vidl1", "vid",
+			"ovr1", "ovr2", "vp1", "vp2";
+
+		ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
+
+		power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
+
+		clocks =	<&k3_clks 67 1>,
+				<&k3_clks 216 1>,
+				<&k3_clks 67 2>;
+		clock-names = "fck", "vp1", "vp2";
+
+		/*
+		 * Set vp2 clk (DPI_1_IN_CLK) mux to PLL4 via
+		 * DIV1. See "Figure 12-3365. DSS Integration"
+		 * in AM65x TRM for details.
+		 */
+		assigned-clocks = <&k3_clks 67 2>;
+		assigned-clock-parents = <&k3_clks 67 5>;
+
+		interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
+
+		status = "disabled";
+
+		dss_ports: ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
 };
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tomi Valkeinen]

Add DSS node for J721E SoC.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 57 +++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index 0b9d14b838a1..21c362042ecf 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -736,6 +736,63 @@
 		};
 	};
 
+	dss: dss@04a00000 {
+		compatible = "ti,j721e-dss";
+		reg =
+			<0x00 0x04a00000 0x00 0x10000>, /* common_m */
+			<0x00 0x04a10000 0x00 0x10000>, /* common_s0*/
+			<0x00 0x04b00000 0x00 0x10000>, /* common_s1*/
+			<0x00 0x04b10000 0x00 0x10000>, /* common_s2*/
+
+			<0x00 0x04a20000 0x00 0x10000>, /* vidl1 */
+			<0x00 0x04a30000 0x00 0x10000>, /* vidl2 */
+			<0x00 0x04a50000 0x00 0x10000>, /* vid1 */
+			<0x00 0x04a60000 0x00 0x10000>, /* vid2 */
+
+			<0x00 0x04a70000 0x00 0x10000>, /* ovr1 */
+			<0x00 0x04a90000 0x00 0x10000>, /* ovr2 */
+			<0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */
+			<0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */
+
+			<0x00 0x04a80000 0x00 0x10000>, /* vp1 */
+			<0x00 0x04aa0000 0x00 0x10000>, /* vp2 */
+			<0x00 0x04ac0000 0x00 0x10000>, /* vp3 */
+			<0x00 0x04ae0000 0x00 0x10000>, /* vp4 */
+			<0x00 0x04af0000 0x00 0x10000>; /* wb */
+
+		reg-names = "common_m", "common_s0",
+			"common_s1", "common_s2",
+			"vidl1", "vidl2","vid1","vid2",
+			"ovr1", "ovr2", "ovr3", "ovr4",
+			"vp1", "vp2", "vp3", "vp4",
+			"wb";
+
+		clocks =	<&k3_clks 152 0>,
+				<&k3_clks 152 1>,
+				<&k3_clks 152 4>,
+				<&k3_clks 152 9>,
+				<&k3_clks 152 13>;
+		clock-names = "fck", "vp1", "vp2", "vp3", "vp4";
+
+		power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>;
+
+		interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "common_m",
+				  "common_s0",
+				  "common_s1",
+				  "common_s2";
+
+		status = "disabled";
+
+		dss_ports: ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
 	mcasp0: mcasp@2b00000 {
 		compatible = "ti,am33xx-mcasp-audio";
 		reg = <0x0 0x02b00000 0x0 0x2000>,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/3] arm64: dts: ti: k3-j721e-common-proc-board: add assigned clks for DSS

[Tomi Valkeinen]

The DSS related clock muxes are set via assigned-clocks in a way which
provides us:

VP0 - DisplayPort SST
VP1 - DPI0
VP2 - DSI
VP3 - DPI1

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../dts/ti/k3-j721e-common-proc-board.dts     | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
index 98e5e17e3ff7..6df823aaa37c 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
@@ -472,3 +472,23 @@
 	phy-mode = "rgmii-rxid";
 	phy-handle = <&phy0>;
 };
+
+&dss {
+	/*
+	 * These clock assignments are chosen to enable the following outputs:
+	 *
+	 * VP0 - DisplayPort SST
+	 * VP1 - DPI0
+	 * VP2 - DSI
+	 * VP3 - DPI1
+	 */
+
+	assigned-clocks = <&k3_clks 152 1>,
+			  <&k3_clks 152 4>,
+			  <&k3_clks 152 9>,
+			  <&k3_clks 152 13>;
+	assigned-clock-parents = <&k3_clks 152 2>,	/* PLL16_HSDIV0 */
+				 <&k3_clks 152 6>,	/* PLL19_HSDIV0 */
+				 <&k3_clks 152 11>,	/* PLL18_HSDIV0 */
+				 <&k3_clks 152 18>;	/* PLL23_HSDIV0 */
+};
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 1/3] arm64: dts: ti: am654: Add DSS node

[Tero Kristo]

On 22/04/2020 12:15, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Add DSS node to k3-am65-main.dtsi with labels for board specific
> support and syscon node for oldi-io-ctrl.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 44 ++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index 11887c72f23a..7d1bc991708e 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -287,6 +287,11 @@
>   			mux-reg-masks = <0x4080 0x3>, /* SERDES0 lane select */
>   					<0x4090 0x3>; /* SERDES1 lane select */
>   		};
> +
> +		dss_oldi_io_ctrl: dss_oldi_io_ctrl@41E0 {
> +			compatible = "syscon";
> +			reg = <0x0000041E0 0x14>;
> +		};
>   	};
>   
>   	dwc3_0: dwc3@4000000 {
> @@ -746,4 +751,43 @@
>   			};
>   		};
>   	};
> +
> +	dss: dss@04a00000 {
> +		compatible = "ti,am65x-dss";
> +		reg =	<0x0 0x04a00000 0x0 0x1000>, /* common */
> +			<0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
> +			<0x0 0x04a06000 0x0 0x1000>, /* vid */
> +			<0x0 0x04a07000 0x0 0x1000>, /* ovr1 */
> +			<0x0 0x04a08000 0x0 0x1000>, /* ovr2 */
> +			<0x0 0x04a0a000 0x0 0x1000>, /* vp1 */
> +			<0x0 0x04a0b000 0x0 0x1000>; /* vp2 */
> +		reg-names = "common", "vidl1", "vid",
> +			"ovr1", "ovr2", "vp1", "vp2";
> +
> +		ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
> +
> +		power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
> +
> +		clocks =	<&k3_clks 67 1>,
> +				<&k3_clks 216 1>,
> +				<&k3_clks 67 2>;
> +		clock-names = "fck", "vp1", "vp2";
> +
> +		/*
> +		 * Set vp2 clk (DPI_1_IN_CLK) mux to PLL4 via
> +		 * DIV1. See "Figure 12-3365. DSS Integration"
> +		 * in AM65x TRM for details.
> +		 */
> +		assigned-clocks = <&k3_clks 67 2>;
> +		assigned-clock-parents = <&k3_clks 67 5>;
> +
> +		interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
> +
> +		status = "disabled";

Any reason why the node is disabled? Are you planning to enable it 
somewhere later on, or is that left for the user to do?

-Tero

> +
> +		dss_ports: ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};
>   };
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tero Kristo]

On 22/04/2020 12:15, Tomi Valkeinen wrote:
> Add DSS node for J721E SoC.

Subject should drop .dtsi, I can fix that locally though. Got a question 
below.

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 57 +++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index 0b9d14b838a1..21c362042ecf 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -736,6 +736,63 @@
>   		};
>   	};
>   
> +	dss: dss@04a00000 {
> +		compatible = "ti,j721e-dss";
> +		reg =
> +			<0x00 0x04a00000 0x00 0x10000>, /* common_m */
> +			<0x00 0x04a10000 0x00 0x10000>, /* common_s0*/
> +			<0x00 0x04b00000 0x00 0x10000>, /* common_s1*/
> +			<0x00 0x04b10000 0x00 0x10000>, /* common_s2*/
> +
> +			<0x00 0x04a20000 0x00 0x10000>, /* vidl1 */
> +			<0x00 0x04a30000 0x00 0x10000>, /* vidl2 */
> +			<0x00 0x04a50000 0x00 0x10000>, /* vid1 */
> +			<0x00 0x04a60000 0x00 0x10000>, /* vid2 */
> +
> +			<0x00 0x04a70000 0x00 0x10000>, /* ovr1 */
> +			<0x00 0x04a90000 0x00 0x10000>, /* ovr2 */
> +			<0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */
> +			<0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */
> +
> +			<0x00 0x04a80000 0x00 0x10000>, /* vp1 */
> +			<0x00 0x04aa0000 0x00 0x10000>, /* vp2 */
> +			<0x00 0x04ac0000 0x00 0x10000>, /* vp3 */
> +			<0x00 0x04ae0000 0x00 0x10000>, /* vp4 */
> +			<0x00 0x04af0000 0x00 0x10000>; /* wb */
> +
> +		reg-names = "common_m", "common_s0",
> +			"common_s1", "common_s2",
> +			"vidl1", "vidl2","vid1","vid2",
> +			"ovr1", "ovr2", "ovr3", "ovr4",
> +			"vp1", "vp2", "vp3", "vp4",
> +			"wb";
> +
> +		clocks =	<&k3_clks 152 0>,
> +				<&k3_clks 152 1>,
> +				<&k3_clks 152 4>,
> +				<&k3_clks 152 9>,
> +				<&k3_clks 152 13>;
> +		clock-names = "fck", "vp1", "vp2", "vp3", "vp4";
> +
> +		power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>;
> +
> +		interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "common_m",
> +				  "common_s0",
> +				  "common_s1",
> +				  "common_s2";
> +
> +		status = "disabled";

Again, why disabled by default?

-Tero

> +
> +		dss_ports: ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};
> +
>   	mcasp0: mcasp@2b00000 {
>   		compatible = "ti,am33xx-mcasp-audio";
>   		reg = <0x0 0x02b00000 0x0 0x2000>,
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Jyri Sarha]

On 27/04/2020 13:09, Tero Kristo wrote:
>> +        status = "disabled";
> 
> Again, why disabled by default?
> 

tidss device is not functional without a defined video-port. The driver
is not implemented in a way that it would handle a broken configuration
gracefully.

Best regards,
Jyri



-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tero Kristo]

On 27/04/2020 13:37, Jyri Sarha wrote:
> On 27/04/2020 13:09, Tero Kristo wrote:
>>> +        status = "disabled";
>>
>> Again, why disabled by default?
>>
> 
> tidss device is not functional without a defined video-port. The driver
> is not implemented in a way that it would handle a broken configuration
> gracefully.

What/where/when is the video-port going to be defined then? Is this 
going to be done in an overlay?

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Jyri Sarha]

On 27/04/2020 13:41, Tero Kristo wrote:
> On 27/04/2020 13:37, Jyri Sarha wrote:
>> On 27/04/2020 13:09, Tero Kristo wrote:
>>>> +        status = "disabled";
>>>
>>> Again, why disabled by default?
>>>
>>
>> tidss device is not functional without a defined video-port. The driver
>> is not implemented in a way that it would handle a broken configuration
>> gracefully.
> 
> What/where/when is the video-port going to be defined then? Is this
> going to be done in an overlay?
> 

Yes. It should be defined in the board specific dts or dtso file, where
the video-connector or -panel is.

BR,
Jyri


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 1/3] arm64: dts: ti: am654: Add DSS node

[Tomi Valkeinen]

On 27/04/2020 13:08, Tero Kristo wrote:
> On 22/04/2020 12:15, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@ti.com>
>>
>> Add DSS node to k3-am65-main.dtsi with labels for board specific
>> support and syscon node for oldi-io-ctrl.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 44 ++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> index 11887c72f23a..7d1bc991708e 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
>> @@ -287,6 +287,11 @@
>>               mux-reg-masks = <0x4080 0x3>, /* SERDES0 lane select */
>>                       <0x4090 0x3>; /* SERDES1 lane select */
>>           };
>> +
>> +        dss_oldi_io_ctrl: dss_oldi_io_ctrl@41E0 {
>> +            compatible = "syscon";
>> +            reg = <0x0000041E0 0x14>;
>> +        };
>>       };
>>       dwc3_0: dwc3@4000000 {
>> @@ -746,4 +751,43 @@
>>               };
>>           };
>>       };
>> +
>> +    dss: dss@04a00000 {
>> +        compatible = "ti,am65x-dss";
>> +        reg =    <0x0 0x04a00000 0x0 0x1000>, /* common */
>> +            <0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
>> +            <0x0 0x04a06000 0x0 0x1000>, /* vid */
>> +            <0x0 0x04a07000 0x0 0x1000>, /* ovr1 */
>> +            <0x0 0x04a08000 0x0 0x1000>, /* ovr2 */
>> +            <0x0 0x04a0a000 0x0 0x1000>, /* vp1 */
>> +            <0x0 0x04a0b000 0x0 0x1000>; /* vp2 */
>> +        reg-names = "common", "vidl1", "vid",
>> +            "ovr1", "ovr2", "vp1", "vp2";
>> +
>> +        ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
>> +
>> +        power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
>> +
>> +        clocks =    <&k3_clks 67 1>,
>> +                <&k3_clks 216 1>,
>> +                <&k3_clks 67 2>;
>> +        clock-names = "fck", "vp1", "vp2";
>> +
>> +        /*
>> +         * Set vp2 clk (DPI_1_IN_CLK) mux to PLL4 via
>> +         * DIV1. See "Figure 12-3365. DSS Integration"
>> +         * in AM65x TRM for details.
>> +         */
>> +        assigned-clocks = <&k3_clks 67 2>;
>> +        assigned-clock-parents = <&k3_clks 67 5>;
>> +
>> +        interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
>> +
>> +        status = "disabled";
> 
> Any reason why the node is disabled? Are you planning to enable it somewhere later on, or is that 
> left for the user to do?

It's enabled in board DT files or DT overlays which add a display and the DT graph for the 
connections. Having DSS driver probe without any displays is a waste of resources.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tomi Valkeinen]

On 27/04/2020 13:37, Jyri Sarha wrote:
> On 27/04/2020 13:09, Tero Kristo wrote:
>>> +        status = "disabled";
>>
>> Again, why disabled by default?
>>
> 
> tidss device is not functional without a defined video-port. The driver
> is not implemented in a way that it would handle a broken configuration
> gracefully.

Then we need to fix it. The driver should handle the case where there are no ports defined just fine.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Jyri Sarha]

On 27/04/2020 13:51, Tomi Valkeinen wrote:
> On 27/04/2020 13:37, Jyri Sarha wrote:
>> On 27/04/2020 13:09, Tero Kristo wrote:
>>>> +        status = "disabled";
>>>
>>> Again, why disabled by default?
>>>
>>
>> tidss device is not functional without a defined video-port. The driver
>> is not implemented in a way that it would handle a broken configuration
>> gracefully.
> 
> Then we need to fix it. The driver should handle the case where there
> are no ports defined just fine.
> 

Just by reading the code, I would say that currently the probe would
fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs.

So should the probe fail gracefully and silently, or should we try to
register a DRM device with no CRTCs? Is that even possible?

BR,
Jyri


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tomi Valkeinen]

On 27/04/2020 14:10, Jyri Sarha wrote:
> On 27/04/2020 13:51, Tomi Valkeinen wrote:
>> On 27/04/2020 13:37, Jyri Sarha wrote:
>>> On 27/04/2020 13:09, Tero Kristo wrote:
>>>>> +        status = "disabled";
>>>>
>>>> Again, why disabled by default?
>>>>
>>>
>>> tidss device is not functional without a defined video-port. The driver
>>> is not implemented in a way that it would handle a broken configuration
>>> gracefully.
>>
>> Then we need to fix it. The driver should handle the case where there
>> are no ports defined just fine.
>>
> 
> Just by reading the code, I would say that currently the probe would
> fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs.
> 
> So should the probe fail gracefully and silently, or should we try to
> register a DRM device with no CRTCs? Is that even possible?

My first thought is that the driver should exit probe silently with ENODEV if there are no outputs 
defined (but, of course, with EPROBE_DEFER if there are outputs which haven't been probed yet).

It gets a bit more complex if we ever support writeback, as that can be used as mem-to-mem without 
any displays, but I think we can ignore that for now.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tomi Valkeinen]

On 27/04/2020 14:15, Tomi Valkeinen wrote:
> On 27/04/2020 14:10, Jyri Sarha wrote:
>> On 27/04/2020 13:51, Tomi Valkeinen wrote:
>>> On 27/04/2020 13:37, Jyri Sarha wrote:
>>>> On 27/04/2020 13:09, Tero Kristo wrote:
>>>>>> +        status = "disabled";
>>>>>
>>>>> Again, why disabled by default?
>>>>>
>>>>
>>>> tidss device is not functional without a defined video-port. The driver
>>>> is not implemented in a way that it would handle a broken configuration
>>>> gracefully.
>>>
>>> Then we need to fix it. The driver should handle the case where there
>>> are no ports defined just fine.
>>>
>>
>> Just by reading the code, I would say that currently the probe would
>> fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs.
>>
>> So should the probe fail gracefully and silently, or should we try to
>> register a DRM device with no CRTCs? Is that even possible?
> 
> My first thought is that the driver should exit probe silently with ENODEV if there are no outputs 
> defined (but, of course, with EPROBE_DEFER if there are outputs which haven't been probed yet).
> 
> It gets a bit more complex if we ever support writeback, as that can be used as mem-to-mem without 
> any displays, but I think we can ignore that for now.

In any case, that's not the reason for status = "disabled", so that discussion is not related to 
these patches as such.

The reason to have DSS disabled is just to prevent pointless driver probing. When a board dts or a 
DT overlay adds a display, the DSS DT node has to be modified anyway to add the DT graph and the 
panel/bridge data. So one can as well add the single line of "status = enabled" there.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] arm64: dts: ti: k3-j721e-main.dtsi: Add DSS node

[Tero Kristo]

On 27/04/2020 14:37, Tomi Valkeinen wrote:
> On 27/04/2020 14:15, Tomi Valkeinen wrote:
>> On 27/04/2020 14:10, Jyri Sarha wrote:
>>> On 27/04/2020 13:51, Tomi Valkeinen wrote:
>>>> On 27/04/2020 13:37, Jyri Sarha wrote:
>>>>> On 27/04/2020 13:09, Tero Kristo wrote:
>>>>>>> +        status = "disabled";
>>>>>>
>>>>>> Again, why disabled by default?
>>>>>>
>>>>>
>>>>> tidss device is not functional without a defined video-port. The 
>>>>> driver
>>>>> is not implemented in a way that it would handle a broken 
>>>>> configuration
>>>>> gracefully.
>>>>
>>>> Then we need to fix it. The driver should handle the case where there
>>>> are no ports defined just fine.
>>>>
>>>
>>> Just by reading the code, I would say that currently the probe would
>>> fail with returned -ENOMEM after calling drm_vblank_init() with zero 
>>> CRTCs.
>>>
>>> So should the probe fail gracefully and silently, or should we try to
>>> register a DRM device with no CRTCs? Is that even possible?
>>
>> My first thought is that the driver should exit probe silently with 
>> ENODEV if there are no outputs defined (but, of course, with 
>> EPROBE_DEFER if there are outputs which haven't been probed yet).
>>
>> It gets a bit more complex if we ever support writeback, as that can 
>> be used as mem-to-mem without any displays, but I think we can ignore 
>> that for now.
> 
> In any case, that's not the reason for status = "disabled", so that 
> discussion is not related to these patches as such.
> 
> The reason to have DSS disabled is just to prevent pointless driver 
> probing. When a board dts or a DT overlay adds a display, the DSS DT 
> node has to be modified anyway to add the DT graph and the panel/bridge 
> data. So one can as well add the single line of "status = enabled" there.

Ok, thanks for the explanation, queued all three patches towards 5.8 
based on that.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki