[PATCH V4] PCI: Add MCFG quirks for Tegra194 host controllers
The PCIe controller in Tegra194 SoC is not completely ECAM-compliant.
With the current hardware design limitations in place, ECAM can be enabled
only for one controller (C5 controller to be precise) with bus numbers
starting from 160 instead of 0. A different approach is taken to avoid this
abnormal way of enabling ECAM for just one controller but to enable
configuration space access for all the other controllers. In this approach,
ops are added through MCFG quirk mechanism which access the configuration
spaces by dynamically programming iATU (internal AddressTranslation Unit)
to generate respective configuration accesses just like the way it is
done in DesignWare core sub-system.
This issue is specific to Tegra194 and it would be fixed in the future
generations of Tegra SoCs.
Signed-off-by: Vidya Sagar
---
V4:
* Addressed Bjorn's review comments
* Rebased changes on top of Lorenzo's pci/dwc branch
V3:
* Removed MCFG address hardcoding in pci_mcfg.c file
* Started using 'dbi_base' for accessing root port's own config space
* and using 'config_base' for accessing config space of downstream hierarchy
V2:
* Fixed build issues reported by kbuild test bot
drivers/acpi/pci_mcfg.c| 7 ++
drivers/pci/controller/dwc/Makefile| 2 +-
drivers/pci/controller/dwc/pcie-tegra194.c | 103 +
include/linux/pci-ecam.h | 1 +
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 95f23acd5b80..53cab975f612 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -116,6 +116,13 @@ static struct mcfg_fixup mcfg_quirks[] = {
THUNDER_ECAM_QUIRK(2, 12),
THUNDER_ECAM_QUIRK(2, 13),
+ { "NVIDIA", "TEGRA194", 1, 0, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 1, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 2, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 3, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 4, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 5, MCFG_BUS_ANY, _pcie_ops},
+
#define XGENE_V1_ECAM_MCFG(rev, seg) \
{"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \
_v1_pcie_ecam_ops }
diff --git a/drivers/pci/controller/dwc/Makefile
b/drivers/pci/controller/dwc/Makefile
index 625f6aaeb5b8..2da826ef18ac 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -18,7 +18,6 @@ obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
obj-$(CONFIG_PCI_MESON) += pci-meson.o
-obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
@@ -35,4 +34,5 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
ifdef CONFIG_PCI
obj-$(CONFIG_ARM64) += pcie-al.o
obj-$(CONFIG_ARM64) += pcie-hisi.o
+obj-$(CONFIG_ARM64) += pcie-tegra194.o
endif
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 6fa216e52d14..cb38e94a3033 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -22,6 +22,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -311,6 +313,104 @@ struct tegra_pcie_dw_of_data {
enum dw_pcie_device_mode mode;
};
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
+struct tegra194_pcie_ecam {
+ void __iomem *config_base;
+ void __iomem *iatu_base;
+ void __iomem *dbi_base;
+};
+
+static int tegra194_acpi_init(struct pci_config_window *cfg)
+{
+ struct device *dev = cfg->parent;
+ struct tegra194_pcie_ecam *pcie_ecam;
+
+ pcie_ecam = devm_kzalloc(dev, sizeof(*pcie_ecam), GFP_KERNEL);
+ if (!pcie_ecam)
+ return -ENOMEM;
+
+ pcie_ecam->config_base = cfg->win;
+ pcie_ecam->iatu_base = cfg->win + SZ_256K;
+ pcie_ecam->dbi_base = cfg->win + SZ_512K;
+ cfg->priv = pcie_ecam;
+
+ return 0;
+}
+
+static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
+ u32 val, u32 reg)
+{
+ u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
+
+ writel(val, pcie_ecam->iatu_base + offset + reg);
+}
+
+static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
+int index, int type, u64 cpu_addr,
+u64 pci_addr, u64 size)
+{
+ atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr),
+ PCIE_ATU_LOWER_BASE);
+ atu_reg_write(pcie_ecam, index, upper_32_bits(cpu_addr),
+ PCIE_ATU_UPPER_BASE)
Re: [PATCH V3 2/2] PCI: Add MCFG quirks for Tegra194 host controllers
On 3/6/2021 3:27 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
[+cc Krzysztof for .bus_shift below]
This is [2/2] but I don't see a [1/2]. Is there something missing?
On Sat, Jan 11, 2020 at 12:45:00AM +0530, Vidya Sagar wrote:
The PCIe controller in Tegra194 SoC is not completely ECAM-compliant.
With the current hardware design limitations in place, ECAM can be enabled
only for one controller (C5 controller to be precise) with bus numbers
starting from 160 instead of 0. A different approach is taken to avoid this
abnormal way of enabling ECAM for just one controller but to enable
configuration space access for all the other controllers. In this approach,
ops are added through MCFG quirk mechanism which access the configuration
spaces by dynamically programming iATU (internal AddressTranslation Unit)
to generate respective configuration accesses just like the way it is
done in DesignWare core sub-system.
Is this a published erratum in the device? The purpose of specs is so
we can run existing code on new platforms without having to add quirks
like this, so I'm looking for some acknowledgement that this is an
issue that will be fixed in future designs.
Yes. This would be fixed in the following SoC.
Ideally this would be a URL to published errata, and we would include
the text or a synopsis here in the commit log.
Signed-off-by: Vidya Sagar
Reported-by: kbuild test robot
What is this "Reported-by" telling me? Normally this would be a
person who could supply more information about a defect we're fixing
and might be able to test the fix.
I'll remove this.
---
V3:
* Removed MCFG address hardcoding in pci_mcfg.c file
* Started using 'dbi_base' for accessing root port's own config space
* and using 'config_base' for accessing config space of downstream hierarchy
V2:
* Fixed build issues reported by kbuild test bot
Ah, I see this is probably where the "Reported-by" came from. To me,
it would make sense to add the tag if the commit *only* fixes the
build problem so it's obvious what the robot reported.
But here, the build fix got squashed in before merging, so it's more
like a general review comment and I think the robot's response on the
mailing list is probably enough.
drivers/acpi/pci_mcfg.c| 7 ++
drivers/pci/controller/dwc/Kconfig | 3 +-
drivers/pci/controller/dwc/Makefile| 2 +-
drivers/pci/controller/dwc/pcie-tegra194.c | 102 +
include/linux/pci-ecam.h | 1 +
5 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 6b347d9920cc..707181408173 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -116,6 +116,13 @@ static struct mcfg_fixup mcfg_quirks[] = {
THUNDER_ECAM_QUIRK(2, 12),
THUNDER_ECAM_QUIRK(2, 13),
+ { "NVIDIA", "TEGRA194", 1, 0, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 1, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 2, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 3, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 4, MCFG_BUS_ANY, _pcie_ops},
+ { "NVIDIA", "TEGRA194", 1, 5, MCFG_BUS_ANY, _pcie_ops},
+
#define XGENE_V1_ECAM_MCFG(rev, seg) \
{"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \
_v1_pcie_ecam_ops }
diff --git a/drivers/pci/controller/dwc/Kconfig
b/drivers/pci/controller/dwc/Kconfig
index 0830dfcfa43a..f5b9e75aceed 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -255,7 +255,8 @@ config PCIE_TEGRA194
select PHY_TEGRA194_P2U
help
Say Y here if you want support for DesignWare core based PCIe host
- controller found in NVIDIA Tegra194 SoC.
+ controller found in NVIDIA Tegra194 SoC. ACPI platforms with Tegra194
+ don't need to enable this.
config PCIE_UNIPHIER
bool "Socionext UniPhier PCIe controllers"
diff --git a/drivers/pci/controller/dwc/Makefile
b/drivers/pci/controller/dwc/Makefile
index 8a637cfcf6e9..76a6c52b8500 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -17,7 +17,6 @@ obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
obj-$(CONFIG_PCI_MESON) += pci-meson.o
-obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
# The following drivers are for devices that use the generic ACPI
@@ -33,4 +32,5 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
ifdef CONFIG_PCI
obj-$(CONFIG_ARM64) += pcie-al.o
obj-$(CONFIG_ARM64) += pcie-hisi.o
+obj-$(CONFIG_ARM64) += pcie-tegra194.o
endif
diff --git a/drivers/pci/c
Re: Device driver location for the PCIe root port's DMA engine
On 4/13/2021 11:43 PM, Rob Herring wrote: External email: Use caution opening links or attachments On Mon, Apr 12, 2021 at 12:01 PM Vidya Sagar wrote: Hi I'm starting this mail to seek advice on the best approach to be taken to add support for the driver of the PCIe root port's DMA engine. To give some background, Tegra194's PCIe IPs are dual-mode PCIe IPs i.e. they work either in the root port mode or in the endpoint mode based on the boot time configuration. Since the PCIe hardware IP as such is the same for both (RP and EP) modes, the DMA engine sub-system of the PCIe IP is also made available to both modes of operation. Typically, the DMA engine is seen only in the endpoint mode, and that DMA engine’s configuration registers are made available to the host through one of its BARs. In the situation that we have here, where there is a DMA engine present as part of the root port, the DMA engine isn’t a typical general-purpose DMA engine in the sense that it can’t have both source and destination addresses targeting external memory addresses. RP’s DMA engine, while doing a write operation, would always fetch data (i.e. source) from local memory and write it to the remote memory over PCIe link (i.e. destination would be the BAR of an endpoint) whereas while doing a read operation, would always fetch/read data (i.e. source) from a remote memory over the PCIe link and write it to the local memory. I see that there are at least two ways we can have a driver for this DMA engine. a) DMA engine driver as one of the port service drivers Since the DMA engine is a part of the root port hardware itself (although it is not part of the standard capabilities of the root port), it is one of the options to have the driver for the DMA engine go as one of the port service drivers (along with AER, PME, hot-plug, etc...). Based on Vendor-ID and Device-ID matching runtime, either it gets binded/enabled (like in the case of Tegra194) or it doesn't. b) DMA engine driver as a platform driver The DMA engine hardware can be described as a sub-node under the PCIe controller's node in the device tree and a separate platform driver can be written to work with it. DT expects PCI bridge child nodes to be a PCI device. We've already broken that with the interrupt controller child nodes, but I don't really want to add more. Understood. Is there any other way of specifying the DMA functionality other than as a child node so that it is inline with the DT framework's expectations? I’m inclined to have the DMA engine driver as a port service driver as it makes it cleaner and also in line with the design philosophy (the way I understood it) of the port service drivers. Please let me know your thoughts on this. What is the actual usecase and benefit for using the DMA engine with the RP? The only one I've come up with is the hardware designers think having DMA is better than not having DMA so they include that option on the DWC controller. In Tegra194-to-Tegra194 configuration (with one Tegra194 as RP and the other as EP) better performance is expected when DMA engines on both sides are used for pushing(writing) the data across instead of using only the EP's DMA engine for both push(write) and pull(read). Rob
Re: Device driver location for the PCIe root port's DMA engine
On 4/13/2021 3:23 AM, Bjorn Helgaas wrote: External email: Use caution opening links or attachments [+cc Matthew for portdrv comment] On Mon, Apr 12, 2021 at 10:31:02PM +0530, Vidya Sagar wrote: Hi I'm starting this mail to seek advice on the best approach to be taken to add support for the driver of the PCIe root port's DMA engine. To give some background, Tegra194's PCIe IPs are dual-mode PCIe IPs i.e. they work either in the root port mode or in the endpoint mode based on the boot time configuration. Since the PCIe hardware IP as such is the same for both (RP and EP) modes, the DMA engine sub-system of the PCIe IP is also made available to both modes of operation. Typically, the DMA engine is seen only in the endpoint mode, and that DMA engine’s configuration registers are made available to the host through one of its BARs. In the situation that we have here, where there is a DMA engine present as part of the root port, the DMA engine isn’t a typical general-purpose DMA engine in the sense that it can’t have both source and destination addresses targeting external memory addresses. RP’s DMA engine, while doing a write operation, would always fetch data (i.e. source) from local memory and write it to the remote memory over PCIe link (i.e. destination would be the BAR of an endpoint) whereas while doing a read operation, would always fetch/read data (i.e. source) from a remote memory over the PCIe link and write it to the local memory. I see that there are at least two ways we can have a driver for this DMA engine. a) DMA engine driver as one of the port service drivers Since the DMA engine is a part of the root port hardware itself (although it is not part of the standard capabilities of the root port), it is one of the options to have the driver for the DMA engine go as one of the port service drivers (along with AER, PME, hot-plug, etc...). Based on Vendor-ID and Device-ID matching runtime, either it gets binded/enabled (like in the case of Tegra194) or it doesn't. b) DMA engine driver as a platform driver The DMA engine hardware can be described as a sub-node under the PCIe controller's node in the device tree and a separate platform driver can be written to work with it. I’m inclined to have the DMA engine driver as a port service driver as it makes it cleaner and also in line with the design philosophy (the way I understood it) of the port service drivers. Please let me know your thoughts on this. Personally I'm not a fan of the port service driver model. It creates additional struct devices for things that are not separate devices. And it creates a parallel hierarchy in /sys/bus/pci_express/devices/ that I think does not accurately model the hardware. Agree. The existing port services (AER, DPC, hotplug, etc) are things the device advertises via the PCI Capabilities defined by the generic PCIe spec, and in my opinion the support for them should be directly part of the PCI core and activated when the relevant Capability is present. Is there an on-going activity to remove port service drivers are move AER/DPC/Hotplug etc.. handling within PCI core? The DMA engine is different -- this is device-specific functionality and I think the obvious way to discover it and bind a driver to it is via the PCI Vendor and Device ID. This *is* complicated by the fact that you can't just use pci_register_driver() to claim functionality implemented in Root Ports or Switch Ports because portdrv binds to them before you have a chance. I think that's a defect in the portdrv design. The usual Yes. Hence I was thinking of adding a service driver for the DMA functionality workaround is to use pci_get_device(), which has its own issues (it's ugly, it's outside the normal driver binding model, doesn't work nicely with hotplug or udev, doesn't coordinate with other drivers using the same device, etc). There are many examples of this in the EDAC code. I didn't think of approaching this issue in this way. Thanks for the pointers to EDAC code. I'll wait for comments from Matthew before I proceed with this approach. Bjorn
Device driver location for the PCIe root port's DMA engine
Hi I'm starting this mail to seek advice on the best approach to be taken to add support for the driver of the PCIe root port's DMA engine. To give some background, Tegra194's PCIe IPs are dual-mode PCIe IPs i.e. they work either in the root port mode or in the endpoint mode based on the boot time configuration. Since the PCIe hardware IP as such is the same for both (RP and EP) modes, the DMA engine sub-system of the PCIe IP is also made available to both modes of operation. Typically, the DMA engine is seen only in the endpoint mode, and that DMA engine’s configuration registers are made available to the host through one of its BARs. In the situation that we have here, where there is a DMA engine present as part of the root port, the DMA engine isn’t a typical general-purpose DMA engine in the sense that it can’t have both source and destination addresses targeting external memory addresses. RP’s DMA engine, while doing a write operation, would always fetch data (i.e. source) from local memory and write it to the remote memory over PCIe link (i.e. destination would be the BAR of an endpoint) whereas while doing a read operation, would always fetch/read data (i.e. source) from a remote memory over the PCIe link and write it to the local memory. I see that there are at least two ways we can have a driver for this DMA engine. a) DMA engine driver as one of the port service drivers Since the DMA engine is a part of the root port hardware itself (although it is not part of the standard capabilities of the root port), it is one of the options to have the driver for the DMA engine go as one of the port service drivers (along with AER, PME, hot-plug, etc...). Based on Vendor-ID and Device-ID matching runtime, either it gets binded/enabled (like in the case of Tegra194) or it doesn't. b) DMA engine driver as a platform driver The DMA engine hardware can be described as a sub-node under the PCIe controller's node in the device tree and a separate platform driver can be written to work with it. I’m inclined to have the DMA engine driver as a port service driver as it makes it cleaner and also in line with the design philosophy (the way I understood it) of the port service drivers. Please let me know your thoughts on this. Thanks, Vidya Sagar
Re: RFC: sysfs node for Secondary PCI bus reset (PCIe Hot Reset)
On 3/3/2021 11:26 PM, Pali Rohár wrote:
External email: Use caution opening links or attachments
On Tuesday 02 March 2021 12:58:29 Alex Williamson wrote:
On Mon, 1 Mar 2021 14:28:17 -0600
Bjorn Helgaas wrote:
[+cc Alex, reset expert]
On Mon, Mar 01, 2021 at 06:12:21PM +0100, Pali Rohár wrote:
Hello!
PCIe card can be reset via in-band Hot Reset signal which can be
triggered by PCIe bridge via Secondary Bus Reset bit in PCI config
space.
Kernel already exports sysfs node "reset" for triggering Functional
Reset of particular function of PCI device. But in some cases Functional
Reset is not enough and Hot Reset is required.
Following RFC patch exports sysfs node "reset_bus" for PCI bridges which
triggers Secondary Bus Reset and therefore for PCIe bridges it resets
connected PCIe card.
What do you think about it?
Currently there is userspace script which can trigger PCIe Hot Reset by
modifying PCI config space from userspace:
https://alexforencich.com/wiki/en/pcie/hot-reset-linux
But because kernel already provides way how to trigger Functional Reset
it could provide also way how to trigger PCIe Hot Reset.
What that script does and what this does, or what the existing reset
attribute does, are very different. The script finds the upstream
bridge for a given device, removes the device (ignoring that more than
one device might be affected by the bus reset), uses setpci to trigger
a secondary bus reset, then rescans devices. The below only triggers
the secondary bus reset, neither saving and restoring affected device
state like the existing function level reset attribute, nor removing
and rescanning as the script does. It simply leaves an entire
hierarchy of PCI devices entirely un-programmed yet still has struct
pci_devs attached to them for untold future misery.
In fact, for the case of a single device affected by the bus reset, as
intended by the script, the existing reset attribute will already do
that if the device supports no other reset mechanism. There's actually
a running LFX mentorship project that aims to allow the user to control
the type of reset performed by the existing reset attribute such that a
user could force the bus reset behavior over other reset methods.
Hello Alex? Do you have a link for this "reset" project? I'm interesting
in it as I'm dealing with Compex wifi cards which are causing problems.
For correct initialization I need to issue PCIe Warm Reset for these
cards (Warm Reset is done via PERST# pin which most linux controller
drivers controls via GPIO subsystem). And for now there is no way to
trigger PCIe Warm Reset for particular PCIe device from userspace. As
there is no userspace <--> kernel API for it.
There might be some justification for an attribute that actually
implements the referenced script correctly, perhaps in kernel we could
avoid races with bus rescans, but simply triggering an SBR to quietly
de-program all downstream devices with no state restore or device
rescan is not it. Any affected device would be unusable. Was this
tested? Thanks,
I have tested my change. First I called 'remove' attribute for PCIe
card, then I called this 'bus_reset' on parent PCIe bridge and later I
called 'rescan' attribute on bridge. It correctly rested tested ath9k
card. So I did something similar as in above script. But I agree that
there are race conditions and basically lot of other calls needs to be
done to restore state.
So I see that to make it 'usable' we need to do it automatically in
kernel and also rescan/restore state of PCIe devices behind bridge after
reset...
But, is save-restore alone going to be enough? I mean what is the state
of the device-driver going to be when the device is going through the
reset process? Isn't remove-rescan the correct thing to do here rather
than save/restore?
- Vidya Sagar
Alex
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 50fcb62d59b5..f5e11c589498 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1321,6 +1321,30 @@ static ssize_t reset_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR(reset, 0200, NULL, reset_store);
+static ssize_t reset_bus_store(struct device *dev, struct device_attribute
*attr,
+const char *buf, size_t count)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ unsigned long val;
+ ssize_t result = kstrtoul(buf, 0, );
+
+ if (result < 0)
+ return result;
+
+ if (val != 1)
+ return -EINVAL;
+
+ pm_runtime_get_sync(dev);
+ result = pci_bridge_secondary_bus_reset(pdev);
+ pm_runtime_put(dev);
+ if (result < 0)
+ return result;
+
+ return count;
+}
+
+static DEVICE_ATTR(reset_bus, 0200, NULL, reset_bus_store);
+
static int pci_create_capabilities_sysfs(struct pci_dev *dev)
{
int retval;
@@ -1332,8 +1356,15 @@ static int pci_create_capabilities_sysfs(struct pci_dev
*dev)
if (retval)
Re: [PATCH] PCI: tegra: Disable PTM capabilities for EP mode
On 3/5/2021 5:49 PM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Fri, Mar 05, 2021 at 01:42:34PM +0530, Om Prakash Singh wrote:
PCIe EP compliance expect PTM capabilities (ROOT_CAPABLE, RES_CAPABLE,
CLK_GRAN) to be disabled.
I guess this is just enforcing the PCIe spec requirements that only
Root Ports, RCRBs, and Switches are allowed to set the PTM Responder
Capable bit, and that the Local Clock Granularity is RsvdP if PTM Root
Capable is zero? (PCIe r5.0, sec 7.9.16.2)
Should this be done more generally somewhere in the dwc code as
opposed to in the tegra code?
Agree.
Signed-off-by: Om Prakash Singh
---
drivers/pci/controller/dwc/pcie-tegra194.c | 17 -
include/uapi/linux/pci_regs.h | 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 6fa216e..a588312 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1639,7 +1639,7 @@ static void pex_ep_event_pex_rst_deassert(struct
tegra_pcie_dw *pcie)
struct dw_pcie *pci = >pci;
struct dw_pcie_ep *ep = >ep;
struct device *dev = pcie->dev;
- u32 val;
+ u32 val, ptm_cap_base = 0;
Unnecessary init.
int ret;
if (pcie->ep_state == EP_STATE_ENABLED)
@@ -1760,6 +1760,21 @@ static void pex_ep_event_pex_rst_deassert(struct
tegra_pcie_dw *pcie)
PCI_CAP_ID_EXP);
clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
+ /* Disable PTM root and responder capability */
+ ptm_cap_base = dw_pcie_find_ext_capability(>pci,
+PCI_EXT_CAP_ID_PTM);
+ if (ptm_cap_base) {
+ dw_pcie_dbi_ro_wr_en(pci);
+ val = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP);
+ val &= ~PCI_PTM_CAP_ROOT;
+ dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, val);
+
+ val = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP);
+ val &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK);
Why can't this be clubbed with "val &= ~PCI_PTM_CAP_ROOT;" ?
+ dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, val);
+ dw_pcie_dbi_ro_wr_dis(pci);
+ }
+
val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK);
val |= MSIX_ADDR_MATCH_LOW_OFF_EN;
dw_pcie_writel_dbi(pci, MSIX_ADDR_MATCH_LOW_OFF, val);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8..9dd6f8d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1050,6 +1050,7 @@
/* Precision Time Measurement */
#define PCI_PTM_CAP 0x04/* PTM Capability */
#define PCI_PTM_CAP_REQ 0x0001 /* Requester capable */
+#define PCI_PTM_CAP_RES 0x0002 /* Responder capable */
#define PCI_PTM_CAP_ROOT0x0004 /* Root capable */
#define PCI_PTM_GRANULARITY_MASK0xFF00 /* Clock granularity */
#define PCI_PTM_CTRL 0x08/* PTM Control */
--
2.7.4
Re: [PATCH] arm64: PCI: Enable SMC conduit
On 1/5/2021 10:27 AM, Jeremy Linton wrote:
External email: Use caution opening links or attachments
Given that most arm64 platform's PCI implementations needs quirks
to deal with problematic config accesses, this is a good place to
apply a firmware abstraction. The ARM PCI SMMCCC spec details a
standard SMC conduit designed to provide a simple PCI config
accessor. This specification enhances the existing ACPI/PCI
abstraction and expects power, config, etc functionality is handled
by the platform. It also is very explicit that the resulting config
space registers must behave as is specified by the pci specification.
Lets hook the normal ACPI/PCI config path, and when we detect
missing MADT data, attempt to probe the SMC conduit. If the conduit
exists and responds for the requested segment number (provided by the
ACPI namespace) attach a custom pci_ecam_ops which redirects
all config read/write requests to the firmware.
This patch is based on the Arm PCI Config space access document @
https://developer.arm.com/documentation/den0115/latest
Signed-off-by: Jeremy Linton
---
arch/arm64/kernel/pci.c | 87 +++
include/linux/arm-smccc.h | 26
2 files changed, 113 insertions(+)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 1006ed2d7c60..56d3773aaa25 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -7,6 +7,7 @@
*/
#include
+#include
#include
#include
#include
@@ -107,6 +108,90 @@ static int pci_acpi_root_prepare_resources(struct
acpi_pci_root_info *ci)
return status;
}
+static int smccc_pcie_check_conduit(u16 seg)
+{
+ struct arm_smccc_res res;
+
+ if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
+ return -EOPNOTSUPP;
+
+ arm_smccc_smc(SMCCC_PCI_VERSION, 0, 0, 0, 0, 0, 0, 0, );
+ if ((int)res.a0 < 0)
+ return -EOPNOTSUPP;
+
+ arm_smccc_smc(SMCCC_PCI_SEG_INFO, seg, 0, 0, 0, 0, 0, 0, );
+ if ((int)res.a0 < 0)
+ return -EOPNOTSUPP;
+
+ pr_info("PCI: SMC conduit attached to segment %d\n", seg);
Shouldn't this print be moved towards the end of
pci_acpi_setup_smccc_mapping() API?
+
+ return 0;
+}
+
+static int smccc_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 *val)
+{
+ struct arm_smccc_res res;
+
+ devfn |= bus->number << 8;
+ devfn |= bus->domain_nr << 16;
+
+ arm_smccc_smc(SMCCC_PCI_READ, devfn, where, size, 0, 0, 0, 0, );
+ if (res.a0) {
+ *val = ~0;
+ return -PCIBIOS_BAD_REGISTER_NUMBER;
+ }
+
+ *val = res.a1;
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int smccc_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 val)
+{
+ struct arm_smccc_res res;
+
+ devfn |= bus->number << 8;
+ devfn |= bus->domain_nr << 16;
+
+ arm_smccc_smc(SMCCC_PCI_WRITE, devfn, where, size, val, 0, 0, 0, );
+ if (res.a0)
+ return -PCIBIOS_BAD_REGISTER_NUMBER;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static const struct pci_ecam_ops smccc_pcie_ecam_ops = {
+ .bus_shift = 8,
+ .pci_ops= {
+ .read = smccc_pcie_config_read,
+ .write = smccc_pcie_config_write,
+ }
+};
+
+static struct pci_config_window *
+pci_acpi_setup_smccc_mapping(struct acpi_pci_root *root)
+{
+ struct device *dev = >device->dev;
+ struct resource *bus_res = >secondary;
+ struct pci_config_window *cfg;
+
+ cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return ERR_PTR(-ENOMEM);
+
+ cfg->parent = dev;
+ cfg->ops = _pcie_ecam_ops;
+ cfg->busr.start = bus_res->start;
+ cfg->busr.end = bus_res->end;
+ cfg->busr.flags = IORESOURCE_BUS;
+
+ cfg->res.name = "PCI SMCCC";
+ if (cfg->ops->init)
Since there is no init implemented, what is the purpose of having this?
+ cfg->ops->init(cfg);
+ return cfg;
+}
+
/*
* Lookup the bus range for the domain in MCFG, and set up config space
* mapping.
@@ -125,6 +210,8 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
ret = pci_mcfg_lookup(root, , _ops);
if (ret) {
+ if (!smccc_pcie_check_conduit(seg))
+ return pci_acpi_setup_smccc_mapping(root);
dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
return NULL;
}
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index f860645f6512..327f52533c71 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -89,6 +89,32 @@
#define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED 1
+/* PCI ECAM conduit */
+#define SMCCC_PCI_VERSION
Re: [PATCH] PCI:tegra:Correct typo for PCIe endpoint mode in Tegra194
Thanks for the patch. Acked-by: Vidya Sagar On 12/31/2020 8:55 AM, Wesley Sheng wrote: External email: Use caution opening links or attachments In config PCIE_TEGRA194_EP the mode incorrectly referred to host mode. Signed-off-by: Wesley Sheng --- drivers/pci/controller/dwc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 044a3761c44f..6960babe6161 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -273,7 +273,7 @@ config PCIE_TEGRA194_EP select PCIE_TEGRA194 help Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to - work in host mode. There are two instances of PCIe controllers in + work in endpoint mode. There are two instances of PCIe controllers in Tegra194. This controller can work either as EP or RC. In order to enable host-specific features PCIE_TEGRA194_HOST must be selected and in order to enable device-specific features PCIE_TEGRA194_EP must be -- 2.16.2
[PATCH V3] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V3:
* Rebased on top of linux-next
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 48 +++-
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index 645fa1892375..5b72a5448d2e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
+{
+ /*
+* DesignWare core version 4.90A has this strange design issue
+* where the 'TD' bit in the Control register-1 of the ATU outbound
+* region acts like an override for the ECRC setting i.e. the presence
+* of TLP Digest(ECRC) in the outgoing TLPs is solely determined by
+* this bit. This is contrary to the PCIe spec which says that the
+* enablement of the ECRC is solely determined by the AER registers.
+*
+* Because of this, even when the ECRC is enabled through AER
+* registers, the transactions going through ATU won't have TLP Digest
+* as there is no way the AER sub-system could program the TD bit which
+* is specific to DesignWare core.
+*
+* The best way to handle this scenario is to program the TD bit
+* always. It affects only the traffic from root port to downstream
+* devices.
+*
+* At this point,
+* When ECRC is enabled in AER registers, everything works normally
+* When ECRC is NOT enabled in AER registers, then,
+* on Root Port:- TLP Digest (DWord size) gets appended to each packet
+*even through it is not required. Since downstream
+*TLPs are mostly for configuration accesses and BAR
+*accesses, they are not in critical path and won't
+*have much negative effect on the performance.
+* on End Point:- TLP Digest is received for some/all the packets coming
+*from the root port. TLP Digest is ignored because,
+*as per the PCIe Spec r5.0 v1.0 section 2.2.3
+*"TLP Digest Rules", when an endpoint receives TLP
+*Digest when its ECRC check functionality is disabled
+*in AER registers, received TLP Digest is just ignored.
+* Since there is no issue or error reported either side, best way to
+* handle the scenario is to program TD bit by default.
+*/
+
+ return val | PCIE_ATU_TD;
+}
+
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
@@ -248,6 +288,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie
*pci, u8 func_no,
val = type | PCIE_ATU_FUNC_NUM(func_no);
val = upper_32_bits(size - 1) ?
val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -294,8 +336,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
- PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
/*
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 0207840756c4..5d979953800d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -86,6 +86,7 @@
#define PCIE_ATU_TYPE_IO 0x2
#define PCIE_ATU_TYPE_CFG0 0x4
#define PCIE_ATU_TYPE_CFG1 0x5
+#define PCIE_ATU_TDBIT(8)
#define PCIE
Re: Commit 4257f7e0 ("PCI/ASPM: Save/restore L1SS Capability for suspend/resume") causing hibernate resume failures
Ideally Bjorn's patch should have worked. Could you please collect 'sudo lspci -vv' (please don't forget to give sudo) with Bjorn's patch before and after hibernate? Also, is it right to say that with policy set to "performance" there is no issue during hibernate/resume? - Vidya Sagar On 12/28/2020 12:00 PM, Kenneth R. Crudup wrote: External email: Use caution opening links or attachments On Sun, 27 Dec 2020, Kenneth R. Crudup wrote: I'll try your patch after the revert and see if anything changes. I just realized today's patch makes no sense if it's reverted, so nevermind. -Kenny -- Kenneth R. Crudup Sr. SW Engineer, Scott County Consulting, Orange County CA
Re: dwc: tegra194: issue with card containing a bridge
Thanks Mian for bringing it to our notice.
Have you tried removing the dw_pcie_setup_rc(pp); call from
pcie-tegra194.c file on top of linux-next? and does that solve the issue?
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 5597b2a49598..1c9e9c054592 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -907,7 +907,7 @@ static void tegra_pcie_prepare_host(struct pcie_port
*pp)
dw_pcie_writel_dbi(pci,
CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF, val);
}
- dw_pcie_setup_rc(pp);
+ //dw_pcie_setup_rc(pp);
clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
I took a quick look at the dw_pcie_setup_rc() implementation and I'm not
sure why calling it second time should create any issue for the
enumeration of devices behind a switch. Perhaps I need to spend more
time to debug that part.
In any case, since dw_pcie_setup_rc() is already part of
dw_pcie_host_init(), I think it can be removed from
tegra_pcie_prepare_host() implemention.
Thanks,
Vidya Sagar
On 12/15/2020 3:54 PM, Mian Yousaf Kaukab wrote:
External email: Use caution opening links or attachments
Hi,
I am seeing an issue with next-20201211 with USB3380[1] based PCIe card
(vid:pid 10b5:3380) on Jetson AGX Xavier. Card doesn't show up in the
lspci output.
In non working case (next-20201211):
# lspci
0001:00:00.0 PCI bridge: NVIDIA Corporation Device 1ad2 (rev a1)
0001:01:00.0 SATA controller: Marvell Technology Group Ltd. Device 9171 (rev 13)
0005:00:00.0 PCI bridge: NVIDIA Corporation Device 1ad0 (rev a1)
In working case (v5.10-rc7):
# lspci
0001:00:00.0 PCI bridge: Molex Incorporated Device 1ad2 (rev a1)
0001:01:00.0 SATA controller: Marvell Technology Group Ltd. Device 9171 (rev 13)
0005:00:00.0 PCI bridge: Molex Incorporated Device 1ad0 (rev a1)
0005:01:00.0 PCI bridge: PLX Technology, Inc. Device 3380 (rev ab)
0005:02:02.0 PCI bridge: PLX Technology, Inc. Device 3380 (rev ab)
0005:03:00.0 USB controller: PLX Technology, Inc. Device 3380 (rev ab)
# lspci -t
-+-[0005:00]---00.0-[01-ff]00.0-[02-03]02.0-[03]00.0
+-[0001:00]---00.0-[01-ff]00.0
\-[:00]-
#lspci -v
https://paste.opensuse.org/87573209
git-bisect points to commit b9ac0f9dc8ea ("PCI: dwc: Move dw_pcie_setup_rc() to DWC
common code").
dw_pcie_setup_rc() is not removed from pcie-tegra194.c in this commit.
Could the failure be caused because dw_pcie_setup_rc() is called twice now in
case of tegra194?
BR,
Yousaf
[1]:
https://www.broadcom.com/products/pcie-switches-bridges/usb-pci/usb-controllers/usb3380
Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
Hi Lorenzo,
Apologies to bug you, but wondering if you have any further comments on
this patch that I need to take care of?
Thanks,
Vidya Sagar
On 12/3/2020 5:40 PM, Vidya Sagar wrote:
On 11/25/2020 2:32 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Tue, Nov 24, 2020 at 03:50:01PM +0530, Vidya Sagar wrote:
Hi Bjorn,
Please let me know if this patch needs any further modifications
I'm fine with it, but of course Lorenzo will take care of it.
Thanks Bjorn.
Hi Lorenzo,
Please let me know if you have any comments for this patch.
Thanks,
Vidya Sagar
On 11/12/2020 10:32 PM, Vidya Sagar wrote:
External email: Use caution opening links or attachments
On 11/12/2020 3:59 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Wed, Nov 11, 2020 at 10:21:46PM +0530, Vidya Sagar wrote:
On 11/11/2020 9:57 PM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 11/11/20, 7:12 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in
one of the control
registers of ATU. This bit also needs to be programmed for
proper ECRC
functionality. This is currently identified as an issue
with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52
++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git
a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void
dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index,
u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
What is the reason to use inline here?
Actually, I wanted to move the programming part inside the
respective APIs
but then I wanted to give some details as well in comments so to
avoid
duplication, I came up with this function. But, I'm making it
inline for
better code optimization by compiler.
I don't really care either way, but I'd be surprised if the compiler
didn't inline this all by itself even without the explicit "inline".
I just checked it and you are right that compiler is indeed inlining it
without explicitly mentioning 'inline'.
I hope it is ok to leave it that way.
+{
+ /*
+ * DesignWare core version 4.90A has this strange design
issue
+ * where the 'TD' bit in the Control register-1 of
the ATU outbound
+ * region acts like an override for the ECRC
setting i.e. the presence
+ * of TLP Digest(ECRC) in the outgoing TLPs is
solely determined by
+ * this bit. This is contrary to the PCIe spec
which says that the
+ * enablement of the ECRC is solely determined by
the AER registers.
+ *
+ * Because of this, even when the ECRC is enabled through AER
+ * registers, the transactions going through ATU
won't have TLP Digest
+ * as there is no way the AER sub-system could
program the TD bit which
+ * is specific to DesignWare core.
+ *
+ * The best way to handle this scenario is to program the
TD bit
+ * always. It affects only the traffic from root
port to downstream
+ * devices.
+ *
+ * At this point,
+ * When ECRC is enabled in AER registers,
everything works normally
+ * When ECRC is NOT enabled in AER registers, then,
+ * on Root Port:- TLP Digest (DWord size) gets
appended to each packet
+ * even through it is not required.
Since downstream
+ * TLPs are mostly for
configuration accesses and BAR
+ * accesses, they are not in
critical path and won't
+ * have much negative effect on the
performance.
+ * on End Point:- TLP Digest is received for
some/all the packets coming
+ * from the root port. TLP Digest
is ignored because,
+ * as per the PCIe Spec r5.0 v1.0 section
2.2.3
+ * "TLP Digest Rules", when an
endpoint receives TLP
+ * Digest when its ECRC check
functionality is disabled
+ * in AER registers, received TLP
Digest is just ignored.
+ * Since there is no issue or error reported
either side, best way to
+ * handle the scenario is to program TD bit by default.
+ */
+
+ return val | PCIE_ATU_TD;
+}
Re: [PATCH] PCI: Save/restore L1 PM Substate extended capability registers
There is a change already available for it in linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=4257f7e008ea394fcecc050f1569c3503b8bcc15
Thanks,
Vidya Sagar
On 12/9/2020 3:36 AM, David E. Box wrote:
External email: Use caution opening links or attachments
On Intel systems that support ACPI Low Power Idle it has been observed
that the L1 Substate capability can return disabled after a s2idle
cycle. This causes the loss of L1 Substate support during runtime
leading to higher power consumption. Add save/restore of the L1SS
control registers.
Signed-off-by: David E. Box
---
drivers/pci/pci.c | 49 +++
1 file changed, 49 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e578d34095e9..beee3d9952a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1539,6 +1539,48 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
}
+static void pci_save_l1ss_state(struct pci_dev *dev)
+{
+ int l1ss;
+ struct pci_cap_saved_state *save_state;
+ u16 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!l1ss)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state) {
+ pci_err(dev, "no suspend buffer for L1 Substates\n");
+ return;
+ }
+
+ cap = (u16 *)_state->cap.data[0];
+ pci_read_config_word(dev, l1ss + PCI_L1SS_CTL1, cap++);
+ pci_read_config_word(dev, l1ss + PCI_L1SS_CTL1 + 2, cap++);
+ pci_read_config_word(dev, l1ss + PCI_L1SS_CTL2, cap++);
+}
+
+static void pci_restore_l1ss_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int l1ss;
+ u16 *cap;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state || !l1ss)
+ return;
+
+ cap = (u16 *)_state->cap.data[0];
+ pci_write_config_word(dev, l1ss + PCI_L1SS_CTL1, *cap++);
+ pci_write_config_word(dev, l1ss + PCI_L1SS_CTL1 + 2, *cap++);
+ pci_write_config_word(dev, l1ss + PCI_L1SS_CTL2, *cap++);
+}
+
/**
* pci_save_state - save the PCI configuration space of a device before
* suspending
@@ -1563,6 +1605,7 @@ int pci_save_state(struct pci_dev *dev)
if (i != 0)
return i;
+ pci_save_l1ss_state(dev);
pci_save_ltr_state(dev);
pci_save_dpc_state(dev);
pci_save_aer_state(dev);
@@ -1670,6 +1713,7 @@ void pci_restore_state(struct pci_dev *dev)
*/
pci_restore_ltr_state(dev);
+ pci_restore_l1ss_state(dev);
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
pci_restore_pri_state(dev);
@@ -3332,6 +3376,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
+ 3 * sizeof(u16));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for L1
Substates\n");
+
pci_allocate_vc_save_buffers(dev);
}
--
2.20.1
Re: [PATCH V5 5/5] PCI: tegra: Disable LTSSM during L2 entry
On 12/8/2020 2:07 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
[+cc Jingoo, Gustavo]
On Thu, Dec 03, 2020 at 07:04:51PM +0530, Vidya Sagar wrote:
PCIe cards like Marvell SATA controller and some of the Samsung NVMe
drives don't support taking the link to L2 state. When the link doesn't
go to L2 state, Tegra194 requires the LTSSM to be disabled to allow PHY
to start the next link up process cleanly during suspend/resume sequence.
Failing to disable LTSSM results in the PCIe link not coming up in the
next resume cycle.
Is this a Tegra194-specific issue, or will other DWC-based controllers
need a similar change?
This is a Tegra194 specific issue.
Thanks,
Vidya Sagar
Tested-by: Thierry Reding
Signed-off-by: Vidya Sagar
Acked-by: Thierry Reding
---
V5:
* Added Tested-by and Acked-by from Thierry Reding
V4:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index f4109d71f20b..5597b2a49598 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1506,6 +1506,14 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
data &= ~APPL_PINMUX_PEX_RST;
appl_writel(pcie, data, APPL_PINMUX);
+ /*
+ * Some cards do not go to detect state even after de-asserting
+ * PERST#. So, de-assert LTSSM to bring link to detect state.
+ */
+ data = readl(pcie->appl_base + APPL_CTRL);
+ data &= ~APPL_CTRL_LTSSM_EN;
+ writel(data, pcie->appl_base + APPL_CTRL);
+
err = readl_poll_timeout_atomic(pcie->appl_base + APPL_DEBUG,
data,
((data &
@@ -1513,14 +1521,8 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
APPL_DEBUG_LTSSM_STATE_SHIFT) ==
LTSSM_STATE_PRE_DETECT,
1, LTSSM_TIMEOUT);
- if (err) {
+ if (err)
dev_info(pcie->dev, "Link didn't go to detect state\n");
- } else {
- /* Disable LTSSM after link is in detect state */
- data = appl_readl(pcie, APPL_CTRL);
- data &= ~APPL_CTRL_LTSSM_EN;
- appl_writel(pcie, data, APPL_CTRL);
- }
}
/*
* DBI registers may not be accessible after this as PLL-E would be
--
2.17.1
Re: [PATCH V2] PCI/MSI: Set device flag indicating only 32-bit MSI support
On 12/4/2020 1:24 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Fri, Dec 04, 2020 at 12:33:45AM +0530, Vidya Sagar wrote:
On 12/3/2020 11:54 PM, Bjorn Helgaas wrote:
On Tue, Nov 24, 2020 at 04:20:35PM +0530, Vidya Sagar wrote:
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register. Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
This seems good to me. I'll post a possible revision to set
dev->no_64bit_msi in the device enumeration path instead of in the IRQ
allocation path, since it's really a property of the device, not of
the msi_desc.
I like the extra checking this gives us. Was this prompted by
tripping over something, or is it something you noticed by code
reading? If the former, a hint about what was wrong and how it's
being fixed would be useful.
I observed functionality issue with Marvell SATA controller (1b4b:9171) when
the allocated MSI target address was a 64-bit address. I mentioned the
Marvell SATA controller as an example in the commit message.
I know you mentioned the Marvell controller, but apparently that
device is working perfectly correctly: it does not support 64-bit MSI,
and it does not advertise support for 64-bit MSI.
So if there's a functionality issue, that means something is wrong in
Linux that caused us to assign a 64-bit MSI address to it. *That*
issue is what I want to know about. Your patch only warns about the
issue; it doesn't fix it.
The issue is in the DWC code. I pushed a generic patch to fix that issue
by specifying the limit of MSI target address to 32-bit region. Patch is
up for review at
http://patchwork.ozlabs.org/project/linux-pci/patch/20201117165312.25847-1-vid...@nvidia.com/
I don't think there's any point in specifically mentioning the Marvell
device if it is working correctly, because the same issue should
affect *any* device that doesn't support 64-bit MSI. But if there's
some arch code that incorrectly assigns a 64-bit address, it would
definitely be useful to specify the arch. And hopefully there's a fix
for that arch code, too.
Signed-off-by: Vidya Sagar
---
V2:
* Addressed Bjorn's comment and changed the error message
drivers/pci/msi.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..8de5ba6b4a59 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
@@ -602,8 +604,9 @@ static int msi_verify_entries(struct pci_dev *dev)
for_each_pci_msi_entry(entry, dev) {
if (!dev->no_64bit_msi || !entry->msg.address_hi)
continue;
- pci_err(dev, "Device has broken 64-bit MSI but arch"
- " tried to assign one above 4G\n");
+ pci_err(dev, "Device has either broken 64-bit MSI or "
+ "only 32-bit MSI support but "
+ "arch tried to assign one above 4G\n");
return -EIO;
}
return 0;
--
2.17.1
Re: [PATCH V2] PCI/MSI: Set device flag indicating only 32-bit MSI support
On 12/3/2020 11:54 PM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Tue, Nov 24, 2020 at 04:20:35PM +0530, Vidya Sagar wrote:
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register. Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
This seems good to me. I'll post a possible revision to set
dev->no_64bit_msi in the device enumeration path instead of in the IRQ
allocation path, since it's really a property of the device, not of
the msi_desc.
I like the extra checking this gives us. Was this prompted by
tripping over something, or is it something you noticed by code
reading? If the former, a hint about what was wrong and how it's
being fixed would be useful.
I observed functionality issue with Marvell SATA controller (1b4b:9171)
when the allocated MSI target address was a 64-bit address. I mentioned
the Marvell SATA controller as an example in the commit message.
Thanks,
Vidya Sagar
Signed-off-by: Vidya Sagar
---
V2:
* Addressed Bjorn's comment and changed the error message
drivers/pci/msi.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..8de5ba6b4a59 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
@@ -602,8 +604,9 @@ static int msi_verify_entries(struct pci_dev *dev)
for_each_pci_msi_entry(entry, dev) {
if (!dev->no_64bit_msi || !entry->msg.address_hi)
continue;
- pci_err(dev, "Device has broken 64-bit MSI but arch"
- " tried to assign one above 4G\n");
+ pci_err(dev, "Device has either broken 64-bit MSI or "
+ "only 32-bit MSI support but "
+ "arch tried to assign one above 4G\n");
return -EIO;
}
return 0;
--
2.17.1
[PATCH V5 3/5] PCI: tegra: Continue unconfig sequence even if parts fail
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
Tested-by: Thierry Reding
Signed-off-by: Vidya Sagar
Acked-by: Thierry Reding
---
V5:
* Added Tested-by and Acked-by from Thierry Reding
V4:
* None
V3:
* Modified subject as per Bjorn's suggestion
* Removed tegra_pcie_init_controller()'s error checking part and pushed
a separate patch for it
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 39 +-
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 59163b735c96..471c6d725c70 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1415,43 +1415,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1475,7 +1464,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1544,13 +1534,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -2197,8 +2186,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
PORT_LOGIC_MSI_CTRL_INT_0_EN);
tegra_pcie_downstream_dev_to_D0(pcie);
tegra_pcie_dw_pme_turnoff(pcie);
+ tegra_pcie_unconfig_controller(pcie);
- return __deinit_controller(pcie);
+ return 0;
}
static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2226,7 +2216,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2264,7 +2255,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device
*pdev)
disable_irq(pcie->pci.pp.msi_irq);
tegra_pcie_dw_pme_turnoff(pcie);
- __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
--
2.17.1
[PATCH V5 4/5] PCI: tegra: Check return value of tegra_pcie_init_controller()
The return value of tegra_pcie_init_controller() must be checked before
PCIe link up check and registering debugfs entries subsequently as it
doesn't make sense to do these when the controller initialization itself
has failed.
Tested-by: Thierry Reding
Signed-off-by: Vidya Sagar
Acked-by: Thierry Reding
---
V5:
* Added Tested-by and Acked-by from Thierry Reding
V4:
* None
V3:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 471c6d725c70..f4109d71f20b 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1563,7 +1563,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize controller: %d\n", ret);
+ goto fail_pm_get_sync;
+ }
pcie->link_state = tegra_pcie_dw_link_up(>pci);
if (!pcie->link_state) {
--
2.17.1
[PATCH V5 2/5] PCI: tegra: Set DesignWare IP version
Set the DesignWare IP version for Tegra194 to 0x490A. This would be used by the DesigWare sub-system to do any version specific configuration (Ex:- TD bit programming for ECRC). Tested-by: Thierry Reding Signed-off-by: Vidya Sagar Acked-by: Thierry Reding --- V5: * Added Tested-by and Acked-by from Thierry Reding V4: * None V3: * None V2: * None drivers/pci/controller/dwc/pcie-tegra194.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 4c966e9adb2b..59163b735c96 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -1984,6 +1984,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) pci->ops = _dw_pcie_ops; pci->n_fts[0] = N_FTS_VAL; pci->n_fts[1] = FTS_VAL; + pci->version = 0x490A; pp = >pp; pp->num_vectors = MAX_MSI_IRQS; -- 2.17.1
[PATCH V5 5/5] PCI: tegra: Disable LTSSM during L2 entry
PCIe cards like Marvell SATA controller and some of the Samsung NVMe
drives don't support taking the link to L2 state. When the link doesn't
go to L2 state, Tegra194 requires the LTSSM to be disabled to allow PHY
to start the next link up process cleanly during suspend/resume sequence.
Failing to disable LTSSM results in the PCIe link not coming up in the
next resume cycle.
Tested-by: Thierry Reding
Signed-off-by: Vidya Sagar
Acked-by: Thierry Reding
---
V5:
* Added Tested-by and Acked-by from Thierry Reding
V4:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index f4109d71f20b..5597b2a49598 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1506,6 +1506,14 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
data &= ~APPL_PINMUX_PEX_RST;
appl_writel(pcie, data, APPL_PINMUX);
+ /*
+* Some cards do not go to detect state even after de-asserting
+* PERST#. So, de-assert LTSSM to bring link to detect state.
+*/
+ data = readl(pcie->appl_base + APPL_CTRL);
+ data &= ~APPL_CTRL_LTSSM_EN;
+ writel(data, pcie->appl_base + APPL_CTRL);
+
err = readl_poll_timeout_atomic(pcie->appl_base + APPL_DEBUG,
data,
((data &
@@ -1513,14 +1521,8 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
APPL_DEBUG_LTSSM_STATE_SHIFT) ==
LTSSM_STATE_PRE_DETECT,
1, LTSSM_TIMEOUT);
- if (err) {
+ if (err)
dev_info(pcie->dev, "Link didn't go to detect state\n");
- } else {
- /* Disable LTSSM after link is in detect state */
- data = appl_readl(pcie, APPL_CTRL);
- data &= ~APPL_CTRL_LTSSM_EN;
- appl_writel(pcie, data, APPL_CTRL);
- }
}
/*
* DBI registers may not be accessible after this as PLL-E would be
--
2.17.1
[PATCH V5 0/5] Enhancements to Tegra194 PCIe driver
This series of patches do some enhancements and some bug fixes to the Tegra194 PCIe platform driver like - Fix Vendor-ID corruption - Update DWC IP version - Continue with uninitialization sequence even if parts fail - Check return value of tegra_pcie_init_controller() - Disable LTSSM during link's L2 entry V5: * Rebased the first patch in the series * Dropped the second patch * Added Tested-by and Acked-by for rest of the patches V4: * Added a new patch to address link-up issues with some of the cards V3: * Addressed Bjorn's review comments * Split earlier patch-4 into two - Continue with the uninitialization sequence even if some parts fail - Check return value of tegra_pcie_init_controller() and exit accordingly V2: * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE' Vidya Sagar (5): PCI: tegra: Fix ASPM-L1SS advertisement disable code PCI: tegra: Set DesignWare IP version PCI: tegra: Continue unconfig sequence even if parts fail PCI: tegra: Check return value of tegra_pcie_init_controller() PCI: tegra: Disable LTSSM during L2 entry drivers/pci/controller/dwc/pcie-tegra194.c | 74 +++--- 1 file changed, 36 insertions(+), 38 deletions(-) -- 2.17.1
[PATCH V5 1/5] PCI: tegra: Fix ASPM-L1SS advertisement disable code
If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.
Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar
---
V5:
* Rebased on top of the tree code
V4:
* None
V3:
* None
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 648e731bccfa..4c966e9adb2b 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -863,12 +863,6 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
PCI_CAP_ID_EXP);
- /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
dw_pcie_writel_dbi(pci, PCI_IO_BASE, val);
@@ -897,6 +891,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
init_host_aspm(pcie);
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
--
2.17.1
Re: [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
-Original Message- From: Thierry Reding Sent: Thursday, November 26, 2020 5:04 PM To: Vidya Sagar Cc: lorenzo.pieral...@arm.com; robh...@kernel.org; bhelg...@google.com; Jonathan Hunter ; amanharitsh...@gmail.com; dinghao@zju.edu.cn; k...@linux.com; linux-...@vger.kernel.org; linux- te...@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota ; Manikanta Maddireddy ; sagar...@gmail.com Subject: Re: [PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE On Mon, Nov 09, 2020 at 10:49:33PM +0530, Vidya Sagar wrote: > As specified in the comment for pci_remap_cfgspace() define in > arch/arm64/include/asm/io.h file, PCIe configuration space should be > mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from > devm_ioremap_resource() for mapping DBI space as that is nothing but > the root port's own configuration space. > > Signed-off-by: Vidya Sagar > --- > V4: > * None > > V3: > * None > > V2: > * Changed 'Strongly Ordered' to 'nGnRnE' > > drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c > b/drivers/pci/controller/dwc/pcie-tegra194.c > index b172b1d49713..7a0c64436861 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >} >pcie->dbi_res = dbi_res; > > - pci->dbi_base = devm_ioremap_resource(dev, dbi_res); > + pci->dbi_base = devm_pci_remap_cfgspace(dev, > + dbi_res->start, > + resource_size(dbi_res)); >if (IS_ERR(pci->dbi_base)) >return PTR_ERR(pci->dbi_base); > Similarly to patch 1/6, this is no longer required because it's already part of one of Rob's earlier patches, so this, too, can be dropped. Yes. This patch is not required now. I'll drop it from the next patch series. Thanks, Vidya Sagar Thierry
Re: [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code
-Original Message-
From: Thierry Reding
Sent: Thursday, November 26, 2020 5:03 PM
To: Vidya Sagar
Cc: lorenzo.pieral...@arm.com; robh...@kernel.org; bhelg...@google.com;
Jonathan Hunter ; amanharitsh...@gmail.com;
dinghao@zju.edu.cn; k...@linux.com; linux-...@vger.kernel.org; linux-
te...@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
; Manikanta Maddireddy ;
sagar...@gmail.com
Subject: Re: [PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable
code
On Mon, Nov 09, 2020 at 10:49:32PM +0530, Vidya Sagar wrote:
> If the absence of CLKREQ# signal is indicated by the absence of
> "supports-clkreq" in the device-tree node, current driver is disabling
> the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1
> Sub-States offset is correctly initialized. Since default value of the
> ASPM-L1SS offset is zero, this is causing the Vendor-ID wrongly
> programmed to 0x10d2 instead of Nvidia's 0x10de thereby the quirks
> applicable for Tegra194 are not being applied. This patch fixes this
> issue by refactoring the code that disables the ASPM-L1SS advertisement.
>
> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> Signed-off-by: Vidya Sagar
> ---
> V4:
> * None
>
> V3:
> * None
>
> V2:
> * None
>
> drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
Looks like this no longer applies cleanly after that other fix that you sent
earlier.
But looking more closely, that's because that other fix already incorporates an
equivalent change, so I think this can be dropped from this series.
Yes. This is no longer applies cleanly and I'll fix it in the next
series, but, the current patch is still required.
The other change I pushed is taking care of getting a valid 'dbi'
address before accessing the dbi region, but, this current change would
make sure that 'pcie->cfg_link_cap_l1sub' would have a valid value
before calling disable_aspm_l1/2() APIs.
Thanks,
Vidya Sagar
Thierry
Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
On 11/25/2020 2:32 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Tue, Nov 24, 2020 at 03:50:01PM +0530, Vidya Sagar wrote:
Hi Bjorn,
Please let me know if this patch needs any further modifications
I'm fine with it, but of course Lorenzo will take care of it.
Thanks Bjorn.
Hi Lorenzo,
Please let me know if you have any comments for this patch.
Thanks,
Vidya Sagar
On 11/12/2020 10:32 PM, Vidya Sagar wrote:
External email: Use caution opening links or attachments
On 11/12/2020 3:59 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Wed, Nov 11, 2020 at 10:21:46PM +0530, Vidya Sagar wrote:
On 11/11/2020 9:57 PM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 11/11/20, 7:12 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in
one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue
with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52
++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git
a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void
dw_pcie_writel_ob_unroll(struct dw_pcie *pci, u32 index,
u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
What is the reason to use inline here?
Actually, I wanted to move the programming part inside the
respective APIs
but then I wanted to give some details as well in comments so to avoid
duplication, I came up with this function. But, I'm making it inline for
better code optimization by compiler.
I don't really care either way, but I'd be surprised if the compiler
didn't inline this all by itself even without the explicit "inline".
I just checked it and you are right that compiler is indeed inlining it
without explicitly mentioning 'inline'.
I hope it is ok to leave it that way.
+{
+ /*
+ * DesignWare core version 4.90A has this strange design issue
+ * where the 'TD' bit in the Control register-1 of
the ATU outbound
+ * region acts like an override for the ECRC
setting i.e. the presence
+ * of TLP Digest(ECRC) in the outgoing TLPs is
solely determined by
+ * this bit. This is contrary to the PCIe spec
which says that the
+ * enablement of the ECRC is solely determined by
the AER registers.
+ *
+ * Because of this, even when the ECRC is enabled through AER
+ * registers, the transactions going through ATU
won't have TLP Digest
+ * as there is no way the AER sub-system could
program the TD bit which
+ * is specific to DesignWare core.
+ *
+ * The best way to handle this scenario is to program the TD bit
+ * always. It affects only the traffic from root
port to downstream
+ * devices.
+ *
+ * At this point,
+ * When ECRC is enabled in AER registers,
everything works normally
+ * When ECRC is NOT enabled in AER registers, then,
+ * on Root Port:- TLP Digest (DWord size) gets
appended to each packet
+ *even through it is not required.
Since downstream
+ *TLPs are mostly for
configuration accesses and BAR
+ *accesses, they are not in
critical path and won't
+ *have much negative effect on the performance.
+ * on End Point:- TLP Digest is received for
some/all the packets coming
+ *from the root port. TLP Digest
is ignored because,
+ *as per the PCIe Spec r5.0 v1.0 section 2.2.3
+ *"TLP Digest Rules", when an
endpoint receives TLP
+ *Digest when its ECRC check
functionality is disabled
+ *in AER registers, received TLP
Digest is just ignored.
+ * Since there is no issue or error reported
either side, best way to
+ * handle the scenario is to program TD bit by default.
+ */
+
+ return val | PCIE_ATU_TD;
+}
Re: [PATCH] PCI: dwc: Set 32-bit DMA mask for MSI target address allocation
Hi,
Could you please review this patch in the context of the following patch?
http://patchwork.ozlabs.org/project/linux-pci/patch/20201124105035.24573-1-vid...@nvidia.com/
Thanks,
Vidya Sagar
On 11/17/2020 10:23 PM, Vidya Sagar wrote:
Set DMA mask to 32-bit while allocating the MSI target address so that
the address is usable for both 32-bit and 64-bit MSI capable devices.
Throw a warning if it fails to set the mask to 32-bit to alert that
devices that are only 32-bit MSI capable may not work properly.
Signed-off-by: Vidya Sagar
---
Given the other patch that I've pushed to the MSI sub-system
http://patchwork.ozlabs.org/project/linux-pci/patch/20201117145728.4516-1-vid...@nvidia.com/
which is going to catch any mismatch between MSI capability (32-bit) of the
device and system's inability to allocate the required MSI target address,
I'm not sure how much sense is this patch going to be make. But, I can
certainly say that if the memory allocation mechanism gives the addresses
from 64-bit pool by default, this patch at least makes sure that MSI target
address is allocated from 32-bit pool.
drivers/pci/controller/dwc/pcie-designware-host.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 44c2a6572199..e6a230eddf66 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -388,6 +388,14 @@ int dw_pcie_host_init(struct pcie_port *pp)
dw_chained_msi_isr,
pp);
+ ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
+ if (!ret) {
+ dev_warn(pci->dev,
+"Failed to set DMA mask to 32-bit. "
+"Devices with only 32-bit MSI support"
+" may not work properly\n");
+ }
+
pp->msi_data = dma_map_single_attrs(pci->dev,
>msi_msg,
sizeof(pp->msi_msg),
DMA_FROM_DEVICE,
Re: [PATCH V2] PCI/MSI: Set device flag indicating only 32-bit MSI support
Hi Bjorn,
Do you have any further comments for this patch?
Thanks,
Vidya Sagar
On 11/24/2020 4:20 PM, Vidya Sagar wrote:
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register. Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
Signed-off-by: Vidya Sagar
---
V2:
* Addressed Bjorn's comment and changed the error message
drivers/pci/msi.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..8de5ba6b4a59 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
@@ -602,8 +604,9 @@ static int msi_verify_entries(struct pci_dev *dev)
for_each_pci_msi_entry(entry, dev) {
if (!dev->no_64bit_msi || !entry->msg.address_hi)
continue;
- pci_err(dev, "Device has broken 64-bit MSI but arch"
- " tried to assign one above 4G\n");
+ pci_err(dev, "Device has either broken 64-bit MSI or "
+ "only 32-bit MSI support but "
+ "arch tried to assign one above 4G\n");
return -EIO;
}
return 0;
Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
-Original Message-
From: Thierry Reding
Sent: Wednesday, November 25, 2020 11:27 PM
To: Vidya Sagar
Cc: lorenzo.pieral...@arm.com; robh...@kernel.org; bhelg...@google.com;
Jonathan Hunter ; amanharitsh...@gmail.com;
dinghao@zju.edu.cn; k...@linux.com; linux-...@vger.kernel.org; linux-
te...@vger.kernel.org; linux-kernel@vger.kernel.org; Krishna Thota
; Manikanta Maddireddy ;
sagar...@gmail.com
Subject: Re: [PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
On Mon, Nov 09, 2020 at 10:49:31PM +0530, Vidya Sagar wrote:
> This series of patches do some enhancements and some bug fixes to the
> Tegra194 PCIe platform driver like
> - Fix Vendor-ID corruption
> - Map DBI space correctly
> - Update DWC IP version
> - Continue with uninitialization sequence even if parts fail
> - Check return value of tegra_pcie_init_controller()
>
> V4:
> * Added a new patch to address link-up issues with some of the cards
>
> V3:
> * Addressed Bjorn's review comments
> * Split earlier patch-4 into two
> - Continue with the uninitialization sequence even if some parts fail
> - Check return value of tegra_pcie_init_controller() and exit
> accordingly
>
> V2:
> * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE'
>
> Vidya Sagar (6):
> PCI: tegra: Fix ASPM-L1SS advertisement disable code
> PCI: tegra: Map configuration space as nGnRnE
> PCI: tegra: Set DesignWare IP version
> PCI: tegra: Continue unconfig sequence even if parts fail
> PCI: tegra: Check return value of tegra_pcie_init_controller()
> PCI: tegra: Disable LTSSM during L2 entry
>
> drivers/pci/controller/dwc/pcie-tegra194.c | 78
> +++---
> 1 file changed, 39 insertions(+), 39 deletions(-)
I was going to test this series, but then I noticed that PCI is causing a crash
on
linux-next (as of fairly recently).
I root caused the crash issue to the following commit
a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource
setup into common code")
I also pushed the following two patches to fix this issue for review
http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192234.2270-1-vid...@nvidia.com/
http://patchwork.ozlabs.org/project/linux-pci/patch/20201125192554.5401-1-vid...@nvidia.com/
So I tried applying this on top of v5.10-rc1, but
that gives me the following:
[3.595161] ahci 0001:01:00.0: version 3.0
[3.595726] ahci 0001:01:00.0: SSS flag set, parallel bus scan
disabled
[4.609923] ahci 0001:01:00.0: controller reset failed (0x)
[4.610343] ahci: probe of 0001:01:00.0 failed with error -5
So the device enumerates fine, but it's not able to reset the SATA controller.
That said, this seems to happen regardless of this patch series, so plain
v5.10-rc1
also shows the above.
This was also a known issue and we need the following commit to make
things work (FWIW, it is already accepted)
9fff3256f93d PCI: dwc: Restore ATU memory resource setup to use last entry
Otherwise, v5.10-rc3 can be used which has working state of PCIe on
Tegra194.
Given the above, I think we should hold off on applying this series until we've
fixed PCI on linux-next and made sure that SATA also works properly, otherwise
we don't have a known good baseline to test this against.
Thierry
[PATCH] PCI: tegra: Read "dbi" base address to program in application logic
PCIe controller in Tegra194 requires the "dbi" region base address to be
programmed in one of the application logic registers to enable CPU access
to the "dbi" region. But, commit a0fd361db8e5 ("PCI: dwc: Move "dbi",
"dbi2", and "addr_space" resource setup into common code") moved the code
that reads the whereabouts of "dbi" region to the common code causing the
existing code in pcie-tegra194.c file to program NULL in the application
logic registers. This is causing null pointer dereference when the "dbi"
registers are accessed. This issue is fixed by explicitly reading the
"dbi" base address from DT node.
Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource
setup into common code")
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-tegra194.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index fa54d9aaa430..ac2225175087 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1053,9 +1053,16 @@ static int tegra_pcie_enable_phy(struct tegra_pcie_dw
*pcie)
static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie)
{
+ struct platform_device *pdev = to_platform_device(pcie->dev);
struct device_node *np = pcie->dev->of_node;
int ret;
+ pcie->dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"dbi");
+ if (!pcie->dbi_res) {
+ dev_err(pcie->dev, "Failed to find \"dbi\" region\n");
+ return -ENODEV;
+ }
+
ret = of_property_read_u32(np, "nvidia,aspm-cmrt-us", >aspm_cmrt);
if (ret < 0) {
dev_info(pcie->dev, "Failed to read ASPM T_cmrt: %d\n", ret);
--
2.17.1
[PATCH] PCI: tegra: Move "dbi" accesses to post common DWC initialization
commit a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space"
resource setup into common code") moved the code that sets up dbi_base
to DWC common code thereby creating a requirement to not access the "dbi"
region before calling common DWC initialization code. But, Tegra194
already had some code that programs some of the "dbi" registers resulting
in system crash. This patch addresses that issue by refactoring the code
to have accesses to the "dbi" region only after common DWC initialization.
Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource
setup into common code")
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-tegra194.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index ac2225175087..648e731bccfa 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -859,6 +859,16 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
u32 val;
+ if (!pcie->pcie_cap_base)
+ pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
+ PCI_CAP_ID_EXP);
+
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
dw_pcie_writel_dbi(pci, PCI_IO_BASE, val);
@@ -1389,15 +1399,6 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
reset_control_deassert(pcie->core_rst);
- pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
- PCI_CAP_ID_EXP);
-
- /* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
return ret;
fail_phy:
--
2.17.1
[PATCH V2] PCI/MSI: Set device flag indicating only 32-bit MSI support
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register. Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
Signed-off-by: Vidya Sagar
---
V2:
* Addressed Bjorn's comment and changed the error message
drivers/pci/msi.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..8de5ba6b4a59 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
@@ -602,8 +604,9 @@ static int msi_verify_entries(struct pci_dev *dev)
for_each_pci_msi_entry(entry, dev) {
if (!dev->no_64bit_msi || !entry->msg.address_hi)
continue;
- pci_err(dev, "Device has broken 64-bit MSI but arch"
- " tried to assign one above 4G\n");
+ pci_err(dev, "Device has either broken 64-bit MSI or "
+ "only 32-bit MSI support but "
+ "arch tried to assign one above 4G\n");
return -EIO;
}
return 0;
--
2.17.1
Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
Hi Bjorn,
Please let me know if this patch needs any further modifications
Thanks,
Vidya Sagar
On 11/12/2020 10:32 PM, Vidya Sagar wrote:
External email: Use caution opening links or attachments
On 11/12/2020 3:59 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Wed, Nov 11, 2020 at 10:21:46PM +0530, Vidya Sagar wrote:
On 11/11/2020 9:57 PM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 11/11/20, 7:12 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in one of the
control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with
DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52
++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void dw_pcie_writel_ob_unroll(struct
dw_pcie *pci, u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
What is the reason to use inline here?
Actually, I wanted to move the programming part inside the respective
APIs
but then I wanted to give some details as well in comments so to avoid
duplication, I came up with this function. But, I'm making it inline for
better code optimization by compiler.
I don't really care either way, but I'd be surprised if the compiler
didn't inline this all by itself even without the explicit "inline".
I just checked it and you are right that compiler is indeed inlining it
without explicitly mentioning 'inline'.
I hope it is ok to leave it that way.
+{
+ /*
+ * DesignWare core version 4.90A has this strange design issue
+ * where the 'TD' bit in the Control register-1 of the ATU
outbound
+ * region acts like an override for the ECRC setting i.e. the
presence
+ * of TLP Digest(ECRC) in the outgoing TLPs is solely
determined by
+ * this bit. This is contrary to the PCIe spec which says
that the
+ * enablement of the ECRC is solely determined by the AER
registers.
+ *
+ * Because of this, even when the ECRC is enabled through AER
+ * registers, the transactions going through ATU won't have
TLP Digest
+ * as there is no way the AER sub-system could program the TD
bit which
+ * is specific to DesignWare core.
+ *
+ * The best way to handle this scenario is to program the TD bit
+ * always. It affects only the traffic from root port to
downstream
+ * devices.
+ *
+ * At this point,
+ * When ECRC is enabled in AER registers, everything works
normally
+ * When ECRC is NOT enabled in AER registers, then,
+ * on Root Port:- TLP Digest (DWord size) gets appended to
each packet
+ * even through it is not required. Since
downstream
+ * TLPs are mostly for configuration accesses
and BAR
+ * accesses, they are not in critical path and
won't
+ * have much negative effect on the performance.
+ * on End Point:- TLP Digest is received for some/all the
packets coming
+ * from the root port. TLP Digest is ignored
because,
+ * as per the PCIe Spec r5.0 v1.0 section 2.2.3
+ * "TLP Digest Rules", when an endpoint
receives TLP
+ * Digest when its ECRC check functionality is
disabled
+ * in AER registers, received TLP Digest is
just ignored.
+ * Since there is no issue or error reported either side,
best way to
+ * handle the scenario is to program TD bit by default.
+ */
+
+ return val | PCIE_ATU_TD;
+}
Re: [PATCH] PCI/MSI: Set device flag indicating only 32-bit MSI support
On 11/21/2020 3:00 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Tue, Nov 17, 2020 at 08:27:28PM +0530, Vidya Sagar wrote:
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register.
I *think* you're saying these devices behave correctly per spec: they
don't support 64-bit MSI, and they don't advertise support for 64-bit
MSI. Right?
Yes. That is what I intended to say.
Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
And you want msi_verify_entries() to catch attempts by the arch code
to assign a 64-bit MSI address?
Yes.
That sounds OK, but the error message ("Device has broken 64-bit MSI")
is not appropriate if the device is actually *not* broken.
Ok. I didn't change the existing error message. I'll change it to cover
both the scenarios i.e. either the device is broken or the device
doesn't really support 64-bit MSI.
Signed-off-by: Vidya Sagar
---
drivers/pci/msi.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..af49da28854e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
--
2.17.1
[PATCH V2 2/2] PCI: dwc: Add support to program ATU for >4GB memory
Add support to program the ATU to enable translations for >4GB sizes of
the prefetchable memory apertures.
Tested-by: Thierry Reding
Tested-by: Jon Hunter
Signed-off-by: Vidya Sagar
Reviewed-by: Rob Herring
Acked-by: Jingoo
---
V2:
* Added 'Tested-by', 'Reviewed-by' and 'Acked-by'
drivers/pci/controller/dwc/pcie-designware.c | 12 +++-
drivers/pci/controller/dwc/pcie-designware.h | 3 ++-
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..b5e438b70cd5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -228,7 +228,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
-u32 size)
+u64 size)
{
u32 retries, val;
u64 limit_addr = cpu_addr + size - 1;
@@ -245,8 +245,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
-type | PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ val = upper_32_bits(size - 1) ?
+ val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -267,7 +269,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie
*pci, u8 func_no,
static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
int index, int type, u64 cpu_addr,
- u64 pci_addr, u32 size)
+ u64 pci_addr, u64 size)
{
u32 retries, val;
@@ -311,7 +313,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
}
void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
- u64 cpu_addr, u64 pci_addr, u32 size)
+ u64 cpu_addr, u64 pci_addr, u64 size)
{
__dw_pcie_prog_outbound_atu(pci, 0, index, type,
cpu_addr, pci_addr, size);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index ed19c34dd0fe..f9a20ce9ab9a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -81,6 +81,7 @@
#define PCIE_ATU_REGION_INBOUNDBIT(31)
#define PCIE_ATU_REGION_OUTBOUND 0
#define PCIE_ATU_CR1 0x904
+#define PCIE_ATU_INCREASE_REGION_SIZE BIT(13)
#define PCIE_ATU_TYPE_MEM 0x0
#define PCIE_ATU_TYPE_IO 0x2
#define PCIE_ATU_TYPE_CFG0 0x4
@@ -293,7 +294,7 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci);
int dw_pcie_wait_for_link(struct dw_pcie *pci);
void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
int type, u64 cpu_addr, u64 pci_addr,
- u32 size);
+ u64 size);
void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
int type, u64 cpu_addr, u64 pci_addr,
u32 size);
--
2.17.1
[PATCH V2 1/2] PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit
As per PCIe spec r5.0, sec 7.5.1.3.8 only 32-bit BAR registers are defined for non-prefetchable memory and hence a warning should be reported when the size of them go beyond 32-bits. Tested-by: Thierry Reding Tested-by: Jon Hunter Signed-off-by: Vidya Sagar Reviewed-by: Rob Herring --- V2: * Added 'Tested-by' and 'Reviewed-by' drivers/pci/of.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index ac24cd5439a9..5ea472ae22ac 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -556,6 +556,11 @@ static int pci_parse_request_of_pci_ranges(struct device *dev, break; case IORESOURCE_MEM: res_valid |= !(res->flags & IORESOURCE_PREFETCH); + + if (!(res->flags & IORESOURCE_PREFETCH)) + if (upper_32_bits(resource_size(res))) + dev_warn(dev, "Memory resource size exceeds max for 32 bits\n"); + break; } } -- 2.17.1
[PATCH V2 0/2] Add support to handle prefetchable memory
This patch series adds support for configuring the DesignWare IP's ATU region for prefetchable memory translations. It first starts by flagging a warning if the size of non-prefetchable aperture goes beyond 32-bit as PCIe spec doesn't allow it. And then adds required support for programming the ATU to handle higher (i.e. >4GB) sizes. V2: * Dropped third patch from the series as Rob's patch (commit: 9fff3256f93d PCI: dwc: Restore ATU memory resource setup to use last entry) is already accepted * Rebased first two patches on top of Rob's latest patch http://patchwork.ozlabs.org/project/linux-pci/patch/20201026181652.418729-1-r...@kernel.org/ Vidya Sagar (2): PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit PCI: dwc: Add support to program ATU for >4GB memory drivers/pci/controller/dwc/pcie-designware.c | 12 +++- drivers/pci/controller/dwc/pcie-designware.h | 3 ++- drivers/pci/of.c | 5 + 3 files changed, 14 insertions(+), 6 deletions(-) -- 2.17.1
Re: [PATCH 0/3] Add support to handle prefetchable memoryg
On 11/17/2020 5:40 PM, Lorenzo Pieralisi wrote: External email: Use caution opening links or attachments On Tue, Nov 17, 2020 at 10:08:35AM +0530, Vidya Sagar wrote: Hi Lorenzo & Bjorn, Sorry to bother you. Could you please take a look at the patches-1 & 2 from this series? IIUC we should: (1) apply https://patchwork.kernel.org/project/linux-pci/patch/20201026181652.418729-1-r...@kernel.org (2) apply [1,2] from this series For (2), are they rebased against v5.10-rc3 with (1) applied ? I need to check but I will probably have to use v5.10-rc3 as baseline owing to commit: 9fff3256f93d (1) depends on it. Lorenzo My patches [1,2] from this series apply cleanly on v5.10-rc3. But with (1) applied first, there is a trivial rebase required. Let me know if you want me to send the trivial rebased version (of patch-2 particularly). Thanks, Vidya Sagar Thanks, Vidya Sagar On 11/4/2020 1:16 PM, Vidya Sagar wrote: External email: Use caution opening links or attachments Lorenzo / Bjorn, Could you please review patches-1 & 2 in this series? For the third patch, we already went with Rob's patch @ http://patchwork.ozlabs.org/project/linux-pci/patch/20201026154852.221483-1-r...@kernel.org/ Thanks, Vidya Sagar On 10/26/2020 6:02 PM, Thierry Reding wrote: On Sat, Oct 24, 2020 at 04:03:41AM +, Jingoo Han wrote: On 10/23/20, 3:57 PM, Vidya Sagar wrote: This patch series adds support for configuring the DesignWare IP's ATU region for prefetchable memory translations. It first starts by flagging a warning if the size of non-prefetchable aperture goes beyond 32-bit as PCIe spec doesn't allow it. And then adds required support for programming the ATU to handle higher (i.e. >4GB) sizes and then finally adds support for differentiating between prefetchable and non-prefetchable regions and configuring one of the ATU regions for prefetchable memory translations purpose. Vidya Sagar (3): PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit PCI: dwc: Add support to program ATU for >4GB memory aperture sizes PCI: dwc: Add support to handle prefetchable memory mapping For 2nd & 3rd, Acked-by: Jingoo But, I still want someone to ack 1st patch, not me. To Vidya, If possible, can you ask your coworker to give 'Tested-by'? It will be very helpful. Thank you. On next-20201026 (but also going back quite a while) I'm seeing this during boot on Jetson AGX Xavier (Tegra194): [ 3.493382] ahci 0001:01:00.0: version 3.0 [ 3.493889] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled [ 4.497706] ahci 0001:01:00.0: controller reset failed (0x) [ 4.498114] ahci: probe of 0001:01:00.0 failed with error -5 After applying this series, AHCI over PCI is back to normal: [ 3.543230] ahci 0001:01:00.0: AHCI 0001. 32 slots 1 ports 6 Gbps 0x1 impl SATA mode [ 3.550841] ahci 0001:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs [ 3.559747] scsi host0: ahci [ 3.561998] ata1: SATA max UDMA/133 abar m512@0x123001 port 0x1230010100 irq 63 So for the series: Tested-by: Thierry Reding
[PATCH] PCI: dwc: Set 32-bit DMA mask for MSI target address allocation
Set DMA mask to 32-bit while allocating the MSI target address so that
the address is usable for both 32-bit and 64-bit MSI capable devices.
Throw a warning if it fails to set the mask to 32-bit to alert that
devices that are only 32-bit MSI capable may not work properly.
Signed-off-by: Vidya Sagar
---
Given the other patch that I've pushed to the MSI sub-system
http://patchwork.ozlabs.org/project/linux-pci/patch/20201117145728.4516-1-vid...@nvidia.com/
which is going to catch any mismatch between MSI capability (32-bit) of the
device and system's inability to allocate the required MSI target address,
I'm not sure how much sense is this patch going to be make. But, I can
certainly say that if the memory allocation mechanism gives the addresses
from 64-bit pool by default, this patch at least makes sure that MSI target
address is allocated from 32-bit pool.
drivers/pci/controller/dwc/pcie-designware-host.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 44c2a6572199..e6a230eddf66 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -388,6 +388,14 @@ int dw_pcie_host_init(struct pcie_port *pp)
dw_chained_msi_isr,
pp);
+ ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32));
+ if (!ret) {
+ dev_warn(pci->dev,
+"Failed to set DMA mask to 32-bit. "
+"Devices with only 32-bit MSI support"
+" may not work properly\n");
+ }
+
pp->msi_data = dma_map_single_attrs(pci->dev,
>msi_msg,
sizeof(pp->msi_msg),
DMA_FROM_DEVICE,
--
2.17.1
[PATCH] PCI/MSI: Set device flag indicating only 32-bit MSI support
There are devices (Ex:- Marvell SATA controller) that don't support
64-bit MSIs and the same is advertised through their MSI capability
register. Set no_64bit_msi flag explicitly for such devices in the
MSI setup code so that the msi_verify_entries() API would catch
if the MSI arch code tries to use 64-bit MSI.
Signed-off-by: Vidya Sagar
---
drivers/pci/msi.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..af49da28854e 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -581,10 +581,12 @@ msi_setup_entry(struct pci_dev *dev, int nvec, struct
irq_affinity *affd)
entry->msi_attrib.multi_cap = (control & PCI_MSI_FLAGS_QMASK) >> 1;
entry->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
- if (control & PCI_MSI_FLAGS_64BIT)
+ if (control & PCI_MSI_FLAGS_64BIT) {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_64;
- else
+ } else {
entry->mask_pos = dev->msi_cap + PCI_MSI_MASK_32;
+ dev->no_64bit_msi = 1;
+ }
/* Save the initial mask status */
if (entry->msi_attrib.maskbit)
--
2.17.1
Re: [PATCH 0/3] Add support to handle prefetchable memory
Hi Lorenzo & Bjorn, Sorry to bother you. Could you please take a look at the patches-1 & 2 from this series? Thanks, Vidya Sagar On 11/4/2020 1:16 PM, Vidya Sagar wrote: External email: Use caution opening links or attachments Lorenzo / Bjorn, Could you please review patches-1 & 2 in this series? For the third patch, we already went with Rob's patch @ http://patchwork.ozlabs.org/project/linux-pci/patch/20201026154852.221483-1-r...@kernel.org/ Thanks, Vidya Sagar On 10/26/2020 6:02 PM, Thierry Reding wrote: On Sat, Oct 24, 2020 at 04:03:41AM +, Jingoo Han wrote: On 10/23/20, 3:57 PM, Vidya Sagar wrote: This patch series adds support for configuring the DesignWare IP's ATU region for prefetchable memory translations. It first starts by flagging a warning if the size of non-prefetchable aperture goes beyond 32-bit as PCIe spec doesn't allow it. And then adds required support for programming the ATU to handle higher (i.e. >4GB) sizes and then finally adds support for differentiating between prefetchable and non-prefetchable regions and configuring one of the ATU regions for prefetchable memory translations purpose. Vidya Sagar (3): PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit PCI: dwc: Add support to program ATU for >4GB memory aperture sizes PCI: dwc: Add support to handle prefetchable memory mapping For 2nd & 3rd, Acked-by: Jingoo But, I still want someone to ack 1st patch, not me. To Vidya, If possible, can you ask your coworker to give 'Tested-by'? It will be very helpful. Thank you. On next-20201026 (but also going back quite a while) I'm seeing this during boot on Jetson AGX Xavier (Tegra194): [ 3.493382] ahci 0001:01:00.0: version 3.0 [ 3.493889] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled [ 4.497706] ahci 0001:01:00.0: controller reset failed (0x) [ 4.498114] ahci: probe of 0001:01:00.0 failed with error -5 After applying this series, AHCI over PCI is back to normal: [ 3.543230] ahci 0001:01:00.0: AHCI 0001. 32 slots 1 ports 6 Gbps 0x1 impl SATA mode [ 3.550841] ahci 0001:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs [ 3.559747] scsi host0: ahci [ 3.561998] ata1: SATA max UDMA/133 abar m512@0x123001 port 0x1230010100 irq 63 So for the series: Tested-by: Thierry Reding
Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
On 11/12/2020 3:59 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Wed, Nov 11, 2020 at 10:21:46PM +0530, Vidya Sagar wrote:
On 11/11/2020 9:57 PM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 11/11/20, 7:12 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52 ++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
What is the reason to use inline here?
Actually, I wanted to move the programming part inside the respective APIs
but then I wanted to give some details as well in comments so to avoid
duplication, I came up with this function. But, I'm making it inline for
better code optimization by compiler.
I don't really care either way, but I'd be surprised if the compiler
didn't inline this all by itself even without the explicit "inline".
I just checked it and you are right that compiler is indeed inlining it
without explicitly mentioning 'inline'.
I hope it is ok to leave it that way.
+{
+ /*
+ * DesignWare core version 4.90A has this strange design issue
+ * where the 'TD' bit in the Control register-1 of the ATU outbound
+ * region acts like an override for the ECRC setting i.e. the presence
+ * of TLP Digest(ECRC) in the outgoing TLPs is solely determined by
+ * this bit. This is contrary to the PCIe spec which says that the
+ * enablement of the ECRC is solely determined by the AER registers.
+ *
+ * Because of this, even when the ECRC is enabled through AER
+ * registers, the transactions going through ATU won't have TLP Digest
+ * as there is no way the AER sub-system could program the TD bit which
+ * is specific to DesignWare core.
+ *
+ * The best way to handle this scenario is to program the TD bit
+ * always. It affects only the traffic from root port to downstream
+ * devices.
+ *
+ * At this point,
+ * When ECRC is enabled in AER registers, everything works normally
+ * When ECRC is NOT enabled in AER registers, then,
+ * on Root Port:- TLP Digest (DWord size) gets appended to each packet
+ *even through it is not required. Since downstream
+ *TLPs are mostly for configuration accesses and BAR
+ *accesses, they are not in critical path and won't
+ *have much negative effect on the performance.
+ * on End Point:- TLP Digest is received for some/all the packets coming
+ *from the root port. TLP Digest is ignored because,
+ *as per the PCIe Spec r5.0 v1.0 section 2.2.3
+ *"TLP Digest Rules", when an endpoint receives TLP
+ *Digest when its ECRC check functionality is disabled
+ *in AER registers, received TLP Digest is just ignored.
+ * Since there is no issue or error reported either side, best way to
+ * handle the scenario is to program TD bit by default.
+ */
+
+ return val | PCIE_ATU_TD;
+}
Re: [PATCH V2] PCI: dwc: Add support to configure for ECRC
On 11/11/2020 9:57 PM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 11/11/20, 7:12 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52 ++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
What is the reason to use inline here?
Actually, I wanted to move the programming part inside the respective
APIs but then I wanted to give some details as well in comments so to
avoid duplication, I came up with this function. But, I'm making it
inline for better code optimization by compiler.
Best regards,
Jingoo Han
+{
+ /*
+ * DesignWare core version 4.90A has this strange design issue
+ * where the 'TD' bit in the Control register-1 of the ATU outbound
+ * region acts like an override for the ECRC setting i.e. the presence
+ * of TLP Digest(ECRC) in the outgoing TLPs is solely determined by
+ * this bit. This is contrary to the PCIe spec which says that the
+ * enablement of the ECRC is solely determined by the AER registers.
+ *
+ * Because of this, even when the ECRC is enabled through AER
+ * registers, the transactions going through ATU won't have TLP Digest
+ * as there is no way the AER sub-system could program the TD bit which
+ * is specific to DesignWare core.
+ *
+ * The best way to handle this scenario is to program the TD bit
+ * always. It affects only the traffic from root port to downstream
+ * devices.
+ *
+ * At this point,
+ * When ECRC is enabled in AER registers, everything works normally
+ * When ECRC is NOT enabled in AER registers, then,
+ * on Root Port:- TLP Digest (DWord size) gets appended to each packet
+ *even through it is not required. Since downstream
+ *TLPs are mostly for configuration accesses and BAR
+ *accesses, they are not in critical path and won't
+ *have much negative effect on the performance.
+ * on End Point:- TLP Digest is received for some/all the packets coming
+ *from the root port. TLP Digest is ignored because,
+ *as per the PCIe Spec r5.0 v1.0 section 2.2.3
+ *"TLP Digest Rules", when an endpoint receives TLP
+ *Digest when its ECRC check functionality is disabled
+ *in AER registers, received TLP Digest is just ignored.
+ * Since there is no issue or error reported either side, best way to
+ * handle the scenario is to program TD bit by default.
+ */
+
+ return val | PCIE_ATU_TD;
+}
+
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
@@ -245,8 +285,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
- type | PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -292,8 +334,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
-PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pci
[PATCH V2] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
Acked-by: Bjorn Helgaas
---
V2:
* Addressed Bjorn's comments
drivers/pci/controller/dwc/pcie-designware.c | 52 ++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ec0d13ab6bad 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,46 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
+{
+ /*
+* DesignWare core version 4.90A has this strange design issue
+* where the 'TD' bit in the Control register-1 of the ATU outbound
+* region acts like an override for the ECRC setting i.e. the presence
+* of TLP Digest(ECRC) in the outgoing TLPs is solely determined by
+* this bit. This is contrary to the PCIe spec which says that the
+* enablement of the ECRC is solely determined by the AER registers.
+*
+* Because of this, even when the ECRC is enabled through AER
+* registers, the transactions going through ATU won't have TLP Digest
+* as there is no way the AER sub-system could program the TD bit which
+* is specific to DesignWare core.
+*
+* The best way to handle this scenario is to program the TD bit
+* always. It affects only the traffic from root port to downstream
+* devices.
+*
+* At this point,
+* When ECRC is enabled in AER registers, everything works normally
+* When ECRC is NOT enabled in AER registers, then,
+* on Root Port:- TLP Digest (DWord size) gets appended to each packet
+*even through it is not required. Since downstream
+*TLPs are mostly for configuration accesses and BAR
+*accesses, they are not in critical path and won't
+*have much negative effect on the performance.
+* on End Point:- TLP Digest is received for some/all the packets coming
+*from the root port. TLP Digest is ignored because,
+*as per the PCIe Spec r5.0 v1.0 section 2.2.3
+*"TLP Digest Rules", when an endpoint receives TLP
+*Digest when its ECRC check functionality is disabled
+*in AER registers, received TLP Digest is just ignored.
+* Since there is no issue or error reported either side, best way to
+* handle the scenario is to program TD bit by default.
+*/
+
+ return val | PCIE_ATU_TD;
+}
+
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
@@ -245,8 +285,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
-type | PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -292,8 +334,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
- PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
/*
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 9d2f511f13fa..285c0ae364ae 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -88,6 +
[PATCH] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a.
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-designware.c | 50 ++--
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..ebdc37a58e94 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -225,6 +225,44 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
dw_pcie_writel_atu(pci, offset + reg, val);
}
+static inline u32 dw_pcie_enable_ecrc(u32 val)
+{
+ /*
+* DesignWare core version 4.90A has this strange design issue
+* where the 'TD' bit in the Control register-1 of the ATU outbound
+* region acts like an override for the ECRC setting i.e. the presence
+* of TLP Digest(ECRC) in the outgoing TLPs is solely determined by
+* this bit. This is contrary to the PCIe spec which says that the
+* enablement of the ECRC is solely determined by the AER registers.
+* Because of this, even when the ECRC is enabled through AER
+* registers, the transactions going through ATU won't have TLP Digest
+* as there is no way the AER sub-system could program the TD bit which
+* is specific to DsignWare core.
+*The best way to handle this scenario is to program the TD bit
+* always. It affects only the traffic from root port to downstream
+* devices.
+* At this point,
+* When ECRC is enabled in AER registers, everything works
+* normally
+* When ECRC is NOT enabled in AER registers, then,
+* on Root Port:- TLP Digest (DWord size) gets appended to each packet
+*even through it is not required. Since downstream
+*TLPs are mostly for configuration accesses and BAR
+*accesses, they are not in critical path and won't
+*have much negative effect on the performance.
+* on End Point:- TLP Digest is received for some/all the packets coming
+*from the root port. TLP Digest is ignored because,
+*as per the PCIe Spec r5.0 v1.0 section 2.2.3 "TLP
Digest Rules",
+*when an endpoint receives TLP Digest when its
+*ECRC check functionality is disabled in AER registers,
+*received TLP Digest is just ignored.
+* Since there is no issue or error reported either side, best way to
+* handle the scenario is to program TD bit by default.
+*/
+
+ return val | PCIE_ATU_TD;
+}
+
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
@@ -245,8 +283,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
-type | PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -292,8 +332,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
- PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ if (pci->version == 0x490A)
+ val = dw_pcie_enable_ecrc(val);
+ dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
/*
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 9d2f511f13fa..285c0ae364ae 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -88,6 +88,7 @@
#define PCIE_ATU_TYPE_IO 0x2
#define PC
[PATCH V4 6/6] PCI: tegra: Disable LTSSM during L2 entry
PCIe cards like Marvell SATA controller and some of the Samsung NVMe
drives don't support taking the link to L2 state. When the link doesn't
go to L2 state, Tegra194 requires the LTSSM to be disabled to allow PHY
to start the next link up process cleanly during suspend/resume sequence.
Failing to disable LTSSM results in the PCIe link not coming up in the
next resume cycle.
Signed-off-by: Vidya Sagar
---
V4:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 8c08998b9ce1..57ff0657bbe2 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1513,6 +1513,14 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
data &= ~APPL_PINMUX_PEX_RST;
appl_writel(pcie, data, APPL_PINMUX);
+ /*
+* Some cards do not go to detect state even after de-asserting
+* PERST#. So, de-assert LTSSM to bring link to detect state.
+*/
+ data = readl(pcie->appl_base + APPL_CTRL);
+ data &= ~APPL_CTRL_LTSSM_EN;
+ writel(data, pcie->appl_base + APPL_CTRL);
+
err = readl_poll_timeout_atomic(pcie->appl_base + APPL_DEBUG,
data,
((data &
@@ -1520,14 +1528,8 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
APPL_DEBUG_LTSSM_STATE_SHIFT) ==
LTSSM_STATE_PRE_DETECT,
1, LTSSM_TIMEOUT);
- if (err) {
+ if (err)
dev_info(pcie->dev, "Link didn't go to detect state\n");
- } else {
- /* Disable LTSSM after link is in detect state */
- data = appl_readl(pcie, APPL_CTRL);
- data &= ~APPL_CTRL_LTSSM_EN;
- appl_writel(pcie, data, APPL_CTRL);
- }
}
/*
* DBI registers may not be accessible after this as PLL-E would be
--
2.17.1
[PATCH V4 2/6] PCI: tegra: Map configuration space as nGnRnE
As specified in the comment for pci_remap_cfgspace() define in arch/arm64/include/asm/io.h file, PCIe configuration space should be mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from devm_ioremap_resource() for mapping DBI space as that is nothing but the root port's own configuration space. Signed-off-by: Vidya Sagar --- V4: * None V3: * None V2: * Changed 'Strongly Ordered' to 'nGnRnE' drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b172b1d49713..7a0c64436861 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) } pcie->dbi_res = dbi_res; - pci->dbi_base = devm_ioremap_resource(dev, dbi_res); + pci->dbi_base = devm_pci_remap_cfgspace(dev, + dbi_res->start, + resource_size(dbi_res)); if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); -- 2.17.1
[PATCH V4 4/6] PCI: tegra: Continue unconfig sequence even if parts fail
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
Signed-off-by: Vidya Sagar
---
V4:
* None
V3:
* Modified subject as per Bjorn's suggestion
* Removed tegra_pcie_init_controller()'s error checking part and pushed
a separate patch for it
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 39 +-
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..9be10c8953df 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -2238,8 +2227,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
PORT_LOGIC_MSI_CTRL_INT_0_EN);
tegra_pcie_downstream_dev_to_D0(pcie);
tegra_pcie_dw_pme_turnoff(pcie);
+ tegra_pcie_unconfig_controller(pcie);
- return __deinit_controller(pcie);
+ return 0;
}
static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2267,7 +2257,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2305,7 +2296,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device
*pdev)
disable_irq(pcie->pci.pp.msi_irq);
tegra_pcie_dw_pme_turnoff(pcie);
- __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
--
2.17.1
[PATCH V4 5/6] PCI: tegra: Check return value of tegra_pcie_init_controller()
The return value of tegra_pcie_init_controller() must be checked before
PCIe link up check and registering debugfs entries subsequently as it
doesn't make sense to do these when the controller initialization itself
has failed.
Signed-off-by: Vidya Sagar
---
V4:
* None
V3:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 9be10c8953df..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1579,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize controller: %d\n", ret);
+ goto fail_pm_get_sync;
+ }
pcie->link_state = tegra_pcie_dw_link_up(>pci);
if (!pcie->link_state) {
--
2.17.1
[PATCH V4 1/6] PCI: tegra: Fix ASPM-L1SS advertisement disable code
If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.
Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar
---
V4:
* None
V3:
* None
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index aa511ec0d800..b172b1d49713 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -896,6 +896,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
init_host_aspm(pcie);
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
@@ -1400,12 +1406,6 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
PCI_CAP_ID_EXP);
- /* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
return ret;
fail_phy:
--
2.17.1
[PATCH V4 3/6] PCI: tegra: Set DesignWare IP version
Set the DesignWare IP version for Tegra194 to 0x490A. This would be used by the DesigWare sub-system to do any version specific configuration (Ex:- TD bit programming for ECRC). Signed-off-by: Vidya Sagar --- V4: * None V3: * None V2: * None drivers/pci/controller/dwc/pcie-tegra194.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 7a0c64436861..253d91033bc3 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2011,6 +2011,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) pci->ops = _dw_pcie_ops; pci->n_fts[0] = N_FTS_VAL; pci->n_fts[1] = FTS_VAL; + pci->version = 0x490A; pp = >pp; pcie->dev = >dev; -- 2.17.1
[PATCH V4 0/6] Enhancements to Tegra194 PCIe driver
This series of patches do some enhancements and some bug fixes to the Tegra194 PCIe platform driver like - Fix Vendor-ID corruption - Map DBI space correctly - Update DWC IP version - Continue with uninitialization sequence even if parts fail - Check return value of tegra_pcie_init_controller() V4: * Added a new patch to address link-up issues with some of the cards V3: * Addressed Bjorn's review comments * Split earlier patch-4 into two - Continue with the uninitialization sequence even if some parts fail - Check return value of tegra_pcie_init_controller() and exit accordingly V2: * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE' Vidya Sagar (6): PCI: tegra: Fix ASPM-L1SS advertisement disable code PCI: tegra: Map configuration space as nGnRnE PCI: tegra: Set DesignWare IP version PCI: tegra: Continue unconfig sequence even if parts fail PCI: tegra: Check return value of tegra_pcie_init_controller() PCI: tegra: Disable LTSSM during L2 entry drivers/pci/controller/dwc/pcie-tegra194.c | 78 +++--- 1 file changed, 39 insertions(+), 39 deletions(-) -- 2.17.1
Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
On 11/4/2020 9:52 PM, Bjorn Helgaas wrote: External email: Use caution opening links or attachments On Wed, Nov 04, 2020 at 05:13:07PM +0530, Vidya Sagar wrote: On 11/4/2020 2:37 AM, Bjorn Helgaas wrote: On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: On Thu, Oct 29, 2020 at 11:09:59AM +0530, Vidya Sagar wrote: DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. I guess this is a hardware defect, right? Yes. This is common across all DWC implementations (version 4.90 precisely) How much of a problem would it be if we instead added a "no_ecrc" quirk for this hardware so we never enabled ECRC? Well, on Tegra for some of the high fidelity use cases, ECRC is required to be turned on and if it can be done safely with these patches, why shouldn't we not enable ECRC at all? IIUC, the current Linux support of ECRC is a single choice at boot-time: by default ECRC is not enabled, but if you boot with "pci=ecrc=on", we turn on ECRC for every device. That seems like the minimal support, but I think the spec allows ECRC to be enabled selectively, on individual devices. I can imagine a sysfs knob that would allow us to enable/disable ECRC per-device at run-time. If we had such a sysfs knob, it would be pretty ugly and maybe impractical to work around this hardware issue. So I'm a little bit hesitant to add functionality that might have to be removed in the future. Agree with this. But since it is a boot-time choice at this point, I think we can still go ahead with this approach to have a working ECRC mechanism right? I don't see any sysfs knob for AER controlling at this point. I don't want to do anything that will prevent us from adding per-device ECRC control in the future. My concern is that if we add a run-time sysfs knob in the future, the user experience on this hardware will be poor because there's no convenient path to twiddle the PCIE_ATU_TD bit when the generic code changes the AER Control bit. Agree. Can we add it to the documentation that run time changing of ECRC settings are not supported on this (i.e. Tegra194) platform (or for that matter on any SoC with PCIe based on DesignWare core version 4.90A). By 'not supported', I meant that the ECRC digest part may not work but the normal functionality will continue to work without reporting any AER errors. I tried to emulate the following scenarios on Tegra194 silicon and here are my observations. FWIW, I'm referring to the PCIe spec Rev 5.0 Ver 1.0 (22 May 2019) What is the failure mode in these scenarios: - User boots with defaults, ECRC is disabled. - User enables ECRC via sysfs. - What happens here? ECRC is enabled via AER Control but not via DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe errors (malformed TLP, etc)? Since DWC TD bit is not programmed, for all the transactions that go through ATU, TLP Digest won't get generated (although AER registers indicate that ECRC should get generated). As per the spec section "2.7.1 ECRC Rules" --- If a device Function is enabled to generate ECRC, it must calculate and apply ECRC for all TLPs originated by the Function --- So the RP would be violating the PCIe spec, but it doesn't result in any error because the same section has the following rule as well --- If a device Function is enabled to check ECRC, it must do so for all TLPs with ECRC where the device is the ultimate PCI Express Receiver Note that it is still possible for the Function to receive TLPs without ECRC, and these are processed normally - this is not an error --- so, even if the EP has ECRC check enabled, because of the above rule, it just processes those packets without ECRC as normal packets. Basically, whoever is enabling ECRC run time gets cheated as transactions routed through ATU don't really have ECRC digest Or this one: - User boots with "pci=ecrc=on", ECRC is enabled. - ECRC works fine. - User disables ECRC via sysfs. - What happens here? ECRC is disabled via AER Control, but DWC TD bit thinks it's still enabled. In this case, the EP doesn't have ECRC check enabled but receives TLPs with ECRC digest. This again won't result in any error because of the section "2.2.3 TLP Digest Rules" which says --- If an intermediate or ultimate PCI Express Receiver of the TLP does not support ECRC checking, the Receiver must ignore the TLP Digest --- So the EP just ignores the TLP Digest and process the TLP normally. Although functionality wise there is no issue here, there could be some impact on the perf because of the extra DWord data. This is again debatable as th
Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
On 11/4/2020 2:37 AM, Bjorn Helgaas wrote: External email: Use caution opening links or attachments On Tue, Nov 03, 2020 at 08:57:01AM +0530, Vidya Sagar wrote: On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: On Thu, Oct 29, 2020 at 11:09:59AM +0530, Vidya Sagar wrote: DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. I guess this is a hardware defect, right? Yes. This is common across all DWC implementations (version 4.90 precisely) How much of a problem would it be if we instead added a "no_ecrc" quirk for this hardware so we never enabled ECRC? Well, on Tegra for some of the high fidelity use cases, ECRC is required to be turned on and if it can be done safely with these patches, why shouldn't we not enable ECRC at all? IIUC, the current Linux support of ECRC is a single choice at boot-time: by default ECRC is not enabled, but if you boot with "pci=ecrc=on", we turn on ECRC for every device. That seems like the minimal support, but I think the spec allows ECRC to be enabled selectively, on individual devices. I can imagine a sysfs knob that would allow us to enable/disable ECRC per-device at run-time. If we had such a sysfs knob, it would be pretty ugly and maybe impractical to work around this hardware issue. So I'm a little bit hesitant to add functionality that might have to be removed in the future. Agree with this. But since it is a boot-time choice at this point, I think we can still go ahead with this approach to have a working ECRC mechanism right? I don't see any sysfs knob for AER controlling at this point. I don't want to do anything that will prevent us from adding per-device ECRC control in the future. My concern is that if we add a run-time sysfs knob in the future, the user experience on this hardware will be poor because there's no convenient path to twiddle the PCIE_ATU_TD bit when the generic code changes the AER Control bit. Agree. Can we add it to the documentation that run time changing of ECRC settings are not supported on this (i.e. Tegra194) platform (or for that matter on any SoC with PCIe based on DesignWare core version 4.90A). By 'not supported', I meant that the ECRC digest part may not work but the normal functionality will continue to work without reporting any AER errors. I tried to emulate the following scenarios on Tegra194 silicon and here are my observations. FWIW, I'm referring to the PCIe spec Rev 5.0 Ver 1.0 (22 May 2019) What is the failure mode in these scenarios: - User boots with defaults, ECRC is disabled. - User enables ECRC via sysfs. - What happens here? ECRC is enabled via AER Control but not via DWC TD bit. I assume ECRC doesn't work. Does it cause PCIe errors (malformed TLP, etc)? Since DWC TD bit is not programmed, for all the transactions that go through ATU, TLP Digest won't get generated (although AER registers indicate that ECRC should get generated). As per the spec section "2.7.1 ECRC Rules" --- If a device Function is enabled to generate ECRC, it must calculate and apply ECRC for all TLPs originated by the Function --- So the RP would be violating the PCIe spec, but it doesn't result in any error because the same section has the following rule as well --- If a device Function is enabled to check ECRC, it must do so for all TLPs with ECRC where the device is the ultimate PCI Express Receiver Note that it is still possible for the Function to receive TLPs without ECRC, and these are processed normally - this is not an error --- so, even if the EP has ECRC check enabled, because of the above rule, it just processes those packets without ECRC as normal packets. Basically, whoever is enabling ECRC run time gets cheated as transactions routed through ATU don't really have ECRC digest Or this one: - User boots with "pci=ecrc=on", ECRC is enabled. - ECRC works fine. - User disables ECRC via sysfs. - What happens here? ECRC is disabled via AER Control, but DWC TD bit thinks it's still enabled. In this case, the EP doesn't have ECRC check enabled but receives TLPs with ECRC digest. This again won't result in any error because of the section "2.2.3 TLP Digest Rules" which says --- If an intermediate or ultimate PCI Express Receiver of the TLP does not support ECRC checking, the Receiver must ignore the TLP Digest --- So the EP just ignores the TLP Digest and process the TLP normally. Although functionality wise there is no issue here, there could be some impact on the perf because of the extra DWord data. This is again debatable as the perf/data path is typically from EP's DMA engine to host system memory and not config/BAR acce
[PATCH V3 0/5] Enhancements to Tegra194 PCIe driver
This series of patches do some enhancements and some bug fixes to the Tegra194 PCIe platform driver like - Fix Vendor-ID corruption - Map DBI space correctly - Update DWC IP version - Continue with uninitialization sequence even if parts fail - Check return value of tegra_pcie_init_controller() V3: * Addressed Bjorn's review comments * Split earlier patch-4 into two - Continue with the uninitialization sequence even if some parts fail - Check return value of tegra_pcie_init_controller() and exit accordingly V2: * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE' Vidya Sagar (5): PCI: tegra: Fix ASPM-L1SS advertisement disable code PCI: tegra: Map configuration space as nGnRnE PCI: tegra: Set DesignWare IP version PCI: tegra: Continue unconfig sequence even if parts fail PCI: tegra: Check return value of tegra_pcie_init_controller() drivers/pci/controller/dwc/pcie-tegra194.c | 62 +++--- 1 file changed, 30 insertions(+), 32 deletions(-) -- 2.17.1
[PATCH V3 4/5] PCI: tegra: Continue unconfig sequence even if parts fail
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
Signed-off-by: Vidya Sagar
---
V3:
* Modified subject as per Bjorn's suggestion
* Removed tegra_pcie_init_controller()'s error checking part and pushed
a separate patch for it
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 39 +-
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..9be10c8953df 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -2238,8 +2227,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
PORT_LOGIC_MSI_CTRL_INT_0_EN);
tegra_pcie_downstream_dev_to_D0(pcie);
tegra_pcie_dw_pme_turnoff(pcie);
+ tegra_pcie_unconfig_controller(pcie);
- return __deinit_controller(pcie);
+ return 0;
}
static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2267,7 +2257,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2305,7 +2296,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device
*pdev)
disable_irq(pcie->pci.pp.msi_irq);
tegra_pcie_dw_pme_turnoff(pcie);
- __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
--
2.17.1
[PATCH V3 2/5] PCI: tegra: Map configuration space as nGnRnE
As specified in the comment for pci_remap_cfgspace() define in arch/arm64/include/asm/io.h file, PCIe configuration space should be mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from devm_ioremap_resource() for mapping DBI space as that is nothing but the root port's own configuration space. Signed-off-by: Vidya Sagar --- V3: * None V2: * Changed 'Strongly Ordered' to 'nGnRnE' drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b172b1d49713..7a0c64436861 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) } pcie->dbi_res = dbi_res; - pci->dbi_base = devm_ioremap_resource(dev, dbi_res); + pci->dbi_base = devm_pci_remap_cfgspace(dev, + dbi_res->start, + resource_size(dbi_res)); if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); -- 2.17.1
[PATCH V3 1/5] PCI: tegra: Fix ASPM-L1SS advertisement disable code
If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.
Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar
---
V3:
* None
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index aa511ec0d800..b172b1d49713 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -896,6 +896,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
init_host_aspm(pcie);
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
@@ -1400,12 +1406,6 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
PCI_CAP_ID_EXP);
- /* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
return ret;
fail_phy:
--
2.17.1
Re: [PATCH V2 4/4] PCI: tegra: Handle error conditions properly
On 11/4/2020 2:18 AM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
Hi Vidya,
Can you update the subject to replace "properly" with more details
about what the patch is doing? "Properly" is really meaningless in
usages like this -- nobody writes patches to do the *wrong* thing, so
it goes without saying that every patch is intended to things
"properly".
It would also help to have some context. My first thought was that
"error conditions" referred to PCIe errors like completion timeouts,
completer abort, etc.
Maybe something like:
PCI: tegra: Continue unconfig sequence even if parts fail
Thanks for reviewing the change.
Sure. I'll go with the above subject line.
PCI: tegra: Return init error (not unconfig error) on init failure
On Thu, Oct 29, 2020 at 10:48:39AM +0530, Vidya Sagar wrote:
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
It also adds checking return value for error for a cleaner exit path.
This paragraph uses "it" to refer to both "the driver" (second
sentence) and "this patch" (last sentence). That's confusing.
There's no reason to refer to "this patch" at all. I'd rather have
"Add checking ..." than "It adds checking ..."
I think that last sentence must be referring to the
tegra_pcie_init_controller() change to return the initialization error
rather than the error from __deinit_controller(). That seems right,
but should be a separate patch.
Sure. I'll push a new patch for this.
Signed-off-by: Vidya Sagar
---
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 45 ++
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -1590,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "
[PATCH V3 5/5] PCI: tegra: Check return value of tegra_pcie_init_controller()
The return value of tegra_pcie_init_controller() must be checked before
PCIe link up check and registering debugfs entries subsequently as it
doesn't make sense to do these when the controller initialization itself
has failed.
Signed-off-by: Vidya Sagar
---
V3:
* New patch in this series
drivers/pci/controller/dwc/pcie-tegra194.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 9be10c8953df..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1579,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize controller: %d\n", ret);
+ goto fail_pm_get_sync;
+ }
pcie->link_state = tegra_pcie_dw_link_up(>pci);
if (!pcie->link_state) {
--
2.17.1
[PATCH V3 3/5] PCI: tegra: Set DesignWare IP version
Set the DesignWare IP version for Tegra194 to 0x490A. This would be used by the DesigWare sub-system to do any version specific configuration (Ex:- TD bit programming for ECRC). Signed-off-by: Vidya Sagar --- V3: * None V2: * None drivers/pci/controller/dwc/pcie-tegra194.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 7a0c64436861..253d91033bc3 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2011,6 +2011,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) pci->ops = _dw_pcie_ops; pci->n_fts[0] = N_FTS_VAL; pci->n_fts[1] = FTS_VAL; + pci->version = 0x490A; pp = >pp; pcie->dev = >dev; -- 2.17.1
Re: [PATCH 0/3] Add support to handle prefetchable memory
Lorenzo / Bjorn, Could you please review patches-1 & 2 in this series? For the third patch, we already went with Rob's patch @ http://patchwork.ozlabs.org/project/linux-pci/patch/20201026154852.221483-1-r...@kernel.org/ Thanks, Vidya Sagar On 10/26/2020 6:02 PM, Thierry Reding wrote: On Sat, Oct 24, 2020 at 04:03:41AM +, Jingoo Han wrote: On 10/23/20, 3:57 PM, Vidya Sagar wrote: This patch series adds support for configuring the DesignWare IP's ATU region for prefetchable memory translations. It first starts by flagging a warning if the size of non-prefetchable aperture goes beyond 32-bit as PCIe spec doesn't allow it. And then adds required support for programming the ATU to handle higher (i.e. >4GB) sizes and then finally adds support for differentiating between prefetchable and non-prefetchable regions and configuring one of the ATU regions for prefetchable memory translations purpose. Vidya Sagar (3): PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit PCI: dwc: Add support to program ATU for >4GB memory aperture sizes PCI: dwc: Add support to handle prefetchable memory mapping For 2nd & 3rd, Acked-by: Jingoo But, I still want someone to ack 1st patch, not me. To Vidya, If possible, can you ask your coworker to give 'Tested-by'? It will be very helpful. Thank you. On next-20201026 (but also going back quite a while) I'm seeing this during boot on Jetson AGX Xavier (Tegra194): [3.493382] ahci 0001:01:00.0: version 3.0 [3.493889] ahci 0001:01:00.0: SSS flag set, parallel bus scan disabled [4.497706] ahci 0001:01:00.0: controller reset failed (0x) [4.498114] ahci: probe of 0001:01:00.0 failed with error -5 After applying this series, AHCI over PCI is back to normal: [3.543230] ahci 0001:01:00.0: AHCI 0001. 32 slots 1 ports 6 Gbps 0x1 impl SATA mode [3.550841] ahci 0001:01:00.0: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs [3.559747] scsi host0: ahci [3.561998] ata1: SATA max UDMA/133 abar m512@0x123001 port 0x1230010100 irq 63 So for the series: Tested-by: Thierry Reding
Re: [PATCH] PCI/ASPM: Save/restore ASPM-L1SS controls for suspend/resume
Bjorn / Lorenzo,
Could you please review this change?
Thanks,
Vidya Sagar
On 10/25/2020 12:34 AM, Vidya Sagar wrote:
Previously ASPM L1-Sub-States control registers (CTL1 and CTL2) weren't
saved and restored during suspend/resume leading to ASPM-L1SS
configuration being lost post resume.
Save the ASPM-L1SS control registers so that the configuration is retained
post resume.
Signed-off-by: Vidya Sagar
---
v1:
* It would be really good if someone can verify it on a non tegra194 platform
drivers/pci/pci.c | 7 +++
drivers/pci/pci.h | 4
drivers/pci/pcie/aspm.c | 41 +
3 files changed, 52 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a458c46d7e39..034497264bde 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1551,6 +1551,7 @@ int pci_save_state(struct pci_dev *dev)
return i;
pci_save_ltr_state(dev);
+ pci_save_aspm_l1ss_state(dev);
pci_save_dpc_state(dev);
pci_save_aer_state(dev);
return pci_save_vc_state(dev);
@@ -1656,6 +1657,7 @@ void pci_restore_state(struct pci_dev *dev)
* LTR itself (in the PCIe capability).
*/
pci_restore_ltr_state(dev);
+ pci_restore_aspm_l1ss_state(dev);
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
@@ -3319,6 +3321,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
+ 2 * sizeof(u32));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for
ASPM-L1SS\n");
+
pci_allocate_vc_save_buffers(dev);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..8d2135f61e36 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -565,11 +565,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
void pcie_aspm_exit_link_state(struct pci_dev *pdev);
void pcie_aspm_pm_state_change(struct pci_dev *pdev);
void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pci_save_aspm_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
#else
static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
+static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
+static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
#endif
#ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..d965bbc563ed 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -742,6 +742,47 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state
*link, u32 state)
PCI_L1SS_CTL1_L1SS_MASK, val);
}
+void pci_save_aspm_l1ss_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int aspm_l1ss;
+ u32 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!aspm_l1ss)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state)
+ return;
+
+ cap = (u32 *)_state->cap.data[0];
+ pci_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, cap++);
+ pci_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, cap++);
+}
+
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int aspm_l1ss;
+ u32 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state || !aspm_l1ss)
+ return;
+
+ cap = (u32 *)_state->cap.data[0];
+ pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++);
+ pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
+}
+
static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
{
pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
On 11/3/2020 4:32 AM, Bjorn Helgaas wrote: External email: Use caution opening links or attachments On Thu, Oct 29, 2020 at 11:09:59AM +0530, Vidya Sagar wrote: DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. I guess this is a hardware defect, right? Yes. This is common across all DWC implementations (version 4.90 precisely) How much of a problem would it be if we instead added a "no_ecrc" quirk for this hardware so we never enabled ECRC? Well, on Tegra for some of the high fidelity use cases, ECRC is required to be turned on and if it can be done safely with these patches, why shouldn't we not enable ECRC at all? IIUC, the current Linux support of ECRC is a single choice at boot-time: by default ECRC is not enabled, but if you boot with "pci=ecrc=on", we turn on ECRC for every device. That seems like the minimal support, but I think the spec allows ECRC to be enabled selectively, on individual devices. I can imagine a sysfs knob that would allow us to enable/disable ECRC per-device at run-time. If we had such a sysfs knob, it would be pretty ugly and maybe impractical to work around this hardware issue. So I'm a little bit hesitant to add functionality that might have to be removed in the future. Agree with this. But since it is a boot-time choice at this point, I think we can still go ahead with this approach to have a working ECRC mechanism right? I don't see any sysfs knob for AER controlling at this point. Signed-off-by: Vidya Sagar Reviewed-by: Jingoo Han --- V3: * Added 'Reviewed-by: Jingoo Han ' V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index b5e438b70cd5..cbd651b219d2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; val = upper_32_bits(size - 1) ? val | PCIE_ATU_INCREASE_REGION_SIZE : val; dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); @@ -294,8 +296,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | -PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); /* diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index e7f441441db2..b01ef407fd52 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -89,6 +89,7 @@ #define PCIE_ATU_TYPE_IO 0x2 #define PCIE_ATU_TYPE_CFG0 0x4 #define PCIE_ATU_TYPE_CFG1 0x5 +#define PCIE_ATU_TD_SHIFT8 #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) #define PCIE_ATU_CR2 0x908 #define PCIE_ATU_ENABLE BIT(31) -- 2.17.1
Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
On 11/2/2020 7:45 PM, Rob Herring wrote: External email: Use caution opening links or attachments On Thu, Oct 29, 2020 at 12:40 AM Vidya Sagar wrote: DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. Signed-off-by: Vidya Sagar Reviewed-by: Jingoo Han --- V3: * Added 'Reviewed-by: Jingoo Han ' V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index b5e438b70cd5..cbd651b219d2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; val = upper_32_bits(size - 1) ? val | PCIE_ATU_INCREASE_REGION_SIZE : val; dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); @@ -294,8 +296,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | - PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) Is this even possible? Are the non-unroll ATU registers available post 4.80? I'm not sure. Gustavo might have information about this. I made this change so that it is taken care off even if they available. Rob
Re: [PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
On 10/30/2020 3:33 AM, Jingoo Han wrote: External email: Use caution opening links or attachments On 10/29/20, 1:40 AM, Vidya Sagar wrote: DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. Signed-off-by: Vidya Sagar Reviewed-by: Jingoo Han No, it should be Acked-by. I gave you Acked-by, not Reviewed-by. Acked-by: Jingoo Han Apologies. My bad. I saw the 'Reviewed-by' of the other patch (i.e. PCI/AER: Add pcie_is_ecrc_enabled() API) and put that for this patch as well. I'll update. Best regards, Jingoo Han --- V3: * Added 'Reviewed-by: Jingoo Han ' V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) [...]
[PATCH V2 1/4] PCI: tegra: Fix ASPM-L1SS advertisement disable code
If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.
Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar
---
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index aa511ec0d800..b172b1d49713 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -896,6 +896,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
init_host_aspm(pcie);
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
@@ -1400,12 +1406,6 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
PCI_CAP_ID_EXP);
- /* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
return ret;
fail_phy:
--
2.17.1
[PATCH V2 3/4] PCI: tegra: Set DesignWare IP version
Set the DesignWare IP version for Tegra194 to 0x490A. This would be used by the DesigWare sub-system to do any version specific configuration (Ex:- TD bit programming for ECRC). Signed-off-by: Vidya Sagar --- V2: * None drivers/pci/controller/dwc/pcie-tegra194.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 7a0c64436861..253d91033bc3 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2011,6 +2011,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) pci->ops = _dw_pcie_ops; pci->n_fts[0] = N_FTS_VAL; pci->n_fts[1] = FTS_VAL; + pci->version = 0x490A; pp = >pp; pcie->dev = >dev; -- 2.17.1
[PATCH V2 2/4] PCI: tegra: Map configuration space as nGnRnE
As specified in the comment for pci_remap_cfgspace() define in arch/arm64/include/asm/io.h file, PCIe configuration space should be mapped as nGnRnE. Hence changing to dev_pci_remap_cfgspace() from devm_ioremap_resource() for mapping DBI space as that is nothing but the root port's own configuration space. Signed-off-by: Vidya Sagar --- V2: * Changed 'Strongly Ordered' to 'nGnRnE' drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b172b1d49713..7a0c64436861 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) } pcie->dbi_res = dbi_res; - pci->dbi_base = devm_ioremap_resource(dev, dbi_res); + pci->dbi_base = devm_pci_remap_cfgspace(dev, + dbi_res->start, + resource_size(dbi_res)); if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); -- 2.17.1
[PATCH V3 0/2] Add support to configure DWC for ECRC
This series has two patches. Patch-1: Adds a public API to query if the system has ECRC policty turned on. Patch-2: DesignWare core PCIe IP has a TLP Digest (TD) override bit in one of its control registers of ATU. This bit needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. DWC code queries the PCIe sub-system through the API added in Patch-1 to find out if ECRC is turned on or not and configures ATU accordingly. V3: * Addressed Ethan Zhao's comments for patch-1 V2: * Addressed Jingoo's review comments Vidya Sagar (2): PCI/AER: Add pcie_is_ecrc_enabled() API PCI: dwc: Add support to configure for ECRC drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + drivers/pci/pci.h| 2 ++ drivers/pci/pcie/aer.c | 11 +++ 4 files changed, 20 insertions(+), 2 deletions(-) -- 2.17.1
[PATCH V3 2/2] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. Signed-off-by: Vidya Sagar Reviewed-by: Jingoo Han --- V3: * Added 'Reviewed-by: Jingoo Han ' V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index b5e438b70cd5..cbd651b219d2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; val = upper_32_bits(size - 1) ? val | PCIE_ATU_INCREASE_REGION_SIZE : val; dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); @@ -294,8 +296,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | - PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); /* diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index e7f441441db2..b01ef407fd52 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -89,6 +89,7 @@ #define PCIE_ATU_TYPE_IO 0x2 #define PCIE_ATU_TYPE_CFG0 0x4 #define PCIE_ATU_TYPE_CFG1 0x5 +#define PCIE_ATU_TD_SHIFT 8 #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) #define PCIE_ATU_CR2 0x908 #define PCIE_ATU_ENABLEBIT(31) -- 2.17.1
[PATCH V3 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API
Adds pcie_is_ecrc_enabled() API to let other sub-systems (like DesignWare)
to query if ECRC policy is enabled and perform any configuration
required in those respective sub-systems.
Signed-off-by: Vidya Sagar
Reviewed-by: Jingoo Han
---
V3:
* Address Ethan Zhao's comments
* Added 'Reviewed-by: Jingoo Han '
V2:
* None from V1
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/aer.c | 11 +++
2 files changed, 13 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..325fdbf91dde 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -575,9 +575,11 @@ static inline void pcie_aspm_powersave_config_link(struct
pci_dev *pdev) { }
#ifdef CONFIG_PCIE_ECRC
void pcie_set_ecrc_checking(struct pci_dev *dev);
void pcie_ecrc_get_policy(char *str);
+bool pcie_is_ecrc_enabled(void);
#else
static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }
static inline void pcie_ecrc_get_policy(char *str) { }
+static inline bool pcie_is_ecrc_enabled(void) { return false; }
#endif
#ifdef CONFIG_PCIE_PTM
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..d0f5a7043aff 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -207,6 +207,17 @@ void pcie_ecrc_get_policy(char *str)
ecrc_policy = i;
}
+
+/**
+ * pcie_is_ecrc_enabled - returns if ECRC is enabled in the system or not
+ *
+ * Returns true if ECRC policy is enabled and false otherwise
+ */
+bool pcie_is_ecrc_enabled(void)
+{
+ return ecrc_policy == ECRC_POLICY_ON;
+}
+EXPORT_SYMBOL(pcie_is_ecrc_enabled);
#endif /* CONFIG_PCIE_ECRC */
#definePCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE |
PCI_EXP_DEVCTL_NFERE | \
--
2.17.1
[PATCH V2 4/4] PCI: tegra: Handle error conditions properly
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
It also adds checking return value for error for a cleaner exit path.
Signed-off-by: Vidya Sagar
---
V2:
* None
drivers/pci/controller/dwc/pcie-tegra194.c | 45 ++
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -1590,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize controller: %d\n", ret);
+ goto fail_pm_get_sync;
+ }
pcie->link_state = tegra_pcie_dw_link_up(>pci);
if (!pcie->link_state) {
@@ -2238,8 +2231,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
PORT_LOGIC_MSI_CTRL_INT_0_EN);
tegra_pcie_downstream_dev_to_D0(pcie);
tegra_pcie_dw_pme_turnoff(pcie);
+ tegra_pcie_unconfig_controller(pcie);
- return __deinit_controller(pcie);
+ return 0;
}
static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2267,7 +2261,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2305,7 +2300,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device
*pdev)
disable_irq(pcie->pci.pp.msi_irq);
tegra_pcie_dw_pme_turnoff(pcie);
- __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
--
2.17.1
[PATCH V2 0/4] Enhancements to Tegra194 PCIe driver
This series of patches do some enhancements and some bug fixes to the Tegra194 PCIe platform driver like - Fixing Vendor-ID corruption - Mapping DBI space correctly - Updating DWC IP version - Handling error conditions properly V2; * Addressed Rob's comments. Changed 'Strongly Ordered' to 'nGnRnE' Vidya Sagar (4): PCI: tegra: Fix ASPM-L1SS advertisement disable code PCI: tegra: Map configuration space as nGnRnE PCI: tegra: Set DesignWare IP version PCI: tegra: Handle error conditions properly drivers/pci/controller/dwc/pcie-tegra194.c | 62 +++--- 1 file changed, 30 insertions(+), 32 deletions(-) -- 2.17.1
[PATCH V2 2/2] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control registers of ATU. This bit also needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. This patch does the required programming in ATU upon querying the system policy for ECRC. Signed-off-by: Vidya Sagar --- V2: * Addressed Jingoo's review comment * Removed saving 'td' bit information in 'dw_pcie' structure drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index b5e438b70cd5..cbd651b219d2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -246,6 +246,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET, upper_32_bits(pci_addr)); val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; val = upper_32_bits(size - 1) ? val | PCIE_ATU_INCREASE_REGION_SIZE : val; dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val); @@ -294,8 +296,10 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no, lower_32_bits(pci_addr)); dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET, upper_32_bits(pci_addr)); - dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type | - PCIE_ATU_FUNC_NUM(func_no)); + val = type | PCIE_ATU_FUNC_NUM(func_no); + if (pci->version == 0x490A) + val = val | pcie_is_ecrc_enabled() << PCIE_ATU_TD_SHIFT; + dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val); dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE); /* diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 21dd06831b50..e5449b205c22 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -90,6 +90,7 @@ #define PCIE_ATU_TYPE_IO 0x2 #define PCIE_ATU_TYPE_CFG0 0x4 #define PCIE_ATU_TYPE_CFG1 0x5 +#define PCIE_ATU_TD_SHIFT 8 #define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20) #define PCIE_ATU_CR2 0x908 #define PCIE_ATU_ENABLEBIT(31) -- 2.17.1
[PATCH V2 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API
Adds pcie_is_ecrc_enabled() API to let other sub-systems (like DesignWare)
to query if ECRC policy is enabled and perform any configuration
required in those respective sub-systems.
Signed-off-by: Vidya Sagar
---
V2:
* None from V1
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/aer.c | 11 +++
2 files changed, 13 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..325fdbf91dde 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -575,9 +575,11 @@ static inline void pcie_aspm_powersave_config_link(struct
pci_dev *pdev) { }
#ifdef CONFIG_PCIE_ECRC
void pcie_set_ecrc_checking(struct pci_dev *dev);
void pcie_ecrc_get_policy(char *str);
+bool pcie_is_ecrc_enabled(void);
#else
static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }
static inline void pcie_ecrc_get_policy(char *str) { }
+static inline bool pcie_is_ecrc_enabled(void) { return false; }
#endif
#ifdef CONFIG_PCIE_PTM
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..24363c895aba 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -207,6 +207,17 @@ void pcie_ecrc_get_policy(char *str)
ecrc_policy = i;
}
+
+/**
+ * pcie_is_ecrc_enabled - returns if ECRC is enabled in the system or not
+ *
+ * Returns 1 if ECRC policy is enabled and 0 otherwise
+ */
+bool pcie_is_ecrc_enabled(void)
+{
+ return ecrc_policy == ECRC_POLICY_ON;
+}
+EXPORT_SYMBOL(pcie_is_ecrc_enabled);
#endif /* CONFIG_PCIE_ECRC */
#definePCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE |
PCI_EXP_DEVCTL_NFERE | \
--
2.17.1
[PATCH V2 0/2] Add support to configure DWC for ECRC
This series has two patches. Patch-1: Adds a public API to query if the system has ECRC policty turned on. Patch-2: DesignWare core PCIe IP has a TLP Digest (TD) override bit in one of its control registers of ATU. This bit needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. DWC code queries the PCIe sub-system through the API added in Patch-1 to find out if ECRC is turned on or not and configures ATU accordingly. V2: * Addressed Jingoo's review comments Vidya Sagar (2): PCI/AER: Add pcie_is_ecrc_enabled() API PCI: dwc: Add support to configure for ECRC drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 1 + drivers/pci/pci.h| 2 ++ drivers/pci/pcie/aer.c | 11 +++ 4 files changed, 20 insertions(+), 2 deletions(-) -- 2.17.1
Re: [PATCH 2/2] PCI: dwc: Add support to configure for ECRC
On 10/26/2020 2:19 AM, Jingoo Han wrote:
External email: Use caution opening links or attachments
On 10/25/20, 3:31 AM, Vidya Sagar wrote:
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a. This patch does the required programming in ATU upon
querying the system policy for ECRC.
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-designware.c | 8 ++--
drivers/pci/controller/dwc/pcie-designware.h | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index b5e438b70cd5..810dcbdbe869 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -245,7 +245,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- val = type | PCIE_ATU_FUNC_NUM(func_no);
+ val = type | PCIE_ATU_FUNC_NUM(func_no) | pci->td << PCIE_ATU_TD_SHIFT;
val = upper_32_bits(size - 1) ?
val | PCIE_ATU_INCREASE_REGION_SIZE : val;
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
@@ -295,7 +295,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
-PCIE_ATU_FUNC_NUM(func_no));
+PCIE_ATU_FUNC_NUM(func_no) |
+pci->td << PCIE_ATU_TD_SHIFT);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
/*
@@ -565,6 +566,9 @@ void dw_pcie_setup(struct dw_pcie *pci)
dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
"enabled" : "disabled");
+ if (pci->version == 0x490A)
+ pci->td = pcie_is_ecrc_enabled();
+
if (pci->link_gen > 0)
dw_pcie_link_set_max_speed(pci, pci->link_gen);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 21dd06831b50..d34723e42e79 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -90,6 +90,7 @@
#define PCIE_ATU_TYPE_IO 0x2
#define PCIE_ATU_TYPE_CFG0 0x4
#define PCIE_ATU_TYPE_CFG1 0x5
+#define PCIE_ATU_TD_SHIFT8
#define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20)
#define PCIE_ATU_CR2 0x908
#define PCIE_ATU_ENABLE BIT(31)
@@ -276,6 +277,7 @@ struct dw_pcie {
int num_lanes;
int link_gen;
u8 n_fts[2];
+ booltd; /* TLP Digest (for ECRC purpose) */
If possible, don't add a new variable to 'dw_pcie' structure.
Please find a way to set TD bit without adding a new variable to 'dw_pcie'
structure'.
I can use pcie_is_ecrc_enabled() directly in place of pci->td. That
should be fine right? BTW, curious to know if there is any specific
reason behind asking not to add any new variables to 'dw_pcie' structure?
Best regards,
Jingoo Han
};
#define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
--
2.17.1
[PATCH 3/4] PCI: tegra: Set DesignWare IP version
Set the DesignWare IP version for Tegra194 to 0x490A. This would be used by the DesigWare sub-system to do any version specific configuration (Ex:- TD bit programming for ECRC). Signed-off-by: Vidya Sagar --- drivers/pci/controller/dwc/pcie-tegra194.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 7a0c64436861..253d91033bc3 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2011,6 +2011,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) pci->ops = _dw_pcie_ops; pci->n_fts[0] = N_FTS_VAL; pci->n_fts[1] = FTS_VAL; + pci->version = 0x490A; pp = >pp; pcie->dev = >dev; -- 2.17.1
[PATCH 4/4] PCI: tegra: Handle error conditions properly
Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.
It also adds checking return value for error for a cleaner exit path.
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-tegra194.c | 45 ++
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 253d91033bc3..8c08998b9ce1 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
return ret;
}
-static int __deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
{
int ret;
ret = reset_control_assert(pcie->core_rst);
- if (ret) {
- dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
- ret);
- return ret;
- }
+ if (ret)
+ dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
ret);
tegra_pcie_disable_phy(pcie);
ret = reset_control_assert(pcie->core_apb_rst);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
- return ret;
- }
clk_disable_unprepare(pcie->core_clk);
ret = regulator_disable(pcie->pex_ctl_supply);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
- return ret;
- }
tegra_pcie_disable_slot_regulators(pcie);
ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
- if (ret) {
+ if (ret)
dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
pcie->cid, ret);
- return ret;
- }
-
- return ret;
}
static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
@@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct
tegra_pcie_dw *pcie)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
@@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct
tegra_pcie_dw *pcie)
appl_writel(pcie, data, APPL_PINMUX);
}
-static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
+static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
{
tegra_pcie_downstream_dev_to_D0(pcie);
dw_pcie_host_deinit(>pci.pp);
tegra_pcie_dw_pme_turnoff(pcie);
-
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
@@ -1590,7 +1579,11 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw
*pcie)
goto fail_pm_get_sync;
}
- tegra_pcie_init_controller(pcie);
+ ret = tegra_pcie_init_controller(pcie);
+ if (ret < 0) {
+ dev_err(dev, "Failed to initialize controller: %d\n", ret);
+ goto fail_pm_get_sync;
+ }
pcie->link_state = tegra_pcie_dw_link_up(>pci);
if (!pcie->link_state) {
@@ -2238,8 +2231,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
PORT_LOGIC_MSI_CTRL_INT_0_EN);
tegra_pcie_downstream_dev_to_D0(pcie);
tegra_pcie_dw_pme_turnoff(pcie);
+ tegra_pcie_unconfig_controller(pcie);
- return __deinit_controller(pcie);
+ return 0;
}
static int tegra_pcie_dw_resume_noirq(struct device *dev)
@@ -2267,7 +2261,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
return 0;
fail_host_init:
- return __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
+ return ret;
}
static int tegra_pcie_dw_resume_early(struct device *dev)
@@ -2305,7 +2300,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device
*pdev)
disable_irq(pcie->pci.pp.msi_irq);
tegra_pcie_dw_pme_turnoff(pcie);
- __deinit_controller(pcie);
+ tegra_pcie_unconfig_controller(pcie);
}
static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
--
2.17.1
[PATCH 0/4] Enhancements to Tegra194 PCIe driver
This series of patches do some enhancements and some bug fixes to the Tegra194 PCIe platform driver like - Fixing Vendor-ID corruption - Mapping DBI space correctly - Updating DWC IP version - Handling error conditions properly Vidya Sagar (4): PCI: tegra: Fix ASPM-L1SS advertisement disable code PCI: tegra: Map configuration space as strongly ordered PCI: tegra: Set DesignWare IP version PCI: tegra: Handle error conditions properly drivers/pci/controller/dwc/pcie-tegra194.c | 62 +++--- 1 file changed, 30 insertions(+), 32 deletions(-) -- 2.17.1
[PATCH 1/4] PCI: tegra: Fix ASPM-L1SS advertisement disable code
If the absence of CLKREQ# signal is indicated by the absence of
"supports-clkreq" in the device-tree node, current driver is disabling
the advertisement of ASPM-L1 Sub-States *before* the ASPM-L1 Sub-States
offset is correctly initialized. Since default value of the ASPM-L1SS
offset is zero, this is causing the Vendor-ID wrongly programmed to 0x10d2
instead of Nvidia's 0x10de thereby the quirks applicable for Tegra194 are
not being applied. This patch fixes this issue by refactoring the
code that disables the ASPM-L1SS advertisement.
Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-tegra194.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index aa511ec0d800..b172b1d49713 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -896,6 +896,12 @@ static void tegra_pcie_prepare_host(struct pcie_port *pp)
init_host_aspm(pcie);
+ /* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+ if (!pcie->supports_clkreq) {
+ disable_aspm_l11(pcie);
+ disable_aspm_l12(pcie);
+ }
+
val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
@@ -1400,12 +1406,6 @@ static int tegra_pcie_config_controller(struct
tegra_pcie_dw *pcie,
pcie->pcie_cap_base = dw_pcie_find_capability(>pci,
PCI_CAP_ID_EXP);
- /* Disable ASPM-L1SS advertisement as there is no CLKREQ routing */
- if (!pcie->supports_clkreq) {
- disable_aspm_l11(pcie);
- disable_aspm_l12(pcie);
- }
-
return ret;
fail_phy:
--
2.17.1
[PATCH 2/4] PCI: tegra: Map configuration space as strongly ordered
As specified in the comment for pci_remap_cfgspace() define in arch/arm64/include/asm/io.h file, PCIe configuration space should be mapped as strongly ordered. Hence changing to dev_pci_remap_cfgspace() from devm_ioremap_resource() for mapping DBI space as that is nothing but the root port's own configuration space. Signed-off-by: Vidya Sagar --- drivers/pci/controller/dwc/pcie-tegra194.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b172b1d49713..7a0c64436861 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2108,7 +2108,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) } pcie->dbi_res = dbi_res; - pci->dbi_base = devm_ioremap_resource(dev, dbi_res); + pci->dbi_base = devm_pci_remap_cfgspace(dev, + dbi_res->start, + resource_size(dbi_res)); if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); -- 2.17.1
[PATCH 2/2] arm64: tegra: Fix DT binding for IO High Voltage entry
Fix the device-tree entry that represents I/O High Voltage property
by replacing 'nvidia,io-high-voltage' with 'nvidia,io-hv' as the former
entry is deprecated.
Fixes: dbb72e2c305b ("arm64: tegra: Add configuration for PCIe C5 sideband
signals")
Signed-off-by: Vidya Sagar
---
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 48160f48003a..5007a2a8647c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -155,7 +155,7 @@
nvidia,schmitt = ;
nvidia,lpdr = ;
nvidia,enable-input =
;
- nvidia,io-high-voltage =
;
+ nvidia,io-hv = ;
nvidia,tristate = ;
nvidia,pull = ;
};
@@ -167,7 +167,7 @@
nvidia,schmitt = ;
nvidia,lpdr = ;
nvidia,enable-input =
;
- nvidia,io-high-voltage =
;
+ nvidia,io-hv = ;
nvidia,tristate = ;
nvidia,pull = ;
};
--
2.17.1
[PATCH 1/2] dt-bindings: Fix entry name for I/O High Voltage property
Correct the name of the I/O High Voltage Property from
'nvidia,io-high-voltage' to 'nvidia,io-hv'.
Fixes: 2585a584f844 ("pinctrl: Add Tegra194 pinctrl DT bindings")
Signed-off-by: Vidya Sagar
---
.../devicetree/bindings/pinctrl/nvidia,tegra194-pinmux.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra194-pinmux.txt
b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra194-pinmux.txt
index 8763f448c376..90d38f710635 100644
--- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra194-pinmux.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra194-pinmux.txt
@@ -99,7 +99,7 @@ Example:
nvidia,schmitt = ;
nvidia,lpdr = ;
nvidia,enable-input =
;
- nvidia,io-high-voltage =
;
+ nvidia,io-hv = ;
nvidia,tristate = ;
nvidia,pull = ;
};
--
2.17.1
[PATCH 0/2] Add support to configure DWC for ECRC
This series has two patches. Patch-1: Adds a public API to query if the system has ECRC policty turned on. Patch-2: DesignWare core PCIe IP has a TLP Digest (TD) override bit in one of its control registers of ATU. This bit needs to be programmed for proper ECRC functionality. This is currently identified as an issue with DesignWare IP version 4.90a. DWC code queries the PCIe sub-system through the API added in Patch-1 to find out if ECRC is turned on or not and configures ATU accordingly. Vidya Sagar (2): PCI/AER: Add pcie_is_ecrc_enabled() API PCI: dwc: Add support to configure for ECRC drivers/pci/controller/dwc/pcie-designware.c | 8 ++-- drivers/pci/controller/dwc/pcie-designware.h | 2 ++ drivers/pci/pci.h| 2 ++ drivers/pci/pcie/aer.c | 11 +++ 4 files changed, 21 insertions(+), 2 deletions(-) -- 2.17.1
[PATCH 1/2] PCI/AER: Add pcie_is_ecrc_enabled() API
Adds pcie_is_ecrc_enabled() API to let other sub-systems (like DesignWare)
to query if ECRC policy is enabled and perform any configuration
required in those respective sub-systems.
Signed-off-by: Vidya Sagar
---
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/aer.c | 11 +++
2 files changed, 13 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..325fdbf91dde 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -575,9 +575,11 @@ static inline void pcie_aspm_powersave_config_link(struct
pci_dev *pdev) { }
#ifdef CONFIG_PCIE_ECRC
void pcie_set_ecrc_checking(struct pci_dev *dev);
void pcie_ecrc_get_policy(char *str);
+bool pcie_is_ecrc_enabled(void);
#else
static inline void pcie_set_ecrc_checking(struct pci_dev *dev) { }
static inline void pcie_ecrc_get_policy(char *str) { }
+static inline bool pcie_is_ecrc_enabled(void) { return false; }
#endif
#ifdef CONFIG_PCIE_PTM
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..24363c895aba 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -207,6 +207,17 @@ void pcie_ecrc_get_policy(char *str)
ecrc_policy = i;
}
+
+/**
+ * pcie_is_ecrc_enabled - returns if ECRC is enabled in the system or not
+ *
+ * Returns 1 if ECRC policy is enabled and 0 otherwise
+ */
+bool pcie_is_ecrc_enabled(void)
+{
+ return ecrc_policy == ECRC_POLICY_ON;
+}
+EXPORT_SYMBOL(pcie_is_ecrc_enabled);
#endif /* CONFIG_PCIE_ECRC */
#definePCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE |
PCI_EXP_DEVCTL_NFERE | \
--
2.17.1
[PATCH 2/2] PCI: dwc: Add support to configure for ECRC
DesignWare core has a TLP digest (TD) override bit in one of the control
registers of ATU. This bit also needs to be programmed for proper ECRC
functionality. This is currently identified as an issue with DesignWare
IP version 4.90a. This patch does the required programming in ATU upon
querying the system policy for ECRC.
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-designware.c | 8 ++--
drivers/pci/controller/dwc/pcie-designware.h | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index b5e438b70cd5..810dcbdbe869 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -245,7 +245,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie
*pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- val = type | PCIE_ATU_FUNC_NUM(func_no);
+ val = type | PCIE_ATU_FUNC_NUM(func_no) | pci->td << PCIE_ATU_TD_SHIFT;
val = upper_32_bits(size - 1) ?
val | PCIE_ATU_INCREASE_REGION_SIZE : val;
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
@@ -295,7 +295,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, type |
- PCIE_ATU_FUNC_NUM(func_no));
+ PCIE_ATU_FUNC_NUM(func_no) |
+ pci->td << PCIE_ATU_TD_SHIFT);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
/*
@@ -565,6 +566,9 @@ void dw_pcie_setup(struct dw_pcie *pci)
dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
"enabled" : "disabled");
+ if (pci->version == 0x490A)
+ pci->td = pcie_is_ecrc_enabled();
+
if (pci->link_gen > 0)
dw_pcie_link_set_max_speed(pci, pci->link_gen);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 21dd06831b50..d34723e42e79 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -90,6 +90,7 @@
#define PCIE_ATU_TYPE_IO 0x2
#define PCIE_ATU_TYPE_CFG0 0x4
#define PCIE_ATU_TYPE_CFG1 0x5
+#define PCIE_ATU_TD_SHIFT 8
#define PCIE_ATU_FUNC_NUM(pf) ((pf) << 20)
#define PCIE_ATU_CR2 0x908
#define PCIE_ATU_ENABLEBIT(31)
@@ -276,6 +277,7 @@ struct dw_pcie {
int num_lanes;
int link_gen;
u8 n_fts[2];
+ booltd; /* TLP Digest (for ECRC purpose) */
};
#define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
--
2.17.1
[PATCH] PCI/ASPM: Save/restore ASPM-L1SS controls for suspend/resume
Previously ASPM L1-Sub-States control registers (CTL1 and CTL2) weren't
saved and restored during suspend/resume leading to ASPM-L1SS
configuration being lost post resume.
Save the ASPM-L1SS control registers so that the configuration is retained
post resume.
Signed-off-by: Vidya Sagar
---
v1:
* It would be really good if someone can verify it on a non tegra194 platform
drivers/pci/pci.c | 7 +++
drivers/pci/pci.h | 4
drivers/pci/pcie/aspm.c | 41 +
3 files changed, 52 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a458c46d7e39..034497264bde 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1551,6 +1551,7 @@ int pci_save_state(struct pci_dev *dev)
return i;
pci_save_ltr_state(dev);
+ pci_save_aspm_l1ss_state(dev);
pci_save_dpc_state(dev);
pci_save_aer_state(dev);
return pci_save_vc_state(dev);
@@ -1656,6 +1657,7 @@ void pci_restore_state(struct pci_dev *dev)
* LTR itself (in the PCIe capability).
*/
pci_restore_ltr_state(dev);
+ pci_restore_aspm_l1ss_state(dev);
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
@@ -3319,6 +3321,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
+ 2 * sizeof(u32));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for
ASPM-L1SS\n");
+
pci_allocate_vc_save_buffers(dev);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..8d2135f61e36 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -565,11 +565,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
void pcie_aspm_exit_link_state(struct pci_dev *pdev);
void pcie_aspm_pm_state_change(struct pci_dev *pdev);
void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pci_save_aspm_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
#else
static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
+static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
+static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
#endif
#ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..d965bbc563ed 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -742,6 +742,47 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state
*link, u32 state)
PCI_L1SS_CTL1_L1SS_MASK, val);
}
+void pci_save_aspm_l1ss_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int aspm_l1ss;
+ u32 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!aspm_l1ss)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state)
+ return;
+
+ cap = (u32 *)_state->cap.data[0];
+ pci_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, cap++);
+ pci_read_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, cap++);
+}
+
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int aspm_l1ss;
+ u32 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+ aspm_l1ss = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_L1SS);
+ if (!save_state || !aspm_l1ss)
+ return;
+
+ cap = (u32 *)_state->cap.data[0];
+ pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL1, *cap++);
+ pci_write_config_dword(dev, aspm_l1ss + PCI_L1SS_CTL2, *cap++);
+}
+
static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
{
pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
--
2.17.1
[PATCH 1/3] PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit
As per PCIe spec r5.0, sec 7.5.1.3.8 only 32-bit BAR registers are defined for non-prefetchable memory and hence a warning should be reported when the size of them go beyond 32-bits. Signed-off-by: Vidya Sagar --- drivers/pci/of.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index ac24cd5439a9..5ea472ae22ac 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -556,6 +556,11 @@ static int pci_parse_request_of_pci_ranges(struct device *dev, break; case IORESOURCE_MEM: res_valid |= !(res->flags & IORESOURCE_PREFETCH); + + if (!(res->flags & IORESOURCE_PREFETCH)) + if (upper_32_bits(resource_size(res))) + dev_warn(dev, "Memory resource size exceeds max for 32 bits\n"); + break; } } -- 2.17.1
[PATCH 3/3] PCI: dwc: Add support to handle prefetchable memory mapping
DWC sub-system currently doesn't differentiate between prefetchable and
non-prefetchable memory aperture entries in the 'ranges' property and
provides ATU mapping only for the first memory aperture entry out of all
the entries present. This was introduced by the
commit 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources").
Mapping for a memory apreture is required if its CPU address and the bus
address are different and the current mechanism works only if the memory
aperture which needs mapping happens to be the first entry. It doesn't
work either if the memory aperture that needs mapping is not the first
entry or if both prefetchable and non-prefetchable apertures need mapping.
This patch fixes this issue by differentiating between prefetchable and
non-prefetchable apertures in the 'ranges' property there by removing the
dependency on the order in which they are specified and adds support for
mapping prefetchable aperture using ATU region-3 if required.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Link:
http://patchwork.ozlabs.org/project/linux-pci/patch/20200513190855.23318-1-vid...@nvidia.com/
Signed-off-by: Vidya Sagar
---
Changes from previous versions:
* Addressed Rob's comments and as part of that split the patch into three
sub-patches
* Rewrote commit subject and description
* Addressed review comments from Lorenzo
.../pci/controller/dwc/pcie-designware-host.c | 39 ---
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 674f32db85ca..a1f319ccd816 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -529,9 +529,39 @@ static struct pci_ops dw_pcie_ops = {
.write = pci_generic_config_write,
};
+static void dw_pcie_setup_mem_atu(struct pcie_port *pp,
+ struct resource_entry *win)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ /* Check for prefetchable memory aperture */
+ if (win->res->flags & IORESOURCE_PREFETCH) {
+ /* Number of view ports must at least be 4 to enable mapping */
+ if (pci->num_viewport < 4) {
+ dev_warn(pci->dev,
+"Insufficient ATU regions to map Prefetchable
memory\n");
+ } else {
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX3,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+ } else { /* Non-prefetchable memory aperture */
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX0,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+}
+
void dw_pcie_setup_rc(struct pcie_port *pp)
{
u32 val, ctrl, num_ctrls;
+ struct resource_entry *win;
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
/*
@@ -586,13 +616,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
* ATU, so we should not program the ATU here.
*/
if (pp->bridge->child_ops == _child_pcie_ops) {
- struct resource_entry *entry =
- resource_list_first_type(>bridge->windows,
IORESOURCE_MEM);
+ resource_list_for_each_entry(win, >bridge->windows)
+ if (resource_type(win->res) == IORESOURCE_MEM)
+ dw_pcie_setup_mem_atu(pp, win);
- dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_MEM, entry->res->start,
- entry->res->start - entry->offset,
- resource_size(entry->res));
if (pci->num_viewport > 2)
dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
PCIE_ATU_TYPE_IO, pp->io_base,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index e7f441441db2..21dd06831b50 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designwa
[PATCH 0/3] Add support to handle prefetchable memory
This patch series adds support for configuring the DesignWare IP's ATU region for prefetchable memory translations. It first starts by flagging a warning if the size of non-prefetchable aperture goes beyond 32-bit as PCIe spec doesn't allow it. And then adds required support for programming the ATU to handle higher (i.e. >4GB) sizes and then finally adds support for differentiating between prefetchable and non-prefetchable regions and configuring one of the ATU regions for prefetchable memory translations purpose. Vidya Sagar (3): PCI: of: Warn if non-prefetchable memory aperture size is > 32-bit PCI: dwc: Add support to program ATU for >4GB memory aperture sizes PCI: dwc: Add support to handle prefetchable memory mapping .../pci/controller/dwc/pcie-designware-host.c | 39 --- drivers/pci/controller/dwc/pcie-designware.c | 12 +++--- drivers/pci/controller/dwc/pcie-designware.h | 4 +- drivers/pci/of.c | 5 +++ 4 files changed, 48 insertions(+), 12 deletions(-) -- 2.17.1
[PATCH 2/3] PCI: dwc: Add support to program ATU for >4GB memory aperture sizes
Add support to program the ATU to enable translations for >4GB sizes of
the prefetchable memory apertures.
Signed-off-by: Vidya Sagar
---
drivers/pci/controller/dwc/pcie-designware.c | 12 +++-
drivers/pci/controller/dwc/pcie-designware.h | 3 ++-
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index c2dea8fc97c8..b5e438b70cd5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -228,7 +228,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
-u32 size)
+u64 size)
{
u32 retries, val;
u64 limit_addr = cpu_addr + size - 1;
@@ -245,8 +245,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
-type | PCIE_ATU_FUNC_NUM(func_no));
+ val = type | PCIE_ATU_FUNC_NUM(func_no);
+ val = upper_32_bits(size - 1) ?
+ val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
PCIE_ATU_ENABLE);
@@ -267,7 +269,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie
*pci, u8 func_no,
static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
int index, int type, u64 cpu_addr,
- u64 pci_addr, u32 size)
+ u64 pci_addr, u64 size)
{
u32 retries, val;
@@ -311,7 +313,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie
*pci, u8 func_no,
}
void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
- u64 cpu_addr, u64 pci_addr, u32 size)
+ u64 cpu_addr, u64 pci_addr, u64 size)
{
__dw_pcie_prog_outbound_atu(pci, 0, index, type,
cpu_addr, pci_addr, size);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 9d2f511f13fa..e7f441441db2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -84,6 +84,7 @@
#define PCIE_ATU_REGION_INDEX1 0x1
#define PCIE_ATU_REGION_INDEX0 0x0
#define PCIE_ATU_CR1 0x904
+#define PCIE_ATU_INCREASE_REGION_SIZE BIT(13)
#define PCIE_ATU_TYPE_MEM 0x0
#define PCIE_ATU_TYPE_IO 0x2
#define PCIE_ATU_TYPE_CFG0 0x4
@@ -295,7 +296,7 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci);
int dw_pcie_wait_for_link(struct dw_pcie *pci);
void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
int type, u64 cpu_addr, u64 pci_addr,
- u32 size);
+ u64 size);
void dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
int type, u64 cpu_addr, u64 pci_addr,
u32 size);
--
2.17.1
Re: [PATCH V2] PCI: dwc: Add support to handle prefetchable memory mapping
On 10/23/2020 9:07 PM, Rob Herring wrote:
External email: Use caution opening links or attachments
On Fri, Oct 23, 2020 at 2:38 AM Vidya Sagar wrote:
On 10/23/2020 12:38 AM, Rob Herring wrote:
External email: Use caution opening links or attachments
On Tue, Oct 20, 2020 at 2:59 PM Vidya Sagar wrote:
DWC sub-system currently doesn't differentiate between prefetchable and
non-prefetchable memory aperture entries in the 'ranges' property and
provides ATU mapping only for the first memory aperture entry out of all
the entries present. This was introduced by the
commit 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources").
Mapping for a memory apreture is required if its CPU address and the bus
address are different and the current mechanism works only if the memory
aperture which needs mapping happens to be the first entry. It doesn't
work either if the memory aperture that needs mapping is not the first
entry or if both prefetchable and non-prefetchable apertures need mapping.
Well that's subtle...
This patch fixes this issue by differentiating between prefetchable and
non-prefetchable apertures in the 'ranges' property there by removing the
dependency on the order in which they are specified and adds support for
mapping prefetchable aperture using ATU region-3 if required.
Now you don't do any iATU entry for a 1:1 memory range which is a
change for pretty much every other platform. That means we leave the
PCI transaction config to the whims of how h/w designers hooked up the
sideband signals. I guess this is how Uniphier works as it only has 1
viewport...
I think the assignment should be in this order:
- config space
- non-prefetchable (IIRC, it's required)
- prefetchable
- i/o
Stopping assignment and warning if you run out of viewports. Looking
at the platforms, I think that would always work. There's only
uniphier and ls1012a where we run out. Those would still behave the
same.
As I see from the code this is how the current mapping is done
Region-0: [Fixed] Non-Prefetchable memory mapping
Region-1: [Shared] if the num of view ports <= 2
Used for I/O by default but whenever config space is accessed,
it is programmed to generate config space, and once done, will program
it back for I/O generation. I'm not sure how they are synchonized when
two different entities are trying to access the config space as well as
the I/O at the same time. I don't see any locking mechanism (or am I
missing something here??)
They aren't synchronized. We just get lucky that I/O and config
accesses aren't interleaved frequently. IMO, we should just not
support I/O space on those platforms. AIUI, almost everything doesn't
use I/O nowadays.
[Fixed] if the num of view ports > 2
Used to generate configuration space accesses
Region-2: [Fixed] I/O accesses
Region-3: [Fixed] Prefetchable memory mapping (This patch is adding it)
I honestly think that an attempt to re-assing what region is used for
what purpose should go into a different patch.
Normally, I'd agree for a fix. If you want to fix it in a minimal way
such that we just setup the last memory region instead of the first,
then that's fine. But to implement the review feedback, you will
simply be adding support for multiple memory regions. The ATU doesn't
care about prefetchable or not. I think it will end up being a more
simple implementation if you just do the I/O region last and only if
you have an available.
I'll surely take it up next but would really like to hear from Jingoo
and Gustavo if this breaks any legacy implementations. For now, I'll
just program the ATU irrespective of 1:1 mapping as I want to get ToT
working again on Tegra194.
I agree that I'm removing configuring an iATU region if it is for 1:1
memory mapping. What I heard from our HW designers is that it is the
default behavior of the Synopsys IP that if the CPU access falls in the
aperture owned by Synopsys IP and there is no iATU region mapped to
capture and generate any specific transaction for that address, then, by
default it generates a memory transaction over the PCIe bus.
It is also possible that some implementors might have chosen to alter
this behavior.
If we assume they haven't, that pretty much guarantees they did. :)
Ok. I'll address it in the next version
In any case, I can continue to have the iATU programming
done irrespective of whether it is for 1:1 mapping or not.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Link:
http://patchwork.ozlabs.org/project/linux-pci/patch/20200513190855.23318-1-vid...@nvidia.com/
Signed-off-by: Vidya Sagar
---
V2:
* Rewrote commit subject and description
* Addressed review comments from Lorenzo
.../pci/controller/dwc/pcie-designware-host.c | 42 ---
drivers/pci/controller/dwc/pcie-designware.c | 12 +++---
drivers/pci/controller/dwc/pcie-designware.h | 4 +-
3 files changed, 46 insertions
Re: [PATCH V2] PCI: dwc: Add support to handle prefetchable memory mapping
On 10/23/2020 12:38 AM, Rob Herring wrote:
External email: Use caution opening links or attachments
On Tue, Oct 20, 2020 at 2:59 PM Vidya Sagar wrote:
DWC sub-system currently doesn't differentiate between prefetchable and
non-prefetchable memory aperture entries in the 'ranges' property and
provides ATU mapping only for the first memory aperture entry out of all
the entries present. This was introduced by the
commit 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources").
Mapping for a memory apreture is required if its CPU address and the bus
address are different and the current mechanism works only if the memory
aperture which needs mapping happens to be the first entry. It doesn't
work either if the memory aperture that needs mapping is not the first
entry or if both prefetchable and non-prefetchable apertures need mapping.
Well that's subtle...
This patch fixes this issue by differentiating between prefetchable and
non-prefetchable apertures in the 'ranges' property there by removing the
dependency on the order in which they are specified and adds support for
mapping prefetchable aperture using ATU region-3 if required.
Now you don't do any iATU entry for a 1:1 memory range which is a
change for pretty much every other platform. That means we leave the
PCI transaction config to the whims of how h/w designers hooked up the
sideband signals. I guess this is how Uniphier works as it only has 1
viewport...
I think the assignment should be in this order:
- config space
- non-prefetchable (IIRC, it's required)
- prefetchable
- i/o
Stopping assignment and warning if you run out of viewports. Looking
at the platforms, I think that would always work. There's only
uniphier and ls1012a where we run out. Those would still behave the
same.
As I see from the code this is how the current mapping is done
Region-0: [Fixed] Non-Prefetchable memory mapping
Region-1: [Shared] if the num of view ports <= 2
Used for I/O by default but whenever config space is accessed,
it is programmed to generate config space, and once done, will program
it back for I/O generation. I'm not sure how they are synchonized when
two different entities are trying to access the config space as well as
the I/O at the same time. I don't see any locking mechanism (or am I
missing something here??)
[Fixed] if the num of view ports > 2
Used to generate configuration space accesses
Region-2: [Fixed] I/O accesses
Region-3: [Fixed] Prefetchable memory mapping (This patch is adding it)
I honestly think that an attempt to re-assing what region is used for
what purpose should go into a different patch.
I agree that I'm removing configuring an iATU region if it is for 1:1
memory mapping. What I heard from our HW designers is that it is the
default behavior of the Synopsys IP that if the CPU access falls in the
aperture owned by Synopsys IP and there is no iATU region mapped to
capture and generate any specific transaction for that address, then, by
default it generates a memory transaction over the PCIe bus.
It is also possible that some implementors might have chosen to alter
this behavior. In any case, I can continue to have the iATU programming
done irrespective of whether it is for 1:1 mapping or not.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Link:
http://patchwork.ozlabs.org/project/linux-pci/patch/20200513190855.23318-1-vid...@nvidia.com/
Signed-off-by: Vidya Sagar
---
V2:
* Rewrote commit subject and description
* Addressed review comments from Lorenzo
.../pci/controller/dwc/pcie-designware-host.c | 42 ---
drivers/pci/controller/dwc/pcie-designware.c | 12 +++---
drivers/pci/controller/dwc/pcie-designware.h | 4 +-
3 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index db547ee6ff3a..dae6da39bb90 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -521,9 +521,42 @@ static struct pci_ops dw_pcie_ops = {
.write = pci_generic_config_write,
};
+static void dw_pcie_setup_mem_atu(struct pcie_port *pp,
+ struct resource_entry *win)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ /* Check for prefetchable memory aperture */
+ if (win->res->flags & IORESOURCE_PREFETCH && win->offset) {
+ /* Number of view ports must at least be 4 to enable mapping */
+ if (pci->num_viewport < 4) {
+ dev_warn(pci->dev,
+"Insufficient ATU regions to map Prefetchable
memory\n");
+ } else {
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX3,
+
[PATCH V2] PCI: dwc: Add support to handle prefetchable memory mapping
DWC sub-system currently doesn't differentiate between prefetchable and
non-prefetchable memory aperture entries in the 'ranges' property and
provides ATU mapping only for the first memory aperture entry out of all
the entries present. This was introduced by the
commit 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources").
Mapping for a memory apreture is required if its CPU address and the bus
address are different and the current mechanism works only if the memory
aperture which needs mapping happens to be the first entry. It doesn't
work either if the memory aperture that needs mapping is not the first
entry or if both prefetchable and non-prefetchable apertures need mapping.
This patch fixes this issue by differentiating between prefetchable and
non-prefetchable apertures in the 'ranges' property there by removing the
dependency on the order in which they are specified and adds support for
mapping prefetchable aperture using ATU region-3 if required.
Fixes: 0f71c60ffd26 ("PCI: dwc: Remove storing of PCI resources")
Link:
http://patchwork.ozlabs.org/project/linux-pci/patch/20200513190855.23318-1-vid...@nvidia.com/
Signed-off-by: Vidya Sagar
---
V2:
* Rewrote commit subject and description
* Addressed review comments from Lorenzo
.../pci/controller/dwc/pcie-designware-host.c | 42 ---
drivers/pci/controller/dwc/pcie-designware.c | 12 +++---
drivers/pci/controller/dwc/pcie-designware.h | 4 +-
3 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index db547ee6ff3a..dae6da39bb90 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -521,9 +521,42 @@ static struct pci_ops dw_pcie_ops = {
.write = pci_generic_config_write,
};
+static void dw_pcie_setup_mem_atu(struct pcie_port *pp,
+ struct resource_entry *win)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ /* Check for prefetchable memory aperture */
+ if (win->res->flags & IORESOURCE_PREFETCH && win->offset) {
+ /* Number of view ports must at least be 4 to enable mapping */
+ if (pci->num_viewport < 4) {
+ dev_warn(pci->dev,
+"Insufficient ATU regions to map Prefetchable
memory\n");
+ } else {
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX3,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+ } else if (win->offset) { /* Non-prefetchable memory aperture */
+ if (upper_32_bits(resource_size(win->res)))
+ dev_warn(pci->dev,
+"Memory resource size exceeds max for 32
bits\n");
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX0,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+}
+
void dw_pcie_setup_rc(struct pcie_port *pp)
{
u32 val, ctrl, num_ctrls;
+ struct resource_entry *win;
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
/*
@@ -578,13 +611,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
* ATU, so we should not program the ATU here.
*/
if (pp->bridge->child_ops == _child_pcie_ops) {
- struct resource_entry *entry =
- resource_list_first_type(>bridge->windows,
IORESOURCE_MEM);
+ resource_list_for_each_entry(win, >bridge->windows)
+ if (resource_type(win->res) == IORESOURCE_MEM)
+ dw_pcie_setup_mem_atu(pp, win);
- dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_MEM, entry->res->start,
- entry->res->start - entry->offset,
- resource_size(entry->res));
if (pci->num_viewport > 2)
dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
PCIE_ATU_TYPE_IO, pp->io_base,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pc
Re: [PATCH] PCI: dwc: Use ATU regions to map memory regions
On 10/20/2020 6:50 PM, Lorenzo Pieralisi wrote:
External email: Use caution opening links or attachments
On Mon, Oct 19, 2020 at 11:21:54AM +0530, Vidya Sagar wrote:
Hi Lorenzo, Rob, Gustavo,
Could you please review this change?
Next cycle - we are in the middle of the merge window and I am not
queueing any more patches.
Thanks for the update.
FWIW, PCIe is broken on Tegra194 with Rob's patches (which got accepted
already) and without the current patch.
Thanks,
Vidya Sagar
Thanks,
Lorenzo
Thanks,
Vidya Sagar
On 10/5/2020 5:43 PM, Vidya Sagar wrote:
Use ATU region-3 and region-0 to setup mapping for prefetchable and
non-prefetchable memory regions respectively only if their respective CPU
and bus addresses are different.
Signed-off-by: Vidya Sagar
---
.../pci/controller/dwc/pcie-designware-host.c | 44 ---
drivers/pci/controller/dwc/pcie-designware.c | 12 ++---
drivers/pci/controller/dwc/pcie-designware.h | 4 +-
3 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 317ff512f8df..cefde8e813e9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -515,9 +515,40 @@ static struct pci_ops dw_pcie_ops = {
.write = pci_generic_config_write,
};
+static void dw_pcie_setup_mem_atu(struct pcie_port *pp,
+ struct resource_entry *win)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ if (win->res->flags & IORESOURCE_PREFETCH && pci->num_viewport >= 4 &&
+ win->offset) {
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX3,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ } else if (win->res->flags & IORESOURCE_PREFETCH &&
+ pci->num_viewport < 4) {
+ dev_warn(pci->dev,
+"Insufficient ATU regions to map Prefetchable memory\n");
+ } else if (win->offset) {
+ if (upper_32_bits(resource_size(win->res)))
+ dev_warn(pci->dev,
+"Memory resource size exceeds max for 32 bits\n");
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX0,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+}
+
void dw_pcie_setup_rc(struct pcie_port *pp)
{
u32 val, ctrl, num_ctrls;
+ struct resource_entry *win;
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
/*
@@ -572,13 +603,14 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
* ATU, so we should not program the ATU here.
*/
if (pp->bridge->child_ops == _child_pcie_ops) {
- struct resource_entry *entry =
- resource_list_first_type(>bridge->windows,
IORESOURCE_MEM);
+ resource_list_for_each_entry(win, >bridge->windows) {
+ switch (resource_type(win->res)) {
+ case IORESOURCE_MEM:
+ dw_pcie_setup_mem_atu(pp, win);
+ break;
+ }
+ }
- dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_MEM, entry->res->start,
- entry->res->start - entry->offset,
- resource_size(entry->res));
if (pci->num_viewport > 2)
dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
PCIE_ATU_TYPE_IO, pp->io_base,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index 3c1f17c78241..6033689abb15 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -227,7 +227,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
-u32 size)
+u64 size)
{
u32 retries, val;
u64 limit_addr = cpu_addr + size - 1;
@@ -244,8 +244,10 @@ static void dw_pcie_prog_ou
Re: [PATCH] PCI: dwc: Use ATU regions to map memory regions
Hi Lorenzo, Rob, Gustavo,
Could you please review this change?
Thanks,
Vidya Sagar
On 10/5/2020 5:43 PM, Vidya Sagar wrote:
Use ATU region-3 and region-0 to setup mapping for prefetchable and
non-prefetchable memory regions respectively only if their respective CPU
and bus addresses are different.
Signed-off-by: Vidya Sagar
---
.../pci/controller/dwc/pcie-designware-host.c | 44 ---
drivers/pci/controller/dwc/pcie-designware.c | 12 ++---
drivers/pci/controller/dwc/pcie-designware.h | 4 +-
3 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 317ff512f8df..cefde8e813e9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -515,9 +515,40 @@ static struct pci_ops dw_pcie_ops = {
.write = pci_generic_config_write,
};
+static void dw_pcie_setup_mem_atu(struct pcie_port *pp,
+ struct resource_entry *win)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ if (win->res->flags & IORESOURCE_PREFETCH && pci->num_viewport >= 4 &&
+ win->offset) {
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX3,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ } else if (win->res->flags & IORESOURCE_PREFETCH &&
+ pci->num_viewport < 4) {
+ dev_warn(pci->dev,
+"Insufficient ATU regions to map Prefetchable
memory\n");
+ } else if (win->offset) {
+ if (upper_32_bits(resource_size(win->res)))
+ dev_warn(pci->dev,
+"Memory resource size exceeds max for 32
bits\n");
+ dw_pcie_prog_outbound_atu(pci,
+ PCIE_ATU_REGION_INDEX0,
+ PCIE_ATU_TYPE_MEM,
+ win->res->start,
+ win->res->start - win->offset,
+ resource_size(win->res));
+ }
+}
+
void dw_pcie_setup_rc(struct pcie_port *pp)
{
u32 val, ctrl, num_ctrls;
+ struct resource_entry *win;
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
/*
@@ -572,13 +603,14 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
* ATU, so we should not program the ATU here.
*/
if (pp->bridge->child_ops == _child_pcie_ops) {
- struct resource_entry *entry =
- resource_list_first_type(>bridge->windows,
IORESOURCE_MEM);
+ resource_list_for_each_entry(win, >bridge->windows) {
+ switch (resource_type(win->res)) {
+ case IORESOURCE_MEM:
+ dw_pcie_setup_mem_atu(pp, win);
+ break;
+ }
+ }
- dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
- PCIE_ATU_TYPE_MEM, entry->res->start,
- entry->res->start - entry->offset,
- resource_size(entry->res));
if (pci->num_viewport > 2)
dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX2,
PCIE_ATU_TYPE_IO, pp->io_base,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index 3c1f17c78241..6033689abb15 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -227,7 +227,7 @@ static void dw_pcie_writel_ob_unroll(struct dw_pcie *pci,
u32 index, u32 reg,
static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
int index, int type,
u64 cpu_addr, u64 pci_addr,
-u32 size)
+u64 size)
{
u32 retries, val;
u64 limit_addr = cpu_addr + size - 1;
@@ -244,8 +244,10 @@ static void dw_pcie_prog_outbound_atu_unroll(struct
dw_pcie *pci, u8 func_no,
lower_32_bits(pci_addr));
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
- dw_p
Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume
On 10/12/2020 5:07 PM, Robin Murphy wrote:
External email: Use caution opening links or attachments
On 2020-10-09 08:55, Jisheng Zhang wrote:
Currently, dw_pcie_msi_init() allocates and maps page for msi, then
program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
may lose power during suspend-to-RAM, so when we resume, we want to
redo the latter but not the former. If designware based driver (for
example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
msi page will be leaked.
As pointed out by Rob and Ard, there's no need to allocate a page for
the MSI address, we could use an address in the driver data.
To avoid map the MSI msg again during resume, we move the map MSI msg
from dw_pcie_msi_init() to dw_pcie_host_init().
You should move the unmap there as well. As soon as you know what the
relevant address would be if you *were* to do DMA to this location, then
the exercise is complete. Leaving it mapped for the lifetime of the
device in order to do not-DMA to it seems questionable (and represents
technically incorrect API usage without at least a sync_for_cpu call
before any other access to the data).
Another point of note is that using streaming DMA mappings at all is a
bit fragile (regardless of this change). If the host controller itself
has a limited DMA mask relative to physical memory (which integrators
still seem to keep doing...) then you could end up punching your MSI
hole right in the middle of the SWIOTLB bounce buffer, where it's then
almost *guaranteed* to interfere with real DMA :(
Agree with Robin. Since the MSI page is going to be locked till
shutdown/reboot, wouldn't it make sense to use dma_alloc_coherent() API?
Also, shouldn't we call dma_set_mask() to limit the address to only
32-bits so as to enable MSI for even those legacy PCIe devices with only
32-bit MSI capability?
- Vidya Sagar
If no DWC users have that problem and the current code is working well
enough, then I see little reason not to make this partucular change to
tidy up the implementation, just bear in mind that there's always the
possibility of having to come back and change it yet again in future to
make it more robust. I had it in mind that this trick was done with a
coherent DMA allocation, which would be safe from addressing problems
but would need to be kept around for the lifetime of the device, but
maybe that was a different driver :/
Robin.
Suggested-by: Rob Herring
Signed-off-by: Jisheng Zhang
Reviewed-by: Rob Herring
---
drivers/pci/controller/dwc/pci-dra7xx.c | 18 +-
.../pci/controller/dwc/pcie-designware-host.c | 33 ++-
drivers/pci/controller/dwc/pcie-designware.h | 2 +-
3 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c
b/drivers/pci/controller/dwc/pci-dra7xx.c
index 8f0b6d644e4b..6d012d2b1e90 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -466,7 +466,9 @@ static struct irq_chip
dra7xx_pci_msi_bottom_irq_chip = {
static int dra7xx_pcie_msi_host_init(struct pcie_port *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct device *dev = pci->dev;
u32 ctrl, num_ctrls;
+ int ret;
pp->msi_irq_chip = _pci_msi_bottom_irq_chip;
@@ -482,7 +484,21 @@ static int dra7xx_pcie_msi_host_init(struct
pcie_port *pp)
~0);
}
- return dw_pcie_allocate_domains(pp);
+ ret = dw_pcie_allocate_domains(pp);
+ if (ret)
+ return ret;
+
+ pp->msi_data = dma_map_single_attrs(dev, >msi_msg,
+ sizeof(pp->msi_msg),
+ DMA_FROM_DEVICE,
+ DMA_ATTR_SKIP_CPU_SYNC);
+ ret = dma_mapping_error(dev, pp->msi_data);
+ if (ret) {
+ dev_err(dev, "Failed to map MSI data\n");
+ pp->msi_data = 0;
+ dw_pcie_free_msi(pp);
+ }
+ return ret;
}
static const struct dw_pcie_host_ops dra7xx_pcie_host_ops = {
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index d3e9ea11ce9e..d02c7e74738d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -266,30 +266,23 @@ void dw_pcie_free_msi(struct pcie_port *pp)
irq_domain_remove(pp->msi_domain);
irq_domain_remove(pp->irq_domain);
- if (pp->msi_page)
- __free_page(pp->msi_page);
+ if (pp->msi_data) {
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct device *dev = pci->dev;
+
+ dma_unmap_single_attrs(dev, pp->msi_data,
sizeof(pp->msi_msg),
+ DMA_FROM_DEVICE,
DMA_ATTR_SKIP_CPU_SYNC);
+ }
}
void dw_pcie_msi_init(struct pcie_port *pp
Re: [PATCH v2 5/5] PCI: dwc: Move dw_pcie_msi_init() from each users to designware host
On 9/24/2020 4:37 PM, Jisheng Zhang wrote:
External email: Use caution opening links or attachments
Currently, dw_pcie_msi_init() allocates and maps page for msi, then
program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
may lose power during suspend-to-RAM, so when we resume, we want to
redo the latter but not the former. If designware based driver (for
example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
previous msi page will be leaked. From another side, except
pci-dra7xx.c we can move the dw_pcie_msi_init() from each users to
designware host, I.E move the msi page allocation and mapping to
dw_pcie_host_init() and move the PCIE_MSI_ADDR_* programming to
dw_pcie_setup_rc(). After this moving, we solve the msi page leakage
as well.
Signed-off-by: Jisheng Zhang
---
drivers/pci/controller/dwc/pci-dra7xx.c | 1 +
drivers/pci/controller/dwc/pci-exynos.c | 2 --
drivers/pci/controller/dwc/pci-imx6.c | 3 ---
drivers/pci/controller/dwc/pci-meson.c| 8 ---
drivers/pci/controller/dwc/pcie-artpec6.c | 10
.../pci/controller/dwc/pcie-designware-host.c | 24 ---
.../pci/controller/dwc/pcie-designware-plat.c | 3 ---
drivers/pci/controller/dwc/pcie-designware.h | 5
drivers/pci/controller/dwc/pcie-histb.c | 3 ---
drivers/pci/controller/dwc/pcie-kirin.c | 3 ---
drivers/pci/controller/dwc/pcie-qcom.c| 3 ---
drivers/pci/controller/dwc/pcie-spear13xx.c | 1 -
drivers/pci/controller/dwc/pcie-tegra194.c| 2 --
drivers/pci/controller/dwc/pcie-uniphier.c| 9 +--
14 files changed, 22 insertions(+), 55 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c
b/drivers/pci/controller/dwc/pci-dra7xx.c
index dc387724cf08..d8b74389e353 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -210,6 +210,7 @@ static int dra7xx_pcie_host_init(struct pcie_port *pp)
dra7xx_pcie_establish_link(pci);
dw_pcie_wait_for_link(pci);
dw_pcie_msi_init(pp);
+ dw_pcie_msi_config(pp);
dra7xx_pcie_enable_interrupts(dra7xx);
return 0;
diff --git a/drivers/pci/controller/dwc/pci-exynos.c
b/drivers/pci/controller/dwc/pci-exynos.c
index 8d82c43ae299..9cca0ce79777 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -298,8 +298,6 @@ static void exynos_pcie_msi_init(struct exynos_pcie *ep)
struct pcie_port *pp = >pp;
u32 val;
- dw_pcie_msi_init(pp);
-
/* enable MSI interrupt */
val = exynos_pcie_readl(ep->mem_res->elbi_base, PCIE_IRQ_EN_LEVEL);
val |= IRQ_MSI_ENABLE;
diff --git a/drivers/pci/controller/dwc/pci-imx6.c
b/drivers/pci/controller/dwc/pci-imx6.c
index 5fef2613b223..dba6e351e3df 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -848,9 +848,6 @@ static int imx6_pcie_host_init(struct pcie_port *pp)
dw_pcie_setup_rc(pp);
imx6_pcie_establish_link(imx6_pcie);
- if (IS_ENABLED(CONFIG_PCI_MSI))
- dw_pcie_msi_init(pp);
-
return 0;
}
diff --git a/drivers/pci/controller/dwc/pci-meson.c
b/drivers/pci/controller/dwc/pci-meson.c
index 4f183b96afbb..cd0d9dd8dd61 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -377,12 +377,6 @@ static int meson_pcie_establish_link(struct meson_pcie *mp)
return dw_pcie_wait_for_link(pci);
}
-static void meson_pcie_enable_interrupts(struct meson_pcie *mp)
-{
- if (IS_ENABLED(CONFIG_PCI_MSI))
- dw_pcie_msi_init(>pci.pp);
-}
-
static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
u32 *val)
{
@@ -467,8 +461,6 @@ static int meson_pcie_host_init(struct pcie_port *pp)
if (ret)
return ret;
- meson_pcie_enable_interrupts(mp);
-
return 0;
}
diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c
b/drivers/pci/controller/dwc/pcie-artpec6.c
index 97d50bb50f06..af1e6bb28e7a 100644
--- a/drivers/pci/controller/dwc/pcie-artpec6.c
+++ b/drivers/pci/controller/dwc/pcie-artpec6.c
@@ -346,15 +346,6 @@ static void artpec6_pcie_deassert_core_reset(struct
artpec6_pcie *artpec6_pcie)
usleep_range(100, 200);
}
-static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
-{
- struct dw_pcie *pci = artpec6_pcie->pci;
- struct pcie_port *pp = >pp;
-
- if (IS_ENABLED(CONFIG_PCI_MSI))
- dw_pcie_msi_init(pp);
-}
-
static int artpec6_pcie_host_init(struct pcie_port *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -368,7 +359,6 @@ static int artpec6_pcie_host_init(struct pcie_port *pp)
dw_pcie_setup_rc(pp);
artpec6_pcie_establish_link(pci);
dw_pcie_wait_for_link(pci);
-
