[PATCH 0/9] drm/meson: dw-hdmi: clean-up

[PATCH 0/9] drm/meson: dw-hdmi: clean-up

[Jerome Brunet]

This patchset is a clean up of the Amlogic HDMI phy driver.

It is part of an effort to remove the use HHI syscon from the driver
and properly use frameworks, such as PD and clocks, instead of going
for an incorrectly global register region.

When this is done, it should be easier to remove the usage of the
component API from the Amlogic display drivers.

To be clear, this patchset does not go this far yet. It stops
short of making any controversial DT changes. To decouple the HDMI
phy driver and main DRM driver, allowing the PHY to get hold of its
registers, I believe a DT ABI break is inevitable. Ideally the
register region of the PHY within the HHI should provided, not the
whole HHI. That's pain for another day ...

The last 2 patches should not be applied yet. They depend on DT
changes which recently got applied. Time is needed for the DT changes
to sink in u-boot and distros, to avoid breaking platforms which don't
take DT from the kernel. These 2 patches are provided as a note that
this should happen eventually.

Jerome Brunet (9):
  drm/meson: hdmi: move encoder settings out of phy driver
  drm/meson: vclk: drop hdmi system clock setup
  drm/meson: dw-hdmi: use generic clock helpers
  drm/meson: dw-hdmi: fix incorrect comment in suspend
  drm/meson: dw-hdmi: split resets out of hw init.
  drm/meson: dw-hdmi: convert to regmap
  drm/meson: dw-hdmi: use matched data
  drm/meson: dw-hdmi: don't write power controller registers
  drm/meson: dw-hdmi: drop hdmi system clock setup

 drivers/gpu/drm/meson/meson_dw_hdmi.c      | 746 ++++++++++-----------
 drivers/gpu/drm/meson/meson_dw_hdmi.h      |  49 +-
 drivers/gpu/drm/meson/meson_encoder_hdmi.c |  16 +
 drivers/gpu/drm/meson/meson_vclk.c         |   8 -
 4 files changed, 389 insertions(+), 430 deletions(-)

-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 1/9] drm/meson: hdmi: move encoder settings out of phy driver

[Jerome Brunet]

This relocates register pokes of the HDMI VPU encoder out of the
HDMI phy driver. As far as HDMI is concerned, the sequence in which
the setup is done remains mostly the same.

This was tested with modetest, cycling through the following resolutions:
  #0 3840x2160 60.00
  #1 3840x2160 59.94
  #2 3840x2160 50.00
  #3 3840x2160 30.00
  #4 3840x2160 29.97
  #5 3840x2160 25.00
  #6 3840x2160 24.00
  #7 3840x2160 23.98
  #8 1920x1080 60.00
  #9 1920x1080 60.00
  #10 1920x1080 59.94
  #11 1920x1080i 30.00
  #12 1920x1080i 29.97
  #13 1920x1080 50.00
  #14 1920x1080i 25.00
  #15 1920x1080 30.00
  #16 1920x1080 29.97
  #17 1920x1080 25.00
  #18 1920x1080 24.00
  #19 1920x1080 23.98
  #20 1280x1024 60.02
  #21 1152x864 59.97
  #22 1280x720 60.00
  #23 1280x720 59.94
  #24 1280x720 50.00
  #25 1024x768 60.00
  #26 800x600 60.32
  #27 720x576 50.00
  #28 720x480 59.94

No regression to report.

This is part of an effort to clean up Amlogic HDMI related drivers which
should eventually allow to stop using the component API and HHI syscon.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c      | 38 ----------------------
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 16 +++++++++
 2 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5565f7777529..bcf4f83582f2 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -115,12 +115,6 @@
 
 static DEFINE_SPINLOCK(reg_lock);
 
-enum meson_venc_source {
-	MESON_VENC_SOURCE_NONE = 0,
-	MESON_VENC_SOURCE_ENCI = 1,
-	MESON_VENC_SOURCE_ENCP = 2,
-};
-
 struct meson_dw_hdmi;
 
 struct meson_dw_hdmi_data {
@@ -376,8 +370,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
 	struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
 	bool is_hdmi2_sink = display->hdmi.scdc.supported;
 	struct meson_drm *priv = dw_hdmi->priv;
-	unsigned int wr_clk =
-		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
 	bool mode_is_420 = false;
 
 	DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
@@ -421,36 +413,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
 	meson_dw_hdmi_phy_reset(dw_hdmi);
 	meson_dw_hdmi_phy_reset(dw_hdmi);
 
-	/* Temporary Disable VENC video stream */
-	if (priv->venc.hdmi_use_enci)
-		writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
-	else
-		writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));
-
-	/* Temporary Disable HDMI video stream to HDMI-TX */
-	writel_bits_relaxed(0x3, 0,
-			    priv->io_base + _REG(VPU_HDMI_SETTING));
-	writel_bits_relaxed(0xf << 8, 0,
-			    priv->io_base + _REG(VPU_HDMI_SETTING));
-
-	/* Re-Enable VENC video stream */
-	if (priv->venc.hdmi_use_enci)
-		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
-	else
-		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
-
-	/* Push back HDMI clock settings */
-	writel_bits_relaxed(0xf << 8, wr_clk & (0xf << 8),
-			    priv->io_base + _REG(VPU_HDMI_SETTING));
-
-	/* Enable and Select HDMI video source for HDMI-TX */
-	if (priv->venc.hdmi_use_enci)
-		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
-				    priv->io_base + _REG(VPU_HDMI_SETTING));
-	else
-		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
-				    priv->io_base + _REG(VPU_HDMI_SETTING));
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 0593a1cde906..1c3e3e5526eb 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -45,6 +45,12 @@ struct meson_encoder_hdmi {
 	struct cec_notifier *cec_notifier;
 };
 
+enum meson_venc_source {
+	MESON_VENC_SOURCE_NONE = 0,
+	MESON_VENC_SOURCE_ENCI = 1,
+	MESON_VENC_SOURCE_ENCP = 2,
+};
+
 #define bridge_to_meson_encoder_hdmi(x) \
 	container_of(x, struct meson_encoder_hdmi, bridge)
 
@@ -247,6 +253,14 @@ static void meson_encoder_hdmi_atomic_enable(struct drm_bridge *bridge,
 		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
 	else
 		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
+
+	/* Enable and Select HDMI video source for HDMI-TX */
+	if (priv->venc.hdmi_use_enci)
+		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
+				    priv->io_base + _REG(VPU_HDMI_SETTING));
+	else
+		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
+				    priv->io_base + _REG(VPU_HDMI_SETTING));
 }
 
 static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
@@ -257,6 +271,8 @@ static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
 
 	writel_bits_relaxed(0x3, 0,
 			    priv->io_base + _REG(VPU_HDMI_SETTING));
+	writel_bits_relaxed(0xf << 8, 0,
+			    priv->io_base + _REG(VPU_HDMI_SETTING));
 
 	writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
 	writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 2/9] drm/meson: vclk: drop hdmi system clock setup

[Jerome Brunet]

Poking the HHI syscon is a way to setup clocks behind CCF's back.
Also, 2 drm code paths, the encoder and the hdmi-phy, are racing to do the
same setup of the HDMI system clock.

This clock is used is used by the HDMI phy and should not be set by the
encoder, so drop those HHI pokes from vclk.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_vclk.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
index 2a942dc6a6dc..bf5cc5d92346 100644
--- a/drivers/gpu/drm/meson/meson_vclk.c
+++ b/drivers/gpu/drm/meson/meson_vclk.c
@@ -813,14 +813,6 @@ static void meson_vclk_set(struct meson_drm *priv, unsigned int pll_base_freq,
 {
 	unsigned int m = 0, frac = 0;
 
-	/* Set HDMI-TX sys clock */
-	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
-			   CTS_HDMI_SYS_SEL_MASK, 0);
-	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
-			   CTS_HDMI_SYS_DIV_MASK, 0);
-	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
-			   CTS_HDMI_SYS_EN, CTS_HDMI_SYS_EN);
-
 	/* Set HDMI PLL rate */
 	if (!od1 && !od2 && !od3) {
 		meson_hdmi_pll_generic_set(priv, pll_base_freq);
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 3/9] drm/meson: dw-hdmi: use generic clock helpers

[Jerome Brunet]

The Amlogic HDMI phy driver is not doing anything with the clocks
besides enabling on probe. CCF provides generic helpers to do that.

Use the generic clock helpers rather than using a custom one to get and
enable clocks.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 36 +++------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index bcf4f83582f2..2890796f9d49 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -619,29 +619,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 
 }
 
-static void meson_disable_clk(void *data)
-{
-	clk_disable_unprepare(data);
-}
-
-static int meson_enable_clk(struct device *dev, char *name)
-{
-	struct clk *clk;
-	int ret;
-
-	clk = devm_clk_get(dev, name);
-	if (IS_ERR(clk)) {
-		dev_err(dev, "Unable to get %s pclk\n", name);
-		return PTR_ERR(clk);
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (!ret)
-		ret = devm_add_action_or_reset(dev, meson_disable_clk, clk);
-
-	return ret;
-}
-
 static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 				void *data)
 {
@@ -651,6 +628,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = data;
 	struct meson_drm *priv = drm->dev_private;
 	struct dw_hdmi_plat_data *dw_plat_data;
+	struct clk_bulk_data *clks;
 	int irq;
 	int ret;
 
@@ -701,17 +679,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	if (IS_ERR(meson_dw_hdmi->hdmitx))
 		return PTR_ERR(meson_dw_hdmi->hdmitx);
 
-	ret = meson_enable_clk(dev, "isfr");
-	if (ret)
-		return ret;
-
-	ret = meson_enable_clk(dev, "iahb");
+	ret = devm_clk_bulk_get_all_enable(dev, &clks);
 	if (ret)
-		return ret;
-
-	ret = meson_enable_clk(dev, "venci");
-	if (ret)
-		return 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);
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 4/9] drm/meson: dw-hdmi: fix incorrect comment in suspend

[Jerome Brunet]

Comment in suspend says TOP is put in suspend, but the register
poke following is actually de-asserting the reset, like in init.

It is doing the opposite of what the comment says.

Align the comment with what the code is doing for now and add
a FIXME note to sort this out later

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 2890796f9d49..5cd3264ab874 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -751,7 +751,7 @@ static int __maybe_unused meson_dw_hdmi_pm_suspend(struct device *dev)
 	if (!meson_dw_hdmi)
 		return 0;
 
-	/* Reset TOP */
+	/* FIXME: This actually bring top out reset on suspend, why ? */
 	meson_dw_hdmi->data->top_write(meson_dw_hdmi,
 				       HDMITX_TOP_SW_RESET, 0);
 
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 5/9] drm/meson: dw-hdmi: split resets out of hw init.

[Jerome Brunet]

This prepares the migration to regmap usage.

To properly setup regmap, the APB needs to be in working order.
This is easier handled if the resets are not mixed with hw init.

More checks are required to determine if the resets are needed
on resume or not. Add a note for this.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5cd3264ab874..47aa3e184e98 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -581,11 +581,6 @@ 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);
 
-	/* Reset HDMITX APB & TX & PHY */
-	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
-	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
-	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
-
 	/* Enable APB3 fail on error */
 	if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
 		writel_bits_relaxed(BIT(15), BIT(15),
@@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 		return PTR_ERR(meson_dw_hdmi->hdmitx_phy);
 	}
 
+	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
+	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
+	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
+
 	meson_dw_hdmi->hdmitx = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(meson_dw_hdmi->hdmitx))
 		return PTR_ERR(meson_dw_hdmi->hdmitx);
@@ -765,6 +764,11 @@ static int __maybe_unused meson_dw_hdmi_pm_resume(struct device *dev)
 	if (!meson_dw_hdmi)
 		return 0;
 
+	/* TODO: Is this really necessary/desirable on resume ? */
+	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
+	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
+	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
+
 	meson_dw_hdmi_init(meson_dw_hdmi);
 
 	dw_hdmi_resume(meson_dw_hdmi->hdmi);
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Jerome Brunet]

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@baylibre.com>
---
 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
+#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.
+ */
+#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)
 #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);
+
+	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);
+
+	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;
 };
 
 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)
 
 /*
  * 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 */
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 7/9] drm/meson: dw-hdmi: use matched data

[Jerome Brunet]

Using several string comparisons with if/else if/else clauses
is fairly inefficient and does not scale well.

Use matched data to tweak the driver depending on the matched
SoC instead.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 209 +++++++++++++++++---------
 1 file changed, 139 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 7c39e5c99043..ef059c5ef520 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -125,8 +125,17 @@
 #define HHI_HDMI_PHY_CNTL4	0x3b0 /* 0xec */
 #define HHI_HDMI_PHY_CNTL5	0x3b4 /* 0xed */
 
+struct meson_dw_hdmi_speed {
+	const struct reg_sequence *regs;
+	unsigned int reg_num;
+	unsigned int limit;
+};
+
 struct meson_dw_hdmi_data {
 	int (*reg_init)(struct device *dev);
+	const struct meson_dw_hdmi_speed *speeds;
+	unsigned int speed_num;
+	bool use_drm_infoframe;
 	u32 cntl0_init;
 	u32 cntl1_init;
 };
@@ -185,78 +194,33 @@ struct meson_dw_hdmi {
 	struct regmap *top;
 };
 
-static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
-					const char *compat)
-{
-	return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
-}
-
-/* Bridge */
-
 /* Setup PHY bandwidth modes */
-static void meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
+static int meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
 				      const struct drm_display_mode *mode,
 				      bool mode_is_420)
 {
 	struct meson_drm *priv = dw_hdmi->priv;
 	unsigned int pixel_clock = mode->clock;
+	int i;
 
 	/* For 420, pixel clock is half unlike venc clock */
-	if (mode_is_420) pixel_clock /= 2;
-
-	if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
-	    dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi")) {
-		if (pixel_clock >= 371250) {
-			/* 5.94Gbps, 3.7125Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x333d3282);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2136315b);
-		} else if (pixel_clock >= 297000) {
-			/* 2.97Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303382);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2036315b);
-		} else if (pixel_clock >= 148500) {
-			/* 1.485Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303362);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2016315b);
-		} else {
-			/* 742.5Mbps, and below */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33604142);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x0016315b);
-		}
-	} else if (dw_hdmi_is_compatible(dw_hdmi,
-					 "amlogic,meson-gxbb-dw-hdmi")) {
-		if (pixel_clock >= 371250) {
-			/* 5.94Gbps, 3.7125Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33353245);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2100115b);
-		} else if (pixel_clock >= 297000) {
-			/* 2.97Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33634283);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0xb000115b);
-		} else {
-			/* 1.485Gbps, and below */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33632122);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2000115b);
-		}
-	} else if (dw_hdmi_is_compatible(dw_hdmi,
-					 "amlogic,meson-g12a-dw-hdmi")) {
-		if (pixel_clock >= 371250) {
-			/* 5.94Gbps, 3.7125Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x37eb65c4);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x0000080b);
-		} else if (pixel_clock >= 297000) {
-			/* 2.97Gbps */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb6262);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
-		} else {
-			/* 1.485Gbps, and below */
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb4242);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
-			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
-		}
+	if (mode_is_420)
+		pixel_clock /= 2;
+
+	for (i = 0; i < dw_hdmi->data->speed_num; i++) {
+		if (pixel_clock >= dw_hdmi->data->speeds[i].limit)
+			break;
 	}
+
+	/* No match found - Last entry should have a 0 limit */
+	if (WARN_ON(i == dw_hdmi->data->speed_num))
+		return -EINVAL;
+
+	regmap_multi_reg_write(priv->hhi,
+			       dw_hdmi->data->speeds[i].regs,
+			       dw_hdmi->data->speeds[i].reg_num);
+
+	return 0;
 }
 
 static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
@@ -543,22 +507,133 @@ static int meson_dw_init_regmap_g12(struct device *dev)
 	return 0;
 }
 
+static const struct reg_sequence gxbb_3g7_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33353245 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2100115b },
+};
+
+static const struct reg_sequence gxbb_3g_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33634283 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0xb000115b },
+};
+
+static const struct reg_sequence gxbb_def_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33632122 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2000115b },
+};
+
+static const struct meson_dw_hdmi_speed gxbb_speeds[] = {
+	{
+		.limit = 371250,
+		.regs = gxbb_3g7_regs,
+		.reg_num = ARRAY_SIZE(gxbb_3g7_regs)
+	}, {
+		.limit = 297000,
+		.regs = gxbb_3g_regs,
+		.reg_num = ARRAY_SIZE(gxbb_3g_regs)
+	}, {
+		.regs = gxbb_def_regs,
+		.reg_num = ARRAY_SIZE(gxbb_def_regs)
+	}
+};
+
 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,
+	.use_drm_infoframe = false,
+	.speeds = gxbb_speeds,
+	.speed_num = ARRAY_SIZE(gxbb_speeds),
+};
+
+static const struct reg_sequence gxl_3g7_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x333d3282 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2136315b },
+};
+
+static const struct reg_sequence gxl_3g_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303382 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2036315b },
+};
+
+static const struct reg_sequence gxl_def_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303362 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2016315b },
+};
+
+static const struct reg_sequence gxl_270m_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33604142 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x0016315b },
+};
+
+static const struct meson_dw_hdmi_speed gxl_speeds[] = {
+	{
+		.limit = 371250,
+		.regs = gxl_3g7_regs,
+		.reg_num = ARRAY_SIZE(gxl_3g7_regs)
+	}, {
+		.limit = 297000,
+		.regs = gxl_3g_regs,
+		.reg_num = ARRAY_SIZE(gxl_3g_regs)
+	}, {
+		.limit = 148500,
+		.regs = gxl_def_regs,
+		.reg_num = ARRAY_SIZE(gxl_def_regs)
+	}, {
+		.regs = gxl_270m_regs,
+		.reg_num = ARRAY_SIZE(gxl_270m_regs)
+	}
 };
 
 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,
+	.use_drm_infoframe = true,
+	.speeds = gxl_speeds,
+	.speed_num = ARRAY_SIZE(gxl_speeds),
+};
+
+static const struct reg_sequence g12a_3g7_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x37eb65c4 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
+	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x0000080b },
+};
+
+static const struct reg_sequence g12a_3g_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb6262 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
+	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
+};
+
+static const struct reg_sequence g12a_def_regs[] = {
+	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb4242 },
+	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
+	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
+};
+
+static const struct meson_dw_hdmi_speed g12a_speeds[] = {
+	{
+		.limit = 371250,
+		.regs = g12a_3g7_regs,
+		.reg_num = ARRAY_SIZE(g12a_3g7_regs)
+	}, {
+		.limit = 297000,
+		.regs = g12a_3g_regs,
+		.reg_num = ARRAY_SIZE(g12a_3g_regs)
+	}, {
+		.regs = g12a_def_regs,
+		.reg_num = ARRAY_SIZE(g12a_def_regs)
+	}
 };
 
 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,
+	.use_drm_infoframe = true,
+	.speeds = g12a_speeds,
+	.speed_num = ARRAY_SIZE(g12a_speeds),
 };
 
 static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
@@ -590,7 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	platform_set_drvdata(pdev, meson_dw_hdmi);
 
 	meson_dw_hdmi->priv = priv;
-	meson_dw_hdmi->dev = dev;
 	meson_dw_hdmi->data = match;
 	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
 
@@ -650,7 +724,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	meson_dw_hdmi_init(meson_dw_hdmi);
 
 	/* Bridge / Connector */
-
 	dw_plat_data->priv_data = meson_dw_hdmi;
 	dw_plat_data->phy_ops = &meson_dw_hdmi_phy_ops;
 	dw_plat_data->phy_name = "meson_dw_hdmi_phy";
@@ -659,11 +732,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
 	dw_plat_data->ycbcr_420_allowed = true;
 	dw_plat_data->disable_cec = true;
 	dw_plat_data->output_port = 1;
-
-	if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
-	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
-	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
-		dw_plat_data->use_drm_infoframe = true;
+	dw_plat_data->use_drm_infoframe = meson_dw_hdmi->data->use_drm_infoframe;
 
 	meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
 	if (IS_ERR(meson_dw_hdmi->hdmi))
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH LATER 8/9] drm/meson: dw-hdmi: don't write power controller registers

[Jerome Brunet]

The HDMI phy has a power domain properly set in DT.

Writing the power controller register directly from the hdmi driver is
incorrect. The power domain framework should be used for that.

HHI is a collection of Amlogic devices, such as clocks, reset,
power domains and phys.

This is another step to get rid of HHI access in Amlogic display drivers
and possibly stop using the component API.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

 This change depends on:
 * f1ab099d6591 ("arm64: dts: amlogic: add power domain to hdmitx")

Time is needed for these changes to sink in u-boot and distros,
making this change safe to apply.

 drivers/gpu/drm/meson/meson_dw_hdmi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index ef059c5ef520..6c18d97b8b16 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -111,7 +111,6 @@
 #define HDMITX_TOP_G12A_OFFSET	0x8000
 
 /* HHI Registers */
-#define HHI_MEM_PD_REG0		0x100 /* 0x40 */
 #define HHI_HDMI_CLK_CNTL	0x1cc /* 0x73 */
 #define HHI_HDMI_PHY_CNTL0	0x3a0 /* 0xe8 */
 #define HHI_HDMI_PHY_CNTL1	0x3a4 /* 0xe9 */
@@ -423,9 +422,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 	/* Enable clocks */
 	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
 
-	/* Bring HDMITX MEM output of power down */
-	regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0);
-
 	/* Bring out of reset */
 	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
 	msleep(20);
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH LATER 9/9] drm/meson: dw-hdmi: drop hdmi system clock setup

[Jerome Brunet]

Poking the HHI syscon is a way to setup clocks behind CCF's back.
Drop these poke and let CCF handle this using DT assigned-clocks.

HHI is a collection of Amlogic devices, such as clocks, reset,
power domains and phys.

This is another step to get rid of HHI access in Amlogic display drivers
and possibly stop using the component API.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

 This change depends on:
 * 0602ba0dcd0e ("arm64: dts: amlogic: gx: correct hdmi clocks")
 * 1443b6ea806d ("arm64: dts: amlogic: setup hdmi system clock")

Time is needed for these changes to sink in u-boot and distros,
making this change safe to apply.

 drivers/gpu/drm/meson/meson_dw_hdmi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 6c18d97b8b16..b54c1e3093e9 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -111,7 +111,6 @@
 #define HDMITX_TOP_G12A_OFFSET	0x8000
 
 /* HHI Registers */
-#define HHI_HDMI_CLK_CNTL	0x1cc /* 0x73 */
 #define HHI_HDMI_PHY_CNTL0	0x3a0 /* 0xe8 */
 #define HHI_HDMI_PHY_CNTL1	0x3a4 /* 0xe9 */
 #define  PHY_CNTL1_INIT		0x03900000
@@ -419,9 +418,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
 {
 	struct meson_drm *priv = meson_dw_hdmi->priv;
 
-	/* Enable clocks */
-	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
-
 	/* Bring out of reset */
 	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
 	msleep(20);
-- 
2.43.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/9] drm/meson: vclk: drop hdmi system clock setup

[Martin Blumenstingl]

On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> Poking the HHI syscon is a way to setup clocks behind CCF's back.
> Also, 2 drm code paths, the encoder and the hdmi-phy, are racing to do the
> same setup of the HDMI system clock.
>
> This clock is used is used by the HDMI phy and should not be set by the
> encoder, so drop those HHI pokes from vclk.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/9] drm/meson: dw-hdmi: use generic clock helpers

[Martin Blumenstingl]

On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> The Amlogic HDMI phy driver is not doing anything with the clocks
> besides enabling on probe. CCF provides generic helpers to do that.
>
> Use the generic clock helpers rather than using a custom one to get and
> enable clocks.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

note to self: even if we need to manage one of the clocks specifically
we can do it with clk_bulk_data

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 4/9] drm/meson: dw-hdmi: fix incorrect comment in suspend

[Martin Blumenstingl]

On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> Comment in suspend says TOP is put in suspend, but the register
> poke following is actually de-asserting the reset, like in init.
>
> It is doing the opposite of what the comment says.
>
> Align the comment with what the code is doing for now and add
> a FIXME note to sort this out later
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
(based on public S912 and A311 datasheets)

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 5/9] drm/meson: dw-hdmi: split resets out of hw init.

[Martin Blumenstingl]

Hi Jerome,

On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> This prepares the migration to regmap usage.
>
> To properly setup regmap, the APB needs to be in working order.
> This is easier handled if the resets are not mixed with hw init.
>
> More checks are required to determine if the resets are needed
> on resume or not. Add a note for this.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 5cd3264ab874..47aa3e184e98 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -581,11 +581,6 @@ 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);
>
> -       /* Reset HDMITX APB & TX & PHY */
> -       reset_control_reset(meson_dw_hdmi->hdmitx_apb);
> -       reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
> -       reset_control_reset(meson_dw_hdmi->hdmitx_phy);
> -
>         /* Enable APB3 fail on error */
>         if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>                 writel_bits_relaxed(BIT(15), BIT(15),
> @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>                 return PTR_ERR(meson_dw_hdmi->hdmitx_phy);
>         }
>
> +       reset_control_reset(meson_dw_hdmi->hdmitx_apb);
> +       reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
> +       reset_control_reset(meson_dw_hdmi->hdmitx_phy);
The old out of tree vendor driver [0] enables the "isfr" and "iahb"
(in P_HHI_HDMI_CLK_CNTL and P_HHI_GCLK_MPEG2) clocks before triggering
the resets.
Previously meson_dw_hdmi's behavior was identical as it enabled the
clocks in meson_dw_hdmi_bind() and only later triggered the resets.

I'm totally fine with moving the resets to meson_dw_hdmi_bind() but I
think it should happen after devm_clk_bulk_get_all_enable() has been
called (to keep the order and thus avoid side-effects that we don't
know about yet).

Also out of curiosity: are you planning to convert the driver to use
devm_reset_control_bulk_get_exclusive()?


Best regards,
Martin


[0] https://github.com/endlessm/linux-s905x/blob/master/drivers/amlogic/hdmi/hdmi_tx_20/hw/hdmi_tx_hw.c#L470

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data

[Martin Blumenstingl]

Hi Jerome,

On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> +       }, {
> +               .limit = 297000,
> +               .regs = gxbb_3g_regs,
> +               .reg_num = ARRAY_SIZE(gxbb_3g_regs)
Just as a side-note: this looked odd when reading for the first time
as I thought that it's a typo (and it should be gxbb_2g97_regs - but
that name is not used).

[...]
> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
> +       {
> +               .limit = 371250,
> +               .regs = gxl_3g7_regs,
> +               .reg_num = ARRAY_SIZE(gxl_3g7_regs)
> +       }, {
> +               .limit = 297000,
> +               .regs = gxl_3g_regs,
> +               .reg_num = ARRAY_SIZE(gxl_3g_regs)
> +       }, {
> +               .limit = 148500,
> +               .regs = gxl_def_regs,
> +               .reg_num = ARRAY_SIZE(gxl_def_regs)
this is not consistent with what we have above or below so it either
needs to be updated or a comment.
I think this should be called gxl_1g48_regs

> +       }, {
> +               .regs = gxl_270m_regs,
> +               .reg_num = ARRAY_SIZE(gxl_270m_regs)
and this should be called gxl_def_regs



Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/9] drm/meson: dw-hdmi: use generic clock helpers

