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

[Zulkifli, Muhammad Husaini]

>-----Original Message-----
>From: Sudeep Holla <sudeep.holla@xxxxxxx>
>Sent: Monday, October 5, 2020 4:45 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>
>Cc: Michal Simek <michal.simek@xxxxxxxxxx>; Hunter, Adrian
><adrian.hunter@xxxxxxxxx>; 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
>
>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@xxxxxxx>
>> >Sent: Friday, October 2, 2020 10:51 PM
>> >To: Michal Simek <michal.simek@xxxxxxxxxx>
>> >Cc: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@xxxxxxxxx>;
>> >Hunter, Adrian <adrian.hunter@xxxxxxxxx>; ulf.hansson@xxxxxxxxxx;
>> >linux- mmc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>> >linux- kernel@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai
>> ><lakshmi.bai.raja.subramanian@xxxxxxxxx>; arnd@xxxxxxxx; Sudeep Holla
>> ><sudeep.holla@xxxxxxx>; 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 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