[PATCH v4 0/4] Add PinePhone Pro display support

[PATCH v4 0/4] Add PinePhone Pro display support

[Javier Martinez Canillas]

This series add support for the display present in the PinePhone Pro.

Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.

Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
an entry for the driver in the MAINTAINERS file.

Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
to enable both the display and the touchscreen. This makes the upstream DTS
much more usable and will allow for example to enable support for the phone
in the Fedora distribution.

I only added myself as the maintainer for the driver because I don't know
if Kamil and Ondrej that worked in the driver would be interested. Please
let me know folks if you are, and I can add you too in the next revision.

This is a v4 of the patch-set that addresses issues pointed out in v3:

https://lists.freedesktop.org/archives/dri-devel/2022-December/384560.html

The patches were tested on a PinePhone Pro Explorer Edition using a Fedora
37 Workstation image.

Best regards,
Javier

Changes in v4:
- Add fallback "himax,hx8394" compatible for the panel controller (Jagan Teki).
- Add Tom Fitzhenry's Tested-by tag.
- Add Sam Ravnborg's Acked-by tag.
- Add Tom Fitzhenry's Tested-by tag.
- Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).

Changes in v3:
- Fix example snippet for `make dt_binding_check` to pass (Krzysztof Kozlowski).
- Add Sam Ravnborg's reviwed-by tag.
- Move driver patch after one introducing the DT binding (Sam Ravnborg).

Changes in v2:
- Drop redundant "bindings" in subject (Krzysztof Kozlowski).
- Drop "device tree bindings" in title (Krzysztof Kozlowski).
- Put port next to other "true" properties (Krzysztof Kozlowski).
- Add Krzysztof Kozlowski's Reviewed-by tag.
- Add year to driver's copyright notice (Sam Ravnborg)
- Remove unused <video/display_timing.h> header include (Sam Ravnborg).
- Use mipi_dsi_dcs_write_seq() helper and drop custom macro (Sam Ravnborg).
- Drop unnecessary info messages and move useful one to debug (Sam Ravnborg).
- Fix regulator node names (Maya Matuszczyk).
- Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
- Remove unnecessary comments in panel node (Maya Matuszczyk).

Javier Martinez Canillas (2):
  dt-bindings: display: Add Himax HX8394 panel controller
  MAINTAINERS: Add entry for Himax HX8394 panel controller driver

Kamil Trzciński (1):
  drm: panel: Add Himax HX8394 panel controller driver

Ondrej Jirman (1):
  arm64: dts: rk3399-pinephone-pro: Add internal display support

 .../bindings/display/panel/himax,hx8394.yaml  |  75 +++
 MAINTAINERS                                   |   7 +
 .../dts/rockchip/rk3399-pinephone-pro.dts     | 123 +++++
 drivers/gpu/drm/panel/Kconfig                 |  12 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-himax-hx8394.c    | 446 ++++++++++++++++++
 6 files changed, 664 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-himax-hx8394.c

-- 
2.38.1

[PATCH v4 1/4] dt-bindings: display: Add Himax HX8394 panel controller

[Javier Martinez Canillas]

Add device tree bindings for panels based on the Himax HX8394 controller,
such as the HannStar HSD060BHW4 720x1440 TFT LCD panel that is connected
through a MIPI-DSI video interface.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Changes in v4:
- Add fallback "himax,hx8394" compatible for the panel controller (Jagan Teki).

Changes in v3:
- Fix example snippet for `make dt_binding_check` to pass (Krzysztof Kozlowski).

Changes in v2:
- Drop redundant "bindings" in subject (Krzysztof Kozlowski).
- Drop "device tree bindings" in title (Krzysztof Kozlowski).
- Put port next to other "true" properties (Krzysztof Kozlowski).
- Add Krzysztof Kozlowski's Reviewed-by tag.

 .../bindings/display/panel/himax,hx8394.yaml  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
