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@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.hindex 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.cindex 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.cindex 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 |= 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)
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
