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

[Jani Nikula <jani.nikula@linux.intel.com>]

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);

> +
>  	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