> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Friday, September 30, 2022 12:05 PM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/5] drm/i915/mtl: Add Support for C10, C20 PHY
> Message Bus
>
> On Thu, 29 Sep 2022, Mika Kahola <mika.kahola@xxxxxxxxx> wrote:
> > From: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> >
> > 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.
Yes, this was an intermediate step to get around gcc warn on unused functions.
>
> >
> > Bspec: 64599, 65100, 65101, 67610, 67636
> >
> > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx> (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?
I don't think this C10 patch series use intel_uncore.h directly. I have to double check that though. If not this intel_uncore.h is not needed.
>
> > +
> > +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;
Yeah, true.
>
> 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.
Yes, I could do some tidying up here
>
> 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.
I will delete this line
>
> > + return (u8)REG_FIELD_GET(XELPDP_PORT_P2M_DATA_MASK, val);
>
> Unnecessary cast.
Fixing it with next set of patches.
>
> > +}
> > +
> > +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?
This would benefit an own function. I will fix that in the next series of patches.
>
> > +
> > + 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.
I'll try to rephrase these.
>
> > +
> > + /* 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.
Yes.
Thanks for the comments and a review. I will try to address these finding with the next iteration of this patch series.
-Mika-
>
> > +}
> > +
> > +__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
