[U-Boot] [PATCH v1 1/2] misc: Add SCU IPC driver for Intel MID platforms

[Andy Shevchenko]

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].

-- 
Andy Shevchenko <andriy.shevchenko at linux.intel.com>
Intel Finland Oy