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

[Bjorn Helgaas <helgaas@kernel.org>]

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