Re: [PATCH v4 06/12] drm/mediatek: mtk_dp: Rework register offsets for proper PHY usage

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 8/28/26 05:32, CK Hu (胡俊光) wrote:
> On Thu, 2026-07-09 at 13:31 +0200, AngeloGioacchino Del Regno wrote:
>> Rework all of the register offsets to subtract the DP PHY register
>> range from the MediaTek DisplayPort IP register range which starts
>> from TOP_OFFSET instead.
>>
>> This is done in preparation for adding support for registering the
>> PHY from devicetree, and also for properly handling PHY registers
>> in the PHY driver (instead of half and half...), which is also one
>> prerogative to add support for new IP versions in the future.
>>
>> Note that the current regmap_config was renamed to make sure that
>> it being legacy is mentioned, as a new config will be required in
>> future changes.
>>
>> Of course, with the TOP offset being related to hardware registers
>> this will never change for the currently supported SoCs, hence for
>> simplifying code, the MTK_DP_TOP_OFFSET_LEGACY is added statically
>> to the legacy regmap configuration while, on the other hand, it is
>> added dynamically to the mtk_dp structure, as the latter is used
>> to add an offset to every register write in the mtk_dp driver.
>>
>> The regmap_config provided reg_base (which does exactly the same)
>> cannot be used here, because the PHY driver is using the very same
>> regmap pointer, and the offset shall not be taken into account in
>> writes performed by the PHY driver (only ones performed by mtk_dp!).
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_dp.c     | 101 +++++-----
>>   drivers/gpu/drm/mediatek/mtk_dp_reg.h | 255 ++++++++++++--------------
>>   2 files changed, 179 insertions(+), 177 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> index b50264ca1f31..b2e5fa0914bc 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -35,6 +35,27 @@
>>   
>>   #include "mtk_dp_reg.h"
>>   
>> +/* PHY Registers - for legacy only */
>> +#define DP_PHY_GLB_BIAS_GEN_00		0x0
>> +#  define RG_XTP_GLB_BIAS_INTR_CTRL	GENMASK(20, 16)
> 
> Align the register definition indent to other mtk dp register definition.
> 

The plan is to make cosmetic changes later to align all register definitions
in the entire mediatek-drm to follow that, because this indent style is a bit
more readable, especially when definitions get a bit more complex.

#define REGISTER
#  define REG_BITMASK0
#    define BITMASK_RELATIVE_VALUE
#  define REG_BITMASK1

