[PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

From: Ondrej Jirman <megi@xff.cz>

The phone's display is using a Hannstar LCD panel. Support it by adding a
panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Martijn Braam <martijn@brixit.nl>
Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
- Fix assigned-clock-parents in vopb node (Ondrej Jirman).
- Add vopl and vopl nodes.

 .../dts/rockchip/rk3399-pinephone-pro.dts     | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index a0795a2b1cb1..5116f156d548 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -29,6 +29,12 @@ chosen {
 		stdout-path = "serial2:115200n8";
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm0 0 1000000 0>;
+		pwm-delay-us = <10000>;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		pinctrl-names = "default";
@@ -102,6 +108,32 @@ wifi_pwrseq: sdio-wifi-pwrseq {
 		/* WL_REG_ON on module */
 		reset-gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
 	};
+
+	/* MIPI DSI panel 1.8v supply */
+	vcc1v8_lcd: vcc1v8-lcd {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc1v8_lcd";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren1>;
+	};
+
+	/* MIPI DSI panel 2.8v supply */
+	vcc2v8_lcd: vcc2v8-lcd {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc2v8_lcd";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren>;
+	};
 };
 
 &cpu_alert0 {
@@ -139,6 +171,11 @@ &emmc_phy {
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &i2c0 {
 	clock-frequency = <400000>;
 	i2c-scl-rising-time-ns = <168>;
@@ -362,6 +399,40 @@ &io_domains {
 	status = "okay";
 };
 
+&mipi_dsi {
+	status = "okay";
+	clock-master;
+
+	ports {
+		mipi_out: port@1 {
+			#address-cells = <0>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			mipi_out_panel: endpoint {
+				remote-endpoint = <&mipi_in_panel>;
+			};
+		};
+	};
+
+	panel@0 {
+		compatible = "hannstar,hsd060bhw4";
+		reg = <0>;
+		backlight = <&backlight>;
+		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
+		vcc-supply = <&vcc2v8_lcd>; // 2v8
+		iovcc-supply = <&vcc1v8_lcd>; // 1v8
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_rst_l>;
+
+		port {
+			mipi_in_panel: endpoint {
+				remote-endpoint = <&mipi_out_panel>;
+			};
+		};
+	};
+};
+
 &pmu_io_domains {
 	pmu1830-supply = <&vcc_1v8>;
 	status = "okay";
@@ -374,6 +445,20 @@ pwrbtn_pin: pwrbtn-pin {
 		};
 	};
 
+	dsi {
+		display_rst_l: display-rst-l {
+			rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren: display-pwren {
+			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren1: display-pwren1 {
+			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+
 	pmic {
 		pmic_int_l: pmic-int-l {
 			rockchip,pins = <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
@@ -429,6 +514,10 @@ &sdio0 {
 	status = "okay";
 };
 
+&pwm0 {
+	status = "okay";
+};
+
 &sdmmc {
 	bus-width = <4>;
 	cap-sd-highspeed;
@@ -479,3 +568,25 @@ bluetooth {
 &uart2 {
 	status = "okay";
 };
+
+&vopb {
+	status = "okay";
+	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>, <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
+	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
+	assigned-clock-parents = <&cru PLL_GPLL>, <&cru DCLK_VOP0_DIV>;
+};
+
+&vopb_mmu {
+	status = "okay";
+};
+
+&vopl {
+	status = "okay";
+	assigned-clocks = <&cru DCLK_VOP1_DIV>, <&cru DCLK_VOP1>, <&cru ACLK_VOP1>, <&cru HCLK_VOP1>;
+	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
+	assigned-clock-parents = <&cru PLL_GPLL>, <&cru DCLK_VOP1_DIV>;
+};
+
+&vopl_mmu {
+	status = "okay";
+};

base-commit: da8e7da11e4ba758caf4c149cc8d8cd555aefe5f
-- 
2.39.2

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Jarrah]

Hi Javier,

On 3/27/23 18:41, Javier Martinez Canillas wrote:
> From: Ondrej Jirman <megi@xff.cz>
>
> The phone's display is using a Hannstar LCD panel. Support it by adding a
> panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).


Any reason not to include this with the correct compatible string? It's 
been available since 
https://lore.kernel.org/all/20220813043821.9981-1-kernel@undef.tools/. 
Swapping out gt917s for gt1158 in the node from your previous patch 
should be enough.

Thanks,

Jarrah.

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Jarrah <kernel@undef.tools> writes:

> Hi Javier,
>
> On 3/27/23 18:41, Javier Martinez Canillas wrote:
>> From: Ondrej Jirman <megi@xff.cz>
>>
>> The phone's display is using a Hannstar LCD panel. Support it by adding a
>> panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
>>
>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>> Co-developed-by: Martijn Braam <martijn@brixit.nl>
>> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>> Changes in v2:
>> - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
>
>
> Any reason not to include this with the correct compatible string? It's 
> been available since 
> https://lore.kernel.org/all/20220813043821.9981-1-kernel@undef.tools/. 
> Swapping out gt917s for gt1158 in the node from your previous patch 
> should be enough.
>

Yes, but I didn't know that the driver already had the compatible string
so I just dropped it for now and was meaning to take a look to that later.

Adding the touchscreen node can be done as a follow-up though in another
patch.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Heiko Stübner]

Am Montag, 27. März 2023, 09:55:15 CEST schrieb Jarrah:
> Hi Javier,
> 
> On 3/27/23 18:41, Javier Martinez Canillas wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> >
> > The phone's display is using a Hannstar LCD panel. Support it by adding a
> > panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
> >
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Co-developed-by: Martijn Braam <martijn@brixit.nl>
> > Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> >
> > Changes in v2:
> > - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
> 
> 
> Any reason not to include this with the correct compatible string? It's 
> been available since 
> https://lore.kernel.org/all/20220813043821.9981-1-kernel@undef.tools/. 
> Swapping out gt917s for gt1158 in the node from your previous patch 
> should be enough.

Just wondering if I'm blind, Javier's patch doesn't contain any touchscreen
controller (haven't found neither gt9* or gt11* mentioned there), so
I'm inclined to go with the "touchscreen can be added later" thing.


Heiko

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Jarrah]

On 3/27/23 20:11, Heiko Stübner wrote:
> Am Montag, 27. März 2023, 09:55:15 CEST schrieb Jarrah:
>> Hi Javier,
>>
>> On 3/27/23 18:41, Javier Martinez Canillas wrote:
>>> From: Ondrej Jirman <megi@xff.cz>
>>>
>>> The phone's display is using a Hannstar LCD panel. Support it by adding a
>>> panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
>>>
>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>> Co-developed-by: Martijn Braam <martijn@brixit.nl>
>>> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
>>
>> Any reason not to include this with the correct compatible string? It's
>> been available since
>> https://lore.kernel.org/all/20220813043821.9981-1-kernel@undef.tools/.
>> Swapping out gt917s for gt1158 in the node from your previous patch
>> should be enough.
> Just wondering if I'm blind, Javier's patch doesn't contain any touchscreen
> controller (haven't found neither gt9* or gt11* mentioned there), so
> I'm inclined to go with the "touchscreen can be added later" thing.


I was just commenting on the "Changes in v2" section. v1 had a 
touchscreen node in it (assuming I found the right v1).


All good, I can follow up with a patch for the touchscreen.

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Jarrah <kernel@undef.tools> writes:

> On 3/27/23 20:11, Heiko Stübner wrote:
>> Am Montag, 27. März 2023, 09:55:15 CEST schrieb Jarrah:
>>> Hi Javier,
>>>
>>> On 3/27/23 18:41, Javier Martinez Canillas wrote:
>>>> From: Ondrej Jirman <megi@xff.cz>
>>>>
>>>> The phone's display is using a Hannstar LCD panel. Support it by adding a
>>>> panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
>>>>
>>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>>> Co-developed-by: Martijn Braam <martijn@brixit.nl>
>>>> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
>>>
>>> Any reason not to include this with the correct compatible string? It's
>>> been available since
>>> https://lore.kernel.org/all/20220813043821.9981-1-kernel@undef.tools/.
>>> Swapping out gt917s for gt1158 in the node from your previous patch
>>> should be enough.
>> Just wondering if I'm blind, Javier's patch doesn't contain any touchscreen
>> controller (haven't found neither gt9* or gt11* mentioned there), so
>> I'm inclined to go with the "touchscreen can be added later" thing.
>
>
> I was just commenting on the "Changes in v2" section. v1 had a 
> touchscreen node in it (assuming I found the right v1).
>
>
> All good, I can follow up with a patch for the touchscreen.
>

That would be great, thanks Jarrah!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

Hi Javier,

I've tried the patch on top of linus/master and it works as expected. My
DRM test app shows 16.669ms between frames. The display output is ok on
developer batch pinephone pro, and is corrupted on production version.
Display also doesn't come back after blanking. All as expected.

Tested-by: Ondrej Jirman <megi@xff.cz>

A few more comments below.

On Mon, Mar 27, 2023 at 09:41:35AM +0200, Javier Martinez Canillas wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> The phone's display is using a Hannstar LCD panel. Support it by adding a
> panel DT node and all needed nodes (backlight, MIPI DSI, regulators, etc).
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v2:
> - Drop touchscreen node because used the wrong compatible (Ondrej Jirman).
> - Fix assigned-clock-parents in vopb node (Ondrej Jirman).
> - Add vopl and vopl nodes.
> 
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index a0795a2b1cb1..5116f156d548 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -29,6 +29,12 @@ chosen {
>  		stdout-path = "serial2:115200n8";
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm0 0 1000000 0>;

This (1 kHz) seems to be outside of the range of recommended dimming frequency
of SY7203: https://megous.com/dl/tmp/fb79af4023a5f102.png It's too low.

The consequence is that there's a large ripple on the positive input of the
feedback comparator https://megous.com/dl/tmp/e155900fecb0323f.png which
will cause similar instability in backlight brightness.

I've hooked up a photoresistor to a scope, and the display is indeed varying the
brightness at 1 kHz https://megous.com/dl/tmp/09cb95c7b4b2892b.png

There are two variants of SY7203 which differ by ouput regulation technique.
One with this internal integrator, and other with direct PWM control of the
output. My guess is that PPP uses the integrator variant.

I switched PWM period to 50000 (20 kHz recommended by the datasheet and the
flicker is gone https://megous.com/dl/tmp/31b6bfc51badde3b.png

So I think higher PWM frequency will be better suited here, and this may really
be the LED driver variant with the integrator.

(Photoresistors are not that fast, but I've hooked a LED to signal generator,
to simulate 20kHz blinking backlight, and I was still able to catch the pattern
on the scope via a photoresistor, so it looks like this verifies that it
would still be possible to measure some flicker at 20 kHz using this technique.
I guess I should buy a PIN diode for the next time. :))

> +		pwm-delay-us = <10000>;
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		pinctrl-names = "default";
> @@ -102,6 +108,32 @@ wifi_pwrseq: sdio-wifi-pwrseq {
>  		/* WL_REG_ON on module */
>  		reset-gpios = <&gpio0 RK_PB2 GPIO_ACTIVE_LOW>;
>  	};
> +
> +	/* MIPI DSI panel 1.8v supply */
> +	vcc1v8_lcd: vcc1v8-lcd {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-name = "vcc1v8_lcd";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc3v3_sys>;
> +		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_pwren1>;
> +	};
> +
> +	/* MIPI DSI panel 2.8v supply */
> +	vcc2v8_lcd: vcc2v8-lcd {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-name = "vcc2v8_lcd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&vcc3v3_sys>;
> +		gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_pwren>;
> +	};
>  };
>  
>  &cpu_alert0 {
> @@ -139,6 +171,11 @@ &emmc_phy {
>  	status = "okay";
>  };
>  
> +&gpu {
> +	mali-supply = <&vdd_gpu>;
> +	status = "okay";
> +};
> +
>  &i2c0 {
>  	clock-frequency = <400000>;
>  	i2c-scl-rising-time-ns = <168>;
> @@ -362,6 +399,40 @@ &io_domains {
>  	status = "okay";
>  };
>  
> +&mipi_dsi {
> +	status = "okay";
> +	clock-master;
> +
> +	ports {
> +		mipi_out: port@1 {
> +			#address-cells = <0>;
> +			#size-cells = <0>;
> +			reg = <1>;
> +
> +			mipi_out_panel: endpoint {
> +				remote-endpoint = <&mipi_in_panel>;
> +			};
> +		};
> +	};
> +
> +	panel@0 {
> +		compatible = "hannstar,hsd060bhw4";
> +		reg = <0>;
> +		backlight = <&backlight>;
> +		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
> +		vcc-supply = <&vcc2v8_lcd>; // 2v8
> +		iovcc-supply = <&vcc1v8_lcd>; // 1v8

The comments here are a bit useless.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_rst_l>;
> +
> +		port {
> +			mipi_in_panel: endpoint {
> +				remote-endpoint = <&mipi_out_panel>;
> +			};
> +		};
> +	};
> +};
> +
>  &pmu_io_domains {
>  	pmu1830-supply = <&vcc_1v8>;
>  	status = "okay";
> @@ -374,6 +445,20 @@ pwrbtn_pin: pwrbtn-pin {
>  		};
>  	};
>  
> +	dsi {
> +		display_rst_l: display-rst-l {
> +			rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};

This pin already has oboard pull-down resistor: https://megous.com/dl/tmp/ec7f9852bfaa46af.png
And it's named LCD_RST in the schematic.

> +		display_pwren: display-pwren {
> +			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		display_pwren1: display-pwren1 {
> +			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};

I wonder if there's any use in enabling pull-downs when the pin is always driven
by the SoC. Also these pins are name LCD_PWREN / LCD_PWREN1 in the schematic.

kind regards,
	o.

> +	};
> +
>  	pmic {
>  		pmic_int_l: pmic-int-l {
>  			rockchip,pins = <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
> @@ -429,6 +514,10 @@ &sdio0 {
>  	status = "okay";
>  };
>  
> +&pwm0 {
> +	status = "okay";
> +};
> +
>  &sdmmc {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> @@ -479,3 +568,25 @@ bluetooth {
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&vopb {
> +	status = "okay";
> +	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>, <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
> +	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
> +	assigned-clock-parents = <&cru PLL_GPLL>, <&cru DCLK_VOP0_DIV>;
> +};
> +
> +&vopb_mmu {
> +	status = "okay";
> +};
> +
> +&vopl {
> +	status = "okay";
> +	assigned-clocks = <&cru DCLK_VOP1_DIV>, <&cru DCLK_VOP1>, <&cru ACLK_VOP1>, <&cru HCLK_VOP1>;
> +	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
> +	assigned-clock-parents = <&cru PLL_GPLL>, <&cru DCLK_VOP1_DIV>;
> +};
> +
> +&vopl_mmu {
> +	status = "okay";
> +};
> 
> base-commit: da8e7da11e4ba758caf4c149cc8d8cd555aefe5f
> -- 
> 2.39.2
> 

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Ondřej Jirman <megi@xff.cz> writes:

Hell Ondřej,

> Hi Javier,
>
> I've tried the patch on top of linus/master and it works as expected. My
> DRM test app shows 16.669ms between frames. The display output is ok on
> developer batch pinephone pro, and is corrupted on production version.
> Display also doesn't come back after blanking. All as expected.
>
> Tested-by: Ondrej Jirman <megi@xff.cz>
>

Thanks for testing.

> A few more comments below.
>

I'm OK with these comments but I did a git diff with your orange-pi-6.3
branch just before posting and this was the latest that's in your tree.

So feel free to either post a v3 addressing the things you are pointing
out or lets land this and we can post any further cleanups on top IMO.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Heiko Stübner]

Am Montag, 27. März 2023, 17:39:57 CEST schrieb Javier Martinez Canillas:
> Ondřej Jirman <megi@xff.cz> writes:
> 
> Hell Ondřej,
> 
> > Hi Javier,
> >
> > I've tried the patch on top of linus/master and it works as expected. My
> > DRM test app shows 16.669ms between frames. The display output is ok on
> > developer batch pinephone pro, and is corrupted on production version.
> > Display also doesn't come back after blanking. All as expected.
> >
> > Tested-by: Ondrej Jirman <megi@xff.cz>
> >
> 
> Thanks for testing.
> 
> > A few more comments below.
> >
> 
> I'm OK with these comments but I did a git diff with your orange-pi-6.3
> branch just before posting and this was the latest that's in your tree.
> 
> So feel free to either post a v3 addressing the things you are pointing
> out or lets land this and we can post any further cleanups on top IMO.

I would really like to _not_ apply essentially broken code, so really
would prefer the v3-approach.

Thanks
Heiko

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Heiko Stübner <heiko@sntech.de> writes:

> Am Montag, 27. März 2023, 17:39:57 CEST schrieb Javier Martinez Canillas:
>> Ondřej Jirman <megi@xff.cz> writes:
>> 
>> Hell Ondřej,
>> 
>> > Hi Javier,
>> >
>> > I've tried the patch on top of linus/master and it works as expected. My
>> > DRM test app shows 16.669ms between frames. The display output is ok on
>> > developer batch pinephone pro, and is corrupted on production version.
>> > Display also doesn't come back after blanking. All as expected.
>> >
>> > Tested-by: Ondrej Jirman <megi@xff.cz>
>> >
>> 
>> Thanks for testing.
>> 
>> > A few more comments below.
>> >
>> 
>> I'm OK with these comments but I did a git diff with your orange-pi-6.3
>> branch just before posting and this was the latest that's in your tree.
>> 
>> So feel free to either post a v3 addressing the things you are pointing
>> out or lets land this and we can post any further cleanups on top IMO.
>
> I would really like to _not_ apply essentially broken code, so really
> would prefer the v3-approach.
>


> Thanks
> Heiko
>
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Heiko Stübner <heiko@sntech.de> writes:

> Am Montag, 27. März 2023, 17:39:57 CEST schrieb Javier Martinez Canillas:
>> Ondřej Jirman <megi@xff.cz> writes:
>> 
>> Hell Ondřej,
>> 
>> > Hi Javier,
>> >
>> > I've tried the patch on top of linus/master and it works as expected. My
>> > DRM test app shows 16.669ms between frames. The display output is ok on
>> > developer batch pinephone pro, and is corrupted on production version.
>> > Display also doesn't come back after blanking. All as expected.
>> >
>> > Tested-by: Ondrej Jirman <megi@xff.cz>
>> >
>> 
>> Thanks for testing.
>> 
>> > A few more comments below.
>> >
>> 
>> I'm OK with these comments but I did a git diff with your orange-pi-6.3
>> branch just before posting and this was the latest that's in your tree.
>> 
>> So feel free to either post a v3 addressing the things you are pointing
>> out or lets land this and we can post any further cleanups on top IMO.
>
> I would really like to _not_ apply essentially broken code, so really
> would prefer the v3-approach.
>
> Thanks
> Heiko
>
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Heiko Stübner <heiko@sntech.de> writes:

Hello Heiko,

> Am Montag, 27. März 2023, 17:39:57 CEST schrieb Javier Martinez Canillas:
>> Ondřej Jirman <megi@xff.cz> writes:
>> 
>> Hell Ondřej,
>> 
>> > Hi Javier,
>> >
>> > I've tried the patch on top of linus/master and it works as expected. My
>> > DRM test app shows 16.669ms between frames. The display output is ok on
>> > developer batch pinephone pro, and is corrupted on production version.
>> > Display also doesn't come back after blanking. All as expected.
>> >
>> > Tested-by: Ondrej Jirman <megi@xff.cz>
>> >
>> 
>> Thanks for testing.
>> 
>> > A few more comments below.
>> >
>> 
>> I'm OK with these comments but I did a git diff with your orange-pi-6.3
>> branch just before posting and this was the latest that's in your tree.
>> 
>> So feel free to either post a v3 addressing the things you are pointing
>> out or lets land this and we can post any further cleanups on top IMO.
>
> I would really like to _not_ apply essentially broken code, so really
> would prefer the v3-approach.
>

It is broken though? This is what is in Ondrej downstream tree and I see
no issues on my Pinephone Pro. He mentioned some flicker when looking at
the signals with a scope and hooking a photoresistor.

But that's fair. I'll let Ondrej then post a v3 if he wants to address the
issues he pointed out, since is his patch after all.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

On Mon, Mar 27, 2023 at 06:15:55PM +0200, Javier Martinez Canillas wrote:
> Heiko Stübner <heiko@sntech.de> writes:
> 
> Hello Heiko,
> 
> > Am Montag, 27. März 2023, 17:39:57 CEST schrieb Javier Martinez Canillas:
> >> Ondřej Jirman <megi@xff.cz> writes:
> >> 
> >> Hell Ondřej,
> >> 
> >> > Hi Javier,
> >> >
> >> > I've tried the patch on top of linus/master and it works as expected. My
> >> > DRM test app shows 16.669ms between frames. The display output is ok on
> >> > developer batch pinephone pro, and is corrupted on production version.
> >> > Display also doesn't come back after blanking. All as expected.
> >> >
> >> > Tested-by: Ondrej Jirman <megi@xff.cz>
> >> >
> >> 
> >> Thanks for testing.
> >> 
> >> > A few more comments below.
> >> >
> >> 
> >> I'm OK with these comments but I did a git diff with your orange-pi-6.3
> >> branch just before posting and this was the latest that's in your tree.
> >> 
> >> So feel free to either post a v3 addressing the things you are pointing
> >> out or lets land this and we can post any further cleanups on top IMO.
> >
> > I would really like to _not_ apply essentially broken code, so really
> > would prefer the v3-approach.
> >
> 
> It is broken though? This is what is in Ondrej downstream tree and I see
> no issues on my Pinephone Pro. He mentioned some flicker when looking at
> the signals with a scope and hooking a photoresistor.

LED regulator is driven out of spec by a frequency that's 20x lower than
recommended, if you want short version of what's broken about the DT patch.

> But that's fair. I'll let Ondrej then post a v3 if he wants to address the
> issues he pointed out, since is his patch after all.

It's not my patch. Original author of the DT is Martijn or Kamil. I just carry
their DT work in split-up patches in my tree, and I sometimes try to find solutions
to bugs I find when using PPP. That's the story of these DT changes you're posting.

Since you posted this DT patch for upstreaming, I wanted to help you by reviewed
it more completely, so I opened the schematic and datasheets for the components
that are described in this patch, and discovered these new issues I commented
about. And I also tested it on top of linus/master.

Just because something is in my tree doesn't mean it's mine, or that I reviewed
it in detail and prepared it for upstreaming, or that I'm interested in
upstreaming it. I'm just trying to help you with your upstreaming effort by
testing and review since I got to know the hardware quite well over the last
years and can check the schematics and datasheets quickly, and I like to think
upstream code is held to higher standard. That's all.

kind regards,
	o.


> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Ondřej Jirman <megi@xff.cz> writes:

> On Mon, Mar 27, 2023 at 06:15:55PM +0200, Javier Martinez Canillas wrote:

[...]

>> 
>> It is broken though? This is what is in Ondrej downstream tree and I see
>> no issues on my Pinephone Pro. He mentioned some flicker when looking at
>> the signals with a scope and hooking a photoresistor.
>
> LED regulator is driven out of spec by a frequency that's 20x lower than
> recommended, if you want short version of what's broken about the DT patch.
>
>> But that's fair. I'll let Ondrej then post a v3 if he wants to address the
>> issues he pointed out, since is his patch after all.
>
> It's not my patch. Original author of the DT is Martijn or Kamil. I just carry
> their DT work in split-up patches in my tree, and I sometimes try to find solutions
> to bugs I find when using PPP. That's the story of these DT changes you're posting.
>
> Since you posted this DT patch for upstreaming, I wanted to help you by reviewed
> it more completely, so I opened the schematic and datasheets for the components
> that are described in this patch, and discovered these new issues I commented
> about. And I also tested it on top of linus/master.
>
> Just because something is in my tree doesn't mean it's mine, or that I reviewed
> it in detail and prepared it for upstreaming, or that I'm interested in

Thanks for the clarification. Because the patch had your authorship I
wrongly assumed that came from you. Sorry about the confusion.

> upstreaming it. I'm just trying to help you with your upstreaming effort by
> testing and review since I got to know the hardware quite well over the last
> years and can check the schematics and datasheets quickly, and I like to think
> upstream code is held to higher standard. That's all.
>

Appreciate your help and I agree that upstream code should be held to a
high standard. But since the DTS in mainline is pretty basic anyways (you
can only boot to serial right now), is not really usable for other thing
than development and keep adding the missing support.

So I thought that we could do it in steps without creating that much work
for the people trying to post the downstream patches and having to re-spin
too many times.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

On Mon, Mar 27, 2023 at 08:15:52PM +0200, Javier Martinez Canillas wrote:
> Ondřej Jirman <megi@xff.cz> writes:
> 
> > On Mon, Mar 27, 2023 at 06:15:55PM +0200, Javier Martinez Canillas wrote:
> 
> [...]
> 
> >> 
> >> It is broken though? This is what is in Ondrej downstream tree and I see
> >> no issues on my Pinephone Pro. He mentioned some flicker when looking at
> >> the signals with a scope and hooking a photoresistor.
> >
> > LED regulator is driven out of spec by a frequency that's 20x lower than
> > recommended, if you want short version of what's broken about the DT patch.
> >
> >> But that's fair. I'll let Ondrej then post a v3 if he wants to address the
> >> issues he pointed out, since is his patch after all.
> >
> > It's not my patch. Original author of the DT is Martijn or Kamil. I just carry
> > their DT work in split-up patches in my tree, and I sometimes try to find solutions
> > to bugs I find when using PPP. That's the story of these DT changes you're posting.
> >
> > Since you posted this DT patch for upstreaming, I wanted to help you by reviewed
> > it more completely, so I opened the schematic and datasheets for the components
> > that are described in this patch, and discovered these new issues I commented
> > about. And I also tested it on top of linus/master.
> >
> > Just because something is in my tree doesn't mean it's mine, or that I reviewed
> > it in detail and prepared it for upstreaming, or that I'm interested in
> 
> Thanks for the clarification. Because the patch had your authorship I
> wrongly assumed that came from you. Sorry about the confusion.

Ever since base DT support for Pinephone Pro was merged, none of the DT patches
in my tree are in the original form as prepared by the authors + fixes I've
added. That's simply impossible anymore.

To look up who did what, you'd have to look at older branches, pre-merge.

Patches after the merge just came from squashing everything into one patch,
cleaning it up, and re-splitting along some vague functionality boundaries,
while trying to keep best-effort original SoBs as faithfully as possible, so
that I can keep maintaining the PPP support in a sane manner.

Anyway, SoB's are added in chronological order. So:

https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926

Means the author of the changes is Martijn + Kamil (roughly) and I may have
a line of code in there too, since my SoB is last. For some reason, the order is
inverted in the patch you posted, making it seem I developed these changes
originally.

> > upstreaming it. I'm just trying to help you with your upstreaming effort by
> > testing and review since I got to know the hardware quite well over the last
> > years and can check the schematics and datasheets quickly, and I like to think
> > upstream code is held to higher standard. That's all.
> >
> 
> Appreciate your help and I agree that upstream code should be held to a
> high standard. But since the DTS in mainline is pretty basic anyways (you
> can only boot to serial right now), is not really usable for other thing
> than development and keep adding the missing support.

Having wrong frequency used is not a missing support for something. Sorry it's
too bothersome to get the review piecemeal, but sometimes people have more time to
look at patches in-depth, and at other times they don't and you just get surface
level or no review.

One point of posting patches to the mailing list is to get review. And it's not
that great to do in-depth review for you, up to going through schematics and
datasheets, testing, and even proposing and testing solutions for found issues,
just to be dismissed without technical reason.

The thing is this review will most likely happen just once, and noone will go
back, read through the entire huge DT along with a schematic, to look at whether
this or that pullup is really necessary, whether some parameter is out of spec
from the datasheet for each part, or things like that. That's just not
pragmatic.

Instead, people will happily attribute non-obvious issues caused by these
omissions of manual review to shoddy or slow or power-inefficient HW. "1kHz
+ harmonics interference in codec because high power backlight DC-DC regulator
basically spews off 1kHz of 1-2W load + harmonics because it's driven
incorrectly? Ah, the phone just has a shitty audio codec!"

So, don't take it as some perfectionism. Upstreaming just seems like the best
time to look at things that were overlooked in the past in more detail and get
these little things right, because the DT additions are done piecemal and
slowly/gradually, so it's more manageable to review and fix right away. This
will just not happen later on for these obscure devices like Pinephone Pro,
where the whole effort that goes into it is like one half of a fulltime
developer time split over 4 mildly interested real persons, slowly tapering off
over time as the device ages.

regards,
	o.

> So I thought that we could do it in steps without creating that much work
> for the people trying to post the downstream patches and having to re-spin
> too many times.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Ondřej Jirman <megi@xff.cz> writes:

> On Mon, Mar 27, 2023 at 08:15:52PM +0200, Javier Martinez Canillas wrote:
>> Ondřej Jirman <megi@xff.cz> writes:

[...]

>> > Just because something is in my tree doesn't mean it's mine, or that I reviewed
>> > it in detail and prepared it for upstreaming, or that I'm interested in
>> 
>> Thanks for the clarification. Because the patch had your authorship I
>> wrongly assumed that came from you. Sorry about the confusion.
>
> Ever since base DT support for Pinephone Pro was merged, none of the DT patches
> in my tree are in the original form as prepared by the authors + fixes I've
> added. That's simply impossible anymore.
>
> To look up who did what, you'd have to look at older branches, pre-merge.
>
> Patches after the merge just came from squashing everything into one patch,
> cleaning it up, and re-splitting along some vague functionality boundaries,
> while trying to keep best-effort original SoBs as faithfully as possible, so
> that I can keep maintaining the PPP support in a sane manner.
>

Go it.

> Anyway, SoB's are added in chronological order. So:
>
> https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926
>
> Means the author of the changes is Martijn + Kamil (roughly) and I may have
> a line of code in there too, since my SoB is last. For some reason, the order is
> inverted in the patch you posted, making it seem I developed these changes
> originally.
>

Since the patch had your authorship I changed the order so that your S-o-B
was first but I'll then change the author in v3 and make it match the
first S-o-B entry in your tree (Martijn) then.

>> > upstreaming it. I'm just trying to help you with your upstreaming effort by
>> > testing and review since I got to know the hardware quite well over the last
>> > years and can check the schematics and datasheets quickly, and I like to think
>> > upstream code is held to higher standard. That's all.
>> >
>> 
>> Appreciate your help and I agree that upstream code should be held to a
>> high standard. But since the DTS in mainline is pretty basic anyways (you
>> can only boot to serial right now), is not really usable for other thing
>> than development and keep adding the missing support.
>
> Having wrong frequency used is not a missing support for something. Sorry it's
> too bothersome to get the review piecemeal, but sometimes people have more time to
> look at patches in-depth, and at other times they don't and you just get surface
> level or no review.
>

Ok.

> One point of posting patches to the mailing list is to get review. And it's not
> that great to do in-depth review for you, up to going through schematics and
> datasheets, testing, and even proposing and testing solutions for found issues,
> just to be dismissed without technical reason.
>
> The thing is this review will most likely happen just once, and noone will go
> back, read through the entire huge DT along with a schematic, to look at whether
> this or that pullup is really necessary, whether some parameter is out of spec
> from the datasheet for each part, or things like that. That's just not
> pragmatic.
>

That's fair.

> Instead, people will happily attribute non-obvious issues caused by these
> omissions of manual review to shoddy or slow or power-inefficient HW. "1kHz
> + harmonics interference in codec because high power backlight DC-DC regulator
> basically spews off 1kHz of 1-2W load + harmonics because it's driven
> incorrectly? Ah, the phone just has a shitty audio codec!"
>
> So, don't take it as some perfectionism. Upstreaming just seems like the best
> time to look at things that were overlooked in the past in more detail and get
> these little things right, because the DT additions are done piecemal and
> slowly/gradually, so it's more manageable to review and fix right away. This
> will just not happen later on for these obscure devices like Pinephone Pro,
> where the whole effort that goes into it is like one half of a fulltime
> developer time split over 4 mildly interested real persons, slowly tapering off
> over time as the device ages.
>

Makes sense. I'll address your comments in v3 then and also include a
separate patch (again with Martijn as author and the S-o-B as ordered in
your tree), that includes the touchscreen DT nodes as well. Since Jarrah
pointed out that there's already the correct compatible string in the
driver's OF device ID table.

> regards,
> 	o.
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

One small note...

On Mon, Mar 27, 2023 at 03:01:48PM +0200, megi xff wrote:
> Hi Javier,
> 
> [...]
>
> This (1 kHz) seems to be outside of the range of recommended dimming frequency
> of SY7203: https://megous.com/dl/tmp/fb79af4023a5f102.png It's too low.
> 
> The consequence is that there's a large ripple on the positive input of the
> feedback comparator https://megous.com/dl/tmp/e155900fecb0323f.png which
> will cause similar instability in backlight brightness.
> 
> I've hooked up a photoresistor to a scope, and the display is indeed varying the
> brightness at 1 kHz https://megous.com/dl/tmp/09cb95c7b4b2892b.png
> 
> There are two variants of SY7203 which differ by ouput regulation technique.
> One with this internal integrator, and other with direct PWM control of the
> output. My guess is that PPP uses the integrator variant.
> 
> I switched PWM period to 50000 (20 kHz recommended by the datasheet and the
> flicker is gone https://megous.com/dl/tmp/31b6bfc51badde3b.png
> 
> So I think higher PWM frequency will be better suited here, and this may really
> be the LED driver variant with the integrator.
> 
> (Photoresistors are not that fast, but I've hooked a LED to signal generator,
> to simulate 20kHz blinking backlight, and I was still able to catch the pattern
> on the scope via a photoresistor, so it looks like this verifies that it
> would still be possible to measure some flicker at 20 kHz using this technique.
> I guess I should buy a PIN diode for the next time. :))

Experimentally SY7203 will only start up with duty cycle of 250ns or more.

So this means that default curve generated by the kernel will not work at 20 kHz
at low ranges, because cie1931 -> pwm duty cycle covnersion done by the
kernel will result in too small duty cycle at brightness < 5%, because that
translates to duty cycle of 250ns or less. In other words, kernel will generate
3124 brightness steps for PWM period of 50us, with bottom ~150 steps being
unusable and behaving weirdly (sometimes some of them work sometimes not,
depending whether the LED regulator is already running or not).

So the cie1931 curve may need a bit of a Y shift, by specifying a minimum duty
cycle usable by the hardware.

Something like these 50 brightness levels work nicely, starting from minimum
250ns and going up:

	brightness-levels =
		<0 250 360 470 580 690 810 949 1110 1294 1502
		1737 1998 2289 2610 2964 3351 3774 4233 4731
		5268 5847 6467 7133 7845 8604 9412 10271 11182
		12146 13164 14239 15374 16568 17822 19140 20521
		21969 23483 25068 26722 28447 30247 32121 34071
		36099 38210 40400 42669 45026 47468 50000>;
	default-brightness-level = <17>;

when put into backlight node.

Or if we want 100 steps, then this curve would work, too:

brightness-levels = <250 304 360 414 470 524 580 634 690 747 810 877
	949 1027 1110 1199 1294 1395 1502 1616 1737 1864 1998 2140 2289
	2446 2610 2783 2964 3154 3351 3559 3774 3999 4233 4477 4731 4994
	5268 5552 5847 6152 6467 6795 7133 7483 7845 8219 8604 9002 9412
	9835 10271 10719 11182 11656 12146 12648 13164 13695 14239 14799
	15374 15963 16568 17186 17822 18474 19140 19822 20521 21236 21969
	22717 23483 24267 25068 25885 26722 27575 28447 29338 30247 31173
	32121 33087 34071 35077 36099 37145 38210 39292 40400 41523 42669
	43839 45026 46237 47468 48722 50000>;


> > +		pwm-delay-us = <10000>;

Also this doesn't seem to be documented anywhere or used in the kernel code...
So we should remove it.

kind regards,
	o.

> > +	};
> > +
> >  	gpio-keys {
> >  		compatible = "gpio-keys";
> >  		pinctrl-names = "default";