From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933544AbcKPF2L (ORCPT ); Wed, 16 Nov 2016 00:28:11 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33144 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750806AbcKPF2J (ORCPT ); Wed, 16 Nov 2016 00:28:09 -0500 MIME-Version: 1.0 In-Reply-To: <20161115154845.24803-3-thierry.reding@gmail.com> References: <20161115154845.24803-1-thierry.reding@gmail.com> <20161115154845.24803-3-thierry.reding@gmail.com> From: Jassi Brar Date: Wed, 16 Nov 2016 10:58:07 +0530 Message-ID: Subject: Re: [PATCH v4 2/2] mailbox: Add Tegra HSP driver To: Thierry Reding Cc: Jon Hunter , "linux-tegra@vger.kernel.org" , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 15, 2016 at 9:18 PM, Thierry Reding wrote: .... > + > +struct tegra_hsp_channel; > +struct tegra_hsp; > + > +struct tegra_hsp_channel_ops { > + int (*send_data)(struct tegra_hsp_channel *channel, void *data); > + int (*startup)(struct tegra_hsp_channel *channel); > + void (*shutdown)(struct tegra_hsp_channel *channel); > + bool (*last_tx_done)(struct tegra_hsp_channel *channel); > +}; > + > +struct tegra_hsp_channel { > + struct tegra_hsp *hsp; > + const struct tegra_hsp_channel_ops *ops; > + struct mbox_chan *chan; > + void __iomem *regs; > +}; > + > +static struct tegra_hsp_channel *to_tegra_hsp_channel(struct mbox_chan *chan) > +{ > + return chan->con_priv; > +} > + It seems channel = to_tegra_hsp_channel(chan); is no simpler ritual than channel = chan->con_priv; ? > +struct tegra_hsp_doorbell { > + struct tegra_hsp_channel channel; > + struct list_head list; > + const char *name; > + unsigned int master; > + unsigned int index; > +}; > + > +static struct tegra_hsp_doorbell * > +to_tegra_hsp_doorbell(struct tegra_hsp_channel *channel) > +{ > + if (!channel) > + return NULL; > + > + return container_of(channel, struct tegra_hsp_doorbell, channel); > +} > + But you don't check for NULL returned, before dereferencing the pointer 'db' > +struct tegra_hsp_db_map { > + const char *name; > + unsigned int master; > + unsigned int index; > +}; > + > +struct tegra_hsp_soc { > + const struct tegra_hsp_db_map *map; > +}; > + > +struct tegra_hsp { > + const struct tegra_hsp_soc *soc; > + struct mbox_controller mbox; > + void __iomem *regs; > + unsigned int irq; > + unsigned int num_sm; > + unsigned int num_as; > + unsigned int num_ss; > + unsigned int num_db; > + unsigned int num_si; > + spinlock_t lock; > + > + struct list_head doorbells; > +}; > + > +static inline struct tegra_hsp * > +to_tegra_hsp(struct mbox_controller *mbox) > +{ > + return container_of(mbox, struct tegra_hsp, mbox); > +} > + > +static inline u32 tegra_hsp_readl(struct tegra_hsp *hsp, unsigned int offset) > +{ > + return readl(hsp->regs + offset); > +} > + > +static inline void tegra_hsp_writel(struct tegra_hsp *hsp, u32 value, > + unsigned int offset) > +{ > + writel(value, hsp->regs + offset); > +} > + > +static inline u32 tegra_hsp_channel_readl(struct tegra_hsp_channel *channel, > + unsigned int offset) > +{ > + return readl(channel->regs + offset); > +} > + > +static inline void tegra_hsp_channel_writel(struct tegra_hsp_channel *channel, > + u32 value, unsigned int offset) > +{ > + writel(value, channel->regs + offset); > +} > + > +static bool tegra_hsp_doorbell_can_ring(struct tegra_hsp_doorbell *db) > +{ > + u32 value; > + > + value = tegra_hsp_channel_readl(&db->channel, HSP_DB_ENABLE); > + > + return (value & BIT(TEGRA_HSP_DB_MASTER_CCPLEX)) != 0; > +} > + > +static struct tegra_hsp_doorbell * > +__tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master) > +{ > + struct tegra_hsp_doorbell *entry; > + > + list_for_each_entry(entry, &hsp->doorbells, list) > + if (entry->master == master) > + return entry; > + > + return NULL; > +} > + > +static struct tegra_hsp_doorbell * > +tegra_hsp_doorbell_get(struct tegra_hsp *hsp, unsigned int master) > +{ > + struct tegra_hsp_doorbell *db; > + unsigned long flags; > + > + spin_lock_irqsave(&hsp->lock, flags); > + db = __tegra_hsp_doorbell_get(hsp, master); > + spin_unlock_irqrestore(&hsp->lock, flags); > + > + return db; > +} > + ..... > + > +static int tegra_hsp_doorbell_send_data(struct tegra_hsp_channel *channel, > + void *data) > +{ > + tegra_hsp_channel_writel(channel, 1, HSP_DB_TRIGGER); > + > + return 0; > +} > + > +static int tegra_hsp_doorbell_startup(struct tegra_hsp_channel *channel) > +{ > + struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel); > + struct tegra_hsp *hsp = channel->hsp; > + struct tegra_hsp_doorbell *ccplex; > + unsigned long flags; > + u32 value; > + > + if (db->master >= hsp->mbox.num_chans) { > + dev_err(hsp->mbox.dev, > + "invalid master ID %u for HSP channel\n", > + db->master); > + return -EINVAL; > + } > + > + ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX); > + if (!ccplex) > + return -ENODEV; > + > + if (!tegra_hsp_doorbell_can_ring(db)) > + return -ENODEV; > + > + spin_lock_irqsave(&hsp->lock, flags); > + > + value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE); > + value |= BIT(db->master); > + tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE); > + > + spin_unlock_irqrestore(&hsp->lock, flags); > + > + return 0; > +} > + > +static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_channel *channel) > +{ > + struct tegra_hsp_doorbell *db = to_tegra_hsp_doorbell(channel); > + struct tegra_hsp *hsp = channel->hsp; > + struct tegra_hsp_doorbell *ccplex; > + unsigned long flags; > + u32 value; > + > + ccplex = tegra_hsp_doorbell_get(hsp, TEGRA_HSP_DB_MASTER_CCPLEX); > + if (!ccplex) > + return; > + > + spin_lock_irqsave(&hsp->lock, flags); > + > + value = tegra_hsp_channel_readl(&ccplex->channel, HSP_DB_ENABLE); > + value &= ~BIT(db->master); > + tegra_hsp_channel_writel(&ccplex->channel, value, HSP_DB_ENABLE); > + > + spin_unlock_irqrestore(&hsp->lock, flags); > +} > + > +static bool tegra_hsp_doorbell_last_tx_done(struct tegra_hsp_channel *channel) > +{ > + return true; > +} Just curious, is the IPC done instantly after writing HSP_DB_TRIGGER bit? Usually there is at least some bit that stays (un)set as a 'busy flag'. > + > +static const struct tegra_hsp_channel_ops tegra_hsp_doorbell_ops = { > + .send_data = tegra_hsp_doorbell_send_data, > + .startup = tegra_hsp_doorbell_startup, > + .shutdown = tegra_hsp_doorbell_shutdown, > + .last_tx_done = tegra_hsp_doorbell_last_tx_done, > +}; > + .... > +static int tegra_hsp_send_data(struct mbox_chan *chan, void *data) > +{ > + struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan); > + > + return channel->ops->send_data(channel, data); > +} > + > +static int tegra_hsp_startup(struct mbox_chan *chan) > +{ > + struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan); > + > + return channel->ops->startup(channel); > +} > + > +static void tegra_hsp_shutdown(struct mbox_chan *chan) > +{ > + struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan); > + > + return channel->ops->shutdown(channel); > +} > + > +static bool tegra_hsp_last_tx_done(struct mbox_chan *chan) > +{ > + struct tegra_hsp_channel *channel = to_tegra_hsp_channel(chan); > + > + return channel->ops->last_tx_done(channel); > +} > + > +static const struct mbox_chan_ops tegra_hsp_ops = { > + .send_data = tegra_hsp_send_data, > + .startup = tegra_hsp_startup, > + .shutdown = tegra_hsp_shutdown, > + .last_tx_done = tegra_hsp_last_tx_done, > +}; > + These 4 above seem overkill. Why not directly use tegra_hsp_doorbell_xxx() ? Cheers!