On 17 July 2014 20:39, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
>
> On 17/07/14 13:56, Jassi Brar wrote:
>>
>> On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@xxxxxxx> 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 understand that and what I am saying is the MHU is designed like that
> and protocol is just using it. There's nothing specific to protocol. Ideally
> if an implementation has both Rx and Tx interrupts, the RX_CLEAR from here
> raises an interrupt to the firmware. In absence of it we need polling that's
> what both Linux and firmware does for Tx case.
>
> Even on Juno, it's same. But we should be able to extend it to support Tx
> if an implementation supports. Similarly the firmware can make use of the
> same when Linux clears Rx(it would be Tx complete/ack for the firmware)
>
> When we need to make this driver work across different firmware, you just
> can't rely on the firmware protocol, hence I am asking to drop those
> comments.
>
OK, I will remove the comment.
>
>> 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.
>>
>
> It should be possible if the remote protocol just use the same hardware
> feature without any extra software policy at the lowest level(raw Tx and
> Rx).
>
I wouldn't count on f/w always done the right way. And I am speaking
from my whatever first hand experience :D
And sometimes there may just be bugs that need some quirks at controller level.
> I believe that's what we need here if we want this driver to work
> on both Juno and your platform. Agree ?
>
Does this driver not work for Juno?
If no, may I see your driver and the MHU manual (mine is 90% Japanese)?
>
>>>
>>>>>> +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 :)
>>
>
> No what I meant is unless there is a real need to use hard irq, we
> should prefer threaded one otherwise.
>
And how does controller discern a "real need" from a "soft need" to
use hard irq?
Even if the controller driver pushes data up from a threaded function,
the client can't know the context and can't sleep because the
intermediate API says the rx_callback should be assumed to be atomic.
Again, it maybe more efficient if I see your implementation of the
driver and understand your concerns about mine.
> Also by latency I meant what if
> the remote firmware misbehaves. In threaded version you have little
> more flexibility whereas hard irq is executed with interrupts disabled.
> At least the system is responsive and only MHU interrupts are disabled.
>
If the remote firmware misbehaves, that is a bug of the platform. No
mailbox API could/should account for such malfunctions.
Thanks
Jassi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
