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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <db60efac-c583-4b2a-3ad9-7bd93dfb5323@xilinx.com>
Date:   Wed, 7 Oct 2020 15:37:04 +0200
From:   Michal Simek <michal.simek@...inx.com>
To:     "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>,
        Michal Simek <michal.simek@...inx.com>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>,
        "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>,
        "arnd@...db.de" <arnd@...db.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.simek@...inx.com>
>> Sent: Wednesday, October 7, 2020 4:20 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
>> Hunter, Adrian <adrian.hunter@...el.com>; michal.simek@...inx.com;
>> sudeep.holla@....com; ulf.hansson@...aro.org; linux-mmc@...r.kernel.org;
>> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
>> Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@...el.com>;
>> Wan Mohamad, Wan Ahmad Zainie
>> <wan.ahmad.zainie.wan.mohamad@...el.com>; arnd@...db.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@...el.com wrote:
>>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.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@...el.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@...el.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