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

From: Olof Johansson
Date: Mon Jun 19 2017 - 01:35:29 EST

Hi,

On Thu, Jun 8, 2017 at 8:23 AM, Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> ---
> 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)

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

Why are you looking to COMPILE_TEST a very platform-tied driver like
this on other 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.



-Olof