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

[Marek Vasut]

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 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.

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 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. 

How does the __always_inline help with that ?
[...]
-- 
Best regards,
Marek Vasut