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