[RFC v2 0/3] ACPI Platform Communication Channel driver

[RFC v2 0/3] ACPI Platform Communication Channel driver

[Ashwin Chaugule]

Hello,

This patchset adds initial support for the PCC (Platform Communication Channel)
interface as described in the current ACPI 5.0a spec. See Section 14 of the
ACPI spec - http://acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf for more details on
how PCC works.

In brief PCC (Platform Communication Channel) is a generic means for PCC clients,
to talk to the firmware. The PCC register space is typically memory mapped IO and
uses a doorbell mechanism to communicate synchronously from the OS to the firmware.
The PCC driver is completely agnostic to the protocol implemented by the PCC clients.
It only implements the enumeration of PCC channels and the low level transport mechanism
and leaves the rest to the PCC clients.

The PCC is meant to be useable in the future by clients such as CPPC
(Collaborative Processor Performance Control), RAS (Reliability,
Availability and Serviceability) and MPST (Memory Power State Tables) and possibly others.

While the PCC clients will come as following patches whenever they're ready, I wanted
to get feedback on this common driver and hope that it could be merged upstream. This
should hopefully help various folks that are working on drivers that rely on the PCC
interface.

Cheers,
Ashwin

=== Testing Methodology ===

The PCC test driver in [2/2] is a simple driver that was used to
demonstrate how PCC clients would use the PCC driver. The PCC driver was
tested by sending multiple PCC READS and PCC WRITES across a shared memory
region on an MSM ARMv7 platform. This memory is shared between an apps processor
and a power controller processor. So, one end of the PCC channel is the PCC test driver
running on the apps processor and the other end is a debugger script
(running on a JTAG debugger) thats attached to the power processor. The debugger
script is busy looping on the doorbell address waiting for a bit to flip. This bit
indicates a synchronous communication from the apps processor. The doorbell is rung
when the OS sends a PCC READ or PCC WRITE command. The PCC communication channel
is simply a few fake 32 bit registers that are incremented by the OS and the debugger
script. The OS increments the registers and then sends the PCC WRITE command. On a PCC
READ, the script increments these registers and then OS reads them back. The values
are always incremented by 1 by either end, so we know what value to expect for each
PCC READ/WRITE.


=== Changelog ===

Changes since V1:

-	Integration with Mailbox framework - https://lkml.org/lkml/2014/5/15/49


Ashwin Chaugule (3):
  Mailbox: Add support for ACPI
  ACPI: Add support for Platform Communication Channel
  PCC test driver

 drivers/acpi/Makefile              |   2 +-
 drivers/acpi/pcc-test.c            | 208 ++++++++++++++++++++++++++++++++++
 drivers/acpi/pcc.c                 | 225 +++++++++++++++++++++++++++++++++++++
 drivers/mailbox/Kconfig            |  11 ++
 drivers/mailbox/mailbox.c          | 155 ++++++++++++++++---------
 include/linux/mailbox_client.h     |   3 +
 include/linux/mailbox_controller.h |   6 +
 7 files changed, 557 insertions(+), 53 deletions(-)
 create mode 100644 drivers/acpi/pcc-test.c
 create mode 100644 drivers/acpi/pcc.c

-- 
1.8.3.2

[RFC v2 0/3] ACPI Platform Communication Channel driver

[Ashwin Chaugule]

Hello,

This patchset adds initial support for the PCC (Platform Communication Channel)
interface as described in the current ACPI 5.0a spec. See Section 14 of the
ACPI spec - http://acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf for more details on
how PCC works.

In brief PCC (Platform Communication Channel) is a generic means for PCC clients,
to talk to the firmware. The PCC register space is typically memory mapped IO and
uses a doorbell mechanism to communicate synchronously from the OS to the firmware.
The PCC driver is completely agnostic to the protocol implemented by the PCC clients.
It only implements the enumeration of PCC channels and the low level transport mechanism
and leaves the rest to the PCC clients.

The PCC is meant to be useable in the future by clients such as CPPC
(Collaborative Processor Performance Control), RAS (Reliability,
Availability and Serviceability) and MPST (Memory Power State Tables) and possibly others.

While the PCC clients will come as following patches whenever they're ready, I wanted
to get feedback on this common driver and hope that it could be merged upstream. This
should hopefully help various folks that are working on drivers that rely on the PCC
interface.

Cheers,
Ashwin

=== Testing Methodology ===

The PCC test driver in [2/2] is a simple driver that was used to
demonstrate how PCC clients would use the PCC driver. The PCC driver was
tested by sending multiple PCC READS and PCC WRITES across a shared memory
region on an MSM ARMv7 platform. This memory is shared between an apps processor
and a power controller processor. So, one end of the PCC channel is the PCC test driver
running on the apps processor and the other end is a debugger script
(running on a JTAG debugger) thats attached to the power processor. The debugger
script is busy looping on the doorbell address waiting for a bit to flip. This bit
indicates a synchronous communication from the apps processor. The doorbell is rung
when the OS sends a PCC READ or PCC WRITE command. The PCC communication channel
is simply a few fake 32 bit registers that are incremented by the OS and the debugger
script. The OS increments the registers and then sends the PCC WRITE command. On a PCC
READ, the script increments these registers and then OS reads them back. The values
are always incremented by 1 by either end, so we know what value to expect for each
PCC READ/WRITE.


=== Changelog ===

Changes since V1:

-	Integration with Mailbox framework - https://lkml.org/lkml/2014/5/15/49


Ashwin Chaugule (3):
  Mailbox: Add support for ACPI
  ACPI: Add support for Platform Communication Channel
  PCC test driver

 drivers/acpi/Makefile              |   2 +-
 drivers/acpi/pcc-test.c            | 208 ++++++++++++++++++++++++++++++++++
 drivers/acpi/pcc.c                 | 225 +++++++++++++++++++++++++++++++++++++
 drivers/mailbox/Kconfig            |  11 ++
 drivers/mailbox/mailbox.c          | 155 ++++++++++++++++---------
 include/linux/mailbox_client.h     |   3 +
 include/linux/mailbox_controller.h |   6 +
 7 files changed, 557 insertions(+), 53 deletions(-)
 create mode 100644 drivers/acpi/pcc-test.c
 create mode 100644 drivers/acpi/pcc.c

-- 
1.8.3.2

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 155 ++++++++++++++++++++++++-------------
 include/linux/mailbox_client.h     |   3 +
 include/linux/mailbox_controller.h |   6 ++
 3 files changed, 112 insertions(+), 52 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..c7d17f8 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -273,100 +273,143 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- *		ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+		struct mbox_client *cl)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+
+	if (!cl->tx_tout) /* wait for ever */
+		cl->tx_tout = msecs_to_jiffies(3600000);
+	else
+		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+	if (chan->txdone_method	== TXDONE_BY_POLL
+			&& cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan\n");
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_ACPI
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
+{
+	struct mbox_controller *mbox;
+	int i;
+
+	list_for_each_entry(mbox, &mbox_cons, node) {
+		if (!strcmp(cl->ctrl_name, mbox->name)) {
+			for (i = 0; i < mbox->num_chans; i++) {
+				*chan = &mbox->chans[i];
+				if (!strcmp(cl->chan_name, (*chan)->name))
+					return init_channel(*chan, cl);
+			}
+		}
+	}
+
+	return -ENODEV;
+}
+#endif
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
 {
 	struct device *dev = cl->dev;
 	struct mbox_controller *mbox;
 	struct of_phandle_args spec;
-	struct mbox_chan *chan;
-	unsigned long flags;
 	int count, i, ret;
 
 	if (!dev || !dev->of_node) {
 		pr_err("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	count = of_property_count_strings(dev->of_node, "mbox-names");
 	if (count < 0) {
 		pr_err("%s: mbox-names property of node '%s' missing\n",
 			__func__, dev->of_node->full_name);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
-
-	mutex_lock(&con_mutex);
-
-	ret = -ENODEV;
 	for (i = 0; i < count; i++) {
 		const char *s;
 
 		if (of_property_read_string_index(dev->of_node,
-						"mbox-names", i, &s))
+					"mbox-names", i, &s))
 			continue;
 
 		if (strcmp(cl->chan_name, s))
 			continue;
 
 		if (of_parse_phandle_with_args(dev->of_node,
-					 "mbox", "#mbox-cells",	i, &spec))
+					"mbox", "#mbox-cells",	i, &spec))
 			continue;
 
-		chan = NULL;
 		list_for_each_entry(mbox, &mbox_cons, node)
 			if (mbox->dev->of_node == spec.np) {
-				chan = mbox->of_xlate(mbox, &spec);
+				*chan = mbox->of_xlate(mbox, &spec);
 				break;
 			}
 
 		of_node_put(spec.np);
 
-		if (!chan)
+		if (!(*chan))
 			continue;
 
 		ret = -EBUSY;
-		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+		if (!(*chan)->cl && try_module_get(mbox->dev->driver->owner))
 			break;
 	}
 
 	if (i == count) {
 		mutex_unlock(&con_mutex);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
-	spin_lock_irqsave(&chan->lock, flags);
-	chan->msg_free = 0;
-	chan->msg_count = 0;
-	chan->active_req = NULL;
-	chan->cl = cl;
-	if (!cl->tx_tout) /* wait for ever */
-		cl->tx_tout = msecs_to_jiffies(3600000);
-	else
-		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
-	if (chan->txdone_method	== TXDONE_BY_POLL
-			&& cl->knows_txdone)
-		chan->txdone_method |= TXDONE_BY_ACK;
-	spin_unlock_irqrestore(&chan->lock, flags);
+	return init_channel(*chan, cl);
+}
 
-	ret = chan->mbox->ops->startup(chan);
-	if (ret) {
-		pr_err("Unable to startup the chan\n");
-		mbox_free_channel(chan);
-		chan = ERR_PTR(ret);
-	}
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+	struct mbox_chan *chan = NULL;
+	int ret;
+
+	mutex_lock(&con_mutex);
+
+	ret = get_of_mbox_chan(cl, &chan);
+
+#ifdef CONFIG_ACPI
+	ret = get_acpi_mbox_chan(cl, &chan);
+#endif
+	if (ret)
+		pr_err("No mailbox channels found\n");
 
 	mutex_unlock(&con_mutex);
 	return chan;
@@ -394,11 +437,14 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
+#ifndef CONFIG_ACPI
 	module_put(chan->mbox->dev->driver->owner);
+#endif
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
 
+#ifndef CONFIG_ACPI
 static struct mbox_chan *
 of_mbox_index_xlate(struct mbox_controller *mbox,
 				const struct of_phandle_args *sp)
@@ -410,7 +456,7 @@ of_mbox_index_xlate(struct mbox_controller *mbox,
 
 	return &mbox->chans[ind];
 }
-
+#endif
 /**
  * mbox_controller_register - Register the mailbox controller
  * @mbox:	Pointer to the mailbox controller.
@@ -422,9 +468,13 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	int i, txdone;
 
 	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+	if (!mbox || !mbox->ops || !mbox->num_chans)
 		return -EINVAL;
 
+#ifndef CONFIG_ACPI
+	if (!mbox->dev)
+		return -EINVAL;
+#endif
 	if (mbox->txdone_irq)
 		txdone = TXDONE_BY_IRQ;
 	else if (mbox->txdone_poll)
@@ -446,9 +496,10 @@ int mbox_controller_register(struct mbox_controller *mbox)
 		spin_lock_init(&chan->lock);
 	}
 
+#ifndef CONFIG_ACPI
 	if (!mbox->of_xlate)
 		mbox->of_xlate = of_mbox_index_xlate;
-
+#endif
 	mutex_lock(&con_mutex);
 	list_add_tail(&mbox->node, &mbox_cons);
 	mutex_unlock(&con_mutex);
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..21568a1 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -35,6 +35,9 @@ struct mbox_client {
 	bool tx_block;
 	unsigned long tx_tout;
 	bool knows_txdone;
+#ifdef CONFIG_ACPI
+	const char *ctrl_name;
+#endif
 };
 
 struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index cf81e80..dc9d290 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,9 @@ struct mbox_controller {
 	unsigned period;
 	/* Hook to add to the global controller list */
 	struct list_head node;
+#ifdef CONFIG_ACPI
+	char *name;
+#endif
 } __aligned(32);
 
 /*
@@ -111,6 +114,9 @@ struct mbox_chan {
 
 	/* Private data for controller */
 	void *con_priv;
+#ifdef CONFIG_ACPI
+	char *name;
+#endif
 } __aligned(32);
 
 int mbox_controller_register(struct mbox_controller *mbox);
-- 
1.8.3.2

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 155 ++++++++++++++++++++++++-------------
 include/linux/mailbox_client.h     |   3 +
 include/linux/mailbox_controller.h |   6 ++
 3 files changed, 112 insertions(+), 52 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..c7d17f8 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -273,100 +273,143 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- *		ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+		struct mbox_client *cl)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+
+	if (!cl->tx_tout) /* wait for ever */
+		cl->tx_tout = msecs_to_jiffies(3600000);
+	else
+		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+	if (chan->txdone_method	== TXDONE_BY_POLL
+			&& cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan\n");
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_ACPI
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
+{
+	struct mbox_controller *mbox;
+	int i;
+
+	list_for_each_entry(mbox, &mbox_cons, node) {
+		if (!strcmp(cl->ctrl_name, mbox->name)) {
+			for (i = 0; i < mbox->num_chans; i++) {
+				*chan = &mbox->chans[i];
+				if (!strcmp(cl->chan_name, (*chan)->name))
+					return init_channel(*chan, cl);
+			}
+		}
+	}
+
+	return -ENODEV;
+}
+#endif
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
 {
 	struct device *dev = cl->dev;
 	struct mbox_controller *mbox;
 	struct of_phandle_args spec;
-	struct mbox_chan *chan;
-	unsigned long flags;
 	int count, i, ret;
 
 	if (!dev || !dev->of_node) {
 		pr_err("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	count = of_property_count_strings(dev->of_node, "mbox-names");
 	if (count < 0) {
 		pr_err("%s: mbox-names property of node '%s' missing\n",
 			__func__, dev->of_node->full_name);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
-
-	mutex_lock(&con_mutex);
-
-	ret = -ENODEV;
 	for (i = 0; i < count; i++) {
 		const char *s;
 
 		if (of_property_read_string_index(dev->of_node,
-						"mbox-names", i, &s))
+					"mbox-names", i, &s))
 			continue;
 
 		if (strcmp(cl->chan_name, s))
 			continue;
 
 		if (of_parse_phandle_with_args(dev->of_node,
-					 "mbox", "#mbox-cells",	i, &spec))
+					"mbox", "#mbox-cells",	i, &spec))
 			continue;
 
-		chan = NULL;
 		list_for_each_entry(mbox, &mbox_cons, node)
 			if (mbox->dev->of_node == spec.np) {
-				chan = mbox->of_xlate(mbox, &spec);
+				*chan = mbox->of_xlate(mbox, &spec);
 				break;
 			}
 
 		of_node_put(spec.np);
 
-		if (!chan)
+		if (!(*chan))
 			continue;
 
 		ret = -EBUSY;
-		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+		if (!(*chan)->cl && try_module_get(mbox->dev->driver->owner))
 			break;
 	}
 
 	if (i == count) {
 		mutex_unlock(&con_mutex);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
-	spin_lock_irqsave(&chan->lock, flags);
-	chan->msg_free = 0;
-	chan->msg_count = 0;
-	chan->active_req = NULL;
-	chan->cl = cl;
-	if (!cl->tx_tout) /* wait for ever */
-		cl->tx_tout = msecs_to_jiffies(3600000);
-	else
-		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
-	if (chan->txdone_method	== TXDONE_BY_POLL
-			&& cl->knows_txdone)
-		chan->txdone_method |= TXDONE_BY_ACK;
-	spin_unlock_irqrestore(&chan->lock, flags);
+	return init_channel(*chan, cl);
+}
 
-	ret = chan->mbox->ops->startup(chan);
-	if (ret) {
-		pr_err("Unable to startup the chan\n");
-		mbox_free_channel(chan);
-		chan = ERR_PTR(ret);
-	}
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+	struct mbox_chan *chan = NULL;
+	int ret;
+
+	mutex_lock(&con_mutex);
+
+	ret = get_of_mbox_chan(cl, &chan);
+
+#ifdef CONFIG_ACPI
+	ret = get_acpi_mbox_chan(cl, &chan);
+#endif
+	if (ret)
+		pr_err("No mailbox channels found\n");
 
 	mutex_unlock(&con_mutex);
 	return chan;
@@ -394,11 +437,14 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
+#ifndef CONFIG_ACPI
 	module_put(chan->mbox->dev->driver->owner);
+#endif
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
 
+#ifndef CONFIG_ACPI
 static struct mbox_chan *
 of_mbox_index_xlate(struct mbox_controller *mbox,
 				const struct of_phandle_args *sp)
@@ -410,7 +456,7 @@ of_mbox_index_xlate(struct mbox_controller *mbox,
 
 	return &mbox->chans[ind];
 }
-
+#endif
 /**
  * mbox_controller_register - Register the mailbox controller
  * @mbox:	Pointer to the mailbox controller.
@@ -422,9 +468,13 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	int i, txdone;
 
 	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+	if (!mbox || !mbox->ops || !mbox->num_chans)
 		return -EINVAL;
 
+#ifndef CONFIG_ACPI
+	if (!mbox->dev)
+		return -EINVAL;
+#endif
 	if (mbox->txdone_irq)
 		txdone = TXDONE_BY_IRQ;
 	else if (mbox->txdone_poll)
@@ -446,9 +496,10 @@ int mbox_controller_register(struct mbox_controller *mbox)
 		spin_lock_init(&chan->lock);
 	}
 
+#ifndef CONFIG_ACPI
 	if (!mbox->of_xlate)
 		mbox->of_xlate = of_mbox_index_xlate;
-
+#endif
 	mutex_lock(&con_mutex);
 	list_add_tail(&mbox->node, &mbox_cons);
 	mutex_unlock(&con_mutex);
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..21568a1 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -35,6 +35,9 @@ struct mbox_client {
 	bool tx_block;
 	unsigned long tx_tout;
 	bool knows_txdone;
+#ifdef CONFIG_ACPI
+	const char *ctrl_name;
+#endif
 };
 
 struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index cf81e80..dc9d290 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,9 @@ struct mbox_controller {
 	unsigned period;
 	/* Hook to add to the global controller list */
 	struct list_head node;
+#ifdef CONFIG_ACPI
+	char *name;
+#endif
 } __aligned(32);
 
 /*
@@ -111,6 +114,9 @@ struct mbox_chan {
 
 	/* Private data for controller */
 	void *con_priv;
+#ifdef CONFIG_ACPI
+	char *name;
+#endif
 } __aligned(32);
 
 int mbox_controller_register(struct mbox_controller *mbox);
-- 
1.8.3.2

[RFC v2 2/3] ACPI: Add support for Platform Communication Channel

[Ashwin Chaugule]

ACPI 5.0a+ spec defines a generic mode of communication
between the OS and a platform such as the BMC. This medium
(PCC) is typically used by CPPC (ACPI CPU Performance management),
RAS (ACPI reliability protocol) and MPST (ACPI Memory power
states).

