Re: [PATCH 6/9] drm/meson: dw-hdmi: convert to regmap

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

On 30/07/2024 14:50, Jerome Brunet wrote:
> The Amlogic mixes direct register access and regmap ones, with several
> custom helpers. Using a single API makes rework and maintenance easier.
>
> Convert the Amlogic phy driver to regmap and use it to have more consistent
> access to the registers in the driver, with less custom helpers. Add
> register bit definitions when missing.
>
> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 475 ++++++++++++--------------
>   drivers/gpu/drm/meson/meson_dw_hdmi.h |  49 +--
>   2 files changed, 239 insertions(+), 285 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 47aa3e184e98..7c39e5c99043 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -90,16 +90,25 @@
>    * - CEC Management
>    */
>   
> -/* TOP Block Communication Channel */
> -#define HDMITX_TOP_ADDR_REG	0x0
> -#define HDMITX_TOP_DATA_REG	0x4
> -#define HDMITX_TOP_CTRL_REG	0x8
> -#define HDMITX_TOP_G12A_OFFSET	0x8000
> +/* Indirect channel definition for GX */
> +#define HDMITX_TOP_REGS		0x0
> +#define HDMITX_DWC_REGS		0x10
> +
> +#define GX_ADDR_OFFSET		0x0
> +#define GX_DATA_OFFSET		0x4
> +#define GX_CTRL_OFFSET		0x8

I don't see the point renaming thos defines

> +#define  GX_CTRL_APB3_ERRFAIL	BIT(15)
>   
> -/* Controller Communication Channel */
> -#define HDMITX_DWC_ADDR_REG	0x10
> -#define HDMITX_DWC_DATA_REG	0x14
> -#define HDMITX_DWC_CTRL_REG	0x18
> +/*
> + * NOTE: G12 use direct addressing:
> + * Ideally it should receive one memory region for each of the top
> + * and dwc register regions but fixing this would require to change
> + * the DT bindings. Doing so is a pain. Keep the region as it and work
> + * around the problem, at least for now.
> + * Future supported SoCs should properly describe the regions in the
> + * DT bindings instead of using this trick.

well I disagree here, there's a single memory region for the HDMITX module,
and the DWC region is controlled by the TOP registers, so the DWC region
_cannot_ be accessed without correctly configuring the TOP registers.

So I disagree strongly with this comment.

> + */
> +#define HDMITX_TOP_G12A_OFFSET	0x8000
>   
>   /* HHI Registers */
>   #define HHI_MEM_PD_REG0		0x100 /* 0x40 */
> @@ -108,28 +117,59 @@
>   #define HHI_HDMI_PHY_CNTL1	0x3a4 /* 0xe9 */
>   #define  PHY_CNTL1_INIT		0x03900000
>   #define  PHY_INVERT		BIT(17)
> +#define  PHY_FIFOS		GENMASK(3, 2)
> +#define  PHY_CLOCK_EN		BIT(1)
> +#define  PHY_SOFT_RST		BIT(0)

Please move those changes before or after this patch

