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

[Simon Goldschmidt]

Am 10.03.2020 um 17:42 schrieb Ang, Chee Hong:
>> -----Original Message-----
>> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> Sent: Wednesday, March 11, 2020 12:17 AM
>> To: Ang, Chee Hong <chee.hong.ang at intel.com>
>> Cc: u-boot at lists.denx.de; Marek Vasut <marex at denx.de>; See, Chin Liang
>> <chin.liang.see at intel.com>; Tan, Ley Foon <ley.foon.tan at intel.com>;
>> Westergreen, Dalon <dalon.westergreen at intel.com>; Gong, Richard
>> <richard.gong at 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 at intel.com:
>>> From: Chee Hong Ang <chee.hong.ang at 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 at 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,
>>> +};
>>>
>