Message-ID: <6bd1e3d5-5e8b-6d02-cd80-9ff2c21de15d@arm.com>
Date:   Tue, 9 May 2017 13:41:10 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Jassi Brar <jassisinghbrar@...il.com>
Cc:     Sudeep Holla <sudeep.holla@....com>, Rob Herring <robh@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Klimov <alexey.klimov@....com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [PATCH 2/6] Documentation: devicetree: add bindings to support
 ARM MHU subchannels



On 09/05/17 12:55, Jassi Brar wrote:
> On Tue, May 9, 2017 at 4:23 PM, Sudeep Holla <sudeep.holla@....com> wrote:
>>
>>
>> On 09/05/17 11:31, Jassi Brar wrote:
>>> On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla@....com>
>>> wrote:
>>>
>>>>>
>>>>> If it is still not clear, please share your client driver. I
>>>>> will adapt that to work with existing MHU driver & bindings.
>>>>>
>>>>
>>>> Just take example of SCPI in the mainline. Assume there's another
>>>> protocol SCMI which uses few more bits in the same channel and the
>>>> remote firmware implements both but both are totally independent
>>>> and not related/linked. Also be keep in mind that SCPI is used by
>>>> other platforms and so will be the new protocol. We simply make
>>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out.
>>>>
>>> Not sure what you mean by "that's ruled out".
>>
>> 1. The mailbox client bindings should be independent of this ARM MHU
>>    mailbox bindings
>> 2. All we need in client is a mailbox to point at and not any meta data
>>    That's what I meant by ruled-out as both client and MHU can be used
>>    independent of each other and *should not* be linked.
>>
> I am shocked at this coming from you.
> 
> You design SCMI based upon MHU assumption of single bit "doorbell" and
> then you say a client should be independent of the underlying
> controller? Do you intend SCMI to work only over MHU?
> 

No, I never said that. What I said is SCMI protocol will be on doorbell
based.

>  What if some controller does not support the simple "doorbell" and
> expects detailed info? For example, apart from SCMI, the remote also
> supports platform specific functions like thermal, watchdog, wakeup
> etc. The SCMI's would just be a subset of the full command set.
> You/SCMI can not dictate what numerical value the platform assigns to
> SCMI commands... 

What ? That's the whole point of specification. The command set is
*fixed* and can be implemented on any platform and have generic driver
for that.

> all that is required is the remote firmware should
> uniquely identify which command is it and implement what the SCMI
> protocol expects.
> 

Yes, not just uniquely but exactly as the specification.

> Have a look at  mbox_send_message(struct mbox_chan *chan, void *mssg)
> The 'mssg' is the pointer to _controller-specified_ structure. The
> client driver (SCMI) _must_ know what the underlying controller
> expects and pass that info. For example of 'mssg', look at "struct
> brcm_message" in include/linux/mailbox/brcm-message.h   The client
> driver must use that platform specific knowledge to send a message
> i.e, you can not make a mailbox client driver 100% provider agnostic.
> 

I disagree. The whole idea is to make it generic with doorbell kind of
logic. We don't pass any command in the doorbell register, everything is
part of shared memory and all standardized.

> You need to divide SCMI into two parts - one that manages the SCMI
> protocol and the other platform specific glue that talks to the
> mailbox controller over the mailbox api.
> 

Why ? That's totally unnecessary IMO.

> 
>>> You have already shared this "v2" MHU driver, now please also share
>>> your client driver. I'll make it work with original MHU driver and
>>> that should settle your confusion.
>>
>> It should first work with SCPI in the mainline. Then we will add another
>> similar protocol soon. So I think you have all you need in the mainline.
>> Today we have hack in the SCPI driver to pass bit 0 set in data. But
>> that's broken as we may want different slot on some other platform.
>> Basically SCPI is designed with the use of doorbell and it should not
>> have any details on how to write that into a particular register as
>> along as we just choose the right channel.
>>

Check arm_scpi.c, the aim is to eliminate the need to send the data with
BIT(0) set as that's platform specific and the protocol has to be generic.

>> On digging more about different mailbox controllers, I found
>> mailbox-sti.c has exactly similar logic as what I have done in this series.
>>

Did you look at this driver ?

>> Also don't mix implementation with the binding. I need a simple answer
>> in this binding. How do I represent specific bits if each bit is
>> implemented as a doorbell ? That's all. First let's agree on that when
>> we use this mailbox independently and please *don't mix* with any
>> client here. It's simple, this controller has 2-3 sets of 32 doorbell
>> bits. And I am aiming to come up with the binding for that as your
>> initial bindings didn't consider that.
>>
> Please send in whatever changes you plan to do, and I'll modify it so
> we don't have to bloat the MHU driver and add bindings for a software
> feature. Until then ... Cheers!
> 

Changes to what ? arm_mhu.c ? This series is complete and implements
doorbell completely.

You seem to have missed to answer some of my questions above. Please do.

-- 
Regards,
Sudeep

Powered by blists - more mailing lists