[U-Boot] [PATCH 4/9] misc: add Tegra BPMP driver

[Stephen Warren]

On 07/31/2016 07:02 PM, Simon Glass wrote:
> On 27 July 2016 at 15:24, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren at nvidia.com>
>>
>> The Tegra BPMP (Boot and Power Management Processor) is a separate
>> auxiliary CPU embedded into Tegra to perform power management work, and
>> controls related features such as clocks, resets, power domains, PMIC I2C
>> bus, etc. This driver provides the core low-level communication path by
>> which feature-specific drivers (such as clock) can make requests to the
>> BPMP. This driver is similar to an MFD driver in the Linux kernel. It is
>> unconditionally selected by CONFIG_TEGRA186 since virtually any Tegra186
>> build of U-Boot will need the feature.

>> diff --git a/arch/arm/include/asm/arch-tegra/bpmp_abi.h b/arch/arm/include/asm/arch-tegra/bpmp_abi.h
>> new file mode 100644
>> index 000000000000..0aaef5960e29
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-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/>.
>
> Can this use SPDX? Or if it is generated, where from?

I forgot to convert this one. These headers are shipped alongside the 
BPMP FW so I deliberately edited it as little as possible, but I did 
mean to convert the license header.

>> diff --git a/drivers/misc/tegra186_bpmp.c b/drivers/misc/tegra186_bpmp.c

>> +int tegra186_bpmp_call(struct udevice *dev, uint32_t mrq,
>> +                      void *tx_msg, uint32_t tx_size,
>> +                      void *rx_msg, uint32_t rx_size)
>
> ... Also why is this exported? Shouldn't calls come in via DM??

This is an internal API between the core BPMP driver and the "services" 
drivers that are part of it. More below.

>> +/**
>> + * The BPMP exposes multiple different services. We create a sub-device for
>> + * each separate type of service, since each device must be of the appropriate
>> + * UCLASS.
>> + */
>> +static int tegra186_bpmp_bind(struct udevice *dev)
>> +{
>> +       int ret;
>> +       struct udevice *child;
>> +
>> +       debug("%s(dev=%p)\n", __func__, dev);
>> +
>> +       ret = device_bind_driver_to_node(dev, "tegra186_clk", "tegra186_clk",
>> +                                        dev->of_offset, &child);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = device_bind_driver_to_node(dev, "tegra186_reset",
>> +                                        "tegra186_reset", dev->of_offset,
>> +                                        &child);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = device_bind_driver_to_node(dev, "tegra186_power_domain",
>> +                                        "tegra186_power_domain",
>> +                                        dev->of_offset, &child);
>> +       if (ret)
>> +               return ret;
>
> What's happening here? Is there one device tree node with 3 devices?

The BPMP is a single device, hence a single DT node, that exports a 
multitude of services. This file/patch is the top-level/core driver for 
the BPMP. This function is instantiating a separate U-Boot driver for 
each type of service; DM requires each device/drive object be a single 
uclass. This is logically extremely similar to MFD devices in the Linux 
kernel.

>> +static ulong tegra186_bpmp_get_shmem(struct udevice *dev, int index)
>> +{
>> +       int ret;
>> +       struct fdtdec_phandle_args args;
>> +       struct udevice fakedev;
>> +       fdt_addr_t reg;
>> +
>> +       ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,
>> +                                             "shmem", NULL, 0, index, &args);
>> +       if (ret < 0) {
>> +               error("fdtdec_parse_phandle_with_args() failed: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       fakedev.of_offset = args.node;
>> +       reg = dev_get_addr_index(&fakedev, 0);
>
> This is nasty. If you don't set fakedev.parent, how does this work?

Hmmm. Good question; I thought dev_get_addr_index() only used the 
device's own of_offset, but it does look like it uses the parent device 
too. I'll have to look into this.

> Can you instead call fdtdec_get_addr_size_auto_parent() or similar?

dev_get_addr_index() is the only API which applies address translation 
(i.e. processes the DT ranges property).

>> +static void tegra186_bpmp_ivc_notify(struct tegra_ivc *ivc)
>> +{
>> +       struct tegra186_bpmp *priv =
>> +               container_of(ivc, struct tegra186_bpmp, ivc);
>> +       int ret;
>> +
>> +       ret = mbox_send(&priv->mbox, NULL);
>> +       if (ret)
>> +               error("mbox_send() failed: %d\n", ret);
>
> Then why not return the error?

This is a callback from the IVC library. The callback prototype doesn't 
allow errors to be returned. I'd rather not change that code since it's 
almost identical across various OSs (e.g. shared with mainline Linux).