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

[Thomas Zimmermann]

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 at 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)
-------------- 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/7d53602d/attachment.sig>