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 <tzimmerm...@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.cindex 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)
OpenPGP_signature
Description: OpenPGP digital signature