This patch adds initial support for PCC as a mailbox controller.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/pcc.c      | 225 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mailbox/Kconfig |  11 +++
 3 files changed, 237 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pcc.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index fc133d4..d8aa613 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -74,7 +74,7 @@ obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
-
+obj-$(CONFIG_ACPI_PCC)		+= pcc.o
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
 processor-y			+= processor_idle.o processor_thermal.o
diff --git a/drivers/acpi/pcc.c b/drivers/acpi/pcc.c
new file mode 100644
index 0000000..b93a8e7
--- /dev/null
+++ b/drivers/acpi/pcc.c
@@ -0,0 +1,225 @@
+/*
+ *	Copyright (C) 2014 Linaro Ltd.
+ *	Author:	Ashwin Chaugule <ashwin.chaugule@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/init.h>
+#include <linux/cpufreq.h>
+#include <linux/delay.h>
+#include <linux/ioctl.h>
+#include <linux/vmalloc.h>
+#include <linux/mailbox_controller.h>
+
+#include <acpi/actbl.h>
+
+#define MAX_PCC_SUBSPACES	256
+#define PCCS_SS_SIG_MAGIC	0x50434300
+#define PCC_CMD_COMPLETE	0x1
+#define PCC_VERSION			"0.1"
+
+struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];
+
+static bool pcc_tx_done(struct mbox_chan *chan)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Wait for Platform to consume. */
+	while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE))
+		udelay(cmd_delay);
+
+	return true;
+}
+
+/* Channel lock is already held by mbox controller code. */
+static int pcc_send_data(struct mbox_chan *chan, void *data)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	struct acpi_generic_address doorbell;
+	u64 doorbell_preserve;
+	u64 doorbell_val;
+	u64 doorbell_write;
+	u16 cmd = 0;
+	u16 ss_idx = 0;
+	int ret = 0;
+
+	/*
+	 * Min time in usec that OSPM is expected to wait
+	 * before sending the next PCC cmd.
+	 */
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Get PCC CMD */
+	ret = kstrtou16((char*)data, 0, &cmd);
+	if (ret < 0) {
+		pr_err("Err while converting PCC CMD to u16: %d\n", ret);
+		goto out_err;
+	}
+
+	/* Get PCC Subspace ID */
+	ret = kstrtou16(chan->name, 0, &ss_idx);
+	if (ret < 0) {
+		pr_err("Err while converting PCC ss_idx to u16: %d\n", ret);
+		goto out_err;
+	}
+
+	doorbell = pcct_ss->doorbell_register;
+	doorbell_preserve = pcct_ss->preserve_mask;
+	doorbell_write = pcct_ss->write_mask;
+
+	/* Loop until CMD complete bit is set. For prev cmds. */
+	while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE))
+		udelay(cmd_delay);
+
+	/* Write to the shared comm region. */
+	iowrite16(cmd, &generic_comm_base->command);
+
+	/* Write Subspace MAGIC value so platform can identify destination. */
+	iowrite32((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
+
+	/* Flip CMD COMPLETE bit */
+	iowrite16(0, &generic_comm_base->status);
+
+	/* Sync notification from OSPM to Platform. */
+	acpi_read(&doorbell_val, &doorbell);
+	acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
+			&doorbell);
+
+out_err:
+	return ret;
+}
+
+static int pcc_chan_startup(struct mbox_chan *chan)
+{
+	return 0;
+}
+
+static void pcc_chan_shutdown(struct mbox_chan *chan)
+{
+	return;
+}
+
+static struct mbox_controller pcc_mbox_ctrl = {};
+
+static struct mbox_chan_ops pcc_chan_ops = {
+	.send_data	=	pcc_send_data,
+	.startup	=	pcc_chan_startup,
+	.shutdown	=	pcc_chan_shutdown,
+	.last_tx_done	=	pcc_tx_done,
+};
+
+static int parse_pcc_subspace(struct acpi_subtable_header *header,
+		const unsigned long end)
+{
+	struct acpi_pcct_subspace *pcct_ss;
+	char *chan_name;
+
+	if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
+		pcct_ss = (struct acpi_pcct_subspace *) header;
+
+		if (pcct_ss->header.type != ACPI_PCCT_TYPE_GENERIC_SUBSPACE) {
+			pr_err("Incorrect PCC Subspace type detected\n");
+			return -EINVAL;
+		}
+
+		/* New mbox channel entry for each PCC subspace detected. */
+		pcc_mbox_chan[pcc_mbox_ctrl.num_chans].con_priv = pcct_ss;
+
+		/* Use the PCC subspace ID as the channel name. */
+		chan_name = (char *)kmalloc(sizeof(short), GFP_KERNEL);
+		if (!chan_name) {
+			pr_err("No mem for channel name\n");
+			return -ENOSPC;
+		}
+
+		pcc_mbox_chan[pcc_mbox_ctrl.num_chans].name = chan_name;
+		snprintf(pcc_mbox_chan[pcc_mbox_ctrl.num_chans].name,
+				sizeof(short), "%d", pcc_mbox_ctrl.num_chans);
+
+		pcc_mbox_ctrl.num_chans++;
+
+	} else {
+		pr_err("No more space for PCC subspaces.\n");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static int __init pcc_probe(void)
+{
+	acpi_status status = AE_OK;
+	acpi_size pcct_tbl_header_size;
+	struct acpi_table_pcct *pcct_tbl;
+
+	/* Search for PCCT */
+	status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
+			(struct acpi_table_header **)&pcct_tbl,
+			&pcct_tbl_header_size);
+
+	if (ACPI_SUCCESS(status) && !pcct_tbl) {
+		pr_warn("PCCT header not found.\n");
+		status = AE_NOT_FOUND;
+		goto out_err;
+	}
+
+	status = acpi_table_parse_entries(ACPI_SIG_PCCT,
+			sizeof(struct acpi_table_pcct),
+			ACPI_PCCT_TYPE_GENERIC_SUBSPACE,
+			parse_pcc_subspace, MAX_PCC_SUBSPACES);
+
+	if (ACPI_SUCCESS(status))
+		pr_err("Error parsing PCC subspaces from PCCT\n");
+
+	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
+
+	pcc_mbox_ctrl.chans = pcc_mbox_chan;
+	pcc_mbox_ctrl.ops = &pcc_chan_ops;
+	pcc_mbox_ctrl.name = "PCCT";
+	pcc_mbox_ctrl.txdone_poll = true;
+	pcc_mbox_ctrl.txpoll_period = 1;
+
+	pr_info("Registering PCC driver as mbox controller\n");
+	mbox_controller_register(&pcc_mbox_ctrl);
+
+out_err:
+	return ACPI_SUCCESS(status) ? 1 : 0;
+}
+
+static int __init pcc_init(void)
+{
+	int ret;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	/* Check if PCC support is available. */
+	ret = pcc_probe();
+
+	if (ret) {
+		pr_debug("PCC probe failed.\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+device_initcall(pcc_init);
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c8b5c13..7aea896 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -50,4 +50,15 @@ config OMAP_MBOX_KFIFO_SIZE
 	  Specify the default size of mailbox's kfifo buffers (bytes).
 	  This can also be changed at runtime (via the mbox_kfifo_size
 	  module parameter).
+
+config ACPI_PCC
+	bool "ACPI Platform Communication Channel"
+	def_bool n
+	depends on ACPI
+	help
+	Enable this option if your platform supports PCC as defined in the
+	ACPI spec 5.0a+. PCC is a generic mechanism for the OS to communicate
+	with a platform such as a BMC. PCC is typically used by CPPC, RAS
+	and MPST.
+
 endif
-- 
1.8.3.2

[RFC v2 2/3] ACPI: Add support for Platform Communication Channel

[Ashwin Chaugule]

ACPI 5.0a+ spec defines a generic mode of communication
between the OS and a platform such as the BMC. This medium
(PCC) is typically used by CPPC (ACPI CPU Performance management),
RAS (ACPI reliability protocol) and MPST (ACPI Memory power
states).

This patch adds initial support for PCC as a mailbox controller.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/pcc.c      | 225 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mailbox/Kconfig |  11 +++
 3 files changed, 237 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pcc.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index fc133d4..d8aa613 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -74,7 +74,7 @@ obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
-
+obj-$(CONFIG_ACPI_PCC)		+= pcc.o
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
 processor-y			+= processor_idle.o processor_thermal.o
diff --git a/drivers/acpi/pcc.c b/drivers/acpi/pcc.c
new file mode 100644
index 0000000..b93a8e7
--- /dev/null
+++ b/drivers/acpi/pcc.c
@@ -0,0 +1,225 @@
+/*
+ *	Copyright (C) 2014 Linaro Ltd.
+ *	Author:	Ashwin Chaugule <ashwin.chaugule@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/init.h>
+#include <linux/cpufreq.h>
+#include <linux/delay.h>
+#include <linux/ioctl.h>
+#include <linux/vmalloc.h>
+#include <linux/mailbox_controller.h>
+
+#include <acpi/actbl.h>
+
+#define MAX_PCC_SUBSPACES	256
+#define PCCS_SS_SIG_MAGIC	0x50434300
+#define PCC_CMD_COMPLETE	0x1
+#define PCC_VERSION			"0.1"
+
+struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];
+
+static bool pcc_tx_done(struct mbox_chan *chan)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Wait for Platform to consume. */
+	while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE))
+		udelay(cmd_delay);
+
+	return true;
+}
+
+/* Channel lock is already held by mbox controller code. */
+static int pcc_send_data(struct mbox_chan *chan, void *data)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	struct acpi_generic_address doorbell;
+	u64 doorbell_preserve;
+	u64 doorbell_val;
+	u64 doorbell_write;
+	u16 cmd = 0;
+	u16 ss_idx = 0;
+	int ret = 0;
+
+	/*
+	 * Min time in usec that OSPM is expected to wait
+	 * before sending the next PCC cmd.
+	 */
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Get PCC CMD */
+	ret = kstrtou16((char*)data, 0, &cmd);
+	if (ret < 0) {
+		pr_err("Err while converting PCC CMD to u16: %d\n", ret);
+		goto out_err;
+	}
+
+	/* Get PCC Subspace ID */
+	ret = kstrtou16(chan->name, 0, &ss_idx);
+	if (ret < 0) {
+		pr_err("Err while converting PCC ss_idx to u16: %d\n", ret);
+		goto out_err;
+	}
+
+	doorbell = pcct_ss->doorbell_register;
+	doorbell_preserve = pcct_ss->preserve_mask;
+	doorbell_write = pcct_ss->write_mask;
+
+	/* Loop until CMD complete bit is set. For prev cmds. */
+	while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE))
+		udelay(cmd_delay);
+
+	/* Write to the shared comm region. */
+	iowrite16(cmd, &generic_comm_base->command);
+
+	/* Write Subspace MAGIC value so platform can identify destination. */
+	iowrite32((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
+
+	/* Flip CMD COMPLETE bit */
+	iowrite16(0, &generic_comm_base->status);
+
+	/* Sync notification from OSPM to Platform. */
+	acpi_read(&doorbell_val, &doorbell);
+	acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
+			&doorbell);
+
+out_err:
+	return ret;
+}
+
+static int pcc_chan_startup(struct mbox_chan *chan)
+{
+	return 0;
+}
+
+static void pcc_chan_shutdown(struct mbox_chan *chan)
+{
+	return;
+}
+
+static struct mbox_controller pcc_mbox_ctrl = {};
+
+static struct mbox_chan_ops pcc_chan_ops = {
+	.send_data	=	pcc_send_data,
+	.startup	=	pcc_chan_startup,
+	.shutdown	=	pcc_chan_shutdown,
+	.last_tx_done	=	pcc_tx_done,
+};
+
+static int parse_pcc_subspace(struct acpi_subtable_header *header,
+		const unsigned long end)
+{
+	struct acpi_pcct_subspace *pcct_ss;
+	char *chan_name;
+
+	if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
+		pcct_ss = (struct acpi_pcct_subspace *) header;
+
+		if (pcct_ss->header.type != ACPI_PCCT_TYPE_GENERIC_SUBSPACE) {
+			pr_err("Incorrect PCC Subspace type detected\n");
+			return -EINVAL;
+		}
+
+		/* New mbox channel entry for each PCC subspace detected. */
+		pcc_mbox_chan[pcc_mbox_ctrl.num_chans].con_priv = pcct_ss;
+
+		/* Use the PCC subspace ID as the channel name. */
+		chan_name = (char *)kmalloc(sizeof(short), GFP_KERNEL);
+		if (!chan_name) {
+			pr_err("No mem for channel name\n");
+			return -ENOSPC;
+		}
+
+		pcc_mbox_chan[pcc_mbox_ctrl.num_chans].name = chan_name;
+		snprintf(pcc_mbox_chan[pcc_mbox_ctrl.num_chans].name,
+				sizeof(short), "%d", pcc_mbox_ctrl.num_chans);
+
+		pcc_mbox_ctrl.num_chans++;
+
+	} else {
+		pr_err("No more space for PCC subspaces.\n");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static int __init pcc_probe(void)
+{
+	acpi_status status = AE_OK;
+	acpi_size pcct_tbl_header_size;
+	struct acpi_table_pcct *pcct_tbl;
+
+	/* Search for PCCT */
+	status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
+			(struct acpi_table_header **)&pcct_tbl,
+			&pcct_tbl_header_size);
+
+	if (ACPI_SUCCESS(status) && !pcct_tbl) {
+		pr_warn("PCCT header not found.\n");
+		status = AE_NOT_FOUND;
+		goto out_err;
+	}
+
+	status = acpi_table_parse_entries(ACPI_SIG_PCCT,
+			sizeof(struct acpi_table_pcct),
+			ACPI_PCCT_TYPE_GENERIC_SUBSPACE,
+			parse_pcc_subspace, MAX_PCC_SUBSPACES);
+
+	if (ACPI_SUCCESS(status))
+		pr_err("Error parsing PCC subspaces from PCCT\n");
+
+	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
+
+	pcc_mbox_ctrl.chans = pcc_mbox_chan;
+	pcc_mbox_ctrl.ops = &pcc_chan_ops;
+	pcc_mbox_ctrl.name = "PCCT";
+	pcc_mbox_ctrl.txdone_poll = true;
+	pcc_mbox_ctrl.txpoll_period = 1;
+
+	pr_info("Registering PCC driver as mbox controller\n");
+	mbox_controller_register(&pcc_mbox_ctrl);
+
+out_err:
+	return ACPI_SUCCESS(status) ? 1 : 0;
+}
+
+static int __init pcc_init(void)
+{
+	int ret;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	/* Check if PCC support is available. */
+	ret = pcc_probe();
+
+	if (ret) {
+		pr_debug("PCC probe failed.\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+device_initcall(pcc_init);
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c8b5c13..7aea896 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -50,4 +50,15 @@ config OMAP_MBOX_KFIFO_SIZE
 	  Specify the default size of mailbox's kfifo buffers (bytes).
 	  This can also be changed at runtime (via the mbox_kfifo_size
 	  module parameter).
+
+config ACPI_PCC
+	bool "ACPI Platform Communication Channel"
+	def_bool n
+	depends on ACPI
+	help
+	Enable this option if your platform supports PCC as defined in the
+	ACPI spec 5.0a+. PCC is a generic mechanism for the OS to communicate
+	with a platform such as a BMC. PCC is typically used by CPPC, RAS
+	and MPST.
+
 endif
-- 
1.8.3.2

[RFC v2 3/3] PCC test driver

[Ashwin Chaugule]

A simple test driver to register as a PCC mailbox client and
send PCC reads/writes via the PCC mailbox controller.

echo 2 > /proc/pcc_test to register client.

echo 1 > /proc/pcc_test for PCC writes.

echo 0 > /proc/pcc_test for PCC reads.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/pcc-test.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pcc-test.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d8aa613..f7bbbe9 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -74,7 +74,7 @@ obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
-obj-$(CONFIG_ACPI_PCC)		+= pcc.o
+obj-$(CONFIG_ACPI_PCC)		+= pcc.o pcc-test.o
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
 processor-y			+= processor_idle.o processor_thermal.o
diff --git a/drivers/acpi/pcc-test.c b/drivers/acpi/pcc-test.c
new file mode 100644
index 0000000..4ba8c1f
--- /dev/null
+++ b/drivers/acpi/pcc-test.c
@@ -0,0 +1,208 @@
+/*
+ *	Copyright (C) 2014 Linaro Ltd.
+ *	Author:	Ashwin Chaugule <ashwin.chaugule@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/init.h>
+#include <linux/cpufreq.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+
+#include <asm/uaccess.h>
+
+#include <acpi/actbl.h>
+
+static void __iomem *comm_base_addr; 	/* For use after ioremap */
+
+extern int mbox_controller_register(struct mbox_controller *mbox);
+extern struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
+extern int mbox_send_message(struct mbox_chan *chan, void *mssg);
+
+
+/* XXX: The PCC Subspace id is hard coded here only for test purposes.
+ * In reality it should be parsed from the PCC package as described
+ * in the PCC client table. e.g. CPC for CPPC 
+ */
+#define PCC_SUBSPACE_IDX 	0
+#define CMD_COMPLETE    	1
+
+struct mbox_chan *pcc_test_chan;
+enum ppc_cmds {
+	CMD_READ,
+	CMD_WRITE,
+	RESERVED,
+};
+
+static u32 reg1, reg2;
+
+static void run_pcc_test_read(void)
+{
+	u32 reg1_addr = (u32)comm_base_addr + 0x100;
+	u32 reg2_addr = (u32)comm_base_addr + 0x110;
+	char mssg[2];
+	int ret;
+
+	/* READ part of the test */
+	pr_info("Sending PCC read req from Channel base addr: %x\n", (u32)comm_base_addr);
+
+	snprintf(mssg, sizeof(short), "%d", CMD_READ);
+	ret = mbox_send_message(pcc_test_chan, &mssg);
+	if (ret >= 0) {
+		pr_info("Read updated values from Platform.\n");
+		reg1 = ioread32((void*)reg1_addr);
+		reg2 = ioread32((void*)reg2_addr);
+		pr_info("updated value of reg1:%x\n", reg1);
+		pr_info("updated value of reg2:%x\n", reg2);
+	} else
+		pr_err("Failed to read PCC parameters: ret=%d\n", ret);
+}
+
+static void run_pcc_test_write(void)
+{
+	u32 reg1_addr = (u32)comm_base_addr + 0x100;
+	u32 reg2_addr = (u32)comm_base_addr + 0x110;
+	char mssg[2];
+	int ret;
+
+	/* WRITE part of the test */
+	reg1++;
+	reg2++;
+
+	iowrite32(reg1, (void *)reg1_addr);
+	iowrite32(reg2, (void *)reg2_addr);
+
+	pr_info("Sending PCC write req from Channel base addr: %x\n", (u32)comm_base_addr);
+	snprintf(mssg, sizeof(short), "%d", CMD_WRITE);
+
+	ret = mbox_send_message(pcc_test_chan, &mssg);
+
+	if (ret >= 0)
+		pr_info("OSPM successfully sent PCC write cmd to platform\n");
+	else
+		pr_err("Failed to write PCC parameters. ret= %d\n", ret);
+}
+
+static void pcc_chan_tx_done(struct mbox_client *cl, void *mssg, enum mbox_result r)
+{
+	if (r == MBOX_OK)
+		pr_info("PCC channel TX successfully completed. CMD sent = %s\n", (char*)mssg);
+	else
+		pr_warn("PCC channel TX did not complete: CMD sent = %s\n", (char*)mssg);
+}
+
+struct mbox_client pcc_mbox_cl = {
+	.chan_name	=	"0",
+	.ctrl_name	=	"PCCT",
+	.tx_done	=	pcc_chan_tx_done,
+};
+
+void get_pcc_comm(void)
+{
+	u32 base_addr;
+	u32 len;
+	struct acpi_pcct_subspace *pcc_ss;
+
+	pcc_test_chan = mbox_request_channel(&pcc_mbox_cl);
+
+	if (!pcc_test_chan) {
+		pr_err("PCC Channel not found!\n");
+		return;
+	}
+
+	pcc_ss = pcc_test_chan->con_priv;
+
+	base_addr = pcc_ss->base_address;
+	len = pcc_ss->length;
+
+	pr_info("ioremapping: %x, len: %x\n", base_addr, len);
+
+	comm_base_addr = ioremap_nocache(base_addr, len);
+
+	if (!comm_base_addr) {
+		pr_err("Could not ioremap channel\n");
+		return;
+	}
+
+	pcc_ss->base_address = (u32)comm_base_addr;
+
+	pr_info("Comm_base_addr: %x\n", (u32)comm_base_addr);
+}
+
+static ssize_t pcc_test_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *offs)
+{
+	char ctl[2];
+
+	if (count != 2 || *offs)
+		return -EINVAL;
+
+	if (copy_from_user(ctl, buf, count))
+		return -EFAULT;
+
+	switch (ctl[0]) {
+		case '0':
+			/* PCC read */
+			run_pcc_test_read();
+			break;
+		case '1':
+			/* PCC write */
+			run_pcc_test_write();
+			break;
+		case '2':
+			/* Get PCC channel */
+			get_pcc_comm();
+			break;
+		default:
+			pr_err("Unknown val\n");
+			break;
+	}
+
+	return count;
+}
+
+static int pcc_test_open(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static int pcc_test_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static const struct file_operations pcc_test_fops = {
+	.open		= pcc_test_open,
+//	.read		= seq_read,
+	.write		= pcc_test_write,
+	.release	= pcc_test_release,
+};
+
+static int __init pcc_test(void)
+{
+	struct proc_dir_entry *pe;
+
+	pe = proc_create("pcc_test", 0644, NULL, &pcc_test_fops);
+
+	if (!pe)
+		return -ENOMEM;
+
+	return 0;
+}
+
+late_initcall(pcc_test);
-- 
1.8.3.2

[RFC v2 3/3] PCC test driver

[Ashwin Chaugule]

A simple test driver to register as a PCC mailbox client and
send PCC reads/writes via the PCC mailbox controller.

echo 2 > /proc/pcc_test to register client.

echo 1 > /proc/pcc_test for PCC writes.

echo 0 > /proc/pcc_test for PCC reads.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/pcc-test.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pcc-test.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d8aa613..f7bbbe9 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -74,7 +74,7 @@ obj-$(CONFIG_ACPI_HED)		+= hed.o
 obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
-obj-$(CONFIG_ACPI_PCC)		+= pcc.o
+obj-$(CONFIG_ACPI_PCC)		+= pcc.o pcc-test.o
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
 processor-y			+= processor_idle.o processor_thermal.o
diff --git a/drivers/acpi/pcc-test.c b/drivers/acpi/pcc-test.c
new file mode 100644
index 0000000..4ba8c1f
--- /dev/null
+++ b/drivers/acpi/pcc-test.c
@@ -0,0 +1,208 @@
+/*
+ *	Copyright (C) 2014 Linaro Ltd.
+ *	Author:	Ashwin Chaugule <ashwin.chaugule@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/init.h>
+#include <linux/cpufreq.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+
+#include <asm/uaccess.h>
+
+#include <acpi/actbl.h>
+
+static void __iomem *comm_base_addr; 	/* For use after ioremap */
+
+extern int mbox_controller_register(struct mbox_controller *mbox);
+extern struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
+extern int mbox_send_message(struct mbox_chan *chan, void *mssg);
+
+
+/* XXX: The PCC Subspace id is hard coded here only for test purposes.
+ * In reality it should be parsed from the PCC package as described
+ * in the PCC client table. e.g. CPC for CPPC 
+ */
+#define PCC_SUBSPACE_IDX 	0
+#define CMD_COMPLETE    	1
+
+struct mbox_chan *pcc_test_chan;
+enum ppc_cmds {
+	CMD_READ,
+	CMD_WRITE,
+	RESERVED,
+};
+
+static u32 reg1, reg2;
+
+static void run_pcc_test_read(void)
+{
+	u32 reg1_addr = (u32)comm_base_addr + 0x100;
+	u32 reg2_addr = (u32)comm_base_addr + 0x110;
+	char mssg[2];
+	int ret;
+
+	/* READ part of the test */
+	pr_info("Sending PCC read req from Channel base addr: %x\n", (u32)comm_base_addr);
+
+	snprintf(mssg, sizeof(short), "%d", CMD_READ);
+	ret = mbox_send_message(pcc_test_chan, &mssg);
+	if (ret >= 0) {
+		pr_info("Read updated values from Platform.\n");
+		reg1 = ioread32((void*)reg1_addr);
+		reg2 = ioread32((void*)reg2_addr);
+		pr_info("updated value of reg1:%x\n", reg1);
+		pr_info("updated value of reg2:%x\n", reg2);
+	} else
+		pr_err("Failed to read PCC parameters: ret=%d\n", ret);
+}
+
+static void run_pcc_test_write(void)
+{
+	u32 reg1_addr = (u32)comm_base_addr + 0x100;
+	u32 reg2_addr = (u32)comm_base_addr + 0x110;
+	char mssg[2];
+	int ret;
+
+	/* WRITE part of the test */
+	reg1++;
+	reg2++;
+
+	iowrite32(reg1, (void *)reg1_addr);
+	iowrite32(reg2, (void *)reg2_addr);
+
+	pr_info("Sending PCC write req from Channel base addr: %x\n", (u32)comm_base_addr);
+	snprintf(mssg, sizeof(short), "%d", CMD_WRITE);
+
+	ret = mbox_send_message(pcc_test_chan, &mssg);
+
+	if (ret >= 0)
+		pr_info("OSPM successfully sent PCC write cmd to platform\n");
+	else
+		pr_err("Failed to write PCC parameters. ret= %d\n", ret);
+}
+
+static void pcc_chan_tx_done(struct mbox_client *cl, void *mssg, enum mbox_result r)
+{
+	if (r == MBOX_OK)
+		pr_info("PCC channel TX successfully completed. CMD sent = %s\n", (char*)mssg);
+	else
+		pr_warn("PCC channel TX did not complete: CMD sent = %s\n", (char*)mssg);
+}
+
+struct mbox_client pcc_mbox_cl = {
+	.chan_name	=	"0",
+	.ctrl_name	=	"PCCT",
+	.tx_done	=	pcc_chan_tx_done,
+};
+
+void get_pcc_comm(void)
+{
+	u32 base_addr;
+	u32 len;
+	struct acpi_pcct_subspace *pcc_ss;
+
+	pcc_test_chan = mbox_request_channel(&pcc_mbox_cl);
+
+	if (!pcc_test_chan) {
+		pr_err("PCC Channel not found!\n");
+		return;
+	}
+
+	pcc_ss = pcc_test_chan->con_priv;
+
+	base_addr = pcc_ss->base_address;
+	len = pcc_ss->length;
+
+	pr_info("ioremapping: %x, len: %x\n", base_addr, len);
+
+	comm_base_addr = ioremap_nocache(base_addr, len);
+
+	if (!comm_base_addr) {
+		pr_err("Could not ioremap channel\n");
+		return;
+	}
+
+	pcc_ss->base_address = (u32)comm_base_addr;
+
+	pr_info("Comm_base_addr: %x\n", (u32)comm_base_addr);
+}
+
+static ssize_t pcc_test_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *offs)
+{
+	char ctl[2];
+
+	if (count != 2 || *offs)
+		return -EINVAL;
+
+	if (copy_from_user(ctl, buf, count))
+		return -EFAULT;
+
+	switch (ctl[0]) {
+		case '0':
+			/* PCC read */
+			run_pcc_test_read();
+			break;
+		case '1':
+			/* PCC write */
+			run_pcc_test_write();
+			break;
+		case '2':
+			/* Get PCC channel */
+			get_pcc_comm();
+			break;
+		default:
+			pr_err("Unknown val\n");
+			break;
+	}
+
+	return count;
+}
+
+static int pcc_test_open(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static int pcc_test_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static const struct file_operations pcc_test_fops = {
+	.open		= pcc_test_open,
+//	.read		= seq_read,
+	.write		= pcc_test_write,
+	.release	= pcc_test_release,
+};
+
+static int __init pcc_test(void)
+{
+	struct proc_dir_entry *pe;
+
+	pe = proc_create("pcc_test", 0644, NULL, &pcc_test_fops);
+
+	if (!pe)
+		return -ENOMEM;
+
+	return 0;
+}
+
+late_initcall(pcc_test);
-- 
1.8.3.2

Re: [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Jassi,

On 12 June 2014 12:48, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> The current mailbox framework only supports DT based bindings.
> Add another mechanism for mailbox clients to register with mailbox
> controllers and request for specific mailbox channels. This enables
> usage of the mailbox framework on kernels with ACPI support.
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 155 ++++++++++++++++++++++++-------------
>  include/linux/mailbox_client.h     |   3 +
>  include/linux/mailbox_controller.h |   6 ++
>  3 files changed, 112 insertions(+), 52 deletions(-)
>

Can you please try this change on your platform(s) with DT support?

Cheers,
Ashwin

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Jassi,

On 12 June 2014 12:48, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> The current mailbox framework only supports DT based bindings.
> Add another mechanism for mailbox clients to register with mailbox
> controllers and request for specific mailbox channels. This enables
> usage of the mailbox framework on kernels with ACPI support.
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 155 ++++++++++++++++++++++++-------------
>  include/linux/mailbox_client.h     |   3 +
>  include/linux/mailbox_controller.h |   6 ++
>  3 files changed, 112 insertions(+), 52 deletions(-)
>

Can you please try this change on your platform(s) with DT support?

Cheers,
Ashwin

Re: [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
> 
> +#ifndef CONFIG_ACPI
>         if (!mbox->of_xlate)
>                 mbox->of_xlate = of_mbox_index_xlate;
> -
> +#endif
>         mutex_lock(&con_mutex);
>         list_add_tail(&mbox->node, &mbox_cons);
>         mutex_unlock(&con_mutex);
> 

You can't do #ifndef here, the driver must still work if
both OF and ACPI are enabled.

	Arnd

[RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
> 
> +#ifndef CONFIG_ACPI
>         if (!mbox->of_xlate)
>                 mbox->of_xlate = of_mbox_index_xlate;
> -
> +#endif
>         mutex_lock(&con_mutex);
>         list_add_tail(&mbox->node, &mbox_cons);
>         mutex_unlock(&con_mutex);
> 

You can't do #ifndef here, the driver must still work if
both OF and ACPI are enabled.

	Arnd

Re: [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>
>> +#ifndef CONFIG_ACPI
>>         if (!mbox->of_xlate)
>>                 mbox->of_xlate = of_mbox_index_xlate;
>> -
>> +#endif
>>         mutex_lock(&con_mutex);
>>         list_add_tail(&mbox->node, &mbox_cons);
>>         mutex_unlock(&con_mutex);
>>
>
> You can't do #ifndef here, the driver must still work if
> both OF and ACPI are enabled.

Ok. Here we could skip the macro altogether, since of_xlate wont be
called with ACPI anyway.

Will need to look into using/faking mbox->dev to remove the other ifndefs.

Cheers,
Ashwin

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>
>> +#ifndef CONFIG_ACPI
>>         if (!mbox->of_xlate)
>>                 mbox->of_xlate = of_mbox_index_xlate;
>> -
>> +#endif
>>         mutex_lock(&con_mutex);
>>         list_add_tail(&mbox->node, &mbox_cons);
>>         mutex_unlock(&con_mutex);
>>
>
> You can't do #ifndef here, the driver must still work if
> both OF and ACPI are enabled.

Ok. Here we could skip the macro altogether, since of_xlate wont be
called with ACPI anyway.

Will need to look into using/faking mbox->dev to remove the other ifndefs.

Cheers,
Ashwin

Re: [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hello,

On 12 June 2014 13:14, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>>
>>> +#ifndef CONFIG_ACPI
>>>         if (!mbox->of_xlate)
>>>                 mbox->of_xlate = of_mbox_index_xlate;
>>> -
>>> +#endif
>>>         mutex_lock(&con_mutex);
>>>         list_add_tail(&mbox->node, &mbox_cons);
>>>         mutex_unlock(&con_mutex);
>>>
>>
>> You can't do #ifndef here, the driver must still work if
>> both OF and ACPI are enabled.
>
> Ok. Here we could skip the macro altogether, since of_xlate wont be
> called with ACPI anyway.
>
> Will need to look into using/faking mbox->dev to remove the other ifndefs.


So, in order to get an mbox->dev for ACPI platforms, we'd need an
entry in the DSDT table. That seems rather pointless, since the DSDT
is reserved for devices and is supposed to be OS agnostic. Since the
mailbox controller itself is not really a "device" with a resource
descriptor, I dont see the point in adding a dummy DSDT entry for the
sake of getting this `struct device`. Also, I'm told adding new
entries to this table requires registering a unique 4 character
identifier and approval from some committees. If there are other ways
to get this structure I'd like to hear about it.

The other alternative would be to piggy back on the ACPI CPU detection
code, which looks for the ACPI0007 device node in the DSDT and use
that as the mbox controller device. This node is already registered
and is an established method to detect CPUs. But I'm not sure what
happens when CPUs are hotplugged off, we surely dont want mailbox
clients such as PCC to break.

The third alternative is to ignore the dev refcounts for ACPI as shown
in this totally untested patch:

----------------8<----------------------------------------


>From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Date: Wed, 11 Jun 2014 16:09:35 -0400
Subject: [PATCH] Mailbox: Add support for ACPI

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
 include/linux/mailbox_client.h     |   2 +-
 include/linux/mailbox_controller.h |   1 +
 3 files changed, 129 insertions(+), 51 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..e2704f5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>

@@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan,
void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);

-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- * ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+ struct mbox_client *cl)
+{
+ unsigned long flags;
+ int ret;
+
+ if (!chan) {
+ pr_err("No mailbox channel specified\n");
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&chan->lock, flags);
+ chan->msg_free = 0;
+ chan->msg_count = 0;
+ chan->active_req = NULL;
+ chan->cl = cl;
+
+ if (!cl->tx_tout) /* wait for ever */
+ cl->tx_tout = msecs_to_jiffies(3600000);
+ else
+ cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+ if (chan->txdone_method == TXDONE_BY_POLL
+ && cl->knows_txdone)
+ chan->txdone_method |= TXDONE_BY_ACK;
+ spin_unlock_irqrestore(&chan->lock, flags);
+
+ ret = chan->mbox->ops->startup(chan);
+ if (ret) {
+ pr_err("Unable to startup the chan\n");
+ mbox_free_channel(chan);
+ chan = ERR_PTR(ret);
+ }
+
+ return ret;
+}
+
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+ struct mbox_chan **chan)
+{
+ struct mbox_controller *mbox;
+ int chan_id, ret, len;
+ char *chan_ptr;
+
+ if (!cl->chan_name)
+ return -ENODEV;
+
+ list_for_each_entry(mbox, &mbox_cons, node) {
+ if (mbox->name) {
+ /*
+ * The cl->chan_name has the format => controller:channel
+ * as described in mailbox_client.h
+ */
+
+ len = strlen(mbox->name);
+ chan_ptr = cl->chan_name + len + 1;
+
+ ret = kstrtou32(cl->chan_name, 0, &chan_id);
+
+ if (ret < 0) {
+ pr_err("Err while parsing mailbox:%s channel idx\n",
+ mbox->name);
+ continue;
+ }
+
+ if (!strncmp(cl->chan_name, mbox->name, len)) {
+ *chan = &mbox->chans[chan_id];
+ return init_channel(*chan, cl);
+ }
+ }
+ }
+
+ return -ENODEV;
+}
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+ struct mbox_chan **chan)
 {
  struct device *dev = cl->dev;
  struct mbox_controller *mbox;
  struct of_phandle_args spec;
- struct mbox_chan *chan;
- unsigned long flags;
  int count, i, ret;

  if (!dev || !dev->of_node) {
  pr_err("%s: No owner device node\n", __func__);
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
  }

  count = of_property_count_strings(dev->of_node, "mbox-names");
  if (count < 0) {
  pr_err("%s: mbox-names property of node '%s' missing\n",
  __func__, dev->of_node->full_name);
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
  }
-
- mutex_lock(&con_mutex);
-
- ret = -ENODEV;
  for (i = 0; i < count; i++) {
  const char *s;

  if (of_property_read_string_index(dev->of_node,
- "mbox-names", i, &s))
+ "mbox-names", i, &s))
  continue;

  if (strcmp(cl->chan_name, s))
  continue;

  if (of_parse_phandle_with_args(dev->of_node,
- "mbox", "#mbox-cells", i, &spec))
+ "mbox", "#mbox-cells", i, &spec))
  continue;

- chan = NULL;
  list_for_each_entry(mbox, &mbox_cons, node)
  if (mbox->dev->of_node == spec.np) {
- chan = mbox->of_xlate(mbox, &spec);
+ *chan = mbox->of_xlate(mbox, &spec);
  break;
  }

  of_node_put(spec.np);

- if (!chan)
+ if (!(*chan))
  continue;

  ret = -EBUSY;
- if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+ if (!(*chan)->cl && acpi_disabled &&
+ try_module_get(mbox->dev->driver->owner))
  break;
  }

  if (i == count) {
  mutex_unlock(&con_mutex);
- return ERR_PTR(ret);
+ return ret;
  }

- spin_lock_irqsave(&chan->lock, flags);
- chan->msg_free = 0;
- chan->msg_count = 0;
- chan->active_req = NULL;
- chan->cl = cl;
- if (!cl->tx_tout) /* wait for ever */
- cl->tx_tout = msecs_to_jiffies(3600000);
+ return init_channel(*chan, cl);
+}
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ * ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+ struct mbox_chan *chan = NULL;
+ int ret;
+
+ mutex_lock(&con_mutex);
+
+ if (acpi_disabled)
+ ret = get_of_mbox_chan(cl, &chan);
  else
- cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
- if (chan->txdone_method == TXDONE_BY_POLL
- && cl->knows_txdone)
- chan->txdone_method |= TXDONE_BY_ACK;
- spin_unlock_irqrestore(&chan->lock, flags);
+ ret = get_acpi_mbox_chan(cl, &chan);

- ret = chan->mbox->ops->startup(chan);
- if (ret) {
- pr_err("Unable to startup the chan\n");
- mbox_free_channel(chan);
- chan = ERR_PTR(ret);
- }
+ if (ret)
+ pr_err("No mailbox channels found\n");

  mutex_unlock(&con_mutex);
  return chan;
@@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan)
  if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
  chan->txdone_method = TXDONE_BY_POLL;

- module_put(chan->mbox->dev->driver->owner);
+ if (chan->mbox->dev)
+ module_put(chan->mbox->dev->driver->owner);
+
  spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
@@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox)
  int i, txdone;

  /* Sanity check */
- if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+ if (!mbox || !mbox->ops || !mbox->num_chans)
+ return -EINVAL;
+
+ /*
+ * For ACPI platforms, to get mbox->dev, we'd need to
+ * have a fake meaningless entry in the DSDT for the
+ * mailbox controller.
+ */
+ if (acpi_disabled && !mbox->dev)
  return -EINVAL;

  if (mbox->txdone_irq)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..716fbae 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -29,7 +29,7 @@ struct mbox_chan;
  */
 struct mbox_client {
  struct device *dev;
- const char *chan_name;
+ char *chan_name;
  void (*rx_callback)(struct mbox_client *cl, void *mssg);
  void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
  bool tx_block;
diff --git a/include/linux/mailbox_controller.h
b/include/linux/mailbox_controller.h
index cf81e80..06476ef 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,7 @@ struct mbox_controller {
  unsigned period;
  /* Hook to add to the global controller list */
  struct list_head node;
+ char *name;
 } __aligned(32);

 /*
-- 
1.8.3.2





-------------------------8<-------------------------------

Btw I havent rebased to v7 of the mailbox patchwork yet, since I'd
like to hear feedback on the direction I'm taking with ACPI before I
rebase.


Cheers,
Ashwin

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hello,

On 12 June 2014 13:14, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>>
>>> +#ifndef CONFIG_ACPI
>>>         if (!mbox->of_xlate)
>>>                 mbox->of_xlate = of_mbox_index_xlate;
>>> -
>>> +#endif
>>>         mutex_lock(&con_mutex);
>>>         list_add_tail(&mbox->node, &mbox_cons);
>>>         mutex_unlock(&con_mutex);
>>>
>>
>> You can't do #ifndef here, the driver must still work if
>> both OF and ACPI are enabled.
>
> Ok. Here we could skip the macro altogether, since of_xlate wont be
> called with ACPI anyway.
>
> Will need to look into using/faking mbox->dev to remove the other ifndefs.


So, in order to get an mbox->dev for ACPI platforms, we'd need an
entry in the DSDT table. That seems rather pointless, since the DSDT
is reserved for devices and is supposed to be OS agnostic. Since the
mailbox controller itself is not really a "device" with a resource
descriptor, I dont see the point in adding a dummy DSDT entry for the
sake of getting this `struct device`. Also, I'm told adding new
entries to this table requires registering a unique 4 character
identifier and approval from some committees. If there are other ways
to get this structure I'd like to hear about it.

The other alternative would be to piggy back on the ACPI CPU detection
code, which looks for the ACPI0007 device node in the DSDT and use
that as the mbox controller device. This node is already registered
and is an established method to detect CPUs. But I'm not sure what
happens when CPUs are hotplugged off, we surely dont want mailbox
clients such as PCC to break.

The third alternative is to ignore the dev refcounts for ACPI as shown
in this totally untested patch:

----------------8<----------------------------------------


>From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Date: Wed, 11 Jun 2014 16:09:35 -0400
Subject: [PATCH] Mailbox: Add support for ACPI

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
 include/linux/mailbox_client.h     |   2 +-
 include/linux/mailbox_controller.h |   1 +
 3 files changed, 129 insertions(+), 51 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..e2704f5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>

@@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan,
void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);

-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- * ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+ struct mbox_client *cl)
+{
+ unsigned long flags;
+ int ret;
+
+ if (!chan) {
+ pr_err("No mailbox channel specified\n");
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&chan->lock, flags);
+ chan->msg_free = 0;
+ chan->msg_count = 0;
+ chan->active_req = NULL;
+ chan->cl = cl;
+
+ if (!cl->tx_tout) /* wait for ever */
+ cl->tx_tout = msecs_to_jiffies(3600000);
+ else
+ cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+ if (chan->txdone_method == TXDONE_BY_POLL
+ && cl->knows_txdone)
+ chan->txdone_method |= TXDONE_BY_ACK;
+ spin_unlock_irqrestore(&chan->lock, flags);
+
+ ret = chan->mbox->ops->startup(chan);
+ if (ret) {
+ pr_err("Unable to startup the chan\n");
+ mbox_free_channel(chan);
+ chan = ERR_PTR(ret);
+ }
+
+ return ret;
+}
+
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+ struct mbox_chan **chan)
+{
+ struct mbox_controller *mbox;
+ int chan_id, ret, len;
+ char *chan_ptr;
+
+ if (!cl->chan_name)
+ return -ENODEV;
+
+ list_for_each_entry(mbox, &mbox_cons, node) {
+ if (mbox->name) {
+ /*
+ * The cl->chan_name has the format => controller:channel
+ * as described in mailbox_client.h
+ */
+
+ len = strlen(mbox->name);
+ chan_ptr = cl->chan_name + len + 1;
+
+ ret = kstrtou32(cl->chan_name, 0, &chan_id);
+
+ if (ret < 0) {
+ pr_err("Err while parsing mailbox:%s channel idx\n",
+ mbox->name);
+ continue;
+ }
+
+ if (!strncmp(cl->chan_name, mbox->name, len)) {
+ *chan = &mbox->chans[chan_id];
+ return init_channel(*chan, cl);
+ }
+ }
+ }
+
+ return -ENODEV;
+}
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+ struct mbox_chan **chan)
 {
  struct device *dev = cl->dev;
  struct mbox_controller *mbox;
  struct of_phandle_args spec;
- struct mbox_chan *chan;
- unsigned long flags;
  int count, i, ret;

  if (!dev || !dev->of_node) {
  pr_err("%s: No owner device node\n", __func__);
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
  }

  count = of_property_count_strings(dev->of_node, "mbox-names");
  if (count < 0) {
  pr_err("%s: mbox-names property of node '%s' missing\n",
  __func__, dev->of_node->full_name);
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
  }
-
- mutex_lock(&con_mutex);
-
- ret = -ENODEV;
  for (i = 0; i < count; i++) {
  const char *s;

  if (of_property_read_string_index(dev->of_node,
- "mbox-names", i, &s))
+ "mbox-names", i, &s))
  continue;

  if (strcmp(cl->chan_name, s))
  continue;

  if (of_parse_phandle_with_args(dev->of_node,
- "mbox", "#mbox-cells", i, &spec))
+ "mbox", "#mbox-cells", i, &spec))
  continue;

- chan = NULL;
  list_for_each_entry(mbox, &mbox_cons, node)
  if (mbox->dev->of_node == spec.np) {
- chan = mbox->of_xlate(mbox, &spec);
+ *chan = mbox->of_xlate(mbox, &spec);
  break;
  }

  of_node_put(spec.np);

- if (!chan)
+ if (!(*chan))
  continue;

  ret = -EBUSY;
- if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+ if (!(*chan)->cl && acpi_disabled &&
+ try_module_get(mbox->dev->driver->owner))
  break;
  }

  if (i == count) {
  mutex_unlock(&con_mutex);
- return ERR_PTR(ret);
+ return ret;
  }

- spin_lock_irqsave(&chan->lock, flags);
- chan->msg_free = 0;
- chan->msg_count = 0;
- chan->active_req = NULL;
- chan->cl = cl;
- if (!cl->tx_tout) /* wait for ever */
- cl->tx_tout = msecs_to_jiffies(3600000);
+ return init_channel(*chan, cl);
+}
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ * ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+ struct mbox_chan *chan = NULL;
+ int ret;
+
+ mutex_lock(&con_mutex);
+
+ if (acpi_disabled)
+ ret = get_of_mbox_chan(cl, &chan);
  else
- cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
- if (chan->txdone_method == TXDONE_BY_POLL
- && cl->knows_txdone)
- chan->txdone_method |= TXDONE_BY_ACK;
- spin_unlock_irqrestore(&chan->lock, flags);
+ ret = get_acpi_mbox_chan(cl, &chan);

- ret = chan->mbox->ops->startup(chan);
- if (ret) {
- pr_err("Unable to startup the chan\n");
- mbox_free_channel(chan);
- chan = ERR_PTR(ret);
- }
+ if (ret)
+ pr_err("No mailbox channels found\n");

  mutex_unlock(&con_mutex);
  return chan;
@@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan)
  if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
  chan->txdone_method = TXDONE_BY_POLL;

- module_put(chan->mbox->dev->driver->owner);
+ if (chan->mbox->dev)
+ module_put(chan->mbox->dev->driver->owner);
+
  spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
@@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox)
  int i, txdone;

  /* Sanity check */
- if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+ if (!mbox || !mbox->ops || !mbox->num_chans)
+ return -EINVAL;
+
+ /*
+ * For ACPI platforms, to get mbox->dev, we'd need to
+ * have a fake meaningless entry in the DSDT for the
+ * mailbox controller.
+ */
+ if (acpi_disabled && !mbox->dev)
  return -EINVAL;

  if (mbox->txdone_irq)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..716fbae 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -29,7 +29,7 @@ struct mbox_chan;
  */
 struct mbox_client {
  struct device *dev;
- const char *chan_name;
+ char *chan_name;
  void (*rx_callback)(struct mbox_client *cl, void *mssg);
  void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
  bool tx_block;
diff --git a/include/linux/mailbox_controller.h
b/include/linux/mailbox_controller.h
index cf81e80..06476ef 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,7 @@ struct mbox_controller {
  unsigned period;
  /* Hook to add to the global controller list */
  struct list_head node;
+ char *name;
 } __aligned(32);

 /*
-- 
1.8.3.2





-------------------------8<-------------------------------

Btw I havent rebased to v7 of the mailbox patchwork yet, since I'd
like to hear feedback on the direction I'm taking with ACPI before I
rebase.


Cheers,
Ashwin

Re: [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On 20 June 2014 14:55, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> Hello,
>
> On 12 June 2014 13:14, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
>> On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>>>
>>>> +#ifndef CONFIG_ACPI
>>>>         if (!mbox->of_xlate)
>>>>                 mbox->of_xlate = of_mbox_index_xlate;
>>>> -
>>>> +#endif
>>>>         mutex_lock(&con_mutex);
>>>>         list_add_tail(&mbox->node, &mbox_cons);
>>>>         mutex_unlock(&con_mutex);
>>>>
>>>
>>> You can't do #ifndef here, the driver must still work if
>>> both OF and ACPI are enabled.
>>
>> Ok. Here we could skip the macro altogether, since of_xlate wont be
>> called with ACPI anyway.
>>
>> Will need to look into using/faking mbox->dev to remove the other ifndefs.
>
>
> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> entry in the DSDT table. That seems rather pointless, since the DSDT
> is reserved for devices and is supposed to be OS agnostic. Since the
> mailbox controller itself is not really a "device" with a resource
> descriptor, I dont see the point in adding a dummy DSDT entry for the
> sake of getting this `struct device`. Also, I'm told adding new
> entries to this table requires registering a unique 4 character
> identifier and approval from some committees. If there are other ways
> to get this structure I'd like to hear about it.
>
> The other alternative would be to piggy back on the ACPI CPU detection
> code, which looks for the ACPI0007 device node in the DSDT and use
> that as the mbox controller device. This node is already registered
> and is an established method to detect CPUs. But I'm not sure what
> happens when CPUs are hotplugged off, we surely dont want mailbox
> clients such as PCC to break.
>
> The third alternative is to ignore the dev refcounts for ACPI as shown
> in this totally untested patch:
>
> ----------------8<----------------------------------------
>
>
> From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
> From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> Date: Wed, 11 Jun 2014 16:09:35 -0400
> Subject: [PATCH] Mailbox: Add support for ACPI
>
> The current mailbox framework only supports DT based bindings.
> Add another mechanism for mailbox clients to register with mailbox
> controllers and request for specific mailbox channels. This enables
> usage of the mailbox framework on kernels with ACPI support.
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
>  include/linux/mailbox_client.h     |   2 +-
>  include/linux/mailbox_controller.h |   1 +
>  3 files changed, 129 insertions(+), 51 deletions(-)
>

> -------------------------8<-------------------------------
>
> Btw I havent rebased to v7 of the mailbox patchwork yet, since I'd
> like to hear feedback on the direction I'm taking with ACPI before I
> rebase.
>
>

That probably messed up the formatting.. Attaching the patch.

[-- Attachment #2: 0001-Mailbox-Add-support-for-ACPI.patch --]
[-- Type: text/x-patch, Size: 8409 bytes --]

From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Date: Wed, 11 Jun 2014 16:09:35 -0400
Subject: [PATCH] Mailbox: Add support for ACPI

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
 include/linux/mailbox_client.h     |   2 +-
 include/linux/mailbox_controller.h |   1 +
 3 files changed, 129 insertions(+), 51 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..e2704f5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>
 
@@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- *		ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+		struct mbox_client *cl)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!chan) {
+		pr_err("No mailbox channel specified\n");
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+
+	if (!cl->tx_tout) /* wait for ever */
+		cl->tx_tout = msecs_to_jiffies(3600000);
+	else
+		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+	if (chan->txdone_method	== TXDONE_BY_POLL
+			&& cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan\n");
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	return ret;
+}
+
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
+{
+	struct mbox_controller *mbox;
+	int chan_id, ret, len;
+	char *chan_ptr;
+
+	if (!cl->chan_name)
+		return -ENODEV;
+
+	list_for_each_entry(mbox, &mbox_cons, node) {
+		if (mbox->name) {
+			/*
+			 * The cl->chan_name has the format => controller:channel
+			 * as described in mailbox_client.h
+			 */
+
+			len = strlen(mbox->name);
+			chan_ptr = cl->chan_name + len + 1;
+
+			ret = kstrtou32(cl->chan_name, 0, &chan_id);
+
+			if (ret < 0) {
+				pr_err("Err while parsing mailbox:%s channel idx\n",
+						mbox->name);
+				continue;
+			}
+
+			if (!strncmp(cl->chan_name, mbox->name, len)) {
+				*chan = &mbox->chans[chan_id];
+				return init_channel(*chan, cl);
+			}
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
 {
 	struct device *dev = cl->dev;
 	struct mbox_controller *mbox;
 	struct of_phandle_args spec;
-	struct mbox_chan *chan;
-	unsigned long flags;
 	int count, i, ret;
 
 	if (!dev || !dev->of_node) {
 		pr_err("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	count = of_property_count_strings(dev->of_node, "mbox-names");
 	if (count < 0) {
 		pr_err("%s: mbox-names property of node '%s' missing\n",
 			__func__, dev->of_node->full_name);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
-
-	mutex_lock(&con_mutex);
-
-	ret = -ENODEV;
 	for (i = 0; i < count; i++) {
 		const char *s;
 
 		if (of_property_read_string_index(dev->of_node,
-						"mbox-names", i, &s))
+					"mbox-names", i, &s))
 			continue;
 
 		if (strcmp(cl->chan_name, s))
 			continue;
 
 		if (of_parse_phandle_with_args(dev->of_node,
-					 "mbox", "#mbox-cells",	i, &spec))
+					"mbox", "#mbox-cells",	i, &spec))
 			continue;
 
-		chan = NULL;
 		list_for_each_entry(mbox, &mbox_cons, node)
 			if (mbox->dev->of_node == spec.np) {
-				chan = mbox->of_xlate(mbox, &spec);
+				*chan = mbox->of_xlate(mbox, &spec);
 				break;
 			}
 
 		of_node_put(spec.np);
 
-		if (!chan)
+		if (!(*chan))
 			continue;
 
 		ret = -EBUSY;
-		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+		if (!(*chan)->cl && acpi_disabled &&
+				try_module_get(mbox->dev->driver->owner))
 			break;
 	}
 
 	if (i == count) {
 		mutex_unlock(&con_mutex);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
-	spin_lock_irqsave(&chan->lock, flags);
-	chan->msg_free = 0;
-	chan->msg_count = 0;
-	chan->active_req = NULL;
-	chan->cl = cl;
-	if (!cl->tx_tout) /* wait for ever */
-		cl->tx_tout = msecs_to_jiffies(3600000);
+	return init_channel(*chan, cl);
+}
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+	struct mbox_chan *chan = NULL;
+	int ret;
+
+	mutex_lock(&con_mutex);
+
+	if (acpi_disabled)
+		ret = get_of_mbox_chan(cl, &chan);
 	else
-		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
-	if (chan->txdone_method	== TXDONE_BY_POLL
-			&& cl->knows_txdone)
-		chan->txdone_method |= TXDONE_BY_ACK;
-	spin_unlock_irqrestore(&chan->lock, flags);
+		ret = get_acpi_mbox_chan(cl, &chan);
 
-	ret = chan->mbox->ops->startup(chan);
-	if (ret) {
-		pr_err("Unable to startup the chan\n");
-		mbox_free_channel(chan);
-		chan = ERR_PTR(ret);
-	}
+	if (ret)
+		pr_err("No mailbox channels found\n");
 
 	mutex_unlock(&con_mutex);
 	return chan;
@@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
-	module_put(chan->mbox->dev->driver->owner);
+	if (chan->mbox->dev)
+		module_put(chan->mbox->dev->driver->owner);
+
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
@@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	int i, txdone;
 
 	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+	if (!mbox || !mbox->ops || !mbox->num_chans)
+		return -EINVAL;
+
+	/*
+	 * For ACPI platforms, to get mbox->dev, we'd need to 
+	 * have a fake meaningless entry in the DSDT for the
+	 * mailbox controller.
+	 */
+	if (acpi_disabled && !mbox->dev)
 		return -EINVAL;
 
 	if (mbox->txdone_irq)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..716fbae 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -29,7 +29,7 @@ struct mbox_chan;
  */
 struct mbox_client {
 	struct device *dev;
-	const char *chan_name;
+	char *chan_name;
 	void (*rx_callback)(struct mbox_client *cl, void *mssg);
 	void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
 	bool tx_block;
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index cf81e80..06476ef 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,7 @@ struct mbox_controller {
 	unsigned period;
 	/* Hook to add to the global controller list */
 	struct list_head node;
+	char *name;
 } __aligned(32);
 
 /*
-- 
1.8.3.2

[RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

On 20 June 2014 14:55, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> Hello,
>
> On 12 June 2014 13:14, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
>> On 12 June 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thursday 12 June 2014 12:48:10 Ashwin Chaugule wrote:
>>>>
>>>> +#ifndef CONFIG_ACPI
>>>>         if (!mbox->of_xlate)
>>>>                 mbox->of_xlate = of_mbox_index_xlate;
>>>> -
>>>> +#endif
>>>>         mutex_lock(&con_mutex);
>>>>         list_add_tail(&mbox->node, &mbox_cons);
>>>>         mutex_unlock(&con_mutex);
>>>>
>>>
>>> You can't do #ifndef here, the driver must still work if
>>> both OF and ACPI are enabled.
>>
>> Ok. Here we could skip the macro altogether, since of_xlate wont be
>> called with ACPI anyway.
>>
>> Will need to look into using/faking mbox->dev to remove the other ifndefs.
>
>
> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> entry in the DSDT table. That seems rather pointless, since the DSDT
> is reserved for devices and is supposed to be OS agnostic. Since the
> mailbox controller itself is not really a "device" with a resource
> descriptor, I dont see the point in adding a dummy DSDT entry for the
> sake of getting this `struct device`. Also, I'm told adding new
> entries to this table requires registering a unique 4 character
> identifier and approval from some committees. If there are other ways
> to get this structure I'd like to hear about it.
>
> The other alternative would be to piggy back on the ACPI CPU detection
> code, which looks for the ACPI0007 device node in the DSDT and use
> that as the mbox controller device. This node is already registered
> and is an established method to detect CPUs. But I'm not sure what
> happens when CPUs are hotplugged off, we surely dont want mailbox
> clients such as PCC to break.
>
> The third alternative is to ignore the dev refcounts for ACPI as shown
> in this totally untested patch:
>
> ----------------8<----------------------------------------
>
>
> From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
> From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> Date: Wed, 11 Jun 2014 16:09:35 -0400
> Subject: [PATCH] Mailbox: Add support for ACPI
>
> The current mailbox framework only supports DT based bindings.
> Add another mechanism for mailbox clients to register with mailbox
> controllers and request for specific mailbox channels. This enables
> usage of the mailbox framework on kernels with ACPI support.
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
>  include/linux/mailbox_client.h     |   2 +-
>  include/linux/mailbox_controller.h |   1 +
>  3 files changed, 129 insertions(+), 51 deletions(-)
>

> -------------------------8<-------------------------------
>
> Btw I havent rebased to v7 of the mailbox patchwork yet, since I'd
> like to hear feedback on the direction I'm taking with ACPI before I
> rebase.
>
>

That probably messed up the formatting.. Attaching the patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Mailbox-Add-support-for-ACPI.patch
Type: text/x-patch
Size: 8409 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140620/4018cce4/attachment.bin>

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> entry in the DSDT table. That seems rather pointless, since the DSDT
> is reserved for devices and is supposed to be OS agnostic. Since the
> mailbox controller itself is not really a "device" with a resource
> descriptor, I dont see the point in adding a dummy DSDT entry for the
> sake of getting this `struct device`. Also, I'm told adding new
> entries to this table requires registering a unique 4 character
> identifier and approval from some committees. If there are other ways
> to get this structure I'd like to hear about it.
> 
> The other alternative would be to piggy back on the ACPI CPU detection
> code, which looks for the ACPI0007 device node in the DSDT and use
> that as the mbox controller device. This node is already registered
> and is an established method to detect CPUs. But I'm not sure what
> happens when CPUs are hotplugged off, we surely dont want mailbox
> clients such as PCC to break.

The main question here is whether you expect having to support multiple
mailbox devices in an ACPI system. If you think there is never more than
one, you wouldn't need a DSDT entry, but if you can end up in a situation
where another device needs to specify which mailbox it is using, then
you need that entry anyway.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> entry in the DSDT table. That seems rather pointless, since the DSDT
> is reserved for devices and is supposed to be OS agnostic. Since the
> mailbox controller itself is not really a "device" with a resource
> descriptor, I dont see the point in adding a dummy DSDT entry for the
> sake of getting this `struct device`. Also, I'm told adding new
> entries to this table requires registering a unique 4 character
> identifier and approval from some committees. If there are other ways
> to get this structure I'd like to hear about it.
> 
> The other alternative would be to piggy back on the ACPI CPU detection
> code, which looks for the ACPI0007 device node in the DSDT and use
> that as the mbox controller device. This node is already registered
> and is an established method to detect CPUs. But I'm not sure what
> happens when CPUs are hotplugged off, we surely dont want mailbox
> clients such as PCC to break.

The main question here is whether you expect having to support multiple
mailbox devices in an ACPI system. If you think there is never more than
one, you wouldn't need a DSDT entry, but if you can end up in a situation
where another device needs to specify which mailbox it is using, then
you need that entry anyway.

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
>> So, in order to get an mbox->dev for ACPI platforms, we'd need an
>> entry in the DSDT table. That seems rather pointless, since the DSDT
>> is reserved for devices and is supposed to be OS agnostic. Since the
>> mailbox controller itself is not really a "device" with a resource
>> descriptor, I dont see the point in adding a dummy DSDT entry for the
>> sake of getting this `struct device`. Also, I'm told adding new
>> entries to this table requires registering a unique 4 character
>> identifier and approval from some committees. If there are other ways
>> to get this structure I'd like to hear about it.
>>
>> The other alternative would be to piggy back on the ACPI CPU detection
>> code, which looks for the ACPI0007 device node in the DSDT and use
>> that as the mbox controller device. This node is already registered
>> and is an established method to detect CPUs. But I'm not sure what
>> happens when CPUs are hotplugged off, we surely dont want mailbox
>> clients such as PCC to break.
>
> The main question here is whether you expect having to support multiple
> mailbox devices in an ACPI system. If you think there is never more than
> one, you wouldn't need a DSDT entry, but if you can end up in a situation
> where another device needs to specify which mailbox it is using, then
> you need that entry anyway.

At this point, I dont see the need for multiple mailbox devices. But
I'm not seeing why we'd need a DSDT entry only if there are more than
one mailbox devices? I'd obviously prefer not having a DSDT entry for
this, and the patch I posted is the only way I could see to keep DT
and ACPI mbox supported at runtime without DSDT involved. Please let
me know if there are better ways.

Cheers,
Ashwin

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
>> So, in order to get an mbox->dev for ACPI platforms, we'd need an
>> entry in the DSDT table. That seems rather pointless, since the DSDT
>> is reserved for devices and is supposed to be OS agnostic. Since the
>> mailbox controller itself is not really a "device" with a resource
>> descriptor, I dont see the point in adding a dummy DSDT entry for the
>> sake of getting this `struct device`. Also, I'm told adding new
>> entries to this table requires registering a unique 4 character
>> identifier and approval from some committees. If there are other ways
>> to get this structure I'd like to hear about it.
>>
>> The other alternative would be to piggy back on the ACPI CPU detection
>> code, which looks for the ACPI0007 device node in the DSDT and use
>> that as the mbox controller device. This node is already registered
>> and is an established method to detect CPUs. But I'm not sure what
>> happens when CPUs are hotplugged off, we surely dont want mailbox
>> clients such as PCC to break.
>
> The main question here is whether you expect having to support multiple
> mailbox devices in an ACPI system. If you think there is never more than
> one, you wouldn't need a DSDT entry, but if you can end up in a situation
> where another device needs to specify which mailbox it is using, then
> you need that entry anyway.

At this point, I dont see the need for multiple mailbox devices. But
I'm not seeing why we'd need a DSDT entry only if there are more than
one mailbox devices? I'd obviously prefer not having a DSDT entry for
this, and the patch I posted is the only way I could see to keep DT
and ACPI mbox supported at runtime without DSDT involved. Please let
me know if there are better ways.

Cheers,
Ashwin

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 15:29:18 Ashwin Chaugule wrote:
> 
> On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
> >> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> >> entry in the DSDT table. That seems rather pointless, since the DSDT
> >> is reserved for devices and is supposed to be OS agnostic. Since the
> >> mailbox controller itself is not really a "device" with a resource
> >> descriptor, I dont see the point in adding a dummy DSDT entry for the
> >> sake of getting this `struct device`. Also, I'm told adding new
> >> entries to this table requires registering a unique 4 character
> >> identifier and approval from some committees. If there are other ways
> >> to get this structure I'd like to hear about it.
> >>
> >> The other alternative would be to piggy back on the ACPI CPU detection
> >> code, which looks for the ACPI0007 device node in the DSDT and use
> >> that as the mbox controller device. This node is already registered
> >> and is an established method to detect CPUs. But I'm not sure what
> >> happens when CPUs are hotplugged off, we surely dont want mailbox
> >> clients such as PCC to break.
> >
> > The main question here is whether you expect having to support multiple
> > mailbox devices in an ACPI system. If you think there is never more than
> > one, you wouldn't need a DSDT entry, but if you can end up in a situation
> > where another device needs to specify which mailbox it is using, then
> > you need that entry anyway.
> 
> At this point, I dont see the need for multiple mailbox devices. But
> I'm not seeing why we'd need a DSDT entry only if there are more than
> one mailbox devices? I'd obviously prefer not having a DSDT entry for
> this, and the patch I posted is the only way I could see to keep DT
> and ACPI mbox supported at runtime without DSDT involved. Please let
> me know if there are better ways.

It's mostly a matter of consistency: We can have multiple interrupt
controllers, pin controllers, clock controllers, dma engines, etc,
and in the DT case we use references to the nodes wherever we have
other devices referring to a mailbox name.

I believe Intel's embedded chips are moving in the same direction
with their ACPI support. If the ACPI spec gains support for mailbox
devices, locking them into having only a single device may be
a problem later for them.

Note that "device" here doesn't have to mean a platform device that
is instantiated from DSDT, it can be any mailbox provider that is
registered in an arbitrary way, as long as you have a method to map
back from the (consumer-device, name-string) tuple back to the
(provider, channel) tuple. I have read your patch again now and noticed
that you actually tried to do this, but unfortunately you got it
wrong by requiring the consumer to fill out the name of the provider
in the request. You can't do that, because it's not generic enough
to support devices that can be reused, and it means that drivers
using the API are never portable between DT and ACPI. You have to
get rid of the "ctrl_name" field in the mbox_client structure and
change the lookup to be based only on cd->dev and cl->chan_name,
using whatever tables you have available in ACPI.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 15:29:18 Ashwin Chaugule wrote:
> 
> On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
> >> So, in order to get an mbox->dev for ACPI platforms, we'd need an
> >> entry in the DSDT table. That seems rather pointless, since the DSDT
> >> is reserved for devices and is supposed to be OS agnostic. Since the
> >> mailbox controller itself is not really a "device" with a resource
> >> descriptor, I dont see the point in adding a dummy DSDT entry for the
> >> sake of getting this `struct device`. Also, I'm told adding new
> >> entries to this table requires registering a unique 4 character
> >> identifier and approval from some committees. If there are other ways
> >> to get this structure I'd like to hear about it.
> >>
> >> The other alternative would be to piggy back on the ACPI CPU detection
> >> code, which looks for the ACPI0007 device node in the DSDT and use
> >> that as the mbox controller device. This node is already registered
> >> and is an established method to detect CPUs. But I'm not sure what
> >> happens when CPUs are hotplugged off, we surely dont want mailbox
> >> clients such as PCC to break.
> >
> > The main question here is whether you expect having to support multiple
> > mailbox devices in an ACPI system. If you think there is never more than
> > one, you wouldn't need a DSDT entry, but if you can end up in a situation
> > where another device needs to specify which mailbox it is using, then
> > you need that entry anyway.
> 
> At this point, I dont see the need for multiple mailbox devices. But
> I'm not seeing why we'd need a DSDT entry only if there are more than
> one mailbox devices? I'd obviously prefer not having a DSDT entry for
> this, and the patch I posted is the only way I could see to keep DT
> and ACPI mbox supported at runtime without DSDT involved. Please let
> me know if there are better ways.

It's mostly a matter of consistency: We can have multiple interrupt
controllers, pin controllers, clock controllers, dma engines, etc,
and in the DT case we use references to the nodes wherever we have
other devices referring to a mailbox name.

I believe Intel's embedded chips are moving in the same direction
with their ACPI support. If the ACPI spec gains support for mailbox
devices, locking them into having only a single device may be
a problem later for them.

Note that "device" here doesn't have to mean a platform device that
is instantiated from DSDT, it can be any mailbox provider that is
registered in an arbitrary way, as long as you have a method to map
back from the (consumer-device, name-string) tuple back to the
(provider, channel) tuple. I have read your patch again now and noticed
that you actually tried to do this, but unfortunately you got it
wrong by requiring the consumer to fill out the name of the provider
in the request. You can't do that, because it's not generic enough
to support devices that can be reused, and it means that drivers
using the API are never portable between DT and ACPI. You have to
get rid of the "ctrl_name" field in the mbox_client structure and
change the lookup to be based only on cd->dev and cl->chan_name,
using whatever tables you have available in ACPI.

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]

Hello,

On 20 June 2014 16:49, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 June 2014 15:29:18 Ashwin Chaugule wrote:
>>
>> On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
>> >> So, in order to get an mbox->dev for ACPI platforms, we'd need an
>> >> entry in the DSDT table. That seems rather pointless, since the DSDT
>> >> is reserved for devices and is supposed to be OS agnostic. Since the
>> >> mailbox controller itself is not really a "device" with a resource
>> >> descriptor, I dont see the point in adding a dummy DSDT entry for the
>> >> sake of getting this `struct device`. Also, I'm told adding new
>> >> entries to this table requires registering a unique 4 character
>> >> identifier and approval from some committees. If there are other ways
>> >> to get this structure I'd like to hear about it.
>> >>
>> >> The other alternative would be to piggy back on the ACPI CPU detection
>> >> code, which looks for the ACPI0007 device node in the DSDT and use
>> >> that as the mbox controller device. This node is already registered
>> >> and is an established method to detect CPUs. But I'm not sure what
>> >> happens when CPUs are hotplugged off, we surely dont want mailbox
>> >> clients such as PCC to break.
>> >
>> > The main question here is whether you expect having to support multiple
>> > mailbox devices in an ACPI system. If you think there is never more than
>> > one, you wouldn't need a DSDT entry, but if you can end up in a situation
>> > where another device needs to specify which mailbox it is using, then
>> > you need that entry anyway.
>>
>> At this point, I dont see the need for multiple mailbox devices. But
>> I'm not seeing why we'd need a DSDT entry only if there are more than
>> one mailbox devices? I'd obviously prefer not having a DSDT entry for
>> this, and the patch I posted is the only way I could see to keep DT
>> and ACPI mbox supported at runtime without DSDT involved. Please let
>> me know if there are better ways.
>
> It's mostly a matter of consistency: We can have multiple interrupt
> controllers, pin controllers, clock controllers, dma engines, etc,
> and in the DT case we use references to the nodes wherever we have
> other devices referring to a mailbox name.
>
> I believe Intel's embedded chips are moving in the same direction
> with their ACPI support. If the ACPI spec gains support for mailbox
> devices, locking them into having only a single device may be
> a problem later for them.
>
> Note that "device" here doesn't have to mean a platform device that
> is instantiated from DSDT, it can be any mailbox provider that is
> registered in an arbitrary way, as long as you have a method to map
> back from the (consumer-device, name-string) tuple back to the
> (provider, channel) tuple. I have read your patch again now and noticed
> that you actually tried to do this, but unfortunately you got it
> wrong by requiring the consumer to fill out the name of the provider
> in the request. You can't do that, because it's not generic enough
> to support devices that can be reused, and it means that drivers
> using the API are never portable between DT and ACPI. You have to
> get rid of the "ctrl_name" field in the mbox_client structure and
> change the lookup to be based only on cd->dev and cl->chan_name,
> using whatever tables you have available in ACPI.

I think you looked at the previous version of the patch. I'm attaching
the latest version here again FWIW. In this version, I removed the
"ctrl_name" field and rely on the cl->chan_name to provide the info as
described in Jassi' original patch.


linux/mailbox_client.h

 18  * struct mbox_client - User of a mailbox
 19  * @dev:        The client device
 20  * @chan_name:      The "controller:channel" this client wants

Instead of dev, I added a name string to the mbox controller
structure. So now the client gets its channel by requesting
"controller:channel" where controller should match with mbox->name and
channel becomes an index into mbox->chans[].


Cheers,
Ashwin

[-- Attachment #2: 0001-Mailbox-Add-support-for-ACPI.patch --]
[-- Type: text/x-patch, Size: 8409 bytes --]

From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001
From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Date: Wed, 11 Jun 2014 16:09:35 -0400
Subject: [PATCH] Mailbox: Add support for ACPI

The current mailbox framework only supports DT based bindings.
Add another mechanism for mailbox clients to register with mailbox
controllers and request for specific mailbox channels. This enables
usage of the mailbox framework on kernels with ACPI support.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/mailbox.c          | 177 ++++++++++++++++++++++++++-----------
 include/linux/mailbox_client.h     |   2 +-
 include/linux/mailbox_controller.h |   1 +
 3 files changed, 129 insertions(+), 51 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index d83d12c..e2704f5 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>
 
@@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
-/**
- * mbox_request_channel - Request a mailbox channel.
- * @cl: Identity of the client requesting the channel.
- *
- * The Client specifies its requirements and capabilities while asking for
- * a mailbox channel by name. It can't be called from atomic context.
- * The channel is exclusively allocated and can't be used by another
- * client before the owner calls mbox_free_channel.
- * After assignment, any packet received on this channel will be
- * handed over to the client via the 'rx_callback'.
- *
- * Return: Pointer to the channel assigned to the client if successful.
- *		ERR_PTR for request failure.
- */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+static int init_channel(struct mbox_chan *chan,
+		struct mbox_client *cl)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!chan) {
+		pr_err("No mailbox channel specified\n");
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&chan->lock, flags);
+	chan->msg_free = 0;
+	chan->msg_count = 0;
+	chan->active_req = NULL;
+	chan->cl = cl;
+
+	if (!cl->tx_tout) /* wait for ever */
+		cl->tx_tout = msecs_to_jiffies(3600000);
+	else
+		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
+	if (chan->txdone_method	== TXDONE_BY_POLL
+			&& cl->knows_txdone)
+		chan->txdone_method |= TXDONE_BY_ACK;
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	ret = chan->mbox->ops->startup(chan);
+	if (ret) {
+		pr_err("Unable to startup the chan\n");
+		mbox_free_channel(chan);
+		chan = ERR_PTR(ret);
+	}
+
+	return ret;
+}
+
+static int get_acpi_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
+{
+	struct mbox_controller *mbox;
+	int chan_id, ret, len;
+	char *chan_ptr;
+
+	if (!cl->chan_name)
+		return -ENODEV;
+
+	list_for_each_entry(mbox, &mbox_cons, node) {
+		if (mbox->name) {
+			/*
+			 * The cl->chan_name has the format => controller:channel
+			 * as described in mailbox_client.h
+			 */
+
+			len = strlen(mbox->name);
+			chan_ptr = cl->chan_name + len + 1;
+
+			ret = kstrtou32(cl->chan_name, 0, &chan_id);
+
+			if (ret < 0) {
+				pr_err("Err while parsing mailbox:%s channel idx\n",
+						mbox->name);
+				continue;
+			}
+
+			if (!strncmp(cl->chan_name, mbox->name, len)) {
+				*chan = &mbox->chans[chan_id];
+				return init_channel(*chan, cl);
+			}
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int get_of_mbox_chan(struct mbox_client *cl,
+		struct mbox_chan **chan)
 {
 	struct device *dev = cl->dev;
 	struct mbox_controller *mbox;
 	struct of_phandle_args spec;
-	struct mbox_chan *chan;
-	unsigned long flags;
 	int count, i, ret;
 
 	if (!dev || !dev->of_node) {
 		pr_err("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	count = of_property_count_strings(dev->of_node, "mbox-names");
 	if (count < 0) {
 		pr_err("%s: mbox-names property of node '%s' missing\n",
 			__func__, dev->of_node->full_name);
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
-
-	mutex_lock(&con_mutex);
-
-	ret = -ENODEV;
 	for (i = 0; i < count; i++) {
 		const char *s;
 
 		if (of_property_read_string_index(dev->of_node,
-						"mbox-names", i, &s))
+					"mbox-names", i, &s))
 			continue;
 
 		if (strcmp(cl->chan_name, s))
 			continue;
 
 		if (of_parse_phandle_with_args(dev->of_node,
-					 "mbox", "#mbox-cells",	i, &spec))
+					"mbox", "#mbox-cells",	i, &spec))
 			continue;
 
-		chan = NULL;
 		list_for_each_entry(mbox, &mbox_cons, node)
 			if (mbox->dev->of_node == spec.np) {
-				chan = mbox->of_xlate(mbox, &spec);
+				*chan = mbox->of_xlate(mbox, &spec);
 				break;
 			}
 
 		of_node_put(spec.np);
 
-		if (!chan)
+		if (!(*chan))
 			continue;
 
 		ret = -EBUSY;
-		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+		if (!(*chan)->cl && acpi_disabled &&
+				try_module_get(mbox->dev->driver->owner))
 			break;
 	}
 
 	if (i == count) {
 		mutex_unlock(&con_mutex);
-		return ERR_PTR(ret);
+		return ret;
 	}
 
-	spin_lock_irqsave(&chan->lock, flags);
-	chan->msg_free = 0;
-	chan->msg_count = 0;
-	chan->active_req = NULL;
-	chan->cl = cl;
-	if (!cl->tx_tout) /* wait for ever */
-		cl->tx_tout = msecs_to_jiffies(3600000);
+	return init_channel(*chan, cl);
+}
+
+/**
+ * mbox_request_channel - Request a mailbox channel.
+ * @cl: Identity of the client requesting the channel.
+ *
+ * The Client specifies its requirements and capabilities while asking for
+ * a mailbox channel by name. It can't be called from atomic context.
+ * The channel is exclusively allocated and can't be used by another
+ * client before the owner calls mbox_free_channel.
+ * After assignment, any packet received on this channel will be
+ * handed over to the client via the 'rx_callback'.
+ *
+ * Return: Pointer to the channel assigned to the client if successful.
+ *		ERR_PTR for request failure.
+ */
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+{
+	struct mbox_chan *chan = NULL;
+	int ret;
+
+	mutex_lock(&con_mutex);
+
+	if (acpi_disabled)
+		ret = get_of_mbox_chan(cl, &chan);
 	else
-		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);
-	if (chan->txdone_method	== TXDONE_BY_POLL
-			&& cl->knows_txdone)
-		chan->txdone_method |= TXDONE_BY_ACK;
-	spin_unlock_irqrestore(&chan->lock, flags);
+		ret = get_acpi_mbox_chan(cl, &chan);
 
-	ret = chan->mbox->ops->startup(chan);
-	if (ret) {
-		pr_err("Unable to startup the chan\n");
-		mbox_free_channel(chan);
-		chan = ERR_PTR(ret);
-	}
+	if (ret)
+		pr_err("No mailbox channels found\n");
 
 	mutex_unlock(&con_mutex);
 	return chan;
@@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
 		chan->txdone_method = TXDONE_BY_POLL;
 
-	module_put(chan->mbox->dev->driver->owner);
+	if (chan->mbox->dev)
+		module_put(chan->mbox->dev->driver->owner);
+
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
@@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	int i, txdone;
 
 	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+	if (!mbox || !mbox->ops || !mbox->num_chans)
+		return -EINVAL;
+
+	/*
+	 * For ACPI platforms, to get mbox->dev, we'd need to 
+	 * have a fake meaningless entry in the DSDT for the
+	 * mailbox controller.
+	 */
+	if (acpi_disabled && !mbox->dev)
 		return -EINVAL;
 
 	if (mbox->txdone_irq)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index bbac2d2..716fbae 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -29,7 +29,7 @@ struct mbox_chan;
  */
 struct mbox_client {
 	struct device *dev;
-	const char *chan_name;
+	char *chan_name;
 	void (*rx_callback)(struct mbox_client *cl, void *mssg);
 	void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r);
 	bool tx_block;
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index cf81e80..06476ef 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -78,6 +78,7 @@ struct mbox_controller {
 	unsigned period;
 	/* Hook to add to the global controller list */
 	struct list_head node;
+	char *name;
 } __aligned(32);
 
 /*
-- 
1.8.3.2

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hello,

On 20 June 2014 16:49, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 June 2014 15:29:18 Ashwin Chaugule wrote:
>>
>> On 20 June 2014 15:08, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 20 June 2014 14:55:16 Ashwin Chaugule wrote:
>> >> So, in order to get an mbox->dev for ACPI platforms, we'd need an
>> >> entry in the DSDT table. That seems rather pointless, since the DSDT
>> >> is reserved for devices and is supposed to be OS agnostic. Since the
>> >> mailbox controller itself is not really a "device" with a resource
>> >> descriptor, I dont see the point in adding a dummy DSDT entry for the
>> >> sake of getting this `struct device`. Also, I'm told adding new
>> >> entries to this table requires registering a unique 4 character
>> >> identifier and approval from some committees. If there are other ways
>> >> to get this structure I'd like to hear about it.
>> >>
>> >> The other alternative would be to piggy back on the ACPI CPU detection
>> >> code, which looks for the ACPI0007 device node in the DSDT and use
>> >> that as the mbox controller device. This node is already registered
>> >> and is an established method to detect CPUs. But I'm not sure what
>> >> happens when CPUs are hotplugged off, we surely dont want mailbox
>> >> clients such as PCC to break.
>> >
>> > The main question here is whether you expect having to support multiple
>> > mailbox devices in an ACPI system. If you think there is never more than
>> > one, you wouldn't need a DSDT entry, but if you can end up in a situation
>> > where another device needs to specify which mailbox it is using, then
>> > you need that entry anyway.
>>
>> At this point, I dont see the need for multiple mailbox devices. But
>> I'm not seeing why we'd need a DSDT entry only if there are more than
>> one mailbox devices? I'd obviously prefer not having a DSDT entry for
>> this, and the patch I posted is the only way I could see to keep DT
>> and ACPI mbox supported at runtime without DSDT involved. Please let
>> me know if there are better ways.
>
> It's mostly a matter of consistency: We can have multiple interrupt
> controllers, pin controllers, clock controllers, dma engines, etc,
> and in the DT case we use references to the nodes wherever we have
> other devices referring to a mailbox name.
>
> I believe Intel's embedded chips are moving in the same direction
> with their ACPI support. If the ACPI spec gains support for mailbox
> devices, locking them into having only a single device may be
> a problem later for them.
>
> Note that "device" here doesn't have to mean a platform device that
> is instantiated from DSDT, it can be any mailbox provider that is
> registered in an arbitrary way, as long as you have a method to map
> back from the (consumer-device, name-string) tuple back to the
> (provider, channel) tuple. I have read your patch again now and noticed
> that you actually tried to do this, but unfortunately you got it
> wrong by requiring the consumer to fill out the name of the provider
> in the request. You can't do that, because it's not generic enough
> to support devices that can be reused, and it means that drivers
> using the API are never portable between DT and ACPI. You have to
> get rid of the "ctrl_name" field in the mbox_client structure and
> change the lookup to be based only on cd->dev and cl->chan_name,
> using whatever tables you have available in ACPI.

I think you looked at the previous version of the patch. I'm attaching
the latest version here again FWIW. In this version, I removed the
"ctrl_name" field and rely on the cl->chan_name to provide the info as
described in Jassi' original patch.


linux/mailbox_client.h

 18  * struct mbox_client - User of a mailbox
 19  * @dev:        The client device
 20  * @chan_name:      The "controller:channel" this client wants

Instead of dev, I added a name string to the mbox controller
structure. So now the client gets its channel by requesting
"controller:channel" where controller should match with mbox->name and
channel becomes an index into mbox->chans[].


Cheers,
Ashwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Mailbox-Add-support-for-ACPI.patch
Type: text/x-patch
Size: 8409 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140620/5a4ff595/attachment.bin>

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 17:43:05 Ashwin Chaugule wrote:
> >
> > Note that "device" here doesn't have to mean a platform device that
> > is instantiated from DSDT, it can be any mailbox provider that is
> > registered in an arbitrary way, as long as you have a method to map
> > back from the (consumer-device, name-string) tuple back to the
> > (provider, channel) tuple. I have read your patch again now and noticed
> > that you actually tried to do this, but unfortunately you got it
> > wrong by requiring the consumer to fill out the name of the provider
> > in the request. You can't do that, because it's not generic enough
> > to support devices that can be reused, and it means that drivers
> > using the API are never portable between DT and ACPI. You have to
> > get rid of the "ctrl_name" field in the mbox_client structure and
> > change the lookup to be based only on cd->dev and cl->chan_name,
> > using whatever tables you have available in ACPI.
> 
> I think you looked at the previous version of the patch. I'm attaching
> the latest version here again FWIW. In this version, I removed the
> "ctrl_name" field and rely on the cl->chan_name to provide the info as
> described in Jassi' original patch.
> 
> 
> linux/mailbox_client.h
> 
>  18  * struct mbox_client - User of a mailbox
>  19  * @dev:        The client device
>  20  * @chan_name:      The "controller:channel" this client wants
> 
> Instead of dev, I added a name string to the mbox controller
> structure. So now the client gets its channel by requesting
> "controller:channel" where controller should match with mbox->name and
> channel becomes an index into mbox->chans[].
> 

Right, I looked at the wrong version, sorry about that.

However, it seems you still make the same mistake here: The name that
gets passed as chan_name in the mailbox API is a local identifier
that is supposed to be interpreted for the client device and used
to look up a pointer to the mailbox device and channel. If you require
drivers to put global data (e.g. the mbox->name, or the channel
number) in there, it's impossible to write a driver that works on
both DT and ACPI. If you want to use the mbox_request_channel()
interface from a driver, you need some form of lookup table in 
the ACPI data to do the conversion.

The alternative would be not to use mbox_request_channel() at all
for now, but to add a new interface that can only be used PCC and
that matches by ID but is independent of the use of ACPI or DT,
something like:

struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, 
			char *name, unsigned chan_id,
			struct mbox_chan **chan)
{
	struct mbox_controller *mbox;
	mbox = mbox_find_pcc_controller(name, ...);

	*chan = &mbox->chans[chan_id];
	return init_channel(*chan, cl);
}

This would mean that we'd have to special-case "pcc" users, which is
not very nice, but at least it would work on both DT and ACPI,
and a future ACPI version could still add support for the mailbox
API later.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Friday 20 June 2014 17:43:05 Ashwin Chaugule wrote:
> >
> > Note that "device" here doesn't have to mean a platform device that
> > is instantiated from DSDT, it can be any mailbox provider that is
> > registered in an arbitrary way, as long as you have a method to map
> > back from the (consumer-device, name-string) tuple back to the
> > (provider, channel) tuple. I have read your patch again now and noticed
> > that you actually tried to do this, but unfortunately you got it
> > wrong by requiring the consumer to fill out the name of the provider
> > in the request. You can't do that, because it's not generic enough
> > to support devices that can be reused, and it means that drivers
> > using the API are never portable between DT and ACPI. You have to
> > get rid of the "ctrl_name" field in the mbox_client structure and
> > change the lookup to be based only on cd->dev and cl->chan_name,
> > using whatever tables you have available in ACPI.
> 
> I think you looked at the previous version of the patch. I'm attaching
> the latest version here again FWIW. In this version, I removed the
> "ctrl_name" field and rely on the cl->chan_name to provide the info as
> described in Jassi' original patch.
> 
> 
> linux/mailbox_client.h
> 
>  18  * struct mbox_client - User of a mailbox
>  19  * @dev:        The client device
>  20  * @chan_name:      The "controller:channel" this client wants
> 
> Instead of dev, I added a name string to the mbox controller
> structure. So now the client gets its channel by requesting
> "controller:channel" where controller should match with mbox->name and
> channel becomes an index into mbox->chans[].
> 

Right, I looked at the wrong version, sorry about that.

However, it seems you still make the same mistake here: The name that
gets passed as chan_name in the mailbox API is a local identifier
that is supposed to be interpreted for the client device and used
to look up a pointer to the mailbox device and channel. If you require
drivers to put global data (e.g. the mbox->name, or the channel
number) in there, it's impossible to write a driver that works on
both DT and ACPI. If you want to use the mbox_request_channel()
interface from a driver, you need some form of lookup table in 
the ACPI data to do the conversion.

The alternative would be not to use mbox_request_channel() at all
for now, but to add a new interface that can only be used PCC and
that matches by ID but is independent of the use of ACPI or DT,
something like:

struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, 
			char *name, unsigned chan_id,
			struct mbox_chan **chan)
{
	struct mbox_controller *mbox;
	mbox = mbox_find_pcc_controller(name, ...);

	*chan = &mbox->chans[chan_id];
	return init_channel(*chan, cl);
}

This would mean that we'd have to special-case "pcc" users, which is
not very nice, but at least it would work on both DT and ACPI,
and a future ACPI version could still add support for the mailbox
API later.

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> linux/mailbox_client.h
>>
>>  18  * struct mbox_client - User of a mailbox
>>  19  * @dev:        The client device
>>  20  * @chan_name:      The "controller:channel" this client wants

Please correct me if I'm mistaken, but I think this comment in the
header is confusing. It gives the impression that the user is expected
to fill in this field as "controller name: channel id". But, looking
at an example of a DT based mbox client [1] , that doesnt seem to be
the case. And "chan_name" is compared with "mbox-names", which seems
to contain a list of Channel names. The mailbox is then identified by
a separate DT binding : "mbox", which has the mailbox name and the
channel id. So shouldnt this comment not say anything about the
"controller" and the DT binding should be changed to "channel-names",
instead of "mbox-names" to keep things consistent?


>>
>> Instead of dev, I added a name string to the mbox controller
>> structure. So now the client gets its channel by requesting
>> "controller:channel" where controller should match with mbox->name and
>> channel becomes an index into mbox->chans[].
>>
>
> Right, I looked at the wrong version, sorry about that.

No problem. Many thanks for the review.

>
> However, it seems you still make the same mistake here: The name that
> gets passed as chan_name in the mailbox API is a local identifier
> that is supposed to be interpreted for the client device and used
> to look up a pointer to the mailbox device and channel. If you require
> drivers to put global data (e.g. the mbox->name, or the channel
> number) in there, it's impossible to write a driver that works on
> both DT and ACPI. If you want to use the mbox_request_channel()
> interface from a driver, you need some form of lookup table in
> the ACPI data to do the conversion.

Fair point. The more I think about this, it seems that if we want to
use the mailbox framework for ACPI kernels, we should have a PCC
specific bypass, something like the one you suggested below. The ACPI
spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
clients defined as well; CPPC, MPST and RASF. Each of these have their
own ACPI tables and so they dont require special DSDT entries.
Moreover, these PCC client drivers will be very ACPI specific anyway.
So, trying to emulate DT like mbox controller-client matching in ACPI
at this point is rather pointless. It will require creating dummy DSDT
entries for the PCC mailbox controller and PCC mailbox clients which
have their own well defined ACPI tables (and so dont belong in the OS
agnostic DSDT) and then coming up with customized Device Specific
Methods (DSMs) for mbox clients to refer to mbox controllers.

The other alternative is to skip the mailbox framework altogether. One
thing to note is that the PCC driver and its clients should work on
X86, ARMv8 and any other platform that has ACPI support. Currently the
Mailbox framework looks platform agnostic but is tied to DT, so it may
not work well for everyone. But like I mentioned early on, the
framework provides for async notification and queuing which is useful
for PCC, so I'd prefer the PCC specific bypass option.

>
> The alternative would be not to use mbox_request_channel() at all
> for now, but to add a new interface that can only be used PCC and
> that matches by ID but is independent of the use of ACPI or DT,
> something like:
>
> struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
>                         char *name, unsigned chan_id,
>                         struct mbox_chan **chan)
> {
>         struct mbox_controller *mbox;
>         mbox = mbox_find_pcc_controller(name, ...);
>
>         *chan = &mbox->chans[chan_id];
>         return init_channel(*chan, cl);
> }
>
> This would mean that we'd have to special-case "pcc" users, which is
> not very nice, but at least it would work on both DT and ACPI,
> and a future ACPI version could still add support for the mailbox
> API later.

I'll play around with this idea a bit and see how it looks.

Cheers,
Ashwin

[1] - https://github.com/hackerspace/rpi-linux/commit/cd0b9584cbedf46812cfd220ba47d80e86b8b7ea

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> linux/mailbox_client.h
>>
>>  18  * struct mbox_client - User of a mailbox
>>  19  * @dev:        The client device
>>  20  * @chan_name:      The "controller:channel" this client wants

Please correct me if I'm mistaken, but I think this comment in the
header is confusing. It gives the impression that the user is expected
to fill in this field as "controller name: channel id". But, looking
at an example of a DT based mbox client [1] , that doesnt seem to be
the case. And "chan_name" is compared with "mbox-names", which seems
to contain a list of Channel names. The mailbox is then identified by
a separate DT binding : "mbox", which has the mailbox name and the
channel id. So shouldnt this comment not say anything about the
"controller" and the DT binding should be changed to "channel-names",
instead of "mbox-names" to keep things consistent?


>>
>> Instead of dev, I added a name string to the mbox controller
>> structure. So now the client gets its channel by requesting
>> "controller:channel" where controller should match with mbox->name and
>> channel becomes an index into mbox->chans[].
>>
>
> Right, I looked at the wrong version, sorry about that.

No problem. Many thanks for the review.

>
> However, it seems you still make the same mistake here: The name that
> gets passed as chan_name in the mailbox API is a local identifier
> that is supposed to be interpreted for the client device and used
> to look up a pointer to the mailbox device and channel. If you require
> drivers to put global data (e.g. the mbox->name, or the channel
> number) in there, it's impossible to write a driver that works on
> both DT and ACPI. If you want to use the mbox_request_channel()
> interface from a driver, you need some form of lookup table in
> the ACPI data to do the conversion.

Fair point. The more I think about this, it seems that if we want to
use the mailbox framework for ACPI kernels, we should have a PCC
specific bypass, something like the one you suggested below. The ACPI
spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
clients defined as well; CPPC, MPST and RASF. Each of these have their
own ACPI tables and so they dont require special DSDT entries.
Moreover, these PCC client drivers will be very ACPI specific anyway.
So, trying to emulate DT like mbox controller-client matching in ACPI
at this point is rather pointless. It will require creating dummy DSDT
entries for the PCC mailbox controller and PCC mailbox clients which
have their own well defined ACPI tables (and so dont belong in the OS
agnostic DSDT) and then coming up with customized Device Specific
Methods (DSMs) for mbox clients to refer to mbox controllers.

The other alternative is to skip the mailbox framework altogether. One
thing to note is that the PCC driver and its clients should work on
X86, ARMv8 and any other platform that has ACPI support. Currently the
Mailbox framework looks platform agnostic but is tied to DT, so it may
not work well for everyone. But like I mentioned early on, the
framework provides for async notification and queuing which is useful
for PCC, so I'd prefer the PCC specific bypass option.

>
> The alternative would be not to use mbox_request_channel() at all
> for now, but to add a new interface that can only be used PCC and
> that matches by ID but is independent of the use of ACPI or DT,
> something like:
>
> struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
>                         char *name, unsigned chan_id,
>                         struct mbox_chan **chan)
> {
>         struct mbox_controller *mbox;
>         mbox = mbox_find_pcc_controller(name, ...);
>
>         *chan = &mbox->chans[chan_id];
>         return init_channel(*chan, cl);
> }
>
> This would mean that we'd have to special-case "pcc" users, which is
> not very nice, but at least it would work on both DT and ACPI,
> and a future ACPI version could still add support for the mailbox
> API later.

I'll play around with this idea a bit and see how it looks.

Cheers,
Ashwin

[1] - https://github.com/hackerspace/rpi-linux/commit/cd0b9584cbedf46812cfd220ba47d80e86b8b7ea

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 14:25:26 Ashwin Chaugule wrote:
> Hi Arnd,
> 
> On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> linux/mailbox_client.h
> >>
> >>  18  * struct mbox_client - User of a mailbox
> >>  19  * @dev:        The client device
> >>  20  * @chan_name:      The "controller:channel" this client wants
> 
> Please correct me if I'm mistaken, but I think this comment in the
> header is confusing.

Yes, definitely. Thanks for pointing that out.

> It gives the impression that the user is expected
> to fill in this field as "controller name: channel id". But, looking
> at an example of a DT based mbox client [1] , that doesnt seem to be
> the case. And "chan_name" is compared with "mbox-names", which seems
> to contain a list of Channel names. The mailbox is then identified by
> a separate DT binding : "mbox", which has the mailbox name and the
> channel id. So shouldnt this comment not say anything about the
> "controller" and the DT binding should be changed to "channel-names",
> instead of "mbox-names" to keep things consistent?

The comment should be changed, but the property name is good the
way it is. We follow the exact same pattern we have for registers,
interrupts, dma-channels, etc.

> > However, it seems you still make the same mistake here: The name that
> > gets passed as chan_name in the mailbox API is a local identifier
> > that is supposed to be interpreted for the client device and used
> > to look up a pointer to the mailbox device and channel. If you require
> > drivers to put global data (e.g. the mbox->name, or the channel
> > number) in there, it's impossible to write a driver that works on
> > both DT and ACPI. If you want to use the mbox_request_channel()
> > interface from a driver, you need some form of lookup table in
> > the ACPI data to do the conversion.
> 
> Fair point. The more I think about this, it seems that if we want to
> use the mailbox framework for ACPI kernels, we should have a PCC
> specific bypass, something like the one you suggested below. The ACPI
> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> clients defined as well; CPPC, MPST and RASF. Each of these have their
> own ACPI tables and so they dont require special DSDT entries.

Ok, I see. Can you describe what data is in these tables?

> Moreover, these PCC client drivers will be very ACPI specific anyway.
> So, trying to emulate DT like mbox controller-client matching in ACPI
> at this point is rather pointless. It will require creating dummy DSDT
> entries for the PCC mailbox controller and PCC mailbox clients which
> have their own well defined ACPI tables (and so dont belong in the OS
> agnostic DSDT) and then coming up with customized Device Specific
> Methods (DSMs) for mbox clients to refer to mbox controllers.

Actually you wouldn't necessarily need DSDT entries, the ACPI core could
just call platform_device_create() to instantiate the devices based on
the PCC tables.

> The other alternative is to skip the mailbox framework altogether. One
> thing to note is that the PCC driver and its clients should work on
> X86, ARMv8 and any other platform that has ACPI support. Currently the
> Mailbox framework looks platform agnostic but is tied to DT, so it may
> not work well for everyone. But like I mentioned early on, the
> framework provides for async notification and queuing which is useful
> for PCC, so I'd prefer the PCC specific bypass option.

The mailbox API should still work fine without DT, it would be easy
enough to add a lookup mechanism for architectures that create their
own platform devices from hardcoded kernel structures, or from ACPI
tables that are meant to emulate the DT bindings on embedded x86.

But treating PCC special probably does make most sense here, at
least the lookup path.

> > The alternative would be not to use mbox_request_channel() at all
> > for now, but to add a new interface that can only be used PCC and
> > that matches by ID but is independent of the use of ACPI or DT,
> > something like:
> >
> > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
> >                         char *name, unsigned chan_id,
> >                         struct mbox_chan **chan)
> > {
> >         struct mbox_controller *mbox;
> >         mbox = mbox_find_pcc_controller(name, ...);
> >
> >         *chan = &mbox->chans[chan_id];
> >         return init_channel(*chan, cl);
> > }
> >
> > This would mean that we'd have to special-case "pcc" users, which is
> > not very nice, but at least it would work on both DT and ACPI,
> > and a future ACPI version could still add support for the mailbox
> > API later.
> 
> I'll play around with this idea a bit and see how it looks.
> 

Ok, thanks for looking into this.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 14:25:26 Ashwin Chaugule wrote:
> Hi Arnd,
> 
> On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> linux/mailbox_client.h
> >>
> >>  18  * struct mbox_client - User of a mailbox
> >>  19  * @dev:        The client device
> >>  20  * @chan_name:      The "controller:channel" this client wants
> 
> Please correct me if I'm mistaken, but I think this comment in the
> header is confusing.

Yes, definitely. Thanks for pointing that out.

> It gives the impression that the user is expected
> to fill in this field as "controller name: channel id". But, looking
> at an example of a DT based mbox client [1] , that doesnt seem to be
> the case. And "chan_name" is compared with "mbox-names", which seems
> to contain a list of Channel names. The mailbox is then identified by
> a separate DT binding : "mbox", which has the mailbox name and the
> channel id. So shouldnt this comment not say anything about the
> "controller" and the DT binding should be changed to "channel-names",
> instead of "mbox-names" to keep things consistent?

The comment should be changed, but the property name is good the
way it is. We follow the exact same pattern we have for registers,
interrupts, dma-channels, etc.

> > However, it seems you still make the same mistake here: The name that
> > gets passed as chan_name in the mailbox API is a local identifier
> > that is supposed to be interpreted for the client device and used
> > to look up a pointer to the mailbox device and channel. If you require
> > drivers to put global data (e.g. the mbox->name, or the channel
> > number) in there, it's impossible to write a driver that works on
> > both DT and ACPI. If you want to use the mbox_request_channel()
> > interface from a driver, you need some form of lookup table in
> > the ACPI data to do the conversion.
> 
> Fair point. The more I think about this, it seems that if we want to
> use the mailbox framework for ACPI kernels, we should have a PCC
> specific bypass, something like the one you suggested below. The ACPI
> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> clients defined as well; CPPC, MPST and RASF. Each of these have their
> own ACPI tables and so they dont require special DSDT entries.

Ok, I see. Can you describe what data is in these tables?

> Moreover, these PCC client drivers will be very ACPI specific anyway.
> So, trying to emulate DT like mbox controller-client matching in ACPI
> at this point is rather pointless. It will require creating dummy DSDT
> entries for the PCC mailbox controller and PCC mailbox clients which
> have their own well defined ACPI tables (and so dont belong in the OS
> agnostic DSDT) and then coming up with customized Device Specific
> Methods (DSMs) for mbox clients to refer to mbox controllers.

Actually you wouldn't necessarily need DSDT entries, the ACPI core could
just call platform_device_create() to instantiate the devices based on
the PCC tables.

> The other alternative is to skip the mailbox framework altogether. One
> thing to note is that the PCC driver and its clients should work on
> X86, ARMv8 and any other platform that has ACPI support. Currently the
> Mailbox framework looks platform agnostic but is tied to DT, so it may
> not work well for everyone. But like I mentioned early on, the
> framework provides for async notification and queuing which is useful
> for PCC, so I'd prefer the PCC specific bypass option.

The mailbox API should still work fine without DT, it would be easy
enough to add a lookup mechanism for architectures that create their
own platform devices from hardcoded kernel structures, or from ACPI
tables that are meant to emulate the DT bindings on embedded x86.

But treating PCC special probably does make most sense here, at
least the lookup path.

> > The alternative would be not to use mbox_request_channel() at all
> > for now, but to add a new interface that can only be used PCC and
> > that matches by ID but is independent of the use of ACPI or DT,
> > something like:
> >
> > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
> >                         char *name, unsigned chan_id,
> >                         struct mbox_chan **chan)
> > {
> >         struct mbox_controller *mbox;
> >         mbox = mbox_find_pcc_controller(name, ...);
> >
> >         *chan = &mbox->chans[chan_id];
> >         return init_channel(*chan, cl);
> > }
> >
> > This would mean that we'd have to special-case "pcc" users, which is
> > not very nice, but at least it would work on both DT and ACPI,
> > and a future ACPI version could still add support for the mailbox
> > API later.
> 
> I'll play around with this idea a bit and see how it looks.
> 

Ok, thanks for looking into this.

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hello,

On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
>> Fair point. The more I think about this, it seems that if we want to
>> use the mailbox framework for ACPI kernels, we should have a PCC
>> specific bypass, something like the one you suggested below. The ACPI
>> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
>> clients defined as well; CPPC, MPST and RASF. Each of these have their
>> own ACPI tables and so they dont require special DSDT entries.
>
> Ok, I see. Can you describe what data is in these tables?

For CPPC, its a field for version number, number of entries and then
followed by a bunch of PCC entries that have the following structure:

 51 struct pcc_register_resource {
 52     u8 descriptor;
 53     u16 length;
 54     u8 space_id;
 55     u8 bit_width;
 56     u8 bit_offset;
 57     u8 access_size;
 58     u64 address;
 59 } __attribute__ ((packed));

These essentially describe the PCC register space to be used by the
respective protocol. e.g. CPPC uses these to exchange CPU performance
metrics between the OS and the firmware.
I believe MPST and RASF also follow the same format.

>
>> Moreover, these PCC client drivers will be very ACPI specific anyway.
>> So, trying to emulate DT like mbox controller-client matching in ACPI
>> at this point is rather pointless. It will require creating dummy DSDT
>> entries for the PCC mailbox controller and PCC mailbox clients which
>> have their own well defined ACPI tables (and so dont belong in the OS
>> agnostic DSDT) and then coming up with customized Device Specific
>> Methods (DSMs) for mbox clients to refer to mbox controllers.
>
> Actually you wouldn't necessarily need DSDT entries, the ACPI core could
> just call platform_device_create() to instantiate the devices based on
> the PCC tables.
>
>> The other alternative is to skip the mailbox framework altogether. One
>> thing to note is that the PCC driver and its clients should work on
>> X86, ARMv8 and any other platform that has ACPI support. Currently the
>> Mailbox framework looks platform agnostic but is tied to DT, so it may
>> not work well for everyone. But like I mentioned early on, the
>> framework provides for async notification and queuing which is useful
>> for PCC, so I'd prefer the PCC specific bypass option.
>
> The mailbox API should still work fine without DT, it would be easy
> enough to add a lookup mechanism for architectures that create their
> own platform devices from hardcoded kernel structures, or from ACPI
> tables that are meant to emulate the DT bindings on embedded x86.

Right, a generic lookup method would be useful. I think we should
probably revisit this option when/if there are ACPI cases which use
anything other than the PCC mailbox controller.

>
> But treating PCC special probably does make most sense here, at
> least the lookup path.

Agreed.

>
>> > The alternative would be not to use mbox_request_channel() at all
>> > for now, but to add a new interface that can only be used PCC and
>> > that matches by ID but is independent of the use of ACPI or DT,
>> > something like:
>> >
>> > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
>> >                         char *name, unsigned chan_id,
>> >                         struct mbox_chan **chan)
>> > {
>> >         struct mbox_controller *mbox;
>> >         mbox = mbox_find_pcc_controller(name, ...);
>> >
>> >         *chan = &mbox->chans[chan_id];
>> >         return init_channel(*chan, cl);
>> > }
>> >
>> > This would mean that we'd have to special-case "pcc" users, which is
>> > not very nice, but at least it would work on both DT and ACPI,
>> > and a future ACPI version could still add support for the mailbox
>> > API later.
>>
>> I'll play around with this idea a bit and see how it looks.
>>
>
> Ok, thanks for looking into this.

Cheers,
Ashwin

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hello,

On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
>> Fair point. The more I think about this, it seems that if we want to
>> use the mailbox framework for ACPI kernels, we should have a PCC
>> specific bypass, something like the one you suggested below. The ACPI
>> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
>> clients defined as well; CPPC, MPST and RASF. Each of these have their
>> own ACPI tables and so they dont require special DSDT entries.
>
> Ok, I see. Can you describe what data is in these tables?

For CPPC, its a field for version number, number of entries and then
followed by a bunch of PCC entries that have the following structure:

 51 struct pcc_register_resource {
 52     u8 descriptor;
 53     u16 length;
 54     u8 space_id;
 55     u8 bit_width;
 56     u8 bit_offset;
 57     u8 access_size;
 58     u64 address;
 59 } __attribute__ ((packed));

These essentially describe the PCC register space to be used by the
respective protocol. e.g. CPPC uses these to exchange CPU performance
metrics between the OS and the firmware.
I believe MPST and RASF also follow the same format.

>
>> Moreover, these PCC client drivers will be very ACPI specific anyway.
>> So, trying to emulate DT like mbox controller-client matching in ACPI
>> at this point is rather pointless. It will require creating dummy DSDT
>> entries for the PCC mailbox controller and PCC mailbox clients which
>> have their own well defined ACPI tables (and so dont belong in the OS
>> agnostic DSDT) and then coming up with customized Device Specific
>> Methods (DSMs) for mbox clients to refer to mbox controllers.
>
> Actually you wouldn't necessarily need DSDT entries, the ACPI core could
> just call platform_device_create() to instantiate the devices based on
> the PCC tables.
>
>> The other alternative is to skip the mailbox framework altogether. One
>> thing to note is that the PCC driver and its clients should work on
>> X86, ARMv8 and any other platform that has ACPI support. Currently the
>> Mailbox framework looks platform agnostic but is tied to DT, so it may
>> not work well for everyone. But like I mentioned early on, the
>> framework provides for async notification and queuing which is useful
>> for PCC, so I'd prefer the PCC specific bypass option.
>
> The mailbox API should still work fine without DT, it would be easy
> enough to add a lookup mechanism for architectures that create their
> own platform devices from hardcoded kernel structures, or from ACPI
> tables that are meant to emulate the DT bindings on embedded x86.

Right, a generic lookup method would be useful. I think we should
probably revisit this option when/if there are ACPI cases which use
anything other than the PCC mailbox controller.

>
> But treating PCC special probably does make most sense here, at
> least the lookup path.

Agreed.

>
>> > The alternative would be not to use mbox_request_channel() at all
>> > for now, but to add a new interface that can only be used PCC and
>> > that matches by ID but is independent of the use of ACPI or DT,
>> > something like:
>> >
>> > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl,
>> >                         char *name, unsigned chan_id,
>> >                         struct mbox_chan **chan)
>> > {
>> >         struct mbox_controller *mbox;
>> >         mbox = mbox_find_pcc_controller(name, ...);
>> >
>> >         *chan = &mbox->chans[chan_id];
>> >         return init_channel(*chan, cl);
>> > }
>> >
>> > This would mean that we'd have to special-case "pcc" users, which is
>> > not very nice, but at least it would work on both DT and ACPI,
>> > and a future ACPI version could still add support for the mailbox
>> > API later.
>>
>> I'll play around with this idea a bit and see how it looks.
>>
>
> Ok, thanks for looking into this.

Cheers,
Ashwin

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
> Hello,
> 
> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
> >> Fair point. The more I think about this, it seems that if we want to
> >> use the mailbox framework for ACPI kernels, we should have a PCC
> >> specific bypass, something like the one you suggested below. The ACPI
> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
> >> own ACPI tables and so they dont require special DSDT entries.
> >
> > Ok, I see. Can you describe what data is in these tables?
> 
> For CPPC, its a field for version number, number of entries and then
> followed by a bunch of PCC entries that have the following structure:
> 
>  51 struct pcc_register_resource {
>  52     u8 descriptor;
>  53     u16 length;
>  54     u8 space_id;
>  55     u8 bit_width;
>  56     u8 bit_offset;
>  57     u8 access_size;
>  58     u64 address;
>  59 } __attribute__ ((packed));
> 
> These essentially describe the PCC register space to be used by the
> respective protocol. e.g. CPPC uses these to exchange CPU performance
> metrics between the OS and the firmware.
> I believe MPST and RASF also follow the same format.

Interesting. So I guess it's one entry per client of the PCC? How
exactly does the client know which index to use in this table?

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
> Hello,
> 
> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
> >> Fair point. The more I think about this, it seems that if we want to
> >> use the mailbox framework for ACPI kernels, we should have a PCC
> >> specific bypass, something like the one you suggested below. The ACPI
> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
> >> own ACPI tables and so they dont require special DSDT entries.
> >
> > Ok, I see. Can you describe what data is in these tables?
> 
> For CPPC, its a field for version number, number of entries and then
> followed by a bunch of PCC entries that have the following structure:
> 
>  51 struct pcc_register_resource {
>  52     u8 descriptor;
>  53     u16 length;
>  54     u8 space_id;
>  55     u8 bit_width;
>  56     u8 bit_offset;
>  57     u8 access_size;
>  58     u64 address;
>  59 } __attribute__ ((packed));
> 
> These essentially describe the PCC register space to be used by the
> respective protocol. e.g. CPPC uses these to exchange CPU performance
> metrics between the OS and the firmware.
> I believe MPST and RASF also follow the same format.

Interesting. So I guess it's one entry per client of the PCC? How
exactly does the client know which index to use in this table?

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
>> Hello,
>>
>> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> Fair point. The more I think about this, it seems that if we want to
>> >> use the mailbox framework for ACPI kernels, we should have a PCC
>> >> specific bypass, something like the one you suggested below. The ACPI
>> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
>> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
>> >> own ACPI tables and so they dont require special DSDT entries.
>> >
>> > Ok, I see. Can you describe what data is in these tables?
>>
>> For CPPC, its a field for version number, number of entries and then
>> followed by a bunch of PCC entries that have the following structure:
>>
>>  51 struct pcc_register_resource {
>>  52     u8 descriptor;
>>  53     u16 length;
>>  54     u8 space_id;
>>  55     u8 bit_width;
>>  56     u8 bit_offset;
>>  57     u8 access_size;
>>  58     u64 address;
>>  59 } __attribute__ ((packed));
>>
>> These essentially describe the PCC register space to be used by the
>> respective protocol. e.g. CPPC uses these to exchange CPU performance
>> metrics between the OS and the firmware.
>> I believe MPST and RASF also follow the same format.
>
> Interesting. So I guess it's one entry per client of the PCC? How
> exactly does the client know which index to use in this table?

Nah. Just when you'd think - surely there cant be any more 4 letter
ACPI acronyms. ;)

CPPC is one PCC client by itself.

There is a table called PCCT, which describes a list of all PCC
subspaces. Each subspace is like a shared mem region dedicated to a
PCC client. e.g. a platform may choose subspace id 1 for CPPC. The
CPPC has its own table(s) (in case of CPPC, it is a table per CPU),
which lists the PCC registers using the structure above. These
registers reside in the dedicated PCC subspace.

The PCC client drivers will probe their own tables and parse the PCC
register entries from within. e.g. the CPPC driver will look for the
CPC table (per CPU) and parse its PCC registers. Each PCC register
entry reuses the "access_size" field to indicate a PCC subspace index.
This is the index into the PCCT list of subspaces. From here the
client can get the base address of its shared mem region and the
doorbell semantics for communication.

Cheers,
Ashwin

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
>> Hello,
>>
>> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> Fair point. The more I think about this, it seems that if we want to
>> >> use the mailbox framework for ACPI kernels, we should have a PCC
>> >> specific bypass, something like the one you suggested below. The ACPI
>> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
>> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
>> >> own ACPI tables and so they dont require special DSDT entries.
>> >
>> > Ok, I see. Can you describe what data is in these tables?
>>
>> For CPPC, its a field for version number, number of entries and then
>> followed by a bunch of PCC entries that have the following structure:
>>
>>  51 struct pcc_register_resource {
>>  52     u8 descriptor;
>>  53     u16 length;
>>  54     u8 space_id;
>>  55     u8 bit_width;
>>  56     u8 bit_offset;
>>  57     u8 access_size;
>>  58     u64 address;
>>  59 } __attribute__ ((packed));
>>
>> These essentially describe the PCC register space to be used by the
>> respective protocol. e.g. CPPC uses these to exchange CPU performance
>> metrics between the OS and the firmware.
>> I believe MPST and RASF also follow the same format.
>
> Interesting. So I guess it's one entry per client of the PCC? How
> exactly does the client know which index to use in this table?

Nah. Just when you'd think - surely there cant be any more 4 letter
ACPI acronyms. ;)

CPPC is one PCC client by itself.

There is a table called PCCT, which describes a list of all PCC
subspaces. Each subspace is like a shared mem region dedicated to a
PCC client. e.g. a platform may choose subspace id 1 for CPPC. The
CPPC has its own table(s) (in case of CPPC, it is a table per CPU),
which lists the PCC registers using the structure above. These
registers reside in the dedicated PCC subspace.

The PCC client drivers will probe their own tables and parse the PCC
register entries from within. e.g. the CPPC driver will look for the
CPC table (per CPU) and parse its PCC registers. Each PCC register
entry reuses the "access_size" field to indicate a PCC subspace index.
This is the index into the PCCT list of subspaces. From here the
client can get the base address of its shared mem region and the
doorbell semantics for communication.

Cheers,
Ashwin

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 17:27:21 Ashwin Chaugule wrote:
> On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
> >> Hello,
> >>
> >> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >> Fair point. The more I think about this, it seems that if we want to
> >> >> use the mailbox framework for ACPI kernels, we should have a PCC
> >> >> specific bypass, something like the one you suggested below. The ACPI
> >> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> >> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
> >> >> own ACPI tables and so they dont require special DSDT entries.
> >> >
> >> > Ok, I see. Can you describe what data is in these tables?
> >>
> >> For CPPC, its a field for version number, number of entries and then
> >> followed by a bunch of PCC entries that have the following structure:
> >>
> >>  51 struct pcc_register_resource {
> >>  52     u8 descriptor;
> >>  53     u16 length;
> >>  54     u8 space_id;
> >>  55     u8 bit_width;
> >>  56     u8 bit_offset;
> >>  57     u8 access_size;
> >>  58     u64 address;
> >>  59 } __attribute__ ((packed));
> >>
> >> These essentially describe the PCC register space to be used by the
> >> respective protocol. e.g. CPPC uses these to exchange CPU performance
> >> metrics between the OS and the firmware.
> >> I believe MPST and RASF also follow the same format.
> >
> > Interesting. So I guess it's one entry per client of the PCC? How
> > exactly does the client know which index to use in this table?
> 
> Nah. Just when you'd think - surely there cant be any more 4 letter
> ACPI acronyms. ;)
> 
> CPPC is one PCC client by itself.

Ok, I see.

> There is a table called PCCT, which describes a list of all PCC
> subspaces. Each subspace is like a shared mem region dedicated to a
> PCC client. e.g. a platform may choose subspace id 1 for CPPC. The
> CPPC has its own table(s) (in case of CPPC, it is a table per CPU),
> which lists the PCC registers using the structure above. These
> registers reside in the dedicated PCC subspace.
>
> The PCC client drivers will probe their own tables and parse the PCC
> register entries from within. e.g. the CPPC driver will look for the
> CPC table (per CPU) and parse its PCC registers. Each PCC register
> entry reuses the "access_size" field to indicate a PCC subspace index.
> This is the index into the PCCT list of subspaces. From here the
> client can get the base address of its shared mem region and the
> doorbell semantics for communication.

I think a model that's closer to the mailbox subsystem would imply
that the common mailbox code (or the pcc driver itself) parses the
PCCT table, while the slave driver only passes an index.
Or even better, the slave driver would only pass a device pointer,
from which the pcc driver can find the pcc_register_resource in
the corresponding ACPI table. The name of that table can be the
string you pass down to the mailbox API. I suspect there is some
issue that makes this all break down though, but that would be
a portable way to do this for both DT and ACPI:

If we wanted to use DT with the same driver, we would put the
name of the table containing pcc_register_resource into the
mbox-names property, and that could get used to look up a
reference to the pcc device, and to the other data that you
have in pcc_register_resource and PCCT.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Monday 23 June 2014 17:27:21 Ashwin Chaugule wrote:
> On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote:
> >> Hello,
> >>
> >> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >> Fair point. The more I think about this, it seems that if we want to
> >> >> use the mailbox framework for ACPI kernels, we should have a PCC
> >> >> specific bypass, something like the one you suggested below. The ACPI
> >> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC
> >> >> clients defined as well; CPPC, MPST and RASF. Each of these have their
> >> >> own ACPI tables and so they dont require special DSDT entries.
> >> >
> >> > Ok, I see. Can you describe what data is in these tables?
> >>
> >> For CPPC, its a field for version number, number of entries and then
> >> followed by a bunch of PCC entries that have the following structure:
> >>
> >>  51 struct pcc_register_resource {
> >>  52     u8 descriptor;
> >>  53     u16 length;
> >>  54     u8 space_id;
> >>  55     u8 bit_width;
> >>  56     u8 bit_offset;
> >>  57     u8 access_size;
> >>  58     u64 address;
> >>  59 } __attribute__ ((packed));
> >>
> >> These essentially describe the PCC register space to be used by the
> >> respective protocol. e.g. CPPC uses these to exchange CPU performance
> >> metrics between the OS and the firmware.
> >> I believe MPST and RASF also follow the same format.
> >
> > Interesting. So I guess it's one entry per client of the PCC? How
> > exactly does the client know which index to use in this table?
> 
> Nah. Just when you'd think - surely there cant be any more 4 letter
> ACPI acronyms. ;)
> 
> CPPC is one PCC client by itself.

Ok, I see.

> There is a table called PCCT, which describes a list of all PCC
> subspaces. Each subspace is like a shared mem region dedicated to a
> PCC client. e.g. a platform may choose subspace id 1 for CPPC. The
> CPPC has its own table(s) (in case of CPPC, it is a table per CPU),
> which lists the PCC registers using the structure above. These
> registers reside in the dedicated PCC subspace.
>
> The PCC client drivers will probe their own tables and parse the PCC
> register entries from within. e.g. the CPPC driver will look for the
> CPC table (per CPU) and parse its PCC registers. Each PCC register
> entry reuses the "access_size" field to indicate a PCC subspace index.
> This is the index into the PCCT list of subspaces. From here the
> client can get the base address of its shared mem region and the
> doorbell semantics for communication.

I think a model that's closer to the mailbox subsystem would imply
that the common mailbox code (or the pcc driver itself) parses the
PCCT table, while the slave driver only passes an index.
Or even better, the slave driver would only pass a device pointer,
from which the pcc driver can find the pcc_register_resource in
the corresponding ACPI table. The name of that table can be the
string you pass down to the mailbox API. I suspect there is some
issue that makes this all break down though, but that would be
a portable way to do this for both DT and ACPI:

If we wanted to use DT with the same driver, we would put the
name of the table containing pcc_register_resource into the
mbox-names property, and that could get used to look up a
reference to the pcc device, and to the other data that you
have in pcc_register_resource and PCCT.

	Arnd

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 06/24/2014 10:18 AM, Arnd Bergmann wrote:

>
> I think a model that's closer to the mailbox subsystem would imply
> that the common mailbox code (or the pcc driver itself) parses the
> PCCT table, while the slave driver only passes an index.

This is pretty close to what I have here:

The PCC mailbox version:

https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review


Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller 
which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC 
client that sends dummy PCC reads/writes.


The PCC + CPPC non-mailbox version:

https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev

Here, the CPPC is the PCC client driver which parses the CPC tables. The 
mailbox conversion for this stuff is a WIP. But it should give an idea 
of how PCC and PCC clients would work.

> Or even better, the slave driver would only pass a device pointer,
> from which the pcc driver can find the pcc_register_resource in
> the corresponding ACPI table. The name of that table can be the
> string you pass down to the mailbox API. I suspect there is some
> issue that makes this all break down though, but that would be
> a portable way to do this for both DT and ACPI:
>
> If we wanted to use DT with the same driver, we would put the
> name of the table containing pcc_register_resource into the
> mbox-names property, and that could get used to look up a
> reference to the pcc device, and to the other data that you
> have in pcc_register_resource and PCCT.

So I dont think we should worry about the PCC clients being used in the 
DT case, since the PCC and its client specification is very ACPI centric 
and platforms that want to use these drivers will need an ACPI based 
firmware anyway. Which is why I think having a separate PCC specific 
mbox API makes sense.

Something like what you suggested should work well for ACPI based platforms.

struct mbox_controller *
mbox_find_pcc_controller(char *name)
{
	list_for_each_entry(mbox, &mbox_cons, node) {
		if (mbox->name)
			if (!strncmp(mbox->name, name))
				return mbox;
	}

	return -ENODEV;
}

int pcc_mbox_get_channel(struct mbox_client *cl,
		char *name, unsigned chan_id,
		struct mbox_chan **chan)
{
	struct mbox_controller *mbox;
	mbox = mbox_find_pcc_controller(name);

	if (!mbox) {
		pr_err("PCC mbox %s not found.\n", name);
		return -ENODEV;
	}

	*chan = &mbox->chans[chan_id];
	return init_channel(*chan, cl);
}


Cheers,
Ashwin

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Ashwin Chaugule]

Hi Arnd,

On 06/24/2014 10:18 AM, Arnd Bergmann wrote:

>
> I think a model that's closer to the mailbox subsystem would imply
> that the common mailbox code (or the pcc driver itself) parses the
> PCCT table, while the slave driver only passes an index.

This is pretty close to what I have here:

The PCC mailbox version:

https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review


Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller 
which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC 
client that sends dummy PCC reads/writes.


The PCC + CPPC non-mailbox version:

https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev

Here, the CPPC is the PCC client driver which parses the CPC tables. The 
mailbox conversion for this stuff is a WIP. But it should give an idea 
of how PCC and PCC clients would work.

> Or even better, the slave driver would only pass a device pointer,
> from which the pcc driver can find the pcc_register_resource in
> the corresponding ACPI table. The name of that table can be the
> string you pass down to the mailbox API. I suspect there is some
> issue that makes this all break down though, but that would be
> a portable way to do this for both DT and ACPI:
>
> If we wanted to use DT with the same driver, we would put the
> name of the table containing pcc_register_resource into the
> mbox-names property, and that could get used to look up a
> reference to the pcc device, and to the other data that you
> have in pcc_register_resource and PCCT.

So I dont think we should worry about the PCC clients being used in the 
DT case, since the PCC and its client specification is very ACPI centric 
and platforms that want to use these drivers will need an ACPI based 
firmware anyway. Which is why I think having a separate PCC specific 
mbox API makes sense.

Something like what you suggested should work well for ACPI based platforms.

struct mbox_controller *
mbox_find_pcc_controller(char *name)
{
	list_for_each_entry(mbox, &mbox_cons, node) {
		if (mbox->name)
			if (!strncmp(mbox->name, name))
				return mbox;
	}

	return -ENODEV;
}

int pcc_mbox_get_channel(struct mbox_client *cl,
		char *name, unsigned chan_id,
		struct mbox_chan **chan)
{
	struct mbox_controller *mbox;
	mbox = mbox_find_pcc_controller(name);

	if (!mbox) {
		pr_err("PCC mbox %s not found.\n", name);
		return -ENODEV;
	}

	*chan = &mbox->chans[chan_id];
	return init_channel(*chan, cl);
}


Cheers,
Ashwin

Re: [Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Tuesday 24 June 2014 13:55:42 Ashwin Chaugule wrote:
> 
> Hi Arnd,
> 
> On 06/24/2014 10:18 AM, Arnd Bergmann wrote:
> 
> >
> > I think a model that's closer to the mailbox subsystem would imply
> > that the common mailbox code (or the pcc driver itself) parses the
> > PCCT table, while the slave driver only passes an index.
> 
> This is pretty close to what I have here:
> 
> The PCC mailbox version:
> 
> https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review
> 
> 
> Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller 
> which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC 
> client that sends dummy PCC reads/writes.
> 
> 
> The PCC + CPPC non-mailbox version:
> 
> https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev
> 
> Here, the CPPC is the PCC client driver which parses the CPC tables. The 
> mailbox conversion for this stuff is a WIP. But it should give an idea 
> of how PCC and PCC clients would work.

Ok, but unfortunately it seems that there is no way for the CPPC to tell
the PCC driver to pull the index out of the CPPC tables as far as I can
tell.

> > Or even better, the slave driver would only pass a device pointer,
> > from which the pcc driver can find the pcc_register_resource in
> > the corresponding ACPI table. The name of that table can be the
> > string you pass down to the mailbox API. I suspect there is some
> > issue that makes this all break down though, but that would be
> > a portable way to do this for both DT and ACPI:
> >
> > If we wanted to use DT with the same driver, we would put the
> > name of the table containing pcc_register_resource into the
> > mbox-names property, and that could get used to look up a
> > reference to the pcc device, and to the other data that you
> > have in pcc_register_resource and PCCT.
> 
> So I dont think we should worry about the PCC clients being used in the 
> DT case, since the PCC and its client specification is very ACPI centric 
> and platforms that want to use these drivers will need an ACPI based 
> firmware anyway. Which is why I think having a separate PCC specific 
> mbox API makes sense.

I think we should be prepared to add any feature that exists in ACPI
also for DT if the need arises, even if we don't expect it to be
necessary.

There are a number of reasons why you might want to use the drivers
with DT, e.g. board bringup (before firmware is available), or to use
some features of a SoC that cannot be represented in ACPI but that
may be useful for a special-purpose appliance.

> Something like what you suggested should work well for ACPI based platforms.
> 
> struct mbox_controller *
> mbox_find_pcc_controller(char *name)
> {
> 	list_for_each_entry(mbox, &mbox_cons, node) {
> 		if (mbox->name)
> 			if (!strncmp(mbox->name, name))
> 				return mbox;
> 	}
> 
> 	return -ENODEV;
> }
> 
> int pcc_mbox_get_channel(struct mbox_client *cl,
> 		char *name, unsigned chan_id,
> 		struct mbox_chan **chan)
> {
> 	struct mbox_controller *mbox;
> 	mbox = mbox_find_pcc_controller(name);
> 
> 	if (!mbox) {
> 		pr_err("PCC mbox %s not found.\n", name);
> 		return -ENODEV;
> 	}
> 
> 	*chan = &mbox->chans[chan_id];
> 	return init_channel(*chan, cl);
> }

Yes, that seems fine, and it will just work with DT as well
if we need that.

	Arnd

[Linaro-acpi] [RFC v2 1/3] Mailbox: Add support for ACPI

[Arnd Bergmann]

On Tuesday 24 June 2014 13:55:42 Ashwin Chaugule wrote:
> 
> Hi Arnd,
> 
> On 06/24/2014 10:18 AM, Arnd Bergmann wrote:
> 
> >
> > I think a model that's closer to the mailbox subsystem would imply
> > that the common mailbox code (or the pcc driver itself) parses the
> > PCCT table, while the slave driver only passes an index.
> 
> This is pretty close to what I have here:
> 
> The PCC mailbox version:
> 
> https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review
> 
> 
> Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller 
> which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC 
> client that sends dummy PCC reads/writes.
> 
> 
> The PCC + CPPC non-mailbox version:
> 
> https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev
> 
> Here, the CPPC is the PCC client driver which parses the CPC tables. The 
> mailbox conversion for this stuff is a WIP. But it should give an idea 
> of how PCC and PCC clients would work.

Ok, but unfortunately it seems that there is no way for the CPPC to tell
the PCC driver to pull the index out of the CPPC tables as far as I can
tell.

> > Or even better, the slave driver would only pass a device pointer,
> > from which the pcc driver can find the pcc_register_resource in
> > the corresponding ACPI table. The name of that table can be the
> > string you pass down to the mailbox API. I suspect there is some
> > issue that makes this all break down though, but that would be
> > a portable way to do this for both DT and ACPI:
> >
> > If we wanted to use DT with the same driver, we would put the
> > name of the table containing pcc_register_resource into the
> > mbox-names property, and that could get used to look up a
> > reference to the pcc device, and to the other data that you
> > have in pcc_register_resource and PCCT.
> 
> So I dont think we should worry about the PCC clients being used in the 
> DT case, since the PCC and its client specification is very ACPI centric 
> and platforms that want to use these drivers will need an ACPI based 
> firmware anyway. Which is why I think having a separate PCC specific 
> mbox API makes sense.

I think we should be prepared to add any feature that exists in ACPI
also for DT if the need arises, even if we don't expect it to be
necessary.

There are a number of reasons why you might want to use the drivers
with DT, e.g. board bringup (before firmware is available), or to use
some features of a SoC that cannot be represented in ACPI but that
may be useful for a special-purpose appliance.

> Something like what you suggested should work well for ACPI based platforms.
> 
> struct mbox_controller *
> mbox_find_pcc_controller(char *name)
> {
> 	list_for_each_entry(mbox, &mbox_cons, node) {
> 		if (mbox->name)
> 			if (!strncmp(mbox->name, name))
> 				return mbox;
> 	}
> 
> 	return -ENODEV;
> }
> 
> int pcc_mbox_get_channel(struct mbox_client *cl,
> 		char *name, unsigned chan_id,
> 		struct mbox_chan **chan)
> {
> 	struct mbox_controller *mbox;
> 	mbox = mbox_find_pcc_controller(name);
> 
> 	if (!mbox) {
> 		pr_err("PCC mbox %s not found.\n", name);
> 		return -ENODEV;
> 	}
> 
> 	*chan = &mbox->chans[chan_id];
> 	return init_channel(*chan, cl);
> }

Yes, that seems fine, and it will just work with DT as well
if we need that.

	Arnd