[U-Boot] [PATCH 1/4] x86: cpu: introduce scu_ipc_raw_command()

[Bin Meng]

On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko
<andy.shevchenko at gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii
> <georgii.staroselskii at emlid.com> wrote:
> >
> > This interface will be used to configure properly some pins on
> > Merrifield that are shared with SCU.
> >
> > scu_ipc_raw_command() writes SPTR and DPTR registers before sending
> > a command to SCU.
> >
> > This code has been ported from Linux work done by Andy Shevchenko.
> >
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko at gmail.com>

Somehow I did not receive the original patch email ...

Please see below

>
> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii at emlid.com>
> > ---
> >  arch/x86/include/asm/scu.h |  4 ++++
> >  arch/x86/lib/scu.c         | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/scu.h b/arch/x86/include/asm/scu.h
> > index 7ce5824..f5ec5a1 100644
> > --- a/arch/x86/include/asm/scu.h
> > +++ b/arch/x86/include/asm/scu.h
> > @@ -6,6 +6,8 @@
> >  #define _X86_ASM_SCU_IPC_H_
> >
> >  /* IPC defines the following message types */
> > +#define IPCMSG_INDIRECT_READ   0x02
> > +#define IPCMSG_INDIRECT_WRITE  0x05
> >  #define IPCMSG_WARM_RESET      0xf0
> >  #define IPCMSG_COLD_RESET      0xf1
> >  #define IPCMSG_SOFT_RESET      0xf2
> > @@ -23,5 +25,7 @@ struct ipc_ifwi_version {
> >  /* Issue commands to the SCU with or without data */
> >  int scu_ipc_simple_command(u32 cmd, u32 sub);
> >  int scu_ipc_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out, int outlen);
> > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > +                       int outlen, u32 dptr, u32 sptr);
> >

Can we also add the complete function header with comments that
describe the parameters and return value? I see
scu_ipc_simple_command() has one in the .c file, but scu_ipc_command()
does not. For consistency, either we document the API in the .c, or
move the comment block to the .h?

> >  #endif /* _X86_ASM_SCU_IPC_H_ */
> > diff --git a/arch/x86/lib/scu.c b/arch/x86/lib/scu.c
> > index caa04c6..847bb77 100644
> > --- a/arch/x86/lib/scu.c
> > +++ b/arch/x86/lib/scu.c
> > @@ -101,6 +101,41 @@ static int scu_ipc_cmd(struct ipc_regs *regs, u32 cmd, u32 sub,
> >         return err;
> >  }
> >
> > +int scu_ipc_raw_command(u32 cmd, u32 sub, u32 *in, int inlen, u32 *out,
> > +                       int outlen, u32 dptr, u32 sptr)
> > +{
> > +       int inbuflen = DIV_ROUND_UP(inlen, 4);
> > +       struct udevice *dev;
> > +       struct scu *scu;
> > +       int ret;
> > +
> > +       ret = syscon_get_by_driver_data(X86_SYSCON_SCU, &dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       scu = dev_get_priv(dev);
> > +
> > +       /* Up to 16 bytes */
> > +       if (inbuflen > 4)
> > +               return -EINVAL;
> > +
> > +       writel(dptr, &scu->regs->dptr);
> > +       writel(sptr, &scu->regs->sptr);
> > +

It looks like that this new API shares some common codes with existing
API scu_ipc_command(). Is it possible to do some refactoring?

> > +       /*
> > +        * SRAM controller doesn't support 8-bit writes, it only
> > +        * supports 32-bit writes, so we have to copy input data into
> > +        * the temporary buffer, and SCU FW will use the inlen to
> > +        * determine the actual input data length in the temporary
> > +        * buffer.
> > +        */
> > +
> > +       u32 inbuf[4] = {0};
> > +
> > +       memcpy(inbuf, in, inlen);
> > +
> > +       return scu_ipc_cmd(scu->regs, cmd, sub, inbuf, inlen, out, outlen);
> > +}
> >  /**
> >   * scu_ipc_simple_command() - send a simple command
> >   * @cmd: command

Regards,
Bin