>   #define HHI_HDMI_PHY_CNTL2	0x3a8 /* 0xea */
>   #define HHI_HDMI_PHY_CNTL3	0x3ac /* 0xeb */
>   #define HHI_HDMI_PHY_CNTL4	0x3b0 /* 0xec */
>   #define HHI_HDMI_PHY_CNTL5	0x3b4 /* 0xed */
>   
> -static DEFINE_SPINLOCK(reg_lock);
> -
> -struct meson_dw_hdmi;
> -
>   struct meson_dw_hdmi_data {
> -	unsigned int	(*top_read)(struct meson_dw_hdmi *dw_hdmi,
> -				    unsigned int addr);
> -	void		(*top_write)(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr, unsigned int data);
> -	unsigned int	(*dwc_read)(struct meson_dw_hdmi *dw_hdmi,
> -				    unsigned int addr);
> -	void		(*dwc_write)(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr, unsigned int data);
> +	int (*reg_init)(struct device *dev);
>   	u32 cntl0_init;
>   	u32 cntl1_init;
>   };
>   
> +static int hdmi_tx_indirect_reg_read(void *context,
> +					 unsigned int reg,
> +					 unsigned int *result)
> +{
> +	void __iomem *base = context;
> +
> +	/* Must write the read address twice ... */
> +	writel(reg, base + GX_ADDR_OFFSET);
> +	writel(reg, base + GX_ADDR_OFFSET);
> +
> +	/* ... and read the data twice as well */
> +	*result = readl(base + GX_DATA_OFFSET);
> +	*result = readl(base + GX_DATA_OFFSET);

Why did you change the comments ?

> +
> +	return 0;
> +}
> +
> +static int hdmi_tx_indirect_reg_write(void *context,
> +				      unsigned int reg,
> +				      unsigned int val)
> +{
> +	void __iomem *base = context;
> +
> +	/* Must write the read address twice ... */
> +	writel(reg, base + GX_ADDR_OFFSET);
> +	writel(reg, base + GX_ADDR_OFFSET);
> +
> +	/* ... but write the data only once */
> +	writel(val, base + GX_DATA_OFFSET);

Ditto ? were they wrong ?

> +
> +	return 0;
> +}
> +
> +static const struct regmap_bus hdmi_tx_indirect_mmio = {
> +	.fast_io = true,
> +	.reg_read = hdmi_tx_indirect_reg_read,
> +	.reg_write = hdmi_tx_indirect_reg_write,
> +};
> +
>   struct meson_dw_hdmi {
>   	struct dw_hdmi_plat_data dw_plat_data;
>   	struct meson_drm *priv;
> @@ -139,9 +179,10 @@ struct meson_dw_hdmi {
>   	struct reset_control *hdmitx_apb;
>   	struct reset_control *hdmitx_ctrl;
>   	struct reset_control *hdmitx_phy;
> -	u32 irq_stat;
> +	unsigned int irq_stat;
>   	struct dw_hdmi *hdmi;
>   	struct drm_bridge *bridge;
> +	struct regmap *top;

The name could be better, like top_regs or top_regmap

>   };
>   
>   static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
> @@ -150,136 +191,6 @@ static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
>   	return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
>   }
>   
> -/* PHY (via TOP bridge) and Controller dedicated register interface */
> -
> -static unsigned int dw_hdmi_top_read(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr)
> -{
> -	unsigned long flags;
> -	unsigned int data;
> -
> -	spin_lock_irqsave(&reg_lock, flags);
> -
> -	/* ADDR must be written twice */
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
> -
> -	/* Read needs a second DATA read */
> -	data = readl(dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
> -	data = readl(dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
> -
> -	spin_unlock_irqrestore(&reg_lock, flags);
> -
> -	return data;
> -}
> -
> -static unsigned int dw_hdmi_g12a_top_read(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr)
> -{
> -	return readl(dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET + (addr << 2));
> -}
> -
> -static inline void dw_hdmi_top_write(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr, unsigned int data)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&reg_lock, flags);
> -
> -	/* ADDR must be written twice */
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
> -
> -	/* Write needs single DATA write */
> -	writel(data, dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
> -
> -	spin_unlock_irqrestore(&reg_lock, flags);
> -}
> -
> -static inline void dw_hdmi_g12a_top_write(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr, unsigned int data)
> -{
> -	writel(data, dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET + (addr << 2));
> -}
> -
> -/* Helper to change specific bits in PHY registers */
> -static inline void dw_hdmi_top_write_bits(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr,
> -					  unsigned int mask,
> -					  unsigned int val)
> -{
> -	unsigned int data = dw_hdmi->data->top_read(dw_hdmi, addr);
> -
> -	data &= ~mask;
> -	data |= val;
> -
> -	dw_hdmi->data->top_write(dw_hdmi, addr, data);
> -}
> -
> -static unsigned int dw_hdmi_dwc_read(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr)
> -{
> -	unsigned long flags;
> -	unsigned int data;
> -
> -	spin_lock_irqsave(&reg_lock, flags);
> -
> -	/* ADDR must be written twice */
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
> -
> -	/* Read needs a second DATA read */
> -	data = readl(dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
> -	data = readl(dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
> -
> -	spin_unlock_irqrestore(&reg_lock, flags);
> -
> -	return data;
> -}
> -
> -static unsigned int dw_hdmi_g12a_dwc_read(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr)
> -{
> -	return readb(dw_hdmi->hdmitx + addr);
> -}
> -
> -static inline void dw_hdmi_dwc_write(struct meson_dw_hdmi *dw_hdmi,
> -				     unsigned int addr, unsigned int data)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&reg_lock, flags);
> -
> -	/* ADDR must be written twice */
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
> -	writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
> -
> -	/* Write needs single DATA write */
> -	writel(data, dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
> -
> -	spin_unlock_irqrestore(&reg_lock, flags);
> -}
> -
> -static inline void dw_hdmi_g12a_dwc_write(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr, unsigned int data)
> -{
> -	writeb(data, dw_hdmi->hdmitx + addr);
> -}
> -
> -/* Helper to change specific bits in controller registers */
> -static inline void dw_hdmi_dwc_write_bits(struct meson_dw_hdmi *dw_hdmi,
> -					  unsigned int addr,
> -					  unsigned int mask,
> -					  unsigned int val)
> -{
> -	unsigned int data = dw_hdmi->data->dwc_read(dw_hdmi, addr);
> -
> -	data &= ~mask;
> -	data |= val;
> -
> -	dw_hdmi->data->dwc_write(dw_hdmi, addr, data);
> -}
> -
>   /* Bridge */
>   
>   /* Setup PHY bandwidth modes */
> @@ -353,13 +264,15 @@ static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
>   	struct meson_drm *priv = dw_hdmi->priv;
>   
>   	/* Enable and software reset */
> -	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0xf);
> -
> +	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
> +			   PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST,
> +			   PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST);
>   	mdelay(2);
>   
>   	/* Enable and unreset */
> -	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0xe);
> -
> +	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
> +			   PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST,
> +			   PHY_FIFOS | PHY_CLOCK_EN);
>   	mdelay(2);
>   }
>   
> @@ -382,27 +295,30 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>   
>   	/* TMDS pattern setup */
>   	if (mode->clock > 340000 && !mode_is_420) {
> -		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
> -				  0);
> -		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
> -				  0x03ff03ff);
> +		regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_01,
> +			     0);
> +		regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_23,
> +			     0x03ff03ff);
>   	} else {
> -		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
> -				  0x001f001f);
> -		dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
> -				  0x001f001f);
> +		regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_01,
> +			     0x001f001f);
> +		regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_23,
> +			     0x001f001f);
>   	}
>   
>   	/* Load TMDS pattern */
> -	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x1);
> +	regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_CNTL,
> +		     TOP_TDMS_CLK_PTTN_LOAD);
>   	msleep(20);
> -	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x2);
> +	regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_CNTL,
> +		     TOP_TDMS_CLK_PTTN_SHFT);
>   
>   	/* Setup PHY parameters */
>   	meson_hdmi_phy_setup_mode(dw_hdmi, mode, mode_is_420);
>   
>   	/* Disable clock, fifo, fifo_wr */
> -	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0);
> +	regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
> +			   PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST, 0);
>   
>   	dw_hdmi_set_high_tmds_clock_ratio(hdmi, display);
>   
> @@ -433,8 +349,11 @@ static enum drm_connector_status dw_hdmi_read_hpd(struct dw_hdmi *hdmi,
>   			     void *data)
>   {
>   	struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
> +	unsigned int stat;
>   
> -	return !!dw_hdmi->data->top_read(dw_hdmi, HDMITX_TOP_STAT0) ?
> +	regmap_read(dw_hdmi->top, HDMITX_TOP_STAT0, &stat);
> +
> +	return !!stat ?
>   		connector_status_connected : connector_status_disconnected;
>   }
>   
> @@ -444,17 +363,18 @@ static void dw_hdmi_setup_hpd(struct dw_hdmi *hdmi,
>   	struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
>   
>   	/* Setup HPD Filter */
> -	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_HPD_FILTER,
> -			  (0xa << 12) | 0xa0);
> +	regmap_write(dw_hdmi->top, HDMITX_TOP_HPD_FILTER,
> +		     FIELD_PREP(TOP_HPD_GLITCH_WIDTH, 10) |
> +		     FIELD_PREP(TOP_HPD_VALID_WIDTH, 160));
>   
>   	/* Clear interrupts */
> -	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_INTR_STAT_CLR,
> -			  HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL);
> +	regmap_write(dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR,
> +		     TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL);
>   
>   	/* Unmask interrupts */
> -	dw_hdmi_top_write_bits(dw_hdmi, HDMITX_TOP_INTR_MASKN,
> -			HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL,
> -			HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL);
> +	regmap_update_bits(dw_hdmi->top, HDMITX_TOP_INTR_MASKN,
> +			   TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL,
> +			   TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL);
>   }
>   
>   static const struct dw_hdmi_phy_ops meson_dw_hdmi_phy_ops = {
> @@ -467,23 +387,22 @@ static const struct dw_hdmi_phy_ops meson_dw_hdmi_phy_ops = {
>   static irqreturn_t dw_hdmi_top_irq(int irq, void *dev_id)
>   {
>   	struct meson_dw_hdmi *dw_hdmi = dev_id;
> -	u32 stat;
> +	unsigned int stat;
>   
> -	stat = dw_hdmi->data->top_read(dw_hdmi, HDMITX_TOP_INTR_STAT);
> -	dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_INTR_STAT_CLR, stat);
> +	regmap_read(dw_hdmi->top, HDMITX_TOP_INTR_STAT, &stat);
> +	regmap_write(dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR, stat);
>   
>   	/* HPD Events, handle in the threaded interrupt handler */
> -	if (stat & (HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL)) {
> +	if (stat & (TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL)) {
>   		dw_hdmi->irq_stat = stat;
>   		return IRQ_WAKE_THREAD;
>   	}
>   
>   	/* HDMI Controller Interrupt */
> -	if (stat & 1)
> +	if (stat & TOP_INTR_CORE)
>   		return IRQ_NONE;
>   
>   	/* TOFIX Handle HDCP Interrupts */
> -
>   	return IRQ_HANDLED;
>   }
>   
> @@ -494,10 +413,10 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>   	u32 stat = dw_hdmi->irq_stat;
>   
>   	/* HPD Events */
> -	if (stat & (HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL)) {
> +	if (stat & (TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL)) {
>   		bool hpd_connected = false;
>   
> -		if (stat & HDMITX_TOP_INTR_HPD_RISE)
> +		if (stat & TOP_INTR_HPD_RISE)
>   			hpd_connected = true;
>   
>   		dw_hdmi_setup_rx_sense(dw_hdmi->hdmi, hpd_connected,
> @@ -512,63 +431,25 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>   
> -/* DW HDMI Regmap */
> -
> -static int meson_dw_hdmi_reg_read(void *context, unsigned int reg,
> -				  unsigned int *result)
> -{
> -	struct meson_dw_hdmi *dw_hdmi = context;
> -
> -	*result = dw_hdmi->data->dwc_read(dw_hdmi, reg);
> -
> -	return 0;
> -
> -}
> -
> -static int meson_dw_hdmi_reg_write(void *context, unsigned int reg,
> -				   unsigned int val)
> -{
> -	struct meson_dw_hdmi *dw_hdmi = context;
> -
> -	dw_hdmi->data->dwc_write(dw_hdmi, reg, val);
> -
> -	return 0;
> -}
> -
> -static const struct regmap_config meson_dw_hdmi_regmap_config = {
> -	.reg_bits = 32,
> -	.val_bits = 8,
> -	.reg_read = meson_dw_hdmi_reg_read,
> -	.reg_write = meson_dw_hdmi_reg_write,
> -	.max_register = 0x10000,
> -	.fast_io = true,
> -};
> -
> -static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
> -	.top_read = dw_hdmi_top_read,
> -	.top_write = dw_hdmi_top_write,
> -	.dwc_read = dw_hdmi_dwc_read,
> -	.dwc_write = dw_hdmi_dwc_write,
> -	.cntl0_init = 0x0,
> -	.cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
> +static const struct regmap_config top_gx_regmap_cfg = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.reg_shift	= -2,
> +	.val_bits	= 32,
> +	.max_register	= 0x40,
>   };
>   
> -static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
> -	.top_read = dw_hdmi_top_read,
> -	.top_write = dw_hdmi_top_write,
> -	.dwc_read = dw_hdmi_dwc_read,
> -	.dwc_write = dw_hdmi_dwc_write,
> -	.cntl0_init = 0x0,
> -	.cntl1_init = PHY_CNTL1_INIT,
> +static const struct regmap_config top_g12_regmap_cfg = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x40,
>   };
>   
> -static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
> -	.top_read = dw_hdmi_g12a_top_read,
> -	.top_write = dw_hdmi_g12a_top_write,
> -	.dwc_read = dw_hdmi_g12a_dwc_read,
> -	.dwc_write = dw_hdmi_g12a_dwc_write,
> -	.cntl0_init = 0x000b4242, /* Bandgap */
> -	.cntl1_init = PHY_CNTL1_INIT,
> +static const struct regmap_config dwc_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 8,
> +	.max_register = 0x8000,
>   };
>   
>   static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
> @@ -581,41 +462,107 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
>   	/* Bring HDMITX MEM output of power down */
>   	regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0);
>   
> -	/* Enable APB3 fail on error */
> -	if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> -		writel_bits_relaxed(BIT(15), BIT(15),
> -				    meson_dw_hdmi->hdmitx + HDMITX_TOP_CTRL_REG);
> -		writel_bits_relaxed(BIT(15), BIT(15),
> -				    meson_dw_hdmi->hdmitx + HDMITX_DWC_CTRL_REG);
> -	}
> -
>   	/* Bring out of reset */
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi,
> -				       HDMITX_TOP_SW_RESET,  0);
> -
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
>   	msleep(20);
>   
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi,
> -				       HDMITX_TOP_CLK_CNTL, 0xff);
> +	/* Enable clocks */
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_CLK_CNTL,
> +		     TOP_CLK_EN);
>   
>   	/* Enable normal output to PHY */
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_BIST_CNTL,
> +		     TOP_BIST_TMDS_EN);
>   
>   	/* Setup PHY */
> -	regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL1, meson_dw_hdmi->data->cntl1_init);
> -	regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, meson_dw_hdmi->data->cntl0_init);
> +	regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL1,
> +		     meson_dw_hdmi->data->cntl1_init);
> +	regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0,
> +		     meson_dw_hdmi->data->cntl0_init);
>   
>   	/* Enable HDMI-TX Interrupt */
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_INTR_STAT_CLR,
> -				       HDMITX_TOP_INTR_CORE);
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR,
> +		     GENMASK(31, 0));
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_INTR_MASKN,
> +		     TOP_INTR_CORE);
> +}
> +
> +static int meson_dw_init_regmap_gx(struct device *dev)
> +{
> +	struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
> +	struct regmap *map;
>   
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_INTR_MASKN,
> -				       HDMITX_TOP_INTR_CORE);
> +	/* Register TOP glue zone */
> +	writel_bits_relaxed(GX_CTRL_APB3_ERRFAIL, GX_CTRL_APB3_ERRFAIL,
> +			    meson_dw_hdmi->hdmitx + HDMITX_TOP_REGS + GX_CTRL_OFFSET);
>   
> +	map = devm_regmap_init(dev, &hdmi_tx_indirect_mmio,
> +			       meson_dw_hdmi->hdmitx + HDMITX_TOP_REGS,
> +			       &top_gx_regmap_cfg);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map), "failed to init top regmap\n");
> +
> +	meson_dw_hdmi->top = map;
> +
> +	/* Register DWC zone */
> +	writel_bits_relaxed(GX_CTRL_APB3_ERRFAIL, GX_CTRL_APB3_ERRFAIL,
> +			    meson_dw_hdmi->hdmitx + HDMITX_DWC_REGS + GX_CTRL_OFFSET);
> +
> +	map = devm_regmap_init(dev, &hdmi_tx_indirect_mmio,
> +			       meson_dw_hdmi->hdmitx + HDMITX_DWC_REGS,
> +			       &dwc_regmap_cfg);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map), "failed to init dwc regmap\n");
> +
> +	meson_dw_hdmi->dw_plat_data.regm = map;
> +
> +	return 0;
> +}
> +
> +static int meson_dw_init_regmap_g12(struct device *dev)
> +{
> +	struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
> +	struct regmap *map;
> +
> +	/* Register TOP glue zone with the offset */
> +	map = devm_regmap_init_mmio(dev, meson_dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET,
> +				    &top_g12_regmap_cfg);
> +	if (IS_ERR(map))
> +		dev_err_probe(dev, PTR_ERR(map), "failed to init top regmap\n");
> +
> +	meson_dw_hdmi->top = map;
> +
> +	/* Register DWC zone */
> +	map = devm_regmap_init_mmio(dev, meson_dw_hdmi->hdmitx,
> +				    &dwc_regmap_cfg);
> +	if (IS_ERR(map))
> +		dev_err_probe(dev, PTR_ERR(map), "failed to init dwc regmap\n");
> +
> +	meson_dw_hdmi->dw_plat_data.regm = map;
> +
> +	return 0;
>   }
>   
> +static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
> +	.reg_init = meson_dw_init_regmap_gx,
> +	.cntl0_init = 0x0,
> +	.cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
> +};
> +
> +static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
> +	.reg_init = meson_dw_init_regmap_gx,
> +	.cntl0_init = 0x0,
> +	.cntl1_init = PHY_CNTL1_INIT,
> +};
> +
> +static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
> +	.reg_init = meson_dw_init_regmap_g12,
> +	.cntl0_init = 0x000b4242, /* Bandgap */
> +	.cntl1_init = PHY_CNTL1_INIT,
> +};
> +
>   static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> -				void *data)
> +			      void *data)
>   {
>   	struct platform_device *pdev = to_platform_device(dev);
>   	const struct meson_dw_hdmi_data *match;
> @@ -640,6 +587,8 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	if (!meson_dw_hdmi)
>   		return -ENOMEM;
>   
> +	platform_set_drvdata(pdev, meson_dw_hdmi);
> +
>   	meson_dw_hdmi->priv = priv;
>   	meson_dw_hdmi->dev = dev;
>   	meson_dw_hdmi->data = match;
> @@ -682,10 +631,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to enable all clocks\n");
>   
> -	dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
> -					      &meson_dw_hdmi_regmap_config);
> -	if (IS_ERR(dw_plat_data->regm))
> -		return PTR_ERR(dw_plat_data->regm);
> +	ret = meson_dw_hdmi->data->reg_init(dev);
> +	if (ret)
> +		return ret;
>   
>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
> @@ -717,8 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
>   		dw_plat_data->use_drm_infoframe = true;
>   
> -	platform_set_drvdata(pdev, meson_dw_hdmi);
> -
>   	meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
>   	if (IS_ERR(meson_dw_hdmi->hdmi))
>   		return PTR_ERR(meson_dw_hdmi->hdmi);
> @@ -751,8 +697,7 @@ static int __maybe_unused meson_dw_hdmi_pm_suspend(struct device *dev)
>   		return 0;
>   
>   	/* FIXME: This actually bring top out reset on suspend, why ? */
> -	meson_dw_hdmi->data->top_write(meson_dw_hdmi,
> -				       HDMITX_TOP_SW_RESET, 0);
> +	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.h b/drivers/gpu/drm/meson/meson_dw_hdmi.h
> index 08e1c14e4ea0..3ab8c56d5fe1 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.h
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.h
> @@ -28,6 +28,7 @@
>    *     0=Release from reset. Default 1.
>    */
>   #define HDMITX_TOP_SW_RESET                     (0x000)
> +#define  TOP_RST_EN				GENMASK(4, 0)

