[PATCH 00/10] drm/ast: Detect device type before init

[PATCH 00/10] drm/ast: Detect device type before init

[Thomas Zimmermann]

Detecting the ast device's chipset type and configuration mode
involves several registers, DT properties and possibly POSTing
parts of the chip. It is preferable to do this before initializing
the DRM driver, so that that each chip type can have an individual
setup code.

The patchset addresses the problem by moving all early detection
code before the allocation of the ast device.

Patch one gets a lock out of the way. The lock is only relevant
for mode setting. Move it there.

Patches 2 and 3 rework the detection of the correct I/O memory
ranges. It is now self-contained, more readable and works without
an instance of struct ast_device.

Patches 4 to 7 rework the setup of various registers that are
required for detection. Access helpers for I/O can now operate
without an instance of struct ast_device. The setup functions
operate on the I/O ranges that have been made available with
patch 3, but again without struct ast_device.

With the detection's internals done, patches 8 and 9 rework the
chip's and config-mode's detection code to operate without struct
ast_device as well.

Finally, patch 10 moves the detection code into the PCI probe
function. it runs before any of the DRM device code. The fucntion
for creating an ast device, ast_device_create(), receives the
detected I/O memory ranges, chip type and configuration mode.

This cleans up the detection code. There is more chip-specific
code in other parts of the driver. In a later patch, the ast device
setup can be split up so that each chip type gets its own code
path that does not interfere with other chips.

Tested on AST1100 and AST2100.

Thomas Zimmermann (10):
  drm/ast: Turn ioregs_lock to modeset_lock
  drm/ast: Rework I/O register setup
  drm/ast: Retrieve I/O-memory ranges without ast device
  drm/ast: Add I/O helpers without ast device
  drm/ast: Enable VGA without ast device instance
  drm/ast: Enable MMIO without ast device instance
  drm/ast: Partially implement POST without ast device instance
  drm/ast: Add enum ast_config_mode
  drm/ast: Detect ast device type and config mode without ast device
  drm/ast: Move detection code into PCI probe helper

 drivers/gpu/drm/ast/ast_drv.c  | 261 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/ast/ast_drv.h  | 101 +++++++++----
 drivers/gpu/drm/ast/ast_main.c | 244 ++----------------------------
 drivers/gpu/drm/ast/ast_mode.c |  26 ++--
 drivers/gpu/drm/ast/ast_post.c |  73 +++++----
 drivers/gpu/drm/ast/ast_reg.h  |  12 +-
 6 files changed, 411 insertions(+), 306 deletions(-)


base-commit: b7816c393496dc4497c1327310821407f7171d8b
prerequisite-patch-id: 0aa359f6144c4015c140c8a6750be19099c676fb
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: cbc453ee02fae02af22fbfdce56ab732c7a88c36
-- 
2.42.0

[PATCH 01/10] drm/ast: Turn ioregs_lock to modeset_lock

[Thomas Zimmermann]

The lock for the I/O registers is only relevant during mode-setting
operations. It protects the registers from concurrent access from
reading EDID information.

