This patch series first implements a function called pcie_get_speed_cap_mask in the PCI subsystem based off from drm_pcie_get_speed_cap_mask in drm. Then it removes the latter and fixes all references to it. And ultimately, it implements an architecture-specific version of the same function for ppc64.
The refactor is done because the function that was used by drm is more architecture goo than module-specific. Whilst the function also needed a platform-specific implementation to get PCIE Gen2 speeds on ppc64.
Lucas Kannebley Tavares (3): pci: added pcie_get_speed_cap_mask function drm: removed drm_pcie_get_speed_cap_mask function ppc64: implemented pcibios_get_speed_cap_mask
arch/powerpc/platforms/pseries/pci.c | 35 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_pci.c | 38 ----------------------------- drivers/gpu/drm/radeon/evergreen.c | 5 ++- drivers/gpu/drm/radeon/r600.c | 5 ++- drivers/gpu/drm/radeon/rv770.c | 5 ++- drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 6 ---- include/linux/pci.h | 6 ++++ 8 files changed, 94 insertions(+), 50 deletions(-)
Added function to gather the speed cap for a device and return a mask to supported speeds. The function is divided into an interface and a weak implementation so that architecture-specific functions can be called.
This is the first step in moving function drm_pcie_get_speed_cap_mask from the drm subsystem to the pci one.
Signed-off-by: Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com --- drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 6 ++++++ 2 files changed, 50 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b099e00..d94ab79 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3931,6 +3931,50 @@ static int __init pci_setup(char *str) } early_param("pci", pci_setup);
+int __weak pcibios_get_speed_cap_mask(struct pci_dev *dev, u32 *mask) +{ + struct pci_dev *root; + u32 lnkcap, lnkcap2; + + *mask = 0; + if (!dev) + return -EINVAL; + + root = dev->bus->self; + + /* we've been informed via and serverworks don't make the cut */ + if (root->vendor == PCI_VENDOR_ID_VIA || + root->vendor == PCI_VENDOR_ID_SERVERWORKS) + return -EINVAL; + + pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap); + pcie_capability_read_dword(root, PCI_EXP_LNKCAP2, &lnkcap2); + + if (lnkcap2) { /* PCIe r3.0-compliant */ + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB) + *mask |= PCIE_SPEED_25; + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB) + *mask |= PCIE_SPEED_50; + if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB) + *mask |= PCIE_SPEED_80; + } else { /* pre-r3.0 */ + if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB) + *mask |= PCIE_SPEED_25; + if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB) + *mask |= (PCIE_SPEED_25 | PCIE_SPEED_50); + } + + dev_info(&dev->dev, "probing gen 2 caps for device %x:%x = %x/%x\n", + root->vendor, root->device, lnkcap, lnkcap2); + return 0; +} + +int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *mask) +{ + return pcibios_get_speed_cap_mask(dev, mask); +} +EXPORT_SYMBOL(pcie_get_speed_cap_mask); + EXPORT_SYMBOL(pci_reenable_device); EXPORT_SYMBOL(pci_enable_device_io); EXPORT_SYMBOL(pci_enable_device_mem); diff --git a/include/linux/pci.h b/include/linux/pci.h index 2461033a..24a2f63 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1861,4 +1861,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev) */ struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+#define PCIE_SPEED_25 1 +#define PCIE_SPEED_50 2 +#define PCIE_SPEED_80 4 + +extern int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *speed_mask); + #endif /* LINUX_PCI_H */
On Tue, Mar 19, 2013 at 11:24 PM, Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com wrote:
Added function to gather the speed cap for a device and return a mask to supported speeds. The function is divided into an interface and a weak implementation so that architecture-specific functions can be called.
This is the first step in moving function drm_pcie_get_speed_cap_mask from the drm subsystem to the pci one.
This still doesn't feel right to me.
I'm definitely not a hardware guy, but my understanding based on the PCIe spec r3.0, sec 6.11, is that the hardware will automatically maintain the link at the highest speed supported by both ends of the link, unless software sets a lower Target Link Speed. The only users of this function are some Radeon drivers, and it looks like they only use it because the hardware doesn't conform to this aspect of the spec and requires device-specific tweaking.
We already have bus->max_bus_speed, which should tell you the max speed supported by the upstream component. Is that enough information? Maybe the radeon drivers could simply do something like this:
speed = rdev->ddev->pdev->bus->max_bus_speed; if (speed == PCI_SPEED_UNKNOWN || speed < PCIE_SPEED_5_0GT) return;
In your original email [1], you mentioned a null pointer dereference, presumably when reading the PCIe capability for dev->pdev->bus->self, with "self" being NULL. This only happens for a bus with no upstream P2P bridge, i.e., self will be NULL only if pdev is on a root bus. But we also know pdev is a PCIe device, and I think a PCIe device on a root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec 1.3.2.3). Such a device does not have a link at all, so there's no point in fiddling with its link speed.
I don't see how a radeon device could be an integrated endpoint, but in your hypervisor environment, maybe it isn't quite spec-compliant in terms of its attachment. Can you collect the output of "lspci -vv"? Maybe that will make things clearer.
In any case, if a radeon device is on a root bus, I think the root bus max_bus_speed will be PCI_SPEED_UNKNOWN, and if it's not on a root bus, max_bus_speed should be set correctly based on the upstream PCIe port.
Bjorn
[1] http://lkml.kernel.org/r/50819140.8030806@linux.vnet.ibm.com
Signed-off-by: Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com
drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 6 ++++++ 2 files changed, 50 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b099e00..d94ab79 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3931,6 +3931,50 @@ static int __init pci_setup(char *str) } early_param("pci", pci_setup);
+int __weak pcibios_get_speed_cap_mask(struct pci_dev *dev, u32 *mask) +{
struct pci_dev *root;u32 lnkcap, lnkcap2;*mask = 0;if (!dev)return -EINVAL;root = dev->bus->self;/* we've been informed via and serverworks don't make the cut */if (root->vendor == PCI_VENDOR_ID_VIA ||root->vendor == PCI_VENDOR_ID_SERVERWORKS)return -EINVAL;pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap);pcie_capability_read_dword(root, PCI_EXP_LNKCAP2, &lnkcap2);if (lnkcap2) { /* PCIe r3.0-compliant */if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)*mask |= PCIE_SPEED_25;if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)*mask |= PCIE_SPEED_50;if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)*mask |= PCIE_SPEED_80;} else { /* pre-r3.0 */if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)*mask |= PCIE_SPEED_25;if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)*mask |= (PCIE_SPEED_25 | PCIE_SPEED_50);}dev_info(&dev->dev, "probing gen 2 caps for device %x:%x = %x/%x\n",root->vendor, root->device, lnkcap, lnkcap2);return 0;+}
+int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *mask) +{
return pcibios_get_speed_cap_mask(dev, mask);+} +EXPORT_SYMBOL(pcie_get_speed_cap_mask);
EXPORT_SYMBOL(pci_reenable_device); EXPORT_SYMBOL(pci_enable_device_io); EXPORT_SYMBOL(pci_enable_device_mem); diff --git a/include/linux/pci.h b/include/linux/pci.h index 2461033a..24a2f63 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1861,4 +1861,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev) */ struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+#define PCIE_SPEED_25 1 +#define PCIE_SPEED_50 2 +#define PCIE_SPEED_80 4
+extern int pcie_get_speed_cap_mask(struct pci_dev *dev, u32 *speed_mask);
#endif /* LINUX_PCI_H */
1.7.4.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-26 at 12:39 -0600, Bjorn Helgaas wrote:
But we also know pdev is a PCIe device, and I think a PCIe device on a root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec 1.3.2.3). Such a device does not have a link at all, so there's no point in fiddling with its link speed.
This is where our IBM hypervisor makes things murky. It doesn't expose the PCIe parents (basically somewhat makes PCIe look like PCI except we still have the PCIe caps on the child devices, just no access to the parent device).
It's garbage but can't be fixed (would break AIX :-)
However we might be able to populate the bus->max_bus_speed from some architecture specific quirk and have radeon use that.
Cheers, Ben.
On Wed, Mar 27, 2013 at 9:25 AM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
On Tue, 2013-03-26 at 12:39 -0600, Bjorn Helgaas wrote:
But we also know pdev is a PCIe device, and I think a PCIe device on a root bus must be a "Root Complex Integrated Endpoint" (PCIe spec sec 1.3.2.3). Such a device does not have a link at all, so there's no point in fiddling with its link speed.
This is where our IBM hypervisor makes things murky. It doesn't expose the PCIe parents (basically somewhat makes PCIe look like PCI except we still have the PCIe caps on the child devices, just no access to the parent device).
Interesting. I wonder if we'll trip over this anywhere else, e.g., ASPM or other link-related things. I guess we'll just have to see if anything else breaks.
It's garbage but can't be fixed (would break AIX :-)
However we might be able to populate the bus->max_bus_speed from some architecture specific quirk and have radeon use that.
That sounds like a good solution to me. It seems like it's really an arch-specific deviation from the spec, so it'd be nice to have an arch-specific solution for it.
Bjorn
This function was moved to the pci subsystem where it fits better, as this is much more of a generic pci task, than a drm specific one. All references to the function (all in the radeon driver) are updated.
This is the second step in moving function drm_pcie_get_speed_cap_mask from the drm subsystem to the pci one.
Signed-off-by: Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com --- drivers/gpu/drm/drm_pci.c | 38 ------------------------------------ drivers/gpu/drm/radeon/evergreen.c | 5 ++- drivers/gpu/drm/radeon/r600.c | 5 ++- drivers/gpu/drm/radeon/rv770.c | 5 ++- include/drm/drmP.h | 6 ----- 5 files changed, 9 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index bd719e9..ba70844 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -439,44 +439,6 @@ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) return 0; }
-int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *mask) -{ - struct pci_dev *root; - u32 lnkcap, lnkcap2; - - *mask = 0; - if (!dev->pdev) - return -EINVAL; - - root = dev->pdev->bus->self; - - /* we've been informed via and serverworks don't make the cut */ - if (root->vendor == PCI_VENDOR_ID_VIA || - root->vendor == PCI_VENDOR_ID_SERVERWORKS) - return -EINVAL; - - pcie_capability_read_dword(root, PCI_EXP_LNKCAP, &lnkcap); - pcie_capability_read_dword(root, PCI_EXP_LNKCAP2, &lnkcap2); - - if (lnkcap2) { /* PCIe r3.0-compliant */ - if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB) - *mask |= DRM_PCIE_SPEED_25; - if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB) - *mask |= DRM_PCIE_SPEED_50; - if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB) - *mask |= DRM_PCIE_SPEED_80; - } else { /* pre-r3.0 */ - if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB) - *mask |= DRM_PCIE_SPEED_25; - if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB) - *mask |= (DRM_PCIE_SPEED_25 | DRM_PCIE_SPEED_50); - } - - DRM_INFO("probing gen 2 caps for device %x:%x = %x/%x\n", root->vendor, root->device, lnkcap, lnkcap2); - return 0; -} -EXPORT_SYMBOL(drm_pcie_get_speed_cap_mask); - #else
int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 305a657..6ba204d 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -24,6 +24,7 @@ #include <linux/firmware.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/pci.h> #include <drm/drmP.h> #include "radeon.h" #include "radeon_asic.h" @@ -3871,11 +3872,11 @@ void evergreen_pcie_gen2_enable(struct radeon_device *rdev) if (ASIC_IS_X2(rdev)) return;
- ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask); + ret = pcie_get_speed_cap_mask(rdev->ddev->pdev, &mask); if (ret != 0) return;
- if (!(mask & DRM_PCIE_SPEED_50)) + if (!(mask & PCIE_SPEED_50)) return;
speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL); diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 0740db3..89a7387 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -30,6 +30,7 @@ #include <linux/firmware.h> #include <linux/platform_device.h> #include <linux/module.h> +#include <linux/pci.h> #include <drm/drmP.h> #include <drm/radeon_drm.h> #include "radeon.h" @@ -4371,11 +4372,11 @@ static void r600_pcie_gen2_enable(struct radeon_device *rdev) if (rdev->family <= CHIP_R600) return;
- ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask); + ret = pcie_get_speed_cap_mask(rdev->ddev->pdev, &mask); if (ret != 0) return;
- if (!(mask & DRM_PCIE_SPEED_50)) + if (!(mask & PCIE_SPEED_50)) return;
speed_cntl = RREG32_PCIE_P(PCIE_LC_SPEED_CNTL); diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c index d63fe1d..81c7f1c 100644 --- a/drivers/gpu/drm/radeon/rv770.c +++ b/drivers/gpu/drm/radeon/rv770.c @@ -28,6 +28,7 @@ #include <linux/firmware.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/pci.h> #include <drm/drmP.h> #include "radeon.h" #include "radeon_asic.h" @@ -1254,11 +1255,11 @@ static void rv770_pcie_gen2_enable(struct radeon_device *rdev) if (ASIC_IS_X2(rdev)) return;
- ret = drm_pcie_get_speed_cap_mask(rdev->ddev, &mask); + ret = pcie_get_speed_cap_mask(rdev->ddev->pdev, &mask); if (ret != 0) return;
- if (!(mask & DRM_PCIE_SPEED_50)) + if (!(mask & PCIE_SPEED_50)) return;
DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n"); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2d94d74..39b2872 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1788,12 +1788,6 @@ extern int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, struct drm_driver *driver);
-#define DRM_PCIE_SPEED_25 1 -#define DRM_PCIE_SPEED_50 2 -#define DRM_PCIE_SPEED_80 4 - -extern int drm_pcie_get_speed_cap_mask(struct drm_device *dev, u32 *speed_mask); - /* platform section */ extern int drm_platform_init(struct drm_driver *driver, struct platform_device *platform_device); extern void drm_platform_exit(struct drm_driver *driver, struct platform_device *platform_device);
Implementation of a architecture-specific pcibios_get_speed_cap_mask. This implementation detects bus capabilities based on OF ibm,pcie-link-speed-stats property.
Signed-off-by: Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com --- arch/powerpc/platforms/pseries/pci.c | 35 ++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c index 0b580f4..4da8deb 100644 --- a/arch/powerpc/platforms/pseries/pci.c +++ b/arch/powerpc/platforms/pseries/pci.c @@ -24,6 +24,7 @@ #include <linux/kernel.h> #include <linux/pci.h> #include <linux/string.h> +#include <linux/device.h>
#include <asm/eeh.h> #include <asm/pci-bridge.h> @@ -108,3 +109,37 @@ static void fixup_winbond_82c105(struct pci_dev* dev) } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105, fixup_winbond_82c105); + +int pcibios_get_speed_cap_mask(struct pci_dev *dev, u32 *mask) +{ + struct device_node *dn, *pdn; + const uint32_t *pcie_link_speed_stats = NULL; + + *mask = 0; + dn = pci_bus_to_OF_node(dev->bus); + + /* Find nearest ibm,pcie-link-speed-stats, walking up the device tree */ + for (pdn = dn; pdn != NULL; pdn = pdn->parent) { + pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn, + "ibm,pcie-link-speed-stats", NULL); + if (pcie_link_speed_stats != NULL) + break; + } + + if (pcie_link_speed_stats == NULL) { + dev_info(&dev->dev, "no ibm,pcie-link-speed-stats property\n"); + return -EINVAL; + } + + switch (pcie_link_speed_stats[0]) { + case 0x02: + *mask |= PCIE_SPEED_50; + case 0x01: + *mask |= PCIE_SPEED_25; + case 0x00: + default: + return -EINVAL; + } + + return 0; +}
On Wed, 2013-03-20 at 02:24 -0300, Lucas Kannebley Tavares wrote:
Implementation of a architecture-specific pcibios_get_speed_cap_mask. This implementation detects bus capabilities based on OF ibm,pcie-link-speed-stats property.
The problem with your approach is that it's not a runtime detection...
If the pseries machine is compiled into the kernel binary, it will override pcibios_get_speed_cap_mask() using the device-tree, regardless of whether the machine is currently booted on a pseries machine or not.
This wouldn't be a big problem if the pseries pcibios_get_speed_cap_mask() was capable of doing a fallback to the generic one if the device-tree property is absent but that isn't the case.
I think what you need to do is:
- Make it so the generic one can be called by the override. This can look a bit tricky but it's better that way. One way to do it is to have the actual implementation be in a __pci_* variant called by the weak pcibios_* variant
- Move the powerpc on to arch/powerpc/kernel/pci-common.c and make it call a ppc_md.pcibios_get_speed_cap_mask(). If the hook is absent (NULL), make it call the generic one
- pseries can then populate the hook in ppc_md. with its custom variant.
Cheers, Ben.
On Wed, Mar 20, 2013 at 1:24 AM, Lucas Kannebley Tavares lucaskt@linux.vnet.ibm.com wrote:
This patch series first implements a function called pcie_get_speed_cap_mask in the PCI subsystem based off from drm_pcie_get_speed_cap_mask in drm. Then it removes the latter and fixes all references to it. And ultimately, it implements an architecture-specific version of the same function for ppc64.
The refactor is done because the function that was used by drm is more architecture goo than module-specific. Whilst the function also needed a platform-specific implementation to get PCIE Gen2 speeds on ppc64.
Looks good to me but we probably want some one from the pci side to take a look
Reviewed-by: Jerome Glisse jglisse@redhat.com
Lucas Kannebley Tavares (3): pci: added pcie_get_speed_cap_mask function drm: removed drm_pcie_get_speed_cap_mask function ppc64: implemented pcibios_get_speed_cap_mask
arch/powerpc/platforms/pseries/pci.c | 35 +++++++++++++++++++++++++++ drivers/gpu/drm/drm_pci.c | 38 ----------------------------- drivers/gpu/drm/radeon/evergreen.c | 5 ++- drivers/gpu/drm/radeon/r600.c | 5 ++- drivers/gpu/drm/radeon/rv770.c | 5 ++- drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 6 ---- include/linux/pci.h | 6 ++++ 8 files changed, 94 insertions(+), 50 deletions(-)
-- 1.7.4.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Benjamin Herrenschmidt -
Bjorn Helgaas -
Jerome Glisse -
Lucas Kannebley Tavares