On Mon, Nov 21, 2022 at 09:43:58PM +0900, Yoshihiro Shimoda wrote: > Add R-Car Gen4 PCIe Host support. This controller is based on > Synopsys DesignWare PCIe. > > This controller doesn't support MSI-X interrupt. So, introduce > no_msix flag in struct dw_pcie_host_ops to clear MSI_FLAG_PCI_MSIX > from dw_pcie_msi_domain_info. > + /* Enable L1 Substates */ > + val = dw_pcie_readl_dbi(dw, L1PSCAP(PCI_L1SS_CTL1)); > + val &= ~PCI_L1SS_CTL1_L1SS_MASK; > + val |= PCI_L1SS_CTL1_PCIPM_L1_2 | PCI_L1SS_CTL1_PCIPM_L1_1 | > + PCI_L1SS_CTL1_ASPM_L1_2 | PCI_L1SS_CTL1_ASPM_L1_1; > + dw_pcie_writel_dbi(dw, L1PSCAP(PCI_L1SS_CTL1), val); This seems like something that ought to be done by the PCI core in pcie/aspm.c. L1.2 also depends on LTR being supported and configured. If it needs to be enabled here, can you expand the comment to say why and how LTR is being configured? > + rcar_gen4_pcie_disable_bar(dw, BAR0MASKF); > + rcar_gen4_pcie_disable_bar(dw, BAR1MASKF); > + > + /* Set Root Control */ > + val = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_RTCTL)); > + val |= PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE | > + PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE | > + PCI_EXP_RTCTL_CRSSVE; > + dw_pcie_writel_dbi(dw, EXPCAP(PCI_EXP_RTCTL), val); > + > + /* Set Interrupt Disable, SERR# Enable, Parity Error Response */ > + val = dw_pcie_readl_dbi(dw, PCI_COMMAND); > + val |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR | > + PCI_COMMAND_INTX_DISABLE; > + dw_pcie_writel_dbi(dw, PCI_COMMAND, val); > + > + /* Enable SERR */ > + val = dw_pcie_readb_dbi(dw, PCI_BRIDGE_CONTROL); > + val |= PCI_BRIDGE_CTL_SERR; > + dw_pcie_writeb_dbi(dw, PCI_BRIDGE_CONTROL, val); > + > + /* Device control */ > + val = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_DEVCTL)); > + val |= PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | > + PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE; > + dw_pcie_writel_dbi(dw, EXPCAP(PCI_EXP_DEVCTL), val); The above also looks like things that should be configured by the PCI core. > + dev_err(&pdev->dev, "Failed to initialize host\n"); > + dev_err(dev, "failed to request resource: %d\n", err); Pick a capitalization style. > + dev_err(dev, "%s: failed to resume/get Runtime PM\n", __func__); The driver name + device ID + message text printed by dev_err() should be enough that __func__ isn't needed. Bjorn