Reduce lock coverage to mode setting, rename the lock and move it
entirely into the mode-setting code. No functional changes, as the
I/O lock was never used for anything else than mode setting.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  3 ++-
 drivers/gpu/drm/ast/ast_main.c |  4 ----
 drivers/gpu/drm/ast/ast_mode.c | 26 +++++++++++++++-----------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 2aee32344f4a2..8b5d6e2954858 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -181,7 +181,6 @@ to_ast_sil164_connector(struct drm_connector *connector)
 struct ast_device {
 	struct drm_device base;
 
-	struct mutex ioregs_lock; /* Protects access to I/O registers in ioregs */
 	void __iomem *regs;
 	void __iomem *ioregs;
 	void __iomem *dp501_fw_buf;
@@ -196,6 +195,8 @@ struct ast_device {
 	unsigned long	vram_size;
 	unsigned long	vram_fb_available;
 
+	struct mutex modeset_lock; /* Protects access to modeset I/O registers in ioregs */
+
 	struct ast_plane primary_plane;
 	struct ast_plane cursor_plane;
 	struct drm_crtc crtc;
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index f4ab40e22ceac..445cf47871a43 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -440,10 +440,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 
 	pci_set_drvdata(pdev, dev);
 
-	ret = drmm_mutex_init(dev, &ast->ioregs_lock);
-	if (ret)
-		return ERR_PTR(ret);
-
 	ast->regs = pcim_iomap(pdev, 1, 0);
 	if (!ast->regs)
 		return ERR_PTR(-EIO);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cb96149842851..817c291aef2c4 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1358,13 +1358,13 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
-	mutex_lock(&ast->ioregs_lock);
+	mutex_lock(&ast->modeset_lock);
 
 	edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
 	if (!edid)
 		goto err_mutex_unlock;
 
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
@@ -1372,7 +1372,7 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 	return count;
 
 err_mutex_unlock:
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
@@ -1464,13 +1464,13 @@ static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
-	mutex_lock(&ast->ioregs_lock);
+	mutex_lock(&ast->modeset_lock);
 
 	edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
 	if (!edid)
 		goto err_mutex_unlock;
 
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
@@ -1478,7 +1478,7 @@ static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector
 	return count;
 
 err_mutex_unlock:
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
@@ -1670,13 +1670,13 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
-	mutex_lock(&ast->ioregs_lock);
+	mutex_lock(&ast->modeset_lock);
 
 	succ = ast_astdp_read_edid(connector->dev, edid);
 	if (succ < 0)
 		goto err_mutex_unlock;
 
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 
 	drm_connector_update_edid_property(connector, edid);
 	count = drm_add_edid_modes(connector, edid);
@@ -1685,7 +1685,7 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
 	return count;
 
 err_mutex_unlock:
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 	kfree(edid);
 err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
@@ -1827,9 +1827,9 @@ static void ast_mode_config_helper_atomic_commit_tail(struct drm_atomic_state *s
 	 * display modes. Protect access to I/O registers by acquiring
 	 * the I/O-register lock. Released in atomic_flush().
 	 */
-	mutex_lock(&ast->ioregs_lock);
+	mutex_lock(&ast->modeset_lock);
 	drm_atomic_helper_commit_tail_rpm(state);
-	mutex_unlock(&ast->ioregs_lock);
+	mutex_unlock(&ast->modeset_lock);
 }
 
 static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs = {
@@ -1866,6 +1866,10 @@ int ast_mode_config_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	int ret;
 
+	ret = drmm_mutex_init(dev, &ast->modeset_lock);
+	if (ret)
+		return ret;
+
 	ret = drmm_mode_config_init(dev);
 	if (ret)
 		return ret;
-- 
2.42.0

[PATCH 02/10] drm/ast: Rework I/O register setup

[Thomas Zimmermann]

There are three different ways of retrieving the I/O-memory ranges
for AST devices: either from PCI BAR 1, from PCI BAR 2 or from PCI
BAR 1 by 'guessing'.

Make the respective code more readable by making each case self-
contained. Also add error checking against the length of the PCI
BARs.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 40 +++++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_reg.h  |  1 +
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 445cf47871a43..2c2700256a966 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -444,22 +444,42 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	if (!ast->regs)
 		return ERR_PTR(-EIO);
 
-	/*
-	 * After AST2500, MMIO is enabled by default, and it should be adopted
-	 * to be compatible with Arm.
-	 */
 	if (pdev->revision >= 0x40) {
+		/*
+		 * On AST2500 and later models, MMIO is enabled by
+		 * default. Adopt it to be compatible with ARM.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 1);
+
+		if (len < AST_IO_MM_OFFSET)
+			return ERR_PTR(-EIO);
+		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
+			return ERR_PTR(-EIO);
 		ast->ioregs = ast->regs + AST_IO_MM_OFFSET;
-	} else if (!(pci_resource_flags(pdev, 2) & IORESOURCE_IO)) {
-		drm_info(dev, "platform has no IO space, trying MMIO\n");
-		ast->ioregs = ast->regs + AST_IO_MM_OFFSET;
-	}
+	} else if (pci_resource_flags(pdev, 2) & IORESOURCE_IO) {
+		/*
+		 * Map I/O registers if we have a PCI BAR for I/O.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 2);
 
-	/* "map" IO regs if the above hasn't done so already */
-	if (!ast->ioregs) {
+		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
+			return ERR_PTR(-EIO);
 		ast->ioregs = pcim_iomap(pdev, 2, 0);
 		if (!ast->ioregs)
 			return ERR_PTR(-EIO);
+	} else {
+		/*
+		 * Anything else is best effort.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 1);
+
+		if (len < AST_IO_MM_OFFSET)
+			return ERR_PTR(-EIO);
+		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
+			return ERR_PTR(-EIO);
+		ast->ioregs = ast->regs + AST_IO_MM_OFFSET;
+
+		drm_info(dev, "Platform has no I/O space, using MMIO\n");
 	}
 
 	if (!ast_is_vga_enabled(dev)) {
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index 555286ecf5209..f1e1700c01ed2 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -10,6 +10,7 @@
  */
 
 #define AST_IO_MM_OFFSET		(0x380)
+#define AST_IO_MM_LENGTH		(0xff)
 
 #define AST_IO_VGAARI_W			(0x40)
 #define AST_IO_VGAMR_W			(0x42)
-- 
2.42.0

[PATCH 03/10] drm/ast: Retrieve I/O-memory ranges without ast device

[Thomas Zimmermann]

Read the I/O-memory ranges into local variables before setting
them in the ast device instanace. We'll later need this to split
detecting the device type from the creation of the ast device
instance.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 2c2700256a966..43116df577276 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -432,6 +432,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	struct ast_device *ast;
 	bool need_post = false;
 	int ret = 0;
+	void __iomem *regs;
+	void __iomem *ioregs;
 
 	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
 	if (IS_ERR(ast))
@@ -440,8 +442,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 
 	pci_set_drvdata(pdev, dev);
 
-	ast->regs = pcim_iomap(pdev, 1, 0);
-	if (!ast->regs)
+	regs = pcim_iomap(pdev, 1, 0);
+	if (!regs)
 		return ERR_PTR(-EIO);
 
 	if (pdev->revision >= 0x40) {
@@ -455,7 +457,7 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 			return ERR_PTR(-EIO);
 		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
 			return ERR_PTR(-EIO);
-		ast->ioregs = ast->regs + AST_IO_MM_OFFSET;
+		ioregs = regs + AST_IO_MM_OFFSET;
 	} else if (pci_resource_flags(pdev, 2) & IORESOURCE_IO) {
 		/*
 		 * Map I/O registers if we have a PCI BAR for I/O.
@@ -464,8 +466,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 
 		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
 			return ERR_PTR(-EIO);
-		ast->ioregs = pcim_iomap(pdev, 2, 0);
-		if (!ast->ioregs)
+		ioregs = pcim_iomap(pdev, 2, 0);
+		if (!ioregs)
 			return ERR_PTR(-EIO);
 	} else {
 		/*
@@ -477,11 +479,14 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 			return ERR_PTR(-EIO);
 		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
 			return ERR_PTR(-EIO);
-		ast->ioregs = ast->regs + AST_IO_MM_OFFSET;
+		ioregs = regs + AST_IO_MM_OFFSET;
 
 		drm_info(dev, "Platform has no I/O space, using MMIO\n");
 	}
 
+	ast->regs = regs;
+	ast->ioregs = ioregs;
+
 	if (!ast_is_vga_enabled(dev)) {
 		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
 		need_post = true;
-- 
2.42.0

[PATCH 04/10] drm/ast: Add I/O helpers without ast device

[Thomas Zimmermann]

Implement I/O access in helpers that do now use an ast device
instance, but the raw pointer to the I/O memory. We'll later need
these helpers to detect the device type before allocating the ast
device instance.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h | 73 +++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 8b5d6e2954858..b82be890d9fce 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -262,55 +262,94 @@ static inline bool __ast_gen_is_eq(struct ast_device *ast, unsigned long gen)
 #define IS_AST_GEN6(__ast)	__ast_gen_is_eq(__ast, 6)
 #define IS_AST_GEN7(__ast)	__ast_gen_is_eq(__ast, 7)
 
+static inline u8 __ast_read8(const void __iomem *addr, u32 reg)
+{
+	return ioread8(addr + reg);
+}
+
+static inline u32 __ast_read32(const void __iomem *addr, u32 reg)
+{
+	return ioread32(addr + reg);
+}
+
+static inline void __ast_write8(void __iomem *addr, u32 reg, u8 val)
+{
+	iowrite8(val, addr + reg);
+}
+
+static inline void __ast_write32(void __iomem *addr, u32 reg, u32 val)
+{
+	iowrite32(val, addr + reg);
+}
+
+static inline u8 __ast_read8_i(void __iomem *addr, u32 reg, u8 index)
+{
+	__ast_write8(addr, reg, index);
+	return __ast_read8(addr, reg + 1);
+}
+
+static inline u8 __ast_read8_i_masked(void __iomem *addr, u32 reg, u8 index, u8 read_mask)
+{
+	u8 val = __ast_read8_i(addr, reg, index);
+
+	return val & read_mask;
+}
+
+static inline void __ast_write8_i(void __iomem *addr, u32 reg, u8 index, u8 val)
+{
+	__ast_write8(addr, reg, index);
+	__ast_write8(addr, reg + 1, val);
+}
+
+static inline void __ast_write8_i_masked(void __iomem *addr, u32 reg, u8 index, u8 read_mask,
+					 u8 val)
+{
+	u8 tmp = __ast_read8_i_masked(addr, reg, index, read_mask);
+
+	tmp |= val;
+	__ast_write8_i(addr, reg, index, tmp);
+}
+
 static inline u32 ast_read32(struct ast_device *ast, u32 reg)
 {
-	return ioread32(ast->regs + reg);
+	return __ast_read32(ast->regs, reg);
 }
 
 static inline void ast_write32(struct ast_device *ast, u32 reg, u32 val)
 {
-	iowrite32(val, ast->regs + reg);
+	__ast_write32(ast->regs, reg, val);
 }
 
 static inline u8 ast_io_read8(struct ast_device *ast, u32 reg)
 {
-	return ioread8(ast->ioregs + reg);
+	return __ast_read8(ast->ioregs, reg);
 }
 
 static inline void ast_io_write8(struct ast_device *ast, u32 reg, u8 val)
 {
-	iowrite8(val, ast->ioregs + reg);
+	__ast_write8(ast->ioregs, reg, val);
 }
 
 static inline u8 ast_get_index_reg(struct ast_device *ast, u32 base, u8 index)
 {
-	ast_io_write8(ast, base, index);
-	++base;
-	return ast_io_read8(ast, base);
+	return __ast_read8_i(ast->ioregs, base, index);
 }
 
 static inline u8 ast_get_index_reg_mask(struct ast_device *ast, u32 base, u8 index,
 					u8 preserve_mask)
 {
-	u8 val = ast_get_index_reg(ast, base, index);
-
-	return val & preserve_mask;
+	return __ast_read8_i_masked(ast->ioregs, base, index, preserve_mask);
 }
 
 static inline void ast_set_index_reg(struct ast_device *ast, u32 base, u8 index, u8 val)
 {
-	ast_io_write8(ast, base, index);
-	++base;
-	ast_io_write8(ast, base, val);
+	__ast_write8_i(ast->ioregs, base, index, val);
 }
 
 static inline void ast_set_index_reg_mask(struct ast_device *ast, u32 base, u8 index,
 					  u8 preserve_mask, u8 val)
 {
-	u8 tmp = ast_get_index_reg_mask(ast, base, index, preserve_mask);
-
-	tmp |= val;
-	ast_set_index_reg(ast, base, index, tmp);
+	__ast_write8_i_masked(ast->ioregs, base, index, preserve_mask, val);
 }
 
 #define AST_VIDMEM_SIZE_8M    0x00800000
-- 
2.42.0

[PATCH 05/10] drm/ast: Enable VGA without ast device instance

[Thomas Zimmermann]

We'll have to enable the VGA functionality for detecting the ast
device type. Make this work without an instance of the ast device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 29 ++++++++++++-----------------
 drivers/gpu/drm/ast/ast_reg.h  |  9 +++++++--
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 43116df577276..1d2ec77c1d8d9 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,22 +35,17 @@
 
 #include "ast_drv.h"
 
-static bool ast_is_vga_enabled(struct drm_device *dev)
+static bool ast_is_vga_enabled(void __iomem *ioregs)
 {
-	struct ast_device *ast = to_ast_device(dev);
-	u8 ch;
-
-	ch = ast_io_read8(ast, AST_IO_VGAER);
+	u8 vgaer = __ast_read8(ioregs, AST_IO_VGAER);
 
-	return !!(ch & 0x01);
+	return vgaer & AST_IO_VGAER_VGA_ENABLE;
 }
 
-static void ast_enable_vga(struct drm_device *dev)
+static void ast_enable_vga(void __iomem *ioregs)
 {
-	struct ast_device *ast = to_ast_device(dev);
-
-	ast_io_write8(ast, AST_IO_VGAER, 0x01);
-	ast_io_write8(ast, AST_IO_VGAMR_W, 0x01);
+	__ast_write8(ioregs, AST_IO_VGAER, AST_IO_VGAER_VGA_ENABLE);
+	__ast_write8(ioregs, AST_IO_VGAMR_W, AST_IO_VGAMR_IOSEL);
 }
 
 /*
@@ -74,9 +69,9 @@ static int ast_enable_mmio(struct ast_device *ast)
 	return devm_add_action_or_reset(dev->dev, ast_enable_mmio_release, ast);
 }
 
-static void ast_open_key(struct ast_device *ast)
+static void ast_open_key(void __iomem *ioregs)
 {
-	ast_set_index_reg(ast, AST_IO_VGACRI, 0x80, 0xA8);
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, AST_IO_VGACR80_PASSWORD);
 }
 
 static int ast_device_config_init(struct ast_device *ast)
@@ -487,7 +482,7 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	ast->regs = regs;
 	ast->ioregs = ioregs;
 
-	if (!ast_is_vga_enabled(dev)) {
+	if (!ast_is_vga_enabled(ioregs)) {
 		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
 		need_post = true;
 	}
@@ -497,10 +492,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	 * access to the scratch registers will fail.
 	 */
 	if (need_post)
-		ast_enable_vga(dev);
-
+		ast_enable_vga(ioregs);
 	/* Enable extended register access */
-	ast_open_key(ast);
+	ast_open_key(ioregs);
+
 	ret = ast_enable_mmio(ast);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index f1e1700c01ed2..826fcb08398ec 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -13,8 +13,14 @@
 #define AST_IO_MM_LENGTH		(0xff)
 
 #define AST_IO_VGAARI_W			(0x40)
+
 #define AST_IO_VGAMR_W			(0x42)
+#define AST_IO_VGAMR_R			(0x4c)
+#define AST_IO_VGAMR_IOSEL		BIT(0)
+
 #define AST_IO_VGAER			(0x43)
+#define AST_IO_VGAER_VGA_ENABLE		BIT(0)
+
 #define AST_IO_VGASRI			(0x44)
 #define AST_IO_VGADRR			(0x47)
 #define AST_IO_VGADWR			(0x48)
@@ -22,14 +28,13 @@
 #define AST_IO_VGAGRI			(0x4E)
 
 #define AST_IO_VGACRI			(0x54)
+#define AST_IO_VGACR80_PASSWORD		(0xa8)
 #define AST_IO_VGACRCB_HWC_16BPP	BIT(0) /* set: ARGB4444, cleared: 2bpp palette */
 #define AST_IO_VGACRCB_HWC_ENABLED	BIT(1)
 
 #define AST_IO_VGAIR1_R			(0x5A)
 #define AST_IO_VGAIR1_VREFRESH		BIT(3)
 
-#define AST_IO_VGAMR_R			(0x4C)
-
 /*
  * Display Transmitter Type
  */
-- 
2.42.0

[PATCH 06/10] drm/ast: Enable MMIO without ast device instance

[Thomas Zimmermann]

We'll have to enable the MMIO access for detecting the ast device
type. Make this work without an instance of the ast device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 16 +++++++++-------
 drivers/gpu/drm/ast/ast_reg.h  |  2 ++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 1d2ec77c1d8d9..a24d3529373c1 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -54,19 +54,21 @@ static void ast_enable_vga(void __iomem *ioregs)
  */
 static void ast_enable_mmio_release(void *data)
 {
-	struct ast_device *ast = data;
+	void __iomem *ioregs = (void __force __iomem *)data;
 
 	/* enable standard VGA decode */
-	ast_set_index_reg(ast, AST_IO_VGACRI, 0xa1, 0x04);
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1, AST_IO_VGACRA1_MMIO_ENABLED);
 }
 
-static int ast_enable_mmio(struct ast_device *ast)
+static int ast_enable_mmio(struct device *dev, void __iomem *ioregs)
 {
-	struct drm_device *dev = &ast->base;
+	void *data = (void __force *)ioregs;
 
-	ast_set_index_reg(ast, AST_IO_VGACRI, 0xa1, 0x06);
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1,
+		       AST_IO_VGACRA1_MMIO_ENABLED |
+		       AST_IO_VGACRA1_VGAIO_DISABLED);
 
-	return devm_add_action_or_reset(dev->dev, ast_enable_mmio_release, ast);
+	return devm_add_action_or_reset(dev, ast_enable_mmio_release, data);
 }
 
 static void ast_open_key(void __iomem *ioregs)
@@ -496,7 +498,7 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	/* Enable extended register access */
 	ast_open_key(ioregs);
 
-	ret = ast_enable_mmio(ast);
+	ret = ast_enable_mmio(&pdev->dev, ioregs);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/ast/ast_reg.h b/drivers/gpu/drm/ast/ast_reg.h
index 826fcb08398ec..236547a0280d9 100644
--- a/drivers/gpu/drm/ast/ast_reg.h
+++ b/drivers/gpu/drm/ast/ast_reg.h
@@ -29,6 +29,8 @@
 
 #define AST_IO_VGACRI			(0x54)
 #define AST_IO_VGACR80_PASSWORD		(0xa8)
+#define AST_IO_VGACRA1_VGAIO_DISABLED	BIT(1)
+#define AST_IO_VGACRA1_MMIO_ENABLED	BIT(2)
 #define AST_IO_VGACRCB_HWC_16BPP	BIT(0) /* set: ARGB4444, cleared: 2bpp palette */
 #define AST_IO_VGACRCB_HWC_ENABLED	BIT(1)
 
-- 
2.42.0

[PATCH 07/10] drm/ast: Partially implement POST without ast device instance

[Thomas Zimmermann]

We'll have to do some of the GPU POSTing for detecting the ast device
type. Make this work without an instance of the ast device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  2 +-
 drivers/gpu/drm/ast/ast_main.c |  2 +-
 drivers/gpu/drm/ast/ast_post.c | 73 +++++++++++++++++++++-------------
 3 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index b82be890d9fce..491603a13151c 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -471,7 +471,7 @@ int ast_mm_init(struct ast_device *ast);
 void ast_post_gpu(struct drm_device *dev);
 u32 ast_mindwm(struct ast_device *ast, u32 r);
 void ast_moutdwm(struct ast_device *ast, u32 r, u32 v);
-void ast_patch_ahb_2500(struct ast_device *ast);
+void ast_patch_ahb_2500(void __iomem *regs);
 /* ast dp501 */
 void ast_set_dp501_video_output(struct drm_device *dev, u8 mode);
 bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index a24d3529373c1..f100df8d74f71 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -113,7 +113,7 @@ static int ast_device_config_init(struct ast_device *ast)
 			/* Patch AST2500/AST2510 */
 			if ((pdev->revision & 0xf0) == 0x40) {
 				if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
-					ast_patch_ahb_2500(ast);
+					ast_patch_ahb_2500(ast->regs);
 			}
 
 			/* Double check that it's actually working */
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 7a993a3843147..22f548805dfb0 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -77,28 +77,42 @@ ast_set_def_ext_reg(struct drm_device *dev)
 	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb6, 0xff, reg);
 }
 
-u32 ast_mindwm(struct ast_device *ast, u32 r)
+static u32 __ast_mindwm(void __iomem *regs, u32 r)
 {
-	uint32_t data;
+	u32 data;
 
-	ast_write32(ast, 0xf004, r & 0xffff0000);
-	ast_write32(ast, 0xf000, 0x1);
+	__ast_write32(regs, 0xf004, r & 0xffff0000);
+	__ast_write32(regs, 0xf000, 0x1);
 
 	do {
-		data = ast_read32(ast, 0xf004) & 0xffff0000;
+		data = __ast_read32(regs, 0xf004) & 0xffff0000;
 	} while (data != (r & 0xffff0000));
-	return ast_read32(ast, 0x10000 + (r & 0x0000ffff));
+
+	return __ast_read32(regs, 0x10000 + (r & 0x0000ffff));
 }
 
-void ast_moutdwm(struct ast_device *ast, u32 r, u32 v)
+static void __ast_moutdwm(void __iomem *regs, u32 r, u32 v)
 {
-	uint32_t data;
-	ast_write32(ast, 0xf004, r & 0xffff0000);
-	ast_write32(ast, 0xf000, 0x1);
+	u32 data;
+
+	__ast_write32(regs, 0xf004, r & 0xffff0000);
+	__ast_write32(regs, 0xf000, 0x1);
+
 	do {
-		data = ast_read32(ast, 0xf004) & 0xffff0000;
+		data = __ast_read32(regs, 0xf004) & 0xffff0000;
 	} while (data != (r & 0xffff0000));
-	ast_write32(ast, 0x10000 + (r & 0x0000ffff), v);
+
+	__ast_write32(regs, 0x10000 + (r & 0x0000ffff), v);
+}
+
+u32 ast_mindwm(struct ast_device *ast, u32 r)
+{
+	return __ast_mindwm(ast->regs, r);
+}
+
+void ast_moutdwm(struct ast_device *ast, u32 r, u32 v)
+{
+	__ast_moutdwm(ast->regs, r, v);
 }
 
 /*
@@ -1987,17 +2001,18 @@ static bool ast_dram_init_2500(struct ast_device *ast)
 	return true;
 }
 
-void ast_patch_ahb_2500(struct ast_device *ast)
+void ast_patch_ahb_2500(void __iomem *regs)
 {
-	u32	data;
+	u32 data;
 
 	/* Clear bus lock condition */
-	ast_moutdwm(ast, 0x1e600000, 0xAEED1A03);
-	ast_moutdwm(ast, 0x1e600084, 0x00010000);
-	ast_moutdwm(ast, 0x1e600088, 0x00000000);
-	ast_moutdwm(ast, 0x1e6e2000, 0x1688A8A8);
-	data = ast_mindwm(ast, 0x1e6e2070);
-	if (data & 0x08000000) {					/* check fast reset */
+	__ast_moutdwm(regs, 0x1e600000, 0xAEED1A03);
+	__ast_moutdwm(regs, 0x1e600084, 0x00010000);
+	__ast_moutdwm(regs, 0x1e600088, 0x00000000);
+	__ast_moutdwm(regs, 0x1e6e2000, 0x1688A8A8);
+
+	data = __ast_mindwm(regs, 0x1e6e2070);
+	if (data & 0x08000000) { /* check fast reset */
 		/*
 		 * If "Fast restet" is enabled for ARM-ICE debugger,
 		 * then WDT needs to enable, that
@@ -2009,16 +2024,18 @@ void ast_patch_ahb_2500(struct ast_device *ast)
 		 *	[1]:= 1:WDT will be cleeared and disabled after timeout occurs
 		 *	[0]:= 1:WDT enable
 		 */
-		ast_moutdwm(ast, 0x1E785004, 0x00000010);
-		ast_moutdwm(ast, 0x1E785008, 0x00004755);
-		ast_moutdwm(ast, 0x1E78500c, 0x00000033);
+		__ast_moutdwm(regs, 0x1E785004, 0x00000010);
+		__ast_moutdwm(regs, 0x1E785008, 0x00004755);
+		__ast_moutdwm(regs, 0x1E78500c, 0x00000033);
 		udelay(1000);
 	}
+
 	do {
-		ast_moutdwm(ast, 0x1e6e2000, 0x1688A8A8);
-		data = ast_mindwm(ast, 0x1e6e2000);
-	}	while (data != 1);
-	ast_moutdwm(ast, 0x1e6e207c, 0x08000000);	/* clear fast reset */
+		__ast_moutdwm(regs, 0x1e6e2000, 0x1688A8A8);
+		data = __ast_mindwm(regs, 0x1e6e2000);
+	} while (data != 1);
+
+	__ast_moutdwm(regs, 0x1e6e207c, 0x08000000); /* clear fast reset */
 }
 
 void ast_post_chip_2500(struct drm_device *dev)
@@ -2030,7 +2047,7 @@ void ast_post_chip_2500(struct drm_device *dev)
 	reg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd0, 0xff);
 	if ((reg & AST_VRAM_INIT_STATUS_MASK) == 0) {/* vga only */
 		/* Clear bus lock condition */
-		ast_patch_ahb_2500(ast);
+		ast_patch_ahb_2500(ast->regs);
 
 		/* Disable watchdog */
 		ast_moutdwm(ast, 0x1E78502C, 0x00000000);
-- 
2.42.0

[PATCH 08/10] drm/ast: Add enum ast_config_mode

[Thomas Zimmermann]

The config mode used to be a field in struct ast_device. Turn it into
a named type. We'll need this for device detection.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 491603a13151c..b0c899f2ecfd7 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -98,6 +98,12 @@ enum ast_tx_chip {
 #define AST_TX_DP501_BIT	BIT(AST_TX_DP501)
 #define AST_TX_ASTDP_BIT	BIT(AST_TX_ASTDP)
 
+enum ast_config_mode {
+	ast_use_p2a,
+	ast_use_dt,
+	ast_use_defaults
+};
+
 #define AST_DRAM_512Mx16 0
 #define AST_DRAM_1Gx16   1
 #define AST_DRAM_512Mx32 2
@@ -185,7 +191,9 @@ struct ast_device {
 	void __iomem *ioregs;
 	void __iomem *dp501_fw_buf;
 
+	enum ast_config_mode config_mode;
 	enum ast_chip chip;
+
 	uint32_t dram_bus_width;
 	uint32_t dram_type;
 	uint32_t mclk;
@@ -224,11 +232,6 @@ struct ast_device {
 	} output;
 
 	bool support_wide_screen;
-	enum {
-		ast_use_p2a,
-		ast_use_dt,
-		ast_use_defaults
-	} config_mode;
 
 	unsigned long tx_chip_types;		/* bitfield of enum ast_chip_type */
 	u8 *dp501_fw_addr;
-- 
2.42.0

[PATCH 09/10] drm/ast: Detect ast device type and config mode without ast device

[Thomas Zimmermann]

Return the ast chip and config in the detection function's parameters
instead of storing them directly in the ast device instance.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 104 ++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index f100df8d74f71..331a9a861153b 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -76,25 +76,27 @@ static void ast_open_key(void __iomem *ioregs)
 	__ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, AST_IO_VGACR80_PASSWORD);
 }
 
-static int ast_device_config_init(struct ast_device *ast)
+static int ast_detect_chip(struct pci_dev *pdev,
+			   void __iomem *regs, void __iomem *ioregs,
+			   enum ast_chip *chip_out,
+			   enum ast_config_mode *config_mode_out)
 {
-	struct drm_device *dev = &ast->base;
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	struct device_node *np = dev->dev->of_node;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	enum ast_config_mode config_mode = ast_use_defaults;
 	uint32_t scu_rev = 0xffffffff;
+	enum ast_chip chip;
 	u32 data;
-	u8 jregd0, jregd1;
+	u8 vgacrd0, vgacrd1;
 
 	/*
 	 * Find configuration mode and read SCU revision
 	 */
 
-	ast->config_mode = ast_use_defaults;
-
 	/* Check if we have device-tree properties */
 	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
 		/* We do, disable P2A access */
-		ast->config_mode = ast_use_dt;
+		config_mode = ast_use_dt;
 		scu_rev = data;
 	} else if (pdev->device == PCI_CHIP_AST2000) { // Not all families have a P2A bridge
 		/*
@@ -102,9 +104,9 @@ static int ast_device_config_init(struct ast_device *ast)
 		 * is disabled. We force using P2A if VGA only mode bit
 		 * is set D[7]
 		 */
-		jregd0 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd0, 0xff);
-		jregd1 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
-		if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
+		vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
+		vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
+		if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
 
 			/*
 			 * We have a P2A bridge and it is enabled.
@@ -112,32 +114,32 @@ static int ast_device_config_init(struct ast_device *ast)
 
 			/* Patch AST2500/AST2510 */
 			if ((pdev->revision & 0xf0) == 0x40) {
-				if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
-					ast_patch_ahb_2500(ast->regs);
+				if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
+					ast_patch_ahb_2500(regs);
 			}
 
 			/* Double check that it's actually working */
