Re: [PATCH v4 11/21] misc: altera_sysmgr: Add Altera System Manager driver

Am 10.03.2020 um 17:42 schrieb Ang, Chee Hong:

...

-----Original Message----- From: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Sent: Wednesday, March 11, 2020 12:17 AM To: Ang, Chee Hong chee.hong.ang@intel.com Cc: u-boot@lists.denx.de; Marek Vasut marex@denx.de; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Gong, Richard richard.gong@intel.com Subject: Re: [PATCH v4 11/21] misc: altera_sysmgr: Add Altera System Manager driver

Am 09.03.2020 um 10:07 schrieb chee.hong.ang@intel.com:

...

From: Chee Hong Ang chee.hong.ang@intel.com

This driver (misc uclass) handle the read/write access to System Manager. For 64 bits platforms, processor needs to be in secure mode to has write access to most of the System Manager's registers (except boot scratch registers). When the processor is running in EL2 (non-secure), this driver will invoke the SMC call to ATF to perform write access to the System Manager's registers. All other drivers that require access to System Manager should go through this driver.

Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com

drivers/misc/Makefile | 1 + drivers/misc/altera_sysmgr.c | 115 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 drivers/misc/altera_sysmgr.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 2b843de..9fa2411 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -29,6 +29,7 @@ endif endif obj-$(CONFIG_ALI152X) += ali512x.o obj-$(CONFIG_ALTERA_SYSID) += altera_sysid.o +obj-$(CONFIG_ARCH_SOCFPGA) += altera_sysmgr.o obj-$(CONFIG_ATSHA204A) += atsha204a-i2c.o obj-$(CONFIG_CBMEM_CONSOLE) += cbmem_console.o obj-$(CONFIG_DS4510) += ds4510.o diff --git a/drivers/misc/altera_sysmgr.c b/drivers/misc/altera_sysmgr.c new file mode 100644 index 0000000..b36ecae --- /dev/null +++ b/drivers/misc/altera_sysmgr.c

I think this file should have something in the name specifying it is for s10/agilex. I will post a misc/sysmgr for gen5 that needs a specific name, too

Gen5/A10/S10/Agilex are using same DW MMC/MAC drivers and these drivers access system manager. Therefore, this driver is enabled for all platforms. Gen5/A10, S10/Agilex all are using it. Can I know what does your gen5 sysmgr driver do ? I can change the name to avoid conflict but Gen5 will have 2 sysmgr drivers for different purposes. Are you OK with that ?

...

...

@@ -0,0 +1,115 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2020 Intel Corporation <www.intel.com> */

+#include <common.h> +#include <command.h> +#include <dm.h> +#include <errno.h> +#include <misc.h> +#include <asm/io.h> +#include <asm/arch/misc.h> +#include <linux/intel-smc.h>

+#define IS_OUT_OF_SYSMGR(addr, range) ((addr) > (range))

+struct altera_sysmgr_priv {

• fdt_addr_t base_addr;
• fdt_addr_t base_size;

+};

+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) static int +secure_write32(u32 val, fdt_addr_t addr) {

• int ret;
• u64 args[2];
• args[0] = (u64)addr;
• args[1] = val;
• ret = invoke_smc(INTEL_SIP_SMC_REG_WRITE, args, 2, NULL, 0);

Hmm, so you're just using misc_ops to still issue generic writes. From the discussion with Marek in the last version, I would have thought you wanted to create a higher level API instead of still tunnelling reads and writes?

Any response to this?

Thanks, Simon

...

In my gen5 series to abstract the gen5 sysmgr, I have used 'ioctl' and 'call' from misc_ops to have an API.

Regards, Simon

...

• if (ret)

• return -EIO;
  

• return 0;

+} +#endif

+static int write32(u32 val, fdt_addr_t addr) { #if +!defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF)

• return secure_write32(val, addr);

+#else

• writel(val, addr);
• return 0;

+#endif +}

+static int altera_sysmgr_read(struct udevice *dev,

• 	     int offset, void *buf, int size) {
  

• struct altera_sysmgr_priv *priv = dev_get_priv(dev);
• fdt_addr_t addr = priv->base_addr + offset;
• if (IS_OUT_OF_SYSMGR(addr, priv->base_addr + priv->base_size - size))

• return -EINVAL;
  

• if (size != sizeof(u32))

• return -EIO;
  

• *(u32 *)buf = readl(addr);
• return 0;

+}

+static int altera_sysmgr_write(struct udevice *dev, int offset,

• 		const void *buf, int size)
  

+{

• struct altera_sysmgr_priv *priv = dev_get_priv(dev);
• fdt_addr_t addr = priv->base_addr + offset;
• if (IS_OUT_OF_SYSMGR(addr, priv->base_addr + priv->base_size - size))

• return -EINVAL;
  

• if (size != sizeof(u32))

• return -EIO;
  

• return write32(*(u32 *)buf, addr);

+}

+static int altera_sysmgr_probe(struct udevice *dev) {

• struct altera_sysmgr_priv *priv = dev_get_priv(dev);
• fdt_addr_t addr;
• fdt_size_t size;
• addr = ofnode_get_addr_size_index(dev_ofnode(dev), 0, &size);
• if (addr == FDT_ADDR_T_NONE)

• return -EINVAL;
  

• priv->base_addr = addr;
• priv->base_size = size;
• return 0;

+}

+static const struct misc_ops altera_sysmgr_ops = {

• .read = altera_sysmgr_read,
• .write = altera_sysmgr_write,

+};

+static const struct udevice_id altera_sysmgr_ids[] = {

• { .compatible = "altr,sys-mgr" },
• {}

+};

+U_BOOT_DRIVER(altera_sysmgr) = {

• .name = "altr,sys-mgr",
• .id = UCLASS_MISC,
• .of_match = altera_sysmgr_ids,
• .priv_auto_alloc_size = sizeof(struct altera_sysmgr_priv),
• .probe = altera_sysmgr_probe,
• .ops = &altera_sysmgr_ops,

+};