[PATCH 7/8] mailbox: f_mhu: add driver for Fujitsu MHU controller

[Jassi Brar]

On 17 July 2014 16:01, Sudeep Holla <sudeep.holla at arm.com> wrote:
> On 17/07/14 07:25, Jassi Brar wrote:
>>
>>>
>>>> +       u32 val;
>>>> +
>>>> +       pr_debug("%s:%d\n", __func__, __LINE__);
>>>
>>>
>>>
>>> Please remove all these debug prints.
>>>
>> These are as good as absent unless DEBUG is defined.
>>
>
> Yes, but with mailbox framework, the controller driver(esp. this one)is
> almost like a shim layer. Instead of each driver adding these debugs,
> IMO it would be good to have these in the framework.
>
OK

>>>
>>>> +       /* See NOTE_RX_DONE */
>>>> +       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
>>>> +       mbox_chan_received_data(chan, (void *)val);
>>>> +
>>>> +       /*
>>>> +        * It is agreed with the remote firmware that the receiver
>>>> +        * will clear the STAT register indicating it is ready to
>>>> +        * receive next data - NOTE_RX_DONE
>>>> +        */
>>>
>>>
>>> This note could be added as how this mailbox works in general and
>>> it's not just Rx right ? Even Tx done is based on this logic.
>>> Basically the logic is valid on both directions.
>>>
>> Yes that is a protocol level agreement. Different f/w may behave
>> differently.
>>
>
> Ok, I am not sure if that's entirely true because the MHU spec says it
> drives
> the signal using a 32-bit register, with all 32 bits logically ORed
> together.
> Usually only Rx signals are wired to interrupts and Tx needs to be polled
> but that's entirely implementation choice I believe.
>
On my platform, _STAT register only carries the command code but
actual data is exchanged via SharedMemory/SHM. Now we need to know
when the sent data packet (in SHM) has been consumed by the remote -
for which our protocol mandates the remote clear the TX_STAT register.
And vice-versa for RX.

 Some other f/w may choose some other mechanism for TX-Done - say some
ACK bit set or even some time bound guarantee.

> More over if it's protocol level agreement it should not belong here :)
>
My f/w expects the RX_STAT cleared after reading data from SHM. Where
do you think is the right place to clear RX_STAT?

I have said many times in many threads... its the mailbox controller
_and_ the remote f/w that should be seen as one entity. It may not be
possible to write a controller driver that works with any remote
firmware.

>
>>>> +static int mhu_startup(struct mbox_chan *chan)
>>>> +{
>>>> +       struct mhu_link *mlink = (struct mhu_link *)chan->con_priv;
>>>> +       unsigned long flags;
>>>> +       u32 val;
>>>> +       int ret;
>>>> +
>>>> +       pr_debug("%s:%d\n", __func__, __LINE__);
>>>> +       spin_lock_irqsave(&mlink->lock, flags);
>>>> +       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>>>> +       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
>>>> +       spin_unlock_irqrestore(&mlink->lock, flags);
>>>> +
>>>> +       ret = request_irq(mlink->irq, mhu_rx_interrupt,
>>>> +                         IRQF_SHARED, "mhu_link", chan);
>>>
>>>
>>>
>>> Just a thought: Can this be threaded_irq instead ?
>>> Can move request_irq to probe instead esp. if threaded_irq ?
>>> That provides some flexibility to client's rx_callback.
>>>
>> This is controller driver, and can not know which client want
>> rx_callback in hard-irq context and which in thread_fn context.
>> Otherwise, request_irq() does evaluate to request_threaded_irq(), if
>> thats what you mean.
>
> Agreed but on contrary since MHU involves external firmware(running
> on different processor) which might have it's own latency, I prefer
> threaded over hard irq. And yes request_irq does evaluate to
> request_threaded_irq but with thread_fn = NULL which is not what I want.
>
If remote has its own latency, that does not mean we add some more :)

>>   There might be use-cases like (diagnostic or other) data transfer
>> over mailbox where we don't wanna increase latency, so we have to
>> provide a rx_callback in hard-irq context.
>>
> OK
>
Cool.

>
>>>
>>>> +       for (i = 0; i < 3; i++) {
>>>> +               mhu->chan[i].con_priv = &mhu->mlink[i];
>>>> +               spin_lock_init(&mhu->mlink[i].lock);
>>>> +               res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>>>> +               mhu->mlink[i].irq = res->start;
>>>> +               mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
>>>> +               mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + 0x100;
>>>> +       }
>>>> +
>>>> +       mhu->mbox.dev = &pdev->dev;
>>>> +       mhu->mbox.chans = &mhu->chan[0];
>>>> +       mhu->mbox.num_chans = 3;
>>>
>>>
>>>
>>> Change this to 2, we shouldn't expose secular channel here as Linux can't
>>> access that anyway.
>>>
>> On the contrary, I think the device driver code should be able to
>> manage every resource - secure or non-secure. If we remove secure
>> channel option, what do we do on some other platform that needs it?
>> And Linux may not always run in non-secure mode.
>
>
> In general secure accesses are avoided these days in Linux as we have no
> way to identify it. I agree there are few place where we do access.
> Even though I don't like you have secure channel access in Linux, you
> have valid reasons. In case you decide to support it, there is some
> restrictions in bit 31, though RAZ/WI you need to notify that to protocol
> if it tries to access it ?
>
>
>>   So here we populate all channels and let the clients knows which
>> channel to use via platform specific details - DT. Please note the
>> driver doesn't touch any secure resource if client doesn't ask it to
>> (except SCFG for now, which I think should have some S vs NS DT
>> binding).
>>
>
> Not sure of this though. We need feedback from DT maintainers.
>
Yup.


Cheers,
Jassi