| Message ID | 9acc746885d6b06c2333f0493413c44b85fa7f02.1724968351.git.quic_uchalich@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SCM: Support latest version of waitq-aware firmware | expand |
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@quicinc.com> > --- > 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? Also, the subtraction comments take some guesswork.. perhaps something like 0..31 etc. would be easier. The MAX_(E)SPI macros could also just have the hwirq number to make the if-conditions below simpler > + > +#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 > +} > + > +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) > + 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.. 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 Konrad
On Thu, Aug 29, 2024 at 03:15:54PM GMT, Unnathi Chalicheemala wrote: > Bootloader and firmware for SM8650 and older chipsets expect node > name as "qcom_scm". Perhaps we can add the reason why bootloader/firmware is looking for this node? Perhaps "looks for a node named qcom_scm, in order to patch the wait queue IRQ information." ? > 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 Capitalize IRQ, and Linux throughout the message. Regards, Bjorn > defined fill_irq_fwspec_params(). This linux irq number is then supplied to > the threaded_irq call. > > Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com> > --- > 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 > + > +#define GIC_IRQ_TYPE_SPI 0 > +#define GIC_IRQ_TYPE_ESPI 2 > + > 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; > +} > + > +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; > + 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) > return irq; > diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h > index 685b8f59e7a6..ab0f88f5f777 100644 > --- a/drivers/firmware/qcom/qcom_scm.h > +++ b/drivers/firmware/qcom/qcom_scm.h > @@ -143,6 +143,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void); > #define QCOM_SCM_SVC_WAITQ 0x24 > #define QCOM_SCM_WAITQ_RESUME 0x02 > #define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03 > +#define QCOM_SCM_WAITQ_GET_INFO 0x04 > > #define QCOM_SCM_SVC_GPU 0x28 > #define QCOM_SCM_SVC_GPU_INIT_REGS 0x01 > -- > 2.34.1 >
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@quicinc.com> >> --- >> 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
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 + +#define GIC_IRQ_TYPE_SPI 0 +#define GIC_IRQ_TYPE_ESPI 2 + 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; +} + +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; + 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) return irq; diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h index 685b8f59e7a6..ab0f88f5f777 100644 --- a/drivers/firmware/qcom/qcom_scm.h +++ b/drivers/firmware/qcom/qcom_scm.h @@ -143,6 +143,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void); #define QCOM_SCM_SVC_WAITQ 0x24 #define QCOM_SCM_WAITQ_RESUME 0x02 #define QCOM_SCM_WAITQ_GET_WQ_CTX 0x03 +#define QCOM_SCM_WAITQ_GET_INFO 0x04 #define QCOM_SCM_SVC_GPU 0x28 #define QCOM_SCM_SVC_GPU_INIT_REGS 0x01
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@quicinc.com> --- drivers/firmware/qcom/qcom_scm.c | 59 +++++++++++++++++++++++++++++++- drivers/firmware/qcom/qcom_scm.h | 1 + 2 files changed, 59 insertions(+), 1 deletion(-)