Re: [Intel-gfx] [PATCH 1/5] drm/i915/mtl: Add Support for C10, C20 PHY Message Bus

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

On Thu, 29 Sep 2022, Mika Kahola <mika.kahola@intel.com> wrote:
> From: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>
> XELPDP has C10 and C20 phys from Synopsys to drive displays. Each phy
> has a dedicated PIPE 5.2 Message bus for configuration. This message
> bus is used to configure the phy internal registers.

This looks like a silly intermediate step, adding a bunch of static
functions with __maybe_unused, just to be modified again in the next
patch.

>
> Bspec: 64599, 65100, 65101, 67610, 67636
>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com> (v4)
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 179 +++++++++++++++++++
>  1 file changed, 179 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_cx0_phy.c
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> new file mode 100644
> index 000000000000..7930b0255cfa
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include "intel_de.h"
> +#include "intel_uncore.h"

Do you use anything from intel_uncore.h directly, or is it just
intel_de.h?

> +
> +static void intel_cx0_bus_reset(struct drm_i915_private *i915, enum port port, int lane)
> +{
> +	enum phy phy = intel_port_to_phy(i915, port);
> +
> +	/* Bring the phy to idle. */
> +	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +		       XELPDP_PORT_M2P_TRANSACTION_RESET);
> +
> +	/* Wait for Idle Clear. */
> +	if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +				    XELPDP_PORT_M2P_TRANSACTION_RESET,
> +				    XELPDP_MSGBUS_TIMEOUT_SLOW)) {
> +		drm_err_once(&i915->drm, "Failed to bring PHY %c to idle. \n", phy_name(phy));
> +		return;
> +	}
> +
> +	intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), ~0);
> +	return;

Unnecessary return statement.

