Patchwork powerpc/85xx: Fix SMP when "cpu-release-addr" is in lowmem

login
register
mail settings
Submitter Peter Tyser
Date Dec. 18, 2009, 10:50 p.m.
Message ID <1261176637-23912-1-git-send-email-ptyser@xes-inc.com>
Download mbox | patch
Permalink /patch/41436/
State Accepted
Commit d1d47ec6e62ab08d2ebb925fd9203abfad3adfbf
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Peter Tyser - Dec. 18, 2009, 10:50 p.m.
Recent U-Boot commit 5ccd29c3679b3669b0bde5c501c1aa0f325a7acb caused
the "cpu-release-addr" device tree property to contain the physical RAM
location that secondary cores were spinning at.  Previously, the
"cpu-release-addr" property contained a value referencing the boot page
translation address range of 0xfffffxxx, which then indirectly accessed
RAM.

The "cpu-release-addr" is currently ioremapped and the secondary cores
kicked.  However, due to the recent change in "cpu-release-addr", it
sometimes points to a memory location in low memory that cannot be
ioremapped.  For example on a P2020-based board with 512MB of RAM the
following error occurs on bootup:

  <...>
  mpic: requesting IPIs ...
  __ioremap(): phys addr 0x1ffff000 is RAM lr c05df9a0
  Unable to handle kernel paging request for data at address 0x00000014
  Faulting instruction address: 0xc05df9b0
  Oops: Kernel access of bad area, sig: 11 [#1]
  SMP NR_CPUS=2 P2020 RDB
  Modules linked in:
  <... eventual kernel panic>

Adding logic to conditionally ioremap or access memory directly resolves
the issue.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
Signed-off-by: Nate Case <ncase@xes-inc.com>
Reported-by: Dipen Dudhat <B09055@freescale.com>
Tested-by: Dipen Dudhat <B09055@freescale.com>
---
 arch/powerpc/platforms/85xx/smp.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)
Hunter Cobbs - Dec. 19, 2009, 2:35 a.m.
Hello list.  I've run into a rather odd problem.  I seem to have a
problem with full-speed isochronous transfers across a USB2.0 Hub.  I
believe this was observed before with the general EHCI drivers in Linux.

In the latest branch of the kernel, the EHCI driver has some "Enhanced
Transaction Translation" scheduling.  This would be great for me to use
as it seems to directly address the issues I've seen.  However, I'm not
really sure on how to proceed because the DWC_OTG driver is not GPL code
while the EHCI code I'd like to use is.  Therefore, I don't believe that
I can port the Enhanced Transaction Translation routine into the DWC_OTG
driver without violating both the GPL and Synopsis's own driver.

Does anyone have a suggestion on how to proceed?
Wolfgang Denk - Dec. 19, 2009, 6:56 p.m.
Dear Hunter Cobbs,

In message <1261190115.14590.5.camel@mobiLinux> you wrote:
> Hello list.  I've run into a rather odd problem.  I seem to have a
> problem with full-speed isochronous transfers across a USB2.0 Hub.  I
> believe this was observed before with the general EHCI drivers in Linux.

Which exazct kernel / driver cversions are you talking about?

> In the latest branch of the kernel, the EHCI driver has some "Enhanced
> Transaction Translation" scheduling.  This would be great for me to use
> as it seems to directly address the issues I've seen.  However, I'm not
> really sure on how to proceed because the DWC_OTG driver is not GPL code

Oops? if that wa the case you could not use (and distribute) this code
in a Linux context at all.

> while the EHCI code I'd like to use is.  Therefore, I don't believe that
> I can port the Enhanced Transaction Translation routine into the DWC_OTG
> driver without violating both the GPL and Synopsis's own driver.

What makes you think so?

The Synopsis dwc_otg driver license does not seem to be  in  conflict
to  the  GPL,  if  you  ask  me (but then, I am not a layer). When we
worked on this, we were assured that the use of this driver source in
the context of GPLed software like the Linux kernel (or  U-Boot)  was
not considered an issue by Synopsis.

> Does anyone have a suggestion on how to proceed?

We have been doing some work on the DWC drivers in the past, fixing a
number of issues. [We never  attempted  to  psuh  any  of  this  into
mainline  as  the  Synopsis  drivers  are  -  in  our  opinion  - not
avcceptable for mainline but require more  or  less  a  rewrite  from
scratch. But you can find all this stuff in our linux-2.6-denx repo.]

I recommend you give a try to our latest stable branch.

Best regards,

Wolfgang Denk
Hunter Cobbs - Dec. 20, 2009, 8:13 p.m.
Well, thank you for the assurance on the Synopsis drivers not being in
conflict.  That makes me feel a little better.

We're using Linux 2.6.30(.3)-denx.  We've worked on this issue a bit
before, although at the time you were doing some work for AMCC on just
accessing devices across a hub in general.  We've been using the driver
for a few months now and it is very stable on first-use of the driver,
but once we close a full-speed port and try to reopen, that's when we
get error concerning not being able to allocate resources and getting
-28 errors.  I don't have anything precise in front of me at the moment,
my test equipment is at the office.  But this seems to be the issues
that lead to the Enhanced Transaction Translation Scheduling changes in
the main-line kernel.

I'm mainly being, I believe, overly cautious about not including GPL
code in the Synopsis driver simply because I'd rather be overly careful
than be exposed to some liability later down the road if Synopsis
decides they want to make their driver completely proprietary.  While
the code is available, I'm not sure if we can include code from GPL
sources in the Synopsis drivers without requiring the Synopsis driver to
become completely GPL.  I am also not a lawyer, but I'd rather be
cautious.

I did do a comparison on the previous branch we are (2.6.30.3-denx) and
the current stable.  The only differences in the code were minor enough
that I simply merged them into our existing codebase.  It didn't really
help our issue, but on block transfers and standard connect/disconnect
the driver seemed more responsive.  I believe this was mainly from 
Setphan's actions to put some sleeps inside of tight loops that would
chew up major CPU time on a non-preemptable kernel(which we had earlier).

On Sat, 2009-12-19 at 19:56 +0100, Wolfgang Denk wrote:
> Dear Hunter Cobbs,
> 
> In message <1261190115.14590.5.camel@mobiLinux> you wrote:
> > Hello list.  I've run into a rather odd problem.  I seem to have a
> > problem with full-speed isochronous transfers across a USB2.0 Hub.  I
> > believe this was observed before with the general EHCI drivers in Linux.
> 
> Which exazct kernel / driver cversions are you talking about?
> 
> > In the latest branch of the kernel, the EHCI driver has some "Enhanced
> > Transaction Translation" scheduling.  This would be great for me to use
> > as it seems to directly address the issues I've seen.  However, I'm not
> > really sure on how to proceed because the DWC_OTG driver is not GPL code
> 
> Oops? if that wa the case you could not use (and distribute) this code
> in a Linux context at all.
> 
> > while the EHCI code I'd like to use is.  Therefore, I don't believe that
> > I can port the Enhanced Transaction Translation routine into the DWC_OTG
> > driver without violating both the GPL and Synopsis's own driver.
> 
> What makes you think so?
> 
> The Synopsis dwc_otg driver license does not seem to be  in  conflict
> to  the  GPL,  if  you  ask  me (but then, I am not a layer). When we
> worked on this, we were assured that the use of this driver source in
> the context of GPLed software like the Linux kernel (or  U-Boot)  was
> not considered an issue by Synopsis.
> 
> > Does anyone have a suggestion on how to proceed?
> 
> We have been doing some work on the DWC drivers in the past, fixing a
> number of issues. [We never  attempted  to  psuh  any  of  this  into
> mainline  as  the  Synopsis  drivers  are  -  in  our  opinion  - not
> avcceptable for mainline but require more  or  less  a  rewrite  from
> scratch. But you can find all this stuff in our linux-2.6-denx repo.]
> 
> I recommend you give a try to our latest stable branch.
> 
> Best regards,
> 
> Wolfgang Denk
>
Wolfgang Denk - Dec. 20, 2009, 9:58 p.m.
Dear Hunter Cobbs,

In message <1261340025.3473.15.camel@mobiLinux> you wrote:
> 
> We're using Linux 2.6.30(.3)-denx.  We've worked on this issue a bit
> before, although at the time you were doing some work for AMCC on just
> accessing devices across a hub in general.  We've been using the driver
> for a few months now and it is very stable on first-use of the driver,
> but once we close a full-speed port and try to reopen, that's when we
> get error concerning not being able to allocate resources and getting
> -28 errors.  I don't have anything precise in front of me at the moment,
> my test equipment is at the office.  But this seems to be the issues
> that lead to the Enhanced Transaction Translation Scheduling changes in
> the main-line kernel.

If you have a detailed bug report, ideally including an easy to
reproduce test case to trigger the problem then please feel free to
send it to our support address.

> I'm mainly being, I believe, overly cautious about not including GPL
> code in the Synopsis driver simply because I'd rather be overly careful
> than be exposed to some liability later down the road if Synopsis
> decides they want to make their driver completely proprietary.  While
> the code is available, I'm not sure if we can include code from GPL
> sources in the Synopsis drivers without requiring the Synopsis driver to
> become completely GPL.  I am also not a lawyer, but I'd rather be
> cautious.

Well, their license is relatively simple: the driver is unsupported,
it "IS NOT an item of Licensed Software or Licensed Product under any
End User Software License Agreement or Agreement for Licensed
Product", and you "are permitted to use andredistribute this Software
in source and binary forms, with or without modification, provided
that redistributions of source code must retain this notice." And
there is no warranty, of course.

IANAL, but I see no risks here.


Best regards,

Wolfgang Denk
Peter Tyser - Jan. 21, 2010, 10:28 p.m.
On Fri, 2009-12-18 at 16:50 -0600, Peter Tyser wrote:
> Recent U-Boot commit 5ccd29c3679b3669b0bde5c501c1aa0f325a7acb caused
> the "cpu-release-addr" device tree property to contain the physical RAM
> location that secondary cores were spinning at.  Previously, the
> "cpu-release-addr" property contained a value referencing the boot page
> translation address range of 0xfffffxxx, which then indirectly accessed
> RAM.
> 
> The "cpu-release-addr" is currently ioremapped and the secondary cores
> kicked.  However, due to the recent change in "cpu-release-addr", it
> sometimes points to a memory location in low memory that cannot be
> ioremapped.  For example on a P2020-based board with 512MB of RAM the
> following error occurs on bootup:
> 
>   <...>
>   mpic: requesting IPIs ...
>   __ioremap(): phys addr 0x1ffff000 is RAM lr c05df9a0
>   Unable to handle kernel paging request for data at address 0x00000014
>   Faulting instruction address: 0xc05df9b0
>   Oops: Kernel access of bad area, sig: 11 [#1]
>   SMP NR_CPUS=2 P2020 RDB
>   Modules linked in:
>   <... eventual kernel panic>
> 
> Adding logic to conditionally ioremap or access memory directly resolves
> the issue.
> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> Signed-off-by: Nate Case <ncase@xes-inc.com>
> Reported-by: Dipen Dudhat <B09055@freescale.com>
> Tested-by: Dipen Dudhat <B09055@freescale.com>

Any chance this going to be picked up for 2.6.33?  The issue is
currently going to bite anyone using an MP-capable 85xx system that
doesn't use highmem.

Thanks,
Peter
Kumar Gala - Jan. 25, 2010, 4:50 p.m.
On Jan 21, 2010, at 4:28 PM, Peter Tyser wrote:

> On Fri, 2009-12-18 at 16:50 -0600, Peter Tyser wrote:
>> Recent U-Boot commit 5ccd29c3679b3669b0bde5c501c1aa0f325a7acb caused
>> the "cpu-release-addr" device tree property to contain the physical RAM
>> location that secondary cores were spinning at.  Previously, the
>> "cpu-release-addr" property contained a value referencing the boot page
>> translation address range of 0xfffffxxx, which then indirectly accessed
>> RAM.
>> 
>> The "cpu-release-addr" is currently ioremapped and the secondary cores
>> kicked.  However, due to the recent change in "cpu-release-addr", it
>> sometimes points to a memory location in low memory that cannot be
>> ioremapped.  For example on a P2020-based board with 512MB of RAM the
>> following error occurs on bootup:
>> 
>>  <...>
>>  mpic: requesting IPIs ...
>>  __ioremap(): phys addr 0x1ffff000 is RAM lr c05df9a0
>>  Unable to handle kernel paging request for data at address 0x00000014
>>  Faulting instruction address: 0xc05df9b0
>>  Oops: Kernel access of bad area, sig: 11 [#1]
>>  SMP NR_CPUS=2 P2020 RDB
>>  Modules linked in:
>>  <... eventual kernel panic>
>> 
>> Adding logic to conditionally ioremap or access memory directly resolves
>> the issue.
>> 
>> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
>> Signed-off-by: Nate Case <ncase@xes-inc.com>
>> Reported-by: Dipen Dudhat <B09055@freescale.com>
>> Tested-by: Dipen Dudhat <B09055@freescale.com>
> 
> Any chance this going to be picked up for 2.6.33?  The issue is
> currently going to bite anyone using an MP-capable 85xx system that
> doesn't use highmem.

This just got lost in my queue.  Will apply and send up for .33

- k
Kumar Gala - Jan. 25, 2010, 4:55 p.m.
On Dec 18, 2009, at 4:50 PM, Peter Tyser wrote:

> Recent U-Boot commit 5ccd29c3679b3669b0bde5c501c1aa0f325a7acb caused
> the "cpu-release-addr" device tree property to contain the physical RAM
> location that secondary cores were spinning at.  Previously, the
> "cpu-release-addr" property contained a value referencing the boot page
> translation address range of 0xfffffxxx, which then indirectly accessed
> RAM.
> 
> The "cpu-release-addr" is currently ioremapped and the secondary cores
> kicked.  However, due to the recent change in "cpu-release-addr", it
> sometimes points to a memory location in low memory that cannot be
> ioremapped.  For example on a P2020-based board with 512MB of RAM the
> following error occurs on bootup:
> 
>  <...>
>  mpic: requesting IPIs ...
>  __ioremap(): phys addr 0x1ffff000 is RAM lr c05df9a0
>  Unable to handle kernel paging request for data at address 0x00000014
>  Faulting instruction address: 0xc05df9b0
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  SMP NR_CPUS=2 P2020 RDB
>  Modules linked in:
>  <... eventual kernel panic>
> 
> Adding logic to conditionally ioremap or access memory directly resolves
> the issue.
> 
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> Signed-off-by: Nate Case <ncase@xes-inc.com>
> Reported-by: Dipen Dudhat <B09055@freescale.com>
> Tested-by: Dipen Dudhat <B09055@freescale.com>
> ---
> arch/powerpc/platforms/85xx/smp.c |   21 +++++++++++++++++++--
> 1 files changed, 19 insertions(+), 2 deletions(-)

applied to merge for 2.6.33

- k

Patch

diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index 04160a4..a15f582 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -46,6 +46,7 @@  smp_85xx_kick_cpu(int nr)
 	__iomem u32 *bptr_vaddr;
 	struct device_node *np;
 	int n = 0;
+	int ioremappable;
 
 	WARN_ON (nr < 0 || nr >= NR_CPUS);
 
@@ -59,21 +60,37 @@  smp_85xx_kick_cpu(int nr)
 		return;
 	}
 
+	/*
+	 * A secondary core could be in a spinloop in the bootpage
+	 * (0xfffff000), somewhere in highmem, or somewhere in lowmem.
+	 * The bootpage and highmem can be accessed via ioremap(), but
+	 * we need to directly access the spinloop if its in lowmem.
+	 */
+	ioremappable = *cpu_rel_addr > virt_to_phys(high_memory);
+
 	/* Map the spin table */
-	bptr_vaddr = ioremap(*cpu_rel_addr, SIZE_BOOT_ENTRY);
+	if (ioremappable)
+		bptr_vaddr = ioremap(*cpu_rel_addr, SIZE_BOOT_ENTRY);
+	else
+		bptr_vaddr = phys_to_virt(*cpu_rel_addr);
 
 	local_irq_save(flags);
 
 	out_be32(bptr_vaddr + BOOT_ENTRY_PIR, nr);
 	out_be32(bptr_vaddr + BOOT_ENTRY_ADDR_LOWER, __pa(__early_start));
 
+	if (!ioremappable)
+		flush_dcache_range((ulong)bptr_vaddr,
+				(ulong)(bptr_vaddr + SIZE_BOOT_ENTRY));
+
 	/* Wait a bit for the CPU to ack. */
 	while ((__secondary_hold_acknowledge != nr) && (++n < 1000))
 		mdelay(1);
 
 	local_irq_restore(flags);
 
-	iounmap(bptr_vaddr);
+	if (ioremappable)
+		iounmap(bptr_vaddr);
 
 	pr_debug("waited %d msecs for CPU #%d.\n", n, nr);
 }