RFC: DSI panel lane frequency

Commit Message

Linus Walleij Oct. 16, 2018, 2:06 p.m. UTC

Hi folks,

I just randomly add some people that committed code to the
DSI core so I can get some reasonable feedback.

I started looking at some DSI drivers I'm adding and it seems
this platform (Ux500 MCDE) can control the bus frequency
of the DSI interface. It can be controlled independently for
command and video mode, and there is an LP (low power)
frequency and a HS (high speed) frequency for the lane.

The MIPI specification seems to say "The maximum Lane
frequency shall be documented by the DSI device manufacturer."
Then it goes on to specify tolerance for the HS and LP
frequency.

So apparently those are not standard frequencies.

I was thinking to add something like this:

LS/HS frequency? (If zero, we could assume some default
I guess.)

Yours,
Linus Walleij

Comments

Andrzej Hajda Oct. 16, 2018, 2:44 p.m. UTC | #1

On 16.10.2018 16:06, Linus Walleij wrote:
> Hi folks,
>
> I just randomly add some people that committed code to the
> DSI core so I can get some reasonable feedback.
>
> I started looking at some DSI drivers I'm adding and it seems
> this platform (Ux500 MCDE) can control the bus frequency
> of the DSI interface. It can be controlled independently for
> command and video mode, and there is an LP (low power)
> frequency and a HS (high speed) frequency for the lane.
>
> The MIPI specification seems to say "The maximum Lane
> frequency shall be documented by the DSI device manufacturer."
> Then it goes on to specify tolerance for the HS and LP
> frequency.
>
> So apparently those are not standard frequencies.
>
> I was thinking to add something like this:
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4fef19064b0f..9c78eb78b027 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -168,6 +168,8 @@ struct mipi_dsi_device_info {
>   * @format: pixel format for video mode
>   * @lanes: number of active data lanes
>   * @mode_flags: DSI operation mode related flags
> + * @hs_frequency: Maximum frequency for high speed operation
> + * @lp_frequency: Maximum frequency for low power operation

Yes, it is missing part, please add units (Hz ?) and  some optional
propositions:
1. maybe hs_speed instead of hs_frequency - shorter.
2. s/operation/mode/

>   */
>  struct mipi_dsi_device {
>         struct mipi_dsi_host *host;
> @@ -178,6 +180,8 @@ struct mipi_dsi_device {
>         unsigned int lanes;
>         enum mipi_dsi_pixel_format format;
>         unsigned long mode_flags;
> +       unsigned long hs_frequency;
> +       unsigned long lp_frequency;

I hope in case of Hz it will not reach MAX_ULONG.

>  };
>
> Is this what we should do to make DSI panels expose their max
> LS/HS frequency? (If zero, we could assume some default
> I guess.)

I guess one can assume sth based on display timings, but I am not sure
if it is proper way, temporary hosts should work as before if 0 is
specified, but new devices should specify HS speed explicitly IMO.
Regarding LP as I remember spec says sth about some fraction of HS, but
I wouldn't be against requiring it as well.

Regards
Andrzej

>
> Yours,
> Linus Walleij
>
>

Linus Walleij Oct. 17, 2018, 6:56 a.m. UTC | #2

On Tue, Oct 16, 2018 at 4:45 PM Andrzej Hajda <[e-mail]> wrote:

> Yes, it is missing part, please add units (Hz ?) and  some optional
> propositions:
> 1. maybe hs_speed instead of hs_frequency - shorter.
> 2. s/operation/mode/

OK I fix!

> > +       unsigned long hs_frequency;
> > +       unsigned long lp_frequency;
>
> I hope in case of Hz it will not reach MAX_ULONG.

It supports at least 4.3 GHz according to the standard.

The clock framework in <linux/clk.h> already has this
prototype:
unsigned long clk_get_rate(struct clk *clk);

So if we get frequencies above 4.3GHz we're gonna need to
refactor the whole kernel anyway. DSI speed is gonna be
the least of our problems.

> if it is proper way, temporary hosts should work as before if 0 is
> specified, but new devices should specify HS speed explicitly IMO.
> Regarding LP as I remember spec says sth about some fraction of HS, but
> I wouldn't be against requiring it as well.

OK I post some patch. I need this for my new driver which
has a bunch of out-of-tree display drivers that specify
HS and LP frequencies so let's add it before we accumulate
too much unspecified panels.

Yours,
Linus Walleij

Andrzej Hajda Oct. 17, 2018, 7:22 a.m. UTC | #3

On 17.10.2018 08:56, Linus Walleij wrote:
> On Tue, Oct 16, 2018 at 4:45 PM Andrzej Hajda <[e-mail]> wrote:
>
>> Yes, it is missing part, please add units (Hz ?) and  some optional
>> propositions:
>> 1. maybe hs_speed instead of hs_frequency - shorter.

Now I see some disadvantage of hs_speed - it suggests Mbps, not Hz,
maybe hs_rate ?
Up to you.

>> 2. s/operation/mode/
> OK I fix!
>
>>> +       unsigned long hs_frequency;
>>> +       unsigned long lp_frequency;
>> I hope in case of Hz it will not reach MAX_ULONG.
> It supports at least 4.3 GHz according to the standard.
>
> The clock framework in <linux/clk.h> already has this
> prototype:
> unsigned long clk_get_rate(struct clk *clk);
>
> So if we get frequencies above 4.3GHz we're gonna need to
> refactor the whole kernel anyway. DSI speed is gonna be
> the least of our problems.

We could just use u64 to avoid the issue.

Regards
Andrzej

>
>> if it is proper way, temporary hosts should work as before if 0 is
>> specified, but new devices should specify HS speed explicitly IMO.
>> Regarding LP as I remember spec says sth about some fraction of HS, but
>> I wouldn't be against requiring it as well.
> OK I post some patch. I need this for my new driver which
> has a bunch of out-of-tree display drivers that specify
> HS and LP frequencies so let's add it before we accumulate
> too much unspecified panels.
>
> Yours,
> Linus Walleij
>
>

Linus Walleij Oct. 17, 2018, 2:07 p.m. UTC | #4

On Wed, Oct 17, 2018 at 9:22 AM Andrzej Hajda <[e-mail]> wrote:
> On 17.10.2018 08:56, Linus Walleij wrote:
> > On Tue, Oct 16, 2018 at 4:45 PM Andrzej Hajda <[e-mail]> wrote:
> >
> >> Yes, it is missing part, please add units (Hz ?) and  some optional
> >> propositions:
> >> 1. maybe hs_speed instead of hs_frequency - shorter.
>
> Now I see some disadvantage of hs_speed - it suggests Mbps, not Hz,
> maybe hs_rate ?
> Up to you.

Makes sense. Fixed and sent out.

> > So if we get frequencies above 4.3GHz we're gonna need to
> > refactor the whole kernel anyway. DSI speed is gonna be
> > the least of our problems.
>
> We could just use u64 to avoid the issue.

I opted not to do that for this reason:

u64 foo = 1;
unsigned long bar = foo;

This will give compiler warnings on some 32bit architectures
and you have to explicitly:

unsigned long bar = (unsigned long)foo;

Which is gonna look really bad. Since this will in 10 cases out
of 10 be backed by the clk subsystem and <linux/clk.h> I think
we should not use any other type than what clk is using.
When the change, we change.

Yours,
Linus Walleij

Patch

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4fef19064b0f..9c78eb78b027 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -168,6 +168,8 @@  struct mipi_dsi_device_info {
  * @format: pixel format for video mode
  * @lanes: number of active data lanes
  * @mode_flags: DSI operation mode related flags
+ * @hs_frequency: Maximum frequency for high speed operation
+ * @lp_frequency: Maximum frequency for low power operation
  */
 struct mipi_dsi_device {
        struct mipi_dsi_host *host;
@@ -178,6 +180,8 @@  struct mipi_dsi_device {
        unsigned int lanes;
        enum mipi_dsi_pixel_format format;
        unsigned long mode_flags;
+       unsigned long hs_frequency;
+       unsigned long lp_frequency;
 };

Is this what we should do to make DSI panels expose their max