Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753995AbdFSLZw (ORCPT ); Mon, 19 Jun 2017 07:25:52 -0400 Received: from mail-wr0-f169.google.com ([209.85.128.169]:34394 "EHLO mail-wr0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753830AbdFSLZu (ORCPT ); Mon, 19 Jun 2017 07:25:50 -0400 Subject: Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing To: Olof Johansson , Andy Gross , Arnd Bergmann References: <1496935418-14142-1-git-send-email-stanimir.varbanov@linaro.org> Cc: "linux-kernel@vger.kernel.org" , linux-arm-msm , Stephen Boyd , Bjorn Andersson From: Stanimir Varbanov Message-ID: <9e0b85dd-3621-cd8c-7ec6-67e319cab071@linaro.org> Date: Mon, 19 Jun 2017 14:25:47 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9353 Lines: 228 Hi Olof, On 06/19/2017 08:35 AM, Olof Johansson wrote: > Hi, > > On Thu, Jun 8, 2017 at 8:23 AM, Stanimir Varbanov > 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 >> Cc: Bjorn Andersson >> Signed-off-by: Stanimir Varbanov >> --- >> 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. -- regards, Stan