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

[Zulkifli, Muhammad Husaini]

Hi Michal,

Thanks for the feedback. I replied inline

>-----Original Message-----
>From: Michal Simek <michal.simek@xxxxxxxxxx>
>Sent: Wednesday, October 7, 2020 4:20 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>;
>Hunter, Adrian <adrian.hunter@xxxxxxxxx>; michal.simek@xxxxxxxxxx;
>sudeep.holla@xxxxxxx; ulf.hansson@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx;
>linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>;
>Wan Mohamad, Wan Ahmad Zainie
><wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; arnd@xxxxxxxx
>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

>
>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@xxxxxxxxx wrote:
>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>>  .../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@xxxxxxxxx>
>> + */
>> +
>> +#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)
{
	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;
}

>
>
>> +}
>> +
>> +int keembay_sd_voltage_selection(int volt)
>
>as was reported by robot
Added the func prototype. 
int keembay_sd_voltage_selection(int volt);
No error observed after that.
>
>> +{
>> +	return do_fw_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, 0); }
>> +#else static inline int keembay_sd_voltage_selection(int volt) {
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
>>
>
>M