lkml.org 
[lkml]   [2017]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] soc: xilinx: zynqmp: Add firmware interface
From
Date
On 16.8.2017 13:51, Michal Simek 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.
>
>
>>
>>> +
>>> +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.

Sorry got your comment now - will mark that internal functions as static
to get them out of this header.

Thanks,
Michal


\
 
 \ /
  Last update: 2017-08-16 14:06    [W:3.523 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site