Received-SPF: pass (google.com: domain of linux-kernel+bounces-151193-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 19 Apr 2024 09:53:45 +0100
From: Sudeep Holla <sudeep.holla@arm.com>
To: Elliot Berman <quic_eberman@quicinc.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Sebastian Reichel <sre@kernel.org>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Andy Yan <andy.yan@rock-chips.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
	Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>,
	Melody Olvera <quic_molvera@quicinc.com>,
	Shivendra Pratap <quic_spratap@quicinc.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	linux-pm@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2
Message-ID: <20240419085345.4ovebbbmcabo3f73@bogus>
References: <20240414-arm-psci-system_reset2-vendor-reboots-v2-0-da9a055a648f@quicinc.com>
 <Zh5GWqt2oCNHdF_h@bogus>
 <20240417140957985-0700.eberman@hu-eberman-lv.qualcomm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240417140957985-0700.eberman@hu-eberman-lv.qualcomm.com>

On Wed, Apr 17, 2024 at 02:54:41PM -0700, Elliot Berman wrote:
> On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
> > On Sun, Apr 14, 2024 at 12:30:23PM -0700, Elliot Berman wrote:
> > > The PSCI SYSTEM_RESET2 call allows vendor firmware to define additional
> > > reset types which could be mapped to the reboot argument.
> > >
> > > Setting up reboot on Qualcomm devices can be inconsistent from chipset
> > > to chipset.
> >
> > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > expected ? Does it mean it is not conformant to the specification ?
> >
>
> I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
> register and IMEM cookie can change between chipsets. Using
> SYSTEM_RESET2 alows us to abstract how to perform the reset.

Fair enough. But I assume you are not providing the details of PMIC register
or IMEM cookie via DT.

Anyways you did confirm if PSCI SYSTEM_RESET works as expected or not. That
is default and must work.

> > > Generally, there is a PMIC register that gets written to
> > > decide the reboot type. There is also sometimes a cookie that can be
> > > written to indicate that the bootloader should behave differently than a
> > > regular boot. These knobs evolve over product generations and require
> > > more drivers. Qualcomm firmwares are beginning to expose vendor
> > > SYSTEM_RESET2 types to simplify driver requirements from Linux.
> > >
> >
> > Why can't this be fully userspace driven ? What is the need to keep the
> > cookie in the DT ?
>
> As Dmitry pointed out, this information isn't discoverable. I suppose
> we could technically use bootconfig or kernel command-line to convey the
> map although I think devicetree is the right spot for this mapping.
>

Yes and as usual DT has become dumping ground for firmware that don't
make things discoverable. Make crap that Qcom puts in the DT are firmware
related and can be make discoverable. Anyways it is sad that no efforts
to make it so are done as DT is always there to provide shortcuts.

> - Other vendor-specific bits for PSCI are described in the devicetree.
>   One example is the suspend param (e.g. the StateID) for cpu idle
>   states.

You are right, but that is the only example I can see and it was done
in very early days of PSCI. It shouldn't be example if there are better
ways.

> - Describing firmware bits in the DT isn't unprecedented, and putting
>   this information outside the DT means that other OSes (besides Linux)
>   need their own way to convey this information.

Correct but it can be Qcom specific firmware interface. There are so many
already. This splitting information between firmware and DT works well
for vertically integrated things which probably is the case with most of
Qcom SoCs but it is prone to issues if DT and firmware mismatch. Firmware
discovery eliminates such issues.

> - PSCI would be the odd one out that reboot mode map is not described in
>   DT. Other reboot-mode drivers specify the mapping in the DT. Userspace
>   that runs with firmware that support vendor reset2 need to make sure
>   they can configure the mapping early enough.
>

Well I am not saying not to this yet, just exploring and getting more info
so that whatever is done here can be reused on all PSCI based systems.

--
Regards,
Sudeep