* [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
@ 2016-05-06 18:38 Hoan Tran
2016-05-09 9:43 ` Alexey Klimov
2016-05-10 12:00 ` Ashwin Chaugule
0 siblings, 2 replies; 11+ messages in thread
From: Hoan Tran @ 2016-05-06 18:38 UTC (permalink / raw)
To: Jassi Brar, Ashwin Chaugule, Rafael J. Wysocki, Robert Moore,
alexey.klimov, linux-kernel, linux-acpi
Cc: lho, dhdang, hotran
From: hotran <hotran@apm.com>
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.
This patch depends on patch [1] which supports PCC subspace type 2 header
[1] https://lkml.org/lkml/2016/5/5/14
- [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
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 | 395 +++++++++++++++++++++++++++++++++++++-------------
1 file changed, 296 insertions(+), 99 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 043828d..58c9a67 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,27 +69,179 @@
#include "mailbox.h"
#define MAX_PCC_SUBSPACES 256
+#define MBOX_IRQ_NAME "pcc-mbox"
-static struct mbox_chan *pcc_mbox_channels;
+/**
+ * PCC mailbox channel information
+ *
+ * @chan: Pointer to mailbox communication channel
+ * @pcc_doorbell_vaddr: PCC doorbell register address
+ * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
+ * @irq: Interrupt number of the channel
+ */
+struct pcc_mbox_chan {
+ struct mbox_chan *chan;
+ void __iomem *pcc_doorbell_vaddr;
+ void __iomem *pcc_doorbell_ack_vaddr;
+ int irq;
+};
-/* Array of cached virtual address for doorbell registers */
-static void __iomem **pcc_doorbell_vaddr;
+/**
+ * PCC mailbox controller data
+ *
+ * @mb_ctrl: Representation of the communication channel controller
+ * @mbox_chan: Array of PCC mailbox channels of the controller
+ * @chans: Array of mailbox communication channels
+ */
+struct pcc_mbox {
+ struct mbox_controller mbox_ctrl;
+ struct pcc_mbox_chan *mbox_chans;
+ struct mbox_chan *chans;
+};
+
+static struct pcc_mbox pcc_mbox_ctx = {};
-static struct mbox_controller pcc_mbox_ctrl = {};
/**
* get_pcc_channel - Given a PCC subspace idx, get
- * the respective mbox_channel.
+ * the respective pcc mbox_channel.
* @id: PCC subspace index.
*
* Return: ERR_PTR(errno) if error, else pointer
- * to mbox channel.
+ * to pcc mbox channel.
*/
-static struct mbox_chan *get_pcc_channel(int id)
+static struct pcc_mbox_chan *get_pcc_channel(int id)
{
- if (id < 0 || id > pcc_mbox_ctrl.num_chans)
+ if (id < 0 || id > pcc_mbox_ctx.mbox_ctrl.num_chans)
return ERR_PTR(-ENOENT);
- return &pcc_mbox_channels[id];
+ return &pcc_mbox_ctx.mbox_chans[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 *id)
+{
+ struct acpi_generic_address *doorbell_ack;
+ struct acpi_pcct_hw_reduced *pcct_ss;
+ struct pcc_mbox_chan *pcc_chan = id;
+ u64 doorbell_ack_preserve;
+ struct mbox_chan *chan;
+ u64 doorbell_ack_write;
+ u64 doorbell_ack_val;
+ int ret;
+
+ chan = pcc_chan->chan;
+ 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;
+
+ 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_chan->pcc_doorbell_ack_vaddr,
+ &doorbell_ack_val,
+ doorbell_ack->bit_width);
+ if (ret)
+ return IRQ_NONE;
+
+ ret = write_register(pcc_chan->pcc_doorbell_ack_vaddr,
+ (doorbell_ack_val & doorbell_ack_preserve)
+ | doorbell_ack_write,
+ doorbell_ack->bit_width);
+ if (ret)
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
}
/**
@@ -107,7 +260,8 @@ static struct mbox_chan *get_pcc_channel(int id)
struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
int subspace_id)
{
- struct device *dev = pcc_mbox_ctrl.dev;
+ struct device *dev = pcc_mbox_ctx.mbox_ctrl.dev;
+ struct pcc_mbox_chan *pcc_chan;
struct mbox_chan *chan;
unsigned long flags;
@@ -118,8 +272,13 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
* This returns a pointer to the PCC subspace
* for the Client to operate on.
*/
- chan = get_pcc_channel(subspace_id);
+ pcc_chan = get_pcc_channel(subspace_id);
+ if (IS_ERR(pcc_chan)) {
+ dev_err(dev, "PCC Channel not found for idx: %d\n", subspace_id);
+ return ERR_PTR(-EBUSY);
+ }
+ chan = pcc_chan->chan;
if (IS_ERR(chan) || chan->cl) {
dev_err(dev, "Channel not found for idx: %d\n", subspace_id);
return ERR_PTR(-EBUSY);
@@ -135,6 +294,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_chan->irq > 0) {
+ int rc;
+
+ rc = devm_request_irq(dev, pcc_chan->irq, pcc_mbox_irq, 0,
+ MBOX_IRQ_NAME, pcc_chan);
+ if (unlikely(rc)) {
+ dev_err(dev, "failed to register PCC interrupt %d\n",
+ pcc_chan->irq);
+ chan = ERR_PTR(rc);
+ }
+ }
+
spin_unlock_irqrestore(&chan->lock, flags);
return chan;
@@ -149,7 +320,9 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
*/
void pcc_mbox_free_channel(struct mbox_chan *chan)
{
+ struct pcc_mbox_chan *pcc_chan;
unsigned long flags;
+ u32 id;
if (!chan || !chan->cl)
return;
@@ -160,69 +333,19 @@ void pcc_mbox_free_channel(struct mbox_chan *chan)
if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
chan->txdone_method = TXDONE_BY_POLL;
+ id = chan - pcc_mbox_ctx.chans;
+ pcc_chan = get_pcc_channel(id);
+ if (IS_ERR(pcc_chan)) {
+ pr_debug("PCC Channel not found for idx: %d\n", id);
+ } else {
+ if (pcc_chan->irq > 0)
+ devm_free_irq(chan->mbox->dev, pcc_chan->irq, 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
@@ -240,36 +363,44 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
{
struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
struct acpi_generic_address *doorbell;
+ struct pcc_mbox_chan *pcc_chan;
u64 doorbell_preserve;
u64 doorbell_val;
u64 doorbell_write;
- u32 id = chan - pcc_mbox_channels;
- int ret = 0;
+ u32 id = chan - pcc_mbox_ctx.chans;
+ int ret;
- if (id >= pcc_mbox_ctrl.num_chans) {
+ if (id >= pcc_mbox_ctx.mbox_ctrl.num_chans) {
pr_debug("pcc_send_data: Invalid mbox_chan passed\n");
return -ENOENT;
}
+ pcc_chan = get_pcc_channel(id);
+ if (IS_ERR(pcc_chan)) {
+ pr_debug("PCC Channel not found for idx: %d\n", id);
+ return -EBUSY;
+ }
+
doorbell = &pcct_ss->doorbell_register;
doorbell_preserve = pcct_ss->preserve_mask;
doorbell_write = pcct_ss->write_mask;
/* Sync notification from OS to Platform. */
- if (pcc_doorbell_vaddr[id]) {
- ret = read_register(pcc_doorbell_vaddr[id], &doorbell_val,
- doorbell->bit_width);
+ if (pcc_chan->pcc_doorbell_vaddr) {
+ ret = read_register(pcc_chan->pcc_doorbell_vaddr, &doorbell_val,
+ doorbell->bit_width);
if (ret)
return ret;
- ret = write_register(pcc_doorbell_vaddr[id],
- (doorbell_val & doorbell_preserve) | doorbell_write,
- doorbell->bit_width);
+ ret = write_register(pcc_chan->pcc_doorbell_vaddr,
+ (doorbell_val & doorbell_preserve)
+ | doorbell_write,
+ doorbell->bit_width);
} else {
ret = acpi_read(&doorbell_val, doorbell);
if (ret)
return ret;
ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
- doorbell);
+ doorbell);
}
return ret;
}
@@ -293,11 +424,13 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
{
struct acpi_pcct_hw_reduced *pcct_ss;
- if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
+ if (pcc_mbox_ctx.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 +440,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.
+ * @mbox_chans: Pointer to the PCC mailbox channel data
+ * @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(struct pcc_mbox_chan *mbox_chans,
+ struct acpi_pcct_hw_reduced *pcct_ss)
+{
+ mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
+ (u32)pcct_ss->flags);
+ if (mbox_chans->irq <= 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;
+
+ mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
+ pcct2_ss->doorbell_ack_register.address,
+ pcct2_ss->doorbell_ack_register.bit_width / 8);
+ if (!mbox_chans->pcc_doorbell_ack_vaddr) {
+ 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 +486,8 @@ 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;
acpi_status status = AE_OK;
/* Search for PCCT */
@@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
parse_pcc_subspace, MAX_PCC_SUBSPACES);
+ 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);
+
if (count <= 0) {
pr_err("Error parsing PCC subspaces from PCCT\n");
return -EINVAL;
}
- pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
- count, GFP_KERNEL);
+ pcc_mbox_ctx.chans = kzalloc(sizeof(struct mbox_chan) *
+ count, GFP_KERNEL);
- if (!pcc_mbox_channels) {
+ if (!pcc_mbox_ctx.chans) {
pr_err("Could not allocate space for PCC mbox channels\n");
return -ENOMEM;
}
- pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
- if (!pcc_doorbell_vaddr) {
- kfree(pcc_mbox_channels);
+ pcc_mbox_ctx.mbox_chans = kzalloc(sizeof(struct pcc_mbox_chan) *
+ count, GFP_KERNEL);
+ if (!pcc_mbox_ctx.mbox_chans) {
+ pr_err("Could not allocate space for PCC mbox channel data\n");
return -ENOMEM;
}
@@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
pcct_entry = (struct acpi_subtable_header *) (
(unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
+ acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
+ if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
+ pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
+
for (i = 0; i < count; i++) {
struct acpi_generic_address *db_reg;
struct acpi_pcct_hw_reduced *pcct_ss;
- pcc_mbox_channels[i].con_priv = pcct_entry;
+
+ pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
+ pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
+
+ pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
+
+ pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
+ if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
+ rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
+ pcct_ss);
+ if (rc < 0)
+ return rc;
+ }
/* 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,
+ if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+ pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
+ acpi_os_ioremap(db_reg->address,
db_reg->bit_width/8);
+ }
+
pcct_entry = (struct acpi_subtable_header *)
((unsigned long) pcct_entry + pcct_entry->length);
}
- pcc_mbox_ctrl.num_chans = count;
+ pcc_mbox_ctx.mbox_ctrl.num_chans = count;
- pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
+ pr_info("Detected %d PCC Subspaces\n",
+ pcc_mbox_ctx.mbox_ctrl.num_chans);
return 0;
}
@@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
{
int ret = 0;
- pcc_mbox_ctrl.chans = pcc_mbox_channels;
- pcc_mbox_ctrl.ops = &pcc_chan_ops;
- pcc_mbox_ctrl.dev = &pdev->dev;
+ pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
+ pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
+ pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
pr_info("Registering PCC driver as Mailbox controller\n");
- ret = mbox_controller_register(&pcc_mbox_ctrl);
+ ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);
if (ret) {
pr_err("Err registering PCC as Mailbox controller: %d\n", ret);
--
1.9.1
--
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
for the sole use of the intended recipient(s) and contains information that
is confidential and proprietary to Applied Micro Circuits Corporation or
its subsidiaries. It is to be used solely for the purpose of furthering the
parties' business relationship. All unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient, please
contact the sender by reply e-mail and destroy all copies of the original
message.
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-06 18:38 [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2 Hoan Tran
@ 2016-05-09 9:43 ` Alexey Klimov
2016-05-09 17:38 ` Hoan Tran
2016-05-10 12:00 ` Ashwin Chaugule
1 sibling, 1 reply; 11+ messages in thread
From: Alexey Klimov @ 2016-05-09 9:43 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Ashwin Chaugule, Rafael J. Wysocki, Robert Moore,
linux-kernel, linux-acpi, lho, dhdang
Hi Hoan,
On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
> From: hotran <hotran@apm.com>
>
> 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.
>
> This patch depends on patch [1] which supports PCC subspace type 2 header
> [1] https://lkml.org/lkml/2016/5/5/14
> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
So you finally decided to use separate structure declaration for type 2. Good.
> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 296 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..58c9a67 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>
[...]
> @@ -307,6 +440,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.
> + * @mbox_chans: Pointer to the PCC mailbox channel data
> + * @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(struct pcc_mbox_chan *mbox_chans,
> + struct acpi_pcct_hw_reduced *pcct_ss)
> +{
> + mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
> + (u32)pcct_ss->flags);
> + if (mbox_chans->irq <= 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;
> +
> + mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
> + pcct2_ss->doorbell_ack_register.address,
> + pcct2_ss->doorbell_ack_register.bit_width / 8);
> + if (!mbox_chans->pcc_doorbell_ack_vaddr) {
> + 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 +486,8 @@ 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;
> acpi_status status = AE_OK;
>
> /* Search for PCCT */
> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>
> + 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);
> +
> if (count <= 0) {
> pr_err("Error parsing PCC subspaces from PCCT\n");
> return -EINVAL;
> }
Looks like after first call to acpi_table_parse_entries() you may have negative
number in count. And then you add counted number of type 2 subtables to count variable.
I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
subspaces or don't probe any subspaces at all with such approach. Or other side effects.
Best regards,
Alexey Klimov
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-09 9:43 ` Alexey Klimov
@ 2016-05-09 17:38 ` Hoan Tran
2016-05-10 10:34 ` Alexey Klimov
0 siblings, 1 reply; 11+ messages in thread
From: Hoan Tran @ 2016-05-09 17:38 UTC (permalink / raw)
To: Alexey Klimov
Cc: Jassi Brar, Ashwin Chaugule, Rafael J. Wysocki, Robert Moore,
linux-kernel, linux-acpi, Loc Ho, Duc Dang
Hi Alexey,
On Mon, May 9, 2016 at 2:43 AM, Alexey Klimov <alexey.klimov@arm.com> wrote:
> Hi Hoan,
>
> On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
>> From: hotran <hotran@apm.com>
>>
>> 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.
>>
>> This patch depends on patch [1] which supports PCC subspace type 2 header
>> [1] https://lkml.org/lkml/2016/5/5/14
>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>
> So you finally decided to use separate structure declaration for type 2. Good.
>
>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 296 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 043828d..58c9a67 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>
>
> [...]
>
>> @@ -307,6 +440,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.
>> + * @mbox_chans: Pointer to the PCC mailbox channel data
>> + * @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(struct pcc_mbox_chan *mbox_chans,
>> + struct acpi_pcct_hw_reduced *pcct_ss)
>> +{
>> + mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
>> + (u32)pcct_ss->flags);
>> + if (mbox_chans->irq <= 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;
>> +
>> + mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
>> + pcct2_ss->doorbell_ack_register.address,
>> + pcct2_ss->doorbell_ack_register.bit_width / 8);
>> + if (!mbox_chans->pcc_doorbell_ack_vaddr) {
>> + 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 +486,8 @@ 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;
>> acpi_status status = AE_OK;
>>
>> /* Search for PCCT */
>> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>>
>> + 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);
>> +
>> if (count <= 0) {
>> pr_err("Error parsing PCC subspaces from PCCT\n");
>> return -EINVAL;
>> }
>
> Looks like after first call to acpi_table_parse_entries() you may have negative
> number in count. And then you add counted number of type 2 subtables to count variable.
>
> I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
> subspaces or don't probe any subspaces at all with such approach. Or other side effects.
I should check the "count" at first call acpi_table_parse_entries().
If "count < 0", assign count=0, then call the next
acpi_table_parse_entries().
Thanks for your review
-Hoan
>
>
> Best regards,
> Alexey Klimov
--
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
for the sole use of the intended recipient(s) and contains information that
is confidential and proprietary to Applied Micro Circuits Corporation or
its subsidiaries. It is to be used solely for the purpose of furthering the
parties' business relationship. All unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient, please
contact the sender by reply e-mail and destroy all copies of the original
message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-09 17:38 ` Hoan Tran
@ 2016-05-10 10:34 ` Alexey Klimov
2016-05-11 4:05 ` Hoan Tran
0 siblings, 1 reply; 11+ messages in thread
From: Alexey Klimov @ 2016-05-10 10:34 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Ashwin Chaugule, Rafael J. Wysocki, Robert Moore,
linux-kernel, linux-acpi, Loc Ho, Duc Dang
On Mon, May 09, 2016 at 10:38:24AM -0700, Hoan Tran wrote:
> Hi Alexey,
>
> On Mon, May 9, 2016 at 2:43 AM, Alexey Klimov <alexey.klimov@arm.com> wrote:
> > Hi Hoan,
> >
> > On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
> >> From: hotran <hotran@apm.com>
> >>
> >> 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.
> >>
> >> This patch depends on patch [1] which supports PCC subspace type 2 header
> >> [1] https://lkml.org/lkml/2016/5/5/14
> >> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
> >
> > So you finally decided to use separate structure declaration for type 2. Good.
> >
> >> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 296 insertions(+), 99 deletions(-)
> >>
> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> >> index 043828d..58c9a67 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>
> >
> > [...]
> >
> >> @@ -307,6 +440,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.
> >> + * @mbox_chans: Pointer to the PCC mailbox channel data
> >> + * @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(struct pcc_mbox_chan *mbox_chans,
> >> + struct acpi_pcct_hw_reduced *pcct_ss)
> >> +{
> >> + mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
> >> + (u32)pcct_ss->flags);
> >> + if (mbox_chans->irq <= 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;
> >> +
> >> + mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
> >> + pcct2_ss->doorbell_ack_register.address,
> >> + pcct2_ss->doorbell_ack_register.bit_width / 8);
> >> + if (!mbox_chans->pcc_doorbell_ack_vaddr) {
> >> + 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 +486,8 @@ 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;
> >> acpi_status status = AE_OK;
> >>
> >> /* Search for PCCT */
> >> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
> >> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> >> parse_pcc_subspace, MAX_PCC_SUBSPACES);
> >>
> >> + 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);
> >> +
> >> if (count <= 0) {
> >> pr_err("Error parsing PCC subspaces from PCCT\n");
> >> return -EINVAL;
> >> }
> >
> > Looks like after first call to acpi_table_parse_entries() you may have negative
> > number in count. And then you add counted number of type 2 subtables to count variable.
> >
> > I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
> > subspaces or don't probe any subspaces at all with such approach. Or other side effects.
> I should check the "count" at first call acpi_table_parse_entries().
> If "count < 0", assign count=0, then call the next
> acpi_table_parse_entries().
>
> Thanks for your review
> -Hoan
That second call to acpi_table_parse_entries() might overwrite count with negative number again.
What about the following below?
int count;
int sum = 0;
count = acpi_table_parse_entries(type1);
sum += count >= 0 ? count : 0;
count = acpi_table_parse_entries(type2);
sum += count >= 0 ? count : 0;
if (sum <= 0 || sum >= MAX_PCC_SUBSPACES) {
pr_err();
return -ENODEV;
}
It's possible that I overlooked some corner case.
BTW, zero number of subspaces here doesn't indicate error actually (that's probably not the scope of this change).
Best regards,
Alexey
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-06 18:38 [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2 Hoan Tran
2016-05-09 9:43 ` Alexey Klimov
@ 2016-05-10 12:00 ` Ashwin Chaugule
2016-05-11 4:21 ` Hoan Tran
1 sibling, 1 reply; 11+ messages in thread
From: Ashwin Chaugule @ 2016-05-10 12:00 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, lho, dhdang
Hello,
On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
> From: hotran <hotran@apm.com>
>
> 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.
>
> This patch depends on patch [1] which supports PCC subspace type 2 header
> [1] https://lkml.org/lkml/2016/5/5/14
> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>
> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 296 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..58c9a67 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,27 +69,179 @@
> #include "mailbox.h"
>
> #define MAX_PCC_SUBSPACES 256
> +#define MBOX_IRQ_NAME "pcc-mbox"
>
> -static struct mbox_chan *pcc_mbox_channels;
> +/**
> + * PCC mailbox channel information
> + *
> + * @chan: Pointer to mailbox communication channel
> + * @pcc_doorbell_vaddr: PCC doorbell register address
> + * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
> + * @irq: Interrupt number of the channel
> + */
> +struct pcc_mbox_chan {
> + struct mbox_chan *chan;
> + void __iomem *pcc_doorbell_vaddr;
> + void __iomem *pcc_doorbell_ack_vaddr;
> + int irq;
> +};
>
> -/* Array of cached virtual address for doorbell registers */
> -static void __iomem **pcc_doorbell_vaddr;
> +/**
> + * PCC mailbox controller data
> + *
> + * @mb_ctrl: Representation of the communication channel controller
> + * @mbox_chan: Array of PCC mailbox channels of the controller
> + * @chans: Array of mailbox communication channels
> + */
> +struct pcc_mbox {
> + struct mbox_controller mbox_ctrl;
> + struct pcc_mbox_chan *mbox_chans;
> + struct mbox_chan *chans;
> +};
> +
> +static struct pcc_mbox pcc_mbox_ctx = {};
Im missing the idea behind creating this structure. Are you
registering multiple controllers somewhere?
[...]
>
> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
> pcct_entry = (struct acpi_subtable_header *) (
> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>
> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
> +
> for (i = 0; i < count; i++) {
> struct acpi_generic_address *db_reg;
> struct acpi_pcct_hw_reduced *pcct_ss;
> - pcc_mbox_channels[i].con_priv = pcct_entry;
> +
> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
> +
> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
> +
> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
> + pcct_ss);
> + if (rc < 0)
> + return rc;
> + }
I think we should parse the remaining entries and register them if
they are sane. Some other PCC clients can then continue to function.
>
> /* 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,
> + if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> + pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
> + acpi_os_ioremap(db_reg->address,
> db_reg->bit_width/8);
> + }
> +
> pcct_entry = (struct acpi_subtable_header *)
> ((unsigned long) pcct_entry + pcct_entry->length);
> }
>
> - pcc_mbox_ctrl.num_chans = count;
> + pcc_mbox_ctx.mbox_ctrl.num_chans = count;
>
> - pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
> + pr_info("Detected %d PCC Subspaces\n",
> + pcc_mbox_ctx.mbox_ctrl.num_chans);
>
> return 0;
> }
> @@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
> {
> int ret = 0;
>
> - pcc_mbox_ctrl.chans = pcc_mbox_channels;
> - pcc_mbox_ctrl.ops = &pcc_chan_ops;
> - pcc_mbox_ctrl.dev = &pdev->dev;
> + pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
> + pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
> + pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
>
> pr_info("Registering PCC driver as Mailbox controller\n");
> - ret = mbox_controller_register(&pcc_mbox_ctrl);
> + ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);
That pcc_mbox_ctx usage everywhere seems questionable to me. But its
also too early in the morning here. ;)
Regards,
Ashwin.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-10 10:34 ` Alexey Klimov
@ 2016-05-11 4:05 ` Hoan Tran
0 siblings, 0 replies; 11+ messages in thread
From: Hoan Tran @ 2016-05-11 4:05 UTC (permalink / raw)
To: Alexey Klimov
Cc: Jassi Brar, Ashwin Chaugule, Rafael J. Wysocki, Robert Moore,
linux-kernel, linux-acpi, Loc Ho, Duc Dang
Hi Alexey,
On Tue, May 10, 2016 at 3:34 AM, Alexey Klimov <alexey.klimov@arm.com> wrote:
> On Mon, May 09, 2016 at 10:38:24AM -0700, Hoan Tran wrote:
>> Hi Alexey,
>>
>> On Mon, May 9, 2016 at 2:43 AM, Alexey Klimov <alexey.klimov@arm.com> wrote:
>> > Hi Hoan,
>> >
>> > On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
>> >> From: hotran <hotran@apm.com>
>> >>
>> >> 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.
>> >>
>> >> This patch depends on patch [1] which supports PCC subspace type 2 header
>> >> [1] https://lkml.org/lkml/2016/5/5/14
>> >> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>> >
>> > So you finally decided to use separate structure declaration for type 2. Good.
>> >
>> >> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>> >> 1 file changed, 296 insertions(+), 99 deletions(-)
>> >>
>> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> >> index 043828d..58c9a67 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>
>> >
>> > [...]
>> >
>> >> @@ -307,6 +440,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.
>> >> + * @mbox_chans: Pointer to the PCC mailbox channel data
>> >> + * @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(struct pcc_mbox_chan *mbox_chans,
>> >> + struct acpi_pcct_hw_reduced *pcct_ss)
>> >> +{
>> >> + mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
>> >> + (u32)pcct_ss->flags);
>> >> + if (mbox_chans->irq <= 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;
>> >> +
>> >> + mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
>> >> + pcct2_ss->doorbell_ack_register.address,
>> >> + pcct2_ss->doorbell_ack_register.bit_width / 8);
>> >> + if (!mbox_chans->pcc_doorbell_ack_vaddr) {
>> >> + 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 +486,8 @@ 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;
>> >> acpi_status status = AE_OK;
>> >>
>> >> /* Search for PCCT */
>> >> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
>> >> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>> >> parse_pcc_subspace, MAX_PCC_SUBSPACES);
>> >>
>> >> + 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);
>> >> +
>> >> if (count <= 0) {
>> >> pr_err("Error parsing PCC subspaces from PCCT\n");
>> >> return -EINVAL;
>> >> }
>> >
>> > Looks like after first call to acpi_table_parse_entries() you may have negative
>> > number in count. And then you add counted number of type 2 subtables to count variable.
>> >
>> > I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
>> > subspaces or don't probe any subspaces at all with such approach. Or other side effects.
>> I should check the "count" at first call acpi_table_parse_entries().
>> If "count < 0", assign count=0, then call the next
>> acpi_table_parse_entries().
>>
>> Thanks for your review
>> -Hoan
>
> That second call to acpi_table_parse_entries() might overwrite count with negative number again.
Yes, it could
>
> What about the following below?
>
> int count;
> int sum = 0;
>
> count = acpi_table_parse_entries(type1);
> sum += count >= 0 ? count : 0;
>
> count = acpi_table_parse_entries(type2);
> sum += count >= 0 ? count : 0;
>
> if (sum <= 0 || sum >= MAX_PCC_SUBSPACES) {
> pr_err();
> return -ENODEV;
> }
>
> It's possible that I overlooked some corner case.
> BTW, zero number of subspaces here doesn't indicate error actually (that's probably not the scope of this change).
OK, I will use this suggestion with minor changes
int count;
int sum = 0;
count = acpi_table_parse_entries(type1);
sum += count > 0 ? count : 0;
count = acpi_table_parse_entries(type2);
sum += count > 0 ? count : 0;
if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
pr_err();
return -ENODEV;
}
Thanks and Regards
Hoan
>
> Best regards,
> Alexey
>
--
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
for the sole use of the intended recipient(s) and contains information that
is confidential and proprietary to Applied Micro Circuits Corporation or
its subsidiaries. It is to be used solely for the purpose of furthering the
parties' business relationship. All unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient, please
contact the sender by reply e-mail and destroy all copies of the original
message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-10 12:00 ` Ashwin Chaugule
@ 2016-05-11 4:21 ` Hoan Tran
2016-05-11 11:57 ` Ashwin Chaugule
0 siblings, 1 reply; 11+ messages in thread
From: Hoan Tran @ 2016-05-11 4:21 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,
Thanks for your review !
On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> Hello,
>
> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>> From: hotran <hotran@apm.com>
>>
>> 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.
>>
>> This patch depends on patch [1] which supports PCC subspace type 2 header
>> [1] https://lkml.org/lkml/2016/5/5/14
>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>
>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 296 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 043828d..58c9a67 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,27 +69,179 @@
>> #include "mailbox.h"
>>
>> #define MAX_PCC_SUBSPACES 256
>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>
>> -static struct mbox_chan *pcc_mbox_channels;
>> +/**
>> + * PCC mailbox channel information
>> + *
>> + * @chan: Pointer to mailbox communication channel
>> + * @pcc_doorbell_vaddr: PCC doorbell register address
>> + * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
>> + * @irq: Interrupt number of the channel
>> + */
>> +struct pcc_mbox_chan {
>> + struct mbox_chan *chan;
>> + void __iomem *pcc_doorbell_vaddr;
>> + void __iomem *pcc_doorbell_ack_vaddr;
>> + int irq;
>> +};
>>
>> -/* Array of cached virtual address for doorbell registers */
>> -static void __iomem **pcc_doorbell_vaddr;
>> +/**
>> + * PCC mailbox controller data
>> + *
>> + * @mb_ctrl: Representation of the communication channel controller
>> + * @mbox_chan: Array of PCC mailbox channels of the controller
>> + * @chans: Array of mailbox communication channels
>> + */
>> +struct pcc_mbox {
>> + struct mbox_controller mbox_ctrl;
>> + struct pcc_mbox_chan *mbox_chans;
>> + struct mbox_chan *chans;
>> +};
>> +
>> +static struct pcc_mbox pcc_mbox_ctx = {};
>
> Im missing the idea behind creating this structure. Are you
> registering multiple controllers somewhere?
No, I just want to use a structure to store all global variables into
>
>
> [...]
>
>>
>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>> pcct_entry = (struct acpi_subtable_header *) (
>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>
>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>> +
>> for (i = 0; i < count; i++) {
>> struct acpi_generic_address *db_reg;
>> struct acpi_pcct_hw_reduced *pcct_ss;
>> - pcc_mbox_channels[i].con_priv = pcct_entry;
>> +
>> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>> +
>> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>> +
>> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>> + pcct_ss);
>> + if (rc < 0)
>> + return rc;
>> + }
>
> I think we should parse the remaining entries and register them if
> they are sane. Some other PCC clients can then continue to function.
I think, it could be an error of PCCT table and need to be returned.
>
>>
>> /* 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,
>> + if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> + pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
>> + acpi_os_ioremap(db_reg->address,
>> db_reg->bit_width/8);
>> + }
>> +
>> pcct_entry = (struct acpi_subtable_header *)
>> ((unsigned long) pcct_entry + pcct_entry->length);
>> }
>>
>> - pcc_mbox_ctrl.num_chans = count;
>> + pcc_mbox_ctx.mbox_ctrl.num_chans = count;
>>
>> - pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
>> + pr_info("Detected %d PCC Subspaces\n",
>> + pcc_mbox_ctx.mbox_ctrl.num_chans);
>>
>> return 0;
>> }
>> @@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>> {
>> int ret = 0;
>>
>> - pcc_mbox_ctrl.chans = pcc_mbox_channels;
>> - pcc_mbox_ctrl.ops = &pcc_chan_ops;
>> - pcc_mbox_ctrl.dev = &pdev->dev;
>> + pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
>> + pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
>> + pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
>>
>> pr_info("Registering PCC driver as Mailbox controller\n");
>> - ret = mbox_controller_register(&pcc_mbox_ctrl);
>> + ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);
>
> That pcc_mbox_ctx usage everywhere seems questionable to me. But its
> also too early in the morning here. ;)
As the same with previous comments, I would like to move all global
variables into a structure
Thanks and Regards
Hoan
>
> Regards,
> Ashwin.
--
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
for the sole use of the intended recipient(s) and contains information that
is confidential and proprietary to Applied Micro Circuits Corporation or
its subsidiaries. It is to be used solely for the purpose of furthering the
parties' business relationship. All unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient, please
contact the sender by reply e-mail and destroy all copies of the original
message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-11 4:21 ` Hoan Tran
@ 2016-05-11 11:57 ` Ashwin Chaugule
2016-05-11 18:15 ` Hoan Tran
0 siblings, 1 reply; 11+ messages in thread
From: Ashwin Chaugule @ 2016-05-11 11:57 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang
On 11 May 2016 at 00:21, Hoan Tran <hotran@apm.com> wrote:
> Hi Ashwin,
Hi,
> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>>> From: hotran <hotran@apm.com>
>>>
>>> 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.
>>>
>>> This patch depends on patch [1] which supports PCC subspace type 2 header
>>> [1] https://lkml.org/lkml/2016/5/5/14
>>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>>
>>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>>> 1 file changed, 296 insertions(+), 99 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 043828d..58c9a67 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,27 +69,179 @@
>>> #include "mailbox.h"
>>>
>>> #define MAX_PCC_SUBSPACES 256
>>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>>
>>> -static struct mbox_chan *pcc_mbox_channels;
>>> +/**
>>> + * PCC mailbox channel information
>>> + *
>>> + * @chan: Pointer to mailbox communication channel
>>> + * @pcc_doorbell_vaddr: PCC doorbell register address
>>> + * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
>>> + * @irq: Interrupt number of the channel
>>> + */
>>> +struct pcc_mbox_chan {
>>> + struct mbox_chan *chan;
>>> + void __iomem *pcc_doorbell_vaddr;
>>> + void __iomem *pcc_doorbell_ack_vaddr;
>>> + int irq;
>>> +};
>>>
>>> -/* Array of cached virtual address for doorbell registers */
>>> -static void __iomem **pcc_doorbell_vaddr;
>>> +/**
>>> + * PCC mailbox controller data
>>> + *
>>> + * @mb_ctrl: Representation of the communication channel controller
>>> + * @mbox_chan: Array of PCC mailbox channels of the controller
>>> + * @chans: Array of mailbox communication channels
>>> + */
>>> +struct pcc_mbox {
>>> + struct mbox_controller mbox_ctrl;
>>> + struct pcc_mbox_chan *mbox_chans;
>>> + struct mbox_chan *chans;
>>> +};
>>> +
>>> +static struct pcc_mbox pcc_mbox_ctx = {};
>>
>> Im missing the idea behind creating this structure. Are you
>> registering multiple controllers somewhere?
> No, I just want to use a structure to store all global variables into
>>
I dont see how it makes things better. The functionality added by this
patchset is actually much smaller without this cosmetic change. So I'd
prefer if you could only introduce the IRQ related changes for now and
we'll deal with cosmetic cleanups later if needed. As it stands even
with this new structure, the "pcc_mbox::chans" and
"pcc_mbox_chan::chan" are quite confusing already.
>>
>> [...]
>>
>>>
>>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>>> pcct_entry = (struct acpi_subtable_header *) (
>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>
>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>>> +
>>> for (i = 0; i < count; i++) {
>>> struct acpi_generic_address *db_reg;
>>> struct acpi_pcct_hw_reduced *pcct_ss;
>>> - pcc_mbox_channels[i].con_priv = pcct_entry;
>>> +
>>> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>>> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>>> +
>>> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>>> +
>>> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>>> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>>> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>>> + pcct_ss);
>>> + if (rc < 0)
>>> + return rc;
>>> + }
>>
>> I think we should parse the remaining entries and register them if
>> they are sane. Some other PCC clients can then continue to function.
> I think, it could be an error of PCCT table and need to be returned.
>>
Well, that could be dealt with a pr_err/warn() for that specific entry
? IIRC not all subspaces require IRQ's. Its optional, isnt it?
>>>
>>> /* 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,
>>> + if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>>> + pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
>>> + acpi_os_ioremap(db_reg->address,
>>> db_reg->bit_width/8);
>>> + }
>>> +
>>> pcct_entry = (struct acpi_subtable_header *)
>>> ((unsigned long) pcct_entry + pcct_entry->length);
>>> }
>>>
>>> - pcc_mbox_ctrl.num_chans = count;
>>> + pcc_mbox_ctx.mbox_ctrl.num_chans = count;
>>>
>>> - pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
>>> + pr_info("Detected %d PCC Subspaces\n",
>>> + pcc_mbox_ctx.mbox_ctrl.num_chans);
>>>
>>> return 0;
>>> }
>>> @@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>>> {
>>> int ret = 0;
>>>
>>> - pcc_mbox_ctrl.chans = pcc_mbox_channels;
>>> - pcc_mbox_ctrl.ops = &pcc_chan_ops;
>>> - pcc_mbox_ctrl.dev = &pdev->dev;
>>> + pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
>>> + pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
>>> + pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
>>>
>>> pr_info("Registering PCC driver as Mailbox controller\n");
>>> - ret = mbox_controller_register(&pcc_mbox_ctrl);
>>> + ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);
>>
>> That pcc_mbox_ctx usage everywhere seems questionable to me. But its
>> also too early in the morning here. ;)
> As the same with previous comments, I would like to move all global
> variables into a structure
>
Unfortunately, it doesn't add any value to the functionality you are
introducing. So please revert that back and add only the IRQ changes.
Regards,
Ashwin.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-11 11:57 ` Ashwin Chaugule
@ 2016-05-11 18:15 ` Hoan Tran
2016-05-13 17:23 ` Ashwin Chaugule
0 siblings, 1 reply; 11+ messages in thread
From: Hoan Tran @ 2016-05-11 18:15 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 Wed, May 11, 2016 at 4:57 AM, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> On 11 May 2016 at 00:21, Hoan Tran <hotran@apm.com> wrote:
>> Hi Ashwin,
>
> Hi,
>
>> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>>> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>>>> From: hotran <hotran@apm.com>
>>>>
>>>> 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.
>>>>
>>>> This patch depends on patch [1] which supports PCC subspace type 2 header
>>>> [1] https://lkml.org/lkml/2016/5/5/14
>>>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>>>
>>>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>>>> 1 file changed, 296 insertions(+), 99 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 043828d..58c9a67 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,27 +69,179 @@
>>>> #include "mailbox.h"
>>>>
>>>> #define MAX_PCC_SUBSPACES 256
>>>> +#define MBOX_IRQ_NAME "pcc-mbox"
>>>>
>>>> -static struct mbox_chan *pcc_mbox_channels;
>>>> +/**
>>>> + * PCC mailbox channel information
>>>> + *
>>>> + * @chan: Pointer to mailbox communication channel
>>>> + * @pcc_doorbell_vaddr: PCC doorbell register address
>>>> + * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
>>>> + * @irq: Interrupt number of the channel
>>>> + */
>>>> +struct pcc_mbox_chan {
>>>> + struct mbox_chan *chan;
>>>> + void __iomem *pcc_doorbell_vaddr;
>>>> + void __iomem *pcc_doorbell_ack_vaddr;
>>>> + int irq;
>>>> +};
>>>>
>>>> -/* Array of cached virtual address for doorbell registers */
>>>> -static void __iomem **pcc_doorbell_vaddr;
>>>> +/**
>>>> + * PCC mailbox controller data
>>>> + *
>>>> + * @mb_ctrl: Representation of the communication channel controller
>>>> + * @mbox_chan: Array of PCC mailbox channels of the controller
>>>> + * @chans: Array of mailbox communication channels
>>>> + */
>>>> +struct pcc_mbox {
>>>> + struct mbox_controller mbox_ctrl;
>>>> + struct pcc_mbox_chan *mbox_chans;
>>>> + struct mbox_chan *chans;
>>>> +};
>>>> +
>>>> +static struct pcc_mbox pcc_mbox_ctx = {};
>>>
>>> Im missing the idea behind creating this structure. Are you
>>> registering multiple controllers somewhere?
>> No, I just want to use a structure to store all global variables into
>>>
>
> I dont see how it makes things better. The functionality added by this
> patchset is actually much smaller without this cosmetic change. So I'd
> prefer if you could only introduce the IRQ related changes for now and
> we'll deal with cosmetic cleanups later if needed. As it stands even
> with this new structure, the "pcc_mbox::chans" and
> "pcc_mbox_chan::chan" are quite confusing already.
Ok, I'll remove these 2 structures and create 2 global array
variables: "pcc_doorbell_ack_vaddr" and "irq"
>
>
>>>
>>> [...]
>>>
>>>>
>>>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>>>> pcct_entry = (struct acpi_subtable_header *) (
>>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>>
>>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>>> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>>>> +
>>>> for (i = 0; i < count; i++) {
>>>> struct acpi_generic_address *db_reg;
>>>> struct acpi_pcct_hw_reduced *pcct_ss;
>>>> - pcc_mbox_channels[i].con_priv = pcct_entry;
>>>> +
>>>> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>>>> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>>>> +
>>>> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>>>> +
>>>> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>>>> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>>>> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>>>> + pcct_ss);
>>>> + if (rc < 0)
>>>> + return rc;
>>>> + }
>>>
>>> I think we should parse the remaining entries and register them if
>>> they are sane. Some other PCC clients can then continue to function.
>> I think, it could be an error of PCCT table and need to be returned.
>>>
>
> Well, that could be dealt with a pr_err/warn() for that specific entry
> ?
But what happens if the PCC client requests this failed PCC subspace.
"pcc_parse_subspace_irq" function does "pr_err" for error cases.
> IIRC not all subspaces require IRQ's. Its optional, isnt it?
Maybe I misunderstood: if "doorbell interrupt" bit of "platform
communications channel global flags" is set, the platform is capable
of generating an interrupt to indicate completion of a command. And if
this bit is set, "doorbell interrupt" and "doorbell interrupt flags"
fields of each subspace are active
Could you correct me, thanks
>
>>>>
>>>> /* 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,
>>>> + if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>>>> + pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
>>>> + acpi_os_ioremap(db_reg->address,
>>>> db_reg->bit_width/8);
>>>> + }
>>>> +
>>>> pcct_entry = (struct acpi_subtable_header *)
>>>> ((unsigned long) pcct_entry + pcct_entry->length);
>>>> }
>>>>
>>>> - pcc_mbox_ctrl.num_chans = count;
>>>> + pcc_mbox_ctx.mbox_ctrl.num_chans = count;
>>>>
>>>> - pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
>>>> + pr_info("Detected %d PCC Subspaces\n",
>>>> + pcc_mbox_ctx.mbox_ctrl.num_chans);
>>>>
>>>> return 0;
>>>> }
>>>> @@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>>>> {
>>>> int ret = 0;
>>>>
>>>> - pcc_mbox_ctrl.chans = pcc_mbox_channels;
>>>> - pcc_mbox_ctrl.ops = &pcc_chan_ops;
>>>> - pcc_mbox_ctrl.dev = &pdev->dev;
>>>> + pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
>>>> + pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
>>>> + pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
>>>>
>>>> pr_info("Registering PCC driver as Mailbox controller\n");
>>>> - ret = mbox_controller_register(&pcc_mbox_ctrl);
>>>> + ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);
>>>
>>> That pcc_mbox_ctx usage everywhere seems questionable to me. But its
>>> also too early in the morning here. ;)
>> As the same with previous comments, I would like to move all global
>> variables into a structure
>>
>
> Unfortunately, it doesn't add any value to the functionality you are
> introducing. So please revert that back and add only the IRQ changes.
Yes, I'll do it
Thanks and Regards
Hoan
>
> Regards,
> Ashwin.
--
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is
for the sole use of the intended recipient(s) and contains information that
is confidential and proprietary to Applied Micro Circuits Corporation or
its subsidiaries. It is to be used solely for the purpose of furthering the
parties' business relationship. All unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient, please
contact the sender by reply e-mail and destroy all copies of the original
message.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-11 18:15 ` Hoan Tran
@ 2016-05-13 17:23 ` Ashwin Chaugule
2016-05-13 20:25 ` Hoan Tran
0 siblings, 1 reply; 11+ messages in thread
From: Ashwin Chaugule @ 2016-05-13 17:23 UTC (permalink / raw)
To: Hoan Tran
Cc: Jassi Brar, Rafael J. Wysocki, Robert Moore, Alexey Klimov, lkml,
linux acpi, Loc Ho, Duc Dang
On 11 May 2016 at 14:15, Hoan Tran <hotran@apm.com> wrote:
> On Wed, May 11, 2016 at 4:57 AM, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
>> On 11 May 2016 at 00:21, Hoan Tran <hotran@apm.com> wrote:
>>> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>>>> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>>>>> From: hotran <hotran@apm.com>
>>>>>
>>>>> 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.
>>>>>
>>>>> This patch depends on patch [1] which supports PCC subspace type 2 header
>>>>> [1] https://lkml.org/lkml/2016/5/5/14
>>>>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>>>>
>>>>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>>>>> 1 file changed, 296 insertions(+), 99 deletions(-)
[..]
>>>>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>>>>> pcct_entry = (struct acpi_subtable_header *) (
>>>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>>>
>>>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>>>> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>>>>> +
>>>>> for (i = 0; i < count; i++) {
>>>>> struct acpi_generic_address *db_reg;
>>>>> struct acpi_pcct_hw_reduced *pcct_ss;
>>>>> - pcc_mbox_channels[i].con_priv = pcct_entry;
>>>>> +
>>>>> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>>>>> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>>>>> +
>>>>> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>>>>> +
>>>>> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>>>>> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>>>>> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>>>>> + pcct_ss);
>>>>> + if (rc < 0)
>>>>> + return rc;
>>>>> + }
>>>>
>>>> I think we should parse the remaining entries and register them if
>>>> they are sane. Some other PCC clients can then continue to function.
>>> I think, it could be an error of PCCT table and need to be returned.
>>>>
>>
>> Well, that could be dealt with a pr_err/warn() for that specific entry
>> ?
> But what happens if the PCC client requests this failed PCC subspace.
> "pcc_parse_subspace_irq" function does "pr_err" for error cases.
>
>> IIRC not all subspaces require IRQ's. Its optional, isnt it?
> Maybe I misunderstood: if "doorbell interrupt" bit of "platform
> communications channel global flags" is set, the platform is capable
> of generating an interrupt to indicate completion of a command. And if
> this bit is set, "doorbell interrupt" and "doorbell interrupt flags"
> fields of each subspace are active
> Could you correct me, thanks
>>
You're right. My concern is addressed by your check for txdone_irq.
Are you able to test this on a system which doesn't have Type 2
support?
Thanks,
Ashwin.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2
2016-05-13 17:23 ` Ashwin Chaugule
@ 2016-05-13 20:25 ` Hoan Tran
0 siblings, 0 replies; 11+ messages in thread
From: Hoan Tran @ 2016-05-13 20:25 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 Fri, May 13, 2016 at 10:23 AM, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> On 11 May 2016 at 14:15, Hoan Tran <hotran@apm.com> wrote:
>> On Wed, May 11, 2016 at 4:57 AM, Ashwin Chaugule
>> <ashwin.chaugule@linaro.org> wrote:
>>> On 11 May 2016 at 00:21, Hoan Tran <hotran@apm.com> wrote:
>>>> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>>>>> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>>>>>> From: hotran <hotran@apm.com>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> This patch depends on patch [1] which supports PCC subspace type 2 header
>>>>>> [1] https://lkml.org/lkml/2016/5/5/14
>>>>>> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>>>>>
>>>>>> 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 | 395 +++++++++++++++++++++++++++++++++++++-------------
>>>>>> 1 file changed, 296 insertions(+), 99 deletions(-)
>
> [..]
>
>>>>>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>>>>>> pcct_entry = (struct acpi_subtable_header *) (
>>>>>> (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>>>>
>>>>>> + acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>>>>> + if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>>>>> + pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>>>>>> +
>>>>>> for (i = 0; i < count; i++) {
>>>>>> struct acpi_generic_address *db_reg;
>>>>>> struct acpi_pcct_hw_reduced *pcct_ss;
>>>>>> - pcc_mbox_channels[i].con_priv = pcct_entry;
>>>>>> +
>>>>>> + pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>>>>>> + pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>>>>>> +
>>>>>> + pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>>>>>> +
>>>>>> + pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>>>>>> + if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>>>>>> + rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>>>>>> + pcct_ss);
>>>>>> + if (rc < 0)
>>>>>> + return rc;
>>>>>> + }
>>>>>
>>>>> I think we should parse the remaining entries and register them if
>>>>> they are sane. Some other PCC clients can then continue to function.
>>>> I think, it could be an error of PCCT table and need to be returned.
>>>>>
>>>
>>> Well, that could be dealt with a pr_err/warn() for that specific entry
>>> ?
>> But what happens if the PCC client requests this failed PCC subspace.
>> "pcc_parse_subspace_irq" function does "pr_err" for error cases.
>>
>>> IIRC not all subspaces require IRQ's. Its optional, isnt it?
>> Maybe I misunderstood: if "doorbell interrupt" bit of "platform
>> communications channel global flags" is set, the platform is capable
>> of generating an interrupt to indicate completion of a command. And if
>> this bit is set, "doorbell interrupt" and "doorbell interrupt flags"
>> fields of each subspace are active
>> Could you correct me, thanks
>>>
>
> You're right. My concern is addressed by your check for txdone_irq.
> Are you able to test this on a system which doesn't have Type 2
> support?
This is a testcase which I'm using CPPC driver. It is the same with
the system doesn't support "doorbell interrupt".
As CPPC driver sends CPPC command but doesn't allow platform to
generate interrupt ("Generate Doorbell Interrupt" bit of "Generic
Communications Channel Command Field" is 0)
I'm not sure if it answers your question
Thanks and Regards
-Hoan
>
> Thanks,
> Ashwin.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-05-13 20:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-06 18:38 [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2 Hoan Tran
2016-05-09 9:43 ` Alexey Klimov
2016-05-09 17:38 ` Hoan Tran
2016-05-10 10:34 ` Alexey Klimov
2016-05-11 4:05 ` Hoan Tran
2016-05-10 12:00 ` Ashwin Chaugule
2016-05-11 4:21 ` Hoan Tran
2016-05-11 11:57 ` Ashwin Chaugule
2016-05-11 18:15 ` Hoan Tran
2016-05-13 17:23 ` Ashwin Chaugule
2016-05-13 20:25 ` Hoan Tran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).