[PATCH 00/14] drm/ast: Refactor the device-detection code

[PATCH 00/14] drm/ast: Refactor the device-detection code

[Thomas Zimmermann]

Ast's code for detecting the device type and features is convoluted.
It mixes up several state fields, chip types and sub-models. Rework
the driver into somehting more understandable.

Patches 1 fixes a long-standing bug. The affected code has never
worked correctly.

Patches 2 to 8 make various changes to the init code, or remove dead
and duplicated code paths.

Patch 9 introduces chip generations. Until now, ast used the value
of enum ast_chip to represent a certain set of related modes, and
also used the enum to represent individal models. This makes the
driver code hard to understand in certain places. The patch encodes
a chip generation in each model enum and converts the driver to use
it.

Patches 10 to 12 replace duplicated model checks with the correct
enum value. Detection of wide-screen functionality and the transmitter
chip can then be moved into individual functions in patch 13.

Patch 14 merges the detection of the silicon revision and the chip
model into s single function. Both need to be done in the same place
and affect each other.

Tested on AST1100 and AST2300.

Thomas Zimmermann (14):
  drm/ast: Fix DRAM init on AST2200
  drm/ast: Remove vga2_clone field
  drm/ast: Implement register helpers in ast_drv.h
  drm/ast: Remove dead else branch in POST code
  drm/ast: Remove device POSTing and config from chip detection
  drm/ast: Set PCI config before accessing I/O registers
  drm/ast: Enable and unlock device access early during init
  drm/ast: Set up release action right after enabling MMIO
  drm/ast: Distinguish among chip generations
  drm/ast: Detect AST 1300 model
  drm/ast: Detect AST 1400 model
  drm/ast: Detect AST 2510 model
  drm/ast: Move widescreen- and tx-chip detection into separate helpers
  drm/ast: Merge config and chip detection

 drivers/gpu/drm/ast/ast_dp501.c |   6 +-
 drivers/gpu/drm/ast/ast_drv.h   |  97 +++++++---
 drivers/gpu/drm/ast/ast_main.c  | 320 +++++++++++++++++++-------------
 drivers/gpu/drm/ast/ast_mm.c    |   2 -
 drivers/gpu/drm/ast/ast_mode.c  |  35 ++--
 drivers/gpu/drm/ast/ast_post.c  |  74 ++------
 6 files changed, 294 insertions(+), 240 deletions(-)

-- 
2.41.0

[PATCH 01/14] drm/ast: Fix DRAM init on AST2200

[Thomas Zimmermann]

Fix the test for the AST2200 in the DRAM initialization. The value
in ast->chip has to be compared against an enum constant instead of
a numerical value.

This bug got introduced when the driver was first imported into the
kernel.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 312fec1405dd ("drm: Initial KMS driver for AST (ASpeed Technologies) 2000 series (v2)")
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v3.5+
---
 drivers/gpu/drm/ast/ast_post.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index a005aec18a020..0262aaafdb1c5 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -291,7 +291,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
 				;
 			} while (ast_read32(ast, 0x10100) != 0xa8);
 		} else {/* AST2100/1100 */
-			if (ast->chip == AST2100 || ast->chip == 2200)
+			if (ast->chip == AST2100 || ast->chip == AST2200)
 				dram_reg_info = ast2100_dram_table_data;
 			else
 				dram_reg_info = ast1100_dram_table_data;
-- 
2.41.0

Re: [01/14] drm/ast: Fix DRAM init on AST2200

[Sui Jingfeng]

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Fix the test for the AST2200 in the DRAM initialization. The value
> in ast->chip has to be compared against an enum constant instead of
> a numerical value.
>
> This bug got introduced when the driver was first imported into the
> kernel.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>


> Fixes: 312fec1405dd ("drm: Initial KMS driver for AST (ASpeed Technologies) 2000 series (v2)")
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v3.5+
> ---
>   drivers/gpu/drm/ast/ast_post.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index a005aec18a020..0262aaafdb1c5 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -291,7 +291,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   				;
>   			} while (ast_read32(ast, 0x10100) != 0xa8);
>   		} else {/* AST2100/1100 */
> -			if (ast->chip == AST2100 || ast->chip == 2200)
> +			if (ast->chip == AST2100 || ast->chip == AST2200)
>   				dram_reg_info = ast2100_dram_table_data;
>   			else
>   				dram_reg_info = ast1100_dram_table_data;

-- 
Jingfeng

[PATCH 02/14] drm/ast: Remove vga2_clone field

[Thomas Zimmermann]

Remove the unused field vga2_clone from struct ast_device. No
functional changes.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 5498a6676f2e8..fc4760a67596f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -166,7 +166,6 @@ struct ast_device {
 	void __iomem *dp501_fw_buf;
 
 	enum ast_chip chip;
-	bool vga2_clone;
 	uint32_t dram_bus_width;
 	uint32_t dram_type;
 	uint32_t mclk;
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 1f35438f614a7..da33cfc6366ec 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -179,7 +179,6 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
 			drm_info(dev, "AST 2100 detected\n");
 			break;
 		}
-		ast->vga2_clone = false;
 	} else {
 		ast->chip = AST2000;
 		drm_info(dev, "AST 2000 detected\n");
-- 
2.41.0

Re: [02/14] drm/ast: Remove vga2_clone field

[Sui Jingfeng]

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Remove the unused field vga2_clone from struct ast_device. No
> functional changes.

Yeah, it doesn't get used at all.

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  | 1 -
>   drivers/gpu/drm/ast/ast_main.c | 1 -
>   2 files changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 5498a6676f2e8..fc4760a67596f 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -166,7 +166,6 @@ struct ast_device {
>   	void __iomem *dp501_fw_buf;
>   
>   	enum ast_chip chip;
> -	bool vga2_clone;
>   	uint32_t dram_bus_width;
>   	uint32_t dram_type;
>   	uint32_t mclk;
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 1f35438f614a7..da33cfc6366ec 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -179,7 +179,6 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>   			drm_info(dev, "AST 2100 detected\n");
>   			break;
>   		}
> -		ast->vga2_clone = false;
>   	} else {
>   		ast->chip = AST2000;
>   		drm_info(dev, "AST 2000 detected\n");

-- 
Jingfeng

[PATCH 03/14] drm/ast: Implement register helpers in ast_drv.h

[Thomas Zimmermann]

There are already a number of register I/O functions in ast_drv.h.
For consistency, move the remaining functions there as well. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 34 ++++++++++++++++++++++++----------
 drivers/gpu/drm/ast/ast_main.c | 28 ----------------------------
 2 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index fc4760a67596f..0141705beaee9 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -257,22 +257,36 @@ static inline void ast_io_write8(struct ast_device *ast, u32 reg, u8 val)
 	iowrite8(val, ast->ioregs + reg);
 }
 
-static inline void ast_set_index_reg(struct ast_device *ast,
-				     uint32_t base, uint8_t index,
-				     uint8_t 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);
+}
+
+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;
+}
+
+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);
 }
 
-void ast_set_index_reg_mask(struct ast_device *ast,
-			    uint32_t base, uint8_t index,
-			    uint8_t mask, uint8_t val);
-uint8_t ast_get_index_reg(struct ast_device *ast,
-			  uint32_t base, uint8_t index);
-uint8_t ast_get_index_reg_mask(struct ast_device *ast,
-			       uint32_t base, uint8_t index, uint8_t mask);
+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);
+}
 
 static inline void ast_open_key(struct ast_device *ast)
 {
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index da33cfc6366ec..862fdf02f6100 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,34 +35,6 @@
 
 #include "ast_drv.h"
 
-void ast_set_index_reg_mask(struct ast_device *ast,
-			    uint32_t base, uint8_t index,
-			    uint8_t mask, uint8_t val)
-{
-	u8 tmp;
-	ast_io_write8(ast, base, index);
-	tmp = (ast_io_read8(ast, base + 1) & mask) | val;
-	ast_set_index_reg(ast, base, index, tmp);
-}
-
-uint8_t ast_get_index_reg(struct ast_device *ast,
-			  uint32_t base, uint8_t index)
-{
-	uint8_t ret;
-	ast_io_write8(ast, base, index);
-	ret = ast_io_read8(ast, base + 1);
-	return ret;
-}
-
-uint8_t ast_get_index_reg_mask(struct ast_device *ast,
-			       uint32_t base, uint8_t index, uint8_t mask)
-{
-	uint8_t ret;
-	ast_io_write8(ast, base, index);
-	ret = ast_io_read8(ast, base + 1) & mask;
-	return ret;
-}
-
 static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 {
 	struct device_node *np = dev->dev->of_node;
-- 
2.41.0

Re: [03/14] drm/ast: Implement register helpers in ast_drv.h

[Sui Jingfeng]

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> There are already a number of register I/O functions in ast_drv.h.
> For consistency, move the remaining functions there as well. No
> functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  | 34 ++++++++++++++++++++++++----------
>   drivers/gpu/drm/ast/ast_main.c | 28 ----------------------------
>   2 files changed, 24 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index fc4760a67596f..0141705beaee9 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -257,22 +257,36 @@ static inline void ast_io_write8(struct ast_device *ast, u32 reg, u8 val)
>   	iowrite8(val, ast->ioregs + reg);
>   }
>   
> -static inline void ast_set_index_reg(struct ast_device *ast,
> -				     uint32_t base, uint8_t index,
> -				     uint8_t 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);
> +}
> +
> +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;
> +}
> +
> +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);
>   }
>   
> -void ast_set_index_reg_mask(struct ast_device *ast,
> -			    uint32_t base, uint8_t index,
> -			    uint8_t mask, uint8_t val);
> -uint8_t ast_get_index_reg(struct ast_device *ast,
> -			  uint32_t base, uint8_t index);
> -uint8_t ast_get_index_reg_mask(struct ast_device *ast,
> -			       uint32_t base, uint8_t index, uint8_t mask);
> +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);
> +}
>   
>   static inline void ast_open_key(struct ast_device *ast)
>   {
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index da33cfc6366ec..862fdf02f6100 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,34 +35,6 @@
>   
>   #include "ast_drv.h"
>   
> -void ast_set_index_reg_mask(struct ast_device *ast,
> -			    uint32_t base, uint8_t index,
> -			    uint8_t mask, uint8_t val)
> -{
> -	u8 tmp;
> -	ast_io_write8(ast, base, index);
> -	tmp = (ast_io_read8(ast, base + 1) & mask) | val;
> -	ast_set_index_reg(ast, base, index, tmp);
> -}
> -
> -uint8_t ast_get_index_reg(struct ast_device *ast,
> -			  uint32_t base, uint8_t index)
> -{
> -	uint8_t ret;
> -	ast_io_write8(ast, base, index);
> -	ret = ast_io_read8(ast, base + 1);
> -	return ret;
> -}
> -
> -uint8_t ast_get_index_reg_mask(struct ast_device *ast,
> -			       uint32_t base, uint8_t index, uint8_t mask)
> -{
> -	uint8_t ret;
> -	ast_io_write8(ast, base, index);
> -	ret = ast_io_read8(ast, base + 1) & mask;
> -	return ret;
> -}
> -
>   static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>   {
>   	struct device_node *np = dev->dev->of_node;

-- 
Jingfeng

[PATCH 04/14] drm/ast: Remove dead else branch in POST code

[Thomas Zimmermann]

According to the chip detection in ast_detect_chip(), AST2300 and
later always have a PCI revision of 0x20 or higher. Therefore the
code in ast_set_def_ext_reg() can not use the else branch when
selecing the EXT register values. Remove the dead branch and the
related values.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_post.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 0262aaafdb1c5..aa3f2cb00f82c 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -64,14 +64,12 @@ bool ast_is_vga_enabled(struct drm_device *dev)
 }
 
 static const u8 extreginfo[] = { 0x0f, 0x04, 0x1c, 0xff };
-static const u8 extreginfo_ast2300a0[] = { 0x0f, 0x04, 0x1c, 0xff };
 static const u8 extreginfo_ast2300[] = { 0x0f, 0x04, 0x1f, 0xff };
 
 static void
 ast_set_def_ext_reg(struct drm_device *dev)
 {
 	struct ast_device *ast = to_ast_device(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	u8 i, index, reg;
 	const u8 *ext_reg_info;
 
@@ -79,13 +77,9 @@ ast_set_def_ext_reg(struct drm_device *dev)
 	for (i = 0x81; i <= 0x9f; i++)
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, i, 0x00);
 
-	if (ast->chip == AST2300 || ast->chip == AST2400 ||
-	    ast->chip == AST2500) {
-		if (pdev->revision >= 0x20)
-			ext_reg_info = extreginfo_ast2300;
-		else
-			ext_reg_info = extreginfo_ast2300a0;
-	} else
+	if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip == AST2500)
+		ext_reg_info = extreginfo_ast2300;
+	else
 		ext_reg_info = extreginfo;
 
 	index = 0xa0;
-- 
2.41.0

[PATCH 05/14] drm/ast: Remove device POSTing and config from chip detection

[Thomas Zimmermann]

There's way too much going on in ast_detect_chip(). Move the POST
and config code from the top of the function into the caller. No
functional changes.

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

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 862fdf02f6100..c6987e0446618 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -44,7 +44,6 @@ static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 
 	/* Defaults */
 	ast->config_mode = ast_use_defaults;
-	*scu_rev = 0xffffffff;
 
 	/* Check if we have device-tree properties */
 	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id",
@@ -92,32 +91,11 @@ static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 	drm_info(dev, "P2A bridge disabled, using default configuration\n");
 }
 
