Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Alexandre Courbot]

Sorry, I will probably need to do several passes on this one to
understand everything, but here is what I can say after a first look:

On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@xxxxxxxxxx> wrote:
> The Tegra BPMP (Boot and Power Management Processor) is designed for the
> booting process handling, offloading the power management tasks and
> some system control services from the CPU. It can be clock, DVFS,
> thermal/EDP, power gating operation and system suspend/resume handling.
> So the CPU and the drivers of these modules can base on the service that
> the BPMP firmware driver provided to signal the event for the specific PM
> action to BPMP and receive the status update from BPMP.
>
> Comparing to the ARM SCPI, the service provided by BPMP is message-based
> communication but not method-based. The BPMP firmware driver provides the
> send/receive service for the users, when the user concerns the response
> time. If the user needs to get the event or update from the firmware, it
> can request the MRQ service as well. The user needs to take care of the
> message format, which we call BPMP ABI.
>
> The BPMP ABI defines the message format for different modules or usages.
> For example, the clock operation needs an MRQ service code called
> MRQ_CLK with specific message format which includes different sub
> commands for various clock operations. This is the message format that
> BPMP can recognize.
>
> So the user needs two things to initiate IPC between BPMP. Get the
> service from the bpmp_ops structure and maintain the message format as
> the BPMP ABI defined.
>
> Based-on-the-work-by:
> Sivaram Nair <sivaramn@xxxxxxxxxx>
>
> Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
> ---
> Changes in V2:
> - None
> ---
>  drivers/firmware/tegra/Kconfig  |   12 +
>  drivers/firmware/tegra/Makefile |    1 +
>  drivers/firmware/tegra/bpmp.c   |  713 +++++++++++++++++
>  include/soc/tegra/bpmp.h        |   29 +
>  include/soc/tegra/bpmp_abi.h    | 1601 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 2356 insertions(+)
>  create mode 100644 drivers/firmware/tegra/bpmp.c
>  create mode 100644 include/soc/tegra/bpmp.h
>  create mode 100644 include/soc/tegra/bpmp_abi.h
>
> diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
> index 1fa3e4e136a5..ff2730d5c468 100644
> --- a/drivers/firmware/tegra/Kconfig
> +++ b/drivers/firmware/tegra/Kconfig
> @@ -10,4 +10,16 @@ config TEGRA_IVC
>           keeps the content is synchronization between host CPU and remote
>           processors.
>
> +config TEGRA_BPMP
> +       bool "Tegra BPMP driver"
> +       depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
> +       help
> +         BPMP (Boot and Power Management Processor) is designed to off-loading

s/off-loading/off-load

> +         the PM functions which include clock/DVFS/thermal/power from the CPU.
> +         It needs HSP as the HW synchronization and notification module and
> +         IVC module as the message communication protocol.
> +
> +         This driver manages the IPC interface between host CPU and the
> +         firmware running on BPMP.
> +
>  endmenu
> diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
> index 92e2153e8173..e34a2f79e1ad 100644
> --- a/drivers/firmware/tegra/Makefile
> +++ b/drivers/firmware/tegra/Makefile
> @@ -1 +1,2 @@
> +obj-$(CONFIG_TEGRA_BPMP)       += bpmp.o
>  obj-$(CONFIG_TEGRA_IVC)                += ivc.o
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> new file mode 100644
> index 000000000000..24fda626610e
> --- /dev/null
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -0,0 +1,713 @@
> +/*
> + * Copyright (c) 2016, 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,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/mailbox_client.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/semaphore.h>
> +
> +#include <soc/tegra/bpmp.h>
> +#include <soc/tegra/bpmp_abi.h>
> +#include <soc/tegra/ivc.h>
> +
> +#define BPMP_MSG_SZ            128
> +#define BPMP_MSG_DATA_SZ       120
> +
> +#define __MRQ_ATTRS            0xff000000
> +#define __MRQ_INDEX(id)                ((id) & ~__MRQ_ATTRS)
> +
> +#define DO_ACK                 BIT(0)
> +#define RING_DOORBELL          BIT(1)
> +
> +struct tegra_bpmp_soc_data {
> +       u32 ch_index;           /* channel index */
> +       u32 thread_ch_index;    /* thread channel index */
> +       u32 cpu_rx_ch_index;    /* CPU Rx channel index */
> +       u32 nr_ch;              /* number of total channels */
> +       u32 nr_thread_ch;       /* number of thread channels */
> +       u32 ch_timeout;         /* channel timeout */
> +       u32 thread_ch_timeout;  /* thread channel timeout */
> +};

