Re: [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes
From: Mikko Perttunen
Date: Tue Jun 19 2018 - 08:52:32 EST
On 22.05.2018 18:36, Jon Hunter wrote:
On 08/05/18 12:43, Mikko Perttunen wrote:
The HSP driver is currently in many places written with the assumption
of only supporting doorbells. Prepare for the addition of shared
mailbox support by removing these assumptions and cleaning up the code.
Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
---
 drivers/mailbox/tegra-hsp.c | 124
+++++++++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 42 deletions(-)
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 0cde356c11ab..16eb970f2c9f 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ * Copyright (c) 2016-2018, NVIDIA CORPORATION. All rights reserved.
ÂÂ *
ÂÂ * This program is free software; you can redistribute it and/or
modify it
ÂÂ * under the terms and conditions of the GNU General Public License,
@@ -42,6 +42,7 @@ struct tegra_hsp_channel;
 struct tegra_hsp;
 struct tegra_hsp_channel {
+ÂÂÂ unsigned int type;
ÂÂÂÂÂ struct tegra_hsp *hsp;
ÂÂÂÂÂ struct mbox_chan *chan;
ÂÂÂÂÂ void __iomem *regs;
@@ -55,6 +56,12 @@ struct tegra_hsp_doorbell {
ÂÂÂÂÂ unsigned int index;
 };
+static inline struct tegra_hsp_doorbell *
+channel_to_doorbell(struct tegra_hsp_channel *channel)
+{
+ÂÂÂ return container_of(channel, struct tegra_hsp_doorbell, channel);
+}
+
 struct tegra_hsp_db_map {
ÂÂÂÂÂ const char *name;
ÂÂÂÂÂ unsigned int master;
@@ -69,7 +76,7 @@ struct tegra_hsp {
ÂÂÂÂÂ const struct tegra_hsp_soc *soc;
ÂÂÂÂÂ struct mbox_controller mbox;
ÂÂÂÂÂ void __iomem *regs;
-ÂÂÂ unsigned int irq;
+ÂÂÂ unsigned int doorbell_irq;
ÂÂÂÂÂ unsigned int num_sm;
ÂÂÂÂÂ unsigned int num_as;
ÂÂÂÂÂ unsigned int num_ss;
@@ -194,7 +201,7 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp,
const char *name,
ÂÂÂÂÂ if (!db)
ÂÂÂÂÂÂÂÂÂ return ERR_PTR(-ENOMEM);
-ÂÂÂ offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) << 16;
+ÂÂÂ offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) *
SZ_64K;
ÂÂÂÂÂ offset += index * 0x100;
ÂÂÂÂÂ db->channel.regs = hsp->regs + offset;
@@ -218,18 +225,8 @@ static void __tegra_hsp_doorbell_destroy(struct
tegra_hsp_doorbell *db)
ÂÂÂÂÂ kfree(db);
 }
-static int tegra_hsp_doorbell_send_data(struct mbox_chan *chan, void
*data)
-{
-ÂÂÂ struct tegra_hsp_doorbell *db = chan->con_priv;
-
-ÂÂÂ tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
-
-ÂÂÂ return 0;
-}
-
-static int tegra_hsp_doorbell_startup(struct mbox_chan *chan)
+static int tegra_hsp_doorbell_startup(struct tegra_hsp_doorbell *db)
 {
-ÂÂÂ struct tegra_hsp_doorbell *db = chan->con_priv;
ÂÂÂÂÂ struct tegra_hsp *hsp = db->channel.hsp;
ÂÂÂÂÂ struct tegra_hsp_doorbell *ccplex;
ÂÂÂÂÂ unsigned long flags;
@@ -260,9 +257,8 @@ static int tegra_hsp_doorbell_startup(struct
mbox_chan *chan)
ÂÂÂÂÂ return 0;
 }
-static void tegra_hsp_doorbell_shutdown(struct mbox_chan *chan)
+static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_doorbell *db)
 {
-ÂÂÂ struct tegra_hsp_doorbell *db = chan->con_priv;
ÂÂÂÂÂ struct tegra_hsp *hsp = db->channel.hsp;
ÂÂÂÂÂ struct tegra_hsp_doorbell *ccplex;
ÂÂÂÂÂ unsigned long flags;
@@ -281,35 +277,61 @@ static void tegra_hsp_doorbell_shutdown(struct
mbox_chan *chan)
ÂÂÂÂÂ spin_unlock_irqrestore(&hsp->lock, flags);
 }
