* [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
@ 2016-05-20 0:32 Hoan Tran
2016-05-31 16:32 ` Hoan Tran
2016-05-31 19:05 ` Ashwin Chaugule
0 siblings, 2 replies; 20+ messages in thread
From: Hoan Tran @ 2016-05-20 0:32 UTC (permalink / raw)
To: Jassi Brar, Ashwin Chaugule
Cc: Rafael J. Wysocki, Robert Moore, alexey.klimov, linux-kernel,
linux-acpi, lho, dhdang, Hoan Tran
ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
to acknowledge doorbell interrupt. This patch provides the implementation
for the Communication Subspace Type 2.
v3
* Remove 2 global structures
* Correct parsing subspace type 1 and subspace type 2
v2
* Remove changes inside "actbl3.h". This file is taken care by ACPICA.
* Parse both subspace type 1 and subspace type 2
* Remove unnecessary variable initialization
* ISR returns IRQ_NONE in case of error
v1
* Initial
Signed-off-by: Hoan Tran <hotran@apm.com>
---
drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
1 file changed, 245 insertions(+), 71 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 043828d..c98bd94 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -59,6 +59,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/list.h>
#include <linux/platform_device.h>
#include <linux/mailbox_controller.h>
@@ -68,11 +69,16 @@
#include "mailbox.h"
#define MAX_PCC_SUBSPACES 256
+#define MBOX_IRQ_NAME "pcc-mbox"
static struct mbox_chan *pcc_mbox_channels;
/* Array of cached virtual address for doorbell registers */
static void __iomem **pcc_doorbell_vaddr;
+/* Array of cached virtual address for doorbell ack registers */
+static void __iomem **pcc_doorbell_ack_vaddr;
+/* Array of doorbell interrupts */
+static int *pcc_doorbell_irq;
static struct mbox_controller pcc_mbox_ctrl = {};
/**
@@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
return &pcc_mbox_channels[id];
}
+/*
+ * PCC can be used with perf critical drivers such as CPPC
+ * So it makes sense to locally cache the virtual address and
+ * use it to read/write to PCC registers such as doorbell register
+ *
+ * The below read_register and write_registers are used to read and
+ * write from perf critical registers such as PCC doorbell register
+ */
+static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
+{
+ int ret_val = 0;
+
+ switch (bit_width) {
+ case 8:
+ *val = readb(vaddr);
+ break;
+ case 16:
+ *val = readw(vaddr);
+ break;
+ case 32:
+ *val = readl(vaddr);
+ break;
+ case 64:
+ *val = readq(vaddr);
+ break;
+ default:
+ pr_debug("Error: Cannot read register of %u bit width",
+ bit_width);
+ ret_val = -EFAULT;
+ break;
+ }
+ return ret_val;
+}
+
+static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
+{
+ int ret_val = 0;
+
+ switch (bit_width) {
+ case 8:
+ writeb(val, vaddr);
+ break;
+ case 16:
+ writew(val, vaddr);
+ break;
+ case 32:
+ writel(val, vaddr);
+ break;
+ case 64:
+ writeq(val, vaddr);
+ break;
+ default:
+ pr_debug("Error: Cannot write register of %u bit width",
+ bit_width);
+ ret_val = -EFAULT;
+ break;
+ }
+ return ret_val;
+}
+
+/**
+ * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
+ * @interrupt: GSI number.
+ * @flags: interrupt flags
+ *
+ * Returns: a valid linux IRQ number on success
+ * 0 or -EINVAL on failure
+ */
+static int pcc_map_interrupt(u32 interrupt, u32 flags)
+{
+ int trigger, polarity;
+
+ if (!interrupt)
+ return 0;
+
+ trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
+ : ACPI_LEVEL_SENSITIVE;
+
+ polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
+ : ACPI_ACTIVE_HIGH;
+
+ return acpi_register_gsi(NULL, interrupt, trigger, polarity);
+}
+
+/**
+ * pcc_mbox_irq - PCC mailbox interrupt handler
+ */
+static irqreturn_t pcc_mbox_irq(int irq, void *p)
+{
+ struct acpi_generic_address *doorbell_ack;
+ struct acpi_pcct_hw_reduced *pcct_ss;
+ struct mbox_chan *chan = p;
+ u64 doorbell_ack_preserve;
+ u64 doorbell_ack_write;
+ u64 doorbell_ack_val;
+ int ret;
+
+ pcct_ss = chan->con_priv;
+
+ mbox_chan_received_data(chan, NULL);
+
+ if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+ struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv;
+ u32 id = chan - pcc_mbox_channels;
+
+ doorbell_ack = &pcct2_ss->doorbell_ack_register;
+ doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
+ doorbell_ack_write = pcct2_ss->ack_write_mask;
+
+ ret = read_register(pcc_doorbell_ack_vaddr[id],
+ &doorbell_ack_val,
+ doorbell_ack->bit_width);
+ if (ret)
+ return IRQ_NONE;
+
+ ret = write_register(pcc_doorbell_ack_vaddr[id],
+ (doorbell_ack_val & doorbell_ack_preserve)
+ | doorbell_ack_write,
+ doorbell_ack->bit_width);
+ if (ret)
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
/**
* pcc_mbox_request_channel - PCC clients call this function to
* request a pointer to their PCC subspace, from which they
@@ -135,6 +267,18 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
chan->txdone_method |= TXDONE_BY_ACK;
+ if (pcc_doorbell_irq[subspace_id] > 0) {
+ int rc;
+
+ rc = devm_request_irq(dev, pcc_doorbell_irq[subspace_id],
+ pcc_mbox_irq, 0, MBOX_IRQ_NAME, chan);
+ if (unlikely(rc)) {
+ dev_err(dev, "failed to register PCC interrupt %d\n",
+ pcc_doorbell_irq[subspace_id]);
+ chan = ERR_PTR(rc);
+ }
+ }
+
spin_unlock_irqrestore(&chan->lock, flags);
return chan;
@@ -149,80 +293,30 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
*/
void pcc_mbox_free_channel(struct mbox_chan *chan)
{
+ u32 id = chan - pcc_mbox_channels;
unsigned long flags;
if (!chan || !chan->cl)
return;
+ if (id >= pcc_mbox_ctrl.num_chans) {
+ pr_debug("pcc_mbox_free_channel: Invalid mbox_chan passed\n");
+ 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;
+ if (pcc_doorbell_irq[id] > 0)
+ devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
+
spin_unlock_irqrestore(&chan->lock, flags);
}
EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
-/*
- * PCC can be used with perf critical drivers such as CPPC
- * So it makes sense to locally cache the virtual address and
- * use it to read/write to PCC registers such as doorbell register
- *
- * The below read_register and write_registers are used to read and
- * write from perf critical registers such as PCC doorbell register
- */
-static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
-{
- int ret_val = 0;
-
- switch (bit_width) {
- case 8:
- *val = readb(vaddr);
- break;
- case 16:
- *val = readw(vaddr);
- break;
- case 32:
- *val = readl(vaddr);
- break;
- case 64:
- *val = readq(vaddr);
- break;
- default:
- pr_debug("Error: Cannot read register of %u bit width",
- bit_width);
- ret_val = -EFAULT;
- break;
- }
- return ret_val;
-}
-
-static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
-{
- int ret_val = 0;
-
- switch (bit_width) {
- case 8:
- writeb(val, vaddr);
- break;
- case 16:
- writew(val, vaddr);
- break;
- case 32:
- writel(val, vaddr);
- break;
- case 64:
- writeq(val, vaddr);
- break;
- default:
- pr_debug("Error: Cannot write register of %u bit width",
- bit_width);
- ret_val = -EFAULT;
- break;
- }
- return ret_val;
-}
/**
* pcc_send_data - Called from Mailbox Controller code. Used
@@ -296,8 +390,10 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
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) {
+ if ((pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
+ && (pcct_ss->header.type !=
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
pr_err("Incorrect PCC Subspace type detected\n");
return -EINVAL;
}
@@ -307,6 +403,43 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
}
/**
+ * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
+ * There should be one entry per PCC client.
+ * @id: PCC subspace index.
+ * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int pcc_parse_subspace_irq(int id,
+ struct acpi_pcct_hw_reduced *pcct_ss)
+{
+ pcc_doorbell_irq[id] = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
+ (u32)pcct_ss->flags);
+ if (pcc_doorbell_irq[id] <= 0) {
+ pr_err("PCC GSI %d not registered\n",
+ pcct_ss->doorbell_interrupt);
+ return -EINVAL;
+ }
+
+ if (pcct_ss->header.type
+ == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+ struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
+
+ pcc_doorbell_ack_vaddr[id] = acpi_os_ioremap(
+ pcct2_ss->doorbell_ack_register.address,
+ pcct2_ss->doorbell_ack_register.bit_width / 8);
+ if (!pcc_doorbell_ack_vaddr[id]) {
+ pr_err("Failed to ioremap PCC ACK register\n");
+ return -ENOMEM;
+ }
+ }
+
+ return 0;
+}
+
+/**
* acpi_pcc_probe - Parse the ACPI tree for the PCCT.
*
* Return: 0 for Success, else errno.
@@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
acpi_size pcct_tbl_header_size;
struct acpi_table_header *pcct_tbl;
struct acpi_subtable_header *pcct_entry;
- int count, i;
+ struct acpi_table_pcct *acpi_pcct_tbl;
+ int count, i, rc;
+ int sum = 0;
acpi_status status = AE_OK;
/* Search for PCCT */
@@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
sizeof(struct acpi_table_pcct),
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
parse_pcc_subspace, MAX_PCC_SUBSPACES);
+ sum += (count > 0)? count: 0;
+
+ count = acpi_table_parse_entries(ACPI_SIG_PCCT,
+ sizeof(struct acpi_table_pcct),
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
+ parse_pcc_subspace, MAX_PCC_SUBSPACES);
+ sum += (count > 0)? count: 0;
- if (count <= 0) {
+ if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
pr_err("Error parsing PCC subspaces from PCCT\n");
return -EINVAL;
}
pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
- count, GFP_KERNEL);
-
+ sum, GFP_KERNEL);
if (!pcc_mbox_channels) {
pr_err("Could not allocate space for PCC mbox channels\n");
return -ENOMEM;
}
- pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
+ pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
if (!pcc_doorbell_vaddr) {
- kfree(pcc_mbox_channels);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto err_free_mbox;
+ }
+
+ pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+ if (!pcc_doorbell_ack_vaddr) {
+ rc = -ENOMEM;
+ goto err_free_db_vaddr;
+ }
+
+ pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
+ if (!pcc_doorbell_irq) {
+ rc = -ENOMEM;
+ goto err_free_db_ack_vaddr;
}
/* Point to the first PCC subspace entry */
pcct_entry = (struct acpi_subtable_header *) (
(unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
- for (i = 0; i < count; i++) {
+ acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
+ if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
+ pcc_mbox_ctrl.txdone_irq = true;
+
+ for (i = 0; i < sum; i++) {
struct acpi_generic_address *db_reg;
struct acpi_pcct_hw_reduced *pcct_ss;
pcc_mbox_channels[i].con_priv = pcct_entry;
+ pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
+
+ if (pcc_mbox_ctrl.txdone_irq) {
+ rc = pcc_parse_subspace_irq(i, pcct_ss);
+ if (rc < 0)
+ goto err;
+ }
+
/* If doorbell is in system memory cache the virt address */
- pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
db_reg = &pcct_ss->doorbell_register;
if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
pcc_doorbell_vaddr[i] = acpi_os_ioremap(db_reg->address,
@@ -372,11 +536,21 @@ static int __init acpi_pcc_probe(void)
((unsigned long) pcct_entry + pcct_entry->length);
}
- pcc_mbox_ctrl.num_chans = count;
+ pcc_mbox_ctrl.num_chans = sum;
pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
return 0;
+
+err:
+ kfree(pcc_doorbell_irq);
+err_free_db_ack_vaddr:
+ kfree(pcc_doorbell_ack_vaddr);
+err_free_db_vaddr:
+ kfree(pcc_doorbell_vaddr);
+err_free_mbox:
+ kfree(pcc_mbox_channels);
+ return rc;
}
/**
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-05-20 0:32 [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2 Hoan Tran
@ 2016-05-31 16:32 ` Hoan Tran
2016-05-31 19:05 ` Ashwin Chaugule
1 sibling, 0 replies; 20+ messages in thread
From: Hoan Tran @ 2016-05-31 16:32 UTC (permalink / raw)
To: Jassi Brar, Ashwin Chaugule
Cc: Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml, linux acpi,
Loc Ho, Duc Dang, Hoan Tran
Hi Ashwin,
Do you have any comments ?
Thanks
Hoan
On Thu, May 19, 2016 at 5:32 PM, Hoan Tran <hotran@apm.com> wrote:
> ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
> to acknowledge doorbell interrupt. This patch provides the implementation
> for the Communication Subspace Type 2.
>
> v3
> * Remove 2 global structures
> * Correct parsing subspace type 1 and subspace type 2
>
> v2
> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
> * Parse both subspace type 1 and subspace type 2
> * Remove unnecessary variable initialization
> * ISR returns IRQ_NONE in case of error
>
> v1
> * Initial
>
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---
> drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 245 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..c98bd94 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -59,6 +59,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/list.h>
> #include <linux/platform_device.h>
> #include <linux/mailbox_controller.h>
> @@ -68,11 +69,16 @@
> #include "mailbox.h"
>
> #define MAX_PCC_SUBSPACES 256
> +#define MBOX_IRQ_NAME "pcc-mbox"
>
> static struct mbox_chan *pcc_mbox_channels;
>
> /* Array of cached virtual address for doorbell registers */
> static void __iomem **pcc_doorbell_vaddr;
> +/* Array of cached virtual address for doorbell ack registers */
> +static void __iomem **pcc_doorbell_ack_vaddr;
> +/* Array of doorbell interrupts */
> +static int *pcc_doorbell_irq;
>
> static struct mbox_controller pcc_mbox_ctrl = {};
> /**
> @@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
> return &pcc_mbox_channels[id];
> }
>
> +/*
> + * PCC can be used with perf critical drivers such as CPPC
> + * So it makes sense to locally cache the virtual address and
> + * use it to read/write to PCC registers such as doorbell register
> + *
> + * The below read_register and write_registers are used to read and
> + * write from perf critical registers such as PCC doorbell register
> + */
> +static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
> +{
> + int ret_val = 0;
> +
> + switch (bit_width) {
> + case 8:
> + *val = readb(vaddr);
> + break;
> + case 16:
> + *val = readw(vaddr);
> + break;
> + case 32:
> + *val = readl(vaddr);
> + break;
> + case 64:
> + *val = readq(vaddr);
> + break;
> + default:
> + pr_debug("Error: Cannot read register of %u bit width",
> + bit_width);
> + ret_val = -EFAULT;
> + break;
> + }
> + return ret_val;
> +}
> +
> +static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
> +{
> + int ret_val = 0;
> +
> + switch (bit_width) {
> + case 8:
> + writeb(val, vaddr);
> + break;
> + case 16:
> + writew(val, vaddr);
> + break;
> + case 32:
> + writel(val, vaddr);
> + break;
> + case 64:
> + writeq(val, vaddr);
> + break;
> + default:
> + pr_debug("Error: Cannot write register of %u bit width",
> + bit_width);
> + ret_val = -EFAULT;
> + break;
> + }
> + return ret_val;
> +}
> +
> +/**
> + * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
> + * @interrupt: GSI number.
> + * @flags: interrupt flags
> + *
> + * Returns: a valid linux IRQ number on success
> + * 0 or -EINVAL on failure
> + */
> +static int pcc_map_interrupt(u32 interrupt, u32 flags)
> +{
> + int trigger, polarity;
> +
> + if (!interrupt)
> + return 0;
> +
> + trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
> + : ACPI_LEVEL_SENSITIVE;
> +
> + polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
> + : ACPI_ACTIVE_HIGH;
> +
> + return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> +}
> +
> +/**
> + * pcc_mbox_irq - PCC mailbox interrupt handler
> + */
> +static irqreturn_t pcc_mbox_irq(int irq, void *p)
> +{
> + struct acpi_generic_address *doorbell_ack;
> + struct acpi_pcct_hw_reduced *pcct_ss;
> + struct mbox_chan *chan = p;
> + u64 doorbell_ack_preserve;
> + u64 doorbell_ack_write;
> + u64 doorbell_ack_val;
> + int ret;
> +
> + pcct_ss = chan->con_priv;
> +
> + mbox_chan_received_data(chan, NULL);
> +
> + if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> + struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv;
> + u32 id = chan - pcc_mbox_channels;
> +
> + doorbell_ack = &pcct2_ss->doorbell_ack_register;
> + doorbell_ack_preserve = pcct2_ss->ack_preserve_mask;
> + doorbell_ack_write = pcct2_ss->ack_write_mask;
> +
> + ret = read_register(pcc_doorbell_ack_vaddr[id],
> + &doorbell_ack_val,
> + doorbell_ack->bit_width);
> + if (ret)
> + return IRQ_NONE;
> +
> + ret = write_register(pcc_doorbell_ack_vaddr[id],
> + (doorbell_ack_val & doorbell_ack_preserve)
> + | doorbell_ack_write,
> + doorbell_ack->bit_width);
> + if (ret)
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> /**
> * pcc_mbox_request_channel - PCC clients call this function to
> * request a pointer to their PCC subspace, from which they
> @@ -135,6 +267,18 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
> if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
> chan->txdone_method |= TXDONE_BY_ACK;
>
> + if (pcc_doorbell_irq[subspace_id] > 0) {
> + int rc;
> +
> + rc = devm_request_irq(dev, pcc_doorbell_irq[subspace_id],
> + pcc_mbox_irq, 0, MBOX_IRQ_NAME, chan);
> + if (unlikely(rc)) {
> + dev_err(dev, "failed to register PCC interrupt %d\n",
> + pcc_doorbell_irq[subspace_id]);
> + chan = ERR_PTR(rc);
> + }
> + }
> +
> spin_unlock_irqrestore(&chan->lock, flags);
>
> return chan;
> @@ -149,80 +293,30 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
> */
> void pcc_mbox_free_channel(struct mbox_chan *chan)
> {
> + u32 id = chan - pcc_mbox_channels;
> unsigned long flags;
>
> if (!chan || !chan->cl)
> return;
>
> + if (id >= pcc_mbox_ctrl.num_chans) {
> + pr_debug("pcc_mbox_free_channel: Invalid mbox_chan passed\n");
> + 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;
>
> + if (pcc_doorbell_irq[id] > 0)
> + devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan);
> +
> spin_unlock_irqrestore(&chan->lock, flags);
> }
> EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>
> -/*
> - * PCC can be used with perf critical drivers such as CPPC
> - * So it makes sense to locally cache the virtual address and
> - * use it to read/write to PCC registers such as doorbell register
> - *
> - * The below read_register and write_registers are used to read and
> - * write from perf critical registers such as PCC doorbell register
> - */
> -static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
> -{
> - int ret_val = 0;
> -
> - switch (bit_width) {
> - case 8:
> - *val = readb(vaddr);
> - break;
> - case 16:
> - *val = readw(vaddr);
> - break;
> - case 32:
> - *val = readl(vaddr);
> - break;
> - case 64:
> - *val = readq(vaddr);
> - break;
> - default:
> - pr_debug("Error: Cannot read register of %u bit width",
> - bit_width);
> - ret_val = -EFAULT;
> - break;
> - }
> - return ret_val;
> -}
> -
> -static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
> -{
> - int ret_val = 0;
> -
> - switch (bit_width) {
> - case 8:
> - writeb(val, vaddr);
> - break;
> - case 16:
> - writew(val, vaddr);
> - break;
> - case 32:
> - writel(val, vaddr);
> - break;
> - case 64:
> - writeq(val, vaddr);
> - break;
> - default:
> - pr_debug("Error: Cannot write register of %u bit width",
> - bit_width);
> - ret_val = -EFAULT;
> - break;
> - }
> - return ret_val;
> -}
>
> /**
> * pcc_send_data - Called from Mailbox Controller code. Used
> @@ -296,8 +390,10 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
> 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) {
> + if ((pcct_ss->header.type !=
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
> + && (pcct_ss->header.type !=
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
> pr_err("Incorrect PCC Subspace type detected\n");
> return -EINVAL;
> }
> @@ -307,6 +403,43 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
> }
>
> /**
> + * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
> + * There should be one entry per PCC client.
> + * @id: PCC subspace index.
> + * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
> + *
> + * Return: 0 for Success, else errno.
> + *
> + * This gets called for each entry in the PCC table.
> + */
> +static int pcc_parse_subspace_irq(int id,
> + struct acpi_pcct_hw_reduced *pcct_ss)
> +{
> + pcc_doorbell_irq[id] = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
> + (u32)pcct_ss->flags);
> + if (pcc_doorbell_irq[id] <= 0) {
> + pr_err("PCC GSI %d not registered\n",
> + pcct_ss->doorbell_interrupt);
> + return -EINVAL;
> + }
> +
> + if (pcct_ss->header.type
> + == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> + struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
> +
> + pcc_doorbell_ack_vaddr[id] = acpi_os_ioremap(
> + pcct2_ss->doorbell_ack_register.address,
> + pcct2_ss->doorbell_ack_register.bit_width / 8);
> + if (!pcc_doorbell_ack_vaddr[id]) {
> + pr_err("Failed to ioremap PCC ACK register\n");
> + return -ENOMEM;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
> *
> * Return: 0 for Success, else errno.
> @@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
> acpi_size pcct_tbl_header_size;
> struct acpi_table_header *pcct_tbl;
> struct acpi_subtable_header *pcct_entry;
> - int count, i;
> + struct acpi_table_pcct *acpi_pcct_tbl;
> + int count, i, rc;
> + int sum = 0;
> acpi_status status = AE_OK;
>
> /* Search for PCCT */
> @@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
> sizeof(struct acpi_table_pcct),
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> parse_pcc_subspace, MAX_PCC_SUBSPACES);
> + sum += (count > 0)? count: 0;
> +
> + count = acpi_table_parse_entries(ACPI_SIG_PCCT,
> + sizeof(struct acpi_table_pcct),
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
> + sum += (count > 0)? count: 0;
>
> - if (count <= 0) {
> + if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
> pr_err("Error parsing PCC subspaces from PCCT\n");
> return -EINVAL;
> }
>
> pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
> - count, GFP_KERNEL);
> -
> + sum, GFP_KERNEL);
> if (!pcc_mbox_channels) {
> pr_err("Could not allocate space for PCC mbox channels\n");
> return -ENOMEM;
> }
>
> - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
> + pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> if (!pcc_doorbell_vaddr) {
> - kfree(pcc_mbox_channels);
> - return -ENOMEM;
> + rc = -ENOMEM;
> + goto err_free_mbox;
> + }
> +
> + pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> + if (!pcc_doorbell_ack_vaddr) {
> + rc = -ENOMEM;
> + goto err_free_db_vaddr;
> + }
> +
> + pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
> + if (!pcc_doorbell_irq) {
> + rc = -ENOMEM;
> + goto err_free_db_ack_vaddr;
> }
>
> /* Point to the first PCC subspace entry */
> pcct_entry = (struct acpi_subtable_header *) (
> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>
> - for (i = 0; i < count; i++) {
> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
> + pcc_mbox_ctrl.txdone_irq = true;
> +
> + for (i = 0; i < sum; i++) {
> struct acpi_generic_address *db_reg;
> struct acpi_pcct_hw_reduced *pcct_ss;
> pcc_mbox_channels[i].con_priv = pcct_entry;
>
> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
> +
> + if (pcc_mbox_ctrl.txdone_irq) {
> + rc = pcc_parse_subspace_irq(i, pcct_ss);
> + if (rc < 0)
> + goto err;
> + }
> +
> /* If doorbell is in system memory cache the virt address */
> - pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
> db_reg = &pcct_ss->doorbell_register;
> if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> pcc_doorbell_vaddr[i] = acpi_os_ioremap(db_reg->address,
> @@ -372,11 +536,21 @@ static int __init acpi_pcc_probe(void)
> ((unsigned long) pcct_entry + pcct_entry->length);
> }
>
> - pcc_mbox_ctrl.num_chans = count;
> + pcc_mbox_ctrl.num_chans = sum;
>
> pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
>
> return 0;
> +
> +err:
> + kfree(pcc_doorbell_irq);
> +err_free_db_ack_vaddr:
> + kfree(pcc_doorbell_ack_vaddr);
> +err_free_db_vaddr:
> + kfree(pcc_doorbell_vaddr);
> +err_free_mbox:
> + kfree(pcc_mbox_channels);
> + return rc;
> }
>
> /**
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-05-20 0:32 [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2 Hoan Tran
2016-05-31 16:32 ` Hoan Tran
@ 2016-05-31 19:05 ` Ashwin Chaugule
2016-05-31 19:35 ` Hoan Tran
1 sibling, 1 reply; 20+ messages in thread
From: Ashwin Chaugule @ 2016-05-31 19:05 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang
Hi Hoan,
On 19 May 2016 at 20:32, Hoan Tran <hotran@apm.com> wrote:
> ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
> to acknowledge doorbell interrupt. This patch provides the implementation
> for the Communication Subspace Type 2.
>
> v3
> * Remove 2 global structures
> * Correct parsing subspace type 1 and subspace type 2
>
> v2
> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
> * Parse both subspace type 1 and subspace type 2
> * Remove unnecessary variable initialization
> * ISR returns IRQ_NONE in case of error
>
> v1
> * Initial
>
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---
> drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 245 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..c98bd94 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -59,6 +59,7 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/list.h>
> #include <linux/platform_device.h>
> #include <linux/mailbox_controller.h>
> @@ -68,11 +69,16 @@
> #include "mailbox.h"
>
> #define MAX_PCC_SUBSPACES 256
> +#define MBOX_IRQ_NAME "pcc-mbox"
>
> static struct mbox_chan *pcc_mbox_channels;
>
> /* Array of cached virtual address for doorbell registers */
> static void __iomem **pcc_doorbell_vaddr;
> +/* Array of cached virtual address for doorbell ack registers */
> +static void __iomem **pcc_doorbell_ack_vaddr;
> +/* Array of doorbell interrupts */
> +static int *pcc_doorbell_irq;
>
> static struct mbox_controller pcc_mbox_ctrl = {};
> /**
> @@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
> return &pcc_mbox_channels[id];
> }
>
> +/*
> + * PCC can be used with perf critical drivers such as CPPC
> + * So it makes sense to locally cache the virtual address and
> + * use it to read/write to PCC registers such as doorbell register
> + *
> + * The below read_register and write_registers are used to read and
> + * write from perf critical registers such as PCC doorbell register
> + */
> +static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
> +{
> + int ret_val = 0;
> +
> + switch (bit_width) {
> + case 8:
> + *val = readb(vaddr);
> + break;
> + case 16:
> + *val = readw(vaddr);
> + break;
> + case 32:
> + *val = readl(vaddr);
> + break;
> + case 64:
> + *val = readq(vaddr);
> + break;
> + default:
> + pr_debug("Error: Cannot read register of %u bit width",
> + bit_width);
> + ret_val = -EFAULT;
> + break;
> + }
> + return ret_val;
> +}
> +
> +static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
> +{
> + int ret_val = 0;
> +
> + switch (bit_width) {
> + case 8:
> + writeb(val, vaddr);
> + break;
> + case 16:
> + writew(val, vaddr);
> + break;
> + case 32:
> + writel(val, vaddr);
> + break;
> + case 64:
> + writeq(val, vaddr);
> + break;
> + default:
> + pr_debug("Error: Cannot write register of %u bit width",
> + bit_width);
> + ret_val = -EFAULT;
> + break;
> + }
> + return ret_val;
> +}
> +
Isn't this just reordering of function locations? I don't mean to be
nitpicky, but your cosmetic changes in this file make it harder to
review the meat of the patch.
> +/**
> + * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
> + * @interrupt: GSI number.
> + * @flags: interrupt flags
> + *
> + * Returns: a valid linux IRQ number on success
> + * 0 or -EINVAL on failure
> + */
> +static int pcc_map_interrupt(u32 interrupt, u32 flags)
> +{
> + int trigger, polarity;
> +
> + if (!interrupt)
> + return 0;
> +
> + trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
> + : ACPI_LEVEL_SENSITIVE;
> +
> + polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
> + : ACPI_ACTIVE_HIGH;
> +
> + return acpi_register_gsi(NULL, interrupt, trigger, polarity);
> +}
> +
> +/**
> + * pcc_mbox_irq - PCC mailbox interrupt handler
> + */
> +static irqreturn_t pcc_mbox_irq(int irq, void *p)
> +{
> + struct acpi_generic_address *doorbell_ack;
> + struct acpi_pcct_hw_reduced *pcct_ss;
> + struct mbox_chan *chan = p;
> + u64 doorbell_ack_preserve;
> + u64 doorbell_ack_write;
> + u64 doorbell_ack_val;
> + int ret;
> +
> + pcct_ss = chan->con_priv;
> +
> + mbox_chan_received_data(chan, NULL);
Does this really do anything? IIUC, your mbox controller rings
doorbell, platform sets cmd_completion and sends an IRQ to the OS.
This is the function where you field it to ACK the doorbell. But
you're not really consuming any new data sent by the platform, right?
i.e. this IRQ use case is not the same as the async notification from
platform as described in the ACPIv5+ spec. Or did I misunderstand the
bigger picture?
[...]
> +
> +/**
> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
> *
> * Return: 0 for Success, else errno.
> @@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
> acpi_size pcct_tbl_header_size;
> struct acpi_table_header *pcct_tbl;
> struct acpi_subtable_header *pcct_entry;
> - int count, i;
> + struct acpi_table_pcct *acpi_pcct_tbl;
> + int count, i, rc;
> + int sum = 0;
> acpi_status status = AE_OK;
>
> /* Search for PCCT */
> @@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
> sizeof(struct acpi_table_pcct),
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> parse_pcc_subspace, MAX_PCC_SUBSPACES);
> + sum += (count > 0)? count: 0;
> +
> + count = acpi_table_parse_entries(ACPI_SIG_PCCT,
> + sizeof(struct acpi_table_pcct),
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
> + sum += (count > 0)? count: 0;
>
> - if (count <= 0) {
> + if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
> pr_err("Error parsing PCC subspaces from PCCT\n");
> return -EINVAL;
> }
>
> pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
> - count, GFP_KERNEL);
> -
> + sum, GFP_KERNEL);
> if (!pcc_mbox_channels) {
> pr_err("Could not allocate space for PCC mbox channels\n");
> return -ENOMEM;
> }
>
> - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
> + pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> if (!pcc_doorbell_vaddr) {
> - kfree(pcc_mbox_channels);
> - return -ENOMEM;
> + rc = -ENOMEM;
> + goto err_free_mbox;
> + }
> +
> + pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> + if (!pcc_doorbell_ack_vaddr) {
> + rc = -ENOMEM;
> + goto err_free_db_vaddr;
> + }
> +
> + pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
> + if (!pcc_doorbell_irq) {
> + rc = -ENOMEM;
> + goto err_free_db_ack_vaddr;
> }
>
> /* Point to the first PCC subspace entry */
> pcct_entry = (struct acpi_subtable_header *) (
> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>
> - for (i = 0; i < count; i++) {
> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
> + pcc_mbox_ctrl.txdone_irq = true;
> +
This flag is still bothering me. Can't you have a system where one
client requires polling on the command completion bit and another can
send a command completion IRQ? Seems to me that you need a channel
specific flag to indicate this. Besides, this global flag seems to be
used for async platform notifications. i.e. when the OS sets the
Generate doorbell irq bit in the Command field (Section 14.2.1 in ACPI
v6.1).
Thanks,
Ashwin.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-05-31 19:05 ` Ashwin Chaugule
@ 2016-05-31 19:35 ` Hoan Tran
2016-06-08 12:18 ` Ashwin Chaugule
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-05-31 19:35 UTC (permalink / raw)
To: Ashwin Chaugule
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang
Hi Ashwin,
On Tue, May 31, 2016 at 12:05 PM, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> Hi Hoan,
>
> On 19 May 2016 at 20:32, Hoan Tran <hotran@apm.com> wrote:
>> ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
>> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
>> to acknowledge doorbell interrupt. This patch provides the implementation
>> for the Communication Subspace Type 2.
>>
>> v3
>> * Remove 2 global structures
>> * Correct parsing subspace type 1 and subspace type 2
>>
>> v2
>> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
>> * Parse both subspace type 1 and subspace type 2
>> * Remove unnecessary variable initialization
>> * ISR returns IRQ_NONE in case of error
>>
>> v1
>> * Initial
>>
>> Signed-off-by: Hoan Tran <hotran@apm.com>
>> ---
>> drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 245 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 043828d..c98bd94 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -59,6 +59,7 @@
>> #include <linux/delay.h>
>> #include <linux/io.h>
>> #include <linux/init.h>
>> +#include <linux/interrupt.h>
>> #include <linux/list.h>
>> #include <linux/platform_device.h>
>> #include <linux/mailbox_controller.h>
>> @@ -68,11 +69,16 @@
>> #include "mailbox.h"
>>
>> #define MAX_PCC_SUBSPACES 256
>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>
>> static struct mbox_chan *pcc_mbox_channels;
>>
>> /* Array of cached virtual address for doorbell registers */
>> static void __iomem **pcc_doorbell_vaddr;
>> +/* Array of cached virtual address for doorbell ack registers */
>> +static void __iomem **pcc_doorbell_ack_vaddr;
>> +/* Array of doorbell interrupts */
>> +static int *pcc_doorbell_irq;
>>
>> static struct mbox_controller pcc_mbox_ctrl = {};
>> /**
>> @@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
>> return &pcc_mbox_channels[id];
>> }
>>
>> +/*
>> + * PCC can be used with perf critical drivers such as CPPC
>> + * So it makes sense to locally cache the virtual address and
>> + * use it to read/write to PCC registers such as doorbell register
>> + *
>> + * The below read_register and write_registers are used to read and
>> + * write from perf critical registers such as PCC doorbell register
>> + */
>> +static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
>> +{
>> + int ret_val = 0;
>> +
>> + switch (bit_width) {
>> + case 8:
>> + *val = readb(vaddr);
>> + break;
>> + case 16:
>> + *val = readw(vaddr);
>> + break;
>> + case 32:
>> + *val = readl(vaddr);
>> + break;
>> + case 64:
>> + *val = readq(vaddr);
>> + break;
>> + default:
>> + pr_debug("Error: Cannot read register of %u bit width",
>> + bit_width);
>> + ret_val = -EFAULT;
>> + break;
>> + }
>> + return ret_val;
>> +}
>> +
>> +static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
>> +{
>> + int ret_val = 0;
>> +
>> + switch (bit_width) {
>> + case 8:
>> + writeb(val, vaddr);
>> + break;
>> + case 16:
>> + writew(val, vaddr);
>> + break;
>> + case 32:
>> + writel(val, vaddr);
>> + break;
>> + case 64:
>> + writeq(val, vaddr);
>> + break;
>> + default:
>> + pr_debug("Error: Cannot write register of %u bit width",
>> + bit_width);
>> + ret_val = -EFAULT;
>> + break;
>> + }
>> + return ret_val;
>> +}
>> +
>
> Isn't this just reordering of function locations? I don't mean to be
> nitpicky, but your cosmetic changes in this file make it harder to
> review the meat of the patch.
Yes, it is. As I would like to call these functions but actually, they
are declared quite late.
Do you have a any suggestion? Thanks
>
>> +/**
>> + * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
>> + * @interrupt: GSI number.
>> + * @flags: interrupt flags
>> + *
>> + * Returns: a valid linux IRQ number on success
>> + * 0 or -EINVAL on failure
>> + */
>> +static int pcc_map_interrupt(u32 interrupt, u32 flags)
>> +{
>> + int trigger, polarity;
>> +
>> + if (!interrupt)
>> + return 0;
>> +
>> + trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
>> + : ACPI_LEVEL_SENSITIVE;
>> +
>> + polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
>> + : ACPI_ACTIVE_HIGH;
>> +
>> + return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>> +}
>> +
>> +/**
>> + * pcc_mbox_irq - PCC mailbox interrupt handler
>> + */
>> +static irqreturn_t pcc_mbox_irq(int irq, void *p)
>> +{
>> + struct acpi_generic_address *doorbell_ack;
>> + struct acpi_pcct_hw_reduced *pcct_ss;
>> + struct mbox_chan *chan = p;
>> + u64 doorbell_ack_preserve;
>> + u64 doorbell_ack_write;
>> + u64 doorbell_ack_val;
>> + int ret;
>> +
>> + pcct_ss = chan->con_priv;
>> +
>> + mbox_chan_received_data(chan, NULL);
>
> Does this really do anything? IIUC, your mbox controller rings
> doorbell, platform sets cmd_completion and sends an IRQ to the OS.
> This is the function where you field it to ACK the doorbell. But
> you're not really consuming any new data sent by the platform, right?
> i.e. this IRQ use case is not the same as the async notification from
> platform as described in the ACPIv5+ spec. Or did I misunderstand the
> bigger picture?
>
The purpose of this function call is
1./ Notify the completion of a command to OSPM.
For example, instead of using polling with udelay, CPPC could use this
call to complete a "completion".
I'm preparing to post a version to support it for CPPC.
2./ Notify OSPM about a notification message sent by Platform
>
> [...]
>
>> +
>> +/**
>> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>> *
>> * Return: 0 for Success, else errno.
>> @@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
>> acpi_size pcct_tbl_header_size;
>> struct acpi_table_header *pcct_tbl;
>> struct acpi_subtable_header *pcct_entry;
>> - int count, i;
>> + struct acpi_table_pcct *acpi_pcct_tbl;
>> + int count, i, rc;
>> + int sum = 0;
>> acpi_status status = AE_OK;
>>
>> /* Search for PCCT */
>> @@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
>> sizeof(struct acpi_table_pcct),
>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>> + sum += (count > 0)? count: 0;
>> +
>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT,
>> + sizeof(struct acpi_table_pcct),
>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
>> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
>> + sum += (count > 0)? count: 0;
>>
>> - if (count <= 0) {
>> + if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
>> pr_err("Error parsing PCC subspaces from PCCT\n");
>> return -EINVAL;
>> }
>>
>> pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
>> - count, GFP_KERNEL);
>> -
>> + sum, GFP_KERNEL);
>> if (!pcc_mbox_channels) {
>> pr_err("Could not allocate space for PCC mbox channels\n");
>> return -ENOMEM;
>> }
>>
>> - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
>> + pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>> if (!pcc_doorbell_vaddr) {
>> - kfree(pcc_mbox_channels);
>> - return -ENOMEM;
>> + rc = -ENOMEM;
>> + goto err_free_mbox;
>> + }
>> +
>> + pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>> + if (!pcc_doorbell_ack_vaddr) {
>> + rc = -ENOMEM;
>> + goto err_free_db_vaddr;
>> + }
>> +
>> + pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
>> + if (!pcc_doorbell_irq) {
>> + rc = -ENOMEM;
>> + goto err_free_db_ack_vaddr;
>> }
>>
>> /* Point to the first PCC subspace entry */
>> pcct_entry = (struct acpi_subtable_header *) (
>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>
>> - for (i = 0; i < count; i++) {
>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>> + pcc_mbox_ctrl.txdone_irq = true;
>> +
>
> This flag is still bothering me. Can't you have a system where one
> client requires polling on the command completion bit and another can
> send a command completion IRQ? Seems to me that you need a channel
> specific flag to indicate this. Besides, this global flag seems to be
> used for async platform notifications. i.e. when the OS sets the
> Generate doorbell irq bit in the Command field (Section 14.2.1 in ACPI
> v6.1).
I thought this bit is the global configuration for PCC.
We still can support the polling mode if this flag is ON. Just don't
set "Generate Doorbell Interrupt" bit inside Command Field.
Thanks and Regards
Hoan
>
>
> Thanks,
> Ashwin.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-05-31 19:35 ` Hoan Tran
@ 2016-06-08 12:18 ` Ashwin Chaugule
2016-06-08 16:24 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Ashwin Chaugule @ 2016-06-08 12:18 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prashanth Prakash
+ Prashanth (Can you please have a look as well?)
On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
> Hi Ashwin,
Hi,
Sorry about the delay. I'm in the middle of switching jobs and
locations, so its been a bit crazy lately. I dont have any major
concerns with this code, although there could be subtle issues with
this IRQ thing. In this patchset, your intent is to add support for
PCC subspace type 2. But you're also adding support for tx command
completion which is not specific to Type 2. We could support that even
in Type 1. Hence I wanted to separate the two, not just for review,
but also the async IRQ completion has subtle issues esp. in the case
of async platform notification, where you could have a PCC client in
the OS writing to the cmd bit and the platform sending an async
notification by writing to some bits in the same 8byte address as the
cmd bit. So we need some mutual exclusivity there, otherwise the OS
and platform could step on each other. Perhaps Prashanth has better
insight into this.
>
> On Tue, May 31, 2016 at 12:05 PM, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
>> On 19 May 2016 at 20:32, Hoan Tran <hotran@apm.com> wrote:
>>> ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
>>> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
>>> to acknowledge doorbell interrupt. This patch provides the implementation
>>> for the Communication Subspace Type 2.
>>>
>>> v3
>>> * Remove 2 global structures
>>> * Correct parsing subspace type 1 and subspace type 2
>>>
>>> v2
>>> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
>>> * Parse both subspace type 1 and subspace type 2
>>> * Remove unnecessary variable initialization
>>> * ISR returns IRQ_NONE in case of error
>>>
>>> v1
>>> * Initial
>>>
>>> Signed-off-by: Hoan Tran <hotran@apm.com>
>>> ---
>>> drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 245 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 043828d..c98bd94 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -59,6 +59,7 @@
>>> #include <linux/delay.h>
>>> #include <linux/io.h>
>>> #include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> #include <linux/list.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/mailbox_controller.h>
>>> @@ -68,11 +69,16 @@
>>> #include "mailbox.h"
>>>
>>> #define MAX_PCC_SUBSPACES 256
>>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>>
>>> static struct mbox_chan *pcc_mbox_channels;
>>>
>>> /* Array of cached virtual address for doorbell registers */
>>> static void __iomem **pcc_doorbell_vaddr;
>>> +/* Array of cached virtual address for doorbell ack registers */
>>> +static void __iomem **pcc_doorbell_ack_vaddr;
>>> +/* Array of doorbell interrupts */
>>> +static int *pcc_doorbell_irq;
>>>
>>> static struct mbox_controller pcc_mbox_ctrl = {};
>>> /**
>>> @@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
>>> return &pcc_mbox_channels[id];
>>> }
>>>
>>> +/*
>>> + * PCC can be used with perf critical drivers such as CPPC
>>> + * So it makes sense to locally cache the virtual address and
>>> + * use it to read/write to PCC registers such as doorbell register
>>> + *
>>> + * The below read_register and write_registers are used to read and
>>> + * write from perf critical registers such as PCC doorbell register
>>> + */
>>> +static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
>>> +{
>>> + int ret_val = 0;
>>> +
>>> + switch (bit_width) {
>>> + case 8:
>>> + *val = readb(vaddr);
>>> + break;
>>> + case 16:
>>> + *val = readw(vaddr);
>>> + break;
>>> + case 32:
>>> + *val = readl(vaddr);
>>> + break;
>>> + case 64:
>>> + *val = readq(vaddr);
>>> + break;
>>> + default:
>>> + pr_debug("Error: Cannot read register of %u bit width",
>>> + bit_width);
>>> + ret_val = -EFAULT;
>>> + break;
>>> + }
>>> + return ret_val;
>>> +}
>>> +
>>> +static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
>>> +{
>>> + int ret_val = 0;
>>> +
>>> + switch (bit_width) {
>>> + case 8:
>>> + writeb(val, vaddr);
>>> + break;
>>> + case 16:
>>> + writew(val, vaddr);
>>> + break;
>>> + case 32:
>>> + writel(val, vaddr);
>>> + break;
>>> + case 64:
>>> + writeq(val, vaddr);
>>> + break;
>>> + default:
>>> + pr_debug("Error: Cannot write register of %u bit width",
>>> + bit_width);
>>> + ret_val = -EFAULT;
>>> + break;
>>> + }
>>> + return ret_val;
>>> +}
>>> +
>>
>> Isn't this just reordering of function locations? I don't mean to be
>> nitpicky, but your cosmetic changes in this file make it harder to
>> review the meat of the patch.
>
> Yes, it is. As I would like to call these functions but actually, they
> are declared quite late.
> Do you have a any suggestion? Thanks
>
No this is fine. I missed the place where you needed to call these.
>>
>>> +/**
>>> + * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
>>> + * @interrupt: GSI number.
>>> + * @flags: interrupt flags
>>> + *
>>> + * Returns: a valid linux IRQ number on success
>>> + * 0 or -EINVAL on failure
>>> + */
>>> +static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>> +{
>>> + int trigger, polarity;
>>> +
>>> + if (!interrupt)
>>> + return 0;
>>> +
>>> + trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
>>> + : ACPI_LEVEL_SENSITIVE;
>>> +
>>> + polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
>>> + : ACPI_ACTIVE_HIGH;
>>> +
>>> + return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>> +}
>>> +
>>> +/**
>>> + * pcc_mbox_irq - PCC mailbox interrupt handler
>>> + */
>>> +static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>> +{
>>> + struct acpi_generic_address *doorbell_ack;
>>> + struct acpi_pcct_hw_reduced *pcct_ss;
>>> + struct mbox_chan *chan = p;
>>> + u64 doorbell_ack_preserve;
>>> + u64 doorbell_ack_write;
>>> + u64 doorbell_ack_val;
>>> + int ret;
>>> +
>>> + pcct_ss = chan->con_priv;
>>> +
>>> + mbox_chan_received_data(chan, NULL);
>>
>> Does this really do anything? IIUC, your mbox controller rings
>> doorbell, platform sets cmd_completion and sends an IRQ to the OS.
>> This is the function where you field it to ACK the doorbell. But
>> you're not really consuming any new data sent by the platform, right?
>> i.e. this IRQ use case is not the same as the async notification from
>> platform as described in the ACPIv5+ spec. Or did I misunderstand the
>> bigger picture?
>>
>
> The purpose of this function call is
> 1./ Notify the completion of a command to OSPM.
> For example, instead of using polling with udelay, CPPC could use this
> call to complete a "completion".
> I'm preparing to post a version to support it for CPPC.
>
I see. This is the part I was missing. Seems like you could call the
mbox_chan_received_data() only after you add support for this.
> 2./ Notify OSPM about a notification message sent by Platform
This will require adding some sort of rx_callback to field the notification.
>>
>> [...]
>>
>>> +
>>> +/**
>>> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>>> *
>>> * Return: 0 for Success, else errno.
>>> @@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
>>> acpi_size pcct_tbl_header_size;
>>> struct acpi_table_header *pcct_tbl;
>>> struct acpi_subtable_header *pcct_entry;
>>> - int count, i;
>>> + struct acpi_table_pcct *acpi_pcct_tbl;
>>> + int count, i, rc;
>>> + int sum = 0;
>>> acpi_status status = AE_OK;
>>>
>>> /* Search for PCCT */
>>> @@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
>>> sizeof(struct acpi_table_pcct),
>>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>>> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>>> + sum += (count > 0)? count: 0;
>>> +
>>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT,
>>> + sizeof(struct acpi_table_pcct),
>>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
>>> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
>>> + sum += (count > 0)? count: 0;
>>>
>>> - if (count <= 0) {
>>> + if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
>>> pr_err("Error parsing PCC subspaces from PCCT\n");
>>> return -EINVAL;
>>> }
>>>
>>> pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
>>> - count, GFP_KERNEL);
>>> -
>>> + sum, GFP_KERNEL);
>>> if (!pcc_mbox_channels) {
>>> pr_err("Could not allocate space for PCC mbox channels\n");
>>> return -ENOMEM;
>>> }
>>>
>>> - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
>>> + pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>>> if (!pcc_doorbell_vaddr) {
>>> - kfree(pcc_mbox_channels);
>>> - return -ENOMEM;
>>> + rc = -ENOMEM;
>>> + goto err_free_mbox;
>>> + }
>>> +
>>> + pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>>> + if (!pcc_doorbell_ack_vaddr) {
>>> + rc = -ENOMEM;
>>> + goto err_free_db_vaddr;
>>> + }
>>> +
>>> + pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
>>> + if (!pcc_doorbell_irq) {
>>> + rc = -ENOMEM;
>>> + goto err_free_db_ack_vaddr;
>>> }
>>>
>>> /* Point to the first PCC subspace entry */
>>> pcct_entry = (struct acpi_subtable_header *) (
>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>
>>> - for (i = 0; i < count; i++) {
>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>> + pcc_mbox_ctrl.txdone_irq = true;
>>> +
>>
>> This flag is still bothering me. Can't you have a system where one
>> client requires polling on the command completion bit and another can
>> send a command completion IRQ? Seems to me that you need a channel
>> specific flag to indicate this. Besides, this global flag seems to be
>> used for async platform notifications. i.e. when the OS sets the
>> Generate doorbell irq bit in the Command field (Section 14.2.1 in ACPI
>> v6.1).
>
> I thought this bit is the global configuration for PCC.
> We still can support the polling mode if this flag is ON. Just don't
> set "Generate Doorbell Interrupt" bit inside Command Field.
>
>From the spec point of view it would seem so, but does the mbox
controller code allow for that? i.e. let individual PCC channels
decide whether to use txdone irq or poll? Seems like
pcc_mbox_ctrl.txdone_irq is a global setting for all mbox channels.
I apologize again, going forward my replies could be further delayed.
In process of relocating to a new job and role to a different city
which is several time zones away. :)
Thanks,
Ashwin.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-08 12:18 ` Ashwin Chaugule
@ 2016-06-08 16:24 ` Hoan Tran
2016-06-09 0:32 ` Prakash, Prashanth
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-06-08 16:24 UTC (permalink / raw)
To: Ashwin Chaugule
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prashanth Prakash
Hi Ashwin,
On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> + Prashanth (Can you please have a look as well?)
>
> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>> Hi Ashwin,
>
> Hi,
>
> Sorry about the delay. I'm in the middle of switching jobs and
> locations, so its been a bit crazy lately.
It's ok and hope you're doing well.
> I dont have any major
> concerns with this code, although there could be subtle issues with
> this IRQ thing. In this patchset, your intent is to add support for
> PCC subspace type 2. But you're also adding support for tx command
> completion which is not specific to Type 2. We could support that even
> in Type 1. Hence I wanted to separate the two, not just for review,
> but also the async IRQ completion has subtle issues esp. in the case
> of async platform notification, where you could have a PCC client in
> the OS writing to the cmd bit and the platform sending an async
> notification by writing to some bits in the same 8byte address as the
> cmd bit. So we need some mutual exclusivity there, otherwise the OS
> and platform could step on each other. Perhaps Prashanth has better
> insight into this.
I think, this mutual exclusivity could be in another patch.
>
>>
>> On Tue, May 31, 2016 at 12:05 PM, Ashwin Chaugule
>> <ashwin.chaugule@linaro.org> wrote:
>>> On 19 May 2016 at 20:32, Hoan Tran <hotran@apm.com> wrote:
>>>> ACPI 6.1 has a PCC HW-Reduced Communication Subspace type 2 intended for
>>>> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
>>>> to acknowledge doorbell interrupt. This patch provides the implementation
>>>> for the Communication Subspace Type 2.
>>>>
>>>> v3
>>>> * Remove 2 global structures
>>>> * Correct parsing subspace type 1 and subspace type 2
>>>>
>>>> v2
>>>> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
>>>> * Parse both subspace type 1 and subspace type 2
>>>> * Remove unnecessary variable initialization
>>>> * ISR returns IRQ_NONE in case of error
>>>>
>>>> v1
>>>> * Initial
>>>>
>>>> Signed-off-by: Hoan Tran <hotran@apm.com>
>>>> ---
>>>> drivers/mailbox/pcc.c | 316 ++++++++++++++++++++++++++++++++++++++------------
>>>> 1 file changed, 245 insertions(+), 71 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 043828d..c98bd94 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -59,6 +59,7 @@
>>>> #include <linux/delay.h>
>>>> #include <linux/io.h>
>>>> #include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> #include <linux/list.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/mailbox_controller.h>
>>>> @@ -68,11 +69,16 @@
>>>> #include "mailbox.h"
>>>>
>>>> #define MAX_PCC_SUBSPACES 256
>>>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>>>
>>>> static struct mbox_chan *pcc_mbox_channels;
>>>>
>>>> /* Array of cached virtual address for doorbell registers */
>>>> static void __iomem **pcc_doorbell_vaddr;
>>>> +/* Array of cached virtual address for doorbell ack registers */
>>>> +static void __iomem **pcc_doorbell_ack_vaddr;
>>>> +/* Array of doorbell interrupts */
>>>> +static int *pcc_doorbell_irq;
>>>>
>>>> static struct mbox_controller pcc_mbox_ctrl = {};
>>>> /**
>>>> @@ -91,6 +97,132 @@ static struct mbox_chan *get_pcc_channel(int id)
>>>> return &pcc_mbox_channels[id];
>>>> }
>>>>
>>>> +/*
>>>> + * PCC can be used with perf critical drivers such as CPPC
>>>> + * So it makes sense to locally cache the virtual address and
>>>> + * use it to read/write to PCC registers such as doorbell register
>>>> + *
>>>> + * The below read_register and write_registers are used to read and
>>>> + * write from perf critical registers such as PCC doorbell register
>>>> + */
>>>> +static int read_register(void __iomem *vaddr, u64 *val, unsigned int bit_width)
>>>> +{
>>>> + int ret_val = 0;
>>>> +
>>>> + switch (bit_width) {
>>>> + case 8:
>>>> + *val = readb(vaddr);
>>>> + break;
>>>> + case 16:
>>>> + *val = readw(vaddr);
>>>> + break;
>>>> + case 32:
>>>> + *val = readl(vaddr);
>>>> + break;
>>>> + case 64:
>>>> + *val = readq(vaddr);
>>>> + break;
>>>> + default:
>>>> + pr_debug("Error: Cannot read register of %u bit width",
>>>> + bit_width);
>>>> + ret_val = -EFAULT;
>>>> + break;
>>>> + }
>>>> + return ret_val;
>>>> +}
>>>> +
>>>> +static int write_register(void __iomem *vaddr, u64 val, unsigned int bit_width)
>>>> +{
>>>> + int ret_val = 0;
>>>> +
>>>> + switch (bit_width) {
>>>> + case 8:
>>>> + writeb(val, vaddr);
>>>> + break;
>>>> + case 16:
>>>> + writew(val, vaddr);
>>>> + break;
>>>> + case 32:
>>>> + writel(val, vaddr);
>>>> + break;
>>>> + case 64:
>>>> + writeq(val, vaddr);
>>>> + break;
>>>> + default:
>>>> + pr_debug("Error: Cannot write register of %u bit width",
>>>> + bit_width);
>>>> + ret_val = -EFAULT;
>>>> + break;
>>>> + }
>>>> + return ret_val;
>>>> +}
>>>> +
>>>
>>> Isn't this just reordering of function locations? I don't mean to be
>>> nitpicky, but your cosmetic changes in this file make it harder to
>>> review the meat of the patch.
>>
>> Yes, it is. As I would like to call these functions but actually, they
>> are declared quite late.
>> Do you have a any suggestion? Thanks
>>
>
> No this is fine. I missed the place where you needed to call these.
>
>>>
>>>> +/**
>>>> + * pcc_map_interrupt - Map a PCC subspace GSI to a linux IRQ number
>>>> + * @interrupt: GSI number.
>>>> + * @flags: interrupt flags
>>>> + *
>>>> + * Returns: a valid linux IRQ number on success
>>>> + * 0 or -EINVAL on failure
>>>> + */
>>>> +static int pcc_map_interrupt(u32 interrupt, u32 flags)
>>>> +{
>>>> + int trigger, polarity;
>>>> +
>>>> + if (!interrupt)
>>>> + return 0;
>>>> +
>>>> + trigger = (flags & ACPI_PCCT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
>>>> + : ACPI_LEVEL_SENSITIVE;
>>>> +
>>>> + polarity = (flags & ACPI_PCCT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
>>>> + : ACPI_ACTIVE_HIGH;
>>>> +
>>>> + return acpi_register_gsi(NULL, interrupt, trigger, polarity);
>>>> +}
>>>> +
>>>> +/**
>>>> + * pcc_mbox_irq - PCC mailbox interrupt handler
>>>> + */
>>>> +static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>> +{
>>>> + struct acpi_generic_address *doorbell_ack;
>>>> + struct acpi_pcct_hw_reduced *pcct_ss;
>>>> + struct mbox_chan *chan = p;
>>>> + u64 doorbell_ack_preserve;
>>>> + u64 doorbell_ack_write;
>>>> + u64 doorbell_ack_val;
>>>> + int ret;
>>>> +
>>>> + pcct_ss = chan->con_priv;
>>>> +
>>>> + mbox_chan_received_data(chan, NULL);
>>>
>>> Does this really do anything? IIUC, your mbox controller rings
>>> doorbell, platform sets cmd_completion and sends an IRQ to the OS.
>>> This is the function where you field it to ACK the doorbell. But
>>> you're not really consuming any new data sent by the platform, right?
>>> i.e. this IRQ use case is not the same as the async notification from
>>> platform as described in the ACPIv5+ spec. Or did I misunderstand the
>>> bigger picture?
>>>
>>
>> The purpose of this function call is
>> 1./ Notify the completion of a command to OSPM.
>> For example, instead of using polling with udelay, CPPC could use this
>> call to complete a "completion".
>> I'm preparing to post a version to support it for CPPC.
>>
>
> I see. This is the part I was missing. Seems like you could call the
> mbox_chan_received_data() only after you add support for this.
>
>> 2./ Notify OSPM about a notification message sent by Platform
>
> This will require adding some sort of rx_callback to field the notification.
>
>>>
>>> [...]
>>>
>>>> +
>>>> +/**
>>>> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>>>> *
>>>> * Return: 0 for Success, else errno.
>>>> @@ -316,7 +449,9 @@ static int __init acpi_pcc_probe(void)
>>>> acpi_size pcct_tbl_header_size;
>>>> struct acpi_table_header *pcct_tbl;
>>>> struct acpi_subtable_header *pcct_entry;
>>>> - int count, i;
>>>> + struct acpi_table_pcct *acpi_pcct_tbl;
>>>> + int count, i, rc;
>>>> + int sum = 0;
>>>> acpi_status status = AE_OK;
>>>>
>>>> /* Search for PCCT */
>>>> @@ -333,37 +468,66 @@ static int __init acpi_pcc_probe(void)
>>>> sizeof(struct acpi_table_pcct),
>>>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>>>> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>>>> + sum += (count > 0)? count: 0;
>>>> +
>>>> + count = acpi_table_parse_entries(ACPI_SIG_PCCT,
>>>> + sizeof(struct acpi_table_pcct),
>>>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
>>>> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
>>>> + sum += (count > 0)? count: 0;
>>>>
>>>> - if (count <= 0) {
>>>> + if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
>>>> pr_err("Error parsing PCC subspaces from PCCT\n");
>>>> return -EINVAL;
>>>> }
>>>>
>>>> pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
>>>> - count, GFP_KERNEL);
>>>> -
>>>> + sum, GFP_KERNEL);
>>>> if (!pcc_mbox_channels) {
>>>> pr_err("Could not allocate space for PCC mbox channels\n");
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
>>>> + pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>>>> if (!pcc_doorbell_vaddr) {
>>>> - kfree(pcc_mbox_channels);
>>>> - return -ENOMEM;
>>>> + rc = -ENOMEM;
>>>> + goto err_free_mbox;
>>>> + }
>>>> +
>>>> + pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
>>>> + if (!pcc_doorbell_ack_vaddr) {
>>>> + rc = -ENOMEM;
>>>> + goto err_free_db_vaddr;
>>>> + }
>>>> +
>>>> + pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
>>>> + if (!pcc_doorbell_irq) {
>>>> + rc = -ENOMEM;
>>>> + goto err_free_db_ack_vaddr;
>>>> }
>>>>
>>>> /* Point to the first PCC subspace entry */
>>>> pcct_entry = (struct acpi_subtable_header *) (
>>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>>
>>>> - for (i = 0; i < count; i++) {
>>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>>> + pcc_mbox_ctrl.txdone_irq = true;
>>>> +
>>>
>>> This flag is still bothering me. Can't you have a system where one
>>> client requires polling on the command completion bit and another can
>>> send a command completion IRQ? Seems to me that you need a channel
>>> specific flag to indicate this. Besides, this global flag seems to be
>>> used for async platform notifications. i.e. when the OS sets the
>>> Generate doorbell irq bit in the Command field (Section 14.2.1 in ACPI
>>> v6.1).
>>
>> I thought this bit is the global configuration for PCC.
>> We still can support the polling mode if this flag is ON. Just don't
>> set "Generate Doorbell Interrupt" bit inside Command Field.
>>
>
> From the spec point of view it would seem so, but does the mbox
> controller code allow for that? i.e. let individual PCC channels
> decide whether to use txdone irq or poll? Seems like
> pcc_mbox_ctrl.txdone_irq is a global setting for all mbox channels.
I think for PCC, it's depend on the PCC client not mbox controller.
Client can decide to send a command in polling or interrupt mode by
configuring "Generate Doorbell Interrupt" bit.
For the txdone_irq flag, it is a global setting for all mbox channels.
If txdone_irq is enabled, the client has to use mbox_chan_txdone()
function to notify the framework that the last command has completed.
Thanks and Regards
Hoan
>
> I apologize again, going forward my replies could be further delayed.
> In process of relocating to a new job and role to a different city
> which is several time zones away. :)
>
> Thanks,
> Ashwin.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-08 16:24 ` Hoan Tran
@ 2016-06-09 0:32 ` Prakash, Prashanth
2016-06-09 0:41 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Prakash, Prashanth @ 2016-06-09 0:32 UTC (permalink / raw)
To: Hoan Tran, Ashwin Chaugule
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang
On 6/8/2016 10:24 AM, Hoan Tran wrote:
> Hi Ashwin,
>
> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
>> + Prashanth (Can you please have a look as well?)
>>
>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>> Hi Ashwin,
>> Hi,
>>
>> Sorry about the delay. I'm in the middle of switching jobs and
>> locations, so its been a bit crazy lately.
> It's ok and hope you're doing well.
>
>> I dont have any major
>> concerns with this code, although there could be subtle issues with
>> this IRQ thing. In this patchset, your intent is to add support for
>> PCC subspace type 2. But you're also adding support for tx command
>> completion which is not specific to Type 2. We could support that even
>> in Type 1. Hence I wanted to separate the two, not just for review,
>> but also the async IRQ completion has subtle issues esp. in the case
>> of async platform notification, where you could have a PCC client in
>> the OS writing to the cmd bit and the platform sending an async
>> notification by writing to some bits in the same 8byte address as the
>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>> and platform could step on each other. Perhaps Prashanth has better
>> insight into this.
> I think, this mutual exclusivity could be in another patch.
Ashwin,
Sorry, I am not sure how we can prevent platform and OSPM from stepping on
each other. There is a line is spec that says "all operations on status field
must be made using interlocked operations", but not sure what these
interlocked operation translates to.
Hoan,
Even if we are not using platform notification, we still need to clear the doorbell
interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4). I didn't see
clearing the doorbell interrupt bit in this patch (and platform is supposed to set
it again when it is sending the interrupt again). Did I miss it? or is it intentionally
left out to avoid the race that Ashwin mentioned above?
Thanks,
Prashanth
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-09 0:32 ` Prakash, Prashanth
@ 2016-06-09 0:41 ` Hoan Tran
2016-06-09 20:47 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-06-09 0:41 UTC (permalink / raw)
To: Prakash, Prashanth
Cc: Ashwin Chaugule, Jassi Brar, Rafael J. Wysocki, Robert Moore,
Alexey Klimov, lkml, linux acpi, Loc Ho, Duc Dang
Hi Prashanth,
On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
<pprakash@codeaurora.org> wrote:
>
>
> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>> Hi Ashwin,
>>
>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>> <ashwin.chaugule@linaro.org> wrote:
>>> + Prashanth (Can you please have a look as well?)
>>>
>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>> Hi Ashwin,
>>> Hi,
>>>
>>> Sorry about the delay. I'm in the middle of switching jobs and
>>> locations, so its been a bit crazy lately.
>> It's ok and hope you're doing well.
>>
>>> I dont have any major
>>> concerns with this code, although there could be subtle issues with
>>> this IRQ thing. In this patchset, your intent is to add support for
>>> PCC subspace type 2. But you're also adding support for tx command
>>> completion which is not specific to Type 2. We could support that even
>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>> but also the async IRQ completion has subtle issues esp. in the case
>>> of async platform notification, where you could have a PCC client in
>>> the OS writing to the cmd bit and the platform sending an async
>>> notification by writing to some bits in the same 8byte address as the
>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>> and platform could step on each other. Perhaps Prashanth has better
>>> insight into this.
>> I think, this mutual exclusivity could be in another patch.
> Ashwin,
> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
> each other. There is a line is spec that says "all operations on status field
> must be made using interlocked operations", but not sure what these
> interlocked operation translates to.
Yes, I had the same question about how to prevent it.
>
> Hoan,
> Even if we are not using platform notification, we still need to clear the doorbell
> interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4). I didn't see
> clearing the doorbell interrupt bit in this patch (and platform is supposed to set
> it again when it is sending the interrupt again). Did I miss it? or is it intentionally
> left out to avoid the race that Ashwin mentioned above?
>
The PCC client driver is supposed to do that. Which mean, the
mbox_chan_received_data() function should clear it.
Thanks
Hoan
>
> Thanks,
> Prashanth
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-09 0:41 ` Hoan Tran
@ 2016-06-09 20:47 ` Hoan Tran
2016-06-09 22:25 ` Prakash, Prashanth
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-06-09 20:47 UTC (permalink / raw)
To: Prakash, Prashanth
Cc: Ashwin Chaugule, Jassi Brar, Rafael J. Wysocki, Robert Moore,
Alexey Klimov, lkml, linux acpi, Loc Ho, Duc Dang
Hi Ashwin and Prashanth,
On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
> Hi Prashanth,
>
>
> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
> <pprakash@codeaurora.org> wrote:
>>
>>
>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>> Hi Ashwin,
>>>
>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>> <ashwin.chaugule@linaro.org> wrote:
>>>> + Prashanth (Can you please have a look as well?)
>>>>
>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>> Hi Ashwin,
>>>> Hi,
>>>>
>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>> locations, so its been a bit crazy lately.
>>> It's ok and hope you're doing well.
>>>
>>>> I dont have any major
>>>> concerns with this code, although there could be subtle issues with
>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>> PCC subspace type 2. But you're also adding support for tx command
>>>> completion which is not specific to Type 2. We could support that even
>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>> of async platform notification, where you could have a PCC client in
>>>> the OS writing to the cmd bit and the platform sending an async
>>>> notification by writing to some bits in the same 8byte address as the
>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>> and platform could step on each other. Perhaps Prashanth has better
>>>> insight into this.
>>> I think, this mutual exclusivity could be in another patch.
>> Ashwin,
>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>> each other. There is a line is spec that says "all operations on status field
>> must be made using interlocked operations", but not sure what these
>> interlocked operation translates to.
>
> Yes, I had the same question about how to prevent it.
For platform notification, if the hardware doesn't support interlocked
operations. I think we can use a workaround that, platform triggers
interrupt to OSPM without touching status field. The OSPM PCC client
will decide what to do with this interrupt. For example, OSPM sends a
consumer command to check it.
Thanks
Hoan
>
>>
>> Hoan,
>> Even if we are not using platform notification, we still need to clear the doorbell
>> interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4). I didn't see
>> clearing the doorbell interrupt bit in this patch (and platform is supposed to set
>> it again when it is sending the interrupt again). Did I miss it? or is it intentionally
>> left out to avoid the race that Ashwin mentioned above?
>>
>
> The PCC client driver is supposed to do that. Which mean, the
> mbox_chan_received_data() function should clear it.
>
> Thanks
> Hoan
>
>>
>> Thanks,
>> Prashanth
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-09 20:47 ` Hoan Tran
@ 2016-06-09 22:25 ` Prakash, Prashanth
2016-06-09 22:43 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Prakash, Prashanth @ 2016-06-09 22:25 UTC (permalink / raw)
To: Hoan Tran
Cc: Ashwin Chaugule, Jassi Brar, Rafael J. Wysocki, Robert Moore,
Alexey Klimov, lkml, linux acpi, Loc Ho, Duc Dang
On 6/9/2016 2:47 PM, Hoan Tran wrote:
> Hi Ashwin and Prashanth,
>
> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>> Hi Prashanth,
>>
>>
>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>> <pprakash@codeaurora.org> wrote:
>>>
>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>>> Hi Ashwin,
>>>>
>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>>> <ashwin.chaugule@linaro.org> wrote:
>>>>> + Prashanth (Can you please have a look as well?)
>>>>>
>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>>> Hi Ashwin,
>>>>> Hi,
>>>>>
>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>>> locations, so its been a bit crazy lately.
>>>> It's ok and hope you're doing well.
>>>>
>>>>> I dont have any major
>>>>> concerns with this code, although there could be subtle issues with
>>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>>> PCC subspace type 2. But you're also adding support for tx command
>>>>> completion which is not specific to Type 2. We could support that even
>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>>> of async platform notification, where you could have a PCC client in
>>>>> the OS writing to the cmd bit and the platform sending an async
>>>>> notification by writing to some bits in the same 8byte address as the
>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>>> and platform could step on each other. Perhaps Prashanth has better
>>>>> insight into this.
>>>> I think, this mutual exclusivity could be in another patch.
>>> Ashwin,
>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>>> each other. There is a line is spec that says "all operations on status field
>>> must be made using interlocked operations", but not sure what these
>>> interlocked operation translates to.
>> Yes, I had the same question about how to prevent it.
> For platform notification, if the hardware doesn't support interlocked
> operations. I think we can use a workaround that, platform triggers
> interrupt to OSPM without touching status field. The OSPM PCC client
> will decide what to do with this interrupt. For example, OSPM sends a
> consumer command to check it.
How do we decide which platform can support this interlocked operation?
and how do we decide between a completion notification and platform
notification?
I think the ACPI spec on platform notification is quite ambiguous and it is
best to get the necessary clarification and/or correction before implementing
anything related to platform notification.
With respect to to this patch, since we are not doing anything specific to
platform notification and the interrupt can be used only for notification
of completion, I suppose we should be okay.
Thanks,
Prashanth
> Thanks
> Hoan
>
>>> Hoan,
>>> Even if we are not using platform notification, we still need to clear the doorbell
>>> interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4). I didn't see
>>> clearing the doorbell interrupt bit in this patch (and platform is supposed to set
>>> it again when it is sending the interrupt again). Did I miss it? or is it intentionally
>>> left out to avoid the race that Ashwin mentioned above?
>>>
>> The PCC client driver is supposed to do that. Which mean, the
>> mbox_chan_received_data() function should clear it.
>>
>> Thanks
>> Hoan
>>
>>> Thanks,
>>> Prashanth
>>>
>>>
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-09 22:25 ` Prakash, Prashanth
@ 2016-06-09 22:43 ` Hoan Tran
2016-06-15 16:19 ` Prakash, Prashanth
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-06-09 22:43 UTC (permalink / raw)
To: Prakash, Prashanth
Cc: Ashwin Chaugule, Jassi Brar, Rafael J. Wysocki, Robert Moore,
Alexey Klimov, lkml, linux acpi, Loc Ho, Duc Dang
Hi Prashanth,
On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
<pprakash@codeaurora.org> wrote:
>
>
> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>> Hi Ashwin and Prashanth,
>>
>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>>> Hi Prashanth,
>>>
>>>
>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>>> <pprakash@codeaurora.org> wrote:
>>>>
>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>>>> Hi Ashwin,
>>>>>
>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>>>> <ashwin.chaugule@linaro.org> wrote:
>>>>>> + Prashanth (Can you please have a look as well?)
>>>>>>
>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>>>> Hi Ashwin,
>>>>>> Hi,
>>>>>>
>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>>>> locations, so its been a bit crazy lately.
>>>>> It's ok and hope you're doing well.
>>>>>
>>>>>> I dont have any major
>>>>>> concerns with this code, although there could be subtle issues with
>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>>>> PCC subspace type 2. But you're also adding support for tx command
>>>>>> completion which is not specific to Type 2. We could support that even
>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>>>> of async platform notification, where you could have a PCC client in
>>>>>> the OS writing to the cmd bit and the platform sending an async
>>>>>> notification by writing to some bits in the same 8byte address as the
>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>>>> and platform could step on each other. Perhaps Prashanth has better
>>>>>> insight into this.
>>>>> I think, this mutual exclusivity could be in another patch.
>>>> Ashwin,
>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>>>> each other. There is a line is spec that says "all operations on status field
>>>> must be made using interlocked operations", but not sure what these
>>>> interlocked operation translates to.
>>> Yes, I had the same question about how to prevent it.
>> For platform notification, if the hardware doesn't support interlocked
>> operations. I think we can use a workaround that, platform triggers
>> interrupt to OSPM without touching status field. The OSPM PCC client
>> will decide what to do with this interrupt. For example, OSPM sends a
>> consumer command to check it.
> How do we decide which platform can support this interlocked operation?
> and how do we decide between a completion notification and platform
> notification?
Truly, we should follow the specification. But I don't know if there's
any hardware support this interlocked operation.
For the decide between a completion notification and platform notification
- Completion notification: Bit "Command Complete" is set.
- Platform notification: Bit "Command Complete" is not set.
>
> I think the ACPI spec on platform notification is quite ambiguous and it is
> best to get the necessary clarification and/or correction before implementing
> anything related to platform notification.
Agreed, a clarification inside ACPI Specification is needed
Thanks
Hoan
>
> With respect to to this patch, since we are not doing anything specific to
> platform notification and the interrupt can be used only for notification
> of completion, I suppose we should be okay.
>
> Thanks,
> Prashanth
>> Thanks
>> Hoan
>>
>>>> Hoan,
>>>> Even if we are not using platform notification, we still need to clear the doorbell
>>>> interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4). I didn't see
>>>> clearing the doorbell interrupt bit in this patch (and platform is supposed to set
>>>> it again when it is sending the interrupt again). Did I miss it? or is it intentionally
>>>> left out to avoid the race that Ashwin mentioned above?
>>>>
>>> The PCC client driver is supposed to do that. Which mean, the
>>> mbox_chan_received_data() function should clear it.
>>>
>>> Thanks
>>> Hoan
>>>
>>>> Thanks,
>>>> Prashanth
>>>>
>>>>
>> --
>> 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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-09 22:43 ` Hoan Tran
@ 2016-06-15 16:19 ` Prakash, Prashanth
2016-06-27 18:27 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Prakash, Prashanth @ 2016-06-15 16:19 UTC (permalink / raw)
To: Hoan Tran
Cc: Ashwin Chaugule, Jassi Brar, Rafael J. Wysocki, Robert Moore,
Alexey Klimov, lkml, linux acpi, Loc Ho, Duc Dang
On 6/9/2016 4:43 PM, Hoan Tran wrote:
> Hi Prashanth,
>
> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
> <pprakash@codeaurora.org> wrote:
>>
>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>>> Hi Ashwin and Prashanth,
>>>
>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>>>> Hi Prashanth,
>>>>
>>>>
>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>>>> <pprakash@codeaurora.org> wrote:
>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>>>>> Hi Ashwin,
>>>>>>
>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>>>>> <ashwin.chaugule@linaro.org> wrote:
>>>>>>> + Prashanth (Can you please have a look as well?)
>>>>>>>
>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>>>>> Hi Ashwin,
>>>>>>> Hi,
>>>>>>>
>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>>>>> locations, so its been a bit crazy lately.
>>>>>> It's ok and hope you're doing well.
>>>>>>
>>>>>>> I dont have any major
>>>>>>> concerns with this code, although there could be subtle issues with
>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>>>>> PCC subspace type 2. But you're also adding support for tx command
>>>>>>> completion which is not specific to Type 2. We could support that even
>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>>>>> of async platform notification, where you could have a PCC client in
>>>>>>> the OS writing to the cmd bit and the platform sending an async
>>>>>>> notification by writing to some bits in the same 8byte address as the
>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>>>>> and platform could step on each other. Perhaps Prashanth has better
>>>>>>> insight into this.
>>>>>> I think, this mutual exclusivity could be in another patch.
>>>>> Ashwin,
>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>>>>> each other. There is a line is spec that says "all operations on status field
>>>>> must be made using interlocked operations", but not sure what these
>>>>> interlocked operation translates to.
>>>> Yes, I had the same question about how to prevent it.
>>> For platform notification, if the hardware doesn't support interlocked
>>> operations. I think we can use a workaround that, platform triggers
>>> interrupt to OSPM without touching status field. The OSPM PCC client
>>> will decide what to do with this interrupt. For example, OSPM sends a
>>> consumer command to check it.
>> How do we decide which platform can support this interlocked operation?
>> and how do we decide between a completion notification and platform
>> notification?
> Truly, we should follow the specification. But I don't know if there's
> any hardware support this interlocked operation.
> For the decide between a completion notification and platform notification
> - Completion notification: Bit "Command Complete" is set.
> - Platform notification: Bit "Command Complete" is not set.
>
>> I think the ACPI spec on platform notification is quite ambiguous and it is
>> best to get the necessary clarification and/or correction before implementing
>> anything related to platform notification.
> Agreed, a clarification inside ACPI Specification is needed
This patch look good to me, as it doesn't deal with platform notification.
We can try to get some clarification from spec side before handling the platform
notification pieces.
Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
Thanks,
Prashanth
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-15 16:19 ` Prakash, Prashanth
@ 2016-06-27 18:27 ` Hoan Tran
2016-06-27 21:32 ` Rafael J. Wysocki
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-06-27 18:27 UTC (permalink / raw)
To: Jassi Brar, Rafael J. Wysocki
Cc: Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml, linux acpi,
Loc Ho, Duc Dang, Prakash, Prashanth
Hi Jassi and Rafael,
On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
<pprakash@codeaurora.org> wrote:
>
>
> On 6/9/2016 4:43 PM, Hoan Tran wrote:
>> Hi Prashanth,
>>
>> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
>> <pprakash@codeaurora.org> wrote:
>>>
>>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>>>> Hi Ashwin and Prashanth,
>>>>
>>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>>>>> Hi Prashanth,
>>>>>
>>>>>
>>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>>>>> <pprakash@codeaurora.org> wrote:
>>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>>>>>> Hi Ashwin,
>>>>>>>
>>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>>>>>> <ashwin.chaugule@linaro.org> wrote:
>>>>>>>> + Prashanth (Can you please have a look as well?)
>>>>>>>>
>>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>>>>>> Hi Ashwin,
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>>>>>> locations, so its been a bit crazy lately.
>>>>>>> It's ok and hope you're doing well.
>>>>>>>
>>>>>>>> I dont have any major
>>>>>>>> concerns with this code, although there could be subtle issues with
>>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>>>>>> PCC subspace type 2. But you're also adding support for tx command
>>>>>>>> completion which is not specific to Type 2. We could support that even
>>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>>>>>> of async platform notification, where you could have a PCC client in
>>>>>>>> the OS writing to the cmd bit and the platform sending an async
>>>>>>>> notification by writing to some bits in the same 8byte address as the
>>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>>>>>> and platform could step on each other. Perhaps Prashanth has better
>>>>>>>> insight into this.
>>>>>>> I think, this mutual exclusivity could be in another patch.
>>>>>> Ashwin,
>>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>>>>>> each other. There is a line is spec that says "all operations on status field
>>>>>> must be made using interlocked operations", but not sure what these
>>>>>> interlocked operation translates to.
>>>>> Yes, I had the same question about how to prevent it.
>>>> For platform notification, if the hardware doesn't support interlocked
>>>> operations. I think we can use a workaround that, platform triggers
>>>> interrupt to OSPM without touching status field. The OSPM PCC client
>>>> will decide what to do with this interrupt. For example, OSPM sends a
>>>> consumer command to check it.
>>> How do we decide which platform can support this interlocked operation?
>>> and how do we decide between a completion notification and platform
>>> notification?
>> Truly, we should follow the specification. But I don't know if there's
>> any hardware support this interlocked operation.
>> For the decide between a completion notification and platform notification
>> - Completion notification: Bit "Command Complete" is set.
>> - Platform notification: Bit "Command Complete" is not set.
>>
>>> I think the ACPI spec on platform notification is quite ambiguous and it is
>>> best to get the necessary clarification and/or correction before implementing
>>> anything related to platform notification.
>> Agreed, a clarification inside ACPI Specification is needed
> This patch look good to me, as it doesn't deal with platform notification.
> We can try to get some clarification from spec side before handling the platform
> notification pieces.
>
> Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
Do you have plan to apply this patch ?
Thanks
Hoan
>
> Thanks,
> Prashanth
> --
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-27 18:27 ` Hoan Tran
@ 2016-06-27 21:32 ` Rafael J. Wysocki
2016-07-13 22:00 ` Hoan Tran
2016-08-15 16:45 ` Hoan Tran
0 siblings, 2 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2016-06-27 21:32 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
> Hi Jassi and Rafael,
>
> On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
> <pprakash@codeaurora.org> wrote:
> >
> >
> > On 6/9/2016 4:43 PM, Hoan Tran wrote:
> >> Hi Prashanth,
> >>
> >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
> >> <pprakash@codeaurora.org> wrote:
> >>>
> >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
> >>>> Hi Ashwin and Prashanth,
> >>>>
> >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
> >>>>> Hi Prashanth,
> >>>>>
> >>>>>
> >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
> >>>>> <pprakash@codeaurora.org> wrote:
> >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
> >>>>>>> Hi Ashwin,
> >>>>>>>
> >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
> >>>>>>> <ashwin.chaugule@linaro.org> wrote:
> >>>>>>>> + Prashanth (Can you please have a look as well?)
> >>>>>>>>
> >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
> >>>>>>>>> Hi Ashwin,
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
> >>>>>>>> locations, so its been a bit crazy lately.
> >>>>>>> It's ok and hope you're doing well.
> >>>>>>>
> >>>>>>>> I dont have any major
> >>>>>>>> concerns with this code, although there could be subtle issues with
> >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
> >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
> >>>>>>>> completion which is not specific to Type 2. We could support that even
> >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
> >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
> >>>>>>>> of async platform notification, where you could have a PCC client in
> >>>>>>>> the OS writing to the cmd bit and the platform sending an async
> >>>>>>>> notification by writing to some bits in the same 8byte address as the
> >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
> >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
> >>>>>>>> insight into this.
> >>>>>>> I think, this mutual exclusivity could be in another patch.
> >>>>>> Ashwin,
> >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
> >>>>>> each other. There is a line is spec that says "all operations on status field
> >>>>>> must be made using interlocked operations", but not sure what these
> >>>>>> interlocked operation translates to.
> >>>>> Yes, I had the same question about how to prevent it.
> >>>> For platform notification, if the hardware doesn't support interlocked
> >>>> operations. I think we can use a workaround that, platform triggers
> >>>> interrupt to OSPM without touching status field. The OSPM PCC client
> >>>> will decide what to do with this interrupt. For example, OSPM sends a
> >>>> consumer command to check it.
> >>> How do we decide which platform can support this interlocked operation?
> >>> and how do we decide between a completion notification and platform
> >>> notification?
> >> Truly, we should follow the specification. But I don't know if there's
> >> any hardware support this interlocked operation.
> >> For the decide between a completion notification and platform notification
> >> - Completion notification: Bit "Command Complete" is set.
> >> - Platform notification: Bit "Command Complete" is not set.
> >>
> >>> I think the ACPI spec on platform notification is quite ambiguous and it is
> >>> best to get the necessary clarification and/or correction before implementing
> >>> anything related to platform notification.
> >> Agreed, a clarification inside ACPI Specification is needed
> > This patch look good to me, as it doesn't deal with platform notification.
> > We can try to get some clarification from spec side before handling the platform
> > notification pieces.
> >
> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
>
> Do you have plan to apply this patch ?
Yes.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-27 21:32 ` Rafael J. Wysocki
@ 2016-07-13 22:00 ` Hoan Tran
2016-08-15 16:45 ` Hoan Tran
1 sibling, 0 replies; 20+ messages in thread
From: Hoan Tran @ 2016-07-13 22:00 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
Hi Rafael,
On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
>> Hi Jassi and Rafael,
>>
>> On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
>> <pprakash@codeaurora.org> wrote:
>> >
>> >
>> > On 6/9/2016 4:43 PM, Hoan Tran wrote:
>> >> Hi Prashanth,
>> >>
>> >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
>> >> <pprakash@codeaurora.org> wrote:
>> >>>
>> >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>> >>>> Hi Ashwin and Prashanth,
>> >>>>
>> >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>> >>>>> Hi Prashanth,
>> >>>>>
>> >>>>>
>> >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>> >>>>> <pprakash@codeaurora.org> wrote:
>> >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>> >>>>>>> Hi Ashwin,
>> >>>>>>>
>> >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>> >>>>>>> <ashwin.chaugule@linaro.org> wrote:
>> >>>>>>>> + Prashanth (Can you please have a look as well?)
>> >>>>>>>>
>> >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>> >>>>>>>>> Hi Ashwin,
>> >>>>>>>> Hi,
>> >>>>>>>>
>> >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>> >>>>>>>> locations, so its been a bit crazy lately.
>> >>>>>>> It's ok and hope you're doing well.
>> >>>>>>>
>> >>>>>>>> I dont have any major
>> >>>>>>>> concerns with this code, although there could be subtle issues with
>> >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>> >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
>> >>>>>>>> completion which is not specific to Type 2. We could support that even
>> >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>> >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>> >>>>>>>> of async platform notification, where you could have a PCC client in
>> >>>>>>>> the OS writing to the cmd bit and the platform sending an async
>> >>>>>>>> notification by writing to some bits in the same 8byte address as the
>> >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>> >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
>> >>>>>>>> insight into this.
>> >>>>>>> I think, this mutual exclusivity could be in another patch.
>> >>>>>> Ashwin,
>> >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>> >>>>>> each other. There is a line is spec that says "all operations on status field
>> >>>>>> must be made using interlocked operations", but not sure what these
>> >>>>>> interlocked operation translates to.
>> >>>>> Yes, I had the same question about how to prevent it.
>> >>>> For platform notification, if the hardware doesn't support interlocked
>> >>>> operations. I think we can use a workaround that, platform triggers
>> >>>> interrupt to OSPM without touching status field. The OSPM PCC client
>> >>>> will decide what to do with this interrupt. For example, OSPM sends a
>> >>>> consumer command to check it.
>> >>> How do we decide which platform can support this interlocked operation?
>> >>> and how do we decide between a completion notification and platform
>> >>> notification?
>> >> Truly, we should follow the specification. But I don't know if there's
>> >> any hardware support this interlocked operation.
>> >> For the decide between a completion notification and platform notification
>> >> - Completion notification: Bit "Command Complete" is set.
>> >> - Platform notification: Bit "Command Complete" is not set.
>> >>
>> >>> I think the ACPI spec on platform notification is quite ambiguous and it is
>> >>> best to get the necessary clarification and/or correction before implementing
>> >>> anything related to platform notification.
>> >> Agreed, a clarification inside ACPI Specification is needed
>> > This patch look good to me, as it doesn't deal with platform notification.
>> > We can try to get some clarification from spec side before handling the platform
>> > notification pieces.
>> >
>> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
>>
>> Do you have plan to apply this patch ?
>
> Yes.
Thanks and hope it'll be in 4.8.
Thanks
Hoan
>
> Thanks,
> Rafael
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-06-27 21:32 ` Rafael J. Wysocki
2016-07-13 22:00 ` Hoan Tran
@ 2016-08-15 16:45 ` Hoan Tran
2016-08-15 23:18 ` Rafael J. Wysocki
1 sibling, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-08-15 16:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
> > Hi Jassi and Rafael,
> >
> > On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
> > <pprakash@codeaurora.org> wrote:
> > >
> > >
> > > On 6/9/2016 4:43 PM, Hoan Tran wrote:
> > >> Hi Prashanth,
> > >>
> > >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
> > >> <pprakash@codeaurora.org> wrote:
> > >>>
> > >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
> > >>>> Hi Ashwin and Prashanth,
> > >>>>
> > >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
> > >>>>> Hi Prashanth,
> > >>>>>
> > >>>>>
> > >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
> > >>>>> <pprakash@codeaurora.org> wrote:
> > >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
> > >>>>>>> Hi Ashwin,
> > >>>>>>>
> > >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
> > >>>>>>> <ashwin.chaugule@linaro.org> wrote:
> > >>>>>>>> + Prashanth (Can you please have a look as well?)
> > >>>>>>>>
> > >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
> > >>>>>>>>> Hi Ashwin,
> > >>>>>>>> Hi,
> > >>>>>>>>
> > >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
> > >>>>>>>> locations, so its been a bit crazy lately.
> > >>>>>>> It's ok and hope you're doing well.
> > >>>>>>>
> > >>>>>>>> I dont have any major
> > >>>>>>>> concerns with this code, although there could be subtle issues with
> > >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
> > >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
> > >>>>>>>> completion which is not specific to Type 2. We could support that even
> > >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
> > >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
> > >>>>>>>> of async platform notification, where you could have a PCC client in
> > >>>>>>>> the OS writing to the cmd bit and the platform sending an async
> > >>>>>>>> notification by writing to some bits in the same 8byte address as the
> > >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
> > >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
> > >>>>>>>> insight into this.
> > >>>>>>> I think, this mutual exclusivity could be in another patch.
> > >>>>>> Ashwin,
> > >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
> > >>>>>> each other. There is a line is spec that says "all operations on status field
> > >>>>>> must be made using interlocked operations", but not sure what these
> > >>>>>> interlocked operation translates to.
> > >>>>> Yes, I had the same question about how to prevent it.
> > >>>> For platform notification, if the hardware doesn't support interlocked
> > >>>> operations. I think we can use a workaround that, platform triggers
> > >>>> interrupt to OSPM without touching status field. The OSPM PCC client
> > >>>> will decide what to do with this interrupt. For example, OSPM sends a
> > >>>> consumer command to check it.
> > >>> How do we decide which platform can support this interlocked operation?
> > >>> and how do we decide between a completion notification and platform
> > >>> notification?
> > >> Truly, we should follow the specification. But I don't know if there's
> > >> any hardware support this interlocked operation.
> > >> For the decide between a completion notification and platform notification
> > >> - Completion notification: Bit "Command Complete" is set.
> > >> - Platform notification: Bit "Command Complete" is not set.
> > >>
> > >>> I think the ACPI spec on platform notification is quite ambiguous and it is
> > >>> best to get the necessary clarification and/or correction before implementing
> > >>> anything related to platform notification.
> > >> Agreed, a clarification inside ACPI Specification is needed
> > > This patch look good to me, as it doesn't deal with platform notification.
> > > We can try to get some clarification from spec side before handling the platform
> > > notification pieces.
> > >
> > > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
> >
> > Do you have plan to apply this patch ?
>
> Yes.
>
> Thanks,
> Rafael
>
Hi Rafael,
This patch had an ACK from Prashanth. Can you consider to merge
this patch please?
Thanks
Hoan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-08-15 16:45 ` Hoan Tran
@ 2016-08-15 23:18 ` Rafael J. Wysocki
2016-08-15 23:41 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Rafael J. Wysocki @ 2016-08-15 23:18 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
On Monday, August 15, 2016 09:45:24 AM Hoan Tran wrote:
> On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
> > > Hi Jassi and Rafael,
> > >
> > > On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
> > > <pprakash@codeaurora.org> wrote:
> > > >
> > > >
> > > > On 6/9/2016 4:43 PM, Hoan Tran wrote:
> > > >> Hi Prashanth,
> > > >>
> > > >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
> > > >> <pprakash@codeaurora.org> wrote:
> > > >>>
> > > >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
> > > >>>> Hi Ashwin and Prashanth,
> > > >>>>
> > > >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
> > > >>>>> Hi Prashanth,
> > > >>>>>
> > > >>>>>
> > > >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
> > > >>>>> <pprakash@codeaurora.org> wrote:
> > > >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
> > > >>>>>>> Hi Ashwin,
> > > >>>>>>>
> > > >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
> > > >>>>>>> <ashwin.chaugule@linaro.org> wrote:
> > > >>>>>>>> + Prashanth (Can you please have a look as well?)
> > > >>>>>>>>
> > > >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
> > > >>>>>>>>> Hi Ashwin,
> > > >>>>>>>> Hi,
> > > >>>>>>>>
> > > >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
> > > >>>>>>>> locations, so its been a bit crazy lately.
> > > >>>>>>> It's ok and hope you're doing well.
> > > >>>>>>>
> > > >>>>>>>> I dont have any major
> > > >>>>>>>> concerns with this code, although there could be subtle issues with
> > > >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
> > > >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
> > > >>>>>>>> completion which is not specific to Type 2. We could support that even
> > > >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
> > > >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
> > > >>>>>>>> of async platform notification, where you could have a PCC client in
> > > >>>>>>>> the OS writing to the cmd bit and the platform sending an async
> > > >>>>>>>> notification by writing to some bits in the same 8byte address as the
> > > >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
> > > >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
> > > >>>>>>>> insight into this.
> > > >>>>>>> I think, this mutual exclusivity could be in another patch.
> > > >>>>>> Ashwin,
> > > >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
> > > >>>>>> each other. There is a line is spec that says "all operations on status field
> > > >>>>>> must be made using interlocked operations", but not sure what these
> > > >>>>>> interlocked operation translates to.
> > > >>>>> Yes, I had the same question about how to prevent it.
> > > >>>> For platform notification, if the hardware doesn't support interlocked
> > > >>>> operations. I think we can use a workaround that, platform triggers
> > > >>>> interrupt to OSPM without touching status field. The OSPM PCC client
> > > >>>> will decide what to do with this interrupt. For example, OSPM sends a
> > > >>>> consumer command to check it.
> > > >>> How do we decide which platform can support this interlocked operation?
> > > >>> and how do we decide between a completion notification and platform
> > > >>> notification?
> > > >> Truly, we should follow the specification. But I don't know if there's
> > > >> any hardware support this interlocked operation.
> > > >> For the decide between a completion notification and platform notification
> > > >> - Completion notification: Bit "Command Complete" is set.
> > > >> - Platform notification: Bit "Command Complete" is not set.
> > > >>
> > > >>> I think the ACPI spec on platform notification is quite ambiguous and it is
> > > >>> best to get the necessary clarification and/or correction before implementing
> > > >>> anything related to platform notification.
> > > >> Agreed, a clarification inside ACPI Specification is needed
> > > > This patch look good to me, as it doesn't deal with platform notification.
> > > > We can try to get some clarification from spec side before handling the platform
> > > > notification pieces.
> > > >
> > > > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
> > >
> > > Do you have plan to apply this patch ?
> >
> > Yes.
> >
> > Thanks,
> > Rafael
> >
>
> Hi Rafael,
>
> This patch had an ACK from Prashanth. Can you consider to merge
> this patch please?
Can you please resend it with the ACK?
Thanks,
Rafael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-08-15 23:18 ` Rafael J. Wysocki
@ 2016-08-15 23:41 ` Hoan Tran
2016-08-16 11:56 ` Rafael J. Wysocki
0 siblings, 1 reply; 20+ messages in thread
From: Hoan Tran @ 2016-08-15 23:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
Hi Rafael,
On Mon, Aug 15, 2016 at 4:18 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, August 15, 2016 09:45:24 AM Hoan Tran wrote:
>> On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >
>> > On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
>> > > Hi Jassi and Rafael,
>> > >
>> > > On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
>> > > <pprakash@codeaurora.org> wrote:
>> > > >
>> > > >
>> > > > On 6/9/2016 4:43 PM, Hoan Tran wrote:
>> > > >> Hi Prashanth,
>> > > >>
>> > > >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
>> > > >> <pprakash@codeaurora.org> wrote:
>> > > >>>
>> > > >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>> > > >>>> Hi Ashwin and Prashanth,
>> > > >>>>
>> > > >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>> > > >>>>> Hi Prashanth,
>> > > >>>>>
>> > > >>>>>
>> > > >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>> > > >>>>> <pprakash@codeaurora.org> wrote:
>> > > >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>> > > >>>>>>> Hi Ashwin,
>> > > >>>>>>>
>> > > >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>> > > >>>>>>> <ashwin.chaugule@linaro.org> wrote:
>> > > >>>>>>>> + Prashanth (Can you please have a look as well?)
>> > > >>>>>>>>
>> > > >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>> > > >>>>>>>>> Hi Ashwin,
>> > > >>>>>>>> Hi,
>> > > >>>>>>>>
>> > > >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>> > > >>>>>>>> locations, so its been a bit crazy lately.
>> > > >>>>>>> It's ok and hope you're doing well.
>> > > >>>>>>>
>> > > >>>>>>>> I dont have any major
>> > > >>>>>>>> concerns with this code, although there could be subtle issues with
>> > > >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>> > > >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
>> > > >>>>>>>> completion which is not specific to Type 2. We could support that even
>> > > >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>> > > >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>> > > >>>>>>>> of async platform notification, where you could have a PCC client in
>> > > >>>>>>>> the OS writing to the cmd bit and the platform sending an async
>> > > >>>>>>>> notification by writing to some bits in the same 8byte address as the
>> > > >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>> > > >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
>> > > >>>>>>>> insight into this.
>> > > >>>>>>> I think, this mutual exclusivity could be in another patch.
>> > > >>>>>> Ashwin,
>> > > >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>> > > >>>>>> each other. There is a line is spec that says "all operations on status field
>> > > >>>>>> must be made using interlocked operations", but not sure what these
>> > > >>>>>> interlocked operation translates to.
>> > > >>>>> Yes, I had the same question about how to prevent it.
>> > > >>>> For platform notification, if the hardware doesn't support interlocked
>> > > >>>> operations. I think we can use a workaround that, platform triggers
>> > > >>>> interrupt to OSPM without touching status field. The OSPM PCC client
>> > > >>>> will decide what to do with this interrupt. For example, OSPM sends a
>> > > >>>> consumer command to check it.
>> > > >>> How do we decide which platform can support this interlocked operation?
>> > > >>> and how do we decide between a completion notification and platform
>> > > >>> notification?
>> > > >> Truly, we should follow the specification. But I don't know if there's
>> > > >> any hardware support this interlocked operation.
>> > > >> For the decide between a completion notification and platform notification
>> > > >> - Completion notification: Bit "Command Complete" is set.
>> > > >> - Platform notification: Bit "Command Complete" is not set.
>> > > >>
>> > > >>> I think the ACPI spec on platform notification is quite ambiguous and it is
>> > > >>> best to get the necessary clarification and/or correction before implementing
>> > > >>> anything related to platform notification.
>> > > >> Agreed, a clarification inside ACPI Specification is needed
>> > > > This patch look good to me, as it doesn't deal with platform notification.
>> > > > We can try to get some clarification from spec side before handling the platform
>> > > > notification pieces.
>> > > >
>> > > > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
>> > >
>> > > Do you have plan to apply this patch ?
>> >
>> > Yes.
>> >
>> > Thanks,
>> > Rafael
>> >
>>
>> Hi Rafael,
>>
>> This patch had an ACK from Prashanth. Can you consider to merge
>> this patch please?
>
> Can you please resend it with the ACK?
Correcting, it got a "Reviewed-by: Prashanth Prakash
<pprakash@codeaurora.org>". Is it OK?
Thanks
Hoan
>
> Thanks,
> Rafael
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-08-15 23:41 ` Hoan Tran
@ 2016-08-16 11:56 ` Rafael J. Wysocki
2016-08-16 16:27 ` Hoan Tran
0 siblings, 1 reply; 20+ messages in thread
From: Rafael J. Wysocki @ 2016-08-16 11:56 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
On Monday, August 15, 2016 04:41:22 PM Hoan Tran wrote:
> Hi Rafael,
>
> On Mon, Aug 15, 2016 at 4:18 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, August 15, 2016 09:45:24 AM Hoan Tran wrote:
> >> On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> >
> >> > On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
> >> > > Hi Jassi and Rafael,
> >> > >
> >> > > On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
> >> > > <pprakash@codeaurora.org> wrote:
> >> > > >
> >> > > >
> >> > > > On 6/9/2016 4:43 PM, Hoan Tran wrote:
> >> > > >> Hi Prashanth,
> >> > > >>
> >> > > >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
> >> > > >> <pprakash@codeaurora.org> wrote:
> >> > > >>>
> >> > > >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
> >> > > >>>> Hi Ashwin and Prashanth,
> >> > > >>>>
> >> > > >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
> >> > > >>>>> Hi Prashanth,
> >> > > >>>>>
> >> > > >>>>>
> >> > > >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
> >> > > >>>>> <pprakash@codeaurora.org> wrote:
> >> > > >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
> >> > > >>>>>>> Hi Ashwin,
> >> > > >>>>>>>
> >> > > >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
> >> > > >>>>>>> <ashwin.chaugule@linaro.org> wrote:
> >> > > >>>>>>>> + Prashanth (Can you please have a look as well?)
> >> > > >>>>>>>>
> >> > > >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
> >> > > >>>>>>>>> Hi Ashwin,
> >> > > >>>>>>>> Hi,
> >> > > >>>>>>>>
> >> > > >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
> >> > > >>>>>>>> locations, so its been a bit crazy lately.
> >> > > >>>>>>> It's ok and hope you're doing well.
> >> > > >>>>>>>
> >> > > >>>>>>>> I dont have any major
> >> > > >>>>>>>> concerns with this code, although there could be subtle issues with
> >> > > >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
> >> > > >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
> >> > > >>>>>>>> completion which is not specific to Type 2. We could support that even
> >> > > >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
> >> > > >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
> >> > > >>>>>>>> of async platform notification, where you could have a PCC client in
> >> > > >>>>>>>> the OS writing to the cmd bit and the platform sending an async
> >> > > >>>>>>>> notification by writing to some bits in the same 8byte address as the
> >> > > >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
> >> > > >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
> >> > > >>>>>>>> insight into this.
> >> > > >>>>>>> I think, this mutual exclusivity could be in another patch.
> >> > > >>>>>> Ashwin,
> >> > > >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
> >> > > >>>>>> each other. There is a line is spec that says "all operations on status field
> >> > > >>>>>> must be made using interlocked operations", but not sure what these
> >> > > >>>>>> interlocked operation translates to.
> >> > > >>>>> Yes, I had the same question about how to prevent it.
> >> > > >>>> For platform notification, if the hardware doesn't support interlocked
> >> > > >>>> operations. I think we can use a workaround that, platform triggers
> >> > > >>>> interrupt to OSPM without touching status field. The OSPM PCC client
> >> > > >>>> will decide what to do with this interrupt. For example, OSPM sends a
> >> > > >>>> consumer command to check it.
> >> > > >>> How do we decide which platform can support this interlocked operation?
> >> > > >>> and how do we decide between a completion notification and platform
> >> > > >>> notification?
> >> > > >> Truly, we should follow the specification. But I don't know if there's
> >> > > >> any hardware support this interlocked operation.
> >> > > >> For the decide between a completion notification and platform notification
> >> > > >> - Completion notification: Bit "Command Complete" is set.
> >> > > >> - Platform notification: Bit "Command Complete" is not set.
> >> > > >>
> >> > > >>> I think the ACPI spec on platform notification is quite ambiguous and it is
> >> > > >>> best to get the necessary clarification and/or correction before implementing
> >> > > >>> anything related to platform notification.
> >> > > >> Agreed, a clarification inside ACPI Specification is needed
> >> > > > This patch look good to me, as it doesn't deal with platform notification.
> >> > > > We can try to get some clarification from spec side before handling the platform
> >> > > > notification pieces.
> >> > > >
> >> > > > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
> >> > >
> >> > > Do you have plan to apply this patch ?
> >> >
> >> > Yes.
> >> >
> >> > Thanks,
> >> > Rafael
> >> >
> >>
> >> Hi Rafael,
> >>
> >> This patch had an ACK from Prashanth. Can you consider to merge
> >> this patch please?
> >
> > Can you please resend it with the ACK?
>
> Correcting, it got a "Reviewed-by: Prashanth Prakash
> <pprakash@codeaurora.org>". Is it OK?
Yes, it is.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2
2016-08-16 11:56 ` Rafael J. Wysocki
@ 2016-08-16 16:27 ` Hoan Tran
0 siblings, 0 replies; 20+ messages in thread
From: Hoan Tran @ 2016-08-16 16:27 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Jassi Brar, Ashwin Chaugule, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang, Prakash, Prashanth
On Tue, Aug 16, 2016 at 4:56 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, August 15, 2016 04:41:22 PM Hoan Tran wrote:
>> Hi Rafael,
>>
>> On Mon, Aug 15, 2016 at 4:18 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Monday, August 15, 2016 09:45:24 AM Hoan Tran wrote:
>> >> On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >> >
>> >> > On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
>> >> > > Hi Jassi and Rafael,
>> >> > >
>> >> > > On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
>> >> > > <pprakash@codeaurora.org> wrote:
>> >> > > >
>> >> > > >
>> >> > > > On 6/9/2016 4:43 PM, Hoan Tran wrote:
>> >> > > >> Hi Prashanth,
>> >> > > >>
>> >> > > >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
>> >> > > >> <pprakash@codeaurora.org> wrote:
>> >> > > >>>
>> >> > > >>> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>> >> > > >>>> Hi Ashwin and Prashanth,
>> >> > > >>>>
>> >> > > >>>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>> >> > > >>>>> Hi Prashanth,
>> >> > > >>>>>
>> >> > > >>>>>
>> >> > > >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>> >> > > >>>>> <pprakash@codeaurora.org> wrote:
>> >> > > >>>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>> >> > > >>>>>>> Hi Ashwin,
>> >> > > >>>>>>>
>> >> > > >>>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>> >> > > >>>>>>> <ashwin.chaugule@linaro.org> wrote:
>> >> > > >>>>>>>> + Prashanth (Can you please have a look as well?)
>> >> > > >>>>>>>>
>> >> > > >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>> >> > > >>>>>>>>> Hi Ashwin,
>> >> > > >>>>>>>> Hi,
>> >> > > >>>>>>>>
>> >> > > >>>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>> >> > > >>>>>>>> locations, so its been a bit crazy lately.
>> >> > > >>>>>>> It's ok and hope you're doing well.
>> >> > > >>>>>>>
>> >> > > >>>>>>>> I dont have any major
>> >> > > >>>>>>>> concerns with this code, although there could be subtle issues with
>> >> > > >>>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>> >> > > >>>>>>>> PCC subspace type 2. But you're also adding support for tx command
>> >> > > >>>>>>>> completion which is not specific to Type 2. We could support that even
>> >> > > >>>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>> >> > > >>>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>> >> > > >>>>>>>> of async platform notification, where you could have a PCC client in
>> >> > > >>>>>>>> the OS writing to the cmd bit and the platform sending an async
>> >> > > >>>>>>>> notification by writing to some bits in the same 8byte address as the
>> >> > > >>>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>> >> > > >>>>>>>> and platform could step on each other. Perhaps Prashanth has better
>> >> > > >>>>>>>> insight into this.
>> >> > > >>>>>>> I think, this mutual exclusivity could be in another patch.
>> >> > > >>>>>> Ashwin,
>> >> > > >>>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>> >> > > >>>>>> each other. There is a line is spec that says "all operations on status field
>> >> > > >>>>>> must be made using interlocked operations", but not sure what these
>> >> > > >>>>>> interlocked operation translates to.
>> >> > > >>>>> Yes, I had the same question about how to prevent it.
>> >> > > >>>> For platform notification, if the hardware doesn't support interlocked
>> >> > > >>>> operations. I think we can use a workaround that, platform triggers
>> >> > > >>>> interrupt to OSPM without touching status field. The OSPM PCC client
>> >> > > >>>> will decide what to do with this interrupt. For example, OSPM sends a
>> >> > > >>>> consumer command to check it.
>> >> > > >>> How do we decide which platform can support this interlocked operation?
>> >> > > >>> and how do we decide between a completion notification and platform
>> >> > > >>> notification?
>> >> > > >> Truly, we should follow the specification. But I don't know if there's
>> >> > > >> any hardware support this interlocked operation.
>> >> > > >> For the decide between a completion notification and platform notification
>> >> > > >> - Completion notification: Bit "Command Complete" is set.
>> >> > > >> - Platform notification: Bit "Command Complete" is not set.
>> >> > > >>
>> >> > > >>> I think the ACPI spec on platform notification is quite ambiguous and it is
>> >> > > >>> best to get the necessary clarification and/or correction before implementing
>> >> > > >>> anything related to platform notification.
>> >> > > >> Agreed, a clarification inside ACPI Specification is needed
>> >> > > > This patch look good to me, as it doesn't deal with platform notification.
>> >> > > > We can try to get some clarification from spec side before handling the platform
>> >> > > > notification pieces.
>> >> > > >
>> >> > > > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org>
>> >> > >
>> >> > > Do you have plan to apply this patch ?
>> >> >
>> >> > Yes.
>> >> >
>> >> > Thanks,
>> >> > Rafael
>> >> >
>> >>
>> >> Hi Rafael,
>> >>
>> >> This patch had an ACK from Prashanth. Can you consider to merge
>> >> this patch please?
>> >
>> > Can you please resend it with the ACK?
>>
>> Correcting, it got a "Reviewed-by: Prashanth Prakash
>> <pprakash@codeaurora.org>". Is it OK?
>
> Yes, it is.
Already resend.
Thanks
Hoan
>
> Thanks,
> Rafael
>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-08-16 16:28 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-20 0:32 [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2 Hoan Tran
2016-05-31 16:32 ` Hoan Tran
2016-05-31 19:05 ` Ashwin Chaugule
2016-05-31 19:35 ` Hoan Tran
2016-06-08 12:18 ` Ashwin Chaugule
2016-06-08 16:24 ` Hoan Tran
2016-06-09 0:32 ` Prakash, Prashanth
2016-06-09 0:41 ` Hoan Tran
2016-06-09 20:47 ` Hoan Tran
2016-06-09 22:25 ` Prakash, Prashanth
2016-06-09 22:43 ` Hoan Tran
2016-06-15 16:19 ` Prakash, Prashanth
2016-06-27 18:27 ` Hoan Tran
2016-06-27 21:32 ` Rafael J. Wysocki
2016-07-13 22:00 ` Hoan Tran
2016-08-15 16:45 ` Hoan Tran
2016-08-15 23:18 ` Rafael J. Wysocki
2016-08-15 23:41 ` Hoan Tran
2016-08-16 11:56 ` Rafael J. Wysocki
2016-08-16 16:27 ` Hoan Tran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.