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

[Michal Simek]

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.


> 
>> +
>> +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.


> 
> 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.

Anyway I don't need this function to be in this interface driver that's
why I will remove this part in v2.

>> 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.

Will remove.

> 
>> +/*
>> + * 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);

This will go out too.

>> +int zynqmp_pm_get_chipid(u32 *idcode, u32 *version);
> 
> The "internal" functions probably shouldn't be declared in a global
> header file.

Ok. This function will be called by nvmem driver which provides an
option to expose chip id to kernel and user space.

I can call this function just once internally and then expose another
function which will simply just provide access to that static variable.
Please let me know what you prefer.

Thanks,
Michal