diff mbox

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

Message ID 58ebc743-953b-e2f4-08ca-fa1642f27a5b@linaro.org (mailing list archive)
State Deferred
Delegated to: Andy Gross
Headers show

Commit Message

Stanimir Varbanov June 19, 2017, 12:19 p.m. UTC 
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,

Comments

Bjorn Andersson June 19, 2017, 9 p.m. UTC | #1 
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
Stanimir Varbanov June 20, 2017, 8:43 a.m. UTC | #2 
Hi Bjorn,

On 06/20/2017 12:00 AM, Bjorn Andersson wrote:
> 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?

Yes, qcom video codec driver depends directly on QCOM_SCM. But I'm
relying on the fact that QCOM_MDT_LOADER depends on QCOM_SCM, and
probably QCOM_MDT_LOADER will need to depends on QCOM_SCM forever.
diff mbox

Patch

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"