| Message ID | 1531977529-23304-1-git-send-email-rnayak@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | power: reset: msm: Add support for download-mode control | expand |
On Wed 18 Jul 22:18 PDT 2018, Rajendra Nayak wrote: > commit '8c1b7dc9b: firmware: qcom: scm: Expose download-mode control' > added support for download-mode control using the scm firmware > driver for platforms which require a secure call to write the magic > cookie into the tcsr location. > > For platforms which *do not* need an scm call and where the kernel can > write the cookie by a direct read/write, add similar support in the > msm-poweroff driver. > Similar to the scm driver, the msm-poweroff driver clears the cookie > during a clean reboot. > > Download mode using msm-poweroff driver can be enabled by including > msm-poweroff.download_mode=1 on the command line. > Should have thought about this when I wrote the scm code... I would prefer if we could find a way to consolidate the two implementations behind the same Kconfig and command line parameters. > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > .../bindings/power/reset/msm-poweroff.txt | 3 ++ > drivers/power/reset/Kconfig | 11 +++++++ > drivers/power/reset/msm-poweroff.c | 38 ++++++++++++++++++++++ > 3 files changed, 52 insertions(+) > > 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? > + bool "Qualcomm download mode enabled by default" > + depends on POWER_RESET_MSM > + help > + A device with "download mode" enabled will upon an unexpected > + warm-restart enter a special debug mode that allows the user to > + "download" memory content over USB for offline postmortem analysis. > + The feature can be enabled/disabled on the kernel command line. > + > + Say Y here to enable "download mode" by default. > + > config POWER_RESET_OCELOT_RESET > bool "Microsemi Ocelot reset driver" > depends on MSCC_OCELOT || COMPILE_TEST > diff --git a/drivers/power/reset/msm-poweroff.c b/drivers/power/reset/msm-poweroff.c > index 01b8c71..c33eac5 100644 > --- a/drivers/power/reset/msm-poweroff.c > +++ b/drivers/power/reset/msm-poweroff.c > @@ -18,11 +18,20 @@ > #include <linux/io.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/reboot.h> > +#include <linux/regmap.h> > #include <linux/pm.h> > > +static bool download_mode = IS_ENABLED(CONFIG_POWER_RESET_MSM_DOWNLOAD_MODE); > +module_param(download_mode, bool, 0); Iirc it's possible to have multiple implementations of __setup() for the same string. I'm uncertain if this would be considered okay though. > + > +#define QCOM_SET_DLOAD_MODE 0x10 > static void __iomem *msm_ps_hold; > +static struct regmap *tcsr_regmap; > +static unsigned int dload_mode_offset; > + > static int deassert_pshold(struct notifier_block *nb, unsigned long action, > void *data) > { > @@ -44,9 +53,30 @@ static void do_msm_poweroff(void) > > static int msm_restart_probe(struct platform_device *pdev) > { > + int ret; > + struct of_phandle_args args; > struct device *dev = &pdev->dev; > struct resource *mem; > > + if (download_mode) { > + ret = of_parse_phandle_with_fixed_args(dev->of_node, > + "qcom,dload-mode", 1, 0, > + &args); > + if (ret < 0) > + return ret; > + > + tcsr_regmap = syscon_node_to_regmap(args.np); > + of_node_put(args.np); > + if (IS_ERR(tcsr_regmap)) > + return PTR_ERR(tcsr_regmap); > + > + dload_mode_offset = args.args[0]; > + > + /* Enable download mode by writing the cookie */ > + regmap_write(tcsr_regmap, dload_mode_offset, > + QCOM_SET_DLOAD_MODE); > + } > + > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > msm_ps_hold = devm_ioremap_resource(dev, mem); > if (IS_ERR(msm_ps_hold)) > @@ -59,6 +89,13 @@ static int msm_restart_probe(struct platform_device *pdev) > return 0; > } > > +static void msm_restart_shutdown(struct platform_device *pdev) > +{ > + /* Clean shutdown, disable download mode to allow normal restart */ > + if (download_mode) > + regmap_write(tcsr_regmap, dload_mode_offset, 0x0); > +} > + > static const struct of_device_id of_msm_restart_match[] = { > { .compatible = "qcom,pshold", }, > {}, > @@ -71,6 +108,7 @@ static int msm_restart_probe(struct platform_device *pdev) > .name = "msm-restart", > .of_match_table = of_match_ptr(of_msm_restart_match), > }, > + .shutdown = msm_restart_shutdown, > }; Implementation looks good. 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 7/19/2018 11:12 AM, Bjorn Andersson wrote: > On Wed 18 Jul 22:18 PDT 2018, Rajendra Nayak wrote: > >> commit '8c1b7dc9b: firmware: qcom: scm: Expose download-mode control' >> added support for download-mode control using the scm firmware >> driver for platforms which require a secure call to write the magic >> cookie into the tcsr location. >> >> For platforms which *do not* need an scm call and where the kernel can >> write the cookie by a direct read/write, add similar support in the >> msm-poweroff driver. >> Similar to the scm driver, the msm-poweroff driver clears the cookie >> during a clean reboot. >> >> Download mode using msm-poweroff driver can be enabled by including >> msm-poweroff.download_mode=1 on the command line. >> > > Should have thought about this when I wrote the scm code... > > I would prefer if we could find a way to consolidate the two > implementations behind the same Kconfig and command line parameters. The only problem I saw with this was that *both* drivers would think its enabled and try their own ways to enable it, and one of it would always fail. We could fail silently, which would mean we will miss cases when its a genuine failure but that should be fine? >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> .../bindings/power/reset/msm-poweroff.txt | 3 ++ >> drivers/power/reset/Kconfig | 11 +++++++ >> drivers/power/reset/msm-poweroff.c | 38 ++++++++++++++++++++++ >> 3 files changed, 52 insertions(+) >> >> 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? regards, Rajendra > >> + bool "Qualcomm download mode enabled by default" >> + depends on POWER_RESET_MSM >> + help >> + A device with "download mode" enabled will upon an unexpected >> + warm-restart enter a special debug mode that allows the user to >> + "download" memory content over USB for offline postmortem analysis. >> + The feature can be enabled/disabled on the kernel command line. >> + >> + Say Y here to enable "download mode" by default. >> + >> config POWER_RESET_OCELOT_RESET >> bool "Microsemi Ocelot reset driver" >> depends on MSCC_OCELOT || COMPILE_TEST >> diff --git a/drivers/power/reset/msm-poweroff.c b/drivers/power/reset/msm-poweroff.c >> index 01b8c71..c33eac5 100644 >> --- a/drivers/power/reset/msm-poweroff.c >> +++ b/drivers/power/reset/msm-poweroff.c >> @@ -18,11 +18,20 @@ >> #include <linux/io.h> >> #include <linux/of.h> >> #include <linux/platform_device.h> >> +#include <linux/mfd/syscon.h> >> #include <linux/module.h> >> #include <linux/reboot.h> >> +#include <linux/regmap.h> >> #include <linux/pm.h> >> >> +static bool download_mode = IS_ENABLED(CONFIG_POWER_RESET_MSM_DOWNLOAD_MODE); >> +module_param(download_mode, bool, 0); > > Iirc it's possible to have multiple implementations of __setup() for the > same string. I'm uncertain if this would be considered okay though. > >> + >> +#define QCOM_SET_DLOAD_MODE 0x10 >> static void __iomem *msm_ps_hold; >> +static struct regmap *tcsr_regmap; >> +static unsigned int dload_mode_offset; >> + >> static int deassert_pshold(struct notifier_block *nb, unsigned long action, >> void *data) >> { >> @@ -44,9 +53,30 @@ static void do_msm_poweroff(void) >> >> static int msm_restart_probe(struct platform_device *pdev) >> { >> + int ret; >> + struct of_phandle_args args; >> struct device *dev = &pdev->dev; >> struct resource *mem; >> >> + if (download_mode) { >> + ret = of_parse_phandle_with_fixed_args(dev->of_node, >> + "qcom,dload-mode", 1, 0, >> + &args); >> + if (ret < 0) >> + return ret; >> + >> + tcsr_regmap = syscon_node_to_regmap(args.np); >> + of_node_put(args.np); >> + if (IS_ERR(tcsr_regmap)) >> + return PTR_ERR(tcsr_regmap); >> + >> + dload_mode_offset = args.args[0]; >> + >> + /* Enable download mode by writing the cookie */ >> + regmap_write(tcsr_regmap, dload_mode_offset, >> + QCOM_SET_DLOAD_MODE); >> + } >> + >> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> msm_ps_hold = devm_ioremap_resource(dev, mem); >> if (IS_ERR(msm_ps_hold)) >> @@ -59,6 +89,13 @@ static int msm_restart_probe(struct platform_device *pdev) >> return 0; >> } >> >> +static void msm_restart_shutdown(struct platform_device *pdev) >> +{ >> + /* Clean shutdown, disable download mode to allow normal restart */ >> + if (download_mode) >> + regmap_write(tcsr_regmap, dload_mode_offset, 0x0); >> +} >> + >> static const struct of_device_id of_msm_restart_match[] = { >> { .compatible = "qcom,pshold", }, >> {}, >> @@ -71,6 +108,7 @@ static int msm_restart_probe(struct platform_device *pdev) >> .name = "msm-restart", >> .of_match_table = of_match_ptr(of_msm_restart_match), >> }, >> + .shutdown = msm_restart_shutdown, >> }; > > Implementation looks good. > > 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
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: > > > >> commit '8c1b7dc9b: firmware: qcom: scm: Expose download-mode control' > >> added support for download-mode control using the scm firmware > >> driver for platforms which require a secure call to write the magic > >> cookie into the tcsr location. > >> > >> For platforms which *do not* need an scm call and where the kernel can > >> write the cookie by a direct read/write, add similar support in the > >> msm-poweroff driver. > >> Similar to the scm driver, the msm-poweroff driver clears the cookie > >> during a clean reboot. > >> > >> Download mode using msm-poweroff driver can be enabled by including > >> msm-poweroff.download_mode=1 on the command line. > >> > > > > Should have thought about this when I wrote the scm code... > > > > I would prefer if we could find a way to consolidate the two > > implementations behind the same Kconfig and command line parameters. > > The only problem I saw with this was that *both* drivers would think > its enabled and try their own ways to enable it, and one of it would > always fail. We could fail silently, which would mean we will miss > cases when its a genuine failure but that should be fine? Should be fine if SCM fails silently. If TCSR is specified it better work because that doesn't really rely on anything besides writing into a memory location. > > >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > >> --- > >> .../bindings/power/reset/msm-poweroff.txt | 3 ++ > >> drivers/power/reset/Kconfig | 11 +++++++ > >> drivers/power/reset/msm-poweroff.c | 38 ++++++++++++++++++++++ > >> 3 files changed, 52 insertions(+) > >> > >> 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. -- 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
Hi, What's the status of this patch? It looks good to me, should it be merged, did I miss a newer version? -- Sebastian On Fri, Jul 20, 2018 at 10:44:53AM -0700, Stephen Boyd wrote: > 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: > > > > > >> commit '8c1b7dc9b: firmware: qcom: scm: Expose download-mode control' > > >> added support for download-mode control using the scm firmware > > >> driver for platforms which require a secure call to write the magic > > >> cookie into the tcsr location. > > >> > > >> For platforms which *do not* need an scm call and where the kernel can > > >> write the cookie by a direct read/write, add similar support in the > > >> msm-poweroff driver. > > >> Similar to the scm driver, the msm-poweroff driver clears the cookie > > >> during a clean reboot. > > >> > > >> Download mode using msm-poweroff driver can be enabled by including > > >> msm-poweroff.download_mode=1 on the command line. > > >> > > > > > > Should have thought about this when I wrote the scm code... > > > > > > I would prefer if we could find a way to consolidate the two > > > implementations behind the same Kconfig and command line parameters. > > > > The only problem I saw with this was that *both* drivers would think > > its enabled and try their own ways to enable it, and one of it would > > always fail. We could fail silently, which would mean we will miss > > cases when its a genuine failure but that should be fine? > > Should be fine if SCM fails silently. If TCSR is specified it better > work because that doesn't really rely on anything besides writing into a > memory location. > > > > > >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > >> --- > > >> .../bindings/power/reset/msm-poweroff.txt | 3 ++ > > >> drivers/power/reset/Kconfig | 11 +++++++ > > >> drivers/power/reset/msm-poweroff.c | 38 ++++++++++++++++++++++ > > >> 3 files changed, 52 insertions(+) > > >> > > >> 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. > >
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.
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. 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. Regards, Bjorn
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.
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 + bool "Qualcomm download mode enabled by default" + depends on POWER_RESET_MSM + help + A device with "download mode" enabled will upon an unexpected + warm-restart enter a special debug mode that allows the user to + "download" memory content over USB for offline postmortem analysis. + The feature can be enabled/disabled on the kernel command line. + + Say Y here to enable "download mode" by default. + config POWER_RESET_OCELOT_RESET bool "Microsemi Ocelot reset driver" depends on MSCC_OCELOT || COMPILE_TEST diff --git a/drivers/power/reset/msm-poweroff.c b/drivers/power/reset/msm-poweroff.c index 01b8c71..c33eac5 100644 --- a/drivers/power/reset/msm-poweroff.c +++ b/drivers/power/reset/msm-poweroff.c @@ -18,11 +18,20 @@ #include <linux/io.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/reboot.h> +#include <linux/regmap.h> #include <linux/pm.h> +static bool download_mode = IS_ENABLED(CONFIG_POWER_RESET_MSM_DOWNLOAD_MODE); +module_param(download_mode, bool, 0); + +#define QCOM_SET_DLOAD_MODE 0x10 static void __iomem *msm_ps_hold; +static struct regmap *tcsr_regmap; +static unsigned int dload_mode_offset; + static int deassert_pshold(struct notifier_block *nb, unsigned long action, void *data) { @@ -44,9 +53,30 @@ static void do_msm_poweroff(void) static int msm_restart_probe(struct platform_device *pdev) { + int ret; + struct of_phandle_args args; struct device *dev = &pdev->dev; struct resource *mem; + if (download_mode) { + ret = of_parse_phandle_with_fixed_args(dev->of_node, + "qcom,dload-mode", 1, 0, + &args); + if (ret < 0) + return ret; + + tcsr_regmap = syscon_node_to_regmap(args.np); + of_node_put(args.np); + if (IS_ERR(tcsr_regmap)) + return PTR_ERR(tcsr_regmap); + + dload_mode_offset = args.args[0]; + + /* Enable download mode by writing the cookie */ + regmap_write(tcsr_regmap, dload_mode_offset, + QCOM_SET_DLOAD_MODE); + } + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); msm_ps_hold = devm_ioremap_resource(dev, mem); if (IS_ERR(msm_ps_hold)) @@ -59,6 +89,13 @@ static int msm_restart_probe(struct platform_device *pdev) return 0; } +static void msm_restart_shutdown(struct platform_device *pdev) +{ + /* Clean shutdown, disable download mode to allow normal restart */ + if (download_mode) + regmap_write(tcsr_regmap, dload_mode_offset, 0x0); +} + static const struct of_device_id of_msm_restart_match[] = { { .compatible = "qcom,pshold", }, {}, @@ -71,6 +108,7 @@ static int msm_restart_probe(struct platform_device *pdev) .name = "msm-restart", .of_match_table = of_match_ptr(of_msm_restart_match), }, + .shutdown = msm_restart_shutdown, }; static int __init msm_restart_init(void)
commit '8c1b7dc9b: firmware: qcom: scm: Expose download-mode control' added support for download-mode control using the scm firmware driver for platforms which require a secure call to write the magic cookie into the tcsr location. For platforms which *do not* need an scm call and where the kernel can write the cookie by a direct read/write, add similar support in the msm-poweroff driver. Similar to the scm driver, the msm-poweroff driver clears the cookie during a clean reboot. Download mode using msm-poweroff driver can be enabled by including msm-poweroff.download_mode=1 on the command line. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- .../bindings/power/reset/msm-poweroff.txt | 3 ++ drivers/power/reset/Kconfig | 11 +++++++ drivers/power/reset/msm-poweroff.c | 38 ++++++++++++++++++++++ 3 files changed, 52 insertions(+)