| Message ID | 20201001142149.23445-3-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC | expand |
On Thu, Oct 01, 2020 at 10:21:48PM +0800, muhammad.husaini.zulkifli@intel.com wrote: > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > > Add generic firmware driver for Keem Bay SOC to support > Arm Trusted Firmware Services call. > > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > 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.
Hi Sudeep, On 01. 10. 20 17:35, Sudeep Holla wrote: > On Thu, Oct 01, 2020 at 10:21:48PM +0800, muhammad.husaini.zulkifli@intel.com wrote: >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> >> >> Add generic firmware driver for Keem Bay SOC to support >> Arm Trusted Firmware Services call. >> >> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> >> --- >> 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
Hi Sudeep, >-----Original Message----- >From: Michal Simek <michal.simek@xilinx.com> >Sent: Friday, October 2, 2020 4:23 PM >To: Sudeep Holla <sudeep.holla@arm.com>; Zulkifli, Muhammad Husaini ><muhammad.husaini.zulkifli@intel.com> >Cc: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com; >ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-arm- >kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Raja Subramanian, >Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan >Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com> >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@intel.com wrote: >>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> >>> >>> Add generic firmware driver for Keem Bay SOC to support Arm Trusted >>> Firmware Services call. >>> >>> Signed-off-by: Muhammad Husaini Zulkifli >>> <muhammad.husaini.zulkifli@intel.com> >>> --- >>> 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 > >
Hi Michal, On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: > Hi Sudeep, > > On 01. 10. 20 17:35, Sudeep Holla wrote: [...] > > > > 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. OK, driver is fine but no dt-bindings as it is discoverable. It can also be just a wrapper library instead as it needs no explicit initialisation like drivers to setup. > 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. Agreed and one must use arm_smccc_get_conduit or something similar. No additional bindings for each and ever platform and driver that uses SMCCC please. > Of course if there is a better/cleaner way how this should be done I am > happy to get more information about it. > Let me know what you think about my thoughts stated above.
On Fri, Oct 02, 2020 at 10:22:46AM +0000, Zulkifli, Muhammad Husaini wrote: > Hi Sudeep, > > >-----Original Message----- > >From: Michal Simek <michal.simek@xilinx.com> > >Sent: Friday, October 2, 2020 4:23 PM > >To: Sudeep Holla <sudeep.holla@arm.com>; Zulkifli, Muhammad Husaini > ><muhammad.husaini.zulkifli@intel.com> > >Cc: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com; > >ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; linux-arm- > >kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Raja Subramanian, > >Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan > >Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com> > >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@intel.com wrote: > >>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > >>> > >>> Add generic firmware driver for Keem Bay SOC to support Arm Trusted > >>> Firmware Services call. > >>> > >>> Signed-off-by: Muhammad Husaini Zulkifli > >>> <muhammad.husaini.zulkifli@intel.com> > >>> --- > >>> 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? Yes, HAVE_ARM_SMCCC_DISCOVERY is right dependency and allows you to get smccc version via arm_smccc_get_version which may be useful in future. > Not really clear about this. As for HAVE_ARM_SMCCC will include > support for the SMC and HVC. > Yes, but for sake of correctness I prefer HAVE_ARM_SMCCC_DISCOVERY.
Hi Sudeep, On 02. 10. 20 12:58, Sudeep Holla wrote: > Hi Michal, > > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: >> Hi Sudeep, >> >> On 01. 10. 20 17:35, Sudeep Holla wrote: > > [...] > >>> >>> 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. > > OK, driver is fine but no dt-bindings as it is discoverable. It can > also be just a wrapper library instead as it needs no explicit > initialisation like drivers to setup. I am fine with it. Do we have any example which we can point him to? > >> 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. > > Agreed and one must use arm_smccc_get_conduit or something similar. No > additional bindings for each and ever platform and driver that uses SMCCC > please. > >> Of course if there is a better/cleaner way how this should be done I am >> happy to get more information about it. >> > > Let me know what you think about my thoughts stated above. I am fine with it. The key point is to have these sort it out because I see that a lot of drivers just simply call that SMCs from drivers which is IMHO wrong. BTW: I see you have added soc id reading which you are saying is the part of smcc v1.2 but I can't see any implementation in TF-A. Is this spec publicly available? Thanks, Michal
Hi Michal, On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: > Hi Sudeep, > > On 02. 10. 20 12:58, Sudeep Holla wrote: > > Hi Michal, > > > > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: > >> Hi Sudeep, > >> > >> On 01. 10. 20 17:35, Sudeep Holla wrote: > > > > [...] > > > >>> > >>> 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. > > > > OK, driver is fine but no dt-bindings as it is discoverable. It can > > also be just a wrapper library instead as it needs no explicit > > initialisation like drivers to setup. > > I am fine with it. Do we have any example which we can point him to? > You seem to have figured that out already with SOC_ID example. That was quick I must say 😄. > > > > >> 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. > > > > Agreed and one must use arm_smccc_get_conduit or something similar. No > > additional bindings for each and ever platform and driver that uses SMCCC > > please. > > > >> Of course if there is a better/cleaner way how this should be done I am > >> happy to get more information about it. > >> > > > > Let me know what you think about my thoughts stated above. > > > I am fine with it. The key point is to have these sort it out because I > see that a lot of drivers just simply call that SMCs from drivers which > is IMHO wrong. > Sure, sorry I didn't express my concern properly. I want to avoid dt bindings for these and use the SMCCC discovery we have in place already if possible. If this driver had consumers in the DT and it needs to be represented in DT, it is a different story and I agree for need for a driver there. But I don't see one in this usecase. > > BTW: I see you have added soc id reading which you are saying is the > part of smcc v1.2 but I can't see any implementation in TF-A. Is this > spec publicly available? > Spec is out[1], include/linux/arm-smccc.h points to the latest spec. TF-A does have implementation as I tested with it and even reported bug that I discovered when I tested with my patches that are now merged upstream. Are you referring to master of TF-A or last release version ? If latter, it had bug and may not be working. I may be wrong though, as I am just telling what was told to me couple of months back and things might have changed in TF-A land. -- Regards, Sudeep [1] https://developer.arm.com/documentation/den0028/latest
Hi Sudeep, >-----Original Message----- >From: Sudeep Holla <sudeep.holla@arm.com> >Sent: Friday, October 2, 2020 10:51 PM >To: Michal Simek <michal.simek@xilinx.com> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >Hunter, Adrian <adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Sudeep Holla ><sudeep.holla@arm.com>; Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.mohamad@intel.com> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >Hi Michal, > >On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: >> Hi Sudeep, >> >> On 02. 10. 20 12:58, Sudeep Holla wrote: >> > Hi Michal, >> > >> > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: >> >> Hi Sudeep, >> >> >> >> On 01. 10. 20 17:35, Sudeep Holla wrote: >> > >> > [...] >> > >> >>> >> >>> 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. >> > >> > OK, driver is fine but no dt-bindings as it is discoverable. It can >> > also be just a wrapper library instead as it needs no explicit >> > initialisation like drivers to setup. >> >> I am fine with it. Do we have any example which we can point him to? >> > >You seem to have figured that out already with SOC_ID example. >That was quick I must say 😄. > >> >> > >> >> 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. I will add func for HVC also in case in the future if someone want to use it. >> > >> > Agreed and one must use arm_smccc_get_conduit or something similar. >> > No additional bindings for each and ever platform and driver that >> > uses SMCCC please. >> > >> >> Of course if there is a better/cleaner way how this should be done >> >> I am happy to get more information about it. >> >> >> > >> > Let me know what you think about my thoughts stated above. >> >> >> I am fine with it. The key point is to have these sort it out because >> I see that a lot of drivers just simply call that SMCs from drivers >> which is IMHO wrong. >> > >Sure, sorry I didn't express my concern properly. I want to avoid dt bindings for >these and use the SMCCC discovery we have in place already if possible. > >If this driver had consumers in the DT and it needs to be represented in DT, it is >a different story and I agree for need for a driver there. >But I don't see one in this usecase. Does it ok if I do some checking in arasan controller driver as below and represented it in the DT of arasan,sdhci.yaml: This is to ensure that for Keem Bay SOC specific, Keembay_firmware node must be consume. if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { struct device_node *dn; struct gpio_desc *uhs; dn = of_find_node_by_name(NULL, "keembay_firmware"); if (!dn) return dev_err_probe(dev, PTR_ERR(dn), "can't find keembay_firmware node\n"); of_node_put(dn); .......... } Thanks > >> >> BTW: I see you have added soc id reading which you are saying is the >> part of smcc v1.2 but I can't see any implementation in TF-A. Is this >> spec publicly available? >> > >Spec is out[1], include/linux/arm-smccc.h points to the latest spec. >TF-A does have implementation as I tested with it and even reported bug that I >discovered when I tested with my patches that are now merged upstream. Are >you referring to master of TF-A or last release version ? >If latter, it had bug and may not be working. I may be wrong though, as I am just >telling what was told to me couple of months back and things might have >changed in TF-A land. > >-- >Regards, >Sudeep > >[1] https://developer.arm.com/documentation/den0028/latest
Hi Sudeep, I am facing an error during sending yesterday. I response again to your feedback as below >-----Original Message----- >From: Sudeep Holla <sudeep.holla@arm.com> >Sent: Friday, October 2, 2020 10:51 PM >To: Michal Simek <michal.simek@xilinx.com> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >Hunter, Adrian <adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Sudeep Holla ><sudeep.holla@arm.com>; Wan Mohamad, Wan Ahmad Zainie ><wan.ahmad.zainie.wan.mohamad@intel.com> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >Hi Michal, > >On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: >> Hi Sudeep, >> >> On 02. 10. 20 12:58, Sudeep Holla wrote: >> > Hi Michal, >> > >> > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: >> >> Hi Sudeep, >> >> >> >> On 01. 10. 20 17:35, Sudeep Holla wrote: >> > >> > [...] >> > >> >>> >> >>> 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. >> > >> > OK, driver is fine but no dt-bindings as it is discoverable. It can >> > also be just a wrapper library instead as it needs no explicit >> > initialisation like drivers to setup. >> >> I am fine with it. Do we have any example which we can point him to? >> > >You seem to have figured that out already with SOC_ID example. >That was quick I must say 😄. > >> >> > >> >> 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. >> > >> > Agreed and one must use arm_smccc_get_conduit or something similar. >> > No additional bindings for each and ever platform and driver that >> > uses SMCCC please. >> > >> >> Of course if there is a better/cleaner way how this should be done >> >> I am happy to get more information about it. >> >> >> > >> > Let me know what you think about my thoughts stated above. >> >> >> I am fine with it. The key point is to have these sort it out because >> I see that a lot of drivers just simply call that SMCs from drivers >> which is IMHO wrong. >> > >Sure, sorry I didn't express my concern properly. I want to avoid dt bindings for >these and use the SMCCC discovery we have in place already if possible. > >If this driver had consumers in the DT and it needs to be represented in DT, it is >a different story and I agree for need for a driver there. >But I don't see one in this usecase. Does it ok if I do some checking in arasan controller driver as below and represented it in the DT of arasan,sdhci.yaml: This is to ensure that for Keem Bay SOC specific, the firmware driver must be consume. if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { struct device_node *dn; struct gpio_desc *uhs; dn = of_find_node_by_name(NULL, "keembay_firmware"); if (!dn) return dev_err_probe(dev, PTR_ERR(dn), "can't find keembay_firmware node\n"); of_node_put(dn); .......... } > >> >> BTW: I see you have added soc id reading which you are saying is the >> part of smcc v1.2 but I can't see any implementation in TF-A. Is this >> spec publicly available? >> > >Spec is out[1], include/linux/arm-smccc.h points to the latest spec. >TF-A does have implementation as I tested with it and even reported bug that I >discovered when I tested with my patches that are now merged upstream. Are >you referring to master of TF-A or last release version ? >If latter, it had bug and may not be working. I may be wrong though, as I am just >telling what was told to me couple of months back and things might have >changed in TF-A land. > >-- >Regards, >Sudeep > >[1] https://developer.arm.com/documentation/den0028/latest
On Mon, Oct 05, 2020 at 08:37:13AM +0000, Zulkifli, Muhammad Husaini wrote: > Hi Sudeep, > > I am facing an error during sending yesterday. > I response again to your feedback as below > > >-----Original Message----- > >From: Sudeep Holla <sudeep.holla@arm.com> > >Sent: Friday, October 2, 2020 10:51 PM > >To: Michal Simek <michal.simek@xilinx.com> > >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; > >Hunter, Adrian <adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- > >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai > ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Sudeep Holla > ><sudeep.holla@arm.com>; Wan Mohamad, Wan Ahmad Zainie > ><wan.ahmad.zainie.wan.mohamad@intel.com> > >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted > >Firmware Service call > > > >Hi Michal, > > > >On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: > >> Hi Sudeep, > >> > >> On 02. 10. 20 12:58, Sudeep Holla wrote: > >> > Hi Michal, > >> > > >> > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: > >> >> Hi Sudeep, > >> >> > >> >> On 01. 10. 20 17:35, Sudeep Holla wrote: > >> > > >> > [...] > >> > > >> >>> > >> >>> 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. > >> > > >> > OK, driver is fine but no dt-bindings as it is discoverable. It can > >> > also be just a wrapper library instead as it needs no explicit > >> > initialisation like drivers to setup. > >> > >> I am fine with it. Do we have any example which we can point him to? > >> > > > >You seem to have figured that out already with SOC_ID example. > >That was quick I must say 😄. > > > >> > >> > > >> >> 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. > >> > > >> > Agreed and one must use arm_smccc_get_conduit or something similar. > >> > No additional bindings for each and ever platform and driver that > >> > uses SMCCC please. > >> > > >> >> Of course if there is a better/cleaner way how this should be done > >> >> I am happy to get more information about it. > >> >> > >> > > >> > Let me know what you think about my thoughts stated above. > >> > >> > >> I am fine with it. The key point is to have these sort it out because > >> I see that a lot of drivers just simply call that SMCs from drivers > >> which is IMHO wrong. > >> > > > >Sure, sorry I didn't express my concern properly. I want to avoid dt bindings for > >these and use the SMCCC discovery we have in place already if possible. > > > >If this driver had consumers in the DT and it needs to be represented in DT, it is > >a different story and I agree for need for a driver there. > >But I don't see one in this usecase. > > > Does it ok if I do some checking in arasan controller driver as below and represented it in the DT of arasan,sdhci.yaml: > This is to ensure that for Keem Bay SOC specific, the firmware driver must be consume. > > if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { > struct device_node *dn; > struct gpio_desc *uhs; > > dn = of_find_node_by_name(NULL, "keembay_firmware"); You have keembay_sd_voltage_selection function as Michal prefers, I have no objections for that. But please no keembay_firmware node in DT. You can implement this as a driver or simple smccc based function library without DT node using SMCCC get_version. I hope the firmware gives error for unimplemented FIDs, thereby eliminating the need for any DT node or config option.
>-----Original Message----- >From: Sudeep Holla <sudeep.holla@arm.com> >Sent: Monday, October 5, 2020 4:45 PM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> >Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian ><adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan Mohamad, >Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >On Mon, Oct 05, 2020 at 08:37:13AM +0000, Zulkifli, Muhammad Husaini wrote: >> Hi Sudeep, >> >> I am facing an error during sending yesterday. >> I response again to your feedback as below >> >> >-----Original Message----- >> >From: Sudeep Holla <sudeep.holla@arm.com> >> >Sent: Friday, October 2, 2020 10:51 PM >> >To: Michal Simek <michal.simek@xilinx.com> >> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >> >Hunter, Adrian <adrian.hunter@intel.com>; ulf.hansson@linaro.org; >> >linux- mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> >linux- kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai >> ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Sudeep Holla >> ><sudeep.holla@arm.com>; Wan Mohamad, Wan Ahmad Zainie >> ><wan.ahmad.zainie.wan.mohamad@intel.com> >> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm >> >Trusted Firmware Service call >> > >> >Hi Michal, >> > >> >On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: >> >> Hi Sudeep, >> >> >> >> On 02. 10. 20 12:58, Sudeep Holla wrote: >> >> > Hi Michal, >> >> > >> >> > On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: >> >> >> Hi Sudeep, >> >> >> >> >> >> On 01. 10. 20 17:35, Sudeep Holla wrote: >> >> > >> >> > [...] >> >> > >> >> >>> >> >> >>> 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. >> >> > >> >> > OK, driver is fine but no dt-bindings as it is discoverable. It >> >> > can also be just a wrapper library instead as it needs no >> >> > explicit initialisation like drivers to setup. >> >> >> >> I am fine with it. Do we have any example which we can point him to? >> >> >> > >> >You seem to have figured that out already with SOC_ID example. >> >That was quick I must say 😄. >> > >> >> >> >> > >> >> >> 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. >> >> > >> >> > Agreed and one must use arm_smccc_get_conduit or something similar. >> >> > No additional bindings for each and ever platform and driver that >> >> > uses SMCCC please. >> >> > >> >> >> Of course if there is a better/cleaner way how this should be >> >> >> done I am happy to get more information about it. >> >> >> >> >> > >> >> > Let me know what you think about my thoughts stated above. >> >> >> >> >> >> I am fine with it. The key point is to have these sort it out >> >> because I see that a lot of drivers just simply call that SMCs from >> >> drivers which is IMHO wrong. >> >> >> > >> >Sure, sorry I didn't express my concern properly. I want to avoid dt >> >bindings for these and use the SMCCC discovery we have in place already if >possible. >> > >> >If this driver had consumers in the DT and it needs to be represented >> >in DT, it is a different story and I agree for need for a driver there. >> >But I don't see one in this usecase. >> >> >> Does it ok if I do some checking in arasan controller driver as below and >represented it in the DT of arasan,sdhci.yaml: >> This is to ensure that for Keem Bay SOC specific, the firmware driver must be >consume. >> >> if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) { >> struct device_node *dn; >> struct gpio_desc *uhs; >> >> dn = of_find_node_by_name(NULL, "keembay_firmware"); > >You have keembay_sd_voltage_selection function as Michal prefers, I have no >objections for that. But please no keembay_firmware node in DT. >You can implement this as a driver or simple smccc based function library >without DT node using SMCCC get_version. I hope the firmware gives error for >unimplemented FIDs, thereby eliminating the need for any DT node or config >option. To be clarify keembay_sd_voltage_selection function as Michal's prefers is actually using the firmware driver. This function located in firmware driver. I will call this func during voltage switching from arasan controller. I believe this implementation require DT to specify the compatible name and method use either smc/hvc. Are you saying that by using simple smcc based function library I should call below func() in arasan controller. For example 1) arm_smccc_get_version(void) 2) arm_smccc_version_init(arm_smccc_get_version(), SMCCC_CONDUIT_SMC); 3) arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, voltage_value , &res); Please advices. Thanks > >-- >Regards, >Sudeep
On Mon, Oct 05, 2020 at 05:04:10PM +0000, Zulkifli, Muhammad Husaini wrote: > To be clarify keembay_sd_voltage_selection function as Michal's prefers is > actually using the firmware driver. This function located in firmware > driver. OK, it can be just one function place it in any file you think is more appropriate need not be arasan controller driver. Any reasons why this can't work ? Can even be in some header. int keembay_sd_voltage_selection(int volt) { int res; arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, &res) /* appropriate error check if needed here */ return res; } > I will call this func during voltage switching from arasan controller. I > believe this implementation require DT to specify the compatible name and > method use either smc/hvc. No, use the standard one as detected by arm_smccc_1_1_invoke (It calls arm_smccc_get_conduit internally and use SMC/HVC based on that) > > Are you saying that by using simple smcc based function library I should > call below func() in arasan controller. For example > 1) arm_smccc_get_version(void) > 2) arm_smccc_version_init(arm_smccc_get_version(), SMCCC_CONDUIT_SMC); Nope > 3) arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, voltage_value , &res); Just this.
Hi Michal, >-----Original Message----- >From: Sudeep Holla <sudeep.holla@arm.com> >Sent: Tuesday, October 6, 2020 4:08 AM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> >Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian ><adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan Mohamad, >Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >On Mon, Oct 05, 2020 at 05:04:10PM +0000, Zulkifli, Muhammad Husaini wrote: > >> To be clarify keembay_sd_voltage_selection function as Michal's >> prefers is actually using the firmware driver. This function located >> in firmware driver. > >OK, it can be just one function place it in any file you think is more appropriate >need not be arasan controller driver. Any reasons why this can't work ? Can even >be in some header. > >int keembay_sd_voltage_selection(int volt) { > int res; > > arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, >&res) > > /* appropriate error check if needed here */ > > return res; >} > >> I will call this func during voltage switching from arasan controller. >> I believe this implementation require DT to specify the compatible >> name and method use either smc/hvc. > >No, use the standard one as detected by arm_smccc_1_1_invoke (It calls >arm_smccc_get_conduit internally and use SMC/HVC based on that) > >> >> Are you saying that by using simple smcc based function library I >> should call below func() in arasan controller. For example >> 1) arm_smccc_get_version(void) >> 2) arm_smccc_version_init(arm_smccc_get_version(), >SMCCC_CONDUIT_SMC); > >Nope > >> 3) arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, >voltage_value >> , &res); > >Just this. Is it ok not using the centralize firmware drivers? I would just revert back everything as in V1 by directly call the arm_smccc_1_1_invoke in arasan controller. > >-- >Regards, >Sudeep
HI Sudeep and Michal, >-----Original Message----- >From: Sudeep Holla <sudeep.holla@arm.com> >Sent: Tuesday, October 6, 2020 4:08 AM >To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> >Cc: Michal Simek <michal.simek@xilinx.com>; Hunter, Adrian ><adrian.hunter@intel.com>; ulf.hansson@linaro.org; linux- >mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai ><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan Mohamad, >Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com> >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted >Firmware Service call > >On Mon, Oct 05, 2020 at 05:04:10PM +0000, Zulkifli, Muhammad Husaini wrote: > >> To be clarify keembay_sd_voltage_selection function as Michal's >> prefers is actually using the firmware driver. This function located >> in firmware driver. > >OK, it can be just one function place it in any file you think is more appropriate >need not be arasan controller driver. Any reasons why this can't work ? Can even >be in some header. > >int keembay_sd_voltage_selection(int volt) { > int res; > > arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, volt, >&res) > > /* appropriate error check if needed here */ > > return res; >} > Yeah I believe it can work. I will create one header file in include/linux/firmware/intel/Keembay_firmware.h To handle this func and arasan controller can call this func. Are you guys ok with this? >> I will call this func during voltage switching from arasan controller. >> I believe this implementation require DT to specify the compatible >> name and method use either smc/hvc. > >No, use the standard one as detected by arm_smccc_1_1_invoke (It calls >arm_smccc_get_conduit internally and use SMC/HVC based on that) > >> >> Are you saying that by using simple smcc based function library I >> should call below func() in arasan controller. For example >> 1) arm_smccc_get_version(void) >> 2) arm_smccc_version_init(arm_smccc_get_version(), >SMCCC_CONDUIT_SMC); > >Nope > >> 3) arm_smccc_1_1_invoke(KEEMBAY_SET_SD_VOLTAGE_FUNC_ID, >voltage_value >> , &res); > >Just this. > >-- >Regards, >Sudeep
On Tue, Oct 06, 2020 at 01:22:31AM +0000, Zulkifli, Muhammad Husaini wrote: > HI Sudeep and Michal, > > Yeah I believe it can work. I will create one header file in include/linux/firmware/intel/Keembay_firmware.h > To handle this func and arasan controller can call this func. > Are you guys ok with this? > Sounds good to me. No change w.r.t arasan controller as it still needs to call the same api(keembay_sd_voltage_selection), just w/o a firmware driver for it. -- Regards, Sudeep
Hi, On 06. 10. 20 13:13, Sudeep Holla wrote: > On Tue, Oct 06, 2020 at 01:22:31AM +0000, Zulkifli, Muhammad Husaini wrote: >> HI Sudeep and Michal, >> >> Yeah I believe it can work. I will create one header file in include/linux/firmware/intel/Keembay_firmware.h >> To handle this func and arasan controller can call this func. >> Are you guys ok with this? >> > > Sounds good to me. No change w.r.t arasan controller as it still needs > to call the same api(keembay_sd_voltage_selection), just w/o a firmware > driver for it. I am also fine with it. Just please make sure that driver can be compiled also on non ARM platforms. Thanks, Michal
Hi, On 02. 10. 20 16:51, Sudeep Holla wrote: > Hi Michal, > > On Fri, Oct 02, 2020 at 03:53:33PM +0200, Michal Simek wrote: >> Hi Sudeep, >> >> On 02. 10. 20 12:58, Sudeep Holla wrote: >>> Hi Michal, >>> >>> On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: >>>> Hi Sudeep, >>>> >>>> On 01. 10. 20 17:35, Sudeep Holla wrote: >>> >>> [...] >>> >>>>> >>>>> 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. >>> >>> OK, driver is fine but no dt-bindings as it is discoverable. It can >>> also be just a wrapper library instead as it needs no explicit >>> initialisation like drivers to setup. >> >> I am fine with it. Do we have any example which we can point him to? >> > > You seem to have figured that out already with SOC_ID example. > That was quick I must say 😄. I would expect that instead of of if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2) return 0; if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) { pr_err("%s: invalid SMCCC conduit\n", __func__); return -EOPNOTSUPP; } arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_ARCH_SOC_ID, &res); you will simply call arm_smccc_1_2_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, ARM_SMCCC_ARCH_SOC_ID, &res); ...(check ret) arm_smccc_1_2_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res); ...(check ret) where it is clear from 1_2 that it has to be at least 1.2 version. <snip> > >> >> BTW: I see you have added soc id reading which you are saying is the >> part of smcc v1.2 but I can't see any implementation in TF-A. Is this >> spec publicly available? >> > > Spec is out[1], include/linux/arm-smccc.h points to the latest spec. > TF-A does have implementation as I tested with it and even reported > bug that I discovered when I tested with my patches that are now merged > upstream. Are you referring to master of TF-A or last release version ? > If latter, it had bug and may not be working. I may be wrong though, as > I am just telling what was told to me couple of months back and things > might have changed in TF-A land. I will read it and take a look when I have time. Thanks, Michal
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 + 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); +} +EXPORT_SYMBOL_GPL(keembay_sd_voltage_selection); + +/** + * keembay_get_invoke_func() - method of call + * @np: Pointer to the device_node structure + * + * Return: Returns 0 on success or error code + */ +static int keembay_get_invoke_func(struct device_node *np) +{ + const char *method; + + if (of_property_read_string(np, "method", &method)) { + pr_warn("%s missing \"method\" property\n", __func__); + return -ENXIO; + } + + if (!strcmp("smc", method)) { + do_fw_call = do_fw_call_smc; + } else { + pr_warn("%s Invalid \"method\" property: %s\n", + __func__, method); + return -EINVAL; + } + + return 0; +} + +static int keembay_firmware_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + int ret; + + ret = keembay_get_invoke_func(dev->of_node); + if (ret) { + dev_err(&pdev->dev, "Failed to get method of call\n"); + return ret; + } + + return 0; +} + +static int keembay_firmware_remove(struct platform_device *pdev) +{ + return 0; +} + +static const struct of_device_id keembay_firmware_of_match[] = { + {.compatible = "keembay,firmware"}, + {}, +}; +MODULE_DEVICE_TABLE(of, keembay_firmware_of_match); + +static struct platform_driver keembay_firmware_driver = { + .driver = { + .name = "keembay_firmware", + .of_match_table = keembay_firmware_of_match, + }, + .probe = keembay_firmware_probe, + .remove = keembay_firmware_remove, +}; +module_platform_driver(keembay_firmware_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Intel Keembay SOC Firmware Layer"); +MODULE_AUTHOR("Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>"); diff --git a/include/linux/firmware/intel/keembay_smc.h b/include/linux/firmware/intel/keembay_smc.h new file mode 100644 index 000000000000..87b1417f7e4e --- /dev/null +++ b/include/linux/firmware/intel/keembay_smc.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Intel Keembay SOC Firmware 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__ + +/* Setting for Keem Bay IO Pad Line Voltage Selection */ +#define KEEMBAY_SIP_FUNC_ID 0x8200ff26 +#define KEEMBAY_SET_1V8_VOLT 0x01 +#define KEEMBAY_SET_3V3_VOLT 0x00 + +#if IS_ENABLED(CONFIG_KEEMBAY_FIRMWARE) +int keembay_sd_voltage_selection(int volt); +#else +static inline int keembay_sd_voltage_selection(int volt) +{ + return -ENODEV; +} +#endif + +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */