| Message ID | 20230327074136.1459212-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: dts: rk3399-pinephone-pro: Add internal display support | expand |
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.
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.
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
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.
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!
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 >
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.
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
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 > >
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 > >
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.
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";
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 >
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.
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 >
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. >
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"; +};