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 11:31:13 +0100 [thread overview]
Message-ID: <53C7A5F1.30209@arm.com> (raw)
In-Reply-To: <CAJe_Zhe_kqmeeu9vF3CZwu3WR=P-nj13Bx+VOBCE7NfGzGPx=g@mail.gmail.com>
On 17/07/14 07:25, Jassi Brar wrote:
> On 16 July 2014 23:07, Sudeep Holla <sudeep.holla@arm•com> wrote:
>> Hi Mollie,
>>
>>
>> On 13/07/14 07:32, Mollie Wu wrote:
>>>
>>> Add driver for the proprietary Mailbox controller (f_mhu) in MB86S7x.
[...]
>>
>>> + 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.
>>
>>> + /* 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.
More over if it's protocol level agreement it should not belong here :)
>>> +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.
> 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
>>
>>> + 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.
Regards,
Sudeep
next prev parent reply other threads:[~2014-07-17 10:31 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 [this message]
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
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=53C7A5F1.30209@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