Re: [PATCH v2 01/18] firmware: qcom_scm: Rename macros and structures

[Stephen Boyd]

Quoting Elliot Berman (2019-11-12 13:22:37)
> Rename legacy-specific structures and macros with legacy prefix; rename
> smccc-specific structures and macros with smccc prefix.

Yes that's what's happening in the patch, but there isn't the most
important part in the commit text here, which is _why_ this change is
meaningful. Presumably the reason is to clearly separate the original
SCM calling convention from the newer SCM calling conventions.

> 
> Signed-off-by: Elliot Berman <eberman@xxxxxxxxxxxxxx>
> ---
>  drivers/firmware/qcom_scm-32.c | 70 ++++++++++++++++++++++--------------------
>  drivers/firmware/qcom_scm-64.c | 53 ++++++++++++++++----------------
>  2 files changed, 64 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index bee8729..5d52641 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -39,22 +39,22 @@ static struct qcom_scm_entry qcom_scm_wb[] = {
>  static DEFINE_MUTEX(qcom_scm_lock);
>  
>  /**
> - * struct qcom_scm_command - one SCM command buffer
> + * struct qcom_scm_legacy_command - one SCM command buffer
>   * @len: total available memory for command and response
>   * @buf_offset: start of command buffer
>   * @resp_hdr_offset: start of response buffer
>   * @id: command to be executed
> - * @buf: buffer returned from qcom_scm_get_command_buffer()
> + * @buf: buffer returned from legacy_get_command_buffer()

I'd prefer to keep qcom_ or at least scm_ somewhere in the name. Just
plain legacy_ is too generic.

>   *
>   * An SCM command is laid out in memory as follows:
>   *
> - *     ------------------- <--- struct qcom_scm_command
> + *     ------------------- <--- struct qcom_scm_legacy_command
>   *     | command header  |
> - *     ------------------- <--- qcom_scm_get_command_buffer()
> + *     ------------------- <--- legacy_get_command_buffer()
>   *     | command buffer  |
> - *     ------------------- <--- struct qcom_scm_response and
> - *     | response header |      qcom_scm_command_to_response()
> - *     ------------------- <--- qcom_scm_get_response_buffer()
> + *     ------------------- <--- struct qcom_scm_legacy_response and
> + *     | response header |      legacy_command_to_response()
> + *     ------------------- <--- legacy_get_response_buffer()
>   *     | response buffer |
>   *     -------------------
>   *
> @@ -62,7 +62,7 @@ static DEFINE_MUTEX(qcom_scm_lock);
>   * you should always use the appropriate qcom_scm_get_*_buffer() routines

Shouldn't this comment be updated too to say "legacy"?

>   * to access the buffers in a safe manner.
>   */
> -struct qcom_scm_command {
> +struct qcom_scm_legacy_command {

Like here, it's called qcom_scm_legacy_<foo> so maybe that should be how
it works.


>         __le32 len;
>         __le32 buf_offset;
>         __le32 resp_hdr_offset;
> @@ -71,52 +71,55 @@ struct qcom_scm_command {
>  };
>  
>  /**
> - * struct qcom_scm_response - one SCM response buffer
> + * struct qcom_scm_legacy_response - one SCM response buffer
>   * @len: total available memory for response
> - * @buf_offset: start of response data relative to start of qcom_scm_response
> + * @buf_offset: start of response data relative to start of
> + *              qcom_scm_legacy_response
>   * @is_complete: indicates if the command has finished processing
>   */
> -struct qcom_scm_response {
> +struct qcom_scm_legacy_response {
>         __le32 len;
>         __le32 buf_offset;
>         __le32 is_complete;
>  };
>  
>  /**
> - * qcom_scm_command_to_response() - Get a pointer to a qcom_scm_response
> + * legacy_command_to_response() - Get a pointer to a qcom_scm_legacy_response
>   * @cmd: command
>   *
>   * Returns a pointer to a response for a command.
>   */
> -static inline struct qcom_scm_response *qcom_scm_command_to_response(
> -               const struct qcom_scm_command *cmd)
> +static inline struct qcom_scm_legacy_response *legacy_command_to_response(
> +               const struct qcom_scm_legacy_command *cmd)
>  {
>         return (void *)cmd + le32_to_cpu(cmd->resp_hdr_offset);
>  }
>  
>  /**
> - * qcom_scm_get_command_buffer() - Get a pointer to a command buffer
> + * legacy_get_command_buffer() - Get a pointer to a command buffer
>   * @cmd: command
>   *
>   * Returns a pointer to the command buffer of a command.
>   */
> -static inline void *qcom_scm_get_command_buffer(const struct qcom_scm_command *cmd)
> +static inline void *legacy_get_command_buffer(
> +               const struct qcom_scm_legacy_command *cmd)
>  {
>         return (void *)cmd->buf;
>  }
>  
>  /**
> - * qcom_scm_get_response_buffer() - Get a pointer to a response buffer
> + * legacy_get_response_buffer() - Get a pointer to a response buffer
>   * @rsp: response
>   *
>   * Returns a pointer to a response buffer of a response.
>   */
> -static inline void *qcom_scm_get_response_buffer(const struct qcom_scm_response *rsp)
> +static inline void *legacy_get_response_buffer(
> +               const struct qcom_scm_legacy_response *rsp)
>  {
>         return (void *)rsp + le32_to_cpu(rsp->buf_offset);
>  }
>  
> -static u32 smc(u32 cmd_addr)
> +static u32 __qcom_scm_call_do(u32 cmd_addr)
>  {
>         int context_id;
>         register u32 r0 asm("r0") = 1;
> @@ -164,8 +167,8 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>                          size_t resp_len)
>  {
>         int ret;
> -       struct qcom_scm_command *cmd;
> -       struct qcom_scm_response *rsp;
> +       struct qcom_scm_legacy_command *cmd;
> +       struct qcom_scm_legacy_response *rsp;
>         size_t alloc_len = sizeof(*cmd) + cmd_len + sizeof(*rsp) + resp_len;
>         dma_addr_t cmd_phys;
>  
> @@ -179,9 +182,9 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>  
>         cmd->id = cpu_to_le32((svc_id << 10) | cmd_id);
>         if (cmd_buf)
> -               memcpy(qcom_scm_get_command_buffer(cmd), cmd_buf, cmd_len);
> +               memcpy(legacy_get_command_buffer(cmd), cmd_buf, cmd_len);
>  
> -       rsp = qcom_scm_command_to_response(cmd);
> +       rsp = legacy_command_to_response(cmd);
>  
>         cmd_phys = dma_map_single(dev, cmd, alloc_len, DMA_TO_DEVICE);
>         if (dma_mapping_error(dev, cmd_phys)) {
> @@ -190,7 +193,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>         }
>  
>         mutex_lock(&qcom_scm_lock);
> -       ret = smc(cmd_phys);
> +       ret = __qcom_scm_call_do(cmd_phys);

What is this change about? Is it confusing to have a function called
'smc'? Please mention why this should change in the commit text.

>         if (ret < 0)
>                 ret = qcom_scm_remap_error(ret);
>         mutex_unlock(&qcom_scm_lock);
> @@ -206,7 +209,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>                 dma_sync_single_for_cpu(dev, cmd_phys + sizeof(*cmd) + cmd_len +
>                                         le32_to_cpu(rsp->buf_offset),
>                                         resp_len, DMA_FROM_DEVICE);
> -               memcpy(resp_buf, qcom_scm_get_response_buffer(rsp),
> +               memcpy(resp_buf, legacy_get_response_buffer(rsp),
>                        resp_len);
>         }
>  out:
> @@ -215,11 +218,12 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>         return ret;
>  }
>  
> -#define SCM_CLASS_REGISTER     (0x2 << 8)
> -#define SCM_MASK_IRQS          BIT(5)
> -#define SCM_ATOMIC(svc, cmd, n) (((((svc) << 10)|((cmd) & 0x3ff)) << 12) | \
> -                               SCM_CLASS_REGISTER | \
> -                               SCM_MASK_IRQS | \
> +#define LEGACY_CLASS_REGISTER          (0x2 << 8)
> +#define LEGACY_MASK_IRQS               BIT(5)
> +#define LEGACY_ATOMIC_ID(svc, cmd, n) \
> +                               (((((svc) << 10)|((cmd) & 0x3ff)) << 12) | \
> +                               LEGACY_CLASS_REGISTER | \
> +                               LEGACY_MASK_IRQS | \
>                                 (n & 0xf))
>  
>  /**
> @@ -235,7 +239,7 @@ static s32 qcom_scm_call_atomic1(u32 svc, u32 cmd, u32 arg1)
>  {
>         int context_id;
>  
> -       register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, 1);
> +       register u32 r0 asm("r0") = LEGACY_ATOMIC_ID(svc, cmd, 1);
>         register u32 r1 asm("r1") = (u32)&context_id;
>         register u32 r2 asm("r2") = arg1;
>  
> @@ -268,7 +272,7 @@ static s32 qcom_scm_call_atomic2(u32 svc, u32 cmd, u32 arg1, u32 arg2)
>  {
>         int context_id;
>  
> -       register u32 r0 asm("r0") = SCM_ATOMIC(svc, cmd, 2);
> +       register u32 r0 asm("r0") = LEGACY_ATOMIC_ID(svc, cmd, 2);
>         register u32 r1 asm("r1") = (u32)&context_id;
>         register u32 r2 asm("r2") = arg1;
>         register u32 r3 asm("r3") = arg2;

Is this hunk really necessary? The defines are local to the file.

> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 7686786..8226b94 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -14,7 +14,7 @@
>  
>  #include "qcom_scm.h"
>  
> -#define QCOM_SCM_FNID(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))
> +#define SMCCC_FUNCNUM(s, c) ((((s) & 0xFF) << 8) | ((c) & 0xFF))

Is this generic? Maybe it should go into the SMCCC file then if it isn't
qcom specific? Otherwise, please leave QCOM in the name.

>  
>  #define MAX_QCOM_SCM_ARGS 10
>  #define MAX_QCOM_SCM_RETS 3
> @@ -58,11 +58,11 @@ static DEFINE_MUTEX(qcom_scm_lock);
>  #define QCOM_SCM_EBUSY_WAIT_MS 30
>  #define QCOM_SCM_EBUSY_MAX_RETRY 20
>  
> -#define N_EXT_QCOM_SCM_ARGS 7
> -#define FIRST_EXT_ARG_IDX 3
> -#define N_REGISTER_ARGS (MAX_QCOM_SCM_ARGS - N_EXT_QCOM_SCM_ARGS + 1)
> +#define SMCCC_N_EXT_ARGS 7
> +#define SMCCC_FIRST_EXT_IDX 3
> +#define SMCCC_N_REG_ARGS (MAX_QCOM_SCM_ARGS - SMCCC_N_EXT_ARGS + 1)
>  
> -static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
> +static void __qcom_scm_call_do_quirk(const struct qcom_scm_desc *desc,
>                                struct arm_smccc_res *res, u32 fn_id,
>                                u64 x5, u32 type)
>  {

>From here....

> @@ -85,22 +85,23 @@ static void __qcom_scm_call_do(const struct qcom_scm_desc *desc,
>         } while (res->a0 == QCOM_SCM_INTERRUPTED);
>  }
>  
> -static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
> +static void qcom_scm_call_do_smccc(const struct qcom_scm_desc *desc,
>                              struct arm_smccc_res *res, u32 fn_id,
>                              u64 x5, bool atomic)
>  {
>         int retry_count = 0;
>  
>         if (atomic) {
> -               __qcom_scm_call_do(desc, res, fn_id, x5, ARM_SMCCC_FAST_CALL);
> +               __qcom_scm_call_do_quirk(desc, res, fn_id, x5,
> +                                        ARM_SMCCC_FAST_CALL);
>                 return;
>         }
>  
>         do {
>                 mutex_lock(&qcom_scm_lock);
>  
> -               __qcom_scm_call_do(desc, res, fn_id, x5,
> -                                  ARM_SMCCC_STD_CALL);
> +               __qcom_scm_call_do_quirk(desc, res, fn_id, x5,
> +                                        ARM_SMCCC_STD_CALL);
>  
>                 mutex_unlock(&qcom_scm_lock);
>  
> @@ -112,21 +113,21 @@ static void qcom_scm_call_do(const struct qcom_scm_desc *desc,
>         }  while (res->a0 == QCOM_SCM_V2_EBUSY);
>  }
>  
> -static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
> -                           const struct qcom_scm_desc *desc,
> -                           struct arm_smccc_res *res, bool atomic)
> +static int ___qcom_scm_call_smccc(struct device *dev, u32 svc_id, u32 cmd_id,
> +                                 const struct qcom_scm_desc *desc,
> +                                 struct arm_smccc_res *res, bool atomic)
>  {
>         int arglen = desc->arginfo & 0xf;
>         int i;
> -       u32 fn_id = QCOM_SCM_FNID(svc_id, cmd_id);
> -       u64 x5 = desc->args[FIRST_EXT_ARG_IDX];
> +       u32 fn_id = SMCCC_FUNCNUM(svc_id, cmd_id);
> +       u64 x5 = desc->args[SMCCC_FIRST_EXT_IDX];
>         dma_addr_t args_phys = 0;
>         void *args_virt = NULL;
>         size_t alloc_len;
>         gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>  
> -       if (unlikely(arglen > N_REGISTER_ARGS)) {
> -               alloc_len = N_EXT_QCOM_SCM_ARGS * sizeof(u64);
> +       if (unlikely(arglen > SMCCC_N_REG_ARGS)) {
> +               alloc_len = SMCCC_N_EXT_ARGS * sizeof(u64);
>                 args_virt = kzalloc(PAGE_ALIGN(alloc_len), flag);
>  
>                 if (!args_virt)
> @@ -135,15 +136,15 @@ static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>                 if (qcom_smccc_convention == ARM_SMCCC_SMC_32) {
>                         __le32 *args = args_virt;
>  
> -                       for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> +                       for (i = 0; i < SMCCC_N_EXT_ARGS; i++)
>                                 args[i] = cpu_to_le32(desc->args[i +
> -                                                     FIRST_EXT_ARG_IDX]);
> +                                                     SMCCC_FIRST_EXT_IDX]);
>                 } else {
>                         __le64 *args = args_virt;
>  
> -                       for (i = 0; i < N_EXT_QCOM_SCM_ARGS; i++)
> +                       for (i = 0; i < SMCCC_N_EXT_ARGS; i++)
>                                 args[i] = cpu_to_le64(desc->args[i +
> -                                                     FIRST_EXT_ARG_IDX]);
> +                                                     SMCCC_FIRST_EXT_IDX]);
>                 }
>  
>                 args_phys = dma_map_single(dev, args_virt, alloc_len,
> @@ -157,7 +158,7 @@ static int ___qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>                 x5 = args_phys;
>         }
>  
> -       qcom_scm_call_do(desc, res, fn_id, x5, atomic);
> +       qcom_scm_call_do_smccc(desc, res, fn_id, x5, atomic);
>  
>         if (args_virt) {
>                 dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
> @@ -185,7 +186,7 @@ static int qcom_scm_call(struct device *dev, u32 svc_id, u32 cmd_id,
>                          struct arm_smccc_res *res)
>  {
>         might_sleep();
> -       return ___qcom_scm_call(dev, svc_id, cmd_id, desc, res, false);
> +       return ___qcom_scm_call_smccc(dev, svc_id, cmd_id, desc, res, false);
>  }
>  
>  /**
> @@ -203,7 +204,7 @@ static int qcom_scm_call_atomic(struct device *dev, u32 svc_id, u32 cmd_id,
>                                 const struct qcom_scm_desc *desc,
>                                 struct arm_smccc_res *res)
>  {
> -       return ___qcom_scm_call(dev, svc_id, cmd_id, desc, res, true);
> +       return ___qcom_scm_call_smccc(dev, svc_id, cmd_id, desc, res, true);
>  }
>  
>  /**
> @@ -253,7 +254,7 @@ int __qcom_scm_is_call_available(struct device *dev, u32 svc_id, u32 cmd_id)
>         struct arm_smccc_res res;
>  
>         desc.arginfo = QCOM_SCM_ARGS(1);
> -       desc.args[0] = QCOM_SCM_FNID(svc_id, cmd_id) |
> +       desc.args[0] = SMCCC_FUNCNUM(svc_id, cmd_id) |
>                         (ARM_SMCCC_OWNER_SIP << ARM_SMCCC_OWNER_SHIFT);
>  
>         ret = qcom_scm_call(dev, QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD,
> @@ -295,7 +296,7 @@ void __qcom_scm_init(void)
>  {
>         u64 cmd;
>         struct arm_smccc_res res;
> -       u32 function = QCOM_SCM_FNID(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
> +       u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, QCOM_IS_CALL_AVAIL_CMD);
>  
>         /* First try a SMC64 call */
>         cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64,

... to here I don't understand why any of it needs to change. It looks
like a bunch of churn and it conflates qcom SCM calls with SMCCC which
is not desirable. Those two concepts are different.