Message-ID: <YNDL/yLanerZ4hM9@gerhold.net>
Date:   Mon, 21 Jun 2021 19:27:27 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
Cc:     bjorn.andersson@...aro.org, agross@...nel.org,
        daniel.lezcano@...aro.org, rjw@...ysocki.net,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, phone-devel@...r.kernel.org,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org, jeffrey.l.hugo@...il.com,
        jamipkettunen@...ainline.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v5 1/3] cpuidle: qcom_spm: Detach state machine from main
 SPM handling

On Mon, Jun 21, 2021 at 06:12:06PM +0200, AngeloGioacchino Del Regno wrote:
> Il 21/06/21 13:54, Stephan Gerhold ha scritto:
> > On Sat, Jun 19, 2021 at 12:56:18AM +0200, AngeloGioacchino Del Regno wrote:
> > > In commit a871be6b8eee ("cpuidle: Convert Qualcomm SPM driver to a generic
> > > CPUidle driver") the SPM driver has been converted to a
> > > generic CPUidle driver: that was mainly made to simplify the
> > > driver and that was a great accomplishment;
> > > Though, it was ignored that the SPM driver is not used only
> > > on the ARM architecture.
> > > 
> > 
> > Can you please reword this sentence like I suggested in v4? The way you
> > write it at the moment it sounds like this fixes a regression, but
> > actually it extends the driver to cover more use cases.
> > 
> 
> I don't see any regression implied: I'm explaining the reason of this
> change just one line after that and .. besides that, I haven't put any
> "Fixes:" tag to this commit.
> When you fix regressions, bad behavior, or anything else relative to a
> patch, you add that tag to say that you're fixing something.
> 
> Moreover, I can't see anything wrong in the description for this change,
> nor anything to clarify about it and that as long as you read it in full
> 

I don't really want to get into a long discussion about a single
sentence here but the sentence just doesn't make any sense to me:

  1. You say that I "ignored" something in my commit, which isn't the case.
  2. The mainline "SPM driver" *is* only used on ARM*32* at the moment.
     (This is something you want to change in this patch series!)

To me that sentence suggests the "SPM driver" already had support for
ARM64/your use case and I *deliberately* ignored that while converting
the driver to a direct CPU Idle driver.

I'm merely suggesting making the situation a bit more clear by replacing

> > > Though, it was ignored that the SPM driver is not used only
> > > on the ARM architecture.

with something like:
"However, on newer ARM64 SoCs the SPM hardware is used for purposes
other than CPUIdle."

> > > In preparation for the enablement of SPM features on AArch64/ARM64,
> > > split the cpuidle-qcom-spm driver in two: the CPUIdle related
> > > state machine (currently used only on ARM SoCs) stays there, while
> > > the SPM communication handling lands back in soc/qcom/spm.c and
> > > also making sure to not discard the simplifications that were
> > > introduced in the aforementioned commit.
> > > 
> > > Since now the "two drivers" are split, the SCM dependency in the
> > > main SPM handling is gone and for this reason it was also possible
> > > to move the SPM initialization early: this will also make sure that
> > > whenever the SAW CPUIdle driver is getting initialized, the SPM
> > > driver will be ready to do the job.
> > > 
> > > Please note that the anticipation of the SPM initialization was
> > > also done to optimize the boot times on platforms that have their
> > > CPU/L2 idle states managed by other means (such as PSCI), while
> > > needing SAW initialization for other purposes, like AVS control.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> > > ---
> > >   drivers/cpuidle/Kconfig.arm        |   1 +
> > >   drivers/cpuidle/cpuidle-qcom-spm.c | 295 ++++++-----------------------
> > >   drivers/soc/qcom/Kconfig           |   9 +
> > >   drivers/soc/qcom/Makefile          |   1 +
> > >   drivers/soc/qcom/spm.c             | 198 +++++++++++++++++++
> > >   include/soc/qcom/spm.h             |  43 +++++
> > >   6 files changed, 311 insertions(+), 236 deletions(-)
> > >   create mode 100644 drivers/soc/qcom/spm.c
> > >   create mode 100644 include/soc/qcom/spm.h
> > > 
> > > [...]
> > > diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> > > new file mode 100644
> > > index 000000000000..719c604a8402
> > > --- /dev/null
> > > +++ b/include/soc/qcom/spm.h
> > > @@ -0,0 +1,43 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2014,2015, Linaro Ltd.
> > > + * Copyright (C) 2021, AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> > > + */
> > > +
> > > +#ifndef __SPM_H__
> > > +#define __SPM_H__
> > > +
> > > +#include <linux/cpuidle.h>
> > > +
> > > +#define MAX_PMIC_DATA		2
> > > +#define MAX_SEQ_DATA		64
> > > +
> > > +enum pm_sleep_mode {
> > > +	PM_SLEEP_MODE_STBY,
> > > +	PM_SLEEP_MODE_RET,
> > > +	PM_SLEEP_MODE_SPC,
> > > +	PM_SLEEP_MODE_PC,
> > > +	PM_SLEEP_MODE_NR,
> > > +};
> > > +
> > > +struct spm_reg_data {
> > > +	const u8 *reg_offset;
> > > +	u32 spm_cfg;
> > > +	u32 spm_dly;
> > > +	u32 pmic_dly;
> > > +	u32 pmic_data[MAX_PMIC_DATA];
> > > +	u8 seq[MAX_SEQ_DATA];
> > > +	u8 start_index[PM_SLEEP_MODE_NR];
> > > +};
> > > +
> > > +struct spm_driver_data {
> > > +	struct cpuidle_driver cpuidle_driver;
> > 
> > Given that the SPM hardware seems to have several different uses,
> > not just CPUidle, wouldn't it be better to allocate the memory for the
> > cpuidle_driver from cpuidle-qcom-spm.c? Just devm_kzalloc() something
> > like:
> > 
> > struct spm_cpuidle_driver {
> > 	struct cpuidle_driver cpuidle_driver;
> > 	struct spm_driver_data *spm;
> > };
> > 
> > in cpuidle-qcom-spm.c.
> > 
> > And then there wouldn't be a need to have the implementation details of
> > the SPM driver in the spm.h header, all that cpuidle-qcom-spm needs is
> > 
> > struct spm_driver_data;
> > 
> > enum pm_sleep_mode {
> > 	PM_SLEEP_MODE_STBY,
> > 	PM_SLEEP_MODE_RET,
> > 	PM_SLEEP_MODE_SPC,
> > 	PM_SLEEP_MODE_PC,
> > 	PM_SLEEP_MODE_NR,
> > };
> > 
> > void spm_set_low_power_mode(struct spm_driver_data *drv,
> > 			    enum pm_sleep_mode mode);
> > 
> > Everything else can remain private to the spm.c driver,
> > like it was before.
> > 
> 
> I don't completely dislike the approach but I honestly think that this
> would put some cognitive strain: having a header included in two files
> using "things" defined in there gives people an easy path to follow
> when looking at one of the two files.
> 

OK, I guess this is personal preference. I don't really mind if you keep
the structs defined in the shared header...

> Regarding the addition of that one more structure... I am seriously
> undecided on the matter: wasting this kind of amount of memory on a
> ARM64 platform (usually having more than 1GB RAM) is completely
> unnoticeable but, on the other hand, it's still a waste of memory,
> even if it's minimal that much.
> 

In my opinion it also makes the two drivers better separated.
The SPM driver manages the SPM hardware and provides functionality to
configure it. The CPU idle driver makes use of that functionality
to implement the CPU idle use case. This does not require the SPM driver
to know how exactly the CPU idle driver looks.

Stephan

Powered by blists - more mailing lists