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

[Marek Vasut]

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