| Message ID | 20170527063308.10483-2-bjorn.andersson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, May 26, 2017 at 11:33:07PM -0700, 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. This must be happening somewhere before the kernel is entered? Or warm-restarts are not the norm? > Download mode has to be enabled by including qcom_scm.download_mode=1 on > the command line. This looks similar to reboot reason functionality (i.e. boot into mode X). I'd expect this to use that at least for the kernel. Not sure about bindings though. > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v1: > - Specify DT propert being two-cell > - Correct handling of scm-call return code > > .../devicetree/bindings/firmware/qcom,scm.txt | 2 + > drivers/firmware/qcom_scm-32.c | 6 +++ > drivers/firmware/qcom_scm-64.c | 13 +++++++ > drivers/firmware/qcom_scm.c | 43 ++++++++++++++++++++++ > drivers/firmware/qcom_scm.h | 2 + > 5 files changed, 66 insertions(+) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 31 May 12:31 PDT 2017, Rob Herring wrote: > On Fri, May 26, 2017 at 11:33:07PM -0700, 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. > > This must be happening somewhere before the kernel is entered? Or > warm-restarts are not the norm? > Not sure I'm getting what you're asking here. We set the flag on boot and clear it on shutdown, that way the early boot stages will be able to detect if the board restarted uncleanly - e.g. from accessing a protected register. > > Download mode has to be enabled by including qcom_scm.download_mode=1 on > > the command line. > > This looks similar to reboot reason functionality (i.e. boot into mode > X). I'd expect this to use that at least for the kernel. Not sure about > bindings though. > It's very much like reboot reason except the small detail that we want to enter this state when the board reboots without the kernels knowledge. Thinking about it, it may make sense to not clear the flag if we exit through the panic handler. But if that's wanted it could be done in a follow up patch, I only added this to catch an error where the RPM hit its error handler and reboots the board. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/31, Bjorn Andersson wrote: > 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. Ok. Sounds good. The commandline will still be nice to have to override whatever is in the build without having to recompile. And it sounds like the plan is to have both pieces. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) Example for MSM8916: diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c index 11fdb1584823..68b2033bc30e 100644 --- a/drivers/firmware/qcom_scm-32.c +++ b/drivers/firmware/qcom_scm-32.c @@ -561,6 +561,12 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset) return ret ? : le32_to_cpu(out); } +int __qcom_scm_set_dload_mode(struct device *dev, bool enable) +{ + return qcom_scm_call_atomic2(QCOM_SCM_SVC_BOOT, QCOM_SCM_SET_DLOAD_MODE, + enable ? QCOM_SCM_SET_DLOAD_MODE : 0, 0); +} + int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id) { struct { diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c index bf50fb59852e..3fea6f563ca9 100644 --- a/drivers/firmware/qcom_scm-64.c +++ b/drivers/firmware/qcom_scm-64.c @@ -440,6 +440,19 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size, return ret; } +int __qcom_scm_set_dload_mode(struct device *dev, bool enable) +{ + struct qcom_scm_desc desc = {0}; + struct arm_smccc_res res; + + desc.args[0] = QCOM_SCM_SET_DLOAD_MODE; + desc.args[1] = enable ? QCOM_SCM_SET_DLOAD_MODE : 0; + desc.arginfo = QCOM_SCM_ARGS(2); + + return qcom_scm_call(dev, QCOM_SCM_SVC_BOOT, QCOM_SCM_SET_DLOAD_MODE, + &desc, &res); +} + int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr, unsigned int *val) { 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); + #define SCM_HAS_CORE_CLK BIT(0) #define SCM_HAS_IFACE_CLK BIT(1) #define SCM_HAS_BUS_CLK BIT(2) @@ -38,6 +42,8 @@ struct qcom_scm { struct clk *iface_clk; struct clk *bus_clk; struct reset_controller_dev reset; + + phys_addr_t dload_mode_addr; }; static struct qcom_scm *__scm; @@ -345,6 +351,28 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val) } EXPORT_SYMBOL(qcom_scm_io_writel); +static void qcom_scm_set_download_mode(bool enable) +{ + bool avail; + int ret; + + avail = __qcom_scm_is_call_available(__scm->dev, + QCOM_SCM_SVC_BOOT, + QCOM_SCM_SET_DLOAD_MODE); + if (avail) { + ret = __qcom_scm_set_dload_mode(__scm->dev, enable); + if (ret) + dev_err(__scm->dev, "SCM failed to set download mode: %d\n", ret); + } else if (__scm->dload_mode_addr) { + ret = __qcom_scm_io_writel(__scm->dev, __scm->dload_mode_addr, + enable ? QCOM_SCM_SET_DLOAD_MODE : 0); + if (ret) + dev_err(__scm->dev, "SCM failed to set download mode: %d\n", ret); + } else { + dev_err(__scm->dev, "No available mechanism for setting download mode\n"); + } +} + /** * qcom_scm_is_available() - Checks if SCM is available */ @@ -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; + + if (download_mode) + qcom_scm_set_download_mode(true); + return 0; } +void qcom_scm_shutdown(struct platform_device *pdev) +{ + if (download_mode) + qcom_scm_set_download_mode(false); +} + static const struct of_device_id qcom_scm_dt_match[] = { { .compatible = "qcom,scm-apq8064", /* FIXME: This should have .data = (void *) SCM_HAS_CORE_CLK */ @@ -448,6 +490,7 @@ static struct platform_driver qcom_scm_driver = { .of_match_table = qcom_scm_dt_match, }, .probe = qcom_scm_probe, + .shutdown = qcom_scm_shutdown, }; static int __init qcom_scm_init(void) diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index a60e4b9b1394..83f171c23943 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -14,9 +14,11 @@ #define QCOM_SCM_SVC_BOOT 0x1 #define QCOM_SCM_BOOT_ADDR 0x1 +#define QCOM_SCM_SET_DLOAD_MODE 0x10 #define QCOM_SCM_BOOT_ADDR_MC 0x11 #define QCOM_SCM_SET_REMOTE_STATE 0xa extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id); +extern int __qcom_scm_set_dload_mode(struct device *dev, bool enable); #define QCOM_SCM_FLAG_HLOS 0x01 #define QCOM_SCM_FLAG_COLDBOOT_MC 0x02
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. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v1: - Specify DT propert being two-cell - Correct handling of scm-call return code .../devicetree/bindings/firmware/qcom,scm.txt | 2 + drivers/firmware/qcom_scm-32.c | 6 +++ drivers/firmware/qcom_scm-64.c | 13 +++++++ drivers/firmware/qcom_scm.c | 43 ++++++++++++++++++++++ drivers/firmware/qcom_scm.h | 2 + 5 files changed, 66 insertions(+) -- 2.12.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html