[PATCH 1/2 v2] [POWERPC] Add PPC4xx L2-cache support (440GX)
This patch adds support for the 256k L2 cache found on some IBM/AMCC
4xx PPC's. It introduces a common 4xx SoC file (sysdev/ppc4xx_soc.c)
which currently "only" adds the L2 cache init code. Other common 4xx
stuff can be added later here.
The L2 cache handling code is a copy of Eugene's code in arch/ppc
with small modifications.
Tested on AMCC Taishan 440GX.
Signed-off-by: Stefan Roese <[EMAIL PROTECTED]>
---
Small changes included as suggested by Stephen Rothwell. It also removes
mentioning 460EX/GT, since L2 cache handling on these PPC's is not clear
right now.
arch/powerpc/Kconfig |3 +
arch/powerpc/platforms/44x/Kconfig |2 +
arch/powerpc/sysdev/Makefile |1 +
arch/powerpc/sysdev/ppc4xx_soc.c | 178
include/asm-powerpc/dcr-regs.h | 78
5 files changed, 258 insertions(+), 0 deletions(-)
create mode 100644 arch/powerpc/sysdev/ppc4xx_soc.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1189d8d..69d4738 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -490,6 +490,9 @@ config FSL_PCI
bool
select PPC_INDIRECT_PCI
+config 4xx_SOC
+ bool
+
# Yes MCA RS/6000s exist but Linux-PPC does not currently support any
config MCA
bool
diff --git a/arch/powerpc/platforms/44x/Kconfig
b/arch/powerpc/platforms/44x/Kconfig
index 83155fe..061ba3c 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -120,6 +120,7 @@ config 440GP
config 440GX
bool
+ select 4xx_SOC
select IBM_NEW_EMAC_EMAC4
select IBM_NEW_EMAC_RGMII
select IBM_NEW_EMAC_ZMII #test only
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index 15f3e85..851a0be 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PPC_INDIRECT_PCI)+= indirect_pci.o
obj-$(CONFIG_PPC_I8259)+= i8259.o
obj-$(CONFIG_IPIC) += ipic.o
obj-$(CONFIG_4xx) += uic.o
+obj-$(CONFIG_4xx_SOC) += ppc4xx_soc.o
obj-$(CONFIG_XILINX_VIRTEX)+= xilinx_intc.o
obj-$(CONFIG_OF_RTC) += of_rtc.o
ifeq ($(CONFIG_PCI),y)
diff --git a/arch/powerpc/sysdev/ppc4xx_soc.c b/arch/powerpc/sysdev/ppc4xx_soc.c
new file mode 100644
index 000..4847555
--- /dev/null
+++ b/arch/powerpc/sysdev/ppc4xx_soc.c
@@ -0,0 +1,178 @@
+/*
+ * IBM/AMCC PPC4xx SoC setup code
+ *
+ * Copyright 2008 DENX Software Engineering, Stefan Roese <[EMAIL PROTECTED]>
+ *
+ * L2 cache routines cloned from arch/ppc/syslib/ibm440gx_common.c which is:
+ * Eugene Surovegin <[EMAIL PROTECTED]> or <[EMAIL PROTECTED]>
+ * Copyright (c) 2003 - 2006 Zultys Technologies
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+static u32 dcrbase;
+
+/*
+ * L2-cache
+ */
+
+/* Issue L2C diagnostic command */
+static inline u32 l2c_diag(u32 addr)
+{
+ mtdcr(dcrbase + DCRN_L2C0_ADDR, addr);
+ mtdcr(dcrbase + DCRN_L2C0_CMD, L2C_CMD_DIAG);
+ while (!(mfdcr(dcrbase + DCRN_L2C0_SR) & L2C_SR_CC))
+ ;
+
+ return mfdcr(dcrbase + DCRN_L2C0_DATA);
+}
+
+static irqreturn_t l2c_error_handler(int irq, void *dev)
+{
+ u32 sr = mfdcr(dcrbase + DCRN_L2C0_SR);
+
+ if (sr & L2C_SR_CPE) {
+ /* Read cache trapped address */
+ u32 addr = l2c_diag(0x4200);
+ printk(KERN_EMERG "L2C: Cache Parity Error, addr[16:26] =
0x%08x\n",
+ addr);
+ }
+ if (sr & L2C_SR_TPE) {
+ /* Read tag trapped address */
+ u32 addr = l2c_diag(0x8200) >> 16;
+ printk(KERN_EMERG "L2C: Tag Parity Error, addr[16:26] =
0x%08x\n",
+ addr);
+ }
+
+ /* Clear parity errors */
+ if (sr & (L2C_SR_CPE | L2C_SR_TPE)){
+ mtdcr(dcrbase + DCRN_L2C0_ADDR, 0);
+ mtdcr(dcrbase + DCRN_L2C0_CMD, L2C_CMD_CCP | L2C_CMD_CTE);
+ } else {
+ printk(KERN_EMERG "L2C: LRU error\n");
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int __init ppc4xx_l2c_probe(void)
+{
+ struct device_node *np;
+ u32 r;
+ unsigned long flags;
+ int irq;
+ const u32 *dcrreg;
+ u32 dcrbase_isram;
+ int len;
+
+ np = of_find_compatible_node(np, NULL, "ibm,l2-cache");
+ if (!np)
+ return 0;
+
+ /* Map DCRs */
+ dcrreg = of_get_property(np, "dcr-reg", &len);
+ if (!dcrreg || (len != 4 * sizeof(u32))) {
+ printk(KERN_ERR "%s: Can't get DCR register base !",
+ np->full_nam
[PATCH 2/2 v2] [POWERPC] Add L2 cache node to AMCC Taishan dts file
This patch adds the L2 cache node to the Taishan 440GX dts file.
Signed-off-by: Stefan Roese <[EMAIL PROTECTED]>
---
Unit address removed.
arch/powerpc/boot/dts/taishan.dts | 10 ++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/taishan.dts
b/arch/powerpc/boot/dts/taishan.dts
index 8278068..d0bff33 100644
--- a/arch/powerpc/boot/dts/taishan.dts
+++ b/arch/powerpc/boot/dts/taishan.dts
@@ -104,6 +104,16 @@
// FIXME: anything else?
};
+ L2C0: l2c {
+ compatible = "ibm,l2-cache-440gx", "ibm,l2-cache";
+ dcr-reg = <20 8 /* Internal SRAM DCR's */
+ 30 8>; /* L2 cache DCR's */
+ cache-line-size = <20>; /* 32 bytes */
+ cache-size = <4>; /* L2, 256K */
+ interrupt-parent = <&UIC2>;
+ interrupts = <17 1>;
+ };
+
plb {
compatible = "ibm,plb-440gx", "ibm,plb4";
#address-cells = <2>;
--
1.5.4.4
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Oops with TQM5200 on TQM5200
Grant Likely wrote:
On Thu, Mar 20, 2008 at 10:17 AM, Bartlomiej Sieka <[EMAIL PROTECTED]> wrote:
Anatolij Gustschin wrote:
> Hello Wolfgang,
>
> Wolfgang Grandegger wrote:
>
>> I just tried Linux 2.6.25-rc6 on my TQM5200 module and got the attached
>> oops. Are there any known patches fixing the problems?
>
> try the patch below for tqm5200.dts, rebuild dtb and boot
> again. Not sure if it works for Linux 2.6.25-rc6, but for
> 2.6.25-rc3 it does.
It helps 2.6.25-rc6 too - thanks Anatolij.
>
> Anatolij
> --
> diff --git a/arch/powerpc/boot/dts/tqm5200.dts
> b/arch/powerpc/boot/dts/tqm5200.dts
> index c86464f..7c23bb3 100644
> --- a/arch/powerpc/boot/dts/tqm5200.dts
> +++ b/arch/powerpc/boot/dts/tqm5200.dts
> @@ -83,6 +83,7 @@
> };
>
> [EMAIL PROTECTED] {
> +device_type = "dma-controller";
This actually fixes the Oops.
Hmm, this sounds like a band-aid; the kernel shouldn't oops if this is
missing from the device tree. Fail, perhaps, but oops is worrisome.
Would someone like to investigate?
results of some further investigation:
the "bestcomm-core" driver defines its of_match table as follows
static struct of_device_id mpc52xx_bcom_of_match[] = {
{ .type = "dma-controller", .compatible = "fsl,mpc5200-bestcomm", },
{ .type = "dma-controller", .compatible = "mpc5200-bestcomm", },
{},
};
so while registering the driver the drivers probe function won't be
called because of_match_node shows unexpected behavior in the case
that device tree node lacks device_type = "dma-controller" definition.
here is the current of_match_node code for quick reference:
drivers/of/base.c:309
const struct of_device_id *of_match_node(const struct of_device_id *matches,
const struct device_node *node)
{
while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
int match = 1;
if (matches->name[0])
match &= node->name
&& !strcmp(matches->name, node->name);
if (matches->type[0])
match &= node->type
&& !strcmp(matches->type, node->type);
if (matches->compatible[0])
match &= of_device_is_compatible(node,
matches->compatible);
if (match)
return matches;
matches++;
}
return NULL;
}
So, the bestcomm-core driver provided the type field, but the
device tree node lacks it. The "if (matches->type[0])" case is true
and "node->type && !strcmp(matches->type, node->type)" evaluates to
zero, so match flag is set to zero. Now there is still a chance that
compatible property will match but even if it is the case, match flag
remains zero:
"match &= of_device_is_compatible(node, matches->compatible)" always
sets match flag to zero if match was zero previously. of_match_node
returns NULL, the bestcomm-core driver probe won't be called and thus
the drivers bcom_engine structure won't be allocated. Referencing this
structure later causes observed Oops.
Checking bcom_eng pointer for NULL before referencing data pointed
by it prevents oopsing, but fec driver still doesn't work (because
of the lost bestcomm match and resulted task allocation failure).
Actually the compatible property exists and should match and so
the fec driver shoud work.
I suggest removing .type = "dma-controller" from the bestcomm driver's
mpc52xx_bcom_of_match table to solve the problem.
What do you think?
Signed-off-by: Anatolij Gustschin <[EMAIL PROTECTED]>
---
diff --git a/arch/powerpc/sysdev/bestcomm/bestcomm.c
b/arch/powerpc/sysdev/bestcomm/bestcomm.c
index f58..137d830 100644
--- a/arch/powerpc/sysdev/bestcomm/bestcomm.c
+++ b/arch/powerpc/sysdev/bestcomm/bestcomm.c
@@ -484,8 +484,8 @@ mpc52xx_bcom_remove(struct of_device *op)
}
static struct of_device_id mpc52xx_bcom_of_match[] = {
- { .type = "dma-controller", .compatible = "fsl,mpc5200-bestcomm", },
- { .type = "dma-controller", .compatible = "mpc5200-bestcomm", },
+ { .compatible = "fsl,mpc5200-bestcomm", },
+ { .compatible = "mpc5200-bestcomm", },
{},
};
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Oops with TQM5200 on TQM5200
On Sat, Mar 22, 2008 at 4:49 AM, Anatolij Gustschin <[EMAIL PROTECTED]> wrote:
> Checking bcom_eng pointer for NULL before referencing data pointed
> by it prevents oopsing, but fec driver still doesn't work (because
> of the lost bestcomm match and resulted task allocation failure).
> Actually the compatible property exists and should match and so
> the fec driver shoud work.
>
> I suggest removing .type = "dma-controller" from the bestcomm driver's
> mpc52xx_bcom_of_match table to solve the problem.
>
> What do you think?
Yes, I agree. .compatible is completely sufficient to match the
device so .type is superfluous in this case. Removing it is
appropriate.
I've already sent a patch to fix the null pointer deref.
Acked-by: Grant Likely <[EMAIL PROTECTED]>
Paul, here's one more bug fix to pick up for .25. (I think we're done now)
Cheers,
g.
>
> Signed-off-by: Anatolij Gustschin <[EMAIL PROTECTED]>
> ---
> diff --git a/arch/powerpc/sysdev/bestcomm/bestcomm.c
> b/arch/powerpc/sysdev/bestcomm/bestcomm.c
> index f58..137d830 100644
> --- a/arch/powerpc/sysdev/bestcomm/bestcomm.c
> +++ b/arch/powerpc/sysdev/bestcomm/bestcomm.c
> @@ -484,8 +484,8 @@ mpc52xx_bcom_remove(struct of_device *op)
> }
>
> static struct of_device_id mpc52xx_bcom_of_match[] = {
> - { .type = "dma-controller", .compatible = "fsl,mpc5200-bestcomm", },
> - { .type = "dma-controller", .compatible = "mpc5200-bestcomm", },
> + { .compatible = "fsl,mpc5200-bestcomm", },
> + { .compatible = "mpc5200-bestcomm", },
> {},
> };
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.
On Fri, Mar 21, 2008 at 3:21 AM, Paul Mackerras <[EMAIL PROTECTED]> wrote: > Grant Likely writes: > > > Personally, I'm not fond of this approach. There is already some > > traction to using the reg-shift property to specify spacing, and I > > think it would be appropriate to also define a reg-offset property to > > handle the +3 offset and then let the xilinx 16550 nodes use those. > > Why do we need a reg-offset property when we can just add the offset > to the appropriate word(s) in the reg property? Primarily because the device creates 32 byte registers starting at 0; but they are also big-endian byte accessible so a byte read at offset 8 also works. reg-offset seems to be a better description of the hardware to me. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.
On Fri, Mar 21, 2008 at 7:00 AM, Sergei Shtylyov <[EMAIL PROTECTED]> wrote: > Grant Likely wrote: > > Personally, I'm not fond of this approach. There is already some > > traction to using the reg-shift property to specify spacing, and I > > think it would be appropriate to also define a reg-offset property to > > handle the +3 offset and then let the xilinx 16550 nodes use those. > > That's making things only worse than the mere "reg-shift" idea. I think > that both are totally wrong. Everything about the programming interface > should > be said in the "compatible" and possibly "model" properties. of_serial driver > should recognize them and pass the necessary details to 8250.c. As for me, > I'm > strongly against plaguing the device tree with the *Linux driver > implementation specifics* (despite I was trying this with MTD -- there it > seemed somewhat more grounded :-). Not true. Compatible defines what the node is describing. It is perfectly valid for a compatible value definition to also defines some additional properties that can be queried for interface details. Xilinx is completely free to define a "xlnx,..." compatible value for their ns16550 compatible device. However, 'sparse' ns16550 devices are a common and well known variation so I think it is valid and reasonable to define a compatible binding for this case. As for using a new binding like "sparse16550" instead of extending "ns16550"; it is because reg-shift and reg-offset would be required nodes and therefore is not compatible with drivers using the original ns16550 binding. Using a new namespace gives freedom to define the required properties. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: crash in init_ipic_sysfs on efika
On Fri, Mar 21, 2008 at 12:51:46PM -0600, Grant Likely wrote: > On Fri, Mar 21, 2008 at 7:14 AM, Matt Sealey <[EMAIL PROTECTED]> wrote: > > Is the MPC5200B PSC-AC97 driver in there? > > Audio drivers need to go in via one of the ALSA developers and I > haven't been paying close enough attention to know if it has not in > yet. BTW, it was reported to me that the ethernet drivers don't get autoloaded by udev. Is this a failure of udev missing the of_plateform support, or a deficiency of the ethernet drivers ? Friendly, Sven Luther ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.
Grant Likely wrote: > Personally, I'm not fond of this approach. There is already some > traction to using the reg-shift property to specify spacing, and I > think it would be appropriate to also define a reg-offset property to > handle the +3 offset and then let the xilinx 16550 nodes use those. Why do we need a reg-offset property when we can just add the offset to the appropriate word(s) in the reg property? Primarily because the device creates 32 byte registers starting at 0; but they are also big-endian byte accessible so a byte read at offset 8 also works. reg-offset seems to be a better description of the hardware to me. Ugh... I was just going is it possible to access the chip registers as 32-bit entities, and employ UPIO_MEM32 mode of 8250.c -- just to avoid that reg-offset wart. Now you're telling everybody that it's completely superfluous... :-) Cheers, g. WBR, Sergei ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/3] [POWERPC] Xilinx: of_serial support for Xilinx uart 16550.
Grant Likely wrote: > Personally, I'm not fond of this approach. There is already some > traction to using the reg-shift property to specify spacing, and I > think it would be appropriate to also define a reg-offset property to > handle the +3 offset and then let the xilinx 16550 nodes use those. That's making things only worse than the mere "reg-shift" idea. I think that both are totally wrong. Everything about the programming interface should be said in the "compatible" and possibly "model" properties. of_serial driver should recognize them and pass the necessary details to 8250.c. As for me, I'm strongly against plaguing the device tree with the *Linux driver implementation specifics* (despite I was trying this with MTD -- there it seemed somewhat more grounded :-). Not true. Compatible defines what the node is describing. It is perfectly valid for a compatible value definition to also defines some additional properties that can be queried for interface details. Xilinx is completely free to define a "xlnx,..." compatible value for their ns16550 compatible device. However, 'sparse' ns16550 devices are a common and well known variation so I think it is valid and reasonable to define a compatible binding for this case. We have been mostly talking about the 16550-compatible devices which external circuitry makes "sparse" so far. This is surely not a property of a 16550 device to be "sparse" or not, although some say that this doesn't matter. :-) As for using a new binding like "sparse16550" instead of extending That "sparse16550" again... what if it's a superset of 16550 (not an uncommon case too), will you also define "sparce16650", "sparse16570", and so on? :-) "ns16550"; it is because reg-shift and reg-offset would be required nodes and therefore is not compatible with drivers using the original ns16550 binding. Using a new namespace gives freedom to define the required properties. You'll have to define several namespaces I'm afraid... Cheers, g. WBR, Sergei ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] POWERPC: Move a.out.h to header-y since it doesn't check __KERNEL__.
Since a.out.h doesn't check the value of __KERNEL__, there's no point in unifdef'ing it. Signed-off-by: Robert P. J. Day <[EMAIL PROTECTED]> --- diff --git a/include/asm-powerpc/Kbuild b/include/asm-powerpc/Kbuild index 5f640e5..7381916 100644 --- a/include/asm-powerpc/Kbuild +++ b/include/asm-powerpc/Kbuild @@ -1,5 +1,6 @@ include include/asm-generic/Kbuild.asm +header-y += a.out.h header-y += auxvec.h header-y += ioctls.h header-y += mman.h @@ -23,7 +24,6 @@ header-y += sigcontext.h header-y += statfs.h header-y += ps3fb.h -unifdef-y += a.out.h unifdef-y += asm-compat.h unifdef-y += bootx.h unifdef-y += byteorder.h Robert P. J. Day Linux Consulting, Training and Annoying Kernel Pedantry: Have classroom, will lecture. http://crashcourse.ca Waterloo, Ontario, CANADA ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sat, 2008-01-26 at 12:55 +0100, Étienne Bersac wrote: > From: Étienne Bersac <[EMAIL PROTECTED]> > > Implement a new driver named windfarm_pm121 which drive fans on PowerMac > 12,1 machine : iMac G5 iSight (rev C) 17" and 20". It's based on > windfarm_pm81 driver from Benjamin Herrenschmidt. Is it just coincidence, or does it only seem to stop the fans when I cat /sys/devices/platform/windfarm.0/cpu-temp ? -- dwmw2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sat, 2008-03-22 at 19:35 +, David Woodhouse wrote: > On Sat, 2008-01-26 at 12:55 +0100, Étienne Bersac wrote: > > From: Étienne Bersac <[EMAIL PROTECTED]> > > > > Implement a new driver named windfarm_pm121 which drive fans on PowerMac > > 12,1 machine : iMac G5 iSight (rev C) 17" and 20". It's based on > > windfarm_pm81 driver from Benjamin Herrenschmidt. > > Is it just coincidence, or does it only seem to stop the fans when I > cat /sys/devices/platform/windfarm.0/cpu-temp ? Is it actually working ? If the SMU thinks there is no fan control done by the OS, it will ramp them up ... but bring them back down when it gets ping. So if for some reason the control loop isn't starting (because it can't find something it wants on this machine), the fans will stay up, but you'll cause such a "ping" when reading the CPU temp. That may be bogus, just thinking what it could be... Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sun, 2008-03-23 at 09:13 +1100, Benjamin Herrenschmidt wrote: > On Sat, 2008-03-22 at 19:35 +, David Woodhouse wrote: > > On Sat, 2008-01-26 at 12:55 +0100, Étienne Bersac wrote: > > > From: Étienne Bersac <[EMAIL PROTECTED]> > > > > > > Implement a new driver named windfarm_pm121 which drive fans on PowerMac > > > 12,1 machine : iMac G5 iSight (rev C) 17" and 20". It's based on > > > windfarm_pm81 driver from Benjamin Herrenschmidt. > > > > Is it just coincidence, or does it only seem to stop the fans when I > > cat /sys/devices/platform/windfarm.0/cpu-temp ? > > Is it actually working ? > > If the SMU thinks there is no fan control done by the OS, it will ramp > them up ... but bring them back down when it gets ping. > > So if for some reason the control loop isn't starting (because it can't > find something it wants on this machine), the fans will stay up, but > you'll cause such a "ping" when reading the CPU temp. Yeah, there's weird shit going on with the sensor/control registration. I think GCC is be miscompiling it -- the sequence of all = all && pm121_register_control(foo...); all = all && pm121_register_control(bar...); is bailing out as soon as 'all' gets set to zero. Despite the fact that pm121_register_control() quite blatantly has side-effects. -- dwmw2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sat, 2008-03-22 at 23:02 +, David Woodhouse wrote: > > Yeah, there's weird shit going on with the sensor/control > registration. > I think GCC is be miscompiling it -- the sequence of > all = all && pm121_register_control(foo...); > all = all && pm121_register_control(bar...); > is bailing out as soon as 'all' gets set to zero. Despite the fact > that pm121_register_control() quite blatantly has side-effects. That's after I fix the names in pm121_new_control() and fix pm121_register_control() to set controls[id] instead of always setting controls[FAN_OD], btw. Without that I don't think it could ever have worked. But it's still broken. I'll try to cut it down and file a GCC bug... .pm121_new_control: .LFB929: .loc 1 883 0 .LVL44: mflr 0 .LCFI38: std 0,16(1) .LCFI39: std 30,-16(1) .LCFI40: std 31,-8(1) .LCFI41: stdu 1,-128(1) .LCFI42: ld 30,[EMAIL PROTECTED](2) mr 31,3 .loc 1 886 0 ld 9,.LC47-.LCTOC1(30) lwz 0,48(9) cmpwi 7,0,0 bne 7,.L31 .LVL45: .loc 1 889 0 ld 4,.LC49-.LCTOC1(30) li 5,2 bl .pm121_register_control cmpdi 7,3,0 beq 7,.L31 .loc 1 890 0 mr 3,31 ld 4,.LC51-.LCTOC1(30) li 5,1 bl .pm121_register_control cmpdi 7,3,0 beq 7,.L31 .loc 1 891 0 mr 3,31 ld 4,.LC53-.LCTOC1(30) li 5,0 bl .pm121_register_control cmpdi 7,3,0 beq 7,.L31 .loc 1 892 0 mr 3,31 ld 4,.LC55-.LCTOC1(30) li 5,3 bl .pm121_register_control cmpdi 7,3,0 beq 7,.L31 .loc 1 895 0 ld 3,.LC57-.LCTOC1(30) bl .printk nop .loc 1 896 0 ld 9,.LC47-.LCTOC1(30) li 0,1 stw 0,48(9) .LVL46: .L31: .loc 1 898 0 addi 1,1,128 ld 0,16(1) mtlr 0 ld 30,-16(1) ld 31,-8(1) .LVL47: blr -- dwmw2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sat, 2008-03-22 at 23:14 +, David Woodhouse wrote: > On Sat, 2008-03-22 at 23:02 +, David Woodhouse wrote: > > > > Yeah, there's weird shit going on with the sensor/control > > registration. > > I think GCC is be miscompiling it -- the sequence of > > all = all && pm121_register_control(foo...); > > all = all && pm121_register_control(bar...); > > is bailing out as soon as 'all' gets set to zero. Despite the fact > > that pm121_register_control() quite blatantly has side-effects. And it's just supposed to do that no ? Ben. > That's after I fix the names in pm121_new_control() and fix > pm121_register_control() to set controls[id] instead of always setting > controls[FAN_OD], btw. Without that I don't think it could ever have > worked. > > But it's still broken. I'll try to cut it down and file a GCC bug... > > .pm121_new_control: > .LFB929: > .loc 1 883 0 > .LVL44: > mflr 0 > .LCFI38: > std 0,16(1) > .LCFI39: > std 30,-16(1) > .LCFI40: > std 31,-8(1) > .LCFI41: > stdu 1,-128(1) > .LCFI42: > ld 30,[EMAIL PROTECTED](2) > mr 31,3 > .loc 1 886 0 > ld 9,.LC47-.LCTOC1(30) > lwz 0,48(9) > cmpwi 7,0,0 > bne 7,.L31 > .LVL45: > .loc 1 889 0 > ld 4,.LC49-.LCTOC1(30) > li 5,2 > bl .pm121_register_control > cmpdi 7,3,0 > beq 7,.L31 > .loc 1 890 0 > mr 3,31 > ld 4,.LC51-.LCTOC1(30) > li 5,1 > bl .pm121_register_control > cmpdi 7,3,0 > beq 7,.L31 > .loc 1 891 0 > mr 3,31 > ld 4,.LC53-.LCTOC1(30) > li 5,0 > bl .pm121_register_control > cmpdi 7,3,0 > beq 7,.L31 > .loc 1 892 0 > mr 3,31 > ld 4,.LC55-.LCTOC1(30) > li 5,3 > bl .pm121_register_control > cmpdi 7,3,0 > beq 7,.L31 > .loc 1 895 0 > ld 3,.LC57-.LCTOC1(30) > bl .printk > nop > .loc 1 896 0 > ld 9,.LC47-.LCTOC1(30) > li 0,1 > stw 0,48(9) > .LVL46: > .L31: > .loc 1 898 0 > addi 1,1,128 > ld 0,16(1) > mtlr 0 > ld 30,-16(1) > ld 31,-8(1) > .LVL47: > blr > > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sat, 22 Mar 2008 23:02:46 + David Woodhouse <[EMAIL PROTECTED]> wrote: > > Yeah, there's weird shit going on with the sensor/control registration. > I think GCC is be miscompiling it -- the sequence of > all = all && pm121_register_control(foo...); > all = all && pm121_register_control(bar...); > is bailing out as soon as 'all' gets set to zero. Despite the fact that > pm121_register_control() quite blatantly has side-effects. Why would you expect otherwise (from the C standard): "Unlike the bitwise binary & operator, the && operator guarantees left-to-right evaluation; there is a sequence point after the evaluation of the first operand. If the first operand compares equal to 0, the second operand is not evaluated." -- Cheers, Stephen Rothwell[EMAIL PROTECTED] http://www.canb.auug.org.au/~sfr/ pgpNXKqidKq1H.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
On Sun, 2008-03-23 at 10:25 +1100, Stephen Rothwell wrote:
> Why would you expect otherwise (from the C standard):
>
> "Unlike the bitwise binary & operator, the && operator guarantees
> left-to-right evaluation; there is a sequence point after the evaluation
> of the first operand. If the first operand compares equal to 0, the
> second operand is not evaluated."
Because it's Saturday night, I wasn't expecting that construct there
(because it seems to require that the sensors/controls are detected in
the precise order that they're checked for, otherwise it'll never match
the ones which are found out of order), and mostly because I'm stupid.
This seems to fix it (note the change of the control names too):
--- drivers/macintosh/windfarm_pm121.c~ 2008-03-22 19:13:22.0 +
+++ drivers/macintosh/windfarm_pm121.c 2008-03-22 23:23:44.0 +
@@ -867,7 +867,7 @@ static struct wf_control* pm121_register
{
if (controls[id] == NULL && !strcmp(ct->name, match)) {
if (wf_get_control(ct) == 0)
- controls[FAN_OD] = ct;
+ controls[id] = ct;
}
return controls[id];
}
@@ -879,10 +879,10 @@ static void pm121_new_control(struct wf_
if (pm121_all_controls_ok)
return;
- all = all && pm121_register_control(ct, "optical-driver-fan", FAN_OD);
- all = all && pm121_register_control(ct, "hard-driver-fan", FAN_HD);
- all = all && pm121_register_control(ct, "cpu-driver-fan", FAN_CPU);
- all = all && pm121_register_control(ct, "cpufreq-clamp", CPUFREQ);
+ all = pm121_register_control(ct, "optical-drive-fan", FAN_OD) && all;
+ all = pm121_register_control(ct, "hard-drive-fan", FAN_HD) && all;
+ all = pm121_register_control(ct, "cpu-fan", FAN_CPU) && all;
+ all = pm121_register_control(ct, "cpufreq-clamp", CPUFREQ) && all;
if (all)
pm121_all_controls_ok = 1;
@@ -909,24 +909,24 @@ static void pm121_new_sensor(struct wf_s
if (pm121_all_sensors_ok)
return;
- all = all && pm121_register_sensor(sr, "cpu-temp",
- &sensor_cpu_temp);
- all = all && pm121_register_sensor(sr, "cpu-current",
- &sensor_cpu_current);
- all = all && pm121_register_sensor(sr, "cpu-voltage",
- &sensor_cpu_voltage);
- all = all && pm121_register_sensor(sr, "cpu-power",
- &sensor_cpu_power);
- all = all && pm121_register_sensor(sr, "hard-drive-temp",
- &sensor_hard_drive_temp);
- all = all && pm121_register_sensor(sr, "optical-drive-temp",
- &sensor_optical_drive_temp);
- all = all && pm121_register_sensor(sr, "incoming-air-temp",
- &sensor_incoming_air_temp);
- all = all && pm121_register_sensor(sr, "north-bridge-temp",
- &sensor_north_bridge_temp);
- all = all && pm121_register_sensor(sr, "gpu-temp",
- &sensor_gpu_temp);
+ all = pm121_register_sensor(sr, "cpu-temp",
+ &sensor_cpu_temp) && all;
+ all = pm121_register_sensor(sr, "cpu-current",
+ &sensor_cpu_current) && all;
+ all = pm121_register_sensor(sr, "cpu-voltage",
+ &sensor_cpu_voltage) && all;
+ all = pm121_register_sensor(sr, "cpu-power",
+ &sensor_cpu_power) && all;
+ all = pm121_register_sensor(sr, "hard-drive-temp",
+ &sensor_hard_drive_temp) && all;
+ all = pm121_register_sensor(sr, "optical-drive-temp",
+ &sensor_optical_drive_temp) && all;
+ all = pm121_register_sensor(sr, "incoming-air-temp",
+ &sensor_incoming_air_temp) && all;
+ all = pm121_register_sensor(sr, "north-bridge-temp",
+ &sensor_north_bridge_temp) && all;
+ all = pm121_register_sensor(sr, "gpu-temp",
+ &sensor_gpu_temp) && all;
if (all)
pm121_all_sensors_ok = 1;
--
dwmw2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
David Woodhouse <[EMAIL PROTECTED]> writes: > - all = all && pm121_register_control(ct, "optical-driver-fan", FAN_OD); > - all = all && pm121_register_control(ct, "hard-driver-fan", FAN_HD); > - all = all && pm121_register_control(ct, "cpu-driver-fan", FAN_CPU); > - all = all && pm121_register_control(ct, "cpufreq-clamp", CPUFREQ); > + all = pm121_register_control(ct, "optical-drive-fan", FAN_OD) && all; > + all = pm121_register_control(ct, "hard-drive-fan", FAN_HD) && all; > + all = pm121_register_control(ct, "cpu-fan", FAN_CPU) && all; > + all = pm121_register_control(ct, "cpufreq-clamp", CPUFREQ) && all; You can also write all &= ... if pm121_register_control always return 0/1 (or make it 0/1 with !!). Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] windfarm: add PowerMac 12,1 support
Hi Dave, On Sat, 22 Mar 2008 23:31:13 + David Woodhouse <[EMAIL PROTECTED]> wrote: > > On Sun, 2008-03-23 at 10:25 +1100, Stephen Rothwell wrote: > > Why would you expect otherwise (from the C standard): > > > > "Unlike the bitwise binary & operator, the && operator guarantees > > left-to-right evaluation; there is a sequence point after the evaluation > > of the first operand. If the first operand compares equal to 0, the > > second operand is not evaluated." > > Because it's Saturday night That seems like all the explanation necessary :-) -- Cheers, Stephen Rothwell[EMAIL PROTECTED] http://www.canb.auug.org.au/~sfr/ pgpwemJ7QTYin.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
