.Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
From: Felipe Balbi felipe.balbi@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.tinelli@intel.com Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@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 havediff --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_MIDdefault 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