RE: [PATCH v7 7/9] PCI: rcar-gen4: Add R-Car Gen4 PCIe Host support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux