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.
> /* 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), 0x008C0)
> +#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_THRESH_1_UDW_PB, \
> _ICL_DSC1_RC_BUF_THRESH_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((fia), 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
--
Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
