Message-ID: <ZrCQunPL/BWYwaR3@hu-varada-blr.qualcomm.com>
Date: Mon, 5 Aug 2024 14:13:38 +0530
From: Varadarajan Narayanan <quic_varada@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@...aro.org>
CC: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
        "Andy Gross" <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>, Niklas Cassel
	<nks@...wful.org>,
        Liam Girdwood <lgirdwood@...il.com>, Mark Brown
	<broonie@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "Rafael J. Wysocki"
	<rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Ulf Hansson
	<ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Robert Marko <robimarko@...il.com>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-pm@...r.kernel.org>, Jeffrey Hugo
	<quic_jhugo@...cinc.com>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...ainline.org>
Subject: Re: [PATCH v15 09/10] soc: qcom: Add a driver for CPR3+

On Mon, Jul 08, 2024 at 02:22:40PM +0200, Konrad Dybcio wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>
> Introduce a new driver (based on qcom-cpr for CPRv1 IP) for the newer
> Qualcomm Core Power Reduction hardware, known downstream as CPR[34h]
> (h for hardened).
>
> In these new CPR versions, support for various new features was introduced.
> That includes:
> * voltage reduction for the GPU
> * security hardening
> * a new way of controlling CPU DVFS, based on internal communication
>   between CPRh and Operating State Manager MCUs.
>
> The CPR v3, v4 and CPRh are present in a broad range of SoCs, from the
> mid-range to the high end ones including, but not limited to:
> MSM8953/8996/8998 and SDM630/636/660/845.
>
> Note that to reduce the giant review and testing matrix of the driver, this
> patch (admittedly, somewhat confusingly but for good reasons) omits support
> for CPR*3* specifically, which is otherwise quite straightforward to add.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> [Konrad: rebase, a whole lot of cleanup/fixes]
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> ---
>  drivers/pmdomain/qcom/Kconfig      |   22 +
>  drivers/pmdomain/qcom/Makefile     |    4 +-
>  drivers/pmdomain/qcom/cpr-common.h |    2 +
>  drivers/pmdomain/qcom/cpr3.c       | 2711 ++++++++++++++++++++++++++++++++++++
>  include/soc/qcom/cpr.h             |   17 +
>  5 files changed, 2755 insertions(+), 1 deletion(-)
>
	.
	.
	.
> +/**
> + * cpr_irq_handler() - Handle CPR3/CPR4 status interrupts
> + * @irq: Number of the interrupt
> + * @dev: Pointer to the cpr_thread structure
> + *
> + * Handle the interrupts coming from non-hardened CPR HW as to get
> + * an ok to scale voltages immediately, or to pass error status to
> + * the hardware (either success/ACK or failure/NACK).
> + *
> + * Return: IRQ_SUCCESS for success, IRQ_NONE if the CPR is disabled.
> + */
> +static irqreturn_t cpr_irq_handler(int irq, void *dev)
> +{
> +	struct cpr_thread *thread = dev;

While registering this handler with devm_request_threaded_irq,
'cpr_drv' pointer is used as the cookie. But, here the cookie is
being assumed to be cpr_thread. Is that intentional?

Getting this crash while testing:-

	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
	Mem abort info:
	  ESR = 0x0000000096000004
	  EC = 0x25: DABT (current EL), IL = 32 bits
	  SET = 0, FnV = 0
	  EA = 0, S1PTW = 0
	  FSC = 0x04: level 0 translation fault
	Data abort info:
	  ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
	  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
	  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
	user pgtable: 4k pages, 48-bit VAs, pgdp=0000000046efd000
	[0000000000000010] pgd=0000000000000000, p4d=0000000000000000
	Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
	Modules linked in:
	CPU: 0 UID: 0 PID: 72 Comm: irq/26-cpr Not tainted 6.11.0-rc1-next-20240729-00019-g244903f9355b-dirty #28
	Hardware name: Qualcomm Technologies, Inc. IPQ9574/AP-AL02-C7 (DT)
	pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	pc : mutex_lock+0x20/0x54
	lr : cpr_irq_handler+0x28/0x260
	sp : ffff80008613bd80
	x29: ffff80008613bd80 x28: ffff8000800b267c x27: ffff000006f92080
	x26: ffff000002989200 x25: ffff000002b28000 x24: 0000000000000001
	x23: ffff0000029892ac x22: ffff000002b28000 x21: 0000000000000000
	x20: 0000000000000010 x19: ffff000002ba2880 x18: 0000000000000001
	x17: 0000000073babcd7 x16: 0000000000000000 x15: ffff00003fc914c0
	x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
	x11: 0000000000000000 x10: 00000000000009b0 x9 : ffff80008613bd20
	x8 : ffff000002b28a10 x7 : ffff00003fc91700 x6 : ffff00003fc90cc0
	x5 : 00000000410fd090 x4 : 0000000000100000 x3 : 0000000000000000
	x2 : ffff000002b28000 x1 : ffff000002ba2880 x0 : 0000000000000010
	Call trace:
	 mutex_lock+0x20/0x54
	 irq_thread_fn+0x2c/0xa8
	 irq_thread+0x16c/0x274
	 kthread+0x110/0x114
	 ret_from_fork+0x10/0x20
	Code: b5000141 d65f03c0 d2800003 f9800011 (c85ffc01)
	---[ end trace 0000000000000000 ]---

Changing it to cpr_thread pointer in devm_request_threaded_irq
fixes the above crash.

> +	struct cpr_drv *drv = thread->drv;
> +	irqreturn_t ret = IRQ_HANDLED;
> +	int i, rc;
> +	enum voltage_change_dir dir = NO_CHANGE;
> +	u32 val;
> +
> +	guard(mutex)(&drv->lock);
> +
> +	val = cpr_read(thread, CPR3_REG_IRQ_STATUS);
> +
> +	dev_vdbg(drv->dev, "IRQ_STATUS = 0x%x\n", val);
> +
	.
	.
	.
> +/**
> + * cpr_thread_init() - Initialize CPR thread related parameters
> + * @drv: Main driver structure
> + * @tid: Thread ID
> + *
> + * Return: Zero for success, negative number on error
> + */
> +static int cpr_thread_init(struct cpr_drv *drv, int tid)
> +{
> +	const struct cpr_desc *desc = drv->desc;
> +	const struct cpr_thread_desc *tdesc = desc->threads[tid];
> +	struct cpr_thread *thread = &drv->threads[tid];
> +	bool pd_registered = false;
> +	int ret, i;
	.
	.
	.
> +	/* On CPRhardened, the interrupts are managed in firmware */
> +	if (desc->cpr_type != CTRL_TYPE_CPRH) {
> +		INIT_WORK(&thread->restart_work, cpr_restart_worker);
> +
> +		ret = devm_request_threaded_irq(drv->dev, drv->irq,
> +						NULL, cpr_irq_handler,
> +						IRQF_ONESHOT |
> +						IRQF_TRIGGER_RISING,
> +						"cpr", drv);

'drv' or 'thread' ?

Thanks
Varada

> +		if (ret)
> +			goto fail;
> +	}

Powered by blists - more mailing lists