Well NAK, the registers are named HDMITX_TOP_XXXX in the datasheet,
and TOP_XXXX defines could collide with other too generic defines,
and in any case don't mix defines renaming with other changes.

Just move the GENMASK in a new patch and leave the HDMITX_ prefix alone.

>   
>   /*
>    * Bit 31 RW free_clk_en: 0=Enable clock gating for power saving; 1= Disable
> @@ -45,7 +46,8 @@
>    * Bit 1 RW tmds_clk_en: 1=enable tmds_clk;  0=disable. Default 0.
>    * Bit 0 RW pixel_clk_en: 1=enable pixel_clk; 0=disable. Default 0.
>    */
> -#define HDMITX_TOP_CLK_CNTL                     (0x001)
> +#define HDMITX_TOP_CLK_CNTL                     (0x004)
> +#define  TOP_CLK_EN				GENMASK(7, 0)
>   
>   /*
>    * Bit 31:28 RW rxsense_glitch_width: starting from G12A
> @@ -53,7 +55,9 @@
>    * Bit 11: 0 RW hpd_valid_width: filter out width <= M*1024.    Default 0.
>    * Bit 15:12 RW hpd_glitch_width: filter out glitch <= N.       Default 0.
>    */
> -#define HDMITX_TOP_HPD_FILTER                   (0x002)
> +#define HDMITX_TOP_HPD_FILTER                   (0x008)
> +#define  TOP_HPD_GLITCH_WIDTH			GENMASK(15, 12)
> +#define  TOP_HPD_VALID_WIDTH			GENMASK(11, 0)
>   
>   /*
>    * intr_maskn: MASK_N, one bit per interrupt source.
> @@ -67,7 +71,7 @@
>    * [  1] hpd_rise_intr
>    * [  0] core_intr
>    */
> -#define HDMITX_TOP_INTR_MASKN                   (0x003)
> +#define HDMITX_TOP_INTR_MASKN                   (0x00c)
>   
>   /*
>    * Bit 30: 0 RW intr_stat: For each bit, write 1 to manually set the interrupt
> @@ -80,7 +84,7 @@
>    * Bit     1 RW hpd_rise
>    * Bit     0 RW IP interrupt
>    */
> -#define HDMITX_TOP_INTR_STAT                    (0x004)
> +#define HDMITX_TOP_INTR_STAT                    (0x010)
>   
>   /*
>    * [7]    rxsense_fall starting from G12A
> @@ -92,13 +96,12 @@
>    * [1]	  hpd_rise
>    * [0]	  core_intr_rise
>    */
> -#define HDMITX_TOP_INTR_STAT_CLR                (0x005)
> -
> -#define HDMITX_TOP_INTR_CORE		BIT(0)
> -#define HDMITX_TOP_INTR_HPD_RISE	BIT(1)
> -#define HDMITX_TOP_INTR_HPD_FALL	BIT(2)
> -#define HDMITX_TOP_INTR_RXSENSE_RISE	BIT(6)
> -#define HDMITX_TOP_INTR_RXSENSE_FALL	BIT(7)
> +#define HDMITX_TOP_INTR_STAT_CLR                (0x014)
> +#define  TOP_INTR_CORE				BIT(0)
> +#define  TOP_INTR_HPD_RISE			BIT(1)
> +#define  TOP_INTR_HPD_FALL			BIT(2)
> +#define  TOP_INTR_RXSENSE_RISE			BIT(6)
> +#define  TOP_INTR_RXSENSE_FALL			BIT(7)
>   
>   /*
>    * Bit 14:12 RW tmds_sel: 3'b000=Output zero; 3'b001=Output normal TMDS data;
> @@ -112,29 +115,31 @@
>    *     2=Output 1-bit pattern; 3=output 10-bit pattern. Default 0.
>    * Bit 0 RW prbs_pttn_en: 1=Enable PRBS generator; 0=Disable. Default 0.
>    */
> -#define HDMITX_TOP_BIST_CNTL                    (0x006)
> +#define HDMITX_TOP_BIST_CNTL                    (0x018)
> +#define  TOP_BIST_OUT_MASK			GENMASK(14, 12)
> +#define  TOP_BIST_TMDS_EN			BIT(12)
>   
>   /* Bit 29:20 RW shift_pttn_data[59:50]. Default 0. */
>   /* Bit 19:10 RW shift_pttn_data[69:60]. Default 0. */
>   /* Bit  9: 0 RW shift_pttn_data[79:70]. Default 0. */
> -#define HDMITX_TOP_SHIFT_PTTN_012               (0x007)
> +#define HDMITX_TOP_SHIFT_PTTN_012               (0x01c)
>   
>   /* Bit 29:20 RW shift_pttn_data[29:20]. Default 0. */
>   /* Bit 19:10 RW shift_pttn_data[39:30]. Default 0. */
>   /* Bit  9: 0 RW shift_pttn_data[49:40]. Default 0. */
> -#define HDMITX_TOP_SHIFT_PTTN_345               (0x008)
> +#define HDMITX_TOP_SHIFT_PTTN_345               (0x020)
>   
>   /* Bit 19:10 RW shift_pttn_data[ 9: 0]. Default 0. */
>   /* Bit  9: 0 RW shift_pttn_data[19:10]. Default 0. */
> -#define HDMITX_TOP_SHIFT_PTTN_67                (0x009)
> +#define HDMITX_TOP_SHIFT_PTTN_67                (0x024)
>   
>   /* Bit 25:16 RW tmds_clk_pttn[19:10]. Default 0. */
>   /* Bit  9: 0 RW tmds_clk_pttn[ 9: 0]. Default 0. */
> -#define HDMITX_TOP_TMDS_CLK_PTTN_01             (0x00A)
> +#define HDMITX_TOP_TMDS_CLK_PTTN_01             (0x028)
>   
>   /* Bit 25:16 RW tmds_clk_pttn[39:30]. Default 0. */
>   /* Bit  9: 0 RW tmds_clk_pttn[29:20]. Default 0. */
> -#define HDMITX_TOP_TMDS_CLK_PTTN_23             (0x00B)
> +#define HDMITX_TOP_TMDS_CLK_PTTN_23             (0x02c)
>   
>   /*
>    * Bit 1 RW shift_tmds_clk_pttn:1=Enable shifting clk pattern,
> @@ -143,18 +148,22 @@
>    * [	1] shift_tmds_clk_pttn
>    * [	0] load_tmds_clk_pttn
>    */
> -#define HDMITX_TOP_TMDS_CLK_PTTN_CNTL           (0x00C)
> +#define HDMITX_TOP_TMDS_CLK_PTTN_CNTL           (0x030)
> +#define  TOP_TDMS_CLK_PTTN_LOAD			BIT(0)
> +#define  TOP_TDMS_CLK_PTTN_SHFT			BIT(1)
>   
>   /*
>    * Bit 0 RW revocmem_wr_fail: Read back 1 to indicate Host write REVOC MEM
>    * failure, write 1 to clear the failure flag.  Default 0.
>    */
> -#define HDMITX_TOP_REVOCMEM_STAT                (0x00D)
> +#define HDMITX_TOP_REVOCMEM_STAT                (0x034)
>   
>   /*
>    * Bit	   1 R	filtered RxSense status
>    * Bit     0 R  filtered HPD status.
>    */
> -#define HDMITX_TOP_STAT0                        (0x00E)
> +#define HDMITX_TOP_STAT0                        (0x038)
> +#define  TOP_STAT0_HPD				BIT(0)
> +#define  TOP_STAT0_RXSENSE			BIT(1)
>   
>   #endif /* __MESON_DW_HDMI_H */