Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

From: Stephen Warren
Date: Wed Aug 27 2014 - 13:51:00 EST

On 08/27/2014 11:38 AM, Andrew Bresticker wrote:
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
On 08/18/2014 11:08 AM, Andrew Bresticker wrote:

The Tegra xHCI controller's firmware communicates requests to the host
processor through a mailbox interface. While there is only a single
communication channel, messages sent by the controller can be divided
into two groups: those intended for the PHY driver and those intended
for the host-controller driver. This mailbox driver exposes the two
channels and routes incoming messages to the appropriate channel based
on the command encoded in the message.


diff --git a/drivers/mailbox/tegra-xusb-mailbox.c
b/drivers/mailbox/tegra-xusb-mailbox.c

+static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox,
u32 cmd)
+{
+ switch (cmd) {
+ case MBOX_CMD_INC_FALC_CLOCK:
+ case MBOX_CMD_DEC_FALC_CLOCK:
+ case MBOX_CMD_INC_SSPI_CLOCK:
+ case MBOX_CMD_DEC_SSPI_CLOCK:
+ case MBOX_CMD_SET_BW:
+ return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST];
+ case MBOX_CMD_SAVE_DFE_CTLE_CTX:
+ case MBOX_CMD_START_HSIC_IDLE:
+ case MBOX_CMD_STOP_HSIC_IDLE:
+ return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY];
+ default:
+ return NULL;
+ }
+}


This makes me think that the CHAN_HOST/CHAN_PHY values are purely a facet of
the Linux driver's message de-multiplexing, rather than anything to do with
the HW.

Yup, they are...

I'm not even sure if it's appropriate for the low-level mailbox driver to
know about the semantics of the message, rather than simply sending them on
to the client driver? Perhaps when drivers register(?) for callbacks(?) for
messages, they should state which types of messages they want to listen to?

So there's not really a way for the client driver to tell the mailbox
driver which types of messages it wants to listen to on a particular
channel with the mailbox framework - it simply provides a way for
clients to bind with channels. I think there are a couple of options
here, either: a) have a channel per message (as you mentioned in the
previous patch), which allows the client to only register for messages
(channels) it wants to handle, or b) extend the mailbox framework to
allow shared channels so that both clients can receive messages on the
single channel and handle messages appropriately. The disadvantage
of (a) is that the commands are firmware defined and could
theoretically change between releases of the firmware, though I'm not
sure how common that is in practice. So that leaves (b) - Jassi, what
do you think about having shared (non-exclusive) channels?

Another alternative might be for each client driver to hard-code a unique dummy channel ID so that each client still gets a separate exclusive channel, but then have the mbox driver broadcast each message to each of those channels. I'm not sure that would be any better though; adding (b) as an explicit option to the mbox subsystem would almost certainly be cleaner.

+static int tegra_xusb_mbox_probe(struct platform_device *pdev)


+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

+ if (!res)
+ return -ENODEV;


Should devm_request_mem_region() be called here to claim the region?

No, the xHCI host driver also needs to map these registers, so they
cannot be mapped exclusively here.

That's unfortunate. Having multiple drivers with overlapping register regions is not a good idea. Can we instead have a top-level driver map all the IO regions, then instantiate the various different sub-components internally, and divide up the address space. Probably via MFD or similar. That would prevent multiple drivers from touching the same register region.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/