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

[Arnd Bergmann]

On Wed, Aug 16, 2017 at 1:51 PM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> On 14.8.2017 17:06, Arnd Bergmann wrote:
>> On Fri, Aug 4, 2017 at 3:45 PM, Michal Simek <michal.simek@xxxxxxxxxx> 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