Re: [PATCH 0/6] mailbox: arm_mhu: add support for subchannels

[Sudeep Holla]

On 03/05/17 04:17, Jassi Brar wrote:
> Hi Sudeep,
> 
> On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>> Hi Jassi,
>>
>> This series adds subchannel support to ARM MHU mailbox controller
>> driver. Since SCPI never used second slot, we were able to use the
>> existing driver as is. However, that's changing soon and the new
>> SCMI protocol under development needs subchannel support. If you
>> recall when you initially added this driver, I was pushing for some
>> of these changes like threaded irq. This patch series adds support
>> for the subchannels on ARM MHU controllers.
>>
>   There are really no "sub-channels" in the ARM MHU controller. There
> are exactly three channels that work on 32bit registers. The SET/CLEAR
> registers are there to prevent races between local and remote
> firmware, and not to emulate virtual channels operating on single
> bits. Please remember all 32-bits work together to generate one
> signal.
> 

If you check 3.4.4 Message Handling Unit (MHU) of Juno TRM [1],

"..the MHU drives the signal using a 32-bit register, with all 32 bits
logically ORed together. The MHU provides a set of registers to enable
software to set, clear, and check the status of each of the bits of this
register independently. The use of 32 bits for each interrupt
line enables software to provide more information about the source of
the interrupt. For example, each bit of the register can be associated
with a type of event that can contribute to raising the interrupt."

So yes, they generate one signal, but that doesn't mean anything. We
have even PMU interrupts tied to single SPI on some SoC. Since the
design of MHU clearly indicates that each bit can be used independently
for different event, for all practical purpose, it can be treated as
different channel.

>  You arrived at the "sub-channel" idea only because your protocol uses
> 1-bit messages. 

May be. It now uses BIT 0 for one channel and BIT 1 for another on the
same physical channel. How do you propose it support that then ? We have
multiple protocols with the same remote, so this is just used as a
doorbell bit and not carrier of any message.

> This patchset seems rather regressive - reduce from
> 2^32 possible signals to mere 32, by bloating the MHU driver.
> 

I don't quite get this. There are only 3 signals as you mentioned above.
Yes there are 2^32 possible values for the register, but how can that be
used ? I don't think that was the design intention at-least from the
above text in the specification. Also the we still continue to support
one channel per physical channel.

>  If it's difficult to see how your protocol can be implemented over
> existing controller driver, please let me know.
> 

It was a workaround just because there was no other protocol so far.
I just set bit 0 and send it as u32 to send_message.

-- 
Regards,
Sudeep

[1]
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515f/DDI0515F_juno_arm_development_platform_soc_trm.pdf