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

[Sui Jingfeng <suijingfeng@xxxxxxxxxxx>]

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.

> It was probably too late at this point.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>   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 ?

>   
>   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);
}

>   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);

--
Jingfeng