-			data = ast_read32(ast, 0xf004);
+			data = __ast_read32(regs, 0xf004);
 			if ((data != 0xffffffff) && (data != 0x00)) {
-				ast->config_mode = ast_use_p2a;
+				config_mode = ast_use_p2a;
 
 				/* Read SCU7c (silicon revision register) */
-				ast_write32(ast, 0xf004, 0x1e6e0000);
-				ast_write32(ast, 0xf000, 0x1);
-				scu_rev = ast_read32(ast, 0x1207c);
+				__ast_write32(regs, 0xf004, 0x1e6e0000);
+				__ast_write32(regs, 0xf000, 0x1);
+				scu_rev = __ast_read32(regs, 0x1207c);
 			}
 		}
 	}
 
-	switch (ast->config_mode) {
+	switch (config_mode) {
 	case ast_use_defaults:
-		drm_info(dev, "Using default configuration\n");
+		dev_info(dev, "Using default configuration\n");
 		break;
 	case ast_use_dt:
-		drm_info(dev, "Using device-tree for configuration\n");
+		dev_info(dev, "Using device-tree for configuration\n");
 		break;
 	case ast_use_p2a:
-		drm_info(dev, "Using P2A bridge for configuration\n");
+		dev_info(dev, "Using P2A bridge for configuration\n");
 		break;
 	}
 
@@ -146,63 +148,66 @@ static int ast_device_config_init(struct ast_device *ast)
 	 */
 
 	if (pdev->revision >= 0x50) {
-		ast->chip = AST2600;
-		drm_info(dev, "AST 2600 detected\n");
+		chip = AST2600;
+		dev_info(dev, "AST 2600 detected\n");
 	} else if (pdev->revision >= 0x40) {
 		switch (scu_rev & 0x300) {
 		case 0x0100:
-			ast->chip = AST2510;
-			drm_info(dev, "AST 2510 detected\n");
+			chip = AST2510;
+			dev_info(dev, "AST 2510 detected\n");
 			break;
 		default:
-			ast->chip = AST2500;
-			drm_info(dev, "AST 2500 detected\n");
+			chip = AST2500;
+			dev_info(dev, "AST 2500 detected\n");
 		}
 	} else if (pdev->revision >= 0x30) {
 		switch (scu_rev & 0x300) {
 		case 0x0100:
-			ast->chip = AST1400;
-			drm_info(dev, "AST 1400 detected\n");
+			chip = AST1400;
+			dev_info(dev, "AST 1400 detected\n");
 			break;
 		default:
-			ast->chip = AST2400;
-			drm_info(dev, "AST 2400 detected\n");
+			chip = AST2400;
+			dev_info(dev, "AST 2400 detected\n");
 		}
 	} else if (pdev->revision >= 0x20) {
 		switch (scu_rev & 0x300) {
 		case 0x0000:
-			ast->chip = AST1300;
-			drm_info(dev, "AST 1300 detected\n");
+			chip = AST1300;
+			dev_info(dev, "AST 1300 detected\n");
 			break;
 		default:
-			ast->chip = AST2300;
-			drm_info(dev, "AST 2300 detected\n");
+			chip = AST2300;
+			dev_info(dev, "AST 2300 detected\n");
 			break;
 		}
 	} else if (pdev->revision >= 0x10) {
 		switch (scu_rev & 0x0300) {
 		case 0x0200:
-			ast->chip = AST1100;
-			drm_info(dev, "AST 1100 detected\n");
+			chip = AST1100;
+			dev_info(dev, "AST 1100 detected\n");
 			break;
 		case 0x0100:
-			ast->chip = AST2200;
-			drm_info(dev, "AST 2200 detected\n");
+			chip = AST2200;
+			dev_info(dev, "AST 2200 detected\n");
 			break;
 		case 0x0000:
-			ast->chip = AST2150;
-			drm_info(dev, "AST 2150 detected\n");
+			chip = AST2150;
+			dev_info(dev, "AST 2150 detected\n");
 			break;
 		default:
-			ast->chip = AST2100;
-			drm_info(dev, "AST 2100 detected\n");
+			chip = AST2100;
+			dev_info(dev, "AST 2100 detected\n");
 			break;
 		}
 	} else {
-		ast->chip = AST2000;
-		drm_info(dev, "AST 2000 detected\n");
+		chip = AST2000;
+		dev_info(dev, "AST 2000 detected\n");
 	}
 
+	*chip_out = chip;
+	*config_mode_out = config_mode;
+
 	return 0;
 }
 
@@ -431,6 +436,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	int ret = 0;
 	void __iomem *regs;
 	void __iomem *ioregs;
+	enum ast_config_mode config_mode;
+	enum ast_chip chip;
 
 	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
 	if (IS_ERR(ast))
@@ -502,10 +509,13 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 
-	ret = ast_device_config_init(ast);
+	ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
 	if (ret)
 		return ERR_PTR(ret);
 
+	ast->chip = chip;
+	ast->config_mode = config_mode;
+
 	ast_detect_widescreen(ast);
 	ast_detect_tx_chip(ast, need_post);
 
-- 
2.42.0

[PATCH 10/10] drm/ast: Move detection code into PCI probe helper

[Thomas Zimmermann]

Detect device type and config mode in the PCI probe helper, but leave
DRM device initialization where it is. Structures the driver probe and
setup code into a detection and an initialization phase.

