* [PATCH 5121 pci 2/3] powerpc: 5121: Add PCI support.
From: John Rigby @ 2008-08-07 17:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: John Rigby
In-Reply-To: <1218130587-31176-1-git-send-email-jrigby@freescale.com>
Uses mpc83xx_add_bridge in fsl_pci.c
Adds second register tuple to pci node register property
as previously done for 83xx device trees in a previous patch.
Signed-off-by: John Rigby <jrigby@freescale.com>
---
arch/powerpc/boot/dts/mpc5121ads.dts | 4 +++-
arch/powerpc/platforms/512x/Kconfig | 2 ++
arch/powerpc/platforms/512x/mpc5121_ads.c | 10 ++++++++++
arch/powerpc/sysdev/fsl_pci.c | 4 ++--
4 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc5121ads.dts b/arch/powerpc/boot/dts/mpc5121ads.dts
index 1f9036c..b5444f7 100644
--- a/arch/powerpc/boot/dts/mpc5121ads.dts
+++ b/arch/powerpc/boot/dts/mpc5121ads.dts
@@ -403,8 +403,10 @@
#interrupt-cells = <1>;
#size-cells = <2>;
#address-cells = <3>;
- reg = <0x80008500 0x100>;
+ reg = <0x80008500 0x100 /* internal registers */
+ 0x80008300 0x8>; /* config space access registers */
compatible = "fsl,mpc5121-pci";
device_type = "pci";
+ primary;
};
};
diff --git a/arch/powerpc/platforms/512x/Kconfig b/arch/powerpc/platforms/512x/Kconfig
index c62f893..326852c 100644
--- a/arch/powerpc/platforms/512x/Kconfig
+++ b/arch/powerpc/platforms/512x/Kconfig
@@ -3,6 +3,8 @@ config PPC_MPC512x
select FSL_SOC
select IPIC
select PPC_CLOCK
+ select PPC_PCI_CHOICE
+ select FSL_PCI if PCI
config PPC_MPC5121
bool
diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c
index 5ebf693..441abc4 100644
--- a/arch/powerpc/platforms/512x/mpc5121_ads.c
+++ b/arch/powerpc/platforms/512x/mpc5121_ads.c
@@ -22,16 +22,26 @@
#include <asm/prom.h>
#include <asm/time.h>
+#include <sysdev/fsl_pci.h>
+
#include "mpc512x.h"
#include "mpc5121_ads.h"
static void __init mpc5121_ads_setup_arch(void)
{
+#ifdef CONFIG_PCI
+ struct device_node *np;
+#endif
printk(KERN_INFO "MPC5121 ADS board from Freescale Semiconductor\n");
/*
* cpld regs are needed early
*/
mpc5121_ads_cpld_map();
+
+#ifdef CONFIG_PCI
+ for_each_compatible_node(np, "pci", "fsl,mpc5121-pci")
+ mpc83xx_add_bridge(np);
+#endif
}
static void __init mpc5121_ads_init_IRQ(void)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index c4bdfc2..0acdd1e 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -251,7 +251,7 @@ DECLARE_PCI_FIXUP_HEADER(0x1957, PCI_DEVICE_ID_MPC8641D, quirk_fsl_pcie_header);
DECLARE_PCI_FIXUP_HEADER(0x1957, PCI_DEVICE_ID_MPC8610, quirk_fsl_pcie_header);
#endif /* CONFIG_PPC_85xx || CONFIG_PPC_86xx */
-#if defined(CONFIG_PPC_83xx)
+#if defined(CONFIG_PPC_83xx) || defined(CONFIG_PPC_MPC512x)
int __init mpc83xx_add_bridge(struct device_node *dev)
{
int len;
@@ -313,7 +313,7 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
setup_indirect_pci(hose, rsrc_cfg.start, rsrc_cfg.start + 4, 0);
- printk(KERN_INFO "Found MPC83xx PCI host bridge at 0x%016llx. "
+ printk(KERN_INFO "Found FSL PCI host bridge at 0x%016llx. "
"Firmware bus number: %d->%d\n",
(unsigned long long)rsrc_reg.start, hose->first_busno,
hose->last_busno);
--
^ permalink raw reply related
* [PATCH 5121 pci 3/3] powerpc: pci: 5121: Hide pci bridge.
From: John Rigby @ 2008-08-07 17:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: John Rigby
In-Reply-To: <1218130587-31176-2-git-send-email-jrigby@freescale.com>
The class of the MPC5121 pci host bridge is PCI_CLASS_BRIDGE_OTHER
while other freescale host bridges have class set to
PCI_CLASS_PROCESSOR_POWERPC.
This patch makes fixup_hide_host_resource_fsl match
PCI_CLASS_BRIDGE_OTHER in addition to PCI_CLASS_PROCESSOR_POWERPC.
Signed-off-by: John Rigby <jrigby@freescale.com>
---
arch/powerpc/kernel/pci_32.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 88db4ff..162c3a8 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -54,11 +54,12 @@ LIST_HEAD(hose_list);
static int pci_bus_count;
static void
-fixup_hide_host_resource_fsl(struct pci_dev* dev)
+fixup_hide_host_resource_fsl(struct pci_dev *dev)
{
int i, class = dev->class >> 8;
- if ((class == PCI_CLASS_PROCESSOR_POWERPC) &&
+ if ((class == PCI_CLASS_PROCESSOR_POWERPC ||
+ class == PCI_CLASS_BRIDGE_OTHER) &&
(dev->hdr_type == PCI_HEADER_TYPE_NORMAL) &&
(dev->bus->parent == NULL)) {
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
--
^ permalink raw reply related
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Timur Tabi @ 2008-08-07 18:11 UTC (permalink / raw)
To: Yoder Stuart
Cc: linuxppc-dev, devicetree-discuss, paulus, miltonm, David Gibson
In-Reply-To: <9696D7A991D0824DBA8DFAC74A9C5FA3043BC7D7@az33exm25.fsl.freescale.net>
Yoder Stuart wrote:
> The second was the idea that we may need a /aliases/stdin as well.
> You could conceptually have stdout be a monitor and a stdin
> be a keyboard.
I don't think the hypervisor console subsystem supports this. I don't see any
way of registering separate clients for input vs. output.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [RFC/PATCH 1/3] powerpc: add ioremap_bat() function for setting up BAT translated IO regions.
From: Kumar Gala @ 2008-08-07 18:21 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, paulus, miltonm
In-Reply-To: <20080807164513.GA22176@ld0162-tx32.am.freescale.net>
On Aug 7, 2008, at 11:45 AM, Scott Wood wrote:
> On Wed, Aug 06, 2008 at 05:11:08PM -0600, Grant Likely wrote:
>> I do this particular test to make absolute sure that the caller
>> absolutely understands the limitations of the block mapping. If they
>> call this with something that isn't 128k aligned, then I make it fail
>> immediately so the coder is forced to go and understand what they are
>> allowed to do. Basically, I'm reinforcing the fact that this is not
>> the same as ioremap().
>>
>> I haven't decided yet if I should test size in the same way.
>> Thoughts?
>
> I'd prefer it round up the size as needed to cover the requested
> mapping
> and needed alignment.
>
> The minimum size is going to be different on Book E, for example,
> and I
> can think of at least one user that will be shared between Book E and
> classic (CPM2 early console).
Which is how ioremap works today. We ask for a size of 3 bytes and it
rounds up to a 4k page. The same should be true of the new interface
(and round up to whatever the smallest size the HW we are using can
handle).
- k
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Milton Miller @ 2008-08-07 19:20 UTC (permalink / raw)
To: Timur Tabi, Yoder Stuart
Cc: linuxppc-dev, devicetree-discuss, paulus, miltonm, David Gibson
TimurTabi wrote:
> Yoder Stuart wrote:
>
> > The second was the idea that we may need a
> > /aliases/stdin as well. You could conceptually have
> > stdout be a monitor and a stdin be a keyboard.
>
> I don't think the hypervisor console subsystem supports
> this. I don't see any way of registering separate clients
> for input vs. output.
Why should what the hvc driver support have any effect on
what the binding should be?
And anyways, both the boot wrapper and udbg (and hence early
debug console) have totally indepenedent functions for
reading and writing.
And for that matter, we also support output on monitor and
input on keyboard for linear frame buffer on 6xx via the
btext output (but only adb input).
milton
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Timur Tabi @ 2008-08-07 19:22 UTC (permalink / raw)
To: miltonm
Cc: linuxppc-dev, devicetree-discuss, paulus, Yoder Stuart,
David Gibson
In-Reply-To: <489b4af2.199.1a42.425913876@bga.com>
Milton Miller wrote:
> Why should what the hvc driver support have any effect on
> what the binding should be?
Because my hypervisor console driver uses the hypervisor console/tty subsystem.
If the subsystem assumes one stdout/stdin device, then I must abide by that.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Scott Wood @ 2008-08-07 19:38 UTC (permalink / raw)
To: Timur Tabi
Cc: devicetree-discuss, Yoder Stuart, miltonm, linuxppc-dev, paulus,
David Gibson
In-Reply-To: <489B4B89.1010507@freescale.com>
Timur Tabi wrote:
> Milton Miller wrote:
>
>> Why should what the hvc driver support have any effect on
>> what the binding should be?
>
> Because my hypervisor console driver uses the hypervisor console/tty subsystem.
> If the subsystem assumes one stdout/stdin device, then I must abide by that.
So fix the subsystem, don't use the subsystem, or live with the fact
that you support a subset of what the device tree can express. Don't
cripple the device tree because of that.
-Scott
^ permalink raw reply
* Device tree question
From: Steven A. Falco @ 2008-08-07 19:56 UTC (permalink / raw)
To: linuxppc-dev
I have added a compact flash to the external bus of a Sequoia
(PPC440EPx) evaluation board. It is wired to CS1, and U-boot is set to
configure CS1 to be at address 0xc1000000. U-boot can access the
device, and reports the correct partition table, etc. so I believe the
hardware is ok.
I've created a device-tree entry under the EBC0 section of the
sequoia.dts file:
pata@1,0 {
compatible = "harris,hydra_temp-pata", "ata-generic";
bank-width = <2>;
reg = <1 0 20 1 80 20>;
reg-shift = <4>;
pio-mode = <4>;
interrupts = <27 4>;
interrupt-parent = <&UIC0>;
};
};
This seems to be correct, because if I turn on debug in prom_parse, I
see a translation that looks reasonable:
OF: translating address: 00000001 00000000
OF: parent bus is default (na=1, ns=1) on /plb/opb
OF: walking ranges...
OF: default map, cp=0, s=4000000, da=100000000
OF: default map, cp=100000000, s=100000, da=100000000
OF: parent translation for: c1000000
OF: with offset: 0
OF: one level translation: c1000000
OF: parent bus is default (na=2, ns=1) on /plb
OF: walking ranges...
OF: default map, cp=0, s=80000000, da=c1000000
OF: default map, cp=80000000, s=80000000, da=c1000000
OF: parent translation for: 00000001 80000000
OF: with offset: 41000000
OF: one level translation: 00000001 c1000000
OF: parent bus is default (na=2, ns=1) on /
OF: no ranges, 1:1 translation
OF: parent translation for: 00000000 00000000
OF: with offset: 1c1000000
OF: one level translation: 00000001 c1000000
OF: reached root node
OF: ** translation for device /plb/opb/ebc/pata@1,0 **
OF: bus is default (na=2, ns=1) on /plb/opb/ebc
(There is another translation for the alternate registers but I'll omit
it for brevity.)
However, there is something wrong, because I get an oops:
Machine check in kernel mode.
Data Write PLB Error
Oops: Machine check, sig: 7 [#1]
LTT NESTING LEVEL : 0
Hydra_temp
Modules linked in:
NIP: c01e4618 LR: c01e4608 CTR: c01e4078
REGS: c0398f50 TRAP: 0214 Not tainted (2.6.25.4-00021-g4b3b5ea-dirty)
MSR: 00029000 <EE,ME> CR: 24044028 XER: 20000007
TASK = cf808400[1] 'swapper' THREAD: cf826000
GPR00: 00000008 cf827ce0 cf808400 cf3ac000 d1078080 00000000 00000001
c03869c0
GPR08: 00000000 c01e4078 cf3ac000 00000001 24044022 00000000 c02e977c
c02e97e0
GPR16: c02e97c8 c036a8bc c02e97f4 c02e9808 c037c0a8 c0386978 00000000
cf360190
GPR24: 00000027 c0386a64 00000000 00000000 cf360190 00000000 cf360194
cf3ac000
NIP [c01e4618] ata_bmdma_freeze+0x44/0x70
LR [c01e4608] ata_bmdma_freeze+0x34/0x70
Call Trace:
[cf827ce0] [0000001f] 0x1f (unreliable)
[cf827cf0] [c01e4c14] __ata_port_freeze+0x3c/0x5c
[cf827d00] [c01e4fa4] ata_eh_freeze_port+0x40/0x5c
[cf827d20] [c01d6868] ata_host_start+0xd8/0x208
[cf827d40] [c01dd2f4] ata_host_activate+0x28/0x124
[cf827d70] [c02a60c4] 0xc02a60c4
[cf827db0] [c02a642c] 0xc02a642c
[cf827e50] [c0222a7c] of_platform_device_probe+0x5c/0x560
[cf827e70] [c01b3148] driver_probe_device+0xb8/0x1e8
[cf827e90] [c01b3470] __driver_attach+0xcc/0xf8
[cf827eb0] [c01b21c4] bus_for_each_dev+0x5c/0x98
[cf827ee0] [c01b2f50] driver_attach+0x24/0x34
[cf827ef0] [c01b2da8] bus_add_driver+0x1d8/0x258
[cf827f20] [c01b371c] driver_register+0x48/0x114
[cf827f40] [c0222950] of_register_driver+0x54/0x70
[cf827f50] [c035ed08] pata_of_platform_init+0x20/0x30
[cf827f60] [c03471cc] kernel_init+0xc8/0x2ac
[cf827ff0] [c000e44c] original_kernel_thread+0x44/0x60
My question is: Did I do the device-tree entry incorrectly or is
something else wrong? I'll keep trying to figure it out on my own, but
if anyone has any tips on debugging this, I'd love to hear them.
Steve
^ permalink raw reply
* Re: [RFC/PATCH 1/3] powerpc: add ioremap_bat() function for setting up BAT translated IO regions.
From: Benjamin Herrenschmidt @ 2008-08-07 22:13 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, paulus, miltonm
In-Reply-To: <63451E1F-30C1-4146-9D1A-BD4973E8F17A@kernel.crashing.org>
On Wed, 2008-08-06 at 20:49 -0500, Kumar Gala wrote:
> On Aug 6, 2008, at 5:28 PM, Benjamin Herrenschmidt wrote:
>
> >
> >> there is a bunch of error checking and difference in semantics that
> >> you need to fix. I think introduce a new API for this is silly,
> >> especially since we expect there to only be one actual invocation of
> >> the API for serial console access.
> >
> > Not necessarily....
> >
> > There's another aspect to BAT mappings here. First, they should be
> > permanent (ie, not unmappable). That way, we have ioremap just use
> > an existing BAT mapping when asked for a device that is covered
> > by a BAT. This allows to have platform code do something like setup
> > a BAT over a bunch of SOC registers or over a device, to automagically
> > get drivers doing ioremap to that area benefit from it.
>
> why should they be permanent.. We could implement reference counting
> around the regions and free BATs if the count = 0.
Do we care ?
> I'm more concerned about this being implemented around the existing
> ioremap core in __ioremap(). We can easily use a flag bit to say use
> "large mappings" or the fact that mem_init_done == 0.
mem_init_done isn't a good indication. We can do page tables when it's
0, we would have to use a separate mem_preinit_done or something :-)
I initially also though about a flag to ioremap_prot to be honest. But
it does obfuscate the normal ioremap code path and if there's a flag,
that means that callers know the difference and thus may as well call
a separate function, don't you think ?
Ben.
^ permalink raw reply
* Re: [RFC/PATCH 1/3] powerpc: add ioremap_bat() function for setting up BAT translated IO regions.
From: Benjamin Herrenschmidt @ 2008-08-07 22:09 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, paulus, miltonm
In-Reply-To: <fa686aa40808061611p616208bp2baf9a0d5e095870@mail.gmail.com>
On Wed, 2008-08-06 at 17:11 -0600, Grant Likely wrote:
> I'm not sure what you're asking. Are you asking about this particular
> test, or are you asking why I don't also test the size?
Badly worded.
I meant BAT sizes are masks of bits. IE, they are power of 2 and the
BAT address must be aligned to that power of 2 (ie, the BAT matching
uses the size as a bit mask of relevant bits to compare).
Unless I misread, your code doesn't provide the necessary tests/rounding
of size and alignment of the virtual address.. does it ?
> I do this particular test to make absolute sure that the caller
> absolutely understands the limitations of the block mapping. If they
> call this with something that isn't 128k aligned, then I make it fail
> immediately so the coder is forced to go and understand what they are
> allowed to do. Basically, I'm reinforcing the fact that this is not
> the same as ioremap().
>
> I haven't decided yet if I should test size in the same way.
> Thoughts?
Ben.
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Benjamin Herrenschmidt @ 2008-08-07 22:35 UTC (permalink / raw)
To: Grant Likely
Cc: devicetree-discuss, miltonm, linuxppc-dev, Paul Mackerras,
David Miller
In-Reply-To: <fa686aa40808060631k278e187ej965f0ac08c837937@mail.gmail.com>
> It's not what we do with flattened device trees blobs though. In the
> flattened tree we're not using a /chosen/stdout property, just the
> linux,stdout-path one.
>
> The question that remains is; should there be? Should the dt blobs
> use /chosen/stdout also? (I'm not familiar enough with real OF to
> know the answer. I'm assuming that an instance value is not the same
> as a phandle).
Yup, there are two issues there:
- The instance value would have to be converted to a phandle while
OF is still alive. I initially did that and added a stdout-node or so
property, but that still hit the next issue.
- IBM machines has this weird distinction between the real phandle
and the ibm,phandle, the later being the same except for things
that get hotplugged ... and some of the vdevices. We got really
confused trying to sort that out with the output device.
So in the end, I decided to just convert the ihandle to a path and
stick that path in the device-tree.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Benjamin Herrenschmidt @ 2008-08-07 22:37 UTC (permalink / raw)
To: Grant Likely; +Cc: miltonm, linuxppc-dev, paulus, David Miller
In-Reply-To: <fa686aa40808052335i7a3bbc64g5bcdaca21c3550af@mail.gmail.com>
On Wed, 2008-08-06 at 00:35 -0600, Grant Likely wrote:
> On Wed, Aug 6, 2008 at 12:32 AM, David Miller <davem@davemloft.net> wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> > Date: Wed, 06 Aug 2008 00:02:44 -0600
> >
> >> of_lookup_stdout() is useful for figuring out what device to use as output
> >> for early boot progress messages. It returns the node pointed to by the
> >> linux,stdout-path property in the chosen node.
> >>
> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> >
> > On sparc platforms this is obtained differently. We obtain the 32-bit
> > instance value of "/chosen/stdout" and convert that into a prom device
> > node path using "instance-to-path".
>
> How about if I modify it to try both methods?
The sparc method however cannot be used on powerpc because it requires
a call to OF to do the "live" conversion (ie, instance-to-path). In
fact, our /chosen/linux,stdout-path is just a cached result of that
conversion done during boot.
Ben.
^ permalink raw reply
* Re: [RFC/PATCH 2/3] of: add of_lookup_stdout() utility function
From: Benjamin Herrenschmidt @ 2008-08-07 22:36 UTC (permalink / raw)
To: Timur Tabi; +Cc: miltonm, linuxppc-dev, paulus
In-Reply-To: <ed82fe3e0808060946v39992095pfb6b85726b7d307d@mail.gmail.com>
On Wed, 2008-08-06 at 11:46 -0500, Timur Tabi wrote:
> On Wed, Aug 6, 2008 at 1:02 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > From: Grant Likely <grant.likely@secretlab.ca>
> >
> > of_lookup_stdout() is useful for figuring out what device to use as output
> > for early boot progress messages. It returns the node pointed to by the
> > linux,stdout-path property in the chosen node.
>
> I thought linux,stdout-path is deprecated are we're supposed to be
> using the aliases node instead?
You are mixing two completely different things afaik.
Ben.
^ permalink raw reply
* Re: [RFC/PATCH 1/3] powerpc: add ioremap_bat() function for setting up BAT translated IO regions.
From: Kumar Gala @ 2008-08-08 0:04 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, paulus, miltonm
In-Reply-To: <1218147224.24157.308.camel@pasglop>
On Aug 7, 2008, at 5:13 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2008-08-06 at 20:49 -0500, Kumar Gala wrote:
>> On Aug 6, 2008, at 5:28 PM, Benjamin Herrenschmidt wrote:
>>
>>>
>>>> there is a bunch of error checking and difference in semantics that
>>>> you need to fix. I think introduce a new API for this is silly,
>>>> especially since we expect there to only be one actual invocation
>>>> of
>>>> the API for serial console access.
>>>
>>> Not necessarily....
>>>
>>> There's another aspect to BAT mappings here. First, they should be
>>> permanent (ie, not unmappable). That way, we have ioremap just use
>>> an existing BAT mapping when asked for a device that is covered
>>> by a BAT. This allows to have platform code do something like setup
>>> a BAT over a bunch of SOC registers or over a device, to
>>> automagically
>>> get drivers doing ioremap to that area benefit from it.
>>
>> why should they be permanent.. We could implement reference counting
>> around the regions and free BATs if the count = 0.
>
> Do we care ?
probably not for BATs but for other things we might.
>> I'm more concerned about this being implemented around the existing
>> ioremap core in __ioremap(). We can easily use a flag bit to say use
>> "large mappings" or the fact that mem_init_done == 0.
>
> mem_init_done isn't a good indication. We can do page tables when it's
> 0, we would have to use a separate mem_preinit_done or something :-)
>
> I initially also though about a flag to ioremap_prot to be honest. But
> it does obfuscate the normal ioremap code path and if there's a flag,
> that means that callers know the difference and thus may as well call
> a separate function, don't you think ?
I'm ok with exposing a separate function as far as the API goes.. I'm
not ok with duplicating the logic of __ioremap().
- k
^ permalink raw reply
* Re: problem in booting kernel with mpc836x_mds.dtb
From: Jerry Van Baren @ 2008-08-08 2:24 UTC (permalink / raw)
To: surendranath.moilla; +Cc: Linuxppc-dev
In-Reply-To: <19152.136.182.158.153.1218051991.squirrel@webmail.cmcltd.com>
surendranath.moilla@cmcltd.com wrote:
> Hi,
> I have the following problem, when i am trying to boot linux on
> MPC8360E MDS board with the mpc836x_mds.dtb created using dtc and
> mpc836x_mds.dts in from /arch/powerpc/boot/platforms/dts/ directory of
> linux-2.6.22 version.
>
> fdt_chosen: FDT_ERR_BADMAGIC
>
> after this it is trying to re boot.
> how to resolve this issue, so i need to apply any pathces to
> mpc836x_mds.dts file.
> NOTE: i am using dtc compiled from linux-2.6.26 version.
>
> Regards
> Surendra
Hi Surendra,
Is the error message coming from u-boot or linux? More lines from the
boot sequence (like most or all of them) would be useful.
The error message is saying that whatever you are passing to the kernel
(or setting in u-boot with "fdt addr") for your dtb blob is not a valid
dtb blob. Display memory where you think your blob resides: if it
doesn't have 0xd00dfeed in that location, it isn't a valid binary blob.
I suspect you will find either you didn't compile your .dts file
correctly or your blob address is wrong.
If you gave us your dtc command line, that might be helpful.
HTH,
gvb
^ permalink raw reply
* Strange tg3 regression with UMP fw. link reporting
From: Benjamin Herrenschmidt @ 2008-08-08 7:35 UTC (permalink / raw)
To: mcarlson; +Cc: linuxppc-dev list, netdev, Nathan Lynch, Michael Chan
Hi Matt !
The IBM PowerStation is a machine similar in design to our JS21 blades,
which uses an HT2000 bridge with it's dual 5780 TG3's.
I started investigating recently a problem where with recent kernels,
the machine will appear to "freeze" every second or two for a second
or two. The "freeze" would affect pretty much everything.
We noticed that it disappears when downing eth0, and finally bisected
it down to commit 7c5026aa9b81dd45df8d3f4e0be73e485976a8b6 "Add link
state reporting to UMP firmware".
I don't know yet for sure what happens, but a quick look at the commit
seems to show that the driver synchronously spin-waits for up to 2.5ms
with a lock held multiple times from a timer interrupt. I don't know
yet if that's where the problem comes from, or if it's an issue with
the FW going nuts and the chip hogging the machine's bus or whatever
else, I'll have to do some more experiments on monday, but in any case,
that spin is really not nice.
The relevant pieces of lspci and dmesg are:
0001:00:01.0 PCI bridge: Broadcom HT2000 PCI-X bridge (rev b0)
0001:00:02.0 PCI bridge: Broadcom HT2000 PCI-X bridge (rev b0)
0001:00:03.0 PCI bridge: Broadcom HT2000 PCI-Express bridge (rev b0)
0001:00:04.0 PCI bridge: Broadcom HT2000 PCI-Express bridge (rev b0)
0001:00:05.0 PCI bridge: Broadcom HT2000 PCI-Express bridge (rev b0)
0001:00:06.0 PCI bridge: Broadcom HT2000 PCI-Express bridge (rev b0)
0001:02:04.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5780 Gigabit Ethernet (rev 10)
0001:02:04.1 Ethernet controller: Broadcom Corporation NetXtreme BCM5780 Gigabit Ethernet (rev 10)
tg3.c:v3.91 (April 18, 2008)
tg3 0001:02:04.0: enabling device (0140 -> 0142)
eth0: Tigon3 [partno(BCM95780) rev 8100 PHY(5780)] (PCIX:133MHz:64-bit) 10/100/1000Base-T Ethernet 00:14:5e:9e:01:82
eth0: RXcsums[1] LinkChgREG[0] MIirq[0] ASF[1] WireSpeed[1] TSOcap[1]
eth0: dma_rwctrl[76144000] dma_mask[40-bit]
tg3 0001:02:04.1: enabling device (0140 -> 0142)
eth1: Tigon3 [partno(BCM95780) rev 8100 PHY(5780)] (PCIX:133MHz:64-bit) 10/100/1000Base-T Ethernet 00:14:5e:9e:01:83
eth1: RXcsums[1] LinkChgREG[0] MIirq[0] ASF[0] WireSpeed[1] TSOcap[1]
eth1: dma_rwctrl[76144000] dma_mask[40-bit]
Any help sorting that out would be much appreciated !
Cheers,
Ben.
^ permalink raw reply
* Re: Strange tg3 regression with UMP fw. link reporting
From: Segher Boessenkool @ 2008-08-08 8:58 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev list, Nathan Lynch, mcarlson, Michael Chan, netdev
In-Reply-To: <1218180939.24157.332.camel@pasglop>
> I don't know yet for sure what happens, but a quick look at the commit
> seems to show that the driver synchronously spin-waits for up to 2.5ms
That's what the comment says, but the code says 2.5 _seconds_:
+ /* Wait for up to 2.5 milliseconds */
+ for (i = 0; i < 250000; i++) {
+ if (!(tr32(GRC_RX_CPU_EVENT) & GRC_RX_CPU_DRIVER_EVENT))
+ break;
+ udelay(10);
+ }
(not that milliseconds wouldn't be bad already...)
Segher
^ permalink raw reply
* Re: Strange tg3 regression with UMP fw. link reporting
From: Benjamin Herrenschmidt @ 2008-08-08 9:18 UTC (permalink / raw)
To: Segher Boessenkool
Cc: linuxppc-dev list, Nathan Lynch, mcarlson, Michael Chan, netdev
In-Reply-To: <31233EB5-037E-4615-95C9-7C816E510752@kernel.crashing.org>
On Fri, 2008-08-08 at 10:58 +0200, Segher Boessenkool wrote:
> > I don't know yet for sure what happens, but a quick look at the commit
> > seems to show that the driver synchronously spin-waits for up to 2.5ms
>
> That's what the comment says, but the code says 2.5 _seconds_:
>
> + /* Wait for up to 2.5 milliseconds */
> + for (i = 0; i < 250000; i++) {
> + if (!(tr32(GRC_RX_CPU_EVENT) & GRC_RX_CPU_DRIVER_EVENT))
> + break;
> + udelay(10);
> + }
>
> (not that milliseconds wouldn't be bad already...)
Right, indeed. I think we have a good candidate for the problem :-) I'll
verify that on monday. Now, that leads to two questions:
- What such a synchronous and potentially horribly slow code is
going in a locked section or a timer interrupts ? Ie, the link
watch should probably move to a workqueue if that is to remain,
or the code turned into a state machine that periodically check for
events, or whatever is more sane than the above.
- The code should at least display some error and do something sane in
case of timeout such as disabling the new UMP feature instead of
repeatedly looping ...
- If this is indeed our problem (timing out in the code above), why is
our firmware not emitting the requested event -> maybe the PowerStations
need a tg3 firmware update.
Matt, what's your take on this ?
Cheers,
Ben.
^ permalink raw reply
* Re: Strange tg3 regression with UMP fw. link reporting
From: Arnd Bergmann @ 2008-08-08 9:21 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mcarlson, netdev, Nathan Lynch, Michael Chan
In-Reply-To: <31233EB5-037E-4615-95C9-7C816E510752@kernel.crashing.org>
On Friday 08 August 2008, Segher Boessenkool wrote:
> > I don't know yet for sure what happens, but a quick look at the commit
> > seems to show that the driver synchronously spin-waits for up to 2.5ms
>=20
> That's what the comment says, but the code says 2.5 _seconds_:
>=20
> + =A0 =A0 =A0 /* Wait for up to 2.5 milliseconds */
> + =A0 =A0 =A0 for (i =3D 0; i < 250000; i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!(tr32(GRC_RX_CPU_EVENT) & GRC_RX_CPU_D=
RIVER_EVENT))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10);
> + =A0 =A0 =A0 }
>=20
> (not that milliseconds wouldn't be bad already...)
It can potentially be even much longer, because each udelay will wait
for *more* than 10 microseconds, and tr32() is an mmio read that takes
additional time, possibly in the order of microseconds as well.
The correct way to implement a timeout like this would be to use
time_before() in the condition, aside from better not doing a busy-loop
in the first place.
Arnd <><
^ permalink raw reply
* [PATCH] Re: Level IRQ handling on Xilinx INTC with ARCH=powerpc
From: Sergey Temerkhanov @ 2008-08-08 9:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <360961217332693@webmail22.yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 221 bytes --]
I've prepared the patch to fix the problem being discussed. It adds a field
flags to struct irq_chip. If IRQ_CHIP_UNMASK_ACK is set in this field,
acknowledge is performed before unmasking.
The patch is against 2.6.26
[-- Attachment #2: fix-level-irq-handling.patch --]
[-- Type: text/x-diff, Size: 1696 bytes --]
diff -r 6b0915754563 arch/powerpc/sysdev/xilinx_intc.c
--- a/arch/powerpc/sysdev/xilinx_intc.c Mon Jul 28 19:59:22 2008 +0400
+++ b/arch/powerpc/sysdev/xilinx_intc.c Fri Aug 08 13:13:52 2008 +0400
@@ -73,6 +73,7 @@
.mask = xilinx_intc_mask,
.unmask = xilinx_intc_unmask,
.ack = xilinx_intc_ack,
+ .flags = IRQ_CHIP_UNMASK_ACK,
};
/*
@@ -107,8 +108,8 @@
}
regs = ioremap(res.start, 32);
- printk(KERN_INFO "Xilinx intc at 0x%08LX mapped to 0x%p\n",
- res.start, regs);
+ printk(KERN_INFO "Xilinx intc at 0x%p mapped to 0x%p\n",
+ (void *)res.start, regs);
/* Setup interrupt controller */
out_be32(regs + XINTC_IER, 0); /* disable all irqs */
diff -r 6b0915754563 include/linux/irq.h
--- a/include/linux/irq.h Mon Jul 28 19:59:22 2008 +0400
+++ b/include/linux/irq.h Fri Aug 08 13:13:52 2008 +0400
@@ -114,6 +114,9 @@
int (*retrigger)(unsigned int irq);
int (*set_type)(unsigned int irq, unsigned int flow_type);
int (*set_wake)(unsigned int irq, unsigned int on);
+
+ unsigned int flags;
+#define IRQ_CHIP_UNMASK_ACK 0x00000001
/* Currently used only by UML, might disappear one day.*/
#ifdef CONFIG_IRQ_RELEASE_METHOD
diff -r 6b0915754563 kernel/irq/chip.c
--- a/kernel/irq/chip.c Mon Jul 28 19:59:22 2008 +0400
+++ b/kernel/irq/chip.c Fri Aug 08 13:13:52 2008 +0400
@@ -377,8 +377,12 @@
spin_lock(&desc->lock);
desc->status &= ~IRQ_INPROGRESS;
- if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
- desc->chip->unmask(irq);
+ if (!(desc->status & IRQ_DISABLED)) {
+ if (desc->chip->flags & IRQ_CHIP_UNMASK_ACK)
+ desc->chip->ack(irq);
+ if (desc->chip->unmask)
+ desc->chip->unmask(irq);
+ }
out_unlock:
spin_unlock(&desc->lock);
}
^ permalink raw reply
* [PATCH] watchdog: delete unused driver mpc8xx_wdt.c
From: Jochen Friedrich @ 2008-08-08 11:14 UTC (permalink / raw)
To: Wim Van Sebroeck; +Cc: Adrian Bunk, Kernel, Linux, linuxppc-dev list
The watchdog driver mpc8xx_wdt.c was a device interface to
arch/ppc/syslib/m8xx_wdt.c for MPC8xx hardware. Now that ARCH=ppc is
gone, this driver is of no more use. For ARCH=powerpc, MPC8xx hardware
is supported by mpc8xxx_wdt.c.
Signed-off-by: Jochen Friedrich <jochen@scram.de>
Acked-by: Vitaly Bordug <vitb@kernel.crashing.org>
---
drivers/watchdog/Kconfig | 4 -
drivers/watchdog/Makefile | 1 -
drivers/watchdog/mpc8xx_wdt.c | 170 -----------------------------------------
3 files changed, 0 insertions(+), 175 deletions(-)
delete mode 100644 drivers/watchdog/mpc8xx_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 32b9fe1..46e763c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -691,10 +691,6 @@ config MPC5200_WDT
tristate "MPC5200 Watchdog Timer"
depends on PPC_MPC52xx
-config 8xx_WDT
- tristate "MPC8xx Watchdog Timer"
- depends on 8xx
-
config 8xxx_WDT
tristate "MPC8xxx Platform Watchdog Timer"
depends on PPC_8xx || PPC_83xx || PPC_86xx
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 049c918..6c20717 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -101,7 +101,6 @@ obj-$(CONFIG_TXX9_WDT) += txx9wdt.o
# PARISC Architecture
# POWERPC Architecture
-obj-$(CONFIG_8xx_WDT) += mpc8xx_wdt.o
obj-$(CONFIG_MPC5200_WDT) += mpc5200_wdt.o
obj-$(CONFIG_8xxx_WDT) += mpc8xxx_wdt.o
obj-$(CONFIG_MV64X60_WDT) += mv64x60_wdt.o
diff --git a/drivers/watchdog/mpc8xx_wdt.c b/drivers/watchdog/mpc8xx_wdt.c
deleted file mode 100644
index 1336425..0000000
--- a/drivers/watchdog/mpc8xx_wdt.c
+++ /dev/null
@@ -1,170 +0,0 @@
-/*
- * mpc8xx_wdt.c - MPC8xx watchdog userspace interface
- *
- * Author: Florian Schirmer <jolt@tuxbox.org>
- *
- * 2002 (c) Florian Schirmer <jolt@tuxbox.org> This file is licensed under
- * the terms of the GNU General Public License version 2. This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
- */
-
-#include <linux/fs.h>
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/watchdog.h>
-#include <asm/8xx_immap.h>
-#include <linux/uaccess.h>
-#include <linux/io.h>
-#include <syslib/m8xx_wdt.h>
-
-static unsigned long wdt_opened;
-static int wdt_status;
-static spinlock_t wdt_lock;
-
-static void mpc8xx_wdt_handler_disable(void)
-{
- volatile uint __iomem *piscr;
- piscr = (uint *)&((immap_t *)IMAP_ADDR)->im_sit.sit_piscr;
-
- if (!m8xx_has_internal_rtc)
- m8xx_wdt_stop_timer();
- else
- out_be32(piscr, in_be32(piscr) & ~(PISCR_PIE | PISCR_PTE));
- printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler deactivated\n");
-}
-
-static void mpc8xx_wdt_handler_enable(void)
-{
- volatile uint __iomem *piscr;
- piscr = (uint *)&((immap_t *)IMAP_ADDR)->im_sit.sit_piscr;
-
- if (!m8xx_has_internal_rtc)
- m8xx_wdt_install_timer();
- else
- out_be32(piscr, in_be32(piscr) | PISCR_PIE | PISCR_PTE);
- printk(KERN_NOTICE "mpc8xx_wdt: keep-alive handler activated\n");
-}
-
-static int mpc8xx_wdt_open(struct inode *inode, struct file *file)
-{
- if (test_and_set_bit(0, &wdt_opened))
- return -EBUSY;
- m8xx_wdt_reset();
- mpc8xx_wdt_handler_disable();
- return nonseekable_open(inode, file);
-}
-
-static int mpc8xx_wdt_release(struct inode *inode, struct file *file)
-{
- m8xx_wdt_reset();
-#if !defined(CONFIG_WATCHDOG_NOWAYOUT)
- mpc8xx_wdt_handler_enable();
-#endif
- clear_bit(0, &wdt_opened);
- return 0;
-}
-
-static ssize_t mpc8xx_wdt_write(struct file *file, const char *data,
- size_t len, loff_t *ppos)
-{
- if (len) {
- spin_lock(&wdt_lock);
- m8xx_wdt_reset();
- spin_unlock(&wdt_lock);
- }
- return len;
-}
-
-static long mpc8xx_wdt_ioctl(struct file *file,
- unsigned int cmd, unsigned long arg)
-{
- int timeout;
- static struct watchdog_info info = {
- .options = WDIOF_KEEPALIVEPING,
- .firmware_version = 0,
- .identity = "MPC8xx watchdog",
- };
-
- switch (cmd) {
- case WDIOC_GETSUPPORT:
- if (copy_to_user((void *)arg, &info, sizeof(info)))
- return -EFAULT;
- break;
-
- case WDIOC_GETSTATUS:
- case WDIOC_GETBOOTSTATUS:
- if (put_user(wdt_status, (int *)arg))
- return -EFAULT;
- wdt_status &= ~WDIOF_KEEPALIVEPING;
- break;
-
- case WDIOC_GETTEMP:
- return -EOPNOTSUPP;
-
- case WDIOC_SETOPTIONS:
- return -EOPNOTSUPP;
-
- case WDIOC_KEEPALIVE:
- spin_lock(&wdt_lock);
- m8xx_wdt_reset();
- wdt_status |= WDIOF_KEEPALIVEPING;
- spin_unlock(&wdt_lock);
- break;
-
- case WDIOC_SETTIMEOUT:
- return -EOPNOTSUPP;
-
- case WDIOC_GETTIMEOUT:
- spin_lock(&wdt_lock);
- timeout = m8xx_wdt_get_timeout();
- spin_unlock(&wdt_lock);
- if (put_user(timeout, (int *)arg))
- return -EFAULT;
- break;
-
- default:
- return -ENOTTY;
- }
-
- return 0;
-}
-
-static const struct file_operations mpc8xx_wdt_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .write = mpc8xx_wdt_write,
- .unlocked_ioctl = mpc8xx_wdt_ioctl,
- .open = mpc8xx_wdt_open,
- .release = mpc8xx_wdt_release,
-};
-
-static struct miscdevice mpc8xx_wdt_miscdev = {
- .minor = WATCHDOG_MINOR,
- .name = "watchdog",
- .fops = &mpc8xx_wdt_fops,
-};
-
-static int __init mpc8xx_wdt_init(void)
-{
- spin_lock_init(&wdt_lock);
- return misc_register(&mpc8xx_wdt_miscdev);
-}
-
-static void __exit mpc8xx_wdt_exit(void)
-{
- misc_deregister(&mpc8xx_wdt_miscdev);
-
- m8xx_wdt_reset();
- mpc8xx_wdt_handler_enable();
-}
-
-module_init(mpc8xx_wdt_init);
-module_exit(mpc8xx_wdt_exit);
-
-MODULE_AUTHOR("Florian Schirmer <jolt@tuxbox.org>");
-MODULE_DESCRIPTION("MPC8xx watchdog driver");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
--
1.5.6.3
^ permalink raw reply related
* Re: [PATCH 2/2] Cell OProfile: SPU mutex lock fix, version 4
From: Arnd Bergmann @ 2008-08-08 13:26 UTC (permalink / raw)
To: Carl Love; +Cc: linuxppc-dev, cel, cbe-oss-dev, linux-kernel
In-Reply-To: <1217620879.15667.145.camel@carll-linux-desktop>
On Friday 01 August 2008, Carl Love wrote:
> If an error occurs on opcontrol start, the event and per cpu buffers=20
> are released. =A0If later opcontrol shutdown is called then the free
> function will be called again to free buffers that no longer=20
> exist. =A0This results in a kernel oops. =A0The following changes
> prevent the call to delete buffers that don't exist.
>=20
> Signed-off-by: Carl Love <carll@us.ibm.com>
>=20
vfree(NULL) is defined to be legal, so you don't need to check the
argument for being non-NULL, just set it to NULL after the free.
Arnd <><
^ permalink raw reply
* [2.6 patch] cleanup powerpc/include/asm/ide.h
From: Adrian Bunk @ 2008-08-08 15:18 UTC (permalink / raw)
To: bzolnier; +Cc: linux-ide, linuxppc-dev
This patch removes code that became unused through IDE changes and the
arch/ppc/ removal.
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
arch/powerpc/include/asm/ide.h | 43 ---------------------------------
1 file changed, 1 insertion(+), 42 deletions(-)
363979e32b08c578431a1773e7b885698feb30aa
diff --git a/arch/powerpc/include/asm/ide.h b/arch/powerpc/include/asm/ide.h
index 048480e..da01b20 100644
--- a/arch/powerpc/include/asm/ide.h
+++ b/arch/powerpc/include/asm/ide.h
@@ -6,12 +6,7 @@
#ifndef _ASM_POWERPC_IDE_H
#define _ASM_POWERPC_IDE_H
-#ifdef __KERNEL__
-
-#ifndef __powerpc64__
-#include <linux/sched.h>
-#include <asm/mpc8xx.h>
-#endif
+#include <linux/compiler.h>
#include <asm/io.h>
#define __ide_mm_insw(p, a, c) readsw((void __iomem *)(p), (a), (c))
@@ -19,40 +14,4 @@
#define __ide_mm_outsw(p, a, c) writesw((void __iomem *)(p), (a), (c))
#define __ide_mm_outsl(p, a, c) writesl((void __iomem *)(p), (a), (c))
-#ifndef __powerpc64__
-#include <linux/ioport.h>
-
-/* FIXME: use ide_platform host driver */
-static __inline__ int ide_default_irq(unsigned long base)
-{
-#ifdef CONFIG_PPLUS
- switch (base) {
- case 0x1f0: return 14;
- case 0x170: return 15;
- }
-#endif
- return 0;
-}
-
-/* FIXME: use ide_platform host driver */
-static __inline__ unsigned long ide_default_io_base(int index)
-{
-#ifdef CONFIG_PPLUS
- switch (index) {
- case 0: return 0x1f0;
- case 1: return 0x170;
- }
-#endif
- return 0;
-}
-
-#ifdef CONFIG_BLK_DEV_MPC8xx_IDE
-#define IDE_ARCH_ACK_INTR 1
-#define ide_ack_intr(hwif) ((hwif)->ack_intr ? (hwif)->ack_intr(hwif) : 1)
-#endif
-
-#endif /* __powerpc64__ */
-
-#endif /* __KERNEL__ */
-
#endif /* _ASM_POWERPC_IDE_H */
^ permalink raw reply related
* Re: [PATCH 1/2] Cell OProfile: SPU mutex lock fix, version 4
From: Arnd Bergmann @ 2008-08-08 16:08 UTC (permalink / raw)
To: Carl Love; +Cc: linuxppc-dev, cel, cbe-oss-dev, linux-kernel
In-Reply-To: <1217620877.15667.143.camel@carll-linux-desktop>
On Friday 01 August 2008, Carl Love wrote:
> The issue is the SPU code is not holding the kernel mutex lock while
> adding samples to the kernel buffer.
Thanks for your patch, and sorry for not replying earlier.
It looks good from a functionality perspective, I just have
some style comments left that I hope we can address quickly.
Since this is still a bug fix (though a rather large one), I
guess we can should get it into 2.6.27-rc3.
Arnd <><
> Index: Cell_kernel_6_26_2008/arch/powerpc/oprofile/cell/spu_task_sync.c
> ===================================================================
> --- Cell_kernel_6_26_2008.orig/arch/powerpc/oprofile/cell/spu_task_sync.c
> +++ Cell_kernel_6_26_2008/arch/powerpc/oprofile/cell/spu_task_sync.c
> @@ -35,7 +35,106 @@ static DEFINE_SPINLOCK(buffer_lock);
> static DEFINE_SPINLOCK(cache_lock);
> static int num_spu_nodes;
> int spu_prof_num_nodes;
> -int last_guard_val[MAX_NUMNODES * 8];
> +int last_guard_val[MAX_NUMNODES * SPUS_PER_NODE];
> +static int spu_ctx_sw_seen[MAX_NUMNODES * SPUS_PER_NODE];
> +static int sync_start_registered;
> +static int delayed_work_init;
You don't need the delayed_work_init variable. Just initialize
the work queue in your init function to be sure it's always
right.
I think you should also try to remove sync_start_registered,
these global state variables can easily get annoying when you
try to change something.
AFAICT, spu_sync_stop does not get called unless spu_sync_start
was successful, so the sync_start_registered variable is
redundant.
> +static int oprofile_spu_buff_create(void)
> +{
> + int spu;
> +
> + max_spu_buff = oprofile_get_cpu_buffer_size();
> +
> + for (spu = 0; spu < num_spu_nodes; spu++) {
> + /* create circular buffers to store the data in.
> + * use locks to manage accessing the buffers
> + */
> + spu_buff.index[spu].head = 0;
> + spu_buff.index[spu].tail = 0;
> +
> + /*
> + * Create a buffer for each SPU. Can't reliably
> + * create a single buffer for all spus due to not
> + * enough contiguous kernel memory.
> + */
> +
> + spu_buff.buff[spu] = kzalloc((max_spu_buff
> + * sizeof(unsigned long)),
> + GFP_KERNEL);
> +
> + if (!spu_buff.buff[spu]) {
> + printk(KERN_ERR "SPU_PROF: "
> + "%s, line %d: oprofile_spu_buff_create " \
> + "failed to allocate spu buffer %d.\n",
> + __FUNCTION__, __LINE__, spu);
The formatting of the printk line is a little unconventional. You certainly
don't need the '\' at the end of the line.
Also, please don't use __FUNCTION__ in new code, but instead of the
standard c99 __func__ symbol. The __LINE__ macro is fine.
> +
> + /* release the spu buffers that have been allocated */
> + while (spu >= 0) {
> + if (spu_buff.buff[spu]) {
> + kfree(spu_buff.buff[spu]);
> + spu_buff.buff[spu] = 0;
> + }
> + spu--;
> + }
> + return 1;
> + }
> + }
> + return 0;
> +}
The convention for a function would be to return -ENOMEM here instead
of 1.
> /* The main purpose of this function is to synchronize
> * OProfile with SPUFS by registering to be notified of
> * SPU task switches.
> @@ -372,30 +521,50 @@ static int number_of_online_nodes(void)
> */
> int spu_sync_start(void)
> {
> - int k;
> + int spu;
> int ret = SKIP_GENERIC_SYNC;
> int register_ret;
> unsigned long flags = 0;
>
> spu_prof_num_nodes = number_of_online_nodes();
> num_spu_nodes = spu_prof_num_nodes * 8;
> + delayed_work_init = 0;
> +
> + /* create buffer for storing the SPU data to put in
> + * the kernel buffer.
> + */
> + if (oprofile_spu_buff_create()) {
> + ret = -ENOMEM;
> + sync_start_registered = 0;
> + goto out;
> + }
consequently, this becomes
ret = oprofile_spu_buff_create();
if (ret)
goto out;
> -out:
> +
> + /* remove scheduled work queue item rather then waiting
> + * for every queued entry to execute. Then flush pending
> + * system wide buffer to event buffer. Only try to
> + * remove if it was scheduled. Get kernel errors otherwise.
> + */
> + if (delayed_work_init)
> + cancel_delayed_work(&spu_work);
> +
> + for (k = 0; k < num_spu_nodes; k++) {
> + spu_ctx_sw_seen[k] = 0;
> +
> + /* spu_sys_buff will be null if there was a problem
> + * allocating the buffer. Only delete if it exists.
> + */
> +
> + if (spu_buff.buff[k]) {
> + kfree(spu_buff.buff[k]);
> + spu_buff.buff[k] = 0;
> + }
> + }
The if(spu_buff.buff[k]) is redundant, kfree(NULL) is valid, so you
should simplify this.
> +/* Put head and tail for the spu buffer into a structure to keep
> + * them in the same cache line.
> + */
> +struct head_tail {
> + unsigned int head;
> + unsigned int tail;
> +};
> +
> +struct spu_buffers {
> + unsigned long *buff[MAX_NUMNODES * SPUS_PER_NODE];
> + struct head_tail index[MAX_NUMNODES * SPUS_PER_NODE];
> +};
> +
Did you measure a problem in the cache layout here? A simpler
array of
struct spu_buffer {
int last_guard_val;
int spu_ctx_sw_seen;
unsigned long *buff;
unsigned int head, tail;
};
would otherwise be more reasonable, mostly for readability.
> +/* The function can be used to add a buffer worth of data directly to
> + * the kernel buffer. The buffer is assumed to be a circular buffer.
> + * Take the entries from index start and end at index end, wrapping
> + * at max_entries.
> + */
> +void oprofile_put_buff(unsigned long *buf, unsigned int start,
> + unsigned int stop, unsigned int max)
> +{
> + int i;
> +
> + /* Check the args */
> + if (stop > max) {
> + printk(KERN_ERR "oprofile: oprofile_put_buff(), illegal "\
> + "arguments; stop greater then max."\
> + " stop = %u, max = %u.\n",
> + stop, max);
> + return;
> + }
hmm, this is not a good interface. Better return -EINVAL in case of
error, or, even better, don't call this function with invalid
arguments. Note that in the kernel, the rule is that you expect other
kernel code to be written correctly, and user space code to call you
with any combination of invalid arguments.
Arnd <><
^ permalink raw reply
* [PATCH 0/3] Patches to support QE USB Host Controller
From: Anton Vorontsov @ 2008-08-08 16:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-usb
Cc: linuxppc-dev, David Brownell, Li Yang, linux-kernel, Timur Tabi
Hi all,
Most patches that were needed to support QE USB Host were merged during
2.6.27 merge window, and only three more patches left over. Here they
are.
David, could you bear with gpio_to_chip() exported function, just as
a stopgap for a proper api?
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox