From: Christian Krafft <krafft@de•ibm.com>
To: Nathan Lynch <ntl@pobox•com>
Cc: linuxppc-dev@ozlabs•org, cbe-oss-dev@ozlabs•org
Subject: Re: [patch 1/1] pmi: initial version
Date: Mon, 12 Feb 2007 10:12:22 +0100 [thread overview]
Message-ID: <20070212101222.24abe14c@localhost> (raw)
In-Reply-To: <20070209222956.GH1827@localdomain>
Hi Nathan,
thanks for your comments.
Will send an updated version.
On Fri, 9 Feb 2007 16:29:56 -0600
Nathan Lynch <ntl@pobox•com> wrote:
> Hi Christian-
>=20
> Some minor comments.
>=20
> Christian Krafft wrote:
> > This patch adds driver code for the PMI device found in future IBM prod=
ucts.
> > PMI stands for "Platform Management Interrupt" and is a way to communic=
ate
> > with the BMC. It provides bidirectional communication with a low latenc=
y.
> >=20
> > Signed-off-by: Christian Krafft <krafft@de•ibm.com>
> >=20
> > Index: linux/arch/powerpc/Kconfig
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- linux.orig/arch/powerpc/Kconfig
> > +++ linux/arch/powerpc/Kconfig
> > @@ -524,6 +524,7 @@ config PPC_IBM_CELL_BLADE
> > select MMIO_NVRAM
> > select PPC_UDBG_16550
> > select UDBG_RTAS_CONSOLE
> > +# select PPC_PMI
>=20
> Did you mean for this to be commented out?
no, good point ;-)
>=20
> <snip>
>=20
> > +static void __iomem *of_iomap(struct device_node *np)
> > +{
> > + struct resource res;
> > +
> > + if (of_address_to_resource(np, 0, &res))
> > + return NULL;
> > +
> > + pr_debug("Resource start: 0x%lx\n", res.start);
> > + pr_debug("Resource end: 0x%lx\n", res.end);
>=20
> You need a tab instead of spaces.
copy and past bug,
>=20
>=20
> > +
> > + return ioremap(res.start, 1 + res.end - res.start);
> > +}
> > +
> > +static int pmi_irq_handler(int irq, void *dev_id)
> > +{
> > + struct pmi_data *data;
> > + int type;
> > + struct pmi_handler *handler;
> > +
> > + data =3D dev_id;
> > +
> > + BUG_ON(!data);
>=20
> Not necessary, you'll oops in two lines anyway if data is NULL.
ok,
>=20
>=20
> > +
> > + pr_debug("pmi: got a PMI message\n");
> > +
> > + spin_lock(&data->pmi_spinlock);
> > + type =3D ioread8(data->pmi_reg + PMI_READ_TYPE);
> > + spin_unlock(&data->pmi_spinlock);
> > +
> > + pr_debug("pmi: message type is %d\n", type);
> > +
> > + if (type & PMI_ACK) {
> > + BUG_ON(!data->msg);
> > + BUG_ON(!data->completion);
> > + pr_debug("pmi: got an ACK\n");
> > + data->msg->type =3D type;
> > + data->msg->data0 =3D ioread8(data->pmi_reg + PMI_READ_DATA0);
> > + data->msg->data1 =3D ioread8(data->pmi_reg + PMI_READ_DATA1);
> > + data->msg->data2 =3D ioread8(data->pmi_reg + PMI_READ_DATA2);
>=20
> Should these accesses to data->pmi_reg be performed while holding
> data->pmi_spinlock as well?
yep, good catch.
>=20
>=20
> > + complete(data->completion);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_lock(&data->handler_spinlock);
> > + list_for_each_entry(handler, &data->handler, node) {
> > + pr_debug("pmi: notifying handlers\n");
> > + if (handler->type =3D=3D type) {
> > + pr_debug("pmi: notify handler %p\n", handler);
> > + handler->handle_pmi_message(data->dev, data->msg);
> > + }
> > + }
> > + spin_unlock(&data->handler_spinlock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +
> > +static struct of_device_id pmi_match[] =3D {
> > + { .type =3D "ibm,pmi", .name =3D "pmi" },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, pmi_match);
> > +
> > +static int pmi_of_probe(struct of_device *dev,
> > + const struct of_device_id *match)
> > +{
> > + DEFINE_SPINLOCK(handler_spinlock);
> > + DEFINE_SPINLOCK(pmi_spinlock);
> > +
> > + struct device_node *np =3D dev->node;
> > + struct pmi_data *data;
> > + int rc;
> > +
> > + data =3D kzalloc(sizeof(struct pmi_data), GFP_KERNEL);
> > + if (!data) {
> > + printk(KERN_ERR "pmi: could not allocate memory.\n");
> > + return -EFAULT;
>=20
> -ENOMEM is the appropriate error code here.
will be updated.
>=20
>=20
> > + }
> > +
> > + data->pmi_reg =3D of_iomap(np);
> > +
> > + if (!data->pmi_reg) {
> > + printk(KERN_ERR "pmi: invalid register address.\n");
> > + kfree(data);
> > + return -EFAULT;
> > + }
> > +
> > + INIT_LIST_HEAD(&data->handler);
> > +
> > + data->irq =3D irq_of_parse_and_map(np, 0);
> > + if (data->irq =3D=3D NO_IRQ) {
> > + printk(KERN_ERR "pmi: invalid interrupt.\n");
> > + iounmap(data->pmi_reg);
> > + kfree(data);
> > + return -EFAULT;
> > + }
> > +
> > + data->handler_spinlock =3D handler_spinlock;
> > + data->pmi_spinlock =3D pmi_spinlock;
> > +
> > + rc =3D request_irq(data->irq, pmi_irq_handler,
> > + IRQF_SHARED, "pmi", data);
> > + if (rc) {
> > + printk(KERN_ERR "pmi: can't request IRQ %d: returned %d\n",
> > + data->irq, rc);
> > + iounmap(data->pmi_reg);
> > + kfree(data);
>=20
> Consider using goto to reduce duplication of the iounmap, kfree sequence.
>=20
> > + return -EFAULT;
>=20
> Just return rc; request_irq returns standard error codes.
ok,
>=20
> <snip>
>=20
> > +void pmi_send_message(struct of_device *device, struct pmi_message *ms=
g)
> > +{
> > + struct pmi_data *data;
> > + unsigned long flags;
> > + DECLARE_COMPLETION_ONSTACK(completion);
> > +
> > + BUG_ON(!device || !msg);
>=20
> This BUG_ON is also unnecessary, and could actually make debugging
> more difficult since it wouldn't be immediately apparent which
> condition triggered it.
right,
>=20
> > +
> > + data =3D device->dev.driver_data;
> > + pr_debug("pmi_send_message: data is %p\n", data);
> > +
> > + mutex_lock(&data->msg_mutex);
> > +
> > + data->msg =3D msg;
> > +
> > + pr_debug("pmi_send_message: msg is %p\n", msg);
> > +
> > + data->completion =3D &completion;
> > +
> > + spin_lock_irqsave(&data->pmi_spinlock, flags);
> > + iowrite8(msg->data0, data->pmi_reg + PMI_WRITE_DATA0);
> > + iowrite8(msg->data1, data->pmi_reg + PMI_WRITE_DATA1);
> > + iowrite8(msg->data2, data->pmi_reg + PMI_WRITE_DATA2);
> > + iowrite8(msg->type, data->pmi_reg + PMI_WRITE_TYPE);
> > + spin_unlock_irqrestore(&data->pmi_spinlock, flags);
> > +
> > + pr_debug(KERN_INFO "pmi_send_message: wait for completion %p\n", data=
->completion);
> > +
> > + wait_for_completion_interruptible_timeout(data->completion, PMI_TIMEO=
UT);
> > +
> > + data->msg =3D NULL;
> > + data->completion =3D NULL;
> > +
> > + mutex_unlock(&data->msg_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(pmi_send_message);
> ...
> > +EXPORT_SYMBOL_GPL(pmi_register_handler);
> ...
> > +EXPORT_SYMBOL_GPL(pmi_unregister_handler);
>=20
> Are the symbol exports necessary?
>=20
> <snip>
>=20
> > +void pmi_register_handler(struct of_device *, struct pmi_handler *);
> > +void pmi_unregister_handler(struct of_device *, struct pmi_handler *);
> > +
> > +void pmi_send_message(struct of_device *, struct pmi_message *);
>=20
> Are there users of these functions?
>=20
yes, they will be used by the cbe_cpufreq driver.
I will send a patch for it, as soon as i was able to test it,
hopefully today or tomorrow.
--=20
Mit freundlichen Gr=FCssen,
kind regards,
Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist
next prev parent reply other threads:[~2007-02-12 9:12 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-02-09 17:35 [patch 0/1] pmi: initial version Christian Krafft
2007-02-09 17:45 ` [patch 1/1] " Christian Krafft
2007-02-09 22:29 ` Nathan Lynch
2007-02-12 9:12 ` Christian Krafft [this message]
2007-02-13 13:34 ` [Cbe-oss-dev] [patch v2] " Christian Krafft
2007-02-13 18:44 ` [Cbe-oss-dev] [patch v3] " Christian Krafft
2007-02-13 18:48 ` [Cbe-oss-dev] [patch v3] powerpc: add PMI driver for cell blade Christian Krafft
2007-02-13 19:20 ` Christian Krafft
2007-02-13 19:28 ` Christian Krafft
2007-02-14 0:30 ` Paul Mackerras
2007-02-14 8:39 ` Christian Krafft
2007-02-14 10:22 ` Paul Mackerras
2007-02-14 13:07 ` Christian Krafft
2007-02-14 13:09 ` Christian Krafft
2007-02-15 7:06 ` Heiko J Schick
2007-02-15 10:50 ` Benjamin Herrenschmidt
2007-02-15 19:19 ` Segher Boessenkool
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070212101222.24abe14c@localhost \
--to=krafft@de$(echo .)ibm.com \
--cc=cbe-oss-dev@ozlabs$(echo .)org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=ntl@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox