[PATCH v3 0/3] PCC: Platform Communication Channel

[PATCH v3 0/3] PCC: Platform Communication Channel

[Ashwin Chaugule]

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

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

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


Changes since V2:
- Rebased on top of git://git.linaro.org/landing-teams/working/fujitsu/integration.git 
	branch mailbox-for-3.17
- Added PCC API to mailbox framework as per Arnd's suggestion to allow usage without ACPI.

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

Ashwin Chaugule (3):
  Mailbox: Add support for PCC mailbox and channels
  Add support for Platform Communication Channel
  PCC-test: Test driver to trigger PCC commands

 drivers/mailbox/Kconfig            |  12 ++
 drivers/mailbox/Makefile           |   2 +
 drivers/mailbox/mailbox.c          | 118 +++++++++++++++++--
 drivers/mailbox/pcc-test.c         | 208 +++++++++++++++++++++++++++++++++
 drivers/mailbox/pcc.c              | 228 +++++++++++++++++++++++++++++++++++++
 include/linux/mailbox_client.h     |   6 +
 include/linux/mailbox_controller.h |   1 +
 7 files changed, 568 insertions(+), 7 deletions(-)
 create mode 100644 drivers/mailbox/pcc-test.c
 create mode 100644 drivers/mailbox/pcc.c

-- 
1.9.1

[PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

The PCC (Platform Communication Channel) is a generic
mailbox to be used by PCC clients such as CPPC, RAS
and MPST as defined in the ACPI 5.0+ spec. This patch
modifies the Mailbox framework to lookup PCC mailbox
controllers and channels such that PCC drivers can work
with or without ACPI support in the kernel.

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

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 63ecc17..09ad488 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -356,6 +356,14 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 }
 EXPORT_SYMBOL_GPL(mbox_request_channel);
 
+static void inline free_channel(struct mbox_chan *chan)
+{
+	chan->cl = NULL;
+	chan->active_req = NULL;
+	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
+		chan->txdone_method = TXDONE_BY_POLL;
+}
+
 /**
  * mbox_free_channel - The client relinquishes control of a mailbox
  *			channel by this call.
@@ -369,14 +377,9 @@ void mbox_free_channel(struct mbox_chan *chan)
 		return;
 
 	chan->mbox->ops->shutdown(chan);
-
 	/* The queued TX requests are simply aborted, no callbacks are made */
 	spin_lock_irqsave(&chan->lock, flags);
-	chan->cl = NULL;
-	chan->active_req = NULL;
-	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
-		chan->txdone_method = TXDONE_BY_POLL;
-
+	free_channel(chan);
 	module_put(chan->mbox->dev->driver->owner);
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
@@ -394,6 +397,97 @@ of_mbox_index_xlate(struct mbox_controller *mbox,
 	return &mbox->chans[ind];
 }
 
+#ifdef CONFIG_PCC
+/*
+ * The PCC (Platform Communication Channel) is
+ * defined in the ACPI 5.0+ spec. It is a generic
+ * mailbox interface between an OS and a Platform
+ * such as a BMC. The PCCT (Mailbox controller) has
+ * its own ACPI specific way to describe PCC clients
+ * and their subspace ids (Mailbox channels/clients).
+ *
+ * The following API is added such that PCC
+ * drivers continue to work with this Mailbox
+ * framework with or without ACPI.
+ */
+
+static struct mbox_controller *
+mbox_find_pcc_controller(char *name)
+{
+	struct mbox_controller *mbox;
+	list_for_each_entry(mbox, &mbox_cons, node) {
+		if (mbox->name)
+			if (!strcmp(mbox->name, name))
+				return mbox;
+	}
+
+	return NULL;
+}
+
+struct mbox_chan *
+pcc_mbox_request_channel(struct mbox_client *cl,
+		char *name, unsigned chan_id)
+{
+	struct mbox_controller *mbox;
+	struct mbox_chan *pcc_chan;
+	unsigned long flags;
+	int ret;
+
+	mutex_lock(&con_mutex);
+	mbox = mbox_find_pcc_controller(name);
+
+	if (!mbox) {
+		pr_err("PCC mbox %s not found.\n", name);
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(-ENODEV);
+	}
+
+	pcc_chan = &mbox->chans[chan_id];
+
+	spin_lock_irqsave(&pcc_chan->lock, flags);
+	pcc_chan->msg_free = 0;
+	pcc_chan->msg_count = 0;
+	pcc_chan->active_req = NULL;
+	pcc_chan->cl = cl;
+	init_completion(&pcc_chan->tx_complete);
+
+	if (pcc_chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
+		pcc_chan->txdone_method |= TXDONE_BY_ACK;
+
+	spin_unlock_irqrestore(&pcc_chan->lock, flags);
+
+	ret = pcc_chan->mbox->ops->startup(pcc_chan);
+	if (ret) {
+		pr_err("Unable to startup the PCC channel (%d)\n", ret);
+		mbox_free_channel(pcc_chan);
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_unlock(&con_mutex);
+
+	return pcc_chan;
+}
+
+EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
+
+void pcc_mbox_free_channel(struct mbox_chan *chan)
+{
+	unsigned long flags;
+
+	if (!chan || !chan->cl)
+		return;
+
+	chan->mbox->ops->shutdown(chan);
+
+	spin_lock_irqsave(&chan->lock, flags);
+	free_channel(chan);
+	spin_unlock_irqrestore(&chan->lock, flags);
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
+
+#endif
+
 /**
  * mbox_controller_register - Register the mailbox controller
  * @mbox:	Pointer to the mailbox controller.
@@ -405,7 +499,17 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	int i, txdone;
 
 	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+
+	/*
+	 * PCC clients and controllers are currently not backed by
+	 * platform device structures.
+	 */
+#ifndef CONFIG_PCC
+	if (!mbox->dev)
+		return -EINVAL;
+#endif
+
+	if (!mbox || !mbox->ops || !mbox->num_chans)
 		return -EINVAL;
 
 	if (mbox->txdone_irq)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index 307d9ca..6a78df0 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -37,6 +37,12 @@ struct mbox_client {
 	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
 };
 
+#ifdef CONFIG_PCC
+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *c,
+		char *name, unsigned int chan_id);
+void pcc_mbox_free_channel(struct mbox_chan *chan); /* may sleep */
+#endif
+
 struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);
 int mbox_send_message(struct mbox_chan *chan, void *mssg);
 void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 9ee195b..9f0ae42 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -81,6 +81,7 @@ struct mbox_controller {
 	unsigned txpoll_period;
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
 				      const struct of_phandle_args *sp);
+	char *name;
 	/* Internal to API */
 	struct timer_list poll;
 	unsigned period;
-- 
1.9.1

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:

> The PCC (Platform Communication Channel) is a generic
> mailbox to be used by PCC clients such as CPPC, RAS
> and MPST as defined in the ACPI 5.0+ spec. This patch
> modifies the Mailbox framework to lookup PCC mailbox
> controllers and channels such that PCC drivers can work
> with or without ACPI support in the kernel.
> 
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/mailbox/mailbox.c          | 118 ++++++++++++++++++++++++++++++++++---
>  include/linux/mailbox_client.h     |   6 ++
>  include/linux/mailbox_controller.h |   1 +
>  3 files changed, 118 insertions(+), 7 deletions(-)

Based on the patch description (adding support for a particular kind of
mailbox) I'd expect to see a new driver or helper library being added to
drivers/mailbox rather than changes in the mailbox core.  If changes in
the core are needed to support this I'd expect to see them broken out as
separate patches.

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

This doesn't look particularly PCC specific?

>  	/* Sanity check */
> -	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
> +
> +	/*
> +	 * PCC clients and controllers are currently not backed by
> +	 * platform device structures.
> +	 */
> +#ifndef CONFIG_PCC
> +	if (!mbox->dev)
> +		return -EINVAL;
> +#endif

It seems better to make this consistent - either enforce it all the time
or don't enforce it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Mark,

On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
>
>> The PCC (Platform Communication Channel) is a generic
>> mailbox to be used by PCC clients such as CPPC, RAS
>> and MPST as defined in the ACPI 5.0+ spec. This patch
>> modifies the Mailbox framework to lookup PCC mailbox
>> controllers and channels such that PCC drivers can work
>> with or without ACPI support in the kernel.
>>
>> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>> ---
>>  drivers/mailbox/mailbox.c          | 118 ++++++++++++++++++++++++++++++++++---
>>  include/linux/mailbox_client.h     |   6 ++
>>  include/linux/mailbox_controller.h |   1 +
>>  3 files changed, 118 insertions(+), 7 deletions(-)
>
> Based on the patch description (adding support for a particular kind of
> mailbox) I'd expect to see a new driver or helper library being added to
> drivers/mailbox rather than changes in the mailbox core.  If changes in
> the core are needed to support this I'd expect to see them broken out as
> separate patches.

[..]

>
>> +static struct mbox_controller *
>> +mbox_find_pcc_controller(char *name)
>> +{
>> +     struct mbox_controller *mbox;
>> +     list_for_each_entry(mbox, &mbox_cons, node) {
>> +             if (mbox->name)
>> +                     if (!strcmp(mbox->name, name))
>> +                             return mbox;
>> +     }
>> +
>> +     return NULL;
>> +}
>
> This doesn't look particularly PCC specific?

Call this mbox_find_controller_by_name() instead?

>
>>       /* Sanity check */
>> -     if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
>> +
>> +     /*
>> +      * PCC clients and controllers are currently not backed by
>> +      * platform device structures.
>> +      */
>> +#ifndef CONFIG_PCC
>> +     if (!mbox->dev)
>> +             return -EINVAL;
>> +#endif
>
> It seems better to make this consistent - either enforce it all the time
> or don't enforce it.

So this is where it got really messy. We're trying to create a
"device" out of something that isn't. The PCCT, which is used as a
mailbox controller here, is a table and not a peripheral device. To
treat this as a device (without faking it by manually putting together
a struct device), would require adding a DSDT entry which is really a
wrong place for it. Are there examples today where drivers manually
create a struct driver and struct device and match them internally?
(i.e. w/o using the generic driver subsystem)

The main reason why I thought this Mailbox framework looked useful
(after you pointed me to it) for PCC was due to its async notification
features. But thats easy and small enough to add to the PCC driver
itself. We can also add a generic controller lookup mechanism in the
PCC driver for anyone who doesn't want to use ACPI. I think thats a
much cleaner way to handle PCC support. Adding PCC as a generic
mailbox controller is turning out to be more messier that we'd
originally imagined.

Cheers,
Ashwin

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
> On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:

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

> > This doesn't look particularly PCC specific?

> Call this mbox_find_controller_by_name() instead?

That certainly looks like what it's doing.  Probably also make the name
that gets passed in const while you're at it.

> >>       /* Sanity check */
> >> -     if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
> >> +
> >> +     /*
> >> +      * PCC clients and controllers are currently not backed by
> >> +      * platform device structures.
> >> +      */
> >> +#ifndef CONFIG_PCC
> >> +     if (!mbox->dev)
> >> +             return -EINVAL;
> >> +#endif

> > It seems better to make this consistent - either enforce it all the time
> > or don't enforce it.

> So this is where it got really messy. We're trying to create a

The messiness is orthogonal to my comment here - either it's legal to
request a mailbox without a device or it isn't, it shouldn't depend on a
random kernel configuration option for a particular mailbox driver which
it is.

> "device" out of something that isn't. The PCCT, which is used as a
> mailbox controller here, is a table and not a peripheral device. To
> treat this as a device (without faking it by manually putting together
> a struct device), would require adding a DSDT entry which is really a
> wrong place for it. Are there examples today where drivers manually
> create a struct driver and struct device and match them internally?
> (i.e. w/o using the generic driver subsystem)

Arguably that's what things like cpufreq end up doing, though people
tend to just shove a device into DT.  Are you sure there isn't any
device at all in ACPI that you could hang this off, looking at my
desktop I see rather a lot of apparently synthetic ACPI devices with
names starting LNX including for example LNXSYSTM:00?

> The main reason why I thought this Mailbox framework looked useful
> (after you pointed me to it) for PCC was due to its async notification
> features. But thats easy and small enough to add to the PCC driver
> itself. We can also add a generic controller lookup mechanism in the
> PCC driver for anyone who doesn't want to use ACPI. I think thats a
> much cleaner way to handle PCC support. Adding PCC as a generic
> mailbox controller is turning out to be more messier that we'd
> originally imagined.

If PCC is described by ACPI tables how would non-ACPI users be able to
use it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Mark,

On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
>> On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
>> > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
>> >>       /* Sanity check */
>> >> -     if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
>> >> +
>> >> +     /*
>> >> +      * PCC clients and controllers are currently not backed by
>> >> +      * platform device structures.
>> >> +      */
>> >> +#ifndef CONFIG_PCC
>> >> +     if (!mbox->dev)
>> >> +             return -EINVAL;
>> >> +#endif
>
>> > It seems better to make this consistent - either enforce it all the time
>> > or don't enforce it.
>
>> So this is where it got really messy. We're trying to create a
>
> The messiness is orthogonal to my comment here - either it's legal to
> request a mailbox without a device or it isn't, it shouldn't depend on a
> random kernel configuration option for a particular mailbox driver which
> it is.
>

Fair enough. This was just to show that PCC is unfortunately not a
good candidate as a generic mailbox controller.

>> "device" out of something that isn't. The PCCT, which is used as a
>> mailbox controller here, is a table and not a peripheral device. To
>> treat this as a device (without faking it by manually putting together
>> a struct device), would require adding a DSDT entry which is really a
>> wrong place for it. Are there examples today where drivers manually
>> create a struct driver and struct device and match them internally?
>> (i.e. w/o using the generic driver subsystem)
>
> Arguably that's what things like cpufreq end up doing, though people
> tend to just shove a device into DT.  Are you sure there isn't any
> device at all in ACPI that you could hang this off, looking at my
> desktop I see rather a lot of apparently synthetic ACPI devices with
> names starting LNX including for example LNXSYSTM:00?

Those are special HIDs defined in the ACPI spec and none of those can
be used to back a device for the PCCT itself, since they're unrelated
to the PCC protocol. The PCCT is defined in the spec as a separate
table and if supported, should be visible in your system in the
PCCT.dsl file. Just for the sake of the Mailbox framework, trying to
represent the PCCT (which is a table) as a mailbox controller device,
would require registering a special HID. That in turn would make an
otherwise OS agnostic thing very Linux specific.

>
>> The main reason why I thought this Mailbox framework looked useful
>> (after you pointed me to it) for PCC was due to its async notification
>> features. But thats easy and small enough to add to the PCC driver
>> itself. We can also add a generic controller lookup mechanism in the
>> PCC driver for anyone who doesn't want to use ACPI. I think thats a
>> much cleaner way to handle PCC support. Adding PCC as a generic
>> mailbox controller is turning out to be more messier that we'd
>> originally imagined.
>
> If PCC is described by ACPI tables how would non-ACPI users be able to
> use it?

Whoever wants to do that, would need to come up with DT bindings that
describe something similar to the PCCT contents. They could possibly
ignore the ACPI specific bits like signature, asl compiler details
etc. (which are only used by ACPI table parsers) and provide the rest
of it. Its essentially an array of structures that point to various
shared memory regions, each of which is owned by a PCC client and
shared with the firmware. The intercommunication between client and
firmware is via a doorbell, which is also described in these entries
and can be implemented as an SGI or similar.

Thanks,
Ashwin

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote:
> On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:

> > The messiness is orthogonal to my comment here - either it's legal to
> > request a mailbox without a device or it isn't, it shouldn't depend on a
> > random kernel configuration option for a particular mailbox driver which
> > it is.

> Fair enough. This was just to show that PCC is unfortunately not a
> good candidate as a generic mailbox controller.

That seems to be a very big leap...

> >> "device" out of something that isn't. The PCCT, which is used as a
> >> mailbox controller here, is a table and not a peripheral device. To
> >> treat this as a device (without faking it by manually putting together
> >> a struct device), would require adding a DSDT entry which is really a
> >> wrong place for it. Are there examples today where drivers manually
> >> create a struct driver and struct device and match them internally?
> >> (i.e. w/o using the generic driver subsystem)

> > Arguably that's what things like cpufreq end up doing, though people
> > tend to just shove a device into DT.  Are you sure there isn't any
> > device at all in ACPI that you could hang this off, looking at my
> > desktop I see rather a lot of apparently synthetic ACPI devices with
> > names starting LNX including for example LNXSYSTM:00?

> Those are special HIDs defined in the ACPI spec and none of those can
> be used to back a device for the PCCT itself, since they're unrelated
> to the PCC protocol. The PCCT is defined in the spec as a separate
> table and if supported, should be visible in your system in the
> PCCT.dsl file. Just for the sake of the Mailbox framework, trying to
> represent the PCCT (which is a table) as a mailbox controller device,
> would require registering a special HID. That in turn would make an
> otherwise OS agnostic thing very Linux specific.

OK, but then there's always the option of just having some code that
runs on init and instantiates a device if it sees the appropriate thing
in the ACPI tables in a similar manner to how HIDs are handled.  It's a
small amount of work but it will generally make life easier if there is
a struct device.

> > If PCC is described by ACPI tables how would non-ACPI users be able to
> > use it?

> Whoever wants to do that, would need to come up with DT bindings that
> describe something similar to the PCCT contents. They could possibly
> ignore the ACPI specific bits like signature, asl compiler details
> etc. (which are only used by ACPI table parsers) and provide the rest
> of it. Its essentially an array of structures that point to various
> shared memory regions, each of which is owned by a PCC client and
> shared with the firmware. The intercommunication between client and
> firmware is via a doorbell, which is also described in these entries
> and can be implemented as an SGI or similar.

Of course most likely such a binding would involve creating a device
that owns the mailboxes so this'd be fairly straightforward...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Mark,

On 28 August 2014 06:10, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote:
>> On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote:
>> > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
>> >> "device" out of something that isn't. The PCCT, which is used as a
>> >> mailbox controller here, is a table and not a peripheral device. To
>> >> treat this as a device (without faking it by manually putting together
>> >> a struct device), would require adding a DSDT entry which is really a
>> >> wrong place for it. Are there examples today where drivers manually
>> >> create a struct driver and struct device and match them internally?
>> >> (i.e. w/o using the generic driver subsystem)
>
>> > Arguably that's what things like cpufreq end up doing, though people
>> > tend to just shove a device into DT.  Are you sure there isn't any
>> > device at all in ACPI that you could hang this off, looking at my
>> > desktop I see rather a lot of apparently synthetic ACPI devices with
>> > names starting LNX including for example LNXSYSTM:00?
>
>> Those are special HIDs defined in the ACPI spec and none of those can
>> be used to back a device for the PCCT itself, since they're unrelated
>> to the PCC protocol. The PCCT is defined in the spec as a separate
>> table and if supported, should be visible in your system in the
>> PCCT.dsl file. Just for the sake of the Mailbox framework, trying to
>> represent the PCCT (which is a table) as a mailbox controller device,
>> would require registering a special HID. That in turn would make an
>> otherwise OS agnostic thing very Linux specific.
>
> OK, but then there's always the option of just having some code that
> runs on init and instantiates a device if it sees the appropriate thing
> in the ACPI tables in a similar manner to how HIDs are handled.  It's a
> small amount of work but it will generally make life easier if there is
> a struct device.

I need to give this a try. AFAICS, the HIDs get their device created
by the driver subsystem. This would require manually creating one and
I didn't see existing examples. Thinking of a table as a device
(virtual/real) seemed weird to me, especially since we're seemingly
only using it for ref counts here. But it sounds like this is an
acceptable thing.

>
>> > If PCC is described by ACPI tables how would non-ACPI users be able to
>> > use it?
>
>> Whoever wants to do that, would need to come up with DT bindings that
>> describe something similar to the PCCT contents. They could possibly
>> ignore the ACPI specific bits like signature, asl compiler details
>> etc. (which are only used by ACPI table parsers) and provide the rest
>> of it. Its essentially an array of structures that point to various
>> shared memory regions, each of which is owned by a PCC client and
>> shared with the firmware. The intercommunication between client and
>> firmware is via a doorbell, which is also described in these entries
>> and can be implemented as an SGI or similar.
>
> Of course most likely such a binding would involve creating a device
> that owns the mailboxes so this'd be fairly straightforward...

With DT, yes, it seems to be a bit more straightforward.


Thanks,
Ashwin

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
> On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
> > On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
> 
> > >> +static struct mbox_controller *
> > >> +mbox_find_pcc_controller(char *name)
> > >> +{
> > >> +     struct mbox_controller *mbox;
> > >> +     list_for_each_entry(mbox, &mbox_cons, node) {
> > >> +             if (mbox->name)
> > >> +                     if (!strcmp(mbox->name, name))
> > >> +                             return mbox;
> > >> +     }
> > >> +
> > >> +     return NULL;
> > >> +}
> 
> > > This doesn't look particularly PCC specific?
> 
> > Call this mbox_find_controller_by_name() instead?
> 
> That certainly looks like what it's doing.  Probably also make the name
> that gets passed in const while you're at it.

The mailbox API intentionally does not have an interface for
that: you are supposed to get a reference to an mbox controller
from a phandle or similar, not by knowing the name of the controller.

Unfortunately, the three patches that Ashwin posted don't have a
caller for this function, so I don't know what it's actually used for.
Why do we need this function for pcc, and what are the names that
can be passed here?

	Arnd

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:

> > That certainly looks like what it's doing.  Probably also make the name
> > that gets passed in const while you're at it.

> The mailbox API intentionally does not have an interface for
> that: you are supposed to get a reference to an mbox controller
> from a phandle or similar, not by knowing the name of the controller.

Right, and what he's trying to work around here is that ACPI has chosen
to provide a generic binding for some mailboxes which isn't associated
with anything we represent as a device and he doesn't want to provide
that device as a Linux virtual thing.

> Unfortunately, the three patches that Ashwin posted don't have a
> caller for this function, so I don't know what it's actually used for.
> Why do we need this function for pcc, and what are the names that
> can be passed here?

AFAICT the names he's interested in will be defined by the ACPI specs.
It does seem like we should be providing a device for the controller and
then either using references in ACPI to look it up if they exist or a
lookup function for this particular namespace that goes and fetches the
device we created and looks up in its context.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
>> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
>
>> > That certainly looks like what it's doing.  Probably also make the name
>> > that gets passed in const while you're at it.
>
>> The mailbox API intentionally does not have an interface for
>> that: you are supposed to get a reference to an mbox controller
>> from a phandle or similar, not by knowing the name of the controller.
>
> Right, and what he's trying to work around here is that ACPI has chosen
> to provide a generic binding for some mailboxes which isn't associated
> with anything we represent as a device and he doesn't want to provide
> that device as a Linux virtual thing.

Just the idea of a table as a device, when it doesn't do any power
management, hotplug or anything like a device seemed strange. But I'm
open to ideas if we find a good solution. Its highly possible that I'm
not seeing it the way you are because the driver subsystem internals
are fairly new to me. :)

