All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
@ 2009-11-12 14:19 Dieter Kiermaier
  2009-11-12 15:42 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Alexander Clouter
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Dieter Kiermaier @ 2009-11-12 14:19 UTC (permalink / raw)
  To: linux-arm-kernel


Hi,

first: thanks to all for the help while debugging!

Below you can find a first version of the patch which enables PCI on my openrd-base board
together with a PCIe->PCI bridge.
Please let me know if something is wrong with it.

This works together with Maxime Bizon's patch to enable pci bridges:
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-November/003784.html

Regards,
Dieter


>From 25bbc7222100dce9c47a4e3bfc31267f8394a9da Mon Sep 17 00:00:00 2001
From: Dieter Kiermaier <dk-arm-linux@gmx.de>
Date: Thu, 12 Nov 2009 15:12:24 +0100
Subject: [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present

Clear bit 2 of CPU configuration register at offset 0x20100
to disable MBUS-L Error propagation.
If this bit isn't disabled kernel crash during bootup when booted
with current mainline u-boot (version U-Boot 2009.08-00208-g9ef0569)
and a PCIe -> PCI bridge is connected to the system.

Signed-off-by: Dieter Kiermaier <dk-arm-linux@gmx.de>
---
 arch/arm/mach-kirkwood/openrd_base-setup.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
index 77617c7..9e57326 100644
--- a/arch/arm/mach-kirkwood/openrd_base-setup.c
+++ b/arch/arm/mach-kirkwood/openrd_base-setup.c
@@ -14,6 +14,7 @@
 #include <linux/mtd/partitions.h>
 #include <linux/ata_platform.h>
 #include <linux/mv643xx_eth.h>
+#include <asm/io.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 #include <mach/kirkwood.h>
@@ -76,9 +77,19 @@ static void __init openrd_base_init(void)
 
 static int __init openrd_base_pci_init(void)
 {
+	u32 cpu_config_reg;
+	void __iomem *base;
+	base = ioremap(0xf1020100, 4);
+	if (base)
+	{
+		cpu_config_reg = readl(base);
+		cpu_config_reg &= ~(1 << 2);
+		writel(cpu_config_reg, base);
+	}
+	iounmap(base);
+
 	if (machine_is_openrd_base())
 		kirkwood_pcie_init();
-
 	return 0;
  }
 subsys_initcall(openrd_base_pci_init);
-- 
1.5.4.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present
  2009-11-12 14:19 [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Dieter Kiermaier
@ 2009-11-12 15:42 ` Alexander Clouter
  2009-11-12 17:02   ` Dieter Kiermaier
  2009-11-12 19:37 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Lennert Buytenhek
  2009-11-12 20:55 ` Russell King - ARM Linux
  2 siblings, 1 reply; 10+ messages in thread
From: Alexander Clouter @ 2009-11-12 15:42 UTC (permalink / raw)
  To: linux-arm-kernel

Dieter Kiermaier <dk-arm-linux@gmx.de> wrote:
>
> [snipped]
> 
> static int __init openrd_base_pci_init(void)
> {
> +       u32 cpu_config_reg;
> +       void __iomem *base;
> +       base = ioremap(0xf1020100, 4);
>
Ewwwwwwww. :)

If you dig through arch/arm/mach-orion5x/include/mach/orion5x.h you 
should be able to work out that 0xf1020100 is probably best replaced 
with something like (ORION5X_BRIDGE_PHYS_BASE | 0x100), once you add
a matching ORION5X_BRIDGE_PHYS_BASE entry alongside the
ORION5X_BRIDGE_VIRT_BASE[1].  Well, *I* prefer that sort of thing. :)

> +       if (base)
> +       {
> +               cpu_config_reg = readl(base);
> +               cpu_config_reg &= ~(1 << 2);
> +               writel(cpu_config_reg, base);
> +       }
> +       iounmap(base);
> +
>        if (machine_is_openrd_base())
>                kirkwood_pcie_init();
> -
>        return 0;
>  }
> subsys_initcall(openrd_base_pci_init);
>
As was recently explained to me[2], that code is going to run on *all* 
kirkword platforms, not just the OpenRD.  I am guessing you want to 
shove your additional code into a seperate int returning __init function 
and call it from the machine_is_openrd_base() clause.

Also, if for some strange reason the ioremap() failed, you are going to 
call iounmap(NULL) so that should probably be moved up a line into the 
'if' clause?  However on this one I *think* I have been told in the past 
it cannot fail so you might be able to remove the 'if' clause 
altogether.

Cheers

[1] unsure if at that point can can just jump straight in and tinker with
	ORION5X_BRIDGE_VIRT_BASE?
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002699.html

-- 
Alexander Clouter
.sigmonster says: Most people's favorite way to end a game is by winning.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present
  2009-11-12 15:42 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Alexander Clouter
@ 2009-11-12 17:02   ` Dieter Kiermaier
  2009-11-12 18:52     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe?bridge?is present Alexander Clouter
  2009-11-13  7:50     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Simon Kagstrom
  0 siblings, 2 replies; 10+ messages in thread
From: Dieter Kiermaier @ 2009-11-12 17:02 UTC (permalink / raw)
  To: linux-arm-kernel

Am Donnerstag 12 November 2009 16:42:49 schrieb Alexander Clouter:
> Dieter Kiermaier <dk-arm-linux@gmx.de> wrote:
> >
> > [snipped]
> > 
> > static int __init openrd_base_pci_init(void)
> > {
> > +       u32 cpu_config_reg;
> > +       void __iomem *base;
> > +       base = ioremap(0xf1020100, 4);
> >
> Ewwwwwwww. :)
> 
> If you dig through arch/arm/mach-orion5x/include/mach/orion5x.h you 
> should be able to work out that 0xf1020100 is probably best replaced 
> with something like (ORION5X_BRIDGE_PHYS_BASE | 0x100), once you add
> a matching ORION5X_BRIDGE_PHYS_BASE entry alongside the
> ORION5X_BRIDGE_VIRT_BASE[1].  Well, *I* prefer that sort of thing. :)
> 
> > +       if (base)
> > +       {
> > +               cpu_config_reg = readl(base);
> > +               cpu_config_reg &= ~(1 << 2);
> > +               writel(cpu_config_reg, base);
> > +       }
> > +       iounmap(base);
> > +
> >        if (machine_is_openrd_base())
> >                kirkwood_pcie_init();
> > -
> >        return 0;
> >  }
> > subsys_initcall(openrd_base_pci_init);
> >
> As was recently explained to me[2], that code is going to run on *all* 
> kirkword platforms, not just the OpenRD.  I am guessing you want to 
> shove your additional code into a seperate int returning __init function 
> and call it from the machine_is_openrd_base() clause.
> 

