Re: [PATCH v2 1/2] firmware: qcom_scm: Add API to get waitqueue IRQ info

[Unnathi Chalicheemala]

On 8/29/2024 4:45 PM, Konrad Dybcio wrote:
> On 30.08.2024 12:15 AM, Unnathi Chalicheemala wrote:
>> Bootloader and firmware for SM8650 and older chipsets expect node
>> name as "qcom_scm". However, DeviceTree uses node name "scm" and this
>> mismatch prevents firmware from correctly identifying waitqueue IRQ
>> information. Waitqueue IRQ is used for signaling between secure and
>> non-secure worlds.
>>
>> To resolve this, introduce qcom_scm_get_waitq_irq() that'll get the
>> hardware irq number to be used from firmware instead of relying on data
>> provided by devicetree, thereby bypassing the DeviceTree node name
>> mismatch.
>>
>> This hardware irq number is converted to a linux irq number using newly
>> defined fill_irq_fwspec_params(). This linux irq number is then supplied to
>> the threaded_irq call.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@xxxxxxxxxxx>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++-
>>  drivers/firmware/qcom/qcom_scm.h |  1 +
>>  2 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 00c379a3cceb..ed51fbb1c065 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -32,6 +32,14 @@
>>  #include "qcom_scm.h"
>>  #include "qcom_tzmem.h"
>>  
>> +#define GIC_SPI_BASE            32
>> +#define GIC_MAX_SPI             987  // 1019 - 32
>> +#define GIC_ESPI_BASE           4096
>> +#define GIC_MAX_ESPI            1024 // 5120 - 4096
> 
> Are these going to remain constant on different implementations of the
> interrupt controller across different SoCs that use this? Are these
> mandated anywhere in the arm spec and/or present across the tree with
> parts touching gicv3?
> 

Yes they're constant across all SoCs that use Gunyah hypervisor.
They're documented in the GIC v3/v4 programming guide - I don't think
they're present in the tree.
INTID				Interrupt Type 
16 – 31				     PPIs
1056 – 1119 (GICv3.1)

32 – 1019			     SPIs
4096 – 5119 (GICv3.1) 

> Also, the subtraction comments take some guesswork.. perhaps something like
> 0..31 etc. would be easier.
> 
Ack.
> The MAX_(E)SPI macros could also just have the hwirq number to make the
> if-conditions below simpler
> 
Ack. I broke it up so the macros could be understandable. I can make them just
hwirq numbers.
>> +
>> +#define GIC_IRQ_TYPE_SPI        0
>> +#define GIC_IRQ_TYPE_ESPI       2
> 
> We can definitely use dt-bindings for this
> 
>> +
>>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>>  module_param(download_mode, bool, 0);
>>  
>> @@ -1819,6 +1827,55 @@ bool qcom_scm_is_available(void)
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>>  
>> +static int qcom_scm_fill_irq_fwspec_params(struct irq_fwspec *fwspec, u32 virq)
>> +{
>> +	if (WARN(virq < GIC_SPI_BASE, "Unexpected virq: %d\n", virq)) {
>> +		return -ENXIO;
>> +	} else if (virq <= (GIC_SPI_BASE + GIC_MAX_SPI)) {
>> +		fwspec->param_count = 3;
>> +		fwspec->param[0] = GIC_IRQ_TYPE_SPI;
>> +		fwspec->param[1] = virq - GIC_SPI_BASE;
>> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
>> +	} else if (WARN(virq < GIC_ESPI_BASE, "Unexpected virq: %d\n", virq)) {
>> +		return -ENXIO;
>> +	} else if (virq < (GIC_ESPI_BASE + GIC_MAX_ESPI)) {
>> +		fwspec->param_count = 3;
>> +		fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
>> +		fwspec->param[1] = virq - GIC_ESPI_BASE;
>> +		fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
>> +	} else {
>> +		WARN(1, "Unexpected virq: %d\n", virq);
>> +		return -ENXIO;
>> +	}
>> +	return 0;
> 
> This could use some prettifying (incl the previous comment):
> 
> if (GIC_SPI_BASE <= virq && virq <= GIC_SPI_MAX) {
> 	fwspec->param[0] = GIC_IRQ_TYPE_SPI;
> 	fwspec->param[1] = virq - GIC_SPI_BASE;
> } else if (GIC_ESPI_BASE <= virq && virq <= GIC_ESPI_MAX) {
> 	fwspec->param[0] = GIC_IRQ_TYPE_ESPI;
> 	fwspec->param[1] = virq - GIC_ESPI_BASE;
> } else {
> 	WARN(1, "Unexpected virq"...
> 	return -ENXIO;
> }
> 
> fwspec->param[2] = IRQ_TYPE_EDGE_RISING;
> fwspec->param_count = 3;
> 
> is much easier to follow along in my opinion
> 
Ack, thanks!
>> +}
>> +
>> +static int qcom_scm_get_waitq_irq(void)
>> +{
>> +	int ret;
>> +	u32 hwirq;
>> +	struct qcom_scm_desc desc = {
>> +		.svc = QCOM_SCM_SVC_WAITQ,
>> +		.cmd = QCOM_SCM_WAITQ_GET_INFO,
>> +		.owner = ARM_SMCCC_OWNER_SIP
>> +	};
>> +	struct qcom_scm_res res;
>> +	struct irq_fwspec fwspec;
>> +
>> +	ret = qcom_scm_call_atomic(__scm->dev, &desc, &res);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fwspec.fwnode = of_node_to_fwnode(__scm->dev->of_node);
>> +	hwirq = res.result[1] & 0xffff;
> 
> GENMASK(15, 0)
> 
Ack.
>> +	ret = qcom_scm_fill_irq_fwspec_params(&fwspec, hwirq);
>> +	if (ret)
>> +		return ret;
>> +	ret = irq_create_fwspec_mapping(&fwspec);
>> +
>> +	return ret;
>> +}
>> +
>>  static int qcom_scm_assert_valid_wq_ctx(u32 wq_ctx)
>>  {
>>  	/* FW currently only supports a single wq_ctx (zero).
>> @@ -1936,7 +1993,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	/* Let all above stores be available after this */
>>  	smp_store_release(&__scm, scm);
>>  
>> -	irq = platform_get_irq_optional(pdev, 0);
>> +	irq = qcom_scm_get_waitq_irq();
>>  	if (irq < 0) {
>>  		if (irq != -ENXIO)
> 
> Is this smc call left unimplemented on !auto platforms? If it's not
> (or it spits out bogus data), we're going to get a WARN splat in the
> log..
> 
This call is implemented on all platforms(auto and !auto) from SM8650 onward.
Will double-check on this.
> Additionally, this mechanism ties the trustzone and hypervisor together..
> Why isn't this done in gunyah which abstracts these resources? A hypercall
> sounds much saner than tying in a third party into the mix
> 
fill_irqfwspec_params is actually a function in gunyah's header file but I copied it
here as didn't want multi waitqueue support to be dependent on Gunyah's patches. I'll
check if this can be made a hyper call. 
Thanks for the review Konrad!
> Konrad