Re: [PATCH] power: reset: msm: Add support for download-mode control

[Bjorn Andersson]

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@xxxxxxxxxxxxxx>
> ---
>  .../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