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
