[PATCH 09/10] drm/ast: Detect ast device type and config mode without ast device
Thomas Zimmermann
tzimmermann at suse.de
Tue Nov 14 07:47:59 UTC 2023
Hi
Am 13.11.23 um 16:25 schrieb Jocelyn Falempe:
> On 13/11/2023 09:50, Thomas Zimmermann wrote:
>> Return the ast chip and config in the detection function's parameters
>> instead of storing them directly in the ast device instance.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/ast/ast_main.c | 104 ++++++++++++++++++---------------
>> 1 file changed, 57 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_main.c
>> b/drivers/gpu/drm/ast/ast_main.c
>> index f100df8d74f71..331a9a861153b 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -76,25 +76,27 @@ static void ast_open_key(void __iomem *ioregs)
>> __ast_write8_i(ioregs, AST_IO_VGACRI, 0x80,
>> AST_IO_VGACR80_PASSWORD);
>> }
>> -static int ast_device_config_init(struct ast_device *ast)
>> +static int ast_detect_chip(struct pci_dev *pdev,
>> + void __iomem *regs, void __iomem *ioregs,
>> + enum ast_chip *chip_out,
>> + enum ast_config_mode *config_mode_out)
>> {
>> - struct drm_device *dev = &ast->base;
>> - struct pci_dev *pdev = to_pci_dev(dev->dev);
>> - struct device_node *np = dev->dev->of_node;
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + enum ast_config_mode config_mode = ast_use_defaults;
>> uint32_t scu_rev = 0xffffffff;
>> + enum ast_chip chip;
>> u32 data;
>> - u8 jregd0, jregd1;
>> + u8 vgacrd0, vgacrd1;
>> /*
>> * Find configuration mode and read SCU revision
>> */
>> - ast->config_mode = ast_use_defaults;
>> -
>> /* Check if we have device-tree properties */
>> if (np && !of_property_read_u32(np, "aspeed,scu-revision-id",
>> &data)) {
>> /* We do, disable P2A access */
>> - ast->config_mode = ast_use_dt;
>> + config_mode = ast_use_dt;
>> scu_rev = data;
>> } else if (pdev->device == PCI_CHIP_AST2000) { // Not all
>> families have a P2A bridge
>> /*
>> @@ -102,9 +104,9 @@ static int ast_device_config_init(struct
>> ast_device *ast)
>> * is disabled. We force using P2A if VGA only mode bit
>> * is set D[7]
>> */
>> - jregd0 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd0, 0xff);
>> - jregd1 = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
>> - if (!(jregd0 & 0x80) || !(jregd1 & 0x10)) {
>> + vgacrd0 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd0);
>> + vgacrd1 = __ast_read8_i(ioregs, AST_IO_VGACRI, 0xd1);
>> + if (!(vgacrd0 & 0x80) || !(vgacrd1 & 0x10)) {
>> /*
>> * We have a P2A bridge and it is enabled.
>> @@ -112,32 +114,32 @@ static int ast_device_config_init(struct
>> ast_device *ast)
>> /* Patch AST2500/AST2510 */
>> if ((pdev->revision & 0xf0) == 0x40) {
>> - if (!(jregd0 & AST_VRAM_INIT_STATUS_MASK))
>> - ast_patch_ahb_2500(ast->regs);
>> + if (!(vgacrd0 & AST_VRAM_INIT_STATUS_MASK))
>> + ast_patch_ahb_2500(regs);
>> }
>> /* Double check that it's actually working */
>> - data = ast_read32(ast, 0xf004);
>> + data = __ast_read32(regs, 0xf004);
>> if ((data != 0xffffffff) && (data != 0x00)) {
>> - ast->config_mode = ast_use_p2a;
>> + config_mode = ast_use_p2a;
>> /* Read SCU7c (silicon revision register) */
>> - ast_write32(ast, 0xf004, 0x1e6e0000);
>> - ast_write32(ast, 0xf000, 0x1);
>> - scu_rev = ast_read32(ast, 0x1207c);
>> + __ast_write32(regs, 0xf004, 0x1e6e0000);
>> + __ast_write32(regs, 0xf000, 0x1);
>> + scu_rev = __ast_read32(regs, 0x1207c);
>> }
>> }
>> }
>> - switch (ast->config_mode) {
>> + switch (config_mode) {
>> case ast_use_defaults:
>> - drm_info(dev, "Using default configuration\n");
>> + dev_info(dev, "Using default configuration\n");
>> break;
>> case ast_use_dt:
>> - drm_info(dev, "Using device-tree for configuration\n");
>> + dev_info(dev, "Using device-tree for configuration\n");
>> break;
>> case ast_use_p2a:
>> - drm_info(dev, "Using P2A bridge for configuration\n");
>> + dev_info(dev, "Using P2A bridge for configuration\n");
>> break;
>> }
>> @@ -146,63 +148,66 @@ static int ast_device_config_init(struct
>> ast_device *ast)
>> */
>> if (pdev->revision >= 0x50) {
>> - ast->chip = AST2600;
>> - drm_info(dev, "AST 2600 detected\n");
>> + chip = AST2600;
>> + dev_info(dev, "AST 2600 detected\n");
>
> Adding a macro to set chip and printk could be handy here:
>
> something like
>
> #define set_chip(version) \
> chip = version; \
> dev_info(dev, "%s detected\n", #version); \
>
>
> and then set_chip(AST2510)
I was thinking about reworking these messages at some point. Maybe have
a single print somewhere in the code.
>
>
>> } else if (pdev->revision >= 0x40) {
>> switch (scu_rev & 0x300) {
>> case 0x0100:
>> - ast->chip = AST2510;
>> - drm_info(dev, "AST 2510 detected\n");
>> + chip = AST2510;
>> + dev_info(dev, "AST 2510 detected\n");
>> break;
>> default:
>> - ast->chip = AST2500;
>> - drm_info(dev, "AST 2500 detected\n");
>> + chip = AST2500;
>> + dev_info(dev, "AST 2500 detected\n");
>
> Should the default case have break ?
> This one has no break, but later in this function they do. Maybe we can
> have more consistency ?
Sure, of course.
Best regards
Thomas
>
>
>> }
>> } else if (pdev->revision >= 0x30) {
>> switch (scu_rev & 0x300) {
>> case 0x0100:
>> - ast->chip = AST1400;
>> - drm_info(dev, "AST 1400 detected\n");
>> + chip = AST1400;
>> + dev_info(dev, "AST 1400 detected\n");
>> break;
>> default:
>> - ast->chip = AST2400;
>> - drm_info(dev, "AST 2400 detected\n");
>> + chip = AST2400;
>> + dev_info(dev, "AST 2400 detected\n");
>> }
>> } else if (pdev->revision >= 0x20) {
>> switch (scu_rev & 0x300) {
>> case 0x0000:
>> - ast->chip = AST1300;
>> - drm_info(dev, "AST 1300 detected\n");
>> + chip = AST1300;
>> + dev_info(dev, "AST 1300 detected\n");
>> break;
>> default:
>> - ast->chip = AST2300;
>> - drm_info(dev, "AST 2300 detected\n");
>> + chip = AST2300;
>> + dev_info(dev, "AST 2300 detected\n");
>> break;
>> }
>> } else if (pdev->revision >= 0x10) {
>> switch (scu_rev & 0x0300) {
>> case 0x0200:
>> - ast->chip = AST1100;
>> - drm_info(dev, "AST 1100 detected\n");
>> + chip = AST1100;
>> + dev_info(dev, "AST 1100 detected\n");
>> break;
>> case 0x0100:
>> - ast->chip = AST2200;
>> - drm_info(dev, "AST 2200 detected\n");
>> + chip = AST2200;
>> + dev_info(dev, "AST 2200 detected\n");
>> break;
>> case 0x0000:
>> - ast->chip = AST2150;
>> - drm_info(dev, "AST 2150 detected\n");
>> + chip = AST2150;
>> + dev_info(dev, "AST 2150 detected\n");
>> break;
>> default:
>> - ast->chip = AST2100;
>> - drm_info(dev, "AST 2100 detected\n");
>> + chip = AST2100;
>> + dev_info(dev, "AST 2100 detected\n");
>> break;
>> }
>> } else {
>> - ast->chip = AST2000;
>> - drm_info(dev, "AST 2000 detected\n");
>> + chip = AST2000;
>> + dev_info(dev, "AST 2000 detected\n");
>> }
>> + *chip_out = chip;
>> + *config_mode_out = config_mode;
>> +
>> return 0;
>> }
>> @@ -431,6 +436,8 @@ struct ast_device *ast_device_create(const struct
>> drm_driver *drv,
>> int ret = 0;
>> void __iomem *regs;
>> void __iomem *ioregs;
>> + enum ast_config_mode config_mode;
>> + enum ast_chip chip;
>> ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_device, base);
>> if (IS_ERR(ast))
>> @@ -502,10 +509,13 @@ struct ast_device *ast_device_create(const
>> struct drm_driver *drv,
>> if (ret)
>> return ERR_PTR(ret);
>> - ret = ast_device_config_init(ast);
>> + ret = ast_detect_chip(pdev, regs, ioregs, &chip, &config_mode);
>> if (ret)
>> return ERR_PTR(ret);
>> + ast->chip = chip;
>> + ast->config_mode = config_mode;
>> +
>> ast_detect_widescreen(ast);
>> ast_detect_tx_chip(ast, need_post);
>
> Thanks for your patch,
>
> Best regards,
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231114/0245f141/attachment.sig>
More information about the dri-devel
mailing list