Re: [RFC PATCH 00/31] CPUFreq on ARM

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 15/11/17 03:03, Jassi Brar wrote:
> On 15 November 2017 at 02:16, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote:
>> On Tue, Nov 14, 2017 at 12:49 PM, Andre Przywara
>> <andre.przywara@linaro.org> wrote:
>>
> 
>>>>>> 3. Direct ported SCPI protocol, mailbox infrastructure and the ARM SMC triggered mailbox driver. All components except mailbox driver are in mainline Linux.
>>>>>
>>>>> Why do you actually need this mailbox framework?
>>
> It is unnecessary if you are always going to use one particular signal
> mechanism, say SMC. However ...
> 
>>>>> Actually I just
>>>>> proposed the SMC driver the make it fit into the Linux framework. All we
>>>>> actually need for SCPI is to write a simple command into some memory and
>>>>> "press a button". I don't see a need to import the whole Linux
>>>>> framework, especially as our mailbox usage is actually just a corner
>>>>> case of the mailbox's capability (namely a "single-bit" doorbell).
>>>>> The SMC use case is trivial to implement, and I believe using the Juno
>>>>> mailbox is similarly simple, for instance.
>>
> ... Its going to be SMC and MHU now... and you talk about Rockchip as
> well later. That becomes unwieldy.
> 
> 
>>>
>>>> Protocol relies on mailbox feature, so I ported mailbox too. I think,
>>>> it would be much more easy for me to just add
>>>> a few required commands handling with issuing SMC call and without any
>>>> mailbox infrastructure involved.
>>>> But, I want to show what is going on and what place these things come from.
>>>
>>> I appreciate that, but I think we already have enough "bloated" Linux +
>>> glue code in Xen. And in particular the Linux mailbox framework is much
>>> more powerful than we need for SCPI, so we have a lot of unneeded
>>> functionality.
>>
> That is a painful misconception.
> Mailbox api is designed to be (almost) as light weight as being
> transparent. Please have a look at mbox_send_message() and see how
> negligible overhead it adds for "SMC controller" that you compare
> against here..... just integer manipulations protected by a spinlock.
> Of course if your protocol needs async messaging, you pay the price
> but only fair.

Normally I would agree on importing some well designed code rather than
hacking up something yourself.

BUT: This is Xen, which is meant to be lean, micro-kernel like
hypervisor. If we now add code from Linux, there must be a good
rationale why we need it. And this is why we need to make sure that
CPUFreq is really justified in the first place.
So I am a bit wary that pulling some rather unrelated Linux *framework*
into Xen bloats it up and introduces more burden to the trusted code
base. With SCPI being the only user, this controller - client
abstraction is not really needed. And to just trigger an interrupt on
the SCP side we just need to:
	writel(BIT(channel), base_addr + CPU_INTR_H_SET);

I expect other mailboxes to be similarly simple.
The only other code needed is some DT parsing.

That being said I haven't look too closely how much code this actually
pulls in, it is just my gut feeling that it's a bit over the top,
conceptually.

>>> If we just want to support CPUfreq using SCPI via SMC/Juno MHU/Rockchip
>>> mailbox, we can get away with a *much* simpler solution.
>>
>> Agree, but I am afraid that simplifying things now might lead to some
>> difficulties when there is a need
>> to integrate a little bit different mailbox IP. Also, we need to
>> recheck if SCMI, we might want to support as well,
>> have the similar interface with mailbox.
>>
> Exactly.

My understanding is that the SCMI transport protocol is not different
from that used by SCPI.

Cheers,
Andre.

>>> - We would need to port mailbox drivers one-by-one anyway, so we could
>>> as well implement the simple "press-the-button" subset for each mailbox
>>> separately.
>>
> Is it about virtual controller?
> 
>>> The interface between the SCPI code and the mailbox is
>>> probably just "signal_mailbox()".
>>
> Afterall we should have the following to spread the nice feeling of
> "supporting doorbell controllers"  :)
> 
> mailbox_client.h
> *******************
> void signal_mailbox(struct mbox_chan *chan)
> {
>    (void)mbox_send_message(chan, NULL);
>    mbox_client_txdone(chan, 0);
> }
> 
> 
> Cheers!
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel