[U-Boot] [PATCH v1 1/2] misc: Add SCU IPC driver for Intel MID platforms
Simon Glass
sjg at chromium.org
Sun Mar 12 20:21:37 UTC 2017
.Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
> From: Felipe Balbi <felipe.balbi at 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 at intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi at linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko at 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
More information about the U-Boot
mailing list