Re: [PATCH v5 2/3] drm/msm/dp: parser data-lanes and link-frequencies from endpoint node

[Dmitry Baryshkov]

On Wed, 30 Nov 2022 at 02:12, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
>
> Both data-lanes and link-frequencies are property of endpoint. This
> patch parser endpoint to retrieve max data lanes and max link rate
> supported specified at dp_out endpoint. In the case where no endpoint
> specified, then 4 data lanes with HBR2 link rate (5.4G) will be the
> default link configuration.

So, you have two changes in a single patch.
1) Moving the data-lanes to the endpoint
2) Adding link-frequencies.

Please split the patch accordingly. Also keep in mind that you have to
provide backwards compatibility for the data-lanes property.

>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dp/dp_parser.c | 34 ++++++++++++++++++++++++++--------
>  drivers/gpu/drm/msm/dp/dp_parser.h |  2 ++
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index dd73221..9367f8c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -94,16 +94,34 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
>  static int dp_parser_misc(struct dp_parser *parser)
>  {
>         struct device_node *of_node = parser->pdev->dev.of_node;
> -       int len;
> -
> -       len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
> -       if (len < 0) {
> -               DRM_WARN("Invalid property \"data-lanes\", default max DP lanes = %d\n",
> -                        DP_MAX_NUM_DP_LANES);
> -               len = DP_MAX_NUM_DP_LANES;
> +       struct device_node *endpoint;
> +       int cnt;
> +       u64 frequence[4];

frequency

> +
> +       endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */
> +       if (endpoint) {
> +               cnt = of_property_count_u32_elems(endpoint, "data-lanes");
> +               if (cnt < 0)
> +                       parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> +               else
> +                       parser->max_dp_lanes = cnt;
> +
> +               cnt = of_property_count_u64_elems(endpoint, "link-frequencies");
> +               if (cnt < 0) {
> +                       parser->max_dp_link_rate = DP_LINK_FREQUENCY_HBR2; /* 54000 khz */

Wrong number of zeroes

> +               } else {
> +                       if (cnt > 4)    /* 4 frequency at most */
> +                               cnt = 4;

'4 frequencies'. Not to mention that magic '4' should be defined
somewhere. Or removed completely. See below.

> +                       of_property_read_u64_array(endpoint, "link-frequencies", frequence, cnt);

Can you please use of_property_read_u64_index() instead? It also has a
nice feature of modifying the out_value only if the proper data was
found. So you can set the default and then override it with the
of_property_read function. And then divide it by 1000 to get the value
in KHz.

> +                       parser->max_dp_link_rate = (u32)frequence[cnt  -1];
> +                       parser->max_dp_link_rate /= 1000;       /* khz */

The HDR3 rate is 8100 Mb/s. 8 100 000 000. This doesn't fit into u32
(U32_MAX = 4 294 967 295).

> +               }
> +       } else {
> +               /* default */
> +               parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> +               parser->max_dp_link_rate = DP_LINK_FREQUENCY_HBR2; /* 54000 khz */

Wrong number of zeroes. Better use Mb/s or Gb/s directly. Also it is a
rate, not a frequency, so the define should also use 'RATE' in its
name.

>         }
>
> -       parser->max_dp_lanes = len;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 866c1a8..76ddb751 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -15,6 +15,7 @@
>  #define DP_LABEL "MDSS DP DISPLAY"
>  #define DP_MAX_PIXEL_CLK_KHZ   675000
>  #define DP_MAX_NUM_DP_LANES    4
> +#define DP_LINK_FREQUENCY_HBR2 540000
>
>  enum dp_pm_type {
>         DP_CORE_PM,
> @@ -119,6 +120,7 @@ struct dp_parser {
>         struct dp_io io;
>         struct dp_display_data disp_data;
>         u32 max_dp_lanes;
> +       u32 max_dp_link_rate;
>         struct drm_bridge *next_bridge;
>
>         int (*parse)(struct dp_parser *parser);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


--
With best wishes

Dmitry