Hi Jassi,

On 9 November 2014 08:48, Jassi Brar jaswinder.singh@linaro.org wrote:

On 7 November 2014 20:22, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

+/**

• • pcc_mbox_request_channel - PCC clients call this function to

• •         request a pointer to their PCC subspace, from which they
    

• •         can get the details of communicating with the remote.
    

• • @cl: Pointer to Mailbox client, so we know where to bind the

• •         Channel.
    

• • @index: The PCC Subspace index as parsed in the PCC client

• •         ACPI package. This is used to lookup the array of PCC
    

• •         subspaces as parsed by the PCC Mailbox controller.
    

• • Return: Pointer to the Mailbox Channel if successful or

• •         ERR_PTR.
    

• */

+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,

•           int index)
  

+{

•   struct device *dev = pcc_mbox_ctrl.dev;
  

•   struct mbox_chan *chan;
  

•   unsigned long flags;
  

•   /*
  

•    * Each PCC Subspace is a Mailbox Channel.
  

•    * The PCC Clients get their PCC Subspace ID
  

•    * from their own tables and pass it here.
  

•    * This returns a pointer to the PCC subspace
  

•    * for the Client to operate on.
  

•    */
  

•   chan = &pcc_mbox_chan[index];
  

•   if (!chan || chan->cl) {
  

•           dev_err(dev, "%s: PCC mailbox not free\n", __func__);
  

•           return ERR_PTR(-EBUSY);
  

•   }
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->msg_free = 0;
  

•   chan->msg_count = 0;
  

•   chan->active_req = NULL;
  

•   chan->cl = cl;
  

•   init_completion(&chan->tx_complete);
  

•   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
  

•           chan->txdone_method |= TXDONE_BY_ACK;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

•   return chan;
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);

I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and kept same people in CC. That patch keeps the api same for DT and ACPI drivers.

Thanks for taking a look.

Your patch looks similar in concept to one of my earlier patches. But based on the discussions that followed since, we decided that its best to add a separate PCC lookup and registration API. The main reason being, we dont have a way to list all mbox providers in ACPI in a way that DT does. e.g. in DT, the client->dev is used to look up mbox controllers. In ACPI, a client cant specify which mbox controllers to associate with, if it can attach to multiple. With the PCC specific API, if the client calls it, the controller knows where to look, because that lookup is PCC specific.

In your patch, the assumption that PCC is the only ACPI mbox provider, maybe true today, but that can change anytime.

...

+/**

• • pcc_mbox_free_channel - Clients call this to free their Channel.
• • @chan: Pointer to the mailbox channel as returned by

• •         pcc_mbox_request_channel()
    

• */

+void pcc_mbox_free_channel(struct mbox_chan *chan) +{

•   unsigned long flags;
  

•   if (!chan || !chan->cl)
  

•           return;
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->cl = NULL;
  

•   chan->active_req = NULL;
  

•   if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
  

•           chan->txdone_method = TXDONE_BY_POLL;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);

+/**

• • pcc_tx_done - Callback from Mailbox controller code to

• •         check if PCC message transmission completed.
    

• • @chan: Pointer to Mailbox channel on which previous

• •         transmission occurred.
    

• • Return: TRUE if succeeded.
• */

+static bool pcc_tx_done(struct mbox_chan *chan) +{

•   struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

•   u16 cmd_delay = pcct_ss->min_turnaround_time;
  

•   unsigned int retries = 0;
  

•   /* Try a few times while waiting for platform to consume */
  

•   while (!(readw_relaxed(&generic_comm_base->status)
  

•               & PCC_CMD_COMPLETE)) {
  

•           if (retries++ < 5)
  

•                   udelay(cmd_delay);
  

•           else {
  

•                   /*
  

•                    * If the remote is dead, this will cause the Mbox
  

•                    * controller to timeout after mbox client.tx_tout
  

•                    * msecs.
  

•                    */
  

•                   pr_err("PCC platform did not respond.\n");
  

•                   return false;
  

•           }
  

•   }
  

•   return true;
  

+}

+/**

• • get_subspace_id - Given a Mailbox channel, find out the

• •         PCC subspace id.
    

• • @chan: Pointer to Mailbox Channel from which we want

• •         the index.
    

• • Return: Errno if not found, else positive index number.
• */

+static int get_subspace_id(struct mbox_chan *chan) +{

•   int id = chan - pcc_mbox_chan;
  

•   if (id < 0 || id > pcc_mbox_ctrl.num_chans)
  

•           return -ENOENT;
  

•   return id;
  

+}

+/**

• • pcc_send_data - Called from Mailbox Controller code to finally

• • transmit data over channel.
    

• • @chan: Pointer to Mailbox channel over which to send data.
• • @data: Actual data to be written over channel.
• • Return: Err if something failed else 0 for success.
• */

+static int pcc_send_data(struct mbox_chan *chan, void *data) +{

•   struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

•   struct acpi_generic_address doorbell;
  

•   u64 doorbell_preserve;
  

•   u64 doorbell_val;
  

•   u64 doorbell_write;
  

•   u16 cmd = *(u16 *) data;
  

•   u16 ss_idx = -1;
  

•   ss_idx = get_subspace_id(chan);
  

•   if (ss_idx < 0) {
  

•           pr_err("Invalid Subspace ID from PCC client\n");
  

•           return -EINVAL;
  

•   }
  

•   doorbell = pcct_ss->doorbell_register;
  

•   doorbell_preserve = pcct_ss->preserve_mask;
  

•   doorbell_write = pcct_ss->write_mask;
  

•   /* Write to the shared comm region. */
  

•   writew(cmd, &generic_comm_base->command);
  

•   /* Write Subspace MAGIC value so platform can identify destination. */
  

•   writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
  

•   /* Flip CMD COMPLETE bit */
  

•   writew(0, &generic_comm_base->status);
  

•   /* Sync notification from OSPM to Platform. */
  

•   acpi_read(&doorbell_val, &doorbell);
  

•   acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
  

•                   &doorbell);
  

•   return 0;
  

+}

+static struct mbox_chan_ops pcc_chan_ops = {

•   .send_data = pcc_send_data,
  

•   .last_tx_done = pcc_tx_done,
  

+};

+/**

• • parse_pcc_subspace - Parse the PCC table and extract PCC subspace

• •         entries. There should be one entry per PCC client.
    

• • @header: Pointer to the ACPI subtable header under the PCCT.
• • @end: End of subtable entry.
• • Return: 0 for Success, else errno.
• • This gets called for each entry in the PCC table.
• */

+static int parse_pcc_subspace(struct acpi_subtable_header *header,

•           const unsigned long end)
  

+{

•   struct acpi_pcct_hw_reduced *pcct_ss;
  

•   if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
  

•           pcct_ss = (struct acpi_pcct_hw_reduced *) header;
  

•           if (pcct_ss->header.type !=
  

•                           ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) {
  

•                   pr_err("Incorrect PCC Subspace type detected\n");
  

•                   return -EINVAL;
  

•           }
  

•           /* New mbox channel entry for each PCC subspace detected. */
  

•           pcc_mbox_chan[pcc_mbox_ctrl.num_chans].con_priv = pcct_ss;
  

•           pcc_mbox_ctrl.num_chans++;
  

•   } else {
  

•           pr_err("No more space for PCC subspaces.\n");
  

•           return -ENOSPC;
  

•   }
  

•   return 0;
  

+}

+/**

• • acpi_pcc_probe - Parse the ACPI tree for the PCCT.
• • Return: 0 for Success, else errno.
• */

+static int __init acpi_pcc_probe(void) +{

•   acpi_status status = AE_OK;
  

•   acpi_size pcct_tbl_header_size;
  

•   int count;
  

•   struct acpi_table_pcct *pcct_tbl;
  

•   /* Search for PCCT */
  

•   status = acpi_get_table_with_size(ACPI_SIG_PCCT, 0,
  

•                   (struct acpi_table_header **)&pcct_tbl,
  

•                   &pcct_tbl_header_size);
  

•   if (ACPI_FAILURE(status) || !pcct_tbl) {
  

•           pr_warn("PCCT header not found.\n");
  

•           return -ENODEV;
  

•   }
  

•   count = acpi_table_parse_entries(ACPI_SIG_PCCT,
  

•                   sizeof(struct acpi_table_pcct),
  

•                   ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
  

•                   parse_pcc_subspace, MAX_PCC_SUBSPACES);
  

•   if (count <= 0) {
  

•           pr_err("Error parsing PCC subspaces from PCCT\n");
  

•           return -EINVAL;
  

•   }
  

•   pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
  

•   return 0;
  

+}

+/**

• • pcc_mbox_probe - Called when we find a match for the

• • PCCT platform device. This is purely used to represent
    

• • the PCCT as a virtual device for registering with the
    

• • generic Mailbox framework.
    

• • @pdev: Pointer to platform device returned when a match

• • is found.
    

• • Return: 0 for Success, else errno.
    

• */

+static int pcc_mbox_probe(struct platform_device *pdev) +{

•   int ret = 0;
  

•   pcc_mbox_ctrl.chans = pcc_mbox_chan;
  

•   pcc_mbox_ctrl.ops = &pcc_chan_ops;
  

•   pcc_mbox_ctrl.txdone_poll = true;
  

•   pcc_mbox_ctrl.txpoll_period = 10;
  

•   pcc_mbox_ctrl.dev = &pdev->dev;
  

•   pr_info("Registering PCC driver as Mailbox controller\n");
  

•   ret = mbox_controller_register(&pcc_mbox_ctrl);
  

•   if (ret) {
  

•           pr_err("Err registering PCC as Mailbox controller: %d\n", ret);
  

•           ret = -ENODEV;
  

•   }
  

•   return ret;
  

+}

+struct platform_driver pcc_mbox_driver = {

•   .probe = pcc_mbox_probe,
  

•   .driver = {
  

•           .name = "PCCT",
  

•           .owner = THIS_MODULE,
  

•   },
  

+};

+static int __init pcc_init(void) +{

•   int ret;
  

•   struct platform_device *pcc_pdev;
  

•   if (acpi_disabled)
  

•           return -ENODEV;
  

•   /* Check if PCC support is available. */
  

•   ret = acpi_pcc_probe();
  

•   if (ret) {
  

•           pr_err("ACPI PCC probe failed.\n");
  

•           return -ENODEV;
  

•   }
  

•   pcc_pdev = platform_create_bundle(&pcc_mbox_driver,
  

•                   pcc_mbox_probe, NULL, 0, NULL, 0);
  

•   if (!pcc_pdev) {
  

•           pr_err("Err creating PCC platform bundle\n");
  

•           return -ENODEV;
  

•   }
  

•   return 0;
  

+} +device_initcall(pcc_init);

IMO the PCC platform_device should be populated by ACPI or DT core. This PCC controller driver should parse the PCCT ... so it populates .global_xlate() which expect the 'global_id' of channel requested to be (0x50434300 | Client_Class_Code) as specified by ACPI.

It should be possible to have same client and controller driver work for both ACPI and DT.

Any reason why this patch won't work with DT as well? (with the requisite DT bindings)

Thanks, Ashwin

On 9 November 2014 11:18, Jassi Brar jaswinder.singh@linaro.org wrote:

On 9 November 2014 20:50, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

Hi Jassi,

On 9 November 2014 08:48, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 7 November 2014 20:22, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

+/**

• • pcc_mbox_request_channel - PCC clients call this function to

• •         request a pointer to their PCC subspace, from which they
    

• •         can get the details of communicating with the remote.
    

• • @cl: Pointer to Mailbox client, so we know where to bind the

• •         Channel.
    

• • @index: The PCC Subspace index as parsed in the PCC client

• •         ACPI package. This is used to lookup the array of PCC
    

• •         subspaces as parsed by the PCC Mailbox controller.
    

• • Return: Pointer to the Mailbox Channel if successful or

• •         ERR_PTR.
    

• */

+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,

•           int index)
  

+{

•   struct device *dev = pcc_mbox_ctrl.dev;
  

•   struct mbox_chan *chan;
  

•   unsigned long flags;
  

•   /*
  

•    * Each PCC Subspace is a Mailbox Channel.
  

•    * The PCC Clients get their PCC Subspace ID
  

•    * from their own tables and pass it here.
  

•    * This returns a pointer to the PCC subspace
  

•    * for the Client to operate on.
  

•    */
  

•   chan = &pcc_mbox_chan[index];
  

•   if (!chan || chan->cl) {
  

•           dev_err(dev, "%s: PCC mailbox not free\n", __func__);
  

•           return ERR_PTR(-EBUSY);
  

•   }
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->msg_free = 0;
  

•   chan->msg_count = 0;
  

•   chan->active_req = NULL;
  

•   chan->cl = cl;
  

•   init_completion(&chan->tx_complete);
  

•   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
  

•           chan->txdone_method |= TXDONE_BY_ACK;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

•   return chan;
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);

I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and kept same people in CC. That patch keeps the api same for DT and ACPI drivers.

Thanks for taking a look.

Your patch looks similar in concept to one of my earlier patches. But

Using ifdefs wasn't the right way to enable ACPI channel mapping. The mailbox core should be able to work with both DT and ACPI at the same time.

Agreed. But my point there was to add an alternative to the DT specific of_xlate, which is exactly what you've done in global_xlate.

...

based on the discussions that followed since, we decided that its best to add a separate PCC lookup and registration API. The main reason being, we dont have a way to list all mbox providers in ACPI in a way that DT does. e.g. in DT, the client->dev is used to look up mbox controllers. In ACPI, a client cant specify which mbox controllers to associate with, if it can attach to multiple. With the PCC specific API, if the client calls it, the controller knows where to look, because that lookup is PCC specific.

In your patch, the assumption that PCC is the only ACPI mbox provider, maybe true today, but that can change anytime.

Please read my patch again, we can have ACPI as well as DT populated clients. All that you intend to do with this patch can be done there and _without_ adding new apis.

Read it again. Not arguing that your patch wont work for DT and ACPI, but your assumption that ACPI supports PCC as the only mbox controller, may not hold true. The global_xlate function will work fine for PCC, but may not work for other ACPI (non-DT) mbox controllers. Using the signature field/index byte works only for PCC. We've already been through this discussion with Mark and Arnd and we came up with the PCC API.

...

...

...

•   pcc_pdev = platform_create_bundle(&pcc_mbox_driver,
  

•                   pcc_mbox_probe, NULL, 0, NULL, 0);
  

•   if (!pcc_pdev) {
  

•           pr_err("Err creating PCC platform bundle\n");
  

•           return -ENODEV;
  

•   }
  

•   return 0;
  

+} +device_initcall(pcc_init);

IMO the PCC platform_device should be populated by ACPI or DT core. This PCC controller driver should parse the PCCT ... so it populates .global_xlate() which expect the 'global_id' of channel requested to be (0x50434300 | Client_Class_Code) as specified by ACPI.

It should be possible to have same client and controller driver work for both ACPI and DT.

Any reason why this patch won't work with DT as well? (with the requisite DT bindings)

A PCC client would need to call pcc_mbox_request_channel() if populated by ACPI, and mbox_request_channel() if by DT. We want any client driver to call just mbox_request_channel()

This would work if in ACPI (non-DT) cases there is a clear way to map a client to a controller. We dont have that in ACPI when a client could use multiple mbox controllers, hence the separate API. If you assume that in ACPI there is only one mbox controller, then this is fine, but I was guided to not implement with this assumption.

Thanks, Ashwin

On 9 November 2014 23:13, Jassi Brar jaswinder.singh@linaro.org wrote:

On 9 November 2014 22:52, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 9 November 2014 11:18, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 9 November 2014 20:50, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

Hi Jassi,

On 9 November 2014 08:48, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 7 November 2014 20:22, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

+/**

• • pcc_mbox_request_channel - PCC clients call this function to

• •         request a pointer to their PCC subspace, from which they
    

• •         can get the details of communicating with the remote.
    

• • @cl: Pointer to Mailbox client, so we know where to bind the

• •         Channel.
    

• • @index: The PCC Subspace index as parsed in the PCC client

• •         ACPI package. This is used to lookup the array of PCC
    

• •         subspaces as parsed by the PCC Mailbox controller.
    

• • Return: Pointer to the Mailbox Channel if successful or

• •         ERR_PTR.
    

• */

+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,

•           int index)
  

+{

•   struct device *dev = pcc_mbox_ctrl.dev;
  

•   struct mbox_chan *chan;
  

•   unsigned long flags;
  

•   /*
  

•    * Each PCC Subspace is a Mailbox Channel.
  

•    * The PCC Clients get their PCC Subspace ID
  

•    * from their own tables and pass it here.
  

•    * This returns a pointer to the PCC subspace
  

•    * for the Client to operate on.
  

•    */
  

•   chan = &pcc_mbox_chan[index];
  

•   if (!chan || chan->cl) {
  

•           dev_err(dev, "%s: PCC mailbox not free\n", __func__);
  

•           return ERR_PTR(-EBUSY);
  

•   }
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->msg_free = 0;
  

•   chan->msg_count = 0;
  

•   chan->active_req = NULL;
  

•   chan->cl = cl;
  

•   init_completion(&chan->tx_complete);
  

•   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
  

•           chan->txdone_method |= TXDONE_BY_ACK;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

•   return chan;
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);

I have just sent out a patch https://lkml.org/lkml/2014/11/9/75 and kept same people in CC. That patch keeps the api same for DT and ACPI drivers.

Thanks for taking a look.

Your patch looks similar in concept to one of my earlier patches. But

Using ifdefs wasn't the right way to enable ACPI channel mapping. The mailbox core should be able to work with both DT and ACPI at the same time.

Agreed. But my point there was to add an alternative to the DT specific of_xlate, which is exactly what you've done in global_xlate.

We all have good intentions, the problem is the code :)

...

...

...

based on the discussions that followed since, we decided that its best to add a separate PCC lookup and registration API. The main reason being, we dont have a way to list all mbox providers in ACPI in a way that DT does. e.g. in DT, the client->dev is used to look up mbox controllers. In ACPI, a client cant specify which mbox controllers to associate with, if it can attach to multiple. With the PCC specific API, if the client calls it, the controller knows where to look, because that lookup is PCC specific.

In your patch, the assumption that PCC is the only ACPI mbox provider, maybe true today, but that can change anytime.

Please read my patch again, we can have ACPI as well as DT populated clients. All that you intend to do with this patch can be done there and _without_ adding new apis.

Read it again. Not arguing that your patch wont work for DT and ACPI, but your assumption that ACPI supports PCC as the only mbox controller, may not hold true. The global_xlate function will work fine for PCC, but may not work for other ACPI (non-DT) mbox controllers. Using the signature field/index byte works only for PCC. We've already been through this discussion with Mark and Arnd and we came up with the PCC API.

Please read it yet again. There is no assumption that PCC is the only mbox in ACPI (though I think that is very likely). The function and its argument are both named _global_. 'Signature' is mentioned only as an example in case of PCC. The PCC controller driver could expect the global_id to be 'signature' of the subspace, similarly another non-DT mailbox controller driver will expect its own different 'signature' (say 0xdead0000 | id_16bits). In the patch I submitted we try .global_xlate() of all such mboxes and only one, which finds its id-space specified, will return a channel.

Ideally, global-id space isn't very clean, but for mailbox we anyway have to have a direct understanding between controller and client drivers. So having global IDs is a great tradeoff if we avoid messing up the api.

How is this different than expecting the client to pass a string name of the mbox controller it wants?

Ashwin

On 10 November 2014 08:39, Jassi Brar jaswinder.singh@linaro.org wrote:

On 10 November 2014 18:27, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 9 November 2014 23:13, Jassi Brar jaswinder.singh@linaro.org wrote:

...

...

...

...

based on the discussions that followed since, we decided that its best to add a separate PCC lookup and registration API. The main reason being, we dont have a way to list all mbox providers in ACPI in a way that DT does. e.g. in DT, the client->dev is used to look up mbox controllers. In ACPI, a client cant specify which mbox controllers to associate with, if it can attach to multiple. With the PCC specific API, if the client calls it, the controller knows where to look, because that lookup is PCC specific.

In your patch, the assumption that PCC is the only ACPI mbox provider, maybe true today, but that can change anytime.

Please read my patch again, we can have ACPI as well as DT populated clients. All that you intend to do with this patch can be done there and _without_ adding new apis.

Read it again. Not arguing that your patch wont work for DT and ACPI, but your assumption that ACPI supports PCC as the only mbox controller, may not hold true. The global_xlate function will work fine for PCC, but may not work for other ACPI (non-DT) mbox controllers. Using the signature field/index byte works only for PCC. We've already been through this discussion with Mark and Arnd and we came up with the PCC API.

Please read it yet again. There is no assumption that PCC is the only mbox in ACPI (though I think that is very likely). The function and its argument are both named _global_. 'Signature' is mentioned only as an example in case of PCC. The PCC controller driver could expect the global_id to be 'signature' of the subspace, similarly another non-DT mailbox controller driver will expect its own different 'signature' (say 0xdead0000 | id_16bits). In the patch I submitted we try .global_xlate() of all such mboxes and only one, which finds its id-space specified, will return a channel.

Ideally, global-id space isn't very clean, but for mailbox we anyway have to have a direct understanding between controller and client drivers. So having global IDs is a great tradeoff if we avoid messing up the api.

How is this different than expecting the client to pass a string name of the mbox controller it wants?

Global-ID is ugly, string matching is uglier. String matching requires changes to client and provider structures as opposed to simple numerical comparison to find a suitable channel.

And both have the problem that we cant guarantee uniqueness [1][2]. Having a separate API solves this problem.

Ashwin

[1] - http://www.spinics.net/lists/linux-acpi/msg52271.html [2] - http://www.spinics.net/lists/linux-acpi/msg52377.html

On 10 November 2014 09:16, Jassi Brar jaswinder.singh@linaro.org wrote:

On 10 November 2014 19:23, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 10 November 2014 08:39, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 10 November 2014 18:27, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 9 November 2014 23:13, Jassi Brar jaswinder.singh@linaro.org wrote:

...

...

>> based on the discussions that followed since, we decided that its best >> to add a separate PCC lookup and registration API. The main reason >> being, we dont have a way to list all mbox providers in ACPI in a way >> that DT does. e.g. in DT, the client->dev is used to look up mbox >> controllers. In ACPI, a client cant specify which mbox controllers to >> associate with, if it can attach to multiple. With the PCC specific >> API, if the client calls it, the controller knows where to look, >> because that lookup is PCC specific. >> >> In your patch, the assumption that PCC is the only ACPI mbox provider, >> maybe true today, but that can change anytime. >> > Please read my patch again, we can have ACPI as well as DT populated > clients. All that you intend to do with this patch can be done there > and _without_ adding new apis.

Read it again. Not arguing that your patch wont work for DT and ACPI, but your assumption that ACPI supports PCC as the only mbox controller, may not hold true. The global_xlate function will work fine for PCC, but may not work for other ACPI (non-DT) mbox controllers. Using the signature field/index byte works only for PCC. We've already been through this discussion with Mark and Arnd and we came up with the PCC API.

Please read it yet again. There is no assumption that PCC is the only mbox in ACPI (though I think that is very likely). The function and its argument are both named _global_. 'Signature' is mentioned only as an example in case of PCC. The PCC controller driver could expect the global_id to be 'signature' of the subspace, similarly another non-DT mailbox controller driver will expect its own different 'signature' (say 0xdead0000 | id_16bits). In the patch I submitted we try .global_xlate() of all such mboxes and only one, which finds its id-space specified, will return a channel.

Ideally, global-id space isn't very clean, but for mailbox we anyway have to have a direct understanding between controller and client drivers. So having global IDs is a great tradeoff if we avoid messing up the api.

How is this different than expecting the client to pass a string name of the mbox controller it wants?

Global-ID is ugly, string matching is uglier. String matching requires changes to client and provider structures as opposed to simple numerical comparison to find a suitable channel.

And both have the problem that we cant guarantee uniqueness [1][2].

How? Please give some scenario.

What is there to stop two users from coming up with the same signature (0xdeadbeef / "string") for their mbox controllers? Things can break at run time, because with your patch, the first mbox controller with the duplicate signature/name will return a channel. The client may be expecting a channel from the "other" mbox.

...

Having a separate API solves this problem.

NO. You add new api for PCC. If another non-DT provider appears (another instance of PCC or some new non-DT non-PCC mbox device) .... do you plan add yet another api?

Yes. Unfortunately thats the only way as Arnd suggested [1]. Again, the reason is that ACPI does NOT provide a way for client to mbox mapping in a way DT does. [1] You were CC'd on that thread. This patch has been under review for ~5months now and has undergone extensive review from Mark, Arnd and Lv. We're really going around in circles now.

By the way, your patch in this thread can't even cope with 2 instances of PCC (assuming that's possible as you think).

It was never meant to be. There are no two instances of PCC. But there could be another mbox provider(non-PCC) in ACPI in the future. Arnd was guessing something to appear from the newer Intel designs.[2]

Ashwin

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

Please read the part where Arnd suggests the following:

"If you require drivers to put global data (e.g. the mbox->name, or the channel number) in there, it's impossible to write a driver that works on both DT and ACPI. If you want to use the mbox_request_channel() interface from a driver, you need some form of lookup table in the ACPI data to do the conversion."

[2] - https://patches.linaro.org/32299/

Please read the very first comments.

"It's mostly a matter of consistency: We can have multiple interrupt controllers, pin controllers, clock controllers, dma engines, etc, and in the DT case we use references to the nodes wherever we have other devices referring to a mailbox name. I believe Intel's embedded chips are moving in the same direction with their ACPI support. If the ACPI spec gains support for mailbox devices, locking them into having only a single device may be a problem later for them."

On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote:

On 10 November 2014 20:17, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 10 November 2014 09:16, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 10 November 2014 19:23, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

Folks, please delete unneeded context from mails - it can be quite tedious to find the new content otherwise.

...

What is there to stop two users from coming up with the same signature (0xdeadbeef / "string") for their mbox controllers? Things can break at run time, because with your patch, the first mbox controller with the duplicate signature/name will return a channel. The client may be expecting a channel from the "other" mbox.

Two channels with same signature are supposed to be _identical_ i.e, either channel could serve any client asking for a channel with that signature. So even if an "unexpected" instance of the channel is assigned, the client should still be happy.

If a client differentiates between 2 instances of a channel, that's probably a sign of bad design. The knowledge behind client's preference of instance should actually lie on the provider(controller) side. I am open to some example on the contrary.

The problem here is that ACPI isn't defining the context in a way which maps well onto the way Linux looks things up. We may not like that and think it's a bad design but the spec is a done deal here and we have to address reality. From an ACPI point of view the context is the fact that this is a PCC channel (there's a globally unique namespace for PCC channels) but Linux wants a struct device for the client to represent the context with a mapping table of some kind behind that to do the lookup.

There's nothing in the ACPI description of the channel or controller to tie it to the client device, and there's nothing preventing some other mailbox mechanism that gets added to an ACPI system from reusing similar names (bear in mind that idiomatic naming for ACPI appears to be three or four character strings). If we have a PCC channel "FOO" and some new mailbox type which also defines "FOO" the controllers aren't really going to be able to tell them apart just on the string.

We could fit the maping into Linux a bit by having a struct device representing the PCC controller that you use to do the lookup but at that point you may as well have a PCC specific request function that knows that device and does the lookup which is approximately what we have in Ashwin's patch. We could also require that the lookup be something like "PCC:FOO" and use a global_xlate() but it's not clear to me that this is making things clearer.

On 11 November 2014 01:59, Arnd Bergmann arnd@arndb.de wrote:

On Monday 10 November 2014 20:13:10 Mark Brown wrote:

...

On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote:

...

On 10 November 2014 20:17, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

What is there to stop two users from coming up with the same signature (0xdeadbeef / "string") for their mbox controllers? Things can break at run time, because with your patch, the first mbox controller with the duplicate signature/name will return a channel. The client may be expecting a channel from the "other" mbox.

...

Two channels with same signature are supposed to be _identical_ i.e, either channel could serve any client asking for a channel with that signature. So even if an "unexpected" instance of the channel is assigned, the client should still be happy.

...

If a client differentiates between 2 instances of a channel, that's probably a sign of bad design. The knowledge behind client's preference of instance should actually lie on the provider(controller) side. I am open to some example on the contrary.

The problem here is that ACPI isn't defining the context in a way which maps well onto the way Linux looks things up. We may not like that and think it's a bad design but the spec is a done deal here and we have to address reality. From an ACPI point of view the context is the fact that this is a PCC channel (there's a globally unique namespace for PCC channels) but Linux wants a struct device for the client to represent the context with a mapping table of some kind behind that to do the lookup.

Right.

...

There's nothing in the ACPI description of the channel or controller to tie it to the client device, and there's nothing preventing some other mailbox mechanism that gets added to an ACPI system from reusing similar names (bear in mind that idiomatic naming for ACPI appears to be three or four character strings). If we have a PCC channel "FOO" and some new mailbox type which also defines "FOO" the controllers aren't really going to be able to tell them apart just on the string.

We could fit the maping into Linux a bit by having a struct device representing the PCC controller that you use to do the lookup but at that point you may as well have a PCC specific request function that knows that device and does the lookup which is approximately what we have in Ashwin's patch. We could also require that the lookup be something like "PCC:FOO" and use a global_xlate() but it's not clear to me that this is making things clearer.

Agreed. Having a special interface here the way that Ashwin's patch introduces seems the least invasive.

Just to be sure... please have a look at an alternative solution https://lkml.org/lkml/2014/11/9/75 I think 0 new api is lesser invasive than 2 new apis.

I don't think we would risk running into the problems that Jassi mentioned regarding API growth if we get other cases.

Do we anticipate .. a) more than one instance of PCC on a platform? b) PCC alongside another non-DT/ACPI mailbox device?

I discount (a). But for (b) it doesn't look neat to either add yet another PCC like api or make non-PCC clients call pcc_mbox_request_channel(). Adapting mbox_request_channe() to accommodate even non-DT clients (like PCC) seems cleaner and more future proof.

In particular, anything that boots with DT will be able to use the standard method, and even with ACPI if we have additional mailboxes we would most likely use the named properties extension in the future.

On a side note, I think we will actually have to add a DT binding to PCC as well, but that should probably provide both the standard mailbox API as well as support the pcc specific interfaces in order to allow client drivers that do not have multiple ways of doing the same thing.

In that case, wouldn't it be cleaner to have client drivers just call mbox_request_channel() irrespective of being populated by DT or ACPI? Please note, in my implementation the client just need to see if it can populate the mbox_client.dev to be able to work as such on both DT backed and ACPI backed controller driver.

-Jassi

On 10 November 2014 23:04, Jassi Brar jaswinder.singh@linaro.org wrote:

On 11 November 2014 01:43, Mark Brown broonie@kernel.org wrote:

...

On Mon, Nov 10, 2014 at 11:14:48PM +0530, Jassi Brar wrote:

...

On 10 November 2014 20:17, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 10 November 2014 09:16, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 10 November 2014 19:23, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

...

What is there to stop two users from coming up with the same signature (0xdeadbeef / "string") for their mbox controllers? Things can break at run time, because with your patch, the first mbox controller with the duplicate signature/name will return a channel. The client may be expecting a channel from the "other" mbox.

...

Two channels with same signature are supposed to be _identical_ i.e, either channel could serve any client asking for a channel with that signature. So even if an "unexpected" instance of the channel is assigned, the client should still be happy.

...

If a client differentiates between 2 instances of a channel, that's probably a sign of bad design. The knowledge behind client's preference of instance should actually lie on the provider(controller) side. I am open to some example on the contrary.

The problem here is that ACPI isn't defining the context in a way which maps well onto the way Linux looks things up. We may not like that and think it's a bad design but the spec is a done deal here and we have to address reality.

I don't mean that by 'bad design'. Ashwin asked what if a client wants the channel from, say, second instance of PCC instead of first (both are same controllers because they check for same signature).

2 channels from 2 "different" controllers. e.g. Non-PCC controllers, which don't have a "subspace" signature field like PCC does.

I can not imagine a good reason why would a client want that. If the reason is like only the channel of first controller is actually connected to remote (while that of second controller is nipped) ... I say that kind of board specific knowledge belongs closer to controller than the client. The second controller should have known that and not even populated the channel.

...

From an ACPI point of view the context is the fact that this is a PCC channel (there's a globally unique namespace for PCC channels) but Linux wants a struct device for the client to represent the context with a mapping table of some kind behind that to do the lookup.

There's nothing in the ACPI description of the channel or controller to tie it to the client device, and there's nothing preventing some other mailbox mechanism that gets added to an ACPI system from reusing similar names (bear in mind that idiomatic naming for ACPI appears to be three or four character strings). If we have a PCC channel "FOO" and some new mailbox type which also defines "FOO" the controllers aren't really going to be able to tell them apart just on the string.

ACPI does specify a PCC specific signature (0x50434300) for its channels. Any channel is identified by doing OR of that signature and the index of channel/subspace in the array of 256.

This is true only for PCC. Another class of controllers may not even have such a signature. The only thing you can be sure of is that they will have the 4 letter table identifier string e.g. PCCT.

IF another class of controllers is given the same name "_PCC", the ACPI will atleast assign it a different signature (if not ACPI, the controller driver for Linux). So we need not even think about using strings.

...

We could fit the maping into Linux a bit by having a struct device representing the PCC controller that you use to do the lookup but at that point you may as well have a PCC specific request function that knows that device and does the lookup which is approximately what we have in Ashwin's patch. We could also require that the lookup be something like "PCC:FOO" and use a global_xlate() but it's not clear to me that this is making things clearer.

We already have a way to circumvent all of that. I don't just object to Ashwin's patch, I posted a more generic and robust solution here https://lkml.org/lkml/2014/11/9/75

Arnd, Mark, Rafael, Lv, please^n ACK or NAK one of these patches and help us move on.

-Jassi

On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote:

On 11 November 2014 18:32, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 10 November 2014 23:04, Jassi Brar jaswinder.singh@linaro.org wrote:

In even simpler terms.... I prefer controller specific encoding(0x50434300) instead of controller specific api (pcc_mbox_request_channel). For a different class of controller, it is much cleaner to define a new encoding as compared to another xyz_mbox_request_channel() api.

The problem with this approach is that it still leaves the interface as controller specific, because the client now has to know that it must pass the PCC identifier instead of an index.

A client driver using DT would just specify to use the first channel that is defined locally in the "mboxes" properties, so it can ask for

chan = mbox_request_channel(client, 0);

to get the first channel defined for this client while a driver using ACPI has to look up the identifier in its own local tables:

u32 channel_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id);

which is not the same interface as the first, even if you assume the channel_id to be globally unique in the system.

Since we will need clients to have a consistent interface across DT and ACPI, I think the difference in firmware interfaces is better abstracted in a PCC specific interface.

The client driver would still need to parse two different firmware structures, but it would at least have a consistent interface:

u32 channel_id = of_property_read_u32(dev->of_node, "pccid"); if (!channel_id) channel_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id);

Alternatively, the driver could use the regular API on DT:

chan = mbox_request_channel(cl, 0); if (!chan) { u32 chan_id = this_driver_parse_acpi_and_find_pccid(dev); chan = mbox_request_channel(client, channel_id); }

For any firmware that we have control over, the mbox_request_channel() case could even be made to work in ACPI, if we decide to add the properties we use in DT with the new _DSD method.

Arnd

On Tuesday 11 November 2014 23:09:16 Jassi Brar wrote:

On 11 November 2014 22:03, Arnd Bergmann arnd@arndb.de wrote:

...

On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote:

...

On 11 November 2014 18:32, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

On 10 November 2014 23:04, Jassi Brar jaswinder.singh@linaro.org wrote:

In even simpler terms.... I prefer controller specific encoding(0x50434300) instead of controller specific api (pcc_mbox_request_channel). For a different class of controller, it is much cleaner to define a new encoding as compared to another xyz_mbox_request_channel() api.

The problem with this approach is that it still leaves the interface as controller specific, because the client now has to know that it must pass the PCC identifier instead of an index.

Yup. I hope you are aware that the "index" argument of pcc_mbox_request_channel() is just the same thing. The "index" there is actually the 'Type' value defined in ACPI for the client.

The problem is that it's not an index relative to the client, but into an array of the mailbox provide. I only today noticed that both are called 'index' in the source code, which is highly confusing, and the pcc driver should name it 'subspaceid' or similar instead, to minimize the confusion.

...

A client driver using DT would just specify to use the first channel that is defined locally in the "mboxes" properties, so it can ask for

    chan = mbox_request_channel(client, 0);

to get the first channel defined for this client while a driver using ACPI has to look up the identifier in its own local tables:

    u32 channel_id = this_driver_parse_acpi_and_find_pccid(dev);
    chan = mbox_request_channel(client, channel_id);

which is not the same interface as the first, even if you assume the channel_id to be globally unique in the system.

Yes. As I said, same for pcc_mbox_request_channel(). But since PCC clients are limited and specified by ACPI (Type value won't change across platforms, I understand), the client drivers could have that value hardcoded.

Not really:

...

Since we will need clients to have a consistent interface across DT and ACPI, I think the difference in firmware interfaces is better abstracted in a PCC specific interface.

The client driver would still need to parse two different firmware structures, but it would at least have a consistent interface:

    u32 channel_id = of_property_read_u32(dev->of_node, "pccid");
    if (!channel_id)
            channel_id = this_driver_parse_acpi_and_find_pccid(dev);
    chan = mbox_request_channel(client, channel_id);

Alternatively, the driver could use the regular API on DT:

    chan = mbox_request_channel(cl, 0);
    if (!chan) {
            u32 chan_id = this_driver_parse_acpi_and_find_pccid(dev);
            chan =  mbox_request_channel(client, channel_id);
    }

OK, but if the 'Type' value, that a client passes to (pcc_)mbox_req_chan(), does not change across platforms, would we need that?

As I understand the ACPI specification, we have multiple clients to the PCC, and each of them has its own client-specific hardcoded interface to /find/ the subspace ID from the ACPI tables, but the subspace ID itself has to be read from the table and is not constant.

Arnd

On 11 November 2014 14:08, Jassi Brar jaswinder.singh@linaro.org wrote:

Ashwin, would you fix the name and resubmit. Or I do s/index/subspace_id/ before committing?

I'll fix this and submit another patch.

Thanks, Ashwin

On Tue, Nov 11, 2014 at 05:33:28PM +0100, Arnd Bergmann wrote:

On Tuesday 11 November 2014 19:27:07 Jassi Brar wrote:

...

In even simpler terms.... I prefer controller specific encoding(0x50434300) instead of controller specific api (pcc_mbox_request_channel). For a different class of controller, it is much cleaner to define a new encoding as compared to another xyz_mbox_request_channel() api.

The problem with this approach is that it still leaves the interface as controller specific, because the client now has to know that it must pass the PCC identifier instead of an index.

Further to what Arnd is saying here what all these various patterns for looking up PCC mailboxes are saying to me is that whatever we're doing there's probably a place for a helper API that implements them; even if it's just a thin inline wrapper in a header somewhere it saves everyone having to get it right and means if we do get a DT binding or whatever they all magically end up with support.

On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:

On 07/11/14 14:52, Ashwin Chaugule wrote:

...

+config PCC

•   bool "Platform Communication Channel Driver"
  

•   depends on ACPI
  

I am bit confused here, though I prefer PCC to depend on ACPI, I have seen discussion in this thread about using PCC without ACPI, how will that work ?

Been discussed early on. DT users, if at all, will need to come up with their own DT bindings to make this work.

...

+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];

Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ?

Spec says, max of 256. Easier to support it this way.

...

+/**

• • pcc_tx_done - Callback from Mailbox controller code to

• •         check if PCC message transmission completed.
    

• • @chan: Pointer to Mailbox channel on which previous

• •         transmission occurred.
    

• • Return: TRUE if succeeded.
• */

+static bool pcc_tx_done(struct mbox_chan *chan) +{

•   struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

IIUC, the PCCT table has the physical base Address of the shared memory range in the PCC subspace structures. Can you use that directly here ? Or are you mapping that memory elsewhere and updating the mapped address to the table ?

In the client side. Its really owned by the client. pcc_tx_done can never be called on a channel which doesn't have a mapped base address.

...

•   u16 cmd_delay = pcct_ss->min_turnaround_time;
  

•   unsigned int retries = 0;
  

•   /* Try a few times while waiting for platform to consume */
  

•   while (!(readw_relaxed(&generic_comm_base->status)
  

This will explode if the pcct_ss->base_address is not mapped.

It is. Client side. Called only _after_ client is done with its thing.

...

•               & PCC_CMD_COMPLETE)) {
  

•           if (retries++ < 5)
  

•                   udelay(cmd_delay);
  

•           else {
  

•                   /*
  

•                    * If the remote is dead, this will cause the Mbox
  

•                    * controller to timeout after mbox client.tx_tout
  

•                    * msecs.
  

•                    */
  

•                   pr_err("PCC platform did not respond.\n");
  

•                   return false;
  

•           }
  

•   }
  

•   return true;
  

+}

In general you can isolate all the access to the shared memory in the protocol layer. In that case you might have to use mbox_client_txdone instead of this pcc_tx_done.

I dont see the problem in keeping this way though.

...

+/**

• • pcc_send_data - Called from Mailbox Controller code to finally

• • transmit data over channel.
    

• • @chan: Pointer to Mailbox channel over which to send data.
• • @data: Actual data to be written over channel.
• • Return: Err if something failed else 0 for success.
• */

+static int pcc_send_data(struct mbox_chan *chan, void *data) +{

•   struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

•   struct acpi_generic_address doorbell;
  

•   u64 doorbell_preserve;
  

•   u64 doorbell_val;
  

•   u64 doorbell_write;
  

•   u16 cmd = *(u16 *) data;
  

•   u16 ss_idx = -1;
  

•   ss_idx = get_subspace_id(chan);
  

•   if (ss_idx < 0) {
  

•           pr_err("Invalid Subspace ID from PCC client\n");
  

•           return -EINVAL;
  

•   }
  

•   doorbell = pcct_ss->doorbell_register;
  

•   doorbell_preserve = pcct_ss->preserve_mask;
  

•   doorbell_write = pcct_ss->write_mask;
  

•   /* Write to the shared comm region. */
  

•   writew(cmd, &generic_comm_base->command);
  

•   /* Write Subspace MAGIC value so platform can identify
  

destination. */

•   writel((PCCS_SS_SIG_MAGIC | ss_idx),
  

&generic_comm_base->signature);

•   /* Flip CMD COMPLETE bit */
  

•   writew(0, &generic_comm_base->status);
  

IMO it's not clean to modify only first 3 elements namely: Signature, Command and Status while the Communication Space is updated elsewhere. It's better to have all of them in one place if possible as mentioned above.

The access sequence for first 3 elements is the same for each channel. Thats all the controller is expected to do on behalf of the client. So this function reduces code duplication in all client sites.

...

•   /* Sync notification from OSPM to Platform. */
  

•   acpi_read(&doorbell_val, &doorbell);
  

•   acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
  

•                   &doorbell);
  

Only the above 2 must be part of the controller driver, the remaining as I mentioned can be move to the protocol/client layer, thoughts ?

I'd really prefer for it to be separate. Once client is done writing, it calls this and lets the controller driver take over the rest. Its not racy.

...

•   count = acpi_table_parse_entries(ACPI_SIG_PCCT,
  

•                   sizeof(struct acpi_table_pcct),
  

s/struct acpi_table_pcct/*pcct_tbl/

In general, the interrupt part of the PCCT SS is not considered in this patch. It's better to see/discuss how that can be fit into the mailbox framework, though it could be follow up patch.

The IRQ part of the spec seems to be under discussion (single irq per subspace / common IRQ across all) and as you may be aware we're working on trying it out on Juno. That'll guide the design. What I have here is good enough to start off with and has been tested. I dont think we should have a problem using the mailbox API for asyn tx though, but I'd really^n prefer if we get something out there first.

Thanks, Ashwin

On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote:

On 11/11/14 13:18, Ashwin Chaugule wrote:

...

On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:

...

On 07/11/14 14:52, Ashwin Chaugule wrote:

...

+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];

Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ?

Spec says, max of 256. Easier to support it this way.

I tend to disagree, you can allocate dynamically. And use exactly same logic you use in get_subspace_id to populate con_priv later. When you parse PCC Subspace, you can just validate header and not record them in pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries and then do the required allocation.

Right, I think that would be best.

...

...

...

•   count = acpi_table_parse_entries(ACPI_SIG_PCCT,
  

•                   sizeof(struct acpi_table_pcct),
  

s/struct acpi_table_pcct/*pcct_tbl/

In general, the interrupt part of the PCCT SS is not considered in this patch. It's better to see/discuss how that can be fit into the mailbox framework, though it could be follow up patch.

The IRQ part of the spec seems to be under discussion (single irq per subspace / common IRQ across all) and as you may be aware we're working on trying it out on Juno. That'll guide the design. What I have here is good enough to start off with and has been tested. I dont think we should have a problem using the mailbox API for asyn tx though, but I'd really^n prefer if we get something out there first.

That's the different and still under discussion. But you need to support Type 1 subspace as it stands in ACPI v5.1

Why? We should only implement whatever is required to support existing hardware, not because something is in the spec.

Arnd

On 12 November 2014 08:32, Sudeep Holla sudeep.holla@arm.com wrote:

On 11/11/14 20:25, Arnd Bergmann wrote:

...

On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote:

...

On 11/11/14 13:18, Ashwin Chaugule wrote:

...

On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:

...

On 07/11/14 14:52, Ashwin Chaugule wrote:

The IRQ part of the spec seems to be under discussion (single irq per subspace / common IRQ across all) and as you may be aware we're working on trying it out on Juno. That'll guide the design. What I have here is good enough to start off with and has been tested. I dont think we should have a problem using the mailbox API for asyn tx though, but I'd really^n prefer if we get something out there first.

That's the different and still under discussion. But you need to support Type 1 subspace as it stands in ACPI v5.1

Why? We should only implement whatever is required to support existing hardware, not because something is in the spec.

Agreed. I assumed that this was tested on some hardware which adhere to Type 1 subspace of the spec and I asked to implement interrupt mode as it is always better compared to polling mode and current spec. has support for the interrupts.

I do not have hardware to test the IRQ parts as of now, so the plan was to at least get polling mode out there. That will enable a lot of folks and will be useful as a fallback mode later on as well. We can add IRQ mode stuff once it is tested. I dont see the point in supporting it all from the start.

Also, the existing Type1 is not sufficient for the mailbox/PCC on Juno platform, hence the new proposal.

Just as Juno is proving that the IRQ mode in the current spec is insufficient, we may find similar issues with other hardware as it becomes available. It makes more sense to tackle this once we have some hardware to test this part of PCC properly. Just because its in the spec doesn't mean its right. ;)

Btw, if you or anyone has any hardware to test it on, patches are more than welcome. :)

Thanks, Ashwin

On 11 November 2014 15:25, Arnd Bergmann arnd@arndb.de wrote:

On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote:

...

On 11/11/14 13:18, Ashwin Chaugule wrote:

...

On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:

...

On 07/11/14 14:52, Ashwin Chaugule wrote:

...

+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];

Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ?

Spec says, max of 256. Easier to support it this way.

I tend to disagree, you can allocate dynamically. And use exactly same logic you use in get_subspace_id to populate con_priv later. When you parse PCC Subspace, you can just validate header and not record them in pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries and then do the required allocation.

Right, I think that would be best.

Discussed this with Sudeep on IRC. The parse_pcc_subspace() function actually is a useful place to create new mbox channel entries. We get a pointer to the pcct_ss (pcc subspace subtable) in it, for each entry as it is discovered in the PCCT. If we ignore that pointer, then we will need to re-traverse the PCCT in the probe() function, just to get those pointers again.

In theory, we could re-traverse the PCCT in the probe function without using the API provided by the ACPI framework, but that sounds dicey. In the end we would be doing this only to maintain the clever-ish ptr math as it is in get_subspace_id(). Alternately, we could replace the array with a list.

e.g.

struct pcc_channel { struct list_head node; struct mbox_chan chan; };

Then use list_for_each_entry() to get the subspace id from a struct mbox_chan* , and list_for_each() + list_entry() to get the channel from a subspace index. These lists are not expected to get long in practice so traversing them for lookups is not really expensive and its only done at init.

Does this make sense?

Thanks, Ashwin

On 12 November 2014 23:55, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

On 11 November 2014 15:25, Arnd Bergmann arnd@arndb.de wrote:

...

On Tuesday 11 November 2014 15:01:28 Sudeep Holla wrote:

...

On 11/11/14 13:18, Ashwin Chaugule wrote:

...

On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:

...

On 07/11/14 14:52, Ashwin Chaugule wrote:

...

+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];

Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ?

Spec says, max of 256. Easier to support it this way.

I tend to disagree, you can allocate dynamically. And use exactly same logic you use in get_subspace_id to populate con_priv later. When you parse PCC Subspace, you can just validate header and not record them in pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries and then do the required allocation.

Right, I think that would be best.

Discussed this with Sudeep on IRC. The parse_pcc_subspace() function actually is a useful place to create new mbox channel entries. We get a pointer to the pcct_ss (pcc subspace subtable) in it, for each entry as it is discovered in the PCCT. If we ignore that pointer, then we will need to re-traverse the PCCT in the probe() function, just to get those pointers again.

In theory, we could re-traverse the PCCT in the probe function without using the API provided by the ACPI framework, but that sounds dicey. In the end we would be doing this only to maintain the clever-ish ptr math as it is in get_subspace_id(). Alternately, we could replace the array with a list.

e.g.

struct pcc_channel { struct list_head node; struct mbox_chan chan; };

Then use list_for_each_entry() to get the subspace id from a struct mbox_chan* , and list_for_each() + list_entry() to get the channel from a subspace index. These lists are not expected to get long in practice so traversing them for lookups is not really expensive and its only done at init.

Does this make sense?

But what array of channels do you pass to mbox_controller_register()?

-jassi

On Tue, Nov 11, 2014 at 10:30:32AM +0000, Sudeep Holla wrote:

On 07/11/14 14:52, Ashwin Chaugule wrote:

...

+config PCC

•   bool "Platform Communication Channel Driver"
  

•   depends on ACPI
  

I am bit confused here, though I prefer PCC to depend on ACPI, I have seen discussion in this thread about using PCC without ACPI, how will that work ?

We can always remove the dependency when someone adds the code to talk to non-ACPI PCC devices.

On 7 November 2014 20:22, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,

•           int index)
  

+{

•   struct device *dev = pcc_mbox_ctrl.dev;
  

•   struct mbox_chan *chan;
  

•   unsigned long flags;
  

•   /*
  

•    * Each PCC Subspace is a Mailbox Channel.
  

•    * The PCC Clients get their PCC Subspace ID
  

•    * from their own tables and pass it here.
  

•    * This returns a pointer to the PCC subspace
  

•    * for the Client to operate on.
  

•    */
  

•   chan = &pcc_mbox_chan[index];
  

You are mapping 'Type' of a client directly onto SubSpace-ID of the channel allocated. If two clients need an instance of, say, Generic-Communications-Channel i.e, Type-0, this will not work. If no two clients can ask the same channel, MAX_PCC_SUBSPACES should be limited to max 'Type' value defined in ACPI specs (just 2 in ACPI-5.1).

•   if (!chan || chan->cl) {
  

•           dev_err(dev, "%s: PCC mailbox not free\n", __func__);
  

•           return ERR_PTR(-EBUSY);
  

•   }
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->msg_free = 0;
  

•   chan->msg_count = 0;
  

•   chan->active_req = NULL;
  

•   chan->cl = cl;
  

•   init_completion(&chan->tx_complete);
  

•   if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
  

•           chan->txdone_method |= TXDONE_BY_ACK;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

•   return chan;
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);

Nothing PCC specific here. All you have done is define a new api just to pick an element from the array. You simply needed the mbox_request_channel() let you through to controller.

+void pcc_mbox_free_channel(struct mbox_chan *chan) +{

•   unsigned long flags;
  

•   if (!chan || !chan->cl)
  

•           return;
  

•   spin_lock_irqsave(&chan->lock, flags);
  

•   chan->cl = NULL;
  

•   chan->active_req = NULL;
  

•   if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
  

•           chan->txdone_method = TXDONE_BY_POLL;
  

•   spin_unlock_irqrestore(&chan->lock, flags);
  

+} +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);

Ditto, nothing PCC in this new PCC specific api.

+/**

• • pcc_send_data - Called from Mailbox Controller code to finally

• • transmit data over channel.
    

• • @chan: Pointer to Mailbox channel over which to send data.
• • @data: Actual data to be written over channel.
• • Return: Err if something failed else 0 for success.
• */

+static int pcc_send_data(struct mbox_chan *chan, void *data) +{

•   struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

•   struct acpi_generic_address doorbell;
  

•   u64 doorbell_preserve;
  

•   u64 doorbell_val;
  

•   u64 doorbell_write;
  

•   u16 cmd = *(u16 *) data;
  

•   u16 ss_idx = -1;
  

•   ss_idx = get_subspace_id(chan);
  

•   if (ss_idx < 0) {
  

•           pr_err("Invalid Subspace ID from PCC client\n");
  

•           return -EINVAL;
  

•   }
  

•   doorbell = pcct_ss->doorbell_register;
  

•   doorbell_preserve = pcct_ss->preserve_mask;
  

•   doorbell_write = pcct_ss->write_mask;
  

•   /* Write to the shared comm region. */
  

•   writew(cmd, &generic_comm_base->command);
  

•   /* Write Subspace MAGIC value so platform can identify destination. */
  

•   writel((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
  

•   /* Flip CMD COMPLETE bit */
  

•   writew(0, &generic_comm_base->status);
  

•   /* Sync notification from OSPM to Platform. */
  

•   acpi_read(&doorbell_val, &doorbell);
  

•   acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
  

•                   &doorbell);
  

•   return 0;
  

+}

+static struct mbox_chan_ops pcc_chan_ops = {

•   .send_data = pcc_send_data,
  

•   .last_tx_done = pcc_tx_done,
  

+};

+/**

• • parse_pcc_subspace - Parse the PCC table and extract PCC subspace

• •         entries. There should be one entry per PCC client.
    

• • @header: Pointer to the ACPI subtable header under the PCCT.
• • @end: End of subtable entry.
• • Return: 0 for Success, else errno.
• • This gets called for each entry in the PCC table.
• */

+static int parse_pcc_subspace(struct acpi_subtable_header *header,

•           const unsigned long end)
  

+{

•   struct acpi_pcct_hw_reduced *pcct_ss;
  

•   if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
  

•           pcct_ss = (struct acpi_pcct_hw_reduced *) header;
  

•           if (pcct_ss->header.type !=
  

•                           ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) {
  

•                   pr_err("Incorrect PCC Subspace type detected\n");
  

•                   return -EINVAL;
  

•           }
  

The driver supports only ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE client ?!

-Jassi

On 11 November 2014 20:49, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

On 11 November 2014 10:12, Jassi Brar jaswinder.singh@linaro.org wrote:

...

On 7 November 2014 20:22, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:

...

+struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,

•           int index)
  

+{

•   struct device *dev = pcc_mbox_ctrl.dev;
  

•   struct mbox_chan *chan;
  

•   unsigned long flags;
  

•   /*
  

•    * Each PCC Subspace is a Mailbox Channel.
  

•    * The PCC Clients get their PCC Subspace ID
  

•    * from their own tables and pass it here.
  

•    * This returns a pointer to the PCC subspace
  

•    * for the Client to operate on.
  

•    */
  

•   chan = &pcc_mbox_chan[index];
  

You are mapping 'Type' of a client directly onto SubSpace-ID of the channel allocated. If two clients need an instance of, say, Generic-Communications-Channel i.e, Type-0, this will not work. If no two clients can ask the same channel, MAX_PCC_SUBSPACES should be limited to max 'Type' value defined in ACPI specs (just 2 in ACPI-5.1).

Intentional. Type 0 existed for X86 systems and nobody ever used it so far in Linux. Upcoming systems will use ACPI v5.1 defined Type 1. We want to get something out there and will start with Type 1 as the default. As a follow patchset, we will add Type 2, when its ready and Type 0 if at all it is required. Rafael/Lv and Mark have not objected to this.

Well, I do. People might be busy to have overlooked it.

...

The driver supports only ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE client ?!

See above.

The code seems incomplete and broken at many levels. Not to forget the API messup.

-jassi