>> +#define DP_PHY_GLB_DPAUX_TX		0x8
>> +#  define RG_CKM_PT0_CKTX_IMPSEL	GENMASK(23, 20)
>> +#define MTK_DP_0034			0x34
>> +#  define DA_CKM_CKTX0_EN_FORCE_EN	BIT(10)
>> +#define DP_PHY_LANE_TX_0		0x104
>> +#define DP_PHY_LANE_TX_1		0x204
>> +#define DP_PHY_LANE_TX_2		0x304
>> +#define DP_PHY_LANE_TX_3		0x404
>> +#  define RG_XTP_LNx_TX_IMPSEL_PMOS	GENMASK(15, 12)
>> +#  define RG_XTP_LNx_TX_IMPSEL_NMOS	GENMASK(19, 16)
>> +#define DP_PHY_AUX_RX_CTL		0x1040
>> +#  define RG_DPAUX_RX_VALID_DEGLITCH_EN	BIT(2)
>> +#  define RG_XTP_GLB_CKDET_EN		BIT(1)
>> +#  define RG_DPAUX_RX_EN		BIT(0)
>> +
> 
> [snip]
> 
>>   static int mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 val)
>>   {
>> -	int ret = regmap_write(mtk_dp->regs, offset, val);
>> +	int ret = regmap_write(mtk_dp->regs, offset + mtk_dp->legacy_regoff, val);
>>   
>>   	if (ret)
>>   		dev_err(mtk_dp->dev,
>> @@ -436,7 +460,7 @@ static int mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 val)
>>   static int mtk_dp_update_bits(struct mtk_dp *mtk_dp, u32 offset,
>>   			      u32 val, u32 mask)
>>   {
>> -	int ret = regmap_update_bits(mtk_dp->regs, offset, mask, val);
>> +	int ret = regmap_update_bits(mtk_dp->regs, offset + mtk_dp->legacy_regoff, mask, val);
>>   
>>   	if (ret)
>>   		dev_err(mtk_dp->dev,
>> @@ -566,7 +590,8 @@ static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
>>   			   DP_TEST_COLOR_FORMAT_MASK);
>>   
>>   	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C,
>> -			   val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK);
>> +			   FIELD_PREP(PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK, val),
> 
> This is not related to register offset, so move to other patch.
> 
>> +			   PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK);
>>   	return 0;
>>   }
>>   
> 
> [snip]
> 
>>   static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
>> @@ -1401,21 +1414,21 @@ static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>>   			   SW_RST_B_PHYD, SW_RST_B_PHYD);
>>   	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>>   			   DP_PWR_STATE_BANDGAP_TPLL, DP_PWR_STATE_MASK);
>> -	mtk_dp_write(mtk_dp, MTK_DP_1040,
>> +	regmap_write(mtk_dp->regs, DP_PHY_AUX_RX_CTL,
>>   		     RG_DPAUX_RX_VALID_DEGLITCH_EN | RG_XTP_GLB_CKDET_EN |
>>   		     RG_DPAUX_RX_EN);
>> -	mtk_dp_update_bits(mtk_dp, MTK_DP_0034, 0, DA_CKM_CKTX0_EN_FORCE_EN);
>> +	regmap_clear_bits(mtk_dp->regs, MTK_DP_0034, DA_CKM_CKTX0_EN_FORCE_EN);
> 
> How do you know MTK_DP_1040 has a meaningful name DP_PHY_AUX_RX_CTL?

Datasheet states the name.

> Why don't you know MTK_DP_0034 has a meaningful name?

...datasheet doesn't state the name :-)

> 
>>   }
>>   
>>
> 
> [snip]
> 
> 
>> @@ -69,43 +29,59 @@
>>   #define DP_TX2_PRE_EMPH_MASK			GENMASK(19, 18)
>>   #define DP_TX3_VOLT_SWING_MASK			GENMASK(25, 24)
>>   #define DP_TX3_PRE_EMPH_MASK			GENMASK(27, 26)
>> -#define MTK_DP_TOP_RESET_AND_PROBE	0x2020
>> +#define MTK_DP_TOP_RESET_AND_PROBE	0x20
>>   #define SW_RST_B_PHYD				BIT(4)
>> -#define MTK_DP_TOP_IRQ_MASK		0x202c
>> +#define RG_SW_RST_MASK				GENMASK(7, 0)
>> +#define RG_SW_RST				0xff
>> +#define RG_PROBE_LOW_SEL_MASK			GENMASK(18, 16)
>> +#define RG_PROBE_LOW_SEL			BIT(16)
>> +#define RG_PROBE_LOW_HIGH_SWAP_MASK		BIT(23)
>> +#define RG_PROBE_LOW_HIGH_SWAP			BIT(23)
> 
> These new definition is not related to this patch, so move to other patch.
> 
>> +
>> +#define MTK_DP_TOP_IRQ_MASK		0x2c
>> +#define ENCODER_IRQ_MSK				BIT(0)
>> +#define TRANS_IRQ_MSK				BIT(1)
> 
> Ditto.
> 
>>   #define IRQ_MASK_AUX_TOP_IRQ			BIT(2)
>> -#define MTK_DP_TOP_MEM_PD		0x2038
>> +
>> +#define MTK_DP_TOP_MEM_PD		0x38
>>   #define MEM_ISO_EN				BIT(0)
>>   #define FUSE_SEL				BIT(2)
>>   
>> -/* offset: ENC0_OFFSET (0x3000) */
>> -#define MTK_DP_ENC0_P0_3000			0x3000
>> +#define EDP_TX_TOP_CLKGEN_0		0x74
>> +#define EDP_TX_TOP_CLKGEN_REST_MASK		0xf
>> +#define EDP_TX_TOP_CLKGEN_REST_VALUE		0xf
> 
> Ditto.
> 
>> +
>> +/* offset: ENC0_OFFSET (0x1000) */
>> +#define MTK_DP_ENC0_P0_3000			0x1000
>>   #define LANE_NUM_DP_ENC0_P0_MASK			GENMASK(1, 0)
>>   #define VIDEO_MUTE_SW_DP_ENC0_P0			BIT(2)
>>   #define VIDEO_MUTE_SEL_DP_ENC0_P0			BIT(3)
>>   #define ENHANCED_FRAME_EN_DP_ENC0_P0			BIT(4)
>> -#define MTK_DP_ENC0_P0_3004			0x3004
>> +#define DP_I_MODE_ENABLE				BIT(6)
>> +#define REG_BS_SYMBOL_CNT_RESET				BIT(7)
> 
> Ditto.

Ok

> 
>> +#define MTK_DP_ENC0_P0_3004			0x1004
>>   #define VIDEO_M_CODE_SEL_DP_ENC0_P0_MASK		BIT(8)
>>   #define DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0		BIT(9)
>>   #define SDP_RESET_SW_DP_ENC0_P0				BIT(13)
>>
> 
> [snip]
> 
>>   #define VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT		(3 << 8)
>>   #define VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT		(4 << 8)
>>   #define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK		GENMASK(14, 12)
>> -#define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB		(0 << 12)
>> -#define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422		(1 << 12)
>> -#define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420		(2 << 12)
>> +#  define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB		0
>> +#  define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422	1
>> +#  define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420	2
> 
> You change the value. Why?
> 

That's done in order to use the FIELD_PREP macro.

Cheers,
Angelo

> Regards,
> CK
> 
>> +#  define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YONLY		3
>> +#  define PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RAW		4
>>   #define VIDEO_MN_GEN_EN_DP_ENC0_P0			BIT(15)
>> -#define MTK_DP_ENC0_P0_3040			0x3040
>> +#define MTK_DP_ENC0_P0_3040			0x1040
>>   #define SDP_DOWN_CNT_DP_ENC0_P0_VAL			0x20
>>   #define SDP_DOWN_CNT_INIT_DP_ENC0_P0_MASK		GENMASK(11, 0)
>>
>