new file mode 100644
index 000000000000..058b4eae68a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/himax,hx8394.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX8394 MIPI-DSI LCD panel controller
+
+maintainers:
+  - Javier Martinez Canillas <javierm@redhat.com>
+
+description:
+  Device tree bindings for panels based on the Himax HX8394 controller,
+  such as the HannStar HSD060BHW4 720x1440 TFT LCD panel connected with
+  a MIPI-DSI video interface.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - hannstar,hsd060bhw4
+      - const: himax,hx8394
+
+  reg: true
+
+  reset-gpios: true
+
+  backlight: true
+
+  port: true
+
+  vcc-supply:
+    description: Panel power supply
+
+  iovcc-supply:
+    description: I/O voltage supply
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - backlight
+  - port
+  - vcc-supply
+  - iovcc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "hannstar,hsd060bhw4", "himax,hx8394";
+            reg = <0>;
+            vcc-supply = <&reg_2v8_p>;
+            iovcc-supply = <&reg_1v8_p>;
+            reset-gpios = <&gpio3 13 GPIO_ACTIVE_LOW>;
+            backlight = <&backlight>;
+
+            port {
+                mipi_in_panel: endpoint {
+                    remote-endpoint = <&mipi_out_panel>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.38.1

[PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

From: Ondrej Jirman <megi@xff.cz>

The phone's display is using Hannstar LCD panel, and Goodix based
touchscreen. Support it.

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

Changes in v4:
- Add Tom Fitzhenry's Tested-by tag.
- Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).

Changes in v2:
- Fix regulator node names (Maya Matuszczyk).
- Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
- Remove unnecessary comments in panel node (Maya Matuszczyk).

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

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index 04403a76238b..0d48fbc5dbe4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -25,6 +25,12 @@ aliases {
 		mmc2 = &sdhci;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm0 0 1000000 0>;
+		pwm-delay-us = <10000>;
+	};
+
 	chosen {
 		stdout-path = "serial2:115200n8";
 	};
@@ -82,6 +88,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
 		vin-supply = <&vcc3v3_sys>;
 	};
 
+	/* MIPI DSI panel 1.8v supply */
+	vcc1v8_lcd: vcc1v8-lcd-regulator {
+		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-regulator {
+		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>;
+	};
+
 	wifi_pwrseq: sdio-wifi-pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		clocks = <&rk818 1>;
@@ -132,6 +164,11 @@ &emmc_phy {
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &i2c0 {
 	clock-frequency = <400000>;
 	i2c-scl-rising-time-ns = <168>;
@@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
 				regulator-name = "vcc3v0_touch";
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			vcca1v8_codec: LDO_REG3 {
@@ -347,6 +387,25 @@ opp07 {
 	};
 };
 
+&i2c3 {
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+
+	touchscreen@14 {
+		compatible = "goodix,gt917s";
+		reg = <0x14>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
+		irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
+		AVDD28-supply = <&vcc3v0_touch>;
+		VDDIO-supply = <&vcc3v0_touch>;
+		touchscreen-size-x = <720>;
+		touchscreen-size-y = <1440>;
+	};
+};
+
 &io_domains {
 	bt656-supply = <&vcc1v8_dvp>;
 	audio-supply = <&vcca1v8_codec>;
@@ -355,6 +414,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", "himax,hx8394";
+		reg = <0>;
+		backlight = <&backlight>;
+		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
+		vcc-supply = <&vcc2v8_lcd>;
+		iovcc-supply = <&vcc1v8_lcd>;
+		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";
@@ -367,6 +460,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>;
@@ -408,6 +515,10 @@ bt_reset_pin: bt-reset-pin {
 	};
 };
 
+&pwm0 {
+	status = "okay";
+};
+
 &sdio0 {
 	bus-width = <4>;
 	cap-sd-highspeed;
@@ -472,3 +583,15 @@ 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_CPLL>, <&cru DCLK_VOP0_FRAC>;
+};
+
+&vopb_mmu {
+	status = "okay";
+};
-- 
2.38.1

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

Hi Javier,

On Fri, Dec 30, 2022 at 12:31:54PM +0100, Javier Martinez Canillas wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> The phone's display is using Hannstar LCD panel, and Goodix based
> touchscreen. Support it.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
> ---
> 
> Changes in v4:
> - Add Tom Fitzhenry's Tested-by tag.
> - Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).
> 
> Changes in v2:
> - Fix regulator node names (Maya Matuszczyk).
> - Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
> - Remove unnecessary comments in panel node (Maya Matuszczyk).
> 
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 123 ++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 04403a76238b..0d48fbc5dbe4 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -25,6 +25,12 @@ aliases {
>  		mmc2 = &sdhci;
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm0 0 1000000 0>;
> +		pwm-delay-us = <10000>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial2:115200n8";
>  	};
> @@ -82,6 +88,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
>  		vin-supply = <&vcc3v3_sys>;
>  	};
>  
> +	/* MIPI DSI panel 1.8v supply */
> +	vcc1v8_lcd: vcc1v8-lcd-regulator {
> +		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-regulator {
> +		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>;
> +	};
> +
>  	wifi_pwrseq: sdio-wifi-pwrseq {
>  		compatible = "mmc-pwrseq-simple";
>  		clocks = <&rk818 1>;
> @@ -132,6 +164,11 @@ &emmc_phy {
>  	status = "okay";
>  };
>  
> +&gpu {
> +	mali-supply = <&vdd_gpu>;
> +	status = "okay";
> +};
> +
>  &i2c0 {
>  	clock-frequency = <400000>;
>  	i2c-scl-rising-time-ns = <168>;
> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
>  				regulator-name = "vcc3v0_touch";
>  				regulator-min-microvolt = <3000000>;
>  				regulator-max-microvolt = <3000000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};

You're instructing RK818 to shut down the regulator for touch controller during
suspend, but I think Goodix driver expects touch controller to be kept powered on
during suspend. Am I missing something?

https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405

>  			};
>  
>  			vcca1v8_codec: LDO_REG3 {
> @@ -347,6 +387,25 @@ opp07 {
>  	};
>  };
>  
> +&i2c3 {
> +	i2c-scl-rising-time-ns = <450>;
> +	i2c-scl-falling-time-ns = <15>;
> +	status = "okay";
> +
> +	touchscreen@14 {
> +		compatible = "goodix,gt917s";

This is not the correct compatible. Pinephone Pro uses Goodix GT1158:

Goodix-TS 3-0014: ID 1158, version: 0100
Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2


> +		reg = <0x14>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
> +		irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
> +		AVDD28-supply = <&vcc3v0_touch>;
> +		VDDIO-supply = <&vcc3v0_touch>;
> +		touchscreen-size-x = <720>;
> +		touchscreen-size-y = <1440>;
> +	};
> +};
> +
>  &io_domains {
>  	bt656-supply = <&vcc1v8_dvp>;
>  	audio-supply = <&vcca1v8_codec>;
> @@ -355,6 +414,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", "himax,hx8394";
> +		reg = <0>;
> +		backlight = <&backlight>;
> +		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
> +		vcc-supply = <&vcc2v8_lcd>;
> +		iovcc-supply = <&vcc1v8_lcd>;
> +		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";
> @@ -367,6 +460,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>;
> @@ -408,6 +515,10 @@ bt_reset_pin: bt-reset-pin {
>  	};
>  };
>  
> +&pwm0 {
> +	status = "okay";
> +};
> +
>  &sdio0 {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> @@ -472,3 +583,15 @@ 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_CPLL>, <&cru DCLK_VOP0_FRAC>;
> +};

So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
-> DCLK_VOP0_FRAC -> DCLK_VOP0.

Fractional clocks require 20x difference between input and output rates, and
CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
correctly.

Even if this somehow works by fractional clock being bypassed, I did not design
the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.

GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
CPLL 800/74.25 = ~10.77441077441077441077

If you really want to use fractional clock, you'd need to parent it to VPLL
and set VPLL really high, like close to 2GHz.

kind regards,
	o.

> +&vopb_mmu {
> +	status = "okay";
> +};
> -- 
> 2.38.1
> 

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Ondřej,

Thanks a lot for your feedback.

On 12/30/22 16:37, Ondřej Jirman wrote:

[...]

>>  &i2c0 {
>>  	clock-frequency = <400000>;
>>  	i2c-scl-rising-time-ns = <168>;
>> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
>>  				regulator-name = "vcc3v0_touch";
>>  				regulator-min-microvolt = <3000000>;
>>  				regulator-max-microvolt = <3000000>;
>> +				regulator-state-mem {
>> +					regulator-off-in-suspend;
>> +				};
> 
> You're instructing RK818 to shut down the regulator for touch controller during
> suspend, but I think Goodix driver expects touch controller to be kept powered on
> during suspend. Am I missing something?
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405
>

You tell me, it is your patch :) I just cherry-picked this from your tree:

https://github.com/megous/linux/commit/11f8da60d6a5

But if that is not correct, then I can drop the regulator-off-in-suspend.
 
[...]

>> +
>> +	touchscreen@14 {
>> +		compatible = "goodix,gt917s";
> 
> This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
> 
> Goodix-TS 3-0014: ID 1158, version: 0100
> Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
> 
>

Same thing. I wasn't aware of this since your patch was using this compatible
string. If "goodix,gt1158" is the correct compatible string, then I agree we
should have that instead even when the firmware is missing. Because the DT is
supposed to describe the hardware. The FW issue can be tackled as a follow-up.

[...] 

>> +
>> +&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_CPLL>, <&cru DCLK_VOP0_FRAC>;
>> +};
> 
> So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
> -> DCLK_VOP0_FRAC -> DCLK_VOP0.
> 
> Fractional clocks require 20x difference between input and output rates, and
> CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
> correctly.
> 
> Even if this somehow works by fractional clock being bypassed, I did not design
> the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.
> 
> GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
> CPLL 800/74.25 = ~10.77441077441077441077
> 
> If you really want to use fractional clock, you'd need to parent it to VPLL
> and set VPLL really high, like close to 2GHz.
>

Thanks for the explanation. Then I just need to squash on top of this, the
following patch. Is that correct?

https://github.com/megous/linux/commit/f19ce7bb7d72

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 0/4] Add PinePhone Pro display support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

Hi!

> This series add support for the display present in the PinePhone Pro.
> 
> Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
> such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.
> 
> Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
> an entry for the driver in the MAINTAINERS file.
> 
> Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
> to enable both the display and the touchscreen. This makes the upstream DTS
> much more usable and will allow for example to enable support for the phone
> in the Fedora distribution.

Thanks for the series. Please cc: phone-devel@vger.kernel.org with
future patches.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v4 0/4] Add PinePhone Pro display support

[Javier Martinez Canillas]

Hello Pavel,

On 1/1/23 22:21, Pavel Machek wrote:
> Hi!
> 
>> This series add support for the display present in the PinePhone Pro.
>>
>> Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
>> such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.
>>
>> Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
>> an entry for the driver in the MAINTAINERS file.
>>
>> Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
>> to enable both the display and the touchscreen. This makes the upstream DTS
>> much more usable and will allow for example to enable support for the phone
>> in the Fedora distribution.
> 
> Thanks for the series. Please cc: phone-devel@vger.kernel.org with
> future patches.
>

Sure, I will.
 
> Best regards,
> 								Pavel

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

Hello Javier,

On Sat, Dec 31, 2022 at 04:29:49PM +0100, Javier Martinez Canillas wrote:
> Hello Ondřej,
> 
> Thanks a lot for your feedback.
> 
> On 12/30/22 16:37, Ondřej Jirman wrote:
> 
> [...]
> 
> >>  &i2c0 {
> >>  	clock-frequency = <400000>;
> >>  	i2c-scl-rising-time-ns = <168>;
> >> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
> >>  				regulator-name = "vcc3v0_touch";
> >>  				regulator-min-microvolt = <3000000>;
> >>  				regulator-max-microvolt = <3000000>;
> >> +				regulator-state-mem {
> >> +					regulator-off-in-suspend;
> >> +				};
> > 
> > You're instructing RK818 to shut down the regulator for touch controller during
> > suspend, but I think Goodix driver expects touch controller to be kept powered on
> > during suspend. Am I missing something?
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405
> >
> 
> You tell me, it is your patch :) I just cherry-picked this from your tree:

I have other patches to goodix driver that do power off the touch sensor chip
during sleep, so that it doesn't consume excessinve amounts of power when
the phone is suspended. Mainline doesn't. You have to adapt this to mainline,
because you're not upstreaming the required Goodix patches, for regulator-off-in-suspend
to not break things.

> https://github.com/megous/linux/commit/11f8da60d6a5
> 
> But if that is not correct, then I can drop the regulator-off-in-suspend.
>  
> [...]
> 
> >> +
> >> +	touchscreen@14 {
> >> +		compatible = "goodix,gt917s";
> > 
> > This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
> > 
> > Goodix-TS 3-0014: ID 1158, version: 0100
> > Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
> > 
> >
> 
> Same thing. I wasn't aware of this since your patch was using this compatible
> string. If "goodix,gt1158" is the correct compatible string, then I agree we
> should have that instead even when the firmware is missing. Because the DT is
> supposed to describe the hardware. The FW issue can be tackled as a follow-up.
> 
> [...] 

Yes, compatible string is sort of irrelevant, because the driver does runtime
auto-detection based on chip ID. I didn't bother with superficial issues in the
original code from Martijn/Kamil. Now that you're mainlining the code, this
should be sorted out, though.

There's no FW issue, I was just using the log to show you the actual chip ID the
driver detects.

(You should probably put my SoB after Kamil/Martijn, since I took the
maintenance/development of the driver after they wrote the base support
initially in secret. I'm not the original author of the code.)

> >> +
> >> +&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_CPLL>, <&cru DCLK_VOP0_FRAC>;
> >> +};
> > 
> > So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
> > -> DCLK_VOP0_FRAC -> DCLK_VOP0.
> > 
> > Fractional clocks require 20x difference between input and output rates, and
> > CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
> > correctly.
> > 
> > Even if this somehow works by fractional clock being bypassed, I did not design
> > the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.
> > 
> > GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
> > CPLL 800/74.25 = ~10.77441077441077441077
> > 
> > If you really want to use fractional clock, you'd need to parent it to VPLL
> > and set VPLL really high, like close to 2GHz.
> >
> 
> Thanks for the explanation. Then I just need to squash on top of this, the
> following patch. Is that correct?
> 
> https://github.com/megous/linux/commit/f19ce7bb7d72

Yes, and test the driver more thoroughly:

- look at clk_summary to verify clock rate the kernel thinks it's using
- test refresh rate, somehow, to again verify the actual clock rate (kernel can
  lie in debugfs)
- test power cycling the panel (eg. via system suspend/resume or other means)

thank you and kind regards,
	o.

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

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Ondřej,

On 1/2/23 11:57, Ondřej Jirman wrote:

[...]

>>
>> You tell me, it is your patch :) I just cherry-picked this from your tree:
> 
> I have other patches to goodix driver that do power off the touch sensor chip
> during sleep, so that it doesn't consume excessinve amounts of power when
> the phone is suspended. Mainline doesn't. You have to adapt this to mainline,
> because you're not upstreaming the required Goodix patches, for regulator-off-in-suspend
> to not break things.
> 
>> https://github.com/megous/linux/commit/11f8da60d6a5
>>
>> But if that is not correct, then I can drop the regulator-off-in-suspend.
>>

Ah, I see. Missed that. Then I guess that's better to drop the regulator-off-in-suspend
until the goodix driver patches are upstreamed.

>> [...]
>>
>>>> +
>>>> +	touchscreen@14 {
>>>> +		compatible = "goodix,gt917s";
>>>
>>> This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
>>>
>>> Goodix-TS 3-0014: ID 1158, version: 0100
>>> Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
>>>
>>>
>>
>> Same thing. I wasn't aware of this since your patch was using this compatible
>> string. If "goodix,gt1158" is the correct compatible string, then I agree we
>> should have that instead even when the firmware is missing. Because the DT is
>> supposed to describe the hardware. The FW issue can be tackled as a follow-up.
>>
>> [...] 
> 
> Yes, compatible string is sort of irrelevant, because the driver does runtime
> auto-detection based on chip ID. I didn't bother with superficial issues in the
> original code from Martijn/Kamil. Now that you're mainlining the code, this
> should be sorted out, though.
> 
> There's no FW issue, I was just using the log to show you the actual chip ID the
> driver detects.
>

Gotcha.
 
> (You should probably put my SoB after Kamil/Martijn, since I took the
> maintenance/development of the driver after they wrote the base support
> initially in secret. I'm not the original author of the code.)
>

I wasn't aware of that. I just kept the author field as it's in your tree.
 
[...]

>> https://github.com/megous/linux/commit/f19ce7bb7d72
> 
> Yes, and test the driver more thoroughly:
> 
> - look at clk_summary to verify clock rate the kernel thinks it's using
> - test refresh rate, somehow, to again verify the actual clock rate (kernel can
>   lie in debugfs)
> - test power cycling the panel (eg. via system suspend/resume or other means)
> 

Agreed that the more testing the better.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat