On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
> 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.
How is this different than expecting the client to pass a string name
of the mbox controller it wants?
Ashwin
--
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
