Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing
From: Stanimir Varbanov
Date: Mon Jun 19 2017 - 08:20:05 EST
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@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)
>
> 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