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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

+#define XUSB_CFG_ARU_MBOX_CMD			0xe4
+#define  MBOX_FALC_INT_EN			BIT(27)
+#define  MBOX_PME_INT_EN			BIT(28)
+#define  MBOX_SMI_INT_EN			BIT(29)
+#define  MBOX_XHCI_INT_EN			BIT(30)
+#define  MBOX_INT_EN				BIT(31)

Those field names don't match the documentation in the TRM; they're called DEST_xxx rather than xxx_INT_EN. I'm not sure what that disconnect means (i.e. whether it's just a different naming choice, or there's some practical disconnect that will cause issues.)

+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.

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?

+static irqreturn_t tegra_xusb_mbox_irq(int irq, void *p)

+	/* Clear mbox interrupts */
+	reg = mbox_readl(mbox, XUSB_CFG_ARU_SMI_INTR);
+	if (reg & MBOX_SMI_INTR_FW_HANG)
+		dev_err(mbox->mbox.dev, "Controller firmware hang\n");
+	mbox_writel(mbox, reg, XUSB_CFG_ARU_SMI_INTR);

+	/*
+	 * Set the mailbox back to idle.  The recipient of the message is
+	 * responsible for sending an ACK/NAK, if necessary.
+	 */
+	reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD);
+	reg &= ~MBOX_SMI_INT_EN;
+	mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD);

Does the protocol not allow the remote firmware to send another message until the host has ack'd/nak'd the message; the code above turns off the IRQ that indicated to the host that a message was sent to it...

+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?

+	mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start,
+					  resource_size(res));
+	if (!mbox->regs)
+		return -ENOMEM;

Is _nocache required? I don't see other drivers using it. I assume there's nothing special about the mbox registers.

+	mbox->irq = platform_get_irq(pdev, 0);
+	if (mbox->irq < 0)
+		return mbox->irq;
+	ret = devm_request_irq(&pdev->dev, mbox->irq, tegra_xusb_mbox_irq, 0,
+			       dev_name(&pdev->dev), mbox);

Is it possible for an IRQ to occur after tegra_xusb_mbox_remove() has returned, but before the cleanup for the devm IRQ allocation occurs? If that happens, will the code handle it gracefully, or crash?

+MODULE_ALIAS("platform:tegra-xusb-mailbox");

I don't think that's required; it should auto-load based on the of_device_id/MODULE_DEVICE_TABLE(of,...) table.

diff --git a/include/soc/tegra/xusb.h b/include/soc/tegra/xusb.h

+#define TEGRA_XUSB_MBOX_NUM_CHANS 2 /* host + phy */

I'd rather see that definition in the same place as the TEGRA_XUSB_MBOX_CHAN_* values it refers to. Otherwise, it'd be quite easy to add values without updating this constant.
--
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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux