From: sudeep.holla@arm•com (Sudeep Holla)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 7/8] mailbox: f_mhu: add driver for Fujitsu MHU controller
Date: Thu, 17 Jul 2014 19:51:42 +0100 [thread overview]
Message-ID: <53C81B3E.2020503@arm.com> (raw)
In-Reply-To: <CAJe_ZhcROjhXebJetyJus43jNKoBU60jXXRLwnHwMdPJ7LPq7Q@mail.gmail.com>
On 17/07/14 18:07, Jassi Brar wrote:
> On 17 July 2014 20:39, Sudeep Holla <sudeep.holla@arm•com> wrote:
>>
>>
>> On 17/07/14 13:56, Jassi Brar wrote:
>>>
>>> On 17 July 2014 16:01, Sudeep Holla <sudeep.holla@arm•com> wrote:
>>>>
[...]
>>>>>> 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.
>
Agreed, and I too have similar experience. This is exact reason why I am
urging for threaded irq, unless we have real requirement for hard irq.
>> 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?
I have not yet tried yet. For sure secure access will explode.
> If no, may I see your driver and the MHU manual (mine is 90% Japanese)?
>
It's quite similar to your one expect few comments which I have made.
Here is the public version of Juno spec[1]. Not sure if it covers MHU in
detail.
>>
>>>>
>>>>>>> +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.
Yes I am not arguing on that, it should assume atomic and not sleep.
I am saying we can avoid rx_callback in interrupt context if possible.
I will try to look at the protocol implementation tomorrow.
> Again, it maybe more efficient if I see your implementation of the
> driver and understand your concerns about mine.
>
As I said its almost same as this, except I call mbox_chan_received_data
in irq thread context. I prefer enabling other interrupts while copying
payload data.
>> 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.
>
No I didn't intend for any mailbox API to account it.
Regards,
Sudeep
[1] http://infocenter.arm.com/help/topic/com.arm.doc.dto0038a/index.html
next prev parent reply other threads:[~2014-07-17 18:51 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <message-id-of-cover-letter>
2014-07-13 6:28 ` [PATCH 1/8] ARM: Add platform support for Fujitsu MB86S7X SoCs Mollie Wu
2014-07-14 13:33 ` Arnd Bergmann
2014-07-15 17:37 ` Jassi Brar
2014-07-15 20:09 ` Arnd Bergmann
2014-07-17 13:32 ` Jassi Brar
2014-07-17 13:48 ` Arnd Bergmann
2014-07-17 16:54 ` Jassi Brar
2014-07-17 17:12 ` Arnd Bergmann
2014-07-15 15:11 ` Rob Herring
2014-07-15 16:11 ` Nicolas Pitre
2014-07-15 18:03 ` Jassi Brar
2014-07-16 5:52 ` Andy Green
2014-07-15 17:05 ` Nicolas Pitre
2014-07-15 18:16 ` Jassi Brar
2014-07-13 6:29 ` [PATCH 2/8] mmc: sdhci: host: add new f_sdh30 Mollie Wu
2014-07-14 14:04 ` Arnd Bergmann
2014-07-16 9:35 ` Vincent.Yang
2014-07-16 10:10 ` Arnd Bergmann
2014-07-16 11:07 ` Vincent.Yang
2014-07-13 6:30 ` [PATCH 3/8] mmc: core: add manual resume capability Mollie Wu
2014-07-13 6:30 ` [PATCH 4/8] clk: Add clock driver for mb86s7x Mollie Wu
2014-07-14 14:08 ` Arnd Bergmann
2014-07-16 7:09 ` Jassi Brar
2014-07-13 6:31 ` [PATCH 5/8] pinctrl: add driver for MB86S7x Mollie Wu
2014-07-22 16:11 ` Linus Walleij
2014-07-24 18:04 ` Jassi Brar
2014-08-08 12:42 ` Linus Walleij
2014-08-22 7:46 ` Jassi Brar
2014-08-27 16:58 ` Jassi Brar
2014-09-03 9:17 ` Linus Walleij
2014-07-13 6:31 ` [PATCH 6/8] net: ethernet driver: Fujitsu OGMA Mollie Wu
2014-07-14 9:06 ` Tobias Klauser
2014-07-14 10:36 ` Andy Green
2014-07-14 13:50 ` Arnd Bergmann
2014-07-13 6:32 ` [PATCH 7/8] mailbox: f_mhu: add driver for Fujitsu MHU controller Mollie Wu
2014-07-16 17:37 ` Sudeep Holla
2014-07-17 6:25 ` Jassi Brar
2014-07-17 10:31 ` Sudeep Holla
2014-07-17 12:56 ` Jassi Brar
2014-07-17 15:09 ` Sudeep Holla
2014-07-17 17:07 ` Jassi Brar
2014-07-17 18:51 ` Sudeep Holla [this message]
2014-07-18 9:06 ` Jassi Brar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53C81B3E.2020503@arm.com \
--to=sudeep.holla@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox