On 10 November 2014 09:16, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote: > On 10 November 2014 19:23, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote: >> On 10 November 2014 08:39, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote: >>> On 10 November 2014 18:27, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote: >>>> On 9 November 2014 23:13, Jassi Brar <jaswinder.singh@xxxxxxxxxx> 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." -- 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
