| Message ID | 1367872744-25002-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 06, 2013 at 02:39:04PM -0600, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra20 HW appears to have a bug such that PCIe device interrupts, whether > they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around > this, simply disable LP2 if the PCI driver and DT node are both enabled. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > Thierry, > > This patch is physically based on next-20130506 so that it doesn't > conflict with any of the recent cpuidle cleanup work. I'm sending it with > the expectation that you'll apply it to your PCIe development branch > though. If you do that, you'll see some conflicts unless you rebase your > dev branch onto something more recent than next-20130422; I assume you'll > do that rebase soon enough anyway, but if you weren't planning to, I can > resend the patch based on your current dev branch. Yes, I'll be rebasing my development branch soon anyway and I'll pull this patch into the branch. Thanks for writing it. Thierry
On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra20 HW appears to have a bug such that PCIe device interrupts, whether > they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around > this, simply disable LP2 if the PCI driver and DT node are both enabled. > Wouldn't it make more sense to disable LP2 when we actually detect a PCIe device? Cheers, Peter.
On Tue, May 07, 2013 at 03:48:49PM +0300, Peter De Schrijver wrote: > On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: > > From: Stephen Warren <swarren@nvidia.com> > > > > Tegra20 HW appears to have a bug such that PCIe device interrupts, whether > > they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around > > this, simply disable LP2 if the PCI driver and DT node are both enabled. > > > > Wouldn't it make more sense to disable LP2 when we actually detect a PCIe > device? I'm not sure a patch to do so would be as simple as this one. For one, the cpuidle framework will already have been initialized when PCIe enumeration completes. So some way of permanently disabling one state at runtime would be required and I don't think cpuidle provides an API to do so. I know the latter isn't really a good reason, but I don't think adding that kind of API just because Tegra20 seems to have a bug would be appropriate. Furthermore, it is quite likely that the PCIe controller will only be enabled in DT for devices that actually have a PCIe device hooked up. Thierry
On 05/07/2013 07:08 AM, Thierry Reding wrote: > On Tue, May 07, 2013 at 03:48:49PM +0300, Peter De Schrijver > wrote: >> On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> Tegra20 HW appears to have a bug such that PCIe device >>> interrupts, whether they are legacy IRQs or MSI, are lost when >>> LP2 is enabled. To work around this, simply disable LP2 if the >>> PCI driver and DT node are both enabled. >>> >> >> Wouldn't it make more sense to disable LP2 when we actually >> detect a PCIe device? I did consider that, but rejected the idea for the reasons Thierry mentioned. > I'm not sure a patch to do so would be as simple as this one. For > one, the cpuidle framework will already have been initialized when > PCIe enumeration completes. So some way of permanently disabling > one state at runtime would be required and I don't think cpuidle > provides an API to do so. I know the latter isn't really a good > reason, but I don't think adding that kind of API just because > Tegra20 seems to have a bug would be appropriate. There is a way to do this, since it can be done via sysfs, but I don't think it's exposed as an API from cpuidle. I agree it seems a little silly to expose it just to support this HW bug though. > Furthermore, it is quite likely that the PCIe controller will only > be enabled in DT for devices that actually have a PCIe device > hooked up. But this was my main reasoning.
On Tue, May 07, 2013 at 04:54:31PM +0200, Stephen Warren wrote: > On 05/07/2013 07:08 AM, Thierry Reding wrote: > > On Tue, May 07, 2013 at 03:48:49PM +0300, Peter De Schrijver > > wrote: > >> On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: > >>> From: Stephen Warren <swarren@nvidia.com> > >>> > >>> Tegra20 HW appears to have a bug such that PCIe device > >>> interrupts, whether they are legacy IRQs or MSI, are lost when > >>> LP2 is enabled. To work around this, simply disable LP2 if the > >>> PCI driver and DT node are both enabled. > >>> > >> > >> Wouldn't it make more sense to disable LP2 when we actually > >> detect a PCIe device? > > I did consider that, but rejected the idea for the reasons Thierry > mentioned. > > > I'm not sure a patch to do so would be as simple as this one. For > > one, the cpuidle framework will already have been initialized when > > PCIe enumeration completes. So some way of permanently disabling > > one state at runtime would be required and I don't think cpuidle > > provides an API to do so. I know the latter isn't really a good > > reason, but I don't think adding that kind of API just because > > Tegra20 seems to have a bug would be appropriate. > > There is a way to do this, since it can be done via sysfs, but I don't > think it's exposed as an API from cpuidle. I agree it seems a little > silly to expose it just to support this HW bug though. > > > Furthermore, it is quite likely that the PCIe controller will only > > be enabled in DT for devices that actually have a PCIe device > > hooked up. > That's not always true though. Eg Harmony has a miniPCIe slot, so it should be listed in DT, but there is not necessarily a card present in the slot. Cheers, Peter.
On 05/06/2013 10:39 PM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra20 HW appears to have a bug such that PCIe device interrupts, whether > they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around > this, simply disable LP2 if the PCI driver and DT node are both enabled. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > Thierry, > > This patch is physically based on next-20130506 so that it doesn't > conflict with any of the recent cpuidle cleanup work. I'm sending it with > the expectation that you'll apply it to your PCIe development branch > though. If you do that, you'll see some conflicts unless you rebase your > dev branch onto something more recent than next-20130422; I assume you'll > do that rebase soon enough anyway, but if you weren't planning to, I can > resend the patch based on your current dev branch. > > arch/arm/mach-tegra/cpuidle-tegra20.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c > index 0cdba8d..d2c9349 100644 > --- a/arch/arm/mach-tegra/cpuidle-tegra20.c > +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c > @@ -25,6 +25,7 @@ > #include <linux/cpu_pm.h> > #include <linux/clockchips.h> > #include <linux/clk/tegra.h> > +#include <linux/of.h> > > #include <asm/cpuidle.h> > #include <asm/proc-fns.h> > @@ -212,10 +213,39 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, > } > #endif > > +static const struct of_device_id pcie_matches[] __initconst = { > + { .compatible = "nvidia,tegra20-pcie" }, > + { } > +}; > + > +/* > + * Tegra20 HW appears to have a bug such that PCIe device interrupts, whether > + * they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around > + * this, simply disable LP2 if the PCI driver and DT node are both enabled. > + */ > +static void __init tegra20_cpuidle_disable_lp2_with_pcie(void) > +{ > + struct device_node *np; > + > + if (!IS_ENABLED(CONFIG_PCI_TEGRA)) > + return; > + > + np = of_find_matching_node(NULL, pcie_matches); > + if (!np) > + return; > + > + if (!of_device_is_available(np)) > + return; > + > + pr_info("Disabling LP2 cpuidle state, since PCIe is enabled\n"); > + tegra_idle_driver.state_count = 1; Won't be more clear to have the state disabled in the init function like this ? int __init tegra20_cpuidle_init(void) { ... if (tegra20_has_lp2_pcie_bug(void)) tegra_idle_driver.states[1].disabled = true; ... } It is similar than: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/tree/arch/sh/kernel/cpu/shmobile/cpuidle.c > +} > + > int __init tegra20_cpuidle_init(void) > { > #ifdef CONFIG_PM_SLEEP > tegra_tear_down_cpu = tegra20_tear_down_cpu; > #endif > + tegra20_cpuidle_disable_lp2_with_pcie(); > return cpuidle_register(&tegra_idle_driver, cpu_possible_mask); > } >
On Wed, May 08, 2013 at 12:40:04PM +0300, Peter De Schrijver wrote: > On Tue, May 07, 2013 at 04:54:31PM +0200, Stephen Warren wrote: > > On 05/07/2013 07:08 AM, Thierry Reding wrote: > > > On Tue, May 07, 2013 at 03:48:49PM +0300, Peter De Schrijver > > > wrote: > > >> On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: > > >>> From: Stephen Warren <swarren@nvidia.com> > > >>> > > >>> Tegra20 HW appears to have a bug such that PCIe device > > >>> interrupts, whether they are legacy IRQs or MSI, are lost when > > >>> LP2 is enabled. To work around this, simply disable LP2 if the > > >>> PCI driver and DT node are both enabled. > > >>> > > >> > > >> Wouldn't it make more sense to disable LP2 when we actually > > >> detect a PCIe device? > > > > I did consider that, but rejected the idea for the reasons Thierry > > mentioned. > > > > > I'm not sure a patch to do so would be as simple as this one. For > > > one, the cpuidle framework will already have been initialized when > > > PCIe enumeration completes. So some way of permanently disabling > > > one state at runtime would be required and I don't think cpuidle > > > provides an API to do so. I know the latter isn't really a good > > > reason, but I don't think adding that kind of API just because > > > Tegra20 seems to have a bug would be appropriate. > > > > There is a way to do this, since it can be done via sysfs, but I don't > > think it's exposed as an API from cpuidle. I agree it seems a little > > silly to expose it just to support this HW bug though. > > > > > Furthermore, it is quite likely that the PCIe controller will only > > > be enabled in DT for devices that actually have a PCIe device > > > hooked up. > > > > That's not always true though. Eg Harmony has a miniPCIe slot, so it should be > listed in DT, but there is not necessarily a card present in the slot. That's true. I suppose one could argue that Harmony is a development board and more typical setups wouldn't have user-accessible slots. Then again it seems like LP2 causes some more fallout like very slow output on the serial console and a decrease in FPS for some 2D/3D use- cases I've been playing around with. While these are only performance issues they still indicate that something is seriously flawed with the current LP2 implementation (at least on Tegra20). So maybe it would be preferable to disable it altogether if it cannot be fixed otherwise. I was able to mitigate the problems a bit on Tamonten by reducing the value of the nvidia,cpu-pwr-good-time and nvidia,cpu-pwr-off-time from 5000 us to 500 and 100 us respectively. That gives much better performance for the serial console and 2D/3D test-cases, but the 2D/3D performance is still down by about 10% compared to runs with LP2 disabled. Now the 2D and 3D test-cases aren't very complex and there's certainly much that could be improved with regard to job submission and such, but neither removes the fundamental issue that LP2 is causing a lot of problems that it shouldn't. Thierry
On 05/08/2013 03:40 AM, Peter De Schrijver wrote: > On Tue, May 07, 2013 at 04:54:31PM +0200, Stephen Warren wrote: >> On 05/07/2013 07:08 AM, Thierry Reding wrote: >>> On Tue, May 07, 2013 at 03:48:49PM +0300, Peter De Schrijver >>> wrote: >>>> On Mon, May 06, 2013 at 10:39:04PM +0200, Stephen Warren wrote: >>>>> From: Stephen Warren <swarren@nvidia.com> >>>>> >>>>> Tegra20 HW appears to have a bug such that PCIe device >>>>> interrupts, whether they are legacy IRQs or MSI, are lost when >>>>> LP2 is enabled. To work around this, simply disable LP2 if the >>>>> PCI driver and DT node are both enabled. >>>>> >>>> >>>> Wouldn't it make more sense to disable LP2 when we actually >>>> detect a PCIe device? >> >> I did consider that, but rejected the idea for the reasons Thierry >> mentioned. ... >>> Furthermore, it is quite likely that the PCIe controller will only >>> be enabled in DT for devices that actually have a PCIe device >>> hooked up. > > That's not always true though. Eg Harmony has a miniPCIe slot, so it should be > listed in DT, but there is not necessarily a card present in the slot. I initially thought that this issue only affected Tegra20, so I was prepared to ignore the lack of optimality of this solution, since it only affects Harmony. However, initial experimentation with Jay's Tegra30 PCIe patches imply the same issue is present on Tegra30. I need to do more research to confirm whether it's the same problem or something else though. If this turns out to be true, we'll have to come up with a more complex solution, since this current solution would disable LP2 outright on all currently supported Tegra30 boards:-(
On 05/08/2013 04:53 AM, Daniel Lezcano wrote: > On 05/06/2013 10:39 PM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Tegra20 HW appears to have a bug such that PCIe device interrupts, whether >> they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around >> this, simply disable LP2 if the PCI driver and DT node are both enabled. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> Thierry, >> >> This patch is physically based on next-20130506 so that it doesn't >> conflict with any of the recent cpuidle cleanup work. I'm sending it with >> the expectation that you'll apply it to your PCIe development branch >> though. If you do that, you'll see some conflicts unless you rebase your >> dev branch onto something more recent than next-20130422; I assume you'll >> do that rebase soon enough anyway, but if you weren't planning to, I can >> resend the patch based on your current dev branch. >> >> arch/arm/mach-tegra/cpuidle-tegra20.c | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c >> index 0cdba8d..d2c9349 100644 >> --- a/arch/arm/mach-tegra/cpuidle-tegra20.c >> +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c >> @@ -25,6 +25,7 @@ >> #include <linux/cpu_pm.h> >> #include <linux/clockchips.h> >> #include <linux/clk/tegra.h> >> +#include <linux/of.h> >> >> #include <asm/cpuidle.h> >> #include <asm/proc-fns.h> >> @@ -212,10 +213,39 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, >> } >> #endif >> >> +static const struct of_device_id pcie_matches[] __initconst = { >> + { .compatible = "nvidia,tegra20-pcie" }, >> + { } >> +}; >> + >> +/* >> + * Tegra20 HW appears to have a bug such that PCIe device interrupts, whether >> + * they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around >> + * this, simply disable LP2 if the PCI driver and DT node are both enabled. >> + */ >> +static void __init tegra20_cpuidle_disable_lp2_with_pcie(void) >> +{ >> + struct device_node *np; >> + >> + if (!IS_ENABLED(CONFIG_PCI_TEGRA)) >> + return; >> + >> + np = of_find_matching_node(NULL, pcie_matches); >> + if (!np) >> + return; >> + >> + if (!of_device_is_available(np)) >> + return; >> + >> + pr_info("Disabling LP2 cpuidle state, since PCIe is enabled\n"); >> + tegra_idle_driver.state_count = 1; > > Won't be more clear to have the state disabled in the init function like > this ? > > int __init tegra20_cpuidle_init(void) > { > ... > if (tegra20_has_lp2_pcie_bug(void)) > tegra_idle_driver.states[1].disabled = true; > ... > } I'm not sure that's any clearer on its own. One big advantage of that technique is that if we end up with 3 cpuidle states and need to disable the one in the middle, this alternative technique does work, whereas the one in my patch doesn't. So, I'll convert to this technique.
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index 0cdba8d..d2c9349 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -25,6 +25,7 @@ #include <linux/cpu_pm.h> #include <linux/clockchips.h> #include <linux/clk/tegra.h> +#include <linux/of.h> #include <asm/cpuidle.h> #include <asm/proc-fns.h> @@ -212,10 +213,39 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, } #endif +static const struct of_device_id pcie_matches[] __initconst = { + { .compatible = "nvidia,tegra20-pcie" }, + { } +}; + +/* + * Tegra20 HW appears to have a bug such that PCIe device interrupts, whether + * they are legacy IRQs or MSI, are lost when LP2 is enabled. To work around + * this, simply disable LP2 if the PCI driver and DT node are both enabled. + */ +static void __init tegra20_cpuidle_disable_lp2_with_pcie(void) +{ + struct device_node *np; + + if (!IS_ENABLED(CONFIG_PCI_TEGRA)) + return; + + np = of_find_matching_node(NULL, pcie_matches); + if (!np) + return; + + if (!of_device_is_available(np)) + return; + + pr_info("Disabling LP2 cpuidle state, since PCIe is enabled\n"); + tegra_idle_driver.state_count = 1; +} + int __init tegra20_cpuidle_init(void) { #ifdef CONFIG_PM_SLEEP tegra_tear_down_cpu = tegra20_tear_down_cpu; #endif + tegra20_cpuidle_disable_lp2_with_pcie(); return cpuidle_register(&tegra_idle_driver, cpu_possible_mask); }