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@gmail.com wrote:
On Mon, Sep 3, 2018 at 7:40 PM Georgii Staroselskii georgii.staroselskii@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@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@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.