On Fri, 2019-09-13 at 23:24 -0700, Lucas De Marchi wrote:
> On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
> <jose.souza@xxxxxxxxx> wrote:
> > If platform supports and has modular FIA is enabled, the registers
> > bits also change, example: reading TC3 registers with modular FIA
> > enabled, driver should read from FIA2 but with TC1 bits offsets.
> >
> > It is described in BSpec 50231 for DFLEXDPSP, other registers don't
> > have the BSpec description but testing in real hardware have proven
> > that it had moved for all other registers too.
> >
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 2 +
> > drivers/gpu/drm/i915/display/intel_tc.c | 52 ++++++++++++--
> > ------
> > drivers/gpu/drm/i915/display/intel_tc.h | 2 +
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > drivers/gpu/drm/i915/i915_reg.h | 45 ++++++++------
> > ---
> > 5 files changed, 61 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4e001113e828..cd0a248bfe49 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15384,6 +15384,8 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> > if (!HAS_DISPLAY(dev_priv))
> > return;
> >
> > + intel_tc_init(dev_priv);
> > +
> > if (INTEL_GEN(dev_priv) >= 12) {
> > /* TODO: initialize TC ports as well */
> > intel_ddi_init(dev_priv, PORT_A);
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 3a7302e360cc..fc5d0e73cf21 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -23,19 +23,21 @@ static const char *tc_port_mode_name(enum
> > tc_port_mode mode)
> > return names[mode];
> > }
> >
> > -static bool has_modular_fia(struct drm_i915_private *i915)
> > +void intel_tc_init(struct drm_i915_private *i915)
> > {
> > + u32 val;
> > +
> > if (!INTEL_INFO(i915)->display.has_modular_fia)
> > - return false;
> > + return;
> >
> > - return intel_uncore_read(&i915->uncore,
> > - PORT_TX_DFLEXDPSP(FIA1)) &
> > MODULAR_FIA_MASK;
> > + val = intel_uncore_read(&i915->uncore,
> > PORT_TX_DFLEXDPSP(FIA1));
> > + i915->has_modular_fia = val & MODULAR_FIA_MASK;
>
> Not a fan of stuffing tc-private details in i195 struct. Since this
> is
> only executed on initialization maybe it's
> not worth the few register reads we spare. If we go with this
> approach, then I think we should not use
> "has_" prefix so we don't get confused by the device_info fields.
>
> > }
> >
> > static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
> > enum tc_port tc_port)
> > {
> > - if (!has_modular_fia(i915))
> > + if (!i915->has_modular_fia)
> > return FIA1;
> >
> > /*
> > @@ -51,14 +53,15 @@ u32 intel_tc_port_get_lane_mask(struct
> > intel_digital_port *dig_port)
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > struct intel_uncore *uncore = &i915->uncore;
> > u32 lane_mask;
> > + bool mod_fia = i915->has_modular_fia;
>
> s/mod_fia/modular_fia/ or just don't add the additional var
>
> > lane_mask = intel_uncore_read(uncore,
> > PORT_TX_DFLEXDPSP(dig_port-
> > >tc_phy_fia));
> >
> > WARN_ON(lane_mask == 0xffffffff);
> >
> > - return (lane_mask & DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> > + return (lane_mask & DP_LANE_ASSIGNMENT_MASK(mod_fia,
> > tc_port)) >>
> > + DP_LANE_ASSIGNMENT_SHIFT(mod_fia, tc_port);
> > }
> >
> > u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> > @@ -66,6 +69,7 @@ u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> > struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > struct intel_uncore *uncore = &i915->uncore;
> > + bool mod_fia = i915->has_modular_fia;
> > u32 pin_mask;
> >
> > pin_mask = intel_uncore_read(uncore,
> > @@ -73,8 +77,8 @@ u32 intel_tc_port_get_pin_assignment_mask(struct
> > intel_digital_port *dig_port)
> >
> > WARN_ON(pin_mask == 0xffffffff);
> >
> > - return (pin_mask & DP_PIN_ASSIGNMENT_MASK(tc_port)) >>
> > - DP_PIN_ASSIGNMENT_SHIFT(tc_port);
> > + return (pin_mask & DP_PIN_ASSIGNMENT_MASK(mod_fia,
> > tc_port)) >>
> > + DP_PIN_ASSIGNMENT_SHIFT(mod_fia, tc_port);
> > }
> >
> > int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > *dig_port)
> > @@ -114,25 +118,28 @@ void intel_tc_port_set_fia_lane_count(struct
> > intel_digital_port *dig_port,
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > bool lane_reversal = dig_port->saved_port_bits &
> > DDI_BUF_PORT_REVERSAL;
> > struct intel_uncore *uncore = &i915->uncore;
> > + bool mod_fia = i915->has_modular_fia;
> > u32 val;
> >
> > WARN_ON(lane_reversal && dig_port->tc_mode !=
> > TC_PORT_LEGACY);
> >
> > val = intel_uncore_read(uncore,
> > PORT_TX_DFLEXDPMLE1(dig_port-
> > >tc_phy_fia));
> > - val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
> > + val &= ~DFLEXDPMLE1_DPMLETC_MASK(mod_fia, tc_port);
> >
> > switch (required_lanes) {
> > case 1:
> > - val |= lane_reversal ?
> > DFLEXDPMLE1_DPMLETC_ML3(tc_port) :
> > - DFLEXDPMLE1_DPMLETC_ML0(tc_port);
> > + val |= lane_reversal ?
> > + DFLEXDPMLE1_DPMLETC_ML3(mod_fia, tc_port) :
> > + DFLEXDPMLE1_DPMLETC_ML0(mod_fia, tc_port);
> > break;
> > case 2:
> > - val |= lane_reversal ?
> > DFLEXDPMLE1_DPMLETC_ML3_2(tc_port) :
> > - DFLEXDPMLE1_DPMLETC_ML1_0(tc_port);
> > + val |= lane_reversal ?
> > + DFLEXDPMLE1_DPMLETC_ML3_2(mod_fia, tc_port)
> > :
> > + DFLEXDPMLE1_DPMLETC_ML1_0(mod_fia,
> > tc_port);
> > break;
> > case 4:
> > - val |= DFLEXDPMLE1_DPMLETC_ML3_0(tc_port);
> > + val |= DFLEXDPMLE1_DPMLETC_ML3_0(mod_fia, tc_port);
> > break;
> > default:
> > MISSING_CASE(required_lanes);
> > @@ -180,9 +187,9 @@ static u32 tc_port_live_status_mask(struct
> > intel_digital_port *dig_port)
> > return mask;
> > }
> >
> > - if (val & TC_LIVE_STATE_TBT(tc_port))
> > + if (val & TC_LIVE_STATE_TBT(i915->has_modular_fia,
> > tc_port))
> > mask |= BIT(TC_PORT_TBT_ALT);
> > - if (val & TC_LIVE_STATE_TC(tc_port))
> > + if (val & TC_LIVE_STATE_TC(i915->has_modular_fia, tc_port))
> > mask |= BIT(TC_PORT_DP_ALT);
> >
> > if (intel_uncore_read(uncore, SDEISR) &
> > SDE_TC_HOTPLUG_ICP(tc_port))
> > @@ -200,6 +207,7 @@ static bool icl_tc_phy_status_complete(struct
> > intel_digital_port *dig_port)
> > struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > struct intel_uncore *uncore = &i915->uncore;
> > + bool mod_fia = i915->has_modular_fia;
> > u32 val;
> >
> > val = intel_uncore_read(uncore,
> > @@ -210,7 +218,7 @@ static bool icl_tc_phy_status_complete(struct
> > intel_digital_port *dig_port)
> > return false;
> > }
> >
> > - return val & DP_PHY_MODE_STATUS_COMPLETED(tc_port);
> > + return val & DP_PHY_MODE_STATUS_COMPLETED(mod_fia,
> > tc_port);
> > }
> >
> > static bool icl_tc_phy_set_safe_mode(struct intel_digital_port
> > *dig_port,
> > @@ -219,6 +227,7 @@ static bool icl_tc_phy_set_safe_mode(struct
> > intel_digital_port *dig_port,
> > struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > struct intel_uncore *uncore = &i915->uncore;
> > + bool mod_fia = i915->has_modular_fia;
> > u32 val;
> >
> > val = intel_uncore_read(uncore,
> > @@ -231,9 +240,9 @@ static bool icl_tc_phy_set_safe_mode(struct
> > intel_digital_port *dig_port,
> > return false;
> > }
> >
> > - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia, tc_port);
> > if (!enable)
> > - val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia,
> > tc_port);
> >
> > intel_uncore_write(uncore,
> > PORT_TX_DFLEXDPCSSS(dig_port-
> > >tc_phy_fia), val);
> > @@ -250,6 +259,7 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > intel_digital_port *dig_port)
> > struct drm_i915_private *i915 = to_i915(dig_port-
> > >base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(i915, dig_port-
> > >base.port);
> > struct intel_uncore *uncore = &i915->uncore;
> > + bool mod_fia = i915->has_modular_fia;
> > u32 val;
> >
> > val = intel_uncore_read(uncore,
> > @@ -260,7 +270,7 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > intel_digital_port *dig_port)
> > return true;
> > }
> >
> > - return !(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port));
> > + return !(val & DP_PHY_MODE_STATUS_NOT_SAFE(mod_fia,
> > tc_port));
> > }
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> > b/drivers/gpu/drm/i915/display/intel_tc.h
> > index 463f1b3c836f..2374352d7c31 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> > @@ -10,6 +10,7 @@
> > #include <linux/types.h>
> >
> > struct intel_digital_port;
> > +struct drm_i915_private;
> >
> > bool intel_tc_port_connected(struct intel_digital_port *dig_port);
> > u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> > *dig_port);
> > @@ -27,5 +28,6 @@ void intel_tc_port_put_link(struct
> > intel_digital_port *dig_port);
> > bool intel_tc_port_ref_held(struct intel_digital_port *dig_port);
> >
> > void intel_tc_port_init(struct intel_digital_port *dig_port, bool
> > is_legacy);
> > +void intel_tc_init(struct drm_i915_private *dev_priv);
> >
> > #endif /* __INTEL_TC_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index bf600888b3f1..a36d1929fde1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1765,6 +1765,9 @@ struct drm_i915_private {
> > /* Mutex to protect the above hdcp component related
> > values. */
> > struct mutex hdcp_comp_mutex;
> >
> > + /* Monolithic or modular FIA */
> > + bool has_modular_fia;
> > +
> > /*
> > * NOTE: This is the dri1/ums dungeon, don't add stuff
> > here. Your patch
> > * will be rejected. Instead look for a better place.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 16d5548adb84..8aaf53283200 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2165,14 +2165,15 @@ static inline bool
> > i915_mmio_reg_valid(i915_reg_t reg)
> > #define _FIA(fia) _PICK((fia), FIA1_BASE,
> > FIA2_BASE, FIA3_BASE)
> > #define _MMIO_FIA(fia, off) _MMIO(_FIA(fia) + (off))
> >
> > +#define _TC_MOD_PORT_ID(has_modular_fia,
> > tc_port) ((has_modular_fia) ? (tc_port) % 2 : (tc_port))
>
> instead of doing all of this, what about storing a
> dig_port->tc_phy_fia_idx that is set to
> tc_port on monolithic FIA and to tc_port % 2 on modular...? Then it
> would be initialized together with
> dig_port->tc_phy_fia and we could even remove tc_port_to_fia() that
> would not be used anymore since we would
> check that in the caller.
Looks, better going to move to tc_phy_fia_idx.
Thanks
>
> > /* ICL PHY DFLEX registers */
> > -#define PORT_TX_DFLEXDPMLE1(fia) _MMIO_FIA((fia), 0x008C0)
> > -#define DFLEXDPMLE1_DPMLETC_MASK(tc_port) (0xf << (4 *
> > (tc_port)))
>
> in all these macros it would be s/tc_port/idx/
>
> Lucas De Marchi
>
> > -#define DFLEXDPMLE1_DPMLETC_ML0(tc_port) (1 << (4 *
> > (tc_port)))
> > -#define DFLEXDPMLE1_DPMLETC_ML1_0(tc_port) (3 << (4 *
> > (tc_port)))
> > -#define DFLEXDPMLE1_DPMLETC_ML3(tc_port) (8 << (4 *
> > (tc_port)))
> > -#define DFLEXDPMLE1_DPMLETC_ML3_2(tc_port) (12 << (4 *
> > (tc_port)))
> > -#define DFLEXDPMLE1_DPMLETC_ML3_0(tc_port) (15 << (4 *
> > (tc_port)))
> > +#define
> > PORT_TX_DFLEXDPMLE1(fia) _MMIO_FIA((fia), 0x
> > 008C0)
> > +#define DFLEXDPMLE1_DPMLETC_MASK(mod, tc_port) (0xf << (4
> > * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define DFLEXDPMLE1_DPMLETC_ML0(mod, tc_port) (1
> > << (4 * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define DFLEXDPMLE1_DPMLETC_ML1_0(mod, tc_port) (3 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define DFLEXDPMLE1_DPMLETC_ML3(mod, tc_port) (8
> > << (4 * (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define DFLEXDPMLE1_DPMLETC_ML3_2(mod, tc_port) (12 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> > +#define DFLEXDPMLE1_DPMLETC_ML3_0(mod, tc_port) (15 << (4 *
> > (_TC_MOD_PORT_ID(mod, tc_port))))
> >
> > /* BXT PHY Ref registers */
> > #define _PORT_REF_DW3_A 0x16218C
> > @@ -11669,23 +11670,23 @@ enum skl_power_gate {
> > _ICL_DSC1_RC_BUF_TH
> > RESH_1_UDW_PB, \
> > _ICL_DSC1_RC_BUF_TH
> > RESH_1_UDW_PC)
> >
> > -#define PORT_TX_DFLEXDPSP(fia) _MMIO_FIA((fia),
> > 0x008A0)
> > -#define MODULAR_FIA_MASK (1 << 4)
> > -#define TC_LIVE_STATE_TBT(tc_port) (1 << ((tc_port) *
> > 8 + 6))
> > -#define TC_LIVE_STATE_TC(tc_port) (1 << ((tc_port) *
> > 8 + 5))
> > -#define DP_LANE_ASSIGNMENT_SHIFT(tc_port) ((tc_port) * 8)
> > -#define DP_LANE_ASSIGNMENT_MASK(tc_port) (0xf << ((tc_port)
> > * 8))
> > -#define DP_LANE_ASSIGNMENT(tc_port, x) ((x) << ((tc_port)
> > * 8))
> > +#define
> > PORT_TX_DFLEXDPSP(fia) _MMIO_FIA((fia),
> > 0x008A0)
> > +#define MODULAR_FIA_MASK (1 << 4)
> > +#define TC_LIVE_STATE_TBT(mod, tc_port) (1 <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8 + 6))
> > +#define TC_LIVE_STATE_TC(mod, tc_port) (1 <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8 + 5))
> > +#define DP_LANE_ASSIGNMENT_SHIFT(mod,
> > tc_port) ((_TC_MOD_PORT_ID(mod, tc_port)) * 8)
> > +#define DP_LANE_ASSIGNMENT_MASK(mod,
> > tc_port) (0xf << ((_TC_MOD_PORT_ID(mod, tc_port)) *
> > 8))
> > +#define DP_LANE_ASSIGNMENT(mod, tc_port, x) ((x) <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 8))
> >
> > -#define
> > PORT_TX_DFLEXDPPMS(fia) _MMIO_FIA((fia),
> > 0x00890)
> > -#define DP_PHY_MODE_STATUS_COMPLETED(tc_port) (1
> > << (tc_port))
> > +#define
> > PORT_TX_DFLEXDPPMS(fia) _MMIO_FIA((f
> > ia), 0x00890)
> > +#define DP_PHY_MODE_STATUS_COMPLETED(mod, tc_port) (1 <<
> > (_TC_MOD_PORT_ID(mod, tc_port)))
> >
> > -#define PORT_TX_DFLEXDPCSSS(fia) _MMIO_FIA((fia),
> > 0x00894)
> > -#define DP_PHY_MODE_STATUS_NOT_SAFE(tc_port) (1 <<
> > (tc_port))
> > +#define
> > PORT_TX_DFLEXDPCSSS(fia) _MMIO_FIA((fia),
> > 0x00894)
> > +#define DP_PHY_MODE_STATUS_NOT_SAFE(mod, tc_port) (1 <<
> > (_TC_MOD_PORT_ID(mod, tc_port)))
> >
> > -#define PORT_TX_DFLEXPA1(fia) _MMIO_FIA((fia),
> > 0x00880)
> > -#define DP_PIN_ASSIGNMENT_SHIFT(tc_port) ((tc_port)
> > * 4)
> > -#define DP_PIN_ASSIGNMENT_MASK(tc_port) (0xf <<
> > ((tc_port) * 4))
> > -#define DP_PIN_ASSIGNMENT(tc_port, x) ((x) << ((tc_port)
> > * 4))
> > +#define
> > PORT_TX_DFLEXPA1(fia) _MMIO_FIA((fia),
> > 0x00880)
> > +#define DP_PIN_ASSIGNMENT_SHIFT(mod,
> > tc_port) ((_TC_MOD_PORT_ID(mod, tc_port)) * 4)
> > +#define DP_PIN_ASSIGNMENT_MASK(mod, tc_port) (0xf <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 4))
> > +#define DP_PIN_ASSIGNMENT(mod, tc_port, x) ((x) <<
> > ((_TC_MOD_PORT_ID(mod, tc_port)) * 4))
> >
> > #endif /* _I915_REG_H_ */
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
