[PATCH v2 1/3] firmware: qcom: scm: Expose secure IO service

[PATCH v2 1/3] firmware: qcom: scm: Expose secure IO service

[Bjorn Andersson]

The secure IO service provides operations for reading and writing secure
memory from non-secure mode, expose this API through SCM.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Correct scm-call return value handling
- Make scm_io_readl() return data by reference

 drivers/firmware/qcom_scm-32.c | 18 ++++++++++++++++++
 drivers/firmware/qcom_scm-64.c | 31 +++++++++++++++++++++++++++++++
 drivers/firmware/qcom_scm.c    | 12 ++++++++++++
 drivers/firmware/qcom_scm.h    |  6 ++++++
 include/linux/qcom_scm.h       |  4 ++++
 5 files changed, 71 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 93e3b96b6dfa..11fdb1584823 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -596,3 +596,21 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 {
 	return -ENODEV;
 }
+
+int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr,
+			unsigned int *val)
+{
+	int ret;
+
+	ret = qcom_scm_call_atomic1(QCOM_SCM_SVC_IO, QCOM_SCM_IO_READ, addr);
+	if (ret >= 0)
+		*val = ret;
+
+	return ret < 0 ? ret : 0;
+}
+
+int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val)
+{
+	return qcom_scm_call_atomic2(QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE,
+				     addr, val);
+}
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 6e6d561708e2..bf50fb59852e 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -439,3 +439,34 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 
 	return ret;
 }
+
+int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr,
+			unsigned int *val)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+	int ret;
+
+	desc.args[0] = addr;
+	desc.arginfo = QCOM_SCM_ARGS(1);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_IO, QCOM_SCM_IO_READ,
+			    &desc, &res);
+	if (ret >= 0)
+		*val = res.a1;
+
+	return ret < 0 ? ret : 0;
+}
+
+int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+
+	desc.args[0] = addr;
+	desc.args[1] = val;
+	desc.arginfo = QCOM_SCM_ARGS(2);
+
+	return qcom_scm_call(dev, QCOM_SCM_SVC_IO, QCOM_SCM_IO_WRITE,
+			     &desc, &res);
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510d75ba..e18d63935648 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -333,6 +333,18 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
 }
 EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
 
+int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
+{
+	return __qcom_scm_io_readl(__scm->dev, addr, val);
+}
+EXPORT_SYMBOL(qcom_scm_io_readl);
+
+int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
+{
+	return __qcom_scm_io_writel(__scm->dev, addr, val);
+}
+EXPORT_SYMBOL(qcom_scm_io_writel);
+
 /**
  * qcom_scm_is_available() - Checks if SCM is available
  */
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 9bea691f30fb..a60e4b9b1394 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -30,6 +30,12 @@ extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
 #define QCOM_SCM_CMD_CORE_HOTPLUGGED	0x10
 extern void __qcom_scm_cpu_power_down(u32 flags);
 
+#define QCOM_SCM_SVC_IO			0x5
+#define QCOM_SCM_IO_READ		0x1
+#define QCOM_SCM_IO_WRITE		0x2
+extern int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr, unsigned int *val);
+extern int __qcom_scm_io_writel(struct device *dev, phys_addr_t addr, unsigned int val);
+
 #define QCOM_SCM_SVC_INFO		0x6
 #define QCOM_IS_CALL_AVAIL_CMD		0x1
 extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index e5380471c2cd..e8357f570695 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -43,6 +43,8 @@ extern int qcom_scm_set_remote_state(u32 state, u32 id);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
 extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
 extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
+extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
+extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
 #else
 static inline
 int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus)
@@ -73,5 +75,7 @@ qcom_scm_set_remote_state(u32 state,u32 id) { return -ENODEV; }
 static inline int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare) { return -ENODEV; }
 static inline int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size) { return -ENODEV; }
 static inline int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare) { return -ENODEV; }
+static inline int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val) { return -ENODEV; }
+static inline int qcom_scm_io_writel(phys_addr_t addr, unsigned int val) { return -ENODEV; }
 #endif
 #endif
-- 
2.12.0

[PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Bjorn Andersson]

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(+)

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
-- 
2.12.0

Re: [PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Stephen Boyd]

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.

> 
> 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?

>  
>  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?


> +
>  #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)

> +
> +	if (download_mode)
> +		qcom_scm_set_download_mode(true);
> +
>  	return 0;
>  }
>  
> +void qcom_scm_shutdown(struct platform_device *pdev)

static?

> +{
> +	if (download_mode)
> +		qcom_scm_set_download_mode(false);
> +}
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Bjorn Andersson]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Stephen Boyd]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Rob Herring]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] firmware: qcom: scm: Expose download-mode control

[Bjorn Andersson]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] arm64: dts: qcom: Specify dload address for msm8916 and msm8996

[Bjorn Andersson]

On msm8916 and msm8996 boards a secure io-write is used to write the
magic for selecting "download mode", specify this address in the
DeviceTree.

Note that qcom_scm.download_mode=1 must be specified on the kernel
command line for the kernel to attempt selecting download mode.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Add msm8916 to the patch

 arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index ab3093995ded..33013835639d 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -241,6 +241,8 @@
 			clocks = <&gcc GCC_CRYPTO_CLK>, <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
 			clock-names = "core", "bus", "iface";
 			#reset-cells = <1>;
+
+			qcom,dload-mode-addr = <0x0 0x193d100>;
 		};
 	};
 
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 9bc9c857a000..7e64884ab7c7 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -261,6 +261,8 @@
 	firmware {
 		scm {
 			compatible = "qcom,scm-msm8996";
+
+			qcom,dload-mode-addr = <0x0 0x7b3000>;
 		};
 	};
 
-- 
2.12.0

Re: [PATCH v2 3/3] arm64: dts: qcom: Specify dload address for msm8916 and msm8996

[Stephen Boyd]

On 05/26, Bjorn Andersson wrote:
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index ab3093995ded..33013835639d 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -241,6 +241,8 @@
>  			clocks = <&gcc GCC_CRYPTO_CLK>, <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
>  			clock-names = "core", "bus", "iface";
>  			#reset-cells = <1>;
> +
> +			qcom,dload-mode-addr = <0x0 0x193d100>;

This is TCSR, so why not a phandle to a node?

>  		};
>  	};
>  
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 9bc9c857a000..7e64884ab7c7 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -261,6 +261,8 @@
>  	firmware {
>  		scm {
>  			compatible = "qcom,scm-msm8996";
> +
> +			qcom,dload-mode-addr = <0x0 0x7b3000>;

Also TCSR. Same question.

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] arm64: dts: qcom: Specify dload address for msm8916 and msm8996

[Bjorn Andersson]

On Wed 31 May 09:30 PDT 2017, Stephen Boyd wrote:

> On 05/26, Bjorn Andersson wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index ab3093995ded..33013835639d 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -241,6 +241,8 @@
> >  			clocks = <&gcc GCC_CRYPTO_CLK>, <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> >  			clock-names = "core", "bus", "iface";
> >  			#reset-cells = <1>;
> > +
> > +			qcom,dload-mode-addr = <0x0 0x193d100>;
> 
> This is TCSR, so why not a phandle to a node?
> 

That makes sense, the region is not mentioned in my documentation and I
didn't catch that it's in the middle of &tcsr. I'll update it to
reference the existing syscon.

Regards,
Bjorn

Re: [PATCH v2 1/3] firmware: qcom: scm: Expose secure IO service

[Stephen Boyd]

On 05/26, Bjorn Andersson wrote:
> The secure IO service provides operations for reading and writing secure
> memory from non-secure mode, expose this API through SCM.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> Changes since v1:
> - Correct scm-call return value handling
> - Make scm_io_readl() return data by reference

I'm not sure we ever cared to check the return code of
read/write?

Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

-- 
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html