Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing

[Stanimir Varbanov <stanimir.varbanov@linaro.org>]

Hi Olof,

On 06/19/2017 02:25 PM, Stanimir Varbanov wrote:
> Hi Olof,
> 
> On 06/19/2017 08:35 AM, Olof Johansson wrote:
>> Hi,
>>
>> On Thu, Jun 8, 2017 at 8:23 AM, Stanimir Varbanov
>> <stanimir.varbanov@linaro.org> wrote:
>>> Unfortunatly previous attempt to allow consumer drivers to
>>> use COMPILE_TEST option in Kconfig is not enough, because in the
>>> past the consumer drivers used 'depends on' Kconfig option but
>>> now they are using 'select' Kconfig option which means on non ARM
>>> arch'es compilation is triggered. Thus we need to move the ifdefery
>>> one level below by touching the private qcom_scm.h header.
>>>
>>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>> ---
>>> This is second version of the patch with comments addressed:
>>>  * proper identation for static inline functions
>>>  * to avoid duplicating defines group them on top of the header
>>>
>>> The first version has been part of the venus driver patchset
>>> v8 and can be found at:
>>> https://patchwork.kernel.org/patch/9704275/
>>>
>>>  drivers/firmware/Kconfig    |   2 +-
>>>  drivers/firmware/qcom_scm.h | 122 ++++++++++++++++++++++++++++++++++++--------
>>>  include/linux/qcom_scm.h    |  32 ------------
>>>  3 files changed, 101 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index 6e4ed5a9c6fd..480578c3691a 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -204,7 +204,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>
>>>  config QCOM_SCM
>>>         bool
>>> -       depends on ARM || ARM64
>>> +       depends on ARM || ARM64 || COMPILE_TEST
>>>         select RESET_CONTROLLER
>>>
>>>  config QCOM_SCM_32
>>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>>> index 9bea691f30fb..60689fc8a567 100644
>>> --- a/drivers/firmware/qcom_scm.h
>>> +++ b/drivers/firmware/qcom_scm.h
>>> @@ -16,31 +16,20 @@
>>>  #define QCOM_SCM_BOOT_ADDR             0x1
>>>  #define QCOM_SCM_BOOT_ADDR_MC          0x11
>>>  #define QCOM_SCM_SET_REMOTE_STATE      0xa
>>> -extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>>
>>>  #define QCOM_SCM_FLAG_HLOS             0x01
>>>  #define QCOM_SCM_FLAG_COLDBOOT_MC      0x02
>>>  #define QCOM_SCM_FLAG_WARMBOOT_MC      0x04
>>> -extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> -               const cpumask_t *cpus);
>>> -extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>>
>>>  #define QCOM_SCM_CMD_TERMINATE_PC      0x2
>>>  #define QCOM_SCM_FLUSH_FLAG_MASK       0x3
>>>  #define QCOM_SCM_CMD_CORE_HOTPLUGGED   0x10
>>> -extern void __qcom_scm_cpu_power_down(u32 flags);
>>>
>>>  #define QCOM_SCM_SVC_INFO              0x6
>>>  #define QCOM_IS_CALL_AVAIL_CMD         0x1
>>> -extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> -               u32 cmd_id);
>>>
>>>  #define QCOM_SCM_SVC_HDCP              0x11
>>>  #define QCOM_SCM_CMD_HDCP              0x01
>>> -extern int __qcom_scm_hdcp_req(struct device *dev,
>>> -               struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> -
>>> -extern void __qcom_scm_init(void);
>>>
>>>  #define QCOM_SCM_SVC_PIL               0x2
>>>  #define QCOM_SCM_PAS_INIT_IMAGE_CMD    0x1
>>> @@ -49,6 +38,27 @@ extern void __qcom_scm_init(void);
>>>  #define QCOM_SCM_PAS_SHUTDOWN_CMD      0x6
>>>  #define QCOM_SCM_PAS_IS_SUPPORTED_CMD  0x7
>>>  #define QCOM_SCM_PAS_MSS_RESET         0xa
>>> +
>>> +#define QCOM_SCM_SVC_MP                        0xc
>>> +#define QCOM_SCM_RESTORE_SEC_CFG       2
>>> +
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE        3
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT        4
>>> +
>>> +#define QCOM_SCM_SVC_HDCP              0x11
>>> +#define QCOM_SCM_CMD_HDCP              0x01
>>> +
>>> +#if IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64)
>>
>> This isn't right -- this should be IS_ENABLED(QCOM_SCM)
> 
> The idea was to use the __qcom_scm_xxx functions only on ARM and ARM64
> and fallback to the empty stubs on every other architecture.
> 
>>
>>> +extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>> +extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> +               const cpumask_t *cpus);
>>> +extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>> +extern void __qcom_scm_cpu_power_down(u32 flags);
>>> +extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> +               u32 cmd_id);
>>> +extern int __qcom_scm_hdcp_req(struct device *dev,
>>> +               struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> +extern void __qcom_scm_init(void);
>>>  extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
>>>  extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>>                 dma_addr_t metadata_phys);
>>> @@ -57,6 +67,85 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>>  extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>>>  extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>>>  extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>>> +extern int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> +                                     u32 spare);
>>> +extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
>>> +                                            size_t *size);
>>> +extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
>>> +                                            u32 size, u32 spare);
>>> +#else  /* !ARM and !ARM64 */
>>> +static inline int __qcom_scm_set_remote_state(struct device *dev, u32 state,
>>> +                                             u32 id)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> +                                               const cpumask_t *cpus)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_cold_boot_addr(void *entry,
>>> +                                               const cpumask_t *cpus)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_cpu_power_down(u32 flags) {}
>>> +static inline int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> +                                              u32 cmd_id)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_hdcp_req(struct device *dev,
>>> +                                     struct qcom_scm_hdcp_req *req,
>>> +                                     u32 req_cnt, u32 *resp)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_init(void) {}
>>> +static inline bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral)
>>> +{
>>> +       return false;
>>> +}
>>> +static inline int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>> +                                            dma_addr_t metadata_phys)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>> +                                           phys_addr_t addr, phys_addr_t size)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int  __qcom_scm_pas_auth_and_reset(struct device *dev,
>>> +                                                u32 peripheral)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> +                                            u32 spare)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_size(struct device *dev,
>>> +                                                   u32 spare, size_t *size)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_init(struct device *dev,
>>> +                                                   u64 addr, u32 size,
>>> +                                                   u32 spare)
>>> +{
>>> +       return -ENODEV;
>>> +}
>>
>> The amount of boilerplate random stubs here indiciates that the
>> abstraction point for this is wrong.
> 
> This was the best solution I came to.
> 
>>
>> Why are you looking to COMPILE_TEST a very platform-tied driver like
>> this on other architectures?
> 
> In fact the aim is to make possible to COMPILE_TEST drivers which are
> using QCOM_SCM interface. So that qcom_scm driver itself doesn't need
> compile test coverage, and compile testing qcom_scm is impossible on
> non-ARM architectures.
> 
>>
>> It probably makes more sense to stub the driver->scm interface than
>> the internal scm interface if what you're looking for is driver
>> compile_test coverage.
> 
> Actually, this is the state of qcom_scm if we don't apply the this
> patch, and it didn't help. Thinking more on that it looks like that
> adding COMPILE_TEST in 'config QCOM_SCM' is controversial.
> 
> Arnd, Andy any ideas how to proceed. If this patch is not get merged
> (and I/we cannot find better solution) the video driver for qualcomm
> platforms will be rejected for 4.13.
> 

Currently the dependences are:

VIDEO_QCOM_VENUS selects QCOM_MDT_LOADER
QCOM_MDT_LOADER selects QCOM_SCM

And I came to this,

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 9fca977ef18d..b8657c561eae 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -12,7 +12,7 @@ config QCOM_GSBI

 config QCOM_MDT_LOADER
        tristate
-       select QCOM_SCM
+       depends on QCOM_SCM || COMPILE_TEST

 config QCOM_PM
        bool "Qualcomm Power Management"


-- 
regards,
Stan