Hi Bjorn, > From: Bjorn Helgaas, Sent: Wednesday, November 23, 2022 12:05 AM > > 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? Thank you for your review! I realized that this driver should not enable it here, as you mentioned. However, I don't know why but it needs to be enabled here. Otherwise, this driver cannot work. So, I'm investigating the issue now. > > + 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. I think so. I realized that the following settings are not needed here. So, I'll drop them. > > + dw_pcie_writel_dbi(dw, EXPCAP(PCI_EXP_RTCTL), val); > > + dw_pcie_writel_dbi(dw, PCI_COMMAND, val); > > + dw_pcie_writeb_dbi(dw, PCI_BRIDGE_CONTROL, val); > > + dw_pcie_writel_dbi(dw, EXPCAP(PCI_EXP_DEVCTL), val); > > + dev_err(&pdev->dev, "Failed to initialize host\n"); > > + dev_err(dev, "failed to request resource: %d\n", err); > > Pick a capitalization style. I'll fix the 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. I got it. I'll fix this on v8. Best regards, Yoshihiro Shimoda > Bjorn
