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

[Sudeep Holla <sudeep.holla@arm.com>]

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.

-- 
Regards,
Sudeep