| Message ID | 1415371964-4451-2-git-send-email-ashwin.chaugule@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > 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. > > Reviewed-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> > --- > drivers/mailbox/Kconfig | 12 ++ > drivers/mailbox/Makefile | 2 + > drivers/mailbox/mailbox.c | 4 +- > drivers/mailbox/mailbox.h | 16 ++ > drivers/mailbox/pcc.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 398 insertions(+), 3 deletions(-) > create mode 100644 drivers/mailbox/mailbox.h > create mode 100644 drivers/mailbox/pcc.c > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index 9fd9c67..c04fed9 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -33,4 +33,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 Driver" > + 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 94ed7ce..dd412c2 100644 > --- a/drivers/mailbox/Makefile > +++ b/drivers/mailbox/Makefile > @@ -5,3 +5,5 @@ obj-$(CONFIG_MAILBOX) += mailbox.o > obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o > > obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o > + > +obj-$(CONFIG_PCC) += pcc.o > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index afcb430..17e9e4a 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -21,9 +21,7 @@ > #include <linux/mailbox_client.h> > #include <linux/mailbox_controller.h> > > -#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ > -#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ > -#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ > +#include "mailbox.h" > > static LIST_HEAD(mbox_cons); > static DEFINE_MUTEX(con_mutex); > diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h > new file mode 100644 > index 0000000..5a15a25 > --- /dev/null > +++ b/drivers/mailbox/mailbox.h > @@ -0,0 +1,16 @@ > +/* > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __MAILBOX_H > +#define __MAILBOX_H > + > +#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ > +#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ > +#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ > + > +#endif /* __MAILBOX_H */ > + > + > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > new file mode 100644 > index 0000000..49074cd0 > --- /dev/null > +++ b/drivers/mailbox/pcc.c > @@ -0,0 +1,367 @@ > +/* > + * 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. > + * > + * PCC (Platform Communication Channel) is defined in the ACPI 5.0+ > + * specification. It is a mailbox like mechanism to allow clients > + * such as CPPC (Collaborative Processor Performance Control), RAS > + * (Reliability, Availability and Serviceability) and MPST (Memory > + * Node Power State Table) to talk to the platform (e.g. BMC) through > + * shared memory regions as defined in the PCC table entries. The PCC > + * specification supports a Doorbell mechanism for the PCC clients > + * to notify the platform about new data. This Doorbell information > + * is also specified in each PCC table entry. See pcc_send_data() > + * and pcc_tx_done() for basic mode of operation. > + * > + * For more details about PCC, please see the ACPI specification from > + * http://www.uefi.org/ACPIv5.1 Section 14. > + * > + * This file implements PCC as a Mailbox controller and allows for PCC > + * clients to be implemented as its Mailbox Client Channels. > + */ > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/init.h> > +#include <linux/platform_device.h> > +#include <linux/mailbox_controller.h> > +#include <linux/mailbox_client.h> > + > +#include "mailbox.h" > + > +#define MAX_PCC_SUBSPACES 256 > +#define PCCS_SS_SIG_MAGIC 0x50434300 > +#define PCC_CMD_COMPLETE 0x1 > + > +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; > +static struct mbox_controller pcc_mbox_ctrl = {}; > + > +/** > + * pcc_mbox_request_channel - PCC clients call this function to > + * request a pointer to their PCC subspace, from which they > + * can get the details of communicating with the remote. > + * @cl: Pointer to Mailbox client, so we know where to bind the > + * Channel. > + * @index: The PCC Subspace index as parsed in the PCC client > + * ACPI package. This is used to lookup the array of PCC > + * subspaces as parsed by the PCC Mailbox controller. > + * > + * Return: Pointer to the Mailbox Channel if successful or > + * ERR_PTR. > + */ > +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, > + int index) > +{ > + struct device *dev = pcc_mbox_ctrl.dev; > + struct mbox_chan *chan; > + unsigned long flags; > + > + /* > + * Each PCC Subspace is a Mailbox Channel. > + * The PCC Clients get their PCC Subspace ID > + * from their own tables and pass it here. > + * This returns a pointer to the PCC subspace > + * for the Client to operate on. > + */ > + chan = &pcc_mbox_chan[index]; > + > + if (!chan || chan->cl) { > + dev_err(dev, "%s: PCC mailbox not free\n", __func__); > + return ERR_PTR(-EBUSY); > + } > + > + spin_lock_irqsave(&chan->lock, flags); > + chan->msg_free = 0; > + chan->msg_count = 0; > + chan->active_req = NULL; > + chan->cl = cl; > + init_completion(&chan->tx_complete); > + > + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) > + chan->txdone_method |= TXDONE_BY_ACK; > + > + spin_unlock_irqrestore(&chan->lock, flags); > + > + return chan; > +} > +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and kept same people in CC. That patch keeps the api same for DT and ACPI drivers. > + > +/** > + * pcc_mbox_free_channel - Clients call this to free their Channel. > + * > + * @chan: Pointer to the mailbox channel as returned by > + * pcc_mbox_request_channel() > + */ > +void pcc_mbox_free_channel(struct mbox_chan *chan) > +{ > + unsigned long flags; > + > + if (!chan || !chan->cl) > + return; > + > + 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; > + > + spin_unlock_irqrestore(&chan->lock, flags); > +} > +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); > + > +/** > + * pcc_tx_done - Callback from Mailbox controller code to > + * check if PCC message transmission completed. > + * @chan: Pointer to Mailbox channel on which previous > + * transmission occurred. > + * > + * Return: TRUE if succeeded. > + */ > +static bool pcc_tx_done(struct mbox_chan *chan) > +{ > + struct acpi_pcct_hw_reduced *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; > + unsigned int retries = 0; > + > + /* Try a few times while waiting for platform to consume */ > + while (!(readw_relaxed(&generic_comm_base->status) > + & PCC_CMD_COMPLETE)) { > + > + if (retries++ < 5) > + udelay(cmd_delay); > + else { > + /* > + * If the remote is dead, this will cause the Mbox > + * controller to timeout after mbox client.tx_tout > + * msecs. > + */ > + pr_err("PCC platform did not respond.\n"); > + return false; > + } > + } > + return true; > +} > + > +/** > + * get_subspace_id - Given a Mailbox channel, find out the > + * PCC subspace id. > + * @chan: Pointer to Mailbox Channel from which we want > + * the index. > + * Return: Errno if not found, else positive index number. > + */ > +static int get_subspace_id(struct mbox_chan *chan) > +{ > + int id = chan - pcc_mbox_chan; > + > + if (id < 0 || id > pcc_mbox_ctrl.num_chans) > + return -ENOENT; > + > + return id; > +} > + > +/** > + * pcc_send_data - Called from Mailbox Controller code to finally > + * transmit data over channel. > + * @chan: Pointer to Mailbox channel over which to send data. > + * @data: Actual data to be written over channel. > + * > + * Return: Err if something failed else 0 for success. > + */ > +static int pcc_send_data(struct mbox_chan *chan, void *data) > +{ > + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; > + u16 ss_idx = -1; > + > + ss_idx = get_subspace_id(chan); > + > + if (ss_idx < 0) { > + pr_err("Invalid Subspace ID from PCC client\n"); > + return -EINVAL; > + } > + > + doorbell = pcct_ss->doorbell_register; > + doorbell_preserve = pcct_ss->preserve_mask; > + doorbell_write = pcct_ss->write_mask; > + > + /* Write to the shared comm region. */ > + writew(cmd, &generic_comm_base->command); > + > + /* Write Subspace MAGIC value so platform can identify destination. */ > + writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); > + > + /* Flip CMD COMPLETE bit */ > + writew(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); > + > + return 0; > +} > + > +static struct mbox_chan_ops pcc_chan_ops = { > + .send_data = pcc_send_data, > + .last_tx_done = pcc_tx_done, > +}; > + > +/** > + * parse_pcc_subspace - Parse the PCC table and extract PCC subspace > + * entries. There should be one entry per PCC client. > + * @header: Pointer to the ACPI subtable header under the PCCT. > + * @end: End of subtable entry. > + * > + * Return: 0 for Success, else errno. > + * > + * This gets called for each entry in the PCC table. > + */ > +static int parse_pcc_subspace(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_pcct_hw_reduced *pcct_ss; > + > + if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) { > + pcct_ss = (struct acpi_pcct_hw_reduced *) header; > + > + if (pcct_ss->header.type != > + ACPI_PCCT_TYPE_HW_REDUCED_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; > +} > + > +/** > + * acpi_pcc_probe - Parse the ACPI tree for the PCCT. > + * > + * Return: 0 for Success, else errno. > + */ > +static int __init acpi_pcc_probe(void) > +{ > + acpi_status status = AE_OK; > + acpi_size pcct_tbl_header_size; > + int count; > + struct acpi_table_pcct *pcct_tbl; > + > + /* 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_FAILURE(status) || !pcct_tbl) { > + pr_warn("PCCT header not found.\n"); > + return -ENODEV; > + } > + > + count = acpi_table_parse_entries(ACPI_SIG_PCCT, > + sizeof(struct acpi_table_pcct), > + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE, > + parse_pcc_subspace, MAX_PCC_SUBSPACES); > + > + if (count <= 0) { > + pr_err("Error parsing PCC subspaces from PCCT\n"); > + return -EINVAL; > + } > + > + pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans); > + > + return 0; > +} > + > +/** > + * pcc_mbox_probe - Called when we find a match for the > + * PCCT platform device. This is purely used to represent > + * the PCCT as a virtual device for registering with the > + * generic Mailbox framework. > + * > + * @pdev: Pointer to platform device returned when a match > + * is found. > + * > + * Return: 0 for Success, else errno. > + */ > +static int pcc_mbox_probe(struct platform_device *pdev) > +{ > + int ret = 0; > + > + pcc_mbox_ctrl.chans = pcc_mbox_chan; > + pcc_mbox_ctrl.ops = &pcc_chan_ops; > + pcc_mbox_ctrl.txdone_poll = true; > + pcc_mbox_ctrl.txpoll_period = 10; > + pcc_mbox_ctrl.dev = &pdev->dev; > + > + 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: %d\n", ret); > + ret = -ENODEV; > + } > + > + return ret; > +} > + > +struct platform_driver pcc_mbox_driver = { > + .probe = pcc_mbox_probe, > + .driver = { > + .name = "PCCT", > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init pcc_init(void) > +{ > + int ret; > + struct platform_device *pcc_pdev; > + > + if (acpi_disabled) > + return -ENODEV; > + > + /* Check if PCC support is available. */ > + ret = acpi_pcc_probe(); > + > + if (ret) { > + pr_err("ACPI PCC probe failed.\n"); > + return -ENODEV; > + } > + > + pcc_pdev = platform_create_bundle(&pcc_mbox_driver, > + pcc_mbox_probe, NULL, 0, NULL, 0); > + > + if (!pcc_pdev) { > + pr_err("Err creating PCC platform bundle\n"); > + return -ENODEV; > + } > + > + return 0; > +} > +device_initcall(pcc_init); > IMO the PCC platform_device should be populated by ACPI or DT core. This PCC controller driver should parse the PCCT ... so it populates .global_xlate() which expect the 'global_id' of channel requested to be (0x50434300 | Client_Class_Code) as specified by ACPI. It should be possible to have same client and controller driver work for both ACPI and DT. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jassi, On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> + >> +/** >> + * pcc_mbox_request_channel - PCC clients call this function to >> + * request a pointer to their PCC subspace, from which they >> + * can get the details of communicating with the remote. >> + * @cl: Pointer to Mailbox client, so we know where to bind the >> + * Channel. >> + * @index: The PCC Subspace index as parsed in the PCC client >> + * ACPI package. This is used to lookup the array of PCC >> + * subspaces as parsed by the PCC Mailbox controller. >> + * >> + * Return: Pointer to the Mailbox Channel if successful or >> + * ERR_PTR. >> + */ >> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >> + int index) >> +{ >> + struct device *dev = pcc_mbox_ctrl.dev; >> + struct mbox_chan *chan; >> + unsigned long flags; >> + >> + /* >> + * Each PCC Subspace is a Mailbox Channel. >> + * The PCC Clients get their PCC Subspace ID >> + * from their own tables and pass it here. >> + * This returns a pointer to the PCC subspace >> + * for the Client to operate on. >> + */ >> + chan = &pcc_mbox_chan[index]; >> + >> + if (!chan || chan->cl) { >> + dev_err(dev, "%s: PCC mailbox not free\n", __func__); >> + return ERR_PTR(-EBUSY); >> + } >> + >> + spin_lock_irqsave(&chan->lock, flags); >> + chan->msg_free = 0; >> + chan->msg_count = 0; >> + chan->active_req = NULL; >> + chan->cl = cl; >> + init_completion(&chan->tx_complete); >> + >> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >> + chan->txdone_method |= TXDONE_BY_ACK; >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> + >> + return chan; >> +} >> +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); > > I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and > kept same people in CC. > That patch keeps the api same for DT and ACPI drivers. Thanks for taking a look. Your patch looks similar in concept to one of my earlier patches. But based on the discussions that followed since, we decided that its best to add a separate PCC lookup and registration API. The main reason being, we dont have a way to list all mbox providers in ACPI in a way that DT does. e.g. in DT, the client->dev is used to look up mbox controllers. In ACPI, a client cant specify which mbox controllers to associate with, if it can attach to multiple. With the PCC specific API, if the client calls it, the controller knows where to look, because that lookup is PCC specific. In your patch, the assumption that PCC is the only ACPI mbox provider, maybe true today, but that can change anytime. > >> + >> +/** >> + * pcc_mbox_free_channel - Clients call this to free their Channel. >> + * >> + * @chan: Pointer to the mailbox channel as returned by >> + * pcc_mbox_request_channel() >> + */ >> +void pcc_mbox_free_channel(struct mbox_chan *chan) >> +{ >> + unsigned long flags; >> + >> + if (!chan || !chan->cl) >> + return; >> + >> + 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; >> + >> + spin_unlock_irqrestore(&chan->lock, flags); >> +} >> +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); >> + >> +/** >> + * pcc_tx_done - Callback from Mailbox controller code to >> + * check if PCC message transmission completed. >> + * @chan: Pointer to Mailbox channel on which previous >> + * transmission occurred. >> + * >> + * Return: TRUE if succeeded. >> + */ >> +static bool pcc_tx_done(struct mbox_chan *chan) >> +{ >> + struct acpi_pcct_hw_reduced *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; >> + unsigned int retries = 0; >> + >> + /* Try a few times while waiting for platform to consume */ >> + while (!(readw_relaxed(&generic_comm_base->status) >> + & PCC_CMD_COMPLETE)) { >> + >> + if (retries++ < 5) >> + udelay(cmd_delay); >> + else { >> + /* >> + * If the remote is dead, this will cause the Mbox >> + * controller to timeout after mbox client.tx_tout >> + * msecs. >> + */ >> + pr_err("PCC platform did not respond.\n"); >> + return false; >> + } >> + } >> + return true; >> +} >> + >> +/** >> + * get_subspace_id - Given a Mailbox channel, find out the >> + * PCC subspace id. >> + * @chan: Pointer to Mailbox Channel from which we want >> + * the index. >> + * Return: Errno if not found, else positive index number. >> + */ >> +static int get_subspace_id(struct mbox_chan *chan) >> +{ >> + int id = chan - pcc_mbox_chan; >> + >> + if (id < 0 || id > pcc_mbox_ctrl.num_chans) >> + return -ENOENT; >> + >> + return id; >> +} >> + >> +/** >> + * pcc_send_data - Called from Mailbox Controller code to finally >> + * transmit data over channel. >> + * @chan: Pointer to Mailbox channel over which to send data. >> + * @data: Actual data to be written over channel. >> + * >> + * Return: Err if something failed else 0 for success. >> + */ >> +static int pcc_send_data(struct mbox_chan *chan, void *data) >> +{ >> + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; >> + u16 ss_idx = -1; >> + >> + ss_idx = get_subspace_id(chan); >> + >> + if (ss_idx < 0) { >> + pr_err("Invalid Subspace ID from PCC client\n"); >> + return -EINVAL; >> + } >> + >> + doorbell = pcct_ss->doorbell_register; >> + doorbell_preserve = pcct_ss->preserve_mask; >> + doorbell_write = pcct_ss->write_mask; >> + >> + /* Write to the shared comm region. */ >> + writew(cmd, &generic_comm_base->command); >> + >> + /* Write Subspace MAGIC value so platform can identify destination. */ >> + writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); >> + >> + /* Flip CMD COMPLETE bit */ >> + writew(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); >> + >> + return 0; >> +} >> + >> +static struct mbox_chan_ops pcc_chan_ops = { >> + .send_data = pcc_send_data, >> + .last_tx_done = pcc_tx_done, >> +}; >> + >> +/** >> + * parse_pcc_subspace - Parse the PCC table and extract PCC subspace >> + * entries. There should be one entry per PCC client. >> + * @header: Pointer to the ACPI subtable header under the PCCT. >> + * @end: End of subtable entry. >> + * >> + * Return: 0 for Success, else errno. >> + * >> + * This gets called for each entry in the PCC table. >> + */ >> +static int parse_pcc_subspace(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_pcct_hw_reduced *pcct_ss; >> + >> + if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) { >> + pcct_ss = (struct acpi_pcct_hw_reduced *) header; >> + >> + if (pcct_ss->header.type != >> + ACPI_PCCT_TYPE_HW_REDUCED_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; >> +} >> + >> +/** >> + * acpi_pcc_probe - Parse the ACPI tree for the PCCT. >> + * >> + * Return: 0 for Success, else errno. >> + */ >> +static int __init acpi_pcc_probe(void) >> +{ >> + acpi_status status = AE_OK; >> + acpi_size pcct_tbl_header_size; >> + int count; >> + struct acpi_table_pcct *pcct_tbl; >> + >> + /* 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_FAILURE(status) || !pcct_tbl) { >> + pr_warn("PCCT header not found.\n"); >> + return -ENODEV; >> + } >> + >> + count = acpi_table_parse_entries(ACPI_SIG_PCCT, >> + sizeof(struct acpi_table_pcct), >> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE, >> + parse_pcc_subspace, MAX_PCC_SUBSPACES); >> + >> + if (count <= 0) { >> + pr_err("Error parsing PCC subspaces from PCCT\n"); >> + return -EINVAL; >> + } >> + >> + pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans); >> + >> + return 0; >> +} >> + >> +/** >> + * pcc_mbox_probe - Called when we find a match for the >> + * PCCT platform device. This is purely used to represent >> + * the PCCT as a virtual device for registering with the >> + * generic Mailbox framework. >> + * >> + * @pdev: Pointer to platform device returned when a match >> + * is found. >> + * >> + * Return: 0 for Success, else errno. >> + */ >> +static int pcc_mbox_probe(struct platform_device *pdev) >> +{ >> + int ret = 0; >> + >> + pcc_mbox_ctrl.chans = pcc_mbox_chan; >> + pcc_mbox_ctrl.ops = &pcc_chan_ops; >> + pcc_mbox_ctrl.txdone_poll = true; >> + pcc_mbox_ctrl.txpoll_period = 10; >> + pcc_mbox_ctrl.dev = &pdev->dev; >> + >> + 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: %d\n", ret); >> + ret = -ENODEV; >> + } >> + >> + return ret; >> +} >> + >> +struct platform_driver pcc_mbox_driver = { >> + .probe = pcc_mbox_probe, >> + .driver = { >> + .name = "PCCT", >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +static int __init pcc_init(void) >> +{ >> + int ret; >> + struct platform_device *pcc_pdev; >> + >> + if (acpi_disabled) >> + return -ENODEV; >> + >> + /* Check if PCC support is available. */ >> + ret = acpi_pcc_probe(); >> + >> + if (ret) { >> + pr_err("ACPI PCC probe failed.\n"); >> + return -ENODEV; >> + } >> + >> + pcc_pdev = platform_create_bundle(&pcc_mbox_driver, >> + pcc_mbox_probe, NULL, 0, NULL, 0); >> + >> + if (!pcc_pdev) { >> + pr_err("Err creating PCC platform bundle\n"); >> + return -ENODEV; >> + } >> + >> + return 0; >> +} >> +device_initcall(pcc_init); >> > IMO the PCC platform_device should be populated by ACPI or DT core. > This PCC controller driver should parse the PCCT ... so it populates > .global_xlate() which expect the 'global_id' of channel requested to > be (0x50434300 | Client_Class_Code) as specified by ACPI. > > It should be possible to have same client and controller driver work > for both ACPI and DT. Any reason why this patch won't work with DT as well? (with the requisite DT bindings) Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 November 2014 20:50, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > Hi Jassi, > > On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>> + >>> +/** >>> + * pcc_mbox_request_channel - PCC clients call this function to >>> + * request a pointer to their PCC subspace, from which they >>> + * can get the details of communicating with the remote. >>> + * @cl: Pointer to Mailbox client, so we know where to bind the >>> + * Channel. >>> + * @index: The PCC Subspace index as parsed in the PCC client >>> + * ACPI package. This is used to lookup the array of PCC >>> + * subspaces as parsed by the PCC Mailbox controller. >>> + * >>> + * Return: Pointer to the Mailbox Channel if successful or >>> + * ERR_PTR. >>> + */ >>> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >>> + int index) >>> +{ >>> + struct device *dev = pcc_mbox_ctrl.dev; >>> + struct mbox_chan *chan; >>> + unsigned long flags; >>> + >>> + /* >>> + * Each PCC Subspace is a Mailbox Channel. >>> + * The PCC Clients get their PCC Subspace ID >>> + * from their own tables and pass it here. >>> + * This returns a pointer to the PCC subspace >>> + * for the Client to operate on. >>> + */ >>> + chan = &pcc_mbox_chan[index]; >>> + >>> + if (!chan || chan->cl) { >>> + dev_err(dev, "%s: PCC mailbox not free\n", __func__); >>> + return ERR_PTR(-EBUSY); >>> + } >>> + >>> + spin_lock_irqsave(&chan->lock, flags); >>> + chan->msg_free = 0; >>> + chan->msg_count = 0; >>> + chan->active_req = NULL; >>> + chan->cl = cl; >>> + init_completion(&chan->tx_complete); >>> + >>> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >>> + chan->txdone_method |= TXDONE_BY_ACK; >>> + >>> + spin_unlock_irqrestore(&chan->lock, flags); >>> + >>> + return chan; >>> +} >>> +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); >> >> I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and >> kept same people in CC. >> That patch keeps the api same for DT and ACPI drivers. > > Thanks for taking a look. > > Your patch looks similar in concept to one of my earlier patches. But > Using ifdefs wasn't the right way to enable ACPI channel mapping. The mailbox core should be able to work with both DT and ACPI at the same time. > based on the discussions that followed since, we decided that its best > to add a separate PCC lookup and registration API. The main reason > being, we dont have a way to list all mbox providers in ACPI in a way > that DT does. e.g. in DT, the client->dev is used to look up mbox > controllers. In ACPI, a client cant specify which mbox controllers to > associate with, if it can attach to multiple. With the PCC specific > API, if the client calls it, the controller knows where to look, > because that lookup is PCC specific. > > In your patch, the assumption that PCC is the only ACPI mbox provider, > maybe true today, but that can change anytime. > Please read my patch again, we can have ACPI as well as DT populated clients. All that you intend to do with this patch can be done there and _without_ adding new apis. >>> + >>> + pcc_pdev = platform_create_bundle(&pcc_mbox_driver, >>> + pcc_mbox_probe, NULL, 0, NULL, 0); >>> + >>> + if (!pcc_pdev) { >>> + pr_err("Err creating PCC platform bundle\n"); >>> + return -ENODEV; >>> + } >>> + >>> + return 0; >>> +} >>> +device_initcall(pcc_init); >>> >> IMO the PCC platform_device should be populated by ACPI or DT core. >> This PCC controller driver should parse the PCCT ... so it populates >> .global_xlate() which expect the 'global_id' of channel requested to >> be (0x50434300 | Client_Class_Code) as specified by ACPI. >> >> It should be possible to have same client and controller driver work >> for both ACPI and DT. > > Any reason why this patch won't work with DT as well? (with the > requisite DT bindings) > A PCC client would need to call pcc_mbox_request_channel() if populated by ACPI, and mbox_request_channel() if by DT. We want any client driver to call just mbox_request_channel() -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 November 2014 11:18, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 9 November 2014 20:50, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> Hi Jassi, >> >> On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>> + >>>> +/** >>>> + * pcc_mbox_request_channel - PCC clients call this function to >>>> + * request a pointer to their PCC subspace, from which they >>>> + * can get the details of communicating with the remote. >>>> + * @cl: Pointer to Mailbox client, so we know where to bind the >>>> + * Channel. >>>> + * @index: The PCC Subspace index as parsed in the PCC client >>>> + * ACPI package. This is used to lookup the array of PCC >>>> + * subspaces as parsed by the PCC Mailbox controller. >>>> + * >>>> + * Return: Pointer to the Mailbox Channel if successful or >>>> + * ERR_PTR. >>>> + */ >>>> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >>>> + int index) >>>> +{ >>>> + struct device *dev = pcc_mbox_ctrl.dev; >>>> + struct mbox_chan *chan; >>>> + unsigned long flags; >>>> + >>>> + /* >>>> + * Each PCC Subspace is a Mailbox Channel. >>>> + * The PCC Clients get their PCC Subspace ID >>>> + * from their own tables and pass it here. >>>> + * This returns a pointer to the PCC subspace >>>> + * for the Client to operate on. >>>> + */ >>>> + chan = &pcc_mbox_chan[index]; >>>> + >>>> + if (!chan || chan->cl) { >>>> + dev_err(dev, "%s: PCC mailbox not free\n", __func__); >>>> + return ERR_PTR(-EBUSY); >>>> + } >>>> + >>>> + spin_lock_irqsave(&chan->lock, flags); >>>> + chan->msg_free = 0; >>>> + chan->msg_count = 0; >>>> + chan->active_req = NULL; >>>> + chan->cl = cl; >>>> + init_completion(&chan->tx_complete); >>>> + >>>> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >>>> + chan->txdone_method |= TXDONE_BY_ACK; >>>> + >>>> + spin_unlock_irqrestore(&chan->lock, flags); >>>> + >>>> + return chan; >>>> +} >>>> +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); >>> >>> I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and >>> kept same people in CC. >>> That patch keeps the api same for DT and ACPI drivers. >> >> Thanks for taking a look. >> >> Your patch looks similar in concept to one of my earlier patches. But >> > Using ifdefs wasn't the right way to enable ACPI channel mapping. The > mailbox core should be able to work with both DT and ACPI at the same > time. Agreed. But my point there was to add an alternative to the DT specific of_xlate, which is exactly what you've done in global_xlate. > >> based on the discussions that followed since, we decided that its best >> to add a separate PCC lookup and registration API. The main reason >> being, we dont have a way to list all mbox providers in ACPI in a way >> that DT does. e.g. in DT, the client->dev is used to look up mbox >> controllers. In ACPI, a client cant specify which mbox controllers to >> associate with, if it can attach to multiple. With the PCC specific >> API, if the client calls it, the controller knows where to look, >> because that lookup is PCC specific. >> >> In your patch, the assumption that PCC is the only ACPI mbox provider, >> maybe true today, but that can change anytime. >> > Please read my patch again, we can have ACPI as well as DT populated > clients. All that you intend to do with this patch can be done there > and _without_ adding new apis. Read it again. Not arguing that your patch wont work for DT and ACPI, but your assumption that ACPI supports PCC as the only mbox controller, may not hold true. The global_xlate function will work fine for PCC, but may not work for other ACPI (non-DT) mbox controllers. Using the signature field/index byte works only for PCC. We've already been through this discussion with Mark and Arnd and we came up with the PCC API. > >>>> + >>>> + pcc_pdev = platform_create_bundle(&pcc_mbox_driver, >>>> + pcc_mbox_probe, NULL, 0, NULL, 0); >>>> + >>>> + if (!pcc_pdev) { >>>> + pr_err("Err creating PCC platform bundle\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> +device_initcall(pcc_init); >>>> >>> IMO the PCC platform_device should be populated by ACPI or DT core. >>> This PCC controller driver should parse the PCCT ... so it populates >>> .global_xlate() which expect the 'global_id' of channel requested to >>> be (0x50434300 | Client_Class_Code) as specified by ACPI. >>> >>> It should be possible to have same client and controller driver work >>> for both ACPI and DT. >> >> Any reason why this patch won't work with DT as well? (with the >> requisite DT bindings) >> > A PCC client would need to call pcc_mbox_request_channel() if > populated by ACPI, and mbox_request_channel() if by DT. We want any > client driver to call just mbox_request_channel() This would work if in ACPI (non-DT) cases there is a clear way to map a client to a controller. We dont have that in ACPI when a client could use multiple mbox controllers, hence the separate API. If you assume that in ACPI there is only one mbox controller, then this is fine, but I was guided to not implement with this assumption. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 November 2014 22:52, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 9 November 2014 11:18, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 9 November 2014 20:50, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>> Hi Jassi, >>> >>> On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>>> + >>>>> +/** >>>>> + * pcc_mbox_request_channel - PCC clients call this function to >>>>> + * request a pointer to their PCC subspace, from which they >>>>> + * can get the details of communicating with the remote. >>>>> + * @cl: Pointer to Mailbox client, so we know where to bind the >>>>> + * Channel. >>>>> + * @index: The PCC Subspace index as parsed in the PCC client >>>>> + * ACPI package. This is used to lookup the array of PCC >>>>> + * subspaces as parsed by the PCC Mailbox controller. >>>>> + * >>>>> + * Return: Pointer to the Mailbox Channel if successful or >>>>> + * ERR_PTR. >>>>> + */ >>>>> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >>>>> + int index) >>>>> +{ >>>>> + struct device *dev = pcc_mbox_ctrl.dev; >>>>> + struct mbox_chan *chan; >>>>> + unsigned long flags; >>>>> + >>>>> + /* >>>>> + * Each PCC Subspace is a Mailbox Channel. >>>>> + * The PCC Clients get their PCC Subspace ID >>>>> + * from their own tables and pass it here. >>>>> + * This returns a pointer to the PCC subspace >>>>> + * for the Client to operate on. >>>>> + */ >>>>> + chan = &pcc_mbox_chan[index]; >>>>> + >>>>> + if (!chan || chan->cl) { >>>>> + dev_err(dev, "%s: PCC mailbox not free\n", __func__); >>>>> + return ERR_PTR(-EBUSY); >>>>> + } >>>>> + >>>>> + spin_lock_irqsave(&chan->lock, flags); >>>>> + chan->msg_free = 0; >>>>> + chan->msg_count = 0; >>>>> + chan->active_req = NULL; >>>>> + chan->cl = cl; >>>>> + init_completion(&chan->tx_complete); >>>>> + >>>>> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >>>>> + chan->txdone_method |= TXDONE_BY_ACK; >>>>> + >>>>> + spin_unlock_irqrestore(&chan->lock, flags); >>>>> + >>>>> + return chan; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); >>>> >>>> I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and >>>> kept same people in CC. >>>> That patch keeps the api same for DT and ACPI drivers. >>> >>> Thanks for taking a look. >>> >>> Your patch looks similar in concept to one of my earlier patches. But >>> >> Using ifdefs wasn't the right way to enable ACPI channel mapping. The >> mailbox core should be able to work with both DT and ACPI at the same >> time. > > Agreed. But my point there was to add an alternative to the DT > specific of_xlate, which is exactly what you've done in global_xlate. > We all have good intentions, the problem is the code :) >>> based on the discussions that followed since, we decided that its best >>> to add a separate PCC lookup and registration API. The main reason >>> being, we dont have a way to list all mbox providers in ACPI in a way >>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>> controllers. In ACPI, a client cant specify which mbox controllers to >>> associate with, if it can attach to multiple. With the PCC specific >>> API, if the client calls it, the controller knows where to look, >>> because that lookup is PCC specific. >>> >>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>> maybe true today, but that can change anytime. >>> >> Please read my patch again, we can have ACPI as well as DT populated >> clients. All that you intend to do with this patch can be done there >> and _without_ adding new apis. > > Read it again. Not arguing that your patch wont work for DT and ACPI, > but your assumption that ACPI supports PCC as the only mbox > controller, may not hold true. The global_xlate function will work > fine for PCC, but may not work for other ACPI (non-DT) mbox > controllers. Using the signature field/index byte works only for PCC. > We've already been through this discussion with Mark and Arnd and we > came up with the PCC API. > Please read it yet again. There is no assumption that PCC is the only mbox in ACPI (though I think that is very likely). The function and its argument are both named _global_. 'Signature' is mentioned only as an example in case of PCC. The PCC controller driver could expect the global_id to be 'signature' of the subspace, similarly another non-DT mailbox controller driver will expect its own different 'signature' (say 0xdead0000 | id_16bits). In the patch I submitted we try .global_xlate() of all such mboxes and only one, which finds its id-space specified, will return a channel. Ideally, global-id space isn't very clean, but for mailbox we anyway have to have a direct understanding between controller and client drivers. So having global IDs is a great tradeoff if we avoid messing up the api. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 9 November 2014 22:52, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> On 9 November 2014 11:18, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> On 9 November 2014 20:50, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>> Hi Jassi, >>>> >>>> On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>>> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>>>> + >>>>>> +/** >>>>>> + * pcc_mbox_request_channel - PCC clients call this function to >>>>>> + * request a pointer to their PCC subspace, from which they >>>>>> + * can get the details of communicating with the remote. >>>>>> + * @cl: Pointer to Mailbox client, so we know where to bind the >>>>>> + * Channel. >>>>>> + * @index: The PCC Subspace index as parsed in the PCC client >>>>>> + * ACPI package. This is used to lookup the array of PCC >>>>>> + * subspaces as parsed by the PCC Mailbox controller. >>>>>> + * >>>>>> + * Return: Pointer to the Mailbox Channel if successful or >>>>>> + * ERR_PTR. >>>>>> + */ >>>>>> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >>>>>> + int index) >>>>>> +{ >>>>>> + struct device *dev = pcc_mbox_ctrl.dev; >>>>>> + struct mbox_chan *chan; >>>>>> + unsigned long flags; >>>>>> + >>>>>> + /* >>>>>> + * Each PCC Subspace is a Mailbox Channel. >>>>>> + * The PCC Clients get their PCC Subspace ID >>>>>> + * from their own tables and pass it here. >>>>>> + * This returns a pointer to the PCC subspace >>>>>> + * for the Client to operate on. >>>>>> + */ >>>>>> + chan = &pcc_mbox_chan[index]; >>>>>> + >>>>>> + if (!chan || chan->cl) { >>>>>> + dev_err(dev, "%s: PCC mailbox not free\n", __func__); >>>>>> + return ERR_PTR(-EBUSY); >>>>>> + } >>>>>> + >>>>>> + spin_lock_irqsave(&chan->lock, flags); >>>>>> + chan->msg_free = 0; >>>>>> + chan->msg_count = 0; >>>>>> + chan->active_req = NULL; >>>>>> + chan->cl = cl; >>>>>> + init_completion(&chan->tx_complete); >>>>>> + >>>>>> + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) >>>>>> + chan->txdone_method |= TXDONE_BY_ACK; >>>>>> + >>>>>> + spin_unlock_irqrestore(&chan->lock, flags); >>>>>> + >>>>>> + return chan; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); >>>>> >>>>> I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and >>>>> kept same people in CC. >>>>> That patch keeps the api same for DT and ACPI drivers. >>>> >>>> Thanks for taking a look. >>>> >>>> Your patch looks similar in concept to one of my earlier patches. But >>>> >>> Using ifdefs wasn't the right way to enable ACPI channel mapping. The >>> mailbox core should be able to work with both DT and ACPI at the same >>> time. >> >> Agreed. But my point there was to add an alternative to the DT >> specific of_xlate, which is exactly what you've done in global_xlate. >> > We all have good intentions, the problem is the code :) > >>>> based on the discussions that followed since, we decided that its best >>>> to add a separate PCC lookup and registration API. The main reason >>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>> associate with, if it can attach to multiple. With the PCC specific >>>> API, if the client calls it, the controller knows where to look, >>>> because that lookup is PCC specific. >>>> >>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>> maybe true today, but that can change anytime. >>>> >>> Please read my patch again, we can have ACPI as well as DT populated >>> clients. All that you intend to do with this patch can be done there >>> and _without_ adding new apis. >> >> Read it again. Not arguing that your patch wont work for DT and ACPI, >> but your assumption that ACPI supports PCC as the only mbox >> controller, may not hold true. The global_xlate function will work >> fine for PCC, but may not work for other ACPI (non-DT) mbox >> controllers. Using the signature field/index byte works only for PCC. >> We've already been through this discussion with Mark and Arnd and we >> came up with the PCC API. >> > Please read it yet again. There is no assumption that PCC is the only > mbox in ACPI (though I think that is very likely). The function and > its argument are both named _global_. 'Signature' is mentioned only as > an example in case of PCC. The PCC controller driver could expect the > global_id to be 'signature' of the subspace, similarly another non-DT > mailbox controller driver will expect its own different 'signature' > (say 0xdead0000 | id_16bits). In the patch I submitted we try > .global_xlate() of all such mboxes and only one, which finds its > id-space specified, will return a channel. > > Ideally, global-id space isn't very clean, but for mailbox we anyway > have to have a direct understanding between controller and client > drivers. So having global IDs is a great tradeoff if we avoid messing > up the api. How is this different than expecting the client to pass a string name of the mbox controller it wants? Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> >>>>> based on the discussions that followed since, we decided that its best >>>>> to add a separate PCC lookup and registration API. The main reason >>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>> associate with, if it can attach to multiple. With the PCC specific >>>>> API, if the client calls it, the controller knows where to look, >>>>> because that lookup is PCC specific. >>>>> >>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>> maybe true today, but that can change anytime. >>>>> >>>> Please read my patch again, we can have ACPI as well as DT populated >>>> clients. All that you intend to do with this patch can be done there >>>> and _without_ adding new apis. >>> >>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>> but your assumption that ACPI supports PCC as the only mbox >>> controller, may not hold true. The global_xlate function will work >>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>> controllers. Using the signature field/index byte works only for PCC. >>> We've already been through this discussion with Mark and Arnd and we >>> came up with the PCC API. >>> >> Please read it yet again. There is no assumption that PCC is the only >> mbox in ACPI (though I think that is very likely). The function and >> its argument are both named _global_. 'Signature' is mentioned only as >> an example in case of PCC. The PCC controller driver could expect the >> global_id to be 'signature' of the subspace, similarly another non-DT >> mailbox controller driver will expect its own different 'signature' >> (say 0xdead0000 | id_16bits). In the patch I submitted we try >> .global_xlate() of all such mboxes and only one, which finds its >> id-space specified, will return a channel. >> >> Ideally, global-id space isn't very clean, but for mailbox we anyway >> have to have a direct understanding between controller and client >> drivers. So having global IDs is a great tradeoff if we avoid messing >> up the api. > > How is this different than expecting the client to pass a string name > of the mbox controller it wants? > Global-ID is ugly, string matching is uglier. String matching requires changes to client and provider structures as opposed to simple numerical comparison to find a suitable channel. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> >>>>>> based on the discussions that followed since, we decided that its best >>>>>> to add a separate PCC lookup and registration API. The main reason >>>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>>> associate with, if it can attach to multiple. With the PCC specific >>>>>> API, if the client calls it, the controller knows where to look, >>>>>> because that lookup is PCC specific. >>>>>> >>>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>>> maybe true today, but that can change anytime. >>>>>> >>>>> Please read my patch again, we can have ACPI as well as DT populated >>>>> clients. All that you intend to do with this patch can be done there >>>>> and _without_ adding new apis. >>>> >>>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>>> but your assumption that ACPI supports PCC as the only mbox >>>> controller, may not hold true. The global_xlate function will work >>>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>>> controllers. Using the signature field/index byte works only for PCC. >>>> We've already been through this discussion with Mark and Arnd and we >>>> came up with the PCC API. >>>> >>> Please read it yet again. There is no assumption that PCC is the only >>> mbox in ACPI (though I think that is very likely). The function and >>> its argument are both named _global_. 'Signature' is mentioned only as >>> an example in case of PCC. The PCC controller driver could expect the >>> global_id to be 'signature' of the subspace, similarly another non-DT >>> mailbox controller driver will expect its own different 'signature' >>> (say 0xdead0000 | id_16bits). In the patch I submitted we try >>> .global_xlate() of all such mboxes and only one, which finds its >>> id-space specified, will return a channel. >>> >>> Ideally, global-id space isn't very clean, but for mailbox we anyway >>> have to have a direct understanding between controller and client >>> drivers. So having global IDs is a great tradeoff if we avoid messing >>> up the api. >> >> How is this different than expecting the client to pass a string name >> of the mbox controller it wants? >> > Global-ID is ugly, string matching is uglier. String matching requires > changes to client and provider structures as opposed to simple > numerical comparison to find a suitable channel. And both have the problem that we cant guarantee uniqueness [1][2]. Having a separate API solves this problem. Ashwin [1] - http://www.spinics.net/lists/linux-acpi/msg52271.html [2] - http://www.spinics.net/lists/linux-acpi/msg52377.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>> >>>>>>> based on the discussions that followed since, we decided that its best >>>>>>> to add a separate PCC lookup and registration API. The main reason >>>>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>>>> associate with, if it can attach to multiple. With the PCC specific >>>>>>> API, if the client calls it, the controller knows where to look, >>>>>>> because that lookup is PCC specific. >>>>>>> >>>>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>>>> maybe true today, but that can change anytime. >>>>>>> >>>>>> Please read my patch again, we can have ACPI as well as DT populated >>>>>> clients. All that you intend to do with this patch can be done there >>>>>> and _without_ adding new apis. >>>>> >>>>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>>>> but your assumption that ACPI supports PCC as the only mbox >>>>> controller, may not hold true. The global_xlate function will work >>>>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>>>> controllers. Using the signature field/index byte works only for PCC. >>>>> We've already been through this discussion with Mark and Arnd and we >>>>> came up with the PCC API. >>>>> >>>> Please read it yet again. There is no assumption that PCC is the only >>>> mbox in ACPI (though I think that is very likely). The function and >>>> its argument are both named _global_. 'Signature' is mentioned only as >>>> an example in case of PCC. The PCC controller driver could expect the >>>> global_id to be 'signature' of the subspace, similarly another non-DT >>>> mailbox controller driver will expect its own different 'signature' >>>> (say 0xdead0000 | id_16bits). In the patch I submitted we try >>>> .global_xlate() of all such mboxes and only one, which finds its >>>> id-space specified, will return a channel. >>>> >>>> Ideally, global-id space isn't very clean, but for mailbox we anyway >>>> have to have a direct understanding between controller and client >>>> drivers. So having global IDs is a great tradeoff if we avoid messing >>>> up the api. >>> >>> How is this different than expecting the client to pass a string name >>> of the mbox controller it wants? >>> >> Global-ID is ugly, string matching is uglier. String matching requires >> changes to client and provider structures as opposed to simple >> numerical comparison to find a suitable channel. > > And both have the problem that we cant guarantee uniqueness [1][2]. > How? Please give some scenario. > Having a separate API solves this problem. > NO. You add new api for PCC. If another non-DT provider appears (another instance of PCC or some new non-DT non-PCC mbox device) .... do you plan add yet another api? By the way, your patch in this thread can't even cope with 2 instances of PCC (assuming that's possible as you think). -jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>>> >>>>>>>> based on the discussions that followed since, we decided that its best >>>>>>>> to add a separate PCC lookup and registration API. The main reason >>>>>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>>>>> associate with, if it can attach to multiple. With the PCC specific >>>>>>>> API, if the client calls it, the controller knows where to look, >>>>>>>> because that lookup is PCC specific. >>>>>>>> >>>>>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>>>>> maybe true today, but that can change anytime. >>>>>>>> >>>>>>> Please read my patch again, we can have ACPI as well as DT populated >>>>>>> clients. All that you intend to do with this patch can be done there >>>>>>> and _without_ adding new apis. >>>>>> >>>>>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>>>>> but your assumption that ACPI supports PCC as the only mbox >>>>>> controller, may not hold true. The global_xlate function will work >>>>>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>>>>> controllers. Using the signature field/index byte works only for PCC. >>>>>> We've already been through this discussion with Mark and Arnd and we >>>>>> came up with the PCC API. >>>>>> >>>>> Please read it yet again. There is no assumption that PCC is the only >>>>> mbox in ACPI (though I think that is very likely). The function and >>>>> its argument are both named _global_. 'Signature' is mentioned only as >>>>> an example in case of PCC. The PCC controller driver could expect the >>>>> global_id to be 'signature' of the subspace, similarly another non-DT >>>>> mailbox controller driver will expect its own different 'signature' >>>>> (say 0xdead0000 | id_16bits). In the patch I submitted we try >>>>> .global_xlate() of all such mboxes and only one, which finds its >>>>> id-space specified, will return a channel. >>>>> >>>>> Ideally, global-id space isn't very clean, but for mailbox we anyway >>>>> have to have a direct understanding between controller and client >>>>> drivers. So having global IDs is a great tradeoff if we avoid messing >>>>> up the api. >>>> >>>> How is this different than expecting the client to pass a string name >>>> of the mbox controller it wants? >>>> >>> Global-ID is ugly, string matching is uglier. String matching requires >>> changes to client and provider structures as opposed to simple >>> numerical comparison to find a suitable channel. >> >> And both have the problem that we cant guarantee uniqueness [1][2]. >> > How? Please give some scenario. > What is there to stop two users from coming up with the same signature (0xdeadbeef / "string") for their mbox controllers? Things can break at run time, because with your patch, the first mbox controller with the duplicate signature/name will return a channel. The client may be expecting a channel from the "other" mbox. >> Having a separate API solves this problem. >> > NO. > You add new api for PCC. If another non-DT provider appears (another > instance of PCC or some new non-DT non-PCC mbox device) .... do you > plan add yet another api? Yes. Unfortunately thats the only way as Arnd suggested [1]. Again, the reason is that ACPI does NOT provide a way for client to mbox mapping in a way DT does. [1] You were CC'd on that thread. This patch has been under review for ~5months now and has undergone extensive review from Mark, Arnd and Lv. We're really going around in circles now. > By the way, your patch in this thread can't even cope with 2 > instances of PCC (assuming that's possible as you think). It was never meant to be. There are no two instances of PCC. But there could be another mbox provider(non-PCC) in ACPI in the future. Arnd was guessing something to appear from the newer Intel designs.[2] Ashwin [1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265395.html Please read the part where Arnd suggests the following: "If you require drivers to put global data (e.g. the mbox->name, or the channel number) in there, it's impossible to write a driver that works on both DT and ACPI. If you want to use the mbox_request_channel() interface from a driver, you need some form of lookup table in the ACPI data to do the conversion." [2] - https://patches.linaro.org/32299/ Please read the very first comments. "It's mostly a matter of consistency: We can have multiple interrupt controllers, pin controllers, clock controllers, dma engines, etc, and in the DT case we use references to the nodes wherever we have other devices referring to a mailbox name. I believe Intel's embedded chips are moving in the same direction with their ACPI support. If the ACPI spec gains support for mailbox devices, locking them into having only a single device may be a problem later for them." -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>> On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>> On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>>>> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>>>> >>>>>>>>> based on the discussions that followed since, we decided that its best >>>>>>>>> to add a separate PCC lookup and registration API. The main reason >>>>>>>>> being, we dont have a way to list all mbox providers in ACPI in a way >>>>>>>>> that DT does. e.g. in DT, the client->dev is used to look up mbox >>>>>>>>> controllers. In ACPI, a client cant specify which mbox controllers to >>>>>>>>> associate with, if it can attach to multiple. With the PCC specific >>>>>>>>> API, if the client calls it, the controller knows where to look, >>>>>>>>> because that lookup is PCC specific. >>>>>>>>> >>>>>>>>> In your patch, the assumption that PCC is the only ACPI mbox provider, >>>>>>>>> maybe true today, but that can change anytime. >>>>>>>>> >>>>>>>> Please read my patch again, we can have ACPI as well as DT populated >>>>>>>> clients. All that you intend to do with this patch can be done there >>>>>>>> and _without_ adding new apis. >>>>>>> >>>>>>> Read it again. Not arguing that your patch wont work for DT and ACPI, >>>>>>> but your assumption that ACPI supports PCC as the only mbox >>>>>>> controller, may not hold true. The global_xlate function will work >>>>>>> fine for PCC, but may not work for other ACPI (non-DT) mbox >>>>>>> controllers. Using the signature field/index byte works only for PCC. >>>>>>> We've already been through this discussion with Mark and Arnd and we >>>>>>> came up with the PCC API. >>>>>>> >>>>>> Please read it yet again. There is no assumption that PCC is the only >>>>>> mbox in ACPI (though I think that is very likely). The function and >>>>>> its argument are both named _global_. 'Signature' is mentioned only as >>>>>> an example in case of PCC. The PCC controller driver could expect the >>>>>> global_id to be 'signature' of the subspace, similarly another non-DT >>>>>> mailbox controller driver will expect its own different 'signature' >>>>>> (say 0xdead0000 | id_16bits). In the patch I submitted we try >>>>>> .global_xlate() of all such mboxes and only one, which finds its >>>>>> id-space specified, will return a channel. >>>>>> >>>>>> Ideally, global-id space isn't very clean, but for mailbox we anyway >>>>>> have to have a direct understanding between controller and client >>>>>> drivers. So having global IDs is a great tradeoff if we avoid messing >>>>>> up the api. >>>>> >>>>> How is this different than expecting the client to pass a string name >>>>> of the mbox controller it wants? >>>>> >>>> Global-ID is ugly, string matching is uglier. String matching requires >>>> changes to client and provider structures as opposed to simple >>>> numerical comparison to find a suitable channel. >>> >>> And both have the problem that we cant guarantee uniqueness [1][2]. >>> >> How? Please give some scenario. >> > > What is there to stop two users from coming up with the same signature > (0xdeadbeef / "string") for their mbox controllers? Things can break > at run time, because with your patch, the first mbox controller with > the duplicate signature/name will return a channel. The client may be > expecting a channel from the "other" mbox. > Two channels with same signature are supposed to be _identical_ i.e, either channel could serve any client asking for a channel with that signature. So even if an "unexpected" instance of the channel is assigned, the client should still be happy. If a client differentiates between 2 instances of a channel, that's probably a sign of bad design. The knowledge behind client's preference of instance should actually lie on the provider(controller) side. I am open to some example on the contrary. > Again, the reason is that ACPI does NOT provide a way for client to mbox > mapping in a way DT does. [1] You were CC'd on that thread. > That patch was obviously wrong and outright rejected by Arnd. I didn't have to object. > This patch has been under review for ~5months now and has undergone extensive > review from Mark, Arnd and Lv. We're really going around in circles > now. > You kept me out of CC for the last 9 revisions(~5months). I am not sure if I am at fault for proposing a better solution at this stage. BTW, I think Arnd will find I have take care of most(if not all) of his concerns. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote: > On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > > On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@linaro.org> wrote: > >> On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: Folks, please delete unneeded context from mails - it can be quite tedious to find the new content otherwise. > > What is there to stop two users from coming up with the same signature > > (0xdeadbeef / "string") for their mbox controllers? Things can break > > at run time, because with your patch, the first mbox controller with > > the duplicate signature/name will return a channel. The client may be > > expecting a channel from the "other" mbox. > Two channels with same signature are supposed to be _identical_ i.e, > either channel could serve any client asking for a channel with that > signature. So even if an "unexpected" instance of the channel is > assigned, the client should still be happy. > If a client differentiates between 2 instances of a channel, that's > probably a sign of bad design. The knowledge behind client's > preference of instance should actually lie on the provider(controller) > side. I am open to some example on the contrary. The problem here is that ACPI isn't defining the context in a way which maps well onto the way Linux looks things up. We may not like that and think it's a bad design but the spec is a done deal here and we have to address reality. From an ACPI point of view the context is the fact that this is a PCC channel (there's a globally unique namespace for PCC channels) but Linux wants a struct device for the client to represent the context with a mapping table of some kind behind that to do the lookup. There's nothing in the ACPI description of the channel or controller to tie it to the client device, and there's nothing preventing some other mailbox mechanism that gets added to an ACPI system from reusing similar names (bear in mind that idiomatic naming for ACPI appears to be three or four character strings). If we have a PCC channel "FOO" and some new mailbox type which also defines "FOO" the controllers aren't really going to be able to tell them apart just on the string. We could fit the maping into Linux a bit by having a struct device representing the PCC controller that you use to do the lookup but at that point you may as well have a PCC specific request function that knows that device and does the lookup which is approximately what we have in Ashwin's patch. We could also require that the lookup be something like "PCC:FOO" and use a global_xlate() but it's not clear to me that this is making things clearer.
On Monday 10 November 2014 20:13:10 Mark Brown wrote: > On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote: > > On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > > > What is there to stop two users from coming up with the same signature > > > (0xdeadbeef / "string") for their mbox controllers? Things can break > > > at run time, because with your patch, the first mbox controller with > > > the duplicate signature/name will return a channel. The client may be > > > expecting a channel from the "other" mbox. > > > Two channels with same signature are supposed to be _identical_ i.e, > > either channel could serve any client asking for a channel with that > > signature. So even if an "unexpected" instance of the channel is > > assigned, the client should still be happy. > > > If a client differentiates between 2 instances of a channel, that's > > probably a sign of bad design. The knowledge behind client's > > preference of instance should actually lie on the provider(controller) > > side. I am open to some example on the contrary. > > The problem here is that ACPI isn't defining the context in a way which > maps well onto the way Linux looks things up. We may not like that and > think it's a bad design but the spec is a done deal here and we have to > address reality. From an ACPI point of view the context is the fact > that this is a PCC channel (there's a globally unique namespace for PCC > channels) but Linux wants a struct device for the client to represent > the context with a mapping table of some kind behind that to do the > lookup. Right. > There's nothing in the ACPI description of the channel or controller to > tie it to the client device, and there's nothing preventing some other > mailbox mechanism that gets added to an ACPI system from reusing similar > names (bear in mind that idiomatic naming for ACPI appears to be three > or four character strings). If we have a PCC channel "FOO" and some new > mailbox type which also defines "FOO" the controllers aren't really > going to be able to tell them apart just on the string. > > We could fit the maping into Linux a bit by having a struct device > representing the PCC controller that you use to do the lookup but at > that point you may as well have a PCC specific request function that > knows that device and does the lookup which is approximately what we > have in Ashwin's patch. We could also require that the lookup be > something like "PCC:FOO" and use a global_xlate() but it's not clear to > me that this is making things clearer. Agreed. Having a special interface here the way that Ashwin's patch introduces seems the least invasive. I don't think we would risk running into the problems that Jassi mentioned regarding API growth if we get other cases. In particular, anything that boots with DT will be able to use the standard method, and even with ACPI if we have additional mailboxes we would most likely use the named properties extension in the future. On a side note, I think we will actually have to add a DT binding to PCC as well, but that should probably provide both the standard mailbox API as well as support the pcc specific interfaces in order to allow client drivers that do not have multiple ways of doing the same thing. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 01:43, Mark Brown <broonie@kernel.org> wrote: > On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote: >> On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> > On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> >> On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > >> > What is there to stop two users from coming up with the same signature >> > (0xdeadbeef / "string") for their mbox controllers? Things can break >> > at run time, because with your patch, the first mbox controller with >> > the duplicate signature/name will return a channel. The client may be >> > expecting a channel from the "other" mbox. > >> Two channels with same signature are supposed to be _identical_ i.e, >> either channel could serve any client asking for a channel with that >> signature. So even if an "unexpected" instance of the channel is >> assigned, the client should still be happy. > >> If a client differentiates between 2 instances of a channel, that's >> probably a sign of bad design. The knowledge behind client's >> preference of instance should actually lie on the provider(controller) >> side. I am open to some example on the contrary. > > The problem here is that ACPI isn't defining the context in a way which > maps well onto the way Linux looks things up. We may not like that and > think it's a bad design but the spec is a done deal here and we have to > address reality. > I don't mean that by 'bad design'. Ashwin asked what if a client wants the channel from, say, second instance of PCC instead of first (both are same controllers because they check for same signature). I can not imagine a good reason why would a client want that. If the reason is like only the channel of first controller is actually connected to remote (while that of second controller is nipped) ... I say that kind of board specific knowledge belongs closer to controller than the client. The second controller should have known that and not even populated the channel. > From an ACPI point of view the context is the fact > that this is a PCC channel (there's a globally unique namespace for PCC > channels) but Linux wants a struct device for the client to represent > the context with a mapping table of some kind behind that to do the > lookup. > > There's nothing in the ACPI description of the channel or controller to > tie it to the client device, and there's nothing preventing some other > mailbox mechanism that gets added to an ACPI system from reusing similar > names (bear in mind that idiomatic naming for ACPI appears to be three > or four character strings). If we have a PCC channel "FOO" and some new > mailbox type which also defines "FOO" the controllers aren't really > going to be able to tell them apart just on the string. > ACPI does specify a PCC specific signature (0x50434300) for its channels. Any channel is identified by doing OR of that signature and the index of channel/subspace in the array of 256. IF another class of controllers is given the same name "_PCC", the ACPI will atleast assign it a different signature (if not ACPI, the controller driver for Linux). So we need not even think about using strings. > We could fit the maping into Linux a bit by having a struct device > representing the PCC controller that you use to do the lookup but at > that point you may as well have a PCC specific request function that > knows that device and does the lookup which is approximately what we > have in Ashwin's patch. We could also require that the lookup be > something like "PCC:FOO" and use a global_xlate() but it's not clear to > me that this is making things clearer. > We already have a way to circumvent all of that. I don't just object to Ashwin's patch, I posted a more generic and robust solution here https://lkml.org/lkml/2014/11/9/75 -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 01:59, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 10 November 2014 20:13:10 Mark Brown wrote: >> On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote: >> > On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> > > What is there to stop two users from coming up with the same signature >> > > (0xdeadbeef / "string") for their mbox controllers? Things can break >> > > at run time, because with your patch, the first mbox controller with >> > > the duplicate signature/name will return a channel. The client may be >> > > expecting a channel from the "other" mbox. >> >> > Two channels with same signature are supposed to be _identical_ i.e, >> > either channel could serve any client asking for a channel with that >> > signature. So even if an "unexpected" instance of the channel is >> > assigned, the client should still be happy. >> >> > If a client differentiates between 2 instances of a channel, that's >> > probably a sign of bad design. The knowledge behind client's >> > preference of instance should actually lie on the provider(controller) >> > side. I am open to some example on the contrary. >> >> The problem here is that ACPI isn't defining the context in a way which >> maps well onto the way Linux looks things up. We may not like that and >> think it's a bad design but the spec is a done deal here and we have to >> address reality. From an ACPI point of view the context is the fact >> that this is a PCC channel (there's a globally unique namespace for PCC >> channels) but Linux wants a struct device for the client to represent >> the context with a mapping table of some kind behind that to do the >> lookup. > > Right. > >> There's nothing in the ACPI description of the channel or controller to >> tie it to the client device, and there's nothing preventing some other >> mailbox mechanism that gets added to an ACPI system from reusing similar >> names (bear in mind that idiomatic naming for ACPI appears to be three >> or four character strings). If we have a PCC channel "FOO" and some new >> mailbox type which also defines "FOO" the controllers aren't really >> going to be able to tell them apart just on the string. >> >> We could fit the maping into Linux a bit by having a struct device >> representing the PCC controller that you use to do the lookup but at >> that point you may as well have a PCC specific request function that >> knows that device and does the lookup which is approximately what we >> have in Ashwin's patch. We could also require that the lookup be >> something like "PCC:FOO" and use a global_xlate() but it's not clear to >> me that this is making things clearer. > > Agreed. Having a special interface here the way that Ashwin's patch > introduces seems the least invasive. > Just to be sure... please have a look at an alternative solution https://lkml.org/lkml/2014/11/9/75 I think 0 new api is lesser invasive than 2 new apis. > I don't think we would risk running > into the problems that Jassi mentioned regarding API growth if we > get other cases. > Do we anticipate .. a) more than one instance of PCC on a platform? b) PCC alongside another non-DT/ACPI mailbox device? I discount (a). But for (b) it doesn't look neat to either add yet another PCC like api or make non-PCC clients call pcc_mbox_request_channel(). Adapting mbox_request_channe() to accommodate even non-DT clients (like PCC) seems cleaner and more future proof. > In particular, anything that boots with DT will be > able to use the standard method, and even with ACPI if we have additional > mailboxes we would most likely use the named properties extension in > the future. > > On a side note, I think we will actually have to add a DT binding > to PCC as well, but that should probably provide both the standard > mailbox API as well as support the pcc specific interfaces in order > to allow client drivers that do not have multiple ways of doing the > same thing. > In that case, wouldn't it be cleaner to have client drivers just call mbox_request_channel() irrespective of being populated by DT or ACPI? Please note, in my implementation the client just need to see if it can populate the mbox_client.dev to be able to work as such on both DT backed and ACPI backed controller driver. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ashwin, On 07/11/14 14:52, Ashwin Chaugule wrote: > 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. > > Reviewed-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> > --- > drivers/mailbox/Kconfig | 12 ++ > drivers/mailbox/Makefile | 2 + > drivers/mailbox/mailbox.c | 4 +- > drivers/mailbox/mailbox.h | 16 ++ > drivers/mailbox/pcc.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 398 insertions(+), 3 deletions(-) > create mode 100644 drivers/mailbox/mailbox.h > create mode 100644 drivers/mailbox/pcc.c > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index 9fd9c67..c04fed9 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -33,4 +33,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 Driver" > + depends on ACPI I am bit confused here, though I prefer PCC to depend on ACPI, I have seen discussion in this thread about using PCC without ACPI, how will that work ? > + 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/pcc.c b/drivers/mailbox/pcc.c > new file mode 100644 > index 0000000..49074cd0 > --- /dev/null > +++ b/drivers/mailbox/pcc.c > @@ -0,0 +1,367 @@ > +/* > + * 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. > + * > + * PCC (Platform Communication Channel) is defined in the ACPI 5.0+ > + * specification. It is a mailbox like mechanism to allow clients > + * such as CPPC (Collaborative Processor Performance Control), RAS > + * (Reliability, Availability and Serviceability) and MPST (Memory > + * Node Power State Table) to talk to the platform (e.g. BMC) through > + * shared memory regions as defined in the PCC table entries. The PCC > + * specification supports a Doorbell mechanism for the PCC clients > + * to notify the platform about new data. This Doorbell information > + * is also specified in each PCC table entry. See pcc_send_data() > + * and pcc_tx_done() for basic mode of operation. > + * > + * For more details about PCC, please see the ACPI specification from > + * http://www.uefi.org/ACPIv5.1 Section 14. > + * > + * This file implements PCC as a Mailbox controller and allows for PCC > + * clients to be implemented as its Mailbox Client Channels. > + */ > + > +#include <linux/acpi.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/init.h> > +#include <linux/platform_device.h> > +#include <linux/mailbox_controller.h> > +#include <linux/mailbox_client.h> > + > +#include "mailbox.h" > + > +#define MAX_PCC_SUBSPACES 256 > +#define PCCS_SS_SIG_MAGIC 0x50434300 > +#define PCC_CMD_COMPLETE 0x1 > + > +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ? > +static struct mbox_controller pcc_mbox_ctrl = {}; > + [...] > + > +/** > + * pcc_tx_done - Callback from Mailbox controller code to > + * check if PCC message transmission completed. > + * @chan: Pointer to Mailbox channel on which previous > + * transmission occurred. > + * > + * Return: TRUE if succeeded. > + */ > +static bool pcc_tx_done(struct mbox_chan *chan) > +{ > + struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv; > + struct acpi_pcct_shared_memory *generic_comm_base = > + (struct acpi_pcct_shared_memory *) pcct_ss->base_address; IIUC, the PCCT table has the physical base Address of the shared memory range in the PCC subspace structures. Can you use that directly here ? Or are you mapping that memory elsewhere and updating the mapped address to the table ? > + u16 cmd_delay = pcct_ss->min_turnaround_time; > + unsigned int retries = 0; > + > + /* Try a few times while waiting for platform to consume */ > + while (!(readw_relaxed(&generic_comm_base->status) This will explode if the pcct_ss->base_address is not mapped. > + & PCC_CMD_COMPLETE)) { > + > + if (retries++ < 5) > + udelay(cmd_delay); > + else { > + /* > + * If the remote is dead, this will cause the Mbox > + * controller to timeout after mbox client.tx_tout > + * msecs. > + */ > + pr_err("PCC platform did not respond.\n"); > + return false; > + } > + } > + return true; > +} In general you can isolate all the access to the shared memory in the protocol layer. In that case you might have to use mbox_client_txdone instead of this pcc_tx_done. > + > +/** > + * get_subspace_id - Given a Mailbox channel, find out the > + * PCC subspace id. > + * @chan: Pointer to Mailbox Channel from which we want > + * the index. > + * Return: Errno if not found, else positive index number. > + */ > +static int get_subspace_id(struct mbox_chan *chan) > +{ > + int id = chan - pcc_mbox_chan; > + > + if (id < 0 || id > pcc_mbox_ctrl.num_chans) > + return -ENOENT; > + > + return id; > +} > + > +/** > + * pcc_send_data - Called from Mailbox Controller code to finally > + * transmit data over channel. > + * @chan: Pointer to Mailbox channel over which to send data. > + * @data: Actual data to be written over channel. > + * > + * Return: Err if something failed else 0 for success. > + */ > +static int pcc_send_data(struct mbox_chan *chan, void *data) > +{ > + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; > + u16 ss_idx = -1; > + > + ss_idx = get_subspace_id(chan); > + > + if (ss_idx < 0) { > + pr_err("Invalid Subspace ID from PCC client\n"); > + return -EINVAL; > + } > + > + doorbell = pcct_ss->doorbell_register; > + doorbell_preserve = pcct_ss->preserve_mask; > + doorbell_write = pcct_ss->write_mask; > + > + /* Write to the shared comm region. */ > + writew(cmd, &generic_comm_base->command); > + > + /* Write Subspace MAGIC value so platform can identify destination. */ > + writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); > + > + /* Flip CMD COMPLETE bit */ > + writew(0, &generic_comm_base->status); > + IMO it's not clean to modify only first 3 elements namely: Signature, Command and Status while the Communication Space is updated elsewhere. It's better to have all of them in one place if possible as mentioned above. > + /* Sync notification from OSPM to Platform. */ > + acpi_read(&doorbell_val, &doorbell); > + acpi_write((doorbell_val & doorbell_preserve) | doorbell_write, > + &doorbell); > + Only the above 2 must be part of the controller driver, the remaining as I mentioned can be move to the protocol/client layer, thoughts ? > + return 0; > +} > + > +static struct mbox_chan_ops pcc_chan_ops = { > + .send_data = pcc_send_data, > + .last_tx_done = pcc_tx_done, > +}; > + [...] > +/** > + * acpi_pcc_probe - Parse the ACPI tree for the PCCT. > + * > + * Return: 0 for Success, else errno. > + */ > +static int __init acpi_pcc_probe(void) > +{ > + acpi_status status = AE_OK; > + acpi_size pcct_tbl_header_size; > + int count; > + struct acpi_table_pcct *pcct_tbl; > + > + /* 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_FAILURE(status) || !pcct_tbl) { > + pr_warn("PCCT header not found.\n"); > + return -ENODEV; > + } > + > + count = acpi_table_parse_entries(ACPI_SIG_PCCT, > + sizeof(struct acpi_table_pcct), s/struct acpi_table_pcct/*pcct_tbl/ In general, the interrupt part of the PCCT SS is not considered in this patch. It's better to see/discuss how that can be fit into the mailbox framework, though it could be follow up patch. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 11 November 2014 01:43, Mark Brown <broonie@kernel.org> wrote: >> On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote: >>> On 10 November 2014 20:17, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >>> > On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> >> On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> >>> > What is there to stop two users from coming up with the same signature >>> > (0xdeadbeef / "string") for their mbox controllers? Things can break >>> > at run time, because with your patch, the first mbox controller with >>> > the duplicate signature/name will return a channel. The client may be >>> > expecting a channel from the "other" mbox. >> >>> Two channels with same signature are supposed to be _identical_ i.e, >>> either channel could serve any client asking for a channel with that >>> signature. So even if an "unexpected" instance of the channel is >>> assigned, the client should still be happy. >> >>> If a client differentiates between 2 instances of a channel, that's >>> probably a sign of bad design. The knowledge behind client's >>> preference of instance should actually lie on the provider(controller) >>> side. I am open to some example on the contrary. >> >> The problem here is that ACPI isn't defining the context in a way which >> maps well onto the way Linux looks things up. We may not like that and >> think it's a bad design but the spec is a done deal here and we have to >> address reality. >> > I don't mean that by 'bad design'. > Ashwin asked what if a client wants the channel from, say, second > instance of PCC instead of first (both are same controllers because > they check for same signature). 2 channels from 2 "different" controllers. e.g. Non-PCC controllers, which don't have a "subspace" signature field like PCC does. > I can not imagine a good reason why would a client want that. If the > reason is like only the channel of first controller is actually > connected to remote (while that of second controller is nipped) ... I > say that kind of board specific knowledge belongs closer to controller > than the client. The second controller should have known that and not > even populated the channel. > > >> From an ACPI point of view the context is the fact >> that this is a PCC channel (there's a globally unique namespace for PCC >> channels) but Linux wants a struct device for the client to represent >> the context with a mapping table of some kind behind that to do the >> lookup. >> >> There's nothing in the ACPI description of the channel or controller to >> tie it to the client device, and there's nothing preventing some other >> mailbox mechanism that gets added to an ACPI system from reusing similar >> names (bear in mind that idiomatic naming for ACPI appears to be three >> or four character strings). If we have a PCC channel "FOO" and some new >> mailbox type which also defines "FOO" the controllers aren't really >> going to be able to tell them apart just on the string. >> > ACPI does specify a PCC specific signature (0x50434300) for its > channels. Any channel is identified by doing OR of that signature and > the index of channel/subspace in the array of 256. This is true only for PCC. Another class of controllers may not even have such a signature. The only thing you can be sure of is that they will have the 4 letter table identifier string e.g. PCCT. > > IF another class of controllers is given the same name "_PCC", the > ACPI will atleast assign it a different signature (if not ACPI, the > controller driver for Linux). So we need not even think about using > strings. > > >> We could fit the maping into Linux a bit by having a struct device >> representing the PCC controller that you use to do the lookup but at >> that point you may as well have a PCC specific request function that >> knows that device and does the lookup which is approximately what we >> have in Ashwin's patch. We could also require that the lookup be >> something like "PCC:FOO" and use a global_xlate() but it's not clear to >> me that this is making things clearer. >> > We already have a way to circumvent all of that. I don't just object > to Ashwin's patch, I posted a more generic and robust solution here > https://lkml.org/lkml/2014/11/9/75 Arnd, Mark, Rafael, Lv, please^n ACK or NAK one of these patches and help us move on. > > -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 07/11/14 14:52, Ashwin Chaugule wrote: >> +config PCC >> + bool "Platform Communication Channel Driver" >> + depends on ACPI > > > I am bit confused here, though I prefer PCC to depend on ACPI, I have > seen discussion in this thread about using PCC without ACPI, how will > that work ? Been discussed early on. DT users, if at all, will need to come up with their own DT bindings to make this work. >> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; > > > Really, do you want to allocate 256 structures of mbox_chan even on > systems with just 1 - 2 channels(typically that would be the case) ? > Spec says, max of 256. Easier to support it this way. >> + >> +/** >> + * pcc_tx_done - Callback from Mailbox controller code to >> + * check if PCC message transmission completed. >> + * @chan: Pointer to Mailbox channel on which previous >> + * transmission occurred. >> + * >> + * Return: TRUE if succeeded. >> + */ >> +static bool pcc_tx_done(struct mbox_chan *chan) >> +{ >> + struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv; >> + struct acpi_pcct_shared_memory *generic_comm_base = >> + (struct acpi_pcct_shared_memory *) pcct_ss->base_address; > > > IIUC, the PCCT table has the physical base Address of the shared memory > range in the PCC subspace structures. Can you use that directly here ? > Or are you mapping that memory elsewhere and updating the mapped address > to the table ? In the client side. Its really owned by the client. pcc_tx_done can never be called on a channel which doesn't have a mapped base address. > >> + u16 cmd_delay = pcct_ss->min_turnaround_time; >> + unsigned int retries = 0; >> + >> + /* Try a few times while waiting for platform to consume */ >> + while (!(readw_relaxed(&generic_comm_base->status) > > > This will explode if the pcct_ss->base_address is not mapped. It is. Client side. Called only _after_ client is done with its thing. > >> + & PCC_CMD_COMPLETE)) { >> + >> + if (retries++ < 5) >> + udelay(cmd_delay); >> + else { >> + /* >> + * If the remote is dead, this will cause the Mbox >> + * controller to timeout after mbox client.tx_tout >> + * msecs. >> + */ >> + pr_err("PCC platform did not respond.\n"); >> + return false; >> + } >> + } >> + return true; >> +} > > > In general you can isolate all the access to the shared memory in the > protocol layer. In that case you might have to use mbox_client_txdone > instead of this pcc_tx_done. I dont see the problem in keeping this way though. >> + >> +/** >> + * pcc_send_data - Called from Mailbox Controller code to finally >> + * transmit data over channel. >> + * @chan: Pointer to Mailbox channel over which to send data. >> + * @data: Actual data to be written over channel. >> + * >> + * Return: Err if something failed else 0 for success. >> + */ >> +static int pcc_send_data(struct mbox_chan *chan, void *data) >> +{ >> + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; >> + u16 ss_idx = -1; >> + >> + ss_idx = get_subspace_id(chan); >> + >> + if (ss_idx < 0) { >> + pr_err("Invalid Subspace ID from PCC client\n"); >> + return -EINVAL; >> + } >> + >> + doorbell = pcct_ss->doorbell_register; >> + doorbell_preserve = pcct_ss->preserve_mask; >> + doorbell_write = pcct_ss->write_mask; >> + >> + /* Write to the shared comm region. */ >> + writew(cmd, &generic_comm_base->command); >> + >> + /* Write Subspace MAGIC value so platform can identify >> destination. */ >> + writel((PCCS_SS_SIG_MAGIC | ss_idx), >> &generic_comm_base->signature); >> + >> + /* Flip CMD COMPLETE bit */ >> + writew(0, &generic_comm_base->status); >> + > > > IMO it's not clean to modify only first 3 elements namely: Signature, > Command and Status while the Communication Space is updated elsewhere. > It's better to have all of them in one place if possible as mentioned above. The access sequence for first 3 elements is the same for each channel. Thats all the controller is expected to do on behalf of the client. So this function reduces code duplication in all client sites. > >> + /* Sync notification from OSPM to Platform. */ >> + acpi_read(&doorbell_val, &doorbell); >> + acpi_write((doorbell_val & doorbell_preserve) | doorbell_write, >> + &doorbell); >> + > > > Only the above 2 must be part of the controller driver, the remaining as > I mentioned can be move to the protocol/client layer, thoughts ? I'd really prefer for it to be separate. Once client is done writing, it calls this and lets the controller driver take over the rest. Its not racy. >> + >> + count = acpi_table_parse_entries(ACPI_SIG_PCCT, >> + sizeof(struct acpi_table_pcct), > > > s/struct acpi_table_pcct/*pcct_tbl/ > > In general, the interrupt part of the PCCT SS is not considered in this > patch. It's better to see/discuss how that can be fit into the mailbox > framework, though it could be follow up patch. The IRQ part of the spec seems to be under discussion (single irq per subspace / common IRQ across all) and as you may be aware we're working on trying it out on Juno. That'll guide the design. What I have here is good enough to start off with and has been tested. I dont think we should have a problem using the mailbox API for asyn tx though, but I'd really^n prefer if we get something out there first. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > >> I don't mean that by 'bad design'. >> Ashwin asked what if a client wants the channel from, say, second >> instance of PCC instead of first (both are same controllers because >> they check for same signature). > > 2 channels from 2 "different" controllers. e.g. Non-PCC controllers, > which don't have a "subspace" signature field like PCC does. > OK, even that I think I explained already, but here it goes again ... For Mailbox framework, there already has be some direct understanding between the client and controller driver. For ex, the controller defines the format of the payload packet and the client simply provides the pointer, to payload in SharedMemory, as "data" to mailbox api to be transmitted. This packet structure will usually be defined in a header common to client and controller driver. [I almost feel embarrassed to repeat this common rant] Just like payload format, the 'signature' will also be declared by controller and used by clients. For PCC controller, we conveniently use the signature (0x50434300 | Type) defined by the ACPI (we could chose some other coding as well). Another controller of a different class will choose a different signature, say (0xdead0000 | 16bitsCode).... much like we take care while defining a new DT binding that the 'compatible' string doesn't collide with that of another device. Since we don't have DT bindings to map clients onto controller:channel, the client will have to convey the controller:channel information itself... which will be 0x50434301 for HW-Reduced-Comm(type-1) channel of PCC(0x50434300). So the responsibility lies with the client to specify correct signature while requesting a channel. There is nothing to stop a client from specifying a wrong signature, just like there is nothing to stop it from executing BUG() In even simpler terms.... I prefer controller specific encoding(0x50434300) instead of controller specific api (pcc_mbox_request_channel). For a different class of controller, it is much cleaner to define a new encoding as compared to another xyz_mbox_request_channel() api. >> >>> From an ACPI point of view the context is the fact >>> that this is a PCC channel (there's a globally unique namespace for PCC >>> channels) but Linux wants a struct device for the client to represent >>> the context with a mapping table of some kind behind that to do the >>> lookup. >>> >>> There's nothing in the ACPI description of the channel or controller to >>> tie it to the client device, and there's nothing preventing some other >>> mailbox mechanism that gets added to an ACPI system from reusing similar >>> names (bear in mind that idiomatic naming for ACPI appears to be three >>> or four character strings). If we have a PCC channel "FOO" and some new >>> mailbox type which also defines "FOO" the controllers aren't really >>> going to be able to tell them apart just on the string. >>> >> ACPI does specify a PCC specific signature (0x50434300) for its >> channels. Any channel is identified by doing OR of that signature and >> the index of channel/subspace in the array of 256. > > This is true only for PCC. Another class of controllers may not even > have such a signature. The only thing you can be sure of is that they > will have the 4 letter table identifier string e.g. PCCT. > As I said, the PCC controller driver chooses to use ACPI defined signature. Likewise a different class controller may take some value from its specification or simply use random unique value... whatever suits it. -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ashwin, On 11/11/14 13:18, Ashwin Chaugule wrote: > On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 07/11/14 14:52, Ashwin Chaugule wrote: > >>> +config PCC >>> + bool "Platform Communication Channel Driver" >>> + depends on ACPI >> >> >> I am bit confused here, though I prefer PCC to depend on ACPI, I have >> seen discussion in this thread about using PCC without ACPI, how will >> that work ? > > Been discussed early on. DT users, if at all, will need to come up > with their own DT bindings to make this work. > > Thanks understood, I had not followed this thread earlier closely, so asked to get clarified. >>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; >> >> >> Really, do you want to allocate 256 structures of mbox_chan even on >> systems with just 1 - 2 channels(typically that would be the case) ? >> > > Spec says, max of 256. Easier to support it this way. > I tend to disagree, you can allocate dynamically. And use exactly same logic you use in get_subspace_id to populate con_priv later. When you parse PCC Subspace, you can just validate header and not record them in pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries and then do the required allocation. >>> + >>> +/** >>> + * pcc_tx_done - Callback from Mailbox controller code to >>> + * check if PCC message transmission completed. >>> + * @chan: Pointer to Mailbox channel on which previous >>> + * transmission occurred. >>> + * >>> + * Return: TRUE if succeeded. >>> + */ >>> +static bool pcc_tx_done(struct mbox_chan *chan) >>> +{ >>> + struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv; >>> + struct acpi_pcct_shared_memory *generic_comm_base = >>> + (struct acpi_pcct_shared_memory *) pcct_ss->base_address; >> >> >> IIUC, the PCCT table has the physical base Address of the shared memory >> range in the PCC subspace structures. Can you use that directly here ? >> Or are you mapping that memory elsewhere and updating the mapped address >> to the table ? > > In the client side. Its really owned by the client. pcc_tx_done can > never be called on a channel which doesn't have a mapped base address. > Yes but I don't like the way client has to read pcct_ss, read the physical address, map it and replace it in the table before it's used here. That could be issue especially for async notification when the client is not ready. If all the shared memory access happens in one place where it get's mapped, we can avoid such issues. >> >>> + u16 cmd_delay = pcct_ss->min_turnaround_time; I missed this last time, you should not use this here. As per the ACPI spec, it is minimum amount of time that OSPM must wait after the completion of a command before issuing the next command. So you need this at the end of the function. IIUC you need to use nominal latency instead here. >>> + unsigned int retries = 0; >>> + >>> + /* Try a few times while waiting for platform to consume */ >>> + while (!(readw_relaxed(&generic_comm_base->status) >> >> >> This will explode if the pcct_ss->base_address is not mapped. > > It is. Client side. Called only _after_ client is done with its thing. > Yes for now, once you add interrupt handling that may not be true as I mentioned earlier. >> >>> + & PCC_CMD_COMPLETE)) { >>> + >>> + if (retries++ < 5) >>> + udelay(cmd_delay); >>> + else { >>> + /* >>> + * If the remote is dead, this will cause the Mbox >>> + * controller to timeout after mbox client.tx_tout >>> + * msecs. >>> + */ >>> + pr_err("PCC platform did not respond.\n"); >>> + return false; >>> + } >>> + } >>> + return true; >>> +} >> >> >> In general you can isolate all the access to the shared memory in the >> protocol layer. In that case you might have to use mbox_client_txdone >> instead of this pcc_tx_done. > > I dont see the problem in keeping this way though. > For the reasons mentioned above, I prefer isolating the access to shared memory region from the control part(this driver). >>> + >>> +/** >>> + * pcc_send_data - Called from Mailbox Controller code to finally >>> + * transmit data over channel. >>> + * @chan: Pointer to Mailbox channel over which to send data. >>> + * @data: Actual data to be written over channel. >>> + * >>> + * Return: Err if something failed else 0 for success. >>> + */ >>> +static int pcc_send_data(struct mbox_chan *chan, void *data) >>> +{ >>> + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; >>> + u16 ss_idx = -1; >>> + >>> + ss_idx = get_subspace_id(chan); >>> + >>> + if (ss_idx < 0) { >>> + pr_err("Invalid Subspace ID from PCC client\n"); >>> + return -EINVAL; >>> + } >>> + >>> + doorbell = pcct_ss->doorbell_register; >>> + doorbell_preserve = pcct_ss->preserve_mask; >>> + doorbell_write = pcct_ss->write_mask; >>> + >>> + /* Write to the shared comm region. */ >>> + writew(cmd, &generic_comm_base->command); >>> + >>> + /* Write Subspace MAGIC value so platform can identify >>> destination. */ >>> + writel((PCCS_SS_SIG_MAGIC | ss_idx), >>> &generic_comm_base->signature); >>> + >>> + /* Flip CMD COMPLETE bit */ >>> + writew(0, &generic_comm_base->status); >>> + >> >> >> IMO it's not clean to modify only first 3 elements namely: Signature, >> Command and Status while the Communication Space is updated elsewhere. >> It's better to have all of them in one place if possible as mentioned above. > > The access sequence for first 3 elements is the same for each channel. > Thats all the controller is expected to do on behalf of the client. So > this function reduces code duplication in all client sites. > You can have a helper or a macro to avoid duplication. >> >>> + /* Sync notification from OSPM to Platform. */ >>> + acpi_read(&doorbell_val, &doorbell); >>> + acpi_write((doorbell_val & doorbell_preserve) | doorbell_write, >>> + &doorbell); >>> + >> >> >> Only the above 2 must be part of the controller driver, the remaining as >> I mentioned can be move to the protocol/client layer, thoughts ? > > I'd really prefer for it to be separate. Once client is done writing, > it calls this and lets the controller driver take over the rest. Its > not racy. > I think otherwise, so would like to see others' opinion on this :) >>> + >>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT, >>> + sizeof(struct acpi_table_pcct), >> >> >> s/struct acpi_table_pcct/*pcct_tbl/ >> >> In general, the interrupt part of the PCCT SS is not considered in this >> patch. It's better to see/discuss how that can be fit into the mailbox >> framework, though it could be follow up patch. > > The IRQ part of the spec seems to be under discussion (single irq per > subspace / common IRQ across all) and as you may be aware we're > working on trying it out on Juno. That'll guide the design. What I > have here is good enough to start off with and has been tested. I dont > think we should have a problem using the mailbox API for asyn tx > though, but I'd really^n prefer if we get something out there first. > That's the different and still under discussion. But you need to support Type 1 subspace as it stands in ACPI v5.1 Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, > + int index) > +{ > + struct device *dev = pcc_mbox_ctrl.dev; > + struct mbox_chan *chan; > + unsigned long flags; > + > + /* > + * Each PCC Subspace is a Mailbox Channel. > + * The PCC Clients get their PCC Subspace ID > + * from their own tables and pass it here. > + * This returns a pointer to the PCC subspace > + * for the Client to operate on. > + */ > + chan = &pcc_mbox_chan[index]; > + You are mapping 'Type' of a client directly onto SubSpace-ID of the channel allocated. If two clients need an instance of, say, Generic-Communications-Channel i.e, Type-0, this will not work. If no two clients can ask the same channel, MAX_PCC_SUBSPACES should be limited to max 'Type' value defined in ACPI specs (just 2 in ACPI-5.1). > + if (!chan || chan->cl) { > + dev_err(dev, "%s: PCC mailbox not free\n", __func__); > + return ERR_PTR(-EBUSY); > + } > + > + spin_lock_irqsave(&chan->lock, flags); > + chan->msg_free = 0; > + chan->msg_count = 0; > + chan->active_req = NULL; > + chan->cl = cl; > + init_completion(&chan->tx_complete); > + > + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) > + chan->txdone_method |= TXDONE_BY_ACK; > + > + spin_unlock_irqrestore(&chan->lock, flags); > + > + return chan; > +} > +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); > Nothing PCC specific here. All you have done is define a new api just to pick an element from the array. You simply needed the mbox_request_channel() let you through to controller. > +void pcc_mbox_free_channel(struct mbox_chan *chan) > +{ > + unsigned long flags; > + > + if (!chan || !chan->cl) > + return; > + > + 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; > + > + spin_unlock_irqrestore(&chan->lock, flags); > +} > +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); > + Ditto, nothing PCC in this new PCC specific api. > +/** > + * pcc_send_data - Called from Mailbox Controller code to finally > + * transmit data over channel. > + * @chan: Pointer to Mailbox channel over which to send data. > + * @data: Actual data to be written over channel. > + * > + * Return: Err if something failed else 0 for success. > + */ > +static int pcc_send_data(struct mbox_chan *chan, void *data) > +{ > + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; > + u16 ss_idx = -1; > + > + ss_idx = get_subspace_id(chan); > + > + if (ss_idx < 0) { > + pr_err("Invalid Subspace ID from PCC client\n"); > + return -EINVAL; > + } > + > + doorbell = pcct_ss->doorbell_register; > + doorbell_preserve = pcct_ss->preserve_mask; > + doorbell_write = pcct_ss->write_mask; > + > + /* Write to the shared comm region. */ > + writew(cmd, &generic_comm_base->command); > + > + /* Write Subspace MAGIC value so platform can identify destination. */ > + writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); > + > + /* Flip CMD COMPLETE bit */ > + writew(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); > + > + return 0; > +} > + > +static struct mbox_chan_ops pcc_chan_ops = { > + .send_data = pcc_send_data, > + .last_tx_done = pcc_tx_done, > +}; > + > +/** > + * parse_pcc_subspace - Parse the PCC table and extract PCC subspace > + * entries. There should be one entry per PCC client. > + * @header: Pointer to the ACPI subtable header under the PCCT. > + * @end: End of subtable entry. > + * > + * Return: 0 for Success, else errno. > + * > + * This gets called for each entry in the PCC table. > + */ > +static int parse_pcc_subspace(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_pcct_hw_reduced *pcct_ss; > + > + if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) { > + pcct_ss = (struct acpi_pcct_hw_reduced *) header; > + > + if (pcct_ss->header.type != > + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) { > + pr_err("Incorrect PCC Subspace type detected\n"); > + return -EINVAL; > + } > + The driver supports only ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE client ?! -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 10:12, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > >> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >> + int index) >> +{ >> + struct device *dev = pcc_mbox_ctrl.dev; >> + struct mbox_chan *chan; >> + unsigned long flags; >> + >> + /* >> + * Each PCC Subspace is a Mailbox Channel. >> + * The PCC Clients get their PCC Subspace ID >> + * from their own tables and pass it here. >> + * This returns a pointer to the PCC subspace >> + * for the Client to operate on. >> + */ >> + chan = &pcc_mbox_chan[index]; >> + > You are mapping 'Type' of a client directly onto SubSpace-ID of the > channel allocated. > If two clients need an instance of, say, > Generic-Communications-Channel i.e, Type-0, this will not work. > If no two clients can ask the same channel, MAX_PCC_SUBSPACES should > be limited to max 'Type' value defined in ACPI specs (just 2 in > ACPI-5.1). Intentional. Type 0 existed for X86 systems and nobody ever used it so far in Linux. Upcoming systems will use ACPI v5.1 defined Type 1. We want to get something out there and will start with Type 1 as the default. As a follow patchset, we will add Type 2, when its ready and Type 0 if at all it is required. Rafael/Lv and Mark have not objected to this. > The driver supports only ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE client ?! > See above. > -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 20:49, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 11 November 2014 10:12, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> >>> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, >>> + int index) >>> +{ >>> + struct device *dev = pcc_mbox_ctrl.dev; >>> + struct mbox_chan *chan; >>> + unsigned long flags; >>> + >>> + /* >>> + * Each PCC Subspace is a Mailbox Channel. >>> + * The PCC Clients get their PCC Subspace ID >>> + * from their own tables and pass it here. >>> + * This returns a pointer to the PCC subspace >>> + * for the Client to operate on. >>> + */ >>> + chan = &pcc_mbox_chan[index]; >>> + >> You are mapping 'Type' of a client directly onto SubSpace-ID of the >> channel allocated. >> If two clients need an instance of, say, >> Generic-Communications-Channel i.e, Type-0, this will not work. >> If no two clients can ask the same channel, MAX_PCC_SUBSPACES should >> be limited to max 'Type' value defined in ACPI specs (just 2 in >> ACPI-5.1). > > Intentional. Type 0 existed for X86 systems and nobody ever used it so > far in Linux. Upcoming systems will use ACPI v5.1 defined Type 1. We > want to get something out there and will start with Type 1 as the > default. As a follow patchset, we will add Type 2, when its ready and > Type 0 if at all it is required. Rafael/Lv and Mark have not objected > to this. > Well, I do. People might be busy to have overlooked it. >> The driver supports only ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE client ?! >> > > See above. > The code seems incomplete and broken at many levels. Not to forget the API messup. -jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: > On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > In even simpler terms.... I prefer controller specific > encoding(0x50434300) instead of controller specific api > (pcc_mbox_request_channel). For a different class of controller, it > is much cleaner to define a new encoding as compared to another > xyz_mbox_request_channel() api. The problem with this approach is that it still leaves the interface as controller specific, because the client now has to know that it must pass the PCC identifier instead of an index. A client driver using DT would just specify to use the first channel that is defined locally in the "mboxes" properties, so it can ask for chan = mbox_request_channel(client, 0); to get the first channel defined for this client while a driver using ACPI has to look up the identifier in its own local tables: u32 channel_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id); which is not the same interface as the first, even if you assume the channel_id to be globally unique in the system. Since we will need clients to have a consistent interface across DT and ACPI, I think the difference in firmware interfaces is better abstracted in a PCC specific interface. The client driver would still need to parse two different firmware structures, but it would at least have a consistent interface: u32 channel_id = of_property_read_u32(dev->of_node, "pccid"); if (!channel_id) channel_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id); Alternatively, the driver could use the regular API on DT: chan = mbox_request_channel(cl, 0); if (!chan) { u32 chan_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id); } For any firmware that we have control over, the mbox_request_channel() case could even be made to work in ACPI, if we decide to add the properties we use in DT with the new _DSD method. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 11, 2014 at 10:30:32AM +0000, Sudeep Holla wrote: > On 07/11/14 14:52, Ashwin Chaugule wrote: > >+config PCC > >+ bool "Platform Communication Channel Driver" > >+ depends on ACPI > I am bit confused here, though I prefer PCC to depend on ACPI, I have > seen discussion in this thread about using PCC without ACPI, how will > that work ? We can always remove the dependency when someone adds the code to talk to non-ACPI PCC devices.
On 11 November 2014 22:03, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: >> On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> >> In even simpler terms.... I prefer controller specific >> encoding(0x50434300) instead of controller specific api >> (pcc_mbox_request_channel). For a different class of controller, it >> is much cleaner to define a new encoding as compared to another >> xyz_mbox_request_channel() api. > > The problem with this approach is that it still leaves the interface > as controller specific, because the client now has to know that it > must pass the PCC identifier instead of an index. > Yup. I hope you are aware that the "index" argument of pcc_mbox_request_channel() is just the same thing. The "index" there is actually the 'Type' value defined in ACPI for the client. > A client driver using DT would just specify to use the first channel > that is defined locally in the "mboxes" properties, so it can ask for > > chan = mbox_request_channel(client, 0); > > to get the first channel defined for this client while a driver using > ACPI has to look up the identifier in its own local tables: > > u32 channel_id = this_driver_parse_acpi_and_find_pccid(dev); > chan = mbox_request_channel(client, channel_id); > > which is not the same interface as the first, even if you assume the > channel_id to be globally unique in the system. > Yes. As I said, same for pcc_mbox_request_channel(). But since PCC clients are limited and specified by ACPI (Type value won't change across platforms, I understand), the client drivers could have that value hardcoded. > Since we will need clients to have a consistent interface across > DT and ACPI, I think the difference in firmware interfaces is better > abstracted in a PCC specific interface. > > The client driver would still need to parse two different firmware > structures, but it would at least have a consistent interface: > > u32 channel_id = of_property_read_u32(dev->of_node, "pccid"); > if (!channel_id) > channel_id = this_driver_parse_acpi_and_find_pccid(dev); > chan = mbox_request_channel(client, channel_id); > > Alternatively, the driver could use the regular API on DT: > > chan = mbox_request_channel(cl, 0); > if (!chan) { > u32 chan_id = this_driver_parse_acpi_and_find_pccid(dev); > chan = mbox_request_channel(client, channel_id); > } > OK, but if the 'Type' value, that a client passes to (pcc_)mbox_req_chan(), does not change across platforms, would we need that? Thanks, -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 11, 2014 at 05:33:28PM +0100, Arnd Bergmann wrote: > On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: > > In even simpler terms.... I prefer controller specific > > encoding(0x50434300) instead of controller specific api > > (pcc_mbox_request_channel). For a different class of controller, it > > is much cleaner to define a new encoding as compared to another > > xyz_mbox_request_channel() api. > The problem with this approach is that it still leaves the interface > as controller specific, because the client now has to know that it > must pass the PCC identifier instead of an index. Further to what Arnd is saying here what all these various patterns for looking up PCC mailboxes are saying to me is that whatever we're doing there's probably a place for a helper API that implements them; even if it's just a thin inline wrapper in a header somewhere it saves everyone having to get it right and means if we do get a DT binding or whatever they all magically end up with support.
On Tuesday 11 November 2014 23:09:16 Jassi Brar wrote: > On 11 November 2014 22:03, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: > >> On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > >> > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > >> > >> In even simpler terms.... I prefer controller specific > >> encoding(0x50434300) instead of controller specific api > >> (pcc_mbox_request_channel). For a different class of controller, it > >> is much cleaner to define a new encoding as compared to another > >> xyz_mbox_request_channel() api. > > > > The problem with this approach is that it still leaves the interface > > as controller specific, because the client now has to know that it > > must pass the PCC identifier instead of an index. > > > Yup. I hope you are aware that the "index" argument of > pcc_mbox_request_channel() is just the same thing. The "index" there > is actually the 'Type' value defined in ACPI for the client. The problem is that it's not an index relative to the client, but into an array of the mailbox provide. I only today noticed that both are called 'index' in the source code, which is highly confusing, and the pcc driver should name it 'subspaceid' or similar instead, to minimize the confusion. > > A client driver using DT would just specify to use the first channel > > that is defined locally in the "mboxes" properties, so it can ask for > > > > chan = mbox_request_channel(client, 0); > > > > to get the first channel defined for this client while a driver using > > ACPI has to look up the identifier in its own local tables: > > > > u32 channel_id = this_driver_parse_acpi_and_find_pccid(dev); > > chan = mbox_request_channel(client, channel_id); > > > > which is not the same interface as the first, even if you assume the > > channel_id to be globally unique in the system. > > > Yes. As I said, same for pcc_mbox_request_channel(). But since PCC > clients are limited and specified by ACPI (Type value won't change > across platforms, I understand), the client drivers could have that > value hardcoded. Not really: > > Since we will need clients to have a consistent interface across > > DT and ACPI, I think the difference in firmware interfaces is better > > abstracted in a PCC specific interface. > > > > The client driver would still need to parse two different firmware > > structures, but it would at least have a consistent interface: > > > > u32 channel_id = of_property_read_u32(dev->of_node, "pccid"); > > if (!channel_id) > > channel_id = this_driver_parse_acpi_and_find_pccid(dev); > > chan = mbox_request_channel(client, channel_id); > > > > Alternatively, the driver could use the regular API on DT: > > > > chan = mbox_request_channel(cl, 0); > > if (!chan) { > > u32 chan_id = this_driver_parse_acpi_and_find_pccid(dev); > > chan = mbox_request_channel(client, channel_id); > > } > > > OK, but if the 'Type' value, that a client passes to > (pcc_)mbox_req_chan(), does not change across platforms, would we need > that? As I understand the ACPI specification, we have multiple clients to the PCC, and each of them has its own client-specific hardcoded interface to /find/ the subspace ID from the ACPI tables, but the subspace ID itself has to be read from the table and is not constant. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 11/11/14 17:29, Mark Brown wrote: > On Tue, Nov 11, 2014 at 10:30:32AM +0000, Sudeep Holla wrote: >> On 07/11/14 14:52, Ashwin Chaugule wrote: > >>> +config PCC >>> + bool "Platform Communication Channel Driver" >>> + depends on ACPI > >> I am bit confused here, though I prefer PCC to depend on ACPI, I have >> seen discussion in this thread about using PCC without ACPI, how will >> that work ? > > We can always remove the dependency when someone adds the code to talk > to non-ACPI PCC devices. > Agreed, Ashwin clarified that, I had not followed previous versions closely and was speaking only in the context of this patch. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 23:24, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 11 November 2014 23:09:16 Jassi Brar wrote: >> On 11 November 2014 22:03, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: >> >> On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> >> > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: >> >> >> >> In even simpler terms.... I prefer controller specific >> >> encoding(0x50434300) instead of controller specific api >> >> (pcc_mbox_request_channel). For a different class of controller, it >> >> is much cleaner to define a new encoding as compared to another >> >> xyz_mbox_request_channel() api. >> > >> > The problem with this approach is that it still leaves the interface >> > as controller specific, because the client now has to know that it >> > must pass the PCC identifier instead of an index. >> > >> Yup. I hope you are aware that the "index" argument of >> pcc_mbox_request_channel() is just the same thing. The "index" there >> is actually the 'Type' value defined in ACPI for the client. > > The problem is that it's not an index relative to the client, but > into an array of the mailbox provide. I only today noticed that both > are called 'index' in the source code, which is highly confusing, > and the pcc driver should name it 'subspaceid' or similar instead, > to minimize the confusion. > OK. Ashwin, would you fix the name and resubmit. Or I do s/index/subspace_id/ before committing? Arnd, Could I please have your Reviewed/Acked-by? Thanks Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 14:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > Ashwin, would you fix the name and resubmit. Or I do > s/index/subspace_id/ before committing? I'll fix this and submit another patch. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: > On 11/11/14 13:18, Ashwin Chaugule wrote: > > On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> On 07/11/14 14:52, Ashwin Chaugule wrote: > >>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; > >> > >> > >> Really, do you want to allocate 256 structures of mbox_chan even on > >> systems with just 1 - 2 channels(typically that would be the case) ? > >> > > > > Spec says, max of 256. Easier to support it this way. > > > > I tend to disagree, you can allocate dynamically. And use exactly same > logic you use in get_subspace_id to populate con_priv later. When you > parse PCC Subspace, you can just validate header and not record them in > pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries > and then do the required allocation. Right, I think that would be best. > >>> + > >>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT, > >>> + sizeof(struct acpi_table_pcct), > >> > >> > >> s/struct acpi_table_pcct/*pcct_tbl/ > >> > >> In general, the interrupt part of the PCCT SS is not considered in this > >> patch. It's better to see/discuss how that can be fit into the mailbox > >> framework, though it could be follow up patch. > > > > The IRQ part of the spec seems to be under discussion (single irq per > > subspace / common IRQ across all) and as you may be aware we're > > working on trying it out on Juno. That'll guide the design. What I > > have here is good enough to start off with and has been tested. I dont > > think we should have a problem using the mailbox API for asyn tx > > though, but I'd really^n prefer if we get something out there first. > > > > That's the different and still under discussion. But you need to support > Type 1 subspace as it stands in ACPI v5.1 Why? We should only implement whatever is required to support existing hardware, not because something is in the spec. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 12 November 2014 00:38:42 Jassi Brar wrote: > On 11 November 2014 23:24, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 11 November 2014 23:09:16 Jassi Brar wrote: > >> On 11 November 2014 22:03, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote: > >> >> On 11 November 2014 18:32, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > >> >> > On 10 November 2014 23:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > >> >> > >> >> In even simpler terms.... I prefer controller specific > >> >> encoding(0x50434300) instead of controller specific api > >> >> (pcc_mbox_request_channel). For a different class of controller, it > >> >> is much cleaner to define a new encoding as compared to another > >> >> xyz_mbox_request_channel() api. > >> > > >> > The problem with this approach is that it still leaves the interface > >> > as controller specific, because the client now has to know that it > >> > must pass the PCC identifier instead of an index. > >> > > >> Yup. I hope you are aware that the "index" argument of > >> pcc_mbox_request_channel() is just the same thing. The "index" there > >> is actually the 'Type' value defined in ACPI for the client. > > > > The problem is that it's not an index relative to the client, but > > into an array of the mailbox provide. I only today noticed that both > > are called 'index' in the source code, which is highly confusing, > > and the pcc driver should name it 'subspaceid' or similar instead, > > to minimize the confusion. > > > OK. > > Ashwin, would you fix the name and resubmit. Or I do > s/index/subspace_id/ before committing? > Arnd, Could I please have your Reviewed/Acked-by? I'll wait for the new version to dynamically allocate the channels as suggested by Sudeep. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/11/14 20:25, Arnd Bergmann wrote: > On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >> On 11/11/14 13:18, Ashwin Chaugule wrote: >>> On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> On 07/11/14 14:52, Ashwin Chaugule wrote: >>>>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; >>>> >>>> >>>> Really, do you want to allocate 256 structures of mbox_chan even on >>>> systems with just 1 - 2 channels(typically that would be the case) ? >>>> >>> >>> Spec says, max of 256. Easier to support it this way. >>> >> >> I tend to disagree, you can allocate dynamically. And use exactly same >> logic you use in get_subspace_id to populate con_priv later. When you >> parse PCC Subspace, you can just validate header and not record them in >> pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries >> and then do the required allocation. > > Right, I think that would be best. > >>>>> + >>>>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT, >>>>> + sizeof(struct acpi_table_pcct), >>>> >>>> >>>> s/struct acpi_table_pcct/*pcct_tbl/ >>>> >>>> In general, the interrupt part of the PCCT SS is not considered in this >>>> patch. It's better to see/discuss how that can be fit into the mailbox >>>> framework, though it could be follow up patch. >>> >>> The IRQ part of the spec seems to be under discussion (single irq per >>> subspace / common IRQ across all) and as you may be aware we're >>> working on trying it out on Juno. That'll guide the design. What I >>> have here is good enough to start off with and has been tested. I dont >>> think we should have a problem using the mailbox API for asyn tx >>> though, but I'd really^n prefer if we get something out there first. >>> >> >> That's the different and still under discussion. But you need to support >> Type 1 subspace as it stands in ACPI v5.1 > > Why? We should only implement whatever is required to support existing hardware, > not because something is in the spec. Agreed. I assumed that this was tested on some hardware which adhere to Type 1 subspace of the spec and I asked to implement interrupt mode as it is always better compared to polling mode and current spec. has support for the interrupts. Also, the existing Type1 is not sufficient for the mailbox/PCC on Juno platform, hence the new proposal. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 November 2014 08:32, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 11/11/14 20:25, Arnd Bergmann wrote: >> >> On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >>> >>> On 11/11/14 13:18, Ashwin Chaugule wrote: >>>> >>>> On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>>> >>>>> On 07/11/14 14:52, Ashwin Chaugule wrote: >>>> >>>> The IRQ part of the spec seems to be under discussion (single irq per >>>> subspace / common IRQ across all) and as you may be aware we're >>>> working on trying it out on Juno. That'll guide the design. What I >>>> have here is good enough to start off with and has been tested. I dont >>>> think we should have a problem using the mailbox API for asyn tx >>>> though, but I'd really^n prefer if we get something out there first. >>>> >>> >>> That's the different and still under discussion. But you need to support >>> Type 1 subspace as it stands in ACPI v5.1 >> >> >> Why? We should only implement whatever is required to support existing >> hardware, >> not because something is in the spec. > > > Agreed. I assumed that this was tested on some hardware which adhere to > Type 1 subspace of the spec and I asked to implement interrupt mode as > it is always better compared to polling mode and current spec. has > support for the interrupts. I do not have hardware to test the IRQ parts as of now, so the plan was to at least get polling mode out there. That will enable a lot of folks and will be useful as a fallback mode later on as well. We can add IRQ mode stuff once it is tested. I dont see the point in supporting it all from the start. > > Also, the existing Type1 is not sufficient for the mailbox/PCC on Juno > platform, hence the new proposal. Just as Juno is proving that the IRQ mode in the current spec is insufficient, we may find similar issues with other hardware as it becomes available. It makes more sense to tackle this once we have some hardware to test this part of PCC properly. Just because its in the spec doesn't mean its right. ;) Btw, if you or anyone has any hardware to test it on, patches are more than welcome. :) Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/14 13:48, Ashwin Chaugule wrote: > On 12 November 2014 08:32, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 11/11/14 20:25, Arnd Bergmann wrote: >>> >>> On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >>>> >>>> On 11/11/14 13:18, Ashwin Chaugule wrote: >>>>> >>>>> On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>>>> >>>>>> On 07/11/14 14:52, Ashwin Chaugule wrote: >>>>> >>>>> The IRQ part of the spec seems to be under discussion (single irq per >>>>> subspace / common IRQ across all) and as you may be aware we're >>>>> working on trying it out on Juno. That'll guide the design. What I >>>>> have here is good enough to start off with and has been tested. I dont >>>>> think we should have a problem using the mailbox API for asyn tx >>>>> though, but I'd really^n prefer if we get something out there first. >>>>> >>>> >>>> That's the different and still under discussion. But you need to support >>>> Type 1 subspace as it stands in ACPI v5.1 >>> >>> >>> Why? We should only implement whatever is required to support existing >>> hardware, >>> not because something is in the spec. >> >> >> Agreed. I assumed that this was tested on some hardware which adhere to >> Type 1 subspace of the spec and I asked to implement interrupt mode as >> it is always better compared to polling mode and current spec. has >> support for the interrupts. > > I do not have hardware to test the IRQ parts as of now, so the plan > was to at least get polling mode out there. That will enable a lot of > folks and will be useful as a fallback mode later on as well. We can > add IRQ mode stuff once it is tested. I dont see the point in > supporting it all from the start. > Agreed, sorry I was not clear. I didn't mean to say you need to add it now. Since you mentioned about the ongoing proposal, I just wanted to make it clear that there might be hardware adhering to Type 1 and if they want to use interrupts we need to support it and that's different from the new proposal. >> >> Also, the existing Type1 is not sufficient for the mailbox/PCC on Juno >> platform, hence the new proposal. > > Just as Juno is proving that the IRQ mode in the current spec is > insufficient, we may find similar issues with other hardware as it > becomes available. It makes more sense to tackle this once we have > some hardware to test this part of PCC properly. Just because its in > the spec doesn't mean its right. ;) > Agreed. But I was just curious to know will any ACPI implementation use the polling mode for PCC and use Type 1 subspace as is ;). > Btw, if you or anyone has any hardware to test it on, patches are more > than welcome. :) > No, unfortunately I have only Juno and it doesn't fit this Type of PCC Subspace. Even if it fits, I need firmware/platform changes to understand PCC. So I am currently not in a position to test this. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 November 2014 15:25, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >> On 11/11/14 13:18, Ashwin Chaugule wrote: >> > On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> On 07/11/14 14:52, Ashwin Chaugule wrote: >> >>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; >> >> >> >> >> >> Really, do you want to allocate 256 structures of mbox_chan even on >> >> systems with just 1 - 2 channels(typically that would be the case) ? >> >> >> > >> > Spec says, max of 256. Easier to support it this way. >> > >> >> I tend to disagree, you can allocate dynamically. And use exactly same >> logic you use in get_subspace_id to populate con_priv later. When you >> parse PCC Subspace, you can just validate header and not record them in >> pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries >> and then do the required allocation. > > Right, I think that would be best. Discussed this with Sudeep on IRC. The parse_pcc_subspace() function actually is a useful place to create new mbox channel entries. We get a pointer to the pcct_ss (pcc subspace subtable) in it, for each entry as it is discovered in the PCCT. If we ignore that pointer, then we will need to re-traverse the PCCT in the probe() function, just to get those pointers again. In theory, we could re-traverse the PCCT in the probe function without using the API provided by the ACPI framework, but that sounds dicey. In the end we would be doing this only to maintain the clever-ish ptr math as it is in get_subspace_id(). Alternately, we could replace the array with a list. e.g. struct pcc_channel { struct list_head node; struct mbox_chan chan; }; Then use list_for_each_entry() to get the subspace id from a struct mbox_chan* , and list_for_each() + list_entry() to get the channel from a subspace index. These lists are not expected to get long in practice so traversing them for lookups is not really expensive and its only done at init. Does this make sense? Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 12, 2014 at 01:25:31PM -0500, Ashwin Chaugule wrote: > On 11 November 2014 15:25, Arnd Bergmann <arnd@arndb.de> wrote: > struct pcc_channel { > struct list_head node; > struct mbox_chan chan; > }; > Then use list_for_each_entry() to get the subspace id from a struct > mbox_chan* , and list_for_each() + list_entry() to get the channel > from a subspace index. These lists are not expected to get long in > practice so traversing them for lookups is not really expensive and > its only done at init. > Does this make sense? That's how we do it for regulators - even if the list does get a bit long it seems like it's unlikely to get so long that it's going to be a problem in init paths.
On 12 November 2014 23:55, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 11 November 2014 15:25, Arnd Bergmann <arnd@arndb.de> wrote: >> On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >>> On 11/11/14 13:18, Ashwin Chaugule wrote: >>> > On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >> On 07/11/14 14:52, Ashwin Chaugule wrote: >>> >>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; >>> >> >>> >> >>> >> Really, do you want to allocate 256 structures of mbox_chan even on >>> >> systems with just 1 - 2 channels(typically that would be the case) ? >>> >> >>> > >>> > Spec says, max of 256. Easier to support it this way. >>> > >>> >>> I tend to disagree, you can allocate dynamically. And use exactly same >>> logic you use in get_subspace_id to populate con_priv later. When you >>> parse PCC Subspace, you can just validate header and not record them in >>> pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries >>> and then do the required allocation. >> >> Right, I think that would be best. > > Discussed this with Sudeep on IRC. The parse_pcc_subspace() function > actually is a useful place to create new mbox channel entries. We get > a pointer to the pcct_ss (pcc subspace subtable) in it, for each entry > as it is discovered in the PCCT. If we ignore that pointer, then we > will need to re-traverse the PCCT in the probe() function, just to get > those pointers again. > > In theory, we could re-traverse the PCCT in the probe function without > using the API provided by the ACPI framework, but that sounds dicey. > In the end we would be doing this only to maintain the clever-ish ptr > math as it is in get_subspace_id(). Alternately, we could replace the > array with a list. > > e.g. > > struct pcc_channel { > struct list_head node; > struct mbox_chan chan; > }; > > Then use list_for_each_entry() to get the subspace id from a struct > mbox_chan* , and list_for_each() + list_entry() to get the channel > from a subspace index. These lists are not expected to get long in > practice so traversing them for lookups is not really expensive and > its only done at init. > > Does this make sense? > But what array of channels do you pass to mbox_controller_register()? -jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 November 2014 14:04, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 12 November 2014 23:55, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: >> On 11 November 2014 15:25, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote: >>>> On 11/11/14 13:18, Ashwin Chaugule wrote: >>>> > On 11 November 2014 05:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> >> On 07/11/14 14:52, Ashwin Chaugule wrote: >>>> >>> +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; >>>> >> >>>> >> >>>> >> Really, do you want to allocate 256 structures of mbox_chan even on >>>> >> systems with just 1 - 2 channels(typically that would be the case) ? >>>> >> >>>> > >>>> > Spec says, max of 256. Easier to support it this way. >>>> > >>>> >>>> I tend to disagree, you can allocate dynamically. And use exactly same >>>> logic you use in get_subspace_id to populate con_priv later. When you >>>> parse PCC Subspace, you can just validate header and not record them in >>>> pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries >>>> and then do the required allocation. >>> >>> Right, I think that would be best. >> >> Discussed this with Sudeep on IRC. The parse_pcc_subspace() function >> actually is a useful place to create new mbox channel entries. We get >> a pointer to the pcct_ss (pcc subspace subtable) in it, for each entry >> as it is discovered in the PCCT. If we ignore that pointer, then we >> will need to re-traverse the PCCT in the probe() function, just to get >> those pointers again. >> >> In theory, we could re-traverse the PCCT in the probe function without >> using the API provided by the ACPI framework, but that sounds dicey. >> In the end we would be doing this only to maintain the clever-ish ptr >> math as it is in get_subspace_id(). Alternately, we could replace the >> array with a list. >> >> e.g. >> >> struct pcc_channel { >> struct list_head node; >> struct mbox_chan chan; >> }; >> >> Then use list_for_each_entry() to get the subspace id from a struct >> mbox_chan* , and list_for_each() + list_entry() to get the channel >> from a subspace index. These lists are not expected to get long in >> practice so traversing them for lookups is not really expensive and >> its only done at init. >> >> Does this make sense? >> > But what array of channels do you pass to mbox_controller_register()? Yea. Just hit that problem. :) > > -jassi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 9fd9c67..c04fed9 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -33,4 +33,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 Driver" + 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 94ed7ce..dd412c2 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -5,3 +5,5 @@ obj-$(CONFIG_MAILBOX) += mailbox.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o + +obj-$(CONFIG_PCC) += pcc.o diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index afcb430..17e9e4a 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -21,9 +21,7 @@ #include <linux/mailbox_client.h> #include <linux/mailbox_controller.h> -#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ -#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ -#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ +#include "mailbox.h" static LIST_HEAD(mbox_cons); static DEFINE_MUTEX(con_mutex); diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h new file mode 100644 index 0000000..5a15a25 --- /dev/null +++ b/drivers/mailbox/mailbox.h @@ -0,0 +1,16 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __MAILBOX_H +#define __MAILBOX_H + +#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */ +#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */ +#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */ + +#endif /* __MAILBOX_H */ + + diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c new file mode 100644 index 0000000..49074cd0 --- /dev/null +++ b/drivers/mailbox/pcc.c @@ -0,0 +1,367 @@ +/* + * 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. + * + * PCC (Platform Communication Channel) is defined in the ACPI 5.0+ + * specification. It is a mailbox like mechanism to allow clients + * such as CPPC (Collaborative Processor Performance Control), RAS + * (Reliability, Availability and Serviceability) and MPST (Memory + * Node Power State Table) to talk to the platform (e.g. BMC) through + * shared memory regions as defined in the PCC table entries. The PCC + * specification supports a Doorbell mechanism for the PCC clients + * to notify the platform about new data. This Doorbell information + * is also specified in each PCC table entry. See pcc_send_data() + * and pcc_tx_done() for basic mode of operation. + * + * For more details about PCC, please see the ACPI specification from + * http://www.uefi.org/ACPIv5.1 Section 14. + * + * This file implements PCC as a Mailbox controller and allows for PCC + * clients to be implemented as its Mailbox Client Channels. + */ + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/mailbox_controller.h> +#include <linux/mailbox_client.h> + +#include "mailbox.h" + +#define MAX_PCC_SUBSPACES 256 +#define PCCS_SS_SIG_MAGIC 0x50434300 +#define PCC_CMD_COMPLETE 0x1 + +static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES]; +static struct mbox_controller pcc_mbox_ctrl = {}; + +/** + * pcc_mbox_request_channel - PCC clients call this function to + * request a pointer to their PCC subspace, from which they + * can get the details of communicating with the remote. + * @cl: Pointer to Mailbox client, so we know where to bind the + * Channel. + * @index: The PCC Subspace index as parsed in the PCC client + * ACPI package. This is used to lookup the array of PCC + * subspaces as parsed by the PCC Mailbox controller. + * + * Return: Pointer to the Mailbox Channel if successful or + * ERR_PTR. + */ +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, + int index) +{ + struct device *dev = pcc_mbox_ctrl.dev; + struct mbox_chan *chan; + unsigned long flags; + + /* + * Each PCC Subspace is a Mailbox Channel. + * The PCC Clients get their PCC Subspace ID + * from their own tables and pass it here. + * This returns a pointer to the PCC subspace + * for the Client to operate on. + */ + chan = &pcc_mbox_chan[index]; + + if (!chan || chan->cl) { + dev_err(dev, "%s: PCC mailbox not free\n", __func__); + return ERR_PTR(-EBUSY); + } + + spin_lock_irqsave(&chan->lock, flags); + chan->msg_free = 0; + chan->msg_count = 0; + chan->active_req = NULL; + chan->cl = cl; + init_completion(&chan->tx_complete); + + if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) + chan->txdone_method |= TXDONE_BY_ACK; + + spin_unlock_irqrestore(&chan->lock, flags); + + return chan; +} +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); + +/** + * pcc_mbox_free_channel - Clients call this to free their Channel. + * + * @chan: Pointer to the mailbox channel as returned by + * pcc_mbox_request_channel() + */ +void pcc_mbox_free_channel(struct mbox_chan *chan) +{ + unsigned long flags; + + if (!chan || !chan->cl) + return; + + 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; + + spin_unlock_irqrestore(&chan->lock, flags); +} +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); + +/** + * pcc_tx_done - Callback from Mailbox controller code to + * check if PCC message transmission completed. + * @chan: Pointer to Mailbox channel on which previous + * transmission occurred. + * + * Return: TRUE if succeeded. + */ +static bool pcc_tx_done(struct mbox_chan *chan) +{ + struct acpi_pcct_hw_reduced *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; + unsigned int retries = 0; + + /* Try a few times while waiting for platform to consume */ + while (!(readw_relaxed(&generic_comm_base->status) + & PCC_CMD_COMPLETE)) { + + if (retries++ < 5) + udelay(cmd_delay); + else { + /* + * If the remote is dead, this will cause the Mbox + * controller to timeout after mbox client.tx_tout + * msecs. + */ + pr_err("PCC platform did not respond.\n"); + return false; + } + } + return true; +} + +/** + * get_subspace_id - Given a Mailbox channel, find out the + * PCC subspace id. + * @chan: Pointer to Mailbox Channel from which we want + * the index. + * Return: Errno if not found, else positive index number. + */ +static int get_subspace_id(struct mbox_chan *chan) +{ + int id = chan - pcc_mbox_chan; + + if (id < 0 || id > pcc_mbox_ctrl.num_chans) + return -ENOENT; + + return id; +} + +/** + * pcc_send_data - Called from Mailbox Controller code to finally + * transmit data over channel. + * @chan: Pointer to Mailbox channel over which to send data. + * @data: Actual data to be written over channel. + * + * Return: Err if something failed else 0 for success. + */ +static int pcc_send_data(struct mbox_chan *chan, void *data) +{ + struct acpi_pcct_hw_reduced *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 = *(u16 *) data; + u16 ss_idx = -1; + + ss_idx = get_subspace_id(chan); + + if (ss_idx < 0) { + pr_err("Invalid Subspace ID from PCC client\n"); + return -EINVAL; + } + + doorbell = pcct_ss->doorbell_register; + doorbell_preserve = pcct_ss->preserve_mask; + doorbell_write = pcct_ss->write_mask; + + /* Write to the shared comm region. */ + writew(cmd, &generic_comm_base->command); + + /* Write Subspace MAGIC value so platform can identify destination. */ + writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature); + + /* Flip CMD COMPLETE bit */ + writew(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); + + return 0; +} + +static struct mbox_chan_ops pcc_chan_ops = { + .send_data = pcc_send_data, + .last_tx_done = pcc_tx_done, +}; + +/** + * parse_pcc_subspace - Parse the PCC table and extract PCC subspace + * entries. There should be one entry per PCC client. + * @header: Pointer to the ACPI subtable header under the PCCT. + * @end: End of subtable entry. + * + * Return: 0 for Success, else errno. + * + * This gets called for each entry in the PCC table. + */ +static int parse_pcc_subspace(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_pcct_hw_reduced *pcct_ss; + + if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) { + pcct_ss = (struct acpi_pcct_hw_reduced *) header; + + if (pcct_ss->header.type != + ACPI_PCCT_TYPE_HW_REDUCED_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; +} + +/** + * acpi_pcc_probe - Parse the ACPI tree for the PCCT. + * + * Return: 0 for Success, else errno. + */ +static int __init acpi_pcc_probe(void) +{ + acpi_status status = AE_OK; + acpi_size pcct_tbl_header_size; + int count; + struct acpi_table_pcct *pcct_tbl; + + /* 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_FAILURE(status) || !pcct_tbl) { + pr_warn("PCCT header not found.\n"); + return -ENODEV; + } + + count = acpi_table_parse_entries(ACPI_SIG_PCCT, + sizeof(struct acpi_table_pcct), + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE, + parse_pcc_subspace, MAX_PCC_SUBSPACES); + + if (count <= 0) { + pr_err("Error parsing PCC subspaces from PCCT\n"); + return -EINVAL; + } + + pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans); + + return 0; +} + +/** + * pcc_mbox_probe - Called when we find a match for the + * PCCT platform device. This is purely used to represent + * the PCCT as a virtual device for registering with the + * generic Mailbox framework. + * + * @pdev: Pointer to platform device returned when a match + * is found. + * + * Return: 0 for Success, else errno. + */ +static int pcc_mbox_probe(struct platform_device *pdev) +{ + int ret = 0; + + pcc_mbox_ctrl.chans = pcc_mbox_chan; + pcc_mbox_ctrl.ops = &pcc_chan_ops; + pcc_mbox_ctrl.txdone_poll = true; + pcc_mbox_ctrl.txpoll_period = 10; + pcc_mbox_ctrl.dev = &pdev->dev; + + 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: %d\n", ret); + ret = -ENODEV; + } + + return ret; +} + +struct platform_driver pcc_mbox_driver = { + .probe = pcc_mbox_probe, + .driver = { + .name = "PCCT", + .owner = THIS_MODULE, + }, +}; + +static int __init pcc_init(void) +{ + int ret; + struct platform_device *pcc_pdev; + + if (acpi_disabled) + return -ENODEV; + + /* Check if PCC support is available. */ + ret = acpi_pcc_probe(); + + if (ret) { + pr_err("ACPI PCC probe failed.\n"); + return -ENODEV; + } + + pcc_pdev = platform_create_bundle(&pcc_mbox_driver, + pcc_mbox_probe, NULL, 0, NULL, 0); + + if (!pcc_pdev) { + pr_err("Err creating PCC platform bundle\n"); + return -ENODEV; + } + + return 0; +} +device_initcall(pcc_init);