On 9 November 2014 22:52, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote:
> On 9 November 2014 11:18, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
>> On 9 November 2014 20:50, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote:
>>> Hi Jassi,
>>>
>>> On 9 November 2014 08:48, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
>>>> On 7 November 2014 20:22, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> 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.
-Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
