> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Monday, April 3, 2023 1:12 PM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Sripada, Radhakrishna
> <radhakrishna.sripada@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>
> Subject: Re: [PATCH 4/7] drm/i915/mtl: Add Support for C10 PHY message bus
> and pll programming
>
> On Mon, Mar 27, 2023 at 03:34:30PM +0300, Mika Kahola wrote:
> > From: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> >
> > XELPDP has C10 and C20 phys from Synopsys to drive displays. Each phy
> > has a dedicated PIPE 5.2 Message bus for configuration. This message
> > bus is used to configure the phy internal registers.
> >
> > XELPDP has C10 phys to drive output to the EDP and the native output
> > from the display engine. Add structures, programming hardware state
> > readout logic. Port clock calculations are similar to DG2. Use the DG2
> > formulae to calculate the port clock but use the relevant pll signals.
> > Note: PHY lane 0 is always used for PLL programming.
> >
> > Add sequences for C10 phy enable/disable phy lane reset, powerdown
> > change sequence and phy lane programming.
> >
> > Bspec: 64539, 64568, 64599, 65100, 65101, 65450, 65451, 67610, 67636
>
> Shouldn't the basic MTL DP/HDMI modeset sequences be part of this patchset? I
> can't see how things would work otherwise. For DP it is the
>
> "drm/i915/mtl/display: Implement DisplayPort sequences"
>
> patch in the internal tree.
The idea was to get the eDP supported with this C10 series. We could go back to the original form and have all C10/C20/TBT patches in one series.
>
> More things below, besides my earlier review comments.
>
> > [...]
> > +
> > +static void intel_c10_pll_program(struct drm_i915_private *i915,
> > + const struct intel_crtc_state *crtc_state,
> > + struct intel_encoder *encoder)
> > +{
> > + const struct intel_c10mpllb_state *pll_state = &crtc_state-
> >c10mpllb_state;
> > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > + bool lane_reversal = dig_port->saved_port_bits &
> DDI_BUF_PORT_REVERSAL;
> > + u8 master_lane = lane_reversal ? INTEL_CX0_LANE1 :
> > + INTEL_CX0_LANE0;
> > + u8 follower_lane = lane_reversal ? INTEL_CX0_LANE0 :
> > + INTEL_CX0_LANE1;
> > +
> > + int i;
> > + struct intel_dp *intel_dp;
> > + bool use_ssc = false;
> > + u8 cmn0 = 0;
> > +
> > + if (intel_crtc_has_dp_encoder(crtc_state)) {
> > + intel_dp = enc_to_intel_dp(encoder);
> > + use_ssc = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> > + DP_MAX_DOWNSPREAD_0_5);
> > +
> > + if (!intel_panel_use_ssc(i915))
> > + use_ssc = false;
> > +
> > + cmn0 = C10_CMN0_DP_VAL;
> > + }
> > +
> > + intel_cx0_write(i915, encoder->port, INTEL_CX0_BOTH_LANES,
> PHY_C10_VDR_CONTROL(1),
> > + C10_VDR_CTRL_MSGBUS_ACCESS,
> MB_WRITE_COMMITTED);
> > + /* Custom width needs to be programmed to 0 for both the phy lanes */
> > + intel_cx0_rmw(i915, encoder->port, INTEL_CX0_BOTH_LANES,
> > + PHY_C10_VDR_CUSTOM_WIDTH, 0x3, 0,
> MB_WRITE_COMMITTED);
> > + intel_cx0_rmw(i915, encoder->port, follower_lane,
> PHY_C10_VDR_CONTROL(1),
> > + C10_VDR_CTRL_MASTER_LANE,
> C10_VDR_CTRL_UPDATE_CFG,
> > + MB_WRITE_COMMITTED);
> > +
> > + /* Program the pll values only for the master lane */
> > + for (i = 0; i < ARRAY_SIZE(pll_state->pll); i++)
> > + /* If not using ssc pll[4] through pll[8] must be 0*/
> > + intel_cx0_write(i915, encoder->port, master_lane,
> > +PHY_C10_VDR_PLL(i),
>
> This programs the PLL via the INTEL_CX0_LANE1 lane in the lane_reversal=true
> case. However, I haven't found any trace of this being correct in the spec. It just
> says that PLL must be programmed via
> INTEL_CX0_LANE0 in all the cases, so both for lane_reversal and !lane_reversal
> (see Bspec/64539 "Phy Lane and Transmitter Usage"
> table/"Lane for message bus PLL programming" column).
>
> > + (!use_ssc && (i > 3 && i < 9)) ? 0 : pll_state-
> >pll[i],
> > + (i % 4) ? MB_WRITE_UNCOMMITTED :
> MB_WRITE_COMMITTED);
> > +
> > + intel_cx0_write(i915, encoder->port, master_lane,
> PHY_C10_VDR_CMN(0), cmn0, MB_WRITE_COMMITTED);
> > + intel_cx0_write(i915, encoder->port, master_lane,
> PHY_C10_VDR_TX(0), C10_TX0_VAL, MB_WRITE_COMMITTED);
> > + intel_cx0_rmw(i915, encoder->port, master_lane,
> PHY_C10_VDR_CONTROL(1),
> > + C10_VDR_CTRL_MSGBUS_ACCESS,
> C10_VDR_CTRL_MASTER_LANE |
> > + C10_VDR_CTRL_UPDATE_CFG, MB_WRITE_COMMITTED);
>
> For all the above writes, programming INTEL_CX0_LANE1 looks incorrect in the
> lane_reversal=true case, should program INTEL_CX0_LANE0 instead.
So in any case we should program INTEL_CX0_LANE0?
>
> > +}
> > +
> >
> > [...]
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> > index d1ee8a2fc9cf..15e249f46a64 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> > @@ -128,4 +128,34 @@
> > #define XELPDP_SSC_ENABLE_PLLA REG_BIT(1)
> > #define XELPDP_SSC_ENABLE_PLLB REG_BIT(0)
> >
> > -#endif /* __INTEL_CX0_PHY_REGS_H__ */
> > +/* C10 Vendor Registers */
> > +#define PHY_C10_VDR_PLL(idx) (0xC00 + (idx))
> > +#define C10_PLL0_FRACEN REG_BIT8(4)
> > +#define C10_PLL3_MULTIPLIERH_MASK REG_GENMASK8(3, 0)
> > +#define C10_PLL15_TXCLKDIV_MASK REG_GENMASK8(2, 0)
> > +#define PHY_C10_VDR_CMN(idx) (0xC20 + (idx))
> > +#define C10_CMN0_DP_VAL 0x21
> > +#define C10_CMN3_TXVBOOST_MASK REG_GENMASK8(7, 5)
> > +#define C10_CMN3_TXVBOOST(val)
> REG_FIELD_PREP8(C10_CMN3_TXVBOOST_MASK, val)
> > +#define PHY_C10_VDR_TX(idx) (0xC30 + (idx))
> > +#define C10_TX0_VAL 0x10
> > +#define PHY_C10_VDR_CONTROL(idx) (0xC70 + (idx) - 1)
> > +#define C10_VDR_CTRL_MSGBUS_ACCESS REG_BIT8(2)
> > +#define C10_VDR_CTRL_MASTER_LANE REG_BIT8(1)
> > +#define C10_VDR_CTRL_UPDATE_CFG REG_BIT8(0)
> > +#define PHY_C10_VDR_CUSTOM_WIDTH 0xD02
> > +
> > +#define CX0_P0_STATE_ACTIVE 0x0
> > +#define CX0_P2_STATE_READY 0x2
> > +#define CX0_P2PG_STATE_DISABLE 0x9
> > +#define CX0_P4PG_STATE_DISABLE 0xC
> > +#define CX0_P2_STATE_RESET 0x2
> > +
> > +/* PHY_C10_VDR_PLL0 */
> > +#define PLL_C10_MPLL_SSC_EN REG_BIT8(0)
>
> These should be indented and moved to be under their register
> (XELPDP_PORT_BUF_CTL3, PHY_C10_VDR_PLL).
Ok. Let's move these to correct places.
>
> > +
> > +/* PIPE SPEC Defined Registers */
> > +#define PHY_CX0_TX_CONTROL(tx, control) (0x400 + ((tx) - 1) * 0x200 +
> (control))
> > +#define CONTROL2_DISABLE_SINGLE_TX REG_BIT(6)
>
> The flag should be indented.
Ok.
>
> > +
> > +#endif /* __INTEL_CX0_REG_DEFS_H__ */
> >
> > [...]
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h
> > b/drivers/gpu/drm/i915/i915_reg_defs.h
> > index db26de6b57bc..f9d7c03e95d6 100644
> > --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> > +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> > @@ -22,6 +22,19 @@
> > BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \
> > ((__n) < 0 || (__n) > 31))))
> >
> > +/**
> > + * REG_BIT8() - Prepare a u8 bit value
> > + * @__n: 0-based bit number
> > + *
> > + * Local wrapper for BIT() to force u8, with compile time checks.
> > + *
> > + * @return: Value with bit @__n set.
> > + */
> > +#define REG_BIT8(__n) \
> > + ((u8)(BIT(__n) + \
> > + BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \
> > + ((__n) < 0 || (__n) > 7))))
> > +
> > /**
> > * REG_GENMASK() - Prepare a continuous u32 bitmask
> > * @__high: 0-based high bit
> > @@ -52,6 +65,21 @@
> > __is_constexpr(__low) && \
> > ((__low) < 0 || (__high) > 63 || (__low) >
> (__high)))))
> >
> > +/**
> > + * REG_GENMASK8() - Prepare a continuous u8 bitmask
> > + * @__high: 0-based high bit
> > + * @__low: 0-based low bit
> > + *
> > + * Local wrapper for GENMASK() to force u8, with compile time checks.
> > + *
> > + * @return: Continuous bitmask from @__high to @__low, inclusive.
> > + */
> > +#define REG_GENMASK8(__high, __low) \
> > + ((u8)(GENMASK(__high, __low) + \
> > + BUILD_BUG_ON_ZERO(__is_constexpr(__high) && \
> > + __is_constexpr(__low) && \
> > + ((__low) < 0 || (__high) > 7 || (__low) >
> (__high)))))
> > +
> > /*
> > * Local integer constant expression version of is_power_of_2().
> > */
> > @@ -74,6 +102,23 @@
> > BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL <<
> __bf_shf(__mask)))) + \
> >
> > BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val),
> > (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
> >
> > +/**
> > + * REG_FIELD_PREP8() - Prepare a u8 bitfield value
> > + * @__mask: shifted mask defining the field's length and position
> > + * @__val: value to put in the field
> > + *
> > + * Local copy of FIELD_PREP8() to generate an integer constant
> > +expression, force
>
> The above is FIELD_PREP() only.
So use FIELD_PREP() instead of FIELD_PREP8()?
>
> > + * u8 and for consistency with REG_FIELD_GET8(), REG_BIT8() and
> REG_GENMASK8().
> > + *
> > + * @return: @__val masked and shifted into the field defined by @__mask.
> > + */
> > +#define REG_FIELD_PREP8(__mask, __val) \
> > + ((u8)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) + \
> > + BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) + \
> > + BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U8_MAX) +
> \
> > + BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL <<
> __bf_shf(__mask)))) + \
> > +
> > +BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val),
> > +(~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
> > +
> > /**
> > * REG_FIELD_GET() - Extract a u32 bitfield value
> > * @__mask: shifted mask defining the field's length and position @@
> > -155,6 +200,18 @@
> > */
> > #define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__
> > })[__index])
> >
> > +/**
> > + * REG_FIELD_GET8() - Extract a u8 bitfield value
> > + * @__mask: shifted mask defining the field's length and position
> > + * @__val: value to extract the bitfield value from
> > + *
> > + * Local wrapper for FIELD_GET() to force u8 and for consistency with
> > + * REG_FIELD_PREP(), REG_BIT() and REG_GENMASK().
> > + *
> > + * @return: Masked and shifted value of the field defined by @__mask in
> @__val.
> > + */
> > +#define REG_FIELD_GET8(__mask, __val) ((u8)FIELD_GET(__mask, __val))
> > +
> > typedef struct {
> > u32 reg;
> > } i915_reg_t;
> > --
> > 2.34.1
> >
