* Re: [PATCH] powerpc/pseries: Make request_ras_irqs() available to other pseries code
From: Michael Ellerman @ 2010-05-19 6:35 UTC (permalink / raw)
To: Mark Nelson; +Cc: Tseng-hui Lin, engebret, linuxppc-dev
In-Reply-To: <201005191635.43544.markn@au1.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
On Wed, 2010-05-19 at 16:35 +1000, Mark Nelson wrote:
> Hi Michael,
>
> Thanks for looking over these patches!
..
> >
> > Existing code I know, but the error handling in here is a little lax,
> > what's not going to work if we miss some or all of the interrupts?
>
> That's a good point. For the existing code, if we miss an EPOW event
> it just means that the event won't be logged (as that's all we do with
> those events at the moment, although there is a comment saying
> that it should be fixed to take appropriate action depending upon the
> type of power failure); but it's a bigger problem if we miss one of the
> RAS errors because then we could miss a fatal event that we should halt
> the machine on. And for the upcoming IO events it's even worse as we'd
> miss an interrupt from the device...
Yeah that's what I was thinking.
> I would do it in a follow-on patch rather than this one, but what would
> be a good course of action if we can't request the interrupt?
Yes a follow on patch is the way to do it.
There shouldn't be that many reasons the request fails, other than
ENOMEM, or broken device tree perhaps. It's definitely worth a
WARN_ON(), people notice those at least.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Make request_ras_irqs() available to other pseries code
From: Mark Nelson @ 2010-05-19 6:54 UTC (permalink / raw)
To: linuxppc-dev, michael; +Cc: Tseng-hui Lin, engebret
In-Reply-To: <1274250954.7574.1.camel@concordia>
On Wednesday 19 May 2010 16:35:54 Michael Ellerman wrote:
> On Wed, 2010-05-19 at 16:35 +1000, Mark Nelson wrote:
> > Hi Michael,
> >
> > Thanks for looking over these patches!
> ..
> > >
> > > Existing code I know, but the error handling in here is a little lax,
> > > what's not going to work if we miss some or all of the interrupts?
> >
> > That's a good point. For the existing code, if we miss an EPOW event
> > it just means that the event won't be logged (as that's all we do with
> > those events at the moment, although there is a comment saying
> > that it should be fixed to take appropriate action depending upon the
> > type of power failure); but it's a bigger problem if we miss one of the
> > RAS errors because then we could miss a fatal event that we should halt
> > the machine on. And for the upcoming IO events it's even worse as we'd
> > miss an interrupt from the device...
>
> Yeah that's what I was thinking.
>
> > I would do it in a follow-on patch rather than this one, but what would
> > be a good course of action if we can't request the interrupt?
>
> Yes a follow on patch is the way to do it.
>
> There shouldn't be that many reasons the request fails, other than
> ENOMEM, or broken device tree perhaps. It's definitely worth a
> WARN_ON(), people notice those at least.
That sounds good. I'll do a simple follow-on patch that adds the
WARN_ON().
Thanks!
Mark
^ permalink raw reply
* Re: [lm-sensors] [PATCH] hwmon: (tmp421) Add nfactor support (2nd attempt)
From: Andre Prendel @ 2010-05-19 7:26 UTC (permalink / raw)
To: Jeff Angielski; +Cc: Jean Delvare, linuxppc-dev, lm-sensors
In-Reply-To: <4BF2A5A9.6010907@theptrgroup.com>
On Tue, May 18, 2010 at 10:35:21AM -0400, Jeff Angielski wrote:
> On 05/18/2010 07:38 AM, Andre Prendel wrote:
>
> > I'd prefer starting i with 0 and as condition i < data->channels - 1.
> >
> > for (i = 0; i < data->channels -1; i++) {
> > data->nfactor[i] = i2c_smbus_read_byte_data(client,
> > TMP421_NFACTOR[i]);
> > }
> >
> > What do you think?
>
> The first channel is the local channel which does not support nfactor,
> so we skip it. Having the for loop start at 1 illustrates this concept.
>
> > Indentation of the arguments is wrong (see other functions).
>
> Should be fixed in the patch below.
Ok, so there is only one remaining task :) Please update the documentation under
Documentation/hwmon/tmp421. Then you will get my Acked-by.
Jean, should we describe this interface in Documentation/hwmon/sysfs-interface
too? For the moment tmp421 is the only driver implementing it.
> I am not sure why the patch file itself still shows the function arguments
> off by one. When I apply it to a raw tmp421.c all of the arguments
> line up correctly.
>
> After rebasing the changes, I am creating the patch as:
>
> $ git format-patch -1 HEAD
>
> Pretty simple stuff...
>
>
>
> >From 467c74e1d8118e34e84a150f18b5e55c6593c777 Mon Sep 17 00:00:00 2001
> From: Jeff Angielski <jeff@theptrgroup.com>
> Date: Mon, 10 May 2010 10:26:34 -0400
> Subject: [PATCH] hwmon: (tmp421) Add nfactor support
>
> Add support for reading and writing the n-factor correction
> registers. This is needed to compensate for the characteristics
> of a particular sensor hanging off of the remote channels.
>
> Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
> ---
> drivers/hwmon/tmp421.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index 738c472..7944627 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
>
> static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
> static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
> +static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
>
> /* Flags */
> #define TMP421_CONFIG_SHUTDOWN 0x40
> @@ -76,6 +77,7 @@ struct tmp421_data {
> int channels;
> u8 config;
> s16 temp[4];
> + s8 nfactor[3];
> };
>
> static int temp_from_s16(s16 reg)
> @@ -115,6 +117,10 @@ static struct tmp421_data *tmp421_update_device(struct device *dev)
> data->temp[i] |= i2c_smbus_read_byte_data(client,
> TMP421_TEMP_LSB[i]);
> }
> + for (i = 1; i < data->channels; i++) {
> + data->nfactor[i - 1] = i2c_smbus_read_byte_data(client,
> + TMP421_NFACTOR[i - 1]);
> + }
> data->last_updated = jiffies;
> data->valid = 1;
> }
> @@ -157,6 +163,32 @@ static ssize_t show_fault(struct device *dev,
> return sprintf(buf, "0\n");
> }
>
> +static ssize_t show_nfactor(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp421_data *data = tmp421_update_device(dev);
> +
> + return sprintf(buf, "%d\n", data->nfactor[index - 1]);
> +}
> +
> +static ssize_t set_nfactor(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tmp421_data *data = i2c_get_clientdata(client);
> + int index = to_sensor_dev_attr(devattr)->index;
> + int nfactor = simple_strtol(buf, NULL, 10);
> +
> + mutex_lock(&data->update_lock);
> + i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index - 1],
> + SENSORS_LIMIT(nfactor, -128, 127));
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
> int n)
> {
> @@ -177,19 +209,28 @@ static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
> static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
> static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
> static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUSR | S_IWUSR | S_IRGRP,
> + show_nfactor, set_nfactor, 1);
> static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
> static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUSR | S_IWUSR | S_IRGRP,
> + show_nfactor, set_nfactor, 2);
> static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
> static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUSR | S_IWUSR | S_IRGRP,
> + show_nfactor, set_nfactor, 3);
>
> static struct attribute *tmp421_attr[] = {
> &sensor_dev_attr_temp1_input.dev_attr.attr,
> &sensor_dev_attr_temp2_input.dev_attr.attr,
> &sensor_dev_attr_temp2_fault.dev_attr.attr,
> + &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
> &sensor_dev_attr_temp3_input.dev_attr.attr,
> &sensor_dev_attr_temp3_fault.dev_attr.attr,
> + &sensor_dev_attr_temp3_nfactor.dev_attr.attr,
> &sensor_dev_attr_temp4_input.dev_attr.attr,
> &sensor_dev_attr_temp4_fault.dev_attr.attr,
> + &sensor_dev_attr_temp4_nfactor.dev_attr.attr,
> NULL
> };
>
Thanks,
Andre
^ permalink raw reply
* [PATCH v2] powerpc/pseries: Make request_ras_irqs() available to other pseries code
From: Mark Nelson @ 2010-05-19 8:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Tseng-hui Lin, engebret
In-Reply-To: <201005191654.42483.markn@au1.ibm.com>
At the moment only the RAS code uses event-sources interrupts (for EPOW
events and internal errors) so request_ras_irqs() (which actually requests
the event-sources interrupts) is found in ras.c and is static.
We want to be able to use event-sources interrupts in other pseries code,
so let's rename request_ras_irqs() to request_event_sources_irqs() and
move it to event_sources.c.
This will be used in an upcoming patch that adds support for IO Event
interrupts that come through as event sources.
Signed-off-by: Mark Nelson <markn@au1.ibm.com>
---
Changes v1 -> v2:
* move the prototype for request_event_sources_irqs() to pseries.h
arch/powerpc/platforms/pseries/Makefile | 2
arch/powerpc/platforms/pseries/event_sources.c | 79 +++++++++++++++++++++++++
arch/powerpc/platforms/pseries/pseries.h | 7 ++
arch/powerpc/platforms/pseries/ras.c | 62 -------------------
4 files changed, 90 insertions(+), 60 deletions(-)
Index: upstream/arch/powerpc/platforms/pseries/event_sources.c
===================================================================
--- /dev/null
+++ upstream/arch/powerpc/platforms/pseries/event_sources.c
@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2001 Dave Engebretsen IBM Corporation
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <asm/prom.h>
+
+#include "pseries.h"
+
+void request_event_sources_irqs(struct device_node *np,
+ irq_handler_t handler,
+ const char *name)
+{
+ int i, index, count = 0;
+ struct of_irq oirq;
+ const u32 *opicprop;
+ unsigned int opicplen;
+ unsigned int virqs[16];
+
+ /* Check for obsolete "open-pic-interrupt" property. If present, then
+ * map those interrupts using the default interrupt host and default
+ * trigger
+ */
+ opicprop = of_get_property(np, "open-pic-interrupt", &opicplen);
+ if (opicprop) {
+ opicplen /= sizeof(u32);
+ for (i = 0; i < opicplen; i++) {
+ if (count > 15)
+ break;
+ virqs[count] = irq_create_mapping(NULL, *(opicprop++));
+ if (virqs[count] == NO_IRQ)
+ printk(KERN_ERR "Unable to allocate interrupt "
+ "number for %s\n", np->full_name);
+ else
+ count++;
+
+ }
+ }
+ /* Else use normal interrupt tree parsing */
+ else {
+ /* First try to do a proper OF tree parsing */
+ for (index = 0; of_irq_map_one(np, index, &oirq) == 0;
+ index++) {
+ if (count > 15)
+ break;
+ virqs[count] = irq_create_of_mapping(oirq.controller,
+ oirq.specifier,
+ oirq.size);
+ if (virqs[count] == NO_IRQ)
+ printk(KERN_ERR "Unable to allocate interrupt "
+ "number for %s\n", np->full_name);
+ else
+ count++;
+ }
+ }
+
+ /* Now request them */
+ for (i = 0; i < count; i++) {
+ if (request_irq(virqs[i], handler, 0, name, NULL)) {
+ printk(KERN_ERR "Unable to request interrupt %d for "
+ "%s\n", virqs[i], np->full_name);
+ return;
+ }
+ }
+}
+
Index: upstream/arch/powerpc/platforms/pseries/Makefile
===================================================================
--- upstream.orig/arch/powerpc/platforms/pseries/Makefile
+++ upstream/arch/powerpc/platforms/pseries/Makefile
@@ -7,7 +7,7 @@ EXTRA_CFLAGS += -DDEBUG
endif
obj-y := lpar.o hvCall.o nvram.o reconfig.o \
- setup.o iommu.o ras.o \
+ setup.o iommu.o event_sources.o ras.o \
firmware.o power.o dlpar.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_XICS) += xics.o
Index: upstream/arch/powerpc/platforms/pseries/pseries.h
===================================================================
--- upstream.orig/arch/powerpc/platforms/pseries/pseries.h
+++ upstream/arch/powerpc/platforms/pseries/pseries.h
@@ -10,6 +10,13 @@
#ifndef _PSERIES_PSERIES_H
#define _PSERIES_PSERIES_H
+#include <linux/interrupt.h>
+
+struct device_node;
+
+extern void request_event_sources_irqs(struct device_node *np,
+ irq_handler_t handler, const char *name);
+
extern void __init fw_feature_init(const char *hypertas, unsigned long len);
struct pt_regs;
Index: upstream/arch/powerpc/platforms/pseries/ras.c
===================================================================
--- upstream.orig/arch/powerpc/platforms/pseries/ras.c
+++ upstream/arch/powerpc/platforms/pseries/ras.c
@@ -67,63 +67,6 @@ static irqreturn_t ras_epow_interrupt(in
static irqreturn_t ras_error_interrupt(int irq, void *dev_id);
-static void request_ras_irqs(struct device_node *np,
- irq_handler_t handler,
- const char *name)
-{
- int i, index, count = 0;
- struct of_irq oirq;
- const u32 *opicprop;
- unsigned int opicplen;
- unsigned int virqs[16];
-
- /* Check for obsolete "open-pic-interrupt" property. If present, then
- * map those interrupts using the default interrupt host and default
- * trigger
- */
- opicprop = of_get_property(np, "open-pic-interrupt", &opicplen);
- if (opicprop) {
- opicplen /= sizeof(u32);
- for (i = 0; i < opicplen; i++) {
- if (count > 15)
- break;
- virqs[count] = irq_create_mapping(NULL, *(opicprop++));
- if (virqs[count] == NO_IRQ)
- printk(KERN_ERR "Unable to allocate interrupt "
- "number for %s\n", np->full_name);
- else
- count++;
-
- }
- }
- /* Else use normal interrupt tree parsing */
- else {
- /* First try to do a proper OF tree parsing */
- for (index = 0; of_irq_map_one(np, index, &oirq) == 0;
- index++) {
- if (count > 15)
- break;
- virqs[count] = irq_create_of_mapping(oirq.controller,
- oirq.specifier,
- oirq.size);
- if (virqs[count] == NO_IRQ)
- printk(KERN_ERR "Unable to allocate interrupt "
- "number for %s\n", np->full_name);
- else
- count++;
- }
- }
-
- /* Now request them */
- for (i = 0; i < count; i++) {
- if (request_irq(virqs[i], handler, 0, name, NULL)) {
- printk(KERN_ERR "Unable to request interrupt %d for "
- "%s\n", virqs[i], np->full_name);
- return;
- }
- }
-}
-
/*
* Initialize handlers for the set of interrupts caused by hardware errors
* and power system events.
@@ -138,14 +81,15 @@ static int __init init_ras_IRQ(void)
/* Internal Errors */
np = of_find_node_by_path("/event-sources/internal-errors");
if (np != NULL) {
- request_ras_irqs(np, ras_error_interrupt, "RAS_ERROR");
+ request_event_sources_irqs(np, ras_error_interrupt,
+ "RAS_ERROR");
of_node_put(np);
}
/* EPOW Events */
np = of_find_node_by_path("/event-sources/epow-events");
if (np != NULL) {
- request_ras_irqs(np, ras_epow_interrupt, "RAS_EPOW");
+ request_event_sources_irqs(np, ras_epow_interrupt, "RAS_EPOW");
of_node_put(np);
}
^ permalink raw reply
* Re: [PATCH] need check for devices with bad status status property in __of_scan_bus()
From: Sonny Rao @ 2010-05-19 8:47 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, anton
In-Reply-To: <20100511011341.GO4036@us.ibm.com>
On Mon, May 10, 2010 at 08:13:41PM -0500, Sonny Rao wrote:
> Hi Ben, we ran into an issue where it looks like we're not
> properly ignoring a pci device with a non-good status property
> when we walk the device tree and create our device nodes.
>
> However, the EEH init code does look for the property and
> disables EEH on these devices. This leaves us in an
> inconsistent where we are poking at a supposedly bad
> piece of hardware and RTAS will block our config cycles
> because EEH isn't enabled anyway.
>
> This has only been compile tested.
>
> Signed-of-by: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
>
> Index: common/arch/powerpc/kernel/pci_of_scan.c
> ===================================================================
> --- common/arch/powerpc/kernel.orig/pci_of_scan.c 2010-05-10 20:00:40.000000000 -0500
> +++ common/arch/powerpc/kernel/pci_of_scan.c 2010-05-10 20:03:04.000000000 -0500
> @@ -310,6 +310,8 @@ static void __devinit __of_scan_bus(stru
> /* Scan direct children */
> for_each_child_of_node(node, child) {
> pr_debug(" * %s\n", child->full_name);
> + if (!of_device_is_available(child))
> + continue;
> reg = of_get_property(child, "reg", ®len);
> if (reg == NULL || reglen < 20)
> continue;
Ok, it's now been actually tested with firmware
that marks some devices as failed and appears to work.
--
Sonny Rao, LTC OzLabs, BML team
^ permalink raw reply
* Re: PATA/legacy IDE subsystem on PowerMac
From: jjDaNiMoTh @ 2010-05-19 9:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1274219376.21352.857.camel@pasglop>
2010/5/18 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
[cut]
> Nothing obvious... the disk seem to be detected properly. Do you have
> the support for mac partitions and the SCSI disk driver (BLK_DEV_SD)
> enabled ?
The PATA modules was built statically, and BLK_DEV_SD as a module (and
not included in the ramfs).
Now it boot correctly, many thanks.
Have a nice day
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
From: Mark Nelson @ 2010-05-19 10:24 UTC (permalink / raw)
To: michael; +Cc: Tseng-hui Lin, linuxppc-dev
In-Reply-To: <1274189851.26143.63.camel@concordia>
On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:
> On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
> > This patch adds support for handling IO Event interrupts which come
> > through at the /event-sources/ibm,io-events device tree node.
>
> Hi Mark,
>
> You'll have to explain to me offline sometime how it is we ran out of
> interrupts and started needing to multiplex them ..
>
> > There is one ibm,io-events interrupt, but this interrupt might be used
> > for multiple I/O devices, each with their own separate driver. So, we
> > create a platform interrupt handler that will do the RTAS check-exception
> > call and then call the appropriate driver's interrupt handler (the one(s)
> > that registered with a scope that matches the scope of the incoming
> > interrupt).
> >
> > So, a driver for a device that uses IO Event interrupts will register
> > it's interrupt service routine (or interrupt handler) with the platform
> > code using ioei_register_isr(). This register function takes a function
> > pointer to the driver's handler and the scope that the driver is
> > interested in (scopes defined in arch/powerpc/include/asm/io_events.h).
> > The driver's handler must take a pointer to a struct io_events_section
> > and must not return anything.
> >
> > The platform code registers io_event_interrupt() as the interrupt handler
> > for the ibm,io-events interrupt. Upon receiving an IO Event interrupt, it
> > checks the scope of the incoming interrupt and only calls those drivers'
> > handlers that have registered as being interested in that scope.
>
> The "checks the scope" requires an RTAS call, which takes a global lock
> (and you add another) - these aren't going to be used for anything
> performance critical I hope?
I've been told they're not performance critical but I'll have to go
back and have a closer look at the locking again - I probably am being
a bit overzealous.
>
> > It is possible for a single driver to register the same function pointer
> > more than once with different scopes if it is interested in more than one
> > type of IO Event interrupt. If a handler wants to be notified of all
> > incoming IO Event interrupts it can register with IOEI_SCOPE_ANY.
> >
> > A driver can unregister to stop receiving the IO Event interrupts using
> > ioei_unregister_isr(), passing it the same function pointer to the
> > driver's handler and the scope the driver was registered with.
> >
> > Signed-off-by: Mark Nelson <markn@au1.ibm.com>
> > ---
> > arch/powerpc/include/asm/io_events.h | 40 +++++
> > arch/powerpc/platforms/pseries/Makefile | 2
> > arch/powerpc/platforms/pseries/io_events.c | 205 +++++++++++++++++++++++++++++
> > 3 files changed, 246 insertions(+), 1 deletion(-)
> >
> > Index: upstream/arch/powerpc/include/asm/io_events.h
> > ===================================================================
> > --- /dev/null
> > +++ upstream/arch/powerpc/include/asm/io_events.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright 2010 IBM Corporation, Mark Nelson
>
> I usually have name, corp, but I'm not sure if it matters.
I'll find out what the officially sanctioned way is :)
>
> > + * 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.
> > + */
> > +
> > +#ifndef _ASM_POWERPC_IOEVENTS_H
> > +#define _ASM_POWERPC_IOEVENTS_H
> > +
> > +struct io_events_section {
> > + u16 id; // Unique section identifier x00-x01
> > + u16 length; // Section length (bytes) x02-x03
> > + u8 version; // Section Version x04-x04
> > + u8 sec_subtype; // Section subtype x05-x05
> > + u16 creator_id; // Creator Component ID x06-x07
> > + u8 event_type; // IO-Event Type x08-x08
> > + u8 rpc_field_len; // PRC Field Length x09-x09
> > + u8 scope; // Error/Event Scope x0A-x0A
> > + u8 event_subtype; // I/O-Event Sub-Type x0B-x0B
> > + u32 drc_index; // DRC Index x0C-x0F
> > + u32 rpc_data[]; // RPC Data (optional) x10-...
> > +};
>
> I know it's idiomatic in that sort of code, but C++ comments?
Yeah, I'm not too phased - I was just copying what lppaca.h did...
>
> > +#define IOEI_SCOPE_NOT_APP 0x00
> > +#define IOEI_SCOPE_RIO_HUB 0x36
> > +#define IOEI_SCOPE_RIO_BRIDGE 0x37
> > +#define IOEI_SCOPE_PHB 0x38
> > +#define IOEI_SCOPE_EADS_GLOBAL 0x39
> > +#define IOEI_SCOPE_EADS_SLOT 0x3A
> > +#define IOEI_SCOPE_TORRENT_HUB 0x3B
> > +#define IOEI_SCOPE_SERVICE_PROC 0x51
> > +#define IOEI_SCOPE_ANY -1
> > +
> > +int ioei_register_isr(void (*isr)(struct io_events_section *), int scope);
> > +int ioei_unregister_isr(void (*isr)(struct io_events_section *), int scope);
>
> Given these are exported to the whole kernel I'd vote for
> pseries_ioei_register_isr(), if I get to vote that is.
That's a very good point - I'll change that.
>
> > Index: upstream/arch/powerpc/platforms/pseries/io_events.c
> > ===================================================================
> > --- /dev/null
> > +++ upstream/arch/powerpc/platforms/pseries/io_events.c
> > @@ -0,0 +1,205 @@
> > +/*
> > + * Copyright 2010 IBM Corporation, Mark Nelson
> > + *
> > + * 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/errno.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of.h>
> > +
> > +#include <asm/io_events.h>
> > +#include <asm/rtas.h>
> > +#include <asm/irq.h>
> > +
> > +#include "event_sources.h"
> > +
> > +struct ioei_consumer {
> > + void (*ioei_isr)(struct io_events_section *);
>
> Function pointers is one case where a typedef can make the code easier
> to read, if you like.
>
> > + int scope;
> > + struct ioei_consumer *next;
> > +};
>
> You forgot the fourth commandment:
> Thou shalt not write thine own linked list implementation when
> there already exist several perfectly good ones in the kernel!
>
> Or is there some good reason for it I'm missing? :)
No, there was no good reason at all here and I do feel stupid as in a
previous patch I fought to use the in kernel implementation. I'll use
a struct list_head for the next version.
>
> You could even use a hlist to save a void * in your list head.
>
> > +static int ioei_check_exception_token;
> > +
> > +static unsigned char ioei_log_buf[RTAS_ERROR_LOG_MAX];
> > +static DEFINE_SPINLOCK(ioei_log_buf_lock);
> > +
> > +static struct ioei_consumer *ioei_isr_list;
> > +static DEFINE_SPINLOCK(ioei_isr_list_lock);
> > +
> > +int ioei_register_isr(void (*isr)(struct io_events_section *), int scope)
> > +{
> > + struct ioei_consumer *iter;
> > + struct ioei_consumer *cons;
> > + int ret = 0;
> > +
> > + spin_lock(&ioei_isr_list_lock);
>
> Here and unregister you should be using spin_lock_irq(), because you
> need to protect against an interrupt and you know you're not being
> called from an irq handler (so you don't need save/restore - though you
> could use it anyway to be lazy :D).
I actually had that right in an earlier version but I can't remember
what possessed me to change it... I'll fix it.
>
> > + /* check to see if we've already registered this function with
> > + * this scope. If we have, don't register it again
> > + */
> > + iter = ioei_isr_list;
> > + while (iter) {
> > + if (iter->ioei_isr == isr && iter->scope == scope)
> > + break;
> > + iter = iter->next;
> > + }
> > +
> > + if (iter) {
> > + ret = -EEXIST;
> > + goto out;
> > + }
> > +
> > + cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
>
> But you don't want to kmalloc while holding the lock and with interrupts
> off.
I could allocate above, before taking the lock, and then if we get the
case where it already exists in the list I could just free it before
returning. Would that work?
>
> > + if (!cons) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + cons->ioei_isr = isr;
> > + cons->scope = scope;
> > +
> > + cons->next = ioei_isr_list;
> > + ioei_isr_list = cons;
> > +
> > +out:
> > + spin_unlock(&ioei_isr_list_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioei_register_isr);
> > +
> > +int ioei_unregister_isr(void (*isr)(struct io_events_section *), int scope)
> > +{
> > + struct ioei_consumer *iter;
> > + struct ioei_consumer *prev = NULL;
> > + int ret = 0;
> > +
> > + spin_lock(&ioei_isr_list_lock);
> > + iter = ioei_isr_list;
> > + while (iter) {
> > + if (iter->ioei_isr == isr && iter->scope == scope)
> > + break;
> > + prev = iter;
> > + iter = iter->next;
> > + }
> > +
> > + if (!iter) {
> > + ret = -ENOENT;
> > + goto out;
> > + }
> > +
> > + if (prev)
> > + prev->next = iter->next;
> > + else
> > + ioei_isr_list = iter->next;
> > +
> > + kfree(iter);
> > +
> > +out:
> > + spin_unlock(&ioei_isr_list_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioei_unregister_isr);
> > +
> > +static void ioei_call_consumers(int scope, struct io_events_section *sec)
> > +{
> > + struct ioei_consumer *iter;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&ioei_isr_list_lock, flags);
>
> You don't need to disable interrupts, because you're being called from
> the interrupt handler, and it won't be called again until you're
> finished.
You're right. Sorry, I think I mixed up the irq saving between the
register/unregister functions and this function. I'll fix it.
>
> > + iter = ioei_isr_list;
> > + while (iter) {
> > + if (iter->scope == scope || iter->scope == IOEI_SCOPE_ANY)
> > + iter->ioei_isr(sec);
> > + iter = iter->next;
> > + }
> > +
> > + spin_unlock_irqrestore(&ioei_isr_list_lock, flags);
> > +}
> > +
> > +#define EXT_INT_VECTOR_OFFSET 0x500
> > +#define RTAS_TYPE_IO_EVENT 0xE1
> > +
> > +static irqreturn_t io_event_interrupt(int irq, void *dev_id)
> > +{
> > + struct rtas_error_log *rtas_elog;
> > + struct io_events_section *ioei_sec;
> > + char *ch_ptr;
> > + int status;
> > + u16 *sec_len;
> > +
> > + spin_lock(&ioei_log_buf_lock);
> > +
> > + status = rtas_call(ioei_check_exception_token, 6, 1, NULL,
> > + EXT_INT_VECTOR_OFFSET,
> > + irq_map[irq].hwirq,
>
> This is going to be slow anyway, you may as well use virq_to_hw().
Oh yeah, good idea
>
> > + RTAS_IO_EVENTS, 1 /*Time Critical */,
>
> Missing space before the T ^
Nice catch!
>
> > + __pa(&ioei_log_buf),
>
> Does the buffer need to be aligned, and/or inside the RMO? I'd guess
> yes.
Good question, I'll look into it
>
> > + rtas_get_error_log_max());
> > +
> > + rtas_elog = (struct rtas_error_log *)ioei_log_buf;
> > +
> > + if (status != 0)
> > + goto out;
> > +
> > + /* We assume that we will only ever get called for io-event
> > + * interrupts. But if we get called with something else
> > + * make some noise about it.
> > + */
>
> That would mean we'd requested the wrong interrupt wouldn't it? I'd
> almost BUG, but benh always bitches that I do that too often so .. :)
Yeah, I don't think this would ever happen so I'm not sure exactly
what I'm protecting against here...
>
> > + if (rtas_elog->type != RTAS_TYPE_IO_EVENT) {
> > + pr_warning("IO Events: We got called with an event type of %d"
> > + " rather than %d!\n", rtas_elog->type,
> > + RTAS_TYPE_IO_EVENT);
> > + WARN_ON(1);
> > + goto out;
> > + }
> > +
> > + /* there are 24 bytes of event log data before the first section
> > + * (Main-A) begins
> > + */
> > + ch_ptr = (char *)ioei_log_buf + 24;
>
> Any reason you're casting from unsigned char to char?
None that I can fathom now... Good catch :)
>
> > + /* loop through all the sections until we get to the IO Events
> > + * Section, with section ID "IE"
> > + */
> > + while (*ch_ptr != 'I' && *(ch_ptr + 1) != 'E') {
> > + sec_len = (u16 *)(ch_ptr + 2);
> > + ch_ptr += *sec_len;
> > + }
>
> This would be neater if you cast to io_events_section and used the
> fields I think.
That's a good idea and would be a lot nicer than the code above.
>
> And even better if you know the length will be a multiple of the
> section_header size, you can do the arithmetic in those terms.
I'll read the docs again and see if we can do that.
>
> > + ioei_sec = (struct io_events_section *)ch_ptr;
> > +
> > + ioei_call_consumers(ioei_sec->scope, ioei_sec);
>
> Guaranteed to be only one section returned to us per call?
My understanding is that there's only ever one, but I'll double check.
>
> We /could/ copy the ioei_sec and drop the buf lock, which would allow
> another interrupt to come in and start doing the RTAS call (on another
> cpu, and iff there are actually multiple interrupts). But we probably
> don't care.
Good point - I'll update it so that we do the copy.
>
> > +out:
> > + spin_unlock(&ioei_log_buf_lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int __init init_ioei_IRQ(void)
>
> Never understood why IRQ always (sometimes) gets caps ..
Hmmm... Just following the pattern from the RAS code...
>
> > +{
> > + struct device_node *np;
> > +
> > + ioei_check_exception_token = rtas_token("check-exception");
>
> Meep, need to check if it actually exists.
Good catch!
>
> > + np = of_find_node_by_path("/event-sources/ibm,io-events");
> > + if (np != NULL) {
>
> if (np) would usually do it
Definitely. I'll update.
>
> > + request_event_sources_irqs(np, io_event_interrupt, "IO_EVENT");
> > + of_node_put(np);
> > + }
> > +
> > + return 0;
> > +}
> > +device_initcall(init_ioei_IRQ);
>
> Should probably be a machine_initcall().
I'll change it to machine_device_initcall?
>
> > Index: upstream/arch/powerpc/platforms/pseries/Makefile
> > ===================================================================
> > --- upstream.orig/arch/powerpc/platforms/pseries/Makefile
> > +++ upstream/arch/powerpc/platforms/pseries/Makefile
> > @@ -8,7 +8,7 @@ endif
> >
> > obj-y := lpar.o hvCall.o nvram.o reconfig.o \
> > setup.o iommu.o event_sources.o ras.o \
> > - firmware.o power.o dlpar.o
> > + firmware.o power.o dlpar.o io_events.o
>
> The BML guys might appreciate an option to turn it off?
I initially had an option that gets selected by PPC_PSERIES, how about
that?
Thanks for going through this!
Mark
^ permalink raw reply
* [PATCH] net/mpc52xx_phy: Various code cleanups
From: Wolfram Sang @ 2010-05-19 10:21 UTC (permalink / raw)
To: linuxppc-dev; +Cc: netdev
- don't free bus->irq (obsoleted by ca816d98170942371535b3e862813b0aba9b7d90)
- don't dispose irqs (should be done in of_mdiobus_register())
- use fec-pointer consistently in transfer()
- use resource_size()
- cosmetic fixes
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
My phyCORE-MPC5200B-tiny still passes its test-suite after the patch
(running via nfsroot).
drivers/net/fec_mpc52xx_phy.c | 24 +++++-------------------
1 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 7658a08..35c90e0 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -29,15 +29,14 @@ static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
int reg, u32 value)
{
struct mpc52xx_fec_mdio_priv *priv = bus->priv;
- struct mpc52xx_fec __iomem *fec;
+ struct mpc52xx_fec __iomem *fec = priv->regs;
int tries = 3;
value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
- fec = priv->regs;
out_be32(&fec->ievent, FEC_IEVENT_MII);
- out_be32(&priv->regs->mii_data, value);
+ out_be32(&fec->mii_data, value);
/* wait for it to finish, this takes about 23 us on lite5200b */
while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
@@ -47,7 +46,7 @@ static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
return -ETIMEDOUT;
return value & FEC_MII_DATA_OP_RD ?
- in_be32(&priv->regs->mii_data) & FEC_MII_DATA_DATAMSK : 0;
+ in_be32(&fec->mii_data) & FEC_MII_DATA_DATAMSK : 0;
}
static int mpc52xx_fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
@@ -69,9 +68,8 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
struct device_node *np = of->node;
struct mii_bus *bus;
struct mpc52xx_fec_mdio_priv *priv;
- struct resource res = {};
+ struct resource res;
int err;
- int i;
bus = mdiobus_alloc();
if (bus == NULL)
@@ -93,7 +91,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
err = of_address_to_resource(np, 0, &res);
if (err)
goto out_free;
- priv->regs = ioremap(res.start, res.end - res.start + 1);
+ priv->regs = ioremap(res.start, resource_size(&res));
if (priv->regs == NULL) {
err = -ENOMEM;
goto out_free;
@@ -118,10 +116,6 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
out_unmap:
iounmap(priv->regs);
out_free:
- for (i=0; i<PHY_MAX_ADDR; i++)
- if (bus->irq[i] != PHY_POLL)
- irq_dispose_mapping(bus->irq[i]);
- kfree(bus->irq);
kfree(priv);
mdiobus_free(bus);
@@ -133,23 +127,16 @@ static int mpc52xx_fec_mdio_remove(struct of_device *of)
struct device *dev = &of->dev;
struct mii_bus *bus = dev_get_drvdata(dev);
struct mpc52xx_fec_mdio_priv *priv = bus->priv;
- int i;
mdiobus_unregister(bus);
dev_set_drvdata(dev, NULL);
-
iounmap(priv->regs);
- for (i=0; i<PHY_MAX_ADDR; i++)
- if (bus->irq[i] != PHY_POLL)
- irq_dispose_mapping(bus->irq[i]);
kfree(priv);
- kfree(bus->irq);
mdiobus_free(bus);
return 0;
}
-
static struct of_device_id mpc52xx_fec_mdio_match[] = {
{ .compatible = "fsl,mpc5200b-mdio", },
{ .compatible = "fsl,mpc5200-mdio", },
@@ -168,5 +155,4 @@ struct of_platform_driver mpc52xx_fec_mdio_driver = {
/* let fec driver call it, since this has to be registered before it */
EXPORT_SYMBOL_GPL(mpc52xx_fec_mdio_driver);
-
MODULE_LICENSE("Dual BSD/GPL");
--
1.7.0
^ permalink raw reply related
* Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
From: Mark Nelson @ 2010-05-19 10:44 UTC (permalink / raw)
To: Sonny Rao; +Cc: linuxppc-dev, miltonm
In-Reply-To: <20100519042744.GA26215@us.ibm.com>
On Wednesday 19 May 2010 14:27:44 Sonny Rao wrote:
> On Tue, 18 May 2010 23:37:31 +1000, Michael Ellerman wrote:
> >
> > On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
> > > This patch adds support for handling IO Event interrupts which come
> > > through at the /event-sources/ibm,io-events device tree node.
> >
> > Hi Mark,
> >
> > You'll have to explain to me offline sometime how it is we ran out of
> > interrupts and started needing to multiplex them ..
>
> Firmware has decided to multiplex all i/o error reporting through a single
> interrupt for reasons unknown, that is the primary reason for this patch.
>
> One question is, we already register a few RAS interrupts which call
> RTAS using check-exception for getting error information. Those live
> in platforms/pseries/ras.c -- should we combine the two into a common
> implementation somehow?
I developed in ras.c but then moved it out just for functional
separation but you could be right in that they should live together.
I'll have another look at it when I've fixed it up.
>
> > > There is one ibm,io-events interrupt, but this interrupt might be used
> > > for multiple I/O devices, each with their own separate driver. So, we
> > > create a platform interrupt handler that will do the RTAS check-exception
> > > call and then call the appropriate driver's interrupt handler (the one(s)
> > > that registered with a scope that matches the scope of the incoming
> > > interrupt).
> > >
> > > So, a driver for a device that uses IO Event interrupts will register
> > > it's interrupt service routine (or interrupt handler) with the platform
> > > code using ioei_register_isr(). This register function takes a function
> > > pointer to the driver's handler and the scope that the driver is
> > > interested in (scopes defined in arch/powerpc/include/asm/io_events.h).
> > > The driver's handler must take a pointer to a struct io_events_section
> > > and must not return anything.
> > >
> > > The platform code registers io_event_interrupt() as the interrupt handler
> > > for the ibm,io-events interrupt. Upon receiving an IO Event interrupt, it
> > > checks the scope of the incoming interrupt and only calls those drivers'
> > > handlers that have registered as being interested in that scope.
> >
> > The "checks the scope" requires an RTAS call, which takes a global lock
> > (and you add another) - these aren't going to be used for anything
> > performance critical I hope?
>
> Nope it shouldn't be performance critical, but it does raise the point
> that the current RTAS implementation in Linux *always* uses the global
> lock. There is a set of calls which are not required to be serialized
> against each other. This would be a totally different patchset to fix that
> problem. The "check-exception" call is one that doesn't require the global
> RTAS lock. I might work on that someday :-)
>
> <snip>
>
> > > + /* check to see if we've already registered this function with
> > > + * this scope. If we have, don't register it again
> > > + */
> > > + iter = ioei_isr_list;
> > > + while (iter) {
> > > + if (iter->ioei_isr == isr && iter->scope == scope)
> > > + break;
> > > + iter = iter->next;
> > > + }
> > > +
> > > + if (iter) {
> > > + ret = -EEXIST;
> > > + goto out;
> > > + }
> > > +
> > > + cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
> >
> > But you don't want to kmalloc while holding the lock and with interrupts
> > off.
>
> Well, he could use GFP_ATOMIC but that's the wrong approach. You should
> allocate the buffer (using GFP_KERNEL) before taking the spin lock.
>
And then free it before returning in the case that we've got a
duplicate, right?
> <snip>
>
> > > +#define EXT_INT_VECTOR_OFFSET 0x500
> > > +#define RTAS_TYPE_IO_EVENT 0xE1
>
> These defines should probably go in <asm/rtas.h>
>
> I noticed the code in ras.c has it's own define too RAS_VECTOR_OFFSET
> for 0x500 and asm/rtas.h actually has RTAS_TYPE_IO for 0xE1
I'm not sure how I missed RTAS_TYPE_IO in <asm/rtas.h> but I'll use
that and then add EXT_INT_VECTOR_OFFSET and update the RAS code to use
it too.
>
> > > +
> > > +static irqreturn_t io_event_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct rtas_error_log *rtas_elog;
> > > + struct io_events_section *ioei_sec;
> > > + char *ch_ptr;
> > > + int status;
> > > + u16 *sec_len;
> > > +
> > > + spin_lock(&ioei_log_buf_lock);
> > > +
> > > + status = rtas_call(ioei_check_exception_token, 6, 1, NULL,
> > > + EXT_INT_VECTOR_OFFSET,
> > > + irq_map[irq].hwirq,
> >
> > This is going to be slow anyway, you may as well use virq_to_hw().
> >
> > > + RTAS_IO_EVENTS, 1 /*Time Critical */,
> >
> > Missing space before the T ^
> >
> > > + __pa(&ioei_log_buf),
> >
> > Does the buffer need to be aligned, and/or inside the RMO? I'd guess
> > yes.
>
> The docs for check-exception don't particularly specify alignment
> requirements, but RTAS in generally going to want it in the RMO
>
> Also, if we're going to go ahead and use rtas_call() which locks
> it's own buffer which meets the requirements, why do we even need
> a separate buffer? Really, we should make this call, then copy
> the content of the buffer before handing it over to the drivers.
That sounds like a good idea, I'll update it to do the copy.
>
>
> > > + rtas_get_error_log_max());
>
> Here, we're passing back what RTAS told us what it's max is
> which doesn't necessarily equal the static buffer size we
> allocated which can cause a buffer overflow. So this
> argument should be the static size of the buffer.
Excellent pickup. Thanks!
>
> > > +
> > > + rtas_elog = (struct rtas_error_log *)ioei_log_buf;
> > > +
> > > + if (status != 0)
> > > + goto out;
> > > +
> > > + /* We assume that we will only ever get called for io-event
> > > + * interrupts. But if we get called with something else
> > > + * make some noise about it.
> > > + */
> >
> > That would mean we'd requested the wrong interrupt wouldn't it? I'd
> > almost BUG, but benh always bitches that I do that too often so .. :)
> >
> > > + if (rtas_elog->type != RTAS_TYPE_IO_EVENT) {
> > > + pr_warning("IO Events: We got called with an event type of %d"
> > > + " rather than %d!\n", rtas_elog->type,
> > > + RTAS_TYPE_IO_EVENT);
> > > + WARN_ON(1);
> > > + goto out;
> > > + }
>
> Should we try to process this instead of just warning?
> The type we get might be one of the the ones we recognize in
> ras.c; so this is an argument for combining ras.c with this code
> or at least report this in the same manner we report any other
> RTAS error log.
I don't think we would every actually get this case occurring but I'll
definitely have a look at the combining idea.
>
> > > +
> > > + /* there are 24 bytes of event log data before the first section
> > > + * (Main-A) begins
> > > + */
> > > + ch_ptr = (char *)ioei_log_buf + 24;
> >
> > Any reason you're casting from unsigned char to char?
> >
> > > + /* loop through all the sections until we get to the IO Events
> > > + * Section, with section ID "IE"
> > > + */
> > > + while (*ch_ptr != 'I' && *(ch_ptr + 1) != 'E') {
> > > + sec_len = (u16 *)(ch_ptr + 2);
> > > + ch_ptr += *sec_len;
> > > + }
> >
> > This would be neater if you cast to io_events_section and used the
> > fields I think.
> >
> > And even better if you know the length will be a multiple of the
> > section_header size, you can do the arithmetic in those terms.
> >
> > > + ioei_sec = (struct io_events_section *)ch_ptr;
> > > +
> > > + ioei_call_consumers(ioei_sec->scope, ioei_sec);
> >
> > Guaranteed to be only one section returned to us per call?
> >
> > We /could/ copy the ioei_sec and drop the buf lock, which would allow
> > another interrupt to come in and start doing the RTAS call (on another
> > cpu, and iff there are actually multiple interrupts). But we probably
> > don't care.
>
> I think we *have* to copy it because we don't want our lock held when we
> call random handlers.
That's a very good point.
>
> > > +out:
> > > + spin_unlock(&ioei_log_buf_lock);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int __init init_ioei_IRQ(void)
> >
> > Never understood why IRQ always (sometimes) gets caps ..
> >
> > > +{
> > > + struct device_node *np;
> > > +
> > > + ioei_check_exception_token = rtas_token("check-exception");
> >
> > Meep, need to check if it actually exists.
> >
> > > + np = of_find_node_by_path("/event-sources/ibm,io-events");
> > > + if (np != NULL) {
> >
> > if (np) would usually do it
> >
> > > + request_event_sources_irqs(np, io_event_interrupt, "IO_EVENT");
> > > + of_node_put(np);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +device_initcall(init_ioei_IRQ);
> >
> > Should probably be a machine_initcall().
> >
> > > Index: upstream/arch/powerpc/platforms/pseries/Makefile
> > > ===================================================================
> > > --- upstream.orig/arch/powerpc/platforms/pseries/Makefile
> > > +++ upstream/arch/powerpc/platforms/pseries/Makefile
> > > @@ -8,7 +8,7 @@ endif
> > >
> > > obj-y := lpar.o hvCall.o nvram.o reconfig.o \
> > > setup.o iommu.o event_sources.o ras.o \
> > > - firmware.o power.o dlpar.o
> > > + firmware.o power.o dlpar.o io_events.o
> >
> > The BML guys might appreciate an option to turn it off?
>
> Or, we might subvert it for our own evil purposes ;-)
Would an option that gets selected by PPC_PSERIES help?
Thanks for checking over this!
Mark
^ permalink raw reply
* Re: [PATCH]460EX on-chip SATA driver <Kernel 2.6.33> < resubmission >
From: Alan Cox @ 2010-05-19 9:53 UTC (permalink / raw)
To: Jassi Brar
Cc: linuxppc-dev, Rupjyoti Sarmah, linux-kernel, linux-ide, sr,
jgarzik
In-Reply-To: <AANLkTiljlMJKimdT-ptwXrMMBhSxFuPdVdyUuKJ8BZve@mail.gmail.com>
On Wed, 19 May 2010 10:49:59 +0900
Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, May 6, 2010 at 2:57 AM, Rupjyoti Sarmah <rsarmah@amcc.com> wrote:
> > This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
>
> The controller seems to be a thrid party IP (from Synopsys) in your
> SoC and there are many chances the IP will appear in some other
> SOCs too. This implementation doesn't seem to take care of that
> scenario.
I'd question if that is worth it seriously. At the point there are
multiple users of the device you know what it will look like. Until then
you neither know if the work is needed nor what will require abstracting.
Lazy evaluation is good for code too ;)
Alan
^ permalink raw reply
* Re: [PATCH]460EX on-chip SATA driver <Kernel 2.6.33> < resubmission >
From: Jassi Brar @ 2010-05-19 11:19 UTC (permalink / raw)
To: Alan Cox
Cc: linuxppc-dev, Rupjyoti Sarmah, linux-kernel, linux-ide, sr,
jgarzik
In-Reply-To: <20100519105305.54804b57@lxorguk.ukuu.org.uk>
On Wed, May 19, 2010 at 6:53 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 19 May 2010 10:49:59 +0900
> Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
>> On Thu, May 6, 2010 at 2:57 AM, Rupjyoti Sarmah <rsarmah@amcc.com> wrote:
>> > This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
>>
>> The controller seems to be a thrid party IP (from Synopsys) in your
>> SoC and there are many chances the IP will appear in some other
>> SOCs too. This implementation doesn't seem to take care of that
>> scenario.
>
> I'd question if that is worth it seriously. At the point there are
> multiple users of the device you know what it will look like. Until then
> you neither know if the work is needed nor what will require abstracting.
Well, am to start writing driver for some dwc sata core in near
future, not sure of the
exact version because I've not yet had my hands on the specs.
Even if my version happens to be different, we don't wanna count on such odds.
So I believe it's worthwhile.
Btw, I presented two options and the author can always choose the
first one - rename
the driver as SoC specific. And in a way, I'd prefer that :)
> Lazy evaluation is good for code too ;)
Apart from increasing re-usability, that POV brings out neat code too.
^ permalink raw reply
* enabling PCIe on powerpc device tree
From: Thirumalai @ 2010-05-19 11:22 UTC (permalink / raw)
To: devicetree-discuss; +Cc: linuxppc-dev
Hi,
How to enable PCIe interrupt on device tree. I am using linux-2.6.30 on
MPC8640D based Target board.
-Thirumalai
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
From: Michael Ellerman @ 2010-05-19 12:02 UTC (permalink / raw)
To: Mark Nelson; +Cc: Tseng-hui Lin, linuxppc-dev
In-Reply-To: <201005192024.02780.markn@au1.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]
On Wed, 2010-05-19 at 20:24 +1000, Mark Nelson wrote:
> On Tuesday 18 May 2010 23:37:31 Michael Ellerman wrote:
> > On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
> >
> > > + /* check to see if we've already registered this function with
> > > + * this scope. If we have, don't register it again
> > > + */
> > > + iter = ioei_isr_list;
> > > + while (iter) {
> > > + if (iter->ioei_isr == isr && iter->scope == scope)
> > > + break;
> > > + iter = iter->next;
> > > + }
> > > +
> > > + if (iter) {
> > > + ret = -EEXIST;
> > > + goto out;
> > > + }
> > > +
> > > + cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
> >
> > But you don't want to kmalloc while holding the lock and with interrupts
> > off.
>
> I could allocate above, before taking the lock, and then if we get the
> case where it already exists in the list I could just free it before
> returning. Would that work?
Yeah I think so, optimise for the normal case where it doesn't already
exist. The other option would be to take the lock, check, do the alloc,
retake the lock and recheck - but that's a pain and not really worth the
trouble.
> > > + ioei_sec = (struct io_events_section *)ch_ptr;
> > > +
> > > + ioei_call_consumers(ioei_sec->scope, ioei_sec);
> >
> > Guaranteed to be only one section returned to us per call?
>
> My understanding is that there's only ever one, but I'll double check.
OK good to check. Could be worth checking in the code, unless it's going
to be really expensive.
> >
> > We /could/ copy the ioei_sec and drop the buf lock, which would allow
> > another interrupt to come in and start doing the RTAS call (on another
> > cpu, and iff there are actually multiple interrupts). But we probably
> > don't care.
>
> Good point - I'll update it so that we do the copy.
Sounds like we should. It's not such a concern to call the handlers with
the lock held IMHO (Sonny raised that), as long as the handlers don't
try and register/unregister themselves. But that will be pretty obvious
if it happens.
> > > + request_event_sources_irqs(np, io_event_interrupt, "IO_EVENT");
> > > + of_node_put(np);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +device_initcall(init_ioei_IRQ);
> >
> > Should probably be a machine_initcall().
>
> I'll change it to machine_device_initcall?
Yep.
> > > Index: upstream/arch/powerpc/platforms/pseries/Makefile
> > > ===================================================================
> > > --- upstream.orig/arch/powerpc/platforms/pseries/Makefile
> > > +++ upstream/arch/powerpc/platforms/pseries/Makefile
> > > @@ -8,7 +8,7 @@ endif
> > >
> > > obj-y := lpar.o hvCall.o nvram.o reconfig.o \
> > > setup.o iommu.o event_sources.o ras.o \
> > > - firmware.o power.o dlpar.o
> > > + firmware.o power.o dlpar.o io_events.o
> >
> > The BML guys might appreciate an option to turn it off?
>
> I initially had an option that gets selected by PPC_PSERIES, how about
> that?
Select is not great because it disregards dependencies, and BML is
PSERIES. Probably just have an option that depends on PSERIES and is
default y.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
From: Michael Ellerman @ 2010-05-19 12:10 UTC (permalink / raw)
To: Sonny Rao; +Cc: markn, linuxppc-dev, miltonm
In-Reply-To: <20100519042744.GA26215@us.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 4130 bytes --]
On Tue, 2010-05-18 at 23:27 -0500, Sonny Rao wrote:
> On Tue, 18 May 2010 23:37:31 +1000, Michael Ellerman wrote:
> >
> > On Mon, 2010-05-17 at 22:53 +1000, Mark Nelson wrote:
> > > This patch adds support for handling IO Event interrupts which come
> > > through at the /event-sources/ibm,io-events device tree node.
> >
> > Hi Mark,
> >
> > You'll have to explain to me offline sometime how it is we ran out of
> > interrupts and started needing to multiplex them ..
>
> Firmware has decided to multiplex all i/o error reporting through a single
> interrupt for reasons unknown, that is the primary reason for this patch.
Yeah, I just don't get why we would do that, but hey whatever.
> > The "checks the scope" requires an RTAS call, which takes a global lock
> > (and you add another) - these aren't going to be used for anything
> > performance critical I hope?
>
> Nope it shouldn't be performance critical, but it does raise the point
> that the current RTAS implementation in Linux *always* uses the global
> lock. There is a set of calls which are not required to be serialized
> against each other. This would be a totally different patchset to fix that
> problem. The "check-exception" call is one that doesn't require the global
> RTAS lock. I might work on that someday :-)
Aha, that's kind of what I was wondering. I take it PAPR documents which
calls need to be serialised and which don't?
> > > + cons = kmalloc(sizeof(struct ioei_consumer), GFP_KERNEL);
> >
> > But you don't want to kmalloc while holding the lock and with interrupts
> > off.
>
> Well, he could use GFP_ATOMIC but that's the wrong approach. You should
> allocate the buffer (using GFP_KERNEL) before taking the spin lock.
True, this is not a good example of when to use GFP_ATOMIC though :)
> Also, if we're going to go ahead and use rtas_call() which locks
> it's own buffer which meets the requirements, why do we even need
> a separate buffer? Really, we should make this call, then copy
> the content of the buffer before handing it over to the drivers.
But another CPU could rtas_call() and blow away our buffer after we've
dropped the RTAS lock but before we've used the content of the buffer.
> > > + if (rtas_elog->type != RTAS_TYPE_IO_EVENT) {
> > > + pr_warning("IO Events: We got called with an event type of %d"
> > > + " rather than %d!\n", rtas_elog->type,
> > > + RTAS_TYPE_IO_EVENT);
> > > + WARN_ON(1);
> > > + goto out;
> > > + }
>
> Should we try to process this instead of just warning?
> The type we get might be one of the the ones we recognize in
> ras.c; so this is an argument for combining ras.c with this code
> or at least report this in the same manner we report any other
> RTAS error log.
We could, but that would be a massive firmware bug - not that it
wouldn't happen, but it would be Not Our Problem TM.
> > We /could/ copy the ioei_sec and drop the buf lock, which would allow
> > another interrupt to come in and start doing the RTAS call (on another
> > cpu, and iff there are actually multiple interrupts). But we probably
> > don't care.
>
> I think we *have* to copy it because we don't want our lock held when we
> call random handlers.
They're not really random, and as long as they don't call the
register/unregister routines it should be /OK/. But copying is probably
good so we don't hold the lock for too long.
> > > Index: upstream/arch/powerpc/platforms/pseries/Makefile
> > > ===================================================================
> > > --- upstream.orig/arch/powerpc/platforms/pseries/Makefile
> > > +++ upstream/arch/powerpc/platforms/pseries/Makefile
> > > @@ -8,7 +8,7 @@ endif
> > >
> > > obj-y := lpar.o hvCall.o nvram.o reconfig.o \
> > > setup.o iommu.o event_sources.o ras.o \
> > > - firmware.o power.o dlpar.o
> > > + firmware.o power.o dlpar.o io_events.o
> >
> > The BML guys might appreciate an option to turn it off?
>
> Or, we might subvert it for our own evil purposes ;-)
I can only imagine :)
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] rtasd: Don't start event scan if scan rate is zero
From: Michael Ellerman @ 2010-05-19 12:12 UTC (permalink / raw)
To: linuxppc-dev
There appear to be Pegasos systems which have the rtas-event-scan
RTAS tokens, but on which the event scan always fails. They also
have an event-scan-rate property containing 0, which means call
event scan 0 times per minute.
So interpret a scan rate of 0 to mean don't scan at all. This fixes
the problem on the Pegasos machines and makes sense as well.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/kernel/rtasd.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index e907ca6..638883e 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -490,6 +490,12 @@ static int __init rtas_init(void)
return -ENODEV;
}
+ if (!rtas_event_scan_rate) {
+ /* Broken firmware: take a rate of zero to mean don't scan */
+ printk(KERN_DEBUG "rtasd: scan rate is 0, not scanning\n");
+ return 0;
+ }
+
/* Make room for the sequence number */
rtas_error_log_max = rtas_get_error_log_max();
rtas_error_log_buffer_max = rtas_error_log_max + sizeof(int);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] device-tree: Drop properties with "/" in their name
From: Michael Ellerman @ 2010-05-19 12:32 UTC (permalink / raw)
To: lists; +Cc: linuxppc-dev, monstr, microblaze-uclinux
Some bogus firmwares include properties with "/" in their name. This
causes problems when creating the /proc/device-tree file system,
because the slash is taken to indicate a directory.
We don't care about those properties, and we don't want to encourage
them, so just throw them away when creating /proc/device-tree.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
Hi Christian, if you could test this new patch that'd be great, thanks!
fs/proc/proc_devtree.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index ce94801..d9396a4 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -209,6 +209,9 @@ void proc_device_tree_add_node(struct device_node *np,
for (pp = np->properties; pp != NULL; pp = pp->next) {
p = pp->name;
+ if (strchr(p, '/'))
+ continue;
+
if (duplicate_name(de, p))
p = fixup_name(np, de, p);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] powerpc: Use common cpu_die (fixes SMP+SUSPEND build)
From: Anton Vorontsov @ 2010-05-19 12:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Milton Miller, linuxppc-dev
From: Milton Miller <miltonm@bga.com>
Configuring a powerpc 32 bit kernel for both SMP and SUSPEND turns on
CPU_HOTPLUG to enable disable_nonboot_cpus to be called by the common
suspend code. Previously the definition of cpu_die for ppc32 was in
the powermac platform code, causing it to be undefined if that platform
as not selected.
arch/powerpc/kernel/built-in.o: In function 'cpu_idle':
arch/powerpc/kernel/idle.c:98: undefined reference to 'cpu_die'
Move the code from setup_64 to smp.c and rename the power mac
versions to their specific names.
Note that this does not setup the cpu_die pointers in either
smp_ops (request a given cpu die) or ppc_md (make this cpu die),
for other platforms but there are generic versions in smp.c.
Reported-by: Matt Sealey <matt@genesi-usa.com>
Reported-by: Kumar Gala <galak@kernel.crashing.org>
Signed-off-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
arch/powerpc/kernel/setup_64.c | 6 ------
arch/powerpc/kernel/smp.c | 6 ++++++
arch/powerpc/platforms/powermac/pmac.h | 2 ++
arch/powerpc/platforms/powermac/setup.c | 11 ++++++++---
arch/powerpc/platforms/powermac/smp.c | 5 +++--
5 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 9143891..cea6698 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -573,12 +573,6 @@ void ppc64_boot_msg(unsigned int src, const char *msg)
printk("[boot]%04x %s\n", src, msg);
}
-void cpu_die(void)
-{
- if (ppc_md.cpu_die)
- ppc_md.cpu_die();
-}
-
#ifdef CONFIG_SMP
#define PCPU_DYN_SIZE ()
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index c2ee144..a3b8d61 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -631,4 +631,10 @@ void cpu_hotplug_driver_unlock()
{
mutex_unlock(&powerpc_cpu_hotplug_driver_mutex);
}
+
+void cpu_die(void)
+{
+ if (ppc_md.cpu_die)
+ ppc_md.cpu_die();
+}
#endif
diff --git a/arch/powerpc/platforms/powermac/pmac.h b/arch/powerpc/platforms/powermac/pmac.h
index 3362e78..f0bc08f 100644
--- a/arch/powerpc/platforms/powermac/pmac.h
+++ b/arch/powerpc/platforms/powermac/pmac.h
@@ -33,6 +33,8 @@ extern void pmac_setup_pci_dma(void);
extern void pmac_check_ht_link(void);
extern void pmac_setup_smp(void);
+extern void pmac32_cpu_die(void);
+extern void low_cpu_die(void) __attribute__((noreturn));
extern int pmac_nvram_init(void);
extern void pmac_pic_init(void);
diff --git a/arch/powerpc/platforms/powermac/setup.c b/arch/powerpc/platforms/powermac/setup.c
index 15c2241..8b42476 100644
--- a/arch/powerpc/platforms/powermac/setup.c
+++ b/arch/powerpc/platforms/powermac/setup.c
@@ -646,7 +646,7 @@ static int pmac_pci_probe_mode(struct pci_bus *bus)
/* access per cpu vars from generic smp.c */
DECLARE_PER_CPU(int, cpu_state);
-static void pmac_cpu_die(void)
+static void pmac64_cpu_die(void)
{
/*
* turn off as much as possible, we'll be
@@ -717,8 +717,13 @@ define_machine(powermac) {
.pcibios_after_init = pmac_pcibios_after_init,
.phys_mem_access_prot = pci_phys_mem_access_prot,
#endif
-#if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PPC64)
- .cpu_die = pmac_cpu_die,
+#ifdef CONFIG_HOTPLUG_CPU
+#ifdef CONFIG_PPC64
+ .cpu_die = pmac64_cpu_die,
+#endif
+#ifdef CONFIG_PPC32
+ .cpu_die = pmac32_cpu_die,
+#endif
#endif
#if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PPC32)
.cpu_die = generic_mach_cpu_die,
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 6898e82..64f3a46 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -53,6 +53,8 @@
#include <asm/pmac_low_i2c.h>
#include <asm/pmac_pfunc.h>
+#include "pmac.h"
+
#undef DEBUG
#ifdef DEBUG
@@ -878,10 +880,9 @@ int smp_core99_cpu_disable(void)
return 0;
}
-extern void low_cpu_die(void) __attribute__((noreturn)); /* in sleep.S */
static int cpu_dead[NR_CPUS];
-void cpu_die(void)
+void pmac32_cpu_die(void)
{
local_irq_disable();
cpu_dead[smp_processor_id()] = 1;
--
1.7.0.5
^ permalink raw reply related
* Re: [lm-sensors] [PATCH] hwmon: (tmp421) Add nfactor support (2nd attempt)
From: Jean Delvare @ 2010-05-19 13:16 UTC (permalink / raw)
To: Andre Prendel; +Cc: linuxppc-dev, lm-sensors
In-Reply-To: <20100519072659.GA2069@andre-laptop>
Hi Andre,
On Wed, 19 May 2010 09:26:59 +0200, Andre Prendel wrote:
> Jean, should we describe this interface in Documentation/hwmon/sysfs-interface
> too? For the moment tmp421 is the only driver implementing it.
If the values are expressed in a standard unit (i.e. not raw register
values) then yes, it would be nice to have a standard interface
described for these files.
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH RT] ehea: make receive irq handler non-threaded (IRQF_NODELAY)
From: Darren Hart @ 2010-05-19 14:16 UTC (permalink / raw)
To: michael
Cc: Jan-Bernd Themann, dvhltc, linux-kernel, Will Schmidt, Brian King,
niv, Thomas Gleixner, Doug Maxey, linuxppc-dev
In-Reply-To: <1274232324.29980.9.camel@concordia>
On 05/18/2010 06:25 PM, Michael Ellerman wrote:
> On Tue, 2010-05-18 at 15:22 -0700, Darren Hart wrote:
>> On 05/18/2010 02:52 PM, Brian King wrote:
>>> Is IRQF_NODELAY something specific to the RT kernel? I don't see it in mainline...
>>
>> Yes, it basically says "don't make this handler threaded".
>
> That is a good fix for EHEA, but the threaded handling is still broken
> for anything else that is edge triggered isn't it?
No, I don't believe so. Edge triggered interrupts that are reported as
edge triggered interrupts will use the edge handler (which was the
approach Sebastien took to make this work back in 2008). Since XICS
presents all interrupts as Level Triggered, they use the fasteoi path.
>
> The result of the discussion about two years ago on this was that we
> needed a custom flow handler for XICS on RT.
I'm still not clear on why the ultimate solution wasn't to have XICS
report edge triggered as edge triggered. Probably some complexity of the
entire power stack that I am ignorant of.
> Apart from the issue of loosing interrupts there is also the fact that
> masking on the XICS requires an RTAS call which takes a global lock.
Right, one of may reasons why we felt this was the right fix. The other
is that there is no real additional overhead in running this as
non-threaded since the receive handler is so short (just napi_schedule()).
--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
^ permalink raw reply
* Re: [PATCH RT] ehea: make receive irq handler non-threaded (IRQF_NODELAY)
From: Thomas Gleixner @ 2010-05-19 14:38 UTC (permalink / raw)
To: Darren Hart
Cc: Jan-Bernd Themann, dvhltc, linux-kernel, Will Schmidt, Brian King,
niv, Doug Maxey, linuxppc-dev
In-Reply-To: <4BF3F2DB.7030701@us.ibm.com>
On Wed, 19 May 2010, Darren Hart wrote:
> On 05/18/2010 06:25 PM, Michael Ellerman wrote:
> > On Tue, 2010-05-18 at 15:22 -0700, Darren Hart wrote:
> > > On 05/18/2010 02:52 PM, Brian King wrote:
> > > > Is IRQF_NODELAY something specific to the RT kernel? I don't see it in
> > > > mainline...
> > >
> > > Yes, it basically says "don't make this handler threaded".
> >
> > That is a good fix for EHEA, but the threaded handling is still broken
> > for anything else that is edge triggered isn't it?
>
> No, I don't believe so. Edge triggered interrupts that are reported as edge
> triggered interrupts will use the edge handler (which was the approach
> Sebastien took to make this work back in 2008). Since XICS presents all
> interrupts as Level Triggered, they use the fasteoi path.
I wonder whether the XICS interrupts which are edge type can be
identified from the irq_desc->flags. Then we could avoid the masking
for those in the fasteoi_handler in general.
> >
> > The result of the discussion about two years ago on this was that we
> > needed a custom flow handler for XICS on RT.
>
> I'm still not clear on why the ultimate solution wasn't to have XICS report
> edge triggered as edge triggered. Probably some complexity of the entire power
> stack that I am ignorant of.
>
> > Apart from the issue of loosing interrupts there is also the fact that
> > masking on the XICS requires an RTAS call which takes a global lock.
Right, I'd love to avoid that but with real level interrupts we'd run
into an interrupt storm. Though another solution would be to issue the
EOI after the threaded handler finished, that'd work as well, but
needs testing.
> Right, one of may reasons why we felt this was the right fix. The other is
> that there is no real additional overhead in running this as non-threaded
> since the receive handler is so short (just napi_schedule()).
Yes, in the case at hand it's the right thing to do, as we avoid
another wakeup/context switch.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] powerpc: Add i8042 keyboard and mouse irq parsing
From: Grant Likely @ 2010-05-19 16:00 UTC (permalink / raw)
To: Martyn Welch; +Cc: Dmitry Torokhov, linux-input, linuxppc-dev
In-Reply-To: <4BED2859.3050204@ge.com>
On Fri, May 14, 2010 at 4:39 AM, Martyn Welch <martyn.welch@ge.com> wrote:
> Martyn Welch wrote:
>> Currently the irqs for the i8042, which historically provides keyboard a=
nd
>> mouse (aux) support, is hardwired in the driver rather than parsing the
>> dts.
>>
>> In addition the interrupts are provided in the dts, but in a way that is
>> not easily parsable using irq_of_parse_and_map().
>>
>> This patch modifies the powerpc legacy IO code to attempt to parse the
>> device tree for this information, failing back to the hardcoded values i=
f
>> it fails. For this to succeed the interrupts for the keyboard and mouse
>> ports need to be moved from the parent i8042 node to the individual port
>> nodes.
>>
>> Signed-off-by: Martyn Welch <martyn.welch@ge.com>
>> ---
>>
>> To get irq_of_parse_and_map() to successfully parse the interrupts, I ha=
d
>> to do this to my device tree:
>>
>> @@ -120,16 +120,17 @@
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address=
-cells =3D <1>;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D =
<1 0x60 0x1
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A01 0x64 0x1>;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupts=
=3D <1 1 12 1>;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrup=
t-parent =3D <&lpc_pic>;
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 keyboard=
@0 {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 reg =3D <0x0>;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 interrupts =3D <1 1>;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 compatible =3D "pnpPNP,303";
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mouse@1 =
{
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 reg =3D <0x1>;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 interrupts =3D <12 1>;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 compatible =3D "pnpPNP,f03";
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>>
>> I'm not sure how to parse for the correct interrupt if I don't do this. =
I
>> this is incorrect and someone could advise me on how the existing device
>> tree layout can be properly parsed, I'll happily modify this patch.
Call of_irq_parse_and_map() on the parent node, using the second
parameter to specify if you want the first or second irq. You get the
parent node with of_get_parent().
Although looking at this fragment, I don't understand why the i8042
node doesn't have its own compatible property. Maybe there is
something historical that I'm missing here.
g.
^ permalink raw reply
* Re: [Patch v2 1/2] 5200/mpc: improve i2c bus error recovery
From: Grant Likely @ 2010-05-19 16:02 UTC (permalink / raw)
To: Albrecht Dreß
Cc: Linux PPC Development, Devicetree Discussions,
Ben Dooks (embedded platforms), Ira W. Snyder
In-Reply-To: <1274032079.1729.3@antares>
On Sun, May 16, 2010 at 11:47 AM, Albrecht Dre=DF <albrecht.dress@arcor.de>=
wrote:
> Am 06.05.10 20:06 schrieb(en) Grant Likely:
>>
>> > I think, though, the whole stuff has been discussed in depth in
>> > February, so
>> > I do not understand why it's still pending as "new". =A0Grant, did we =
miss
>> > something here?
>>
>> I generally let subsystem maintainers pick up the device driver
>> patches for embedded platforms I'm responsible for unless I'm asked to
>> do otherwise. =A0I think ben has asked me to take these through my tree.
>
> That's <http://git.secretlab.ca/?p=3Dlinux-2.6.git;a=3Dshortlog>, isn't i=
t?
> =A0Hmmm, didn't find it there... :-/
Ugh... Stupid typing too fast. I meant to say, "I *don't* think ben
has asked me to take..."
Well this leaves a bit of a mess. I'll make sure it gets in one way or ano=
ther.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] [RFC] Xilinx Virtex 4 FX Soft FPU support
From: Grant Likely @ 2010-05-19 16:52 UTC (permalink / raw)
To: Sergey Temerkhanov; +Cc: linuxppc-dev
In-Reply-To: <201003191849.03717.temerkhanov@cifronik.ru>
On Fri, Mar 19, 2010 at 9:49 AM, Sergey Temerkhanov
<temerkhanov@cifronik.ru> wrote:
> The patch.
>
> Regards, Resgey Temerkhanov
Hi Sergey. Comments below.
> diff -r 9d9ac97e095d .config
> --- a/.config Thu Feb 25 21:23:42 2010 +0300
> +++ b/.config Thu Feb 25 21:49:02 2010 +0300
.config changes should not appear in your patch file.
> diff -r 9d9ac97e095d arch/powerpc/include/asm/ppc_asm.h
> --- a/arch/powerpc/include/asm/ppc_asm.h Thu Feb 25 21:23:42 2010 +0300
> +++ b/arch/powerpc/include/asm/ppc_asm.h Thu Feb 25 21:49:02 2010 +0300
> @@ -85,13 +85,23 @@
> #define REST_8GPRS(n, base) REST_4GPRS(n, base); REST_4GPRS(n+4, base)
> #define REST_10GPRS(n, base) REST_8GPRS(n, base); REST_2GPRS(n+8, base)
>
> -#define SAVE_FPR(n, base) stfd n,THREAD_FPR0+8*TS_FPRWIDTH*(n)(base)
> +
> +#ifdef CONFIG_XILINX_FPU
> +#define stfr stfs
> +#define lfr lfs
> +#else
> +#define stfr stfd
> +#define lfr lfd
> +#endif
the stfr/lfr redirect is a little weird. Why not simply:
> +
> +#ifdef CONFIG_XILINX_FPU
> +#define SAVE_FPR(n, base) stfs n,THREAD_FPR0+8*TS_FPRWIDTH*(n)(base)
> +#define REST_FPR(n, base) lfs n,THREAD_FPR0+8*TS_FPRWIDTH*(n)(base)
> +#else
> +#define SAVE_FPR(n, base) stfd n,THREAD_FPR0+8*TS_FPRWIDTH*(n)(base)
> +#define REST_FPR(n, base) lfd n,THREAD_FPR0+8*TS_FPRWIDTH*(n)(base)
> +#endif
A comment here describing that only single precision works with the
XILINX 405 FPU wouldn't go amiss either.
> diff -r 9d9ac97e095d arch/powerpc/kernel/fpu.S
> --- a/arch/powerpc/kernel/fpu.S Thu Feb 25 21:23:42 2010 +0300
> +++ b/arch/powerpc/kernel/fpu.S Thu Feb 25 21:49:02 2010 +0300
> @@ -57,6 +57,9 @@
> _GLOBAL(load_up_fpu)
> mfmsr r5
> ori r5,r5,MSR_FP
> +#ifdef CONFIG_XILINX_FPU
> + oris r5,r5,MSR_VEC@h
> +#endif
So AltiVec is being enabled here, but double precision is not
supported? What instructions are supported?
Also, please stick with the same whitespace convention used in the
lines above (tab indent instead of a space). Again, a comment would
not go amiss.
> #ifdef CONFIG_VSX
> BEGIN_FTR_SECTION
> oris r5,r5,MSR_VSX@h
> @@ -85,6 +88,9 @@
> toreal(r5)
> PPC_LL r4,_MSR-STACK_FRAME_OVERHEAD(r5)
> li r10,MSR_FP|MSR_FE0|MSR_FE1
> +#ifdef CONFIG_XILINX_FPU
> + oris r10,r10,MSR_VEC@h
> +#endif
> andc r4,r4,r10 /* disable FP for previous task */
> PPC_STL r4,_MSR-STACK_FRAME_OVERHEAD(r5)
> 1:
> @@ -94,6 +100,9 @@
> mfspr r5,SPRN_SPRG3 /* current task's THREAD (phys) */
> lwz r4,THREAD_FPEXC_MODE(r5)
> ori r9,r9,MSR_FP /* enable FP for current */
> +#ifdef CONFIG_XILINX_FPU
> + oris r9,r9,MSR_VEC@h
> +#endif
> or r9,r9,r4
> #else
> ld r4,PACACURRENT(r13)
> @@ -124,6 +133,9 @@
> _GLOBAL(giveup_fpu)
> mfmsr r5
> ori r5,r5,MSR_FP
> +#ifdef CONFIG_XILINX_FPU
> + oris r5,r5,MSR_VEC@h
> +#endif
> #ifdef CONFIG_VSX
> BEGIN_FTR_SECTION
> oris r5,r5,MSR_VSX@h
> @@ -145,6 +157,9 @@
> beq 1f
> PPC_LL r4,_MSR-STACK_FRAME_OVERHEAD(r5)
> li r3,MSR_FP|MSR_FE0|MSR_FE1
> +#ifdef CONFIG_XILINX_FPU
> + oris r3,r3,MSR_VEC@h
> +#endif
> #ifdef CONFIG_VSX
> BEGIN_FTR_SECTION
> oris r3,r3,MSR_VSX@h
> diff -r 9d9ac97e095d arch/powerpc/kernel/head_40x.S
> --- a/arch/powerpc/kernel/head_40x.S Thu Feb 25 21:23:42 2010 +0300
> +++ b/arch/powerpc/kernel/head_40x.S Thu Feb 25 21:49:02 2010 +0300
> @@ -420,7 +420,19 @@
> addi r3,r1,STACK_FRAME_OVERHEAD
> EXC_XFER_STD(0x700, program_check_exception)
>
> +/* 0x0800 - FPU unavailable Exception */
> +#ifdef CONFIG_PPC_FPU
> + START_EXCEPTION(0x0800, FloatingPointUnavailable)
> + NORMAL_EXCEPTION_PROLOG
> + beq 1f; \
> + bl load_up_fpu; /* if from user, just load it up */ \
> + b fast_exception_return; \
> +1: addi r3,r1,STACK_FRAME_OVERHEAD; \
> + EXC_XFER_EE_LITE(0x800, kernel_fp_unavailable_exception)
> +#else
> EXCEPTION(0x0800, Trap_08, unknown_exception, EXC_XFER_EE)
> +#endif
> +
> EXCEPTION(0x0900, Trap_09, unknown_exception, EXC_XFER_EE)
> EXCEPTION(0x0A00, Trap_0A, unknown_exception, EXC_XFER_EE)
> EXCEPTION(0x0B00, Trap_0B, unknown_exception, EXC_XFER_EE)
> @@ -432,7 +444,7 @@
>
> EXCEPTION(0x0D00, Trap_0D, unknown_exception, EXC_XFER_EE)
> EXCEPTION(0x0E00, Trap_0E, unknown_exception, EXC_XFER_EE)
> - EXCEPTION(0x0F00, Trap_0F, unknown_exception, EXC_XFER_EE)
> + EXCEPTION(0x0F20, Trap_0F, unknown_exception, EXC_XFER_EE)
Why?
>
> /* 0x1000 - Programmable Interval Timer (PIT) Exception */
> START_EXCEPTION(0x1000, Decrementer)
> @@ -821,8 +833,10 @@
> * The PowerPC 4xx family of processors do not have an FPU, so this just
> * returns.
> */
> +#ifndef CONFIG_PPC_FPU
> _ENTRY(giveup_fpu)
> blr
> +#endif
>
> /* This is where the main kernel code starts.
> */
> diff -r 9d9ac97e095d arch/powerpc/platforms/Kconfig.cputype
> --- a/arch/powerpc/platforms/Kconfig.cputype Thu Feb 25 21:23:42 2010 +0300
> +++ b/arch/powerpc/platforms/Kconfig.cputype Thu Feb 25 21:49:02 2010 +0300
> @@ -290,4 +290,9 @@
> config CHECK_CACHE_COHERENCY
> bool
>
> +config XILINX_FPU
> + bool "Xilinx softFPU"
> + select PPC_FPU
> + depends on 40x
> +
Should be more specific. Use something like XILINX_VIRTEX4_405_FPU
(as opposed to Virtex II pro, or the 440 on the Virtex 5.
Also, this looks to be very multiplatform unfriendly, so you'll need
to make sure other 405 board support is not selectable when the Xilinx
FPU is enabled.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [lm-sensors] [PATCH] hwmon: (tmp421) Add nfactor support (2nd attempt)
From: Jeff Angielski @ 2010-05-19 17:13 UTC (permalink / raw)
To: Andre Prendel; +Cc: Jean Delvare, linuxppc-dev, lm-sensors
In-Reply-To: <20100519072659.GA2069@andre-laptop>
On 05/19/2010 03:26 AM, Andre Prendel wrote:
> Ok, so there is only one remaining task :) Please update the documentation under
> Documentation/hwmon/tmp421. Then you will get my Acked-by.
Documentation is updated in this patch along with the source file.
There is a small cosmetic change in this patch. I changed the name of the
sysfs entry and internal variable name from nfactor to n_adjust. This was
done to make sure the users know we are modifying the N[adjust] parameter
in the nfactor corrections register and not the actual nfactor itself. I
also changed the reference to the register from NFACTOR to N_CORRECT since
the datasheet uses that name for the register in question.
I think the method of using the nfactor adjustment value is specific to this
chipset. Consequently, it's not appropriate to go into the general documentation.
>From 61f1c203620b06463695b399bae27a884008f169 Mon Sep 17 00:00:00 2001
From: Jeff Angielski <jeff@theptrgroup.com>
Date: Mon, 10 May 2010 10:26:34 -0400
Subject: [PATCH] hwmon: (tmp421) Add nfactor support
Add support for reading and writing the n-factor correction
registers. This is needed to compensate for the characteristics
of a particular sensor hanging off of the remote channels.
Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
---
Documentation/hwmon/tmp421 | 19 +++++++++++++++++++
drivers/hwmon/tmp421.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 0 deletions(-)
diff --git a/Documentation/hwmon/tmp421 b/Documentation/hwmon/tmp421
index 0cf07f8..668228a 100644
--- a/Documentation/hwmon/tmp421
+++ b/Documentation/hwmon/tmp421
@@ -17,6 +17,7 @@ Supported chips:
Authors:
Andre Prendel <andre.prendel@gmx.de>
+ Jeff Angielski <jeff@theptrgroup.com>
Description
-----------
@@ -34,3 +35,21 @@ the temperature values via the following sysfs files:
temp[1-4]_input
temp[2-4]_fault
+
+The chips allow the user to adjust the n-factor value that is used
+when converting the remote channel measurements to temperature. The
+adjustment has a range of -128 to +127 that yields an effective
+n-factor range of 0.706542 to 1.747977. The power on reset value
+for the adjustment is 0 which results in an n-factor of 1.008.
+
+The effective n-factor is calculated according to the following
+equation:
+
+n_factor = (1.008 * 300) / (300 - nfactor_adjust)
+
+The driver exports the n-factor adjustment value via the following
+sysfs files:
+
+temp[2-4]_n_adjust
+
+
diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 738c472..83edbc8 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
+static const u8 TMP421_N_CORRECT[3] = { 0x21, 0x22, 0x23 };
/* Flags */
#define TMP421_CONFIG_SHUTDOWN 0x40
@@ -76,6 +77,7 @@ struct tmp421_data {
int channels;
u8 config;
s16 temp[4];
+ s8 n_adjust[3];
};
static int temp_from_s16(s16 reg)
@@ -115,6 +117,10 @@ static struct tmp421_data *tmp421_update_device(struct device *dev)
data->temp[i] |= i2c_smbus_read_byte_data(client,
TMP421_TEMP_LSB[i]);
}
+ for (i = 1; i < data->channels; i++) {
+ data->n_adjust[i - 1] = i2c_smbus_read_byte_data(client,
+ TMP421_N_CORRECT[i - 1]);
+ }
data->last_updated = jiffies;
data->valid = 1;
}
@@ -157,6 +163,32 @@ static ssize_t show_fault(struct device *dev,
return sprintf(buf, "0\n");
}
+static ssize_t show_n_adjust(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ int index = to_sensor_dev_attr(devattr)->index;
+ struct tmp421_data *data = tmp421_update_device(dev);
+
+ return sprintf(buf, "%d\n", data->n_adjust[index - 1]);
+}
+
+static ssize_t set_n_adjust(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)->index;
+ int n_adjust= simple_strtol(buf, NULL, 10);
+
+ mutex_lock(&data->update_lock);
+ i2c_smbus_write_byte_data(client, TMP421_N_CORRECT[index - 1],
+ SENSORS_LIMIT(n_adjust, -128, 127));
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
int n)
{
@@ -177,19 +209,28 @@ static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_n_adjust, S_IRUSR | S_IWUSR | S_IRGRP,
+ show_n_adjust, set_n_adjust, 1);
static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp3_n_adjust, S_IRUSR | S_IWUSR | S_IRGRP,
+ show_n_adjust, set_n_adjust, 2);
static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp4_n_adjust, S_IRUSR | S_IWUSR | S_IRGRP,
+ show_n_adjust, set_n_adjust, 3);
static struct attribute *tmp421_attr[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
&sensor_dev_attr_temp2_input.dev_attr.attr,
&sensor_dev_attr_temp2_fault.dev_attr.attr,
+ &sensor_dev_attr_temp2_n_adjust.dev_attr.attr,
&sensor_dev_attr_temp3_input.dev_attr.attr,
&sensor_dev_attr_temp3_fault.dev_attr.attr,
+ &sensor_dev_attr_temp3_n_adjust.dev_attr.attr,
&sensor_dev_attr_temp4_input.dev_attr.attr,
&sensor_dev_attr_temp4_fault.dev_attr.attr,
+ &sensor_dev_attr_temp4_n_adjust.dev_attr.attr,
NULL
};
--
1.7.0.4
--
Jeff Angielski
The PTR Group
www.theptrgroup.com
^ permalink raw reply related
* Re: [PATCH] powerpc/pseries: Add support for IO Event interrupt drivers
From: Sonny Rao @ 2010-05-19 18:34 UTC (permalink / raw)
To: Michael Ellerman; +Cc: markn, linuxppc-dev, miltonm
In-Reply-To: <1274271058.13433.30.camel@concordia>
On Wed, May 19, 2010 at 10:10:58PM +1000, Michael Ellerman wrote:
<snip>
> > > The "checks the scope" requires an RTAS call, which takes a global lock
> > > (and you add another) - these aren't going to be used for anything
> > > performance critical I hope?
> >
> > Nope it shouldn't be performance critical, but it does raise the point
> > that the current RTAS implementation in Linux *always* uses the global
> > lock. There is a set of calls which are not required to be serialized
> > against each other. This would be a totally different patchset to fix that
> > problem. The "check-exception" call is one that doesn't require the global
> > RTAS lock. I might work on that someday :-)
>
> Aha, that's kind of what I was wondering. I take it PAPR documents which
> calls need to be serialised and which don't?
Yeah, here's my workin list of what calls can avoid the global lock:
List of "re-entrant to the number of processors in the system" RTAS Calls
--
ibm,get-xive
ibm,set-xive
ibm,int-off
ibm,int-on
OS machine check and soft-reset handlerse must be able to call rtas
(I'm saying these are therefore re-entrant because we could deadlock
if we took a machine check or reset with the global lock held)
--
nvram-fetch
nvram-store
check-exception (includes our io-events)
display-character
system-reboot
set-power-level(0,0)
power-off
ibm,set-eeh-option
ibm,set-slot-reset
ibm,read-slot-reset-state2
additional serialiaztion group by itself
--
stop-self
start-cpu
set-power-level
<snip>
> > Also, if we're going to go ahead and use rtas_call() which locks
> > it's own buffer which meets the requirements, why do we even need
> > a separate buffer? Really, we should make this call, then copy
> > the content of the buffer before handing it over to the drivers.
>
> But another CPU could rtas_call() and blow away our buffer after we've
> dropped the RTAS lock but before we've used the content of the buffer.
Yeah, maybe I'm getting ahead of myself here -- to me this just highlights
the whole locking problem with the API as written, since the locking is
all done inside that call. The API needs to be extended such that
we have the option of doing our own locking of RTAS calls.
> > > > + if (rtas_elog->type != RTAS_TYPE_IO_EVENT) {
> > > > + pr_warning("IO Events: We got called with an event type of %d"
> > > > + " rather than %d!\n", rtas_elog->type,
> > > > + RTAS_TYPE_IO_EVENT);
> > > > + WARN_ON(1);
> > > > + goto out;
> > > > + }
> >
> > Should we try to process this instead of just warning?
> > The type we get might be one of the the ones we recognize in
> > ras.c; so this is an argument for combining ras.c with this code
> > or at least report this in the same manner we report any other
> > RTAS error log.
>
> We could, but that would be a massive firmware bug - not that it
> wouldn't happen, but it would be Not Our Problem TM.
Yeah, this is paranoia (*cough* Milton's suggestion)
> > > We /could/ copy the ioei_sec and drop the buf lock, which would allow
> > > another interrupt to come in and start doing the RTAS call (on another
> > > cpu, and iff there are actually multiple interrupts). But we probably
> > > don't care.
> >
> > I think we *have* to copy it because we don't want our lock held when we
> > call random handlers.
>
> They're not really random, and as long as they don't call the
> register/unregister routines it should be /OK/. But copying is probably
> good so we don't hold the lock for too long.
Yeah, this is probably ok since it's all happening in interrupt
context anyway the handlers have to be running in an atomic context
anyway.
--
Sonny Rao, LTC OzLabs, BML team
^ 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