Message-ID: <154508853143.19322.18196170541201508404@swboyd.mtv.corp.google.com>
Date:   Mon, 17 Dec 2018 15:15:31 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Rajendra Nayak <rnayak@...eaurora.org>, sre@...nel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] power: reset: msm: Add support for download-mode control

Quoting Bjorn Andersson (2018-12-13 21:30:35)
> On Wed 21 Nov 10:26 PST 2018, Stephen Boyd wrote:
> 
> > Quoting Stephen Boyd (2018-07-20 10:44:53)
> > > Quoting Rajendra Nayak (2018-07-18 23:59:20)
> > > > On 7/19/2018 11:12 AM, Bjorn Andersson wrote:
> > > > > On Wed 18 Jul 22:18 PDT 2018, Rajendra Nayak wrote:
> > > > >> diff --git a/Documentation/devicetree/bindings/power/reset/msm-poweroff.txt b/Documentation/devicetree/bindings/power/reset/msm-poweroff.txt
> > > > >> index ce44ad3..9dd489f 100644
> > > > >> --- a/Documentation/devicetree/bindings/power/reset/msm-poweroff.txt
> > > > >> +++ b/Documentation/devicetree/bindings/power/reset/msm-poweroff.txt
> > > > >> @@ -8,6 +8,9 @@ settings.
> > > > >>   Required Properties:
> > > > >>   -compatible: "qcom,pshold"
> > > > >>   -reg: Specifies the physical address of the ps-hold register
> > > > >> +Optional Properties:
> > > > >> +-qcom,dload-mode: phandle to the TCSR hardware block and offset of the
> > > > >> +             download mode control register
> > > > >>   
> > > > >>   Example:
> > > > >>   
> > > > >> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > > > >> index df58fc8..0c97e34 100644
> > > > >> --- a/drivers/power/reset/Kconfig
> > > > >> +++ b/drivers/power/reset/Kconfig
> > > > >> @@ -104,6 +104,17 @@ config POWER_RESET_MSM
> > > > >>      help
> > > > >>        Power off and restart support for Qualcomm boards.
> > > > >>   
> > > > >> +config POWER_RESET_MSM_DOWNLOAD_MODE
> > > > > 
> > > > > How about moving QCOM_SCM_DOWNLOAD_MODE_DEFAULT to
> > > > > drivers/soc/qcom/Kconfig (and removing "SCM") and referencing this in
> > > > > both drivers?
> > > > 
> > > > yes, thats possible, but I am not sure how to make the command line
> > > > option common for both. One other option I thought was if we could handle it
> > > > within the scm driver itself with an additional
> > > > binding to specify the non-secure download mode address.
> > > > something like qcom,dload-mode-ns?
> > > 
> > > Is the SCM device and driver always going to be present though? It may
> > > be better to make a TCSR platform device driver on designs that would
> > > configure the cookie with direct read/writes from Linux to break the
> > > relationship with scm entirely. Then the different configurations could
> > > flow from the DTS file either describing scm that has scm call, a
> > > special scm_writel address for TCSR, or a specific TCSR node with the
> > > address of the download mode cookie that triggers a TCSR driver to probe
> > > and register a reboot handler.
> > > 
> > 
> > Does my proposal work? I haven't seen anything new on the list since
> > this email.
> > 
> 
> Afaiu the SCM device is still there, even though we don't use all the
> usual functionality.

You mean the SCM device is always there even when TCSR is used to lay
down the download mode cookie?

> 
> I tested on qcs404 and sdm845-mtp (LA boot chain), and they both return
> positive on:
> 
>   __qcom_scm_is_call_available(dev, QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE)
> 
> So how about we change qcom_scm_set_download_mode() to do:
> 
>   if (scm_call_avail(QCOM_SCM_SVC_BOOT, QCOM_SCM_SET_DLOAD_MODE))
>         __qcom_scm_set_dload_mode()
>   else if (scm_call_avail(QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE) && dload_mode_addr)
>         __qcom_scm_io_writel();
>   else if (dload_mode_addr)
>         writel()
> 
> This would also mean that we can put the dload addr in the sdm845.dtsi
> and share that between LA and ATF.
> 

I'd prefer that any ATF firmware put qcom's download mode behind a
vendor specific SYSTEM_RESET2 PSCI call so that ATF can do the magic to
clear the cookie on normal reboots and shutdowns. Abnormal resets where
we want to enter download mode from some panic or crash handler can be
handled by calling the vendor specific PSCI call. Then the only decision
that needs to be made is this one we're talking about now, where someone
wants a way to express that download mode should be entered when a
hardware event causes a reset (i.e. watchdog bite, thermal restart).

Even that decision could be made statically though, in which case I
don't see why SCM is a dependency. The bootloader or firmware can decide
to always put down the cookie early and clear it when the system is
normally rebooted so that bootloader bugs can be detected. I'll admit
that we lose the ability to disable download mode unless we implement
something to clear the cookie, so on firmwares without SCM I imagine the
cookie could be cleared with the direct writel() code. Long story short,
we need a more generic method than qcom_scm_set_download_mode() to do
that because it doesn't make sense to say there's SCM firmware there
when there isn't any.

So can there be some generic kernel piece of code that looks for the scm
firmware node and a tcsr node and then decides to set or clear the
cookie based on a commandline parameter? It can also look for a PSCI
node and if there's a compatible firmware with the vendor specific reset
call it can use that to know that it shouldn't do anything besides allow
the commandline parameter to clear or set the cookie. All of this
wouldn't actually require to be bound to any sort of device or device
node. It's just looking in DT/firmware tables for various pieces of
information and changing how reset, shutdown and the commandline
parameter is handled for the architecture.

Furthermore, the PSCI style reset handling would need to be plumbed into
the ARM/ARM64 reset hooks, so we would already need to add some panic
crash handler calls into the PSCI layer to handle the vendor specific
cases. Coming up with a more generic commandline parameter like
'panic_mode=<vendor>,type' would be good to express that we want to use
qcom download mode when the system panics and then one place for all the
different reboot logic can be contained to focus other SoC manufacturers
on implementing a PSCI call or flow in a similar way.

Powered by blists - more mailing lists