* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
@ 2018-09-26 9:03 chee.hong.ang at intel.com
2018-09-26 14:53 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: chee.hong.ang at intel.com @ 2018-09-26 9:03 UTC (permalink / raw)
To: u-boot
From: "Ang, Chee Hong" <chee.hong.ang@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.
Signed-off-by: Ang, Chee Hong <chee.hong.ang@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)
+{
+ 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) {
+ /* 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_STATE])
+ 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)
{
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-26 9:03 [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10 chee.hong.ang at intel.com
@ 2018-09-26 14:53 ` Marek Vasut
2018-09-27 5:08 ` Ang, Chee Hong
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-09-26 14:53 UTC (permalink / raw)
To: u-boot
On 09/26/2018 11:03 AM, chee.hong.ang at intel.com wrote:
> From: "Ang, Chee Hong" <chee.hong.ang@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 ?
> Signed-off-by: Ang, Chee Hong <chee.hong.ang@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 ?
> +{
> + 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;
...
> + /* 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_STATE])
> + 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)
> {
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-26 14:53 ` Marek Vasut
@ 2018-09-27 5:08 ` Ang, Chee Hong
2018-09-27 6:21 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Ang, Chee Hong @ 2018-09-27 5:08 UTC (permalink / raw)
To: u-boot
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@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.
>
> >
> > Signed-off-by: Ang, Chee Hong <chee.hong.ang@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.
>
> >
> > +{
> > + 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_STATE])
> > + 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)
> > {
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-27 5:08 ` Ang, Chee Hong
@ 2018-09-27 6:21 ` Marek Vasut
2018-09-27 6:37 ` Ang, Chee Hong
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-09-27 6:21 UTC (permalink / raw)
To: u-boot
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@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/ ?
Also, don't add dead code, just submit this alongside the user of this code.
>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@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.
>>>
>>> +{
>>> + 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_STATE])
>>> + 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)
>>> {
>>>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-27 6:21 ` Marek Vasut
@ 2018-09-27 6:37 ` Ang, Chee Hong
2018-09-27 20:39 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Ang, Chee Hong @ 2018-09-27 6:37 UTC (permalink / raw)
To: u-boot
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@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@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)
> > > > {
> > > >
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-27 6:37 ` Ang, Chee Hong
@ 2018-09-27 20:39 ` Marek Vasut
2018-09-28 9:29 ` Ang, Chee Hong
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-09-27 20:39 UTC (permalink / raw)
To: u-boot
On 09/27/2018 08:37 AM, Ang, Chee Hong wrote:
> 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@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.
SoCFPGA dwmac support is already upstream. Can you explain why it needs
to query the FPGA at all ? Is the whole dwmac in the FPGA ? Or is the
dwmac just routed through FPGA ?
> 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.
I'm not really fond of accepting dead code, on which code I didn't even
see and design that's unclear depends.
>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@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.
How does the __always_inline help with that ?
[...]
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-27 20:39 ` Marek Vasut
@ 2018-09-28 9:29 ` Ang, Chee Hong
2018-09-30 8:13 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Ang, Chee Hong @ 2018-09-28 9:29 UTC (permalink / raw)
To: u-boot
On Thu, 2018-09-27 at 22:39 +0200, Marek Vasut wrote:
> On 09/27/2018 08:37 AM, Ang, Chee Hong wrote:
> >
> > 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@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.
> SoCFPGA dwmac support is already upstream. Can you explain why it
> needs
> to query the FPGA at all ? Is the whole dwmac in the FPGA ? Or is the
> dwmac just routed through FPGA ?
dwmac driver need to call this function to access PCS (MAC PHY) which
is routed through FPGA.
>
> >
> > 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.
> I'm not really fond of accepting dead code, on which code I didn't
> even
> see and design that's unclear depends.
I understand. Can I submit another FPGA driver patchsets and refer this
patch as the dependency ? The FPGA driver patchsets is the user of the
common function.
>
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@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.
> How does the __always_inline help with that ?
mbox_get_fpga_config_status() (in .code section)
mbox_get_fpga_config_status_psci() (in __secure section)
Both functions need to implement mbox_get_fpga_config_status_common().
So we clone this mbox_get_fpga_config_status_common() into
mbox_get_fpga_config_status() & mbox_get_fpga_config_status_psci()
without duplicating the same code in those 2 functions.
mbox_get_fpga_config_status() exist when u-boot is active. So this
function is callable by u-boot itself.
mbox_get_fpga_config_status_psci() exist after u-boot boot the OS. U-
boot will actually copy this function to another region of memory which
mostly consist of PSCI/SMC handlers. These services are called by OS to
do certain stuffs like reboot, access secure hardware and etc. So is
actually same piece of code that need to exist at 2 different stage.
arch/arm/mach-socfpga/mailbox_s10.c already has a couple of other
functions which also use this method to make the same piece of code
exist in 2 different regions.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10
2018-09-28 9:29 ` Ang, Chee Hong
@ 2018-09-30 8:13 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2018-09-30 8:13 UTC (permalink / raw)
To: u-boot
On 09/28/2018 11:29 AM, Ang, Chee Hong wrote:
> On Thu, 2018-09-27 at 22:39 +0200, Marek Vasut wrote:
>> On 09/27/2018 08:37 AM, Ang, Chee Hong wrote:
>>>
>>> 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@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.
>> SoCFPGA dwmac support is already upstream. Can you explain why it
>> needs
>> to query the FPGA at all ? Is the whole dwmac in the FPGA ? Or is the
>> dwmac just routed through FPGA ?
> dwmac driver need to call this function to access PCS (MAC PHY) which
> is routed through FPGA.
Then shouldn't this be modeled in the DT ?
And if it is properly modeled in the DT, the FPGA driver code should do
any such checking, not the EMAC code.
>>> 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.
>> I'm not really fond of accepting dead code, on which code I didn't
>> even
>> see and design that's unclear depends.
> I understand. Can I submit another FPGA driver patchsets and refer this
> patch as the dependency ? The FPGA driver patchsets is the user of the
> common function.
Yes please
>>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@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.
>> How does the __always_inline help with that ?
> mbox_get_fpga_config_status() (in .code section)
> mbox_get_fpga_config_status_psci() (in __secure section)
> Both functions need to implement mbox_get_fpga_config_status_common().
> So we clone this mbox_get_fpga_config_status_common() into
> mbox_get_fpga_config_status() & mbox_get_fpga_config_status_psci()
> without duplicating the same code in those 2 functions.
This seems like a really hacky way to do it. Wouldn't it make more sense
to use some linker script trick for this ?
> mbox_get_fpga_config_status() exist when u-boot is active. So this
> function is callable by u-boot itself.
> mbox_get_fpga_config_status_psci() exist after u-boot boot the OS. U-
> boot will actually copy this function to another region of memory which
> mostly consist of PSCI/SMC handlers. These services are called by OS to
> do certain stuffs like reboot, access secure hardware and etc. So is
> actually same piece of code that need to exist at 2 different stage.
OK
> arch/arm/mach-socfpga/mailbox_s10.c already has a couple of other
> functions which also use this method to make the same piece of code
> exist in 2 different regions.
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-30 8:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-26 9:03 [U-Boot] [PATCH] arm: socfpga: stratix10: Add generic FPGA reconfig mailbox API for S10 chee.hong.ang at intel.com
2018-09-26 14:53 ` Marek Vasut
2018-09-27 5:08 ` Ang, Chee Hong
2018-09-27 6:21 ` Marek Vasut
2018-09-27 6:37 ` Ang, Chee Hong
2018-09-27 20:39 ` Marek Vasut
2018-09-28 9:29 ` Ang, Chee Hong
2018-09-30 8:13 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox