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

[Sui Jingfeng]

Hi,

On 2023/6/23 17:02, Thomas Zimmermann wrote:
> Hi
>
> Am 22.06.23 um 17:42 schrieb Sui Jingfeng:
> [...]
>>> +    ret = ast_init_pci_config(pdev);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>
>>
>> Is the calling to ast_init_pci_config() absolute necessary ?
>>
>> I'm asking this question because
>>
>> I think this function is needed to be run only when the chip need POST.
>>
>> On normal case, when you call pcim_enable_device() function,
>>
>> the pci_enable_device() function will do such thing for you.
>
> You're right. We could probably remove this code from the driver. I 
> don't want to do this in a patchset that is about refactoring. Maybe 
> in a later patch.
OK, this sound fine then.
>
>>
>>
>> ```
>>
>> int pci_enable_device(struct pci_dev *dev)
>> {
>>      return pci_enable_device_flags(dev, IORESOURCE_MEM | 
>> IORESOURCE_IO);
>> }
>>
>> ```
>>
>>>       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);
>>
>> You have changed the semantics after this patch is applied .
>>
>> Yes, it still works.
>>
>> But would you like to explain something about this?
>
> I'm looking at the function, but I don't see the change in semantics. 
> I only moved the PCI calls and added error detection. What do you mean?
>
Sorry, I'm using the wrong word. :-)

I means that this function is only needed to be call in ast_post_gpu() 
function.

In fact, on my platform the ast_post_gpu() don't get called even.

I suspect ast_post_gpu() will get called only when the firmware does not 
initial the card.

OK,  You can take another look at this function in the future.

I'm also feel OK about this series.

> Best regards
> Thomas
>
>>
>> We want to learn more knowledge from your patch.
>>
>>>       ast_enable_vga(dev);
>>>       ast_open_key(ast);
>>
>
-- 
Jingfeng