A later patch can add branching to th device-initialization code. Each
chip type can have it own initializer function, if necessary.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c  | 261 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/ast/ast_drv.h  |  10 +-
 drivers/gpu/drm/ast/ast_main.c | 268 ++-------------------------------
 3 files changed, 272 insertions(+), 267 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index cf5b754f044c7..33d45752ff074 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -89,11 +89,192 @@ static const struct pci_device_id ast_pciidlist[] = {
 
 MODULE_DEVICE_TABLE(pci, ast_pciidlist);
 
+static bool ast_is_vga_enabled(void __iomem *ioregs)
+{
+	u8 vgaer = __ast_read8(ioregs, AST_IO_VGAER);
+
+	return vgaer & AST_IO_VGAER_VGA_ENABLE;
+}
+
+static void ast_enable_vga(void __iomem *ioregs)
+{
+	__ast_write8(ioregs, AST_IO_VGAER, AST_IO_VGAER_VGA_ENABLE);
+	__ast_write8(ioregs, AST_IO_VGAMR_W, AST_IO_VGAMR_IOSEL);
+}
+
+/*
+ * Run this function as part of the HW device cleanup; not
+ * when the DRM device gets released.
+ */
+static void ast_enable_mmio_release(void *data)
+{
+	void __iomem *ioregs = (void __force __iomem *)data;
+
+	/* enable standard VGA decode */
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1, AST_IO_VGACRA1_MMIO_ENABLED);
+}
+
+static int ast_enable_mmio(struct device *dev, void __iomem *ioregs)
+{
+	void *data = (void __force *)ioregs;
+
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1,
+		       AST_IO_VGACRA1_MMIO_ENABLED |
+		       AST_IO_VGACRA1_VGAIO_DISABLED);
+
+	return devm_add_action_or_reset(dev, ast_enable_mmio_release, data);
+}
+
+static void ast_open_key(void __iomem *ioregs)
+{
+	__ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, AST_IO_VGACR80_PASSWORD);
+}
+
+static int ast_detect_chip(struct pci_dev *pdev,
+			   void __iomem *regs, void __iomem *ioregs,
+			   enum ast_chip *chip_out,
+			   enum ast_config_mode *config_mode_out)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	enum ast_config_mode config_mode = ast_use_defaults;
+	uint32_t scu_rev = 0xffffffff;
+	enum ast_chip chip;
+	u32 data;
+	u8 vgacrd0, vgacrd1;
+
+	/*
+	 * Find configuration mode and read SCU revision
+	 */
+
+	/* Check if we have device-tree properties */
+	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
+		/* We do, disable P2A access */
+		config_mode = ast_use_dt;
+		scu_rev = data;
+	} else if (pdev->device == PCI_CHIP_AST2000) { // Not all families have a P2A bridge
+		/*
+		 * The BMC will set SCU 0x40 D[12] to 1 if the P2 bridge
+		 * is disabled. We force using P2A if VGA only mode bit
+		 * is set D[7]
+		 */
+		vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
+		vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
+		if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
+
+			/*
+			 * We have a P2A bridge and it is enabled.
+			 */
+
+			/* Patch AST2500/AST2510 */
+			if ((pdev->revision & 0xf0) == 0x40) {
+				if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
+					ast_patch_ahb_2500(regs);
+			}
+
+			/* Double check that it's actually working */
+			data = __ast_read32(regs, 0xf004);
+			if ((data != 0xffffffff) && (data != 0x00)) {
+				config_mode = ast_use_p2a;
+
+				/* Read SCU7c (silicon revision register) */
+				__ast_write32(regs, 0xf004, 0x1e6e0000);
+				__ast_write32(regs, 0xf000, 0x1);
+				scu_rev = __ast_read32(regs, 0x1207c);
+			}
+		}
+	}
+
+	switch (config_mode) {
+	case ast_use_defaults:
+		dev_info(dev, "Using default configuration\n");
+		break;
+	case ast_use_dt:
+		dev_info(dev, "Using device-tree for configuration\n");
+		break;
+	case ast_use_p2a:
+		dev_info(dev, "Using P2A bridge for configuration\n");
+		break;
+	}
+
+	/*
+	 * Identify chipset
+	 */
+
+	if (pdev->revision >= 0x50) {
+		chip = AST2600;
+		dev_info(dev, "AST 2600 detected\n");
+	} else if (pdev->revision >= 0x40) {
+		switch (scu_rev & 0x300) {
+		case 0x0100:
+			chip = AST2510;
+			dev_info(dev, "AST 2510 detected\n");
+			break;
+		default:
+			chip = AST2500;
+			dev_info(dev, "AST 2500 detected\n");
+		}
+	} else if (pdev->revision >= 0x30) {
+		switch (scu_rev & 0x300) {
+		case 0x0100:
+			chip = AST1400;
+			dev_info(dev, "AST 1400 detected\n");
+			break;
+		default:
+			chip = AST2400;
+			dev_info(dev, "AST 2400 detected\n");
+		}
+	} else if (pdev->revision >= 0x20) {
+		switch (scu_rev & 0x300) {
+		case 0x0000:
+			chip = AST1300;
+			dev_info(dev, "AST 1300 detected\n");
+			break;
+		default:
+			chip = AST2300;
+			dev_info(dev, "AST 2300 detected\n");
+			break;
+		}
+	} else if (pdev->revision >= 0x10) {
+		switch (scu_rev & 0x0300) {
+		case 0x0200:
+			chip = AST1100;
+			dev_info(dev, "AST 1100 detected\n");
+			break;
+		case 0x0100:
+			chip = AST2200;
+			dev_info(dev, "AST 2200 detected\n");
+			break;
+		case 0x0000:
+			chip = AST2150;
+			dev_info(dev, "AST 2150 detected\n");
+			break;
+		default:
+			chip = AST2100;
+			dev_info(dev, "AST 2100 detected\n");
+			break;
+		}
+	} else {
+		chip = AST2000;
+		dev_info(dev, "AST 2000 detected\n");
+	}
+
+	*chip_out = chip;
+	*config_mode_out = config_mode;
+
+	return 0;
+}
+
 static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
-	struct ast_device *ast;
-	struct drm_device *dev;
+	struct device *dev = &pdev->dev;
 	int ret;
+	void __iomem *regs;
+	void __iomem *ioregs;
+	enum ast_config_mode config_mode;
+	enum ast_chip chip;
+	struct drm_device *drm;
+	bool need_post = false;
 
 	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
 	if (ret)
@@ -103,16 +284,80 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	ast = ast_device_create(&ast_driver, pdev, ent->driver_data);
-	if (IS_ERR(ast))
-		return PTR_ERR(ast);
-	dev = &ast->base;
+	regs = pcim_iomap(pdev, 1, 0);
+	if (!regs)
+		return -EIO;
+
+	if (pdev->revision >= 0x40) {
+		/*
+		 * On AST2500 and later models, MMIO is enabled by
+		 * default. Adopt it to be compatible with ARM.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 1);
+
+		if (len < AST_IO_MM_OFFSET)
+			return -EIO;
+		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
+			return -EIO;
+		ioregs = regs + AST_IO_MM_OFFSET;
+	} else if (pci_resource_flags(pdev, 2) & IORESOURCE_IO) {
+		/*
+		 * Map I/O registers if we have a PCI BAR for I/O.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 2);
+
+		if (len < AST_IO_MM_OFFSET)
+			return -EIO;
+		ioregs = pcim_iomap(pdev, 2, 0);
+		if (!ioregs)
+			return -EIO;
+	} else {
+		/*
+		 * Anything else is best effort.
+		 */
+		resource_size_t len = pci_resource_len(pdev, 1);
+
+		if (len < AST_IO_MM_OFFSET)
+			return -EIO;
+		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
+			return -EIO;
+		ioregs = regs + AST_IO_MM_OFFSET;
+
+		dev_info(dev, "Platform has no I/O space, using MMIO\n");
+	}
+
+	if (!ast_is_vga_enabled(ioregs)) {
+		dev_info(dev, "VGA not enabled on entry, requesting chip POST\n");
+		need_post = true;
+	}
+
+	/*
+	 * If VGA isn't enabled, we need to enable now or subsequent
+	 * access to the scratch registers will fail.
+	 */
+	if (need_post)
+		ast_enable_vga(ioregs);
+	/* Enable extended register access */
+	ast_open_key(ioregs);
+
+	ret = ast_enable_mmio(dev, ioregs);
+	if (ret)
+		return ret;
+
+	ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
+	if (ret)
+		return ret;
+
+	drm = ast_device_create(pdev, &ast_driver, chip, config_mode, regs, ioregs, need_post);
+	if (IS_ERR(drm))
+		return PTR_ERR(drm);
+	pci_set_drvdata(pdev, drm);
 
-	ret = drm_dev_register(dev, ent->driver_data);
+	ret = drm_dev_register(drm, ent->driver_data);
 	if (ret)
 		return ret;
 
-	drm_fbdev_generic_setup(dev, 32);
+	drm_fbdev_generic_setup(drm, 32);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index b0c899f2ecfd7..c70de7b224c5a 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -243,9 +243,13 @@ static inline struct ast_device *to_ast_device(struct drm_device *dev)
 	return container_of(dev, struct ast_device, base);
 }
 