If we go on thinking about that - I would prefer place the code - without the magick key ;) - 
in kirkwood_pcie_init()?
It will affect later or sooner more kirkwood boards if people switch form marvell stock u-boot
to mainline u-boot.
What do you think about that?

to [2]:
Hm, is this really right? Why is there a function called openrd_base_pci_init() which is inside a file
called openrd_base-setup.c and this function is called on a sheevaplug?

I couldn't believe that (but to be honest I don't know it!).
The 2 board do still have different machnumbers, right?
I've expected that these machnumbers are to determine
which board / hardware the kernel / u-boot is running.
Isn't this the case?

> Also, if for some strange reason the ioremap() failed, you are going to 
> call iounmap(NULL) so that should probably be moved up a line into the 
> 'if' clause?  However on this one I *think* I have been told in the past 
> it cannot fail so you might be able to remove the 'if' clause 
> altogether.
sounds reasonable

> 
> Cheers
> 
> [1] unsure if at that point can can just jump straight in and tinker with
> 	ORION5X_BRIDGE_VIRT_BASE?
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002699.html
> 

Dieter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe?bridge?is present
  2009-11-12 17:02   ` Dieter Kiermaier
@ 2009-11-12 18:52     ` Alexander Clouter
  2009-11-13  7:50     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Simon Kagstrom
  1 sibling, 0 replies; 10+ messages in thread
From: Alexander Clouter @ 2009-11-12 18:52 UTC (permalink / raw)
  To: linux-arm-kernel

Dieter Kiermaier <dk-arm-linux@gmx.de> wrote:
> 
> If we go on thinking about that - I would prefer place the code - 
> without the magick key ;) - in kirkwood_pcie_init()?
>
> It will affect later or sooner more kirkwood boards if people switch 
> form marvell stock u-boot to mainline u-boot.
>
> What do you think about that?
>
No idea, I could try it on both my SheevaPlug and OpenRD-Client, but 
as they are 'in production' and 200 miles away from where I am...I am 
slightly hesitate to update u-boot and test a kernel :)

> to [2]: Hm, is this really right? Why is there a function called 
> openrd_base_pci_init() which is inside a file called 
> openrd_base-setup.c and this function is called on a sheevaplug?
> 
> I couldn't believe that (but to be honest I don't know it!).
> The 2 board do still have different machnumbers, right?
> I've expected that these machnumbers are to determine
> which board / hardware the kernel / u-boot is running.
> Isn't this the case?
>
My understanding is that the subsys_initcall() primes a particular 
function to be called at a particular point when the kernel fires up.  
As this code is *always* run regardless of machine ID[1] then you can 
run into problems as that code will be run on SheevaPlugs too.

include/linux/init.h might explain things better than I have.  It means 
the PCI init code is called *after* all the architecture/platform stuff 
is done, but before device drivers and filesystem support is enabled.

Cheers
 
[1] look at the MACHINE_START function, the entry points do not hook 
	anywhere into the PCI init code, that is called 'out-of-bound'

-- 
Alexander Clouter
.sigmonster says: Every time I lose weight, it finds me again!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
  2009-11-12 14:19 [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Dieter Kiermaier
  2009-11-12 15:42 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Alexander Clouter
@ 2009-11-12 19:37 ` Lennert Buytenhek
  2009-11-13  7:26   ` Dieter Kiermaier
  2009-11-12 20:55 ` Russell King - ARM Linux
  2 siblings, 1 reply; 10+ messages in thread
From: Lennert Buytenhek @ 2009-11-12 19:37 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Nov 12, 2009 at 06:19:42AM -0800, Dieter Kiermaier wrote:

> Below you can find a first version of the patch which enables PCI on my openrd-base board
> together with a PCIe->PCI bridge.
> Please let me know if something is wrong with it.
>
> [...]
> 
> Subject: [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present

In the end, this is what the patch does, but the issue it solves is (at
least in theory) not specific to PCI.


> If this bit isn't disabled kernel crash during bootup when booted
> with current mainline u-boot (version U-Boot 2009.08-00208-g9ef0569)
> and a PCIe -> PCI bridge is connected to the system.

And this should really explain why the crash happens (which is because
there is a PCI master abort because the PCI scanning code probes all
possible device IDs on the secondary side of the bridge).


> diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
> index 77617c7..9e57326 100644
> --- a/arch/arm/mach-kirkwood/openrd_base-setup.c
> +++ b/arch/arm/mach-kirkwood/openrd_base-setup.c

This issue is not OpenRD-specific, so shouldn't be handled in the
OpenRD board setup file.


>  static int __init openrd_base_pci_init(void)
>  {
> +	u32 cpu_config_reg;
> +	void __iomem *base;
> +	base = ioremap(0xf1020100, 4);
> +	if (base)
> +	{
> +		cpu_config_reg = readl(base);
> +		cpu_config_reg &= ~(1 << 2);
> +		writel(cpu_config_reg, base);
> +	}
> +	iounmap(base);

There's no need to ioremap/iounmap -- the 1MB block of CPU peripheral
registers is permanently mapped into the kernel virtual address
region.

How about something like this instead?  (Since you did most of the work
I don't mind having you as the From: on the patch.)



commit 728ae3400ef8fc3da10491d48e6832b6bb7aa281
Author: Lennert Buytenhek <buytenh@wantstofly.org>
Date:   Thu Nov 12 20:31:14 2009 +0100

    [ARM] Kirkwood: disable propagation of mbus error to the CPU local bus
    
    Disable propagation of mbus errors to the CPU local bus, as this causes
    mbus errors (which can occur for example for PCI aborts) to throw CPU
    aborts, which we're not set up to deal with.
    
    Reported-by: Dieter Kiermaier <dk-arm-linux@gmx.de>
    Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>

diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 0acb61f..96c98c2 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -915,6 +915,14 @@ void __init kirkwood_init(void)
 	kirkwood_uart0_data[0].uartclk = kirkwood_tclk;
 	kirkwood_uart1_data[0].uartclk = kirkwood_tclk;
 
+	/*
+	 * Disable propagation of mbus errors to the CPU local bus,
+	 * as this causes mbus errors (which can occur for example
+	 * for PCI aborts) to throw CPU aborts, which we're not set
+	 * up to deal with.
+	 */
+	writel(readl(CPU_CONFIG) & ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);
+
 	kirkwood_setup_cpu_mbus();
 
 #ifdef CONFIG_CACHE_FEROCEON_L2
diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 9e80d92..418f501 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -13,6 +13,9 @@
 
 #include <mach/kirkwood.h>
 
+#define CPU_CONFIG		(BRIDGE_VIRT_BASE | 0x0100)
+#define CPU_CONFIG_ERROR_PROP	0x00000004
+
 #define CPU_CONTROL		(BRIDGE_VIRT_BASE | 0x0104)
 #define CPU_RESET		0x00000002
 
--

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
  2009-11-12 14:19 [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Dieter Kiermaier
  2009-11-12 15:42 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Alexander Clouter
  2009-11-12 19:37 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Lennert Buytenhek
@ 2009-11-12 20:55 ` Russell King - ARM Linux
  2009-11-13  7:50   ` Dieter Kiermaier
  2 siblings, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux @ 2009-11-12 20:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Nov 12, 2009 at 03:19:42PM +0100, Dieter Kiermaier wrote:
> diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
> index 77617c7..9e57326 100644
> --- a/arch/arm/mach-kirkwood/openrd_base-setup.c
> +++ b/arch/arm/mach-kirkwood/openrd_base-setup.c
> @@ -14,6 +14,7 @@
>  #include <linux/mtd/partitions.h>
>  #include <linux/ata_platform.h>
>  #include <linux/mv643xx_eth.h>
> +#include <asm/io.h>

linux/io.h please (and please ensure future patches similarly use that
header rather than asm/io.h).  checkpatch will tell you this.

>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
>  #include <mach/kirkwood.h>
> @@ -76,9 +77,19 @@ static void __init openrd_base_init(void)
>  
>  static int __init openrd_base_pci_init(void)
>  {
> +	u32 cpu_config_reg;
> +	void __iomem *base;
> +	base = ioremap(0xf1020100, 4);
> +	if (base)
> +	{

checkpatch will also tell you to put the opening brace on the previous line.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
  2009-11-12 19:37 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Lennert Buytenhek
@ 2009-11-13  7:26   ` Dieter Kiermaier
  0 siblings, 0 replies; 10+ messages in thread
From: Dieter Kiermaier @ 2009-11-13  7:26 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Lennert,

> On Thu, Nov 12, 2009 at 06:19:42AM -0800, Dieter Kiermaier wrote:
> 
> > Below you can find a first version of the patch which enables PCI on my openrd-base board
> > together with a PCIe->PCI bridge.
> > Please let me know if something is wrong with it.
> >
> > [...]
> > 
> > Subject: [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
> 
> In the end, this is what the patch does, but the issue it solves is (at
> least in theory) not specific to PCI.
> 
> 
> > If this bit isn't disabled kernel crash during bootup when booted
> > with current mainline u-boot (version U-Boot 2009.08-00208-g9ef0569)
> > and a PCIe -> PCI bridge is connected to the system.
> 
> And this should really explain why the crash happens (which is because
> there is a PCI master abort because the PCI scanning code probes all
> possible device IDs on the secondary side of the bridge).

thanks for being patient and explain it that even a newbie can understand what's going on here!

> 
> 
> > diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
> > index 77617c7..9e57326 100644
> > --- a/arch/arm/mach-kirkwood/openrd_base-setup.c
> > +++ b/arch/arm/mach-kirkwood/openrd_base-setup.c
> 
> This issue is not OpenRD-specific, so shouldn't be handled in the
> OpenRD board setup file.
> 
> 
> >  static int __init openrd_base_pci_init(void)
> >  {
> > +	u32 cpu_config_reg;
> > +	void __iomem *base;
> > +	base = ioremap(0xf1020100, 4);
> > +	if (base)
> > +	{
> > +		cpu_config_reg = readl(base);
> > +		cpu_config_reg &= ~(1 << 2);
> > +		writel(cpu_config_reg, base);
> > +	}
> > +	iounmap(base);
> 
> There's no need to ioremap/iounmap -- the 1MB block of CPU peripheral
> registers is permanently mapped into the kernel virtual address
> region.
> 
> How about something like this instead?  (Since you did most of the work
> I don't mind having you as the From: on the patch.)
> 
Your version looks much more comprehensible so I would prefer your's.
And I've no problem with not being mentioned "from:" because most of the
work is done by Maxime, Alexander and yourself!

I've only triggered it - so reported by fits fine!

Will this patch together with Maximes patch:
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-November/003784.html
applied to marvell orion.git and also upstream?

Dieter
> 
> 
> commit 728ae3400ef8fc3da10491d48e6832b6bb7aa281
> Author: Lennert Buytenhek <buytenh@wantstofly.org>
> Date:   Thu Nov 12 20:31:14 2009 +0100
> 
>     [ARM] Kirkwood: disable propagation of mbus error to the CPU local bus
>     
>     Disable propagation of mbus errors to the CPU local bus, as this causes
>     mbus errors (which can occur for example for PCI aborts) to throw CPU
>     aborts, which we're not set up to deal with.
>     
>     Reported-by: Dieter Kiermaier <dk-arm-linux@gmx.de>
>     Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
> 
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index 0acb61f..96c98c2 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -915,6 +915,14 @@ void __init kirkwood_init(void)
>  	kirkwood_uart0_data[0].uartclk = kirkwood_tclk;
>  	kirkwood_uart1_data[0].uartclk = kirkwood_tclk;
>  
> +	/*
> +	 * Disable propagation of mbus errors to the CPU local bus,
> +	 * as this causes mbus errors (which can occur for example
> +	 * for PCI aborts) to throw CPU aborts, which we're not set
> +	 * up to deal with.
> +	 */
> +	writel(readl(CPU_CONFIG) & ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);
> +
>  	kirkwood_setup_cpu_mbus();
>  
>  #ifdef CONFIG_CACHE_FEROCEON_L2
> diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> index 9e80d92..418f501 100644
> --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
> @@ -13,6 +13,9 @@
>  
>  #include <mach/kirkwood.h>
>  
> +#define CPU_CONFIG		(BRIDGE_VIRT_BASE | 0x0100)
> +#define CPU_CONFIG_ERROR_PROP	0x00000004
> +
>  #define CPU_CONTROL		(BRIDGE_VIRT_BASE | 0x0104)
>  #define CPU_RESET		0x00000002
>  
> --
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
  2009-11-12 20:55 ` Russell King - ARM Linux
@ 2009-11-13  7:50   ` Dieter Kiermaier
  2009-11-13 23:19     ` Russell King - ARM Linux
  0 siblings, 1 reply; 10+ messages in thread
From: Dieter Kiermaier @ 2009-11-13  7:50 UTC (permalink / raw)
  To: linux-arm-kernel

Am Donnerstag 12 November 2009 21:55:28 schrieb Russell King - ARM Linux:
> On Thu, Nov 12, 2009 at 03:19:42PM +0100, Dieter Kiermaier wrote:
> > diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
> > index 77617c7..9e57326 100644
> > --- a/arch/arm/mach-kirkwood/openrd_base-setup.c
> > +++ b/arch/arm/mach-kirkwood/openrd_base-setup.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/mtd/partitions.h>
> >  #include <linux/ata_platform.h>
> >  #include <linux/mv643xx_eth.h>
> > +#include <asm/io.h>
> 
> linux/io.h please (and please ensure future patches similarly use that
> header rather than asm/io.h).  checkpatch will tell you this.

Sorry for my stupid question but was is the difference?
Is it that asm/* provides "architecture specific headers and linux/*
abstract this on level more from the hardware?

I've taken it form LDD (thanks Alessandro!)  but never thought about it.

> 
> >  #include <asm/mach-types.h>
> >  #include <asm/mach/arch.h>
> >  #include <mach/kirkwood.h>
> > @@ -76,9 +77,19 @@ static void __init openrd_base_init(void)
> >  
> >  static int __init openrd_base_pci_init(void)
> >  {
> > +	u32 cpu_config_reg;
> > +	void __iomem *base;
> > +	base = ioremap(0xf1020100, 4);
> > +	if (base)
> > +	{
> 
> checkpatch will also tell you to put the opening brace on the previous line.
> 

Something new I learned. I've read much docs but everyday there is something new
like checkpatch.pl - sorry for missing that.

Many thanks for commenting,
Dieter

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present
  2009-11-12 17:02   ` Dieter Kiermaier
  2009-11-12 18:52     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe?bridge?is present Alexander Clouter
@ 2009-11-13  7:50     ` Simon Kagstrom
  1 sibling, 0 replies; 10+ messages in thread
From: Simon Kagstrom @ 2009-11-13  7:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 12 Nov 2009 18:02:10 +0100
Dieter Kiermaier <dk-arm-linux@gmx.de> wrote:

[openrd_base_pci_init]
> > > +       if (base)
> > > +       {
> > > +               cpu_config_reg = readl(base);
> > > +               cpu_config_reg &= ~(1 << 2);
> > > +               writel(cpu_config_reg, base);
> > > +       }
> > > +       iounmap(base);
> > > +
> > >        if (machine_is_openrd_base())
> > >                kirkwood_pcie_init();
> > > -
> > >        return 0;
> > >  }
> > > subsys_initcall(openrd_base_pci_init);
>
> Hm, is this really right? Why is there a function called openrd_base_pci_init() which is inside a file
> called openrd_base-setup.c and this function is called on a sheevaplug?

You can build the kernel with support for multiple boards, let's say
sheevaplug and OpenRD base/client. Since this function is marked as a
subsys_initcall, it will _always_ be called by the kernel when it has
reached far enough into the startup - it doesn't matter which board
support this function resides in.

Thus, even if you run on a sheevaplug board, it will run
openrd_base_pci_init() and clear your bit. Like Alexander, I also did
the same mistake :-)


Of course, in this particular case, it *should* probably be done for
all boards - just not in the openrd_base board support code!

// Simon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present
  2009-11-13  7:50   ` Dieter Kiermaier
@ 2009-11-13 23:19     ` Russell King - ARM Linux
  0 siblings, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2009-11-13 23:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 13, 2009 at 08:50:09AM +0100, Dieter Kiermaier wrote:
> Am Donnerstag 12 November 2009 21:55:28 schrieb Russell King - ARM Linux:
> > On Thu, Nov 12, 2009 at 03:19:42PM +0100, Dieter Kiermaier wrote:
> > > diff --git a/arch/arm/mach-kirkwood/openrd_base-setup.c b/arch/arm/mach-kirkwood/openrd_base-setup.c
> > > index 77617c7..9e57326 100644
> > > --- a/arch/arm/mach-kirkwood/openrd_base-setup.c
> > > +++ b/arch/arm/mach-kirkwood/openrd_base-setup.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/mtd/partitions.h>
> > >  #include <linux/ata_platform.h>
> > >  #include <linux/mv643xx_eth.h>
> > > +#include <asm/io.h>
> > 
> > linux/io.h please (and please ensure future patches similarly use that
> > header rather than asm/io.h).  checkpatch will tell you this.
> 
> Sorry for my stupid question but was is the difference?
> Is it that asm/* provides "architecture specific headers and linux/*
> abstract this on level more from the hardware?

Yes.  Since there is also a general move to propagate stuff out of
architecture files into generic files, it will save hastle later on
if this stuff is already correct.

LDD is sometimes out dated with things like this.  The general advice
here is that for any asm/foo.h, if there exists linux/foo.h then linux/foo.h
should be included instead.  There are exceptions to this, in particular
asm/irq.h and asm/memory.h (since the linux/ equivalents aren't really
to do with these.)

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2009-11-13 23:19 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-12 14:19 [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Dieter Kiermaier
2009-11-12 15:42 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Alexander Clouter
2009-11-12 17:02   ` Dieter Kiermaier
2009-11-12 18:52     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe?bridge?is present Alexander Clouter
2009-11-13  7:50     ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge?is present Simon Kagstrom
2009-11-12 19:37 ` [PATCH] [ARM] Kirkwood: Prevent kernel from crashing if PCIe bridge is present Lennert Buytenhek
2009-11-13  7:26   ` Dieter Kiermaier
2009-11-12 20:55 ` Russell King - ARM Linux
2009-11-13  7:50   ` Dieter Kiermaier
2009-11-13 23:19     ` Russell King - ARM Linux

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.