-static int ast_detect_chip(struct drm_device *dev, bool *need_post)
+static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 {
 	struct ast_device *ast = to_ast_device(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	uint32_t jreg, scu_rev;
-
-	/*
-	 * If VGA isn't enabled, we need to enable now or subsequent
-	 * access to the scratch registers will fail. We also inform
-	 * our caller that it needs to POST the chip
-	 * (Assumption: VGA not enabled -> need to POST)
-	 */
-	if (!ast_is_vga_enabled(dev)) {
-		ast_enable_vga(dev);
-		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
-		*need_post = true;
-	} else
-		*need_post = false;
-
-
-	/* Enable extended register access */
-	ast_open_key(ast);
-	ast_enable_mmio(dev);
-
-	/* Find out whether P2A works or whether to use device-tree */
-	ast_detect_config_mode(dev, &scu_rev);
+	uint32_t jreg;
 
 	/* Identify chipset */
 	if (pdev->revision >= 0x50) {
@@ -195,7 +173,7 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
 	 * is at power-on reset, otherwise we'll incorrectly "detect" a
 	 * SIL164 when there is none.
 	 */
-	if (!*need_post) {
+	if (!need_post) {
 		jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xa3, 0xff);
 		if (jreg & 0x80)
 			ast->tx_chip_types = AST_TX_SIL164_BIT;
@@ -384,8 +362,9 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 {
 	struct drm_device *dev;
 	struct ast_device *ast;
-	bool need_post;
+	bool need_post = false;
 	int ret = 0;
+	u32 scu_rev = 0xffffffff;
 
 	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
 	if (IS_ERR(ast))
@@ -420,7 +399,26 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 			return ERR_PTR(-EIO);
 	}
 
-	ast_detect_chip(dev, &need_post);
+	if (!ast_is_vga_enabled(dev)) {
+		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(dev);
+
+	/* Enable extended register access */
+	ast_open_key(ast);
+	ast_enable_mmio(dev);
+
+	/* Find out whether P2A works or whether to use device-tree */
+	ast_detect_config_mode(dev, &scu_rev);
+
+	ast_detect_chip(dev, need_post, scu_rev);
 
 	ret = ast_get_dram_info(dev);
 	if (ret)
-- 
2.41.0

[PATCH 06/14] drm/ast: Set PCI config before accessing I/O registers

[Thomas Zimmermann]

Access to I/O registers is required to detect and set up the
device. Enable the rsp PCI config bits before. While at it,
convert the magic number to macro constants.

Enabling the PCI config bits was done after trying to detect
the device. It was probably too late at this point.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 0141705beaee9..555a0850957f3 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -52,6 +52,8 @@
 #define PCI_CHIP_AST2000 0x2000
 #define PCI_CHIP_AST2100 0x2010
 
+#define AST_PCI_OPTION_MEM_ACCESS_ENABLE	BIT(1)
+#define AST_PCI_OPTION_IO_ACCESS_ENABLE		BIT(0)
 
 enum ast_chip {
 	AST2000,
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index c6987e0446618..fe054739b494a 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,6 +35,24 @@
 
 #include "ast_drv.h"
 
+static int ast_init_pci_config(struct pci_dev *pdev)
+{
+	int err;
+	u32 pcis04;
+
+	err = pci_read_config_dword(pdev, 0x04, &pcis04);
+	if (err)
+		goto out;
+
+	pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
+		  AST_PCI_OPTION_IO_ACCESS_ENABLE;
+
+	err = pci_write_config_dword(pdev, 0x04, pcis04);
+
+out:
+	return pcibios_err_to_errno(err);
+}
+
 static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 {
 	struct device_node *np = dev->dev->of_node;
@@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 			return ERR_PTR(-EIO);
 	}
 
+	ret = ast_init_pci_config(pdev);
+	if (ret)
+		return ERR_PTR(ret);
+
 	if (!ast_is_vga_enabled(dev)) {
 		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
 		need_post = true;
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index aa3f2cb00f82c..2da5bdb4bac45 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device *dev)
 void ast_post_gpu(struct drm_device *dev)
 {
 	struct ast_device *ast = to_ast_device(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	u32 reg;
-
-	pci_read_config_dword(pdev, 0x04, &reg);
-	reg |= 0x3;
-	pci_write_config_dword(pdev, 0x04, reg);
 
 	ast_enable_vga(dev);
 	ast_open_key(ast);
-- 
2.41.0

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Sui Jingfeng]

Hi

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Access to I/O registers is required to detect and set up the
> device. Enable the rsp PCI config bits before. While at it,
> convert the magic number to macro constants.
>
> Enabling the PCI config bits was done after trying to detect
> the device.

Otherwise the device can not be configured,  its don't even receive 
write and read access anymore.

> It was probably too late at this point.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>   3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 0141705beaee9..555a0850957f3 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -52,6 +52,8 @@
>   #define PCI_CHIP_AST2000 0x2000
>   #define PCI_CHIP_AST2100 0x2010
>   
> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE	BIT(1)
> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE		BIT(0)

You can use the space replace the tab to keep the 'BIT(0)' and 'BIT(1)' 
aligned,

why you need define this two macros youself,

why not use PCI_COMMAND_MEMORY and PCI_COMMAND_IO ?

>   
>   enum ast_chip {
>   	AST2000,
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index c6987e0446618..fe054739b494a 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,6 +35,24 @@
>   
>   #include "ast_drv.h"
>   
> +static int ast_init_pci_config(struct pci_dev *pdev)
> +{
> +	int err;
> +	u32 pcis04;
> +
> +	err = pci_read_config_dword(pdev, 0x04, &pcis04);
> +	if (err)
> +		goto out;
> +
> +	pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
> +		  AST_PCI_OPTION_IO_ACCESS_ENABLE;
> +
> +	err = pci_write_config_dword(pdev, 0x04, pcis04);
> +
> +out:
> +	return pcibios_err_to_errno(err);
> +}
> +

static void ast_enable_mem_io(struct pci_dev *pdev)
{
     u16 cmd;

     pci_read_config_word(pdev, PCI_COMMAND, &cmd);

     cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;

     dev_dbg(&pdev->dev, "pci command: %u", cmd);

     pci_read_config_word(pdev, PCI_COMMAND, &cmd);
}

>   static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>   {
>   	struct device_node *np = dev->dev->of_node;
> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   			return ERR_PTR(-EIO);
>   	}
>   
> +	ret = ast_init_pci_config(pdev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	if (!ast_is_vga_enabled(dev)) {
>   		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
>   		need_post = true;
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index aa3f2cb00f82c..2da5bdb4bac45 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   void ast_post_gpu(struct drm_device *dev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	u32 reg;
> -
> -	pci_read_config_dword(pdev, 0x04, &reg);
> -	reg |= 0x3;
> -	pci_write_config_dword(pdev, 0x04, reg);
>   
>   	ast_enable_vga(dev);
>   	ast_open_key(ast);

-- 
Jingfeng

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Thomas Zimmermann]

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

Hi

Am 17.06.23 um 09:54 schrieb Sui Jingfeng:
> Hi
> 
> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>> Access to I/O registers is required to detect and set up the
>> device. Enable the rsp PCI config bits before. While at it,
>> convert the magic number to macro constants.
>>
>> Enabling the PCI config bits was done after trying to detect
>> the device.
> 
> Otherwise the device can not be configured,  its don't even receive 
> write and read access anymore.

I don't understand. The PCI config reg needs to be set, so that I/O 
spaces are available for detecting the chip type.

> 
>> It was probably too late at this point.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>>   3 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index 0141705beaee9..555a0850957f3 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -52,6 +52,8 @@
>>   #define PCI_CHIP_AST2000 0x2000
>>   #define PCI_CHIP_AST2100 0x2010
>> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
> 
> You can use the space replace the tab to keep the 'BIT(0)' and 'BIT(1)' 
> aligned,
> 
> why you need define this two macros youself,
> 
> why not use PCI_COMMAND_MEMORY and PCI_COMMAND_IO ?

Good point, I'll use those. I wasn't aware of these constants.

> 
>>   enum ast_chip {
>>       AST2000,
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index c6987e0446618..fe054739b494a 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -35,6 +35,24 @@
>>   #include "ast_drv.h"
>> +static int ast_init_pci_config(struct pci_dev *pdev)
>> +{
>> +    int err;
>> +    u32 pcis04;
>> +
>> +    err = pci_read_config_dword(pdev, 0x04, &pcis04);
>> +    if (err)
>> +        goto out;
>> +
>> +    pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
>> +          AST_PCI_OPTION_IO_ACCESS_ENABLE;
>> +
>> +    err = pci_write_config_dword(pdev, 0x04, pcis04);
>> +
>> +out:
>> +    return pcibios_err_to_errno(err);
>> +}
>> +
> 
> static void ast_enable_mem_io(struct pci_dev *pdev)
> {
>      u16 cmd;
> 
>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> 
>      cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> 
>      dev_dbg(&pdev->dev, "pci command: %u", cmd);
> 
>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> }

Although cosmetical, I'm not so super-happy about the specs disagreeing 
here: PCI tends to treat status and command as separate 16-bit regs, 
while the AST spec treats it as one 32-bit register. I'll consider 
changing the code to follow the PCI spec.

Best regards
Thomas

> 
>>   static void ast_detect_config_mode(struct drm_device *dev, u32 
>> *scu_rev)
>>   {
>>       struct device_node *np = dev->dev->of_node;
>> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>               return ERR_PTR(-EIO);
>>       }
>> +    ret = ast_init_pci_config(pdev);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
>>       if (!ast_is_vga_enabled(dev)) {
>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>> POST\n");
>>           need_post = true;
>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>> b/drivers/gpu/drm/ast/ast_post.c
>> index aa3f2cb00f82c..2da5bdb4bac45 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device 
>> *dev)
>>   void ast_post_gpu(struct drm_device *dev)
>>   {
>>       struct ast_device *ast = to_ast_device(dev);
>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>> -    u32 reg;
>> -
>> -    pci_read_config_dword(pdev, 0x04, &reg);
>> -    reg |= 0x3;
>> -    pci_write_config_dword(pdev, 0x04, reg);
>>       ast_enable_vga(dev);
>>       ast_open_key(ast);
> 

-- 
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 --]

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Sui Jingfeng]

Hi,

On 2023/6/19 16:16, Thomas Zimmermann wrote:
> Although cosmetical, I'm not so super-happy about the specs 
> disagreeing here: 
[...]
> PCI tends to treat status and command as separate 16-bit regs, while 
> the AST spec treats it as one 32-bit register.


I don't know this hardware constrain before,

but what's problem if we switch to using pci_read_config_word() function ?


I have tried this function on other platform, no problem.

I will go to test this function on my AST 2400 discrete card again, wait 
a while :-).

If there are hardware defect, I don't mind you keep using 
pci_read_config_dword().

-- 
Jingfeng

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Sui Jingfeng]

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Access to I/O registers is required to detect and set up the
> device. Enable the rsp PCI config bits before. While at it,
> convert the magic number to macro constants.
>
> Enabling the PCI config bits was done after trying to detect
> the device. It was probably too late at this point.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>   3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 0141705beaee9..555a0850957f3 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -52,6 +52,8 @@
>   #define PCI_CHIP_AST2000 0x2000
>   #define PCI_CHIP_AST2100 0x2010
>   
> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE	BIT(1)
> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE		BIT(0)
>   
>   enum ast_chip {
>   	AST2000,
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index c6987e0446618..fe054739b494a 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,6 +35,24 @@
>   
>   #include "ast_drv.h"
>   
> +static int ast_init_pci_config(struct pci_dev *pdev)
> +{
> +	int err;
> +	u32 pcis04;
> +
> +	err = pci_read_config_dword(pdev, 0x04, &pcis04);

The third argument of pci_read_config_dword() function should be 'u16 *' 
type;


> +	if (err)
> +		goto out;
> +
> +	pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
> +		  AST_PCI_OPTION_IO_ACCESS_ENABLE;
> +
> +	err = pci_write_config_dword(pdev, 0x04, pcis04);
> +
> +out:
> +	return pcibios_err_to_errno(err);
> +}


static void ast_enable_mem_io(struct pci_dev *pdev)
{
     u16 cmd;

     pci_read_config_word(pdev, PCI_COMMAND, &cmd);

     cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;

     pci_write_config_word(pdev, PCI_COMMAND, &cmd);
}

>   static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>   {
>   	struct device_node *np = dev->dev->of_node;
> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   			return ERR_PTR(-EIO);
>   	}
>   
> +	ret = ast_init_pci_config(pdev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	if (!ast_is_vga_enabled(dev)) {
>   		drm_info(dev, "VGA not enabled on entry, requesting chip POST\n");
>   		need_post = true;
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index aa3f2cb00f82c..2da5bdb4bac45 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   void ast_post_gpu(struct drm_device *dev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	u32 reg;
> -
> -	pci_read_config_dword(pdev, 0x04, &reg);
> -	reg |= 0x3;
> -	pci_write_config_dword(pdev, 0x04, reg);
>   
>   	ast_enable_vga(dev);
>   	ast_open_key(ast);

-- 
Jingfeng

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Thomas Zimmermann]

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

Hi

Am 17.06.23 um 10:01 schrieb Sui Jingfeng:
> 
> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>> Access to I/O registers is required to detect and set up the
>> device. Enable the rsp PCI config bits before. While at it,
>> convert the magic number to macro constants.
>>
>> Enabling the PCI config bits was done after trying to detect
>> the device. It was probably too late at this point.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>>   3 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index 0141705beaee9..555a0850957f3 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -52,6 +52,8 @@
>>   #define PCI_CHIP_AST2000 0x2000
>>   #define PCI_CHIP_AST2100 0x2010
>> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>>   enum ast_chip {
>>       AST2000,
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index c6987e0446618..fe054739b494a 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -35,6 +35,24 @@
>>   #include "ast_drv.h"
>> +static int ast_init_pci_config(struct pci_dev *pdev)
>> +{
>> +    int err;
>> +    u32 pcis04;
>> +
>> +    err = pci_read_config_dword(pdev, 0x04, &pcis04);
> 
> The third argument of pci_read_config_dword() function should be 'u16 *' 
> type;

No, a dword is a 32-bit integer.

> 
> 
>> +    if (err)
>> +        goto out;
>> +
>> +    pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
>> +          AST_PCI_OPTION_IO_ACCESS_ENABLE;
>> +
>> +    err = pci_write_config_dword(pdev, 0x04, pcis04);
>> +
>> +out:
>> +    return pcibios_err_to_errno(err);
>> +}
> 
> 
> static void ast_enable_mem_io(struct pci_dev *pdev)
> {
>      u16 cmd;
> 
>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> 
>      cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> 
>      pci_write_config_word(pdev, PCI_COMMAND, &cmd);
> }
> 
>>   static void ast_detect_config_mode(struct drm_device *dev, u32 
>> *scu_rev)
>>   {
>>       struct device_node *np = dev->dev->of_node;
>> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>               return ERR_PTR(-EIO);
>>       }
>> +    ret = ast_init_pci_config(pdev);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
>>       if (!ast_is_vga_enabled(dev)) {
>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>> POST\n");
>>           need_post = true;
>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>> b/drivers/gpu/drm/ast/ast_post.c
>> index aa3f2cb00f82c..2da5bdb4bac45 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device 
>> *dev)
>>   void ast_post_gpu(struct drm_device *dev)
>>   {
>>       struct ast_device *ast = to_ast_device(dev);
>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>> -    u32 reg;
>> -
>> -    pci_read_config_dword(pdev, 0x04, &reg);
>> -    reg |= 0x3;
>> -    pci_write_config_dword(pdev, 0x04, reg);
>>       ast_enable_vga(dev);
>>       ast_open_key(ast);
> 

-- 
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 --]

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Sui Jingfeng]

Hi,

On 2023/6/19 15:59, Thomas Zimmermann wrote:
> Hi
>
> Am 17.06.23 um 10:01 schrieb Sui Jingfeng:
>>
>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>> Access to I/O registers is required to detect and set up the
>>> device. Enable the rsp PCI config bits before. While at it,
>>> convert the magic number to macro constants.
>>>
>>> Enabling the PCI config bits was done after trying to detect
>>> the device. It was probably too late at this point.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>>>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>>>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>>>   3 files changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>>> b/drivers/gpu/drm/ast/ast_drv.h
>>> index 0141705beaee9..555a0850957f3 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>> @@ -52,6 +52,8 @@
>>>   #define PCI_CHIP_AST2000 0x2000
>>>   #define PCI_CHIP_AST2100 0x2010
>>> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>>> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>>>   enum ast_chip {
>>>       AST2000,
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>> b/drivers/gpu/drm/ast/ast_main.c
>>> index c6987e0446618..fe054739b494a 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -35,6 +35,24 @@
>>>   #include "ast_drv.h"
>>> +static int ast_init_pci_config(struct pci_dev *pdev)
>>> +{
>>> +    int err;
>>> +    u32 pcis04;
>>> +
>>> +    err = pci_read_config_dword(pdev, 0x04, &pcis04);
>>
>> The third argument of pci_read_config_dword() function should be 'u16 
>> *' type;
>
> No, a dword is a 32-bit integer.
>
I meant the "u32 pcis04" should be declared as "u16 pcis04", right ?

>>
>>
>>> +    if (err)
>>> +        goto out;
>>> +
>>> +    pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
>>> +          AST_PCI_OPTION_IO_ACCESS_ENABLE;
>>> +
>>> +    err = pci_write_config_dword(pdev, 0x04, pcis04);
>>> +
>>> +out:
>>> +    return pcibios_err_to_errno(err);
>>> +}
>>
>>
>> static void ast_enable_mem_io(struct pci_dev *pdev)
>> {
>>      u16 cmd;
>>
>>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>>
>>      cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>>
>>      pci_write_config_word(pdev, PCI_COMMAND, &cmd);
>> }
>>
>>>   static void ast_detect_config_mode(struct drm_device *dev, u32 
>>> *scu_rev)
>>>   {
>>>       struct device_node *np = dev->dev->of_node;
>>> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const 
>>> struct drm_driver *drv,
>>>               return ERR_PTR(-EIO);
>>>       }
>>> +    ret = ast_init_pci_config(pdev);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>>       if (!ast_is_vga_enabled(dev)) {
>>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>>> POST\n");
>>>           need_post = true;
>>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>>> b/drivers/gpu/drm/ast/ast_post.c
>>> index aa3f2cb00f82c..2da5bdb4bac45 100644
>>> --- a/drivers/gpu/drm/ast/ast_post.c
>>> +++ b/drivers/gpu/drm/ast/ast_post.c
>>> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device 
>>> *dev)
>>>   void ast_post_gpu(struct drm_device *dev)
>>>   {
>>>       struct ast_device *ast = to_ast_device(dev);
>>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>>> -    u32 reg;
>>> -
>>> -    pci_read_config_dword(pdev, 0x04, &reg);
>>> -    reg |= 0x3;
>>> -    pci_write_config_dword(pdev, 0x04, reg);
>>>       ast_enable_vga(dev);
>>>       ast_open_key(ast);
>>
>
-- 
Jingfeng

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Sui Jingfeng]

Hi,

On 2023/6/19 15:59, Thomas Zimmermann wrote:
> Hi
>
> Am 17.06.23 um 10:01 schrieb Sui Jingfeng:
>>
>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>> Access to I/O registers is required to detect and set up the
>>> device. Enable the rsp PCI config bits before. While at it,
>>> convert the magic number to macro constants.
>>>
>>> Enabling the PCI config bits was done after trying to detect
>>> the device. It was probably too late at this point.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>>>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>>>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>>>   3 files changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>>> b/drivers/gpu/drm/ast/ast_drv.h
>>> index 0141705beaee9..555a0850957f3 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>> @@ -52,6 +52,8 @@
>>>   #define PCI_CHIP_AST2000 0x2000
>>>   #define PCI_CHIP_AST2100 0x2010
>>> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>>> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>>>   enum ast_chip {
>>>       AST2000,
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>> b/drivers/gpu/drm/ast/ast_main.c
>>> index c6987e0446618..fe054739b494a 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -35,6 +35,24 @@
>>>   #include "ast_drv.h"
>>> +static int ast_init_pci_config(struct pci_dev *pdev)
>>> +{
>>> +    int err;
>>> +    u32 pcis04;
>>> +
>>> +    err = pci_read_config_dword(pdev, 0x04, &pcis04);
>>
>> The third argument of pci_read_config_dword() function should be 'u16 
>> *' type;
>
> No, a dword is a 32-bit integer.
>
Yes, you are right.

'u32' is for the pci_read_config_dword() function.

I'm recommend you to use the pci_read_config_word() function.

Sorry for the noise.

>>
>>
>>> +    if (err)
>>> +        goto out;
>>> +
>>> +    pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
>>> +          AST_PCI_OPTION_IO_ACCESS_ENABLE;
>>> +
>>> +    err = pci_write_config_dword(pdev, 0x04, pcis04);
>>> +
>>> +out:
>>> +    return pcibios_err_to_errno(err);
>>> +}
>>
>>
>> static void ast_enable_mem_io(struct pci_dev *pdev)
>> {
>>      u16 cmd;
>>
>>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>>
>>      cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>>
>>      pci_write_config_word(pdev, PCI_COMMAND, &cmd);
>> }
>>
>>>   static void ast_detect_config_mode(struct drm_device *dev, u32 
>>> *scu_rev)
>>>   {
>>>       struct device_node *np = dev->dev->of_node;
>>> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const 
>>> struct drm_driver *drv,
>>>               return ERR_PTR(-EIO);
>>>       }
>>> +    ret = ast_init_pci_config(pdev);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>>       if (!ast_is_vga_enabled(dev)) {
>>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>>> POST\n");
>>>           need_post = true;
>>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>>> b/drivers/gpu/drm/ast/ast_post.c
>>> index aa3f2cb00f82c..2da5bdb4bac45 100644
>>> --- a/drivers/gpu/drm/ast/ast_post.c
>>> +++ b/drivers/gpu/drm/ast/ast_post.c
>>> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device 
>>> *dev)
>>>   void ast_post_gpu(struct drm_device *dev)
>>>   {
>>>       struct ast_device *ast = to_ast_device(dev);
>>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>>> -    u32 reg;
>>> -
>>> -    pci_read_config_dword(pdev, 0x04, &reg);
>>> -    reg |= 0x3;
>>> -    pci_write_config_dword(pdev, 0x04, reg);
>>>       ast_enable_vga(dev);
>>>       ast_open_key(ast);
>>
>
-- 
Jingfeng

Re: [06/14] drm/ast: Set PCI config before accessing I/O registers

[Thomas Zimmermann]

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

Hi

Am 19.06.23 um 10:19 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/6/19 15:59, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.06.23 um 10:01 schrieb Sui Jingfeng:
>>>
>>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>>> Access to I/O registers is required to detect and set up the
>>>> device. Enable the rsp PCI config bits before. While at it,
>>>> convert the magic number to macro constants.
>>>>
>>>> Enabling the PCI config bits was done after trying to detect
>>>> the device. It was probably too late at this point.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>   drivers/gpu/drm/ast/ast_drv.h  |  2 ++
>>>>   drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++++++++
>>>>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>>>>   3 files changed, 24 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>>>> b/drivers/gpu/drm/ast/ast_drv.h
>>>> index 0141705beaee9..555a0850957f3 100644
>>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>>> @@ -52,6 +52,8 @@
>>>>   #define PCI_CHIP_AST2000 0x2000
>>>>   #define PCI_CHIP_AST2100 0x2010
>>>> +#define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>>>> +#define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>>>>   enum ast_chip {
>>>>       AST2000,
>>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>>> b/drivers/gpu/drm/ast/ast_main.c
>>>> index c6987e0446618..fe054739b494a 100644
>>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>>> @@ -35,6 +35,24 @@
>>>>   #include "ast_drv.h"
>>>> +static int ast_init_pci_config(struct pci_dev *pdev)
>>>> +{
>>>> +    int err;
>>>> +    u32 pcis04;
>>>> +
>>>> +    err = pci_read_config_dword(pdev, 0x04, &pcis04);
>>>
>>> The third argument of pci_read_config_dword() function should be 'u16 
>>> *' type;
>>
>> No, a dword is a 32-bit integer.
>>
> Yes, you are right.
> 
> 'u32' is for the pci_read_config_dword() function.
> 
> I'm recommend you to use the pci_read_config_word() function.
> 
> Sorry for the noise.

No problem, please see my other reply. The PCI and ast specs disagree a 
bit. I'm considering to change this to the 16-bit access.

Best regards
Thomas

> 
>>>
>>>
>>>> +    if (err)
>>>> +        goto out;
>>>> +
>>>> +    pcis04 |= AST_PCI_OPTION_MEM_ACCESS_ENABLE |
>>>> +          AST_PCI_OPTION_IO_ACCESS_ENABLE;
>>>> +
>>>> +    err = pci_write_config_dword(pdev, 0x04, pcis04);
>>>> +
>>>> +out:
>>>> +    return pcibios_err_to_errno(err);
>>>> +}
>>>
>>>
>>> static void ast_enable_mem_io(struct pci_dev *pdev)
>>> {
>>>      u16 cmd;
>>>
>>>      pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>>>
>>>      cmd |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
>>>
>>>      pci_write_config_word(pdev, PCI_COMMAND, &cmd);
>>> }
>>>
>>>>   static void ast_detect_config_mode(struct drm_device *dev, u32 
>>>> *scu_rev)
>>>>   {
>>>>       struct device_node *np = dev->dev->of_node;
>>>> @@ -399,6 +417,10 @@ struct ast_device *ast_device_create(const 
>>>> struct drm_driver *drv,
>>>>               return ERR_PTR(-EIO);
>>>>       }
>>>> +    ret = ast_init_pci_config(pdev);
>>>> +    if (ret)
>>>> +        return ERR_PTR(ret);
>>>> +
>>>>       if (!ast_is_vga_enabled(dev)) {
>>>>           drm_info(dev, "VGA not enabled on entry, requesting chip 
>>>> POST\n");
>>>>           need_post = true;
>>>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>>>> b/drivers/gpu/drm/ast/ast_post.c
>>>> index aa3f2cb00f82c..2da5bdb4bac45 100644
>>>> --- a/drivers/gpu/drm/ast/ast_post.c
>>>> +++ b/drivers/gpu/drm/ast/ast_post.c
>>>> @@ -361,12 +361,6 @@ static void ast_init_dram_reg(struct drm_device 
>>>> *dev)
>>>>   void ast_post_gpu(struct drm_device *dev)
>>>>   {
>>>>       struct ast_device *ast = to_ast_device(dev);
>>>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>>>> -    u32 reg;
>>>> -
>>>> -    pci_read_config_dword(pdev, 0x04, &reg);
>>>> -    reg |= 0x3;
>>>> -    pci_write_config_dword(pdev, 0x04, reg);
>>>>       ast_enable_vga(dev);
>>>>       ast_open_key(ast);
>>>
>>

-- 
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 --]

[PATCH 07/14] drm/ast: Enable and unlock device access early during init

[Thomas Zimmermann]

POST and memory management contains code to enable access to the
device's memory spaces. This is too late. Consolidate this code at
the beginning of the device initialization.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 555a0850957f3..c42dfb86e040d 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -290,11 +290,6 @@ static inline void ast_set_index_reg_mask(struct ast_device *ast, u32 base, u8 i
 	ast_set_index_reg(ast, base, index, tmp);
 }
 
-static inline void ast_open_key(struct ast_device *ast)
-{
-	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0x80, 0xA8);
-}
-
 #define AST_VIDMEM_SIZE_8M    0x00800000
 #define AST_VIDMEM_SIZE_16M   0x01000000
 #define AST_VIDMEM_SIZE_32M   0x02000000
@@ -473,9 +468,6 @@ int ast_mode_config_init(struct ast_device *ast);
 int ast_mm_init(struct ast_device *ast);
 
 /* ast post */
-void ast_enable_vga(struct drm_device *dev);
-void ast_enable_mmio(struct drm_device *dev);
-bool ast_is_vga_enabled(struct drm_device *dev);
 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);
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index fe054739b494a..3295876c09b35 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -53,6 +53,36 @@ static int ast_init_pci_config(struct pci_dev *pdev)
 	return pcibios_err_to_errno(err);
 }
 
+static bool ast_is_vga_enabled(struct drm_device *dev)
+{
+	struct ast_device *ast = to_ast_device(dev);
+	u8 ch;
+
+	ch = ast_io_read8(ast, AST_IO_VGA_ENABLE_PORT);
+
+	return !!(ch & 0x01);
+}
+
+static void ast_enable_vga(struct drm_device *dev)
+{
+	struct ast_device *ast = to_ast_device(dev);
+
+	ast_io_write8(ast, AST_IO_VGA_ENABLE_PORT, 0x01);
+	ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
+}
+
+static void ast_enable_mmio(struct drm_device *dev)
+{
+	struct ast_device *ast = to_ast_device(dev);
+
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
+}
+
+static void ast_open_key(struct ast_device *ast)
+{
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0x80, 0xA8);
+}
+
 static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 {
 	struct device_node *np = dev->dev->of_node;
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index e16af60deef90..bc174bd933b97 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -38,8 +38,6 @@ static u32 ast_get_vram_size(struct ast_device *ast)
 	u8 jreg;
 	u32 vram_size;
 
-	ast_open_key(ast);
-
 	vram_size = AST_VIDMEM_DEFAULT_SIZE;
 	jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xaa, 0xff);
 	switch (jreg & 3) {
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 2da5bdb4bac45..b765eeb55e5f1 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -37,32 +37,6 @@
 static void ast_post_chip_2300(struct drm_device *dev);
 static void ast_post_chip_2500(struct drm_device *dev);
 
-void ast_enable_vga(struct drm_device *dev)
-{
-	struct ast_device *ast = to_ast_device(dev);
-
-	ast_io_write8(ast, AST_IO_VGA_ENABLE_PORT, 0x01);
-	ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
-}
-
-void ast_enable_mmio(struct drm_device *dev)
-{
-	struct ast_device *ast = to_ast_device(dev);
-
-	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
-}
-
-
-bool ast_is_vga_enabled(struct drm_device *dev)
-{
-	struct ast_device *ast = to_ast_device(dev);
-	u8 ch;
-
-	ch = ast_io_read8(ast, AST_IO_VGA_ENABLE_PORT);
-
-	return !!(ch & 0x01);
-}
-
 static const u8 extreginfo[] = { 0x0f, 0x04, 0x1c, 0xff };
 static const u8 extreginfo_ast2300[] = { 0x0f, 0x04, 0x1f, 0xff };
 
@@ -362,9 +336,6 @@ void ast_post_gpu(struct drm_device *dev)
 {
 	struct ast_device *ast = to_ast_device(dev);
 
-	ast_enable_vga(dev);
-	ast_open_key(ast);
-	ast_enable_mmio(dev);
 	ast_set_def_ext_reg(dev);
 
 	if (ast->chip == AST2600) {
-- 
2.41.0

[PATCH 08/14] drm/ast: Set up release action right after enabling MMIO

[Thomas Zimmermann]

Ast sets up a managed release of the MMIO access flags. Move this
code next to the MMIO access code, so that it runs if other errors
occur during the device initialization.

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

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 3295876c09b35..6ff4b837e64d7 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -71,11 +71,25 @@ static void ast_enable_vga(struct drm_device *dev)
 	ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
 }
 
-static void ast_enable_mmio(struct drm_device *dev)
+/*
+ * 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)
 {
-	struct ast_device *ast = to_ast_device(dev);
+	struct ast_device *ast = data;
+
+	/* enable standard VGA decode */
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
+}
+
+static int ast_enable_mmio(struct ast_device *ast)
+{
+	struct drm_device *dev = &ast->base;
 
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
+
+	return devm_add_action_or_reset(dev->dev, ast_enable_mmio_release, ast);
 }
 
 static void ast_open_key(struct ast_device *ast)
@@ -392,18 +406,6 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * Run this function as part of the HW device cleanup; not
- * when the DRM device gets released.
- */
-static void ast_device_release(void *data)
-{
-	struct ast_device *ast = data;
-
-	/* enable standard VGA decode */
-	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
-}
-
 struct ast_device *ast_device_create(const struct drm_driver *drv,
 				     struct pci_dev *pdev,
 				     unsigned long flags)
@@ -465,7 +467,9 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 
 	/* Enable extended register access */
 	ast_open_key(ast);
-	ast_enable_mmio(dev);
+	ret = ast_enable_mmio(ast);
+	if (ret)
+		return ERR_PTR(ret);
 
 	/* Find out whether P2A works or whether to use device-tree */
 	ast_detect_config_mode(dev, &scu_rev);
@@ -498,9 +502,5 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 
-	ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast);
-	if (ret)
-		return ERR_PTR(ret);
-
 	return ast;
 }
-- 
2.41.0

Re: [08/14] drm/ast: Set up release action right after enabling MMIO

[Sui Jingfeng]

Hi,


Tested with ast2400


On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Ast sets up a managed release of the MMIO access flags. Move this
> code next to the MMIO access code, so that it runs if other errors
> occur during the device initialization.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>

> ---
>   drivers/gpu/drm/ast/ast_main.c | 38 +++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 3295876c09b35..6ff4b837e64d7 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -71,11 +71,25 @@ static void ast_enable_vga(struct drm_device *dev)
>   	ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
>   }
>   
> -static void ast_enable_mmio(struct drm_device *dev)
> +/*
> + * 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)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
> +	struct ast_device *ast = data;
> +
> +	/* enable standard VGA decode */
> +	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
> +}
> +
> +static int ast_enable_mmio(struct ast_device *ast)
> +{
> +	struct drm_device *dev = &ast->base;
>   
>   	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
> +
> +	return devm_add_action_or_reset(dev->dev, ast_enable_mmio_release, ast);
>   }
>   
>   static void ast_open_key(struct ast_device *ast)
> @@ -392,18 +406,6 @@ static int ast_get_dram_info(struct drm_device *dev)
>   	return 0;
>   }
>   
> -/*
> - * Run this function as part of the HW device cleanup; not
> - * when the DRM device gets released.
> - */
> -static void ast_device_release(void *data)
> -{
> -	struct ast_device *ast = data;
> -
> -	/* enable standard VGA decode */
> -	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
> -}
> -
>   struct ast_device *ast_device_create(const struct drm_driver *drv,
>   				     struct pci_dev *pdev,
>   				     unsigned long flags)
> @@ -465,7 +467,9 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   
>   	/* Enable extended register access */
>   	ast_open_key(ast);
> -	ast_enable_mmio(dev);
> +	ret = ast_enable_mmio(ast);
> +	if (ret)
> +		return ERR_PTR(ret);
>   
>   	/* Find out whether P2A works or whether to use device-tree */
>   	ast_detect_config_mode(dev, &scu_rev);
> @@ -498,9 +502,5 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	return ast;
>   }

-- 
Jingfeng

Re: [08/14] drm/ast: Set up release action right after enabling MMIO

[Thomas Zimmermann]

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



Am 19.06.23 um 03:57 schrieb Sui Jingfeng:
> Hi,
> 
> 
> Tested with ast2400
> 
> 
> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>> Ast sets up a managed release of the MMIO access flags. Move this
>> code next to the MMIO access code, so that it runs if other errors
>> occur during the device initialization.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

Which model do you test on?

> 
> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
> 
>> ---
>>   drivers/gpu/drm/ast/ast_main.c | 38 +++++++++++++++++-----------------
>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index 3295876c09b35..6ff4b837e64d7 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -71,11 +71,25 @@ static void ast_enable_vga(struct drm_device *dev)
>>       ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
>>   }
>> -static void ast_enable_mmio(struct drm_device *dev)
>> +/*
>> + * 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)
>>   {
>> -    struct ast_device *ast = to_ast_device(dev);
>> +    struct ast_device *ast = data;
>> +
>> +    /* enable standard VGA decode */
>> +    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>> +}
>> +
>> +static int ast_enable_mmio(struct ast_device *ast)
>> +{
>> +    struct drm_device *dev = &ast->base;
>>       ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
>> +
>> +    return devm_add_action_or_reset(dev->dev, 
>> ast_enable_mmio_release, ast);
>>   }
>>   static void ast_open_key(struct ast_device *ast)
>> @@ -392,18 +406,6 @@ static int ast_get_dram_info(struct drm_device *dev)
>>       return 0;
>>   }
>> -/*
>> - * Run this function as part of the HW device cleanup; not
>> - * when the DRM device gets released.
>> - */
>> -static void ast_device_release(void *data)
>> -{
>> -    struct ast_device *ast = data;
>> -
>> -    /* enable standard VGA decode */
>> -    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>> -}
>> -
>>   struct ast_device *ast_device_create(const struct drm_driver *drv,
>>                        struct pci_dev *pdev,
>>                        unsigned long flags)
>> @@ -465,7 +467,9 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>       /* Enable extended register access */
>>       ast_open_key(ast);
>> -    ast_enable_mmio(dev);
>> +    ret = ast_enable_mmio(ast);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>>       /* Find out whether P2A works or whether to use device-tree */
>>       ast_detect_config_mode(dev, &scu_rev);
>> @@ -498,9 +502,5 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>       if (ret)
>>           return ERR_PTR(ret);
>> -    ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast);
>> -    if (ret)
>> -        return ERR_PTR(ret);
>> -
>>       return ast;
>>   }
> 

-- 
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 --]

Re: [08/14] drm/ast: Set up release action right after enabling MMIO

[Thomas Zimmermann]

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



Am 19.06.23 um 10:22 schrieb Thomas Zimmermann:
> 
> 
> Am 19.06.23 um 03:57 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> Tested with ast2400
>>
>>
>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>> Ast sets up a managed release of the MMIO access flags. Move this
>>> code next to the MMIO access code, so that it runs if other errors
>>> occur during the device initialization.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Which model do you test on?

Oh, you mentioned the AST2400.

> 
>>
>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>>> ---
>>>   drivers/gpu/drm/ast/ast_main.c | 38 +++++++++++++++++-----------------
>>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>> b/drivers/gpu/drm/ast/ast_main.c
>>> index 3295876c09b35..6ff4b837e64d7 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -71,11 +71,25 @@ static void ast_enable_vga(struct drm_device *dev)
>>>       ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
>>>   }
>>> -static void ast_enable_mmio(struct drm_device *dev)
>>> +/*
>>> + * 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)
>>>   {
>>> -    struct ast_device *ast = to_ast_device(dev);
>>> +    struct ast_device *ast = data;
>>> +
>>> +    /* enable standard VGA decode */
>>> +    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>>> +}
>>> +
>>> +static int ast_enable_mmio(struct ast_device *ast)
>>> +{
>>> +    struct drm_device *dev = &ast->base;
>>>       ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
>>> +
>>> +    return devm_add_action_or_reset(dev->dev, 
>>> ast_enable_mmio_release, ast);
>>>   }
>>>   static void ast_open_key(struct ast_device *ast)
>>> @@ -392,18 +406,6 @@ static int ast_get_dram_info(struct drm_device 
>>> *dev)
>>>       return 0;
>>>   }
>>> -/*
>>> - * Run this function as part of the HW device cleanup; not
>>> - * when the DRM device gets released.
>>> - */
>>> -static void ast_device_release(void *data)
>>> -{
>>> -    struct ast_device *ast = data;
>>> -
>>> -    /* enable standard VGA decode */
>>> -    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>>> -}
>>> -
>>>   struct ast_device *ast_device_create(const struct drm_driver *drv,
>>>                        struct pci_dev *pdev,
>>>                        unsigned long flags)
>>> @@ -465,7 +467,9 @@ struct ast_device *ast_device_create(const struct 
>>> drm_driver *drv,
>>>       /* Enable extended register access */
>>>       ast_open_key(ast);
>>> -    ast_enable_mmio(dev);
>>> +    ret = ast_enable_mmio(ast);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>>       /* Find out whether P2A works or whether to use device-tree */
>>>       ast_detect_config_mode(dev, &scu_rev);
>>> @@ -498,9 +502,5 @@ struct ast_device *ast_device_create(const struct 
>>> drm_driver *drv,
>>>       if (ret)
>>>           return ERR_PTR(ret);
>>> -    ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast);
>>> -    if (ret)
>>> -        return ERR_PTR(ret);
>>> -
>>>       return ast;
>>>   }
>>
> 