-struct ast_device *ast_device_create(const struct drm_driver *drv,
-				     struct pci_dev *pdev,
-				     unsigned long flags);
+struct drm_device *ast_device_create(struct pci_dev *pdev,
+				     const struct drm_driver *drv,
+				     enum ast_chip chip,
+				     enum ast_config_mode config_mode,
+				     void __iomem *regs,
+				     void __iomem *ioregs,
+				     bool need_post);
 
 static inline unsigned long __ast_gen(struct ast_device *ast)
 {
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 331a9a861153b..2f3ad5f949fcb 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,182 +35,6 @@
 
 #include "ast_drv.h"
 
-static bool ast_is_vga_enabled(void __iomem *ioregs)
-{
-	u8 vgaer = __ast_read8(ioregs, AST_IO_VGAER);
-
-	return vgaer & AST_IO_VGAER_VGA_ENABLE;
-}
-
-static void ast_enable_vga(void __iomem *ioregs)
-{
-	__ast_write8(ioregs, AST_IO_VGAER, AST_IO_VGAER_VGA_ENABLE);
-	__ast_write8(ioregs, AST_IO_VGAMR_W, AST_IO_VGAMR_IOSEL);
-}
-
-/*
- * Run this function as part of the HW device cleanup; not
- * when the DRM device gets released.
- */
-static void ast_enable_mmio_release(void *data)
-{
-	void __iomem *ioregs = (void __force __iomem *)data;
-
-	/* enable standard VGA decode */
-	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1, AST_IO_VGACRA1_MMIO_ENABLED);
-}
-
-static int ast_enable_mmio(struct device *dev, void __iomem *ioregs)
-{
-	void *data = (void __force *)ioregs;
-
-	__ast_write8_i(ioregs, AST_IO_VGACRI, 0xa1,
-		       AST_IO_VGACRA1_MMIO_ENABLED |
-		       AST_IO_VGACRA1_VGAIO_DISABLED);
-
-	return devm_add_action_or_reset(dev, ast_enable_mmio_release, data);
-}
-
-static void ast_open_key(void __iomem *ioregs)
-{
-	__ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, AST_IO_VGACR80_PASSWORD);
-}
-
-static int ast_detect_chip(struct pci_dev *pdev,
-			   void __iomem *regs, void __iomem *ioregs,
-			   enum ast_chip *chip_out,
-			   enum ast_config_mode *config_mode_out)
-{
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	enum ast_config_mode config_mode = ast_use_defaults;
-	uint32_t scu_rev = 0xffffffff;
-	enum ast_chip chip;
-	u32 data;
-	u8 vgacrd0, vgacrd1;
-
-	/*
-	 * Find configuration mode and read SCU revision
-	 */
-
-	/* Check if we have device-tree properties */
-	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
-		/* We do, disable P2A access */
-		config_mode = ast_use_dt;
-		scu_rev = data;
-	} else if (pdev->device == PCI_CHIP_AST2000) { // Not all families have a P2A bridge
-		/*
-		 * The BMC will set SCU 0x40 D[12] to 1 if the P2 bridge
-		 * is disabled. We force using P2A if VGA only mode bit
-		 * is set D[7]
-		 */
-		vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
-		vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
-		if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
-
-			/*
-			 * We have a P2A bridge and it is enabled.
-			 */
-
-			/* Patch AST2500/AST2510 */
-			if ((pdev->revision & 0xf0) == 0x40) {
-				if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
-					ast_patch_ahb_2500(regs);
-			}
-
-			/* Double check that it's actually working */
-			data = __ast_read32(regs, 0xf004);
-			if ((data != 0xffffffff) && (data != 0x00)) {
-				config_mode = ast_use_p2a;
-
-				/* Read SCU7c (silicon revision register) */
-				__ast_write32(regs, 0xf004, 0x1e6e0000);
-				__ast_write32(regs, 0xf000, 0x1);
-				scu_rev = __ast_read32(regs, 0x1207c);
-			}
-		}
-	}
-
-	switch (config_mode) {
-	case ast_use_defaults:
-		dev_info(dev, "Using default configuration\n");
-		break;
-	case ast_use_dt:
-		dev_info(dev, "Using device-tree for configuration\n");
-		break;
-	case ast_use_p2a:
-		dev_info(dev, "Using P2A bridge for configuration\n");
-		break;
-	}
-
-	/*
-	 * Identify chipset
-	 */
-
-	if (pdev->revision >= 0x50) {
-		chip = AST2600;
-		dev_info(dev, "AST 2600 detected\n");
-	} else if (pdev->revision >= 0x40) {
-		switch (scu_rev & 0x300) {
-		case 0x0100:
-			chip = AST2510;
-			dev_info(dev, "AST 2510 detected\n");
-			break;
-		default:
-			chip = AST2500;
-			dev_info(dev, "AST 2500 detected\n");
-		}
-	} else if (pdev->revision >= 0x30) {
-		switch (scu_rev & 0x300) {
-		case 0x0100:
-			chip = AST1400;
-			dev_info(dev, "AST 1400 detected\n");
-			break;
-		default:
-			chip = AST2400;
-			dev_info(dev, "AST 2400 detected\n");
-		}
-	} else if (pdev->revision >= 0x20) {
-		switch (scu_rev & 0x300) {
-		case 0x0000:
-			chip = AST1300;
-			dev_info(dev, "AST 1300 detected\n");
-			break;
-		default:
-			chip = AST2300;
-			dev_info(dev, "AST 2300 detected\n");
-			break;
-		}
-	} else if (pdev->revision >= 0x10) {
-		switch (scu_rev & 0x0300) {
-		case 0x0200:
-			chip = AST1100;
-			dev_info(dev, "AST 1100 detected\n");
-			break;
-		case 0x0100:
-			chip = AST2200;
-			dev_info(dev, "AST 2200 detected\n");
-			break;
-		case 0x0000:
-			chip = AST2150;
-			dev_info(dev, "AST 2150 detected\n");
-			break;
-		default:
-			chip = AST2100;
-			dev_info(dev, "AST 2100 detected\n");
-			break;
-		}
-	} else {
-		chip = AST2000;
-		dev_info(dev, "AST 2000 detected\n");
-	}
-
-	*chip_out = chip;
-	*config_mode_out = config_mode;
-
-	return 0;
-}
-
 static void ast_detect_widescreen(struct ast_device *ast)
 {
 	u8 jreg;
@@ -426,95 +250,27 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
-struct ast_device *ast_device_create(const struct drm_driver *drv,
-				     struct pci_dev *pdev,
-				     unsigned long flags)
+struct drm_device *ast_device_create(struct pci_dev *pdev,
+				     const struct drm_driver *drv,
+				     enum ast_chip chip,
+				     enum ast_config_mode config_mode,
+				     void __iomem *regs,
+				     void __iomem *ioregs,
+				     bool need_post)
 {
 	struct drm_device *dev;
 	struct ast_device *ast;
-	bool need_post = false;
-	int ret = 0;
-	void __iomem *regs;
-	void __iomem *ioregs;
-	enum ast_config_mode config_mode;
-	enum ast_chip chip;
+	int ret;
 
 	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
 	if (IS_ERR(ast))
-		return ast;
+		return ERR_CAST(ast);
 	dev = &ast->base;
 
-	pci_set_drvdata(pdev, dev);
-
-	regs = pcim_iomap(pdev, 1, 0);
-	if (!regs)
-		return ERR_PTR(-EIO);
-
-	if (pdev->revision >= 0x40) {
-		/*
-		 * On AST2500 and later models, MMIO is enabled by
-		 * default. Adopt it to be compatible with ARM.
-		 */
-		resource_size_t len = pci_resource_len(pdev, 1);
-
-		if (len < AST_IO_MM_OFFSET)
-			return ERR_PTR(-EIO);
-		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
-			return ERR_PTR(-EIO);
-		ioregs = regs + AST_IO_MM_OFFSET;
-	} else if (pci_resource_flags(pdev, 2) & IORESOURCE_IO) {
-		/*
-		 * Map I/O registers if we have a PCI BAR for I/O.
-		 */
-		resource_size_t len = pci_resource_len(pdev, 2);
-
-		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
-			return ERR_PTR(-EIO);
-		ioregs = pcim_iomap(pdev, 2, 0);
-		if (!ioregs)
-			return ERR_PTR(-EIO);
-	} else {
-		/*
-		 * Anything else is best effort.
-		 */
-		resource_size_t len = pci_resource_len(pdev, 1);
-
-		if (len < AST_IO_MM_OFFSET)
-			return ERR_PTR(-EIO);
-		if ((len - AST_IO_MM_OFFSET) < AST_IO_MM_LENGTH)
-			return ERR_PTR(-EIO);
-		ioregs = regs + AST_IO_MM_OFFSET;
-
-		drm_info(dev, "Platform has no I/O space, using MMIO\n");
-	}
-
-	ast->regs = regs;
-	ast->ioregs = ioregs;
-
-	if (!ast_is_vga_enabled(ioregs)) {
-		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
-		need_post = true;
-	}
-
-	/*
-	 * If VGA isn't enabled, we need to enable now or subsequent
-	 * access to the scratch registers will fail.
-	 */
-	if (need_post)
-		ast_enable_vga(ioregs);
-	/* Enable extended register access */
-	ast_open_key(ioregs);
-
-	ret = ast_enable_mmio(&pdev->dev, ioregs);
-	if (ret)
-		return ERR_PTR(ret);
-
-	ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
-	if (ret)
-		return ERR_PTR(ret);
-
 	ast->chip = chip;
 	ast->config_mode = config_mode;
+	ast->regs = regs;
+	ast->ioregs = ioregs;
 
 	ast_detect_widescreen(ast);
 	ast_detect_tx_chip(ast, need_post);
@@ -545,5 +301,5 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return ast;
+	return dev;
 }
-- 
2.42.0

Re: [PATCH 09/10] drm/ast: Detect ast device type and config mode without ast device

[Jocelyn Falempe]

On 13/11/2023 09:50, Thomas Zimmermann wrote:
> Return the ast chip and config in the detection function's parameters
> instead of storing them directly in the ast device instance.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 104 ++++++++++++++++++---------------
>   1 file changed, 57 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index f100df8d74f71..331a9a861153b 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -76,25 +76,27 @@ static void ast_open_key(void __iomem *ioregs)
>   	__ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, AST_IO_VGACR80_PASSWORD);
>   }
>   
> -static int ast_device_config_init(struct ast_device *ast)
> +static int ast_detect_chip(struct pci_dev *pdev,
> +			   void __iomem *regs, void __iomem *ioregs,
> +			   enum ast_chip *chip_out,
> +			   enum ast_config_mode *config_mode_out)
>   {
> -	struct drm_device *dev = &ast->base;
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	struct device_node *np = dev->dev->of_node;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	enum ast_config_mode config_mode = ast_use_defaults;
>   	uint32_t scu_rev = 0xffffffff;
> +	enum ast_chip chip;
>   	u32 data;
> -	u8 jregd0, jregd1;
> +	u8 vgacrd0, vgacrd1;
>   
>   	/*
>   	 * Find configuration mode and read SCU revision
>   	 */
>   
> -	ast->config_mode = ast_use_defaults;
> -
>   	/* Check if we have device-tree properties */
>   	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
>   		/* We do, disable P2A access */
> -		ast->config_mode = ast_use_dt;
> +		config_mode = ast_use_dt;
>   		scu_rev = data;
>   	} else if (pdev->device == PCI_CHIP_AST2000) { // Not all families have a P2A bridge
>   		/*
> @@ -102,9 +104,9 @@ static int ast_device_config_init(struct ast_device *ast)
>   		 * is disabled. We force using P2A if VGA only mode bit
>   		 * is set D[7]
>   		 */
> -		jregd0 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd0, 0xff);
> -		jregd1 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
> -		if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
> +		vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
> +		vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
> +		if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
>   
>   			/*
>   			 * We have a P2A bridge and it is enabled.
> @@ -112,32 +114,32 @@ static int ast_device_config_init(struct ast_device *ast)
>   
>   			/* Patch AST2500/AST2510 */
>   			if ((pdev->revision & 0xf0) == 0x40) {
> -				if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
> -					ast_patch_ahb_2500(ast->regs);
> +				if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
> +					ast_patch_ahb_2500(regs);
>   			}
>   
>   			/* Double check that it's actually working */
> -			data = ast_read32(ast, 0xf004);
> +			data = __ast_read32(regs, 0xf004);
>   			if ((data != 0xffffffff) && (data != 0x00)) {
> -				ast->config_mode = ast_use_p2a;
> +				config_mode = ast_use_p2a;
>   
>   				/* Read SCU7c (silicon revision register) */
> -				ast_write32(ast, 0xf004, 0x1e6e0000);
> -				ast_write32(ast, 0xf000, 0x1);
> -				scu_rev = ast_read32(ast, 0x1207c);
> +				__ast_write32(regs, 0xf004, 0x1e6e0000);
> +				__ast_write32(regs, 0xf000, 0x1);
> +				scu_rev = __ast_read32(regs, 0x1207c);
>   			}
>   		}
>   	}
>   
> -	switch (ast->config_mode) {
> +	switch (config_mode) {
>   	case ast_use_defaults:
> -		drm_info(dev, "Using default configuration\n");
> +		dev_info(dev, "Using default configuration\n");
>   		break;
>   	case ast_use_dt:
> -		drm_info(dev, "Using device-tree for configuration\n");
> +		dev_info(dev, "Using device-tree for configuration\n");
>   		break;
>   	case ast_use_p2a:
> -		drm_info(dev, "Using P2A bridge for configuration\n");
> +		dev_info(dev, "Using P2A bridge for configuration\n");
>   		break;
>   	}
>   
> @@ -146,63 +148,66 @@ static int ast_device_config_init(struct ast_device *ast)
>   	 */
>   
>   	if (pdev->revision >= 0x50) {
> -		ast->chip = AST2600;
> -		drm_info(dev, "AST 2600 detected\n");
> +		chip = AST2600;
> +		dev_info(dev, "AST 2600 detected\n");

Adding a macro to set chip and printk could be handy here:

something like

#define set_chip(version) \
	chip = version; \
	dev_info(dev, "%s detected\n", #version); \


and then set_chip(AST2510)


>   	} else if (pdev->revision >= 0x40) {
>   		switch (scu_rev & 0x300) {
>   		case 0x0100:
> -			ast->chip = AST2510;
> -			drm_info(dev, "AST 2510 detected\n");
> +			chip = AST2510;
> +			dev_info(dev, "AST 2510 detected\n");
>   			break;
>   		default:
> -			ast->chip = AST2500;
> -			drm_info(dev, "AST 2500 detected\n");
> +			chip = AST2500;
> +			dev_info(dev, "AST 2500 detected\n");

Should the default case have break ?
This one has no break, but later in this function they do. Maybe we can 
have more consistency ?


>   		}
>   	} else if (pdev->revision >= 0x30) {
>   		switch (scu_rev & 0x300) {
>   		case 0x0100:
> -			ast->chip = AST1400;
> -			drm_info(dev, "AST 1400 detected\n");
> +			chip = AST1400;
> +			dev_info(dev, "AST 1400 detected\n");
>   			break;
>   		default:
> -			ast->chip = AST2400;
> -			drm_info(dev, "AST 2400 detected\n");
> +			chip = AST2400;
> +			dev_info(dev, "AST 2400 detected\n");
>   		}
>   	} else if (pdev->revision >= 0x20) {
>   		switch (scu_rev & 0x300) {
>   		case 0x0000:
> -			ast->chip = AST1300;
> -			drm_info(dev, "AST 1300 detected\n");
> +			chip = AST1300;
> +			dev_info(dev, "AST 1300 detected\n");
>   			break;
>   		default:
> -			ast->chip = AST2300;
> -			drm_info(dev, "AST 2300 detected\n");
> +			chip = AST2300;
> +			dev_info(dev, "AST 2300 detected\n");
>   			break;
>   		}
>   	} else if (pdev->revision >= 0x10) {
>   		switch (scu_rev & 0x0300) {
>   		case 0x0200:
> -			ast->chip = AST1100;
> -			drm_info(dev, "AST 1100 detected\n");
> +			chip = AST1100;
> +			dev_info(dev, "AST 1100 detected\n");
>   			break;
>   		case 0x0100:
> -			ast->chip = AST2200;
> -			drm_info(dev, "AST 2200 detected\n");
> +			chip = AST2200;
> +			dev_info(dev, "AST 2200 detected\n");
>   			break;
>   		case 0x0000:
> -			ast->chip = AST2150;
> -			drm_info(dev, "AST 2150 detected\n");
> +			chip = AST2150;
> +			dev_info(dev, "AST 2150 detected\n");
>   			break;
>   		default:
> -			ast->chip = AST2100;
> -			drm_info(dev, "AST 2100 detected\n");
> +			chip = AST2100;
> +			dev_info(dev, "AST 2100 detected\n");
>   			break;
>   		}
>   	} else {
> -		ast->chip = AST2000;
> -		drm_info(dev, "AST 2000 detected\n");
> +		chip = AST2000;
> +		dev_info(dev, "AST 2000 detected\n");
>   	}
>   
> +	*chip_out = chip;
> +	*config_mode_out = config_mode;
> +
>   	return 0;
>   }
>   
> @@ -431,6 +436,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	int ret = 0;
>   	void __iomem *regs;
>   	void __iomem *ioregs;
> +	enum ast_config_mode config_mode;
> +	enum ast_chip chip;
>   
>   	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
>   	if (IS_ERR(ast))
> @@ -502,10 +509,13 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	ret = ast_device_config_init(ast);
> +	ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> +	ast->chip = chip;
> +	ast->config_mode = config_mode;
> +
>   	ast_detect_widescreen(ast);
>   	ast_detect_tx_chip(ast, need_post);
>   

Thanks for your patch,

Best regards,

-- 

Jocelyn

Re: [PATCH 00/10] drm/ast: Detect device type before init

[Jocelyn Falempe]

On 13/11/2023 09:50, Thomas Zimmermann wrote:
> Detecting the ast device's chipset type and configuration mode
> involves several registers, DT properties and possibly POSTing
> parts of the chip. It is preferable to do this before initializing
> the DRM driver, so that that each chip type can have an individual
> setup code.
> 
> The patchset addresses the problem by moving all early detection
> code before the allocation of the ast device.
> 
> Patch one gets a lock out of the way. The lock is only relevant
> for mode setting. Move it there.
> 
> Patches 2 and 3 rework the detection of the correct I/O memory
> ranges. It is now self-contained, more readable and works without
> an instance of struct ast_device.
> 
> Patches 4 to 7 rework the setup of various registers that are
> required for detection. Access helpers for I/O can now operate
> without an instance of struct ast_device. The setup functions
> operate on the I/O ranges that have been made available with
> patch 3, but again without struct ast_device.
> 
> With the detection's internals done, patches 8 and 9 rework the
> chip's and config-mode's detection code to operate without struct
> ast_device as well.
> 
> Finally, patch 10 moves the detection code into the PCI probe
> function. it runs before any of the DRM device code. The fucntion
> for creating an ast device, ast_device_create(), receives the
> detected I/O memory ranges, chip type and configuration mode.
> 
> This cleans up the detection code. There is more chip-specific
> code in other parts of the driver. In a later patch, the ast device
> setup can be split up so that each chip type gets its own code
> path that does not interfere with other chips.
> 
> Tested on AST1100 and AST2100.
> 
> Thomas Zimmermann (10):
>    drm/ast: Turn ioregs_lock to modeset_lock
>    drm/ast: Rework I/O register setup
>    drm/ast: Retrieve I/O-memory ranges without ast device
>    drm/ast: Add I/O helpers without ast device
>    drm/ast: Enable VGA without ast device instance
>    drm/ast: Enable MMIO without ast device instance
>    drm/ast: Partially implement POST without ast device instance
>    drm/ast: Add enum ast_config_mode
>    drm/ast: Detect ast device type and config mode without ast device
>    drm/ast: Move detection code into PCI probe helper
> 
>   drivers/gpu/drm/ast/ast_drv.c  | 261 ++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/ast/ast_drv.h  | 101 +++++++++----
>   drivers/gpu/drm/ast/ast_main.c | 244 ++----------------------------
>   drivers/gpu/drm/ast/ast_mode.c |  26 ++--
>   drivers/gpu/drm/ast/ast_post.c |  73 +++++----
>   drivers/gpu/drm/ast/ast_reg.h  |  12 +-
>   6 files changed, 411 insertions(+), 306 deletions(-)
> 
> 
> base-commit: b7816c393496dc4497c1327310821407f7171d8b
> prerequisite-patch-id: 0aa359f6144c4015c140c8a6750be19099c676fb
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: cbc453ee02fae02af22fbfdce56ab732c7a88c36

I've reviewed the whole series, and I have only a minor comment on patch 
9. That's a good thing to move the chip detection to its own functions, 
and will allow further refactoring later.


For the whole series:
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

-- 

Jocelyn

Re: [PATCH 09/10] drm/ast: Detect ast device type and config mode without ast device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 11239 bytes --]

Hi

Am 13.11.23 um 16:25 schrieb Jocelyn Falempe:
> On 13/11/2023 09:50, Thomas Zimmermann wrote:
>> Return the ast chip and config in the detection function's parameters
>> instead of storing them directly in the ast device instance.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_main.c | 104 ++++++++++++++++++---------------
>>   1 file changed, 57 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index f100df8d74f71..331a9a861153b 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -76,25 +76,27 @@ static void ast_open_key(void __iomem *ioregs)
>>       __ast_write8_i(ioregs, AST_IO_VGACRI, 0x80, 
>> AST_IO_VGACR80_PASSWORD);
>>   }
>> -static int ast_device_config_init(struct ast_device *ast)
>> +static int ast_detect_chip(struct pci_dev *pdev,
>> +               void __iomem *regs, void __iomem *ioregs,
>> +               enum ast_chip *chip_out,
>> +               enum ast_config_mode *config_mode_out)
>>   {
>> -    struct drm_device *dev = &ast->base;
>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>> -    struct device_node *np = dev->dev->of_node;
>> +    struct device *dev = &pdev->dev;
>> +    struct device_node *np = dev->of_node;
>> +    enum ast_config_mode config_mode = ast_use_defaults;
>>       uint32_t scu_rev = 0xffffffff;
>> +    enum ast_chip chip;
>>       u32 data;
>> -    u8 jregd0, jregd1;
>> +    u8 vgacrd0, vgacrd1;
>>       /*
>>        * Find configuration mode and read SCU revision
>>        */
>> -    ast->config_mode = ast_use_defaults;
>> -
>>       /* Check if we have device-tree properties */
>>       if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", 
>> &data)) {
>>           /* We do, disable P2A access */
>> -        ast->config_mode = ast_use_dt;
>> +        config_mode = ast_use_dt;
>>           scu_rev = data;
>>       } else if (pdev->device == PCI_CHIP_AST2000) { // Not all 
>> families have a P2A bridge
>>           /*
>> @@ -102,9 +104,9 @@ static int ast_device_config_init(struct 
>> ast_device *ast)
>>            * is disabled. We force using P2A if VGA only mode bit
>>            * is set D[7]
>>            */
>> -        jregd0 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd0, 0xff);
>> -        jregd1 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
>> -        if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>> +        vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
>> +        vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
>> +        if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
>>               /*
>>                * We have a P2A bridge and it is enabled.
>> @@ -112,32 +114,32 @@ static int ast_device_config_init(struct 
>> ast_device *ast)
>>               /* Patch AST2500/AST2510 */
>>               if ((pdev->revision & 0xf0) == 0x40) {
>> -                if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
>> -                    ast_patch_ahb_2500(ast->regs);
>> +                if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
>> +                    ast_patch_ahb_2500(regs);
>>               }
>>               /* Double check that it's actually working */
>> -            data = ast_read32(ast, 0xf004);
>> +            data = __ast_read32(regs, 0xf004);
>>               if ((data != 0xffffffff) && (data != 0x00)) {
>> -                ast->config_mode = ast_use_p2a;
>> +                config_mode = ast_use_p2a;
>>                   /* Read SCU7c (silicon revision register) */
>> -                ast_write32(ast, 0xf004, 0x1e6e0000);
>> -                ast_write32(ast, 0xf000, 0x1);
>> -                scu_rev = ast_read32(ast, 0x1207c);
>> +                __ast_write32(regs, 0xf004, 0x1e6e0000);
>> +                __ast_write32(regs, 0xf000, 0x1);
>> +                scu_rev = __ast_read32(regs, 0x1207c);
>>               }
>>           }
>>       }
>> -    switch (ast->config_mode) {
>> +    switch (config_mode) {
>>       case ast_use_defaults:
>> -        drm_info(dev, "Using default configuration\n");
>> +        dev_info(dev, "Using default configuration\n");
>>           break;
>>       case ast_use_dt:
>> -        drm_info(dev, "Using device-tree for configuration\n");
>> +        dev_info(dev, "Using device-tree for configuration\n");
>>           break;
>>       case ast_use_p2a:
>> -        drm_info(dev, "Using P2A bridge for configuration\n");
>> +        dev_info(dev, "Using P2A bridge for configuration\n");
>>           break;
>>       }
>> @@ -146,63 +148,66 @@ static int ast_device_config_init(struct 
>> ast_device *ast)
>>        */
>>       if (pdev->revision >= 0x50) {
>> -        ast->chip = AST2600;
>> -        drm_info(dev, "AST 2600 detected\n");
>> +        chip = AST2600;
>> +        dev_info(dev, "AST 2600 detected\n");
> 
> Adding a macro to set chip and printk could be handy here:
> 
> something like
> 
> #define set_chip(version) \
>      chip = version; \
>      dev_info(dev, "%s detected\n", #version); \
> 
> 
> and then set_chip(AST2510)

I was thinking about reworking these messages at some point. Maybe have 
a single print somewhere in the code.

> 
> 
>>       } else if (pdev->revision >= 0x40) {
>>           switch (scu_rev & 0x300) {
>>           case 0x0100:
>> -            ast->chip = AST2510;
>> -            drm_info(dev, "AST 2510 detected\n");
>> +            chip = AST2510;
>> +            dev_info(dev, "AST 2510 detected\n");
>>               break;
>>           default:
>> -            ast->chip = AST2500;
>> -            drm_info(dev, "AST 2500 detected\n");
>> +            chip = AST2500;
>> +            dev_info(dev, "AST 2500 detected\n");
> 
> Should the default case have break ?
> This one has no break, but later in this function they do. Maybe we can 
> have more consistency ?

Sure, of course.

Best regards
Thomas

> 
> 
>>           }
>>       } else if (pdev->revision >= 0x30) {
>>           switch (scu_rev & 0x300) {
>>           case 0x0100:
>> -            ast->chip = AST1400;
>> -            drm_info(dev, "AST 1400 detected\n");
>> +            chip = AST1400;
>> +            dev_info(dev, "AST 1400 detected\n");
>>               break;
>>           default:
>> -            ast->chip = AST2400;
>> -            drm_info(dev, "AST 2400 detected\n");
>> +            chip = AST2400;
>> +            dev_info(dev, "AST 2400 detected\n");
>>           }
>>       } else if (pdev->revision >= 0x20) {
>>           switch (scu_rev & 0x300) {
>>           case 0x0000:
>> -            ast->chip = AST1300;
>> -            drm_info(dev, "AST 1300 detected\n");
>> +            chip = AST1300;
>> +            dev_info(dev, "AST 1300 detected\n");
>>               break;
>>           default:
>> -            ast->chip = AST2300;
>> -            drm_info(dev, "AST 2300 detected\n");
>> +            chip = AST2300;
>> +            dev_info(dev, "AST 2300 detected\n");
>>               break;
>>           }
>>       } else if (pdev->revision >= 0x10) {
>>           switch (scu_rev & 0x0300) {
>>           case 0x0200:
>> -            ast->chip = AST1100;
>> -            drm_info(dev, "AST 1100 detected\n");
>> +            chip = AST1100;
>> +            dev_info(dev, "AST 1100 detected\n");
>>               break;
>>           case 0x0100:
>> -            ast->chip = AST2200;
>> -            drm_info(dev, "AST 2200 detected\n");
>> +            chip = AST2200;
>> +            dev_info(dev, "AST 2200 detected\n");
>>               break;
>>           case 0x0000:
>> -            ast->chip = AST2150;
>> -            drm_info(dev, "AST 2150 detected\n");
>> +            chip = AST2150;
>> +            dev_info(dev, "AST 2150 detected\n");
>>               break;
>>           default:
>> -            ast->chip = AST2100;
>> -            drm_info(dev, "AST 2100 detected\n");
>> +            chip = AST2100;
>> +            dev_info(dev, "AST 2100 detected\n");
>>               break;
>>           }
>>       } else {
>> -        ast->chip = AST2000;
>> -        drm_info(dev, "AST 2000 detected\n");
>> +        chip = AST2000;
>> +        dev_info(dev, "AST 2000 detected\n");
>>       }
>> +    *chip_out = chip;
>> +    *config_mode_out = config_mode;
>> +
>>       return 0;
>>   }
>> @@ -431,6 +436,8 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>       int ret = 0;
>>       void __iomem *regs;
>>       void __iomem *ioregs;
>> +    enum ast_config_mode config_mode;
>> +    enum ast_chip chip;
>>       ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
>>       if (IS_ERR(ast))
>> @@ -502,10 +509,13 @@ struct ast_device *ast_device_create(const 
>> struct drm_driver *drv,
>>       if (ret)
>>           return ERR_PTR(ret);
>> -    ret = ast_device_config_init(ast);
>> +    ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
>>       if (ret)
>>           return ERR_PTR(ret);
>> +    ast->chip = chip;
>> +    ast->config_mode = config_mode;
>> +
>>       ast_detect_widescreen(ast);
>>       ast_detect_tx_chip(ast, need_post);
> 
> Thanks for your patch,
> 
> Best regards,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]