> +}
> +
> +__maybe_unused static u8 intel_cx0_read(struct drm_i915_private *i915, enum port port,
> +			 int lane, u16 addr)
> +{
> +	enum phy phy = intel_port_to_phy(i915, port);
> +	u32 val = 0;
> +	int attempts = 0;
> +
> +retry:
> +	if (attempts == 3) {
> +		drm_err_once(&i915->drm, "PHY %c Read %04x failed after %d retries. Status: 0x%x\n", phy_name(phy), addr, attempts, val ?: 0);
> +		return 0;
> +	}

The code looks like it would benefit from abstracting a non-retrying
read function that returns errors, with this function doing the retry
loop using a conventional for loop.

There's four copy-pasted bits of error handling here that's just error
prone.

> +
> +	/* Wait for pending transactions.*/
> +	if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +				    XELPDP_PORT_M2P_TRANSACTION_PENDING,
> +				    XELPDP_MSGBUS_TIMEOUT_SLOW)) {
> +		drm_dbg(&i915->drm, "PHY %c Timeout waiting for previous transaction to complete. Reset the bus and retry.\n", phy_name(phy));

drm_dbg_kms() throughout.

> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	/* Issue the read command. */
> +	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
> +		       XELPDP_PORT_M2P_COMMAND_READ |
> +		       XELPDP_PORT_M2P_ADDRESS(addr));
> +
> +	/* Wait for response ready. And read response.*/
> +	if (__intel_wait_for_register(&i915->uncore,
> +				      XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane),
> +				      XELPDP_PORT_P2M_RESPONSE_READY,
> +				      XELPDP_PORT_P2M_RESPONSE_READY,
> +				      XELPDP_MSGBUS_TIMEOUT_FAST_US,
> +				      XELPDP_MSGBUS_TIMEOUT_SLOW, &val)) {
> +		drm_dbg(&i915->drm, "PHY %c Timeout waiting for Read response ACK. Status: 0x%x\n", phy_name(phy), val);
> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	/* Check for error. */
> +	if (val & XELPDP_PORT_P2M_ERROR_SET) {
> +		drm_dbg(&i915->drm, "PHY %c Error occurred during read command. Status: 0x%x\n", phy_name(phy), val);
> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	/* Check for Read Ack. */
> +	if (REG_FIELD_GET(XELPDP_PORT_P2M_COMMAND_TYPE_MASK, val) !=
> +	    XELPDP_PORT_P2M_COMMAND_READ_ACK) {
> +		drm_dbg(&i915->drm, "PHY %c Not a Read response. MSGBUS Status: 0x%x.\n", phy_name(phy), val);
> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	/* Clear Response Ready flag.*/
> +	intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), ~0);

Blank line before return.

> +	return (u8)REG_FIELD_GET(XELPDP_PORT_P2M_DATA_MASK, val);

Unnecessary cast.

> +}
> +
> +static int intel_cx0_wait_cwrite_ack(struct drm_i915_private *i915,
> +				      enum port port, int lane)
> +{
> +	enum phy phy = intel_port_to_phy(i915, port);
> +	u32 val;
> +
> +	/* Check for write ack. */
> +	if (__intel_wait_for_register(&i915->uncore,
> +				      XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane),
> +				      XELPDP_PORT_P2M_RESPONSE_READY,
> +				      XELPDP_PORT_P2M_RESPONSE_READY,
> +				      XELPDP_MSGBUS_TIMEOUT_FAST_US,
> +				      XELPDP_MSGBUS_TIMEOUT_SLOW, &val)) {
> +		drm_dbg(&i915->drm, "PHY %c Timeout waiting for Committed message ACK. Status: 0x%x\n", phy_name(phy), val);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if ((REG_FIELD_GET(XELPDP_PORT_P2M_COMMAND_TYPE_MASK, val) !=
> +	     XELPDP_PORT_P2M_COMMAND_WRITE_ACK) || val & XELPDP_PORT_P2M_ERROR_SET) {
> +		drm_dbg(&i915->drm, "PHY %c Unexpected ACK received. MSGBUS STATUS: 0x%x.\n", phy_name(phy), val);
> +		return -EINVAL;
> +	}

This is also copy-paste duplicating the stuff in the previous
function. So why isn't this function used there?

> +
> +	return 0;
> +}
> +
> +__maybe_unused static void intel_cx0_write(struct drm_i915_private *i915, enum port port,
> +			    int lane, u16 addr, u8 data, bool committed)
> +{
> +	enum phy phy = intel_port_to_phy(i915, port);
> +	int attempts = 0;
> +
> +retry:
> +	if (attempts == 3) {
> +		drm_err_once(&i915->drm, "PHY %c Write %04x failed after %d retries.\n", phy_name(phy), addr, attempts);
> +		return;
> +	}

Same here with the retries as in the write. Have a lower level
non-retrying write function, and handle the rewrites at a different
abstraction level.

> +
> +	/* Wait for pending transactions.*/
> +	if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +				    XELPDP_PORT_M2P_TRANSACTION_PENDING,
> +				    XELPDP_MSGBUS_TIMEOUT_SLOW)) {
> +		drm_dbg(&i915->drm, "PHY %c Timeout waiting for previous transaction to complete. Reset the bus and retry.\n", phy_name(phy));
> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	/* Issue the write command. */
> +	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> +		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
> +		       (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> +		       XELPDP_PORT_M2P_COMMAND_WRITE_UNCOMMITTED) |
> +		       XELPDP_PORT_M2P_DATA(data) |
> +		       XELPDP_PORT_M2P_ADDRESS(addr));
> +
> +	/* Check for error. */
> +	if (committed) {
> +		if (intel_cx0_wait_cwrite_ack(i915, port, lane) < 0) {
> +			attempts++;
> +			intel_cx0_bus_reset(i915, port, lane);
> +			goto retry;
> +		}
> +	} else if ((intel_de_read(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(phy, lane)) &
> +			    XELPDP_PORT_P2M_ERROR_SET)) {
> +		drm_dbg(&i915->drm, "PHY %c Error occurred during write command.\n", phy_name(phy));
> +		attempts++;
> +		intel_cx0_bus_reset(i915, port, lane);
> +		goto retry;
> +	}
> +
> +	intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), ~0);
> +
> +	return;

Unnecessary return statement.

> +}
> +
> +__maybe_unused static void intel_cx0_rmw(struct drm_i915_private *i915, enum port port,
> +			  int lane, u16 addr, u8 clear, u8 set, bool committed)
> +{
> +	u8 old, val;
> +
> +	old = intel_cx0_read(i915, port, lane, addr);
> +	val = (old & ~clear) | set;
> +
> +	if (val != old)
> +		intel_cx0_write(i915, port, lane, addr, val, committed);
> +}

-- 
Jani Nikula, Intel Open Source Graphics Center