* Re: [PATCH] Hwmon for Taco
From: Sean MacLennan @ 2008-01-05 18:39 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
In-Reply-To: <1199517763.7291.47.camel@pasglop>
Benjamin Herrenschmidt wrote:
>> +module_exit(ad7414_exit);
>> --- /dev/null 2005-11-20 22:22:37.000000000 -0500
>> +++ drivers/hwmon/taco-dtm.c 2008-01-05 00:06:21.000000000 -0500
>> @@ -0,0 +1,78 @@
>> +/*
>> + * drivers/hwmon/taco-dtm.c
>> + *
>> + * Overview: On the Taco, the fpga controls the fan. This provides
>> + * the temperature to the fpga.
>> + *
>> + * Copyright (c) 2008 PIKA Technologies
>> + * Sean MacLennan <smaclennan@pikatech.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kthread.h>
>> +#include <linux/io.h>
>> +
>> +
>> +#define TACO_FPGA_BASE 0x80000000
>>
>
> That should be in the device-tree...
>
> Cheers,
> Ben.
>
>
>
It is in the device tree, I just forgot to update the driver to use it.
Thanks.
Cheers,
Sean
^ permalink raw reply
* Re: [PATCH] i2c-ibm_iic driver
From: Arnd Bergmann @ 2008-01-05 19:18 UTC (permalink / raw)
To: Sean MacLennan; +Cc: linuxppc-dev, Stefan Roese
In-Reply-To: <477FCE21.9080808@pikatech.com>
On Saturday 05 January 2008, Sean MacLennan wrote:
>
> Ok. The 44x based .dts files do not list 405-iic, so would I think I
> will add two compatibility matches, one for 405 and one for 440EP. That
> way I do not break all the current .dts files. Everybody ok with that?
>
Sounds good. There are obviously no other drivers that know only
about 405 but not about 440, so there is no backwards compatibility
problem. If we ever get a 450/460/470/... that we want to support
with this driver, it can simply claim to be compatible with 440 or 405
if not both.
Arnd <><
^ permalink raw reply
* Re: [PATCH] MTD for Taco
From: Arnd Bergmann @ 2008-01-05 19:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Stefan Roese, Sean MacLennan
In-Reply-To: <477FCA53.8010103@pikatech.com>
On Saturday 05 January 2008, Sean MacLennan wrote:
> > You do break arch/ppc support with this patch. We have to still support
> > arch/ppc a few month, so please don't break this support for now.
> > =A0=20
> Gotcha. Is CONFIG_PPC_MERGED the right flag for things like this?
Yes, but it it's spelled CONFIG_PPC_MERGE, not CONFIG_PPC_MERGED.
Arnd=20
^ permalink raw reply
* Re: [PATCH] ibm_newemac: Increase number of default rx-/tx-buffers
From: Benjamin Herrenschmidt @ 2008-01-05 20:53 UTC (permalink / raw)
To: Stefan Roese; +Cc: netdev, linuxppc-dev
In-Reply-To: <200801051338.17957.sr@denx.de>
On Sat, 2008-01-05 at 13:38 +0100, Stefan Roese wrote:
> On Saturday 05 January 2008, Benjamin Herrenschmidt wrote:
> > On Sat, 2008-01-05 at 10:50 +0100, Stefan Roese wrote:
> > > Performance tests done by AMCC have shown that 256 buffer increase the
> > > performance of the Linux EMAC driver. So let's update the default
> > > values to match this setup.
> > >
> > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > ---
> >
> > Do we have the numbers ? Did they also measure latency ?
>
> I hoped this question would not come. ;) No, unfortunately I don't have any
> numbers. Just the recommendation from AMCC to always use 256 buffers.
Ok. Well, it's just a .config option so I suppose the patch is fine. Can
you also update the defconfigs (at least for the recent AMCC boards) ?
Ben.
^ permalink raw reply
* Re: [PATCH] Miscellaneous for Taco
From: Benjamin Herrenschmidt @ 2008-01-05 20:56 UTC (permalink / raw)
To: Sean MacLennan; +Cc: linuxppc-dev, Stefan Roese
In-Reply-To: <477FCADA.2010706@pikatech.com>
On Sat, 2008-01-05 at 13:22 -0500, Sean MacLennan wrote:
> Ok. I thought of that, but most 440EP boards have PCI set and don't need
> the special option. I was worried if I put 440EP someone down the road
> might remove it as "extraneous". We are probably the only 440EP board
> with no PCI ;)
Well, it's different.... PCI is an option that can be enabled ... or
not by the board. In our case, the HAS_OHCI thing is just a switch to
indicate that an OHCI can exist, it's not actually adding code, so
it can be safely left ON whenever a 440EP processor is there, it has
no bloat and won't conflict with the PCI option.
Ben.
^ permalink raw reply
* Re: [PATCH] ibm_newemac: Increase number of default rx-/tx-buffers
From: Josh Boyer @ 2008-01-05 21:48 UTC (permalink / raw)
To: benh; +Cc: netdev, Stefan Roese, linuxppc-dev
In-Reply-To: <1199566386.7291.58.camel@pasglop>
On Sun, 06 Jan 2008 07:53:06 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sat, 2008-01-05 at 13:38 +0100, Stefan Roese wrote:
> > On Saturday 05 January 2008, Benjamin Herrenschmidt wrote:
> > > On Sat, 2008-01-05 at 10:50 +0100, Stefan Roese wrote:
> > > > Performance tests done by AMCC have shown that 256 buffer increase the
> > > > performance of the Linux EMAC driver. So let's update the default
> > > > values to match this setup.
> > > >
> > > > Signed-off-by: Stefan Roese <sr@denx.de>
> > > > ---
> > >
> > > Do we have the numbers ? Did they also measure latency ?
> >
> > I hoped this question would not come. ;) No, unfortunately I don't have any
> > numbers. Just the recommendation from AMCC to always use 256 buffers.
>
> Ok. Well, it's just a .config option so I suppose the patch is fine. Can
> you also update the defconfigs (at least for the recent AMCC boards) ?
No need for a defconfig update patch. Paul or I usually do a general
defconfig update for most boards before the next kernel version. This
will get picked up then.
josh
^ permalink raw reply
* Re: [PATCH] ibm_newemac: Increase number of default rx-/tx-buffers
From: Benjamin Herrenschmidt @ 2008-01-05 21:54 UTC (permalink / raw)
To: Josh Boyer; +Cc: netdev, Stefan Roese, linuxppc-dev
In-Reply-To: <20080105154805.118eb3d1@vader.jdub.homelinux.org>
On Sat, 2008-01-05 at 15:48 -0600, Josh Boyer wrote:
> No need for a defconfig update patch. Paul or I usually do a general
> defconfig update for most boards before the next kernel version. This
> will get picked up then.
Will it ? I think the defconfigs will stick to the old value.
Ben.
^ permalink raw reply
* Re: [PATCH] i2c-ibm_iic driver
From: David Gibson @ 2008-01-06 0:12 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, Stefan Roese, Sean MacLennan
In-Reply-To: <200801052018.22954.arnd@arndb.de>
On Sat, Jan 05, 2008 at 08:18:22PM +0100, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Sean MacLennan wrote:
> >
> > Ok. The 44x based .dts files do not list 405-iic, so would I think I
> > will add two compatibility matches, one for 405 and one for 440EP. That
> > way I do not break all the current .dts files. Everybody ok with that?
> >
>
> Sounds good. There are obviously no other drivers that know only
> about 405 but not about 440, so there is no backwards compatibility
> problem. If we ever get a 450/460/470/... that we want to support
> with this driver, it can simply claim to be compatible with 440 or 405
> if not both.
Actually, I think checking for "ibm,iic" is ok. I'm sure there are
other IBM produced i2c chips, but "IIC" is also the name of the ASIC
block that implements this controller. "ibm,iic" in the compatible is
supposed to refer to this family of i2c bridges. Not a great choice
on my part, perhaps, but not so awful as to go changing the existing
device trees I think.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [alsa-devel] [PATCH] ASoC drivers for the Freescale MPC8610 SoC
From: David Gibson @ 2008-01-06 0:46 UTC (permalink / raw)
To: Timur Tabi; +Cc: Liam Girdwood, alsa-devel, linuxppc-dev
In-Reply-To: <477EEDFA.9010205@freescale.com>
On Fri, Jan 04, 2008 at 08:39:54PM -0600, Timur Tabi wrote:
> David Gibson wrote:
>
> > And what distinction are you drawing between "first" and "second"
> > here?
>
> Oh, that's an easy one: The CS4270 can work without an I2C or SPI connection,
> but it will never work without an I2S connection.
>
> > Why would the I2S need to scan for codecs? Wouldn't it be up to the
> > codec driver to register with I2S?
>
> Not in ASoC V1. The codec driver registers with ASoC, but the actual
> connection to other devices (e.g. the I2S driver) is done either in the I2S
> driver or in the fabric driver, depending on your mood. And that connection
> is done via a pointer to a structure in the codec driver.
Ok, but couldn't you strucutre your I2S or fabric driver so that it
only becomes fully operational once the codec driver has registered
with it?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH] ibm_newemac: Increase number of default rx-/tx-buffers
From: Josh Boyer @ 2008-01-06 1:43 UTC (permalink / raw)
To: benh; +Cc: netdev, Stefan Roese, linuxppc-dev
In-Reply-To: <1199570068.7291.65.camel@pasglop>
On Sun, 06 Jan 2008 08:54:28 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sat, 2008-01-05 at 15:48 -0600, Josh Boyer wrote:
> > No need for a defconfig update patch. Paul or I usually do a general
> > defconfig update for most boards before the next kernel version. This
> > will get picked up then.
>
> Will it ? I think the defconfigs will stick to the old value.
Put another way, if Kconfig doesn't update it automagically, I'll be
sure to do it myself. :)
josh
^ permalink raw reply
* Re: [PATCH] MTD for Taco
From: Sean MacLennan @ 2008-01-06 3:20 UTC (permalink / raw)
To: Stefan Roese, linuxppc-dev, david
In-Reply-To: <20080105122542.GH4326@localhost.localdomain>
David Gibson wrote:
>> I'm pretty sure that you don't need a board specific mapping driver
>> for NOR flash. physmap_of should be exactly what you need. You just need
>> to fill the device tree properties correctly.
>>
>
> Absolutely. We should not be using C-coded maps in arch/powerpc
>
>
Ok a couple of questions. How do I find out the size of the NOR flash?
It was in a global in the ppc arch.
Also, where is the best place to fixup the partitions? We have paritions
that must be at the top and ones that must be at the bottom, so we
dynamically setup the partitions based on the NOR size. The NOR size is
variable.
And is it ok the name the paritions in the dts based on an index? I
notice others use an address, but since I don't know the address up
front I just used index numbers.
Cheers,
Sean
^ permalink raw reply
* Re: [PATCH 2/3] Handle absolute pathnames correctly in dtc_open_file.
From: David Gibson @ 2008-01-06 3:37 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, jdl
In-Reply-To: <20080104211045.GB28991@ld0162-tx32.am.freescale.net>
On Fri, Jan 04, 2008 at 03:10:45PM -0600, Scott Wood wrote:
> Also, free file->dir when freeing file.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> srcpos.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/srcpos.c b/srcpos.c
> index 7340c33..7a0c47e 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -86,6 +86,16 @@ struct dtc_file *dtc_open_file(const char *fname,
> return file;
> }
>
> + if (fname[0] == '/') {
> + file->file = fopen(fname, "r");
> +
> + if (!file->file)
> + goto out;
> +
> + file->name = strdup(fname);
> + return file;
> + }
> +
> if (!search)
> search = &default_search;
>
> @@ -100,6 +110,7 @@ struct dtc_file *dtc_open_file(const char *fname,
> }
>
> out:
> + free((void *)file->dir);
That cast shouldn't be there.
> free(file);
> return NULL;
> }
> @@ -109,5 +120,6 @@ void dtc_close_file(struct dtc_file *file)
> if (fclose(file->file))
> die("Error closing \"%s\": %s\n", file->name, strerror(errno));
>
> + free((void *)file->dir);
Or here.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH] MTD for Taco
From: David Gibson @ 2008-01-06 3:44 UTC (permalink / raw)
To: Sean MacLennan; +Cc: linuxppc-dev, Stefan Roese
In-Reply-To: <4780491A.8080006@pikatech.com>
On Sat, Jan 05, 2008 at 10:20:58PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> >> I'm pretty sure that you don't need a board specific mapping driver
> >> for NOR flash. physmap_of should be exactly what you need. You just need
> >> to fill the device tree properties correctly.
> >>
> >
> > Absolutely. We should not be using C-coded maps in arch/powerpc
>
> Ok a couple of questions. How do I find out the size of the NOR flash?
> It was in a global in the ppc arch.
The flash size goes in the "reg" property of the flash node.
> Also, where is the best place to fixup the partitions? We have paritions
> that must be at the top and ones that must be at the bottom, so we
> dynamically setup the partitions based on the NOR size. The NOR size is
> variable.
Ok, you can fix this up in the bootwrapper.
> And is it ok the name the paritions in the dts based on an index? I
> notice others use an address, but since I don't know the address up
> front I just used index numbers.
No, the node name must match the node's "reg" property, which is the
partition's offset. If you're creating the partition nodess on the
fly in the bootwrapper you can get that right there. I'm also hoping
soon to add a function to fixup node names to libfdt, which will then
be usable in the wrapper.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* [PATCH] Fix carry bug in 128-bit unsigned integer adding
From: Liu Yu @ 2008-01-06 14:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Liu Yu
This bug exists in math emulation for powerpc.
The macro define are mainly used by multiplication.
When adding two unsigned operands ( r = x + y ),
the carry bit can be counted by whether r is less than x.
However, when adding three unsigned operands, this method does not work.
The original code below uses this method to count carry,
it apparently overlook the case of three operands.
Assume all the operands is 32-bit wide,
( r = x + y + last_carry , x = 0, y = 0xffffffff, last_carry = 1),
then r is no less than x but it actually gets a carry.
I tried to fix this bug, but this patch seems not that pretty.
Are there any better ideas?
Comments are always welcomed!
Signed-off-by: Liu Yu <Yu.Liu@freescale.com>
---
arch/powerpc/math-emu/op-4.h | 22 ++++++++++++++--------
1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/math-emu/op-4.h b/arch/powerpc/math-emu/op-4.h
index fcdd6d0..93c7390 100644
--- a/arch/powerpc/math-emu/op-4.h
+++ b/arch/powerpc/math-emu/op-4.h
@@ -195,18 +195,24 @@
#ifndef __FP_FRAC_ADD_4
#define __FP_FRAC_ADD_4(r3,r2,r1,r0,x3,x2,x1,x0,y3,y2,y1,y0) \
- (r0 = x0 + y0, \
- r1 = x1 + y1 + (r0 < x0), \
- r2 = x2 + y2 + (r1 < x1), \
- r3 = x3 + y3 + (r2 < x2))
+ do { \
+ _FP_W_TYPE _x0 = x0, _x1 = x1, _x2 = x2, _y1 = y1, _y2 = y2; \
+ r0 = x0 + y0; \
+ r1 = x1 + y1 + (r0 < _x0); \
+ r2 = x2 + y2 + (r1 < _x1 || (r1 == _x1 && !(_y1 + 1))); \
+ r3 = x3 + y3 + (r2 < _x2 || (r2 == _x2 && !(_y2 + 1))); \
+ } while(0)
#endif
#ifndef __FP_FRAC_SUB_4
#define __FP_FRAC_SUB_4(r3,r2,r1,r0,x3,x2,x1,x0,y3,y2,y1,y0) \
- (r0 = x0 - y0, \
- r1 = x1 - y1 - (r0 > x0), \
- r2 = x2 - y2 - (r1 > x1), \
- r3 = x3 - y3 - (r2 > x2))
+ do { \
+ _FP_W_TYPE _x0 = x0, _x1 = x1, _x2 = x2, _y1 = y1, _y2 = y2; \
+ r0 = x0 - y0; \
+ r1 = x1 - y1 - (r0 > _x0); \
+ r2 = x2 - y2 - (r1 > _x1 || (r1 == _x1 && !(_y1 + 1))); \
+ r3 = x3 - y3 - (r2 > _x2 || (r2 == _x2 && !(_y2 + 1))); \
+ } while(0)
#endif
#ifndef __FP_FRAC_ADDI_4
--
^ permalink raw reply related
* [PATCH] Fix remainder calculating bug in single floating point division
From: Liu Yu @ 2008-01-06 14:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Liu Yu
This bug exists in the emulation of floating point division for powerpc.
The original code cannot count the remainder correctly.
I can provide a test case to trigger this bug.
When use fdiv to count 1.1754941e-38f / 0.9999999f,
the result is expected to be 1.175494e-38f,
but we will get 1.174921e-38f in the original case.
Comments are always welcomed!
Signed-off-by: Liu Yu <Yu.Liu@freescale.com>
---
arch/powerpc/math-emu/sfp-machine.h | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/math-emu/sfp-machine.h b/arch/powerpc/math-emu/sfp-machine.h
index 4b17d83..644cee2 100644
--- a/arch/powerpc/math-emu/sfp-machine.h
+++ b/arch/powerpc/math-emu/sfp-machine.h
@@ -324,10 +324,10 @@ extern int fp_pack_ds(void *, long, unsigned long, unsigned long, long, long);
__r1 = __r1 * __ll_B | __ll_highpart (n0); \
if (__r1 < __m) \
{ \
- __q1--, __r1 += (d); \
- if (__r1 >= (d)) /* we didn't get carry when adding to __r1 */ \
- if (__r1 < __m) \
+ do { \
__q1--, __r1 += (d); \
+ /* we didn't get carry when adding to __r1 */ \
+ } while (__r1 >= (d) && __r1 < __m); \
} \
__r1 -= __m; \
\
@@ -337,10 +337,9 @@ extern int fp_pack_ds(void *, long, unsigned long, unsigned long, long, long);
__r0 = __r0 * __ll_B | __ll_lowpart (n0); \
if (__r0 < __m) \
{ \
- __q0--, __r0 += (d); \
- if (__r0 >= (d)) \
- if (__r0 < __m) \
+ do { \
__q0--, __r0 += (d); \
+ } while (__r0 >= (d) && __r0 < __m); \
} \
__r0 -= __m; \
\
--
1.5.2
^ permalink raw reply related
* Re: [PATCH] Fix remainder calculating bug in single floating point division
From: Benjamin Herrenschmidt @ 2008-01-06 20:07 UTC (permalink / raw)
To: Liu Yu; +Cc: linuxppc-dev
In-Reply-To: <11996296134110-git-send-email-Yu.Liu@freescale.com>
On Sun, 2008-01-06 at 22:26 +0800, Liu Yu wrote:
> This bug exists in the emulation of floating point division for powerpc.
>
> The original code cannot count the remainder correctly.
> I can provide a test case to trigger this bug.
> When use fdiv to count 1.1754941e-38f / 0.9999999f,
> the result is expected to be 1.175494e-38f,
> but we will get 1.174921e-38f in the original case.
It's nice to see somebody digging in that scary math emu stuff. If you
could also get rid of the warnings, it would be perfect :-)
Now, appart from that, I seem to remember Dan telling me at some point
in the past (I think it was Dan... ) that this code was lifted from
somewhere else (glibc ? gcc soft-float ?), in which case it might be
worth to fix that somewhere else as well, or if it's already been fixed,
maybe resync.
Cheers,
Ben.
^ permalink raw reply
* Re: Enable RTC for Ebony and Walnut
From: Olof Johansson @ 2008-01-06 20:48 UTC (permalink / raw)
To: Josh Boyer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20080102114208.GD23168@localhost.localdomain>
On Wed, Jan 02, 2008 at 10:42:08PM +1100, David Gibson wrote:
> On Fri, Dec 21, 2007 at 03:29:22PM -0600, Olof Johansson wrote:
> > Hi,
> >
> > On Fri, Dec 21, 2007 at 01:21:30PM +1100, David Gibson wrote:
> > > Index: working-2.6/arch/powerpc/platforms/Kconfig
> > > ===================================================================
> > > --- working-2.6.orig/arch/powerpc/platforms/Kconfig 2007-12-10 10:52:44.000000000 +1100
> > > +++ working-2.6/arch/powerpc/platforms/Kconfig 2007-12-21 13:11:34.000000000 +1100
> > > @@ -315,6 +315,9 @@ config FSL_ULI1575
> > > config CPM
> > > bool
> > >
> > > +config OF_RTC
> > > + bool
> > > +
> >
> > New config options should have descriptions, even if they're silent
> > ones.
>
> Uh.. but if there's a description, won't kconfig prompt for the
> option? This one should only ever be selected by the platform
> options.
Sorry, should have said "should have help texts to describe them". Yes,
the one-line description will trigger a question, but adding a short
help text describing what the config option is for does not.
-Olof
^ permalink raw reply
* Re: [patch 2/3] PS3: Add logical performance monitor device support
From: Geoff Levand @ 2008-01-06 20:39 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, paulus
In-Reply-To: <200801051229.33099.arnd@arndb.de>
On 01/05/2008 03:29 AM, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Geoff Levand wrote:
>> + struct layout {
>> + struct ps3_system_bus_device dev;
>> + } *p;
>
> What's the point of this data structure? You don't use the
> struct anywhere, and it only has one member, so you could
> just declare that directly.
Yes, this code was just cut and pasted from a device that
had more members.
>> + if (tmp1 != tmp2) {
>> + pr_debug("%s:%d: wrong lpar\n",
>> + __func__, __LINE__);
>> + result = -1;
>> + goto fail_rights;
>> + }
>> +
>> + if (!(p->dev.lpm.rights & PS3_LPM_RIGHTS_USE_LPM)) {
>> + pr_debug("%s:%d: don't have rights to use lpm\n",
>> + __func__, __LINE__);
>> + result = -1;
>> + goto fail_rights;
>> + }
>> +
>
> I think __init functions should return error codes like -EPERM or
> -EINVAL, not numeric -1.
I'll fix it in the next post.
-Geoff
^ permalink raw reply
* Re: [patch 3/3] PS3: Add logical performance monitor driver support
From: Geoff Levand @ 2008-01-06 20:40 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, paulus, Takashi Yamamoto
In-Reply-To: <200801051256.22732.arnd@arndb.de>
On 01/05/2008 03:56 AM, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Geoff Levand wrote:
>> From: Takashi Yamamoto <Takashi02_Yamamoto@hq.scei.sony.co.jp>
>>
>> Add PS3 logical performance monitor (lpm) device driver.
>>
>> The PS3's LV1 hypervisor provides a Logical Performance Monitor that
>> abstarcts the Cell processor's performance monitor features for use
>> by guest operating systems.
>>
>> Signed-off-by: Takashi Yamamoto <Takashi02_Yamamoto@hq.scei.sony.co.jp>
>> Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
>
> Wow, great stuff!
>
> One bit of information I'm missing in the description is what the users
> of this code are. Your Kconfig text mentions that can be used for both
> perfmon and oprofile. Are these, or one of them, currently under
> work as well?
Yes, perfmon2 support is somewhat working, but still under development.
I added the perfmon2 patches into ps3-linux.git, so the easy way would
be to use those kernel sources. This info written before I merged
perfmon2 into ps3-linux.git, but does give some info on how to use
it:
http://www.kernel.org/pub/linux/kernel/people/geoff/cell/CELL-Linux-CL_20071220-ADDON/doc/ApplicationProgrammingEnvironment.html
I just started to work on the kernel oprofile support, but it is not
usable yet. Any contributions would be welcome. Currently the patch
is just something hacked up to use for testing:
http://git.kernel.org/?p=linux/kernel/git/geoff/ps3-linux-patches.git;a=blob;f=ps3-wip/ps3-oprofile.patch;hb=HEAD
>> --- a/arch/powerpc/platforms/ps3/Kconfig
>> +++ b/arch/powerpc/platforms/ps3/Kconfig
>> @@ -138,4 +138,18 @@ config PS3_FLASH
>> be disabled on the kernel command line using "ps3flash=off", to
>> not allocate this fixed buffer.
>>
>> +config PS3_LPM
>> + tristate "PS3 Logical Performance Monitor support"
>> + depends on PPC_PS3
>> + default n
>
> 'default n' is redundant.
>
>> + help
>> + Include support for the PS3 Logical Performance Monitor.
>> +
>> + This support is required to use the logical performance monitor
>> + of the PS3's LV1 hypervisor.
>> +
>> + If you intend to use the advanced performance monitoring and
>> + profiling support of the Cell processor with programs like
>> + oprofile and perfmon, then say Y or M, otherwise say N.
>> +
>
> Should that be perfmon2 instead of perfmon?
Yes.
> I think once they high-level drivers are merged, it would make sense
> to autoselected by these, so the user doesn't have to read through
> the help text.
Yes, many things can be improved.
>> +/**
>> + * struct ps3_lpm_priv - private lpm device data.
>> + *
>> + * @mutex: Open/close mutex.
>> + * @rights: The lpm rigths granted by the system policy module.
>> + * @pu_id: The lv1 id of the BE prosessor for this lpm instance.
>> + * @lpm_id: The lv1 id of this lpm instance.
>> + * @outlet_id: The outlet created by lv1 for this lpm instance.
>> + * @constructed: A flag indicating the lpm driver has been opened -- can we just use (lpm_id == ???)
>> + * @tb_size: The lv1 trace buffer size
>> + * @tb_cache: Trace buffer cache
>> + * @tb_cache_internal: A flag indicating the trace buffer cache was allocated
>> + * by the driver.
>> + * @tb_cache: Trace buffer cache
>> + * @sizeof_traced_data: Traced data size
>> + * @sbd: the struct ps3_system_bus_device attached to this driver
>> + */
>> +
>> +struct ps3_lpm_priv {
>> + struct mutex mutex;
>> + u64 rights;
>> + u64 pu_id;
>> + u64 lpm_id;
>> + u64 outlet_id;
>> + int constructed;
>> + u64 tb_size;
>> + void *tb_cache;
>> + u64 tb_cache_size;
>> + int tb_cache_internal;
>> + u64 sizeof_traced_data;
>> + u64 sizeof_total_copied_data;
>> + u64 shadow_pm_control;
>> + u64 shadow_pm_start_stop;
>> + u64 shadow_pm_interval;
>> + u64 shadow_group_control;
>> + u64 shadow_debug_bus_control;
>> + struct ps3_system_bus_device *sbd;
>> +};
>
> Some of the members in this struct are not present in the comment,
> which is somewhat confusing.
I'll add those in for the next post.
>> +
>> +/**
>> + * lpm_priv - Static instance of the lpm data.
>> + *
>> + * Since the exported routines don't support the notion of a device
>> + * instance we need to hold the instance in this static variable
>> + * and only allow at most one instance to be created.
>> + */
>> +
>> +static struct ps3_lpm_priv *lpm_priv;
>
> Hmmm, my gut feeling about this is that it would be better to actually
> pass this around through the exported functions.
Yes, I feel the same way. In general, I think we need to make a
better platform abstraction of the processor's pm functions. That
would include support for platform specific instance data, RTAS
for blade, lpm for ps3.
> That would also
> provide some arbitration between the possible users, e.g. oprofile
> would get -EBUSY when asking for the ps3_lpm while perfmon2 is
> already active.
That already happens in ps3_lpm_open(), using the lpm_priv.constructed
variable.
>> +inline u32 ps3_get_hw_thread_id(int cpu)
>> +{
>> + return cpu;
>> +}
>> +EXPORT_SYMBOL_GPL(ps3_get_hw_thread_id);
>
> The 'inline' is bogus, as that doesn't work across exported functions
> and you are not using the function in this file.
>
> Should you perhaps return hard_smp_processor_id(cpu) instead of cpu
> here?
Yes, good point.
>> +/**
>> + * _ps3_copy_trace_buffer - Copy the trace buffer.
>> + */
>> +
>> +static u64 _ps3_copy_trace_buffer(u64 offset, u64 size, u64 *to, int to_user)
>> +{
>> + int result;
>> + u64 sizeof_copied_data;
>> +
>> + if (offset >= lpm_priv->sizeof_traced_data)
>> + return 0;
>> +
>> + result = lv1_copy_lpm_trace_buffer(lpm_priv->lpm_id, offset, size,
>> + &sizeof_copied_data);
>> + if (result) {
>> + dev_err(sbd_core(), "%s:%u: lv1_copy_lpm_trace_buffer failed: "
>> + "offset 0x%lx, size 0x%lx: %s\n", __func__, __LINE__,
>> + offset, size, ps3_result(result));
>> + return 0;
>> + }
>> +
>> + if (to_user) {
>> + result = copy_to_user((void __user *)to, lpm_priv->tb_cache,
>> + sizeof_copied_data);
>> + if (result) {
>> + dev_err(sbd_core(), "%s:%u: copy_to_user() error. "
>> + "offset:0x%lx size:0x%lx dest:0x%p src:0x%p\n",
>> + __func__, __LINE__, offset, sizeof_copied_data,
>> + to, lpm_priv->tb_cache);
>> + return 0;
>> + }
>> + } else
>> + memcpy(to, lpm_priv->tb_cache, sizeof_copied_data);
>> +
>> + return sizeof_copied_data;
>> +}
>
> I don't like how this functions uses the same argument for kernel and user
> pointers, this will cause sparse warnings in the code using it. How about
> making this two separate functions?
OK, good point, I'll consider it.
>> +int ps3_lpm_open(void *tb_cache, u64 tb_cache_size, u64 tb_type)
>> +{
>> + int result;
>> + u64 cbe_node_id;
>> + u64 tb_size;
>> + u64 ctrl_opt;
>> + u64 tb_cache_lpar_addr;
>> + u64 lpm_id;
>> + u64 outlet_id;
>> + u64 used_tb_size;
>> +
>> + if (!lpm_priv) {
>> + BUG();
>> + return -ENODEV;
>> + }
>> +
>> + mutex_lock(&lpm_priv->mutex);
>
> This mutex is documented as the 'open/close' mutex and you use it
> to prevent concurrent execution of the open and close functions.
> However, it's unclear what data structures are actually protected
> by it.
>
> I think this is the result of unusual lifetime rules for the lpm_priv
> object, which I already mentioned above.
Yes, we really just need to use the mutex to allow only a single instance,
and so do a mutex_trylock() in ps3_lpm_open() and return -EBUSY on fail,
and then a mutex_unlock() in ps3_lpm_close().
-Geoff
^ permalink raw reply
* Re: [PATCH] Fix remainder calculating bug in single floating point division
From: Benjamin Herrenschmidt @ 2008-01-06 20:49 UTC (permalink / raw)
To: Dan Malek; +Cc: linuxppc-dev, Liu Yu
In-Reply-To: <66F0B96A-5B53-48D3-BB90-07841F3EA1EE@embeddedalley.com>
On Sun, 2008-01-06 at 12:44 -0800, Dan Malek wrote:
> While it's convenient, I still don't think kernel float
> emulation should be a solution. The tools should
> generate soft-float for the applications and libraries.
I agree, for deployement, it's definitely not a solution, though it's
very convenient for us hackers :-)
Ben.
^ permalink raw reply
* Re: [PATCH] Fix remainder calculating bug in single floating point division
From: Dan Malek @ 2008-01-06 20:44 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, Liu Yu
In-Reply-To: <1199650065.7291.81.camel@pasglop>
On Jan 6, 2008, at 12:07 PM, Benjamin Herrenschmidt wrote:
> It's nice to see somebody digging in that scary math emu stuff. If you
> could also get rid of the warnings, it would be perfect :-)
Yes, it is :-) I didn't think it would have a life beyond MPC8xx.
> .... that this code was lifted from
> somewhere else (glibc ? gcc soft-float ?),
It seems like a lifetime ago.... I copied the framework
from Sparc, and the internals from gcc soft-float. I didn't
change any of the internal emulation functions (hence,
some of the warnings), just the calling interface.
While it's convenient, I still don't think kernel float
emulation should be a solution. The tools should
generate soft-float for the applications and libraries.
Thanks.
-- Dan
^ permalink raw reply
* Re: [patch 3/3] PS3: Add logical performance monitor driver support
From: Arnd Bergmann @ 2008-01-06 21:56 UTC (permalink / raw)
To: Geoff Levand; +Cc: linuxppc-dev, paulus, Takashi Yamamoto
In-Reply-To: <47813CA4.8000203@am.sony.com>
On Sunday 06 January 2008, Geoff Levand wrote:
>
> > This mutex is documented as the 'open/close' mutex and you use it
> > to prevent concurrent execution of the open and close functions.
> > However, it's unclear what data structures are actually protected
> > by it.
> >
> > I think this is the result of unusual lifetime rules for the lpm_priv
> > object, which I already mentioned above.
>
> Yes, we really just need to use the mutex to allow only a single instance,
> and so do a mutex_trylock() in ps3_lpm_open() and return -EBUSY on fail,
> and then a mutex_unlock() in ps3_lpm_close().
A simple atomic_test_and_set in open() should do the job just as well then.
Arnd <><
^ permalink raw reply
* Re: [PATCH 2/3] Handle absolute pathnames correctly in dtc_open_file.
From: Scott Wood @ 2008-01-06 22:46 UTC (permalink / raw)
To: jdl, linuxppc-dev
In-Reply-To: <20080106033716.GL4326@localhost.localdomain>
On Sun, Jan 06, 2008 at 02:37:16PM +1100, David Gibson wrote:
> > out:
> > + free((void *)file->dir);
>
> That cast shouldn't be there.
It generates a constness warning otherwise. I was on the fence over
whether to just remove the constness (even though the data is never
modified during its lifetime); would that be preferable?
-Scott
^ permalink raw reply
* Re: [PATCH 2/3] Look for include files in the directory of the including file.
From: Scott Wood @ 2008-01-06 22:52 UTC (permalink / raw)
To: jdl, linuxppc-dev
In-Reply-To: <20080104042739.GC4326@localhost.localdomain>
On Fri, Jan 04, 2008 at 03:27:39PM +1100, David Gibson wrote:
> > + newfile = dtc_open_file(filename, searchptr);
> > + if (!newfile) {
> > + yyerrorf("Couldn't open \"%s\": %s",
> > + filename, strerror(errno));
> > + exit(1);
>
> Use die() here, that's what it's for.
die() doesn't print file and line information.
> > + while (search) {
> > + if (dtc_open_one(file, search->dir, fname))
> > + return file;
>
> Don't we need a different case here somewhere for if someone specifies
> an include file as an absolute path? Have I missed something?
Yeah, I forgot about that, and sent another patch to fix it when I
noticed (jdl had already pulled, so I didn't send an amended patch).
> [snip]
> > +struct search_path {
> > + const char *dir; /* NULL for current directory */
> > + struct search_path *prev, *next;
> > +};
>
> I wouldn't suggest a doubly linked list here. Or at least not without
> converting our many existing singly linked lists at the same time.
The doubly-linked list is intended to make it easier to construct search
path lists one-at-a-time from arguments in the proper order, without
needing to reverse the list at the end.
-Scott
^ permalink raw reply
* Re: [PATCH 3/3] Return a non-zero exit code if an error occurs during dts parsing.
From: Scott Wood @ 2008-01-06 22:55 UTC (permalink / raw)
To: jdl, linuxppc-dev
In-Reply-To: <20080104043033.GD4326@localhost.localdomain>
On Fri, Jan 04, 2008 at 03:30:33PM +1100, David Gibson wrote:
> This is unequivocally wrong. boot_info should have information about
> the contents of the blob, not state information like the error.
"This blob is invalid" *is* information about the contents of the blob.
> If you're going to use an ugly global, then use it everywhere.
Why go out of our way to make the code even less library-able/thread-safe?
-Scott
^ 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