Re: [Intel-gfx] [PATCH] drm/i915/mtl: Clear possible sticky bits on PICA message bus

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 01, 2023 at 12:51:12PM +0200, Jani Nikula wrote:
> On Wed, 01 Nov 2023, Mika Kahola <mika.kahola@intel.com> wrote:
> > It is possible that sticky bits or error bits are left on
> > message bus status register. Reading and then writing the
> > value back to messagebus status register clears all possible
> > sticky bits and errors.
> 
> Note that I don't know if this is the right thing to do, or the right
> place to do this, but I'll just comment on the *implementation*,
> i.e. please wait for proper review before addressing my comments.
> 
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index b2ad4c6172f6..f439f0c7b400 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -195,6 +195,13 @@ static int __intel_cx0_read_once(struct drm_i915_private *i915, enum port port,
> >  		return -ETIMEDOUT;
> >  	}
> >  
> > +	/*
> > +	 * write XELPDP_PORT_P2M_MSGBUS_STATUS register after read to clear
> > +	 * any error sticky bits set from previous transactions
> > +	 */
> > +	val = intel_de_read(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane));
> > +	intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), val);
> 
> I think it's slightly confusing to use val here, as it's then passed on
> to intel_cx0_wait_for_ack() and you're left wondering if that's required
> or just reuse of the val variable.
> 
> This should do the same thing in one line, without reusing val:
> 
> 	intel_de_rmw(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), 0, 0);

Why is this not just a intel_clear_response_ready_flag()?

Side note: that function name is somewhat misleading...

> 
> > +
> >  	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> >  		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >  		       XELPDP_PORT_M2P_COMMAND_READ |
> > @@ -262,6 +269,13 @@ static int __intel_cx0_write_once(struct drm_i915_private *i915, enum port port,
> >  		return -ETIMEDOUT;
> >  	}
> >  
> > +	/*
> > +	 * write XELPDP_PORT_P2M_MSGBUS_STATUS register after read to clear
> > +	 * any error sticky bits set from previous transactions
> > +	 */
> > +	val = intel_de_read(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane));
> > +	intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), val);
> > +
> >  	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> >  		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >  		       (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel