From: "Souza, Jose" <jose.souza@intel.com>
To: "lucas.de.marchi@gmail.com" <lucas.de.marchi@gmail.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 10/14] drm/i915/tgl: Fix dkl phy register space addressing
Date: Wed, 18 Sep 2019 19:55:08 +0000 [thread overview]
Message-ID: <108c346fb154a1f31061bfc5550f68ce5b328253.camel@intel.com> (raw)
In-Reply-To: <CAKi4VAJ3oAVJ6KFCof2FrdKr5Xc3kkiAs7jHeVJEU4Qu3TLD2Q@mail.gmail.com>
On Sat, 2019-09-14 at 00:26 -0700, Lucas De Marchi wrote:
> On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
> <jose.souza@intel.com> wrote:
> > It was always modifing register space of the first phy in the
> > HIP_INDEX_REG for all ports while it should shift 8 bits for each
> > port inside of HIP_INDEX_REG.
> >
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 34
> > +++++++++++++++----
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 10 ++++--
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > 3 files changed, 38 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index a6a2e00cc075..981e24120a87 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2795,7 +2795,10 @@ tgl_dkl_phy_ddi_vswing_sequence(struct
> > intel_encoder *encoder, int link_clock,
> > * All registers programmed here use HIP_INDEX_REG 0 or 1
> > */
> > for (ln = 0; ln < 2; ln++) {
> > - I915_WRITE(HIP_INDEX_REG(tc_port), ln);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, ln);
>
> INDEX_INDEX?
>
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
>
> we don't really care for a RMW here, do we? We don't access these
> registers in parallel for other ports
> (It would be inherently racy if we did), so
>
> I915_WRITE(HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port,
> ln));
>
> should be sufficient.
>
Okay
> Also I think all these fixes should be squashed in their
> correspondent
> patches in this series with changelogs
> updated.
>
Yeah, I was trying to avoid the fatigue :D
But I will squash in each patch
> > /* All the registers are RMW */
> > val = I915_READ(DKL_TX_DPCNTL0(tc_port));
> > @@ -3134,7 +3137,10 @@ tgl_phy_clock_gating(struct
> > intel_digital_port *dig_port, bool enable)
> > DKL_DP_MODE_CFG_GAONPWR_GATING;
> >
> > for (ln = 0; ln < 2; ln++) {
> > - I915_WRITE(HIP_INDEX_REG(tc_port), ln);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, ln);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > val = I915_READ(DKL_DP_MODE(tc_port));
> > if (enable)
> > @@ -3249,16 +3255,23 @@ static void tgl_program_dkl_dp_mode(struct
> > intel_digital_port *intel_dig_port)
> > struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> > enum port port = intel_dig_port->base.port;
> > enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > - u32 ln0, ln1, lane_mask, pin_mask;
> > + u32 ln0, ln1, lane_mask, pin_mask, val;
> > int num_lanes;
> >
> > if (tc_port == PORT_TC_NONE ||
> > intel_dig_port->tc_mode == TC_PORT_TBT_ALT)
> > return;
> >
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x0);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x0);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > ln0 = I915_READ(DKL_DP_MODE(tc_port));
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x1);
> > +
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x1);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > ln1 = I915_READ(DKL_DP_MODE(tc_port));
> >
> > num_lanes = intel_dig_port->dp.lane_count;
> > @@ -3327,9 +3340,16 @@ static void tgl_program_dkl_dp_mode(struct
> > intel_digital_port *intel_dig_port)
> > return;
> > }
> >
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x0);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x0);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > I915_WRITE(DKL_DP_MODE(tc_port), ln0);
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x1);
> > +
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x1);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > I915_WRITE(DKL_DP_MODE(tc_port), ln1);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index afc9b609b63d..9304606c1c0a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3109,7 +3109,10 @@ static bool dkl_pll_get_hw_state(struct
> > drm_i915_private *dev_priv,
> > * All registers read here have the same HIP_INDEX_REG even
> > though
> > * they are on different building blocks
> > */
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x2);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x2);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > hw_state->mg_refclkin_ctl =
> > I915_READ(DKL_REFCLKIN_CTL(tc_port));
> > hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> > @@ -3301,7 +3304,10 @@ static void dkl_pll_write(struct
> > drm_i915_private *dev_priv,
> > * All registers programmed here have the same
> > HIP_INDEX_REG even
> > * though on different building block
> > */
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x2);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x2);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > /* All the registers are RMW */
> > val = I915_READ(DKL_REFCLKIN_CTL(tc_port));
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 95a4232c8e0a..625f458e9b1c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -10265,6 +10265,9 @@ enum skl_power_gate {
> > #define HIP_INDEX_REG(tc_port) _MMIO((tc_port) < 4 \
> > ? _HIP_INDEX_REG0 \
> > : _HIP_INDEX_REG1)
> > +#define _HIP_INDEX_SHIFT(tc_port) (8 * ((tc_port) % 4))
> > +#define HIP_INDEX_MASK(tc_port) (0xff <<
> > _HIP_INDEX_SHIFT(tc_port))
> > +#define HIP_INDEX_INDEX_VAL(tc_port, index) ((index) <<
> > _HIP_INDEX_SHIFT(tc_port))
>
> #define HIP_INDEX_VAL(index) ((index) | (index) << 8 | ( index) << 16
> > (index) << 24)
> ?
I guess the current one is better, more easy to understand.
>
> Lucas De Marchi
>
> > /* BXT display engine PLL */
> > #define BXT_DE_PLL_CTL _MMIO(0x6d000)
> > --
> > 2.23.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-09-18 19:55 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-13 22:32 [PATCH 00/14] TGL TC enabling José Roberto de Souza
2019-09-13 22:32 ` [PATCH 01/14] drm/i915/tgl: Add missing ddi clock select during DP init sequence José Roberto de Souza
2019-09-13 22:32 ` [PATCH 02/14] drm/i915/tgl: TC helper function to return pin mapping José Roberto de Souza
2019-09-14 5:54 ` Lucas De Marchi
2019-09-17 21:15 ` Souza, Jose
2019-09-13 22:32 ` [PATCH 03/14] drm/i915/tgl: Finish modular FIA support on registers José Roberto de Souza
2019-09-14 6:24 ` Lucas De Marchi
2019-09-18 1:08 ` Souza, Jose
2019-09-13 22:32 ` [PATCH 04/14] drm/i915/tgl: Fix driver crash when update_active_dpll is called José Roberto de Souza
2019-09-14 6:32 ` Lucas De Marchi
2019-09-17 22:59 ` Souza, Jose
2019-09-13 22:32 ` [PATCH 05/14] drm/i915/tgl: Add dkl phy registers José Roberto de Souza
2019-09-13 22:32 ` [PATCH 06/14] drm/i915/tgl: Add initial dkl pll support José Roberto de Souza
2019-09-13 22:32 ` [PATCH 07/14] drm/i915/tgl: Add support for dkl pll write José Roberto de Souza
2019-09-14 6:41 ` Lucas De Marchi
2019-09-16 8:48 ` Jani Nikula
2019-09-13 22:32 ` [PATCH 08/14] drm/i915/tgl: Add dkl phy programming sequences José Roberto de Souza
2019-09-13 22:32 ` [PATCH 09/14] drm/i915/icl: Unify disable and enable phy clock gating functions José Roberto de Souza
2019-09-13 22:32 ` [PATCH 10/14] drm/i915/tgl: Fix dkl phy register space addressing José Roberto de Souza
2019-09-14 7:26 ` Lucas De Marchi
2019-09-18 19:55 ` Souza, Jose [this message]
2019-09-13 22:32 ` [PATCH 11/14] drm/i915/tgl: Check the UC health of tc controllers after power on José Roberto de Souza
2019-09-13 22:32 ` [PATCH 12/14] drm/i915: Add dkl phy pll calculations José Roberto de Souza
2019-09-14 7:38 ` Lucas De Marchi
2019-09-13 22:32 ` [PATCH 13/14] drm/i915/tgl: Use dkl pll hardcoded values José Roberto de Souza
2019-09-13 22:32 ` [PATCH 14/14] drm/i915/tgl: initialize TC and TBT ports José Roberto de Souza
2019-09-13 22:53 ` ✗ Fi.CI.CHECKPATCH: warning for TGL TC enabling Patchwork
2019-09-13 23:12 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-15 7:07 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=108c346fb154a1f31061bfc5550f68ce5b328253.camel@intel.com \
--to=jose.souza@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.de.marchi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.