Re: [PATCH v2 1/2] drm/i915/icl: Factor out combo PHY lane power setup helper

Imre Deak
2019-04-30T12:48:40Z

On Tue, Apr 30, 2019 at 03:44:00PM +0300, Jani Nikula wrote:
> On Thu, 25 Apr 2019, Imre Deak <imre.deak@intel.com> wrote:
> > Factor out the combo PHY lane power configuration code to a separate
> > helper; it will be also needed by the next patch adding the same
> > configuration for DDI ports.
> >
> > Add support for DDI ports and lane reversal as preparation for the next
> > patch.
> >
> > The PWR_DOWN_LN_1 value is unspecified in the BSpec register description
> > so remove it.
> >
> > v2:
> > - Fix up the wrong assumption that the encodings are the same for DDI
> > and DSI ports. (Jani)
> >
>
> Both patches look good to me, but I'm afraid patch 1 conflicts with the
> header refactoring I pushed earlier, as well as the function name
> changes in [1]. I think I'd like the function here to be renamed
> accordingly.
>
> Other than that, for both,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks. Yes, that naming scheme makes sense, will do the rename before
pushing the patches.

>
> [1] http://patchwork.freedesktop.org/patch/msgid/20190430124128.23606-1-jani.nikula@intel.com
>
>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Madhav Chauhan <madhav.chauhan@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > drivers/gpu/drm/i915/i915_reg.h | 1 -
> > drivers/gpu/drm/i915/icl_dsi.c | 26 ++---------------
> > drivers/gpu/drm/i915/intel_combo_phy.c | 52 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 58 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index dc74d33c20aa..87f24d92e355 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3515,6 +3515,9 @@ void icl_combo_phys_init(struct drm_i915_private *dev_priv);
> > void icl_combo_phys_uninit(struct drm_i915_private *dev_priv);
> > void cnl_combo_phys_init(struct drm_i915_private *dev_priv);
> > void cnl_combo_phys_uninit(struct drm_i915_private *dev_priv);
> > +void icl_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> > + enum port port, bool is_dsi,
> > + int lane_count, bool lane_reversal);
> >
> > int intel_gpu_freq(struct drm_i915_private *dev_priv, int val);
> > int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b74824f0b5b1..e332b9f69a4a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1813,7 +1813,6 @@ enum i915_power_well_id {
> > #define PWR_DOWN_LN_3 (0x8 << 4)
> > #define PWR_DOWN_LN_2_1_0 (0x7 << 4)
> > #define PWR_DOWN_LN_1_0 (0x3 << 4)
> > -#define PWR_DOWN_LN_1 (0x2 << 4)
> > #define PWR_DOWN_LN_3_1 (0xa << 4)
> > #define PWR_DOWN_LN_3_1_0 (0xb << 4)
> > #define PWR_DOWN_LN_MASK (0xf << 4)
> > diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> > index 9d962ea1e635..f2113d3798b0 100644
> > --- a/drivers/gpu/drm/i915/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/icl_dsi.c
> > @@ -363,30 +363,10 @@ static void gen11_dsi_power_up_lanes(struct intel_encoder *encoder)
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> > enum port port;
> > - u32 tmp;
> > - u32 lane_mask;
> >
> > - switch (intel_dsi->lane_count) {
> > - case 1:
> > - lane_mask = PWR_DOWN_LN_3_1_0;
> > - break;
> > - case 2:
> > - lane_mask = PWR_DOWN_LN_3_1;
> > - break;
> > - case 3:
> > - lane_mask = PWR_DOWN_LN_3;
> > - break;
> > - case 4:
> > - default:
> > - lane_mask = PWR_UP_ALL_LANES;
> > - break;
> > - }
> > -
> > - for_each_dsi_port(port, intel_dsi->ports) {
> > - tmp = I915_READ(ICL_PORT_CL_DW10(port));
> > - tmp &= ~PWR_DOWN_LN_MASK;
> > - I915_WRITE(ICL_PORT_CL_DW10(port), tmp | lane_mask);
> > - }
> > + for_each_dsi_port(port, intel_dsi->ports)
> > + icl_combo_phy_power_up_lanes(dev_priv, port, true,
> > + intel_dsi->lane_count, false);
> > }
> >
> > static void gen11_dsi_config_phy_lanes_sequence(struct intel_encoder *encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c
> > index 2bf4359d7e41..5478808886f1 100644
> > --- a/drivers/gpu/drm/i915/intel_combo_phy.c
> > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c
> > @@ -203,6 +203,58 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> > return ret;
> > }
> >
> > +void icl_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> > + enum port port, bool is_dsi,
> > + int lane_count, bool lane_reversal)
> > +{
> > + u8 lane_mask;
> > + u32 val;
> > +
> > + if (is_dsi) {
> > + WARN_ON(lane_reversal);
> > +
> > + switch (lane_count) {
> > + case 1:
> > + lane_mask = PWR_DOWN_LN_3_1_0;
> > + break;
> > + case 2:
> > + lane_mask = PWR_DOWN_LN_3_1;
> > + break;
> > + case 3:
> > + lane_mask = PWR_DOWN_LN_3;
> > + break;
> > + default:
> > + MISSING_CASE(lane_count);
> > + /* fall-through */
> > + case 4:
> > + lane_mask = PWR_UP_ALL_LANES;
> > + break;
> > + }
> > + } else {
> > + switch (lane_count) {
> > + case 1:
> > + lane_mask = lane_reversal ? PWR_DOWN_LN_2_1_0 :
> > + PWR_DOWN_LN_3_2_1;
> > + break;
> > + case 2:
> > + lane_mask = lane_reversal ? PWR_DOWN_LN_1_0 :
> > + PWR_DOWN_LN_3_2;
> > + break;
> > + default:
> > + MISSING_CASE(lane_count);
> > + /* fall-through */
> > + case 4:
> > + lane_mask = PWR_UP_ALL_LANES;
> > + break;
> > + }
> > + }
> > +
> > + val = I915_READ(ICL_PORT_CL_DW10(port));
> > + val &= ~PWR_DOWN_LN_MASK;
> > + val |= lane_mask << PWR_DOWN_LN_SHIFT;
> > + I915_WRITE(ICL_PORT_CL_DW10(port), val);
> > +}
> > +
> > void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > {
> > enum port port;
>
> --
> Jani Nikula, Intel Open Source Graphics Center