[PATCH] firmware: bluefield: add boot control driver

[PATCH] firmware: bluefield: add boot control driver

[Chris Metcalf]

The Mellanox BlueField SoC firmware supports a safe upgrade mode as
part of the flow where users put new firmware on the secondary eMMC
boot partition (the one not currently in use), tell the eMMC to make
the secondary boot partition primary, and reset.  This driver is
used to request that the firmware start the ARM watchdog after the
next reset, and also request that the firmware swap the eMMC boot
partition back again on the reset after that (the second reset).
This means that if anything goes wrong, the watchdog will fire, the
system will reset, and the firmware will switch back to the original
boot partition.  If the boot is successful, the user will use this
driver to put the firmware back into the state where it doesn't touch
the eMMC boot partition at reset, and turn off the ARM watchdog.

The firmware allows for more configurability than that, as can
be seen in the code, but the use case above is what the driver
primarily supports.

It is structured as a simple sysfs driver that is loaded based on
an ACPI table entry, and allows reading/writing text strings to
various /sys/bus/platform/drivers/mlx-bootctl/* files.

Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
---
Ingo, since there isn't an overall maintainer for drivers/firmware,
does it make sense for this to go through your tree?  Thanks!

 drivers/firmware/Kconfig       |  12 +++
 drivers/firmware/Makefile      |   1 +
 drivers/firmware/mlx-bootctl.c | 222 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/mlx-bootctl.h | 103 +++++++++++++++++++
 4 files changed, 338 insertions(+)
 create mode 100644 drivers/firmware/mlx-bootctl.c
 create mode 100644 drivers/firmware/mlx-bootctl.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e4ed5a9c6fd..1f2adbcc5acc 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -230,6 +230,18 @@ config TI_SCI_PROTOCOL
 	  This protocol library is used by client drivers to use the features
 	  provided by the system controller.
 
+config MLX_BOOTCTL
+	tristate "Mellanox BlueField Firmware Boot Control"
+	depends on ARM64
+	help
+	  The Mellanox BlueField firmware implements functionality to
+	  request swapping the primary and alternate eMMC boot
+	  partition, and to set up a watchdog that can undo that swap
+	  if the system does not boot up correctly.  This driver
+	  provides sysfs access to the firmware, to be used in
+	  conjunction with the eMMC device driver to do any necessary
+	  initial swap of the boot partition.
+
 config HAVE_ARM_SMCCC
 	bool
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index a37f12e8d137..4f4cad1eb9dd 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_QCOM_SCM_64)	+= qcom_scm-64.o
 obj-$(CONFIG_QCOM_SCM_32)	+= qcom_scm-32.o
 CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQUIRES_SEC=1) -march=armv7-a
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
+obj-$(CONFIG_MLX_BOOTCTL)	+= mlx-bootctl.o
 
 obj-y				+= broadcom/
 obj-y				+= meson/
diff --git a/drivers/firmware/mlx-bootctl.c b/drivers/firmware/mlx-bootctl.c
new file mode 100644
index 000000000000..7fe942e9d7bb
--- /dev/null
+++ b/drivers/firmware/mlx-bootctl.c
@@ -0,0 +1,222 @@
+/*
+ *  Mellanox boot control driver
+ *  This driver provides a sysfs interface for systems management
+ *  software to manage reset-time actions.
+ *
+ *  Copyright (C) 2017 Mellanox Technologies.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License v2.0 as published by
+ *  the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that 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/acpi.h>
+#include <linux/arm-smccc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include "mlx-bootctl.h"
+
+#define DRIVER_NAME		"mlx-bootctl"
+#define DRIVER_VERSION		"1.1"
+#define DRIVER_DESCRIPTION	"Mellanox boot control driver"
+
+struct boot_name {
+	int value;
+	const char name[12];
+};
+
+static struct boot_name boot_names[] = {
+	{ MLNX_BOOT_EXTERNAL,		"external"	},
+	{ MLNX_BOOT_EMMC,		"emmc"		},
+	{ MLNX_BOOT_SWAP_EMMC,		"swap_emmc"	},
+	{ MLNX_BOOT_EMMC_LEGACY,	"emmc_legacy"	},
+	{ MLNX_BOOT_NONE,		"none"		},
+	{ -1,				""		}
+};
+
+/* The SMC calls in question are atomic, so we don't have to lock here. */
+static int smc_call1(unsigned int smc_op, int smc_arg)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(smc_op, smc_arg, 0, 0, 0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+/* Syntactic sugar to avoid having to specify an unused argument. */
+#define smc_call0(smc_op) smc_call1(smc_op, 0)
+
+static int reset_action_to_val(const char *action, size_t len)
+{
+	struct boot_name *bn;
+
+	/* Accept string either with or without a newline terminator */
+	if (action[len-1] == '\n')
+		--len;
+
+	for (bn = boot_names; bn->value >= 0; ++bn)
+		if (strncmp(bn->name, action, len) == 0)
+			break;
+
+	return bn->value;
+}
+
+static const char *reset_action_to_string(int action)
+{
+	struct boot_name *bn;
+
+	for (bn = boot_names; bn->value >= 0; ++bn)
+		if (bn->value == action)
+			break;
+
+	return bn->name;
+}
+
+static ssize_t post_reset_wdog_show(struct device_driver *drv,
+				    char *buf)
+{
+	return sprintf(buf, "%d\n", smc_call0(MLNX_GET_POST_RESET_WDOG));
+}
+
+static ssize_t post_reset_wdog_store(struct device_driver *drv,
+				     const char *buf, size_t count)
+{
+	int err;
+	unsigned long watchdog;
+
+	err = kstrtoul(buf, 10, &watchdog);
+	if (err)
+		return err;
+
+	if (smc_call1(MLNX_SET_POST_RESET_WDOG, watchdog) < 0)
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t reset_action_show(struct device_driver *drv,
+				 char *buf)
+{
+	return sprintf(buf, "%s\n", reset_action_to_string(
+			       smc_call0(MLNX_GET_RESET_ACTION)));
+}
+
+static ssize_t reset_action_store(struct device_driver *drv,
+				  const char *buf, size_t count)
+{
+	int action = reset_action_to_val(buf, count);
+
+	if (action < 0 || action == MLNX_BOOT_NONE)
+		return -EINVAL;
+
+	if (smc_call1(MLNX_SET_RESET_ACTION, action) < 0)
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t second_reset_action_show(struct device_driver *drv,
+					char *buf)
+{
+	return sprintf(buf, "%s\n", reset_action_to_string(
+			       smc_call0(MLNX_GET_SECOND_RESET_ACTION)));
+}
+
+static ssize_t second_reset_action_store(struct device_driver *drv,
+					 const char *buf, size_t count)
+{
+	int action = reset_action_to_val(buf, count);
+
+	if (action < 0)
+		return -EINVAL;
+
+	if (smc_call1(MLNX_SET_SECOND_RESET_ACTION, action) < 0)
+		return -EINVAL;
+
+	return count;
+}
+
+#define MBC_DRV_ATTR(_name) \
+	DRIVER_ATTR(_name, 0600, _name##_show, _name##_store)
+
+static MBC_DRV_ATTR(post_reset_wdog);
+static MBC_DRV_ATTR(reset_action);
+static MBC_DRV_ATTR(second_reset_action);
+
+static struct attribute *mbc_dev_attrs[] = {
+	&driver_attr_post_reset_wdog.attr,
+	&driver_attr_reset_action.attr,
+	&driver_attr_second_reset_action.attr,
+	NULL
+};
+
+static struct attribute_group mbc_attr_group = {
+	.attrs = mbc_dev_attrs
+};
+
+static const struct attribute_group *mbc_attr_groups[] = {
+	&mbc_attr_group,
+	NULL
+};
+
+static const struct of_device_id mbc_dt_ids[] = {
+	{.compatible = "mellanox,bootctl"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, mbc_dt_ids);
+
+static const struct acpi_device_id mbc_acpi_ids[] = {
+	{"MLNXBF04", 0},
+	{},
+};
+
+MODULE_DEVICE_TABLE(acpi, mbc_acpi_ids);
+
+static int mbc_probe(struct platform_device *pdev)
+{
+	struct arm_smccc_res res;
+
+	/*
+	 * Ensure we have the UUID we expect for this service.
+	 * Note that the functionality we want is present in the first
+	 * released version of this service, so we don't check the version.
+	 */
+	arm_smccc_smc(MLNX_SIP_SVC_UID, 0, 0, 0, 0, 0, 0, 0, &res);
+	if (res.a0 != 0x89c036b4 || res.a1 != 0x11e6e7d7 ||
+	    res.a2 != 0x1a009787 || res.a3 != 0xc4bf00ca)
+		return -ENODEV;
+
+	pr_info("%s (version %s)\n", DRIVER_DESCRIPTION, DRIVER_VERSION);
+
+	return 0;
+}
+
+static int mbc_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver mbc_driver = {
+	.probe = mbc_probe,
+	.remove = mbc_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.groups = mbc_attr_groups,
+		.of_match_table = mbc_dt_ids,
+		.acpi_match_table = ACPI_PTR(mbc_acpi_ids),
+	}
+};
+
+module_platform_driver(mbc_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_AUTHOR("Mellanox Technologies");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/mlx-bootctl.h b/drivers/firmware/mlx-bootctl.h
new file mode 100644
index 000000000000..d93cdafc4b43
--- /dev/null
+++ b/drivers/firmware/mlx-bootctl.h
@@ -0,0 +1,103 @@
+/*
+ *  Mellanox boot control driver
+ *  These definitions are shared with the ATF source code.
+ *
+ *  Copyright (C) 2017 Mellanox Technologies.  All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License v2.0 as published by
+ *  the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that 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 __BLUEFIELD_SVC_H__
+#define __BLUEFIELD_SVC_H__
+
+/* BlueField-specific SMC function IDs */
+
+/*
+ * Request that the on-chip watchdog be enabled, or disabled, after
+ * the next chip soft reset. This call does not affect the current
+ * status of the on-chip watchdog. If non-zero, the argument
+ * specifies the watchdog interval in seconds. If zero, the watchdog
+ * will not be enabled after the next soft reset. Non-zero errors are
+ * returned as documented below.
+ */
+#define MLNX_SET_POST_RESET_WDOG	0x82000000
+
+/*
+ * Query the status which has been requested for the on-chip watchdog
+ * after the next chip soft reset. Returns the interval as set by
+ * MLNX_SET_POST_RESET_WDOG.
+ */
+#define MLNX_GET_POST_RESET_WDOG	0x82000001
+
+/*
+ * Request that a specific boot action be taken at the next soft
+ * reset. By default, the boot action is set by external chip pins,
+ * which are sampled on hard reset. Note that the boot action
+ * requested by this call will persist on subsequent resets unless
+ * this service, or the MLNX_SET_SECOND_RESET_ACTION service, is
+ * invoked. See below for the available MLNX_BOOT_xxx parameter
+ * values. Non-zero errors are returned as documented below.
+ */
+#define MLNX_SET_RESET_ACTION		0x82000002
+
+/*
+ * Return the specific boot action which will be taken at the next
+ * soft reset. Returns the reset action (see below for the parameter
+ * values for MLNX_SET_RESET_ACTION).
+ */
+#define MLNX_GET_RESET_ACTION		0x82000003
+
+/*
+ * Request that a specific boot action be taken at the soft reset
+ * after the next soft reset. For a specified valid boot mode, the
+ * effect of this call is identical to that of invoking
+ * MLNX_SET_RESET_ACTION after the next chip soft reset; in
+ * particular, after that reset, the action for the now next reset can
+ * be queried with MLNX_GET_RESET_ACTION and modified with
+ * MLNX_SET_RESET_ACTION. You may also specify the parameter as
+ * MLNX_BOOT_NONE, which is equivalent to specifying that no call to
+ * MLNX_SET_RESET_ACTION be taken after the next chip soft reset.
+ * This call does not affect the action to be taken at the next soft
+ * reset. Non-zero errors are returned as documented below.
+ */
+#define MLNX_SET_SECOND_RESET_ACTION	0x82000004
+
+/*
+ * Return the specific boot action which will be taken at the soft
+ * reset after the next soft reset; this will be one of the valid
+ * actions for MLNX_SET_SECOND_RESET_ACTION.
+ */
+#define MLNX_GET_SECOND_RESET_ACTION	0x82000005
+
+/* SMC function IDs for SiP Service queries */
+#define MLNX_SIP_SVC_CALL_COUNT		0x8200ff00
+#define MLNX_SIP_SVC_UID		0x8200ff01
+#define MLNX_SIP_SVC_VERSION		0x8200ff03
+
+/* ARM Standard Service Calls version numbers */
+#define MLNX_SVC_VERSION_MAJOR		0x0
+#define MLNX_SVC_VERSION_MINOR		0x1
+
+/* Number of svc calls defined. */
+#define MLNX_NUM_SVC_CALLS 9
+
+/* Valid reset actions for MLNX_SET_RESET_ACTION. */
+#define MLNX_BOOT_EXTERNAL	0 /* Do not boot from eMMC */
+#define MLNX_BOOT_EMMC		1 /* Boot from primary eMMC boot partition */
+#define MLNX_BOOT_SWAP_EMMC	2 /* Swap eMMC boot partitions and reboot */
+#define MLNX_BOOT_EMMC_LEGACY	3 /* Boot from primary eMMC in legacy mode */
+
+/* Additional parameter value to disable the MLNX_SET_SECOND_RESET_ACTION. */
+#define MLNX_BOOT_NONE		0x7fffffff /* Don't change next boot action */
+
+/* Error values (non-zero). */
+#define SMCCC_INVALID_PARAMETERS	-2
+
+#endif /* __BLUEFIELD_SVC_H__ */
-- 
2.1.2

Re: [PATCH] firmware: bluefield: add boot control driver

[Sudeep Holla]

(+Mark, Grant)

On 09/10/17 18:16, Chris Metcalf wrote:
> The Mellanox BlueField SoC firmware supports a safe upgrade mode as
> part of the flow where users put new firmware on the secondary eMMC
> boot partition (the one not currently in use), tell the eMMC to make
> the secondary boot partition primary, and reset.  This driver is
> used to request that the firmware start the ARM watchdog after the
> next reset, and also request that the firmware swap the eMMC boot
> partition back again on the reset after that (the second reset).
> This means that if anything goes wrong, the watchdog will fire, the
> system will reset, and the firmware will switch back to the original
> boot partition.  If the boot is successful, the user will use this
> driver to put the firmware back into the state where it doesn't touch
> the eMMC boot partition at reset, and turn off the ARM watchdog.
> 
> The firmware allows for more configurability than that, as can
> be seen in the code, but the use case above is what the driver
> primarily supports.
> 
> It is structured as a simple sysfs driver that is loaded based on
> an ACPI table entry, and allows reading/writing text strings to
> various /sys/bus/platform/drivers/mlx-bootctl/* files.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> ---
> Ingo, since there isn't an overall maintainer for drivers/firmware,
> does it make sense for this to go through your tree?  Thanks!
> 
>  drivers/firmware/Kconfig       |  12 +++
>  drivers/firmware/Makefile      |   1 +
>  drivers/firmware/mlx-bootctl.c | 222 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/mlx-bootctl.h | 103 +++++++++++++++++++
>  4 files changed, 338 insertions(+)
>  create mode 100644 drivers/firmware/mlx-bootctl.c
>  create mode 100644 drivers/firmware/mlx-bootctl.h
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 6e4ed5a9c6fd..1f2adbcc5acc 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -230,6 +230,18 @@ config TI_SCI_PROTOCOL
>  	  This protocol library is used by client drivers to use the features
>  	  provided by the system controller.
>  
> +config MLX_BOOTCTL
> +	tristate "Mellanox BlueField Firmware Boot Control"
> +	depends on ARM64
> +	help
> +	  The Mellanox BlueField firmware implements functionality to
> +	  request swapping the primary and alternate eMMC boot
> +	  partition, and to set up a watchdog that can undo that swap
> +	  if the system does not boot up correctly.  This driver
> +	  provides sysfs access to the firmware, to be used in
> +	  conjunction with the eMMC device driver to do any necessary
> +	  initial swap of the boot partition.
> +
For me, this looks like any other distro upgrade use-case which requires
to set some variable that must persist across reset (i.e. in non
volatile memory)

ARM is proposing Embedded Base Boot Requirements (EBBR)[1] and it covers
this IIUC. I am not sure if we need to achieve that using SMCCC as
proposed in the patch. I may be wrong, but just wanted to check.

-- 
[1]
https://developer.arm.com/products/architecture/system-architecture/embedded-system-architecture
-- 
Regards,
Sudeep

Re: [PATCH] firmware: bluefield: add boot control driver

[Mark Rutland]

On Tue, Oct 10, 2017 at 11:15:39AM +0100, Sudeep Holla wrote:
> (+Mark, Grant)
> 
> On 09/10/17 18:16, Chris Metcalf wrote:
> > The Mellanox BlueField SoC firmware supports a safe upgrade mode as
> > part of the flow where users put new firmware on the secondary eMMC
> > boot partition (the one not currently in use), tell the eMMC to make
> > the secondary boot partition primary, and reset.

When you say "firmware", are you referreind to actual firmware, or a
platform-specific OS image?

For the former, the preferred update mechanism would be UpdateCapsule().

For the latter, I'm not sure what the usual mechanism for doing this
with EFI would be.

Ard, Leif?

Thanks,
Mark.

> > This driver is
> > used to request that the firmware start the ARM watchdog after the
> > next reset, and also request that the firmware swap the eMMC boot
> > partition back again on the reset after that (the second reset).
> > This means that if anything goes wrong, the watchdog will fire, the
> > system will reset, and the firmware will switch back to the original
> > boot partition.  If the boot is successful, the user will use this
> > driver to put the firmware back into the state where it doesn't touch
> > the eMMC boot partition at reset, and turn off the ARM watchdog.
> > 
> > The firmware allows for more configurability than that, as can
> > be seen in the code, but the use case above is what the driver
> > primarily supports.
> > 
> > It is structured as a simple sysfs driver that is loaded based on
> > an ACPI table entry, and allows reading/writing text strings to
> > various /sys/bus/platform/drivers/mlx-bootctl/* files.
> > 
> > Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> > ---
> > Ingo, since there isn't an overall maintainer for drivers/firmware,
> > does it make sense for this to go through your tree?  Thanks!
> > 
> >  drivers/firmware/Kconfig       |  12 +++
> >  drivers/firmware/Makefile      |   1 +
> >  drivers/firmware/mlx-bootctl.c | 222 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/firmware/mlx-bootctl.h | 103 +++++++++++++++++++
> >  4 files changed, 338 insertions(+)
> >  create mode 100644 drivers/firmware/mlx-bootctl.c
> >  create mode 100644 drivers/firmware/mlx-bootctl.h
> > 
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 6e4ed5a9c6fd..1f2adbcc5acc 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -230,6 +230,18 @@ config TI_SCI_PROTOCOL
> >  	  This protocol library is used by client drivers to use the features
> >  	  provided by the system controller.
> >  
> > +config MLX_BOOTCTL
> > +	tristate "Mellanox BlueField Firmware Boot Control"
> > +	depends on ARM64
> > +	help
> > +	  The Mellanox BlueField firmware implements functionality to
> > +	  request swapping the primary and alternate eMMC boot
> > +	  partition, and to set up a watchdog that can undo that swap
> > +	  if the system does not boot up correctly.  This driver
> > +	  provides sysfs access to the firmware, to be used in
> > +	  conjunction with the eMMC device driver to do any necessary
> > +	  initial swap of the boot partition.
> > +
> For me, this looks like any other distro upgrade use-case which requires
> to set some variable that must persist across reset (i.e. in non
> volatile memory)
> 
> ARM is proposing Embedded Base Boot Requirements (EBBR)[1] and it covers
> this IIUC. I am not sure if we need to achieve that using SMCCC as
> proposed in the patch. I may be wrong, but just wanted to check.
> 
> -- 
> [1]
> https://developer.arm.com/products/architecture/system-architecture/embedded-system-architecture
> -- 
> Regards,
> Sudeep

Re: [PATCH] firmware: bluefield: add boot control driver

[Ard Biesheuvel]

On 10 October 2017 at 11:23, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Oct 10, 2017 at 11:15:39AM +0100, Sudeep Holla wrote:
>> (+Mark, Grant)
>>
>> On 09/10/17 18:16, Chris Metcalf wrote:
>> > The Mellanox BlueField SoC firmware supports a safe upgrade mode as
>> > part of the flow where users put new firmware on the secondary eMMC
>> > boot partition (the one not currently in use), tell the eMMC to make
>> > the secondary boot partition primary, and reset.
>
> When you say "firmware", are you referreind to actual firmware, or a
> platform-specific OS image?
>
> For the former, the preferred update mechanism would be UpdateCapsule().
>
> For the latter, I'm not sure what the usual mechanism for doing this
> with EFI would be.
>
> Ard, Leif?
>

UEFI does not really care how you manage your OS images, that is up to
the OS itself. UpdateCapsule() does allow you to update system
firmware (UEFI and what executes before it), device firmware (options
ROMs), and actually, anything the platform vendor thought would be
useful to have as a capsule-updatable image.

UEFI does have the notion of SysPrep and PlatformRecovery variables,
and an ephemeral BootNext variable, so the logic of booting into an
alternative OS loader just once is made available by UEFI to the OS.

Of course, for some vendors, especially in the ARM world, 'firmware'
means ARM-TF + UEFI + kernel + initrd + rootfs, so it does make sense
to clarify what you are updating here.

Re: [PATCH] firmware: bluefield: add boot control driver

[Leif Lindholm]

On Tue, Oct 10, 2017 at 11:23:32AM +0100, Mark Rutland wrote:
> On Tue, Oct 10, 2017 at 11:15:39AM +0100, Sudeep Holla wrote:
> > (+Mark, Grant)
> > 
> > On 09/10/17 18:16, Chris Metcalf wrote:
> > > The Mellanox BlueField SoC firmware supports a safe upgrade mode as
> > > part of the flow where users put new firmware on the secondary eMMC
> > > boot partition (the one not currently in use), tell the eMMC to make
> > > the secondary boot partition primary, and reset.
> 
> When you say "firmware", are you referreind to actual firmware, or a
> platform-specific OS image?
> 
> For the former, the preferred update mechanism would be UpdateCapsule().
> 
> For the latter, I'm not sure what the usual mechanism for doing this
> with EFI would be.
> 
> Ard, Leif?

This sounds to me very much like something we'd want to keep out of
the kernel. Assuming ACPI means UEFI, there should be less invasive
solutions to this problem.

I've added linaro-uefi to cc. Chris - if you'd be willing to have a
side discussion on the overall aproach, please drop the kernel lists
from cc and let's have a chat there.

Best Regards,

Leif

> Thanks,
> Mark.
> 
> > > This driver is
> > > used to request that the firmware start the ARM watchdog after the
> > > next reset, and also request that the firmware swap the eMMC boot
> > > partition back again on the reset after that (the second reset).
> > > This means that if anything goes wrong, the watchdog will fire, the
> > > system will reset, and the firmware will switch back to the original
> > > boot partition.  If the boot is successful, the user will use this
> > > driver to put the firmware back into the state where it doesn't touch
> > > the eMMC boot partition at reset, and turn off the ARM watchdog.
> > > 
> > > The firmware allows for more configurability than that, as can
> > > be seen in the code, but the use case above is what the driver
> > > primarily supports.
> > > 
> > > It is structured as a simple sysfs driver that is loaded based on
> > > an ACPI table entry, and allows reading/writing text strings to
> > > various /sys/bus/platform/drivers/mlx-bootctl/* files.
> > > 
> > > Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> > > ---
> > > Ingo, since there isn't an overall maintainer for drivers/firmware,
> > > does it make sense for this to go through your tree?  Thanks!
> > > 
> > >  drivers/firmware/Kconfig       |  12 +++
> > >  drivers/firmware/Makefile      |   1 +
> > >  drivers/firmware/mlx-bootctl.c | 222 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/firmware/mlx-bootctl.h | 103 +++++++++++++++++++
> > >  4 files changed, 338 insertions(+)
> > >  create mode 100644 drivers/firmware/mlx-bootctl.c
> > >  create mode 100644 drivers/firmware/mlx-bootctl.h
> > > 
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index 6e4ed5a9c6fd..1f2adbcc5acc 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -230,6 +230,18 @@ config TI_SCI_PROTOCOL
> > >  	  This protocol library is used by client drivers to use the features
> > >  	  provided by the system controller.
> > >  
> > > +config MLX_BOOTCTL
> > > +	tristate "Mellanox BlueField Firmware Boot Control"
> > > +	depends on ARM64
> > > +	help
> > > +	  The Mellanox BlueField firmware implements functionality to
> > > +	  request swapping the primary and alternate eMMC boot
> > > +	  partition, and to set up a watchdog that can undo that swap
> > > +	  if the system does not boot up correctly.  This driver
> > > +	  provides sysfs access to the firmware, to be used in
> > > +	  conjunction with the eMMC device driver to do any necessary
> > > +	  initial swap of the boot partition.
> > > +
> > For me, this looks like any other distro upgrade use-case which requires
> > to set some variable that must persist across reset (i.e. in non
> > volatile memory)
> > 
> > ARM is proposing Embedded Base Boot Requirements (EBBR)[1] and it covers
> > this IIUC. I am not sure if we need to achieve that using SMCCC as
> > proposed in the patch. I may be wrong, but just wanted to check.
> > 
> > -- 
> > [1]
> > https://developer.arm.com/products/architecture/system-architecture/embedded-system-architecture
> > -- 
> > Regards,
> > Sudeep

Re: [PATCH] firmware: bluefield: add boot control driver

[Grant Likely]

On Tue, Oct 10, 2017 at 12:58 PM, Leif Lindholm
<leif.lindholm@linaro.org> wrote:
>
> On Tue, Oct 10, 2017 at 11:23:32AM +0100, Mark Rutland wrote:
>> On Tue, Oct 10, 2017 at 11:15:39AM +0100, Sudeep Holla wrote:
>> > (+Mark, Grant)
>> >
>> > On 09/10/17 18:16, Chris Metcalf wrote:
>> > > The Mellanox BlueField SoC firmware supports a safe upgrade mode as
>> > > part of the flow where users put new firmware on the secondary eMMC
>> > > boot partition (the one not currently in use), tell the eMMC to make
>> > > the secondary boot partition primary, and reset.
>>
>> When you say "firmware", are you referreind to actual firmware, or a
>> platform-specific OS image?
>>
>> For the former, the preferred update mechanism would be UpdateCapsule().
>>
>> For the latter, I'm not sure what the usual mechanism for doing this
>> with EFI would be.
>>
>> Ard, Leif?
>
> This sounds to me very much like something we'd want to keep out of
> the kernel. Assuming ACPI means UEFI, there should be less invasive
> solutions to this problem.

Agreed. This looks like a very specific solution to a generic problem.

g.

Re: [PATCH] firmware: bluefield: add boot control driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4274 bytes --]

Hi Chris,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc4 next-20171009]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Metcalf/firmware-bluefield-add-boot-control-driver/20171012-023624
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All error/warnings (new ones prefixed by >>):

>> drivers/firmware/mlx-bootctl.c:146:21: error: expected ')' before numeric constant
     DRIVER_ATTR(_name, 0600, _name##_show, _name##_store)
                        ^
>> drivers/firmware/mlx-bootctl.c:148:8: note: in expansion of macro 'MBC_DRV_ATTR'
    static MBC_DRV_ATTR(post_reset_wdog);
           ^~~~~~~~~~~~
>> drivers/firmware/mlx-bootctl.c:146:21: error: expected ')' before numeric constant
     DRIVER_ATTR(_name, 0600, _name##_show, _name##_store)
                        ^
   drivers/firmware/mlx-bootctl.c:149:8: note: in expansion of macro 'MBC_DRV_ATTR'
    static MBC_DRV_ATTR(reset_action);
           ^~~~~~~~~~~~
>> drivers/firmware/mlx-bootctl.c:146:21: error: expected ')' before numeric constant
     DRIVER_ATTR(_name, 0600, _name##_show, _name##_store)
                        ^
   drivers/firmware/mlx-bootctl.c:150:8: note: in expansion of macro 'MBC_DRV_ATTR'
    static MBC_DRV_ATTR(second_reset_action);
           ^~~~~~~~~~~~
>> drivers/firmware/mlx-bootctl.c:153:3: error: 'driver_attr_post_reset_wdog' undeclared here (not in a function)
     &driver_attr_post_reset_wdog.attr,
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/firmware/mlx-bootctl.c:154:3: error: 'driver_attr_reset_action' undeclared here (not in a function)
     &driver_attr_reset_action.attr,
      ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/firmware/mlx-bootctl.c:155:3: error: 'driver_attr_second_reset_action' undeclared here (not in a function)
     &driver_attr_second_reset_action.attr,
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:131:16: warning: 'second_reset_action_store' defined but not used [-Wunused-function]
    static ssize_t second_reset_action_store(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:124:16: warning: 'second_reset_action_show' defined but not used [-Wunused-function]
    static ssize_t second_reset_action_show(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:110:16: warning: 'reset_action_store' defined but not used [-Wunused-function]
    static ssize_t reset_action_store(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:103:16: warning: 'reset_action_show' defined but not used [-Wunused-function]
    static ssize_t reset_action_show(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:87:16: warning: 'post_reset_wdog_store' defined but not used [-Wunused-function]
    static ssize_t post_reset_wdog_store(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~~~~~
   drivers/firmware/mlx-bootctl.c:81:16: warning: 'post_reset_wdog_show' defined but not used [-Wunused-function]
    static ssize_t post_reset_wdog_show(struct device_driver *drv,
                   ^~~~~~~~~~~~~~~~~~~~

vim +146 drivers/firmware/mlx-bootctl.c

   144	
   145	#define MBC_DRV_ATTR(_name) \
 > 146		DRIVER_ATTR(_name, 0600, _name##_show, _name##_store)
   147	
 > 148	static MBC_DRV_ATTR(post_reset_wdog);
 > 149	static MBC_DRV_ATTR(reset_action);
 > 150	static MBC_DRV_ATTR(second_reset_action);
   151	
   152	static struct attribute *mbc_dev_attrs[] = {
 > 153		&driver_attr_post_reset_wdog.attr,
 > 154		&driver_attr_reset_action.attr,
 > 155		&driver_attr_second_reset_action.attr,
   156		NULL
   157	};
   158	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57569 bytes --]