* 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
* "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: 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
* 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: [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
* 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 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
* 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: [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: [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: [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: 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: 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: [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: [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 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: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: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: 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 17:42 UTC (permalink / raw)
To: Willy Tarreau; +Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml
In-Reply-To: <20080216173315.GU8953@1wt.eu>
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 ;)
--
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: Willy Tarreau @ 2008-02-16 17:33 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: linuxppc-dev, Roel Kluin, cbe-oss-dev, lkml
In-Reply-To: <20080216092552.325e5726@laptopd505.fenrus.org>
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.
Willy
^ permalink raw reply
* [PATCH]2.6.25-rc2-mm1 - Build Failure at security/keys/compat.c on powerpc
From: Kamalesh Babulal @ 2008-02-16 17:31 UTC (permalink / raw)
To: Andrew Morton; +Cc: linuxppc-dev, sds, linux-kernel
In-Reply-To: <20080216002522.9c4bd0fb.akpm@linux-foundation.org>
Hi Andrew,
The 2.6.25-rc2-mm1 kernel build fails on the powerpc(s)
CC security/keys/compat.o
security/keys/compat.c: In function ‘compat_sys_keyctl’:
security/keys/compat.c:83: error: implicit declaration of function ‘keyctl_get_security’
make[2]: *** [security/keys/compat.o] Error 1
make[1]: *** [security/keys] Error 2
make: *** [security] Error 2
The keys-add-keyctl-function-to-get-a-security-label.patch is causing
this build failure.
I have tested the patch for the build failure only
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
--
--- linux-2.6.25-rc2/security/keys/internal.h 2008-02-17 05:03:30.000000000 +0530
+++ linux-2.6.25-rc2/security/keys/~internal.h 2008-02-17 05:46:16.000000000 +0530
@@ -155,6 +155,8 @@ extern long keyctl_negate_key(key_serial
extern long keyctl_set_reqkey_keyring(int);
extern long keyctl_set_timeout(key_serial_t, unsigned);
extern long keyctl_assume_authority(key_serial_t);
+extern long keyctl_get_security(key_serial_t keyid, char __user *buffer,
+ size_t buflen);
/*
--
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.
^ permalink raw reply
* Re: [PATCH 1/3] Fix Unlikely(x) == y
From: Arjan van de Ven @ 2008-02-16 17:25 UTC (permalink / raw)
To: Roel Kluin; +Cc: linuxppc-dev, cbe-oss-dev, lkml
In-Reply-To: <47B70A61.9030306@tiscali.nl>
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.
--
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
* [PATCH 1/3] Fix Unlikely(x) == y
From: Roel Kluin @ 2008-02-16 16:08 UTC (permalink / raw)
To: geoffrey.levand; +Cc: linuxppc-dev, cbe-oss-dev, lkml
The patch below was not yet tested. If it's correct as it is, please comment.
---
Fix Unlikely(x) == y
Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/arch/powerpc/platforms/ps3/interrupt.c b/arch/powerpc/platforms/ps3/interrupt.c
index 3a6db04..a14e5cd 100644
--- a/arch/powerpc/platforms/ps3/interrupt.c
+++ b/arch/powerpc/platforms/ps3/interrupt.c
@@ -709,7 +709,7 @@ static unsigned int ps3_get_irq(void)
asm volatile("cntlzd %0,%1" : "=r" (plug) : "r" (x));
plug &= 0x3f;
- if (unlikely(plug) == NO_IRQ) {
+ if (unlikely(plug == NO_IRQ)) {
pr_debug("%s:%d: no plug found: thread_id %lu\n", __func__,
__LINE__, pd->thread_id);
dump_bmp(&per_cpu(ps3_private, 0));
^ permalink raw reply related
* Re: [BUID_FAILURE] next-20080215 Build failure caused by ide: rework PowerMac media-bay support
From: Bartlomiej Zolnierkiewicz @ 2008-02-16 14:57 UTC (permalink / raw)
To: Kamalesh Babulal; +Cc: linuxppc-dev, linux-kernel, linux-ide, Andrew Morton
In-Reply-To: <47B5D1C3.40702@linux.vnet.ibm.com>
On Friday 15 February 2008, Kamalesh Babulal wrote:
> The linux-next-20080215 kernel build fails on the powerpc with
> following error
>
> CC arch/powerpc/platforms/powermac/setup.o
> In file included from arch/powerpc/platforms/powermac/setup.c:66:
> include/asm/mediabay.h:29: error: syntax error before 'ide_hwif_t'
> include/asm/mediabay.h:29: warning: function declaration isn't a prototype
> make[2]: *** [arch/powerpc/platforms/powermac/setup.o] Error 1
> make[1]: *** [arch/powerpc/platforms/powermac] Error 2
> make: *** [arch/powerpc/platforms] Error 2
>
> This build failure is caused by the ide: rework PowerMac media-bay support
> patch.This problem was reported and solution suggested according to
> http://lkml.org/lkml/2008/2/13/195 including the
>
> #include <linux/ide.h> after
>
> #ifdef CONFIG_BLK_DEV_IDE_PMAC
>
> helps in fixing the build failure.
Thanks, I fixed the original patch.
interdiff:
[...]
v2:
* Fix build by adding <linux/ide.h> include to <asm-powerpc/mediabay.h>.
(Reported by Michael/Kamalesh/Andrew).
Cc: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Cc: Michael Ellerman <michael@ellerman.id.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
[...]
diff -u b/include/asm-powerpc/mediabay.h b/include/asm-powerpc/mediabay.h
--- b/include/asm-powerpc/mediabay.h
+++ b/include/asm-powerpc/mediabay.h
@@ -23,6 +23,8 @@
extern int media_bay_count;
#ifdef CONFIG_BLK_DEV_IDE_PMAC
+#include <linux/ide.h>
+
int check_media_bay_by_base(unsigned long base, int what);
/* called by IDE PMAC host driver to register IDE controller for media bay */
int media_bay_set_ide_infos(struct device_node *which_bay, unsigned long base,
^ 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