[v2] drm/panel: Set max rate for Ilitek ILI9881C

After adding the hs_rate and lp_rate fields to the DSI device
we need to populate these accordingly so display drivers can
respect them.

This figure for HS rate comes from the ILI9881C manual, the
calculation is explained in the comment.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Add LP speed after Andrzej's observation.
- Collect Maxime's ACK (hope it's fine also with the LP speed)
---
 drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

On 18.11.2018 22:31, Linus Walleij wrote:
> After adding the hs_rate and lp_rate fields to the DSI device
> we need to populate these accordingly so display drivers can
> respect them.
>
> This figure for HS rate comes from the ILI9881C manual, the
> calculation is explained in the comment.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Add LP speed after Andrzej's observation.
> - Collect Maxime's ACK (hope it's fine also with the LP speed)
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> index 3ad4a46c4e94..bd276c666318 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> @@ -465,6 +465,19 @@ static int ili9881c_dsi_probe(struct mipi_dsi_device *dsi)
>  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
>  	dsi->lanes = 4;
> +	/*
> +	 * The datasheet (table 39) specifies "limited clock channel speed"
> +	 * for 4 lanes as 550 Mbps for RGB888. As this is 4 bits at the time,
> +	 * the maximum HS frequency is 550/4 = 137.5 MHz.
> +	 */
> +	dsi->hs_rate = 137500000;
> +	/*
> +	 * Table 42 says: "Length of LP-00, LP-01, LP-10 or LP-11 periods":
> +	 * min 50ns, max 75ns. 1/50ns = max LPM rate = 20.000.000Hz.
> +	 * Since LP transmissions are to periods per bit, this should be


s/to/two/


Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> +	 * 10 MHz.
> +	 */
> +	dsi->lp_rate = 10000000;
>  
>  	return mipi_dsi_attach(dsi);
>  }

On 18.11.2018 22:31, Linus Walleij wrote:
> After adding the hs_rate and lp_rate fields to the DSI device
> we need to populate these accordingly so display drivers can
> respect them.
>
> This figure for HS rate comes from the ILI9881C manual, the
> calculation is explained in the comment.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Add LP speed after Andrzej's observation.
> - Collect Maxime's ACK (hope it's fine also with the LP speed)
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> index 3ad4a46c4e94..bd276c666318 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> @@ -465,6 +465,19 @@ static int ili9881c_dsi_probe(struct mipi_dsi_device *dsi)
>  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
>  	dsi->lanes = 4;
> +	/*
> +	 * The datasheet (table 39) specifies "limited clock channel speed"
> +	 * for 4 lanes as 550 Mbps for RGB888. As this is 4 bits at the time,
> +	 * the maximum HS frequency is 550/4 = 137.5 MHz.
> +	 */

Are you sure it is OK?

550Mbps is per lane, so since DSI is DoubleDataRate clock should be the
half of the speed - 275MHz.

Anyway more I think about it more doubts I have. hs_rate can be helpful
for command mode panels - panel refresh rate (provided by timings)
imposes only lower limit on the speed, max hs rate will impose upper limit.

In case of video mode, timings determines exactly value of the clock, so
max hs rate is unnecessary, am I right?


Regards

Andrzej



> +	dsi->hs_rate = 137500000;
> +	/*
> +	 * Table 42 says: "Length of LP-00, LP-01, LP-10 or LP-11 periods":
> +	 * min 50ns, max 75ns. 1/50ns = max LPM rate = 20.000.000Hz.
> +	 * Since LP transmissions are to periods per bit, this should be
> +	 * 10 MHz.
> +	 */
> +	dsi->lp_rate = 10000000;
>  
>  	return mipi_dsi_attach(dsi);
>  }

On Tue, Nov 20, 2018 at 12:02 PM Andrzej Hajda <a.hajda@samsung.com> wrote:

> > +     /*
> > +      * The datasheet (table 39) specifies "limited clock channel speed"
> > +      * for 4 lanes as 550 Mbps for RGB888. As this is 4 bits at the time,
> > +      * the maximum HS frequency is 550/4 = 137.5 MHz.
> > +      */
>
> Are you sure it is OK?
>
> 550Mbps is per lane, so since DSI is DoubleDataRate clock should be the
> half of the speed - 275MHz.

That is a bit confusing indeed.

> Anyway more I think about it more doubts I have. hs_rate can be helpful
> for command mode panels - panel refresh rate (provided by timings)
> imposes only lower limit on the speed, max hs rate will impose upper limit.

Yeah. I added it for the command mode-only Samsung s6d16d0.

> In case of video mode, timings determines exactly value of the clock, so
> max hs rate is unnecessary, am I right?

As far as I can tell, yes, provided that the video mode timings are
<= HS rate. (Here it would mainly be a safeguard/sanity check.)

I wonder what happens with a video mode panel in LP mode
really. It would be good to get feedback from some experts
here.

I assume that in practice in LP mode the frequency is set
down and with the rest of the frame properties remaining
the same, the frame rate will go down (as expected).

So the relationship between mode clock, HS and LP in
practice is a bit of mystery to me. I haven't seen anything
in the upstream kernel making it possible for panels to
enter LP mode, so I assume that is only done by vendor
out-of-tree code as of today.

It would be great if some people who worked with this in
practice could provide some input.

My naive idea is that the panel should provide two
"resolutions", one for HS and one for LP, and they only
really differ in frame rate. So then userspace choose the
resolution with the lower frame rate to save power.

Yours,
Linus Walleij

On 20.11.2018 13:45, Linus Walleij wrote:
> On Tue, Nov 20, 2018 at 12:02 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>>> +     /*
>>> +      * The datasheet (table 39) specifies "limited clock channel speed"
>>> +      * for 4 lanes as 550 Mbps for RGB888. As this is 4 bits at the time,
>>> +      * the maximum HS frequency is 550/4 = 137.5 MHz.
>>> +      */
>> Are you sure it is OK?
>>
>> 550Mbps is per lane, so since DSI is DoubleDataRate clock should be the
>> half of the speed - 275MHz.
> That is a bit confusing indeed.
>
>> Anyway more I think about it more doubts I have. hs_rate can be helpful
>> for command mode panels - panel refresh rate (provided by timings)
>> imposes only lower limit on the speed, max hs rate will impose upper limit.
> Yeah. I added it for the command mode-only Samsung s6d16d0.
>
>> In case of video mode, timings determines exactly value of the clock, so
>> max hs rate is unnecessary, am I right?
> As far as I can tell, yes, provided that the video mode timings are
> <= HS rate. (Here it would mainly be a safeguard/sanity check.)
>
> I wonder what happens with a video mode panel in LP mode
> really. It would be good to get feedback from some experts
> here.
>
> I assume that in practice in LP mode the frequency is set
> down and with the rest of the frame properties remaining
> the same, the frame rate will go down (as expected).


Nope, LP is used mainly to send control (ie. no data) commands, so there
are no special video timings for it.

In some rare cases it could be used to send video data - partial
updates, very low-res/low-rate screens.

LP is used mainly to configuration (for example tc358764 DSI-LVDS
bridge), or to read from device - since HS is only unidirectional.

It can be used only if there is no HS transmission - display off, or in
vblank. In latter case DSI controller has often some logic preventing
sending LP commands in non-vblank periods.


>
> So the relationship between mode clock, HS and LP in
> practice is a bit of mystery to me. I haven't seen anything
> in the upstream kernel making it possible for panels to
> enter LP mode, so I assume that is only done by vendor
> out-of-tree code as of today.


Every read command from panel is in LP for example.


>
> It would be great if some people who worked with this in
> practice could provide some input.
>
> My naive idea is that the panel should provide two
> "resolutions", one for HS and one for LP, and they only
> really differ in frame rate. So then userspace choose the
> resolution with the lower frame rate to save power.


As I said earlier, LP is for non-video transmissions.


Regards

Andrzej


>
> Yours,
> Linus Walleij
>
>

On Tue, Nov 20, 2018 at 12:02 PM Andrzej Hajda <a.hajda@samsung.com> wrote:

> Anyway more I think about it more doubts I have. hs_rate can be helpful
> for command mode panels - panel refresh rate (provided by timings)
> imposes only lower limit on the speed, max hs rate will impose upper limit.
>
> In case of video mode, timings determines exactly value of the clock, so
> max hs rate is unnecessary, am I right?

I looked closer at the code for a particular DSI panel, the Sony
ACX424AKP.

This panel can be used in both command and video mode.

In the command mode, the panel support much higher speed,
but in video mode it is restricted to a lower speed by the
resolution timings:

#define VID_MODE_REFRESH_RATE   60
#define DSI_HS_FREQ_HZ_VID      330000000
#define DSI_HS_FREQ_HZ_CMD      420160000
#define DSI_LP_FREQ_HZ          19200000

(From outoftree code as usual, I might make a proper panel
driver for this display too...)

So I think there is a need for both, especially for this kind
of dual-mode panels, but hs_rate is likely always
>= the vide mode rate.

Yours,
Linus Walleij