.Hi Andy, On 5 March 2017 at 12:17, Andy Shevchenko <andriy.shevche...@linux.intel.com> wrote: > From: Felipe Balbi <felipe.ba...@linux.intel.com> > > Intel MID platforms have few microcontrollers inside SoC, one of them is > so called System Controller Unit (SCU). > > Here is the driver to communicate with microcontroller. > > Signed-off-by: Vincent Tinelli <vincent.tine...@intel.com> > Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > --- > arch/x86/Kconfig | 1 + > drivers/misc/Kconfig | 6 ++ > drivers/misc/Makefile | 1 + > drivers/misc/intel_scu_ipc.c | 199 > +++++++++++++++++++++++++++++++++++++++++++ > include/intel_scu_ipc.h | 57 +++++++++++++ > 5 files changed, 264 insertions(+) > create mode 100644 drivers/misc/intel_scu_ipc.c > create mode 100644 include/intel_scu_ipc.h > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index dfdd7564ea..6a747c332e 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -83,6 +83,7 @@ endchoice > # subarchitectures-specific options below > config INTEL_MID > bool "Intel MID platform support" > + select INTEL_SCU > help > Select to build a U-Boot capable of supporting Intel MID > (Mobile Internet Device) platform systems which do not have > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 1aae4bcd07..8d81f9c51c 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -167,4 +167,10 @@ config I2C_EEPROM > depends on MISC > help > Enable a generic driver for EEPROMs attached via I2C. > + > +config INTEL_SCU > + bool "Enable support for SCU on Intel MID platforms" > + depends on INTEL_MID > + default y
Please add help explaining what SCU is and stands for, and perhaps MID also. > + > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index e3151ea097..47551e44d6 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -51,3 +51,4 @@ obj-$(CONFIG_PCA9551_LED) += pca9551_led.o > obj-$(CONFIG_FSL_DEVICE_DISABLE) += fsl_devdis.o > obj-$(CONFIG_WINBOND_W83627) += winbond_w83627.o > obj-$(CONFIG_QFW) += qfw.o > +obj-$(CONFIG_INTEL_SCU) += intel_scu_ipc.o > diff --git a/drivers/misc/intel_scu_ipc.c b/drivers/misc/intel_scu_ipc.c > new file mode 100644 > index 0000000000..19a99b0b68 > --- /dev/null > +++ b/drivers/misc/intel_scu_ipc.c > @@ -0,0 +1,199 @@ > +/* > + * Copyright (c) 2017 Intel Corporation > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > +#include <common.h> > +#include <dm.h> > +#include <dm/device-internal.h> > +#include <dm/device.h> > +#include <intel_scu_ipc.h> This should go after dm.h. (see http://www.denx.de/wiki/U-Boot/CodingStyle ) > +#include <linux/errno.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/sizes.h> > + > +#define IPC_STATUS_ADDR 0x04 > +#define IPC_SPTR_ADDR 0x08 > +#define IPC_DPTR_ADDR 0x0C > +#define IPC_READ_BUFFER 0x90 > +#define IPC_WRITE_BUFFER 0x80 > +#define IPC_IOC BIT(8) If these offsets don't change can we use a C struct to access the registers? > + > +struct intel_scu { > + void __iomem *base; > +}; > + > +/* Only one for now */ > +static struct intel_scu *the_scu; OK, but we cannot do this with driver model. It can be found using syscon_get_regmap_by_driver_data() perhaps? > + > +static void scu_writel(void __iomem *base, unsigned int offset, unsigned int > val) > +{ > + writel(val, base + offset); > +} > + > +static unsigned int scu_readl(void __iomem *base, unsigned int offset) > +{ > + return readl(base + offset); > +} > + > +/* > + * Command Register (Write Only): > + * A write to this register results in an interrupt to the SCU core processor > + * Format: > + * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)| Please can you use the standard function header format, something like: /* * intel_scu_ipc_send_command() - send a command to the SCU * * @scu: Pointer to SCU * @cmd: Command to send (defined by ..?) */ If there is a return value, the last line would be something like: @return torque propounder coefficient in mm > + */ > +static void intel_scu_ipc_send_command(struct intel_scu *scu, u32 cmd) > +{ > + scu_writel(scu->base, 0x00, cmd | IPC_IOC); > +} > + > +/* > + * IPC Write Buffer (Write Only): > + * 16-byte buffer for sending data associated with IPC command to > + * SCU. Size of the data is specified in the IPC_COMMAND_REG register > + */ > +static void ipc_data_writel(struct intel_scu *scu, u32 data, u32 offset) > +{ > + scu_writel(scu->base, IPC_WRITE_BUFFER + offset, data); > +} > + > +/* > + * Status Register (Read Only): > + * Driver will read this register to get the ready/busy status of the IPC > + * block and error status of the IPC command that was just processed by SCU > + * Format: > + * |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)| > + */ > +static u32 ipc_read_status(struct intel_scu *scu) > +{ > + return scu_readl(scu->base, IPC_STATUS_ADDR); > +} > + > +static u32 ipc_data_readl(struct intel_scu *scu, u32 offset) > +{ > + return scu_readl(scu->base, IPC_READ_BUFFER + offset); All of these functions take scu as a parameter. I wonder if they could take scu->regs, if you use a struct for the registers. > +} > + > +static int intel_scu_ipc_check_status(struct intel_scu *scu) > +{ > + int loop_count = 3000000; 3 million loops? Does that indicate some sort of problem? > + int status; > + > + do { > + status = ipc_read_status(scu); > + udelay(1); > + } while ((status & BIT(0)) && --loop_count); > + if (!loop_count) > + return -ETIMEDOUT; Given the long timeout, how about: start = timer_get_us(); while (1) { status = ... if (!(status & BUSY)) break; if ((int)(timer_get_us() - start) > 3000000) return -ETIMEDOUT; } > + > + if (status & BIT(1)) { > + printf("%s() status=0x%08x\n", __func__, status); > + return -EIO; > + } > + > + return 0; > +} > + > +static int intel_scu_ipc_raw_cmd(struct intel_scu *scu, u32 cmd, u32 sub, > + u8 *in, u8 inlen, u32 *out, u32 outlen, > + u32 dptr, u32 sptr) Function comment. Also try to avoid u32 for integer parameters. Use uint or ulong to avoid problems with 64-bit machines. > +{ > + int i, err; > + u32 wbuf[4]; > + > + if (inlen > 16) > + return -EINVAL; > + > + memcpy(wbuf, in, inlen); > + > + scu_writel(scu->base, IPC_DPTR_ADDR, dptr); > + scu_writel(scu->base, IPC_SPTR_ADDR, sptr); > + > + /** > + * SRAM controller don't support 8bit write, it only supports > + * 32bit write, so we have to write into the WBUF in 32bit, > + * and SCU FW will use the inlen to determine the actual input > + * data length in the WBUF. > + */ > + for (i = 0; i < DIV_ROUND_UP(inlen, 4); i++) > + ipc_data_writel(scu, wbuf[i], 4 * i); > + > + /** > + * Watchdog IPC command is an exception here using double word > + * as the unit of input data size because of some historical > + * reasons and SCU FW is doing so. > + */ > + if ((cmd & 0xff) == IPCMSG_WATCHDOG_TIMER) > + inlen = DIV_ROUND_UP(inlen, 4); > + > + intel_scu_ipc_send_command(scu, (inlen << 16) | (sub << 12) | cmd); > + err = intel_scu_ipc_check_status(scu); > + > + for (i = 0; i < outlen; i++) > + *out++ = ipc_data_readl(scu, 4 * i); > + > + return err; > +} > + > +/** > + * intel_scu_ipc_simple_command - send a simple command > + * @cmd: command > + * @sub: sub type > + * > + * Issue a simple command to the SCU. Do not use this interface if > + * you must then access data as any data values may be overwritten > + * by another SCU access by the time this function returns. > + * > + * This function may sleep. Locking for SCU accesses is handled for > + * the caller. It that true? > + */ > +int intel_scu_ipc_simple_command(int cmd, int sub) > +{ > + intel_scu_ipc_send_command(the_scu, sub << 12 | cmd); > + > + return intel_scu_ipc_check_status(the_scu); > +} > + > +int intel_scu_ipc_command(u32 cmd, u32 sub, u8 *in, u8 inlen, u32 *out, u32 > outlen) > +{ > + return intel_scu_ipc_raw_cmd(the_scu, cmd, sub, in, inlen, out, > outlen, 0, 0); > +} > + > +static int intel_scu_bind(struct udevice *dev) > +{ > + /* This device is needed straight away */ > + return device_probe(dev); > +} > + > +static int intel_scu_probe(struct udevice *dev) > +{ > + struct intel_scu *scu = dev_get_uclass_priv(dev); > + fdt_addr_t base; > + > + base = dev_get_addr(dev); > + if (base == FDT_ADDR_T_NONE) > + return -EINVAL; > + > + scu->base = devm_ioremap(dev, base, SZ_1K); > + if (!scu->base) > + return -ENOMEM; > + > + the_scu = scu; > + > + return 0; > +} > + > +static const struct udevice_id intel_scu_match[] = { > + { .compatible = "intel,scu-ipc" }, > + { /* sentinel */ } > +}; > + > +U_BOOT_DRIVER(intel_scu_ipc) = { > + .name = "intel_scu_ipc", > + .id = UCLASS_MISC, Consider UCLASS_SYSCON since it allows you to find the device using an enum. See 'intel_me_syscon' for example where the device can be found using X86_SYSCON_ME > + .of_match = intel_scu_match, > + .bind = intel_scu_bind, > + .probe = intel_scu_probe, > + .priv_auto_alloc_size = sizeof(struct intel_scu), > +}; > diff --git a/include/intel_scu_ipc.h b/include/intel_scu_ipc.h > new file mode 100644 > index 0000000000..ded6352e10 > --- /dev/null > +++ b/include/intel_scu_ipc.h > @@ -0,0 +1,57 @@ > +/* > + * Copyright (c) 2017 Intel Corporation > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > +#ifndef _INTEL_SCU_IPC_H_ > +#define _INTEL_SCU_IPC_H_ > + > +/* IPC defines the following message types */ > +#define IPCMSG_WARM_RESET 0xF0 > +#define IPCMSG_COLD_RESET 0xF1 > +#define IPCMSG_SOFT_RESET 0xF2 > +#define IPCMSG_COLD_BOOT 0xF3 > +#define IPCMSG_GET_FW_REVISION 0xF4 > +#define IPCMSG_WATCHDOG_TIMER 0xF8 /* Set Kernel Watchdog > Threshold */ > + > +#define IPC_ERR_NONE 0 > +#define IPC_ERR_CMD_NOT_SUPPORTED 1 > +#define IPC_ERR_CMD_NOT_SERVICED 2 > +#define IPC_ERR_UNABLE_TO_SERVICE 3 > +#define IPC_ERR_CMD_INVALID 4 > +#define IPC_ERR_CMD_FAILED 5 > +#define IPC_ERR_EMSECURITY 6 Could be an enum > + > +/* Command id associated with message IPCMSG_VRTC */ > +#define IPC_CMD_VRTC_SETTIME 1 /* Set time */ > +#define IPC_CMD_VRTC_SETALARM 2 /* Set alarm */ > +#define IPC_CMD_VRTC_SYNC_RTC 3 /* Sync MSIC/PMIC RTC to VRTC */ > + > +union ipc_ifwi_version { > + u8 raw[16]; > + struct { > + u16 ifwi_minor; > + u8 ifwi_major; > + u8 hardware_id; > + u32 reserved[3]; > + } fw __attribute__((packed)); > +} __attribute__((packed)); __packed. The inner packed does not seem to be useful. For the outer one, do you have multiple structs packed one after the other? If not, perhaps drop it? > + > +/* Issue commands to the SCU with or without data */ > +int intel_scu_ipc_simple_command(int cmd, int sub); > +int intel_scu_ipc_command(u32 cmd, u32 sub, u8 *in, u8 inlen, u32 *out, u32 > outlen); Full function comments. These functions should be passed a struct udevice for the device they use. > + > +enum intel_mid_cpu_type { > + INTEL_CPU_CHIP_NOTMID = 0, > + INTEL_MID_CPU_CHIP_LINCROFT, > + INTEL_MID_CPU_CHIP_PENWELL, > + INTEL_MID_CPU_CHIP_CLOVERVIEW, > + INTEL_MID_CPU_CHIP_TANGIER, > +}; > + > +static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void) > +{ > + return INTEL_MID_CPU_CHIP_TANGIER; > +} Is this identification a function of the SCU? > + > +#endif /* _INTEL_SCU_IPC_H_ */ > -- > 2.11.0 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot