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

[Zulkifli, Muhammad Husaini]

Hi Sudeep,

>-----Original Message-----
>From: Michal Simek <michal.simek@xxxxxxxxxx>
>Sent: Friday, October 2, 2020 4:23 PM
>To: Sudeep Holla <sudeep.holla@xxxxxxx>; Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@xxxxxxxxx>
>Cc: Hunter, Adrian <adrian.hunter@xxxxxxxxx>; michal.simek@xxxxxxxxxx;
>ulf.hansson@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Raja Subramanian,
>Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>; arnd@xxxxxxxx; Wan
>Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
>Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi Sudeep,
>
>On 01. 10. 20 17:35, Sudeep Holla wrote:
>> On Thu, Oct 01, 2020 at 10:21:48PM +0800,
>muhammad.husaini.zulkifli@xxxxxxxxx wrote:
>>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@xxxxxxxxx>
>>>
>>> Add generic firmware driver for Keem Bay SOC to support Arm Trusted
>>> Firmware Services call.
>>>
>>> Signed-off-by: Muhammad Husaini Zulkifli
>>> <muhammad.husaini.zulkifli@xxxxxxxxx>
>>> ---
>>>  drivers/firmware/Kconfig                   |   1 +
>>>  drivers/firmware/Makefile                  |   1 +
>>>  drivers/firmware/intel/Kconfig             |  14 +++
>>>  drivers/firmware/intel/Makefile            |   4 +
>>>  drivers/firmware/intel/keembay_smc.c       | 119 +++++++++++++++++++++
>>>  include/linux/firmware/intel/keembay_smc.h |  27 +++++
>>>  6 files changed, 166 insertions(+)
>>>  create mode 100644 drivers/firmware/intel/Kconfig  create mode
>>> 100644 drivers/firmware/intel/Makefile  create mode 100644
>>> drivers/firmware/intel/keembay_smc.c
>>>  create mode 100644 include/linux/firmware/intel/keembay_smc.h
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index fbd785dd0513..41de77d2720e 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -305,5 +305,6 @@ source "drivers/firmware/psci/Kconfig"
>>>  source "drivers/firmware/smccc/Kconfig"
>>>  source "drivers/firmware/tegra/Kconfig"
>>>  source "drivers/firmware/xilinx/Kconfig"
>>> +source "drivers/firmware/intel/Kconfig"
>>>
>>>  endmenu
>>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>>> index 99510be9f5ed..00f295ab9860 100644
>>> --- a/drivers/firmware/Makefile
>>> +++ b/drivers/firmware/Makefile
>>> @@ -33,3 +33,4 @@ obj-y				+= psci/
>>>  obj-y				+= smccc/
>>>  obj-y				+= tegra/
>>>  obj-y				+= xilinx/
>>> +obj-y				+= intel/
>>> diff --git a/drivers/firmware/intel/Kconfig
>>> b/drivers/firmware/intel/Kconfig new file mode 100644 index
>>> 000000000000..b2b7a4e5410b
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/Kconfig
>>> @@ -0,0 +1,14 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only menu "Intel Firmware
>>> +Drivers"
>>> +
>>> +config KEEMBAY_FIRMWARE
>>> +	bool "Enable Keem Bay firmware interface support"
>>> +	depends on HAVE_ARM_SMCCC
>>
>> What is the version of SMCCC implemented ?
Our current Arm Trusted Firmware framework supports v1.1.
Does it mean I should use HAVE_ARM_SMCCC_DISCOVERY?
Not really clear about this. As for HAVE_ARM_SMCCC will include 
support for the SMC and HVC.

>> If SMCCC v1.1+, use HAVE_ARM_SMCCC_DISCOVERY
>>
>>> +	default n
>>> +	help
>>> +	  Firmware interface driver is used by device drivers
>>> +	  to communicate with the arm-trusted-firmware
>>> +	  for platform management services.
>>> +	  If in doubt, say "N".
>>> +
>>> +endmenu
>>> diff --git a/drivers/firmware/intel/Makefile
>>> b/drivers/firmware/intel/Makefile new file mode 100644 index
>>> 000000000000..e6d2e1ea69a7
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/Makefile
>>> @@ -0,0 +1,4 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Makefile for Intel firmwares
>>> +
>>> +obj-$(CONFIG_KEEMBAY_FIRMWARE) = keembay_smc.o
>>> diff --git a/drivers/firmware/intel/keembay_smc.c
>>> b/drivers/firmware/intel/keembay_smc.c
>>> new file mode 100644
>>> index 000000000000..24013cd1f5da
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/keembay_smc.c
>>> @@ -0,0 +1,119 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + *  Copyright (C) 2020-2021, Intel Corporation  */
>>> +
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/init.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_platform.h>
>>> +
>>> +#include <linux/firmware/intel/keembay_smc.h>
>>> +
>>> +static noinline int do_fw_call_fail(u64 arg0, u64 arg1) {
>>> +	return -ENODEV;
>>> +}
>>> +
>>> +/**
>>> + * Simple wrapper functions to be able to use a function pointer
>>> + * Invoke do_fw_call_smc or others in future, depending on the
>>> +configuration  */ static int (*do_fw_call)(u64, u64) =
>>> +do_fw_call_fail;
>>> +
>>> +/**
>>> + * do_fw_call_smc() - Call system-level platform management layer (SMC)
>>> + * @arg0:		Argument 0 to SMC call
>>> + * @arg1:		Argument 1 to SMC call
>>> + *
>>> + * Invoke platform management function via SMC call.
>>> + *
>>> + * Return: Returns status, either success or error  */ static
>>> +noinline int do_fw_call_smc(u64 arg0, u64 arg1) {
>>> +	struct arm_smccc_res res;
>>> +
>>> +	arm_smccc_smc(arg0, arg1, 0, 0, 0, 0, 0, 0, &res);
>>> +
>>> +	return res.a0;
>>> +}
>>> +
>>> +/**
>>> + * keembay_sd_voltage_selection() - Set the IO Pad voltage
>>> + * @volt: voltage selection either 1.8V or 3.3V
>>> + *
>>> + * This function is used to set the IO Line Voltage
>>> + *
>>> + * Return: 0 for success, Invalid for failure  */ int
>>> +keembay_sd_voltage_selection(int volt) {
>>> +	return do_fw_call(KEEMBAY_SIP_FUNC_ID, volt);
>>
>>
>> What are the other uses of this KEEMBAY_SIP_* ?
>> For now I tend to move this to the driver making use of it using
>> arm_smccc_1_1_invoke directly if possible. I don't see the need for
>> this to be separate driver. But do let us know the features
>> implemented in the firmware. If it is not v1.1+, reasons for not
>> upgrading as you need v1.1 for some CPU errata implementation.
>
>This driver has been created based on my request to move it out the mmc
>driver. It looks quite hacky to have arm_smccc_res and call
>arm_smccc_smc() also with some IDs where it is visible that the part of ID is just
>based on any spec.
>Also in v1 he is just calling SMC. But maybe there is going a need to call HVC
>instead which is something what device driver shouldn't decide that's why
>IMHO doing step via firmware driver is much better approach.
>Of course if there is a better/cleaner way how this should be done I am happy
>to get more information about it.
>
>Thanks,
>Michal
>
>