On Thu, Apr 25, 2019 at 11:30:36AM +0300, Jani Nikula wrote:
> On Wed, 24 Apr 2019, Imre Deak <imre.deak@xxxxxxxxx> 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.
> >
> > While at it also add support to handle lane reversal which wasn't
> > needed for DSI, but will be needed by DDI ports.
> >
> > Also, remove the macros for the power down flags, they aren't
> > needed any more since we now calculate the power down mask. Many of
> > those macro values (mostly the ones for the currently unused reversed
> > lane configs) were actually undefined in the spec and didn't make much
> > sense either.
>
> Only PWR_DOWN_LN_1 is undefined in the spec.
Arg, yes I missed the DDI/DSI difference. Not sure how since it's quite
close together in the spec and the existing 2 and 3 lane DSI cases in the
code should've made it clear that there is a difference.
>
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > drivers/gpu/drm/i915/i915_reg.h | 9 ---------
> > drivers/gpu/drm/i915/icl_dsi.c | 26 +++-----------------------
> > drivers/gpu/drm/i915/intel_combo_phy.c | 31 +++++++++++++++++++++++++++++++
> > 4 files changed, 37 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index dc74d33c20aa..1207ef080aae 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, 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..29f16bc40b0a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1807,15 +1807,6 @@ enum i915_power_well_id {
> > #define PG_SEQ_DELAY_OVERRIDE_MASK (3 << 25)
> > #define PG_SEQ_DELAY_OVERRIDE_SHIFT 25
> > #define PG_SEQ_DELAY_OVERRIDE_ENABLE (1 << 24)
> > -#define PWR_UP_ALL_LANES (0x0 << 4)
> > -#define PWR_DOWN_LN_3_2_1 (0xe << 4)
> > -#define PWR_DOWN_LN_3_2 (0xc << 4)
> > -#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)
> > #define PWR_DOWN_LN_SHIFT 4
> >
> > diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> > index 9d962ea1e635..88959517b668 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);
> > - }
>
> I look at that and it takes me maybe 10 seconds to figure out what's
> going on, assuming the PWR_DOWN_LN_* macros are right.
>
> > + for_each_dsi_port(port, intel_dsi->ports)
> > + icl_combo_phy_power_up_lanes(dev_priv, port,
> > + 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..e3ed584eca47 100644
> > --- a/drivers/gpu/drm/i915/intel_combo_phy.c
> > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c
> > @@ -203,6 +203,37 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> > return ret;
> > }
> >
> > +static uint8_t reverse_nibble_bits(uint8_t val)
> > +{
> > +#define MOVBIT(v, from, to) (!!((v) & BIT(from)) << (to))
> > +#define SWPBIT(v, b1, b2) (MOVBIT((v), (b1), (b2)) | MOVBIT((v), (b2), (b1)))
> > +
> > + return SWPBIT(val, 0, 3) | SWPBIT(val, 1, 2);
> > +#undef SWPBIT
> > +#undef MOVBIT
> > +}
> > +
> > +void icl_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> > + enum port port, int lane_count,
> > + bool lane_reversal)
> > +{
> > + u32 pwr_down_mask;
> > + u32 val;
> > +
> > + WARN_ON((u32)lane_count > 4);
> > +
> > + pwr_down_mask = BIT(lane_count) - 1;
> > + if (lane_reversal)
> > + pwr_down_mask = reverse_nibble_bits(pwr_down_mask);
> > +
> > + pwr_down_mask = ~pwr_down_mask & 0xf;
> > +
> > + val = I915_READ(ICL_PORT_CL_DW10(port));
> > + val &= ~PWR_DOWN_LN_MASK;
> > + I915_WRITE(ICL_PORT_CL_DW10(port),
> > + val | (pwr_down_mask << PWR_DOWN_LN_SHIFT));
> > +}
>
> Maybe I'm slow today, but it took me 15-30 minutes to figure out that
> this screws up the 1 and 2 lane DSI values.
>
> The unshifted value changes:
>
> DSI x1: 1011b -> 1110b
> DSI x2: 1010b -> 1100b
>
> DSI uses different values from DDI.
Yep, sorry for that.
> There *is* value in keeping it simple and mechanic instead of trying to
> force it into a formula.
Yes, agreed, keeping the current macros and selecting the correct one
based on dsi/ddi seems to be clearer.
>
>
> BR,
> Jani.
>
>
> > +
> > void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > {
> > enum port port;
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
