Re: [PATCH 03/14] drm/i915/tgl: Finish modular FIA support on registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux