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

Message ID 1496935418-14142-1-git-send-email-stanimir.varbanov@linaro.org
State New
Headers show

Commit Message

Stanimir Varbanov June 8, 2017, 3:23 p.m.
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(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Olof Johansson June 19, 2017, 5:35 a.m. | #1
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)

> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov June 19, 2017, 11:25 a.m. | #2
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.

-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov June 19, 2017, 12:19 p.m. | #3
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,



-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.htmldiff --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"

Bjorn Andersson June 19, 2017, 9 p.m. | #4
On Mon 19 Jun 05:19 PDT 2017, Stanimir Varbanov wrote:

> 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

[..]
> >>

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


The problem with this is that QCOM_SCM is not user selectable and you
can't select MDT_LOADER if you make it depend on QCOM_SCM - as this
might not be enabled by the user (if you make it user selectable).


The problem that I see changing this is that there's no point in making
QCOM_SCM and QCOM_MDT_LOADER user selectable - they don't serve a
purpose on their own.


PS. Don't you depend on scm functionality directly from the venus driver
as well?

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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)
+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;
+}
+#endif
 
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
@@ -85,15 +174,4 @@  static inline int qcom_scm_remap_error(int err)
 	return -EINVAL;
 }
 
-#define QCOM_SCM_SVC_MP			0xc
-#define QCOM_SCM_RESTORE_SEC_CFG	2
-extern int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
-				      u32 spare);
-#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE	3
-#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT	4
-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);
-
 #endif
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index e5380471c2cd..b628f735f355 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -23,7 +23,6 @@  struct qcom_scm_hdcp_req {
 	u32 val;
 };
 
-#if IS_ENABLED(CONFIG_QCOM_SCM)
 extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
 extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
 extern bool qcom_scm_is_available(void);
@@ -43,35 +42,4 @@  extern int qcom_scm_set_remote_state(u32 state, u32 id);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
 extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
 extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
-#else
-static inline
-int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
-{
-	return -ENODEV;
-}
-static inline
-int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus)
-{
-	return -ENODEV;
-}
-static inline bool qcom_scm_is_available(void) { return false; }
-static inline bool qcom_scm_hdcp_available(void) { return false; }
-static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
-				    u32 *resp) { return -ENODEV; }
-static inline bool qcom_scm_pas_supported(u32 peripheral) { return false; }
-static inline int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
-					  size_t size) { return -ENODEV; }
-static inline int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
-					 phys_addr_t size) { return -ENODEV; }
-static inline int
-qcom_scm_pas_auth_and_reset(u32 peripheral) { return -ENODEV; }
-static inline int qcom_scm_pas_shutdown(u32 peripheral) { return -ENODEV; }
-static inline void qcom_scm_cpu_power_down(u32 flags) {}
-static inline u32 qcom_scm_get_version(void) { return 0; }
-static inline u32
-qcom_scm_set_remote_state(u32 state,u32 id) { return -ENODEV; }
-static inline int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare) { return -ENODEV; }
-static inline int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size) { return -ENODEV; }
-static inline int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare) { return -ENODEV; }
-#endif
 #endif