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

[Jassi Brar]

On Wed, May 3, 2017 at 2:51 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
>
> 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.
>
That means a lot. That means a MHU signal/message is 32bits, not single bit.

> 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.
>
Please don't mess with controller driver to support your usecase,
which is already well supported.

>>  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 ?
>
There is no "sub-channel", but only physical channel.
You are led to believe each bit represents one channel only because
your protocol uses (1<<N) type signals.

> We have
> multiple protocols with the same remote, so this is just used as a
> doorbell bit and not carrier of any message.
>
Doorbell is a single bit message in mailbox framework :)

>> 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 ?
>
_Your_ protocol don't use more than 32 values, that doesn't mean other
protocols don't either.