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

[Georgii Staroselskii]

On Tue, Sep 04, 2018 at 12:37:49PM +0800, Bin Meng wrote:
> 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 ...

Sorry for that. Not very experienced in using mailing lists. I hope
you'll get this one.
> 
> 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?

Sure. Will do it in the .c, if you don't mind.
> 
> > >  #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?

These two functions seem to be so simple that a refactoring to make use
of the common code might actually make them more complex than they need
to be. So I propose to leave them be. But there's a chance I don't just
see a clear decomposition.
> 
> > > +       /*
> > > +        * 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

Thanks for your comments. I'll send a v2 once I address everything.