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....@intel.com wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> From: "Ang, Chee Hong" <chee.hong....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