With just these comments it is not clear what everything in this
structure does. Maybe a file-level comment explaining how BPMP
basically works and what the different channels are allocated to would
help understanding the code.

> +
> +struct channel_info {
> +       u32 tch_free;
> +       u32 tch_to_complete;
> +       struct semaphore tch_sem;
> +};
> +
> +struct mb_data {
> +       s32 code;
> +       s32 flags;
> +       u8 data[BPMP_MSG_DATA_SZ];
> +} __packed;
> +
> +struct channel_data {
> +       struct mb_data *ib;
> +       struct mb_data *ob;
> +};
> +
> +struct mrq {
> +       struct list_head list;
> +       u32 mrq_code;
> +       bpmp_mrq_handler handler;
> +       void *data;
> +};
> +
> +struct tegra_bpmp {
> +       struct device *dev;
> +       const struct tegra_bpmp_soc_data *soc_data;
> +       void __iomem *tx_base;
> +       void __iomem *rx_base;
> +       struct mbox_client cl;
> +       struct mbox_chan *chan;
> +       struct ivc *ivc_channels;
> +       struct channel_data *ch_area;
> +       struct channel_info ch_info;
> +       struct completion *ch_completion;
> +       struct list_head mrq_list;
> +       struct tegra_bpmp_ops *ops;
> +       spinlock_t lock;
> +       bool init_done;
> +};
> +
> +static struct tegra_bpmp *bpmp;

static? Ok, we only need one... for now. How about a private member in
your ivc structure that allows you to retrieve the bpmp and going
dynamic? This will require an extra argument in many functions, but is
cleaner design IMHO.

> +
> +static int bpmp_get_thread_ch(int idx)
> +{
> +       return bpmp->soc_data->thread_ch_index + idx;
> +}
> +
> +static int bpmp_get_thread_ch_index(int ch)
> +{
> +       if (ch < bpmp->soc_data->thread_ch_index ||
> +           ch >= bpmp->soc_data->cpu_rx_ch_index)

Shouldn't that be ch >= bpmp->soc_data->cpu_rx_ch_index +
bpmp->soc_data->nr_thread_ch?

Either rx_ch_index indicates the upper bound of the threaded channels,
and in that case you don't need tegra_bpmp_soc_data::nr_thread_ch, or
it can be anywhere else and you should use the correct member.

> +               return -1;
> +       return ch - bpmp->soc_data->thread_ch_index;
> +}
> +
> +static int bpmp_get_ob_channel(void)
> +{
> +       return smp_processor_id() + bpmp->soc_data->ch_index;
> +}
> +
> +static struct completion *bpmp_get_completion_obj(int ch)
> +{
> +       int i = bpmp_get_thread_ch_index(ch);
> +
> +       return i < 0 ? NULL : bpmp->ch_completion + i;
> +}
> +
> +static int bpmp_valid_txfer(void *ob_data, int ob_sz, void *ib_data, int ib_sz)
> +{
> +       return ob_sz >= 0 && ob_sz <= BPMP_MSG_DATA_SZ &&
> +              ib_sz >= 0 && ib_sz <= BPMP_MSG_DATA_SZ &&
> +              (!ob_sz || ob_data) && (!ib_sz || ib_data);
> +}
> +
> +static bool bpmp_master_acked(int ch)
> +{
> +       struct ivc *ivc_chan;
> +       void *frame;
> +       bool ready;
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       frame = tegra_ivc_read_get_next_frame(ivc_chan);
> +       ready = !IS_ERR_OR_NULL(frame);
> +       bpmp->ch_area[ch].ib = ready ? frame : NULL;
> +
> +       return ready;
> +}
> +
> +static int bpmp_wait_ack(int ch)
> +{
> +       ktime_t t;
> +
> +       t = ns_to_ktime(local_clock());
> +
> +       do {
> +               if (bpmp_master_acked(ch))
> +                       return 0;
> +       } while (ktime_us_delta(ns_to_ktime(local_clock()), t) <
> +                bpmp->soc_data->ch_timeout);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +static bool bpmp_master_free(int ch)
> +{
> +       struct ivc *ivc_chan;
> +       void *frame;
> +       bool ready;
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       frame = tegra_ivc_write_get_next_frame(ivc_chan);
> +       ready = !IS_ERR_OR_NULL(frame);
> +       bpmp->ch_area[ch].ob = ready ? frame : NULL;
> +
> +       return ready;
> +}
> +
> +static int bpmp_wait_master_free(int ch)
> +{
> +       ktime_t t;
> +
> +       t = ns_to_ktime(local_clock());
> +
> +       do {
> +               if (bpmp_master_free(ch))
> +                       return 0;
> +       } while (ktime_us_delta(ns_to_ktime(local_clock()), t)
> +                < bpmp->soc_data->ch_timeout);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +static int __read_ch(int ch, void *data, int sz)
> +{
> +       struct ivc *ivc_chan;
> +       struct mb_data *p;
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       p = bpmp->ch_area[ch].ib;
> +       if (data)
> +               memcpy_fromio(data, p->data, sz);
> +
> +       return tegra_ivc_read_advance(ivc_chan);
> +}
> +
> +static int bpmp_read_ch(int ch, void *data, int sz)
> +{
> +       unsigned long flags;
> +       int i, ret;
> +
> +       i = bpmp_get_thread_ch_index(ch);

i is not a very good name for this variable.
Also note that bpmp_get_thread_ch_index() can return -1, this case is
not handled.

> +
> +       spin_lock_irqsave(&bpmp->lock, flags);
> +       ret = __read_ch(ch, data, sz);
> +       bpmp->ch_info.tch_free |= (1 << i);
> +       spin_unlock_irqrestore(&bpmp->lock, flags);
> +
> +       up(&bpmp->ch_info.tch_sem);
> +
> +       return ret;
> +}
> +
> +static int __write_ch(int ch, int mrq_code, int flags, void *data, int sz)
> +{
> +       struct ivc *ivc_chan;
> +       struct mb_data *p;
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       p = bpmp->ch_area[ch].ob;
> +
> +       p->code = mrq_code;
> +       p->flags = flags;
> +       if (data)
> +               memcpy_toio(p->data, data, sz);
> +
> +       return tegra_ivc_write_advance(ivc_chan);
> +}
> +
> +static int bpmp_write_threaded_ch(int *ch, int mrq_code, void *data, int sz)
> +{
> +       unsigned long flags;
> +       int ret, i;
> +
> +       ret = down_timeout(&bpmp->ch_info.tch_sem,
> +                          usecs_to_jiffies(bpmp->soc_data->thread_ch_timeout));
> +       if (ret)
> +               return ret;
> +
> +       spin_lock_irqsave(&bpmp->lock, flags);
> +
> +       i = __ffs(bpmp->ch_info.tch_free);
> +       *ch = bpmp_get_thread_ch(i);
> +       ret = bpmp_master_free(*ch) ? 0 : -EFAULT;
> +       if (!ret) {
> +               bpmp->ch_info.tch_free &= ~(1 << i);
> +               __write_ch(*ch, mrq_code, DO_ACK | RING_DOORBELL, data, sz);
> +               bpmp->ch_info.tch_to_complete |= (1 << *ch);
> +       }
> +
> +       spin_unlock_irqrestore(&bpmp->lock, flags);
> +
> +       return ret;
> +}
> +
> +static int bpmp_write_ch(int ch, int mrq_code, int flags, void *data, int sz)
> +{
> +       int ret;
> +
> +       ret = bpmp_wait_master_free(ch);
> +       if (ret)
> +               return ret;
> +
> +       return __write_ch(ch, mrq_code, flags, data, sz);
> +}
> +
> +static int bpmp_send_receive_atomic(int mrq_code, void *ob_data, int ob_sz,
> +                                   void *ib_data, int ib_sz)
> +{
> +       int ch, ret;
> +
> +       if (WARN_ON(!irqs_disabled()))
> +               return -EPERM;
> +
> +       if (!bpmp_valid_txfer(ob_data, ob_sz, ib_data, ib_sz))
> +               return -EINVAL;
> +
> +       if (!bpmp->init_done)
> +               return -ENODEV;
> +
> +       ch = bpmp_get_ob_channel();
> +       ret = bpmp_write_ch(ch, mrq_code, DO_ACK, ob_data, ob_sz);
> +       if (ret)
> +               return ret;
> +
> +       ret = mbox_send_message(bpmp->chan, NULL);
> +       if (ret < 0)
> +               return ret;
> +       mbox_client_txdone(bpmp->chan, 0);
> +
> +       ret = bpmp_wait_ack(ch);
> +       if (ret)
> +               return ret;
> +
> +       return __read_ch(ch, ib_data, ib_sz);
> +}
> +
> +static int bpmp_send_receive(int mrq_code, void *ob_data, int ob_sz,
> +                            void *ib_data, int ib_sz)
> +{
> +       struct completion *comp_obj;
> +       unsigned long timeout;
> +       int ch, ret;
> +
> +       if (WARN_ON(irqs_disabled()))
> +               return -EPERM;
> +
> +       if (!bpmp_valid_txfer(ob_data, ob_sz, ib_data, ib_sz))
> +               return -EINVAL;
> +
> +       if (!bpmp->init_done)
> +               return -ENODEV;
> +
> +       ret = bpmp_write_threaded_ch(&ch, mrq_code, ob_data, ob_sz);
> +       if (ret)
> +               return ret;
> +
> +       ret = mbox_send_message(bpmp->chan, NULL);
> +       if (ret < 0)
> +               return ret;
> +       mbox_client_txdone(bpmp->chan, 0);
> +
> +       comp_obj = bpmp_get_completion_obj(ch);
> +       timeout = usecs_to_jiffies(bpmp->soc_data->thread_ch_timeout);
> +       if (!wait_for_completion_timeout(comp_obj, timeout))
> +               return -ETIMEDOUT;
> +
> +       return bpmp_read_ch(ch, ib_data, ib_sz);
> +}
> +
> +static struct mrq *bpmp_find_mrq(u32 mrq_code)
> +{
> +       struct mrq *mrq;
> +
> +       list_for_each_entry(mrq, &bpmp->mrq_list, list) {
> +               if (mrq_code == mrq->mrq_code)
> +                       return mrq;
> +       }
> +
> +       return NULL;
> +}
> +
> +static void bpmp_mrq_return_data(int ch, int code, void *data, int sz)
> +{
> +       int flags = bpmp->ch_area[ch].ib->flags;
> +       struct ivc *ivc_chan;
> +       struct mb_data *frame;
> +       int ret;
> +
> +       if (WARN_ON(sz > BPMP_MSG_DATA_SZ))
> +               return;
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       ret = tegra_ivc_read_advance(ivc_chan);
> +       WARN_ON(ret);
> +
> +       if (!(flags & DO_ACK))
> +               return;
> +
> +       frame = tegra_ivc_write_get_next_frame(ivc_chan);
> +       if (IS_ERR_OR_NULL(frame)) {
> +               WARN_ON(1);
> +               return;
> +       }
> +
> +       frame->code = code;
> +       if (data != NULL)
> +               memcpy_toio(frame->data, data, sz);
> +       ret = tegra_ivc_write_advance(ivc_chan);
> +       WARN_ON(ret);
> +
> +       if (flags & RING_DOORBELL) {
> +               ret = mbox_send_message(bpmp->chan, NULL);
> +               if (ret < 0) {
> +                       WARN_ON(1);
> +                       return;
> +               }
> +               mbox_client_txdone(bpmp->chan, 0);
> +       }
> +}
> +
> +static void bpmp_mail_return(int ch, int ret_code, int val)
> +{
> +       bpmp_mrq_return_data(ch, ret_code, &val, sizeof(val));
> +}
> +
> +static void bpmp_handle_mrq(int mrq_code, int ch)
> +{
> +       struct mrq *mrq;
> +
> +       spin_lock(&bpmp->lock);
> +
> +       mrq = bpmp_find_mrq(mrq_code);
> +       if (!mrq) {
> +               spin_unlock(&bpmp->lock);
> +               bpmp_mail_return(ch, -EINVAL, 0);
> +               return;
> +       }
> +
> +       mrq->handler(mrq_code, mrq->data, ch);
> +
> +       spin_unlock(&bpmp->lock);
> +}
> +
> +static int bpmp_request_mrq(int mrq_code, bpmp_mrq_handler handler, void *data)
> +{
> +       struct mrq *mrq;
> +       unsigned long flags;
> +
> +       if (!handler)
> +               return -EINVAL;
> +
> +       mrq = devm_kzalloc(bpmp->dev, sizeof(*mrq), GFP_KERNEL);
> +       if (!mrq)
> +               return -ENOMEM;
> +
> +       spin_lock_irqsave(&bpmp->lock, flags);
> +
> +       mrq->mrq_code = __MRQ_INDEX(mrq_code);
> +       mrq->handler = handler;
> +       mrq->data = data;
> +       list_add(&mrq->list, &bpmp->mrq_list);
> +
> +       spin_unlock_irqrestore(&bpmp->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void bpmp_mrq_handle_ping(int mrq_code, void *data, int ch)
> +{
> +       int challenge;
> +       int reply;
> +
> +       challenge = *(int *)bpmp->ch_area[ch].ib->data;
> +       reply = challenge << (smp_processor_id() + 1);
> +       bpmp_mail_return(ch, 0, reply);
> +}
> +
> +static int bpmp_mailman_init(void)
> +{
> +       return bpmp_request_mrq(MRQ_PING, bpmp_mrq_handle_ping, NULL);
> +}
> +
> +static int bpmp_ping(void)
> +{
> +       unsigned long flags;
> +       ktime_t t;
> +       int challenge = 1;

Mmmm, shouldn't use a mrq_ping_request instead of an parameter which
size may vary depending on the architecture? On a 64-bit big endian
architecture, your messages would be corrupted.

> +       int reply = 0;

And this should probably be a mrq_ping_response. These remarks may
also apply to bpmp_mrq_handle_ping().

> +       int ret;
> +
> +       t = ktime_get();
> +       local_irq_save(flags);
> +       ret = bpmp_send_receive_atomic(MRQ_PING, &challenge, sizeof(challenge),
> +                                      &reply, sizeof(reply));
> +       local_irq_restore(flags);
> +       t = ktime_sub(ktime_get(), t);
> +
> +       if (!ret)
> +               dev_info(bpmp->dev,
> +                        "ping ok: challenge: %d, reply: %d, time: %lld\n",
> +                        challenge, reply, ktime_to_us(t));
> +
> +       return ret;
> +}
> +
> +static int bpmp_get_fwtag(void)
> +{
> +       unsigned long flags;
> +       void *vaddr;
> +       dma_addr_t paddr;
> +       u32 addr;

Here also we should use a mrq_query_tag_request.

> +       int ret;
> +
> +       vaddr = dma_alloc_coherent(bpmp->dev, BPMP_MSG_DATA_SZ, &paddr,
> +                                  GFP_KERNEL);

dma_addr_t may be 64 bit here, and you may get an address higher than
the 32 bits allowed by mrq_query_tag_request! I guess you want to add
GFP_DMA32 as flag to your call to dma_alloc_coherent.

> +       if (!vaddr)
> +               return -ENOMEM;
> +       addr = paddr;
> +
> +       local_irq_save(flags);
> +       ret = bpmp_send_receive_atomic(MRQ_QUERY_TAG, &addr, sizeof(addr),
> +                                      NULL, 0);
> +       local_irq_restore(flags);
> +
> +       if (!ret)
> +               dev_info(bpmp->dev, "fwtag: %s\n", (char *)vaddr);
> +
> +       dma_free_coherent(bpmp->dev, BPMP_MSG_DATA_SZ, vaddr, paddr);
> +
> +       return ret;
> +}
> +
> +static void bpmp_signal_thread(int ch)
> +{
> +       int flags = bpmp->ch_area[ch].ob->flags;
> +       struct completion *comp_obj;
> +
> +       if (!(flags & RING_DOORBELL))
> +               return;
> +
> +       comp_obj = bpmp_get_completion_obj(ch);
> +       if (!comp_obj) {
> +               WARN_ON(1);
> +               return;
> +       }
> +
> +       complete(comp_obj);
> +}
> +
> +static void bpmp_handle_rx(struct mbox_client *cl, void *data)
> +{
> +       int i, rx_ch;
> +
> +       rx_ch = bpmp->soc_data->cpu_rx_ch_index;
> +
> +       if (bpmp_master_acked(rx_ch))
> +               bpmp_handle_mrq(bpmp->ch_area[rx_ch].ib->code, rx_ch);
> +
> +       spin_lock(&bpmp->lock);
> +
> +       for (i = 0; i < bpmp->soc_data->nr_thread_ch &&
> +                       bpmp->ch_info.tch_to_complete; i++) {
> +               int ch = bpmp_get_thread_ch(i);
> +
> +               if ((bpmp->ch_info.tch_to_complete & (1 << ch)) &&
> +                   bpmp_master_acked(ch)) {
> +                       bpmp->ch_info.tch_to_complete &= ~(1 << ch);
> +                       bpmp_signal_thread(ch);
> +               }
> +       }
> +
> +       spin_unlock(&bpmp->lock);
> +}
> +
> +static void bpmp_ivc_notify(struct ivc *ivc)
> +{
> +       int ret;
> +
> +       ret = mbox_send_message(bpmp->chan, NULL);
> +       if (ret < 0)
> +               return;
> +
> +       mbox_send_message(bpmp->chan, NULL);

Why the second call to mbox_send_message? May to useful to add a
comment explaining it.

> +}
> +
> +static int bpmp_msg_chan_init(int ch)
> +{
> +       struct ivc *ivc_chan;
> +       u32 hdr_sz, msg_sz, que_sz;
> +       uintptr_t rx_base, tx_base;
> +       int ret;
> +
> +       msg_sz = tegra_ivc_align(BPMP_MSG_SZ);
> +       hdr_sz = tegra_ivc_total_queue_size(0);

I believe hdr_sz is never used?

> +       que_sz = tegra_ivc_total_queue_size(msg_sz);
> +
> +       rx_base =  (uintptr_t)(bpmp->rx_base + que_sz * ch);
> +       tx_base =  (uintptr_t)(bpmp->tx_base + que_sz * ch);
> +
> +       ivc_chan = bpmp->ivc_channels + ch;
> +       ret = tegra_ivc_init(ivc_chan, rx_base, DMA_ERROR_CODE, tx_base,
> +                            DMA_ERROR_CODE, 1, msg_sz, bpmp->dev,
> +                            bpmp_ivc_notify);
> +       if (ret) {
> +               dev_err(bpmp->dev, "%s fail: ch %d returned %d\n",
> +                       __func__, ch, ret);
> +               return ret;
> +       }
> +
> +       /* reset the channel state */
> +       tegra_ivc_channel_reset(ivc_chan);
> +
> +       /* sync the channel state with BPMP */
> +       while (tegra_ivc_channel_notified(ivc_chan))
> +               ;
> +
> +       return 0;
> +}
> +
> +struct tegra_bpmp_ops *tegra_bpmp_get_ops(void)
> +{
> +       if (bpmp->init_done && bpmp->ops)
> +               return bpmp->ops;
> +       return NULL;
> +}
> +EXPORT_SYMBOL(tegra_bpmp_get_ops);
> +
> +static struct tegra_bpmp_ops bpmp_ops = {
> +       .send_receive = bpmp_send_receive,
> +       .send_receive_atomic = bpmp_send_receive_atomic,
> +       .request_mrq = bpmp_request_mrq,
> +       .mrq_return = bpmp_mail_return,
> +};
> +
> +static const struct tegra_bpmp_soc_data soc_data_tegra186 = {
> +       .ch_index = 0,
> +       .thread_ch_index = 6,
> +       .cpu_rx_ch_index = 13,
> +       .nr_ch = 14,
> +       .nr_thread_ch = 7,
> +       .ch_timeout = 60 * USEC_PER_SEC,
> +       .thread_ch_timeout = 600 * USEC_PER_SEC,
> +};
> +
> +static const struct of_device_id tegra_bpmp_match[] = {
> +       { .compatible = "nvidia,tegra186-bpmp", .data = &soc_data_tegra186 },
> +       { }
> +};
> +
> +static int tegra_bpmp_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       struct resource shmem_res;
> +       struct device_node *shmem_np;
> +       int i, ret;
> +
> +       bpmp = devm_kzalloc(&pdev->dev, sizeof(*bpmp), GFP_KERNEL);
> +       if (!bpmp)
> +               return -ENOMEM;
> +       bpmp->dev = &pdev->dev;
> +
> +       match = of_match_device(tegra_bpmp_match, &pdev->dev);
> +       if (!match)
> +               return -EINVAL;
> +       bpmp->soc_data = match->data;
> +
> +       shmem_np = of_parse_phandle(pdev->dev.of_node, "shmem", 0);
> +       of_address_to_resource(shmem_np, 0, &shmem_res);

Maybe check return value for these two calls?

> +       bpmp->tx_base = devm_ioremap_resource(&pdev->dev, &shmem_res);
> +       if (IS_ERR(bpmp->tx_base))
> +               return PTR_ERR(bpmp->tx_base);
> +
> +       shmem_np = of_parse_phandle(pdev->dev.of_node, "shmem", 1);
> +       of_address_to_resource(shmem_np, 0, &shmem_res);

And here too.

> +       bpmp->rx_base = devm_ioremap_resource(&pdev->dev, &shmem_res);
> +       if (IS_ERR(bpmp->rx_base))
> +               return PTR_ERR(bpmp->rx_base);
> +
> +       bpmp->ivc_channels = devm_kcalloc(&pdev->dev, bpmp->soc_data->nr_ch,
> +                                         sizeof(*bpmp->ivc_channels),
> +                                         GFP_KERNEL);
> +       if (!bpmp->ivc_channels)
> +               return -ENOMEM;
> +
> +       bpmp->ch_area = devm_kcalloc(&pdev->dev, bpmp->soc_data->nr_ch,
> +                                    sizeof(*bpmp->ch_area), GFP_KERNEL);
> +       if (!bpmp->ch_area)
> +               return -ENOMEM;
> +
> +       bpmp->ch_completion = devm_kcalloc(&pdev->dev,
> +                                          bpmp->soc_data->nr_thread_ch,
> +                                          sizeof(*bpmp->ch_completion),
> +                                          GFP_KERNEL);
> +       if (!bpmp->ch_completion)
> +               return -ENOMEM;
> +
> +       /* mbox registration */
> +       bpmp->cl.dev = &pdev->dev;
> +       bpmp->cl.rx_callback = bpmp_handle_rx;
> +       bpmp->cl.tx_block = false;
> +       bpmp->cl.knows_txdone = false;
> +       bpmp->chan = mbox_request_channel(&bpmp->cl, 0);
> +       if (IS_ERR(bpmp->chan)) {
> +               if (PTR_ERR(bpmp->chan) != -EPROBE_DEFER)
> +                       dev_err(&pdev->dev,
> +                               "fail to get HSP mailbox, bpmp init fail.\n");
> +               return PTR_ERR(bpmp->chan);
> +       }
> +
> +       /* message channel initialization */
> +       for (i = 0; i < bpmp->soc_data->nr_ch; i++) {
> +               struct completion *comp_obj;
> +
> +               ret = bpmp_msg_chan_init(i);
> +               if (ret)
> +                       return ret;
> +
> +               comp_obj = bpmp_get_completion_obj(i);
> +               if (comp_obj)
> +                       init_completion(comp_obj);
> +       }
> +
> +       bpmp->ch_info.tch_free = (1 << bpmp->soc_data->nr_thread_ch) - 1;
> +       sema_init(&bpmp->ch_info.tch_sem, bpmp->soc_data->nr_thread_ch);
> +
> +       spin_lock_init(&bpmp->lock);
> +       INIT_LIST_HEAD(&bpmp->mrq_list);
> +       if (bpmp_mailman_init())
> +               return -ENODEV;
> +
> +       bpmp->init_done = true;
> +
> +       ret = bpmp_ping();
> +       if (ret)
> +               dev_err(&pdev->dev, "ping failed: %d\n", ret);
> +
> +       ret = bpmp_get_fwtag();
> +       if (ret)
> +               dev_err(&pdev->dev, "get fwtag failed: %d\n", ret);
> +
> +       /* BPMP is ready now. */
> +       bpmp->ops = &bpmp_ops;
> +
> +       return 0;
> +}
> +
> +static struct platform_driver tegra_bpmp_driver = {
> +       .driver = {
> +               .name = "tegra-bpmp",
> +               .of_match_table = tegra_bpmp_match,
> +       },
> +       .probe = tegra_bpmp_probe,
> +};
> +
> +static int __init tegra_bpmp_init(void)
> +{
> +       return platform_driver_register(&tegra_bpmp_driver);
> +}
> +core_initcall(tegra_bpmp_init);
> diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h
> new file mode 100644
> index 000000000000..aaa0ef34ad7b
> --- /dev/null
> +++ b/include/soc/tegra/bpmp.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2016, 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,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef __TEGRA_BPMP_H
> +
> +typedef void (*bpmp_mrq_handler)(int mrq_code, void *data, int ch);
> +
> +struct tegra_bpmp_ops {
> +       int (*send_receive)(int mrq_code, void *ob_data, int ob_sz,
> +                           void *ib_data, int ib_sz);
> +       int (*send_receive_atomic)(int mrq_code, void *ob_data, int ob_sz,
> +                           void *ib_data, int ib_sz);
> +       int (*request_mrq)(int mrq_code, bpmp_mrq_handler handler, void *data);
> +       void (*mrq_return)(int ch, int ret_code, int val);
> +};
> +
> +struct tegra_bpmp_ops *tegra_bpmp_get_ops(void);
> +
> +#endif /* __TEGRA_BPMP_H */
> diff --git a/include/soc/tegra/bpmp_abi.h b/include/soc/tegra/bpmp_abi.h
> new file mode 100644
> index 000000000000..0aaef5960e29
> --- /dev/null
> +++ b/include/soc/tegra/bpmp_abi.h
> @@ -0,0 +1,1601 @@
> +/*
> + * Copyright (c) 2014-2016, 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,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef _ABI_BPMP_ABI_H_
> +#define _ABI_BPMP_ABI_H_
> +
> +#ifdef LK
> +#include <stdint.h>
> +#endif
> +
> +#ifndef __ABI_PACKED
> +#define __ABI_PACKED __attribute__((packed))
> +#endif
> +
> +#ifdef NO_GCC_EXTENSIONS
> +#define EMPTY char empty;
> +#define EMPTY_ARRAY 1
> +#else
> +#define EMPTY
> +#define EMPTY_ARRAY 0
> +#endif
> +
> +#ifndef __UNION_ANON
> +#define __UNION_ANON
> +#endif
> +/**
> + * @file
> + */
> +
> +
> +/**
> + * @defgroup MRQ MRQ Messages
> + * @brief Messages sent to/from BPMP via IPC
> + * @{
> + *   @defgroup MRQ_Format Message Format
> + *   @defgroup MRQ_Codes Message Request (MRQ) Codes
> + *   @defgroup MRQ_Payloads Message Payloads
> + *   @defgroup Error_Codes Error Codes
> + * @}
> + */

...

There is a lot of stuff in this file, most of which we are not using
now - this is ok, but unless this is a file synced from an outside
resource maybe we should trim the structures we don't need and add
them as we make use of them? It helps dividing the work in bite-size
chunks.

Regarding the documentation format of this file, is this valid kernel
documentation since the adoption of Sphynx? Or is it whatever the
origin is using?