linux-kernel - RE: [PATCH v1 5/9] firmware: keembay: Add support for 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: <DM6PR11MB287679CF20BBC7C81B6E38F5B8A40@DM6PR11MB2876.namprd11.prod.outlook.com>
Date:   Mon, 18 Jan 2021 10:28:33 +0000
From:   "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To:     Sudeep Holla <sudeep.holla@....com>,
        Mark Brown <broonie@...nel.org>
CC:     "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "A, Rashmi" <rashmi.a@...el.com>,
        "Vaidya, Mahesh R" <mahesh.r.vaidya@...el.com>
Subject: RE: [PATCH v1 5/9] firmware: keembay: Add support for Trusted
 Firmware Service call

Hi Sudeep and Mark,

Thanks for the review. I replied inline.

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@....com>
>Sent: Saturday, January 16, 2021 2:58 AM
>To: Mark Brown <broonie@...nel.org>
>Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>;
>ulf.hansson@...aro.org; lgirdwood@...il.com; robh+dt@...nel.org;
>devicetree@...r.kernel.org; Hunter, Adrian <adrian.hunter@...el.com>;
>michal.simek@...inx.com; linux-mmc@...r.kernel.org; linux-
>kernel@...r.kernel.org; Shevchenko, Andriy
><andriy.shevchenko@...el.com>; A, Rashmi <rashmi.a@...el.com>; Vaidya,
>Mahesh R <mahesh.r.vaidya@...el.com>; Sudeep Holla
><sudeep.holla@....com>
>Subject: Re: [PATCH v1 5/9] firmware: keembay: Add support for Trusted
>Firmware Service call
>
>On Thu, Jan 14, 2021 at 04:48:11PM +0000, Mark Brown wrote:
>> On Thu, Jan 14, 2021 at 11:26:56PM +0800, Muhammad Husaini Zulkifli
>wrote:
>> > Export inline function to encapsulate AON_CFG1 for controling the
>> > I/O Rail supplied voltage levels which communicate with Trusted Firmware.
>>
>> Adding Sudeep for the SMCCC bits, not deleting any context for his
>> benefit.
>>
>
>Thanks Mark for cc-ing me and joining the dots. I completely forgot about that
>fact that this platform was using SCMI using SMC as transport. Sorry for that and
>it is my fault. I did review the SCMI/SMC support for this platform sometime in
>June/July last year and forgot the fact it is same platform when
>voltage/regulator support patches for SD/MMC was posted sometime later last
>year. I concentrated on SMCCC conventions and other details.

Yes Sudeep. I redesigned the way I handle the smccc call. Previously it was handled directly in mmc driver.
After few discussion, we conclude to create an abstraction using regulator framework to encapsulate this smccc call
during set voltage operation. Using standard abstraction will make things easier for the maintainer.

>
>[...]
>
>> > +#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE		\
>> > +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
>> > +			   ARM_SMCCC_SMC_32,		\
>> > +			   ARM_SMCCC_OWNER_SIP,		\
>> > +			   KEEMBAY_SET_SD_VOLTAGE_ID)
>> > +
>> > +#define ARM_SMCCC_SIP_KEEMBAY_GET_SD_VOLTAGE		\
>> > +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
>> > +			   ARM_SMCCC_SMC_32,		\
>> > +			   ARM_SMCCC_OWNER_SIP,		\
>> > +			   KEEMBAY_GET_SD_VOLTAGE_ID)
>> > +
>> > +#define KEEMBAY_REG_NUM_CONSUMERS 2
>> > +
>> > +struct keembay_reg_supply {
>> > +	struct regulator *consumer;
>> > +};
>> > +
>> > +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
>> > +/*
>> > + * Voltage applied on the IO Rail is controlled from the Always On
>> > +Register using specific
>> > + * bits in AON_CGF1 register. This is a secure register. Keem Bay
>> > +SOC cannot exposed this
>> > + * register address to the outside world.
>> > + */
>> > +static inline int keembay_set_io_rail_supplied_voltage(int volt) {
>> > +	struct arm_smccc_res res;
>> > +
>> > +
>	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTA
>GE, volt,
>> > +&res);
>>
>> There is a SCMI voltage domain protocol intended for just this use
>> case of controlling regulators managed by the firmware, why are you
>> not using that for these systems?  See drivers/firmware/arm_scmi/voltage.c.

>From mmc maintainer's perspective, I should use the common modelling either using 
regulator framework or pinctrl to perform voltage operation. Not just directly invoke 
smccc call in the mmc driver. That is why I came up with this regulator driver to perform 
voltage operation. 

>>
>
>Indeed. Please switch to using the new voltage protocol added for this without
>any extra code. You just need to wire up DT for this.

May I know even if I wire up the DT, how should I call this from the mmc driver
For set/get voltage operation? Any example?

>
>Just for curiosity, where is SCMI platform firmware implemented ? On Cortex-
>A, secure side or external processor. Does this platform run TF-A ?

The KMB SCMI framework is implemented in secure runtime firmware (TF-A BL31). 
Hopefully I am answering your question.

>
>--
>Regards,
>Sudeep