Re: [PATCH v3 2/2] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call

[Michal Simek <michal.simek@xilinx.com>]

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.simek@xilinx.com>
>> Sent: Wednesday, October 7, 2020 4:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
>> Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;
>> sudeep.holla@arm.com; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@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.zulkifli@intel.com wrote:
>>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@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.zulkifli@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.Zulkifli@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.

> {
> 	struct arm_smccc_res res;
> 
> 	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
> 	if ((int)res.a0 < 0)
> 		return -EINVAL;
> 
> 	return 0;
> }

This is fine.

M