lkml.org 
[lkml]   [2019]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH v2 01/18] firmware: qcom_scm: Rename macros and structures
    From
    Date
    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@codeaurora.org>
    > ---
    > 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.

    \
     
     \ /
      Last update: 2019-11-16 00:29    [W:4.228 / U:0.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site