Re: [RFC PATCH 1/3] firmware: smccc: LFA: use smcc 1.2

[Vedashree Vidwans <vvidwans@nvidia.com>]

On 10/10/25 17:02, Andre Przywara wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 8 Oct 2025 19:09:05 +0000
> Vedashree Vidwans <vvidwans@nvidia.com> wrote:
> 
> Hi Vedashree,
> 
>> Update driver to use SMCCC 1.2+ version as mentioned in the LFA spec.
> 
> ah, right, good catch, one call is using x4, so this must be the v1.2
> calling convention.
> 
> Just one small thing below...
> 
>> Signed-off-by: Vedashree Vidwans <vvidwans@nvidia.com>
>> ---
>>   drivers/firmware/smccc/lfa_fw.c | 80 +++++++++++++++++++++++----------
>>   1 file changed, 56 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
>> index 1f333237271d8..49f7feb6a211b 100644
>> --- a/drivers/firmware/smccc/lfa_fw.c
>> +++ b/drivers/firmware/smccc/lfa_fw.c
>> @@ -117,9 +117,13 @@ static struct kobject *lfa_dir;
>>
>>   static int get_nr_lfa_components(void)
>>   {
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
> 
>>
>> -     arm_smccc_1_1_invoke(LFA_1_0_FN_GET_INFO, 0x0, &res);
>> +     args.a0 = LFA_1_0_FN_GET_INFO;
>> +     args.a1 = 0; /* lfa_info_selector = 0 */
>> +
>> +     arm_smccc_1_2_invoke(&args, &res);
> 
> I wonder if we can share the same struct for both request and reply?
>          arm_smccc_1_2_invoke(&args, &args);
> 
> Looks like a lot of stack space used for just a few registers.
> Same for the other occasions where we just do the smc once.
> 
> Cheers,
> Andre.
> 
Thank you for the suggestion.
Yes, I think using same struct for arguments and results should work.

Regards,
Veda>>        if (res.a0 != LFA_SUCCESS)
>>                return res.a0;
>>
>> @@ -129,20 +133,23 @@ static int get_nr_lfa_components(void)
>>   static int call_lfa_activate(void *data)
>>   {
>>        struct image_props *attrs = data;
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>> +
>> +     args.a0 = LFA_1_0_FN_ACTIVATE;
>> +     args.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
>> +     /*
>> +      * As we do not support updates requiring a CPU reset (yet),
>> +      * we pass 0 in args.a3 and args.a4, holding the entry point and context
>> +      * ID respectively.
>> +      * We want to force CPU rendezvous if either cpu_rendezvous or
>> +      * cpu_rendezvous_forced is set. The flag value is flipped as
>> +      * it is called skip_cpu_rendezvous in the spec.
>> +      */
>> +     args.a2 = !(attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous);
>>
>>        do {
>> -             /*
>> -              * As we do not support updates requiring a CPU reset (yet),
>> -              * we pass 0 in x3 and x4, holding the entry point and context
>> -              * ID respectively.
>> -              * We want to force CPU rendezvous if either cpu_rendezvous or
>> -              * cpu_rendezvous_forced is set. The flag value is flipped as
>> -              * it is called skip_cpu_rendezvous in the spec.
>> -              */
>> -             arm_smccc_1_1_invoke(LFA_1_0_FN_ACTIVATE, attrs->fw_seq_id,
>> -                     !(attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous),
>> -                     0, 0, &res);
>> +             arm_smccc_1_2_invoke(&args, &res);
>>        } while (res.a0 == 0 && res.a1 == 1);
>>
>>        return res.a0;
>> @@ -150,7 +157,8 @@ static int call_lfa_activate(void *data)
>>
>>   static int activate_fw_image(struct image_props *attrs)
>>   {
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>>        int ret;
>>
>>        /*
>> @@ -159,8 +167,10 @@ static int activate_fw_image(struct image_props *attrs)
>>         * LFA_PRIME/ACTIVATE will need to be called again.
>>         * res.a1 will become 0 once the prime/activate process completes.
>>         */
>> +     args.a0 = LFA_1_0_FN_PRIME;
>> +     args.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
>>        do {
>> -             arm_smccc_1_1_invoke(LFA_1_0_FN_PRIME, attrs->fw_seq_id, &res);
>> +             arm_smccc_1_2_invoke(&args, &res);
>>                if (res.a0 != LFA_SUCCESS) {
>>                        pr_err("LFA_PRIME failed: %s\n",
>>                                lfa_error_strings[-res.a0]);
>> @@ -211,13 +221,16 @@ static ssize_t activation_pending_show(struct kobject *kobj,
>>   {
>>        struct image_props *attrs = container_of(attr, struct image_props,
>>                                         image_attrs[LFA_ATTR_ACT_PENDING]);
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>>
>>        /*
>>         * Activation pending status can change anytime thus we need to update
>>         * and return its current value
>>         */
>> -     arm_smccc_1_1_invoke(LFA_1_0_FN_GET_INVENTORY, attrs->fw_seq_id, &res);
>> +     args.a0 = LFA_1_0_FN_GET_INVENTORY;
>> +     args.a1 = attrs->fw_seq_id;
>> +     arm_smccc_1_2_invoke(&args, &res);
>>        if (res.a0 == LFA_SUCCESS)
>>                attrs->activation_pending = !!(res.a3 & BIT(1));
>>
>> @@ -298,9 +311,12 @@ static ssize_t cancel_store(struct kobject *kobj, struct kobj_attribute *attr,
>>   {
>>        struct image_props *attrs = container_of(attr, struct image_props,
>>                                                 image_attrs[LFA_ATTR_CANCEL]);
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>>
>> -     arm_smccc_1_1_invoke(LFA_1_0_FN_CANCEL, attrs->fw_seq_id, &res);
>> +     args.a0 = LFA_1_0_FN_CANCEL;
>> +     args.a1 = attrs->fw_seq_id;
>> +     arm_smccc_1_2_invoke(&args, &res);
>>
>>        /*
>>         * When firmware activation is called with "skip_cpu_rendezvous=1",
>> @@ -395,14 +411,17 @@ static int create_fw_inventory(char *fw_uuid, int seq_id, u32 image_flags)
>>
>>   static int create_fw_images_tree(void)
>>   {
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>>        struct uuid_regs image_uuid;
>>        char image_id_str[40];
>>        int ret, num_of_components;
>>
>>        num_of_components = get_nr_lfa_components();
>> +     args.a0 = LFA_1_0_FN_GET_INVENTORY;
>>        for (int i = 0; i < num_of_components; i++) {
>> -             arm_smccc_1_1_invoke(LFA_1_0_FN_GET_INVENTORY, i, &res);
>> +             args.a1 = i; /* fw_seq_id under consideration */
>> +             arm_smccc_1_2_invoke(&args, &res);
>>                if (res.a0 == LFA_SUCCESS) {
>>                        image_uuid.uuid_lo = res.a1;
>>                        image_uuid.uuid_hi = res.a2;
>> @@ -420,10 +439,23 @@ static int create_fw_images_tree(void)
>>
>>   static int __init lfa_init(void)
>>   {
>> -     struct arm_smccc_res res = { 0 };
>> +     struct arm_smccc_1_2_regs args = { 0 };
>> +     struct arm_smccc_1_2_regs res = { 0 };
>>        int err;
>>
>> -     arm_smccc_1_1_invoke(LFA_1_0_FN_GET_VERSION, &res);
>> +     /* LFA requires SMCCC version >= 1.2 */
>> +     if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2) {
>> +             pr_err("Not supported with SMCCC version %u", arm_smccc_get_version());
>> +             return -ENODEV;
>> +     }
>> +
>> +     if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
>> +             pr_err("Invalid SMCCC conduit");
>> +             return -ENODEV;
>> +     }
>> +
>> +     args.a0 = LFA_1_0_FN_GET_VERSION;
>> +     arm_smccc_1_2_invoke(&args, &res);
>>        if (res.a0 == -LFA_NOT_SUPPORTED) {
>>                pr_err("Arm Live Firmware activation(LFA): no firmware agent found\n");
>>                return -ENODEV;
>