Suppose we create a platform_device for the PCCT (mailbox controller)
and another one for the PCC client (mailbox client). How should the
PCC client(s) identify the mailbox controller without passing a name?
In DT, the "mboxes" field in the client DT entry is all strings with
mailbox controller names. The "index" in mbox_request_channel() picks
up one set of strings. How should this work with PCC? Should we use
the PCC client platform_device->dev->platform_data to store mailbox
controller strings?

>
>> Unfortunately, the three patches that Ashwin posted don't have a
>> caller for this function, so I don't know what it's actually used for.
>> Why do we need this function for pcc, and what are the names that
>> can be passed here?
>
> AFAICT the names he's interested in will be defined by the ACPI specs.
> It does seem like we should be providing a device for the controller and
> then either using references in ACPI to look it up if they exist or a
> lookup function for this particular namespace that goes and fetches the
> device we created and looks up in its context.

What is the comparison in this lookup function? A string or a struct
device pointer? If it is the latter, how does the client get the
reference to the controller struct device? One way would be to
register the PCCT as a platform_device and the PCC client as its
platform_driver. But I think that will restrict the number of PCC
clients to who ever matches first. I suspect this is not what you're
implying, so I'd appreciate some more help.

Thanks,
Ashwin

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Mark, Arnd,

On 28 August 2014 16:34, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote:
>> On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
>>> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
>>
>>> > That certainly looks like what it's doing.  Probably also make the name
>>> > that gets passed in const while you're at it.
>>
>>> The mailbox API intentionally does not have an interface for
>>> that: you are supposed to get a reference to an mbox controller
>>> from a phandle or similar, not by knowing the name of the controller.
>>
>> Right, and what he's trying to work around here is that ACPI has chosen
>> to provide a generic binding for some mailboxes which isn't associated
>> with anything we represent as a device and he doesn't want to provide
>> that device as a Linux virtual thing.
>
> Just the idea of a table as a device, when it doesn't do any power
> management, hotplug or anything like a device seemed strange. But I'm
> open to ideas if we find a good solution. Its highly possible that I'm
> not seeing it the way you are because the driver subsystem internals
> are fairly new to me. :)
>
> Suppose we create a platform_device for the PCCT (mailbox controller)
> and another one for the PCC client (mailbox client). How should the
> PCC client(s) identify the mailbox controller without passing a name?
> In DT, the "mboxes" field in the client DT entry is all strings with
> mailbox controller names. The "index" in mbox_request_channel() picks
> up one set of strings. How should this work with PCC? Should we use
> the PCC client platform_device->dev->platform_data to store mailbox
> controller strings?
>
>>
>>> Unfortunately, the three patches that Ashwin posted don't have a
>>> caller for this function, so I don't know what it's actually used for.
>>> Why do we need this function for pcc, and what are the names that
>>> can be passed here?
>>
>> AFAICT the names he's interested in will be defined by the ACPI specs.
>> It does seem like we should be providing a device for the controller and
>> then either using references in ACPI to look it up if they exist or a
>> lookup function for this particular namespace that goes and fetches the
>> device we created and looks up in its context.
>
> What is the comparison in this lookup function? A string or a struct
> device pointer? If it is the latter, how does the client get the
> reference to the controller struct device? One way would be to
> register the PCCT as a platform_device and the PCC client as its
> platform_driver. But I think that will restrict the number of PCC
> clients to who ever matches first. I suspect this is not what you're
> implying, so I'd appreciate some more help.


I dont see a way to create a lookup table for PCC without storing the
name of the controller somewhere. The suggestion of creating a
platform device for the controller and client led to restricting only
one client to the controller. Can you please suggest how to move this
forward?

Thanks,
Ashwin

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Tuesday 02 September 2014 14:16:42 Ashwin Chaugule wrote:
> On 28 August 2014 16:34, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote:
> > On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote:
> >> On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote:
> >>> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
> >>
> >>> > That certainly looks like what it's doing.  Probably also make the name
> >>> > that gets passed in const while you're at it.
> >>
> >>> The mailbox API intentionally does not have an interface for
> >>> that: you are supposed to get a reference to an mbox controller
> >>> from a phandle or similar, not by knowing the name of the controller.
> >>
> >> Right, and what he's trying to work around here is that ACPI has chosen
> >> to provide a generic binding for some mailboxes which isn't associated
> >> with anything we represent as a device and he doesn't want to provide
> >> that device as a Linux virtual thing.
> >
> > Just the idea of a table as a device, when it doesn't do any power
> > management, hotplug or anything like a device seemed strange. But I'm
> > open to ideas if we find a good solution. Its highly possible that I'm
> > not seeing it the way you are because the driver subsystem internals
> > are fairly new to me. 
> >
> > Suppose we create a platform_device for the PCCT (mailbox controller)
> > and another one for the PCC client (mailbox client). How should the
> > PCC client(s) identify the mailbox controller without passing a name?
> > In DT, the "mboxes" field in the client DT entry is all strings with
> > mailbox controller names.

