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

[Michal Simek]

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 ?
> 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