* [PATCH 0/2] Allow parameter in smc/hvc calls
@ 2023-04-09 18:19 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in smc calls from that instance, while
sharing the same smc-id(func_id) among the instances.
This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../bindings/firmware/arm,scmi.yaml | 16 +++++
drivers/firmware/arm_scmi/smc.c | 66 ++++++++++++++++++-
2 files changed, 81 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 0/2] Allow parameter in smc/hvc calls
@ 2023-04-09 18:19 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in smc calls from that instance, while
sharing the same smc-id(func_id) among the instances.
This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../bindings/firmware/arm,scmi.yaml | 16 +++++
drivers/firmware/arm_scmi/smc.c | 66 ++++++++++++++++++-
2 files changed, 81 insertions(+), 1 deletion(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-09 18:19 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..08c331a79b80 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,22 @@ properties:
description:
SMC id required when using smc or hvc transports
+ arm,smc32-params:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description:
+ An optional parameter list passed in smc32 or hvc32 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
+ arm,smc64-params:
+ $ref: /schemas/types.yaml#/definitions/uint64-array
+ description:
+ An optional parameter list passed in smc64 or hvc64 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
linaro,optee-channel-id:
$ref: /schemas/types.yaml#/definitions/uint32
description:
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-09 18:19 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..08c331a79b80 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,22 @@ properties:
description:
SMC id required when using smc or hvc transports
+ arm,smc32-params:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description:
+ An optional parameter list passed in smc32 or hvc32 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
+ arm,smc64-params:
+ $ref: /schemas/types.yaml#/definitions/uint64-array
+ description:
+ An optional parameter list passed in smc64 or hvc64 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
linaro,optee-channel-id:
$ref: /schemas/types.yaml#/definitions/uint32
description:
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-09 18:19 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/smc.c | 66 ++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..39582d1e2c9f 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
#include "common.h"
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+ u64 x;
+ u32 w;
+} scmi_smc_param_t;
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +40,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
*/
struct scmi_smc {
@@ -40,8 +52,51 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ scmi_smc_param_t params[MAX_PARAM_COUNT];
};
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+ struct device_node *np = dev->of_node;
+ u64 params64[MAX_PARAM_COUNT] = { 0 };
+ u32 params32[MAX_PARAM_COUNT] = { 0 };
+ int i, count;
+
+ if (scmi_info->is_smc64) {
+ count = of_property_read_variable_u64_array(np,
+ "arm,smc64-params",
+ ¶ms64[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].x = params64[i];
+ else
+ goto param_err;
+ } else {
+ count = of_property_read_variable_u32_array(np,
+ "arm,smc32-params",
+ ¶ms32[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].w = params32[i];
+ else
+ goto param_err;
+ }
+
+ return;
+
+param_err:
+ dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
static irqreturn_t smc_msg_done_isr(int irq, void *data)
{
struct scmi_smc *scmi_info = data;
@@ -156,6 +211,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+ populate_smc_params(dev, scmi_info);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -179,6 +236,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ scmi_smc_param_t *p = scmi_info->params;
/*
* Channel will be released only once response has been
@@ -188,7 +246,13 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+ if (scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+ p[3].x, p[4].x, p[5].x, 0, &res);
+ else
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+ p[3].w, p[4].w, p[5].w, 0, &res);
+
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-04-09 18:19 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-09 18:19 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/smc.c | 66 ++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..39582d1e2c9f 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
#include "common.h"
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+ u64 x;
+ u32 w;
+} scmi_smc_param_t;
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +40,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
*/
struct scmi_smc {
@@ -40,8 +52,51 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ scmi_smc_param_t params[MAX_PARAM_COUNT];
};
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+ struct device_node *np = dev->of_node;
+ u64 params64[MAX_PARAM_COUNT] = { 0 };
+ u32 params32[MAX_PARAM_COUNT] = { 0 };
+ int i, count;
+
+ if (scmi_info->is_smc64) {
+ count = of_property_read_variable_u64_array(np,
+ "arm,smc64-params",
+ ¶ms64[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].x = params64[i];
+ else
+ goto param_err;
+ } else {
+ count = of_property_read_variable_u32_array(np,
+ "arm,smc32-params",
+ ¶ms32[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].w = params32[i];
+ else
+ goto param_err;
+ }
+
+ return;
+
+param_err:
+ dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
static irqreturn_t smc_msg_done_isr(int irq, void *data)
{
struct scmi_smc *scmi_info = data;
@@ -156,6 +211,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+ populate_smc_params(dev, scmi_info);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -179,6 +236,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ scmi_smc_param_t *p = scmi_info->params;
/*
* Channel will be released only once response has been
@@ -188,7 +246,13 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+ if (scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+ p[3].x, p[4].x, p[5].x, 0, &res);
+ else
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+ p[3].w, p[4].w, p[5].w, 0, &res);
+
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-09 22:22 ` kernel test robot
-1 siblings, 0 replies; 70+ messages in thread
From: kernel test robot @ 2023-04-09 22:22 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: oe-kbuild-all, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Hi Nikunj,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.3-rc6 next-20230406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230409181918.29270-3-quic_nkela%40quicinc.com
patch subject: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230410/202304100606.kUjhsRYf-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9c34de66aa243da9824e8bbe749b24e36b0db483
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
git checkout 9c34de66aa243da9824e8bbe749b24e36b0db483
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/firmware/arm_scmi/smc.c:9:
drivers/firmware/arm_scmi/smc.c: In function 'smc_send_message':
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
vim +436 include/linux/arm-smccc.h
f2d3b2e8759a58 Marc Zyngier 2018-02-06 411
f2d3b2e8759a58 Marc Zyngier 2018-02-06 412 #define __declare_arg_0(a0, res) \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 413 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 414 register unsigned long arg0 asm("r0") = (u32)a0
f2d3b2e8759a58 Marc Zyngier 2018-02-06 415
f2d3b2e8759a58 Marc Zyngier 2018-02-06 416 #define __declare_arg_1(a0, a1, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 417 typeof(a1) __a1 = a1; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 418 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 419 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 420 register typeof(a1) arg1 asm("r1") = __a1
f2d3b2e8759a58 Marc Zyngier 2018-02-06 421
f2d3b2e8759a58 Marc Zyngier 2018-02-06 422 #define __declare_arg_2(a0, a1, a2, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 423 typeof(a1) __a1 = a1; \
755a8bf5579d22 Marc Zyngier 2018-08-24 424 typeof(a2) __a2 = a2; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 425 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 426 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 427 register typeof(a1) arg1 asm("r1") = __a1; \
0794a974d74dc7 Andrew Scull 2020-09-15 428 register typeof(a2) arg2 asm("r2") = __a2
f2d3b2e8759a58 Marc Zyngier 2018-02-06 429
f2d3b2e8759a58 Marc Zyngier 2018-02-06 430 #define __declare_arg_3(a0, a1, a2, a3, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 431 typeof(a1) __a1 = a1; \
755a8bf5579d22 Marc Zyngier 2018-08-24 432 typeof(a2) __a2 = a2; \
755a8bf5579d22 Marc Zyngier 2018-08-24 433 typeof(a3) __a3 = a3; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 434 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 435 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 @436 register typeof(a1) arg1 asm("r1") = __a1; \
0794a974d74dc7 Andrew Scull 2020-09-15 437 register typeof(a2) arg2 asm("r2") = __a2; \
0794a974d74dc7 Andrew Scull 2020-09-15 @438 register typeof(a3) arg3 asm("r3") = __a3
f2d3b2e8759a58 Marc Zyngier 2018-02-06 439
f2d3b2e8759a58 Marc Zyngier 2018-02-06 440 #define __declare_arg_4(a0, a1, a2, a3, a4, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 441 typeof(a4) __a4 = a4; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 442 __declare_arg_3(a0, a1, a2, a3, res); \
0794a974d74dc7 Andrew Scull 2020-09-15 443 register typeof(a4) arg4 asm("r4") = __a4
f2d3b2e8759a58 Marc Zyngier 2018-02-06 444
f2d3b2e8759a58 Marc Zyngier 2018-02-06 445 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 446 typeof(a5) __a5 = a5; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 447 __declare_arg_4(a0, a1, a2, a3, a4, res); \
0794a974d74dc7 Andrew Scull 2020-09-15 @448 register typeof(a5) arg5 asm("r5") = __a5
f2d3b2e8759a58 Marc Zyngier 2018-02-06 449
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-04-09 22:22 ` kernel test robot
0 siblings, 0 replies; 70+ messages in thread
From: kernel test robot @ 2023-04-09 22:22 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: oe-kbuild-all, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Hi Nikunj,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.3-rc6 next-20230406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230409181918.29270-3-quic_nkela%40quicinc.com
patch subject: [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230410/202304100606.kUjhsRYf-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9c34de66aa243da9824e8bbe749b24e36b0db483
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nikunj-Kela/dt-bindings-firmware-arm-scmi-support-parameter-passing-in-smc-hvc/20230410-022129
git checkout 9c34de66aa243da9824e8bbe749b24e36b0db483
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/firmware/arm_scmi/smc.c:9:
drivers/firmware/arm_scmi/smc.c: In function 'smc_send_message':
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:518:33: note: in expansion of macro '__arm_smccc_1_1'
518 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:552:25: note: in expansion of macro 'arm_smccc_1_1_hvc'
552 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:479:17: note: in expansion of macro '__declare_args'
479 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:502:33: note: in expansion of macro '__arm_smccc_1_1'
502 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:555:25: note: in expansion of macro 'arm_smccc_1_1_smc'
555 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:436:29: error: register specified for 'arg1' isn't suitable for data type
436 | register typeof(a1) arg1 asm("r1") = __a1; \
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:438:29: error: register specified for 'arg3' isn't suitable for data type
438 | register typeof(a3) arg3 asm("r3") = __a3
| ^~~~
include/linux/arm-smccc.h:442:9: note: in expansion of macro '__declare_arg_3'
442 | __declare_arg_3(a0, a1, a2, a3, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:447:9: note: in expansion of macro '__declare_arg_4'
447 | __declare_arg_4(a0, a1, a2, a3, a4, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
>> include/linux/arm-smccc.h:448:29: error: register specified for 'arg5' isn't suitable for data type
448 | register typeof(a5) arg5 asm("r5") = __a5
| ^~~~
include/linux/arm-smccc.h:452:9: note: in expansion of macro '__declare_arg_5'
452 | __declare_arg_5(a0, a1, a2, a3, a4, a5, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:457:9: note: in expansion of macro '__declare_arg_6'
457 | __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res); \
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:460:37: note: in expansion of macro '__declare_arg_7'
460 | #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:461:37: note: in expansion of macro '___declare_args'
461 | #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/arm-smccc.h:527:17: note: in expansion of macro '__declare_args'
527 | __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
| ^~~~~~~~~~~~~~
include/linux/arm-smccc.h:558:25: note: in expansion of macro '__fail_smccc_1_1'
558 | __fail_smccc_1_1(__VA_ARGS__); \
| ^~~~~~~~~~~~~~~~
drivers/firmware/arm_scmi/smc.c:250:17: note: in expansion of macro 'arm_smccc_1_1_invoke'
250 | arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
| ^~~~~~~~~~~~~~~~~~~~
vim +436 include/linux/arm-smccc.h
f2d3b2e8759a58 Marc Zyngier 2018-02-06 411
f2d3b2e8759a58 Marc Zyngier 2018-02-06 412 #define __declare_arg_0(a0, res) \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 413 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 414 register unsigned long arg0 asm("r0") = (u32)a0
f2d3b2e8759a58 Marc Zyngier 2018-02-06 415
f2d3b2e8759a58 Marc Zyngier 2018-02-06 416 #define __declare_arg_1(a0, a1, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 417 typeof(a1) __a1 = a1; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 418 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 419 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 420 register typeof(a1) arg1 asm("r1") = __a1
f2d3b2e8759a58 Marc Zyngier 2018-02-06 421
f2d3b2e8759a58 Marc Zyngier 2018-02-06 422 #define __declare_arg_2(a0, a1, a2, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 423 typeof(a1) __a1 = a1; \
755a8bf5579d22 Marc Zyngier 2018-08-24 424 typeof(a2) __a2 = a2; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 425 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 426 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 427 register typeof(a1) arg1 asm("r1") = __a1; \
0794a974d74dc7 Andrew Scull 2020-09-15 428 register typeof(a2) arg2 asm("r2") = __a2
f2d3b2e8759a58 Marc Zyngier 2018-02-06 429
f2d3b2e8759a58 Marc Zyngier 2018-02-06 430 #define __declare_arg_3(a0, a1, a2, a3, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 431 typeof(a1) __a1 = a1; \
755a8bf5579d22 Marc Zyngier 2018-08-24 432 typeof(a2) __a2 = a2; \
755a8bf5579d22 Marc Zyngier 2018-08-24 433 typeof(a3) __a3 = a3; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 434 struct arm_smccc_res *___res = res; \
0794a974d74dc7 Andrew Scull 2020-09-15 435 register unsigned long arg0 asm("r0") = (u32)a0; \
0794a974d74dc7 Andrew Scull 2020-09-15 @436 register typeof(a1) arg1 asm("r1") = __a1; \
0794a974d74dc7 Andrew Scull 2020-09-15 437 register typeof(a2) arg2 asm("r2") = __a2; \
0794a974d74dc7 Andrew Scull 2020-09-15 @438 register typeof(a3) arg3 asm("r3") = __a3
f2d3b2e8759a58 Marc Zyngier 2018-02-06 439
f2d3b2e8759a58 Marc Zyngier 2018-02-06 440 #define __declare_arg_4(a0, a1, a2, a3, a4, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 441 typeof(a4) __a4 = a4; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 442 __declare_arg_3(a0, a1, a2, a3, res); \
0794a974d74dc7 Andrew Scull 2020-09-15 443 register typeof(a4) arg4 asm("r4") = __a4
f2d3b2e8759a58 Marc Zyngier 2018-02-06 444
f2d3b2e8759a58 Marc Zyngier 2018-02-06 445 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res) \
755a8bf5579d22 Marc Zyngier 2018-08-24 446 typeof(a5) __a5 = a5; \
f2d3b2e8759a58 Marc Zyngier 2018-02-06 447 __declare_arg_4(a0, a1, a2, a3, a4, res); \
0794a974d74dc7 Andrew Scull 2020-09-15 @448 register typeof(a5) arg5 asm("r5") = __a5
f2d3b2e8759a58 Marc Zyngier 2018-02-06 449
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-10 17:20 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-10 17:20 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 09/04/2023 20:19, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..08c331a79b80 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,22 @@ properties:
> description:
> SMC id required when using smc or hvc transports
>
> + arm,smc32-params:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description:
> + An optional parameter list passed in smc32 or hvc32 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
> + arm,smc64-params:
> + $ref: /schemas/types.yaml#/definitions/uint64-array
> + description:
> + An optional parameter list passed in smc64 or hvc64 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
These do not look like hardware properties and the fact that you need
two properties for the same also points that you tied it to specific SW
interface.
Why this should be board-specific? Actually better question - why this
should be fixed per board? Doesn't my software want to have different
parameters, depending on some other condition?
You also did not provide any DTS user for this, so difficult to judge
usefulness.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-10 17:20 ` Krzysztof Kozlowski
0 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-10 17:20 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 09/04/2023 20:19, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..08c331a79b80 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,22 @@ properties:
> description:
> SMC id required when using smc or hvc transports
>
> + arm,smc32-params:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description:
> + An optional parameter list passed in smc32 or hvc32 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
> + arm,smc64-params:
> + $ref: /schemas/types.yaml#/definitions/uint64-array
> + description:
> + An optional parameter list passed in smc64 or hvc64 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
These do not look like hardware properties and the fact that you need
two properties for the same also points that you tied it to specific SW
interface.
Why this should be board-specific? Actually better question - why this
should be fixed per board? Doesn't my software want to have different
parameters, depending on some other condition?
You also did not provide any DTS user for this, so difficult to judge
usefulness.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-10 17:20 ` Krzysztof Kozlowski
@ 2023-04-10 17:33 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 17:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
> On 09/04/2023 20:19, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..08c331a79b80 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,22 @@ properties:
>> description:
>> SMC id required when using smc or hvc transports
>>
>> + arm,smc32-params:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + An optional parameter list passed in smc32 or hvc32 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
>> + arm,smc64-params:
>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>> + description:
>> + An optional parameter list passed in smc64 or hvc64 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
> These do not look like hardware properties and the fact that you need
> two properties for the same also points that you tied it to specific SW
> interface.
This is certainly not the H/W property but then smc-id is also not H/W
property either
but that gets passed via DTB. I could use the same property for both
however I wasn't sure
which datatype should be used, uint32-array/uint64-array. Moreover, I
thought if users are
passing parameters, they better know which SMC convention they are using
hence used two
explicit properties.
> Why this should be board-specific? Actually better question - why this
> should be fixed per board? Doesn't my software want to have different
> parameters, depending on some other condition?
Not sure I follow, I didn't say this is board specific. People can use
the parameters to pass
whatever their S/W demands. SMC/HVC calls are made by passing parameters
which is what this patch is enabling.
>
> You also did not provide any DTS user for this, so difficult to judge
> usefulness.
The work is still on going and we will push the dts in few months,
however that shouldn't stop
making changes in advance.
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-10 17:33 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 17:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
> On 09/04/2023 20:19, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..08c331a79b80 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,22 @@ properties:
>> description:
>> SMC id required when using smc or hvc transports
>>
>> + arm,smc32-params:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + An optional parameter list passed in smc32 or hvc32 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
>> + arm,smc64-params:
>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>> + description:
>> + An optional parameter list passed in smc64 or hvc64 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
> These do not look like hardware properties and the fact that you need
> two properties for the same also points that you tied it to specific SW
> interface.
This is certainly not the H/W property but then smc-id is also not H/W
property either
but that gets passed via DTB. I could use the same property for both
however I wasn't sure
which datatype should be used, uint32-array/uint64-array. Moreover, I
thought if users are
passing parameters, they better know which SMC convention they are using
hence used two
explicit properties.
> Why this should be board-specific? Actually better question - why this
> should be fixed per board? Doesn't my software want to have different
> parameters, depending on some other condition?
Not sure I follow, I didn't say this is board specific. People can use
the parameters to pass
whatever their S/W demands. SMC/HVC calls are made by passing parameters
which is what this patch is enabling.
>
> You also did not provide any DTS user for this, so difficult to judge
> usefulness.
The work is still on going and we will push the dts in few months,
however that shouldn't stop
making changes in advance.
> Best regards,
> Krzysztof
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 0/2] Allow parameter in smc/hvc calls
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-10 18:20 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in hvc calls from that instance, while
sharing the same smc-id(func_id) among the instances.
This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.
---
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../bindings/firmware/arm,scmi.yaml | 17 +++++
drivers/firmware/arm_scmi/smc.c | 72 ++++++++++++++++++-
2 files changed, 88 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 0/2] Allow parameter in smc/hvc calls
@ 2023-04-10 18:20 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM and hypervisor associates a tag with each instance
and expects the tag in hvc calls from that instance, while
sharing the same smc-id(func_id) among the instances.
This patch series introduces new optional dtb bindings which
can be used to pass parameters to smc/hvc calls.
---
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../bindings/firmware/arm,scmi.yaml | 17 +++++
drivers/firmware/arm_scmi/smc.c | 72 ++++++++++++++++++-
2 files changed, 88 insertions(+), 1 deletion(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-10 18:20 ` Nikunj Kela
@ 2023-04-10 18:20 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ecf76b763c8c 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,23 @@ properties:
description:
SMC id required when using smc or hvc transports
+ arm,smc32-params:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description:
+ An optional parameter list passed in smc32 or hvc32 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
+ arm,smc64-params:
+ $ref: /schemas/types.yaml#/definitions/uint64-array
+ description:
+ An optional parameter list passed in smc64 or hvc64 calls.
+ This is valid only on ARM64 machines.
+ default: 0
+ minItems: 1
+ maxItems: 6
+
linaro,optee-channel-id:
$ref: /schemas/types.yaml#/definitions/uint32
description:
@@ -427,6 +444,7 @@ examples:
compatible = "arm,scmi-smc";
shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
arm,smc-id = <0xc3000001>;
+ arm,smc64-params = <0xcd974d6c 0x5ed97289>;
#address-cells = <1>;
#size-cells = <0>;
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-10 18:20 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines two optional device tree bindings,
that can be used to pass parameters in smc/hvc calls.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
.../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ecf76b763c8c 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -115,6 +115,23 @@ properties:
description:
SMC id required when using smc or hvc transports
+ arm,smc32-params:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description:
+ An optional parameter list passed in smc32 or hvc32 calls
+ default: 0
+ minItems: 1
+ maxItems: 6
+
+ arm,smc64-params:
+ $ref: /schemas/types.yaml#/definitions/uint64-array
+ description:
+ An optional parameter list passed in smc64 or hvc64 calls.
+ This is valid only on ARM64 machines.
+ default: 0
+ minItems: 1
+ maxItems: 6
+
linaro,optee-channel-id:
$ref: /schemas/types.yaml#/definitions/uint32
description:
@@ -427,6 +444,7 @@ examples:
compatible = "arm,scmi-smc";
shmem = <&cpu_scp_lpri0>, <&cpu_scp_lpri1>;
arm,smc-id = <0xc3000001>;
+ arm,smc64-params = <0xcd974d6c 0x5ed97289>;
#address-cells = <1>;
#size-cells = <0>;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-04-10 18:20 ` Nikunj Kela
@ 2023-04-10 18:20 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/smc.c | 72 ++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..c57d2f3fab87 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
#include "common.h"
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+ u64 x;
+ u32 w;
+} scmi_smc_param_t;
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +40,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
*/
struct scmi_smc {
@@ -40,8 +52,55 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ scmi_smc_param_t params[MAX_PARAM_COUNT];
};
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+ struct device_node *np = dev->of_node;
+ u64 params64[MAX_PARAM_COUNT] = { 0 };
+ u32 params32[MAX_PARAM_COUNT] = { 0 };
+ int i, count;
+
+#ifdef CONFIG_ARM64
+ if (scmi_info->is_smc64) {
+ count = of_property_read_variable_u64_array(np,
+ "arm,smc64-params",
+ ¶ms64[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].x = params64[i];
+ else
+ goto param_err;
+ } else {
+#else
+ {
+#endif
+ count = of_property_read_variable_u32_array(np,
+ "arm,smc32-params",
+ ¶ms32[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].w = params32[i];
+ else
+ goto param_err;
+ }
+
+ return;
+
+param_err:
+ dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
static irqreturn_t smc_msg_done_isr(int irq, void *data)
{
struct scmi_smc *scmi_info = data;
@@ -156,6 +215,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+ populate_smc_params(dev, scmi_info);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -179,6 +240,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ scmi_smc_param_t *p = scmi_info->params;
/*
* Channel will be released only once response has been
@@ -188,7 +250,15 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+ if (scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+ p[3].x, p[4].x, p[5].x, 0, &res);
+ else
+#endif
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+ p[3].w, p[4].w, p[5].w, 0, &res);
+
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-04-10 18:20 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-10 18:20 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp, Nikunj Kela
This patch add support for passing parameters to smc/hvc calls.
This patch is useful when multiple scmi instances are using same
smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/smc.c | 72 ++++++++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..c57d2f3fab87 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,16 @@
#include "common.h"
+#define MAX_PARAM_COUNT 6
+
+/**
+ * scmi_smc_param_t - parameter type for SCMI smc/hvc call
+ */
+typedef union {
+ u64 x;
+ u32 w;
+} scmi_smc_param_t;
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +40,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: A flag, indicating smc64 calling convention.
+ * @params: Optional, smc/hvc call parameters.
*/
struct scmi_smc {
@@ -40,8 +52,55 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ scmi_smc_param_t params[MAX_PARAM_COUNT];
};
+static void populate_smc_params(struct device *dev, struct scmi_smc *scmi_info)
+{
+ struct device_node *np = dev->of_node;
+ u64 params64[MAX_PARAM_COUNT] = { 0 };
+ u32 params32[MAX_PARAM_COUNT] = { 0 };
+ int i, count;
+
+#ifdef CONFIG_ARM64
+ if (scmi_info->is_smc64) {
+ count = of_property_read_variable_u64_array(np,
+ "arm,smc64-params",
+ ¶ms64[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].x = params64[i];
+ else
+ goto param_err;
+ } else {
+#else
+ {
+#endif
+ count = of_property_read_variable_u32_array(np,
+ "arm,smc32-params",
+ ¶ms32[0], 1,
+ MAX_PARAM_COUNT);
+ if (count == -EINVAL) /* if property is not defined */
+ return;
+
+ if (count > 0) /* populate the parameters */
+ for (i = 0; i < count; i++)
+ scmi_info->params[i].w = params32[i];
+ else
+ goto param_err;
+ }
+
+ return;
+
+param_err:
+ dev_warn(dev, "failed to read smc/hvc call parameters\n");
+}
+
static irqreturn_t smc_msg_done_isr(int irq, void *data)
{
struct scmi_smc *scmi_info = data;
@@ -156,6 +215,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
+ populate_smc_params(dev, scmi_info);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -179,6 +240,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ scmi_smc_param_t *p = scmi_info->params;
/*
* Channel will be released only once response has been
@@ -188,7 +250,15 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+ if (scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].x, p[1].x, p[2].x,
+ p[3].x, p[4].x, p[5].x, 0, &res);
+ else
+#endif
+ arm_smccc_1_1_invoke(scmi_info->func_id, p[0].w, p[1].w, p[2].w,
+ p[3].w, p[4].w, p[5].w, 0, &res);
+
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-10 18:20 ` Nikunj Kela
@ 2023-04-11 12:54 ` Sudeep Holla
-1 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-11 12:54 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
Why 2 values ?
> This is useful when multiple scmi instances are used with common smc-id.
>
I really would like to avoid this binding. Because of lack of standard
SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
case like this in a vendor specific way is something I would like to avoid.
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..ecf76b763c8c 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,23 @@ properties:
> description:
> SMC id required when using smc or hvc transports
>
> + arm,smc32-params:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description:
> + An optional parameter list passed in smc32 or hvc32 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
> + arm,smc64-params:
> + $ref: /schemas/types.yaml#/definitions/uint64-array
> + description:
> + An optional parameter list passed in smc64 or hvc64 calls.
> + This is valid only on ARM64 machines.
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
Even if we end up adding(which I would very much like to avoid), I don't see
the need for 32 and 64 bit params like this. There must be ways to avoid that
used by some property in some other binding(I will look for one if we choose
this path)
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-11 12:54 ` Sudeep Holla
0 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-11 12:54 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
Why 2 values ?
> This is useful when multiple scmi instances are used with common smc-id.
>
I really would like to avoid this binding. Because of lack of standard
SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
case like this in a vendor specific way is something I would like to avoid.
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..ecf76b763c8c 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -115,6 +115,23 @@ properties:
> description:
> SMC id required when using smc or hvc transports
>
> + arm,smc32-params:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description:
> + An optional parameter list passed in smc32 or hvc32 calls
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
> + arm,smc64-params:
> + $ref: /schemas/types.yaml#/definitions/uint64-array
> + description:
> + An optional parameter list passed in smc64 or hvc64 calls.
> + This is valid only on ARM64 machines.
> + default: 0
> + minItems: 1
> + maxItems: 6
> +
Even if we end up adding(which I would very much like to avoid), I don't see
the need for 32 and 64 bit params like this. There must be ways to avoid that
used by some property in some other binding(I will look for one if we choose
this path)
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
2023-04-10 18:20 ` Nikunj Kela
@ 2023-04-11 13:01 ` Sudeep Holla
-1 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-11 13:01 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM and hypervisor associates a tag with each instance
> and expects the tag in hvc calls from that instance, while
> sharing the same smc-id(func_id) among the instances.
>
> This patch series introduces new optional dtb bindings which
> can be used to pass parameters to smc/hvc calls.
>
Just to be sure that I understood the problem(as your 2 parameters confused
me), this is just to help hypervisor/secure side to identify the right
channel/shared memory when the same SMC/HVC function id is shared right ?
If that is the case, why can't we just pass the shmem address as the
parameter ? I would like to avoid fragmentation here with every vendor
trying to do different things to achieve the same.
I would just change the driver to do that. Not sure if it breaks any existing
implementation or not. If it does, we can add another compatible to identify
one needing this fixed(shmem address) as additional parameter.
Does that make sense at all ? Or am I missing some/all of the requirements
here ?
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
@ 2023-04-11 13:01 ` Sudeep Holla
0 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-11 13:01 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM and hypervisor associates a tag with each instance
> and expects the tag in hvc calls from that instance, while
> sharing the same smc-id(func_id) among the instances.
>
> This patch series introduces new optional dtb bindings which
> can be used to pass parameters to smc/hvc calls.
>
Just to be sure that I understood the problem(as your 2 parameters confused
me), this is just to help hypervisor/secure side to identify the right
channel/shared memory when the same SMC/HVC function id is shared right ?
If that is the case, why can't we just pass the shmem address as the
parameter ? I would like to avoid fragmentation here with every vendor
trying to do different things to achieve the same.
I would just change the driver to do that. Not sure if it breaks any existing
implementation or not. If it does, we can add another compatible to identify
one needing this fixed(shmem address) as additional parameter.
Does that make sense at all ? Or am I missing some/all of the requirements
here ?
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
2023-04-11 13:01 ` Sudeep Holla
@ 2023-04-11 14:42 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 14:42 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 6:01 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with parameters set
>> to zeros. We are using multiple scmi instances within
>> a VM and hypervisor associates a tag with each instance
>> and expects the tag in hvc calls from that instance, while
>> sharing the same smc-id(func_id) among the instances.
>>
>> This patch series introduces new optional dtb bindings which
>> can be used to pass parameters to smc/hvc calls.
>>
> Just to be sure that I understood the problem(as your 2 parameters confused
> me), this is just to help hypervisor/secure side to identify the right
> channel/shared memory when the same SMC/HVC function id is shared right ?
that's somewhat correct. Our hypervisor allocates 64bit ids on every
boot for each scmi instance which it uses to identify the scmi instance.
Additionally, our hypervisor also categorizes events within each
instance by an event-id that gets passed to hvc call as second
parameter. So we can pass two parameters in addition to function_id.
>
> If that is the case, why can't we just pass the shmem address as the
> parameter ? I would like to avoid fragmentation here with every vendor
> trying to do different things to achieve the same.
that's a good suggestion. Any solution you propose shouldn't just limit
to only one parameter. IMO, there should be some way to pass all 6
parameters since we do have a use case of at least two parameters.
>
> I would just change the driver to do that. Not sure if it breaks any existing
> implementation or not. If it does, we can add another compatible to identify
> one needing this fixed(shmem address) as additional parameter.
>
> Does that make sense at all ? Or am I missing some/all of the requirements
> here ?
The shmem proposal is fine however please also incorporate passing of
other parameters.
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
@ 2023-04-11 14:42 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 14:42 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 6:01 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:56AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with parameters set
>> to zeros. We are using multiple scmi instances within
>> a VM and hypervisor associates a tag with each instance
>> and expects the tag in hvc calls from that instance, while
>> sharing the same smc-id(func_id) among the instances.
>>
>> This patch series introduces new optional dtb bindings which
>> can be used to pass parameters to smc/hvc calls.
>>
> Just to be sure that I understood the problem(as your 2 parameters confused
> me), this is just to help hypervisor/secure side to identify the right
> channel/shared memory when the same SMC/HVC function id is shared right ?
that's somewhat correct. Our hypervisor allocates 64bit ids on every
boot for each scmi instance which it uses to identify the scmi instance.
Additionally, our hypervisor also categorizes events within each
instance by an event-id that gets passed to hvc call as second
parameter. So we can pass two parameters in addition to function_id.
>
> If that is the case, why can't we just pass the shmem address as the
> parameter ? I would like to avoid fragmentation here with every vendor
> trying to do different things to achieve the same.
that's a good suggestion. Any solution you propose shouldn't just limit
to only one parameter. IMO, there should be some way to pass all 6
parameters since we do have a use case of at least two parameters.
>
> I would just change the driver to do that. Not sure if it breaks any existing
> implementation or not. If it does, we can add another compatible to identify
> one needing this fixed(shmem address) as additional parameter.
>
> Does that make sense at all ? Or am I missing some/all of the requirements
> here ?
The shmem proposal is fine however please also incorporate passing of
other parameters.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-11 12:54 ` Sudeep Holla
@ 2023-04-11 14:46 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 14:46 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 5:54 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
> Why 2 values ?
I can do with one property if you prefer that. Its just I wanted to
ensure that whosoever is setting
the parameter list, is mindful of 32bit vs 64bit convention. If we use
one property, do you propose to add a new property like width to specify
if the parameter list is for 32bit vs 64bit?
>> This is useful when multiple scmi instances are used with common smc-id.
>>
> I really would like to avoid this binding. Because of lack of standard
> SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
> case like this in a vendor specific way is something I would like to avoid.
If you have a solution to get rid of FID from DTB node, I will follow
the same however until that happens, my view is that we put the
parameters in dtb.
>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..ecf76b763c8c 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,23 @@ properties:
>> description:
>> SMC id required when using smc or hvc transports
>>
>> + arm,smc32-params:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + An optional parameter list passed in smc32 or hvc32 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
>> + arm,smc64-params:
>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>> + description:
>> + An optional parameter list passed in smc64 or hvc64 calls.
>> + This is valid only on ARM64 machines.
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
> Even if we end up adding(which I would very much like to avoid), I don't see
> the need for 32 and 64 bit params like this. There must be ways to avoid that
> used by some property in some other binding(I will look for one if we choose
> this path)
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-11 14:46 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 14:46 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 5:54 AM, Sudeep Holla wrote:
> On Mon, Apr 10, 2023 at 11:20:57AM -0700, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
> Why 2 values ?
I can do with one property if you prefer that. Its just I wanted to
ensure that whosoever is setting
the parameter list, is mindful of 32bit vs 64bit convention. If we use
one property, do you propose to add a new property like width to specify
if the parameter list is for 32bit vs 64bit?
>> This is useful when multiple scmi instances are used with common smc-id.
>>
> I really would like to avoid this binding. Because of lack of standard
> SMC/HVC FID for SCMI we had to add this binding. Extending for newer use
> case like this in a vendor specific way is something I would like to avoid.
If you have a solution to get rid of FID from DTB node, I will follow
the same however until that happens, my view is that we put the
parameters in dtb.
>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> .../devicetree/bindings/firmware/arm,scmi.yaml | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> index 5824c43e9893..ecf76b763c8c 100644
>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>> @@ -115,6 +115,23 @@ properties:
>> description:
>> SMC id required when using smc or hvc transports
>>
>> + arm,smc32-params:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + An optional parameter list passed in smc32 or hvc32 calls
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
>> + arm,smc64-params:
>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>> + description:
>> + An optional parameter list passed in smc64 or hvc64 calls.
>> + This is valid only on ARM64 machines.
>> + default: 0
>> + minItems: 1
>> + maxItems: 6
>> +
> Even if we end up adding(which I would very much like to avoid), I don't see
> the need for 32 and 64 bit params like this. There must be ways to avoid that
> used by some property in some other binding(I will look for one if we choose
> this path)
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-10 17:33 ` Nikunj Kela
@ 2023-04-11 17:17 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-11 17:17 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 10/04/2023 19:33, Nikunj Kela wrote:
>
> On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
>> On 09/04/2023 20:19, Nikunj Kela wrote:
>>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>>> all set to zeros. This patch defines two optional device tree bindings,
>>> that can be used to pass parameters in smc/hvc calls.
>>>
>>> This is useful when multiple scmi instances are used with common smc-id.
>>>
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> index 5824c43e9893..08c331a79b80 100644
>>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> @@ -115,6 +115,22 @@ properties:
>>> description:
>>> SMC id required when using smc or hvc transports
>>>
>>> + arm,smc32-params:
>>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>>> + description:
>>> + An optional parameter list passed in smc32 or hvc32 calls
>>> + default: 0
>>> + minItems: 1
>>> + maxItems: 6
>>> +
>>> + arm,smc64-params:
>>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>>> + description:
>>> + An optional parameter list passed in smc64 or hvc64 calls
>>> + default: 0
>>> + minItems: 1
>>> + maxItems: 6
>> These do not look like hardware properties and the fact that you need
>> two properties for the same also points that you tied it to specific SW
>> interface.
>
> This is certainly not the H/W property but then smc-id is also not H/W
> property either
Yep, maybe it should be also removed?
>
> but that gets passed via DTB. I could use the same property for both
> however I wasn't sure
>
> which datatype should be used, uint32-array/uint64-array. Moreover, I
> thought if users are
>
> passing parameters, they better know which SMC convention they are using
> hence used two
>
> explicit properties.
Does not solve my concern.
>
>> Why this should be board-specific? Actually better question - why this
>> should be fixed per board? Doesn't my software want to have different
>> parameters, depending on some other condition?
>
> Not sure I follow, I didn't say this is board specific.
But putting it in DTS which is customized per board is then board specific.
> People can use
> the parameters to pass
>
> whatever their S/W demands. SMC/HVC calls are made by passing parameters
> which is what this patch is enabling.
I cannot follow your paragraphs. You cut middle of sentence.
Anyway, if this is to match whatever their SW demands, it is not
Devicetree property.
>
>>
>> You also did not provide any DTS user for this, so difficult to judge
>> usefulness.
>
> The work is still on going and we will push the dts in few months,
> however that shouldn't stop
>
> making changes in advance.
With a proper DTS maybe it would be easier to convince us why SW
interface should be put to Devicetree... but sure, we can skip DTS and
answer is - this looks like SW property and not a DT.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-11 17:17 ` Krzysztof Kozlowski
0 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-11 17:17 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 10/04/2023 19:33, Nikunj Kela wrote:
>
> On 4/10/2023 10:20 AM, Krzysztof Kozlowski wrote:
>> On 09/04/2023 20:19, Nikunj Kela wrote:
>>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>>> all set to zeros. This patch defines two optional device tree bindings,
>>> that can be used to pass parameters in smc/hvc calls.
>>>
>>> This is useful when multiple scmi instances are used with common smc-id.
>>>
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>> .../devicetree/bindings/firmware/arm,scmi.yaml | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> index 5824c43e9893..08c331a79b80 100644
>>> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
>>> @@ -115,6 +115,22 @@ properties:
>>> description:
>>> SMC id required when using smc or hvc transports
>>>
>>> + arm,smc32-params:
>>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>>> + description:
>>> + An optional parameter list passed in smc32 or hvc32 calls
>>> + default: 0
>>> + minItems: 1
>>> + maxItems: 6
>>> +
>>> + arm,smc64-params:
>>> + $ref: /schemas/types.yaml#/definitions/uint64-array
>>> + description:
>>> + An optional parameter list passed in smc64 or hvc64 calls
>>> + default: 0
>>> + minItems: 1
>>> + maxItems: 6
>> These do not look like hardware properties and the fact that you need
>> two properties for the same also points that you tied it to specific SW
>> interface.
>
> This is certainly not the H/W property but then smc-id is also not H/W
> property either
Yep, maybe it should be also removed?
>
> but that gets passed via DTB. I could use the same property for both
> however I wasn't sure
>
> which datatype should be used, uint32-array/uint64-array. Moreover, I
> thought if users are
>
> passing parameters, they better know which SMC convention they are using
> hence used two
>
> explicit properties.
Does not solve my concern.
>
>> Why this should be board-specific? Actually better question - why this
>> should be fixed per board? Doesn't my software want to have different
>> parameters, depending on some other condition?
>
> Not sure I follow, I didn't say this is board specific.
But putting it in DTS which is customized per board is then board specific.
> People can use
> the parameters to pass
>
> whatever their S/W demands. SMC/HVC calls are made by passing parameters
> which is what this patch is enabling.
I cannot follow your paragraphs. You cut middle of sentence.
Anyway, if this is to match whatever their SW demands, it is not
Devicetree property.
>
>>
>> You also did not provide any DTS user for this, so difficult to judge
>> usefulness.
>
> The work is still on going and we will push the dts in few months,
> however that shouldn't stop
>
> making changes in advance.
With a proper DTS maybe it would be easier to convince us why SW
interface should be put to Devicetree... but sure, we can skip DTS and
answer is - this looks like SW property and not a DT.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-10 18:20 ` Nikunj Kela
@ 2023-04-11 17:18 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-11 17:18 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 10/04/2023 20:20, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Since you sent v2 before our discussion finished, let me answer here:
this does not look like property for DT. Do not send new patches without
giving reviewers chance to respond.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-11 17:18 ` Krzysztof Kozlowski
0 siblings, 0 replies; 70+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-11 17:18 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 10/04/2023 20:20, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines two optional device tree bindings,
> that can be used to pass parameters in smc/hvc calls.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Since you sent v2 before our discussion finished, let me answer here:
this does not look like property for DT. Do not send new patches without
giving reviewers chance to respond.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
2023-04-11 17:18 ` Krzysztof Kozlowski
@ 2023-04-11 17:25 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 17:25 UTC (permalink / raw)
To: Krzysztof Kozlowski, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 10:18 AM, Krzysztof Kozlowski wrote:
> On 10/04/2023 20:20, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> Since you sent v2 before our discussion finished, let me answer here:
> this does not look like property for DT. Do not send new patches without
> giving reviewers chance to respond.
Ok. My rationale on that is, if we allow smc-id which goes in r0/w0/x0
registers in smc/hvc call to be part of dtb, then we should allow
parameters which go in r1/w1/x1 to r6/w6/x6 register to be part of dtb.
If you have an alternative suggestion, I am all ears!
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc
@ 2023-04-11 17:25 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-11 17:25 UTC (permalink / raw)
To: Krzysztof Kozlowski, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/11/2023 10:18 AM, Krzysztof Kozlowski wrote:
> On 10/04/2023 20:20, Nikunj Kela wrote:
>> Currently, smc/hvc calls are made with smc-id only. The parameters are
>> all set to zeros. This patch defines two optional device tree bindings,
>> that can be used to pass parameters in smc/hvc calls.
>>
>> This is useful when multiple scmi instances are used with common smc-id.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> Since you sent v2 before our discussion finished, let me answer here:
> this does not look like property for DT. Do not send new patches without
> giving reviewers chance to respond.
Ok. My rationale on that is, if we allow smc-id which goes in r0/w0/x0
registers in smc/hvc call to be part of dtb, then we should allow
parameters which go in r1/w1/x1 to r6/w6/x6 register to be part of dtb.
If you have an alternative suggestion, I am all ears!
> Best regards,
> Krzysztof
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
2023-04-11 14:42 ` Nikunj Kela
@ 2023-04-12 8:37 ` Sudeep Holla
-1 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-12 8:37 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:
> that's a good suggestion. Any solution you propose shouldn't just limit to
> only one parameter. IMO, there should be some way to pass all 6 parameters
> since we do have a use case of at least two parameters.
Please elaborate on your use-case.
> The shmem proposal is fine however please also incorporate passing of other
> parameters.
You are missing the point here. SMC/HVC is just a doorbell and the main point
I made earlier is that there is no need for vendors to try colourful things
here if it is not necessary. So no, I don't want any extra bindings or more
than one param is that is not needed. I will wait for the reason as requested
above.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
@ 2023-04-12 8:37 ` Sudeep Holla
0 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-12 8:37 UTC (permalink / raw)
To: Nikunj Kela
Cc: cristian.marussi, robh+dt, Sudeep Holla, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:
> that's a good suggestion. Any solution you propose shouldn't just limit to
> only one parameter. IMO, there should be some way to pass all 6 parameters
> since we do have a use case of at least two parameters.
Please elaborate on your use-case.
> The shmem proposal is fine however please also incorporate passing of other
> parameters.
You are missing the point here. SMC/HVC is just a doorbell and the main point
I made earlier is that there is no need for vendors to try colourful things
here if it is not necessary. So no, I don't want any extra bindings or more
than one param is that is not needed. I will wait for the reason as requested
above.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
2023-04-12 8:37 ` Sudeep Holla
@ 2023-04-12 14:54 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-12 14:54 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/12/2023 1:37 AM, Sudeep Holla wrote:
> On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:
>
>> that's a good suggestion. Any solution you propose shouldn't just limit to
>> only one parameter. IMO, there should be some way to pass all 6 parameters
>> since we do have a use case of at least two parameters.
> Please elaborate on your use-case.
Based on your comments below, we will change our hypervisor to make use
of shmem.
>
>> The shmem proposal is fine however please also incorporate passing of other
>> parameters.
> You are missing the point here. SMC/HVC is just a doorbell and the main point
> I made earlier is that there is no need for vendors to try colourful things
> here if it is not necessary. So no, I don't want any extra bindings or more
> than one param is that is not needed. I will wait for the reason as requested
> above.
ok, understood. In that case, we will change our hypervisor to use shmem
address as instance identifier. Please add support for one param, thanks!
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v2 0/2] Allow parameter in smc/hvc calls
@ 2023-04-12 14:54 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-12 14:54 UTC (permalink / raw)
To: Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, lkp
On 4/12/2023 1:37 AM, Sudeep Holla wrote:
> On Tue, Apr 11, 2023 at 07:42:50AM -0700, Nikunj Kela wrote:
>
>> that's a good suggestion. Any solution you propose shouldn't just limit to
>> only one parameter. IMO, there should be some way to pass all 6 parameters
>> since we do have a use case of at least two parameters.
> Please elaborate on your use-case.
Based on your comments below, we will change our hypervisor to make use
of shmem.
>
>> The shmem proposal is fine however please also incorporate passing of other
>> parameters.
> You are missing the point here. SMC/HVC is just a doorbell and the main point
> I made earlier is that there is no need for vendors to try colourful things
> here if it is not necessary. So no, I don't want any extra bindings or more
> than one param is that is not needed. I will wait for the reason as requested
> above.
ok, understood. In that case, we will change our hypervisor to use shmem
address as instance identifier. Please add support for one param, thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v3 0/2] Allow parameter in smc/hvc calls
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-17 17:43 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:43 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish
among hvc calls made from different instances.
This patch series introduces new compatible string which
can be used to pass shmem channel address as a parameter
to smc/hvc calls.
---
v3 -> pass shmem channel address as parameter
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
.../bindings/firmware/arm,scmi.yaml | 4 +++
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++-
3 files changed, 29 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v3 0/2] Allow parameter in smc/hvc calls
@ 2023-04-17 17:43 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:43 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish
among hvc calls made from different instances.
This patch series introduces new compatible string which
can be used to pass shmem channel address as a parameter
to smc/hvc calls.
---
v3 -> pass shmem channel address as parameter
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
.../bindings/firmware/arm,scmi.yaml | 4 +++
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++-
3 files changed, 29 insertions(+), 1 deletion(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v3 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
2023-04-17 17:43 ` Nikunj Kela
@ 2023-04-17 17:44 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:44 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address as one parameter in SMC/HVC doorbell.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..e1f39a10d405 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
- description: SCMI compliant firmware with ARM SMC/HVC transport
items:
- const: arm,scmi-smc
+ - description: SCMI compliant firmware with ARM SMC/HVC transport
+ with shmem address as parameter
+ items:
+ - const: arm,scmi-smc-param
- description: SCMI compliant firmware with SCMI Virtio transport.
The virtio transport only supports a single device.
items:
@@ -299,7 +303,9 @@ else:
properties:
compatible:
contains:
- const: arm,scmi-smc
+ enum:
+ - arm,scmi-smc
+ - arm,scmi-smc-param
then:
required:
- arm,smc-id
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v3 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
@ 2023-04-17 17:44 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:44 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address as one parameter in SMC/HVC doorbell.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..e1f39a10d405 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
- description: SCMI compliant firmware with ARM SMC/HVC transport
items:
- const: arm,scmi-smc
+ - description: SCMI compliant firmware with ARM SMC/HVC transport
+ with shmem address as parameter
+ items:
+ - const: arm,scmi-smc-param
- description: SCMI compliant firmware with SCMI Virtio transport.
The virtio transport only supports a single device.
items:
@@ -299,7 +303,9 @@ else:
properties:
compatible:
contains:
- const: arm,scmi-smc
+ enum:
+ - arm,scmi-smc
+ - arm,scmi-smc-param
then:
required:
- arm,smc-id
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-17 17:43 ` Nikunj Kela
@ 2023-04-17 17:44 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:44 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing shmem channel address as parameter
in smc/hvc call. This patch is useful when multiple scmi instances are
using same smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+ { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..e28387346d33 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,9 @@
#include "common.h"
+#define lower32(x) ((u32)((x) & 0xffffffff))
+#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +33,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: smc/hvc calling convention type 64 vs 32
+ * @param: physical address of the shmem channel
*/
struct scmi_smc {
@@ -40,6 +45,8 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ phys_addr_t param;
};
static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
if (ret < 0)
return ret;
+ if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+ scmi_info->param = res.start;
/*
* If there is an interrupt named "a2p", then the service and
* completion of a message is signaled by an interrupt rather than by
@@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+ /*
+ * if SMC32 convention is used, pass 64 bit address in
+ * two parameters
+ */
+ if (!scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id,
+ lower32(scmi_info->param),
+ upper32(scmi_info->param),
+ 0, 0, 0, 0, 0, &res);
+ else
+#endif
+ arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
+ 0, 0, 0, 0, 0, 0, &res);
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-17 17:44 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 17:44 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing shmem channel address as parameter
in smc/hvc call. This patch is useful when multiple scmi instances are
using same smc-id and firmware needs to distiguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+ { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..e28387346d33 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,9 @@
#include "common.h"
+#define lower32(x) ((u32)((x) & 0xffffffff))
+#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +33,8 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @is_smc64: smc/hvc calling convention type 64 vs 32
+ * @param: physical address of the shmem channel
*/
struct scmi_smc {
@@ -40,6 +45,8 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ bool is_smc64;
+ phys_addr_t param;
};
static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
if (ret < 0)
return ret;
+ if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+ scmi_info->param = res.start;
/*
* If there is an interrupt named "a2p", then the service and
* completion of a message is signaled by an interrupt rather than by
@@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
}
scmi_info->func_id = func_id;
+ scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
scmi_info->cinfo = cinfo;
smc_channel_lock_init(scmi_info);
cinfo->transport_info = scmi_info;
@@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+#ifdef CONFIG_ARM64
+ /*
+ * if SMC32 convention is used, pass 64 bit address in
+ * two parameters
+ */
+ if (!scmi_info->is_smc64)
+ arm_smccc_1_1_invoke(scmi_info->func_id,
+ lower32(scmi_info->param),
+ upper32(scmi_info->param),
+ 0, 0, 0, 0, 0, &res);
+ else
+#endif
+ arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
+ 0, 0, 0, 0, 0, 0, &res);
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-17 17:44 ` Nikunj Kela
@ 2023-04-17 18:01 ` Florian Fainelli
-1 siblings, 0 replies; 70+ messages in thread
From: Florian Fainelli @ 2023-04-17 18:01 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/17/23 10:44, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameter
> in smc/hvc call. This patch is useful when multiple scmi instances are
> using same smc-id and firmware needs to distiguish among the instances.
Typo: distinguish.
It really would have been a lot clearer and made a whole lot more sense
to encode a VM ID/channel number within some of the SMCCC parameters,
possibly as part of the function ID itself.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..e28387346d33 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,9 @@
>
> #include "common.h"
>
> +#define lower32(x) ((u32)((x) & 0xffffffff))
> +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
Cannot you use the existing lower_32_bits and upper_32_bits macros from
kernel.h here?
> +
> /**
> * struct scmi_smc - Structure representing a SCMI smc transport
> *
> @@ -30,6 +33,8 @@
> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> * Used when operating in atomic mode.
> * @func_id: smc/hvc call function id
> + * @is_smc64: smc/hvc calling convention type 64 vs 32
> + * @param: physical address of the shmem channel
> */
>
> struct scmi_smc {
> @@ -40,6 +45,8 @@ struct scmi_smc {
> #define INFLIGHT_NONE MSG_TOKEN_MAX
> atomic_t inflight;
> u32 func_id;
> + bool is_smc64;
> + phys_addr_t param;
> };
>
> static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> if (ret < 0)
> return ret;
>
> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> + scmi_info->param = res.start;
There is not even a check that this is going to be part of the kernel's
view of memory, that seems a bit brittle and possibly a security hole,
too. Your hypervisor presumably needs to have carved out some amount of
memory in order for the messages to be written to/read from, and so
would the VM kernel, so eventually we should have a 'reserved-memory'
entry of some sort, no?
> /*
> * If there is an interrupt named "a2p", then the service and
> * completion of a message is signaled by an interrupt rather than by
> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> }
>
> scmi_info->func_id = func_id;
> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
> scmi_info->cinfo = cinfo;
> smc_channel_lock_init(scmi_info);
> cinfo->transport_info = scmi_info;
> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>
> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +#ifdef CONFIG_ARM64
> + /*
> + * if SMC32 convention is used, pass 64 bit address in
> + * two parameters
> + */
> + if (!scmi_info->is_smc64)
There is no need for scmi_info to store is_smc64, just check the func_id
here and declare is_smc64 as a local variable to the function.
Also, another way to approach this would be to encode the parameters
region in 4KB units such that event on a 32-bit system with LPAE you are
guaranteed to fit the region into a 32-bit unsigned long. AFAIR
virtualization and LPAE are indistinguishable on real CPUs?
> + arm_smccc_1_1_invoke(scmi_info->func_id,
> + lower32(scmi_info->param),
> + upper32(scmi_info->param),
> + 0, 0, 0, 0, 0, &res);
> + else
> +#endif
> + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
> + 0, 0, 0, 0, 0, 0, &res);
>
> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> if (res.a0) {
--
Florian
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-17 18:01 ` Florian Fainelli
0 siblings, 0 replies; 70+ messages in thread
From: Florian Fainelli @ 2023-04-17 18:01 UTC (permalink / raw)
To: Nikunj Kela, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/17/23 10:44, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameter
> in smc/hvc call. This patch is useful when multiple scmi instances are
> using same smc-id and firmware needs to distiguish among the instances.
Typo: distinguish.
It really would have been a lot clearer and made a whole lot more sense
to encode a VM ID/channel number within some of the SMCCC parameters,
possibly as part of the function ID itself.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..e28387346d33 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,9 @@
>
> #include "common.h"
>
> +#define lower32(x) ((u32)((x) & 0xffffffff))
> +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
Cannot you use the existing lower_32_bits and upper_32_bits macros from
kernel.h here?
> +
> /**
> * struct scmi_smc - Structure representing a SCMI smc transport
> *
> @@ -30,6 +33,8 @@
> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> * Used when operating in atomic mode.
> * @func_id: smc/hvc call function id
> + * @is_smc64: smc/hvc calling convention type 64 vs 32
> + * @param: physical address of the shmem channel
> */
>
> struct scmi_smc {
> @@ -40,6 +45,8 @@ struct scmi_smc {
> #define INFLIGHT_NONE MSG_TOKEN_MAX
> atomic_t inflight;
> u32 func_id;
> + bool is_smc64;
> + phys_addr_t param;
> };
>
> static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> if (ret < 0)
> return ret;
>
> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> + scmi_info->param = res.start;
There is not even a check that this is going to be part of the kernel's
view of memory, that seems a bit brittle and possibly a security hole,
too. Your hypervisor presumably needs to have carved out some amount of
memory in order for the messages to be written to/read from, and so
would the VM kernel, so eventually we should have a 'reserved-memory'
entry of some sort, no?
> /*
> * If there is an interrupt named "a2p", then the service and
> * completion of a message is signaled by an interrupt rather than by
> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> }
>
> scmi_info->func_id = func_id;
> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
> scmi_info->cinfo = cinfo;
> smc_channel_lock_init(scmi_info);
> cinfo->transport_info = scmi_info;
> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>
> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +#ifdef CONFIG_ARM64
> + /*
> + * if SMC32 convention is used, pass 64 bit address in
> + * two parameters
> + */
> + if (!scmi_info->is_smc64)
There is no need for scmi_info to store is_smc64, just check the func_id
here and declare is_smc64 as a local variable to the function.
Also, another way to approach this would be to encode the parameters
region in 4KB units such that event on a 32-bit system with LPAE you are
guaranteed to fit the region into a 32-bit unsigned long. AFAIR
virtualization and LPAE are indistinguishable on real CPUs?
> + arm_smccc_1_1_invoke(scmi_info->func_id,
> + lower32(scmi_info->param),
> + upper32(scmi_info->param),
> + 0, 0, 0, 0, 0, &res);
> + else
> +#endif
> + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
> + 0, 0, 0, 0, 0, 0, &res);
>
> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> if (res.a0) {
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-17 18:01 ` Florian Fainelli
@ 2023-04-17 18:17 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 18:17 UTC (permalink / raw)
To: Florian Fainelli, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/17/2023 11:01 AM, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
>> This patch add support for passing shmem channel address as parameter
>> in smc/hvc call. This patch is useful when multiple scmi instances are
>> using same smc-id and firmware needs to distiguish among the instances.
>
> Typo: distinguish.
>
Will fix it.
> It really would have been a lot clearer and made a whole lot more
> sense to encode a VM ID/channel number within some of the SMCCC
> parameters, possibly as part of the function ID itself.
smc-id(func-id) is 32 bit long and the spec doesn't define any such
provisions in it. Having said that, there are optional parameters as
session-id and client-id(secureOS-id) that can be passed in w6/r6 and
w7/r7 registers. If maintainers are OK to use dtb to pass them instead,
I can rework the patches.
>
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> drivers/firmware/arm_scmi/driver.c | 1 +
>> drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/driver.c
>> b/drivers/firmware/arm_scmi/driver.c
>> index e7d97b59963b..b5957cc12fee 100644
>> --- a/drivers/firmware/arm_scmi/driver.c
>> +++ b/drivers/firmware/arm_scmi/driver.c
>> @@ -2914,6 +2914,7 @@ static const struct of_device_id
>> scmi_of_match[] = {
>> #endif
>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>> #endif
>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>> diff --git a/drivers/firmware/arm_scmi/smc.c
>> b/drivers/firmware/arm_scmi/smc.c
>> index 93272e4bbd12..e28387346d33 100644
>> --- a/drivers/firmware/arm_scmi/smc.c
>> +++ b/drivers/firmware/arm_scmi/smc.c
>> @@ -20,6 +20,9 @@
>> #include "common.h"
>> +#define lower32(x) ((u32)((x) & 0xffffffff))
>> +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
>
> Cannot you use the existing lower_32_bits and upper_32_bits macros
> from kernel.h here?
>
>> +
>> /**
>> * struct scmi_smc - Structure representing a SCMI smc transport
>> *
>> @@ -30,6 +33,8 @@
>> * @inflight: Atomic flag to protect access to Tx/Rx shared memory
>> area.
>> * Used when operating in atomic mode.
>> * @func_id: smc/hvc call function id
>> + * @is_smc64: smc/hvc calling convention type 64 vs 32
>> + * @param: physical address of the shmem channel
>> */
>> struct scmi_smc {
>> @@ -40,6 +45,8 @@ struct scmi_smc {
>> #define INFLIGHT_NONE MSG_TOKEN_MAX
>> atomic_t inflight;
>> u32 func_id;
>> + bool is_smc64;
>> + phys_addr_t param;
>> };
>> static irqreturn_t smc_msg_done_isr(int irq, void *data)
>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info
>> *cinfo, struct device *dev,
>> if (ret < 0)
>> return ret;
>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>> + scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the
> kernel's view of memory, that seems a bit brittle and possibly a
> security hole, too. Your hypervisor presumably needs to have carved
> out some amount of memory in order for the messages to be written
> to/read from, and so would the VM kernel, so eventually we should have
> a 'reserved-memory' entry of some sort, no?
>
>> /*
>> * If there is an interrupt named "a2p", then the service and
>> * completion of a message is signaled by an interrupt rather
>> than by
>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info
>> *cinfo, struct device *dev,
>> }
>> scmi_info->func_id = func_id;
>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>> scmi_info->cinfo = cinfo;
>> smc_channel_lock_init(scmi_info);
>> cinfo->transport_info = scmi_info;
>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>> scmi_chan_info *cinfo,
>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>> &res);
>> +#ifdef CONFIG_ARM64
>> + /*
>> + * if SMC32 convention is used, pass 64 bit address in
>> + * two parameters
>> + */
>> + if (!scmi_info->is_smc64)
>
> There is no need for scmi_info to store is_smc64, just check the
> func_id here and declare is_smc64 as a local variable to the function.
>
> Also, another way to approach this would be to encode the parameters
> region in 4KB units such that event on a 32-bit system with LPAE you
> are guaranteed to fit the region into a 32-bit unsigned long. AFAIR
> virtualization and LPAE are indistinguishable on real CPUs?
>
>> + arm_smccc_1_1_invoke(scmi_info->func_id,
>> + lower32(scmi_info->param),
>> + upper32(scmi_info->param),
>> + 0, 0, 0, 0, 0, &res);
>> + else
>> +#endif
>> + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
>> + 0, 0, 0, 0, 0, 0, &res);
>> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>> if (res.a0) {
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-17 18:17 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-17 18:17 UTC (permalink / raw)
To: Florian Fainelli, sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/17/2023 11:01 AM, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
>> This patch add support for passing shmem channel address as parameter
>> in smc/hvc call. This patch is useful when multiple scmi instances are
>> using same smc-id and firmware needs to distiguish among the instances.
>
> Typo: distinguish.
>
Will fix it.
> It really would have been a lot clearer and made a whole lot more
> sense to encode a VM ID/channel number within some of the SMCCC
> parameters, possibly as part of the function ID itself.
smc-id(func-id) is 32 bit long and the spec doesn't define any such
provisions in it. Having said that, there are optional parameters as
session-id and client-id(secureOS-id) that can be passed in w6/r6 and
w7/r7 registers. If maintainers are OK to use dtb to pass them instead,
I can rework the patches.
>
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>> drivers/firmware/arm_scmi/driver.c | 1 +
>> drivers/firmware/arm_scmi/smc.c | 25 ++++++++++++++++++++++++-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/driver.c
>> b/drivers/firmware/arm_scmi/driver.c
>> index e7d97b59963b..b5957cc12fee 100644
>> --- a/drivers/firmware/arm_scmi/driver.c
>> +++ b/drivers/firmware/arm_scmi/driver.c
>> @@ -2914,6 +2914,7 @@ static const struct of_device_id
>> scmi_of_match[] = {
>> #endif
>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>> #endif
>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>> diff --git a/drivers/firmware/arm_scmi/smc.c
>> b/drivers/firmware/arm_scmi/smc.c
>> index 93272e4bbd12..e28387346d33 100644
>> --- a/drivers/firmware/arm_scmi/smc.c
>> +++ b/drivers/firmware/arm_scmi/smc.c
>> @@ -20,6 +20,9 @@
>> #include "common.h"
>> +#define lower32(x) ((u32)((x) & 0xffffffff))
>> +#define upper32(x) ((u32)(((u64)(x) >> 32) & 0xffffffff))
>
> Cannot you use the existing lower_32_bits and upper_32_bits macros
> from kernel.h here?
>
>> +
>> /**
>> * struct scmi_smc - Structure representing a SCMI smc transport
>> *
>> @@ -30,6 +33,8 @@
>> * @inflight: Atomic flag to protect access to Tx/Rx shared memory
>> area.
>> * Used when operating in atomic mode.
>> * @func_id: smc/hvc call function id
>> + * @is_smc64: smc/hvc calling convention type 64 vs 32
>> + * @param: physical address of the shmem channel
>> */
>> struct scmi_smc {
>> @@ -40,6 +45,8 @@ struct scmi_smc {
>> #define INFLIGHT_NONE MSG_TOKEN_MAX
>> atomic_t inflight;
>> u32 func_id;
>> + bool is_smc64;
>> + phys_addr_t param;
>> };
>> static irqreturn_t smc_msg_done_isr(int irq, void *data)
>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info
>> *cinfo, struct device *dev,
>> if (ret < 0)
>> return ret;
>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>> + scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the
> kernel's view of memory, that seems a bit brittle and possibly a
> security hole, too. Your hypervisor presumably needs to have carved
> out some amount of memory in order for the messages to be written
> to/read from, and so would the VM kernel, so eventually we should have
> a 'reserved-memory' entry of some sort, no?
>
>> /*
>> * If there is an interrupt named "a2p", then the service and
>> * completion of a message is signaled by an interrupt rather
>> than by
>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info
>> *cinfo, struct device *dev,
>> }
>> scmi_info->func_id = func_id;
>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>> scmi_info->cinfo = cinfo;
>> smc_channel_lock_init(scmi_info);
>> cinfo->transport_info = scmi_info;
>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>> scmi_chan_info *cinfo,
>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>> &res);
>> +#ifdef CONFIG_ARM64
>> + /*
>> + * if SMC32 convention is used, pass 64 bit address in
>> + * two parameters
>> + */
>> + if (!scmi_info->is_smc64)
>
> There is no need for scmi_info to store is_smc64, just check the
> func_id here and declare is_smc64 as a local variable to the function.
>
> Also, another way to approach this would be to encode the parameters
> region in 4KB units such that event on a 32-bit system with LPAE you
> are guaranteed to fit the region into a 32-bit unsigned long. AFAIR
> virtualization and LPAE are indistinguishable on real CPUs?
>
>> + arm_smccc_1_1_invoke(scmi_info->func_id,
>> + lower32(scmi_info->param),
>> + upper32(scmi_info->param),
>> + 0, 0, 0, 0, 0, &res);
>> + else
>> +#endif
>> + arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param,
>> + 0, 0, 0, 0, 0, 0, &res);
>> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>> if (res.a0) {
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-17 18:01 ` Florian Fainelli
@ 2023-04-18 9:58 ` Sudeep Holla
-1 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-18 9:58 UTC (permalink / raw)
To: Florian Fainelli, Nikunj Kela
Cc: cristian.marussi, Sudeep Holla, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameter
> > in smc/hvc call. This patch is useful when multiple scmi instances are
> > using same smc-id and firmware needs to distiguish among the instances.
>
> Typo: distinguish.
>
> It really would have been a lot clearer and made a whole lot more sense to
> encode a VM ID/channel number within some of the SMCCC parameters, possibly
> as part of the function ID itself.
>
Yes I was about to suggest this but then remembered one main reason I have
been given in the past against that:
If the system launches high number of VMs then that means loads of FID
needs to be reserved for SCMI and the hypervisor needs to support it.
Basically it is not scalable which I agree but not sure if such large
number of VMs are used in reality with SCMI. But I agree with the technical
reasoning.
The other reason why I preferred the shmem address itself instead of some
custom VM ID/channel number is that it can easily becomes vendor specific
for no real good reason and then we need to add support for each such
different parameters. Nikunj suggested getting them from DT which I really
don't like if the sole reason is just to identify the channel. We don't
have standard SCMI SMC/HVC but allowing such deviations with params from
DT will just explode with various combinations for silly/no reason.
[...]
> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > if (ret < 0)
> > return ret;
> > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > + scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the kernel's view
> of memory, that seems a bit brittle and possibly a security hole, too. Your
> hypervisor presumably needs to have carved out some amount of memory in
> order for the messages to be written to/read from, and so would the VM
> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
> no?
>
Not disagreeing here. Just checking if my understanding is correct or not.
IIUC, we need reserved-memory if it is part of the memory presented to the
OS right ? You don't need that if it is dedicated memory like part of SRAM
or something similar.
> > /*
> > * If there is an interrupt named "a2p", then the service and
> > * completion of a message is signaled by an interrupt rather than by
> > @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > }
> > scmi_info->func_id = func_id;
> > + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
> > scmi_info->cinfo = cinfo;
> > smc_channel_lock_init(scmi_info);
> > cinfo->transport_info = scmi_info;
> > @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +#ifdef CONFIG_ARM64
> > + /*
> > + * if SMC32 convention is used, pass 64 bit address in
> > + * two parameters
> > + */
> > + if (!scmi_info->is_smc64)
>
> There is no need for scmi_info to store is_smc64, just check the func_id
> here and declare is_smc64 as a local variable to the function.
>
+1
> Also, another way to approach this would be to encode the parameters region
> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
> are indistinguishable on real CPUs?
>
Agree with the idea. But can a single 4kB be used for multiple shmem across
VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
practically.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-18 9:58 ` Sudeep Holla
0 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-04-18 9:58 UTC (permalink / raw)
To: Florian Fainelli, Nikunj Kela
Cc: cristian.marussi, Sudeep Holla, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
> On 4/17/23 10:44, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameter
> > in smc/hvc call. This patch is useful when multiple scmi instances are
> > using same smc-id and firmware needs to distiguish among the instances.
>
> Typo: distinguish.
>
> It really would have been a lot clearer and made a whole lot more sense to
> encode a VM ID/channel number within some of the SMCCC parameters, possibly
> as part of the function ID itself.
>
Yes I was about to suggest this but then remembered one main reason I have
been given in the past against that:
If the system launches high number of VMs then that means loads of FID
needs to be reserved for SCMI and the hypervisor needs to support it.
Basically it is not scalable which I agree but not sure if such large
number of VMs are used in reality with SCMI. But I agree with the technical
reasoning.
The other reason why I preferred the shmem address itself instead of some
custom VM ID/channel number is that it can easily becomes vendor specific
for no real good reason and then we need to add support for each such
different parameters. Nikunj suggested getting them from DT which I really
don't like if the sole reason is just to identify the channel. We don't
have standard SCMI SMC/HVC but allowing such deviations with params from
DT will just explode with various combinations for silly/no reason.
[...]
> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > if (ret < 0)
> > return ret;
> > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > + scmi_info->param = res.start;
>
> There is not even a check that this is going to be part of the kernel's view
> of memory, that seems a bit brittle and possibly a security hole, too. Your
> hypervisor presumably needs to have carved out some amount of memory in
> order for the messages to be written to/read from, and so would the VM
> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
> no?
>
Not disagreeing here. Just checking if my understanding is correct or not.
IIUC, we need reserved-memory if it is part of the memory presented to the
OS right ? You don't need that if it is dedicated memory like part of SRAM
or something similar.
> > /*
> > * If there is an interrupt named "a2p", then the service and
> > * completion of a message is signaled by an interrupt rather than by
> > @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > }
> > scmi_info->func_id = func_id;
> > + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
> > scmi_info->cinfo = cinfo;
> > smc_channel_lock_init(scmi_info);
> > cinfo->transport_info = scmi_info;
> > @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +#ifdef CONFIG_ARM64
> > + /*
> > + * if SMC32 convention is used, pass 64 bit address in
> > + * two parameters
> > + */
> > + if (!scmi_info->is_smc64)
>
> There is no need for scmi_info to store is_smc64, just check the func_id
> here and declare is_smc64 as a local variable to the function.
>
+1
> Also, another way to approach this would be to encode the parameters region
> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
> are indistinguishable on real CPUs?
>
Agree with the idea. But can a single 4kB be used for multiple shmem across
VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
practically.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-18 9:58 ` Sudeep Holla
@ 2023-04-18 14:20 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 14:20 UTC (permalink / raw)
To: Sudeep Holla, Florian Fainelli
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/2023 2:58 AM, Sudeep Holla wrote:
> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>> On 4/17/23 10:44, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameter
>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>> using same smc-id and firmware needs to distiguish among the instances.
>> Typo: distinguish.
>>
>> It really would have been a lot clearer and made a whole lot more sense to
>> encode a VM ID/channel number within some of the SMCCC parameters, possibly
>> as part of the function ID itself.
>>
> Yes I was about to suggest this but then remembered one main reason I have
> been given in the past against that:
> If the system launches high number of VMs then that means loads of FID
> needs to be reserved for SCMI and the hypervisor needs to support it.
> Basically it is not scalable which I agree but not sure if such large
> number of VMs are used in reality with SCMI. But I agree with the technical
> reasoning.
>
> The other reason why I preferred the shmem address itself instead of some
> custom VM ID/channel number is that it can easily becomes vendor specific
> for no real good reason and then we need to add support for each such
> different parameters. Nikunj suggested getting them from DT which I really
> don't like if the sole reason is just to identify the channel. We don't
> have standard SCMI SMC/HVC but allowing such deviations with params from
> DT will just explode with various combinations for silly/no reason.
>
Would you be ok to pass the smc/hvc parameters via kernel parameters in
commandline? If so, I can implement that. I just wanted to keep
everything in one place hence suggested using DTB node.
> [...]
>
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> if (ret < 0)
>>> return ret;
>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> + scmi_info->param = res.start;
>> There is not even a check that this is going to be part of the kernel's view
>> of memory, that seems a bit brittle and possibly a security hole, too. Your
>> hypervisor presumably needs to have carved out some amount of memory in
>> order for the messages to be written to/read from, and so would the VM
>> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
>> no?
>>
> Not disagreeing here. Just checking if my understanding is correct or not.
> IIUC, we need reserved-memory if it is part of the memory presented to the
> OS right ? You don't need that if it is dedicated memory like part of SRAM
> or something similar.
We are not using reserved-memory node. Instead using sram-mmio to carve
out shmem for scmi instances.
>>> /*
>>> * If there is an interrupt named "a2p", then the service and
>>> * completion of a message is signaled by an interrupt rather than by
>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> }
>>> scmi_info->func_id = func_id;
>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>> scmi_info->cinfo = cinfo;
>>> smc_channel_lock_init(scmi_info);
>>> cinfo->transport_info = scmi_info;
>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> +#ifdef CONFIG_ARM64
>>> + /*
>>> + * if SMC32 convention is used, pass 64 bit address in
>>> + * two parameters
>>> + */
>>> + if (!scmi_info->is_smc64)
>> There is no need for scmi_info to store is_smc64, just check the func_id
>> here and declare is_smc64 as a local variable to the function.
>>
> +1
ACK!
>> Also, another way to approach this would be to encode the parameters region
>> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
>> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
>> are indistinguishable on real CPUs?
>>
> Agree with the idea. But can a single 4kB be used for multiple shmem across
> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
> practically.
We have multiple VMs and each VM has multiple instances. We will have
quite a few domains and I am not sure if single page will suffice.
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-18 14:20 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 14:20 UTC (permalink / raw)
To: Sudeep Holla, Florian Fainelli
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/2023 2:58 AM, Sudeep Holla wrote:
> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>> On 4/17/23 10:44, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameter
>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>> using same smc-id and firmware needs to distiguish among the instances.
>> Typo: distinguish.
>>
>> It really would have been a lot clearer and made a whole lot more sense to
>> encode a VM ID/channel number within some of the SMCCC parameters, possibly
>> as part of the function ID itself.
>>
> Yes I was about to suggest this but then remembered one main reason I have
> been given in the past against that:
> If the system launches high number of VMs then that means loads of FID
> needs to be reserved for SCMI and the hypervisor needs to support it.
> Basically it is not scalable which I agree but not sure if such large
> number of VMs are used in reality with SCMI. But I agree with the technical
> reasoning.
>
> The other reason why I preferred the shmem address itself instead of some
> custom VM ID/channel number is that it can easily becomes vendor specific
> for no real good reason and then we need to add support for each such
> different parameters. Nikunj suggested getting them from DT which I really
> don't like if the sole reason is just to identify the channel. We don't
> have standard SCMI SMC/HVC but allowing such deviations with params from
> DT will just explode with various combinations for silly/no reason.
>
Would you be ok to pass the smc/hvc parameters via kernel parameters in
commandline? If so, I can implement that. I just wanted to keep
everything in one place hence suggested using DTB node.
> [...]
>
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> if (ret < 0)
>>> return ret;
>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> + scmi_info->param = res.start;
>> There is not even a check that this is going to be part of the kernel's view
>> of memory, that seems a bit brittle and possibly a security hole, too. Your
>> hypervisor presumably needs to have carved out some amount of memory in
>> order for the messages to be written to/read from, and so would the VM
>> kernel, so eventually we should have a 'reserved-memory' entry of some sort,
>> no?
>>
> Not disagreeing here. Just checking if my understanding is correct or not.
> IIUC, we need reserved-memory if it is part of the memory presented to the
> OS right ? You don't need that if it is dedicated memory like part of SRAM
> or something similar.
We are not using reserved-memory node. Instead using sram-mmio to carve
out shmem for scmi instances.
>>> /*
>>> * If there is an interrupt named "a2p", then the service and
>>> * completion of a message is signaled by an interrupt rather than by
>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> }
>>> scmi_info->func_id = func_id;
>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>> scmi_info->cinfo = cinfo;
>>> smc_channel_lock_init(scmi_info);
>>> cinfo->transport_info = scmi_info;
>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> +#ifdef CONFIG_ARM64
>>> + /*
>>> + * if SMC32 convention is used, pass 64 bit address in
>>> + * two parameters
>>> + */
>>> + if (!scmi_info->is_smc64)
>> There is no need for scmi_info to store is_smc64, just check the func_id
>> here and declare is_smc64 as a local variable to the function.
>>
> +1
ACK!
>> Also, another way to approach this would be to encode the parameters region
>> in 4KB units such that event on a 32-bit system with LPAE you are guaranteed
>> to fit the region into a 32-bit unsigned long. AFAIR virtualization and LPAE
>> are indistinguishable on real CPUs?
>>
> Agree with the idea. But can a single 4kB be used for multiple shmem across
> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is possible
> practically.
We have multiple VMs and each VM has multiple instances. We will have
quite a few domains and I am not sure if single page will suffice.
> --
> Regards,
> Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-18 14:20 ` Nikunj Kela
@ 2023-04-18 16:33 ` Florian Fainelli
-1 siblings, 0 replies; 70+ messages in thread
From: Florian Fainelli @ 2023-04-18 16:33 UTC (permalink / raw)
To: Nikunj Kela, Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/23 07:20, Nikunj Kela wrote:
>
> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>> This patch add support for passing shmem channel address as parameter
>>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>>> using same smc-id and firmware needs to distiguish among the instances.
>>> Typo: distinguish.
>>>
>>> It really would have been a lot clearer and made a whole lot more
>>> sense to
>>> encode a VM ID/channel number within some of the SMCCC parameters,
>>> possibly
>>> as part of the function ID itself.
>>>
>> Yes I was about to suggest this but then remembered one main reason I
>> have
>> been given in the past against that:
>> If the system launches high number of VMs then that means loads of FID
>> needs to be reserved for SCMI and the hypervisor needs to support it.
>> Basically it is not scalable which I agree but not sure if such large
>> number of VMs are used in reality with SCMI. But I agree with the
>> technical
>> reasoning.
>>
>> The other reason why I preferred the shmem address itself instead of some
>> custom VM ID/channel number is that it can easily becomes vendor specific
>> for no real good reason and then we need to add support for each such
>> different parameters. Nikunj suggested getting them from DT which I
>> really
>> don't like if the sole reason is just to identify the channel. We don't
>> have standard SCMI SMC/HVC but allowing such deviations with params from
>> DT will just explode with various combinations for silly/no reason.
>>
> Would you be ok to pass the smc/hvc parameters via kernel parameters in
> commandline? If so, I can implement that. I just wanted to keep
> everything in one place hence suggested using DTB node.
Command line arguments seem a bit unnecessary here and it would force
you to invent a scheme to control per SCMI device/instance parameters.
>
>> [...]
>>
>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info
>>>> *cinfo, struct device *dev,
>>>> if (ret < 0)
>>>> return ret;
>>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>> + scmi_info->param = res.start;
>>> There is not even a check that this is going to be part of the
>>> kernel's view
>>> of memory, that seems a bit brittle and possibly a security hole,
>>> too. Your
>>> hypervisor presumably needs to have carved out some amount of memory in
>>> order for the messages to be written to/read from, and so would the VM
>>> kernel, so eventually we should have a 'reserved-memory' entry of
>>> some sort,
>>> no?
>>>
>> Not disagreeing here. Just checking if my understanding is correct or
>> not.
>> IIUC, we need reserved-memory if it is part of the memory presented to
>> the
>> OS right ? You don't need that if it is dedicated memory like part of
>> SRAM
>> or something similar.
> We are not using reserved-memory node. Instead using sram-mmio to carve
> out shmem for scmi instances.
OK, that works too.
>>>> /*
>>>> * If there is an interrupt named "a2p", then the service and
>>>> * completion of a message is signaled by an interrupt rather
>>>> than by
>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info
>>>> *cinfo, struct device *dev,
>>>> }
>>>> scmi_info->func_id = func_id;
>>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>> scmi_info->cinfo = cinfo;
>>>> smc_channel_lock_init(scmi_info);
>>>> cinfo->transport_info = scmi_info;
>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>>>> scmi_chan_info *cinfo,
>>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>>>> &res);
>>>> +#ifdef CONFIG_ARM64
>>>> + /*
>>>> + * if SMC32 convention is used, pass 64 bit address in
>>>> + * two parameters
>>>> + */
>>>> + if (!scmi_info->is_smc64)
>>> There is no need for scmi_info to store is_smc64, just check the func_id
>>> here and declare is_smc64 as a local variable to the function.
>>>
>> +1
> ACK!
>>> Also, another way to approach this would be to encode the parameters
>>> region
>>> in 4KB units such that event on a 32-bit system with LPAE you are
>>> guaranteed
>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization
>>> and LPAE
>>> are indistinguishable on real CPUs?
>>>
>> Agree with the idea. But can a single 4kB be used for multiple shmem
>> across
>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is
>> possible
>> practically.
> We have multiple VMs and each VM has multiple instances. We will have
> quite a few domains and I am not sure if single page will suffice.
I did not make myself clear enough, you can encode an offset into the
shared memory area, and however big that shared memory area will be,
that offset can be in a 4KB size. So for instance if you have your MMIO
SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the second VM
can use 0x8000_1000 to 0x8000_1fff and so on and so forth.
Even if you need more than 4KB per VM, you can put that information into
the two additional parameters you pass through the SMC/HVC call.
--
Florian
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-18 16:33 ` Florian Fainelli
0 siblings, 0 replies; 70+ messages in thread
From: Florian Fainelli @ 2023-04-18 16:33 UTC (permalink / raw)
To: Nikunj Kela, Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/23 07:20, Nikunj Kela wrote:
>
> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>> This patch add support for passing shmem channel address as parameter
>>>> in smc/hvc call. This patch is useful when multiple scmi instances are
>>>> using same smc-id and firmware needs to distiguish among the instances.
>>> Typo: distinguish.
>>>
>>> It really would have been a lot clearer and made a whole lot more
>>> sense to
>>> encode a VM ID/channel number within some of the SMCCC parameters,
>>> possibly
>>> as part of the function ID itself.
>>>
>> Yes I was about to suggest this but then remembered one main reason I
>> have
>> been given in the past against that:
>> If the system launches high number of VMs then that means loads of FID
>> needs to be reserved for SCMI and the hypervisor needs to support it.
>> Basically it is not scalable which I agree but not sure if such large
>> number of VMs are used in reality with SCMI. But I agree with the
>> technical
>> reasoning.
>>
>> The other reason why I preferred the shmem address itself instead of some
>> custom VM ID/channel number is that it can easily becomes vendor specific
>> for no real good reason and then we need to add support for each such
>> different parameters. Nikunj suggested getting them from DT which I
>> really
>> don't like if the sole reason is just to identify the channel. We don't
>> have standard SCMI SMC/HVC but allowing such deviations with params from
>> DT will just explode with various combinations for silly/no reason.
>>
> Would you be ok to pass the smc/hvc parameters via kernel parameters in
> commandline? If so, I can implement that. I just wanted to keep
> everything in one place hence suggested using DTB node.
Command line arguments seem a bit unnecessary here and it would force
you to invent a scheme to control per SCMI device/instance parameters.
>
>> [...]
>>
>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info
>>>> *cinfo, struct device *dev,
>>>> if (ret < 0)
>>>> return ret;
>>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>> + scmi_info->param = res.start;
>>> There is not even a check that this is going to be part of the
>>> kernel's view
>>> of memory, that seems a bit brittle and possibly a security hole,
>>> too. Your
>>> hypervisor presumably needs to have carved out some amount of memory in
>>> order for the messages to be written to/read from, and so would the VM
>>> kernel, so eventually we should have a 'reserved-memory' entry of
>>> some sort,
>>> no?
>>>
>> Not disagreeing here. Just checking if my understanding is correct or
>> not.
>> IIUC, we need reserved-memory if it is part of the memory presented to
>> the
>> OS right ? You don't need that if it is dedicated memory like part of
>> SRAM
>> or something similar.
> We are not using reserved-memory node. Instead using sram-mmio to carve
> out shmem for scmi instances.
OK, that works too.
>>>> /*
>>>> * If there is an interrupt named "a2p", then the service and
>>>> * completion of a message is signaled by an interrupt rather
>>>> than by
>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct scmi_chan_info
>>>> *cinfo, struct device *dev,
>>>> }
>>>> scmi_info->func_id = func_id;
>>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>> scmi_info->cinfo = cinfo;
>>>> smc_channel_lock_init(scmi_info);
>>>> cinfo->transport_info = scmi_info;
>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>>>> scmi_chan_info *cinfo,
>>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>>>> &res);
>>>> +#ifdef CONFIG_ARM64
>>>> + /*
>>>> + * if SMC32 convention is used, pass 64 bit address in
>>>> + * two parameters
>>>> + */
>>>> + if (!scmi_info->is_smc64)
>>> There is no need for scmi_info to store is_smc64, just check the func_id
>>> here and declare is_smc64 as a local variable to the function.
>>>
>> +1
> ACK!
>>> Also, another way to approach this would be to encode the parameters
>>> region
>>> in 4KB units such that event on a 32-bit system with LPAE you are
>>> guaranteed
>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization
>>> and LPAE
>>> are indistinguishable on real CPUs?
>>>
>> Agree with the idea. But can a single 4kB be used for multiple shmem
>> across
>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it is
>> possible
>> practically.
> We have multiple VMs and each VM has multiple instances. We will have
> quite a few domains and I am not sure if single page will suffice.
I did not make myself clear enough, you can encode an offset into the
shared memory area, and however big that shared memory area will be,
that offset can be in a 4KB size. So for instance if you have your MMIO
SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the second VM
can use 0x8000_1000 to 0x8000_1fff and so on and so forth.
Even if you need more than 4KB per VM, you can put that information into
the two additional parameters you pass through the SMC/HVC call.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
2023-04-18 16:33 ` Florian Fainelli
@ 2023-04-18 17:07 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 17:07 UTC (permalink / raw)
To: Florian Fainelli, Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/2023 9:33 AM, Florian Fainelli wrote:
> On 4/18/23 07:20, Nikunj Kela wrote:
>>
>> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>>> This patch add support for passing shmem channel address as parameter
>>>>> in smc/hvc call. This patch is useful when multiple scmi instances
>>>>> are
>>>>> using same smc-id and firmware needs to distiguish among the
>>>>> instances.
>>>> Typo: distinguish.
>>>>
>>>> It really would have been a lot clearer and made a whole lot more
>>>> sense to
>>>> encode a VM ID/channel number within some of the SMCCC parameters,
>>>> possibly
>>>> as part of the function ID itself.
>>>>
>>> Yes I was about to suggest this but then remembered one main reason
>>> I have
>>> been given in the past against that:
>>> If the system launches high number of VMs then that means loads of FID
>>> needs to be reserved for SCMI and the hypervisor needs to support it.
>>> Basically it is not scalable which I agree but not sure if such large
>>> number of VMs are used in reality with SCMI. But I agree with the
>>> technical
>>> reasoning.
>>>
>>> The other reason why I preferred the shmem address itself instead of
>>> some
>>> custom VM ID/channel number is that it can easily becomes vendor
>>> specific
>>> for no real good reason and then we need to add support for each such
>>> different parameters. Nikunj suggested getting them from DT which I
>>> really
>>> don't like if the sole reason is just to identify the channel. We don't
>>> have standard SCMI SMC/HVC but allowing such deviations with params
>>> from
>>> DT will just explode with various combinations for silly/no reason.
>>>
>> Would you be ok to pass the smc/hvc parameters via kernel parameters
>> in commandline? If so, I can implement that. I just wanted to keep
>> everything in one place hence suggested using DTB node.
>
> Command line arguments seem a bit unnecessary here and it would force
> you to invent a scheme to control per SCMI device/instance parameters.
>
Agreed!
>>
>>> [...]
>>>
>>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>>> + scmi_info->param = res.start;
>>>> There is not even a check that this is going to be part of the
>>>> kernel's view
>>>> of memory, that seems a bit brittle and possibly a security hole,
>>>> too. Your
>>>> hypervisor presumably needs to have carved out some amount of
>>>> memory in
>>>> order for the messages to be written to/read from, and so would the VM
>>>> kernel, so eventually we should have a 'reserved-memory' entry of
>>>> some sort,
>>>> no?
>>>>
>>> Not disagreeing here. Just checking if my understanding is correct
>>> or not.
>>> IIUC, we need reserved-memory if it is part of the memory presented
>>> to the
>>> OS right ? You don't need that if it is dedicated memory like part
>>> of SRAM
>>> or something similar.
>> We are not using reserved-memory node. Instead using sram-mmio to
>> carve out shmem for scmi instances.
>
> OK, that works too.
>
>>>>> /*
>>>>> * If there is an interrupt named "a2p", then the service and
>>>>> * completion of a message is signaled by an interrupt
>>>>> rather than by
>>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>> }
>>>>> scmi_info->func_id = func_id;
>>>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>>> scmi_info->cinfo = cinfo;
>>>>> smc_channel_lock_init(scmi_info);
>>>>> cinfo->transport_info = scmi_info;
>>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>>>>> scmi_chan_info *cinfo,
>>>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>>>>> &res);
>>>>> +#ifdef CONFIG_ARM64
>>>>> + /*
>>>>> + * if SMC32 convention is used, pass 64 bit address in
>>>>> + * two parameters
>>>>> + */
>>>>> + if (!scmi_info->is_smc64)
>>>> There is no need for scmi_info to store is_smc64, just check the
>>>> func_id
>>>> here and declare is_smc64 as a local variable to the function.
>>>>
>>> +1
>> ACK!
>>>> Also, another way to approach this would be to encode the
>>>> parameters region
>>>> in 4KB units such that event on a 32-bit system with LPAE you are
>>>> guaranteed
>>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization
>>>> and LPAE
>>>> are indistinguishable on real CPUs?
>>>>
>>> Agree with the idea. But can a single 4kB be used for multiple shmem
>>> across
>>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it
>>> is possible
>>> practically.
>> We have multiple VMs and each VM has multiple instances. We will have
>> quite a few domains and I am not sure if single page will suffice.
>
> I did not make myself clear enough, you can encode an offset into the
> shared memory area, and however big that shared memory area will be,
> that offset can be in a 4KB size. So for instance if you have your
> MMIO SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the
> second VM can use 0x8000_1000 to 0x8000_1fff and so on and so forth.
>
> Even if you need more than 4KB per VM, you can put that information
> into the two additional parameters you pass through the SMC/HVC call.
Okay. I will float another version, first parameter of smc/hvc call will
be pfn and second the offset!
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter
@ 2023-04-18 17:07 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 17:07 UTC (permalink / raw)
To: Florian Fainelli, Sudeep Holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 4/18/2023 9:33 AM, Florian Fainelli wrote:
> On 4/18/23 07:20, Nikunj Kela wrote:
>>
>> On 4/18/2023 2:58 AM, Sudeep Holla wrote:
>>> On Mon, Apr 17, 2023 at 11:01:13AM -0700, Florian Fainelli wrote:
>>>> On 4/17/23 10:44, Nikunj Kela wrote:
>>>>> This patch add support for passing shmem channel address as parameter
>>>>> in smc/hvc call. This patch is useful when multiple scmi instances
>>>>> are
>>>>> using same smc-id and firmware needs to distiguish among the
>>>>> instances.
>>>> Typo: distinguish.
>>>>
>>>> It really would have been a lot clearer and made a whole lot more
>>>> sense to
>>>> encode a VM ID/channel number within some of the SMCCC parameters,
>>>> possibly
>>>> as part of the function ID itself.
>>>>
>>> Yes I was about to suggest this but then remembered one main reason
>>> I have
>>> been given in the past against that:
>>> If the system launches high number of VMs then that means loads of FID
>>> needs to be reserved for SCMI and the hypervisor needs to support it.
>>> Basically it is not scalable which I agree but not sure if such large
>>> number of VMs are used in reality with SCMI. But I agree with the
>>> technical
>>> reasoning.
>>>
>>> The other reason why I preferred the shmem address itself instead of
>>> some
>>> custom VM ID/channel number is that it can easily becomes vendor
>>> specific
>>> for no real good reason and then we need to add support for each such
>>> different parameters. Nikunj suggested getting them from DT which I
>>> really
>>> don't like if the sole reason is just to identify the channel. We don't
>>> have standard SCMI SMC/HVC but allowing such deviations with params
>>> from
>>> DT will just explode with various combinations for silly/no reason.
>>>
>> Would you be ok to pass the smc/hvc parameters via kernel parameters
>> in commandline? If so, I can implement that. I just wanted to keep
>> everything in one place hence suggested using DTB node.
>
> Command line arguments seem a bit unnecessary here and it would force
> you to invent a scheme to control per SCMI device/instance parameters.
>
Agreed!
>>
>>> [...]
>>>
>>>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>>>> + scmi_info->param = res.start;
>>>> There is not even a check that this is going to be part of the
>>>> kernel's view
>>>> of memory, that seems a bit brittle and possibly a security hole,
>>>> too. Your
>>>> hypervisor presumably needs to have carved out some amount of
>>>> memory in
>>>> order for the messages to be written to/read from, and so would the VM
>>>> kernel, so eventually we should have a 'reserved-memory' entry of
>>>> some sort,
>>>> no?
>>>>
>>> Not disagreeing here. Just checking if my understanding is correct
>>> or not.
>>> IIUC, we need reserved-memory if it is part of the memory presented
>>> to the
>>> OS right ? You don't need that if it is dedicated memory like part
>>> of SRAM
>>> or something similar.
>> We are not using reserved-memory node. Instead using sram-mmio to
>> carve out shmem for scmi instances.
>
> OK, that works too.
>
>>>>> /*
>>>>> * If there is an interrupt named "a2p", then the service and
>>>>> * completion of a message is signaled by an interrupt
>>>>> rather than by
>>>>> @@ -156,6 +165,7 @@ static int smc_chan_setup(struct
>>>>> scmi_chan_info *cinfo, struct device *dev,
>>>>> }
>>>>> scmi_info->func_id = func_id;
>>>>> + scmi_info->is_smc64 = ARM_SMCCC_IS_64(func_id);
>>>>> scmi_info->cinfo = cinfo;
>>>>> smc_channel_lock_init(scmi_info);
>>>>> cinfo->transport_info = scmi_info;
>>>>> @@ -188,7 +198,20 @@ static int smc_send_message(struct
>>>>> scmi_chan_info *cinfo,
>>>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0,
>>>>> &res);
>>>>> +#ifdef CONFIG_ARM64
>>>>> + /*
>>>>> + * if SMC32 convention is used, pass 64 bit address in
>>>>> + * two parameters
>>>>> + */
>>>>> + if (!scmi_info->is_smc64)
>>>> There is no need for scmi_info to store is_smc64, just check the
>>>> func_id
>>>> here and declare is_smc64 as a local variable to the function.
>>>>
>>> +1
>> ACK!
>>>> Also, another way to approach this would be to encode the
>>>> parameters region
>>>> in 4KB units such that event on a 32-bit system with LPAE you are
>>>> guaranteed
>>>> to fit the region into a 32-bit unsigned long. AFAIR virtualization
>>>> and LPAE
>>>> are indistinguishable on real CPUs?
>>>>
>>> Agree with the idea. But can a single 4kB be used for multiple shmem
>>> across
>>> VMs ? IIUC the hypervisor can deal with that, so I prefer it if it
>>> is possible
>>> practically.
>> We have multiple VMs and each VM has multiple instances. We will have
>> quite a few domains and I am not sure if single page will suffice.
>
> I did not make myself clear enough, you can encode an offset into the
> shared memory area, and however big that shared memory area will be,
> that offset can be in a 4KB size. So for instance if you have your
> MMIO SRAM at 0x8000_0000, the first VM can use 0x8000_0ffff, the
> second VM can use 0x8000_1000 to 0x8000_1fff and so on and so forth.
>
> Even if you need more than 4KB per VM, you can put that information
> into the two additional parameters you pass through the SMC/HVC call.
Okay. I will float another version, first parameter of smc/hvc call will
be pfn and second the offset!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 0/2] Allow parameter in smc/hvc calls
2023-04-09 18:19 ` Nikunj Kela
@ 2023-04-18 18:56 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish
among hvc calls made from different instances.
This patch series introduces new compatible string which
can be used to pass shmem channel address as parameters
to smc/hvc calls.
---
v4 -> split shmem address into 4KB-pages and offset
v3 -> pass shmem channel address as parameter
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
3 files changed, 21 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 0/2] Allow parameter in smc/hvc calls
@ 2023-04-18 18:56 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with parameters set
to zeros. We are using multiple scmi instances within
a VM. We are sharing the same smc-id(func_id) with all
scmi instance. The hypervisor needs a way to distinguish
among hvc calls made from different instances.
This patch series introduces new compatible string which
can be used to pass shmem channel address as parameters
to smc/hvc calls.
---
v4 -> split shmem address into 4KB-pages and offset
v3 -> pass shmem channel address as parameter
v2 -> fix the compilation erros on 32bit platform(see below)
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
v1 -> original patches
Nikunj Kela (2):
dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
.../devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
3 files changed, 21 insertions(+), 2 deletions(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
2023-04-18 18:56 ` Nikunj Kela
@ 2023-04-18 18:56 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address(4KB-page, offset) as two parameters in
SMC/HVC doorbell.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ad776911f990 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
- description: SCMI compliant firmware with ARM SMC/HVC transport
items:
- const: arm,scmi-smc
+ - description: SCMI compliant firmware with ARM SMC/HVC transport
+ with shmem address(4KB-page, offset) as parameters
+ items:
+ - const: arm,scmi-smc-param
- description: SCMI compliant firmware with SCMI Virtio transport.
The virtio transport only supports a single device.
items:
@@ -299,7 +303,9 @@ else:
properties:
compatible:
contains:
- const: arm,scmi-smc
+ enum:
+ - arm,scmi-smc
+ - arm,scmi-smc-param
then:
required:
- arm,smc-id
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
@ 2023-04-18 18:56 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
Currently, smc/hvc calls are made with smc-id only. The parameters are
all set to zeros. This patch defines a new compatible string that can
be used to pass shmem address(4KB-page, offset) as two parameters in
SMC/HVC doorbell.
This is useful when multiple scmi instances are used with common smc-id.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..ad776911f990 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -34,6 +34,10 @@ properties:
- description: SCMI compliant firmware with ARM SMC/HVC transport
items:
- const: arm,scmi-smc
+ - description: SCMI compliant firmware with ARM SMC/HVC transport
+ with shmem address(4KB-page, offset) as parameters
+ items:
+ - const: arm,scmi-smc-param
- description: SCMI compliant firmware with SCMI Virtio transport.
The virtio transport only supports a single device.
items:
@@ -299,7 +303,9 @@ else:
properties:
compatible:
contains:
- const: arm,scmi-smc
+ enum:
+ - arm,scmi-smc
+ - arm,scmi-smc-param
then:
required:
- arm,smc-id
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-04-18 18:56 ` Nikunj Kela
@ 2023-04-18 18:56 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing shmem channel address as parameters
in smc/hvc call. The address is split into 4KB-page and offset.
This patch is useful when multiple scmi instances are using same smc-id
and firmware needs to distinguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+ { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..71e080b70df5 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,11 @@
#include "common.h"
+#define SHMEM_SHIFT 12
+#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
+#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
+#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +35,7 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @param: physical address of the shmem channel
*/
struct scmi_smc {
@@ -40,6 +46,7 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ phys_addr_t param;
};
static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
if (ret < 0)
return ret;
+ if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+ scmi_info->param = res.start;
/*
* If there is an interrupt named "a2p", then the service and
* completion of a message is signaled by an interrupt rather than by
@@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ unsigned long page = SHMEM_PAGE(scmi_info->param);
+ unsigned long offset = SHMEM_OFFSET(scmi_info->param);
/*
* Channel will be released only once response has been
@@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
+ &res);
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 70+ messages in thread
* [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-04-18 18:56 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-18 18:56 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel, Nikunj Kela
This patch add support for passing shmem channel address as parameters
in smc/hvc call. The address is split into 4KB-page and offset.
This patch is useful when multiple scmi instances are using same smc-id
and firmware needs to distinguish among the instances.
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
drivers/firmware/arm_scmi/driver.c | 1 +
drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e7d97b59963b..b5957cc12fee 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+ { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 93272e4bbd12..71e080b70df5 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -20,6 +20,11 @@
#include "common.h"
+#define SHMEM_SHIFT 12
+#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
+#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
+#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
+
/**
* struct scmi_smc - Structure representing a SCMI smc transport
*
@@ -30,6 +35,7 @@
* @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
* Used when operating in atomic mode.
* @func_id: smc/hvc call function id
+ * @param: physical address of the shmem channel
*/
struct scmi_smc {
@@ -40,6 +46,7 @@ struct scmi_smc {
#define INFLIGHT_NONE MSG_TOKEN_MAX
atomic_t inflight;
u32 func_id;
+ phys_addr_t param;
};
static irqreturn_t smc_msg_done_isr(int irq, void *data)
@@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
if (ret < 0)
return ret;
+ if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
+ scmi_info->param = res.start;
/*
* If there is an interrupt named "a2p", then the service and
* completion of a message is signaled by an interrupt rather than by
@@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
{
struct scmi_smc *scmi_info = cinfo->transport_info;
struct arm_smccc_res res;
+ unsigned long page = SHMEM_PAGE(scmi_info->param);
+ unsigned long offset = SHMEM_OFFSET(scmi_info->param);
/*
* Channel will be released only once response has been
@@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
- arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+ arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
+ &res);
/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
if (res.a0) {
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH v4 0/2] Allow parameter in smc/hvc calls
2023-04-18 18:56 ` Nikunj Kela
@ 2023-04-25 14:01 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-25 14:01 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
Hi Sudeep, could you please review v4 patches. Thanks!
On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM. We are sharing the same smc-id(func_id) with all
> scmi instance. The hypervisor needs a way to distinguish
> among hvc calls made from different instances.
>
> This patch series introduces new compatible string which
> can be used to pass shmem channel address as parameters
> to smc/hvc calls.
>
> ---
> v4 -> split shmem address into 4KB-pages and offset
>
> v3 -> pass shmem channel address as parameter
>
> v2 -> fix the compilation erros on 32bit platform(see below)
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
>
> v1 -> original patches
>
> Nikunj Kela (2):
> dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
> firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
>
> .../devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 0/2] Allow parameter in smc/hvc calls
@ 2023-04-25 14:01 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-04-25 14:01 UTC (permalink / raw)
To: sudeep.holla
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
Hi Sudeep, could you please review v4 patches. Thanks!
On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with parameters set
> to zeros. We are using multiple scmi instances within
> a VM. We are sharing the same smc-id(func_id) with all
> scmi instance. The hypervisor needs a way to distinguish
> among hvc calls made from different instances.
>
> This patch series introduces new compatible string which
> can be used to pass shmem channel address as parameters
> to smc/hvc calls.
>
> ---
> v4 -> split shmem address into 4KB-pages and offset
>
> v3 -> pass shmem channel address as parameter
>
> v2 -> fix the compilation erros on 32bit platform(see below)
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202304100606.kUjhsRYf-lkp@intel.com/
>
> v1 -> original patches
>
> Nikunj Kela (2):
> dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
> firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
>
> .../devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
2023-04-18 18:56 ` Nikunj Kela
@ 2023-04-25 16:50 ` Rob Herring
-1 siblings, 0 replies; 70+ messages in thread
From: Rob Herring @ 2023-04-25 16:50 UTC (permalink / raw)
To: Nikunj Kela
Cc: linux-kernel, krzysztof.kozlowski+dt, cristian.marussi,
sudeep.holla, devicetree, robh+dt, linux-arm-kernel
On Tue, 18 Apr 2023 11:56:58 -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines a new compatible string that can
> be used to pass shmem address(4KB-page, offset) as two parameters in
> SMC/HVC doorbell.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call
@ 2023-04-25 16:50 ` Rob Herring
0 siblings, 0 replies; 70+ messages in thread
From: Rob Herring @ 2023-04-25 16:50 UTC (permalink / raw)
To: Nikunj Kela
Cc: linux-kernel, krzysztof.kozlowski+dt, cristian.marussi,
sudeep.holla, devicetree, robh+dt, linux-arm-kernel
On Tue, 18 Apr 2023 11:56:58 -0700, Nikunj Kela wrote:
> Currently, smc/hvc calls are made with smc-id only. The parameters are
> all set to zeros. This patch defines a new compatible string that can
> be used to pass shmem address(4KB-page, offset) as two parameters in
> SMC/HVC doorbell.
>
> This is useful when multiple scmi instances are used with common smc-id.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-04-18 18:56 ` Nikunj Kela
@ 2023-05-01 14:39 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-05-01 14:39 UTC (permalink / raw)
To: sudeep.holla, f.fainelli
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
Reminder: Please review this patch! Thanks
On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameters
> in smc/hvc call. The address is split into 4KB-page and offset.
> This patch is useful when multiple scmi instances are using same smc-id
> and firmware needs to distinguish among the instances.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..71e080b70df5 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,11 @@
>
> #include "common.h"
>
> +#define SHMEM_SHIFT 12
> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> +
> /**
> * struct scmi_smc - Structure representing a SCMI smc transport
> *
> @@ -30,6 +35,7 @@
> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> * Used when operating in atomic mode.
> * @func_id: smc/hvc call function id
> + * @param: physical address of the shmem channel
> */
>
> struct scmi_smc {
> @@ -40,6 +46,7 @@ struct scmi_smc {
> #define INFLIGHT_NONE MSG_TOKEN_MAX
> atomic_t inflight;
> u32 func_id;
> + phys_addr_t param;
> };
>
> static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> if (ret < 0)
> return ret;
>
> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> + scmi_info->param = res.start;
> /*
> * If there is an interrupt named "a2p", then the service and
> * completion of a message is signaled by an interrupt rather than by
> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> {
> struct scmi_smc *scmi_info = cinfo->transport_info;
> struct arm_smccc_res res;
> + unsigned long page = SHMEM_PAGE(scmi_info->param);
> + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
>
> /*
> * Channel will be released only once response has been
> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>
> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> + &res);
>
> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> if (res.a0) {
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-05-01 14:39 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-05-01 14:39 UTC (permalink / raw)
To: sudeep.holla, f.fainelli
Cc: cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
Reminder: Please review this patch! Thanks
On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> This patch add support for passing shmem channel address as parameters
> in smc/hvc call. The address is split into 4KB-page and offset.
> This patch is useful when multiple scmi instances are using same smc-id
> and firmware needs to distinguish among the instances.
>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 1 +
> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index e7d97b59963b..b5957cc12fee 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> #endif
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 93272e4bbd12..71e080b70df5 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -20,6 +20,11 @@
>
> #include "common.h"
>
> +#define SHMEM_SHIFT 12
> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> +
> /**
> * struct scmi_smc - Structure representing a SCMI smc transport
> *
> @@ -30,6 +35,7 @@
> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> * Used when operating in atomic mode.
> * @func_id: smc/hvc call function id
> + * @param: physical address of the shmem channel
> */
>
> struct scmi_smc {
> @@ -40,6 +46,7 @@ struct scmi_smc {
> #define INFLIGHT_NONE MSG_TOKEN_MAX
> atomic_t inflight;
> u32 func_id;
> + phys_addr_t param;
> };
>
> static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> if (ret < 0)
> return ret;
>
> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> + scmi_info->param = res.start;
> /*
> * If there is an interrupt named "a2p", then the service and
> * completion of a message is signaled by an interrupt rather than by
> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> {
> struct scmi_smc *scmi_info = cinfo->transport_info;
> struct arm_smccc_res res;
> + unsigned long page = SHMEM_PAGE(scmi_info->param);
> + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
>
> /*
> * Channel will be released only once response has been
> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>
> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> + &res);
>
> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> if (res.a0) {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-05-01 14:39 ` Nikunj Kela
@ 2023-05-02 10:46 ` Sudeep Holla
-1 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-05-02 10:46 UTC (permalink / raw)
To: Nikunj Kela
Cc: f.fainelli, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
Sudeep Holla, linux-arm-kernel, devicetree, linux-kernel
On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
> Reminder: Please review this patch! Thanks
>
Since the current merge window is open, there is no rush and hence I had put
this on hold until merge window close. Anyways, it looks good in general.
Couple of minor nits below:
> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameters
> > in smc/hvc call. The address is split into 4KB-page and offset.
> > This patch is useful when multiple scmi instances are using same smc-id
> > and firmware needs to distinguish among the instances.
> >
Drop the term "patch". You can refer it as change. It is not match after
it is applied to the git.
> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> > ---
> > drivers/firmware/arm_scmi/driver.c | 1 +
> > drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index e7d97b59963b..b5957cc12fee 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> > #endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> > { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> > #endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > index 93272e4bbd12..71e080b70df5 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -20,6 +20,11 @@
> > #include "common.h"
> > +#define SHMEM_SHIFT 12
> > +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> > +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
Since we are dealing with 4kB pages only, I prefer to do:
#define SHMEM_SIZE (SZ_4K)
#define SHMEM_SHIFT 12
#define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))
> > +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> > +
Also it is definitely worth adding comment about supporting just 4kB pages
and limitations associated with it here(e.g. we can support up to 44 bit
address) and also parameters to the SMC call so that others get clear
idea on how to use it if they need that in the future.
> > /**
> > * struct scmi_smc - Structure representing a SCMI smc transport
> > *
> > @@ -30,6 +35,7 @@
> > * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> > * Used when operating in atomic mode.
> > * @func_id: smc/hvc call function id
> > + * @param: physical address of the shmem channel
> > */
> > struct scmi_smc {
> > @@ -40,6 +46,7 @@ struct scmi_smc {
> > #define INFLIGHT_NONE MSG_TOKEN_MAX
> > atomic_t inflight;
> > u32 func_id;
> > + phys_addr_t param;
> > };
> > static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > if (ret < 0)
> > return ret;
> > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > + scmi_info->param = res.start;
> > /*
> > * If there is an interrupt named "a2p", then the service and
> > * completion of a message is signaled by an interrupt rather than by
> > @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > {
> > struct scmi_smc *scmi_info = cinfo->transport_info;
> > struct arm_smccc_res res;
> > + unsigned long page = SHMEM_PAGE(scmi_info->param);
> > + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
While I see you initialise param in smc_chan_setup, I wonder why the page
and offset itself be initialised once and reused instead of computing the
same fixed value on every smc_send_message. You can probably have param_page
and param_offset stashed instead of just single param value ?
> > /*
> > * Channel will be released only once response has been
> > @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> > + &res);
> > /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> > if (res.a0) {
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-05-02 10:46 ` Sudeep Holla
0 siblings, 0 replies; 70+ messages in thread
From: Sudeep Holla @ 2023-05-02 10:46 UTC (permalink / raw)
To: Nikunj Kela
Cc: f.fainelli, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
Sudeep Holla, linux-arm-kernel, devicetree, linux-kernel
On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
> Reminder: Please review this patch! Thanks
>
Since the current merge window is open, there is no rush and hence I had put
this on hold until merge window close. Anyways, it looks good in general.
Couple of minor nits below:
> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameters
> > in smc/hvc call. The address is split into 4KB-page and offset.
> > This patch is useful when multiple scmi instances are using same smc-id
> > and firmware needs to distinguish among the instances.
> >
Drop the term "patch". You can refer it as change. It is not match after
it is applied to the git.
> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> > ---
> > drivers/firmware/arm_scmi/driver.c | 1 +
> > drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index e7d97b59963b..b5957cc12fee 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> > #endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> > { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> > #endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > index 93272e4bbd12..71e080b70df5 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -20,6 +20,11 @@
> > #include "common.h"
> > +#define SHMEM_SHIFT 12
> > +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> > +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
Since we are dealing with 4kB pages only, I prefer to do:
#define SHMEM_SIZE (SZ_4K)
#define SHMEM_SHIFT 12
#define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))
> > +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> > +
Also it is definitely worth adding comment about supporting just 4kB pages
and limitations associated with it here(e.g. we can support up to 44 bit
address) and also parameters to the SMC call so that others get clear
idea on how to use it if they need that in the future.
> > /**
> > * struct scmi_smc - Structure representing a SCMI smc transport
> > *
> > @@ -30,6 +35,7 @@
> > * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> > * Used when operating in atomic mode.
> > * @func_id: smc/hvc call function id
> > + * @param: physical address of the shmem channel
> > */
> > struct scmi_smc {
> > @@ -40,6 +46,7 @@ struct scmi_smc {
> > #define INFLIGHT_NONE MSG_TOKEN_MAX
> > atomic_t inflight;
> > u32 func_id;
> > + phys_addr_t param;
> > };
> > static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > if (ret < 0)
> > return ret;
> > + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
> > + scmi_info->param = res.start;
> > /*
> > * If there is an interrupt named "a2p", then the service and
> > * completion of a message is signaled by an interrupt rather than by
> > @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > {
> > struct scmi_smc *scmi_info = cinfo->transport_info;
> > struct arm_smccc_res res;
> > + unsigned long page = SHMEM_PAGE(scmi_info->param);
> > + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
While I see you initialise param in smc_chan_setup, I wonder why the page
and offset itself be initialised once and reused instead of computing the
same fixed value on every smc_send_message. You can probably have param_page
and param_offset stashed instead of just single param value ?
> > /*
> > * Channel will be released only once response has been
> > @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> > - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> > + &res);
> > /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> > if (res.a0) {
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
2023-05-02 10:46 ` Sudeep Holla
@ 2023-05-02 14:41 ` Nikunj Kela
-1 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-05-02 14:41 UTC (permalink / raw)
To: Sudeep Holla
Cc: f.fainelli, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 5/2/2023 3:46 AM, Sudeep Holla wrote:
> On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
>> Reminder: Please review this patch! Thanks
>>
> Since the current merge window is open, there is no rush and hence I had put
> this on hold until merge window close. Anyways, it looks good in general.
> Couple of minor nits below:
Sure, thanks!
>> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameters
>>> in smc/hvc call. The address is split into 4KB-page and offset.
>>> This patch is useful when multiple scmi instances are using same smc-id
>>> and firmware needs to distinguish among the instances.
>>>
> Drop the term "patch". You can refer it as change. It is not match after
> it is applied to the git.
ACK!
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>> drivers/firmware/arm_scmi/driver.c | 1 +
>>> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>>> index e7d97b59963b..b5957cc12fee 100644
>>> --- a/drivers/firmware/arm_scmi/driver.c
>>> +++ b/drivers/firmware/arm_scmi/driver.c
>>> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
>>> #endif
>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>>> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>>> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>>> #endif
>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>>> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>>> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
>>> index 93272e4bbd12..71e080b70df5 100644
>>> --- a/drivers/firmware/arm_scmi/smc.c
>>> +++ b/drivers/firmware/arm_scmi/smc.c
>>> @@ -20,6 +20,11 @@
>>> #include "common.h"
>>> +#define SHMEM_SHIFT 12
>>> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
>>> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> Since we are dealing with 4kB pages only, I prefer to do:
> #define SHMEM_SIZE (SZ_4K)
> #define SHMEM_SHIFT 12
> #define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))
ACK!
>>> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
>>> +
> Also it is definitely worth adding comment about supporting just 4kB pages
> and limitations associated with it here(e.g. we can support up to 44 bit
> address) and also parameters to the SMC call so that others get clear
> idea on how to use it if they need that in the future.
ACK!
>>> /**
>>> * struct scmi_smc - Structure representing a SCMI smc transport
>>> *
>>> @@ -30,6 +35,7 @@
>>> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
>>> * Used when operating in atomic mode.
>>> * @func_id: smc/hvc call function id
>>> + * @param: physical address of the shmem channel
>>> */
>>> struct scmi_smc {
>>> @@ -40,6 +46,7 @@ struct scmi_smc {
>>> #define INFLIGHT_NONE MSG_TOKEN_MAX
>>> atomic_t inflight;
>>> u32 func_id;
>>> + phys_addr_t param;
>>> };
>>> static irqreturn_t smc_msg_done_isr(int irq, void *data)
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> if (ret < 0)
>>> return ret;
>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> + scmi_info->param = res.start;
>>> /*
>>> * If there is an interrupt named "a2p", then the service and
>>> * completion of a message is signaled by an interrupt rather than by
>>> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> {
>>> struct scmi_smc *scmi_info = cinfo->transport_info;
>>> struct arm_smccc_res res;
>>> + unsigned long page = SHMEM_PAGE(scmi_info->param);
>>> + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
> While I see you initialise param in smc_chan_setup, I wonder why the page
> and offset itself be initialised once and reused instead of computing the
> same fixed value on every smc_send_message. You can probably have param_page
> and param_offset stashed instead of just single param value ?
Yeah, I did think of that but then I dropped it since in the earlier
versions of patches when I was using a flag to identify smc32/smc64
convention used, I was told to not include it in the scmi_info struct,
instead compute using local variable. Anyway, I will use the two values
as advised!
>>> /*
>>> * Channel will be released only once response has been
>>> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
>>> + &res);
>>> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>>> if (res.a0) {
^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters
@ 2023-05-02 14:41 ` Nikunj Kela
0 siblings, 0 replies; 70+ messages in thread
From: Nikunj Kela @ 2023-05-02 14:41 UTC (permalink / raw)
To: Sudeep Holla
Cc: f.fainelli, cristian.marussi, robh+dt, krzysztof.kozlowski+dt,
linux-arm-kernel, devicetree, linux-kernel
On 5/2/2023 3:46 AM, Sudeep Holla wrote:
> On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
>> Reminder: Please review this patch! Thanks
>>
> Since the current merge window is open, there is no rush and hence I had put
> this on hold until merge window close. Anyways, it looks good in general.
> Couple of minor nits below:
Sure, thanks!
>> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
>>> This patch add support for passing shmem channel address as parameters
>>> in smc/hvc call. The address is split into 4KB-page and offset.
>>> This patch is useful when multiple scmi instances are using same smc-id
>>> and firmware needs to distinguish among the instances.
>>>
> Drop the term "patch". You can refer it as change. It is not match after
> it is applied to the git.
ACK!
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>> drivers/firmware/arm_scmi/driver.c | 1 +
>>> drivers/firmware/arm_scmi/smc.c | 14 +++++++++++++-
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>>> index e7d97b59963b..b5957cc12fee 100644
>>> --- a/drivers/firmware/arm_scmi/driver.c
>>> +++ b/drivers/firmware/arm_scmi/driver.c
>>> @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
>>> #endif
>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
>>> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
>>> + { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
>>> #endif
>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
>>> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
>>> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
>>> index 93272e4bbd12..71e080b70df5 100644
>>> --- a/drivers/firmware/arm_scmi/smc.c
>>> +++ b/drivers/firmware/arm_scmi/smc.c
>>> @@ -20,6 +20,11 @@
>>> #include "common.h"
>>> +#define SHMEM_SHIFT 12
>>> +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
>>> +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))
> Since we are dealing with 4kB pages only, I prefer to do:
> #define SHMEM_SIZE (SZ_4K)
> #define SHMEM_SHIFT 12
> #define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))
ACK!
>>> +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
>>> +
> Also it is definitely worth adding comment about supporting just 4kB pages
> and limitations associated with it here(e.g. we can support up to 44 bit
> address) and also parameters to the SMC call so that others get clear
> idea on how to use it if they need that in the future.
ACK!
>>> /**
>>> * struct scmi_smc - Structure representing a SCMI smc transport
>>> *
>>> @@ -30,6 +35,7 @@
>>> * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
>>> * Used when operating in atomic mode.
>>> * @func_id: smc/hvc call function id
>>> + * @param: physical address of the shmem channel
>>> */
>>> struct scmi_smc {
>>> @@ -40,6 +46,7 @@ struct scmi_smc {
>>> #define INFLIGHT_NONE MSG_TOKEN_MAX
>>> atomic_t inflight;
>>> u32 func_id;
>>> + phys_addr_t param;
>>> };
>>> static irqreturn_t smc_msg_done_isr(int irq, void *data)
>>> @@ -137,6 +144,8 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>>> if (ret < 0)
>>> return ret;
>>> + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param"))
>>> + scmi_info->param = res.start;
>>> /*
>>> * If there is an interrupt named "a2p", then the service and
>>> * completion of a message is signaled by an interrupt rather than by
>>> @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> {
>>> struct scmi_smc *scmi_info = cinfo->transport_info;
>>> struct arm_smccc_res res;
>>> + unsigned long page = SHMEM_PAGE(scmi_info->param);
>>> + unsigned long offset = SHMEM_OFFSET(scmi_info->param);
> While I see you initialise param in smc_chan_setup, I wonder why the page
> and offset itself be initialised once and reused instead of computing the
> same fixed value on every smc_send_message. You can probably have param_page
> and param_offset stashed instead of just single param value ?
Yeah, I did think of that but then I dropped it since in the earlier
versions of patches when I was using a flag to identify smc32/smc64
convention used, I was told to not include it in the scmi_info struct,
instead compute using local variable. Anyway, I will use the two values
as advised!
>>> /*
>>> * Channel will be released only once response has been
>>> @@ -188,7 +199,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>>> shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>>> - arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>>> + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
>>> + &res);
>>> /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>>> if (res.a0) {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 70+ messages in thread
end of thread, other threads:[~2023-05-02 14:42 UTC | newest]
Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-09 18:19 [PATCH 0/2] Allow parameter in smc/hvc calls Nikunj Kela
2023-04-09 18:19 ` Nikunj Kela
2023-04-09 18:19 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc Nikunj Kela
2023-04-09 18:19 ` Nikunj Kela
2023-04-10 17:20 ` Krzysztof Kozlowski
2023-04-10 17:20 ` Krzysztof Kozlowski
2023-04-10 17:33 ` Nikunj Kela
2023-04-10 17:33 ` Nikunj Kela
2023-04-11 17:17 ` Krzysztof Kozlowski
2023-04-11 17:17 ` Krzysztof Kozlowski
2023-04-09 18:19 ` [PATCH 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters Nikunj Kela
2023-04-09 18:19 ` Nikunj Kela
2023-04-09 22:22 ` kernel test robot
2023-04-09 22:22 ` kernel test robot
2023-04-10 18:20 ` [PATCH v2 0/2] Allow parameter in smc/hvc calls Nikunj Kela
2023-04-10 18:20 ` Nikunj Kela
2023-04-10 18:20 ` [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: support parameter passing in smc/hvc Nikunj Kela
2023-04-10 18:20 ` Nikunj Kela
2023-04-11 12:54 ` Sudeep Holla
2023-04-11 12:54 ` Sudeep Holla
2023-04-11 14:46 ` Nikunj Kela
2023-04-11 14:46 ` Nikunj Kela
2023-04-11 17:18 ` Krzysztof Kozlowski
2023-04-11 17:18 ` Krzysztof Kozlowski
2023-04-11 17:25 ` Nikunj Kela
2023-04-11 17:25 ` Nikunj Kela
2023-04-10 18:20 ` [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters Nikunj Kela
2023-04-10 18:20 ` Nikunj Kela
2023-04-11 13:01 ` [PATCH v2 0/2] Allow parameter in smc/hvc calls Sudeep Holla
2023-04-11 13:01 ` Sudeep Holla
2023-04-11 14:42 ` Nikunj Kela
2023-04-11 14:42 ` Nikunj Kela
2023-04-12 8:37 ` Sudeep Holla
2023-04-12 8:37 ` Sudeep Holla
2023-04-12 14:54 ` Nikunj Kela
2023-04-12 14:54 ` Nikunj Kela
2023-04-17 17:43 ` [PATCH v3 " Nikunj Kela
2023-04-17 17:43 ` Nikunj Kela
2023-04-17 17:44 ` [PATCH v3 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call Nikunj Kela
2023-04-17 17:44 ` Nikunj Kela
2023-04-17 17:44 ` [PATCH v3 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameter Nikunj Kela
2023-04-17 17:44 ` Nikunj Kela
2023-04-17 18:01 ` Florian Fainelli
2023-04-17 18:01 ` Florian Fainelli
2023-04-17 18:17 ` Nikunj Kela
2023-04-17 18:17 ` Nikunj Kela
2023-04-18 9:58 ` Sudeep Holla
2023-04-18 9:58 ` Sudeep Holla
2023-04-18 14:20 ` Nikunj Kela
2023-04-18 14:20 ` Nikunj Kela
2023-04-18 16:33 ` Florian Fainelli
2023-04-18 16:33 ` Florian Fainelli
2023-04-18 17:07 ` Nikunj Kela
2023-04-18 17:07 ` Nikunj Kela
2023-04-18 18:56 ` [PATCH v4 0/2] Allow parameter in smc/hvc calls Nikunj Kela
2023-04-18 18:56 ` Nikunj Kela
2023-04-18 18:56 ` [PATCH v4 1/2] dt-bindings: firmware: arm,scmi: support for parameter in smc/hvc call Nikunj Kela
2023-04-18 18:56 ` Nikunj Kela
2023-04-25 16:50 ` Rob Herring
2023-04-25 16:50 ` Rob Herring
2023-04-18 18:56 ` [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters Nikunj Kela
2023-04-18 18:56 ` Nikunj Kela
2023-05-01 14:39 ` Nikunj Kela
2023-05-01 14:39 ` Nikunj Kela
2023-05-02 10:46 ` Sudeep Holla
2023-05-02 10:46 ` Sudeep Holla
2023-05-02 14:41 ` Nikunj Kela
2023-05-02 14:41 ` Nikunj Kela
2023-04-25 14:01 ` [PATCH v4 0/2] Allow parameter in smc/hvc calls Nikunj Kela
2023-04-25 14:01 ` Nikunj Kela
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.