[14/14] drm/ast: Merge config and chip detection
Thomas Zimmermann
tzimmermann at suse.de
Mon Jun 19 08:40:41 UTC 2023
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 at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230619/e097557b/attachment-0001.sig>
More information about the dri-devel
mailing list