From: Stephen Boyd <swboyd@chromium.org>
To: Elliot Berman <eberman@codeaurora.org>,
agross@kernel.org, bjorn.andersson@linaro.org,
saiprakash.ranjan@codeaurora.org
Cc: Elliot Berman <eberman@codeaurora.org>,
tsoni@codeaurora.org, sidgup@codeaurora.org,
psodagud@codeaurora.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 01/18] firmware: qcom_scm: Rename macros and structures
Date: Fri, 15 Nov 2019 15:27:22 -0800 [thread overview]
Message-ID: <5dcf345b.1c69fb81.df1ea.f7f6@mx.google.com> (raw)
In-Reply-To: <1573593774-12539-2-git-send-email-eberman@codeaurora.org>
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.
next prev parent reply other threads:[~2019-11-15 23:27 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 21:22 [PATCH v2 00/18] Restructure, improve target support for qcom_scm driver Elliot Berman
2019-11-12 21:22 ` [PATCH v2 01/18] firmware: qcom_scm: Rename macros and structures Elliot Berman
2019-11-15 23:27 ` Stephen Boyd [this message]
2019-11-16 1:19 ` eberman
2019-11-19 21:47 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 02/18] firmware: qcom_scm: Add funcnum IDs Elliot Berman
2019-11-15 23:30 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 03/18] firmware: qcom_scm-64: Make SMCCC macros less magical Elliot Berman
2019-11-15 22:43 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 04/18] firmware: qcom_scm: Apply consistent naming scheme to command IDs Elliot Berman
2019-11-15 22:45 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 05/18] firmware: qcom_scm: Remove unused qcom_scm_get_version Elliot Berman
2019-11-15 22:45 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 06/18] firmware: qcom_scm-64: Move svc/cmd/owner into qcom_scm_desc Elliot Berman
2019-11-15 23:40 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 07/18] firmware: qcom_scm-64: Add SCM results to descriptor Elliot Berman
2019-11-15 23:42 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 08/18] firmware: qcom_scm-64: Remove qcom_scm_call_do_smccc Elliot Berman
2019-11-15 23:45 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 09/18] firmware: qcom_scm-64: Move SMC register filling to qcom_scm_call_smccc Elliot Berman
2019-11-15 23:57 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 10/18] firmware: qcom_scm-64: Improve SMC convention detection Elliot Berman
2019-11-16 0:21 ` Stephen Boyd
2019-11-16 1:29 ` eberman
2019-11-19 21:49 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 11/18] firmware: qcom_scm-32: Use SMC arch wrappers Elliot Berman
2019-11-16 0:41 ` Stephen Boyd
2019-12-12 19:45 ` Elliot Berman
2019-11-12 21:22 ` [PATCH v2 12/18] firmware: qcom_scm-32: Use qcom_scm_desc in non-atomic calls Elliot Berman
2019-11-19 22:03 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 13/18] firmware: qcom_scm-32: Move SMCCC register filling to qcom_scm_call Elliot Berman
2019-11-19 22:05 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 14/18] firmware: qcom_scm-32: Create common legacy atomic call Elliot Berman
2019-11-19 22:11 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 15/18] firmware: qcom_scm-32: Add device argument to atomic calls Elliot Berman
2019-11-19 22:13 ` Stephen Boyd
2019-11-12 21:22 ` [PATCH v2 16/18] firmware: qcom_scm: Remove thin wrappers Elliot Berman
2019-11-12 21:22 ` [PATCH v2 17/18] firmware: qcom_scm: Dynamically support SMCCC and legacy conventions Elliot Berman
2019-11-12 21:22 ` [PATCH v2 18/18] firmware: qcom_scm: Order functions, definitions by service/command Elliot Berman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5dcf345b.1c69fb81.df1ea.f7f6@mx.google.com \
--to=swboyd@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=eberman@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=psodagud@codeaurora.org \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=sidgup@codeaurora.org \
--cc=tsoni@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.