On to, 2015-04-02 at 19:32 +0300, Ville Syrjälä wrote:
> On Tue, Mar 17, 2015 at 11:39:56AM +0200, Imre Deak wrote:
> > From: Vandana Kannan <vandana.kannan@xxxxxxxxx>
> >
> > Add display clock/PHY initialization sequence as per BSpec.
> >
> > Until GOP/VBIOS provides an upper limit value for CDCLK, comparing clock
> > value with 624 MHz and returning 0 in case it exceeds.
> >
> > Note that the CD clock and PHY initialization/uninitialization are done
> > at their current place only for simplicity, in a future patch - when more
> > of the runtime PM features will be enabled - these will be moved to
> > power well#1 and modeset encoder enabling/disabling hooks respectively.
> > This also means that atm dynamic power gating power well #2 is
> > effectively disabled.
>
> OK, I've gone through the PHY stuff a bit now, and skipped the cdclk
> stuff this time.
>
> >
> > v1: Added function definitions in header files
> > v2: Imre's review comments addressed
> > - Moved CDCLK related definitions to i915_reg.h
> > - Removed defintions for CDCLK frequency
> > - Split uninit_cdclk() by adding a phy_uninit function
> > - Calculate freq and decimal based on input frequency
> > - Program SSA precharge based on input frequency
> > - Use wait_for 1ms instead 200us udelay for DE PLL locking
> > - Removed initial value for divider, freq, decimal, ratio.
> > - Replaced polling loops with wait_for
> > - Parameterized latency optim setting
> > - Fix the parts where DE PLL has to be disabled.
> > - Call CDCLK selection from mode set
> >
> > v3: (imre)
> > - add note about the plan to move the cdclk/phy init to a better place
> > - take rps.hw_lock around pcode access
> > - fix DDI PHY timeout value
> > - squash in Vandana's "PORT_CL2CM_DW6_A BUN fix",
> > "DDI PHY programming register defn", "Do ddi_phy_init always",
> > "Check CDCLK upper limit" patches
> > - move PHY register macros next to the corresponding CHV/VLV macros
> > - move DE PLL register macros here from another patch since they are
> > used here first
> > - add BXT_ prefix to CDCLK flags
> > - s/COMMON_RESET/COMMON_RESET_DIS/ and clarify related code comments
> > - fix incorrect read value for the RMW of BXT_PHY_CTL_FAMILY_DDI
> > - fix using GT_DISPLAY_EDP_POWER_ON vs. GT_DISPLAY_DDI_POWER_ON
> > when powering on DDI ports
> > - fix incorrect port when setting BXT_PORT_TX_DW14_LN for DDI ports
> > - add missing masking when programming CDCLK_FREQ_DECIMAL
> > - add missing powering on for DDI-C port, rename OCL2_LDOFUSE_PWR_EN
> > to OCL2_LDOFUSE_PWR_DIS to reduce confusion
> > - add note about mismatch with bspec in the PORT_REF_DW6 fields
> > - factor out PHY init code to a new function, so we can call it for
> > PHY_A and PHY_BC, instead of open-coding the same
> >
> > Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx> (v2)
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 126 +++++++++++++++
> > drivers/gpu/drm/i915/intel_ddi.c | 291 +++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_display.c | 75 +++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 4 +
> > 4 files changed, 496 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b4474d3..a3579c0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1120,6 +1120,110 @@ enum skl_disp_power_wells {
> > #define DPIO_FRC_LATENCY_SHFIT 8
> > #define CHV_TX_DW14(ch, lane) _TXLANE(ch, lane, 0xb8)
> > #define DPIO_UPAR_SHIFT 30
> > +
> > +/* BXT PHY registers */
> > +enum bxt_phy {
> > + BXT_PHY_A,
> > + BXT_PHY_BC
> > +};
>
> We have enum dpio_phy already. Although here we have defined 0 to be the
> single channel PHY and 1 is the two channel PHY, whereas on CHV it's
> the other way around. I'm going to suggest we flip BXT over to use the
> CHV scheme to avoid any surpises later if we actualy try to unify the
> code.
Yea, I haven't noticed it, will use that instead.
> > +
> > +#define BXT_PHY(phy, a, b) ((a) + (phy) * ((b) - (a)))
>
> This seems to be just another _PIPE(), should at least have an
> underscore so that people don't confuse it with something they are
> supposed to use.
Ok, I can rewrite this to
#define _BXT_PHY(phy, a, b) _PIPE(phy, a, b)
>
> > +
> > +#define BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR 0x138090
>
> We can drop the _0_2_0_GTTMMADR suffix, we've never included it for any
> other platforms either.
Ok.
> > +#define _EDP_POWER_ON (1 << 1)
> > +#define _DDI_POWER_ON (1 << 0)
> > +#define GT_DISPLAY_POWER_ON(phy) BXT_PHY(phy, _EDP_POWER_ON, \
> > + _DDI_POWER_ON)
>
> Using a _PIPE() type of macro for register bits seems a bit unusual.
> I'd just open code it as (1 << (phy)) or something. That also work
> better if we the PHYs around so that PHY0 is the dual channel PHY.
Ok, will rewrite it.
> > +
> > +#define _PHY_CTL_FAMILY_EDP 0x64C80
> > +#define _PHY_CTL_FAMILY_DDI 0x64C90
> > +#define COMMON_RESET_DIS (1 << 31)
> > +#define BXT_PHY_CTL_FAMILY(phy) BXT_PHY(phy, _PHY_CTL_FAMILY_EDP, \
> > + _PHY_CTL_FAMILY_DDI)
> > +
> > +/* BXT PHY common lane registers */
> > +#define _PORT_CL1CM_DW0_A 0x162000
> > +#define _PORT_CL1CM_DW0_BC 0x6C000
> > +#define PHY_POWER_GOOD (1 << 16)
> > +#define BXT_PORT_CL1CM_DW0(phy) BXT_PHY(phy, _PORT_CL1CM_DW0_A, \
> > + _PORT_CL1CM_DW0_BC)
>
> I'm a bit sad these are not sharing the CHV reg defines, or even
> resemble them in any way.
I agree this isn't ideal. I've already refactored some parts in the
original version of this patch (see the commit version log) to bring it
closer to CHV, but there is definitely more to do. I'd say it's better
to do this after moving the cdckl/phy init code to the modeset/power
well enable time.
> > +#define _PORT_CL1CM_DW9_A 0x162024
> > +#define _PORT_CL1CM_DW9_BC 0x6C024
> > +#define IREF0RC_OFFSET_SHIFT 8
> > +#define IREF0RC_OFFSET_MASK (0xFF << IREF0RC_OFFSET_SHIFT)
> > +#define BXT_PORT_CL1CM_DW9(phy) BXT_PHY(phy, _PORT_CL1CM_DW9_A, \
> > + _PORT_CL1CM_DW9_BC)
> > +
> > +#define _PORT_CL1CM_DW10_A 0x162028
> > +#define _PORT_CL1CM_DW10_BC 0x6C028
> > +#define IREF1RC_OFFSET_SHIFT 8
> > +#define IREF1RC_OFFSET_MASK (0xFF << IREF1RC_OFFSET_SHIFT)
> > +#define BXT_PORT_CL1CM_DW10(phy) BXT_PHY(phy, _PORT_CL1CM_DW10_A, \
> > + _PORT_CL1CM_DW10_BC)
> > +
> > +#define _PORT_CL1CM_DW28_A 0x162070
> > +#define _PORT_CL1CM_DW28_BC 0x6C070
> > +#define OCL1_POWER_DOWN_EN (1 << 23)
> > +#define DW28_OLDO_DYN_PWR_DOWN_EN (1 << 22)
> > +#define SUS_CLK_CONFIG 0x3
> > +#define BXT_PORT_CL1CM_DW28(phy) BXT_PHY(phy, _PORT_CL1CM_DW28_A, \
> > + _PORT_CL1CM_DW28_BC)
> > +
> > +#define _PORT_CL1CM_DW30_A 0x162078
> > +#define _PORT_CL1CM_DW30_BC 0x6C078
> > +#define OCL2_LDOFUSE_PWR_DIS (1 << 6)
> > +#define BXT_PORT_CL1CM_DW30(phy) BXT_PHY(phy, _PORT_CL1CM_DW30_A, \
> > + _PORT_CL1CM_DW30_BC)
> > +
> > +/* Defined for PHY_BC only */
> > +#define BXT_PORT_CL2CM_DW6_BC 0x6C358
> > +#define DW6_OLDO_DYN_PWR_DOWN_EN (1 << 28)
> > +
> > +/* BXT PHY Ref registers */
> > +#define _PORT_REF_DW3_A 0x16218C
> > +#define _PORT_REF_DW3_BC 0x6C18C
> > +#define GRC_DONE (1 << 22)
> > +#define BXT_PORT_REF_DW3(phy) BXT_PHY(phy, _PORT_REF_DW3_A, \
> > + _PORT_REF_DW3_BC)
> > +
> > +#define _PORT_REF_DW6_A 0x162198
> > +#define _PORT_REF_DW6_BC 0x6C198
> > +/*
> > + * FIXME: BSpec disagrees on the following two fields, check them with
> > + * HW/documentation people.
> > + */
>
> CHV configdb also disagrees.
Ok, thanks. Before fixing this, I want to wait for the HW to test
things.
> > +#define GRC_CODE_SHIFT 23
> > +#define GRC_CODE_MASK (0x1FF << GRC_CODE_SHIFT)
> > +#define GRC_CODE_FAST_SHIFT 16
> > +#define GRC_CODE_FAST_MASK (0x7F << GRC_CODE_FAST_SHIFT)
> > +#define GRC_CODE_SLOW_SHIFT 8
> > +#define GRC_CODE_SLOW_MASK (0xFF << GRC_CODE_SLOW_SHIFT)
> > +#define GRC_CODE_NOM_MASK 0xFF
> > +#define BXT_PORT_REF_DW6(phy) BXT_PHY(phy, _PORT_REF_DW6_A, \
> > + _PORT_REF_DW6_BC)
> > +
> > +#define _PORT_REF_DW8_A 0x1621A0
> > +#define _PORT_REF_DW8_BC 0x6C1A0
> > +#define GRC_DIS (1 << 15)
> > +#define GRC_RDY_OVRD (1 << 1)
> > +#define BXT_PORT_REF_DW8(phy) BXT_PHY(phy, _PORT_REF_DW8_A, \
> > + _PORT_REF_DW8_BC)
> > +
> > +/* BXT PHY TX registers */
> > +#define BXT_LANE_OFFSET(lane) (((lane) >> 1) * 0x200 + \
> > + ((lane) & 1) * 0x80)
> > +
> > +#define _PORT_TX_DW14_LN0_A 0x162538
> > +#define _PORT_TX_DW14_LN0_B 0x6C538
> > +#define _PORT_TX_DW14_LN0_C 0x6C938
> > +#define LATENCY_OPTIM_SHIFT 30
> > +#define LATENCY_OPTIM (1 << LATENCY_OPTIM_SHIFT)
> > +#define BXT_PORT_TX_DW14_LN(port, lane) (_PORT3(port, _PORT_TX_DW14_LN0_A, \
> > + _PORT_TX_DW14_LN0_B, \
> > + _PORT_TX_DW14_LN0_C) + \
> > + BXT_LANE_OFFSET(lane))
> > +
> > /*
> > * Fence registers
> > */
> > @@ -5326,6 +5430,9 @@ enum skl_disp_power_wells {
> > #define DISP_FBC_WM_DIS (1<<15)
> > #define DISP_ARB_CTL2 0x45004
> > #define DISP_DATA_PARTITION_5_6 (1<<6)
> > +#define DBUF_CTL 0x45008
> > +#define DBUF_POWER_REQUEST (1<<31)
> > +#define DBUF_POWER_STATE (1<<30)
> > #define GEN7_MSG_CTL 0x45010
> > #define WAIT_FOR_PCH_RESET_ACK (1<<1)
> > #define WAIT_FOR_PCH_FLR_ACK (1<<0)
> <snip>
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index a203d9d..789682d 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1957,6 +1957,294 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
> > }
> > }
> >
> > +static void bxt_init_phy(struct drm_i915_private *dev_priv, enum bxt_phy phy)
> > +{
> > + enum port port;
> > + uint32_t val;
> > +
> > + val = I915_READ(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR);
> > + val |= GT_DISPLAY_POWER_ON(phy);
> > + I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR, val);
> > +
> > + /* Considering 10ms timeout until BSpec is updated */
> > + if (wait_for(I915_READ(BXT_PORT_CL1CM_DW0(phy)) & PHY_POWER_GOOD, 10))
> > + DRM_ERROR("timeout during PHY#%d power on\n", phy);
> > +
> > + /* Program latency optim setting */
> > + for (port = (phy == BXT_PHY_A ? PORT_A : PORT_B);
> > + port <= (phy == BXT_PHY_A ? PORT_A : PORT_C); port++) {
> > + int lane;
> > +
> > + for (lane = 0; lane < 4; lane++) {
> > + val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> > + val &= ~LATENCY_OPTIM;
> > + if (lane == 1)
>
> Should be != 1
Thanks for catching it, this is a fall-out from my refactoring. Will fix
it.
> > + val |= LATENCY_OPTIM;
> > +
> > + I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> > + }
> > + }
> > +
> > + /* Program PLL Rcomp code offset */
> > + val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
> > + val &= ~IREF0RC_OFFSET_MASK;
> > + val |= 0xE4 << IREF0RC_OFFSET_SHIFT;
> > + I915_WRITE(BXT_PORT_CL1CM_DW9(phy), val);
> > +
> > + val = I915_READ(BXT_PORT_CL1CM_DW10(phy));
> > + val &= ~IREF1RC_OFFSET_MASK;
> > + val |= 0xE4 << IREF1RC_OFFSET_SHIFT;
> > + I915_WRITE(BXT_PORT_CL1CM_DW10(phy), val);
> > +
> > + /* Program power gating */
> > + val = I915_READ(BXT_PORT_CL1CM_DW28(phy));
> > + val |= OCL1_POWER_DOWN_EN | DW28_OLDO_DYN_PWR_DOWN_EN |
> > + SUS_CLK_CONFIG;
> > + I915_WRITE(BXT_PORT_CL1CM_DW28(phy), val);
> > +
> > + if (phy == BXT_PHY_BC) {
> > + val = I915_READ(BXT_PORT_CL2CM_DW6_BC);
> > + val |= DW6_OLDO_DYN_PWR_DOWN_EN;
> > + I915_WRITE(BXT_PORT_CL2CM_DW6_BC, val);
> > + }
> > +
> > + val = I915_READ(BXT_PORT_CL1CM_DW30(phy));
> > + val &= ~OCL2_LDOFUSE_PWR_DIS;
> > + /*
> > + * On PHY_A disable power on the second channel, since no port is
> > + * connected there. On PHY_BC both channels have a port, so leave it
> > + * enabled.
> > + * Note that port C is only connected on BXT-P, so on BXT0/1 we should
> > + * power down the second channel on PHY_BC as well.
> > + */
> > + if (phy == BXT_PHY_A)
> > + val |= OCL2_LDOFUSE_PWR_DIS;
> > + I915_WRITE(BXT_PORT_CL1CM_DW30(phy), val);
> > +
> > + if (phy == BXT_PHY_BC) {
> > + uint32_t grc_code;
> > + /*
> > + * PHY_BC isn't connected to an RCOMP resistor so copy over
> > + * the corresponding calibrated value from PHY_A, and disable
> > + * the automatic calibration on PHY_BC.
> > + */
> > + if (wait_for(I915_READ(BXT_PORT_REF_DW3(BXT_PHY_A)) & GRC_DONE,
> > + 10))
> > + DRM_ERROR("timeout waiting for PHY#0 GRC\n");
> > +
> > + val = I915_READ(BXT_PORT_REF_DW6(BXT_PHY_A));
> > + val = (val & GRC_CODE_MASK) >> GRC_CODE_SHIFT;
> > + grc_code = val << GRC_CODE_FAST_SHIFT |
> > + val << GRC_CODE_SLOW_SHIFT |
> > + val;
> > + I915_WRITE(BXT_PORT_REF_DW6(BXT_PHY_BC), grc_code);
> > +
> > + val = I915_READ(BXT_PORT_REF_DW8(BXT_PHY_BC));
> > + val |= GRC_DIS | GRC_RDY_OVRD;
> > + I915_WRITE(BXT_PORT_REF_DW8(BXT_PHY_BC), val);
> > + }
> > +
> > + /* Release common_reset */
> > + val = I915_READ(BXT_PHY_CTL_FAMILY(phy));
> > + val |= COMMON_RESET_DIS;
> > + I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
> > +}
>
> I suppose we'll want to model this thing as a power well like on CHV
> eventually, but I guess we can start off with initializing it once.
Yep, sounds good, but I'd also prefer to do this as a follow-up.
> > +
> > +void bxt_ddi_phy_init(struct drm_device *dev)
> > +{
> > + /* Enable PHY_A first since it provides Rcomp for PHY_BC */
> > + bxt_init_phy(dev->dev_private, BXT_PHY_A);
> > + bxt_init_phy(dev->dev_private, BXT_PHY_BC);
> > +}
> > +
> > +static void bxt_ddi_phy_uninit(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + uint32_t temp;
> > +
> > + temp = I915_READ(BXT_PHY_CTL_FAMILY(BXT_PHY_A));
> > + I915_WRITE(BXT_PHY_CTL_FAMILY(BXT_PHY_A), temp & ~COMMON_RESET_DIS);
> > +
> > + temp = I915_READ(BXT_PHY_CTL_FAMILY(BXT_PHY_BC));
> > + I915_WRITE(BXT_PHY_CTL_FAMILY(BXT_PHY_BC), temp & ~COMMON_RESET_DIS);
> > +
> > + I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR, 0);
> > +}
>
> This should really be per-phy to avoid confusion when comparing with the
> init function.
Ok, will rewrite it.
> > +
> <snip>
> > +
> > +void bxt_init_cdclk(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > + /* NDE_RSTWRN_OPT RST PCH Handshake En must always be 0b on BXT
> > + * or else the reset will hang because there is no PCH to respond.
> > + * Move the handshake programming to initialization sequence.
> > + * Previously was left up to BIOS.
> > + */
> > + u32 temp = I915_READ(HSW_NDE_RSTWRN_OPT);
> > +
> > + temp &= ~RESET_PCH_HANDSHAKE_ENABLE;
> > + I915_WRITE(HSW_NDE_RSTWRN_OPT, temp);
> > +
> > + /* Enable PG1 for cdclk */
> > + intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > +
> > + /* check if cd clock is enabled */
> > + if (I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_PLL_ENABLE) {
> > + DRM_DEBUG_KMS("Display already initialized\n");
> > + return;
> > + }
> > +
> > + /* FIXME:- The initial CDCLK needs to be read from VBT.
> > + * Need to make this change after VBT has changes for BXT.
> > + */
> > + bxt_select_cdclk_freq(dev, 624000);
> > +
> > + I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> > + udelay(10);
> > +
> > + if (!(I915_READ(DBUF_CTL) & DBUF_POWER_STATE))
> > + DRM_ERROR("DBuf power enable timeout!\n");
> > +}
>
> This code seems like power well territory again. Just stuffing it into
> PG1 would seem like a good enough solution judging by the fact that we
> hold the PG1 reference as long as we have the DBUF power request enabled.
>
> Or are there actual uses for having PG1 enabled w/o this thing?
I don't think so, bspec requires this power request line to be asserted
for all display internal functionality. So I agree moving it to the
power well code is a good plan (as a follow-up).
> > +
> > +void bxt_uninit_cdclk(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > + bxt_ddi_phy_uninit(dev);
> > +
> > + I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) & ~DBUF_POWER_REQUEST);
> > + udelay(10);
> > +
> > + if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> > + DRM_ERROR("DBuf power disable timeout!\n");
> > +
> > + bxt_select_cdclk_freq(dev, 0);
> > +
> > + intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > +}
> > +
> > void intel_ddi_pll_init(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1973,6 +2261,9 @@ void intel_ddi_pll_init(struct drm_device *dev)
> > if (IS_SKYLAKE(dev)) {
> > if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> > DRM_ERROR("LCPLL1 is disabled\n");
> > + } else if (IS_BROXTON(dev)) {
> > + bxt_init_cdclk(dev);
> > + bxt_ddi_phy_init(dev);
> > } else {
> > /*
> > * The LCPLL register should be turned on by the BIOS. For now
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