[Jerome Brunet]

On Tue 06 Aug 2024 at 22:28, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>> The Amlogic HDMI phy driver is not doing anything with the clocks
>> besides enabling on probe. CCF provides generic helpers to do that.
>>
>> Use the generic clock helpers rather than using a custom one to get and
>> enable clocks.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> note to self: even if we need to manage one of the clocks specifically
> we can do it with clk_bulk_data

Honestly I've gone for bulk variant only because calling
devm_clk_get_enabled() 3 times in row and do nothing with the clocks
looks odd.

In I had to do something specific with a clock later on, personnaly I
would back to plain 'struct clk' and use devm_clk_get_enabled()

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 5/9] drm/meson: dw-hdmi: split resets out of hw init.

[Jerome Brunet]

On Tue 06 Aug 2024 at 22:49, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>> This prepares the migration to regmap usage.
>>
>> To properly setup regmap, the APB needs to be in working order.
>> This is easier handled if the resets are not mixed with hw init.
>>
>> More checks are required to determine if the resets are needed
>> on resume or not. Add a note for this.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index 5cd3264ab874..47aa3e184e98 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -581,11 +581,6 @@ 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);
>>
>> -       /* Reset HDMITX APB & TX & PHY */
>> -       reset_control_reset(meson_dw_hdmi->hdmitx_apb);
>> -       reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
>> -       reset_control_reset(meson_dw_hdmi->hdmitx_phy);
>> -
>>         /* Enable APB3 fail on error */
>>         if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>>                 writel_bits_relaxed(BIT(15), BIT(15),
>> @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>>                 return PTR_ERR(meson_dw_hdmi->hdmitx_phy);
>>         }
>>
>> +       reset_control_reset(meson_dw_hdmi->hdmitx_apb);
>> +       reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
>> +       reset_control_reset(meson_dw_hdmi->hdmitx_phy);
> The old out of tree vendor driver [0] enables the "isfr" and "iahb"
> (in P_HHI_HDMI_CLK_CNTL and P_HHI_GCLK_MPEG2) clocks before triggering
> the resets.
> Previously meson_dw_hdmi's behavior was identical as it enabled the
> clocks in meson_dw_hdmi_bind() and only later triggered the resets.
>
> I'm totally fine with moving the resets to meson_dw_hdmi_bind() but I
> think it should happen after devm_clk_bulk_get_all_enable() has been
> called (to keep the order and thus avoid side-effects that we don't
> know about yet).

Good point.

I was also thinking about squashing this with the regmap patch.
I've split it apart for v1 to make things a bit more clear but it only
really makes sense with the regmap conversion. 

>
> Also out of curiosity: are you planning to convert the driver to use
> devm_reset_control_bulk_get_exclusive()?
>

It's been a while this I've done that. I remember I thought about it.
I think it was a bit more difficult to use that clocks. I was looking at
making the driver a bit more clean and simple. It was not really helping
to move it in that direction IIRC.

>
> Best regards,
> Martin
>
>
> [0] https://github.com/endlessm/linux-s905x/blob/master/drivers/amlogic/hdmi/hdmi_tx_20/hw/hdmi_tx_hw.c#L470

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data

[Jerome Brunet]

On Tue 06 Aug 2024 at 23:03, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> +       }, {
>> +               .limit = 297000,
>> +               .regs = gxbb_3g_regs,
>> +               .reg_num = ARRAY_SIZE(gxbb_3g_regs)
> Just as a side-note: this looked odd when reading for the first time
> as I thought that it's a typo (and it should be gxbb_2g97_regs - but
> that name is not used).

I know it looks odd but there is a (perhaps silly) reason for it.

The names are derived from PHY modes used in the Amlogic vendor tree.
Those are magic pokes and we don't really know anything about it. The
vendor tree often update and mainline does not always follow. I know
that we are not 100% aligned right now. No one knows what branch is the
reference to follow anyway.

Using the same names is way to leave breadcrumbs that may help people
linking this to vendor code later on (if necessary)

I think the modes are named after the (rounded) bandwidth they provide,
not necessarily the limit we are using to pick it ... except for def,
which might just mean 'default' I guess.

https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hw_g12a.c#L589

I focused on keeping mainline as it was for the value poked, retaining
as much information as possible to find our way back.

Your proposed naming convention is fine by me as well.

>
> [...]
>> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
>> +       {
>> +               .limit = 371250,
>> +               .regs = gxl_3g7_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_3g7_regs)
>> +       }, {
>> +               .limit = 297000,
>> +               .regs = gxl_3g_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_3g_regs)
>> +       }, {
>> +               .limit = 148500,
>> +               .regs = gxl_def_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_def_regs)
> this is not consistent with what we have above or below so it either
> needs to be updated or a comment.
> I think this should be called gxl_1g48_regs
>
>> +       }, {
>> +               .regs = gxl_270m_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_270m_regs)
> and this should be called gxl_def_regs

def in not the last one, it is another name used by AML

It so happens that on mainline with we have only put the SD/270M for
gxl. In the AML tree, it does exist for G12 too. Maybe it will be needed someday.

>
>
>
> Best regards,
> Martin

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/9] drm/meson: hdmi: move encoder settings out of phy driver

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> This relocates register pokes of the HDMI VPU encoder out of the
> HDMI phy driver. As far as HDMI is concerned, the sequence in which
> the setup is done remains mostly the same.
> 
> This was tested with modetest, cycling through the following resolutions:
>    #0 3840x2160 60.00
>    #1 3840x2160 59.94
>    #2 3840x2160 50.00
>    #3 3840x2160 30.00
>    #4 3840x2160 29.97
>    #5 3840x2160 25.00
>    #6 3840x2160 24.00
>    #7 3840x2160 23.98
>    #8 1920x1080 60.00
>    #9 1920x1080 60.00
>    #10 1920x1080 59.94
>    #11 1920x1080i 30.00
>    #12 1920x1080i 29.97
>    #13 1920x1080 50.00
>    #14 1920x1080i 25.00
>    #15 1920x1080 30.00
>    #16 1920x1080 29.97
>    #17 1920x1080 25.00
>    #18 1920x1080 24.00
>    #19 1920x1080 23.98
>    #20 1280x1024 60.02
>    #21 1152x864 59.97
>    #22 1280x720 60.00
>    #23 1280x720 59.94
>    #24 1280x720 50.00
>    #25 1024x768 60.00
>    #26 800x600 60.32
>    #27 720x576 50.00
>    #28 720x480 59.94
> 
> No regression to report.
> 
> This is part of an effort to clean up Amlogic HDMI related drivers which
> should eventually allow to stop using the component API and HHI syscon.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c      | 38 ----------------------
>   drivers/gpu/drm/meson/meson_encoder_hdmi.c | 16 +++++++++
>   2 files changed, 16 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 5565f7777529..bcf4f83582f2 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -115,12 +115,6 @@
>   
>   static DEFINE_SPINLOCK(reg_lock);
>   
> -enum meson_venc_source {
> -	MESON_VENC_SOURCE_NONE = 0,
> -	MESON_VENC_SOURCE_ENCI = 1,
> -	MESON_VENC_SOURCE_ENCP = 2,
> -};
> -
>   struct meson_dw_hdmi;
>   
>   struct meson_dw_hdmi_data {
> @@ -376,8 +370,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>   	struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
>   	bool is_hdmi2_sink = display->hdmi.scdc.supported;
>   	struct meson_drm *priv = dw_hdmi->priv;
> -	unsigned int wr_clk =
> -		readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>   	bool mode_is_420 = false;
>   
>   	DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> @@ -421,36 +413,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
>   	meson_dw_hdmi_phy_reset(dw_hdmi);
>   	meson_dw_hdmi_phy_reset(dw_hdmi);
>   
> -	/* Temporary Disable VENC video stream */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
> -	else
> -		writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));
> -
> -	/* Temporary Disable HDMI video stream to HDMI-TX */
> -	writel_bits_relaxed(0x3, 0,
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -	writel_bits_relaxed(0xf << 8, 0,
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
> -	/* Re-Enable VENC video stream */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
> -	else
> -		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
> -
> -	/* Push back HDMI clock settings */
> -	writel_bits_relaxed(0xf << 8, wr_clk & (0xf << 8),
> -			    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
> -	/* Enable and Select HDMI video source for HDMI-TX */
> -	if (priv->venc.hdmi_use_enci)
> -		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
> -				    priv->io_base + _REG(VPU_HDMI_SETTING));
> -	else
> -		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
> -				    priv->io_base + _REG(VPU_HDMI_SETTING));
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 0593a1cde906..1c3e3e5526eb 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> @@ -45,6 +45,12 @@ struct meson_encoder_hdmi {
>   	struct cec_notifier *cec_notifier;
>   };
>   
> +enum meson_venc_source {
> +	MESON_VENC_SOURCE_NONE = 0,
> +	MESON_VENC_SOURCE_ENCI = 1,
> +	MESON_VENC_SOURCE_ENCP = 2,
> +};
> +
>   #define bridge_to_meson_encoder_hdmi(x) \
>   	container_of(x, struct meson_encoder_hdmi, bridge)
>   
> @@ -247,6 +253,14 @@ static void meson_encoder_hdmi_atomic_enable(struct drm_bridge *bridge,
>   		writel_relaxed(1, priv->io_base + _REG(ENCI_VIDEO_EN));
>   	else
>   		writel_relaxed(1, priv->io_base + _REG(ENCP_VIDEO_EN));
> +
> +	/* Enable and Select HDMI video source for HDMI-TX */
> +	if (priv->venc.hdmi_use_enci)
> +		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCI,
> +				    priv->io_base + _REG(VPU_HDMI_SETTING));
> +	else
> +		writel_bits_relaxed(0x3, MESON_VENC_SOURCE_ENCP,
> +				    priv->io_base + _REG(VPU_HDMI_SETTING));
>   }
>   
>   static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
> @@ -257,6 +271,8 @@ static void meson_encoder_hdmi_atomic_disable(struct drm_bridge *bridge,
>   
>   	writel_bits_relaxed(0x3, 0,
>   			    priv->io_base + _REG(VPU_HDMI_SETTING));
> +	writel_bits_relaxed(0xf << 8, 0,
> +			    priv->io_base + _REG(VPU_HDMI_SETTING));
>   
>   	writel_relaxed(0, priv->io_base + _REG(ENCI_VIDEO_EN));
>   	writel_relaxed(0, priv->io_base + _REG(ENCP_VIDEO_EN));

Nice usage of the split bridge architecture!
Now we must make sure the atomic enable order doesn't change...

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/9] drm/meson: vclk: drop hdmi system clock setup

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> Poking the HHI syscon is a way to setup clocks behind CCF's back.
> Also, 2 drm code paths, the encoder and the hdmi-phy, are racing to do the
> same setup of the HDMI system clock.
> 
> This clock is used is used by the HDMI phy and should not be set by the
> encoder, so drop those HHI pokes from vclk.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_vclk.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
> index 2a942dc6a6dc..bf5cc5d92346 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -813,14 +813,6 @@ static void meson_vclk_set(struct meson_drm *priv, unsigned int pll_base_freq,
>   {
>   	unsigned int m = 0, frac = 0;
>   
> -	/* Set HDMI-TX sys clock */
> -	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
> -			   CTS_HDMI_SYS_SEL_MASK, 0);
> -	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
> -			   CTS_HDMI_SYS_DIV_MASK, 0);
> -	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL,
> -			   CTS_HDMI_SYS_EN, CTS_HDMI_SYS_EN);
> -
>   	/* Set HDMI PLL rate */
>   	if (!od1 && !od2 && !od3) {
>   		meson_hdmi_pll_generic_set(priv, pll_base_freq);

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/9] drm/meson: dw-hdmi: use generic clock helpers

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> The Amlogic HDMI phy driver is not doing anything with the clocks
> besides enabling on probe. CCF provides generic helpers to do that.
> 
> Use the generic clock helpers rather than using a custom one to get and
> enable clocks.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 36 +++------------------------
>   1 file changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index bcf4f83582f2..2890796f9d49 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -619,29 +619,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
>   
>   }
>   
> -static void meson_disable_clk(void *data)
> -{
> -	clk_disable_unprepare(data);
> -}
> -
> -static int meson_enable_clk(struct device *dev, char *name)
> -{
> -	struct clk *clk;
> -	int ret;
> -
> -	clk = devm_clk_get(dev, name);
> -	if (IS_ERR(clk)) {
> -		dev_err(dev, "Unable to get %s pclk\n", name);
> -		return PTR_ERR(clk);
> -	}
> -
> -	ret = clk_prepare_enable(clk);
> -	if (!ret)
> -		ret = devm_add_action_or_reset(dev, meson_disable_clk, clk);
> -
> -	return ret;
> -}
> -
>   static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   				void *data)
>   {
> @@ -651,6 +628,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	struct drm_device *drm = data;
>   	struct meson_drm *priv = drm->dev_private;
>   	struct dw_hdmi_plat_data *dw_plat_data;
> +	struct clk_bulk_data *clks;
>   	int irq;
>   	int ret;
>   
> @@ -701,17 +679,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	if (IS_ERR(meson_dw_hdmi->hdmitx))
>   		return PTR_ERR(meson_dw_hdmi->hdmitx);
>   
> -	ret = meson_enable_clk(dev, "isfr");
> -	if (ret)
> -		return ret;
> -
> -	ret = meson_enable_clk(dev, "iahb");
> +	ret = devm_clk_bulk_get_all_enable(dev, &clks);
>   	if (ret)
> -		return ret;
> -
> -	ret = meson_enable_clk(dev, "venci");
> -	if (ret)
> -		return 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);

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 4/9] drm/meson: dw-hdmi: fix incorrect comment in suspend

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> Comment in suspend says TOP is put in suspend, but the register
> poke following is actually de-asserting the reset, like in init.
> 
> It is doing the opposite of what the comment says.
> 
> Align the comment with what the code is doing for now and add
> a FIXME note to sort this out later
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 2890796f9d49..5cd3264ab874 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -751,7 +751,7 @@ static int __maybe_unused meson_dw_hdmi_pm_suspend(struct device *dev)
>   	if (!meson_dw_hdmi)
>   		return 0;
>   
> -	/* Reset TOP */
> +	/* FIXME: This actually bring top out reset on suspend, why ? */
>   	meson_dw_hdmi->data->top_write(meson_dw_hdmi,
>   				       HDMITX_TOP_SW_RESET, 0);
>   

