This patchset adds support for the PCC (Platform Communication Channel) interface as described in the current ACPI 5.0 spec. See Section 14 of the ACPI spec - http://acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf for more details on how PCC works.
In brief PCC is a generic means for PCC clients, to talk to the firmware. The PCC register space is typically memory mapped IO and uses a doorbell mechanism to communicate synchronously from the OS to the firmware. The PCC driver is completely agnostic to the protocol implemented by the PCC clients. It only implements the enumeration of PCC channels and the low level transport mechanism and leaves the rest to the PCC clients.
The PCC is meant to be useable in the future by clients such as CPPC (Collaborative Processor Performance Control), RAS (Reliability, Availability and Serviceability) and MPST (Memory Power State Tables) and possibly others.
Changes since V2: - Rebased on top of git://git.linaro.org/landing-teams/working/fujitsu/integration.git branch mailbox-for-3.17 - Added PCC API to mailbox framework as per Arnd's suggestion to allow usage without ACPI.
Changes since V1: -Integration with Mailbox framework - https://lkml.org/lkml/2014/5/15/49
Ashwin Chaugule (3): Mailbox: Add support for PCC mailbox and channels Add support for Platform Communication Channel PCC-test: Test driver to trigger PCC commands
drivers/mailbox/Kconfig | 12 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/mailbox.c | 118 +++++++++++++++++-- drivers/mailbox/pcc-test.c | 208 +++++++++++++++++++++++++++++++++ drivers/mailbox/pcc.c | 228 +++++++++++++++++++++++++++++++++++++ include/linux/mailbox_client.h | 6 + include/linux/mailbox_controller.h | 1 + 7 files changed, 568 insertions(+), 7 deletions(-) create mode 100644 drivers/mailbox/pcc-test.c create mode 100644 drivers/mailbox/pcc.c
The PCC (Platform Communication Channel) is a generic mailbox to be used by PCC clients such as CPPC, RAS and MPST as defined in the ACPI 5.0+ spec. This patch modifies the Mailbox framework to lookup PCC mailbox controllers and channels such that PCC drivers can work with or without ACPI support in the kernel.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org --- drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- include/linux/mailbox_client.h | 6 ++ include/linux/mailbox_controller.h | 1 + 3 files changed, 118 insertions(+), 7 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 63ecc17..09ad488 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -356,6 +356,14 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) } EXPORT_SYMBOL_GPL(mbox_request_channel);
+static void inline free_channel(struct mbox_chan *chan) +{ + chan->cl = NULL; + chan->active_req = NULL; + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) + chan->txdone_method = TXDONE_BY_POLL; +} + /** * mbox_free_channel - The client relinquishes control of a mailbox * channel by this call. @@ -369,14 +377,9 @@ void mbox_free_channel(struct mbox_chan *chan) return;
chan->mbox->ops->shutdown(chan); - /* The queued TX requests are simply aborted, no callbacks are made */ spin_lock_irqsave(&chan->lock, flags); - chan->cl = NULL; - chan->active_req = NULL; - if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) - chan->txdone_method = TXDONE_BY_POLL; - + free_channel(chan); module_put(chan->mbox->dev->driver->owner); spin_unlock_irqrestore(&chan->lock, flags); } @@ -394,6 +397,97 @@ of_mbox_index_xlate(struct mbox_controller *mbox, return &mbox->chans[ind]; }
+#ifdef CONFIG_PCC +/* + * The PCC (Platform Communication Channel) is + * defined in the ACPI 5.0+ spec. It is a generic + * mailbox interface between an OS and a Platform + * such as a BMC. The PCCT (Mailbox controller) has + * its own ACPI specific way to describe PCC clients + * and their subspace ids (Mailbox channels/clients). + * + * The following API is added such that PCC + * drivers continue to work with this Mailbox + * framework with or without ACPI. + */ + +static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{ + struct mbox_controller *mbox; + list_for_each_entry(mbox, &mbox_cons, node) { + if (mbox->name) + if (!strcmp(mbox->name, name)) + return mbox; + } + + return NULL; +} + +struct mbox_chan * +pcc_mbox_request_channel(struct mbox_client *cl, + char *name, unsigned chan_id) +{ + struct mbox_controller *mbox; + struct mbox_chan *pcc_chan; + unsigned long flags; + int ret; + + mutex_lock(&con_mutex); + mbox = mbox_find_pcc_controller(name); + + if (!mbox) { + pr_err("PCC mbox %s not found.\n", name); + mutex_unlock(&con_mutex); + return ERR_PTR(-ENODEV); + } + + pcc_chan = &mbox->chans[chan_id]; + + spin_lock_irqsave(&pcc_chan->lock, flags); + pcc_chan->msg_free = 0; + pcc_chan->msg_count = 0; + pcc_chan->active_req = NULL; + pcc_chan->cl = cl; + init_completion(&pcc_chan->tx_complete); + + if (pcc_chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) + pcc_chan->txdone_method |= TXDONE_BY_ACK; + + spin_unlock_irqrestore(&pcc_chan->lock, flags); + + ret = pcc_chan->mbox->ops->startup(pcc_chan); + if (ret) { + pr_err("Unable to startup the PCC channel (%d)\n", ret); + mbox_free_channel(pcc_chan); + mutex_unlock(&con_mutex); + return ERR_PTR(-ENODEV); + } + + mutex_unlock(&con_mutex); + + return pcc_chan; +} + +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); + +void pcc_mbox_free_channel(struct mbox_chan *chan) +{ + unsigned long flags; + + if (!chan || !chan->cl) + return; + + chan->mbox->ops->shutdown(chan); + + spin_lock_irqsave(&chan->lock, flags); + free_channel(chan); + spin_unlock_irqrestore(&chan->lock, flags); +} +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); + +#endif + /** * mbox_controller_register - Register the mailbox controller * @mbox: Pointer to the mailbox controller. @@ -405,7 +499,17 @@ int mbox_controller_register(struct mbox_controller *mbox) int i, txdone;
/* Sanity check */ - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) + + /* + * PCC clients and controllers are currently not backed by + * platform device structures. + */ +#ifndef CONFIG_PCC + if (!mbox->dev) + return -EINVAL; +#endif + + if (!mbox || !mbox->ops || !mbox->num_chans) return -EINVAL;
if (mbox->txdone_irq) diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h index 307d9ca..6a78df0 100644 --- a/include/linux/mailbox_client.h +++ b/include/linux/mailbox_client.h @@ -37,6 +37,12 @@ struct mbox_client { void (*tx_done)(struct mbox_client *cl, void *mssg, int r); };
+#ifdef CONFIG_PCC +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *c, + char *name, unsigned int chan_id); +void pcc_mbox_free_channel(struct mbox_chan *chan); /* may sleep */ +#endif + struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); int mbox_send_message(struct mbox_chan *chan, void *mssg); void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index 9ee195b..9f0ae42 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -81,6 +81,7 @@ struct mbox_controller { unsigned txpoll_period; struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox, const struct of_phandle_args *sp); + char *name; /* Internal to API */ struct timer_list poll; unsigned period;
ACPI 5.0+ spec defines a generic mode of communication between the OS and a platform such as the BMC. This medium (PCC) is typically used by CPPC (ACPI CPU Performance management), RAS (ACPI reliability protocol) and MPST (ACPI Memory power states).
This patch adds PCC support as a Mailbox Controller.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org --- drivers/mailbox/Kconfig | 12 +++ drivers/mailbox/Makefile | 2 + drivers/mailbox/pcc.c | 228 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+) create mode 100644 drivers/mailbox/pcc.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index c8b5c13..af4a153 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -50,4 +50,16 @@ config OMAP_MBOX_KFIFO_SIZE Specify the default size of mailbox's kfifo buffers (bytes). This can also be changed at runtime (via the mbox_kfifo_size module parameter). + +config PCC + bool "Platform Communication Channel" + depends on ACPI + help + ACPI 5.0+ spec defines a generic mode of communication + between the OS and a platform such as the BMC. This medium + (PCC) is typically used by CPPC (ACPI CPU Performance management), + RAS (ACPI reliability protocol) and MPST (ACPI Memory power + states). Select this driver if your platform implements the + PCC clients mentioned above. + endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 2fa343a..3f57ee9 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -9,3 +9,5 @@ obj-$(CONFIG_OMAP1_MBOX) += mailbox_omap1.o mailbox_omap1-objs := mailbox-omap1.o obj-$(CONFIG_OMAP2PLUS_MBOX) += mailbox_omap2.o mailbox_omap2-objs := mailbox-omap2.o + +obj-$(CONFIG_PCC) += pcc.o diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c new file mode 100644 index 0000000..e925f65 --- /dev/null +++ b/drivers/mailbox/pcc.c @@ -0,0 +1,228 @@ +/* + * Copyright (C) 2014 Linaro Ltd. + * Author: Ashwin Chaugule ashwin.chaugule@linaro.org + * + * 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. + * + */ + +#include <linux/acpi.h> +#include <linux/io.h> +#include <linux/uaccess.h> +#include <linux/init.h> +#include <linux/cpufreq.h> +#include <linux/delay.h> +#include <linux/ioctl.h> +#include <linux/vmalloc.h> +#include <linux/mailbox_controller.h> + +#include <acpi/actbl.h> + +#define MAX_PCC_SUBSPACES 256 +#define PCCS_SS_SIG_MAGIC 0x50434300 +#define PCC_CMD_COMPLETE 0x1 +#define PCC_VERSION "0.1" + +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; +static struct mbox_controller pcc_mbox_ctrl = {}; + +static bool pcc_tx_done(struct mbox_chan *chan) +{ + struct acpi_pcct_subspace *pcct_ss = chan->con_priv; + struct acpi_pcct_shared_memory *generic_comm_base = + (struct acpi_pcct_shared_memory *) pcct_ss->base_address; + u16 cmd_delay = pcct_ss->min_turnaround_time; + + /* Wait for Platform to consume. */ + while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE)) + udelay(cmd_delay); + + return true; +} + +static int get_subspace_id(struct mbox_chan *chan) +{ + unsigned int i; + int ret = -ENOENT; + + for (i = 0; i < pcc_mbox_ctrl.num_chans; i++) { + if (chan == &pcc_mbox_chan[i]) + return i; + } + + return ret; +} + +/* Channel lock is already held by mbox controller code. */ +static int pcc_send_data(struct mbox_chan *chan, void *data) +{ + struct acpi_pcct_subspace *pcct_ss = chan->con_priv; + struct acpi_pcct_shared_memory *generic_comm_base = + (struct acpi_pcct_shared_memory *) pcct_ss->base_address; + struct acpi_generic_address doorbell; + u64 doorbell_preserve; + u64 doorbell_val; + u64 doorbell_write; + u16 cmd = 0; + u16 ss_idx = -1; + int ret = 0; + + /* + * Min time in usec that OS is expected to wait + * before sending the next PCC cmd. + */ + u16 cmd_delay = pcct_ss->min_turnaround_time; + + /* Get PCC CMD */ + ret = kstrtou16((char*)data, 0, &cmd); + if (ret < 0) { + pr_err("Err while converting PCC CMD to u16: %d\n", ret); + goto out_err; + } + + ss_idx = get_subspace_id(chan); + + if (ss_idx < 0) { + pr_err("Invalid Subspace ID from PCC client\n"); + ret = ss_idx; + goto out_err; + } + + doorbell = pcct_ss->doorbell_register; + doorbell_preserve = pcct_ss->preserve_mask; + doorbell_write = pcct_ss->write_mask; + + /* Write to the shared comm region. */ + iowrite16(cmd, &generic_comm_base->command); + + /* Write Subspace MAGIC value so platform can identify destination. */ + iowrite32((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); + + /* Flip CMD COMPLETE bit */ + iowrite16(0, &generic_comm_base->status); + + /* Sync notification from OSPM to Platform. */ + acpi_read(&doorbell_val, &doorbell); + acpi_write((doorbell_val & doorbell_preserve) | doorbell_write, + &doorbell); + +out_err: + return ret; +} + +static int pcc_chan_startup(struct mbox_chan *chan) +{ + return 0; +} + +static void pcc_chan_shutdown(struct mbox_chan *chan) +{ + return; +} + +static struct mbox_chan_ops pcc_chan_ops = { + .send_data = pcc_send_data, + .startup = pcc_chan_startup, + .shutdown = pcc_chan_shutdown, + .last_tx_done = pcc_tx_done, +}; + +static int parse_pcc_subspace(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_pcct_subspace *pcct_ss; + + if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) { + pcct_ss = (struct acpi_pcct_subspace *) header; + + if (pcct_ss->header.type != ACPI_PCCT_TYPE_GENERIC_SUBSPACE) { + pr_err("Incorrect PCC Subspace type detected\n"); + return -EINVAL; + } + + /* New mbox channel entry for each PCC subspace detected. */ + pcc_mbox_chan[pcc_mbox_ctrl.num_chans].con_priv = pcct_ss; + + pcc_mbox_ctrl.num_chans++; + + } else { + pr_err("No more space for PCC subspaces.\n"); + return -ENOSPC; + } + + return 0; +} + +static int __init pcc_probe(void) +{ + acpi_status status = AE_OK; + acpi_size pcct_tbl_header_size; + struct acpi_table_pcct *pcct_tbl; + int ret; + + /* Search for PCCT */ + status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0, + (struct acpi_table_header **)&pcct_tbl, + &pcct_tbl_header_size); + + if (ACPI_SUCCESS(status) && !pcct_tbl) { + pr_warn("PCCT header not found.\n"); + status = AE_NOT_FOUND; + goto out_err; + } + + status = acpi_table_parse_entries(ACPI_SIG_PCCT, + sizeof(struct acpi_table_pcct), + ACPI_PCCT_TYPE_GENERIC_SUBSPACE, + parse_pcc_subspace, MAX_PCC_SUBSPACES); + + if (ACPI_SUCCESS(status)) + pr_err("Error parsing PCC subspaces from PCCT\n"); + + pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans); + + pcc_mbox_ctrl.chans = pcc_mbox_chan; + pcc_mbox_ctrl.ops = &pcc_chan_ops; + pcc_mbox_ctrl.name = "PCCT"; + pcc_mbox_ctrl.txdone_poll = true; + pcc_mbox_ctrl.txpoll_period = 1; + + pr_info("Registering PCC driver as Mailbox controller\n"); + ret = mbox_controller_register(&pcc_mbox_ctrl); + + if (ret) { + pr_err("Err registering PCC as Mailbox controller\n"); + return -ENODEV; + } + +out_err: + return ACPI_SUCCESS(status) ? 1 : 0; +} + +static int __init pcc_init(void) +{ + int ret; + + if (acpi_disabled) + return -ENODEV; + + /* Check if PCC support is available. */ + ret = pcc_probe(); + + if (ret) { + pr_debug("PCC probe failed.\n"); + return -EINVAL; + } + + return ret; +} + +device_initcall(pcc_init);
echo 2 > /proc/pcc_test [ to get channel base address ]
echo 1 > /proc/pcc_test [ to trigger a PCC write ]
echo 0 > /proc/pcc_test [ to trigger a PCC read ]
The pcc-test driver is implemented as an example of a PCC Mailbox client.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org --- drivers/mailbox/Makefile | 2 +- drivers/mailbox/pcc-test.c | 208 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 drivers/mailbox/pcc-test.c
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 3f57ee9..695d6ab 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -10,4 +10,4 @@ mailbox_omap1-objs := mailbox-omap1.o obj-$(CONFIG_OMAP2PLUS_MBOX) += mailbox_omap2.o mailbox_omap2-objs := mailbox-omap2.o
-obj-$(CONFIG_PCC) += pcc.o +obj-$(CONFIG_PCC) += pcc.o pcc-test.o diff --git a/drivers/mailbox/pcc-test.c b/drivers/mailbox/pcc-test.c new file mode 100644 index 0000000..737a036 --- /dev/null +++ b/drivers/mailbox/pcc-test.c @@ -0,0 +1,208 @@ +/* + * Copyright (C) 2014 Linaro Ltd. + * Author: Ashwin Chaugule ashwin.chaugule@linaro.org + * + * 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. + * + */ + +#include <linux/acpi.h> +#include <linux/io.h> +#include <linux/uaccess.h> +#include <linux/init.h> +#include <linux/cpufreq.h> +#include <linux/proc_fs.h> +#include <linux/seq_file.h> +#include <linux/mailbox_client.h> +#include <linux/mailbox_controller.h> + +#include <asm/uaccess.h> + +#include <acpi/actbl.h> + +static void __iomem *comm_base_addr; /* For use after ioremap */ + +extern int mbox_controller_register(struct mbox_controller *mbox); +extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *, char *, unsigned int); +extern int mbox_send_message(struct mbox_chan *chan, void *mssg); + + +/* XXX: The PCC Subspace id is hard coded here only for test purposes. + * In reality it should be parsed from the PCC package as described + * in the PCC client table. e.g. CPC for CPPC + */ +#define PCC_SUBSPACE_IDX 0 +#define CMD_COMPLETE 1 + +struct mbox_chan *pcc_test_chan; +enum ppc_cmds { + CMD_READ, + CMD_WRITE, + RESERVED, +}; + +static u64 reg1, reg2; + +static void run_pcc_test_read(void) +{ + u64 reg1_addr = (u64)comm_base_addr + 0x100; + u64 reg2_addr = (u64)comm_base_addr + 0x110; + char mssg[2]; + int ret; + + /* READ part of the test */ + pr_info("Sending PCC read req from Channel base addr: %llx\n", (u64)comm_base_addr); + + snprintf(mssg, sizeof(short), "%d", CMD_READ); + ret = mbox_send_message(pcc_test_chan, &mssg); + if (ret >= 0) { + pr_info("Read updated values from Platform.\n"); + reg1 = readq((void*)reg1_addr); + reg2 = readq((void*)reg2_addr); + pr_info("updated value of reg1:%llx\n", reg1); + pr_info("updated value of reg2:%llx\n", reg2); + } else + pr_err("Failed to read PCC parameters: ret=%d\n", ret); +} + +static void run_pcc_test_write(void) +{ + u64 reg1_addr = (u64)comm_base_addr + 0x100; + u64 reg2_addr = (u64)comm_base_addr + 0x110; + char mssg[2]; + int ret; + + /* WRITE part of the test */ + reg1++; + reg2++; + + writeq(reg1, (void *)reg1_addr); + writeq(reg2, (void *)reg2_addr); + + pr_info("Sending PCC write req from Channel base addr: %llx\n", (u64)comm_base_addr); + snprintf(mssg, sizeof(short), "%d", CMD_WRITE); + + ret = mbox_send_message(pcc_test_chan, &mssg); + + if (ret >= 0) + pr_info("OSPM successfully sent PCC write cmd to platform\n"); + else + pr_err("Failed to write PCC parameters. ret= %d\n", ret); +} + +static void pcc_chan_tx_done(struct mbox_client *cl, void *mssg, int ret) +{ + if (!ret) + pr_info("PCC channel TX successfully completed. CMD sent = %s\n", (char*)mssg); + else + pr_warn("PCC channel TX did not complete: CMD sent = %s\n", (char*)mssg); +} + +struct mbox_client pcc_mbox_cl = { + .tx_done = pcc_chan_tx_done, +}; + +void get_pcc_comm(void) +{ + u64 base_addr; + u32 len; + struct acpi_pcct_subspace *pcc_ss; + + pcc_test_chan = pcc_mbox_request_channel(&pcc_mbox_cl, "PCCT", PCC_SUBSPACE_IDX); + + if (!pcc_test_chan) { + pr_err("PCC Channel not found!\n"); + return; + } + + pcc_ss = pcc_test_chan->con_priv; + + base_addr = pcc_ss->base_address; + len = pcc_ss->length; + + pr_info("ioremapping: %llx, len: %x\n", base_addr, len); + +// comm_base_addr = ioremap_nocache(base_addr, len); + /* HACK to test PCC commands */ + comm_base_addr = kzalloc(PAGE_SIZE, GFP_KERNEL); + + if (!comm_base_addr) { + pr_err("Could not ioremap channel\n"); + return; + } + + pcc_ss->base_address = (u64)comm_base_addr; + + pr_info("Comm_base_addr: %llx\n", (u64)comm_base_addr); +} + +static ssize_t pcc_test_write(struct file *file, const char __user *buf, + size_t count, loff_t *offs) +{ + char ctl[2]; + + if (count != 2 || *offs) + return -EINVAL; + + if (copy_from_user(ctl, buf, count)) + return -EFAULT; + + switch (ctl[0]) { + case '0': + /* PCC read */ + run_pcc_test_read(); + break; + case '1': + /* PCC write */ + run_pcc_test_write(); + break; + case '2': + /* Get PCC channel */ + get_pcc_comm(); + break; + default: + pr_err("Unknown val\n"); + break; + } + + return count; +} + +static int pcc_test_open(struct inode *inode, struct file *filp) +{ + return 0; +} + +static int pcc_test_release(struct inode *inode, struct file *filp) +{ + return 0; +} + +static const struct file_operations pcc_test_fops = { + .open = pcc_test_open, +// .read = seq_read, + .write = pcc_test_write, + .release = pcc_test_release, +}; + +static int __init pcc_test(void) +{ + struct proc_dir_entry *pe; + + pe = proc_create("pcc_test", 0644, NULL, &pcc_test_fops); + + if (!pe) + return -ENOMEM; + + return 0; +} + +late_initcall(pcc_test);
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
The PCC (Platform Communication Channel) is a generic mailbox to be used by PCC clients such as CPPC, RAS and MPST as defined in the ACPI 5.0+ spec. This patch modifies the Mailbox framework to lookup PCC mailbox controllers and channels such that PCC drivers can work with or without ACPI support in the kernel.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- include/linux/mailbox_client.h | 6 ++ include/linux/mailbox_controller.h | 1 + 3 files changed, 118 insertions(+), 7 deletions(-)
Based on the patch description (adding support for a particular kind of mailbox) I'd expect to see a new driver or helper library being added to drivers/mailbox rather than changes in the mailbox core. If changes in the core are needed to support this I'd expect to see them broken out as separate patches.
+static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{
- struct mbox_controller *mbox;
- list_for_each_entry(mbox, &mbox_cons, node) {
if (mbox->name)if (!strcmp(mbox->name, name))return mbox;- }
- return NULL;
+}
This doesn't look particularly PCC specific?
/* Sanity check */
- if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
- /*
* PCC clients and controllers are currently not backed by* platform device structures.*/+#ifndef CONFIG_PCC
- if (!mbox->dev)
return -EINVAL;+#endif
It seems better to make this consistent - either enforce it all the time or don't enforce it.
On Tue, Aug 26, 2014 at 03:35:37PM -0400, Ashwin Chaugule wrote:
+static int pcc_chan_startup(struct mbox_chan *chan) +{
- return 0;
+}
+static void pcc_chan_shutdown(struct mbox_chan *chan) +{
- return;
+}
If these functions can be empty it seems better to have the mailbox framework accept empty functions than require drivers to provide dummies.
- /* Search for PCCT */
- status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
(struct acpi_table_header **)&pcct_tbl,&pcct_tbl_header_size);- if (ACPI_SUCCESS(status) && !pcct_tbl) {
pr_warn("PCCT header not found.\n");status = AE_NOT_FOUND;goto out_err;- }
Is PCC mandatory? If not I'd not expect to see this as a warning.
On Tue, Aug 26, 2014 at 03:35:38PM -0400, Ashwin Chaugule wrote:
echo 2 > /proc/pcc_test [ to get channel base address ]
echo 1 > /proc/pcc_test [ to trigger a PCC write ]
echo 0 > /proc/pcc_test [ to trigger a PCC read ]
You shouldn't be adding new files to /proc - debugfs is a better home for something like this.
On 27 August 2014 06:30, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:38PM -0400, Ashwin Chaugule wrote:
echo 2 > /proc/pcc_test [ to get channel base address ]
echo 1 > /proc/pcc_test [ to trigger a PCC write ]
echo 0 > /proc/pcc_test [ to trigger a PCC read ]
You shouldn't be adding new files to /proc - debugfs is a better home for something like this.
True. Didnt intend to upstream this part anyway. I used this only to test things for myself.
Thanks, Ashwin
Hi Mark,
On 27 August 2014 06:27, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
The PCC (Platform Communication Channel) is a generic mailbox to be used by PCC clients such as CPPC, RAS and MPST as defined in the ACPI 5.0+ spec. This patch modifies the Mailbox framework to lookup PCC mailbox controllers and channels such that PCC drivers can work with or without ACPI support in the kernel.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- include/linux/mailbox_client.h | 6 ++ include/linux/mailbox_controller.h | 1 + 3 files changed, 118 insertions(+), 7 deletions(-)
Based on the patch description (adding support for a particular kind of mailbox) I'd expect to see a new driver or helper library being added to drivers/mailbox rather than changes in the mailbox core. If changes in the core are needed to support this I'd expect to see them broken out as separate patches.
[..]
+static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{
struct mbox_controller *mbox;list_for_each_entry(mbox, &mbox_cons, node) {if (mbox->name)if (!strcmp(mbox->name, name))return mbox;}return NULL;+}
This doesn't look particularly PCC specific?
Call this mbox_find_controller_by_name() instead?
/* Sanity check */
if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
/** PCC clients and controllers are currently not backed by* platform device structures.*/+#ifndef CONFIG_PCC
if (!mbox->dev)return -EINVAL;+#endif
It seems better to make this consistent - either enforce it all the time or don't enforce it.
So this is where it got really messy. We're trying to create a "device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem)
The main reason why I thought this Mailbox framework looked useful (after you pointed me to it) for PCC was due to its async notification features. But thats easy and small enough to add to the PCC driver itself. We can also add a generic controller lookup mechanism in the PCC driver for anyone who doesn't want to use ACPI. I think thats a much cleaner way to handle PCC support. Adding PCC as a generic mailbox controller is turning out to be more messier that we'd originally imagined.
Cheers, Ashwin
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
On 27 August 2014 06:27, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
+static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{
struct mbox_controller *mbox;list_for_each_entry(mbox, &mbox_cons, node) {if (mbox->name)if (!strcmp(mbox->name, name))return mbox;}return NULL;+}
This doesn't look particularly PCC specific?
Call this mbox_find_controller_by_name() instead?
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
/* Sanity check */
if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
/** PCC clients and controllers are currently not backed by* platform device structures.*/+#ifndef CONFIG_PCC
if (!mbox->dev)return -EINVAL;+#endif
It seems better to make this consistent - either enforce it all the time or don't enforce it.
So this is where it got really messy. We're trying to create a
The messiness is orthogonal to my comment here - either it's legal to request a mailbox without a device or it isn't, it shouldn't depend on a random kernel configuration option for a particular mailbox driver which it is.
"device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem)
Arguably that's what things like cpufreq end up doing, though people tend to just shove a device into DT. Are you sure there isn't any device at all in ACPI that you could hang this off, looking at my desktop I see rather a lot of apparently synthetic ACPI devices with names starting LNX including for example LNXSYSTM:00?
The main reason why I thought this Mailbox framework looked useful (after you pointed me to it) for PCC was due to its async notification features. But thats easy and small enough to add to the PCC driver itself. We can also add a generic controller lookup mechanism in the PCC driver for anyone who doesn't want to use ACPI. I think thats a much cleaner way to handle PCC support. Adding PCC as a generic mailbox controller is turning out to be more messier that we'd originally imagined.
If PCC is described by ACPI tables how would non-ACPI users be able to use it?
Hi Mark,
On 27 August 2014 15:09, Mark Brown broonie@kernel.org wrote:
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
On 27 August 2014 06:27, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
/* Sanity check */
if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
/** PCC clients and controllers are currently not backed by* platform device structures.*/+#ifndef CONFIG_PCC
if (!mbox->dev)return -EINVAL;+#endif
It seems better to make this consistent - either enforce it all the time or don't enforce it.
So this is where it got really messy. We're trying to create a
The messiness is orthogonal to my comment here - either it's legal to request a mailbox without a device or it isn't, it shouldn't depend on a random kernel configuration option for a particular mailbox driver which it is.
Fair enough. This was just to show that PCC is unfortunately not a good candidate as a generic mailbox controller.
"device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem)
Arguably that's what things like cpufreq end up doing, though people tend to just shove a device into DT. Are you sure there isn't any device at all in ACPI that you could hang this off, looking at my desktop I see rather a lot of apparently synthetic ACPI devices with names starting LNX including for example LNXSYSTM:00?
Those are special HIDs defined in the ACPI spec and none of those can be used to back a device for the PCCT itself, since they're unrelated to the PCC protocol. The PCCT is defined in the spec as a separate table and if supported, should be visible in your system in the PCCT.dsl file. Just for the sake of the Mailbox framework, trying to represent the PCCT (which is a table) as a mailbox controller device, would require registering a special HID. That in turn would make an otherwise OS agnostic thing very Linux specific.
The main reason why I thought this Mailbox framework looked useful (after you pointed me to it) for PCC was due to its async notification features. But thats easy and small enough to add to the PCC driver itself. We can also add a generic controller lookup mechanism in the PCC driver for anyone who doesn't want to use ACPI. I think thats a much cleaner way to handle PCC support. Adding PCC as a generic mailbox controller is turning out to be more messier that we'd originally imagined.
If PCC is described by ACPI tables how would non-ACPI users be able to use it?
Whoever wants to do that, would need to come up with DT bindings that describe something similar to the PCCT contents. They could possibly ignore the ACPI specific bits like signature, asl compiler details etc. (which are only used by ACPI table parsers) and provide the rest of it. Its essentially an array of structures that point to various shared memory regions, each of which is owned by a PCC client and shared with the firmware. The intercommunication between client and firmware is via a doorbell, which is also described in these entries and can be implemented as an SGI or similar.
Thanks, Ashwin
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
On 27 August 2014 06:27, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
+static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{
struct mbox_controller *mbox;list_for_each_entry(mbox, &mbox_cons, node) {if (mbox->name)if (!strcmp(mbox->name, name))return mbox;}return NULL;+}
This doesn't look particularly PCC specific?
Call this mbox_find_controller_by_name() instead?
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
Arnd
On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote:
On 27 August 2014 15:09, Mark Brown broonie@kernel.org wrote:
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
The messiness is orthogonal to my comment here - either it's legal to request a mailbox without a device or it isn't, it shouldn't depend on a random kernel configuration option for a particular mailbox driver which it is.
Fair enough. This was just to show that PCC is unfortunately not a good candidate as a generic mailbox controller.
That seems to be a very big leap...
"device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem)
Arguably that's what things like cpufreq end up doing, though people tend to just shove a device into DT. Are you sure there isn't any device at all in ACPI that you could hang this off, looking at my desktop I see rather a lot of apparently synthetic ACPI devices with names starting LNX including for example LNXSYSTM:00?
Those are special HIDs defined in the ACPI spec and none of those can be used to back a device for the PCCT itself, since they're unrelated to the PCC protocol. The PCCT is defined in the spec as a separate table and if supported, should be visible in your system in the PCCT.dsl file. Just for the sake of the Mailbox framework, trying to represent the PCCT (which is a table) as a mailbox controller device, would require registering a special HID. That in turn would make an otherwise OS agnostic thing very Linux specific.
OK, but then there's always the option of just having some code that runs on init and instantiates a device if it sees the appropriate thing in the ACPI tables in a similar manner to how HIDs are handled. It's a small amount of work but it will generally make life easier if there is a struct device.
If PCC is described by ACPI tables how would non-ACPI users be able to use it?
Whoever wants to do that, would need to come up with DT bindings that describe something similar to the PCCT contents. They could possibly ignore the ACPI specific bits like signature, asl compiler details etc. (which are only used by ACPI table parsers) and provide the rest of it. Its essentially an array of structures that point to various shared memory regions, each of which is owned by a PCC client and shared with the firmware. The intercommunication between client and firmware is via a doorbell, which is also described in these entries and can be implemented as an SGI or similar.
Of course most likely such a binding would involve creating a device that owns the mailboxes so this'd be fairly straightforward...
On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
Right, and what he's trying to work around here is that ACPI has chosen to provide a generic binding for some mailboxes which isn't associated with anything we represent as a device and he doesn't want to provide that device as a Linux virtual thing.
Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
AFAICT the names he's interested in will be defined by the ACPI specs. It does seem like we should be providing a device for the controller and then either using references in ACPI to look it up if they exist or a lookup function for this particular namespace that goes and fetches the device we created and looks up in its context.
Hi Arnd,
On 28 August 2014 04:39, Arnd Bergmann arnd@arndb.de wrote:
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
On 27 August 2014 06:27, Mark Brown broonie@kernel.org wrote:
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
+static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{
struct mbox_controller *mbox;list_for_each_entry(mbox, &mbox_cons, node) {if (mbox->name)if (!strcmp(mbox->name, name))return mbox;}return NULL;+}
This doesn't look particularly PCC specific?
Call this mbox_find_controller_by_name() instead?
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
This snippet is based off of your suggestions. [1] [2] :)
Unfortunately, the three patches that Ashwin posted don't have a caller for this function,
mbox_find_pcc_controller() is called from pcc_mbox_request_channel() which is in this patch.
so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
Arnd
[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265395.html [2] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266528.html
Hi Mark,
On 28 August 2014 06:10, Mark Brown broonie@kernel.org wrote:
On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote:
On 27 August 2014 15:09, Mark Brown broonie@kernel.org wrote:
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
"device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem)
Arguably that's what things like cpufreq end up doing, though people tend to just shove a device into DT. Are you sure there isn't any device at all in ACPI that you could hang this off, looking at my desktop I see rather a lot of apparently synthetic ACPI devices with names starting LNX including for example LNXSYSTM:00?
Those are special HIDs defined in the ACPI spec and none of those can be used to back a device for the PCCT itself, since they're unrelated to the PCC protocol. The PCCT is defined in the spec as a separate table and if supported, should be visible in your system in the PCCT.dsl file. Just for the sake of the Mailbox framework, trying to represent the PCCT (which is a table) as a mailbox controller device, would require registering a special HID. That in turn would make an otherwise OS agnostic thing very Linux specific.
OK, but then there's always the option of just having some code that runs on init and instantiates a device if it sees the appropriate thing in the ACPI tables in a similar manner to how HIDs are handled. It's a small amount of work but it will generally make life easier if there is a struct device.
I need to give this a try. AFAICS, the HIDs get their device created by the driver subsystem. This would require manually creating one and I didn't see existing examples. Thinking of a table as a device (virtual/real) seemed weird to me, especially since we're seemingly only using it for ref counts here. But it sounds like this is an acceptable thing.
If PCC is described by ACPI tables how would non-ACPI users be able to use it?
Whoever wants to do that, would need to come up with DT bindings that describe something similar to the PCCT contents. They could possibly ignore the ACPI specific bits like signature, asl compiler details etc. (which are only used by ACPI table parsers) and provide the rest of it. Its essentially an array of structures that point to various shared memory regions, each of which is owned by a PCC client and shared with the firmware. The intercommunication between client and firmware is via a doorbell, which is also described in these entries and can be implemented as an SGI or similar.
Of course most likely such a binding would involve creating a device that owns the mailboxes so this'd be fairly straightforward...
With DT, yes, it seems to be a bit more straightforward.
Thanks, Ashwin
On 28 August 2014 06:15, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
Right, and what he's trying to work around here is that ACPI has chosen to provide a generic binding for some mailboxes which isn't associated with anything we represent as a device and he doesn't want to provide that device as a Linux virtual thing.
Just the idea of a table as a device, when it doesn't do any power management, hotplug or anything like a device seemed strange. But I'm open to ideas if we find a good solution. Its highly possible that I'm not seeing it the way you are because the driver subsystem internals are fairly new to me. :)
Suppose we create a platform_device for the PCCT (mailbox controller) and another one for the PCC client (mailbox client). How should the PCC client(s) identify the mailbox controller without passing a name? In DT, the "mboxes" field in the client DT entry is all strings with mailbox controller names. The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings?
Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
AFAICT the names he's interested in will be defined by the ACPI specs. It does seem like we should be providing a device for the controller and then either using references in ACPI to look it up if they exist or a lookup function for this particular namespace that goes and fetches the device we created and looks up in its context.
What is the comparison in this lookup function? A string or a struct device pointer? If it is the latter, how does the client get the reference to the controller struct device? One way would be to register the PCCT as a platform_device and the PCC client as its platform_driver. But I think that will restrict the number of PCC clients to who ever matches first. I suspect this is not what you're implying, so I'd appreciate some more help.
Thanks, Ashwin
Hi Mark, Arnd,
On 28 August 2014 16:34, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
On 28 August 2014 06:15, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
Right, and what he's trying to work around here is that ACPI has chosen to provide a generic binding for some mailboxes which isn't associated with anything we represent as a device and he doesn't want to provide that device as a Linux virtual thing.
Just the idea of a table as a device, when it doesn't do any power management, hotplug or anything like a device seemed strange. But I'm open to ideas if we find a good solution. Its highly possible that I'm not seeing it the way you are because the driver subsystem internals are fairly new to me. :)
Suppose we create a platform_device for the PCCT (mailbox controller) and another one for the PCC client (mailbox client). How should the PCC client(s) identify the mailbox controller without passing a name? In DT, the "mboxes" field in the client DT entry is all strings with mailbox controller names. The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings?
Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
AFAICT the names he's interested in will be defined by the ACPI specs. It does seem like we should be providing a device for the controller and then either using references in ACPI to look it up if they exist or a lookup function for this particular namespace that goes and fetches the device we created and looks up in its context.
What is the comparison in this lookup function? A string or a struct device pointer? If it is the latter, how does the client get the reference to the controller struct device? One way would be to register the PCCT as a platform_device and the PCC client as its platform_driver. But I think that will restrict the number of PCC clients to who ever matches first. I suspect this is not what you're implying, so I'd appreciate some more help.
I dont see a way to create a lookup table for PCC without storing the name of the controller somewhere. The suggestion of creating a platform device for the controller and client led to restricting only one client to the controller. Can you please suggest how to move this forward?
Thanks, Ashwin
On Tuesday 02 September 2014 14:16:42 Ashwin Chaugule wrote:
On 28 August 2014 16:34, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
On 28 August 2014 06:15, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it.
The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller.
Right, and what he's trying to work around here is that ACPI has chosen to provide a generic binding for some mailboxes which isn't associated with anything we represent as a device and he doesn't want to provide that device as a Linux virtual thing.
Just the idea of a table as a device, when it doesn't do any power management, hotplug or anything like a device seemed strange. But I'm open to ideas if we find a good solution. Its highly possible that I'm not seeing it the way you are because the driver subsystem internals are fairly new to me.
Suppose we create a platform_device for the PCCT (mailbox controller) and another one for the PCC client (mailbox client). How should the PCC client(s) identify the mailbox controller without passing a name? In DT, the "mboxes" field in the client DT entry is all strings with mailbox controller names.
No, it's not a string at all, it's a phandle, which is more like a pointer. We intentionally never match by a global string at all, because those might not be unique.
The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings?
I didn't think there was more than one PCC provider, why do you even need a string?
For the general case in ACPI, there should be a similar way of looking up mailbox providers to what we have in DT, but if I understand you correctly, the PCC specification does not allow that.
Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems.
Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here?
AFAICT the names he's interested in will be defined by the ACPI specs. It does seem like we should be providing a device for the controller and then either using references in ACPI to look it up if they exist or a lookup function for this particular namespace that goes and fetches the device we created and looks up in its context.
What is the comparison in this lookup function? A string or a struct device pointer? If it is the latter, how does the client get the reference to the controller struct device? One way would be to register the PCCT as a platform_device and the PCC client as its platform_driver. But I think that will restrict the number of PCC clients to who ever matches first. I suspect this is not what you're implying, so I'd appreciate some more help.
I dont see a way to create a lookup table for PCC without storing the name of the controller somewhere. The suggestion of creating a platform device for the controller and client led to restricting only one client to the controller. Can you please suggest how to move this forward?
I've forgotten the details, but I thought we had already worked it out when we discussed it the last time. What is the information available to the client driver?
Arnd
Hi Arnd,
On 2 September 2014 15:22, Arnd Bergmann arnd@arndb.de wrote:
Suppose we create a platform_device for the PCCT (mailbox controller) and another one for the PCC client (mailbox client). How should the PCC client(s) identify the mailbox controller without passing a name? In DT, the "mboxes" field in the client DT entry is all strings with mailbox controller names.
No, it's not a string at all, it's a phandle, which is more like a pointer. We intentionally never match by a global string at all, because those might not be unique.
Ok. With PCC, I dont see a way to do this sort of pointer matching.
The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings?
I didn't think there was more than one PCC provider, why do you even need a string?
For the general case in ACPI, there should be a similar way of looking up mailbox providers to what we have in DT, but if I understand you correctly, the PCC specification does not allow that.
Right. At least not in a way DT does. PCC clients know if something needs to be written/read via PCC mailbox and can identify a PCC subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely identified/defined in the spec.
#define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10
So we could use this ID instead of a string and use that to look up the PCC controller for a PCC client.
Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems.
Right. So the question is how do we work around the "mbox->dev" and "client->dev" expectations in the Mailbox framework for PCC, given that these tables aren't backed by "struct devices" ?
Thanks, Ashwin
On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:
Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems.
Right. So the question is how do we work around the "mbox->dev" and "client->dev" expectations in the Mailbox framework for PCC, given that these tables aren't backed by "struct devices" ?
As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:
The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings?
I didn't think there was more than one PCC provider, why do you even need a string?
For the general case in ACPI, there should be a similar way of looking up mailbox providers to what we have in DT, but if I understand you correctly, the PCC specification does not allow that.
Right. At least not in a way DT does. PCC clients know if something needs to be written/read via PCC mailbox and can identify a PCC subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely identified/defined in the spec.
#define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10
So we could use this ID instead of a string and use that to look up the PCC controller for a PCC client.
I didn't realize this was the case. Does that mean we can treat pcc as a linearly accessible address space the way we do for system memory, pci-config etc?
If that works, we should probably just have a regmap for it rather than expose the mailbox API to client drivers.
Arnd
On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote:
On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:
Right. At least not in a way DT does. PCC clients know if something needs to be written/read via PCC mailbox and can identify a PCC subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely identified/defined in the spec.
#define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10
So we could use this ID instead of a string and use that to look up the PCC controller for a PCC client.
I didn't realize this was the case. Does that mean we can treat pcc as a linearly accessible address space the way we do for system memory, pci-config etc?
If that works, we should probably just have a regmap for it rather than expose the mailbox API to client drivers.
A regmap doesn't seem to map very well here - as far as I can tell the addresses referred to are mailboxes rather than registers or memory addresses. I could be misunderstanding though.
On Wednesday 03 September 2014 15:49:21 Mark Brown wrote:
On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote:
On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:
Right. At least not in a way DT does. PCC clients know if something needs to be written/read via PCC mailbox and can identify a PCC subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely identified/defined in the spec.
#define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10
So we could use this ID instead of a string and use that to look up the PCC controller for a PCC client.
I didn't realize this was the case. Does that mean we can treat pcc as a linearly accessible address space the way we do for system memory, pci-config etc?
If that works, we should probably just have a regmap for it rather than expose the mailbox API to client drivers.
A regmap doesn't seem to map very well here - as far as I can tell the addresses referred to are mailboxes rather than registers or memory addresses. I could be misunderstanding though.
No, I think you are right. Nevermind then.
Arnd
On 2 September 2014 19:03, Mark Brown broonie@kernel.org wrote:
On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:
Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems.
Right. So the question is how do we work around the "mbox->dev" and "client->dev" expectations in the Mailbox framework for PCC, given that these tables aren't backed by "struct devices" ?
As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in the PCC specific API that requests a PCC channel, depending on what we do in the registration path, this function can pick out the PCC mailbox pointer and try_module_get(mbox->dev..). Since this is PCC specific anyway, we don't need the client->dev argument at all. Did I understand you correctly?
Thanks, Ashwin
On Wednesday 03 September 2014 11:23:21 Ashwin Chaugule wrote:
On 2 September 2014 19:03, Mark Brown broonie@kernel.org wrote:
On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:
Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems.
Right. So the question is how do we work around the "mbox->dev" and "client->dev" expectations in the Mailbox framework for PCC, given that these tables aren't backed by "struct devices" ?
As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in the PCC specific API that requests a PCC channel, depending on what we do in the registration path, this function can pick out the PCC mailbox pointer and try_module_get(mbox->dev..). Since this is PCC specific anyway, we don't need the client->dev argument at all. Did I understand you correctly?
Yes, I think this would be reasonable.
Arnd
On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:
On 2 September 2014 19:03, Mark Brown broonie@kernel.org wrote:
As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in
Why would you have to do that - can't the PCC specific API manage to hide this?
On Wednesday 03 September 2014 16:36:01 Mark Brown wrote:
On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:
On 2 September 2014 19:03, Mark Brown broonie@kernel.org wrote:
As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in
Why would you have to do that - can't the PCC specific API manage to hide this?
I think that's what Ashwin meant as the 'or' part of the sentence above.
Arnd
On Wed, Sep 03, 2014 at 05:41:20PM +0200, Arnd Bergmann wrote:
On Wednesday 03 September 2014 16:36:01 Mark Brown wrote:
On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:
IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in
Why would you have to do that - can't the PCC specific API manage to hide this?
I think that's what Ashwin meant as the 'or' part of the sentence above.
So it is.
-
Arnd Bergmann -
Ashwin Chaugule -
Mark Brown