| Message ID | 20200316133503.144650-1-icenowy@aosc.io |
|---|---|
| Headers | show |
| Series | Add support for PinePhone LCD panel | expand |
Hello Icenowy, On Mon, Mar 16, 2020 at 09:35:01PM +0800, Icenowy Zheng wrote: > Xingbangda XBD599 is a 5.99" 720x1440 MIPI-DSI IPS LCD panel made by > Xingbangda, which is used on PinePhone final assembled phones. > > [snip] > > +static const struct drm_display_mode xbd599_default_mode = { > + .hdisplay = 720, > + .hsync_start = 720 + 40, > + .hsync_end = 720 + 40 + 40, > + .htotal = 720 + 40 + 40 + 40, > + .vdisplay = 1440, > + .vsync_start = 1440 + 18, > + .vsync_end = 1440 + 18 + 10, > + .vtotal = 1440 + 18 + 10 + 17, > + .vrefresh = 60, Does the display actually run for you at 60fps? I measured 36.63 FPS with a DRM app that just does DRM_IOCTL_MODE_ATOMIC update after each DRM_EVENT_FLIP_COMPLETE event. thank you and regards, o. > + .clock = 69000, > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > + > + .width_mm = 68, > + .height_mm = 136, > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > +}; > +
Hi, On Mon, Mar 16, 2020 at 09:35:02PM +0800, Icenowy Zheng wrote: > The max() function call in horizontal timing calculation shouldn't pad a > length already subtracted with overhead to overhead, instead it should > only prevent the set timing to underflow. Some explanation on why it shouldn't do that would be appreciated. Also, with the changes you have below, the comments above each max call don't match anymore and should be updated. Maxime
Hi Icenowy! Thanks for your patch. On Mon, Mar 16, 2020 at 2:37 PM Icenowy Zheng <icenowy@aosc.io> wrote: > Xingbangda XBD599 is a 5.99" 720x1440 MIPI-DSI IPS LCD panel made by > Xingbangda, which is used on PinePhone final assembled phones. > > Add support for it. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> (...) > +/* Manufacturer specific Commands send via DSI */ > +#define ST7703_CMD_ALL_PIXEL_OFF 0x22 > +#define ST7703_CMD_ALL_PIXEL_ON 0x23 > +#define ST7703_CMD_SETDISP 0xB2 > +#define ST7703_CMD_SETRGBIF 0xB3 > +#define ST7703_CMD_SETCYC 0xB4 > +#define ST7703_CMD_SETBGP 0xB5 > +#define ST7703_CMD_SETVCOM 0xB6 > +#define ST7703_CMD_SETOTP 0xB7 > +#define ST7703_CMD_SETPOWER_EXT 0xB8 > +#define ST7703_CMD_SETEXTC 0xB9 > +#define ST7703_CMD_SETMIPI 0xBA > +#define ST7703_CMD_SETVDC 0xBC > +#define ST7703_CMD_SETSCR 0xC0 > +#define ST7703_CMD_SETPOWER 0xC1 > +#define ST7703_CMD_UNK_C6 0xC6 > +#define ST7703_CMD_SETPANEL 0xCC > +#define ST7703_CMD_SETGAMMA 0xE0 > +#define ST7703_CMD_SETEQ 0xE3 > +#define ST7703_CMD_SETGIP1 0xE9 > +#define ST7703_CMD_SETGIP2 0xEA It appears that the Himax HX8363 is using the same display controller if you look at the datasheet: http://www.datasheet-pdf.com/PDF/HX8369-A-Datasheet-Himax-729024 There you find an explanation to some of the commands. People are trying to add support for this panel too: https://discuss.96boards.org/t/adding-support-to-himax-hx8363-panel/9068 The set-up values etc for the Himax display will be widely different because it is using the same display controller for a display of 480x800(854) pixels. You should definately insert code to read the MTP bytes: 0xDA manufacturer 0xDB driver version 0xDC LCD module/driver And print these, se e.g. my newly added NT35510 driver or the Sony ACX424AKP driver. Nobody seems to have any documentation of these MTP ID bytes but they are certainly consistent among e.g Ilitek or Novatek display drivers. I do not think that either Xingbangda or Himax have made this display controller. The number of advanced display controller vendors in the world is actually pretty limited. Ilitek, Novatek or TPO make most of them. It is a bit of a problem for the development world that display manufacturers hide the details of which display controller they actually use, because we can't have 2 different drivers for the same display controller just because Himax and Xingbada package it up with their own branding. This actually looks very much like an Ilitek display controller. Some commands are clearly identical to Ilitek ILI9342: http://www.ampdisplay.com/documents/pdf/ILI9342_DS_V008_20100331.pdf I would: 1. Try to determine what the actual display controller is. I think it is some Ilitek. 2. Write a panel-ilitek-ili9342.c (if that is the actual controller) and parameterize it for this display controller the same way we do in e.g. panel-novatek-nt35510.c or panel-ilitek-ili9322.c, so you use the compatible string to set up the actual per-display settings for this display controller. > + /* > + * Init sequence was supplied by the panel vendor. > + */ > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETEXTC, > + 0xF1, 0x12, 0x83); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETMIPI, > + 0x33, 0x81, 0x05, 0xF9, 0x0E, 0x0E, 0x20, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25, > + 0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4F, 0x11, > + 0x00, 0x00, 0x37); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER_EXT, > + 0x25, 0x22, 0x20, 0x03); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETRGBIF, > + 0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00, > + 0x00, 0x00); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETSCR, > + 0x73, 0x73, 0x50, 0x50, 0x00, 0xC0, 0x08, 0x70, > + 0x00); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETCYC, 0x80); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0xF0); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETEQ, > + 0x00, 0x00, 0x0B, 0x0B, 0x10, 0x10, 0x00, 0x00, > + 0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10); > + dsi_dcs_write_seq(dsi, 0xC6, 0x01, 0x00, 0xFF, 0xFF, 0x00); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER, > + 0x74, 0x00, 0x32, 0x32, 0x77, 0xF1, 0xFF, 0xFF, > + 0xCC, 0xCC, 0x77, 0x77); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP, 0x07, 0x07); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM, 0x2C, 0x2C); > + dsi_dcs_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00); > + > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP1, > + 0x82, 0x10, 0x06, 0x05, 0xA2, 0x0A, 0xA5, 0x12, > + 0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38, > + 0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00, > + 0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88, > + 0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64, > + 0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88, > + 0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP2, > + 0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88, > + 0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13, > + 0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88, > + 0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x0A, > + 0xA5, 0x00, 0x00, 0x00, 0x00); > + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGAMMA, > + 0x00, 0x09, 0x0D, 0x23, 0x27, 0x3C, 0x41, 0x35, > + 0x07, 0x0D, 0x0E, 0x12, 0x13, 0x10, 0x12, 0x12, > + 0x18, 0x00, 0x09, 0x0D, 0x23, 0x27, 0x3C, 0x41, > + 0x35, 0x07, 0x0D, 0x0E, 0x12, 0x13, 0x10, 0x12, > + 0x12, 0x18); Already just using the HX8363 datasheet you can turn this into something much more readable akin to how the NT35510 driver is done. > +static const struct drm_display_mode xbd599_default_mode = { > + .hdisplay = 720, > + .hsync_start = 720 + 40, > + .hsync_end = 720 + 40 + 40, > + .htotal = 720 + 40 + 40 + 40, > + .vdisplay = 1440, > + .vsync_start = 1440 + 18, > + .vsync_end = 1440 + 18 + 10, > + .vtotal = 1440 + 18 + 10 + 17, > + .vrefresh = 60, I think this vrefresh is going away soon. > + .clock = 69000, > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > + > + .width_mm = 68, > + .height_mm = 136, > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > +}; All of this as well as some of the initialization sequences should be per-variant data. (Switched by the compatible). Yours, Linus Walleij
在 2020-03-16星期一的 21:35 +0800，Icenowy Zheng写道： > PinePhone uses PWM backlight and a XBD599 LCD panel over DSI for > display. > > Add its device nodes. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > No changes in v2. > > .../dts/allwinner/sun50i-a64-pinephone.dtsi | 37 > +++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > index cefda145c3c9..96d9150423e0 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > @@ -16,6 +16,15 @@ aliases { > serial0 = &uart0; > }; > > + backlight: backlight { > + compatible = "pwm-backlight"; > + pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>; > + brightness-levels = <0 16 18 20 22 24 26 29 32 35 38 42 > 46 51 56 62 68 75 83 91 100>; Should I drop the 0 here and replace it with 14? I have heard community complaining about setting 0 to brightness make the screen black. (I think in this situation bl_power or blank the DSI panel can still totally shut down the backlight). > + default-brightness-level = <15>; > + enable-gpios = <&pio 7 10 GPIO_ACTIVE_HIGH>; /* PH10 */ > + power-supply = <®_ldo_io0>; > + }; > + > chosen { > stdout-path = "serial0:115200n8"; > }; > @@ -84,6 +93,30 @@ &dai { > status = "okay"; > }; > > +&de { > + status = "okay"; > +}; > + > +&dphy { > + status = "okay"; > +}; > + > +&dsi { > + vcc-dsi-supply = <®_dldo1>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "okay"; > + > + panel@0 { > + compatible = "xingbangda,xbd599"; > + reg = <0>; > + reset-gpios = <&pio 3 23 GPIO_ACTIVE_LOW>; /* PD23 */ > + iovcc-supply = <®_dldo2>; > + vcc-supply = <®_ldo_io0>; > + backlight = <&backlight>; > + }; > +}; > + > &ehci0 { > status = "okay"; > }; > @@ -188,6 +221,10 @@ &r_pio { > */ > }; > > +&r_pwm { > + status = "okay"; > +}; > + > &r_rsb { > status = "okay"; > > -- > 2.24.1 >
On Thu, Mar 19, 2020 at 10:51:36PM +0800, Icenowy Zheng wrote: > 在 2020-03-16星期一的 21:35 +0800，Icenowy Zheng写道： > > PinePhone uses PWM backlight and a XBD599 LCD panel over DSI for > > display. > > > > Add its device nodes. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > --- > > No changes in v2. > > > > .../dts/allwinner/sun50i-a64-pinephone.dtsi | 37 > > +++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > index cefda145c3c9..96d9150423e0 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > @@ -16,6 +16,15 @@ aliases { > > serial0 = &uart0; > > }; > > > > + backlight: backlight { > > + compatible = "pwm-backlight"; > > + pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>; > > + brightness-levels = <0 16 18 20 22 24 26 29 32 35 38 42 > > 46 51 56 62 68 75 83 91 100>; > > Should I drop the 0 here and replace it with 14? Almost all boards in arm/boot/dts start at 0. > I have heard community complaining about setting 0 to brightness make > the screen black. Level 0 (first value in the list) is special, and turns off the backlight: 123 if (brightness > 0) { 124 pwm_get_state(pb->pwm, &state); 125 state.duty_cycle = compute_duty_cycle(pb, brightness); 126 pwm_apply_state(pb->pwm, &state); 127 pwm_backlight_power_on(pb); 128 } else { 129 pwm_backlight_power_off(pb); 130 } o. > (I think in this situation bl_power or blank the DSI panel can still > totally shut down the backlight). > > > + default-brightness-level = <15>; > > + enable-gpios = <&pio 7 10 GPIO_ACTIVE_HIGH>; /* PH10 */ > > + power-supply = <®_ldo_io0>; > > + }; > > + > > chosen { > > stdout-path = "serial0:115200n8"; > > }; > > @@ -84,6 +93,30 @@ &dai { > > status = "okay"; > > }; > > > > +&de { > > + status = "okay"; > > +}; > > + > > +&dphy { > > + status = "okay"; > > +}; > > + > > +&dsi { > > + vcc-dsi-supply = <®_dldo1>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "okay"; > > + > > + panel@0 { > > + compatible = "xingbangda,xbd599"; > > + reg = <0>; > > + reset-gpios = <&pio 3 23 GPIO_ACTIVE_LOW>; /* PD23 */ > > + iovcc-supply = <®_dldo2>; > > + vcc-supply = <®_ldo_io0>; > > + backlight = <&backlight>; > > + }; > > +}; > > + > > &ehci0 { > > status = "okay"; > > }; > > @@ -188,6 +221,10 @@ &r_pio { > > */ > > }; > > > > +&r_pwm { > > + status = "okay"; > > +}; > > + > > &r_rsb { > > status = "okay"; > > > > -- > > 2.24.1 > > >
So following up on this: We should state in the commit message that this driver is for all displays using the Sitronix ST770x display controllers. The driver should be named panel-sitronix-st770x.c. On Thu, Mar 19, 2020 at 3:08 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > +/* Manufacturer specific Commands send via DSI */ > > +#define ST7703_CMD_ALL_PIXEL_OFF 0x22 > > +#define ST7703_CMD_ALL_PIXEL_ON 0x23 > > +#define ST7703_CMD_SETDISP 0xB2 > > +#define ST7703_CMD_SETRGBIF 0xB3 > > +#define ST7703_CMD_SETCYC 0xB4 > > +#define ST7703_CMD_SETBGP 0xB5 > > +#define ST7703_CMD_SETVCOM 0xB6 > > +#define ST7703_CMD_SETOTP 0xB7 > > +#define ST7703_CMD_SETPOWER_EXT 0xB8 > > +#define ST7703_CMD_SETEXTC 0xB9 > > +#define ST7703_CMD_SETMIPI 0xBA > > +#define ST7703_CMD_SETVDC 0xBC > > +#define ST7703_CMD_SETSCR 0xC0 > > +#define ST7703_CMD_SETPOWER 0xC1 > > +#define ST7703_CMD_UNK_C6 0xC6 > > +#define ST7703_CMD_SETPANEL 0xCC > > +#define ST7703_CMD_SETGAMMA 0xE0 > > +#define ST7703_CMD_SETEQ 0xE3 > > +#define ST7703_CMD_SETGIP1 0xE9 > > +#define ST7703_CMD_SETGIP2 0xEA I should have seen the ST7703 prefix shouldn't I... > This actually looks very much like an Ilitek display controller. > Some commands are clearly identical to Ilitek ILI9342: > http://www.ampdisplay.com/documents/pdf/ILI9342_DS_V008_20100331.pdf I'm still wondering about the apparent similarity between ST770x and Ilitek ILI9342, haha :D > 1. Try to determine what the actual display controller > is. I think it is some Ilitek. OK so this is Sitronix ST770x. > 2. Write a panel-ilitek-ili9342.c (if that is the actual controller) > and parameterize it for this display controller the same > way we do in e.g. panel-novatek-nt35510.c or > panel-ilitek-ili9322.c, so you use the compatible string > to set up the actual per-display settings for this display > controller. This should be panel-sitronix-st770x.c Yours, Linus Walleij