Hi, >-----Original Message----- >From: Michal Simek <michal.si...@xilinx.com> >Sent: Wednesday, October 7, 2020 9:37 PM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>; >Michal Simek <michal.si...@xilinx.com>; Hunter, Adrian ><adrian.hun...@intel.com>; sudeep.ho...@arm.com; ulf.hans...@linaro.org; >linux-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- >ker...@vger.kernel.org >Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subraman...@intel.com>; >Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.moha...@intel.com>; a...@arndb.de >Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >Hi, > >On 07. 10. 20 15:21, Zulkifli, Muhammad Husaini wrote: >> Hi Michal, >> >> Thanks for the feedback. I replied inline >> >>> -----Original Message----- >>> From: Michal Simek <michal.si...@xilinx.com> >>> Sent: Wednesday, October 7, 2020 4:20 PM >>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulki...@intel.com>; >>> Hunter, Adrian <adrian.hun...@intel.com>; michal.si...@xilinx.com; >>> sudeep.ho...@arm.com; ulf.hans...@linaro.org; >>> linux-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; >>> linux-kernel@vger.kernel.org >>> Cc: Raja Subramanian, Lakshmi Bai >>> <lakshmi.bai.raja.subraman...@intel.com>; >>> Wan Mohamad, Wan Ahmad Zainie >>> <wan.ahmad.zainie.wan.moha...@intel.com>; a...@arndb.de >>> Subject: Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm >>> Trusted Firmware Service call >>> >>> Hi, >>> >>> 1. Keem Bay: in subject is wrong. Tools are working with it and you >>> should just use keembay: instead. >> Are you saying like this ? >> Keem Bay: Add support for Arm Trusted Firmware Service call > >like this: >firmware: keembay: Add support for Arm Trusted Firmware Service call > >> >>> >>> 2. This should come first before actual change to keep the tree bisectable. >> Noted. Done the changes >>> >>> On 06. 10. 20 17:55, muhammad.husaini.zulki...@intel.com wrote: >>>> From: Muhammad Husaini Zulkifli >>>> <muhammad.husaini.zulki...@intel.com> >>>> >>>> Add header file to handle API function for device driver to >>>> communicate with Arm Trusted Firmware. >>>> >>>> Signed-off-by: Muhammad Husaini Zulkifli >>>> <muhammad.husaini.zulki...@intel.com> >>>> --- >>>> .../linux/firmware/intel/keembay_firmware.h | 46 +++++++++++++++++++ >>>> 1 file changed, 46 insertions(+) >>>> create mode 100644 include/linux/firmware/intel/keembay_firmware.h >>>> >>>> diff --git a/include/linux/firmware/intel/keembay_firmware.h >>>> b/include/linux/firmware/intel/keembay_firmware.h >>>> new file mode 100644 >>>> index 000000000000..9adb8c87b788 >>>> --- /dev/null >>>> +++ b/include/linux/firmware/intel/keembay_firmware.h >>>> @@ -0,0 +1,46 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* >>>> + * Intel Keembay SOC Firmware API Layer >>>> + * >>>> + * Copyright (C) 2020-2021, Intel Corporation >>>> + * >>>> + * Muhammad Husaini Zulkifli <muhammad.husaini.zulki...@intel.com> >>>> + */ >>>> + >>>> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__ >>>> +#define __FIRMWARE_KEEMBAY_SMC_H__ >>>> + >>>> +#include <linux/arm-smccc.h> >>>> + >>>> +/** >>> >>> This is not a kernel doc comment. Just use /* >>> >>>> + * This file defines API function that can be called by device >>>> + driver in order to >>>> + * communicate with Arm Trusted Firmware. >>>> + */ >>>> + >>>> +/* Setting for Keem Bay IO Pad Line Voltage Selection */ >>>> +#define KEEMBAY_SET_SD_VOLTAGE_FUNC_ID 0x8200ff26 >>> >>> Sudeep: Don't we have any macros for composing these IDs? >>> nit: IMHO composing these IDs from macros would make more sense to me. >>> >>> >>>> +#define KEEMBAY_SET_1V8_VOLT 0x01 >>> >>> 0x01 is just 1 >> Noted. Done the changes >>> >>>> +#define KEEMBAY_SET_3V3_VOLT 0x00 >>> >>> 0x00 is just 0 >> Noted. Done the changes >>> >>>> + >>>> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) >>>> +static int do_fw_invoke(u64 func_id, u64 arg0, u64 arg1) { >>>> + struct arm_smccc_res res; >>>> + >>>> + arm_smccc_1_1_invoke(func_id, arg0, arg1, &res); >>>> + >>>> + return res.a0; >>> >>> I am not big fan of this error propagation in case of failure. >>> >>> If smc fails you get via res.a0 SMCCC_RET_NOT_SUPPORTED which is >>> defined as >>> -1 which is based on errno-base.h defined as EPERM. >>> >>> That driver which Sudeep pointed you to is using EINVAL instead. >>> >>> It means I would add a code to check it. >> >> Yeah I changed to below line of codes. Is this Ok? Tested working. >> int keembay_sd_voltage_selection(int volt) > >static inline here shouldn't hurt. due to func() prototype " int keembay_sd_voltage_selection(int volt);" to solve warning issues by robot , I cannot set static inline here. Will observed below error:
error: static declaration of ‘keembay_sd_voltage_selection’ follows non-static declaration static inline int keembay_sd_voltage_selection(int volt). > >> { >> struct arm_smccc_res res; >> >> > arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAG >E, volt, &res); >> if ((int)res.a0 < 0) >> return -EINVAL; >> >> return 0; >> } > >This is fine. > >M