Re: [DPU PATCH v3 3/5] drm/msm/dp: add displayPort driver support

[Rob Clark <robdclark@gmail.com>]

On Mon, Dec 2, 2019 at 5:48 AM Chandan Uddaraju <chandanu@codeaurora.org> wrote:
>
> Add the needed displayPort files to enable DP driver
> on msm target.
>
> "dp_display" module is the main module that calls into
> other sub-modules. "dp_drm" file represents the interface
> between DRM framework and DP driver.
>
> changes in v2:
> -- Update copyright markings on all relevant files.
> -- Change pr_err() to DRM_ERROR()
> -- Use APIs directly instead of function pointers.
> -- Use drm_display_mode structure to store link parameters in the driver.
> -- Use macros for register definitions instead of hardcoded values.
> -- Replace writel_relaxed/readl_relaxed with writel/readl
>    and remove memory barriers.
> -- Remove unnecessary NULL checks.
> -- Use drm helper functions for dpcd read/write.
> -- Use DRM_DEBUG_DP for debug msgs.
>
> changes in V3:
> -- Removed changes in dpu_io_util.[ch]
> -- Added locking around "is_connected" flag and removed atomic_set()
> -- Removed the argument validation checks in all the static functions
>    except initialization functions and few API calls across msm/dp files
> -- Removed hardcoded values for register reads/writes
> -- Removed vreg related generic structures.
> -- Added return values where ever necessary.
> -- Updated dp_ctrl_on function.
> -- Calling the ctrl specific catalog functions directly instead of
>    function pointers.
> -- Added seperate change that adds standard value in drm_dp_helper file.
> -- Added separate change in this list that is used to initialize
>    displayport in DPU driver.
> -- Added change to use drm_dp_get_adjust_request_voltage() function.
>
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> ---

[snip]

> +
> +void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
> +                                       u32 rate, u32 stream_rate_khz,
> +                                       bool fixed_nvid)
> +{
> +       u32 pixel_m, pixel_n;
> +       u32 mvid, nvid;
> +       u64 mvid_calc;
> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> +                               struct dp_catalog_private, dp_catalog);
> +
> +       if (fixed_nvid) {
> +               nvid = DP_LINK_CONSTANT_N_VALUE;
> +               DRM_DEBUG_DP("use fixed NVID=0x%x\n", nvid);
> +
> +               /*
> +                * For intermediate results, use 64 bit arithmetic to avoid
> +                * loss of precision.
> +                */
> +               mvid_calc = (u64) stream_rate_khz * nvid;
> +               mvid_calc = div_u64(mvid_calc, rate);
> +
> +               /*
> +                * truncate back to 32 bits as this final divided value will
> +                * always be within the range of a 32 bit unsigned int.
> +                */
> +               mvid = (u32) mvid_calc;
> +               DRM_DEBUG_DP("link rate=%dkbps, stream_rate_khz=%uKhz",
> +                       rate, stream_rate_khz);
> +       } else {
> +               pixel_m = dp_read_cc(catalog, MMSS_DP_PIXEL_M);
> +               pixel_n = dp_read_cc(catalog, MMSS_DP_PIXEL_N);

Can we just calculate m/n from the rate instead.  That gets rid of
having to ioremap() the dispcc region, which is really ugly.

BR,
-R

> +               DRM_DEBUG_DP("pixel_m=0x%x, pixel_n=0x%x\n", pixel_m, pixel_n);
> +
> +               mvid = (pixel_m & 0xFFFF) * 5;
> +               nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF);
> +
> +               DRM_DEBUG_DP("rate = %d\n", rate);
> +
> +               switch (drm_dp_link_rate_to_bw_code(rate)) {
> +               case DP_LINK_BW_5_4:
> +                       nvid *= 2;
> +                       break;
> +               case DP_LINK_BW_8_1:
> +                       nvid *= 3;
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +
> +       DRM_DEBUG_DP("mvid=0x%x, nvid=0x%x\n", mvid, nvid);
> +       dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid);
> +       dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid);
> +}