[U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10

[Ang, Chee Hong]

On Thu, 2018-09-27 at 08:21 +0200, Marek Vasut wrote:
> On 09/27/2018 07:08 AM, Ang, Chee Hong wrote:
> > 
> > On Wed, 2018-09-26 at 16:53 +0200, Marek Vasut wrote:
> > > 
> > > On 09/26/2018 11:03 AM, chee.hong.ang at intel.com wrote:
> > > > 
> > > > 
> > > > From: "Ang, Chee Hong" <chee.hong.ang at intel.com>
> > > > 
> > > > Add a generic mailbox API for FPGA reconfig status which can be
> > > > called by others. This new function accepts 2 different mailbox
> > > > commands: CONFIG_STATUS or RECONFIG_STATUS.
> > > What "others" can call this ?
> > This is a common function used to check the readiness of the FPGA.
> > FPGA configuration drivers will need to call this to make sure
> > the FPGA configuration is successfully completed and running.
> > These FPGA configuration drivers will be submitted soon after this
> > patch.
> So this should be in drivers/fpga/ ?
Yes. There is a FPGA configuration driver under this folder. Will
submit soon.
> 
> Also, don't add dead code, just submit this alongside the user of
> this code.
The reason I try to get this common function upstream is because my
colleague is trying to upstream socfpga dwmac driver which also need to
call this function to check whether the soft Ip running on FPGA for the
dwmac driver is up and running. If I bundle this code alongside with my
FPGA configuration driver, it might take longer time to get into the
mainline. So this common function is blocking other to upstream his/her
code.
> 
> > 
> > > 
> > > > 
> > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang at intel.com>
> > > > ---
> > > >  arch/arm/mach-socfpga/include/mach/mailbox_s10.h |  3 +-
> > > >  arch/arm/mach-socfpga/mailbox_s10.c              | 48
> > > > ++++++++++++++++++++++++
> > > >  2 files changed, 50 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/arm/mach-socfpga/include/mach/mailbox_s10.h
> > > > b/arch/arm/mach-socfpga/include/mach/mailbox_s10.h
> > > > index 81a609d..660df35 100644
> > > > --- a/arch/arm/mach-socfpga/include/mach/mailbox_s10.h
> > > > +++ b/arch/arm/mach-socfpga/include/mach/mailbox_s10.h
> > > > @@ -140,5 +140,6 @@ int mbox_qspi_open(void);
> > > >  #endif
> > > >  
> > > >  int mbox_reset_cold(void);
> > > > -
> > > > +int mbox_get_fpga_config_status(u32 cmd);
> > > > +int mbox_get_fpga_config_status_psci(u32 cmd);
> > > >  #endif /* _MAILBOX_S10_H_ */
> > > > diff --git a/arch/arm/mach-socfpga/mailbox_s10.c
> > > > b/arch/arm/mach-
> > > > socfpga/mailbox_s10.c
> > > > index 0d906c3..345485c 100644
> > > > --- a/arch/arm/mach-socfpga/mailbox_s10.c
> > > > +++ b/arch/arm/mach-socfpga/mailbox_s10.c
> > > > @@ -342,6 +342,54 @@ int mbox_reset_cold(void)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/* Accepted commands: CONFIG_STATUS or RECONFIG_STATUS */
> > > > +static __always_inline int
> > > > mbox_get_fpga_config_status_common(u32
> > > > cmd)
> > > Why __always_inline ?
> > This function is needed in 2 sections. Our u-boot run in normal
> > code
> > section and '__secure' section which mainly used for PSCI services.
> > Refer to the 'mbox_get_fpga_config_status()' and
> > 'mbox_get_fpga_config_status_psci()' below. These 2 functions run
> > in 2
> > different sections and they need to call this function.
> But why does this need to be __always_inline ? The compiler can
> decide
> what's best.
This is dangerous. Let me explain in more details why we need this,
'__secure' section and normal '.code' section exist in different time.
'.code' section no longer valid once u-boot boot to OS, but '__secure'
section still exist after booting to OS because OS need to call PSCI
services to achieve certain things. We need to make sure both sections
contain the full code, therefore we need to enforce the compiler to
duplicate this piece of code to both sections as well. 
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +{
> > > > +	u32 reconfig_status_resp_len;
> > > > +	u32
> > > > reconfig_status_resp[RECONFIG_STATUS_RESPONSE_LEN];
> > > > +	int ret;
> > > > +
> > > > +	reconfig_status_resp_len =
> > > > RECONFIG_STATUS_RESPONSE_LEN;
> > > > +	ret = mbox_send_cmd_common(MBOX_ID_UBOOT, cmd,
> > > > +				   MBOX_CMD_DIRECT, 0, NULL,
> > > > 0,
> > > > +				   &reconfig_status_resp_len,
> > > > +				   reconfig_status_resp);
> > > > +	if (!ret) {
> > > if (ret)
> > >  return ret;
> > Will be addressed in v2 patch.
> > > 
> > > 
> > > ...
> > > 
> > > > 
> > > > 
> > > > +		/* Check for any error */
> > > > +		ret =
> > > > reconfig_status_resp[RECONFIG_STATUS_STATE];
> > > > +		if (ret && ret != MBOX_CFGSTAT_STATE_CONFIG)
> > > > +			return ret;
> > > > +
> > > > +		/* Make sure nStatus is not 0 */
> > > > +		ret =
> > > > reconfig_status_resp[RECONFIG_STATUS_PIN_STATUS];
> > > > +		if (!(ret & RCF_PIN_STATUS_NSTATUS))
> > > > +			return
> > > > MBOX_CFGSTAT_STATE_ERROR_HARDWARE;
> > > > +
> > > > +		ret =
> > > > reconfig_status_resp[RECONFIG_STATUS_SOFTFUNC_STATUS];
> > > > +		if (ret & RCF_SOFTFUNC_STATUS_SEU_ERROR)
> > > > +			return
> > > > MBOX_CFGSTAT_STATE_ERROR_HARDWARE;
> > > > +
> > > > +		if ((ret & RCF_SOFTFUNC_STATUS_CONF_DONE) &&
> > > > +		    (ret & RCF_SOFTFUNC_STATUS_INIT_DONE) &&
> > > > +		    !reconfig_status_resp[RECONFIG_STATUS_STAT
> > > > E])
> > > > +			return 0;	/* configuration
> > > > success
> > > > */
> > > > +		else
> > > > +			return MBOX_CFGSTAT_STATE_CONFIG;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +int mbox_get_fpga_config_status(u32 cmd)
> > > > +{
> > > > +	return mbox_get_fpga_config_status_common(cmd);
> > > > +}
> > > > +
> > > > +int __secure mbox_get_fpga_config_status_psci(u32 cmd)
> > > > +{
> > > > +	return mbox_get_fpga_config_status_common(cmd);
> > > > +}
> > > > +
> > > >  int mbox_send_cmd(u8 id, u32 cmd, u8 is_indirect, u32 len, u32
> > > > *arg,
> > > >  		  u8 urgent, u32 *resp_buf_len, u32 *resp_buf)
> > > >  {
> > > > 
>