Re: [Intel-gfx] [Intel-xe] [PATCH 18/42] drm/i915/xe2lpd: Move registers to PICA
On Thu, Aug 24, 2023 at 11:34:59AM +0300, Jani Nikula wrote: On Wed, 23 Aug 2023, Lucas De Marchi wrote: diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h index cb5d1be2ba19..4b5b9a97142d 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h @@ -6,13 +6,15 @@ #ifndef __INTEL_CX0_PHY_REGS_H__ #define __INTEL_CX0_PHY_REGS_H__ +#include "i915_drv.h" Please don't do this. Please don't add inline functions that depend on i915_drv.h etc. being included from headers. This simple headers just changed to including like half the headers in the entire driver. It's that bad. I think the main question is why does anything other than intel_cx0_phy_regs.c need the helpers? It's probably the division between that and intel_ddi.c that's wrong in the first place. because on platform N-1 the register was on DDI and on platform N it moved to the phy. So how would the divide be? Lucas De Marchi That's actually been one of the benefits of splitting the register macros by area; you can tell what registers are used where, and sometimes it gives bad code smells about stuff being accessed in the wrong place. BR, Jani. #include "i915_reg_defs.h" +#include "intel_display_limits.h" #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A 0x64040 #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B 0x64140 #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC1 0x16F240 #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC2 0x16F440 -#define XELPDP_PORT_M2P_MSGBUS_CTL(port, lane) _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ +#define XELPDP_PORT_M2P_MSGBUS_CTL(idx, lane) _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC1, \ @@ -27,7 +29,7 @@ #define XELPDP_PORT_M2P_TRANSACTION_RESETREG_BIT(15) #define XELPDP_PORT_M2P_ADDRESS_MASK REG_GENMASK(11, 0) #define XELPDP_PORT_M2P_ADDRESS(val) REG_FIELD_PREP(XELPDP_PORT_M2P_ADDRESS_MASK, val) -#define XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane) _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ +#define XELPDP_PORT_P2M_MSGBUS_STATUS(idx, lane) _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B, \ _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC1, \ @@ -54,7 +56,7 @@ #define _XELPDP_PORT_BUF_CTL1_LN0_B0x64104 #define _XELPDP_PORT_BUF_CTL1_LN0_USBC10x16F200 #define _XELPDP_PORT_BUF_CTL1_LN0_USBC20x16F400 -#define XELPDP_PORT_BUF_CTL1(port) _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ +#define XELPDP_PORT_BUF_CTL1(idx) _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ _XELPDP_PORT_BUF_CTL1_LN0_A, \ _XELPDP_PORT_BUF_CTL1_LN0_B, \ _XELPDP_PORT_BUF_CTL1_LN0_USBC1, \ @@ -75,7 +77,7 @@ #define XELPDP_PORT_WIDTH_MASK REG_GENMASK(3, 1) #define XELPDP_PORT_WIDTH(val) REG_FIELD_PREP(XELPDP_PORT_WIDTH_MASK, val) -#define XELPDP_PORT_BUF_CTL2(port) _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ +#define XELPDP_PORT_BUF_CTL2(idx) _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ _XELPDP_PORT_BUF_CTL1_LN0_A, \ _XELPDP_PORT_BUF_CTL1_LN0_B, \ _XELPDP_PORT_BUF_CTL1_LN0_USBC1, \ @@ -95,7 +97,7 @@ #define XELPDP_POWER_STATE_READY_MASKREG_GENMASK(7, 4) #define XELPDP_POWER_STATE_READY(val) REG_FIELD_PREP(XELPDP_POWER_STATE_READY_MASK, val) -#define XELPDP_PORT_BUF_CTL3(port) _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ +#define XELPDP_PORT_BUF_CTL3(idx) _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ _XELPDP_PORT_BUF_CTL1_L
Re: [Intel-gfx] [Intel-xe] [PATCH 18/42] drm/i915/xe2lpd: Move registers to PICA
On Wed, 23 Aug 2023, Lucas De Marchi wrote: > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > index cb5d1be2ba19..4b5b9a97142d 100644 > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h > @@ -6,13 +6,15 @@ > #ifndef __INTEL_CX0_PHY_REGS_H__ > #define __INTEL_CX0_PHY_REGS_H__ > > +#include "i915_drv.h" Please don't do this. Please don't add inline functions that depend on i915_drv.h etc. being included from headers. This simple headers just changed to including like half the headers in the entire driver. It's that bad. I think the main question is why does anything other than intel_cx0_phy_regs.c need the helpers? It's probably the division between that and intel_ddi.c that's wrong in the first place. That's actually been one of the benefits of splitting the register macros by area; you can tell what registers are used where, and sometimes it gives bad code smells about stuff being accessed in the wrong place. BR, Jani. > #include "i915_reg_defs.h" > +#include "intel_display_limits.h" > > #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A0x64040 > #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B0x64140 > #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC10x16F240 > #define _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC20x16F440 > -#define XELPDP_PORT_M2P_MSGBUS_CTL(port, lane) > _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ > +#define XELPDP_PORT_M2P_MSGBUS_CTL(idx, lane) > _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC1, \ > @@ -27,7 +29,7 @@ > #define XELPDP_PORT_M2P_TRANSACTION_RESET REG_BIT(15) > #define XELPDP_PORT_M2P_ADDRESS_MASK REG_GENMASK(11, > 0) > #define XELPDP_PORT_M2P_ADDRESS(val) > REG_FIELD_PREP(XELPDP_PORT_M2P_ADDRESS_MASK, val) > -#define XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane) > _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ > +#define XELPDP_PORT_P2M_MSGBUS_STATUS(idx, lane) > _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_A, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_B, \ > > _XELPDP_PORT_M2P_MSGBUS_CTL_LN0_USBC1, \ > @@ -54,7 +56,7 @@ > #define _XELPDP_PORT_BUF_CTL1_LN0_B 0x64104 > #define _XELPDP_PORT_BUF_CTL1_LN0_USBC1 0x16F200 > #define _XELPDP_PORT_BUF_CTL1_LN0_USBC2 0x16F400 > -#define XELPDP_PORT_BUF_CTL1(port) > _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ > +#define XELPDP_PORT_BUF_CTL1(idx) > _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ > > _XELPDP_PORT_BUF_CTL1_LN0_A, \ > > _XELPDP_PORT_BUF_CTL1_LN0_B, \ > > _XELPDP_PORT_BUF_CTL1_LN0_USBC1, \ > @@ -75,7 +77,7 @@ > #define XELPDP_PORT_WIDTH_MASK REG_GENMASK(3, 1) > #define XELPDP_PORT_WIDTH(val) > REG_FIELD_PREP(XELPDP_PORT_WIDTH_MASK, val) > > -#define XELPDP_PORT_BUF_CTL2(port) > _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ > +#define XELPDP_PORT_BUF_CTL2(idx) > _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ > > _XELPDP_PORT_BUF_CTL1_LN0_A, \ > > _XELPDP_PORT_BUF_CTL1_LN0_B, \ > > _XELPDP_PORT_BUF_CTL1_LN0_USBC1, \ > @@ -95,7 +97,7 @@ > #define XELPDP_POWER_STATE_READY_MASK REG_GENMASK(7, > 4) > #define XELPDP_POWER_STATE_READY(val) > REG_FIELD_PREP(XELPDP_POWER_STATE_READY_MASK, val) > > -#define XELPDP_PORT_BUF_CTL3(port) > _MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \ > +#define XELPDP_PORT_BUF_CTL3(idx) > _MMIO(_PICK_EVEN_2RANGES(idx, PORT_TC1, \ > > _XELPDP_PORT_BUF_CTL1_LN0_A, \ >
Re: [Intel-gfx] [Intel-xe] [PATCH 18/42] drm/i915/xe2lpd: Move registers to PICA
On Wed, Aug 23, 2023 at 10:07:16AM -0700, Lucas De Marchi wrote:
> Some registers for DDI A/B moved to PICA and now follow the same format
> as the ones for the PORT_TC ports. The wrapper here deals with 2 issues:
>
> - Share the implementation between xe2lpd and previous
> platforms: there are minor layout changes, it's mostly the
> register location that changed
> - Handle offsets after TC ports
>
> Signed-off-by: Lucas De Marchi
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 81 ++-
> .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 71 ++--
> drivers/gpu/drm/i915/display/intel_ddi.c | 18 +++--
> 3 files changed, 117 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index a5918bf30c31..6533ec417806 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -98,7 +98,7 @@ static void intel_cx0_phy_transaction_end(struct
> intel_encoder *encoder, intel_w
> static void intel_clear_response_ready_flag(struct drm_i915_private *i915,
> enum port port, int lane)
> {
> - intel_de_rmw(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane),
> + intel_de_rmw(i915, xelpdp_port_p2m_msgbus_status_reg(i915, port, lane),
>0, XELPDP_PORT_P2M_RESPONSE_READY |
> XELPDP_PORT_P2M_ERROR_SET);
> }
>
> @@ -106,10 +106,10 @@ static void intel_cx0_bus_reset(struct drm_i915_private
> *i915, enum port port, i
> {
> enum phy phy = intel_port_to_phy(i915, port);
>
> - intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> + intel_de_write(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915, port, lane),
> XELPDP_PORT_M2P_TRANSACTION_RESET);
>
> - if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port,
> lane),
> + if (intel_de_wait_for_clear(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915,
> 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));
> @@ -125,7 +125,7 @@ static int intel_cx0_wait_for_ack(struct drm_i915_private
> *i915, enum port port,
> enum phy phy = intel_port_to_phy(i915, port);
>
> if (__intel_de_wait_for_register(i915,
> - XELPDP_PORT_P2M_MSGBUS_STATUS(port,
> lane),
> +
> xelpdp_port_p2m_msgbus_status_reg(i915, port, lane),
>XELPDP_PORT_P2M_RESPONSE_READY,
>XELPDP_PORT_P2M_RESPONSE_READY,
>XELPDP_MSGBUS_TIMEOUT_FAST_US,
> @@ -160,7 +160,7 @@ static int __intel_cx0_read_once(struct drm_i915_private
> *i915, enum port port,
> int ack;
> u32 val;
>
> - if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port,
> lane),
> + if (intel_de_wait_for_clear(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915,
> port, lane),
> XELPDP_PORT_M2P_TRANSACTION_PENDING,
> XELPDP_MSGBUS_TIMEOUT_SLOW)) {
> drm_dbg_kms(&i915->drm,
> @@ -169,7 +169,7 @@ static int __intel_cx0_read_once(struct drm_i915_private
> *i915, enum port port,
> return -ETIMEDOUT;
> }
>
> - intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> + intel_de_write(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915, port, lane),
> XELPDP_PORT_M2P_TRANSACTION_PENDING |
> XELPDP_PORT_M2P_COMMAND_READ |
> XELPDP_PORT_M2P_ADDRESS(addr));
> @@ -220,7 +220,7 @@ static int __intel_cx0_write_once(struct drm_i915_private
> *i915, enum port port,
> int ack;
> u32 val;
>
> - if (intel_de_wait_for_clear(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port,
> lane),
> + if (intel_de_wait_for_clear(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915,
> port, lane),
> XELPDP_PORT_M2P_TRANSACTION_PENDING,
> XELPDP_MSGBUS_TIMEOUT_SLOW)) {
> drm_dbg_kms(&i915->drm,
> @@ -229,14 +229,14 @@ static int __intel_cx0_write_once(struct
> drm_i915_private *i915, enum port port,
> return -ETIMEDOUT;
> }
>
> - intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> + intel_de_write(i915, xelpdp_port_m2p_msgbus_ctl_reg(i915, 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_
