On Wed 31 May 09:27 PDT 2017, Stephen Boyd wrote: > On 05/26, Bjorn Andersson wrote: > > In order to aid post-mortem debugging the Qualcomm platforms provides a > > "memory download mode", where the boot loader will provide an interface > > for custom tools to "download" the content of RAM to a host machine. > > > > The mode is triggered by writing a magic value somehwere in RAM, that is > > read in the boot code path after a warm-restart. Two mechanism for > > setting this magic value are supported in modern platforms; a direct SCM > > call to enable the mode or through a secure io write of a magic value. > > > > In order for a normal reboot not to trigger "download mode" the magic > > must be cleared during a clean reboot. > > > > Download mode has to be enabled by including qcom_scm.download_mode=1 on > > the command line. > > Why the kernel command line parameter? If we keep the kernel > command line param, perhaps we can also gain a config option to > make it default enabled or default disabled so that we don't have > to specify it on the commandline to get the feature all the time. >
I put a command line parameter there because most people will not have the tools to catch what's given to them by this. Making it a config option definitely makes more sense than forcing certain groups of engineers to always slap a kernel parameter in there. > > > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt > > b/Documentation/devicetree/bindings/firmware/qcom,scm.txt > > index 20f26fbce875..388817d1d00e 100644 > > --- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt > > @@ -18,6 +18,8 @@ Required properties: > > * Core, iface, and bus clocks required for "qcom,scm" > > - clock-names: Must contain "core" for the core clock, "iface" for the > > interface > > clock and "bus" for the bus clock per the requirements of the compatible. > > +- qcom,dload-mode-addr: Specifies the address (2 cells) for the download > > mode > > + magic (optional) > > Was it decided that reg was improper? Or a phandle to a node that > has a reg property? > I didn't want to slap an optional reg on the scm node, but based on the DT comment this should be a syscon reference instead. > > > > Example for MSM8916: > > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > index e18d63935648..98f4747acddb 100644 > > --- a/drivers/firmware/qcom_scm.c > > +++ b/drivers/firmware/qcom_scm.c > > @@ -19,6 +19,7 @@ > > #include <linux/cpumask.h> > > #include <linux/export.h> > > #include <linux/dma-mapping.h> > > +#include <linux/module.h> > > #include <linux/types.h> > > #include <linux/qcom_scm.h> > > #include <linux/of.h> > > @@ -28,6 +29,9 @@ > > > > #include "qcom_scm.h" > > > > +static bool download_mode; > > +module_param(download_mode, bool, 0700); > > 0700? Not 0600? And what if we have it == 1 on the command line > and then write 0 at runtime with module param? Shouldn't we > handle that with a callback and turn off download mode there? > Otherwise when we reboot we will reboot into download mode? > Rather than adding the extra logic I think it makes more sense to just make perm 0, that way it's not possible to change in runtime. > > > + > > #define SCM_HAS_CORE_CLK BIT(0) > > #define SCM_HAS_IFACE_CLK BIT(1) > > #define SCM_HAS_BUS_CLK BIT(2) > > @@ -365,6 +393,7 @@ static int qcom_scm_probe(struct platform_device *pdev) > > struct qcom_scm *scm; > > unsigned long clks; > > int ret; > > + u64 val; > > > > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); > > if (!scm) > > @@ -418,9 +447,22 @@ static int qcom_scm_probe(struct platform_device *pdev) > > > > __qcom_scm_init(); > > > > + ret = of_property_read_u64(pdev->dev.of_node, "qcom,dload-mode-addr", > > &val); > > + if (!ret) > > + scm->dload_mode_addr = val; > > How about: > > of_property_read_u64(pdev->dev.of_node, "qcom,dload-mode-addr", > &scm->dload_mode_addr) > That looks better. > > + > > + if (download_mode) > > + qcom_scm_set_download_mode(true); > > + > > return 0; > > } > > > > +void qcom_scm_shutdown(struct platform_device *pdev) > > static? > Yes. Thanks, Bjorn
