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

[Sui Jingfeng]

Hi,


I know something about this patch:


On 2023/6/21 20:53, 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. It was probably too late at this point.
>
> v2:
> 	* use standard 16-bit PCI r/w access (Jingfeng)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
> Tested-by: Jocelyn Falempe <jfalempe at redhat.com> # AST2600
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  1 -
>   drivers/gpu/drm/ast/ast_main.c | 21 +++++++++++++++++++++
>   drivers/gpu/drm/ast/ast_post.c |  6 ------
>   3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 0141705beaee9..630105feec18a 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -52,7 +52,6 @@
>   #define PCI_CHIP_AST2000 0x2000
>   #define PCI_CHIP_AST2100 0x2010
>   
> -
>   enum ast_chip {
>   	AST2000,
>   	AST2100,
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index c6987e0446618..01f938c2da28f 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -35,6 +35,23 @@
>   
>   #include "ast_drv.h"
>   
> +static int ast_init_pci_config(struct pci_dev *pdev)
> +{
> +	int err;
> +	u16 pcis04;
> +
> +	err = pci_read_config_word(pdev, PCI_COMMAND, &pcis04);
> +	if (err)
> +		goto out;
> +
> +	pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
> +
> +	err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
> +
> +out:
> +	return pcibios_err_to_errno(err);
> +}
> +


This function itself is good enough.


>   static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev)
>   {
>   	struct device_node *np = dev->dev->of_node;
> @@ -399,6 +416,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);
> +


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.


```

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?

We want to learn more knowledge from your patch.

>   
>   	ast_enable_vga(dev);
>   	ast_open_key(ast);

-- 
Jingfeng