| Message ID | 20170305191713.49255-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
.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 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
On Sun, 2017-03-12 at 14:21 -0600, Simon Glass wrote: > > # 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 Thanks for review. My comments / answers below. > > +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, OK. > and perhaps MID also. MID is explained in INTEL_MID help, though I can decode the abbreviation in the explanation of SCU. > > @@ -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 ) Got it. > > +#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? Hmm... How would you think it will look like? > > + > > +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? I'm not familiar with such technique, can you elaborate a bit (perhaps link to some documentation)? > > +/* > > + * 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 OK. > > +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. Maybe. I dunno. > > +} > > + > > +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? SCU is a separate microcontroller inside SoC. It might take time for it to handle the request. 3m might be be big, but 500k is a least we would wait. > > + 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(); It makes it sleepable or what? I'm afraid of the case when it would be possible to send two sequential requests before getting an ACK from SCU. Is it possible case when we switch to time_get_us() ? > while (1) { Btw, I don't like at all "while (true)" style of loops. > status = ... > if (!(status & BUSY)) > break; > if ((int)(timer_get_us() - start) > 3000000) > return -ETIMEDOUT; > } > +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. They are register width / hardware related, so, I would rather not change them. > > +/** > > + * 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? What exactly? Locking? This might be a copy'n'paste from kernel and might need an adjustment. > > +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 OK. > > > + .of_match = intel_scu_match, > > + .bind = intel_scu_bind, > > + .probe = intel_scu_probe, > > + .priv_auto_alloc_size = sizeof(struct intel_scu), > > +}; > > +#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 OK. > > + > > +/* 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. OK. > 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? I dunno. There is very little documentation available and this code as you may notice is originally written not by me or Felipe, so, let's assume the worse case. > > > + > > +/* 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. I have no idea how they find the right device. Can you elaborate a bit? > > +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? Nope. It was previously done in a completely separate header. If you think it makes sense to leave it there, we can create a file [with, for my opinion, 20+ useless lines and few lines over the code].
Hi Andy, On 13 March 2017 at 08:05, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, 2017-03-12 at 14:21 -0600, Simon Glass wrote: >> > # 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 > > Thanks for review. My comments / answers below. > >> > +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, > > OK. > >> and perhaps MID also. > > MID is explained in INTEL_MID help, though I can decode the abbreviation > in the explanation of SCU. > >> > @@ -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 ) > > Got it. > >> > +#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? > > Hmm... How would you think it will look like? Perhaps something like this? struct ipc_regs { u32 reserved0; u32 sptr; u32 dptr; u8 reserved1[0x80 - 0x10]; u32 read[4]; u32 write[4]; }; > >> > + >> > +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? > > I'm not familiar with such technique, can you elaborate a bit (perhaps > link to some documentation)? With your header file you can provide an enum: enum { ROCKCHIP_SYSCON_NOC, ROCKCHIP_SYSCON_GRF, ROCKCHIP_SYSCON_SGRF, ROCKCHIP_SYSCON_PMU, ROCKCHIP_SYSCON_PMUGRF, ROCKCHIP_SYSCON_PMUSGRF, ROCKCHIP_SYSCON_CIC, }; In your driver: static const struct udevice_id rk3288_syscon_ids[] = { { .compatible = "rockchip,rk3288-noc", .data = ROCKCHIP_SYSCON_NOC }, { .compatible = "rockchip,rk3288-grf", .data = ROCKCHIP_SYSCON_GRF }, { .compatible = "rockchip,rk3288-sgrf", .data = ROCKCHIP_SYSCON_SGRF }, { .compatible = "rockchip,rk3288-pmu", .data = ROCKCHIP_SYSCON_PMU }, { } }; U_BOOT_DRIVER(syscon_rk3288) = { .name = "rk3288_syscon", .id = UCLASS_SYSCON, .of_match = rk3288_syscon_ids, }; The device tree has nodes with the above compatible strings, each of which becomes a syscon device. Then in the code somewhere you can call syscon_get_by_driver_data(ROCKCHIP_SYSCON_GRF..) to get access to that particular instance of the syscon_rk3288 driver. > >> > +/* >> > + * 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 > > OK. > >> > +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. > > Maybe. I dunno. > >> > +} >> > + >> > +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? > > SCU is a separate microcontroller inside SoC. It might take time for it > to handle the request. > > 3m might be be big, but 500k is a least we would wait. So that's at least half a second. Hopefully most systems would be fully booted by then! :-) Anyway I guess there is nothing you can do about this. > >> > + 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(); > > It makes it sleepable or what? I'm afraid of the case when it would be > possible to send two sequential requests before getting an ACK from SCU. > > Is it possible case when we switch to time_get_us() ? > >> while (1) { > > Btw, I don't like at all "while (true)" style of loops. OK, well you can perhaps adjust it to check the timeout instead the loop. But really you are running forever until a timeout or condition change. > >> status = ... >> if (!(status & BUSY)) >> break; >> if ((int)(timer_get_us() - start) > 3000000) >> return -ETIMEDOUT; >> } > >> +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. > > They are register width / hardware related, so, I would rather not > change them. OK well we can always deal with any issue later. > >> > +/** >> > + * 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? > > What exactly? Locking? Yes. > > This might be a copy'n'paste from kernel and might need an adjustment. > >> > +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 > > OK. > >> >> > + .of_match = intel_scu_match, >> > + .bind = intel_scu_bind, >> > + .probe = intel_scu_probe, >> > + .priv_auto_alloc_size = sizeof(struct intel_scu), >> > +}; > >> > +#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 > > OK. > >> > + >> > +/* 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. > > OK. > >> 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? > > I dunno. There is very little documentation available and this code as > you may notice is originally written not by me or Felipe, so, let's > assume the worse case. Well how about testing it without? It probably works fine. Having said that, on Intel CPUs there is not really much of a cost for unaligned things. > >> >> > + >> > +/* 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. > > I have no idea how they find the right device. Can you elaborate a bit? > >> > +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? > > Nope. It was previously done in a completely separate header. If you > think it makes sense to leave it there, we can create a file [with, for > my opinion, 20+ useless lines and few lines over the code]. Well I don't really understand what it is for, so I'll leave it to you. Regards, Simon
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 + 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> +#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) + +struct intel_scu { + void __iomem *base; +}; + +/* Only one for now */ +static struct intel_scu *the_scu; + +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)| + */ +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); +} + +static int intel_scu_ipc_check_status(struct intel_scu *scu) +{ + int loop_count = 3000000; + int status; + + do { + status = ipc_read_status(scu); + udelay(1); + } while ((status & BIT(0)) && --loop_count); + if (!loop_count) + 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) +{ + 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. + */ +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, + .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 + +/* 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)); + +/* 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); + +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; +} + +#endif /* _INTEL_SCU_IPC_H_ */