Am 10.03.2020 um 17:42 schrieb Ang, Chee Hong: >> -----Original Message----- >> From: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> >> Sent: Wednesday, March 11, 2020 12:17 AM >> To: Ang, Chee Hong <chee.hong....@intel.com> >> Cc: u-boot@lists.denx.de; Marek Vasut <ma...@denx.de>; See, Chin Liang >> <chin.liang....@intel.com>; Tan, Ley Foon <ley.foon....@intel.com>; >> Westergreen, Dalon <dalon.westergr...@intel.com>; Gong, Richard >> <richard.g...@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....@intel.com: >>> From: Chee Hong Ang <chee.hong....@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....@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.
Ah, I missed that part of the series. I'm still reading it. Making gen5 use misc_read/misc_write seems a bit strange, but I can't think of a better way right now, either... > Can I know what does your gen5 sysmgr driver do ? I moved "pin init", "freezereq" and "get fpga ID" there to have less ad-hoc code in the main SPL file... The series where it's in targets moving as much as I can to DM drivers. Sadly, I still haven't found a way to make it fit into the gen5 SRAM, which is why I haven't posted it, yet... > I can change the name to avoid conflict but Gen5 will have 2 sysmgr drivers > for different purposes. > Are you OK with that ? Hmm, I don't think that will work. That would mean binding 2 drivers to one ofnode. I can split the gen5 driver later and implement read/write like it's needed if this one gets applied as is. >> >>> @@ -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? >> >> 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, >>> +}; >>> >