From: Lee Jones <lee.jones@linaro.org>
To: Jassi Brar <jassisinghbrar@gmail.com>
Cc: "linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
kernel@stlinux.com, Devicetree List <devicetree@vger.kernel.org>
Subject: Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP
Date: Fri, 14 Aug 2015 11:41:23 +0100 [thread overview]
Message-ID: <20150814104123.GL8782@x1> (raw)
In-Reply-To: <CABb+yY07A=jWvDgbPMpDdT=WPhwSzLumMGiJSB4NddTA4z7Hww@mail.gmail.com>
On Fri, 14 Aug 2015, Jassi Brar wrote:
> On Fri, Aug 14, 2015 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>
> >> > +
> >> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> >> > +{
> >> > + struct sti_channel *chan_info = chan->con_priv;
> >> > + struct sti_mbox_device *mdev = chan_info->mdev;
> >> > + unsigned int instance = chan_info->instance;
> >> > + unsigned int channel = chan_info->channel;
> >> > + void __iomem *base = MBOX_BASE(mdev, instance);
> >> > +
> >> > + if (!(chan_info->direction & MBOX_TX))
> >> > + return false;
> >> >
> >> Here the 'direction' is gotten via DT node of the client i.e, you
> >> expect consumer drivers to tell the provider what its limitations are?
> >>
> >> IMO if some physical channel can't do TX then that should be either
> >> hardcoded inside the controller driver or learnt via DT node of the
> >> _controller_.
> >
> > That's a fair point.
> .
>
> > I need to create a new property similar to the
> > already existing 'read-only'. I guess 'tx-only' is equivalent.
> >
> Just to be clear, if you must have such a property it should come from
> the _controller_ node.
Correct. That's the plan.
> However at one point you said, "Only the A9 (Mbox 0) can Rx."
> Which sounds like the 'simplex' constraint is not coming from the
> mailbox controller but from the remote endpoints that don't RX+TX
> except for one of them. That makes more sense than a controller with
> differently capable physical channels. If that is indeed the
> situation, then the controller is actually 'duplex' and there should
> be no tx-only/rx-only property anywhere. Everything automatically
> falls into place because client drivers are written for specific
> targets and, unless you write some code, there can be no TX call to a
> remote that doesn't listen.
Unfortunately it's a restriction of the hardware (or the controller as
you call it, although it's not really a controller). There is only
one IRQ for Rx'ing and that's wired up to the A9's mailbox (Mailbox
0). If one of the remote processors attempted to send a message
through any of the other mailboxes (other than the Mailbox 0), then no
one would hear the doorbell ring and the message would go unserviced.
> >> > +
> >> > + for (i = 0; i < mbox->num_chans; i++) {
> >> > + chan_info = mbox->chans[i].con_priv;
> >> > +
> >> > + /* Is requested channel free? */
> >> > + if (direction != MBOX_LOOPBACK &&
> >> >
> >> Consider this example when 2 clients ask for same physical channel but
> >> in different modes.
> >> mboxes = <&mboxA 0 1 MBOX_TX>;
> >> mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
> >>
> >> You happily assign 2 virtual channels backed by one physical channel
> >> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
> >> shutdown() and send_data() on the channels. But obviously we are
> >> screwed with races like
> >> client1.startup()
> >> -> client2.startup()
> >> -> client2.send_data()
> >> -> client2.shutdown()
> >> -> client1.send_data() XXXX
> >
> > Good catch and a fair point. As you say, it's unlikely to happen, but
> > I would like to prevent it in any case.
> >
> No, such races are a practical problem. We must own them.
>
> I was talking about problems that arise because someone wrote bad
> DT... those are not 'practical' problems because there are too many
> ways to screw up with bad DT properties that if we try to check for
> them we'll go insane.
The only thing I'm having trouble with protecting at the moment is
other clients _also_ requesting a LOOPBACK channel. I would like to
check which clients have already requested one/them, however that
information is not available until _after_ xlate() has been called,
which is pretty frustrating. Perhaps I'll put a comment in instead.
> >> Now you can shove in some more checks to 'fix' the race OR you can
> >> simply expose only physical channels.
> >
> > We can't expose all of the channels. There are too many and would
> > take up too much *unused* memory.
> >
> I am aware of that. I said expose _only_ physical channels, not _all_ :)
It's impossible to know which physical channels will be used by
clients and subsequently which physical channels to expose.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2015-08-14 10:41 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-27 9:44 [PATCH v2 0/6] Mailbox: Provide support STi based platforms Lee Jones
2015-07-27 9:44 ` [PATCH v2 1/6] mailbox: dt: Supply bindings for ST's Mailbox IP Lee Jones
2015-07-27 9:44 ` [PATCH v2 2/6] mailbox: dt-bindings: Add shared [driver <=> device tree] defines Lee Jones
2015-08-10 6:58 ` Jassi Brar
2015-08-10 8:24 ` Lee Jones
2015-08-10 8:47 ` Jassi Brar
2015-08-12 8:53 ` Lee Jones
2015-08-12 10:16 ` Jassi Brar
2015-08-12 10:43 ` Lee Jones
2015-07-27 9:44 ` [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP Lee Jones
2015-07-28 22:06 ` Paul Bolle
2015-07-30 11:45 ` Lee Jones
2015-07-30 12:48 ` Paul Bolle
2015-07-30 13:31 ` Lee Jones
2015-08-13 15:40 ` Jassi Brar
2015-08-14 6:33 ` Lee Jones
2015-08-14 7:39 ` Jassi Brar
2015-08-14 10:41 ` Lee Jones [this message]
2015-07-27 9:44 ` [PATCH v2 4/6] ARM: STi: stih407-family: Add nodes for Mailbox Lee Jones
2015-07-27 9:44 ` [PATCH v2 5/6] mailbox: Add generic mechanism for testing Mailbox Controllers Lee Jones
2015-08-10 6:45 ` Jassi Brar
2015-08-12 10:23 ` Lee Jones
2015-08-13 8:51 ` Jassi Brar
2015-08-13 9:19 ` Lee Jones
2015-08-13 10:01 ` Jassi Brar
2015-08-13 10:23 ` Lee Jones
2015-08-13 10:41 ` Jassi Brar
2015-08-13 11:00 ` Lee Jones
2015-08-13 11:10 ` Jassi Brar
2015-08-13 11:40 ` Lee Jones
2015-08-13 12:47 ` Jassi Brar
2015-08-13 13:07 ` Lee Jones
2015-08-13 13:46 ` Jassi Brar
2015-08-13 14:26 ` Lee Jones
2015-07-27 9:44 ` [PATCH v2 6/6] ARM: STi: DT: STiH407: Enable Mailbox testing facility Lee Jones
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=20150814104123.GL8782@x1 \
--to=lee.jones@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=jassisinghbrar@gmail.com \
--cc=kernel@stlinux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).