Message ID | 1535987232-5588-2-git-send-email-georgii.staroselskii@emlid.com |
---|---|
State | Superseded |
Delegated to: | Bin Meng |
Headers | show |
Series | Add a pinctrl driver for Merrifield to pinmux I2C#6 | expand |
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 ... 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? > > #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
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.
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); #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); + + /* + * 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
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. 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(+)