-- 
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 --]

Re: [08/14] drm/ast: Set up release action right after enabling MMIO

[Sui Jingfeng]

Hi,

On 2023/6/21 19:48, Thomas Zimmermann wrote:
>
>
> Am 19.06.23 um 10:22 schrieb Thomas Zimmermann:
>>
>>
>> Am 19.06.23 um 03:57 schrieb Sui Jingfeng:
>>> Hi,
>>>
>>>
>>> Tested with ast2400
>>>
>>>
>>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>>> Ast sets up a managed release of the MMIO access flags. Move this
>>>> code next to the MMIO access code, so that it runs if other errors
>>>> occur during the device initialization.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Which model do you test on?
>
> Oh, you mentioned the AST2400.


Yes, I didn't answer this question, because I don't know what you are 
really asking back to that time.


>
>>
>>>
>>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>
>>>> ---
>>>>   drivers/gpu/drm/ast/ast_main.c | 38 
>>>> +++++++++++++++++-----------------
>>>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>>> b/drivers/gpu/drm/ast/ast_main.c
>>>> index 3295876c09b35..6ff4b837e64d7 100644
>>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>>> @@ -71,11 +71,25 @@ static void ast_enable_vga(struct drm_device *dev)
>>>>       ast_io_write8(ast, AST_IO_MISC_PORT_WRITE, 0x01);
>>>>   }
>>>> -static void ast_enable_mmio(struct drm_device *dev)
>>>> +/*
>>>> + * 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)
>>>>   {
>>>> -    struct ast_device *ast = to_ast_device(dev);
>>>> +    struct ast_device *ast = data;
>>>> +
>>>> +    /* enable standard VGA decode */
>>>> +    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>>>> +}
>>>> +
>>>> +static int ast_enable_mmio(struct ast_device *ast)
>>>> +{
>>>> +    struct drm_device *dev = &ast->base;
>>>>       ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
>>>> +
>>>> +    return devm_add_action_or_reset(dev->dev, 
>>>> ast_enable_mmio_release, ast);
>>>>   }
>>>>   static void ast_open_key(struct ast_device *ast)
>>>> @@ -392,18 +406,6 @@ static int ast_get_dram_info(struct drm_device 
>>>> *dev)
>>>>       return 0;
>>>>   }
>>>> -/*
>>>> - * Run this function as part of the HW device cleanup; not
>>>> - * when the DRM device gets released.
>>>> - */
>>>> -static void ast_device_release(void *data)
>>>> -{
>>>> -    struct ast_device *ast = data;
>>>> -
>>>> -    /* enable standard VGA decode */
>>>> -    ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>>>> -}
>>>> -
>>>>   struct ast_device *ast_device_create(const struct drm_driver *drv,
>>>>                        struct pci_dev *pdev,
>>>>                        unsigned long flags)
>>>> @@ -465,7 +467,9 @@ struct ast_device *ast_device_create(const 
>>>> struct drm_driver *drv,
>>>>       /* Enable extended register access */
>>>>       ast_open_key(ast);
>>>> -    ast_enable_mmio(dev);
>>>> +    ret = ast_enable_mmio(ast);
>>>> +    if (ret)
>>>> +        return ERR_PTR(ret);
>>>>       /* Find out whether P2A works or whether to use device-tree */
>>>>       ast_detect_config_mode(dev, &scu_rev);
>>>> @@ -498,9 +502,5 @@ struct ast_device *ast_device_create(const 
>>>> struct drm_driver *drv,
>>>>       if (ret)
>>>>           return ERR_PTR(ret);
>>>> -    ret = devm_add_action_or_reset(dev->dev, ast_device_release, 
>>>> ast);
>>>> -    if (ret)
>>>> -        return ERR_PTR(ret);
>>>> -
>>>>       return ast;
>>>>   }
>>>
>>
>
-- 
Jingfeng

[PATCH 09/14] drm/ast: Distinguish among chip generations

[Thomas Zimmermann]

ASpeed distinguishes among various generations of the AST graphics
chipset with various models. [1] The most-recent model AST 2600 is
of the 7th generation, the AST 2500 is of the 6th generation, and so
on.

The ast driver simply picks one of the models as representative for
the whole generation. In several places, individual models of the
same generation need to be handled differently, which then requires
additional code for detecting the model.

Introduce different generations of the Aspeed chipset. In the source
code, refer to the generation instead of the representation model where
possible. The few places that require per-model handling are now clearly
marked.

In the enum ast_chip, we arrange each model's value such that it
encodes the generation. This allows for an easy test. The actual values
are ordered, but not of interest to the driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://web.archive.org/web/20141007093258/http://www.aspeedtech.com/products.php?fPath=20 # 1
---
 drivers/gpu/drm/ast/ast_dp501.c |  6 ++--
 drivers/gpu/drm/ast/ast_drv.h   | 56 +++++++++++++++++++++++++++------
 drivers/gpu/drm/ast/ast_main.c  | 22 ++++++-------
 drivers/gpu/drm/ast/ast_mode.c  | 35 ++++++++++-----------
 drivers/gpu/drm/ast/ast_post.c  | 27 +++++++---------
 5 files changed, 89 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 1bc35a992369d..a5d285850ffb1 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -350,7 +350,7 @@ static bool ast_init_dvo(struct drm_device *dev)
 		data |= 0x00000500;
 		ast_write32(ast, 0x12008, data);
 
-		if (ast->chip == AST2300) {
+		if (IS_AST_GEN4(ast)) {
 			data = ast_read32(ast, 0x12084);
 			/* multi-pins for DVO single-edge */
 			data |= 0xfffe0000;
@@ -366,7 +366,7 @@ static bool ast_init_dvo(struct drm_device *dev)
 			data &= 0xffffffcf;
 			data |= 0x00000020;
 			ast_write32(ast, 0x12090, data);
-		} else { /* AST2400 */
+		} else { /* AST GEN5+ */
 			data = ast_read32(ast, 0x12088);
 			/* multi-pins for DVO single-edge */
 			data |= 0x30000000;
@@ -437,7 +437,7 @@ void ast_init_3rdtx(struct drm_device *dev)
 	struct ast_device *ast = to_ast_device(dev);
 	u8 jreg;
 
-	if (ast->chip == AST2300 || ast->chip == AST2400) {
+	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
 		jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
 		switch (jreg & 0x0e) {
 		case 0x04:
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index c42dfb86e040d..c209d7e4e4194 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -55,18 +55,38 @@
 #define AST_PCI_OPTION_MEM_ACCESS_ENABLE	BIT(1)
 #define AST_PCI_OPTION_IO_ACCESS_ENABLE		BIT(0)
 
+#define __AST_CHIP(__gen, __index)	((__gen) << 16 | (__index))
+
 enum ast_chip {
-	AST2000,
-	AST2100,
-	AST1100,
-	AST2200,
-	AST2150,
-	AST2300,
-	AST2400,
-	AST2500,
-	AST2600,
+	/* 1st gen */
+	AST1000 = __AST_CHIP(1, 0), // unused
+	AST2000 = __AST_CHIP(1, 1),
+	/* 2nd gen */
+	AST1100 = __AST_CHIP(2, 0),
+	AST2100 = __AST_CHIP(2, 1),
+	AST2050 = __AST_CHIP(2, 2), // unused
+	/* 3rd gen */
+	AST2200 = __AST_CHIP(3, 0),
+	AST2150 = __AST_CHIP(3, 1),
+	/* 4th gen */
+	AST2300 = __AST_CHIP(4, 0),
+	AST1300 = __AST_CHIP(4, 1), // unused
+	AST1050 = __AST_CHIP(4, 2), // unused
+	/* 5th gen */
+	AST2400 = __AST_CHIP(5, 0),
+	AST1400 = __AST_CHIP(5, 1), // unused
+	AST1250 = __AST_CHIP(5, 2), // unused
+	/* 6th gen */
+	AST2500 = __AST_CHIP(6, 0),
+	AST2510 = __AST_CHIP(6, 1), // unused
+	AST2520 = __AST_CHIP(6, 2), // unused
+	/* 7th gen */
+	AST2600 = __AST_CHIP(7, 0),
+	AST2620 = __AST_CHIP(7, 1), // unused
 };
 
+#define __AST_CHIP_GEN(__chip)	(((unsigned long)(__chip)) >> 16)
+
 enum ast_tx_chip {
 	AST_TX_NONE,
 	AST_TX_SIL164,
@@ -220,6 +240,24 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 				     struct pci_dev *pdev,
 				     unsigned long flags);
 
+static inline unsigned long __ast_gen(struct ast_device *ast)
+{
+	return __AST_CHIP_GEN(ast->chip);
+}
+#define AST_GEN(__ast)	__ast_gen(__ast)
+
+static inline bool __ast_is_gen(struct ast_device *ast, unsigned long gen)
+{
+	return __ast_gen(ast) == gen;
+}
+#define IS_AST_GEN1(__ast)	__ast_is_gen(__ast, 1)
+#define IS_AST_GEN2(__ast)	__ast_is_gen(__ast, 2)
+#define IS_AST_GEN3(__ast)	__ast_is_gen(__ast, 3)
+#define IS_AST_GEN4(__ast)	__ast_is_gen(__ast, 4)
+#define IS_AST_GEN5(__ast)	__ast_is_gen(__ast, 5)
+#define IS_AST_GEN6(__ast)	__ast_is_gen(__ast, 6)
+#define IS_AST_GEN7(__ast)	__ast_is_gen(__ast, 7)
+
 #define AST_IO_AR_PORT_WRITE		(0x40)
 #define AST_IO_MISC_PORT_WRITE		(0x42)
 #define AST_IO_VGA_ENABLE_PORT		(0x43)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 6ff4b837e64d7..3cd94a74150bf 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -128,7 +128,7 @@ static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
 	jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
 	jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
 	if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
-		/* Patch AST2500 */
+		/* Patch GEN6 */
 		if (((pdev->revision & 0xF0) == 0x40)
 			&& ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
 			ast_patch_ahb_2500(ast);
@@ -197,8 +197,8 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 	}
 
 	/* Check if we support wide screen */
-	switch (ast->chip) {
-	case AST2000:
+	switch (AST_GEN(ast)) {
+	case 1:
 		ast->support_wide_screen = false;
 		break;
 	default:
@@ -218,7 +218,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 			if (ast->chip == AST2500 &&
 			    scu_rev == 0x100)           /* ast2510 */
 				ast->support_wide_screen = true;
-			if (ast->chip == AST2600)		/* ast2600 */
+			if (IS_AST_GEN7(ast))
 				ast->support_wide_screen = true;
 		}
 		break;
@@ -241,9 +241,9 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 			ast->tx_chip_types = AST_TX_SIL164_BIT;
 	}
 
-	if ((ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500)) {
+	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast)) {
 		/*
-		 * On AST2300 and 2400, look the configuration set by the SoC in
+		 * On AST GEN4+, look the configuration set by the SoC in
 		 * the SOC scratch register #1 bits 11:8 (interestingly marked
 		 * as "reserved" in the spec)
 		 */
@@ -265,7 +265,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		case 0x0c:
 			ast->tx_chip_types = AST_TX_DP501_BIT;
 		}
-	} else if (ast->chip == AST2600) {
+	} else if (IS_AST_GEN7(ast)) {
 		if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, TX_TYPE_MASK) ==
 		    ASTDP_DPMCU_TX) {
 			ast->tx_chip_types = AST_TX_ASTDP_BIT;
@@ -297,7 +297,7 @@ static int ast_get_dram_info(struct drm_device *dev)
 	case ast_use_dt:
 		/*
 		 * If some properties are missing, use reasonable
-		 * defaults for AST2400
+		 * defaults for GEN5
 		 */
 		if (of_property_read_u32(np, "aspeed,mcr-configuration",
 					 &mcr_cfg))
@@ -320,7 +320,7 @@ static int ast_get_dram_info(struct drm_device *dev)
 	default:
 		ast->dram_bus_width = 16;
 		ast->dram_type = AST_DRAM_1Gx16;
-		if (ast->chip == AST2500)
+		if (IS_AST_GEN6(ast))
 			ast->mclk = 800;
 		else
 			ast->mclk = 396;
@@ -332,7 +332,7 @@ static int ast_get_dram_info(struct drm_device *dev)
 	else
 		ast->dram_bus_width = 32;
 
-	if (ast->chip == AST2500) {
+	if (IS_AST_GEN6(ast)) {
 		switch (mcr_cfg & 0x03) {
 		case 0:
 			ast->dram_type = AST_DRAM_1Gx16;
@@ -348,7 +348,7 @@ static int ast_get_dram_info(struct drm_device *dev)
 			ast->dram_type = AST_DRAM_8Gx16;
 			break;
 		}
-	} else if (ast->chip == AST2300 || ast->chip == AST2400) {
+	} else if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
 		switch (mcr_cfg & 0x03) {
 		case 0:
 			ast->dram_type = AST_DRAM_512Mx16;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index b3c670af6ef2b..f711d592da52b 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -342,7 +342,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
 	u8 jreg05 = 0, jreg07 = 0, jreg09 = 0, jregAC = 0, jregAD = 0, jregAE = 0;
 	u16 temp, precache = 0;
 
-	if ((ast->chip == AST2500 || ast->chip == AST2600) &&
+	if ((IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) &&
 	    (vbios_mode->enh_table->flags & AST2500PreCatchCRT))
 		precache = 40;
 
@@ -384,7 +384,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xAD, 0x00, jregAD);
 
 	// Workaround for HSync Time non octave pixels (1920x1080@60Hz HSync 44 pixels);
-	if ((ast->chip == AST2600) && (mode->crtc_vdisplay == 1080))
+	if (IS_AST_GEN7(ast) && (mode->crtc_vdisplay == 1080))
 		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 0x02);
 	else
 		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 0x00);
@@ -466,7 +466,7 @@ static void ast_set_dclk_reg(struct ast_device *ast,
 {
 	const struct ast_vbios_dclk_info *clk_info;
 
-	if ((ast->chip == AST2500) || (ast->chip == AST2600))
+	if (IS_AST_GEN6(ast) || IS_AST_GEN7(ast))
 		clk_info = &dclk_table_ast2500[vbios_mode->enh_table->dclk_index];
 	else
 		clk_info = &dclk_table[vbios_mode->enh_table->dclk_index];
@@ -510,17 +510,13 @@ static void ast_set_color_reg(struct ast_device *ast,
 static void ast_set_crtthd_reg(struct ast_device *ast)
 {
 	/* Set Threshold */
-	if (ast->chip == AST2600) {
+	if (IS_AST_GEN7(ast)) {
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0xe0);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0xa0);
-	} else if (ast->chip == AST2300 || ast->chip == AST2400 ||
-	    ast->chip == AST2500) {
+	} else if (IS_AST_GEN6(ast) || IS_AST_GEN5(ast) || IS_AST_GEN4(ast)) {
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x78);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x60);
-	} else if (ast->chip == AST2100 ||
-		   ast->chip == AST1100 ||
-		   ast->chip == AST2200 ||
-		   ast->chip == AST2150) {
+	} else if (IS_AST_GEN3(ast) || IS_AST_GEN2(ast)) {
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x3f);
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x2f);
 	} else {
@@ -1082,9 +1078,10 @@ ast_crtc_helper_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
 		if ((mode->hdisplay == 1152) && (mode->vdisplay == 864))
 			return MODE_OK;
 
-		if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
-		    (ast->chip == AST2300) || (ast->chip == AST2400) ||
-		    (ast->chip == AST2500) || (ast->chip == AST2600)) {
+		if ((ast->chip == AST2100) || // GEN2, but not AST1100 (?)
+		    (ast->chip == AST2200) || // GEN3, but not AST2150 (?)
+		    IS_AST_GEN4(ast) || IS_AST_GEN5(ast) ||
+		    IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) {
 			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
 				return MODE_OK;
 
@@ -1800,12 +1797,12 @@ int ast_mode_config_init(struct ast_device *ast)
 	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
 
-	if (ast->chip == AST2100 ||
-	    ast->chip == AST2200 ||
-	    ast->chip == AST2300 ||
-	    ast->chip == AST2400 ||
-	    ast->chip == AST2500 ||
-	    ast->chip == AST2600) {
+	if (ast->chip == AST2100 || // GEN2, but not AST1100 (?)
+	    ast->chip == AST2200 || // GEN3, but not AST2150 (?)
+	    IS_AST_GEN7(ast) ||
+	    IS_AST_GEN6(ast) ||
+	    IS_AST_GEN5(ast) ||
+	    IS_AST_GEN4(ast)) {
 		dev->mode_config.max_width = 1920;
 		dev->mode_config.max_height = 2048;
 	} else {
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index b765eeb55e5f1..13e15173f2c5b 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -51,7 +51,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
 	for (i = 0x81; i <= 0x9f; i++)
 		ast_set_index_reg(ast, AST_IO_CRTC_PORT, i, 0x00);
 
-	if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip == AST2500)
+	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
 		ext_reg_info = extreginfo_ast2300;
 	else
 		ext_reg_info = extreginfo;
@@ -72,8 +72,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
 
 	/* Enable RAMDAC for A1 */
 	reg = 0x04;
-	if (ast->chip == AST2300 || ast->chip == AST2400 ||
-	    ast->chip == AST2500)
+	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
 		reg |= 0x20;
 	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xff, reg);
 }
@@ -249,7 +248,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
 	j = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
 
 	if ((j & 0x80) == 0) { /* VGA only */
-		if (ast->chip == AST2000) {
+		if (IS_AST_GEN1(ast)) {
 			dram_reg_info = ast2000_dram_table_data;
 			ast_write32(ast, 0xf004, 0x1e6e0000);
 			ast_write32(ast, 0xf000, 0x1);
@@ -258,7 +257,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
 			do {
 				;
 			} while (ast_read32(ast, 0x10100) != 0xa8);
-		} else {/* AST2100/1100 */
+		} else { /* GEN2/GEN3 */
 			if (ast->chip == AST2100 || ast->chip == AST2200)
 				dram_reg_info = ast2100_dram_table_data;
 			else
@@ -281,7 +280,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
 			if (dram_reg_info->index == 0xff00) {/* delay fn */
 				for (i = 0; i < 15; i++)
 					udelay(dram_reg_info->data);
-			} else if (dram_reg_info->index == 0x4 && ast->chip != AST2000) {
+			} else if (dram_reg_info->index == 0x4 && !IS_AST_GEN1(ast)) {
 				data = dram_reg_info->data;
 				if (ast->dram_type == AST_DRAM_1Gx16)
 					data = 0x00000d89;
@@ -307,15 +306,13 @@ static void ast_init_dram_reg(struct drm_device *dev)
 				cbrdlli_ast2150(ast, 32); /* 32 bits */
 		}
 
-		switch (ast->chip) {
-		case AST2000:
+		switch (AST_GEN(ast)) {
+		case 1:
 			temp = ast_read32(ast, 0x10140);
 			ast_write32(ast, 0x10140, temp | 0x40);
 			break;
-		case AST1100:
-		case AST2100:
-		case AST2200:
-		case AST2150:
+		case 2:
+		case 3:
 			temp = ast_read32(ast, 0x1200c);
 			ast_write32(ast, 0x1200c, temp & 0xfffffffd);
 			temp = ast_read32(ast, 0x12040);
@@ -338,13 +335,13 @@ void ast_post_gpu(struct drm_device *dev)
 
 	ast_set_def_ext_reg(dev);
 
-	if (ast->chip == AST2600) {
+	if (IS_AST_GEN7(ast)) {
 		if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
 			ast_dp_launch(dev);
 	} else if (ast->config_mode == ast_use_p2a) {
-		if (ast->chip == AST2500)
+		if (IS_AST_GEN6(ast))
 			ast_post_chip_2500(dev);
-		else if (ast->chip == AST2300 || ast->chip == AST2400)
+		else if (IS_AST_GEN5(ast) || IS_AST_GEN4(ast))
 			ast_post_chip_2300(dev);
 		else
 			ast_init_dram_reg(dev);
-- 
2.41.0

Re: [09/14] drm/ast: Distinguish among chip generations

[Sui Jingfeng]

Hi,

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> ASpeed distinguishes among various generations of the AST graphics
> chipset with various models. [1] The most-recent model AST 2600 is
> of the 7th generation, the AST 2500 is of the 6th generation, and so
> on.
>
> The ast driver simply picks one of the models as representative for
> the whole generation. In several places, individual models of the
> same generation need to be handled differently, which then requires
> additional code for detecting the model.
>
> Introduce different generations of the Aspeed chipset. In the source
> code, refer to the generation instead of the representation model where
> possible. The few places that require per-model handling are now clearly
> marked.
>
> In the enum ast_chip, we arrange each model's value such that it
> encodes the generation. This allows for an easy test. The actual values
> are ordered, but not of interest to the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://web.archive.org/web/20141007093258/http://www.aspeedtech.com/products.php?fPath=20 # 1
> ---
>   drivers/gpu/drm/ast/ast_dp501.c |  6 ++--
>   drivers/gpu/drm/ast/ast_drv.h   | 56 +++++++++++++++++++++++++++------
>   drivers/gpu/drm/ast/ast_main.c  | 22 ++++++-------
>   drivers/gpu/drm/ast/ast_mode.c  | 35 ++++++++++-----------
>   drivers/gpu/drm/ast/ast_post.c  | 27 +++++++---------
>   5 files changed, 89 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index 1bc35a992369d..a5d285850ffb1 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -350,7 +350,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>   		data |= 0x00000500;
>   		ast_write32(ast, 0x12008, data);
>   
> -		if (ast->chip == AST2300) {
> +		if (IS_AST_GEN4(ast)) {
>   			data = ast_read32(ast, 0x12084);
>   			/* multi-pins for DVO single-edge */
>   			data |= 0xfffe0000;
> @@ -366,7 +366,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>   			data &= 0xffffffcf;
>   			data |= 0x00000020;
>   			ast_write32(ast, 0x12090, data);
> -		} else { /* AST2400 */
> +		} else { /* AST GEN5+ */
>   			data = ast_read32(ast, 0x12088);
>   			/* multi-pins for DVO single-edge */
>   			data |= 0x30000000;
> @@ -437,7 +437,7 @@ void ast_init_3rdtx(struct drm_device *dev)
>   	struct ast_device *ast = to_ast_device(dev);
>   	u8 jreg;
>   
> -	if (ast->chip == AST2300 || ast->chip == AST2400) {
> +	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>   		jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
>   		switch (jreg & 0x0e) {
>   		case 0x04:
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index c42dfb86e040d..c209d7e4e4194 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -55,18 +55,38 @@
>   #define AST_PCI_OPTION_MEM_ACCESS_ENABLE	BIT(1)
>   #define AST_PCI_OPTION_IO_ACCESS_ENABLE		BIT(0)
>   
> +#define __AST_CHIP(__gen, __index)	((__gen) << 16 | (__index))
> +
>   enum ast_chip {
> -	AST2000,
> -	AST2100,
> -	AST1100,
> -	AST2200,
> -	AST2150,
> -	AST2300,
> -	AST2400,
> -	AST2500,
> -	AST2600,
> +	/* 1st gen */
> +	AST1000 = __AST_CHIP(1, 0), // unused
> +	AST2000 = __AST_CHIP(1, 1),
> +	/* 2nd gen */
> +	AST1100 = __AST_CHIP(2, 0),
> +	AST2100 = __AST_CHIP(2, 1),
> +	AST2050 = __AST_CHIP(2, 2), // unused
> +	/* 3rd gen */
> +	AST2200 = __AST_CHIP(3, 0),
> +	AST2150 = __AST_CHIP(3, 1),
> +	/* 4th gen */
> +	AST2300 = __AST_CHIP(4, 0),
> +	AST1300 = __AST_CHIP(4, 1), // unused
> +	AST1050 = __AST_CHIP(4, 2), // unused
> +	/* 5th gen */
> +	AST2400 = __AST_CHIP(5, 0),
> +	AST1400 = __AST_CHIP(5, 1), // unused
> +	AST1250 = __AST_CHIP(5, 2), // unused
> +	/* 6th gen */
> +	AST2500 = __AST_CHIP(6, 0),
> +	AST2510 = __AST_CHIP(6, 1), // unused
> +	AST2520 = __AST_CHIP(6, 2), // unused
> +	/* 7th gen */
> +	AST2600 = __AST_CHIP(7, 0),
> +	AST2620 = __AST_CHIP(7, 1), // unused
>   };
>   
> +#define __AST_CHIP_GEN(__chip)	(((unsigned long)(__chip)) >> 16)
> +
>   enum ast_tx_chip {
>   	AST_TX_NONE,
>   	AST_TX_SIL164,
> @@ -220,6 +240,24 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   				     struct pci_dev *pdev,
>   				     unsigned long flags);
>   
> +static inline unsigned long __ast_gen(struct ast_device *ast)
> +{
> +	return __AST_CHIP_GEN(ast->chip);
> +}
> +#define AST_GEN(__ast)	__ast_gen(__ast)
> +
> +static inline bool __ast_is_gen(struct ast_device *ast, unsigned long gen)
> +{
> +	return __ast_gen(ast) == gen;
> +}

Changed to __ast_gen_is_equal() ?


> +#define IS_AST_GEN1(__ast)	__ast_is_gen(__ast, 1)
> +#define IS_AST_GEN2(__ast)	__ast_is_gen(__ast, 2)
> +#define IS_AST_GEN3(__ast)	__ast_is_gen(__ast, 3)
> +#define IS_AST_GEN4(__ast)	__ast_is_gen(__ast, 4)
> +#define IS_AST_GEN5(__ast)	__ast_is_gen(__ast, 5)
> +#define IS_AST_GEN6(__ast)	__ast_is_gen(__ast, 6)
> +#define IS_AST_GEN7(__ast)	__ast_is_gen(__ast, 7)
> +
>   #define AST_IO_AR_PORT_WRITE		(0x40)
>   #define AST_IO_MISC_PORT_WRITE		(0x42)
>   #define AST_IO_VGA_ENABLE_PORT		(0x43)
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 6ff4b837e64d7..3cd94a74150bf 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -128,7 +128,7 @@ static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>   	jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>   	jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
>   	if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
> -		/* Patch AST2500 */
> +		/* Patch GEN6 */
>   		if (((pdev->revision & 0xF0) == 0x40)
>   			&& ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
>   			ast_patch_ahb_2500(ast);
> @@ -197,8 +197,8 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   	}
>   
>   	/* Check if we support wide screen */
> -	switch (ast->chip) {
> -	case AST2000:
> +	switch (AST_GEN(ast)) {
> +	case 1:
>   		ast->support_wide_screen = false;
>   		break;
>   	default:
> @@ -218,7 +218,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   			if (ast->chip == AST2500 &&
>   			    scu_rev == 0x100)           /* ast2510 */
>   				ast->support_wide_screen = true;
> -			if (ast->chip == AST2600)		/* ast2600 */
> +			if (IS_AST_GEN7(ast))
>   				ast->support_wide_screen = true;
>   		}
>   		break;
> @@ -241,9 +241,9 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   			ast->tx_chip_types = AST_TX_SIL164_BIT;
>   	}
>   
> -	if ((ast->chip == AST2300) || (ast->chip == AST2400) || (ast->chip == AST2500)) {
> +	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast)) {
>   		/*
> -		 * On AST2300 and 2400, look the configuration set by the SoC in
> +		 * On AST GEN4+, look the configuration set by the SoC in
>   		 * the SOC scratch register #1 bits 11:8 (interestingly marked
>   		 * as "reserved" in the spec)
>   		 */
> @@ -265,7 +265,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		case 0x0c:
>   			ast->tx_chip_types = AST_TX_DP501_BIT;
>   		}
> -	} else if (ast->chip == AST2600) {
> +	} else if (IS_AST_GEN7(ast)) {
>   		if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, TX_TYPE_MASK) ==
>   		    ASTDP_DPMCU_TX) {
>   			ast->tx_chip_types = AST_TX_ASTDP_BIT;
> @@ -297,7 +297,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>   	case ast_use_dt:
>   		/*
>   		 * If some properties are missing, use reasonable
> -		 * defaults for AST2400
> +		 * defaults for GEN5
>   		 */
>   		if (of_property_read_u32(np, "aspeed,mcr-configuration",
>   					 &mcr_cfg))
> @@ -320,7 +320,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>   	default:
>   		ast->dram_bus_width = 16;
>   		ast->dram_type = AST_DRAM_1Gx16;
> -		if (ast->chip == AST2500)
> +		if (IS_AST_GEN6(ast))
>   			ast->mclk = 800;
>   		else
>   			ast->mclk = 396;
> @@ -332,7 +332,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>   	else
>   		ast->dram_bus_width = 32;
>   
> -	if (ast->chip == AST2500) {
> +	if (IS_AST_GEN6(ast)) {
>   		switch (mcr_cfg & 0x03) {
>   		case 0:
>   			ast->dram_type = AST_DRAM_1Gx16;
> @@ -348,7 +348,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>   			ast->dram_type = AST_DRAM_8Gx16;
>   			break;
>   		}
> -	} else if (ast->chip == AST2300 || ast->chip == AST2400) {
> +	} else if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>   		switch (mcr_cfg & 0x03) {
>   		case 0:
>   			ast->dram_type = AST_DRAM_512Mx16;
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index b3c670af6ef2b..f711d592da52b 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -342,7 +342,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
>   	u8 jreg05 = 0, jreg07 = 0, jreg09 = 0, jregAC = 0, jregAD = 0, jregAE = 0;
>   	u16 temp, precache = 0;
>   
> -	if ((ast->chip == AST2500 || ast->chip == AST2600) &&
> +	if ((IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) &&
>   	    (vbios_mode->enh_table->flags & AST2500PreCatchCRT))
>   		precache = 40;
>   
> @@ -384,7 +384,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
>   	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xAD, 0x00, jregAD);
>   
>   	// Workaround for HSync Time non octave pixels (1920x1080@60Hz HSync 44 pixels);
> -	if ((ast->chip == AST2600) && (mode->crtc_vdisplay == 1080))
> +	if (IS_AST_GEN7(ast) && (mode->crtc_vdisplay == 1080))
>   		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 0x02);
>   	else
>   		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 0x00);
> @@ -466,7 +466,7 @@ static void ast_set_dclk_reg(struct ast_device *ast,
>   {
>   	const struct ast_vbios_dclk_info *clk_info;
>   
> -	if ((ast->chip == AST2500) || (ast->chip == AST2600))
> +	if (IS_AST_GEN6(ast) || IS_AST_GEN7(ast))
>   		clk_info = &dclk_table_ast2500[vbios_mode->enh_table->dclk_index];
>   	else
>   		clk_info = &dclk_table[vbios_mode->enh_table->dclk_index];
> @@ -510,17 +510,13 @@ static void ast_set_color_reg(struct ast_device *ast,
>   static void ast_set_crtthd_reg(struct ast_device *ast)
>   {
>   	/* Set Threshold */
> -	if (ast->chip == AST2600) {
> +	if (IS_AST_GEN7(ast)) {
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0xe0);
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0xa0);
> -	} else if (ast->chip == AST2300 || ast->chip == AST2400 ||
> -	    ast->chip == AST2500) {
> +	} else if (IS_AST_GEN6(ast) || IS_AST_GEN5(ast) || IS_AST_GEN4(ast)) {
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x78);
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x60);
> -	} else if (ast->chip == AST2100 ||
> -		   ast->chip == AST1100 ||
> -		   ast->chip == AST2200 ||
> -		   ast->chip == AST2150) {
> +	} else if (IS_AST_GEN3(ast) || IS_AST_GEN2(ast)) {
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x3f);
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x2f);
>   	} else {
> @@ -1082,9 +1078,10 @@ ast_crtc_helper_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
>   		if ((mode->hdisplay == 1152) && (mode->vdisplay == 864))
>   			return MODE_OK;
>   
> -		if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
> -		    (ast->chip == AST2300) || (ast->chip == AST2400) ||
> -		    (ast->chip == AST2500) || (ast->chip == AST2600)) {
> +		if ((ast->chip == AST2100) || // GEN2, but not AST1100 (?)
> +		    (ast->chip == AST2200) || // GEN3, but not AST2150 (?)

If chips from the same generation is not compatible, then this actually 
introduce confusion.

It's not your patch is bad, it is the naming and/or the hardware is bad.

On such a situation, the patch is not good enough to suppress problems 
incurred by the hardware versions.

It is not clean and It still a tangled implement. But I'm fine if you 
want to see the limitation.

> +		    IS_AST_GEN4(ast) || IS_AST_GEN5(ast) ||
> +		    IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) {
>   			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
>   				return MODE_OK;
>   
> @@ -1800,12 +1797,12 @@ int ast_mode_config_init(struct ast_device *ast)
>   	dev->mode_config.min_height = 0;
>   	dev->mode_config.preferred_depth = 24;
>   
> -	if (ast->chip == AST2100 ||
> -	    ast->chip == AST2200 ||
> -	    ast->chip == AST2300 ||
> -	    ast->chip == AST2400 ||
> -	    ast->chip == AST2500 ||
> -	    ast->chip == AST2600) {
> +	if (ast->chip == AST2100 || // GEN2, but not AST1100 (?)
> +	    ast->chip == AST2200 || // GEN3, but not AST2150 (?)
> +	    IS_AST_GEN7(ast) ||
> +	    IS_AST_GEN6(ast) ||
> +	    IS_AST_GEN5(ast) ||
> +	    IS_AST_GEN4(ast)) {
>   		dev->mode_config.max_width = 1920;
>   		dev->mode_config.max_height = 2048;
>   	} else {
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index b765eeb55e5f1..13e15173f2c5b 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -51,7 +51,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>   	for (i = 0x81; i <= 0x9f; i++)
>   		ast_set_index_reg(ast, AST_IO_CRTC_PORT, i, 0x00);
>   
> -	if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip == AST2500)
> +	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>   		ext_reg_info = extreginfo_ast2300;
>   	else
>   		ext_reg_info = extreginfo;
> @@ -72,8 +72,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>   
>   	/* Enable RAMDAC for A1 */
>   	reg = 0x04;
> -	if (ast->chip == AST2300 || ast->chip == AST2400 ||
> -	    ast->chip == AST2500)
> +	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>   		reg |= 0x20;
>   	ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xff, reg);
>   }
> @@ -249,7 +248,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   	j = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>   
>   	if ((j & 0x80) == 0) { /* VGA only */
> -		if (ast->chip == AST2000) {
> +		if (IS_AST_GEN1(ast)) {
>   			dram_reg_info = ast2000_dram_table_data;
>   			ast_write32(ast, 0xf004, 0x1e6e0000);
>   			ast_write32(ast, 0xf000, 0x1);
> @@ -258,7 +257,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   			do {
>   				;
>   			} while (ast_read32(ast, 0x10100) != 0xa8);
> -		} else {/* AST2100/1100 */
> +		} else { /* GEN2/GEN3 */
>   			if (ast->chip == AST2100 || ast->chip == AST2200)
>   				dram_reg_info = ast2100_dram_table_data;
>   			else
> @@ -281,7 +280,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   			if (dram_reg_info->index == 0xff00) {/* delay fn */
>   				for (i = 0; i < 15; i++)
>   					udelay(dram_reg_info->data);
> -			} else if (dram_reg_info->index == 0x4 && ast->chip != AST2000) {
> +			} else if (dram_reg_info->index == 0x4 && !IS_AST_GEN1(ast)) {
>   				data = dram_reg_info->data;
>   				if (ast->dram_type == AST_DRAM_1Gx16)
>   					data = 0x00000d89;
> @@ -307,15 +306,13 @@ static void ast_init_dram_reg(struct drm_device *dev)
>   				cbrdlli_ast2150(ast, 32); /* 32 bits */
>   		}
>   
> -		switch (ast->chip) {
> -		case AST2000:
> +		switch (AST_GEN(ast)) {
> +		case 1:
>   			temp = ast_read32(ast, 0x10140);
>   			ast_write32(ast, 0x10140, temp | 0x40);
>   			break;
> -		case AST1100:
> -		case AST2100:
> -		case AST2200:
> -		case AST2150:
> +		case 2:
> +		case 3:
>   			temp = ast_read32(ast, 0x1200c);
>   			ast_write32(ast, 0x1200c, temp & 0xfffffffd);
>   			temp = ast_read32(ast, 0x12040);
> @@ -338,13 +335,13 @@ void ast_post_gpu(struct drm_device *dev)
>   
>   	ast_set_def_ext_reg(dev);
>   
> -	if (ast->chip == AST2600) {
> +	if (IS_AST_GEN7(ast)) {
>   		if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>   			ast_dp_launch(dev);
>   	} else if (ast->config_mode == ast_use_p2a) {
> -		if (ast->chip == AST2500)
> +		if (IS_AST_GEN6(ast))
>   			ast_post_chip_2500(dev);
> -		else if (ast->chip == AST2300 || ast->chip == AST2400)
> +		else if (IS_AST_GEN5(ast) || IS_AST_GEN4(ast))
>   			ast_post_chip_2300(dev);
>   		else
>   			ast_init_dram_reg(dev);

-- 
Jingfeng

Re: [09/14] drm/ast: Distinguish among chip generations

[Thomas Zimmermann]

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

Hi

Am 17.06.23 um 10:35 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>> ASpeed distinguishes among various generations of the AST graphics
>> chipset with various models. [1] The most-recent model AST 2600 is
>> of the 7th generation, the AST 2500 is of the 6th generation, and so
>> on.
>>
>> The ast driver simply picks one of the models as representative for
>> the whole generation. In several places, individual models of the
>> same generation need to be handled differently, which then requires
>> additional code for detecting the model.
>>
>> Introduce different generations of the Aspeed chipset. In the source
>> code, refer to the generation instead of the representation model where
>> possible. The few places that require per-model handling are now clearly
>> marked.
>>
>> In the enum ast_chip, we arrange each model's value such that it
>> encodes the generation. This allows for an easy test. The actual values
>> are ordered, but not of interest to the driver.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Link: 
>> https://web.archive.org/web/20141007093258/http://www.aspeedtech.com/products.php?fPath=20 # 1
>> ---
>>   drivers/gpu/drm/ast/ast_dp501.c |  6 ++--
>>   drivers/gpu/drm/ast/ast_drv.h   | 56 +++++++++++++++++++++++++++------
>>   drivers/gpu/drm/ast/ast_main.c  | 22 ++++++-------
>>   drivers/gpu/drm/ast/ast_mode.c  | 35 ++++++++++-----------
>>   drivers/gpu/drm/ast/ast_post.c  | 27 +++++++---------
>>   5 files changed, 89 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c 
>> b/drivers/gpu/drm/ast/ast_dp501.c
>> index 1bc35a992369d..a5d285850ffb1 100644
>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>> @@ -350,7 +350,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>>           data |= 0x00000500;
>>           ast_write32(ast, 0x12008, data);
>> -        if (ast->chip == AST2300) {
>> +        if (IS_AST_GEN4(ast)) {
>>               data = ast_read32(ast, 0x12084);
>>               /* multi-pins for DVO single-edge */
>>               data |= 0xfffe0000;
>> @@ -366,7 +366,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>>               data &= 0xffffffcf;
>>               data |= 0x00000020;
>>               ast_write32(ast, 0x12090, data);
>> -        } else { /* AST2400 */
>> +        } else { /* AST GEN5+ */
>>               data = ast_read32(ast, 0x12088);
>>               /* multi-pins for DVO single-edge */
>>               data |= 0x30000000;
>> @@ -437,7 +437,7 @@ void ast_init_3rdtx(struct drm_device *dev)
>>       struct ast_device *ast = to_ast_device(dev);
>>       u8 jreg;
>> -    if (ast->chip == AST2300 || ast->chip == AST2400) {
>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>>           jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 
>> 0xff);
>>           switch (jreg & 0x0e) {
>>           case 0x04:
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index c42dfb86e040d..c209d7e4e4194 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -55,18 +55,38 @@
>>   #define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>>   #define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>> +#define __AST_CHIP(__gen, __index)    ((__gen) << 16 | (__index))
>> +
>>   enum ast_chip {
>> -    AST2000,
>> -    AST2100,
>> -    AST1100,
>> -    AST2200,
>> -    AST2150,
>> -    AST2300,
>> -    AST2400,
>> -    AST2500,
>> -    AST2600,
>> +    /* 1st gen */
>> +    AST1000 = __AST_CHIP(1, 0), // unused
>> +    AST2000 = __AST_CHIP(1, 1),
>> +    /* 2nd gen */
>> +    AST1100 = __AST_CHIP(2, 0),
>> +    AST2100 = __AST_CHIP(2, 1),
>> +    AST2050 = __AST_CHIP(2, 2), // unused
>> +    /* 3rd gen */
>> +    AST2200 = __AST_CHIP(3, 0),
>> +    AST2150 = __AST_CHIP(3, 1),
>> +    /* 4th gen */
>> +    AST2300 = __AST_CHIP(4, 0),
>> +    AST1300 = __AST_CHIP(4, 1), // unused
>> +    AST1050 = __AST_CHIP(4, 2), // unused
>> +    /* 5th gen */
>> +    AST2400 = __AST_CHIP(5, 0),
>> +    AST1400 = __AST_CHIP(5, 1), // unused
>> +    AST1250 = __AST_CHIP(5, 2), // unused
>> +    /* 6th gen */
>> +    AST2500 = __AST_CHIP(6, 0),
>> +    AST2510 = __AST_CHIP(6, 1), // unused
>> +    AST2520 = __AST_CHIP(6, 2), // unused
>> +    /* 7th gen */
>> +    AST2600 = __AST_CHIP(7, 0),
>> +    AST2620 = __AST_CHIP(7, 1), // unused
>>   };
>> +#define __AST_CHIP_GEN(__chip)    (((unsigned long)(__chip)) >> 16)
>> +
>>   enum ast_tx_chip {
>>       AST_TX_NONE,
>>       AST_TX_SIL164,
>> @@ -220,6 +240,24 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>                        struct pci_dev *pdev,
>>                        unsigned long flags);
>> +static inline unsigned long __ast_gen(struct ast_device *ast)
>> +{
>> +    return __AST_CHIP_GEN(ast->chip);
>> +}
>> +#define AST_GEN(__ast)    __ast_gen(__ast)
>> +
>> +static inline bool __ast_is_gen(struct ast_device *ast, unsigned long 
>> gen)
>> +{
>> +    return __ast_gen(ast) == gen;
>> +}
> 
> Changed to __ast_gen_is_equal() ?

Makes sense.

> 
> 
>> +#define IS_AST_GEN1(__ast)    __ast_is_gen(__ast, 1)
>> +#define IS_AST_GEN2(__ast)    __ast_is_gen(__ast, 2)
>> +#define IS_AST_GEN3(__ast)    __ast_is_gen(__ast, 3)
>> +#define IS_AST_GEN4(__ast)    __ast_is_gen(__ast, 4)
>> +#define IS_AST_GEN5(__ast)    __ast_is_gen(__ast, 5)
>> +#define IS_AST_GEN6(__ast)    __ast_is_gen(__ast, 6)
>> +#define IS_AST_GEN7(__ast)    __ast_is_gen(__ast, 7)
>> +
>>   #define AST_IO_AR_PORT_WRITE        (0x40)
>>   #define AST_IO_MISC_PORT_WRITE        (0x42)
>>   #define AST_IO_VGA_ENABLE_PORT        (0x43)
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index 6ff4b837e64d7..3cd94a74150bf 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -128,7 +128,7 @@ static void ast_detect_config_mode(struct 
>> drm_device *dev, u32 *scu_rev)
>>       jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>>       jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
>>       if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>> -        /* Patch AST2500 */
>> +        /* Patch GEN6 */
>>           if (((pdev->revision & 0xF0) == 0x40)
>>               && ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
>>               ast_patch_ahb_2500(ast);
>> @@ -197,8 +197,8 @@ static int ast_detect_chip(struct drm_device *dev, 
>> bool need_post, u32 scu_rev)
>>       }
>>       /* Check if we support wide screen */
>> -    switch (ast->chip) {
>> -    case AST2000:
>> +    switch (AST_GEN(ast)) {
>> +    case 1:
>>           ast->support_wide_screen = false;
>>           break;
>>       default:
>> @@ -218,7 +218,7 @@ static int ast_detect_chip(struct drm_device *dev, 
>> bool need_post, u32 scu_rev)
>>               if (ast->chip == AST2500 &&
>>                   scu_rev == 0x100)           /* ast2510 */
>>                   ast->support_wide_screen = true;
>> -            if (ast->chip == AST2600)        /* ast2600 */
>> +            if (IS_AST_GEN7(ast))
>>                   ast->support_wide_screen = true;
>>           }
>>           break;
>> @@ -241,9 +241,9 @@ static int ast_detect_chip(struct drm_device *dev, 
>> bool need_post, u32 scu_rev)
>>               ast->tx_chip_types = AST_TX_SIL164_BIT;
>>       }
>> -    if ((ast->chip == AST2300) || (ast->chip == AST2400) || 
>> (ast->chip == AST2500)) {
>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast)) {
>>           /*
>> -         * On AST2300 and 2400, look the configuration set by the SoC in
>> +         * On AST GEN4+, look the configuration set by the SoC in
>>            * the SOC scratch register #1 bits 11:8 (interestingly marked
>>            * as "reserved" in the spec)
>>            */
>> @@ -265,7 +265,7 @@ static int ast_detect_chip(struct drm_device *dev, 
>> bool need_post, u32 scu_rev)
>>           case 0x0c:
>>               ast->tx_chip_types = AST_TX_DP501_BIT;
>>           }
>> -    } else if (ast->chip == AST2600) {
>> +    } else if (IS_AST_GEN7(ast)) {
>>           if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, 
>> TX_TYPE_MASK) ==
>>               ASTDP_DPMCU_TX) {
>>               ast->tx_chip_types = AST_TX_ASTDP_BIT;
>> @@ -297,7 +297,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>>       case ast_use_dt:
>>           /*
>>            * If some properties are missing, use reasonable
>> -         * defaults for AST2400
>> +         * defaults for GEN5
>>            */
>>           if (of_property_read_u32(np, "aspeed,mcr-configuration",
>>                        &mcr_cfg))
>> @@ -320,7 +320,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>>       default:
>>           ast->dram_bus_width = 16;
>>           ast->dram_type = AST_DRAM_1Gx16;
>> -        if (ast->chip == AST2500)
>> +        if (IS_AST_GEN6(ast))
>>               ast->mclk = 800;
>>           else
>>               ast->mclk = 396;
>> @@ -332,7 +332,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>>       else
>>           ast->dram_bus_width = 32;
>> -    if (ast->chip == AST2500) {
>> +    if (IS_AST_GEN6(ast)) {
>>           switch (mcr_cfg & 0x03) {
>>           case 0:
>>               ast->dram_type = AST_DRAM_1Gx16;
>> @@ -348,7 +348,7 @@ static int ast_get_dram_info(struct drm_device *dev)
>>               ast->dram_type = AST_DRAM_8Gx16;
>>               break;
>>           }
>> -    } else if (ast->chip == AST2300 || ast->chip == AST2400) {
>> +    } else if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>>           switch (mcr_cfg & 0x03) {
>>           case 0:
>>               ast->dram_type = AST_DRAM_512Mx16;
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index b3c670af6ef2b..f711d592da52b 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -342,7 +342,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
>>       u8 jreg05 = 0, jreg07 = 0, jreg09 = 0, jregAC = 0, jregAD = 0, 
>> jregAE = 0;
>>       u16 temp, precache = 0;
>> -    if ((ast->chip == AST2500 || ast->chip == AST2600) &&
>> +    if ((IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) &&
>>           (vbios_mode->enh_table->flags & AST2500PreCatchCRT))
>>           precache = 40;
>> @@ -384,7 +384,7 @@ static void ast_set_crtc_reg(struct ast_device *ast,
>>       ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xAD, 0x00, jregAD);
>>       // Workaround for HSync Time non octave pixels (1920x1080@60Hz 
>> HSync 44 pixels);
>> -    if ((ast->chip == AST2600) && (mode->crtc_vdisplay == 1080))
>> +    if (IS_AST_GEN7(ast) && (mode->crtc_vdisplay == 1080))
>>           ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 
>> 0x02);
>>       else
>>           ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 
>> 0x00);
>> @@ -466,7 +466,7 @@ static void ast_set_dclk_reg(struct ast_device *ast,
>>   {
>>       const struct ast_vbios_dclk_info *clk_info;
>> -    if ((ast->chip == AST2500) || (ast->chip == AST2600))
>> +    if (IS_AST_GEN6(ast) || IS_AST_GEN7(ast))
>>           clk_info = 
>> &dclk_table_ast2500[vbios_mode->enh_table->dclk_index];
>>       else
>>           clk_info = &dclk_table[vbios_mode->enh_table->dclk_index];
>> @@ -510,17 +510,13 @@ static void ast_set_color_reg(struct ast_device 
>> *ast,
>>   static void ast_set_crtthd_reg(struct ast_device *ast)
>>   {
>>       /* Set Threshold */
>> -    if (ast->chip == AST2600) {
>> +    if (IS_AST_GEN7(ast)) {
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0xe0);
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0xa0);
>> -    } else if (ast->chip == AST2300 || ast->chip == AST2400 ||
>> -        ast->chip == AST2500) {
>> +    } else if (IS_AST_GEN6(ast) || IS_AST_GEN5(ast) || 
>> IS_AST_GEN4(ast)) {
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x78);
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x60);
>> -    } else if (ast->chip == AST2100 ||
>> -           ast->chip == AST1100 ||
>> -           ast->chip == AST2200 ||
>> -           ast->chip == AST2150) {
>> +    } else if (IS_AST_GEN3(ast) || IS_AST_GEN2(ast)) {
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x3f);
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x2f);
>>       } else {
>> @@ -1082,9 +1078,10 @@ ast_crtc_helper_mode_valid(struct drm_crtc 
>> *crtc, const struct drm_display_mode
>>           if ((mode->hdisplay == 1152) && (mode->vdisplay == 864))
>>               return MODE_OK;
>> -        if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
>> -            (ast->chip == AST2300) || (ast->chip == AST2400) ||
>> -            (ast->chip == AST2500) || (ast->chip == AST2600)) {
>> +        if ((ast->chip == AST2100) || // GEN2, but not AST1100 (?)
>> +            (ast->chip == AST2200) || // GEN3, but not AST2150 (?)
> 
> If chips from the same generation is not compatible, then this actually 
> introduce confusion.
> 
> It's not your patch is bad, it is the naming and/or the hardware is bad.
> 
> On such a situation, the patch is not good enough to suppress problems 
> incurred by the hardware versions.
> 
> It is not clean and It still a tangled implement. But I'm fine if you 
> want to see the limitation.

The history and feature set of the earlier chips appears convoluted. 
The chip generation is meant for the cases where all chips behave the 
same. I'm considering to create a patch that reorganizes these branches 
to be more readable. But for now, it's just about making the current 
code more understandable.

Best regards
Thomas

> 
>> +            IS_AST_GEN4(ast) || IS_AST_GEN5(ast) ||
>> +            IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) {
>>               if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
>>                   return MODE_OK;
>> @@ -1800,12 +1797,12 @@ int ast_mode_config_init(struct ast_device *ast)
>>       dev->mode_config.min_height = 0;
>>       dev->mode_config.preferred_depth = 24;
>> -    if (ast->chip == AST2100 ||
>> -        ast->chip == AST2200 ||
>> -        ast->chip == AST2300 ||
>> -        ast->chip == AST2400 ||
>> -        ast->chip == AST2500 ||
>> -        ast->chip == AST2600) {
>> +    if (ast->chip == AST2100 || // GEN2, but not AST1100 (?)
>> +        ast->chip == AST2200 || // GEN3, but not AST2150 (?)
>> +        IS_AST_GEN7(ast) ||
>> +        IS_AST_GEN6(ast) ||
>> +        IS_AST_GEN5(ast) ||
>> +        IS_AST_GEN4(ast)) {
>>           dev->mode_config.max_width = 1920;
>>           dev->mode_config.max_height = 2048;
>>       } else {
>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>> b/drivers/gpu/drm/ast/ast_post.c
>> index b765eeb55e5f1..13e15173f2c5b 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -51,7 +51,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>>       for (i = 0x81; i <= 0x9f; i++)
>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, i, 0x00);
>> -    if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip == 
>> AST2500)
>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>>           ext_reg_info = extreginfo_ast2300;
>>       else
>>           ext_reg_info = extreginfo;
>> @@ -72,8 +72,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>>       /* Enable RAMDAC for A1 */
>>       reg = 0x04;
>> -    if (ast->chip == AST2300 || ast->chip == AST2400 ||
>> -        ast->chip == AST2500)
>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>>           reg |= 0x20;
>>       ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xff, reg);
>>   }
>> @@ -249,7 +248,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>>       j = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>>       if ((j & 0x80) == 0) { /* VGA only */
>> -        if (ast->chip == AST2000) {
>> +        if (IS_AST_GEN1(ast)) {
>>               dram_reg_info = ast2000_dram_table_data;
>>               ast_write32(ast, 0xf004, 0x1e6e0000);
>>               ast_write32(ast, 0xf000, 0x1);
>> @@ -258,7 +257,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>>               do {
>>                   ;
>>               } while (ast_read32(ast, 0x10100) != 0xa8);
>> -        } else {/* AST2100/1100 */
>> +        } else { /* GEN2/GEN3 */
>>               if (ast->chip == AST2100 || ast->chip == AST2200)
>>                   dram_reg_info = ast2100_dram_table_data;
>>               else
>> @@ -281,7 +280,7 @@ static void ast_init_dram_reg(struct drm_device *dev)
>>               if (dram_reg_info->index == 0xff00) {/* delay fn */
>>                   for (i = 0; i < 15; i++)
>>                       udelay(dram_reg_info->data);
>> -            } else if (dram_reg_info->index == 0x4 && ast->chip != 
>> AST2000) {
>> +            } else if (dram_reg_info->index == 0x4 && 
>> !IS_AST_GEN1(ast)) {
>>                   data = dram_reg_info->data;
>>                   if (ast->dram_type == AST_DRAM_1Gx16)
>>                       data = 0x00000d89;
>> @@ -307,15 +306,13 @@ static void ast_init_dram_reg(struct drm_device 
>> *dev)
>>                   cbrdlli_ast2150(ast, 32); /* 32 bits */
>>           }
>> -        switch (ast->chip) {
>> -        case AST2000:
>> +        switch (AST_GEN(ast)) {
>> +        case 1:
>>               temp = ast_read32(ast, 0x10140);
>>               ast_write32(ast, 0x10140, temp | 0x40);
>>               break;
>> -        case AST1100:
>> -        case AST2100:
>> -        case AST2200:
>> -        case AST2150:
>> +        case 2:
>> +        case 3:
>>               temp = ast_read32(ast, 0x1200c);
>>               ast_write32(ast, 0x1200c, temp & 0xfffffffd);
>>               temp = ast_read32(ast, 0x12040);
>> @@ -338,13 +335,13 @@ void ast_post_gpu(struct drm_device *dev)
>>       ast_set_def_ext_reg(dev);
>> -    if (ast->chip == AST2600) {
>> +    if (IS_AST_GEN7(ast)) {
>>           if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>>               ast_dp_launch(dev);
>>       } else if (ast->config_mode == ast_use_p2a) {
>> -        if (ast->chip == AST2500)
>> +        if (IS_AST_GEN6(ast))
>>               ast_post_chip_2500(dev);
>> -        else if (ast->chip == AST2300 || ast->chip == AST2400)
>> +        else if (IS_AST_GEN5(ast) || IS_AST_GEN4(ast))
>>               ast_post_chip_2300(dev);
>>           else
>>               ast_init_dram_reg(dev);
> 

-- 
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 --]

Re: [09/14] drm/ast: Distinguish among chip generations

[Sui Jingfeng]

Hi

On 2023/6/19 16:22, Thomas Zimmermann wrote:
> Hi
>
> Am 17.06.23 um 10:35 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>>> ASpeed distinguishes among various generations of the AST graphics
>>> chipset with various models. [1] The most-recent model AST 2600 is
>>> of the 7th generation, the AST 2500 is of the 6th generation, and so
>>> on.
>>>
>>> The ast driver simply picks one of the models as representative for
>>> the whole generation. In several places, individual models of the
>>> same generation need to be handled differently, which then requires
>>> additional code for detecting the model.
>>>
>>> Introduce different generations of the Aspeed chipset. In the source
>>> code, refer to the generation instead of the representation model where
>>> possible. The few places that require per-model handling are now 
>>> clearly
>>> marked.
>>>
>>> In the enum ast_chip, we arrange each model's value such that it
>>> encodes the generation. This allows for an easy test. The actual values
>>> are ordered, but not of interest to the driver.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Link: 
>>> https://web.archive.org/web/20141007093258/http://www.aspeedtech.com/products.php?fPath=20 
>>> # 1
>>> ---
>>>   drivers/gpu/drm/ast/ast_dp501.c |  6 ++--
>>>   drivers/gpu/drm/ast/ast_drv.h   | 56 
>>> +++++++++++++++++++++++++++------
>>>   drivers/gpu/drm/ast/ast_main.c  | 22 ++++++-------
>>>   drivers/gpu/drm/ast/ast_mode.c  | 35 ++++++++++-----------
>>>   drivers/gpu/drm/ast/ast_post.c  | 27 +++++++---------
>>>   5 files changed, 89 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c 
>>> b/drivers/gpu/drm/ast/ast_dp501.c
>>> index 1bc35a992369d..a5d285850ffb1 100644
>>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>>> @@ -350,7 +350,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>>>           data |= 0x00000500;
>>>           ast_write32(ast, 0x12008, data);
>>> -        if (ast->chip == AST2300) {
>>> +        if (IS_AST_GEN4(ast)) {
>>>               data = ast_read32(ast, 0x12084);
>>>               /* multi-pins for DVO single-edge */
>>>               data |= 0xfffe0000;
>>> @@ -366,7 +366,7 @@ static bool ast_init_dvo(struct drm_device *dev)
>>>               data &= 0xffffffcf;
>>>               data |= 0x00000020;
>>>               ast_write32(ast, 0x12090, data);
>>> -        } else { /* AST2400 */
>>> +        } else { /* AST GEN5+ */
>>>               data = ast_read32(ast, 0x12088);
>>>               /* multi-pins for DVO single-edge */
>>>               data |= 0x30000000;
>>> @@ -437,7 +437,7 @@ void ast_init_3rdtx(struct drm_device *dev)
>>>       struct ast_device *ast = to_ast_device(dev);
>>>       u8 jreg;
>>> -    if (ast->chip == AST2300 || ast->chip == AST2400) {
>>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>>>           jreg = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 
>>> 0xff);
>>>           switch (jreg & 0x0e) {
>>>           case 0x04:
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>>> b/drivers/gpu/drm/ast/ast_drv.h
>>> index c42dfb86e040d..c209d7e4e4194 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>> @@ -55,18 +55,38 @@
>>>   #define AST_PCI_OPTION_MEM_ACCESS_ENABLE    BIT(1)
>>>   #define AST_PCI_OPTION_IO_ACCESS_ENABLE        BIT(0)
>>> +#define __AST_CHIP(__gen, __index)    ((__gen) << 16 | (__index))
>>> +
>>>   enum ast_chip {
>>> -    AST2000,
>>> -    AST2100,
>>> -    AST1100,
>>> -    AST2200,
>>> -    AST2150,
>>> -    AST2300,
>>> -    AST2400,
>>> -    AST2500,
>>> -    AST2600,
>>> +    /* 1st gen */
>>> +    AST1000 = __AST_CHIP(1, 0), // unused
>>> +    AST2000 = __AST_CHIP(1, 1),
>>> +    /* 2nd gen */
>>> +    AST1100 = __AST_CHIP(2, 0),
>>> +    AST2100 = __AST_CHIP(2, 1),
>>> +    AST2050 = __AST_CHIP(2, 2), // unused
>>> +    /* 3rd gen */
>>> +    AST2200 = __AST_CHIP(3, 0),
>>> +    AST2150 = __AST_CHIP(3, 1),
>>> +    /* 4th gen */
>>> +    AST2300 = __AST_CHIP(4, 0),
>>> +    AST1300 = __AST_CHIP(4, 1), // unused
>>> +    AST1050 = __AST_CHIP(4, 2), // unused
>>> +    /* 5th gen */
>>> +    AST2400 = __AST_CHIP(5, 0),
>>> +    AST1400 = __AST_CHIP(5, 1), // unused
>>> +    AST1250 = __AST_CHIP(5, 2), // unused
>>> +    /* 6th gen */
>>> +    AST2500 = __AST_CHIP(6, 0),
>>> +    AST2510 = __AST_CHIP(6, 1), // unused
>>> +    AST2520 = __AST_CHIP(6, 2), // unused
>>> +    /* 7th gen */
>>> +    AST2600 = __AST_CHIP(7, 0),
>>> +    AST2620 = __AST_CHIP(7, 1), // unused
>>>   };
>>> +#define __AST_CHIP_GEN(__chip)    (((unsigned long)(__chip)) >> 16)
>>> +
>>>   enum ast_tx_chip {
>>>       AST_TX_NONE,
>>>       AST_TX_SIL164,
>>> @@ -220,6 +240,24 @@ struct ast_device *ast_device_create(const 
>>> struct drm_driver *drv,
>>>                        struct pci_dev *pdev,
>>>                        unsigned long flags);
>>> +static inline unsigned long __ast_gen(struct ast_device *ast)
>>> +{
>>> +    return __AST_CHIP_GEN(ast->chip);
>>> +}
>>> +#define AST_GEN(__ast)    __ast_gen(__ast)
>>> +
>>> +static inline bool __ast_is_gen(struct ast_device *ast, unsigned 
>>> long gen)
>>> +{
>>> +    return __ast_gen(ast) == gen;
>>> +}
>>
>> Changed to __ast_gen_is_equal() ?
>
> Makes sense.
>
>>
>>
>>> +#define IS_AST_GEN1(__ast) __ast_is_gen(__ast, 1)
>>> +#define IS_AST_GEN2(__ast)    __ast_is_gen(__ast, 2)
>>> +#define IS_AST_GEN3(__ast)    __ast_is_gen(__ast, 3)
>>> +#define IS_AST_GEN4(__ast)    __ast_is_gen(__ast, 4)
>>> +#define IS_AST_GEN5(__ast)    __ast_is_gen(__ast, 5)
>>> +#define IS_AST_GEN6(__ast)    __ast_is_gen(__ast, 6)
>>> +#define IS_AST_GEN7(__ast)    __ast_is_gen(__ast, 7)
>>> +
>>>   #define AST_IO_AR_PORT_WRITE        (0x40)
>>>   #define AST_IO_MISC_PORT_WRITE        (0x42)
>>>   #define AST_IO_VGA_ENABLE_PORT        (0x43)
>>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>>> b/drivers/gpu/drm/ast/ast_main.c
>>> index 6ff4b837e64d7..3cd94a74150bf 100644
>>> --- a/drivers/gpu/drm/ast/ast_main.c
>>> +++ b/drivers/gpu/drm/ast/ast_main.c
>>> @@ -128,7 +128,7 @@ static void ast_detect_config_mode(struct 
>>> drm_device *dev, u32 *scu_rev)
>>>       jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 
>>> 0xff);
>>>       jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 
>>> 0xff);
>>>       if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>>> -        /* Patch AST2500 */
>>> +        /* Patch GEN6 */
>>>           if (((pdev->revision & 0xF0) == 0x40)
>>>               && ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
>>>               ast_patch_ahb_2500(ast);
>>> @@ -197,8 +197,8 @@ static int ast_detect_chip(struct drm_device 
>>> *dev, bool need_post, u32 scu_rev)
>>>       }
>>>       /* Check if we support wide screen */
>>> -    switch (ast->chip) {
>>> -    case AST2000:
>>> +    switch (AST_GEN(ast)) {
>>> +    case 1:
>>>           ast->support_wide_screen = false;
>>>           break;
>>>       default:
>>> @@ -218,7 +218,7 @@ static int ast_detect_chip(struct drm_device 
>>> *dev, bool need_post, u32 scu_rev)
>>>               if (ast->chip == AST2500 &&
>>>                   scu_rev == 0x100)           /* ast2510 */
>>>                   ast->support_wide_screen = true;
>>> -            if (ast->chip == AST2600)        /* ast2600 */
>>> +            if (IS_AST_GEN7(ast))
>>>                   ast->support_wide_screen = true;
>>>           }
>>>           break;
>>> @@ -241,9 +241,9 @@ static int ast_detect_chip(struct drm_device 
>>> *dev, bool need_post, u32 scu_rev)
>>>               ast->tx_chip_types = AST_TX_SIL164_BIT;
>>>       }
>>> -    if ((ast->chip == AST2300) || (ast->chip == AST2400) || 
>>> (ast->chip == AST2500)) {
>>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast)) {
>>>           /*
>>> -         * On AST2300 and 2400, look the configuration set by the 
>>> SoC in
>>> +         * On AST GEN4+, look the configuration set by the SoC in
>>>            * the SOC scratch register #1 bits 11:8 (interestingly 
>>> marked
>>>            * as "reserved" in the spec)
>>>            */
>>> @@ -265,7 +265,7 @@ static int ast_detect_chip(struct drm_device 
>>> *dev, bool need_post, u32 scu_rev)
>>>           case 0x0c:
>>>               ast->tx_chip_types = AST_TX_DP501_BIT;
>>>           }
>>> -    } else if (ast->chip == AST2600) {
>>> +    } else if (IS_AST_GEN7(ast)) {
>>>           if (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, 
>>> TX_TYPE_MASK) ==
>>>               ASTDP_DPMCU_TX) {
>>>               ast->tx_chip_types = AST_TX_ASTDP_BIT;
>>> @@ -297,7 +297,7 @@ static int ast_get_dram_info(struct drm_device 
>>> *dev)
>>>       case ast_use_dt:
>>>           /*
>>>            * If some properties are missing, use reasonable
>>> -         * defaults for AST2400
>>> +         * defaults for GEN5
>>>            */
>>>           if (of_property_read_u32(np, "aspeed,mcr-configuration",
>>>                        &mcr_cfg))
>>> @@ -320,7 +320,7 @@ static int ast_get_dram_info(struct drm_device 
>>> *dev)
>>>       default:
>>>           ast->dram_bus_width = 16;
>>>           ast->dram_type = AST_DRAM_1Gx16;
>>> -        if (ast->chip == AST2500)
>>> +        if (IS_AST_GEN6(ast))
>>>               ast->mclk = 800;
>>>           else
>>>               ast->mclk = 396;
>>> @@ -332,7 +332,7 @@ static int ast_get_dram_info(struct drm_device 
>>> *dev)
>>>       else
>>>           ast->dram_bus_width = 32;
>>> -    if (ast->chip == AST2500) {
>>> +    if (IS_AST_GEN6(ast)) {
>>>           switch (mcr_cfg & 0x03) {
>>>           case 0:
>>>               ast->dram_type = AST_DRAM_1Gx16;
>>> @@ -348,7 +348,7 @@ static int ast_get_dram_info(struct drm_device 
>>> *dev)
>>>               ast->dram_type = AST_DRAM_8Gx16;
>>>               break;
>>>           }
>>> -    } else if (ast->chip == AST2300 || ast->chip == AST2400) {
>>> +    } else if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>>>           switch (mcr_cfg & 0x03) {
>>>           case 0:
>>>               ast->dram_type = AST_DRAM_512Mx16;
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>>> b/drivers/gpu/drm/ast/ast_mode.c
>>> index b3c670af6ef2b..f711d592da52b 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -342,7 +342,7 @@ static void ast_set_crtc_reg(struct ast_device 
>>> *ast,
>>>       u8 jreg05 = 0, jreg07 = 0, jreg09 = 0, jregAC = 0, jregAD = 0, 
>>> jregAE = 0;
>>>       u16 temp, precache = 0;
>>> -    if ((ast->chip == AST2500 || ast->chip == AST2600) &&
>>> +    if ((IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) &&
>>>           (vbios_mode->enh_table->flags & AST2500PreCatchCRT))
>>>           precache = 40;
>>> @@ -384,7 +384,7 @@ static void ast_set_crtc_reg(struct ast_device 
>>> *ast,
>>>       ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xAD, 0x00, 
>>> jregAD);
>>>       // Workaround for HSync Time non octave pixels (1920x1080@60Hz 
>>> HSync 44 pixels);
>>> -    if ((ast->chip == AST2600) && (mode->crtc_vdisplay == 1080))
>>> +    if (IS_AST_GEN7(ast) && (mode->crtc_vdisplay == 1080))
>>>           ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 
>>> 0x02);
>>>       else
>>>           ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xFC, 0xFD, 
>>> 0x00);
>>> @@ -466,7 +466,7 @@ static void ast_set_dclk_reg(struct ast_device 
>>> *ast,
>>>   {
>>>       const struct ast_vbios_dclk_info *clk_info;
>>> -    if ((ast->chip == AST2500) || (ast->chip == AST2600))
>>> +    if (IS_AST_GEN6(ast) || IS_AST_GEN7(ast))
>>>           clk_info = 
>>> &dclk_table_ast2500[vbios_mode->enh_table->dclk_index];
>>>       else
>>>           clk_info = &dclk_table[vbios_mode->enh_table->dclk_index];
>>> @@ -510,17 +510,13 @@ static void ast_set_color_reg(struct 
>>> ast_device *ast,
>>>   static void ast_set_crtthd_reg(struct ast_device *ast)
>>>   {
>>>       /* Set Threshold */
>>> -    if (ast->chip == AST2600) {
>>> +    if (IS_AST_GEN7(ast)) {
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0xe0);
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0xa0);
>>> -    } else if (ast->chip == AST2300 || ast->chip == AST2400 ||
>>> -        ast->chip == AST2500) {
>>> +    } else if (IS_AST_GEN6(ast) || IS_AST_GEN5(ast) || 
>>> IS_AST_GEN4(ast)) {
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x78);
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x60);
>>> -    } else if (ast->chip == AST2100 ||
>>> -           ast->chip == AST1100 ||
>>> -           ast->chip == AST2200 ||
>>> -           ast->chip == AST2150) {
>>> +    } else if (IS_AST_GEN3(ast) || IS_AST_GEN2(ast)) {
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa7, 0x3f);
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa6, 0x2f);
>>>       } else {
>>> @@ -1082,9 +1078,10 @@ ast_crtc_helper_mode_valid(struct drm_crtc 
>>> *crtc, const struct drm_display_mode
>>>           if ((mode->hdisplay == 1152) && (mode->vdisplay == 864))
>>>               return MODE_OK;
>>> -        if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
>>> -            (ast->chip == AST2300) || (ast->chip == AST2400) ||
>>> -            (ast->chip == AST2500) || (ast->chip == AST2600)) {
>>> +        if ((ast->chip == AST2100) || // GEN2, but not AST1100 (?)
>>> +            (ast->chip == AST2200) || // GEN3, but not AST2150 (?)
>>
>> If chips from the same generation is not compatible, then this 
>> actually introduce confusion.
>>
>> It's not your patch is bad, it is the naming and/or the hardware is bad.
>>
>> On such a situation, the patch is not good enough to suppress 
>> problems incurred by the hardware versions.
>>
>> It is not clean and It still a tangled implement. But I'm fine if you 
>> want to see the limitation.
>
> The history and feature set of the earlier chips appears convoluted. 
> The chip generation is meant for the cases where all chips behave the 
> same. I'm considering to create a patch that reorganizes these 
> branches to be more readable. But for now, it's just about making the 
> current code more understandable.
>
Yeah, the history it too heavy,

there so much model need to be processed, it deserve another patch.

I'm agreed with you for now.

> Best regards
> Thomas
>
>>
>>> +            IS_AST_GEN4(ast) || IS_AST_GEN5(ast) ||
>>> +            IS_AST_GEN6(ast) || IS_AST_GEN7(ast)) {
>>>               if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
>>>                   return MODE_OK;
>>> @@ -1800,12 +1797,12 @@ int ast_mode_config_init(struct ast_device 
>>> *ast)
>>>       dev->mode_config.min_height = 0;
>>>       dev->mode_config.preferred_depth = 24;
>>> -    if (ast->chip == AST2100 ||
>>> -        ast->chip == AST2200 ||
>>> -        ast->chip == AST2300 ||
>>> -        ast->chip == AST2400 ||
>>> -        ast->chip == AST2500 ||
>>> -        ast->chip == AST2600) {
>>> +    if (ast->chip == AST2100 || // GEN2, but not AST1100 (?)
>>> +        ast->chip == AST2200 || // GEN3, but not AST2150 (?)
>>> +        IS_AST_GEN7(ast) ||
>>> +        IS_AST_GEN6(ast) ||
>>> +        IS_AST_GEN5(ast) ||
>>> +        IS_AST_GEN4(ast)) {
>>>           dev->mode_config.max_width = 1920;
>>>           dev->mode_config.max_height = 2048;
>>>       } else {
>>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>>> b/drivers/gpu/drm/ast/ast_post.c
>>> index b765eeb55e5f1..13e15173f2c5b 100644
>>> --- a/drivers/gpu/drm/ast/ast_post.c
>>> +++ b/drivers/gpu/drm/ast/ast_post.c
>>> @@ -51,7 +51,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>>>       for (i = 0x81; i <= 0x9f; i++)
>>>           ast_set_index_reg(ast, AST_IO_CRTC_PORT, i, 0x00);
>>> -    if (ast->chip == AST2300 || ast->chip == AST2400 || ast->chip 
>>> == AST2500)
>>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>>>           ext_reg_info = extreginfo_ast2300;
>>>       else
>>>           ext_reg_info = extreginfo;
>>> @@ -72,8 +72,7 @@ ast_set_def_ext_reg(struct drm_device *dev)
>>>       /* Enable RAMDAC for A1 */
>>>       reg = 0x04;
>>> -    if (ast->chip == AST2300 || ast->chip == AST2400 ||
>>> -        ast->chip == AST2500)
>>> +    if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast) || IS_AST_GEN6(ast))
>>>           reg |= 0x20;
>>>       ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb6, 0xff, reg);
>>>   }
>>> @@ -249,7 +248,7 @@ static void ast_init_dram_reg(struct drm_device 
>>> *dev)
>>>       j = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>>>       if ((j & 0x80) == 0) { /* VGA only */
>>> -        if (ast->chip == AST2000) {
>>> +        if (IS_AST_GEN1(ast)) {
>>>               dram_reg_info = ast2000_dram_table_data;
>>>               ast_write32(ast, 0xf004, 0x1e6e0000);
>>>               ast_write32(ast, 0xf000, 0x1);
>>> @@ -258,7 +257,7 @@ static void ast_init_dram_reg(struct drm_device 
>>> *dev)
>>>               do {
>>>                   ;
>>>               } while (ast_read32(ast, 0x10100) != 0xa8);
>>> -        } else {/* AST2100/1100 */
>>> +        } else { /* GEN2/GEN3 */
>>>               if (ast->chip == AST2100 || ast->chip == AST2200)
>>>                   dram_reg_info = ast2100_dram_table_data;
>>>               else
>>> @@ -281,7 +280,7 @@ static void ast_init_dram_reg(struct drm_device 
>>> *dev)
>>>               if (dram_reg_info->index == 0xff00) {/* delay fn */
>>>                   for (i = 0; i < 15; i++)
>>>                       udelay(dram_reg_info->data);
>>> -            } else if (dram_reg_info->index == 0x4 && ast->chip != 
>>> AST2000) {
>>> +            } else if (dram_reg_info->index == 0x4 && 
>>> !IS_AST_GEN1(ast)) {
>>>                   data = dram_reg_info->data;
>>>                   if (ast->dram_type == AST_DRAM_1Gx16)
>>>                       data = 0x00000d89;
>>> @@ -307,15 +306,13 @@ static void ast_init_dram_reg(struct 
>>> drm_device *dev)
>>>                   cbrdlli_ast2150(ast, 32); /* 32 bits */
>>>           }
>>> -        switch (ast->chip) {
>>> -        case AST2000:
>>> +        switch (AST_GEN(ast)) {
>>> +        case 1:
>>>               temp = ast_read32(ast, 0x10140);
>>>               ast_write32(ast, 0x10140, temp | 0x40);
>>>               break;
>>> -        case AST1100:
>>> -        case AST2100:
>>> -        case AST2200:
>>> -        case AST2150:
>>> +        case 2:
>>> +        case 3:
>>>               temp = ast_read32(ast, 0x1200c);
>>>               ast_write32(ast, 0x1200c, temp & 0xfffffffd);
>>>               temp = ast_read32(ast, 0x12040);
>>> @@ -338,13 +335,13 @@ void ast_post_gpu(struct drm_device *dev)
>>>       ast_set_def_ext_reg(dev);
>>> -    if (ast->chip == AST2600) {
>>> +    if (IS_AST_GEN7(ast)) {
>>>           if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>>>               ast_dp_launch(dev);
>>>       } else if (ast->config_mode == ast_use_p2a) {
>>> -        if (ast->chip == AST2500)
>>> +        if (IS_AST_GEN6(ast))
>>>               ast_post_chip_2500(dev);
>>> -        else if (ast->chip == AST2300 || ast->chip == AST2400)
>>> +        else if (IS_AST_GEN5(ast) || IS_AST_GEN4(ast))
>>>               ast_post_chip_2300(dev);
>>>           else
>>>               ast_init_dram_reg(dev);
>>
>
-- 
Jingfeng

[PATCH 10/14] drm/ast: Detect AST 1300 model

[Thomas Zimmermann]

Detect the 4th-generation AST 1300. Allows to simplify the code
for widescreen support.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index c209d7e4e4194..e3f82c7a823e4 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -70,7 +70,7 @@ enum ast_chip {
 	AST2150 = __AST_CHIP(3, 1),
 	/* 4th gen */
 	AST2300 = __AST_CHIP(4, 0),
-	AST1300 = __AST_CHIP(4, 1), // unused
+	AST1300 = __AST_CHIP(4, 1),
 	AST1050 = __AST_CHIP(4, 2), // unused
 	/* 5th gen */
 	AST2400 = __AST_CHIP(5, 0),
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 3cd94a74150bf..bf8606301ab26 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -170,8 +170,16 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		ast->chip = AST2400;
 		drm_info(dev, "AST 2400 detected\n");
 	} else if (pdev->revision >= 0x20) {
-		ast->chip = AST2300;
-		drm_info(dev, "AST 2300 detected\n");
+		switch (scu_rev & 0x300) {
+		case 0x0000:
+			ast->chip = AST1300;
+			drm_info(dev, "AST 1300 detected\n");
+			break;
+		default:
+			ast->chip = AST2300;
+			drm_info(dev, "AST 2300 detected\n");
+			break;
+		}
 	} else if (pdev->revision >= 0x10) {
 		switch (scu_rev & 0x0300) {
 		case 0x0200:
@@ -209,8 +217,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 			ast->support_wide_screen = true;
 		else {
 			ast->support_wide_screen = false;
-			if (ast->chip == AST2300 &&
-			    (scu_rev & 0x300) == 0x0) /* ast1300 */
+			if (ast->chip == AST1300)
 				ast->support_wide_screen = true;
 			if (ast->chip == AST2400 &&
 			    (scu_rev & 0x300) == 0x100) /* ast1400 */
-- 
2.41.0

[PATCH 11/14] drm/ast: Detect AST 1400 model

[Thomas Zimmermann]

Detect the 5th-generation AST 1400. Allows to simplify the code
for widescreen support.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index e3f82c7a823e4..ef69a1535695b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -74,7 +74,7 @@ enum ast_chip {
 	AST1050 = __AST_CHIP(4, 2), // unused
 	/* 5th gen */
 	AST2400 = __AST_CHIP(5, 0),
-	AST1400 = __AST_CHIP(5, 1), // unused
+	AST1400 = __AST_CHIP(5, 1),
 	AST1250 = __AST_CHIP(5, 2), // unused
 	/* 6th gen */
 	AST2500 = __AST_CHIP(6, 0),
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index bf8606301ab26..72543e97f3586 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -167,8 +167,15 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		ast->chip = AST2500;
 		drm_info(dev, "AST 2500 detected\n");
 	} else if (pdev->revision >= 0x30) {
-		ast->chip = AST2400;
-		drm_info(dev, "AST 2400 detected\n");
+		switch (scu_rev & 0x300) {
+		case 0x0100:
+			ast->chip = AST1400;
+			drm_info(dev, "AST 1400 detected\n");
+			break;
+		default:
+			ast->chip = AST2400;
+			drm_info(dev, "AST 2400 detected\n");
+		}
 	} else if (pdev->revision >= 0x20) {
 		switch (scu_rev & 0x300) {
 		case 0x0000:
@@ -219,8 +226,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 			ast->support_wide_screen = false;
 			if (ast->chip == AST1300)
 				ast->support_wide_screen = true;
-			if (ast->chip == AST2400 &&
-			    (scu_rev & 0x300) == 0x100) /* ast1400 */
+			if (ast->chip == AST1400)
 				ast->support_wide_screen = true;
 			if (ast->chip == AST2500 &&
 			    scu_rev == 0x100)           /* ast2510 */
-- 
2.41.0

[PATCH 12/14] drm/ast: Detect AST 2510 model

[Thomas Zimmermann]

Detect the 6th-generation AST 2510. Allows to simplify the code
for widescreen support.

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

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index ef69a1535695b..c66fe0ba208e7 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -78,7 +78,7 @@ enum ast_chip {
 	AST1250 = __AST_CHIP(5, 2), // unused
 	/* 6th gen */
 	AST2500 = __AST_CHIP(6, 0),
-	AST2510 = __AST_CHIP(6, 1), // unused
+	AST2510 = __AST_CHIP(6, 1),
 	AST2520 = __AST_CHIP(6, 2), // unused
 	/* 7th gen */
 	AST2600 = __AST_CHIP(7, 0),
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 72543e97f3586..7f8fb9a613604 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -164,8 +164,15 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		ast->chip = AST2600;
 		drm_info(dev, "AST 2600 detected\n");
 	} else if (pdev->revision >= 0x40) {
-		ast->chip = AST2500;
-		drm_info(dev, "AST 2500 detected\n");
+		switch (scu_rev & 0x300) {
+		case 0x0100:
+			ast->chip = AST2510;
+			drm_info(dev, "AST 2510 detected\n");
+			break;
+		default:
+			ast->chip = AST2500;
+			drm_info(dev, "AST 2500 detected\n");
+		}
 	} else if (pdev->revision >= 0x30) {
 		switch (scu_rev & 0x300) {
 		case 0x0100:
@@ -228,8 +235,7 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 				ast->support_wide_screen = true;
 			if (ast->chip == AST1400)
 				ast->support_wide_screen = true;
-			if (ast->chip == AST2500 &&
-			    scu_rev == 0x100)           /* ast2510 */
+			if (ast->chip == AST2510)
 				ast->support_wide_screen = true;
 			if (IS_AST_GEN7(ast))
 				ast->support_wide_screen = true;
-- 
2.41.0

[PATCH 13/14] drm/ast: Move widescreen- and tx-chip detection into separate helpers

[Thomas Zimmermann]

Split ast_detect_chip() into three functions and call them one by
one. The new functions detect the transmitter chip and widescreen
support. This will allow for further refactoring.

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

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 7f8fb9a613604..f028b5b47c56e 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -157,7 +157,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 {
 	struct ast_device *ast = to_ast_device(dev);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	uint32_t jreg;
 
 	/* Identify chipset */
 	if (pdev->revision >= 0x50) {
@@ -218,6 +217,13 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		drm_info(dev, "AST 2000 detected\n");
 	}
 
+	return 0;
+}
+
+static void ast_detect_widescreen(struct ast_device *ast)
+{
+	u8 jreg;
+
 	/* Check if we support wide screen */
 	switch (AST_GEN(ast)) {
 	case 1:
@@ -242,6 +248,12 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		}
 		break;
 	}
+}
+
+static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
+{
+	struct drm_device *dev = &ast->base;
+	u8 jreg;
 
 	/* Check 3rd Tx option (digital output afaik) */
 	ast->tx_chip_types |= AST_TX_NONE_BIT;
@@ -301,8 +313,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
 		drm_info(dev, "Using DP501 DisplayPort transmitter\n");
 	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
 		drm_info(dev, "Using ASPEED DisplayPort transmitter\n");
-
-	return 0;
 }
 
 static int ast_get_dram_info(struct drm_device *dev)
@@ -494,6 +504,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	ast_detect_config_mode(dev, &scu_rev);
 
 	ast_detect_chip(dev, need_post, scu_rev);
+	ast_detect_widescreen(ast);
+	ast_detect_tx_chip(ast, need_post);
 
 	ret = ast_get_dram_info(dev);
 	if (ret)
-- 
2.41.0

Re: [13/14] drm/ast: Move widescreen- and tx-chip detection into separate helpers

[Sui Jingfeng]

Hi,


"widescreen-" -> "widescreen"


On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Split ast_detect_chip() into three functions and call them one by
> one. The new functions detect the transmitter chip and widescreen
> support. This will allow for further refactoring.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 7f8fb9a613604..f028b5b47c56e 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -157,7 +157,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	uint32_t jreg;
>   
>   	/* Identify chipset */
>   	if (pdev->revision >= 0x50) {
> @@ -218,6 +217,13 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		drm_info(dev, "AST 2000 detected\n");
>   	}
>   
> +	return 0;
> +}
> +
> +static void ast_detect_widescreen(struct ast_device *ast)
> +{
> +	u8 jreg;
> +
>   	/* Check if we support wide screen */
>   	switch (AST_GEN(ast)) {
>   	case 1:
> @@ -242,6 +248,12 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		}
>   		break;
>   	}
> +}
> +
> +static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
> +{
> +	struct drm_device *dev = &ast->base;
> +	u8 jreg;
>   
>   	/* Check 3rd Tx option (digital output afaik) */
>   	ast->tx_chip_types |= AST_TX_NONE_BIT;
> @@ -301,8 +313,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		drm_info(dev, "Using DP501 DisplayPort transmitter\n");
>   	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>   		drm_info(dev, "Using ASPEED DisplayPort transmitter\n");
> -
> -	return 0;
>   }
>   
>   static int ast_get_dram_info(struct drm_device *dev)
> @@ -494,6 +504,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	ast_detect_config_mode(dev, &scu_rev);
>   
>   	ast_detect_chip(dev, need_post, scu_rev);
> +	ast_detect_widescreen(ast);
> +	ast_detect_tx_chip(ast, need_post);
>   
>   	ret = ast_get_dram_info(dev);
>   	if (ret)

-- 
Jingfeng

Re: [13/14] drm/ast: Move widescreen- and tx-chip detection into separate helpers

[Sui Jingfeng]

Hi,


Split ast_detect_chip() into three functions made it more clear.

As tx-chip is typicality  suffer from changed, and it is a choice of the 
PCB board designer.

tx-chip belong to the output, which chip identify and wide screen 
support is a capability of

the chip itself, once the chip is taped, it is fixed. So this patch 
looks fine.


On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Split ast_detect_chip() into three functions and call them one by
> one. The new functions detect the transmitter chip and widescreen
> support. This will allow for further refactoring.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

> ---
>   drivers/gpu/drm/ast/ast_main.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 7f8fb9a613604..f028b5b47c56e 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -157,7 +157,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	uint32_t jreg;
>   
>   	/* Identify chipset */
>   	if (pdev->revision >= 0x50) {
> @@ -218,6 +217,13 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		drm_info(dev, "AST 2000 detected\n");
>   	}
>   
> +	return 0;
> +}
> +
> +static void ast_detect_widescreen(struct ast_device *ast)
> +{
> +	u8 jreg;
> +
>   	/* Check if we support wide screen */
>   	switch (AST_GEN(ast)) {
>   	case 1:
> @@ -242,6 +248,12 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		}
>   		break;
>   	}
> +}
> +
> +static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
> +{
> +	struct drm_device *dev = &ast->base;
> +	u8 jreg;
>   
>   	/* Check 3rd Tx option (digital output afaik) */
>   	ast->tx_chip_types |= AST_TX_NONE_BIT;
> @@ -301,8 +313,6 @@ static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
>   		drm_info(dev, "Using DP501 DisplayPort transmitter\n");
>   	if (ast->tx_chip_types & AST_TX_ASTDP_BIT)
>   		drm_info(dev, "Using ASPEED DisplayPort transmitter\n");
> -
> -	return 0;
>   }
>   
>   static int ast_get_dram_info(struct drm_device *dev)
> @@ -494,6 +504,8 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	ast_detect_config_mode(dev, &scu_rev);
>   
>   	ast_detect_chip(dev, need_post, scu_rev);
> +	ast_detect_widescreen(ast);
> +	ast_detect_tx_chip(ast, need_post);
>   
>   	ret = ast_get_dram_info(dev);
>   	if (ret)

-- 
Jingfeng

[PATCH 14/14] drm/ast: Merge config and chip detection

[Thomas Zimmermann]

Detection of the configuration mode and the chipset model are
linked to each other. One uses values from the other; namely the
PCI device revision and the SCU revision. Merge this code into
a single function.

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

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index f028b5b47c56e..5fcddd0dac5e8 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -97,68 +97,75 @@ static void ast_open_key(struct ast_device *ast)
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0x80, 0xA8);
 }
 
-static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
+static int ast_device_config_init(struct ast_device *ast)
 {
-	struct device_node *np = dev->dev->of_node;
-	struct ast_device *ast = to_ast_device(dev);
+	struct drm_device *dev = &ast->base;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	uint32_t data, jregd0, jregd1;
+	struct device_node *np = dev->dev->of_node;
+	uint32_t scu_rev = 0xffffffff;
+	u32 data;
+	u8 jregd0, jregd1;
+
+	/*
+	 * Find configuration mode and read SCU revision
+	 */
 
-	/* Defaults */
 	ast->config_mode = ast_use_defaults;
 
 	/* Check if we have device-tree properties */
-	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id",
-					scu_rev)) {
+	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
 		/* We do, disable P2A access */
 		ast->config_mode = ast_use_dt;
-		drm_info(dev, "Using device-tree for configuration\n");
-		return;
-	}
+		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]
+		 */
+		jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
+		jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
+		if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
+
+			/*
+			 * We have a P2A bridge and it is enabled.
+			 */
+
+			/* Patch AST2500/AST2510 */
+			if ((pdev->revision & 0xf0) == 0x40) {
+				if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
+					ast_patch_ahb_2500(ast);
+			}
 
-	/* Not all families have a P2A bridge */
-	if (pdev->device != PCI_CHIP_AST2000)
-		return;
+			/* Double check that it's actually working */
+			data = ast_read32(ast, 0xf004);
+			if ((data != 0xffffffff) && (data != 0x00)) {
+				ast->config_mode = ast_use_p2a;
 
-	/*
-	 * 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]
-	 */
-	jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
-	jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
-	if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
-		/* Patch GEN6 */
-		if (((pdev->revision & 0xF0) == 0x40)
-			&& ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
-			ast_patch_ahb_2500(ast);
-
-		/* Double check it's actually working */
-		data = ast_read32(ast, 0xf004);
-		if ((data != 0xFFFFFFFF) && (data != 0x00)) {
-			/* P2A works, grab silicon revision */
-			ast->config_mode = ast_use_p2a;
-
-			drm_info(dev, "Using P2A bridge for configuration\n");
-
-			/* Read SCU7c (silicon revision register) */
-			ast_write32(ast, 0xf004, 0x1e6e0000);
-			ast_write32(ast, 0xf000, 0x1);
-			*scu_rev = ast_read32(ast, 0x1207c);
-			return;
+				/* Read SCU7c (silicon revision register) */
+				ast_write32(ast, 0xf004, 0x1e6e0000);
+				ast_write32(ast, 0xf000, 0x1);
+				scu_rev = ast_read32(ast, 0x1207c);
+			}
 		}
 	}
 
-	/* We have a P2A bridge but it's disabled */
-	drm_info(dev, "P2A bridge disabled, using default configuration\n");
-}
+	switch (ast->config_mode) {
+	case ast_use_defaults:
+		drm_info(dev, "Using default configuration\n");
+		break;
+	case ast_use_dt:
+		drm_info(dev, "Using device-tree for configuration\n");
+		break;
+	case ast_use_p2a:
+		drm_info(dev, "Using P2A bridge for configuration\n");
+		break;
+	}
 
-static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
-{
-	struct ast_device *ast = to_ast_device(dev);
-	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	/*
+	 * Identify chipset
+	 */
 
-	/* Identify chipset */
 	if (pdev->revision >= 0x50) {
 		ast->chip = AST2600;
 		drm_info(dev, "AST 2600 detected\n");
@@ -443,7 +450,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	struct ast_device *ast;
 	bool need_post = false;
 	int ret = 0;
-	u32 scu_rev = 0xffffffff;
 
 	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
 	if (IS_ERR(ast))
@@ -500,10 +506,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 
-	/* Find out whether P2A works or whether to use device-tree */
-	ast_detect_config_mode(dev, &scu_rev);
+	ret = ast_device_config_init(ast);
+	if (ret)
+		return ERR_PTR(ret);
 
-	ast_detect_chip(dev, need_post, scu_rev);
 	ast_detect_widescreen(ast);
 	ast_detect_tx_chip(ast, need_post);
 
-- 
2.41.0

Re: [14/14] drm/ast: Merge config and chip detection

[Sui Jingfeng]

Hi,

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Detection of the configuration mode and the chipset model are
> linked to each other.

They don't  has a strong relationship,  chipset model detection should 
the first function to be run(should be run early).

chipset model detection should only rely on the information come with 
PCI device itself.

Then  ast_detect_config_mode() follows, ast_detect_config_mode() should 
depend on ast_detect_chip()

>   One uses values from the other; namely the
> PCI device revision and the SCU revision. Merge this code into
> a single function.

In last patch, you split one into three, then in this patch, you merge 
the two into one.

Then you finally got one more patch function only(+2 - 1 = 1).

This is fine, but I noticed that the ast_detect_widescreen() function is 
also depend on the chip identify.

Then, why the ast_detect_widescreen() function is not get merged to 
ast_device_config_init() ?

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 108 +++++++++++++++++----------------
>   1 file changed, 57 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index f028b5b47c56e..5fcddd0dac5e8 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -97,68 +97,75 @@ static void ast_open_key(struct ast_device *ast)
>   	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0x80, 0xA8);
>   }
>   
> -static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
> +static int ast_device_config_init(struct ast_device *ast)
>   {
> -	struct device_node *np = dev->dev->of_node;
> -	struct ast_device *ast = to_ast_device(dev);
> +	struct drm_device *dev = &ast->base;
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
> -	uint32_t data, jregd0, jregd1;
> +	struct device_node *np = dev->dev->of_node;

The OF itself deserve a separated function, as its only available when 
DT is available.

The no need twist so much local variables together.

> +	uint32_t scu_rev = 0xffffffff;
> +	u32 data;
> +	u8 jregd0, jregd1;
> +
> +	/*
> +	 * Find configuration mode and read SCU revision
> +	 */
>   
> -	/* Defaults */
>   	ast->config_mode = ast_use_defaults;
>   
>   	/* Check if we have device-tree properties */
> -	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id",
> -					scu_rev)) {
> +	if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", &data)) {
>   		/* We do, disable P2A access */
>   		ast->config_mode = ast_use_dt;
> -		drm_info(dev, "Using device-tree for configuration\n");
> -		return;
> -	}
> +		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]
> +		 */
> +		jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
> +		jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
> +		if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
> +
> +			/*
> +			 * We have a P2A bridge and it is enabled.
> +			 */
> +
> +			/* Patch AST2500/AST2510 */
> +			if ((pdev->revision & 0xf0) == 0x40) {
> +				if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
> +					ast_patch_ahb_2500(ast);
> +			}
>   
> -	/* Not all families have a P2A bridge */
> -	if (pdev->device != PCI_CHIP_AST2000)
> -		return;
> +			/* Double check that it's actually working */
> +			data = ast_read32(ast, 0xf004);
> +			if ((data != 0xffffffff) && (data != 0x00)) {
> +				ast->config_mode = ast_use_p2a;
>   
> -	/*
> -	 * 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]
> -	 */
> -	jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
> -	jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
> -	if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
> -		/* Patch GEN6 */
> -		if (((pdev->revision & 0xF0) == 0x40)
> -			&& ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
> -			ast_patch_ahb_2500(ast);
> -
> -		/* Double check it's actually working */
> -		data = ast_read32(ast, 0xf004);
> -		if ((data != 0xFFFFFFFF) && (data != 0x00)) {
> -			/* P2A works, grab silicon revision */
> -			ast->config_mode = ast_use_p2a;
> -
> -			drm_info(dev, "Using P2A bridge for configuration\n");
> -
> -			/* Read SCU7c (silicon revision register) */
> -			ast_write32(ast, 0xf004, 0x1e6e0000);
> -			ast_write32(ast, 0xf000, 0x1);
> -			*scu_rev = ast_read32(ast, 0x1207c);
> -			return;
> +				/* Read SCU7c (silicon revision register) */
> +				ast_write32(ast, 0xf004, 0x1e6e0000);
> +				ast_write32(ast, 0xf000, 0x1);
> +				scu_rev = ast_read32(ast, 0x1207c);
> +			}
>   		}
>   	}
>   
> -	/* We have a P2A bridge but it's disabled */
> -	drm_info(dev, "P2A bridge disabled, using default configuration\n");
> -}
> +	switch (ast->config_mode) {
> +	case ast_use_defaults:
> +		drm_info(dev, "Using default configuration\n");
> +		break;
> +	case ast_use_dt:
> +		drm_info(dev, "Using device-tree for configuration\n");
> +		break;
> +	case ast_use_p2a:
> +		drm_info(dev, "Using P2A bridge for configuration\n");
> +		break;
> +	}
>   
> -static int ast_detect_chip(struct drm_device *dev, bool need_post, u32 scu_rev)
> -{
> -	struct ast_device *ast = to_ast_device(dev);
> -	struct pci_dev *pdev = to_pci_dev(dev->dev);
> +	/*
> +	 * Identify chipset
> +	 */
>   
> -	/* Identify chipset */
>   	if (pdev->revision >= 0x50) {
>   		ast->chip = AST2600;
>   		drm_info(dev, "AST 2600 detected\n");
> @@ -443,7 +450,6 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	struct ast_device *ast;
>   	bool need_post = false;
>   	int ret = 0;
> -	u32 scu_rev = 0xffffffff;
>   
>   	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
>   	if (IS_ERR(ast))
> @@ -500,10 +506,10 @@ struct ast_device *ast_device_create(const struct drm_driver *drv,
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	/* Find out whether P2A works or whether to use device-tree */
> -	ast_detect_config_mode(dev, &scu_rev);
> +	ret = ast_device_config_init(ast);
> +	if (ret)
> +		return ERR_PTR(ret);
>   
> -	ast_detect_chip(dev, need_post, scu_rev);
>   	ast_detect_widescreen(ast);
>   	ast_detect_tx_chip(ast, need_post);
>   

-- 
Jingfeng

Re: [14/14] drm/ast: Merge config and chip detection

[Thomas Zimmermann]

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

Hi

Am 19.06.23 um 05:15 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/6/16 21:52, Thomas Zimmermann wrote:
>> Detection of the configuration mode and the chipset model are
>> linked to each other.
> 
> They don't  has a strong relationship,  chipset model detection should 
> the first function to be run(should be run early).
> 
> chipset model detection should only rely on the information come with 
> PCI device itself.
> 
> Then  ast_detect_config_mode() follows, ast_detect_config_mode() should 
> depend on ast_detect_chip()

It's not that easy. Model detection requires the silicon revision stored 
in scu_rev. There are different ways of getting scu_rev, depending on 
the config mode. That involve the PCI device id and the PCI revision, 
plus initialization code for the AST2500. I currently don't see a way of 
splitting this code without regressing. It is best handled in the same 
place.

I possible solution would be to look at the pdev->revision first and for 
each call a revision-specific function that detects the config mode and 
scu_rev. But we're not there yet.

BTW I don't think that this patchset is the last cleanup of that code.

> 
>>   One uses values from the other; namely the
>> PCI device revision and the SCU revision. Merge this code into
>> a single function.
> 
> In last patch, you split one into three, then in this patch, you merge 
> the two into one.
> 
> Then you finally got one more patch function only(+2 - 1 = 1).

I try to go slowly. It's better to have multiple patches than a single 
big one.

> 
> This is fine, but I noticed that the ast_detect_widescreen() function is 
> also depend on the chip identify.
> 
> Then, why the ast_detect_widescreen() function is not get merged to 
> ast_device_config_init() ?

Widescreen handling is only relevant for the modesetting code. I'm 
trying to remove it from the device detection. The same is true for TX 
detection. It would be nice if that could be self-contained in the 
modesetting code.

Best regards
Thomas

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_main.c | 108 +++++++++++++++++----------------
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_main.c 
>> b/drivers/gpu/drm/ast/ast_main.c
>> index f028b5b47c56e..5fcddd0dac5e8 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -97,68 +97,75 @@ static void ast_open_key(struct ast_device *ast)
>>       ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0x80, 0xA8);
>>   }
>> -static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>> +static int ast_device_config_init(struct ast_device *ast)
>>   {
>> -    struct device_node *np = dev->dev->of_node;
>> -    struct ast_device *ast = to_ast_device(dev);
>> +    struct drm_device *dev = &ast->base;
>>       struct pci_dev *pdev = to_pci_dev(dev->dev);
>> -    uint32_t data, jregd0, jregd1;
>> +    struct device_node *np = dev->dev->of_node;
> 
> The OF itself deserve a separated function, as its only available when 
> DT is available.
> 
> The no need twist so much local variables together.
> 
>> +    uint32_t scu_rev = 0xffffffff;
>> +    u32 data;
>> +    u8 jregd0, jregd1;
>> +
>> +    /*
>> +     * Find configuration mode and read SCU revision
>> +     */
>> -    /* Defaults */
>>       ast->config_mode = ast_use_defaults;
>>       /* Check if we have device-tree properties */
>> -    if (np && !of_property_read_u32(np, "aspeed,scu-revision-id",
>> -                    scu_rev)) {
>> +    if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", 
>> &data)) {
>>           /* We do, disable P2A access */
>>           ast->config_mode = ast_use_dt;
>> -        drm_info(dev, "Using device-tree for configuration\n");
>> -        return;
>> -    }
>> +        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]
>> +         */
>> +        jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 
>> 0xff);
>> +        jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 
>> 0xff);
>> +        if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>> +
>> +            /*
>> +             * We have a P2A bridge and it is enabled.
>> +             */
>> +
>> +            /* Patch AST2500/AST2510 */
>> +            if ((pdev->revision & 0xf0) == 0x40) {
>> +                if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
>> +                    ast_patch_ahb_2500(ast);
>> +            }
>> -    /* Not all families have a P2A bridge */
>> -    if (pdev->device != PCI_CHIP_AST2000)
>> -        return;
>> +            /* Double check that it's actually working */
>> +            data = ast_read32(ast, 0xf004);
>> +            if ((data != 0xffffffff) && (data != 0x00)) {
>> +                ast->config_mode = ast_use_p2a;
>> -    /*
>> -     * 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]
>> -     */
>> -    jregd0 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd0, 0xff);
>> -    jregd1 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
>> -    if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>> -        /* Patch GEN6 */
>> -        if (((pdev->revision & 0xF0) == 0x40)
>> -            && ((jregd0 & AST_VRAM_INIT_STATUS_MASK) == 0))
>> -            ast_patch_ahb_2500(ast);
>> -
>> -        /* Double check it's actually working */
>> -        data = ast_read32(ast, 0xf004);
>> -        if ((data != 0xFFFFFFFF) && (data != 0x00)) {
>> -            /* P2A works, grab silicon revision */
>> -            ast->config_mode = ast_use_p2a;
>> -
>> -            drm_info(dev, "Using P2A bridge for configuration\n");
>> -
>> -            /* Read SCU7c (silicon revision register) */
>> -            ast_write32(ast, 0xf004, 0x1e6e0000);
>> -            ast_write32(ast, 0xf000, 0x1);
>> -            *scu_rev = ast_read32(ast, 0x1207c);
>> -            return;
>> +                /* Read SCU7c (silicon revision register) */
>> +                ast_write32(ast, 0xf004, 0x1e6e0000);
>> +                ast_write32(ast, 0xf000, 0x1);
>> +                scu_rev = ast_read32(ast, 0x1207c);
>> +            }
>>           }
>>       }
>> -    /* We have a P2A bridge but it's disabled */
>> -    drm_info(dev, "P2A bridge disabled, using default configuration\n");
>> -}
>> +    switch (ast->config_mode) {
>> +    case ast_use_defaults:
>> +        drm_info(dev, "Using default configuration\n");
>> +        break;
>> +    case ast_use_dt:
>> +        drm_info(dev, "Using device-tree for configuration\n");
>> +        break;
>> +    case ast_use_p2a:
>> +        drm_info(dev, "Using P2A bridge for configuration\n");
>> +        break;
>> +    }
>> -static int ast_detect_chip(struct drm_device *dev, bool need_post, 
>> u32 scu_rev)
>> -{
>> -    struct ast_device *ast = to_ast_device(dev);
>> -    struct pci_dev *pdev = to_pci_dev(dev->dev);
>> +    /*
>> +     * Identify chipset
>> +     */
>> -    /* Identify chipset */
>>       if (pdev->revision >= 0x50) {
>>           ast->chip = AST2600;
>>           drm_info(dev, "AST 2600 detected\n");
>> @@ -443,7 +450,6 @@ struct ast_device *ast_device_create(const struct 
>> drm_driver *drv,
>>       struct ast_device *ast;
>>       bool need_post = false;
>>       int ret = 0;
>> -    u32 scu_rev = 0xffffffff;
>>       ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
>>       if (IS_ERR(ast))
>> @@ -500,10 +506,10 @@ struct ast_device *ast_device_create(const 
>> struct drm_driver *drv,
>>       if (ret)
>>           return ERR_PTR(ret);
>> -    /* Find out whether P2A works or whether to use device-tree */
>> -    ast_detect_config_mode(dev, &scu_rev);
>> +    ret = ast_device_config_init(ast);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> -    ast_detect_chip(dev, need_post, scu_rev);
>>       ast_detect_widescreen(ast);
>>       ast_detect_tx_chip(ast, need_post);
> 

-- 
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 --]

Re: [PATCH 00/14] drm/ast: Refactor the device-detection code

[Sui Jingfeng]

Hi,

On 2023/6/16 21:52, Thomas Zimmermann wrote:
> Ast's code for detecting the device type and features is convoluted.
> It mixes up several state fields, chip types and sub-models. Rework
> the driver into somehting more understandable.
"somehting" -> "something"
> Patches 1 fixes a long-standing bug. The affected code has never
> worked correctly.
>
> Patches 2 to 8 make various changes to the init code, or remove dead
> and duplicated code paths.
>
> Patch 9 introduces chip generations. Until now, ast used the value
> of enum ast_chip to represent a certain set of related modes, and
> also used the enum to represent individal models. This makes the
> driver code hard to understand in certain places. The patch encodes
> a chip generation in each model enum and converts the driver to use
> it.
>
> Patches 10 to 12 replace duplicated model checks with the correct
> enum value. Detection of wide-screen functionality and the transmitter
> chip can then be moved into individual functions in patch 13.
>
> Patch 14 merges the detection of the silicon revision and the chip
> model into s single function.
's' -> 'a'
>   Both need to be done in the same place
> and affect each other.
>
> Tested on AST1100 and AST2300.
>
> Thomas Zimmermann (14):
>    drm/ast: Fix DRAM init on AST2200
>    drm/ast: Remove vga2_clone field
>    drm/ast: Implement register helpers in ast_drv.h
>    drm/ast: Remove dead else branch in POST code
>    drm/ast: Remove device POSTing and config from chip detection
>    drm/ast: Set PCI config before accessing I/O registers
>    drm/ast: Enable and unlock device access early during init
>    drm/ast: Set up release action right after enabling MMIO
>    drm/ast: Distinguish among chip generations
>    drm/ast: Detect AST 1300 model
>    drm/ast: Detect AST 1400 model
>    drm/ast: Detect AST 2510 model
>    drm/ast: Move widescreen- and tx-chip detection into separate helpers
>    drm/ast: Merge config and chip detection
>
>   drivers/gpu/drm/ast/ast_dp501.c |   6 +-
>   drivers/gpu/drm/ast/ast_drv.h   |  97 +++++++---
>   drivers/gpu/drm/ast/ast_main.c  | 320 +++++++++++++++++++-------------
>   drivers/gpu/drm/ast/ast_mm.c    |   2 -
>   drivers/gpu/drm/ast/ast_mode.c  |  35 ++--
>   drivers/gpu/drm/ast/ast_post.c  |  74 ++------
>   6 files changed, 294 insertions(+), 240 deletions(-)
>
-- 
Jingfeng

Re: [PATCH 00/14] drm/ast: Refactor the device-detection code

[Jocelyn Falempe]

On 16/06/2023 15:52, Thomas Zimmermann wrote:
> Ast's code for detecting the device type and features is convoluted.
> It mixes up several state fields, chip types and sub-models. Rework
> the driver into somehting more understandable.
> 
> Patches 1 fixes a long-standing bug. The affected code has never
> worked correctly.
> 
> Patches 2 to 8 make various changes to the init code, or remove dead
> and duplicated code paths.
> 
> Patch 9 introduces chip generations. Until now, ast used the value
> of enum ast_chip to represent a certain set of related modes, and
> also used the enum to represent individal models. This makes the
> driver code hard to understand in certain places. The patch encodes
> a chip generation in each model enum and converts the driver to use
> it.

That's a very good thing, the handling of different AST revisions was a 
bit messy, and there was bugs when a new one was introduced.
> 
> Patches 10 to 12 replace duplicated model checks with the correct
> enum value. Detection of wide-screen functionality and the transmitter
> chip can then be moved into individual functions in patch 13.
> 
> Patch 14 merges the detection of the silicon revision and the chip
> model into s single function. Both need to be done in the same place
> and affect each other.
> 
> Tested on AST1100 and AST2300.

I've also tested (remotely) on AST2600

For the whole series:

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
Tested-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> Thomas Zimmermann (14):
>    drm/ast: Fix DRAM init on AST2200
>    drm/ast: Remove vga2_clone field
>    drm/ast: Implement register helpers in ast_drv.h
>    drm/ast: Remove dead else branch in POST code
>    drm/ast: Remove device POSTing and config from chip detection
>    drm/ast: Set PCI config before accessing I/O registers
>    drm/ast: Enable and unlock device access early during init
>    drm/ast: Set up release action right after enabling MMIO
>    drm/ast: Distinguish among chip generations
>    drm/ast: Detect AST 1300 model
>    drm/ast: Detect AST 1400 model
>    drm/ast: Detect AST 2510 model
>    drm/ast: Move widescreen- and tx-chip detection into separate helpers
>    drm/ast: Merge config and chip detection
> 
>   drivers/gpu/drm/ast/ast_dp501.c |   6 +-
>   drivers/gpu/drm/ast/ast_drv.h   |  97 +++++++---
>   drivers/gpu/drm/ast/ast_main.c  | 320 +++++++++++++++++++-------------
>   drivers/gpu/drm/ast/ast_mm.c    |   2 -
>   drivers/gpu/drm/ast/ast_mode.c  |  35 ++--
>   drivers/gpu/drm/ast/ast_post.c  |  74 ++------
>   6 files changed, 294 insertions(+), 240 deletions(-)
>