Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1711007 > unrolled thread

Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface Arnd Bergmann <arnd@arndb.de> - 2017-08-14 17:10 +0200
    Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface Arnd Bergmann <arnd@arndb.de> - 2017-08-16 14:50 +0200
      Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface Arnd Bergmann <arnd@arndb.de> - 2017-08-16 17:10 +0200
        Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface Arnd Bergmann <arnd@arndb.de> - 2017-08-17 23:20 +0200

#1711007 — Re: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface

On Fri, Aug 4, 2017 at 3:45 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> +static noinline int do_fw_call_smc(u64 arg0, u64 arg1, u64 arg2,
> +                                  u32 *ret_payload)
> +{
> +       struct arm_smccc_res res;
> +
> +       arm_smccc_smc(arg0, arg1, arg2, 0, 0, 0, 0, 0, &res);
> +
> +       if (ret_payload) {
> +               ret_payload[0] = (u32)res.a0;
> +               ret_payload[1] = (u32)(res.a0 >> 32);
> +               ret_payload[2] = (u32)res.a1;
> +               ret_payload[3] = (u32)(res.a1 >> 32);
> +               ret_payload[4] = (u32)res.a2;
> +       }
> +
> +       return zynqmp_pm_ret_code((enum pm_ret_status)res.a0);
> +}

It looks like you forgot to add the cpu_to_le32/le32_to_cpu conversions
here to make this work on big-endian kernels.

> +
> +static u32 pm_api_version;
> +
> +/**
> + * zynqmp_pm_get_api_version - Get version number of PMU PM firmware
> + * @version:   Returned version value
> + *
> + * Return:     Returns status, either success or error+reason
> + */
> +int zynqmp_pm_get_api_version(u32 *version)
> +{
> +       u32 ret_payload[PAYLOAD_ARG_CNT];
> +
> +       if (!version)
> +               return zynqmp_pm_ret_code(XST_PM_CONFLICT);
> +
> +       /* Check is PM API version already verified */
> +       if (pm_api_version > 0) {
> +               *version = pm_api_version;
> +               return XST_PM_SUCCESS;
> +       }
> +       invoke_pm_fn(GET_API_VERSION, 0, 0, 0, 0, ret_payload);
> +       *version = ret_payload[1];
> +
> +       return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_get_api_version);

How is this supposed to be used? API version number interfaces
are generally problematic, as you don't have that interface any
more if you change the version.

Normally this should be based on the "compatible" string
in DT to find our what you are talking to, in combination with
a list of features that you can query to find out if something
is available that you can't just try out by calling.

> diff --git a/include/linux/soc/xilinx/zynqmp/firmware.h b/include/linux/soc/xilinx/zynqmp/firmware.h
> new file mode 100644
> index 000000000000..5beb5988e3de
> --- /dev/null
> +++ b/include/linux/soc/xilinx/zynqmp/firmware.h
> @@ -0,0 +1,246 @@
> +
> +#ifndef __SOC_ZYNQMP_FIRMWARE_H__
> +#define __SOC_ZYNQMP_FIRMWARE_H__
> +
> +#define ZYNQMP_PM_VERSION_MAJOR        0
> +#define ZYNQMP_PM_VERSION_MINOR        3

Again, having the version number hardcoded in a global constant
seems pointless. If you expect to have to support different incompatible
versions in the future, name the header file firmware-0-3.h and
prefix the constants with the current version.

> +/*
> + * Internal functions
> + */
> +int invoke_pm_fn(u32 pm_api_id, u32 arg0, u32 arg1, u32 arg2, u32 arg3,
> +                u32 *ret_payload);
> +int zynqmp_pm_ret_code(u32 ret_status);
> +
> +/* Miscellaneous API functions */
> +int zynqmp_pm_get_api_version(u32 *version);
> +int zynqmp_pm_get_chipid(u32 *idcode, u32 *version);

The "internal" functions probably shouldn't be declared in a global
header file.

      Arnd

[toc] | [next] | [standalone]

#1712916

On Wed, Aug 16, 2017 at 1:51 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 14.8.2017 17:06, Arnd Bergmann wrote:
>> On Fri, Aug 4, 2017 at 3:45 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>>> +static noinline int do_fw_call_smc(u64 arg0, u64 arg1, u64 arg2,
>>> +                                  u32 *ret_payload)
>>> +{
>>> +       struct arm_smccc_res res;
>>> +
>>> +       arm_smccc_smc(arg0, arg1, arg2, 0, 0, 0, 0, 0, &res);
>>> +
>>> +       if (ret_payload) {
>>> +               ret_payload[0] = (u32)res.a0;
>>> +               ret_payload[1] = (u32)(res.a0 >> 32);
>>> +               ret_payload[2] = (u32)res.a1;
>>> +               ret_payload[3] = (u32)(res.a1 >> 32);
>>> +               ret_payload[4] = (u32)res.a2;
>>> +       }
>>> +
>>> +       return zynqmp_pm_ret_code((enum pm_ret_status)res.a0);
>>> +}
>>
>> It looks like you forgot to add the cpu_to_le32/le32_to_cpu conversions
>> here to make this work on big-endian kernels.
>
> We have discussed support for big endian kernels in past and discussion
> end up with that there is no customer for this. It means I can change
> this but none will use this.

Ok, thanks. As a general rule, I prefer kernel code to be written
in a portable way even when you assume that is not necessary.

Besides the obvious problem of users that end up wanting to do
something you don't expect, there is the more general issue of
copying code into another driver that may need to be more portable.

>>> +static u32 pm_api_version;
>>> +
>>> +/**
>>> + * zynqmp_pm_get_api_version - Get version number of PMU PM firmware
>>> + * @version:   Returned version value
>>> + *
>>> + * Return:     Returns status, either success or error+reason
>>> + */
>>> +int zynqmp_pm_get_api_version(u32 *version)
>>> +{
>>> +       u32 ret_payload[PAYLOAD_ARG_CNT];
>>> +
>>> +       if (!version)
>>> +               return zynqmp_pm_ret_code(XST_PM_CONFLICT);
>>> +
>>> +       /* Check is PM API version already verified */
>>> +       if (pm_api_version > 0) {
>>> +               *version = pm_api_version;
>>> +               return XST_PM_SUCCESS;
>>> +       }
>>> +       invoke_pm_fn(GET_API_VERSION, 0, 0, 0, 0, ret_payload);
>>> +       *version = ret_payload[1];
>>> +
>>> +       return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
>>> +}
>>> +EXPORT_SYMBOL_GPL(zynqmp_pm_get_api_version);
>>
>> How is this supposed to be used? API version number interfaces
>> are generally problematic, as you don't have that interface any
>> more if you change the version.
>
> This function is called from power management driver to find out a
> version of PMUFW. It is not a problem to save version in the driver and
> provide another function to access it instead of asking firmware again.
> Or also remove this completely because it is more for power management
> then for communication. And this patch is just about communication.

Ok. For the purpose of the power management driver, you
probably also want a different name, as what you are interested
in is not the API version but the firmware version.

>> Normally this should be based on the "compatible" string
>> in DT to find our what you are talking to, in combination with
>> a list of features that you can query to find out if something
>> is available that you can't just try out by calling.
>
> How can you find out what you are talking to without asking for version?
>
> It should be probably be based on some sort of list of services and
> based on that enabled features.

My point was that you can't even ask for a version number without
first knowing what you are talking to, and that information comes from
the DT node describing the interface.

      Arnd

[toc] | [prev] | [next] | [standalone]

#1713034

On Wed, Aug 16, 2017 at 4:34 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 16.8.2017 16:00, Michal Simek wrote:
>> On 16.8.2017 14:41, Arnd Bergmann wrote:
>>
>
> What do you think?
>                 ret_payload[0] = lower_32_bits(le64_to_cpu(res.a0));
>                 ret_payload[1] = upper_32_bits(le64_to_cpu(res.a0));
>                 ret_payload[2] = lower_32_bits(le64_to_cpu(res.a1));
>                 ret_payload[3] = upper_32_bits(le64_to_cpu(res.a1));
>                 ret_payload[4] = lower_32_bits(le64_to_cpu(res.a2));
>
> There should be probably also change in invoke_pm_fn to do conversion
> from cpu to le64.
>
> int invoke_pm_fn(u32 pm_api_id, u32 arg0, u32 arg1, u32 arg2, u32 arg3,
>                  u32 *ret_payload)
> {
>         /*
>          * Added SIP service call Function Identifier
>          * Make sure to stay in x0 register
>          */
>         u64 smc_arg[4];
>
>         smc_arg[0] = cpu_to_le64(PM_SIP_SVC | pm_api_id);
>         smc_arg[1] = cpu_to_le64(((u64)arg1 << 32) | arg0);
>         smc_arg[2] = cpu_to_le64(((u64)arg3 << 32) | arg2);
>
>         return do_fw_call(smc_arg[0], smc_arg[1], smc_arg[2], ret_payload);
> }
>
> This is not tested on BE just on LE.

Looks good, just make sure you also check with sparse (make C=1)
to ensure you have the right __le64/__le32 types everywhere.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1714390

On Thu, Aug 17, 2017 at 12:48 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 16.8.2017 17:05, Arnd Bergmann wrote:
>> On Wed, Aug 16, 2017 at 4:34 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Looks good, just make sure you also check with sparse (make C=1)
>> to ensure you have the right __le64/__le32 types everywhere.
>
> Are you aware about any doc where it is written that data should be
> passed as little endian?

Looking at http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
now, I think that the structure above is endian-neutral as the arguments
get passed in registers rather than memory.

However, if you pass pointers to data structures in memory, those
data structures would have to be defined with __le32/__le64 types.

> I was playing with it a little bit and this means that these 2(3 with
> hvc) needs to be changed.
>
> asmlinkage void __arm_smccc_smc(__le64 a0, __le64 a1, __le64 a2,
>  __le64 a3,__le64 a4, __le64 a5, __le64 a6, __le64 a7,
> struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
>
>  struct arm_smccc_res {
> -       unsigned long a0;
> -       unsigned long a1;
> -       unsigned long a2;
> -       unsigned long a3;
> +       __le64 a0;
> +       __le64 a1;
> +       __le64 a2;
> +       __le64 a3;
>  };

This is clearly wrong on 32-bit machines, I think this is intentionally
defined as 'unsigned long' to have register sized arguments.

       Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web