On Tue, Sep 4, 2018 at 2:23 AM Andy Shevchenko
<andy.shevche...@gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii
> <georgii.starosels...@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.shevche...@gmail.com>
Somehow I did not receive the original patch email ...
Please see below
>
> > Signed-off-by: Georgii Staroselskii <georgii.starosels...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