-static const struct mbox_chan_ops tegra_hsp_doorbell_ops = {
-ÂÂÂ .send_data = tegra_hsp_doorbell_send_data,
-ÂÂÂ .startup = tegra_hsp_doorbell_startup,
-ÂÂÂ .shutdown = tegra_hsp_doorbell_shutdown,
+static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
+{
+ÂÂÂ struct tegra_hsp_channel *channel = chan->con_priv;
+ÂÂÂ struct tegra_hsp_doorbell *db;
+
+ÂÂÂ switch (channel->type) {
+ÂÂÂ case TEGRA_HSP_MBOX_TYPE_DB:
+ÂÂÂÂÂÂÂ db = channel_to_doorbell(channel);
+ÂÂÂÂÂÂÂ tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
The above appears to be redundant. We go from channel to db and then end
up passing channels. Do we really need the 'db' struct above?
Fixed.
+ÂÂÂ }
+
+ÂÂÂ return -EINVAL;
Does this function always return -EINVAL?
Fixed.
+}
+
+static int tegra_hsp_startup(struct mbox_chan *chan)
+{
+ÂÂÂ struct tegra_hsp_channel *channel = chan->con_priv;
+
+ÂÂÂ switch (channel->type) {
+ÂÂÂ case TEGRA_HSP_MBOX_TYPE_DB:
+ÂÂÂÂÂÂÂ return tegra_hsp_doorbell_startup(channel_to_doorbell(channel));
+ÂÂÂ }
+
+ÂÂÂ return -EINVAL;
+}
+
+static void tegra_hsp_shutdown(struct mbox_chan *chan)
+{
+ÂÂÂ struct tegra_hsp_channel *channel = chan->con_priv;
+
+ÂÂÂ switch (channel->type) {
+ÂÂÂ case TEGRA_HSP_MBOX_TYPE_DB:
+ÂÂÂÂÂÂÂ tegra_hsp_doorbell_shutdown(channel_to_doorbell(channel));
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+}
+
+static const struct mbox_chan_ops tegra_hsp_ops = {
+ÂÂÂ .send_data = tegra_hsp_send_data,
+ÂÂÂ .startup = tegra_hsp_startup,
+ÂÂÂ .shutdown = tegra_hsp_shutdown,
 };
-static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller
*mbox,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const struct of_phandle_args *args)
+static struct mbox_chan *tegra_hsp_doorbell_xlate(struct tegra_hsp *hsp,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int master)
 {
ÂÂÂÂÂ struct tegra_hsp_channel *channel = ERR_PTR(-ENODEV);
-ÂÂÂ struct tegra_hsp *hsp = to_tegra_hsp(mbox);
-ÂÂÂ unsigned int type = args->args[0];
-ÂÂÂ unsigned int master = args->args[1];
ÂÂÂÂÂ struct tegra_hsp_doorbell *db;
ÂÂÂÂÂ struct mbox_chan *chan;
ÂÂÂÂÂ unsigned long flags;
ÂÂÂÂÂ unsigned int i;
-ÂÂÂ switch (type) {
-ÂÂÂ case TEGRA_HSP_MBOX_TYPE_DB:
-ÂÂÂÂÂÂÂ db = tegra_hsp_doorbell_get(hsp, master);
-ÂÂÂÂÂÂÂ if (db)
-ÂÂÂÂÂÂÂÂÂÂÂ channel = &db->channel;
-
-ÂÂÂÂÂÂÂ break;
-
-ÂÂÂ default:
-ÂÂÂÂÂÂÂ break;
-ÂÂÂ }
+ÂÂÂ db = tegra_hsp_doorbell_get(hsp, master);
+ÂÂÂ if (db)
+ÂÂÂÂÂÂÂ channel = &db->channel;
ÂÂÂÂÂ if (IS_ERR(channel))
ÂÂÂÂÂÂÂÂÂ return ERR_CAST(channel);
@@ -321,6 +343,7 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct
mbox_controller *mbox,
ÂÂÂÂÂÂÂÂÂ if (!chan->con_priv) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ chan->con_priv = channel;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ channel->chan = chan;
+ÂÂÂÂÂÂÂÂÂÂÂ channel->type = TEGRA_HSP_MBOX_TYPE_DB;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
I see that you are making the above only used for doorbells, but don't
we still need to set the chan->con_priv for shared mailboxes as well?
That's done elsewhere in the next patch that actually adds the shared
mailbox support.
Cheers
Jon
Thanks,
Mikko