Re: [PATCH v11 2/6] mailbox: qcom: Create APCS child device for clock controller

[Georgi Djakov]

Hi Jassi,

On 01/27/2018 05:44 AM, Jassi Brar wrote:
> On Thu, Jan 4, 2018 at 10:26 PM, Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>> Hi Jassi,
>>
>> On 12/29/2017 08:14 AM, Jassi Brar wrote:
>>> Hi Bjorn,
>>>
>>> On Sun, Dec 24, 2017 at 10:36 AM, Bjorn Andersson
>>> <bjorn.andersson@xxxxxxxxxx> wrote:
>>>> On Fri 22 Dec 20:57 PST 2017, Jassi Brar wrote:
>>>>
>>>>> On Tue, Dec 5, 2017 at 9:16 PM, Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>>>>>> There is a clock controller functionality provided by the APCS hardware
>>>>>> block of msm8916 devices. The device-tree would represent an APCS node
>>>>>> with both mailbox and clock provider properties.
>>>>>>
>>>>> The spec might depict a 'clock' box and 'mailbox' box inside the
>>>>> bigger APCS box. However, from the code I see in this patchset, they
>>>>> are orthogonal and can & should be represented as independent DT
>>>>> nodes.
>>>>
>>>> The APCS consists of a number of different hardware blocks, one of them
>>>> being the "APCS global" block, which is what this node and drivers
>>>> relate to. On 8916 this contains both the IPC register and clock
>>>> control. But it's still just one block according to the hardware
>>>> specification.
>>>>
>>>> As such DT should describe the one hardware block by one node IMHO.
>>>>
>>> In my even humbler opinion, DT should describe a h/w functional unit
>>> which _could_ be seen as a standalone component.
>>
>> The APCS is one separate register block related to the CPU cluster. I
>> haven't seen any strict guidelines for such cases in the DT docs, and
>> during the discussion got the impression that this is the preferred
>> binding. Rob has also reviewed the binding, so we should be fine to move
>> forward with this one.
>>
> Well, I can't overrule Rob. But I am really not happy with random
> device spawning from mailbox drivers. I know there are such instances
> already in the kernel but that doesn't make it legit... unless there
> is some hard dependency. Is there?

The dependency is that on this SoC, these functionalities are combined
into this "CPU subsystem" block.

Initially APCS was a syscon and it was discussed it an year ago in this
mail thread [1] and we are trying to move away from the syscon and come
up with some bindings. During v8 and v9 of this patchset i have switched
from multiple child nodes to a single node as this seem to be the
preferred form.

>>> For example, if this APCS had a mac controller, would we also populate
>>> a netdev from mailbox driver? And what if next revision moves/drops
>>> this clock controller out of APCS, keeping mailbox controller exactly
>>> same?
>>
>> The clock controller may change in some next SoC architecture and that's
>> why the SoC version is also part of the the compatible string.
>>
> So the mailbox driver will be updated to spawn yet another type of clock?
> And again for next revision and so on... I know that is unlikely but
> the point is why not have separate clock drivers for independent h/w
> clocks?

Each revision re-shuffles the APCS block, so at least the offsets would
be different. I don't see anything bad with spawning a child device, but
what are the other options? Should we go back on the bindings and
suggest something new all over again? Or do you have any other idea?
Could you please explain your point further?

Thanks,
Georgi

[1] https://lkml.org/lkml/2017/3/20/991