No, it's not a string at all, it's a phandle, which is more like a
pointer. We intentionally never match by a global string at all,
because those might not be unique.

> > The "index" in mbox_request_channel() picks
> > up one set of strings. How should this work with PCC? Should we use
> > the PCC client platform_device->dev->platform_data to store mailbox
> > controller strings?

I didn't think there was more than one PCC provider, why do you even
need a string?

For the general case in ACPI, there should be a similar way of looking
up mailbox providers to what we have in DT, but if I understand you
correctly, the PCC specification does not allow that.

Using platform_data would no be helpful, because there is no platform
code to fill that out on ACPI based systems.

> >>> Unfortunately, the three patches that Ashwin posted don't have a
> >>> caller for this function, so I don't know what it's actually used for.
> >>> Why do we need this function for pcc, and what are the names that
> >>> can be passed here?
> >>
> >> AFAICT the names he's interested in will be defined by the ACPI specs.
> >> It does seem like we should be providing a device for the controller and
> >> then either using references in ACPI to look it up if they exist or a
> >> lookup function for this particular namespace that goes and fetches the
> >> device we created and looks up in its context.
> >
> > What is the comparison in this lookup function? A string or a struct
> > device pointer? If it is the latter, how does the client get the
> > reference to the controller struct device? One way would be to
> > register the PCCT as a platform_device and the PCC client as its
> > platform_driver. But I think that will restrict the number of PCC
> > clients to who ever matches first. I suspect this is not what you're
> > implying, so I'd appreciate some more help.
> 
> I dont see a way to create a lookup table for PCC without storing the
> name of the controller somewhere. The suggestion of creating a
> platform device for the controller and client led to restricting only
> one client to the controller. Can you please suggest how to move this
> forward?

I've forgotten the details, but I thought we had already worked it
out when we discussed it the last time. What is the information available
to the client driver?

	Arnd

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Arnd,

On 2 September 2014 15:22, Arnd Bergmann <arnd@arndb.de> wrote:
>> > Suppose we create a platform_device for the PCCT (mailbox controller)
>> > and another one for the PCC client (mailbox client). How should the
>> > PCC client(s) identify the mailbox controller without passing a name?
>> > In DT, the "mboxes" field in the client DT entry is all strings with
>> > mailbox controller names.
>
> No, it's not a string at all, it's a phandle, which is more like a
> pointer. We intentionally never match by a global string at all,
> because those might not be unique.

Ok. With PCC, I dont see a way to do this sort of pointer matching.

>
>> > The "index" in mbox_request_channel() picks
>> > up one set of strings. How should this work with PCC? Should we use
>> > the PCC client platform_device->dev->platform_data to store mailbox
>> > controller strings?
>
> I didn't think there was more than one PCC provider, why do you even
> need a string?
>
> For the general case in ACPI, there should be a similar way of looking
> up mailbox providers to what we have in DT, but if I understand you
> correctly, the PCC specification does not allow that.

Right. At least not in a way DT does. PCC clients know if something
needs to be written/read via PCC mailbox and can identify a PCC
subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely
identified/defined in the spec.

#define ACPI_ADR_SPACE_PLATFORM_COMM    (acpi_adr_space_type) 10

So we could use this ID instead of a string and use that to look up
the PCC controller for a PCC client.

>
> Using platform_data would no be helpful, because there is no platform
> code to fill that out on ACPI based systems.

Right. So the question is how do we work around the "mbox->dev" and
"client->dev" expectations in the Mailbox framework for PCC, given
that these tables aren't backed by "struct devices" ?

Thanks,
Ashwin

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:

> > Using platform_data would no be helpful, because there is no platform
> > code to fill that out on ACPI based systems.

> Right. So the question is how do we work around the "mbox->dev" and
> "client->dev" expectations in the Mailbox framework for PCC, given
> that these tables aren't backed by "struct devices" ?

As previously suggested just looking things up in the context of a
device created to represent the PCC controller seems fine; clients know
they're using PCC so can just call a PCC specific API which hides the
mechanics from them (for example, using a global variable to store the
device).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:
>
>> > Using platform_data would no be helpful, because there is no platform
>> > code to fill that out on ACPI based systems.
>
>> Right. So the question is how do we work around the "mbox->dev" and
>> "client->dev" expectations in the Mailbox framework for PCC, given
>> that these tables aren't backed by "struct devices" ?
>
> As previously suggested just looking things up in the context of a
> device created to represent the PCC controller seems fine; clients know
> they're using PCC so can just call a PCC specific API which hides the
> mechanics from them (for example, using a global variable to store the
> device).

IIUC, this means, either modifying the existing
mbox_controller_register() to know when a PCC mbox is being added, or
have another PCC specific API for registering as a mailbox? Then, in
the PCC specific API that requests a PCC channel, depending on what we
do in the registration path, this function can pick out the PCC
mailbox pointer and try_module_get(mbox->dev..). Since this is PCC
specific anyway, we don't need the client->dev argument at all. Did I
understand you correctly?

Thanks,
Ashwin

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Wednesday 03 September 2014 11:23:21 Ashwin Chaugule wrote:
> On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote:
> >
> >> > Using platform_data would no be helpful, because there is no platform
> >> > code to fill that out on ACPI based systems.
> >
> >> Right. So the question is how do we work around the "mbox->dev" and
> >> "client->dev" expectations in the Mailbox framework for PCC, given
> >> that these tables aren't backed by "struct devices" ?
> >
> > As previously suggested just looking things up in the context of a
> > device created to represent the PCC controller seems fine; clients know
> > they're using PCC so can just call a PCC specific API which hides the
> > mechanics from them (for example, using a global variable to store the
> > device).
> 
> IIUC, this means, either modifying the existing
> mbox_controller_register() to know when a PCC mbox is being added, or
> have another PCC specific API for registering as a mailbox? Then, in
> the PCC specific API that requests a PCC channel, depending on what we
> do in the registration path, this function can pick out the PCC
> mailbox pointer and try_module_get(mbox->dev..). Since this is PCC
> specific anyway, we don't need the client->dev argument at all. Did I
> understand you correctly?

Yes, I think this would be reasonable. 

	Arnd

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:
> On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote:

> > As previously suggested just looking things up in the context of a
> > device created to represent the PCC controller seems fine; clients know
> > they're using PCC so can just call a PCC specific API which hides the
> > mechanics from them (for example, using a global variable to store the
> > device).

> IIUC, this means, either modifying the existing
> mbox_controller_register() to know when a PCC mbox is being added, or
> have another PCC specific API for registering as a mailbox? Then, in

Why would you have to do that - can't the PCC specific API manage to
hide this?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Wednesday 03 September 2014 16:36:01 Mark Brown wrote:
> On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:
> > On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote:
> 
> > > As previously suggested just looking things up in the context of a
> > > device created to represent the PCC controller seems fine; clients know
> > > they're using PCC so can just call a PCC specific API which hides the
> > > mechanics from them (for example, using a global variable to store the
> > > device).
> 
> > IIUC, this means, either modifying the existing
> > mbox_controller_register() to know when a PCC mbox is being added, or
> > have another PCC specific API for registering as a mailbox? Then, in
> 
> Why would you have to do that - can't the PCC specific API manage to
> hide this?

I think that's what Ashwin meant as the 'or' part of the sentence above.

	Arnd

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Wed, Sep 03, 2014 at 05:41:20PM +0200, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 16:36:01 Mark Brown wrote:
> > On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote:

> > > IIUC, this means, either modifying the existing
> > > mbox_controller_register() to know when a PCC mbox is being added, or
> > > have another PCC specific API for registering as a mailbox? Then, in

> > Why would you have to do that - can't the PCC specific API manage to
> > hide this?

> I think that's what Ashwin meant as the 'or' part of the sentence above.

So it is.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:
> >
> >> > The "index" in mbox_request_channel() picks
> >> > up one set of strings. How should this work with PCC? Should we use
> >> > the PCC client platform_device->dev->platform_data to store mailbox
> >> > controller strings?
> >
> > I didn't think there was more than one PCC provider, why do you even
> > need a string?
> >
> > For the general case in ACPI, there should be a similar way of looking
> > up mailbox providers to what we have in DT, but if I understand you
> > correctly, the PCC specification does not allow that.
> 
> Right. At least not in a way DT does. PCC clients know if something
> needs to be written/read via PCC mailbox and can identify a PCC
> subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely
> identified/defined in the spec.
> 
> #define ACPI_ADR_SPACE_PLATFORM_COMM    (acpi_adr_space_type) 10
> 
> So we could use this ID instead of a string and use that to look up
> the PCC controller for a PCC client.

I didn't realize this was the case. Does that mean we can treat
pcc as a linearly accessible address space the way we do for
system memory, pci-config etc?

If that works, we should probably just have a regmap for it rather
than expose the mailbox API to client drivers.

	Arnd

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Mark Brown]

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

On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:

> > Right. At least not in a way DT does. PCC clients know if something
> > needs to be written/read via PCC mailbox and can identify a PCC
> > subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely
> > identified/defined in the spec.

> > #define ACPI_ADR_SPACE_PLATFORM_COMM    (acpi_adr_space_type) 10

> > So we could use this ID instead of a string and use that to look up
> > the PCC controller for a PCC client.

> I didn't realize this was the case. Does that mean we can treat
> pcc as a linearly accessible address space the way we do for
> system memory, pci-config etc?

> If that works, we should probably just have a regmap for it rather
> than expose the mailbox API to client drivers.

A regmap doesn't seem to map very well here - as far as I can tell the
addresses referred to are mailboxes rather than registers or memory
addresses.  I could be misunderstanding though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Linaro-acpi] [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Arnd Bergmann]

On Wednesday 03 September 2014 15:49:21 Mark Brown wrote:
> On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote:
> > On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote:
> 
> > > Right. At least not in a way DT does. PCC clients know if something
> > > needs to be written/read via PCC mailbox and can identify a PCC
> > > subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely
> > > identified/defined in the spec.
> 
> > > #define ACPI_ADR_SPACE_PLATFORM_COMM    (acpi_adr_space_type) 10
> 
> > > So we could use this ID instead of a string and use that to look up
> > > the PCC controller for a PCC client.
> 
> > I didn't realize this was the case. Does that mean we can treat
> > pcc as a linearly accessible address space the way we do for
> > system memory, pci-config etc?
> 
> > If that works, we should probably just have a regmap for it rather
> > than expose the mailbox API to client drivers.
> 
> A regmap doesn't seem to map very well here - as far as I can tell the
> addresses referred to are mailboxes rather than registers or memory
> addresses.  I could be misunderstanding though.

No, I think you are right. Nevermind then.

	Arnd

Re: [PATCH v3 1/3] Mailbox: Add support for PCC mailbox and channels

[Ashwin Chaugule]

Hi Arnd,

On 28 August 2014 04:39, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote:
>> On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote:
>> > On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote:
>> > > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote:
>>
>> > >> +static struct mbox_controller *
>> > >> +mbox_find_pcc_controller(char *name)
>> > >> +{
>> > >> +     struct mbox_controller *mbox;
>> > >> +     list_for_each_entry(mbox, &mbox_cons, node) {
>> > >> +             if (mbox->name)
>> > >> +                     if (!strcmp(mbox->name, name))
>> > >> +                             return mbox;
>> > >> +     }
>> > >> +
>> > >> +     return NULL;
>> > >> +}
>>
>> > > This doesn't look particularly PCC specific?
>>
>> > Call this mbox_find_controller_by_name() instead?
>>
>> That certainly looks like what it's doing.  Probably also make the name
>> that gets passed in const while you're at it.
>
> The mailbox API intentionally does not have an interface for
> that: you are supposed to get a reference to an mbox controller
> from a phandle or similar, not by knowing the name of the controller.

This snippet is based off of your suggestions. [1] [2] :)

>
> Unfortunately, the three patches that Ashwin posted don't have a
> caller for this function,

mbox_find_pcc_controller() is called from pcc_mbox_request_channel()
which is in this patch.

> so I don't know what it's actually used for.
> Why do we need this function for pcc, and what are the names that
> can be passed here?
>
>         Arnd

[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265395.html
[2] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266528.html

[PATCH v3 2/3] Add support for Platform Communication Channel

[Ashwin Chaugule]

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

This patch adds PCC support as a Mailbox Controller.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/mailbox/Kconfig  |  12 +++
 drivers/mailbox/Makefile |   2 +
 drivers/mailbox/pcc.c    | 228 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/mailbox/pcc.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index c8b5c13..af4a153 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -50,4 +50,16 @@ config OMAP_MBOX_KFIFO_SIZE
 	  Specify the default size of mailbox's kfifo buffers (bytes).
 	  This can also be changed at runtime (via the mbox_kfifo_size
 	  module parameter).
+
+config PCC
+	bool "Platform Communication Channel"
+	depends on ACPI
+	help
+		ACPI 5.0+ spec defines a generic mode of communication
+		between the OS and a platform such as the BMC. This medium
+		(PCC) is typically used by CPPC (ACPI CPU Performance management),
+		RAS (ACPI reliability protocol) and MPST (ACPI Memory power
+		states). Select this driver if your platform implements the
+		PCC clients mentioned above.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 2fa343a..3f57ee9 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -9,3 +9,5 @@ obj-$(CONFIG_OMAP1_MBOX)	+= mailbox_omap1.o
 mailbox_omap1-objs		:= mailbox-omap1.o
 obj-$(CONFIG_OMAP2PLUS_MBOX)	+= mailbox_omap2.o
 mailbox_omap2-objs		:= mailbox-omap2.o
+
+obj-$(CONFIG_PCC)	+= pcc.o
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
new file mode 100644
index 0000000..e925f65
--- /dev/null
+++ b/drivers/mailbox/pcc.c
@@ -0,0 +1,228 @@
+/*
+ *	Copyright (C) 2014 Linaro Ltd.
+ *	Author:	Ashwin Chaugule <ashwin.chaugule@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+#include <linux/init.h>
+#include <linux/cpufreq.h>
+#include <linux/delay.h>
+#include <linux/ioctl.h>
+#include <linux/vmalloc.h>
+#include <linux/mailbox_controller.h>
+
+#include <acpi/actbl.h>
+
+#define MAX_PCC_SUBSPACES	256
+#define PCCS_SS_SIG_MAGIC	0x50434300
+#define PCC_CMD_COMPLETE	0x1
+#define PCC_VERSION			"0.1"
+
+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];
+static struct mbox_controller pcc_mbox_ctrl = {};
+
+static bool pcc_tx_done(struct mbox_chan *chan)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Wait for Platform to consume. */
+	while (!(ioread16(&generic_comm_base->status) & PCC_CMD_COMPLETE))
+		udelay(cmd_delay);
+
+	return true;
+}
+
+static int get_subspace_id(struct mbox_chan *chan)
+{
+	unsigned int i;
+	int ret = -ENOENT;
+
+	for (i = 0; i < pcc_mbox_ctrl.num_chans; i++) {
+		if (chan == &pcc_mbox_chan[i])
+			return i;
+	}
+
+	return ret;
+}
+
+/* Channel lock is already held by mbox controller code. */
+static int pcc_send_data(struct mbox_chan *chan, void *data)
+{
+	struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
+	struct acpi_pcct_shared_memory *generic_comm_base =
+		(struct acpi_pcct_shared_memory *) pcct_ss->base_address;
+	struct acpi_generic_address doorbell;
+	u64 doorbell_preserve;
+	u64 doorbell_val;
+	u64 doorbell_write;
+	u16 cmd = 0;
+	u16 ss_idx = -1;
+	int ret = 0;
+
+	/*
+	 * Min time in usec that OS is expected to wait
+	 * before sending the next PCC cmd.
+	 */
+	u16 cmd_delay = pcct_ss->min_turnaround_time;
+
+	/* Get PCC CMD */
+	ret = kstrtou16((char*)data, 0, &cmd);
+	if (ret < 0) {
+		pr_err("Err while converting PCC CMD to u16: %d\n", ret);
+		goto out_err;
+	}
+
+	ss_idx = get_subspace_id(chan);
+
+	if (ss_idx < 0) {
+		pr_err("Invalid Subspace ID from PCC client\n");
+		ret = ss_idx;
+		goto out_err;
+	}
+
+	doorbell = pcct_ss->doorbell_register;
+	doorbell_preserve = pcct_ss->preserve_mask;
+	doorbell_write = pcct_ss->write_mask;
+
+	/* Write to the shared comm region. */
+	iowrite16(cmd, &generic_comm_base->command);
+
+	/* Write Subspace MAGIC value so platform can identify destination. */
+	iowrite32((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
+
+	/* Flip CMD COMPLETE bit */
+	iowrite16(0, &generic_comm_base->status);
+
+	/* Sync notification from OSPM to Platform. */
+	acpi_read(&doorbell_val, &doorbell);
+	acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
+			&doorbell);
+
+out_err:
+	return ret;
+}
+
+static int pcc_chan_startup(struct mbox_chan *chan)
+{
+	return 0;
+}
+
+static void pcc_chan_shutdown(struct mbox_chan *chan)
+{
+	return;
+}
+
+static struct mbox_chan_ops pcc_chan_ops = {
+	.send_data	=	pcc_send_data,
+	.startup	=	pcc_chan_startup,
+	.shutdown	=	pcc_chan_shutdown,
+	.last_tx_done	=	pcc_tx_done,
+};
+
+static int parse_pcc_subspace(struct acpi_subtable_header *header,
+		const unsigned long end)
+{
+	struct acpi_pcct_subspace *pcct_ss;
+
+	if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
+		pcct_ss = (struct acpi_pcct_subspace *) header;
+
+		if (pcct_ss->header.type != ACPI_PCCT_TYPE_GENERIC_SUBSPACE) {
+			pr_err("Incorrect PCC Subspace type detected\n");
+			return -EINVAL;
+		}
+
+		/* New mbox channel entry for each PCC subspace detected. */
+		pcc_mbox_chan[pcc_mbox_ctrl.num_chans].con_priv = pcct_ss;
+
+		pcc_mbox_ctrl.num_chans++;
+
+	} else {
+		pr_err("No more space for PCC subspaces.\n");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static int __init pcc_probe(void)
+{
+	acpi_status status = AE_OK;
+	acpi_size pcct_tbl_header_size;
+	struct acpi_table_pcct *pcct_tbl;
+	int ret;
+
+	/* Search for PCCT */
+	status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
+			(struct acpi_table_header **)&pcct_tbl,
+			&pcct_tbl_header_size);
+
+	if (ACPI_SUCCESS(status) && !pcct_tbl) {
+		pr_warn("PCCT header not found.\n");
+		status = AE_NOT_FOUND;
+		goto out_err;
+	}
+
+	status = acpi_table_parse_entries(ACPI_SIG_PCCT,
+			sizeof(struct acpi_table_pcct),
+			ACPI_PCCT_TYPE_GENERIC_SUBSPACE,
+			parse_pcc_subspace, MAX_PCC_SUBSPACES);
+
+	if (ACPI_SUCCESS(status))
+		pr_err("Error parsing PCC subspaces from PCCT\n");
+
+	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
+
+	pcc_mbox_ctrl.chans = pcc_mbox_chan;
+	pcc_mbox_ctrl.ops = &pcc_chan_ops;
+	pcc_mbox_ctrl.name = "PCCT";
+	pcc_mbox_ctrl.txdone_poll = true;
+	pcc_mbox_ctrl.txpoll_period = 1;
+
+	pr_info("Registering PCC driver as Mailbox controller\n");
+	ret = mbox_controller_register(&pcc_mbox_ctrl);
+
+	if (ret) {
+		pr_err("Err registering PCC as Mailbox controller\n");
+		return -ENODEV;
+	}
+
+out_err:
+	return ACPI_SUCCESS(status) ? 1 : 0;
+}
+
+static int __init pcc_init(void)
+{
+	int ret;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	/* Check if PCC support is available. */
+	ret = pcc_probe();
+
+	if (ret) {
+		pr_debug("PCC probe failed.\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+device_initcall(pcc_init);
-- 
1.9.1

Re: [PATCH v3 2/3] Add support for Platform Communication Channel

[Mark Brown]

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

On Tue, Aug 26, 2014 at 03:35:37PM -0400, Ashwin Chaugule wrote:

> +static int pcc_chan_startup(struct mbox_chan *chan)
> +{
> +	return 0;
> +}
> +
> +static void pcc_chan_shutdown(struct mbox_chan *chan)
> +{
> +	return;
> +}

If these functions can be empty it seems better to have the mailbox
framework accept empty functions than require drivers to provide
dummies.

> +	/* Search for PCCT */
> +	status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
> +			(struct acpi_table_header **)&pcct_tbl,
> +			&pcct_tbl_header_size);
> +
> +	if (ACPI_SUCCESS(status) && !pcct_tbl) {
> +		pr_warn("PCCT header not found.\n");
> +		status = AE_NOT_FOUND;
> +		goto out_err;
> +	}

Is PCC mandatory?  If not I'd not expect to see this as a warning.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 3/3] PCC-test: Test driver to trigger PCC commands

[Ashwin Chaugule]

echo 2 > /proc/pcc_test [ to get channel base address ]

echo 1 > /proc/pcc_test [ to trigger a PCC write ]

echo 0 > /proc/pcc_test [ to trigger a PCC read ]

The pcc-test driver is implemented as an example of a PCC
Mailbox client.

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

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

Re: [PATCH v3 3/3] PCC-test: Test driver to trigger PCC commands

[Mark Brown]

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

On Tue, Aug 26, 2014 at 03:35:38PM -0400, Ashwin Chaugule wrote:

> echo 2 > /proc/pcc_test [ to get channel base address ]
> 
> echo 1 > /proc/pcc_test [ to trigger a PCC write ]
> 
> echo 0 > /proc/pcc_test [ to trigger a PCC read ]

You shouldn't be adding new files to /proc - debugfs is a better home
for something like this.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 3/3] PCC-test: Test driver to trigger PCC commands

[Ashwin Chaugule]

On 27 August 2014 06:30, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Aug 26, 2014 at 03:35:38PM -0400, Ashwin Chaugule wrote:
>
>> echo 2 > /proc/pcc_test [ to get channel base address ]
>>
>> echo 1 > /proc/pcc_test [ to trigger a PCC write ]
>>
>> echo 0 > /proc/pcc_test [ to trigger a PCC read ]
>
> You shouldn't be adding new files to /proc - debugfs is a better home
> for something like this.

True. Didnt intend to upstream this part anyway. I used this only to
test things for myself.

Thanks,
Ashwin