* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Willy Tarreau @ 2008-02-16 17:58 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml
In-Reply-To: <20080216094226.1e8eede1@laptopd505.fenrus.org>
On Sat, Feb 16, 2008 at 09:42:26AM -0800, Arjan van de Ven wrote:
> On Sat, 16 Feb 2008 18:33:16 +0100
> Willy Tarreau <w@1wt.eu> wrote:
>
> > On Sat, Feb 16, 2008 at 09:25:52AM -0800, Arjan van de Ven wrote:
> > > On Sat, 16 Feb 2008 17:08:01 +0100
> > > Roel Kluin <12o3l@tiscali.nl> wrote:
> > >
> > > > The patch below was not yet tested. If it's correct as it is,
> > > > please comment. ---
> > > > Fix Unlikely(x) == y
> > > >
> > >
> > > you found a great set of bugs..
> > > but to be honest... I suspect it's just best to remove unlikely
> > > altogether for these cases; unlikely() is almost a
> > > go-faster-stripes thing, and if you don't know how to use it you
> > > shouldn't be using it... so just removing it for all wrong cases is
> > > actually the best thing to do imo.
> >
> > Well, eventhough the author may not know how to use it, "unlikely" at
> > least indicates the intention of the author, or his knowledge of what
> > should happen here. I'd suggest leaving it where it is because the
> > authot of this code is in best position to know that this branch is
> > unlikely to happen, eventhough he does not correctly use the macro.
> >
>
> you have more faith in the authors knowledge of how his code actually behaves than I think is warranted :)
> Or faith in that he knows what "unlikely" means.
> I should write docs about this; but unlikely() means:
> 1) It happens less than 0.01% of the cases.
> 2) The compiler couldn't have figured this out by itself
> (NULL pointer checks are compiler done already, same for some other conditions)
> 3) It's a hot codepath where shaving 0.5 cycles (less even on x86) matters
> (and the author is ok with taking a 500 cycles hit if he's wrong)
>
> If you think unlikely() means something else, we should fix what it maps to towards gcc ;)
> (to.. be empty ;)
eventhough the gcc docs say it's just a hint to help the compiler optimize
the branch it takes by default, I too have noticed that it more often does
bad than good. Code gets completely reordered and even sometimes partially
duplicated (especially when the branch is a return).
Last but not least, gcc 4 tends to emit stupid checks, to the point that I
have replaced unlikely(x) with (x) in my code when gcc >= 4 is detected. What
I observe is that the following code :
if (unlikely(p == NULL)) ...
often gets coded like this :
reg1 = (p == NULL)
if (reg1 != 0) ...
... which clobbers reg1 for nothing and performs a double test.
But yes, I assumed that the author considered its use to be legitimate (I've
not looked at the code). Maybe you're right and it should be removed, but in
this case we would need a large audit of the abuses of unlikely()...
Willy
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Arjan van de Ven @ 2008-02-16 18:29 UTC (permalink / raw)
To: Willy Tarreau; +Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml
In-Reply-To: <20080216175849.GA25636@1wt.eu>
On Sat, 16 Feb 2008 18:58:49 +0100
> > If you think unlikely() means something else, we should fix what it
> > maps to towards gcc ;) (to.. be empty ;)
>
> eventhough the gcc docs say it's just a hint to help the compiler
> optimize the branch it takes by default, I too have noticed that it
> more often does bad than good. Code gets completely reordered and
> even sometimes partially duplicated (especially when the branch is a
> return).
>
> Last but not least, gcc 4 tends to emit stupid checks, to the point
> that I have replaced unlikely(x) with (x) in my code when gcc >= 4 is
> detected. What I observe is that the following code :
>
> if (unlikely(p == NULL)) ...
this is pure bad since GCC already assumes that NULL checks are unlikely,
and with the unlikely() code needing to normalize stuff... it will generate
worse code for sure yes.
>
> often gets coded like this :
>
> reg1 = (p == NULL)
> if (reg1 != 0) ...
>
> ... which clobbers reg1 for nothing and performs a double test.
>
> But yes, I assumed that the author considered its use to be
> legitimate (I've not looked at the code). Maybe you're right and it
> should be removed, but in this case we would need a large audit of
> the abuses of unlikely()...
no argument.. how about we start with all the cases where the author just got it
entirely wrong ... like the ones from this patch ;)
--
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Geoff Levand @ 2008-02-16 18:31 UTC (permalink / raw)
To: Arjan van de Ven
Cc: linuxppc-dev, Roel Kluin, Willy Tarreau, lkml, cbe-oss-dev
In-Reply-To: <20080216094226.1e8eede1@laptopd505.fenrus.org>
On 02/16/2008 09:42 AM, Arjan van de Ven wrote:
> On Sat, 16 Feb 2008 18:33:16 +0100
> Willy Tarreau <w@1wt.eu> wrote:
>
>> On Sat, Feb 16, 2008 at 09:25:52AM -0800, Arjan van de Ven wrote:
>> > On Sat, 16 Feb 2008 17:08:01 +0100
>> > Roel Kluin <12o3l@tiscali.nl> wrote:
>> >
>> > > The patch below was not yet tested. If it's correct as it is,
>> > > please comment. ---
>> > > Fix Unlikely(x) == y
>> > >
>> >
>> > you found a great set of bugs..
>> > but to be honest... I suspect it's just best to remove unlikely
>> > altogether for these cases; unlikely() is almost a
>> > go-faster-stripes thing, and if you don't know how to use it you
>> > shouldn't be using it... so just removing it for all wrong cases is
>> > actually the best thing to do imo.
>>
>> Well, eventhough the author may not know how to use it, "unlikely" at
>> least indicates the intention of the author, or his knowledge of what
>> should happen here. I'd suggest leaving it where it is because the
>> authot of this code is in best position to know that this branch is
>> unlikely to happen, eventhough he does not correctly use the macro.
>>
>
> you have more faith in the authors knowledge of how his code actually behaves than I think is warranted :)
> Or faith in that he knows what "unlikely" means.
> I should write docs about this; but unlikely() means:
> 1) It happens less than 0.01% of the cases.
> 2) The compiler couldn't have figured this out by itself
> (NULL pointer checks are compiler done already, same for some other conditions)
> 3) It's a hot codepath where shaving 0.5 cycles (less even on x86) matters
> (and the author is ok with taking a 500 cycles hit if he's wrong)
>
> If you think unlikely() means something else, we should fix what it maps to towards gcc ;)
> (to.. be empty ;)
Well, I didn't consider what today's compiler does, but used it as a general
indicator, because I think that code will be around a long time. If you show
me some test results that prove it causes harm I might consider removing it.
-Geoff
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Arjan van de Ven @ 2008-02-16 18:39 UTC (permalink / raw)
To: Geoff Levand; +Cc: linuxppc-dev, Roel Kluin, Willy Tarreau, lkml, cbe-oss-dev
In-Reply-To: <47B72BFE.9060302@am.sony.com>
On Sat, 16 Feb 2008 10:31:26 -0800
Geoff Levand <geoffrey.levand@am.sony.com> wrote:
> On 02/16/2008 09:42 AM, Arjan van de Ven wrote:
> > On Sat, 16 Feb 2008 18:33:16 +0100
> > Willy Tarreau <w@1wt.eu> wrote:
> >
> >> On Sat, Feb 16, 2008 at 09:25:52AM -0800, Arjan van de Ven wrote:
> >> > On Sat, 16 Feb 2008 17:08:01 +0100
> >> > Roel Kluin <12o3l@tiscali.nl> wrote:
> >> >
> >> > > The patch below was not yet tested. If it's correct as it is,
> >> > > please comment. ---
> >> > > Fix Unlikely(x) == y
> >> > >
> >> >
> >> > you found a great set of bugs..
> >> > but to be honest... I suspect it's just best to remove unlikely
> >> > altogether for these cases; unlikely() is almost a
> >> > go-faster-stripes thing, and if you don't know how to use it you
> >> > shouldn't be using it... so just removing it for all wrong cases
> >> > is actually the best thing to do imo.
> >>
> >> Well, eventhough the author may not know how to use it, "unlikely"
> >> at least indicates the intention of the author, or his knowledge
> >> of what should happen here. I'd suggest leaving it where it is
> >> because the authot of this code is in best position to know that
> >> this branch is unlikely to happen, eventhough he does not
> >> correctly use the macro.
> >>
> >
> > you have more faith in the authors knowledge of how his code
> > actually behaves than I think is warranted :) Or faith in that he
> > knows what "unlikely" means. I should write docs about this; but
> > unlikely() means: 1) It happens less than 0.01% of the cases.
> > 2) The compiler couldn't have figured this out by itself
> > (NULL pointer checks are compiler done already, same for some
> > other conditions) 3) It's a hot codepath where shaving 0.5 cycles
> > (less even on x86) matters (and the author is ok with taking a 500
> > cycles hit if he's wrong)
> >
> > If you think unlikely() means something else, we should fix what it
> > maps to towards gcc ;) (to.. be empty ;)
>
> Well, I didn't consider what today's compiler does, but used it as a
> general indicator, because I think that code will be around a long
> time. If you show me some test results that prove it causes harm I
> might consider removing it.
for mordern (last 10 years) x86 cpus... the cpu branchpredictor is better
than the coder in general. Same for most other architectures.
unlikely() creates bigger code as well.
Now... we're talking about your super duper hotpath function here right?
One where you care about 0.5 cycle speed improvement? (less on modern
systems ;)
Because if not, the bigger code and general "compiler second guessing" is just
more harmful than good, shown already here by the fact that the code was just incorrect
as a virtue of having the unlikely() in.
--
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Geoff Levand @ 2008-02-16 18:41 UTC (permalink / raw)
To: Roel Kluin; +Cc: linuxppc-dev, cbe-oss-dev, lkml
In-Reply-To: <47B70A61.9030306@tiscali.nl>
On 02/16/2008 08:08 AM, Roel Kluin wrote:
> The patch below was not yet tested. If it's correct as it is, please comment.
> - if (unlikely(plug) == NO_IRQ) {
> + if (unlikely(plug == NO_IRQ)) {
A good catch! I'll put it in with some other 2.6.25 bug fixes.
-Geoff
^ permalink raw reply
* Re: [PATCH 2/2] i2c-ibm_iic driver
From: Sean MacLennan @ 2008-02-16 20:54 UTC (permalink / raw)
To: Jean Delvare; +Cc: LinuxPPC-dev, i2c
In-Reply-To: <20080216103123.50a0128b@hyperion.delvare>
Jean Delvare wrote:
> Hi Sean,
>
> On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
>
>> Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
>>
>> Cheers,
>> Sean
>>
>> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
>>
>
> First of all: please run scripts/checkpatch.pl on your patch and fix
> the reported errors. It tells me:
> total: 10 errors, 5 warnings, 222 lines checked
> which is definitely too much.
>
Many of the errors/warnings are from the cut and paste of the original
code. I realize this doesn't justify my new code, but does beg the
question; should I also cleanup the original code? And how much? I am
tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.
A few comments, everything else I agree with.
>> + return -ENOMEM;
>> + }
>> +
>> + /* This assumes we don't mix index and non-index entries. */
>> + indexp = of_get_property(np, "index", NULL);
>> + dev->idx = indexp ? *indexp : index++;
>>
>
> I don't like this static index thing much. Can't you just make the
> "index" OF property mandatory? Mixing ways to number things can become
> very confusing. In particular as you are using dev->idx later to call
> i2c_add_numbered_adapter(), the caller is really supposed to know what
> they are doing with the bus numbers.
>
I also don't really like mixing the two methods, hence the comment. I
did this so existing dts files, which don't currently have an index,
would work as is. Maybe it would be better to add the index?
Without my patch, or one like it, you cannot currently use the IBM IIC
in the arch/powerpc branch, so maybe now *is* the time to enforce an index?
So if nobody responds with a good reason not to, I will change the code
to require the index to be set. Less testing for me ;)
>> + adap->timeout = 1;
>> + adap->nr = dev->idx;
>>
>
> Looks to me like the block above has much in common with the original
> probe function, maybe it would be worth sharing the code to make future
> maintenance easier?
>
It is my understanding the the arch/ppc branch is going away in the
middle of the year. So I kept the of platform code separate and clean
expecting the CONFIG_IBM_OCP block to be removed in the near future. So
future maintenance should not be an issue.
> Note that I cannot test the code so I am relying on the linuxppc-dev
> folks to test it
I can test the of platform code and the common code. I am not setup to
test the OCP code, which is why I am loath to change it. I use the IBM
IIC for access to an ad7414 thermal chip (currently not in the kernel,
I am working on that too) and for reading an eeprom.
Cheers,
Sean
^ permalink raw reply
* Re: Frustrated question with insmod
From: Bruce_Leonard @ 2008-02-17 8:03 UTC (permalink / raw)
To: Magnus Hjorth; +Cc: linuxppc-embedded
In-Reply-To: <1203136939.5231.0.camel@magnus-desktop>
>
> 'cat /proc/modules' perhaps?
I tried that, but it gives me an odd address (at least I think it's an odd
address): 0xE1188000. I use that address in GDB for adding the symbol
table (i.e., add-symbol-file mymodule 0xE1188000), but then the BDI
reports "*** MMU: address translation for 0xE118822C failed" when I try to
set a breakpoint in the probe function. Admittedly I'm new to driver
writing, but shouldn't the address be somewhere in the 0xC0xxxxxx range?
>
> //Magnus
>
>
> On Fri, 2008-02-15 at 17:06 -0800, Bruce_Leonard@selinc.com wrote:
> > Sorry if this is the wrong place to post this question. I'm
developing a
> > NAND flash driver and I need to do some detailed dubugging using GDB
with
> > a BDI2K. According to the Denx web site, to find out the address that
the
> > module is loading at you load it using the -m parameter to insmod
(i.e.,
> > "insmod -m mymodule"). However, every version of insmod I've tried
> > doesn't recognize ANY options much less -m. Can anyone please point
me in
> > the right direction, or give me another way of knowing what the load
> > address of my module is?
> >
> > Thanks.
> >
> > Bruce
> > _______________________________________________
> > Linuxppc-embedded mailing list
> > Linuxppc-embedded@ozlabs.org
> > https://ozlabs.org/mailman/listinfo/linuxppc-embedded
>
^ permalink raw reply
* Re: APU FPU in Virtex ppc405
From: Thomas Werne @ 2008-02-17 7:34 UTC (permalink / raw)
To: linuxppc-embedded
In-Reply-To: <47B02CB0.8040002@gmail.com>
A. Nolson <alohanono <at> gmail.com> writes:
>
> I have been checking and it seems that the APU FPU patches will give
> some headaches now, so I will probably wait until they merge them with
> the official GCC release. In any case, it seems that the FPU restricts
> the PowerPC and bus frequencies to a max of 200/100.
>
> Anyway, thanks for the info. I will try to keep track of this in case of
> an update.
>
Hi -
You can get the FPU to work with the 2.6.24rc3 Linux (I got it to work just
yesterday). There are a couple of small changes you have to make to kernel
config files and to one of the .h files, plus you need to compile with the
-fshort-double flag to convert all doubles into floats. Let me know if you're
still interested and I can post details and/or patches.
Thomas
--
Thomas Werne
NASA Jet Propulsion Laboratory
All personal and professional opinions presented herein are my
own and do not, in any way, represent the opinion or policy of JPL.
^ permalink raw reply
* Re: [Linux-fbdev-devel] [PATCH 1/2] fb: add support for foreign endianness
From: Geert Uytterhoeven @ 2008-02-17 9:44 UTC (permalink / raw)
To: avorontsov, linux-fbdev-devel
Cc: linuxppc-dev, Andrew Morton, linux-kernel, adaplas
In-Reply-To: <20080215164542.GB16810@localhost.localdomain>
On Fri, 15 Feb 2008, Anton Vorontsov wrote:
> On Thu, Feb 14, 2008 at 10:49:42PM -0800, Andrew Morton wrote:
> > On Tue, 5 Feb 2008 18:44:32 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > > This patch adds support for the framebuffers with non-native
> > > endianness. This is done via FBINFO_FOREIGN_ENDIAN flag that will
> > > be used by the drivers. Depending on the host endianness this flag
> > > will be overwritten by FBINFO_BE_MATH internal flag, or cleared.
> > >
> > > Tested to work on MPC8360E-RDK (BE) + Fujitsu MINT framebuffer (LE).
> >
> > That's a pretty large patch to fbdev core,
>
> Yes, but changes are mostly trivial. No insurance of typos and thinkos
> though. OTOH, it survived my tests.
>
> > and Tony appears to have gone
> > offline again and you didn't cc the fbdev mailing list.
>
> FRAMEBUFFER LAYER
> P: Antonino Daplas
> M: adaplas@gmail.com
> L: linux-fbdev-devel@lists.sourceforge.net (subscribers-only)
>
> I'm lazy to subscribe on occasional patches. Yup, this is hardly
> an excuse...
While linux-fbdev is subscribers-only, non-subscribers are not plainly
rejected, but moderated, so the casual patch/comment/question comes
through.
I'll cook up a patch for MAINTAINERS to clarify.
> > Actually... should CONFIG_FB_FOREIGN_ENDIAN exist, or should this feature
> > be permanently enabled?
>
> It should not. Because with CONFIG_FB_FOREIGN_ENDIAN enabled, drawing
> might be a little bit slower, because of external checks for endianness
> (I didn't noticed any slowdown, but I guess it's still measurable if we
> find some fb benchmark).
> > I'd like to at least queue a patch in -mm so that CONFIG_FB_FOREIGN_ENDIAN
> > is forced-on, so the code gets some runtime testing. Will that break
> > anything?
>
> Nope, it should not break anything, just possible fbconsole/tux drawing
> slowdown.
Tux drawing speed doesn't really matter, as it's done once only.
Console text drawing is different.
`time cat MAINTAINERS' is a good test. Of course, is most time is spent
in scrolling, you won't see much difference.
So probably we need a test file that contains lines of text interspersed
with `HOME' escape sequences, so we don't scroll?
> Thinking about it a little bit more, I believe we want to add a choice,
> which exactly foreign endianness we want to compile-in. Then, if we're
> pretty confident that particular BE machine uses only LE framebuffer,
> gcc can optimize away endianness checks.
>
> Patch on top of previous inlined here. With that patch, slowdown is
> possible with CONFIG_FB_BOTH_ENDIAN option only, which is still
> default if FOREIGN_ENDIAN is selected.
> With this patch FB_FOREIGN_ENDIAN converted to menuconfig with the
> choice inside: which type of foreign endianness we want to compile-in.
>
The notion of `FOREIGN_ENDIAN' is relative, as it depends on the
architecture you're compiling for.
Suppose you have a PCI graphics card with a frame buffer that's always
big endian. When compiling for a big endian platform, the driver won't
depend on FB_FOREIGN_ENDIAN. When compiling for a little endian
platform, it will.
Shouldn't we add LITTLE_ENDIAN and BIG_ENDIAN Kconfig vars first, just
like we have 64BIT?
> As a bonus, now fb subsystem will refuse to register framebuffer with
> unsupported endianness, and will suggest a way to solve the problem.
I'd like to handle this in Kconfig (cfr. above).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [Cbe-oss-dev] [PATCH 1/3] Fix Unlikely(x) == y
From: Andrew Pinski @ 2008-02-17 9:45 UTC (permalink / raw)
To: Willy Tarreau
Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml, Arjan van de Ven
In-Reply-To: <20080216175849.GA25636@1wt.eu>
On Feb 16, 2008 9:58 AM, Willy Tarreau <w@1wt.eu> wrote:
> Last but not least, gcc 4 tends to emit stupid checks, to the point that I
> have replaced unlikely(x) with (x) in my code when gcc >= 4 is detected. What
> I observe is that the following code :
>
> if (unlikely(p == NULL)) ...
>
> often gets coded like this :
>
> reg1 = (p == NULL)
> if (reg1 != 0) ...
>
> ... which clobbers reg1 for nothing and performs a double test.
This really only can happen in GCC 4.0.x and 4.1.x and cannot happen
for 4.2 or 4.3 really because of the way __builtin_expect is handled
for those two.
-- Pinski
^ permalink raw reply
* Re: [Cbe-oss-dev] [PATCH 1/3] Fix Unlikely(x) == y
From: Willy Tarreau @ 2008-02-17 10:08 UTC (permalink / raw)
To: Andrew Pinski
Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml, Arjan van de Ven
In-Reply-To: <de8d50360802170145u4bd60f76x9662b66c63bdf41d@mail.gmail.com>
On Sun, Feb 17, 2008 at 01:45:23AM -0800, Andrew Pinski wrote:
> On Feb 16, 2008 9:58 AM, Willy Tarreau <w@1wt.eu> wrote:
> > Last but not least, gcc 4 tends to emit stupid checks, to the point that I
> > have replaced unlikely(x) with (x) in my code when gcc >= 4 is detected. What
> > I observe is that the following code :
> >
> > if (unlikely(p == NULL)) ...
> >
> > often gets coded like this :
> >
> > reg1 = (p == NULL)
> > if (reg1 != 0) ...
> >
> > ... which clobbers reg1 for nothing and performs a double test.
>
> This really only can happen in GCC 4.0.x and 4.1.x and cannot happen
> for 4.2 or 4.3 really because of the way __builtin_expect is handled
> for those two.
Happy to know that, thanks for the info Andrew!
Willy
^ permalink raw reply
* Re: Could the DTS experts look at this?
From: David Woodhouse @ 2008-02-17 10:22 UTC (permalink / raw)
To: Scott Wood; +Cc: LinuxPPC-dev, Sean MacLennan
In-Reply-To: <47B1EF39.5000703@freescale.com>
On Tue, 2008-02-12 at 13:10 -0600, Scott Wood wrote:
> Hmm? All I meant was that it'd be nice if there were an option in the
> Linux mtd code to disable the "look for another chip and cause a machine
> check if it isn't there" functionality. It was an aside from the
> dts-variant issue.
Yeah, maybe -- although the probe code is hairy enough already; I'm not
massively keen on adding to it. Or even looking at it hard, if the truth
be told.
Alternatively, perhaps we should catch the machine check and recover
(like we do for failing I/O accesses on ppc32).
--
dwmw2
^ permalink raw reply
* Re: [PATCH 2/2] i2c-ibm_iic driver
From: Jean Delvare @ 2008-02-17 10:52 UTC (permalink / raw)
To: Sean MacLennan; +Cc: LinuxPPC-dev, i2c
In-Reply-To: <47B74D76.1000708@pikatech.com>
Hi Sean,
On Sat, 16 Feb 2008 15:54:14 -0500, Sean MacLennan wrote:
> Jean Delvare wrote:
> > Hi Sean,
> >
> > On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
> >
> >> Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
> >>
> >> Cheers,
> >> Sean
> >>
> >> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> >>
> >
> > First of all: please run scripts/checkpatch.pl on your patch and fix
> > the reported errors. It tells me:
> > total: 10 errors, 5 warnings, 222 lines checked
> > which is definitely too much.
> >
> Many of the errors/warnings are from the cut and paste of the original
> code. I realize this doesn't justify my new code, but does beg the
> question; should I also cleanup the original code? And how much? I am
> tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.
Just take care of the code you are adding. The old code will go away
soon anyway as I understand it, so don't bother cleaning it up.
>
> A few comments, everything else I agree with.
>
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + /* This assumes we don't mix index and non-index entries. */
> >> + indexp = of_get_property(np, "index", NULL);
> >> + dev->idx = indexp ? *indexp : index++;
> >>
> >
> > I don't like this static index thing much. Can't you just make the
> > "index" OF property mandatory? Mixing ways to number things can become
> > very confusing. In particular as you are using dev->idx later to call
> > i2c_add_numbered_adapter(), the caller is really supposed to know what
> > they are doing with the bus numbers.
> >
> I also don't really like mixing the two methods, hence the comment. I
> did this so existing dts files, which don't currently have an index,
> would work as is. Maybe it would be better to add the index?
>
> Without my patch, or one like it, you cannot currently use the IBM IIC
> in the arch/powerpc branch, so maybe now *is* the time to enforce an index?
>
> So if nobody responds with a good reason not to, I will change the code
> to require the index to be set. Less testing for me ;)
Yes, I think this is the right time to enforce the index.
> >> + adap->timeout = 1;
> >> + adap->nr = dev->idx;
> >>
> >
> > Looks to me like the block above has much in common with the original
> > probe function, maybe it would be worth sharing the code to make future
> > maintenance easier?
> >
> It is my understanding the the arch/ppc branch is going away in the
> middle of the year. So I kept the of platform code separate and clean
> expecting the CONFIG_IBM_OCP block to be removed in the near future. So
> future maintenance should not be an issue.
OK, fine with me then.
> > Note that I cannot test the code so I am relying on the linuxppc-dev
> > folks to test it
> I can test the of platform code and the common code. I am not setup to
> test the OCP code, which is why I am loath to change it. I use the IBM
> IIC for access to an ad7414 thermal chip (currently not in the kernel,
> I am working on that too) and for reading an eeprom.
Note that Frank Edelhaeuser has submitted a driver for this chip
recently:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-February/022478.html
I didn't have time to review this patch yet, maybe you will want to do
it yourself.
--
Jean Delvare
^ permalink raw reply
* Sequoia NAND
From: Steve Heflin @ 2008-02-17 11:39 UTC (permalink / raw)
To: linuxppc-embedded
I don't see where the new powerpc architecture and drivers/mtd/nand
contains support for the NAND chip on the Sequoia platform. We have
a Linux 2.6.14 kit where Montavista has a driver in "sequoia_nand.c"
which interfaces with the NAND device via NAND Flash Control registers:
struct ppc440ep_ndfc_regs *sequoia_ndfc;
where:
struct ppc440ep_ndfc_regs {
uint cmd;
uint addr;
uint data;
uint reserved1;
uint ecc0;
uint ecc1;
uint ecc2;
uint ecc3;
uint ecc4;
uint ecc5;
uint ecc6;
uint ecc7;
uint b0cr;
uint b1cr;
uint b2cr;
uint b3cr;
uint cr;
uint sr;
uint hwctl;
uint reserved2;
uint revid;
};
Is the Sequoia's NAND Flash Controller supported in the current Linux-2.6.25?
thanks,
Steve
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Michael Ellerman @ 2008-02-17 11:50 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Roel Kluin, lkml, Willy Tarreau, linuxppc-dev, cbe-oss-dev
In-Reply-To: <20080216103927.2a02352b@laptopd505.fenrus.org>
[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]
On Sat, 2008-02-16 at 10:39 -0800, Arjan van de Ven wrote:
> > >> > you found a great set of bugs..
> > >> > but to be honest... I suspect it's just best to remove unlikely
> > >> > altogether for these cases; unlikely() is almost a
> > >> > go-faster-stripes thing, and if you don't know how to use it you
> > >> > shouldn't be using it... so just removing it for all wrong cases
> > >> > is actually the best thing to do imo.
Hi Arjan,
In general I agree with you that unlikely() is overused, and often in
inappropriate places.
> for mordern (last 10 years) x86 cpus... the cpu branchpredictor is better
> than the coder in general. Same for most other architectures.
>
> unlikely() creates bigger code as well.
>
> Now... we're talking about your super duper hotpath function here right?
> One where you care about 0.5 cycle speed improvement? (less on modern
> systems ;)
The first patch was to platforms/ps3 code, which runs on the Cell, in
particular the PPE ... which is not an x86 :)
eg:
[michael@schoenaich ~]$ cat branch.c
#include <stdio.h>
int main(void)
{
int i, j;
for (i = 0, j = 0; i < 1000000000; i++)
if (i % 4 == 0)
j++;
printf("j = %d\n", j);
return 0;
}
[michael@schoenaich ~]$ ppu-gcc -Wall -O3 -o branch branch.c
[michael@schoenaich ~]$ time ./branch
real 0m5.172s
[michael@schoenaich ~]$ cat branch.c
..
for (i = 0, j = 0; i < 1000000000; i++)
if (__builtin_expect(i % 4 == 0, 0))
j++;
..
[michael@schoenaich ~]$ ppu-gcc -Wall -O3 -o branch branch.c
[michael@schoenaich ~]$ time ./branch
real 0m3.762s
Which looks as though unlikely() is helping. Admittedly we don't have a
lot of kernel code that looks like that, but at least unlikely is doing
what we want it to.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Frustrated question with insmod
From: Magnus Hjorth @ 2008-02-17 12:04 UTC (permalink / raw)
To: Bruce_Leonard; +Cc: linuxppc-embedded
In-Reply-To: <OFE029FE92.B8F9D4B0-ON882573F2.002B93E9-882573F2.002C385D@selinc.com>
On Sun, 2008-02-17 at 00:03 -0800, Bruce_Leonard@selinc.com wrote:
> >
> > 'cat /proc/modules' perhaps?
>
> I tried that, but it gives me an odd address (at least I think it's an odd
> address): 0xE1188000. I use that address in GDB for adding the symbol
> table (i.e., add-symbol-file mymodule 0xE1188000), but then the BDI
> reports "*** MMU: address translation for 0xE118822C failed" when I try to
> set a breakpoint in the probe function. Admittedly I'm new to driver
> writing, but shouldn't the address be somewhere in the 0xC0xxxxxx range?
I believe the modules live in the kernel's dynamically allocated memory
area in the 0xExxxxxxx range..
Can you set breakpoints anywhere in the kernel? Try picking a function address from System.map and set a breakpoint there.
I have little experience with what you're doing, but my guess is that you're setting the breakpoint in the wrong context, probably standing in userspace..
>
> >
> > //Magnus
> >
> >
> > On Fri, 2008-02-15 at 17:06 -0800, Bruce_Leonard@selinc.com wrote:
> > > Sorry if this is the wrong place to post this question. I'm
> developing a
> > > NAND flash driver and I need to do some detailed dubugging using GDB
> with
> > > a BDI2K. According to the Denx web site, to find out the address that
> the
> > > module is loading at you load it using the -m parameter to insmod
> (i.e.,
> > > "insmod -m mymodule"). However, every version of insmod I've tried
> > > doesn't recognize ANY options much less -m. Can anyone please point
> me in
> > > the right direction, or give me another way of knowing what the load
> > > address of my module is?
> > >
> > > Thanks.
> > >
> > > Bruce
> > > _______________________________________________
> > > Linuxppc-embedded mailing list
> > > Linuxppc-embedded@ozlabs.org
> > > https://ozlabs.org/mailman/listinfo/linuxppc-embedded
> >
^ permalink raw reply
* Re: Frustrated question with insmod
From: Wolfgang Grandegger @ 2008-02-17 12:28 UTC (permalink / raw)
To: Bruce_Leonard; +Cc: linuxppc-embedded
In-Reply-To: <OF994170A6.977C29F1-ON882573F1.0005A6C7-882573F1.00060311@selinc.com>
Bruce_Leonard@selinc.com wrote:
> Sorry if this is the wrong place to post this question. I'm developing a
> NAND flash driver and I need to do some detailed dubugging using GDB with
> a BDI2K. According to the Denx web site, to find out the address that the
> module is loading at you load it using the -m parameter to insmod (i.e.,
> "insmod -m mymodule"). However, every version of insmod I've tried
> doesn't recognize ANY options much less -m. Can anyone please point me in
> the right direction, or give me another way of knowing what the load
> address of my module is?
# cat /sys/module/<name>/sections/.text
Do not forget to enable CONFIG_KALLSYMS.
Wolfgang.
^ permalink raw reply
* "extra-aflags-y"??
From: Robert P. J. Day @ 2008-02-17 10:47 UTC (permalink / raw)
To: Linux PPC Mailing List
something about this doesn't look right:
$ grep -rw extra-aflags-y *
arch/ppc/boot/simple/Makefile:EXTRA_AFLAGS := $(extra-aflags-y)
$
AFAIK, there is no supported "extra-aflags-y" variable. and
EXTRA_AFLAGS is deprecated in favour of asflags-y anyway, no?
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
^ permalink raw reply
* Re: Sequoia NAND
From: Josh Boyer @ 2008-02-17 13:18 UTC (permalink / raw)
To: Steve Heflin; +Cc: linuxppc-embedded
In-Reply-To: <20080217113932.02EBDDE127@ozlabs.org>
On Sun, 17 Feb 2008 06:39:43 -0500
Steve Heflin <sheflin@newagemicro.com> wrote:
> I don't see where the new powerpc architecture and drivers/mtd/nand
> contains support for the NAND chip on the Sequoia platform. We have
It's not.
> Is the Sequoia's NAND Flash Controller supported in the current Linux-2.6.25?
No.
There are a couple different patches floating around for it, all of
which need work. The driver is drivers/mtd/nand/ndfc.c and work needs
to be done to parse a device tree and present the proper platform
devices so that driver will work. Stefan has something like this
somewhere, I've just been lax in getting it into my tree.
Too late for .25, but I have 4 boards that use this now so I'll be a
bit more interested in getting it into .26.
josh
^ permalink raw reply
* Re: Sequoia NAND - others missing?
From: Steve Heflin @ 2008-02-17 15:27 UTC (permalink / raw)
To: linuxppc-embedded, Josh Boyer; +Cc: linuxppc-embedded
In-Reply-To: <20080217071851.0edbf07f@vader.jdub.homelinux.org>
Are there other devices (beside the NAND Flash Controller) that exist
on the AMCC-440EPx chip and are not supported by the current
Linux-2.6.25 ARCH=powerpc?
thanks for your help,
Steve
At 08:18 AM 2/17/2008, Josh Boyer wrote:
>On Sun, 17 Feb 2008 06:39:43 -0500
>Steve Heflin <sheflin@newagemicro.com> wrote:
>
> > I don't see where the new powerpc architecture and drivers/mtd/nand
> > contains support for the NAND chip on the Sequoia platform. We have
>
>It's not.
>
> > Is the Sequoia's NAND Flash Controller supported in the current
> Linux-2.6.25?
>
>No.
>
>There are a couple different patches floating around for it, all of
>which need work. The driver is drivers/mtd/nand/ndfc.c and work needs
>to be done to parse a device tree and present the proper platform
>devices so that driver will work. Stefan has something like this
>somewhere, I've just been lax in getting it into my tree.
>
>Too late for .25, but I have 4 boards that use this now so I'll be a
>bit more interested in getting it into .26.
>
>josh
^ permalink raw reply
* Re: Sequoia NAND - others missing?
From: Steve Heflin @ 2008-02-17 15:27 UTC (permalink / raw)
To: linuxppc-embedded, Josh Boyer; +Cc: linuxppc-embedded
In-Reply-To: <20080217071851.0edbf07f@vader.jdub.homelinux.org>
Are there other devices (beside the NAND Flash Controller) that exist
on the AMCC-440EPx chip and are not supported by the current
Linux-2.6.25 ARCH=powerpc?
thanks for your help,
Steve
At 08:18 AM 2/17/2008, Josh Boyer wrote:
>On Sun, 17 Feb 2008 06:39:43 -0500
>Steve Heflin <sheflin@newagemicro.com> wrote:
>
> > I don't see where the new powerpc architecture and drivers/mtd/nand
> > contains support for the NAND chip on the Sequoia platform. We have
>
>It's not.
>
> > Is the Sequoia's NAND Flash Controller supported in the current
> Linux-2.6.25?
>
>No.
>
>There are a couple different patches floating around for it, all of
>which need work. The driver is drivers/mtd/nand/ndfc.c and work needs
>to be done to parse a device tree and present the proper platform
>devices so that driver will work. Stefan has something like this
>somewhere, I've just been lax in getting it into my tree.
>
>Too late for .25, but I have 4 boards that use this now so I'll be a
>bit more interested in getting it into .26.
>
>josh
^ permalink raw reply
* Re: [BUG] Linux 2.6.25-rc2 - Regression from 2.6.24-rc1-git1 softlockup while bootup on powerpc
From: Jens Axboe @ 2008-02-17 19:29 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: Dhaval Giani, Linux Kernel Mailing List, Srivatsa Vaddagiri,
linuxppc-dev, Ingo Molnar, Balbir Singh
In-Reply-To: <47B67E5E.4010001@linux.vnet.ibm.com>
On Sat, Feb 16 2008, Kamalesh Babulal wrote:
> Hi,
>
> The softlockup is seen from 2.6.25-rc1-git{1,3} and is visible in the 2.6.24-rc2 kernel,
> While booting up with the 2.6.25-rc1-git{1,3} and 2.6.25-rc2 kernel(s) on the powerbox
>
> Loading st.ko module
> BUG: soft lockup - CPU#1 stuck for 61s! [insmod:379]
> NIP: c0000000001b0620 LR: c0000000001a5dcc CTR: 0000000000000040
> REGS: c00000077caab8a0 TRAP: 0901 Not tainted (2.6.25-rc2-autotest)
> MSR: 8000000000009032 <EE,ME,IR,DR> CR: 84004088 XER: 20000000
> TASK = c00000077cb450a0[379] 'insmod' THREAD: c00000077caa8000 CPU: 1
> GPR00: c00000077c9d4000 c00000077caabb20 c000000000538a40 000000000000000b
> GPR04: ffc0000000000000 c00000077e0c0000 0000000000000036 000000000000000a
> GPR08: 0040000000000000 c00000077c9d4250 c000000000000000 0000000000000000
> GPR12: c00000077c9d4230 c000000000481d00
> NIP [c0000000001b0620] .radix_tree_gang_lookup+0x100/0x1e4
> LR [c0000000001a5dcc] .call_for_each_cic+0x50/0x10c
> Call Trace:
> [c00000077caabb20] [c0000000001a5e2c] .call_for_each_cic+0xb0/0x10c (unreliable)
> [c00000077caabc60] [c00000000019dba4] .exit_io_context+0xf0/0x110
> [c00000077caabcf0] [c000000000061e38] .do_exit+0x820/0x850
> [c00000077caabda0] [c000000000061f34] .do_group_exit+0xcc/0xe8
> [c00000077caabe30] [c00000000000872c] syscall_exit+0x0/0x40
> Instruction dump:
> 7d296214 39290018 e8090000 7caa2038 39290008 2fa00000 409e0018 7caa4215
> 396b0001 418200cc 424000b8 4bffffdc <79691f24> 7d296214 e9690018 2fab0000
It's odd stuff. Could you perhaps try and add some printks to
block/cfq-iosched.c:call_for_each_cic(), like dumping the 'nr' return
from radix_tree_gang_lookup() and the pointer value of cics[i] in the
for() loop after the lookup?
How many SCSI devices are online?
--
Jens Axboe
^ permalink raw reply
* Re: [BUG] Linux 2.6.25-rc2 - Regression from 2.6.24-rc1-git1 softlockup while bootup on powerpc
From: Rafael J. Wysocki @ 2008-02-17 20:08 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: Dhaval Giani, Srivatsa Vaddagiri, Linux Kernel Mailing List,
linuxppc-dev, Jens Axboe, Ingo Molnar, Balbir Singh
In-Reply-To: <47B67E5E.4010001@linux.vnet.ibm.com>
On Saturday, 16 of February 2008, Kamalesh Babulal wrote:
> Hi,
Hi,
> The softlockup is seen from 2.6.25-rc1-git{1,3} and is visible in the 2.6.24-rc2 kernel,
> While booting up with the 2.6.25-rc1-git{1,3} and 2.6.25-rc2 kernel(s) on the powerbox
Can you update the Bugzilla entry at:
http://bugzilla.kernel.org/show_bug.cgi?id=9948
with the above information, please?
Rafael
> Loading st.ko module
> BUG: soft lockup - CPU#1 stuck for 61s! [insmod:379]
> NIP: c0000000001b0620 LR: c0000000001a5dcc CTR: 0000000000000040
> REGS: c00000077caab8a0 TRAP: 0901 Not tainted (2.6.25-rc2-autotest)
> MSR: 8000000000009032 <EE,ME,IR,DR> CR: 84004088 XER: 20000000
> TASK = c00000077cb450a0[379] 'insmod' THREAD: c00000077caa8000 CPU: 1
> GPR00: c00000077c9d4000 c00000077caabb20 c000000000538a40 000000000000000b
> GPR04: ffc0000000000000 c00000077e0c0000 0000000000000036 000000000000000a
> GPR08: 0040000000000000 c00000077c9d4250 c000000000000000 0000000000000000
> GPR12: c00000077c9d4230 c000000000481d00
> NIP [c0000000001b0620] .radix_tree_gang_lookup+0x100/0x1e4
> LR [c0000000001a5dcc] .call_for_each_cic+0x50/0x10c
> Call Trace:
> [c00000077caabb20] [c0000000001a5e2c] .call_for_each_cic+0xb0/0x10c (unreliable)
> [c00000077caabc60] [c00000000019dba4] .exit_io_context+0xf0/0x110
> [c00000077caabcf0] [c000000000061e38] .do_exit+0x820/0x850
> [c00000077caabda0] [c000000000061f34] .do_group_exit+0xcc/0xe8
> [c00000077caabe30] [c00000000000872c] syscall_exit+0x0/0x40
> Instruction dump:
> 7d296214 39290018 e8090000 7caa2038 39290008 2fa00000 409e0018 7caa4215
> 396b0001 418200cc 424000b8 4bffffdc <79691f24> 7d296214 e9690018 2fab0000
> INFO: task insmod:387 blocked for more than 120 seconds.
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> insmod D 000000001000e144 12144 387 1
> Call Trace:
> [c00000077cb97600] [c0000000008fae80] 0xc0000000008fae80 (unreliable)
> [c00000077cb977d0] [c000000000010c7c] .__switch_to+0x11c/0x154
> [c00000077cb97860] [c000000000344498] .schedule+0x5d0/0x6b0
> [c00000077cb97950] [c0000000003447d8] .schedule_timeout+0x3c/0xe8
> [c00000077cb97a20] [c000000000343d34] .wait_for_common+0x150/0x22c
> [c00000077cb97ae0] [c00000000008ef00] .__stop_machine_run+0xbc/0xf0
> [c00000077cb97bb0] [c00000000008ef70] .stop_machine_run+0x3c/0x80
> [c00000077cb97c50] [c0000000000891f0] .sys_init_module+0x14e4/0x1af4
> [c00000077cb97e30] [c00000000000872c] syscall_exit+0x0/0x40
> -- 0:conmux-control -- time-stamp -- Feb/15/08 16:04:12 --
> INFO: task insmod:387 blocked for more than 120 seconds.
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> insmod D 000000001000e144 12144 387 1
> Call Trace:
> [c00000077cb97600] [c0000000008fae80] 0xc0000000008fae80 (unreliable)
> [c00000077cb977d0] [c000000000010c7c] .__switch_to+0x11c/0x154
> [c00000077cb97860] [c000000000344498] .schedule+0x5d0/0x6b0
> [c00000077cb97950] [c0000000003447d8] .schedule_timeout+0x3c/0xe8
> [c00000077cb97a20] [c000000000343d34] .wait_for_common+0x150/0x22c
> [c00000077cb97ae0] [c00000000008ef00] .__stop_machine_run+0xbc/0xf0
> [c00000077cb97bb0] [c00000000008ef70] .stop_machine_run+0x3c/0x80
> [c00000077cb97c50] [c0000000000891f0] .sys_init_module+0x14e4/0x1af4
> [c00000077cb97e30] [c00000000000872c] syscall_exit+0x0/0x40
> -- 0:conmux-control -- time-stamp -- Feb/15/08 16:06:21 --
--
"Premature optimization is the root of all evil." - Donald Knuth
^ permalink raw reply
* Re: Sequoia NAND - others missing?
From: Josh Boyer @ 2008-02-17 20:56 UTC (permalink / raw)
To: Steve Heflin; +Cc: linuxppc-embedded
In-Reply-To: <47b8524c.1286460a.1307.7530SMTPIN_ADDED@mx.google.com>
On Sun, 17 Feb 2008 10:27:22 -0500
Steve Heflin <sheflin@newagemicro.com> wrote:
> Are there other devices (beside the NAND Flash Controller) that exist
> on the AMCC-440EPx chip and are not supported by the current
> Linux-2.6.25 ARCH=powerpc?
i2c, GPIO, the security stuff (if your version has that), and GPT.
Thought GPT has never really been supported in any kernel that I
remember.
Patches for i2c and GPIO are floating around somewhere I think. Just
need to get them polished up and device-tree compliant.
josh
^ permalink raw reply
* Does anyone have a simple UIO driver that uses an interrupt handler?
From: Nick Droogh @ 2008-02-17 23:13 UTC (permalink / raw)
To: linuxppc-embedded
[-- Attachment #1: Type: text/plain, Size: 244 bytes --]
Hi everyone,
I am looking for a driver example for a user space driver utilizing an
interrupt handler. I am having trouble registering a handler in my
driver attempt and would like to see an example of a working UIO driver.
Thanks,
Nick
[-- Attachment #2: ndroogh.vcf --]
[-- Type: text/x-vcard, Size: 220 bytes --]
begin:vcard
fn:Nick Droogh
n:Droogh;Nick
org:CADlink Technology
adr:Suite 100;;2440 Don Reid Drive;Ottawa;Ontario;K1H 1E1;Canada
email;internet:ndroogh@cadlink.com
title:Senior Technical Architect
version:2.1
end:vcard
^ 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