All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shawn Guo <shawn.guo@linaro.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Maulik Shah <quic_mkshah@quicinc.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver
Date: Mon, 6 Dec 2021 21:15:31 +0800	[thread overview]
Message-ID: <20211206131530.GN10105@dragon> (raw)
In-Reply-To: <87wnkikqve.wl-maz@kernel.org>

On Mon, Dec 06, 2021 at 09:46:29AM +0000, Marc Zyngier wrote:
[...]
> > +/* Triggered by RPM when system resumes from deep sleep */
> > +static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
> > +{
> > +	struct qcom_mpm_priv *priv = dev_id;
> > +	unsigned long enable, pending;
> > +	int i, j;
> > +
> > +	for (i = 0; i < priv->reg_stride; i++) {
> > +		enable = qcom_mpm_read(priv, MPM_REG_ENABLE, i);
> > +		pending = qcom_mpm_read(priv, MPM_REG_STATUS, i);
> > +		pending &= enable;
> > +
> > +		for_each_set_bit(j, &pending, 32) {
> > +			unsigned int pin = 32 * i + j;
> > +			struct irq_desc *desc =
> > +					irq_resolve_mapping(priv->domain, pin);
> 
> Assignments on a single line, please.

This is to avoid over 80 columns check patch warning.

> 
> > +			struct irq_data *d = &desc->irq_data;
> > +
> > +			if (!irqd_is_level_type(d))
> > +				irq_set_irqchip_state(d->irq,
> > +						IRQCHIP_STATE_PENDING, true);
> > +
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int qcom_mpm_enter_sleep(struct qcom_mpm_priv *priv)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < priv->reg_stride; i++)
> > +		qcom_mpm_write(priv, MPM_REG_STATUS, i, 0);
> > +
> > +	/* Notify RPM to write vMPM into HW */
> 
> What do you mean by 'into HW'? We just did that, right? or are these
> registers just fake and most of the stuff is in the RPM?

I have a note about this in commit log.

- All the register settings are done by APSS on an internal memory
  region called vMPM, and RPM will flush them into hardware after it
  receives a mailbox/IPC notification from APSS.

So yes, these registers are fake/virtual in memory, and RPM will
actually flush the values into the MPM hardware block.

> 
> > +	ret = mbox_send_message(priv->mbox_chan, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_mpm_cpu_pm_callback(struct notifier_block *nb,
> > +				    unsigned long action, void *data)
> > +{
> > +	struct qcom_mpm_priv *priv = container_of(nb, struct qcom_mpm_priv,
> > +						  pm_nb);
> > +	int ret = NOTIFY_OK;
> > +	int cpus_in_pm;
> > +
> > +	switch (action) {
> > +	case CPU_PM_ENTER:
> > +		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> > +		/*
> > +		 * NOTE: comments for num_online_cpus() point out that it's
> > +		 * only a snapshot so we need to be careful. It should be OK
> > +		 * for us to use, though.  It's important for us not to miss
> > +		 * if we're the last CPU going down so it would only be a
> > +		 * problem if a CPU went offline right after we did the check
> > +		 * AND that CPU was not idle AND that CPU was the last non-idle
> > +		 * CPU. That can't happen. CPUs would have to come out of idle
> > +		 * before the CPU could go offline.
> > +		 */
> > +		if (cpus_in_pm < num_online_cpus())
> > +			return NOTIFY_OK;
> > +		break;
> 
> I'm really not keen on this sort of tracking in an irqchip driver.
> Nobody else needs this. This is also copy-pasted (without even
> mentioning it) from rpmh_rsc_cpu_pm_callback(). Why is that logic
> needed twice, given that the RPM is also involved is this sequence?

Yes, this is copy-pasted from rpmh-rsc and I should have mentioned that
in some way.  But rpmh-rsc is a driver specific to RPM-Hardened (RPMH)
which can be found on Qualcomm high-range SoCs like SDM845, SC7180.  And
on these RPMH based SoCs, PDC works as the wake-up interrupt controller.
However, on mid-range SoCs like SDM660, QCM2290, they run a different
combination, that is RPM + MPM.  Note - RPMH and RPM are two similar but
different and separate blocks.

What we are implementing is for RPM + MPM based SoCs, and rpmh-rsc is
irrelevant here.  The same cpu_pm tracking logic can be reused though.

> 
> > +	case CPU_PM_ENTER_FAILED:
> > +	case CPU_PM_EXIT:
> > +		atomic_dec(&priv->cpus_in_pm);
> > +		return NOTIFY_OK;
> > +	default:
> > +		return NOTIFY_DONE;
> > +	}
> > +
> > +	/*
> > +	 * It's likely we're on the last CPU. Grab the lock and write MPM for
> > +	 * sleep. Grabbing the lock means that if we race with another CPU
> > +	 * coming up we are still guaranteed to be safe.
> > +	 */
> > +	if (raw_spin_trylock(&priv->lock)) {
> > +		if (qcom_mpm_enter_sleep(priv))
> > +			ret = NOTIFY_BAD;
> > +		raw_spin_unlock(&priv->lock);
> > +	} else {
> > +		/* Another CPU must be up */
> > +		return NOTIFY_OK;
> > +	}
> > +
> > +	if (ret == NOTIFY_BAD) {
> > +		/* Double-check if we're here because someone else is up */
> > +		if (cpus_in_pm < num_online_cpus())
> > +			ret = NOTIFY_OK;
> > +		else
> > +			/* We won't be called w/ CPU_PM_ENTER_FAILED */
> > +			atomic_dec(&priv->cpus_in_pm);
> > +	}
> > +
> > +	return ret;
> > +}

[...]

> > +/*
> > + * The mapping between MPM_GIC pin and GIC SPI number on QCM2290.  It's taken
> > + * from downstream qcom-mpm-scuba.c with a little transform on the GIC
> > + * SPI numbers (the second column).  Due to the binding difference from
> > + * the downstream, where GIC SPI numbering starts from 32, we expect the
> > + * numbering starts from 0 here, and that's why we have the number minus 32
> > + * comparing to the downstream.
> 
> This doesn't answer my earlier question: is this list complete? Or is
> it likely to change?

Yes, it's complete for QCM2290.

> Also, why is that in not in the device tree,
> given that it is likely to change from one SoC to another?

Yes, this is a SoC specific data, and will be different from one SoC to
another.  Different subsystem maintainers have different views on such
SoC data.  Some think they belong to kernel/driver and can be matched
using SoC specific compatible, while others prefer to have them in DT.

As I mentioned in the commit log, this SoC data actually consists of two
parts:

 1) Map of MPM_GIC pin <--> GIC interrupt number
 2) Map of MPM_GPIO pin <--> GPIO number

Since we already have map 2) defined in pinctrl driver rather than DT,
I put map 1) in MPM driver.

Shawn

> 
> > + */
> > +const struct mpm_pin qcm2290_gic_pins[] = {
> > +	{ 2, 275 },	/* tsens0_tsens_upper_lower_int */
> > +	{ 5, 296 },	/* lpass_irq_out_sdc */
> > +	{ 12, 422 },	/* b3_lfps_rxterm_irq */
> > +	{ 24, 79 },	/* bi_px_lpi_1_aoss_mx */
> > +	{ 86, 183 },	/* mpm_wake,spmi_m */
> > +	{ 90, 260 },	/* eud_p0_dpse_int_mx */
> > +	{ 91, 260 },	/* eud_p0_dmse_int_mx */
> > +	{ -1 },
> > +};
> > +
> > +const struct mpm_data qcm2290_data = {
> > +	.pin_num = 96,
> > +	.gic_pins = qcm2290_gic_pins,
> > +};
> > +
> > +static int qcm2290_mpm_init(struct platform_device *pdev,
> > +			    struct device_node *parent)
> > +{
> > +	return qcom_mpm_init(pdev, parent, &qcm2290_data);
> > +}
> > +
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(qcom_mpm)
> > +IRQCHIP_MATCH("qcom,qcm2290-mpm", qcm2290_mpm_init)
> > +IRQCHIP_PLATFORM_DRIVER_END(qcom_mpm)
> > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. MSM Power Manager");
> > +MODULE_LICENSE("GPL v2");

  reply	other threads:[~2021-12-06 13:15 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-02 12:21 [PATCH v3 0/3] Add Qualcomm MPM irqchip driver support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 1/3] irqchip: Pass platform_device pointer to init_cb Shawn Guo
2021-12-02 17:52   ` Florian Fainelli
2021-12-02 19:10     ` Marc Zyngier
2021-12-02 21:44       ` Florian Fainelli
2021-12-06  6:40       ` Shawn Guo
2021-12-06  8:29         ` Marc Zyngier
2021-12-06  6:35     ` Shawn Guo
2021-12-02 12:21 ` [PATCH v3 2/3] dt-bindings: interrupt-controller: Add Qualcomm MPM support Shawn Guo
2021-12-02 12:21 ` [PATCH v3 3/3] irqchip: Add Qualcomm MPM controller driver Shawn Guo
2021-12-03  0:00   ` kernel test robot
2021-12-03  0:00     ` kernel test robot
2021-12-06  9:46   ` Marc Zyngier
2021-12-06 13:15     ` Shawn Guo [this message]
2021-12-06 13:48       ` Marc Zyngier
2021-12-07  9:48         ` Shawn Guo
2021-12-07 10:16           ` Marc Zyngier
2021-12-08 10:04             ` Shawn Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211206131530.GN10105@dragon \
    --to=shawn.guo@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=loic.poulain@linaro.org \
    --cc=maz@kernel.org \
    --cc=quic_mkshah@quicinc.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.