by Nikunj Kela

[permalink] [raw]

Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

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 <[email protected]>
>>> ---
>>> 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) {