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

[Thomas Zimmermann <tzimmermann@suse.de>]

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