Hi Andy, On 13 March 2017 at 08:05, Andy Shevchenko <andriy.shevche...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot