On Wed, Nov 01, 2023 at 12:51:12PM +0200, Jani Nikula wrote: > On Wed, 01 Nov 2023, Mika Kahola <mika.kahola@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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