Yes, this is probably useless and should:
meson_dw_hdmi->data->top_write(meson_dw_hdmi,
			       HDMITX_TOP_SW_RESET, 0xffff);

but I think it can be safely removed.

Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 5/9] drm/meson: dw-hdmi: split resets out of hw init.

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> This prepares the migration to regmap usage.
> 
> To properly setup regmap, the APB needs to be in working order.
> This is easier handled if the resets are not mixed with hw init.
> 
> More checks are required to determine if the resets are needed
> on resume or not. Add a note for this.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 5cd3264ab874..47aa3e184e98 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -581,11 +581,6 @@ 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);
>   
> -	/* Reset HDMITX APB & TX & PHY */
> -	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
> -	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
> -	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
> -
>   	/* Enable APB3 fail on error */
>   	if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>   		writel_bits_relaxed(BIT(15), BIT(15),
> @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   		return PTR_ERR(meson_dw_hdmi->hdmitx_phy);
>   	}
>   
> +	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
> +	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
> +	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
> +
>   	meson_dw_hdmi->hdmitx = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(meson_dw_hdmi->hdmitx))
>   		return PTR_ERR(meson_dw_hdmi->hdmitx);
> @@ -765,6 +764,11 @@ static int __maybe_unused meson_dw_hdmi_pm_resume(struct device *dev)
>   	if (!meson_dw_hdmi)
>   		return 0;
>   
> +	/* TODO: Is this really necessary/desirable on resume ? */

Yes to reset the HDMI controller to it's default state, not sure if the note is important here.

> +	reset_control_reset(meson_dw_hdmi->hdmitx_apb);
> +	reset_control_reset(meson_dw_hdmi->hdmitx_ctrl);
> +	reset_control_reset(meson_dw_hdmi->hdmitx_phy);
> +
>   	meson_dw_hdmi_init(meson_dw_hdmi);
>   
>   	dw_hdmi_resume(meson_dw_hdmi->hdmi);


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Neil Armstrong]

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@baylibre.com>
> ---
>   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 */


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> Using several string comparisons with if/else if/else clauses
> is fairly inefficient and does not scale well.

Inefficient in which way ? speed ? code size ?

It doesn't scale, but AFAIK Amlogic stopped using the Synopsys DWC controller after the G12B SoCs....

> 
> Use matched data to tweak the driver depending on the matched
> SoC instead.

This leads to a very overcomplicated code I'll need some time to review and understand

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 209 +++++++++++++++++---------
>   1 file changed, 139 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7c39e5c99043..ef059c5ef520 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -125,8 +125,17 @@
>   #define HHI_HDMI_PHY_CNTL4	0x3b0 /* 0xec */
>   #define HHI_HDMI_PHY_CNTL5	0x3b4 /* 0xed */
>   
> +struct meson_dw_hdmi_speed {
> +	const struct reg_sequence *regs;
> +	unsigned int reg_num;
> +	unsigned int limit;
> +};
> +
>   struct meson_dw_hdmi_data {
>   	int (*reg_init)(struct device *dev);
> +	const struct meson_dw_hdmi_speed *speeds;
> +	unsigned int speed_num;
> +	bool use_drm_infoframe;
>   	u32 cntl0_init;
>   	u32 cntl1_init;
>   };
> @@ -185,78 +194,33 @@ struct meson_dw_hdmi {
>   	struct regmap *top;
>   };
>   
> -static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
> -					const char *compat)
> -{
> -	return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
> -}
> -
> -/* Bridge */
> -
>   /* Setup PHY bandwidth modes */
> -static void meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
> +static int meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
>   				      const struct drm_display_mode *mode,
>   				      bool mode_is_420)
>   {
>   	struct meson_drm *priv = dw_hdmi->priv;
>   	unsigned int pixel_clock = mode->clock;
> +	int i;
>   
>   	/* For 420, pixel clock is half unlike venc clock */
> -	if (mode_is_420) pixel_clock /= 2;
> -
> -	if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> -	    dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi")) {
> -		if (pixel_clock >= 371250) {
> -			/* 5.94Gbps, 3.7125Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x333d3282);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2136315b);
> -		} else if (pixel_clock >= 297000) {
> -			/* 2.97Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303382);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2036315b);
> -		} else if (pixel_clock >= 148500) {
> -			/* 1.485Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303362);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2016315b);
> -		} else {
> -			/* 742.5Mbps, and below */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33604142);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x0016315b);
> -		}
> -	} else if (dw_hdmi_is_compatible(dw_hdmi,
> -					 "amlogic,meson-gxbb-dw-hdmi")) {
> -		if (pixel_clock >= 371250) {
> -			/* 5.94Gbps, 3.7125Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33353245);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2100115b);
> -		} else if (pixel_clock >= 297000) {
> -			/* 2.97Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33634283);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0xb000115b);
> -		} else {
> -			/* 1.485Gbps, and below */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33632122);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2000115b);
> -		}
> -	} else if (dw_hdmi_is_compatible(dw_hdmi,
> -					 "amlogic,meson-g12a-dw-hdmi")) {
> -		if (pixel_clock >= 371250) {
> -			/* 5.94Gbps, 3.7125Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x37eb65c4);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x0000080b);
> -		} else if (pixel_clock >= 297000) {
> -			/* 2.97Gbps */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb6262);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> -		} else {
> -			/* 1.485Gbps, and below */
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb4242);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> -			regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> -		}
> +	if (mode_is_420)
> +		pixel_clock /= 2;
> +
> +	for (i = 0; i < dw_hdmi->data->speed_num; i++) {
> +		if (pixel_clock >= dw_hdmi->data->speeds[i].limit)
> +			break;
>   	}
> +
> +	/* No match found - Last entry should have a 0 limit */
> +	if (WARN_ON(i == dw_hdmi->data->speed_num))
> +		return -EINVAL;
> +
> +	regmap_multi_reg_write(priv->hhi,
> +			       dw_hdmi->data->speeds[i].regs,
> +			       dw_hdmi->data->speeds[i].reg_num);
> +
> +	return 0;
>   }
>   
>   static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
> @@ -543,22 +507,133 @@ static int meson_dw_init_regmap_g12(struct device *dev)
>   	return 0;
>   }
>   
> +static const struct reg_sequence gxbb_3g7_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33353245 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2100115b },
> +};
> +
> +static const struct reg_sequence gxbb_3g_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33634283 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0xb000115b },
> +};
> +
> +static const struct reg_sequence gxbb_def_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33632122 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2000115b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxbb_speeds[] = {
> +	{
> +		.limit = 371250,
> +		.regs = gxbb_3g7_regs,
> +		.reg_num = ARRAY_SIZE(gxbb_3g7_regs)
> +	}, {
> +		.limit = 297000,
> +		.regs = gxbb_3g_regs,
> +		.reg_num = ARRAY_SIZE(gxbb_3g_regs)
> +	}, {
> +		.regs = gxbb_def_regs,
> +		.reg_num = ARRAY_SIZE(gxbb_def_regs)
> +	}
> +};
> +
>   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,
> +	.use_drm_infoframe = false,
> +	.speeds = gxbb_speeds,
> +	.speed_num = ARRAY_SIZE(gxbb_speeds),
> +};
> +
> +static const struct reg_sequence gxl_3g7_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x333d3282 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2136315b },
> +};
> +
> +static const struct reg_sequence gxl_3g_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303382 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2036315b },
> +};
> +
> +static const struct reg_sequence gxl_def_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303362 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2016315b },
> +};
> +
> +static const struct reg_sequence gxl_270m_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33604142 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x0016315b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
> +	{
> +		.limit = 371250,
> +		.regs = gxl_3g7_regs,
> +		.reg_num = ARRAY_SIZE(gxl_3g7_regs)
> +	}, {
> +		.limit = 297000,
> +		.regs = gxl_3g_regs,
> +		.reg_num = ARRAY_SIZE(gxl_3g_regs)
> +	}, {
> +		.limit = 148500,
> +		.regs = gxl_def_regs,
> +		.reg_num = ARRAY_SIZE(gxl_def_regs)
> +	}, {
> +		.regs = gxl_270m_regs,
> +		.reg_num = ARRAY_SIZE(gxl_270m_regs)
> +	}
>   };
>   
>   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,
> +	.use_drm_infoframe = true,
> +	.speeds = gxl_speeds,
> +	.speed_num = ARRAY_SIZE(gxl_speeds),
> +};
> +
> +static const struct reg_sequence g12a_3g7_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x37eb65c4 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> +	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x0000080b },
> +};
> +
> +static const struct reg_sequence g12a_3g_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb6262 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> +	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct reg_sequence g12a_def_regs[] = {
> +	{ .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb4242 },
> +	{ .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> +	{ .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct meson_dw_hdmi_speed g12a_speeds[] = {
> +	{
> +		.limit = 371250,
> +		.regs = g12a_3g7_regs,
> +		.reg_num = ARRAY_SIZE(g12a_3g7_regs)
> +	}, {
> +		.limit = 297000,
> +		.regs = g12a_3g_regs,
> +		.reg_num = ARRAY_SIZE(g12a_3g_regs)
> +	}, {
> +		.regs = g12a_def_regs,
> +		.reg_num = ARRAY_SIZE(g12a_def_regs)
> +	}
>   };
>   
>   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,
> +	.use_drm_infoframe = true,
> +	.speeds = g12a_speeds,
> +	.speed_num = ARRAY_SIZE(g12a_speeds),
>   };
>   
>   static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> @@ -590,7 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	platform_set_drvdata(pdev, meson_dw_hdmi);
>   
>   	meson_dw_hdmi->priv = priv;
> -	meson_dw_hdmi->dev = dev;

Unrelated change

>   	meson_dw_hdmi->data = match;
>   	dw_plat_data = &meson_dw_hdmi->dw_plat_data;
>   
> @@ -650,7 +724,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	meson_dw_hdmi_init(meson_dw_hdmi);
>   
>   	/* Bridge / Connector */
> -

Unrelated change

>   	dw_plat_data->priv_data = meson_dw_hdmi;
>   	dw_plat_data->phy_ops = &meson_dw_hdmi_phy_ops;
>   	dw_plat_data->phy_name = "meson_dw_hdmi_phy";
> @@ -659,11 +732,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>   	dw_plat_data->ycbcr_420_allowed = true;
>   	dw_plat_data->disable_cec = true;
>   	dw_plat_data->output_port = 1;
> -
> -	if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> -	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
> -	    dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
> -		dw_plat_data->use_drm_infoframe = true;
> +	dw_plat_data->use_drm_infoframe = meson_dw_hdmi->data->use_drm_infoframe;

Move this to a separate patch

>   
>   	meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
>   	if (IS_ERR(meson_dw_hdmi->hdmi))


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH LATER 8/9] drm/meson: dw-hdmi: don't write power controller registers

[Neil Armstrong]

On 30/07/2024 14:50, Jerome Brunet wrote:
> The HDMI phy has a power domain properly set in DT.
> 
> Writing the power controller register directly from the hdmi driver is
> incorrect. The power domain framework should be used for that.
> 
> HHI is a collection of Amlogic devices, such as clocks, reset,
> power domains and phys.
> 
> This is another step to get rid of HHI access in Amlogic display drivers
> and possibly stop using the component API.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> 
>   This change depends on:
>   * f1ab099d6591 ("arm64: dts: amlogic: add power domain to hdmitx")
> 
> Time is needed for these changes to sink in u-boot and distros,
> making this change safe to apply.


Well no, we will basically need to wait until none of the stable and long-stable kernel stops
shipping a kernel without this change, but you can check if a power-domain have been associated
with the device and do the same.

> 
>   drivers/gpu/drm/meson/meson_dw_hdmi.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index ef059c5ef520..6c18d97b8b16 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -111,7 +111,6 @@
>   #define HDMITX_TOP_G12A_OFFSET	0x8000
>   
>   /* HHI Registers */
> -#define HHI_MEM_PD_REG0		0x100 /* 0x40 */
>   #define HHI_HDMI_CLK_CNTL	0x1cc /* 0x73 */
>   #define HHI_HDMI_PHY_CNTL0	0x3a0 /* 0xe8 */
>   #define HHI_HDMI_PHY_CNTL1	0x3a4 /* 0xe9 */
> @@ -423,9 +422,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
>   	/* Enable clocks */
>   	regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0xffff, 0x100);
>   
> -	/* Bring HDMITX MEM output of power down */
> -	regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0);
> -
>   	/* Bring out of reset */
>   	regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
>   	msleep(20);


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Jerome Brunet]

On Mon 19 Aug 2024 at 18:22, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> 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@baylibre.com>
>> ---
>>   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

It makes clear that indirect addressing offset are for GX SoC only,
which the original define did not do

>
>> +#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.

Even if one cannot be accessed without configuring the other, it does
not make it a single region.

The regions do not even use the same register value width.
That's a clear indication they are separate.

Anyway, I'll remove the 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

That's not really consistent with priv->hhi, is it ?

>
>>   };
>>     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 */

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic