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.
>
>> 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
--
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
