[PATCH v6 0/2] soc: loongson2_pm: add power management support

[PATCH v6 0/2] soc: loongson2_pm: add power management support

[Yinbo Zhu]

Loongson-2 platform support Power Management Controller (ACPI) and this
series patch was to add PM driver that base on dts and PM binding support.

Change in v6:
		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
		   pm interfaces" had been merged into mainline tree in v6.5-rc1
		   thus this v6 series patch need drop it and need depend on it
		   and it's patch link was:
https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
		2. Adding Ulf Hansson to Cc.
		3. Adding soc@kernel.org to Cc.
		4. Keep indented with one tab +2 spaces in Kconfig help text.
Change in v5:
		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
		   pm interfaces" had been merged into linux-next tree thus this
		   v4 series patch need drop it and need depend on it and it's
		   patch link was:
https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
		2. Swap the positions of compatible for 2k1000 and 2k0500.
Change in v4:
		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
		   pm interfaces" had been merged into linux-next tree thus this
		   v4 series patch need drop it and need depend on it and it's
		   patch link was:
https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
		2. Remove the pmc label in dt-binding patch.
		3. Add the Co-developed-by for driver patch.
		4. Simplify the loongson2_suspend_valid_state that "return
		   (state == PM_SUSPEND_MEM)".
		5. Use Using loongson2_pm_irq_enable() to replace.
		   loongson2_power_button_irq_enable().
		6. Remove the "oneOf" in dt-bindings patch.
		7. Replace "suspend-address" that use "loongson,suspend-address".
		8. Use u64 type that for "loongson,suspend-address".
		9. Rename "pm" to "power-mangement" in dt-bindings patch.
		10. Add the reivewed-by for dt-bindings patch.
Change in v3:
		1. Reword the [1/3] patch commit log and title.
		2. Use the old naming for suspend interface for the [1/3] and
		   [3/3] patch.
		3. Combine some small function in the driver patch.
		4. Rename 'pwrbt' to 'button' in the driver patch.
		5. Use the specific compatible in yaml file.
Change in v2:
		1. Fixup the "suspend-address" description.
		2. Remove the "return -EINVAL" in PM driver probe when firmware
		   no configure "suspend-address" property in dts in oder to
		   other PM state to work.

Yinbo Zhu (2):
  soc: dt-bindings: add loongson-2 pm
  soc: loongson2_pm: add power management support

 .../soc/loongson/loongson,ls2k-pmc.yaml       |  52 +++++
 MAINTAINERS                                   |   7 +
 drivers/soc/loongson/Kconfig                  |  10 +
 drivers/soc/loongson/Makefile                 |   1 +
 drivers/soc/loongson/loongson2_pm.c           | 215 ++++++++++++++++++
 5 files changed, 285 insertions(+)

-- 
2.20.1

[PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Yinbo Zhu]

Add the Loongson-2 SoC Power Management Controller binding with DT
schema format using json-schema.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../soc/loongson/loongson,ls2k-pmc.yaml       | 52 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml

diff --git a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
new file mode 100644
index 000000000000..da2dcfeebf12
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/loongson/loongson,ls2k-pmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-2 Power Manager controller
+
+maintainers:
+  - Yinbo Zhu <zhuyinbo@loongson.cn>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - loongson,ls2k0500-pmc
+          - loongson,ls2k1000-pmc
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  loongson,suspend-address:
+    $ref: /schemas/types.yaml#/definitions/uint64
+    description:
+      The "loongson,suspend-address" is a deep sleep state (Suspend To
+      RAM) firmware entry address which was jumped from kernel and it's
+      value was dependent on specific platform firmware code. In
+      addition, the PM need according to it to indicate that current
+      SoC whether support Suspend To RAM.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    power-management@1fe27000 {
+        compatible = "loongson,ls2k1000-pmc", "syscon";
+        reg = <0x1fe27000 0x58>;
+        interrupt-parent = <&liointc1>;
+        interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
+        loongson,suspend-address = <0x0 0x1c000500>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1089ef3319f2..608a00473498 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12365,6 +12365,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
 F:	drivers/soc/loongson/loongson2_guts.c
 
+LOONGSON-2 SOC SERIES PM DRIVER
+M:	Yinbo Zhu <zhuyinbo@loongson.cn>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
+
 LOONGSON-2 SOC SERIES PINCTRL DRIVER
 M:	zhanghongchen <zhanghongchen@loongson.cn>
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
-- 
2.20.1

[PATCH v6 2/2] soc: loongson2_pm: add power management support

[Yinbo Zhu]

The Loongson-2's power management controller was ACPI, supports ACPI
S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
(USB, GMAC, PWRBTN, etc.). This driver was to add power management
controller support that base on dts for Loongson-2 series SoCs.

Co-developed-by: Liu Yun <liuyun@loongson.cn>
Signed-off-by: Liu Yun <liuyun@loongson.cn>
Co-developed-by: Liu Peibao <liupeibao@loongson.cn>
Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
Cc: soc@kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 MAINTAINERS                         |   1 +
 drivers/soc/loongson/Kconfig        |  10 ++
 drivers/soc/loongson/Makefile       |   1 +
 drivers/soc/loongson/loongson2_pm.c | 215 ++++++++++++++++++++++++++++
 4 files changed, 227 insertions(+)
 create mode 100644 drivers/soc/loongson/loongson2_pm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 608a00473498..35e0757785f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12370,6 +12370,7 @@ M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-pm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
+F:	drivers/soc/loongson/loongson2_pm.c
 
 LOONGSON-2 SOC SERIES PINCTRL DRIVER
 M:	zhanghongchen <zhanghongchen@loongson.cn>
diff --git a/drivers/soc/loongson/Kconfig b/drivers/soc/loongson/Kconfig
index 707f56358dc4..314e13bb3e01 100644
--- a/drivers/soc/loongson/Kconfig
+++ b/drivers/soc/loongson/Kconfig
@@ -16,3 +16,13 @@ config LOONGSON2_GUTS
 	 SoCs. Initially only reading SVR and registering soc device are
 	 supported. Other guts accesses, such as reading firmware configuration
 	 by default, should eventually be added into this driver as well.
+
+config LOONGSON2_PM
+	bool "Loongson-2 SoC Power Management Controller Driver"
+	depends on LOONGARCH && OF
+	help
+	  The Loongson-2's power management controller was ACPI, supports ACPI
+	  S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
+	  Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
+	  (USB, GMAC, PWRBTN, etc.). This driver was to add power management
+	  controller support that base on dts for Loongson-2 series SoCs.
diff --git a/drivers/soc/loongson/Makefile b/drivers/soc/loongson/Makefile
index 263c486df638..4118f50f55e2 100644
--- a/drivers/soc/loongson/Makefile
+++ b/drivers/soc/loongson/Makefile
@@ -4,3 +4,4 @@
 #
 
 obj-$(CONFIG_LOONGSON2_GUTS)		+= loongson2_guts.o
+obj-$(CONFIG_LOONGSON2_PM)		+= loongson2_pm.o
diff --git a/drivers/soc/loongson/loongson2_pm.c b/drivers/soc/loongson/loongson2_pm.c
new file mode 100644
index 000000000000..796add6e8b63
--- /dev/null
+++ b/drivers/soc/loongson/loongson2_pm.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Loongson-2 PM Support
+ *
+ * Copyright (C) 2023 Loongson Technology Corporation Limited
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/suspend.h>
+#include <linux/interrupt.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/platform_device.h>
+#include <asm/bootinfo.h>
+#include <asm/suspend.h>
+
+#define LOONGSON2_PM1_CNT_REG		0x14
+#define LOONGSON2_PM1_STS_REG		0x0c
+#define LOONGSON2_PM1_ENA_REG		0x10
+#define LOONGSON2_GPE0_STS_REG		0x28
+#define LOONGSON2_GPE0_ENA_REG		0x2c
+
+#define LOONGSON2_PM1_PWRBTN_STS	BIT(8)
+#define LOONGSON2_PM1_PCIEXP_WAKE_STS	BIT(14)
+#define LOONGSON2_PM1_WAKE_STS		BIT(15)
+#define LOONGSON2_PM1_CNT_INT_EN	BIT(0)
+#define LOONGSON2_PM1_PWRBTN_EN		LOONGSON2_PM1_PWRBTN_STS
+
+static struct loongson2_pm {
+	void __iomem			*base;
+	struct input_dev		*dev;
+	bool				suspended;
+} loongson2_pm;
+
+#define loongson2_pm_readw(reg)		readw(loongson2_pm.base + reg)
+#define loongson2_pm_readl(reg)		readl(loongson2_pm.base + reg)
+#define loongson2_pm_writew(val, reg)	writew(val, loongson2_pm.base + reg)
+#define loongson2_pm_writel(val, reg)	writel(val, loongson2_pm.base + reg)
+
+static void loongson2_pm_status_clear(void)
+{
+	u16 value;
+
+	value = loongson2_pm_readw(LOONGSON2_PM1_STS_REG);
+	value |= (LOONGSON2_PM1_PWRBTN_STS | LOONGSON2_PM1_PCIEXP_WAKE_STS |
+		  LOONGSON2_PM1_WAKE_STS);
+	loongson2_pm_writew(value, LOONGSON2_PM1_STS_REG);
+	loongson2_pm_writel(loongson2_pm_readl(LOONGSON2_GPE0_STS_REG), LOONGSON2_GPE0_STS_REG);
+}
+
+static void loongson2_pm_irq_enable(void)
+{
+	u16 value;
+
+	value = loongson2_pm_readw(LOONGSON2_PM1_CNT_REG);
+	value |= LOONGSON2_PM1_CNT_INT_EN;
+	loongson2_pm_writew(value, LOONGSON2_PM1_CNT_REG);
+
+	value = loongson2_pm_readw(LOONGSON2_PM1_ENA_REG);
+	value |= LOONGSON2_PM1_PWRBTN_EN;
+	loongson2_pm_writew(value, LOONGSON2_PM1_ENA_REG);
+}
+
+static int loongson2_suspend_enter(suspend_state_t state)
+{
+	loongson2_pm_status_clear();
+	loongarch_common_suspend();
+	loongarch_suspend_enter();
+	loongarch_common_resume();
+	loongson2_pm_irq_enable();
+	pm_set_resume_via_firmware();
+
+	return 0;
+}
+
+static int loongson2_suspend_begin(suspend_state_t state)
+{
+	pm_set_suspend_via_firmware();
+
+	return 0;
+}
+
+static int loongson2_suspend_valid_state(suspend_state_t state)
+{
+	return (state == PM_SUSPEND_MEM);
+}
+
+static const struct platform_suspend_ops loongson2_suspend_ops = {
+	.valid	= loongson2_suspend_valid_state,
+	.begin	= loongson2_suspend_begin,
+	.enter	= loongson2_suspend_enter,
+};
+
+static int loongson2_power_button_init(struct device *dev, int irq)
+{
+	int ret;
+	struct input_dev *button;
+
+	button = input_allocate_device();
+	if (!dev)
+		return -ENOMEM;
+
+	button->name = "Power Button";
+	button->phys = "pm/button/input0";
+	button->id.bustype = BUS_HOST;
+	button->dev.parent = NULL;
+	input_set_capability(button, EV_KEY, KEY_POWER);
+
+	ret = input_register_device(button);
+	if (ret)
+		goto free_dev;
+
+	dev_pm_set_wake_irq(&button->dev, irq);
+	device_set_wakeup_capable(&button->dev, true);
+	device_set_wakeup_enable(&button->dev, true);
+
+	loongson2_pm.dev = button;
+	dev_info(dev, "Power Button: Init successful!\n");
+
+	return 0;
+
+free_dev:
+	input_free_device(button);
+
+	return ret;
+}
+
+static irqreturn_t loongson2_pm_irq_handler(int irq, void *dev_id)
+{
+	u16 status = loongson2_pm_readw(LOONGSON2_PM1_STS_REG);
+
+	if (!loongson2_pm.suspended && (status & LOONGSON2_PM1_PWRBTN_STS)) {
+		pr_info("Power Button pressed...\n");
+		input_report_key(loongson2_pm.dev, KEY_POWER, 1);
+		input_sync(loongson2_pm.dev);
+		input_report_key(loongson2_pm.dev, KEY_POWER, 0);
+		input_sync(loongson2_pm.dev);
+	}
+
+	loongson2_pm_status_clear();
+
+	return IRQ_HANDLED;
+}
+
+static int __maybe_unused loongson2_pm_suspend(struct device *dev)
+{
+	loongson2_pm.suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused loongson2_pm_resume(struct device *dev)
+{
+	loongson2_pm.suspended = false;
+
+	return 0;
+}
+static SIMPLE_DEV_PM_OPS(loongson2_pm_ops, loongson2_pm_suspend, loongson2_pm_resume);
+
+static int loongson2_pm_probe(struct platform_device *pdev)
+{
+	int irq, retval;
+	u64 suspend_addr;
+	struct device *dev = &pdev->dev;
+
+	loongson2_pm.base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(loongson2_pm.base))
+		return PTR_ERR(loongson2_pm.base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	if (!device_property_read_u64(dev, "loongson,suspend-address", &suspend_addr))
+		loongson_sysconf.suspend_addr = (u64)phys_to_virt(suspend_addr);
+	else
+		dev_err(dev, "No loongson,suspend-address, could not support S3!\n");
+
+	if (loongson2_power_button_init(dev, irq))
+		return -EINVAL;
+
+	retval = devm_request_irq(&pdev->dev, irq, loongson2_pm_irq_handler,
+				  IRQF_SHARED, "pm_irq", &loongson2_pm);
+	if (retval)
+		return retval;
+
+	loongson2_pm_irq_enable();
+	loongson2_pm_status_clear();
+
+	if (loongson_sysconf.suspend_addr)
+		suspend_set_ops(&loongson2_suspend_ops);
+
+	return 0;
+}
+
+static const struct of_device_id loongson2_pm_match[] = {
+	{ .compatible = "loongson,ls2k0500-pmc", },
+	{ .compatible = "loongson,ls2k1000-pmc", },
+	{},
+};
+
+static struct platform_driver loongson2_pm_driver = {
+	.driver = {
+		.name = "ls2k-pm",
+		.pm = &loongson2_pm_ops,
+		.of_match_table = loongson2_pm_match,
+	},
+	.probe = loongson2_pm_probe,
+};
+module_platform_driver(loongson2_pm_driver);
+
+MODULE_DESCRIPTION("Loongson-2 PM driver");
+MODULE_LICENSE("GPL");
-- 
2.20.1

Re: [PATCH v6 2/2] soc: loongson2_pm: add power management support

[Arnd Bergmann]

On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
> The Loongson-2's power management controller was ACPI, supports ACPI
> S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
> Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
> (USB, GMAC, PWRBTN, etc.). This driver was to add power management
> controller support that base on dts for Loongson-2 series SoCs.
>
> Co-developed-by: Liu Yun <liuyun@loongson.cn>
> Signed-off-by: Liu Yun <liuyun@loongson.cn>
> Co-developed-by: Liu Peibao <liupeibao@loongson.cn>
> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> Cc: soc@kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

I'm still waiting for Ulf to take a look here to see whether
this should be in drivers/genpd instead, but he might still
be on vacation.

A few minor comments from me in the meantime:

> +#define loongson2_pm_readw(reg)		readw(loongson2_pm.base + reg)
> +#define loongson2_pm_readl(reg)		readl(loongson2_pm.base + reg)
> +#define loongson2_pm_writew(val, reg)	writew(val, loongson2_pm.base + 
> reg)
> +#define loongson2_pm_writel(val, reg)	writel(val, loongson2_pm.base + 
> reg)

I would prefer these to be 'static inline' functions rather than
macros, or you can just open-code them, as each macro is only
used once at the moment.

> +static irqreturn_t loongson2_pm_irq_handler(int irq, void *dev_id)
> +{
> +	u16 status = loongson2_pm_readw(LOONGSON2_PM1_STS_REG);
> +
> +	if (!loongson2_pm.suspended && (status & LOONGSON2_PM1_PWRBTN_STS)) {
> +		pr_info("Power Button pressed...\n");

The message is probably more appropriate as a pr_debug() than
pr_info().

> +static int __maybe_unused loongson2_pm_suspend(struct device *dev)
> +{
> +	loongson2_pm.suspended = true;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused loongson2_pm_resume(struct device *dev)
> +{
> +	loongson2_pm.suspended = false;
> +
> +	return 0;
> +}
> +static SIMPLE_DEV_PM_OPS(loongson2_pm_ops, loongson2_pm_suspend, 
> loongson2_pm_resume);

Please change this to DEFINE_SIMPLE_DEV_PM_OPS() and remove the
__maybe_unused, this is what all drivers should have these days.

> +
> +static int loongson2_pm_probe(struct platform_device *pdev)
> +{
> +	int irq, retval;
> +	u64 suspend_addr;
> +	struct device *dev = &pdev->dev;
> +
> +	loongson2_pm.base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(loongson2_pm.base))
> +		return PTR_ERR(loongson2_pm.base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	if (!device_property_read_u64(dev, "loongson,suspend-address", 
> &suspend_addr))
> +		loongson_sysconf.suspend_addr = (u64)phys_to_virt(suspend_addr);
> +	else

Having a custom "loongson,suspend-address" property here feels wrong
to me. Can't this be moved into the "regs" property that holds
the other mmio registers?

    Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Arnd Bergmann]

On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:

> +  loongson,suspend-address:
> +    $ref: /schemas/types.yaml#/definitions/uint64
> +    description:
> +      The "loongson,suspend-address" is a deep sleep state (Suspend To
> +      RAM) firmware entry address which was jumped from kernel and it's
> +      value was dependent on specific platform firmware code. In
> +      addition, the PM need according to it to indicate that current
> +      SoC whether support Suspend To RAM.
> +

I just commented on this in the driver patch, assuming this
was an MMIO address, but I'm even more confused now, since
we try hard to not rely on being able to just interface with
firmware like this.

If this is executable code, where does this actually reside?
Is this some SRAM that needs to execute the suspend logic
in order to shut down memory and cache controllers? Or is
this a runtime firmware interface similar to how UEFI handles
its runtime services to keep the implementation out of
the kernel?

Does the code work with both traditional suspend-to-ram and
modern suspend-to-idle logic?

      Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Yinbo Zhu]

在 2023/8/3 下午3:44, Arnd Bergmann 写道:
> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
> 
>> +  loongson,suspend-address:
>> +    $ref: /schemas/types.yaml#/definitions/uint64
>> +    description:
>> +      The "loongson,suspend-address" is a deep sleep state (Suspend To
>> +      RAM) firmware entry address which was jumped from kernel and it's
>> +      value was dependent on specific platform firmware code. In
>> +      addition, the PM need according to it to indicate that current
>> +      SoC whether support Suspend To RAM.
>> +
> 
> I just commented on this in the driver patch, assuming this
> was an MMIO address, but I'm even more confused now, since
> we try hard to not rely on being able to just interface with
> firmware like this.
> 
> If this is executable code, where does this actually reside?


Pmon firmware code.

> Is this some SRAM that needs to execute the suspend logic
> in order to shut down memory and cache controllers? 


Yes, The suspend-to-ram after into pmon firmware code and set
self-refresh mode in memory controller and ensure that memory data is
not lost then shut down memory controller.

> Or is
> this a runtime firmware interface similar to how UEFI handles
> its runtime services to keep the implementation out of
> the kernel?


No, The main cpu and other cpu will offline that after into firmware and
finished Corresponding operations, the pmon firmware will not run.

> 
> Does the code work with both traditional suspend-to-ram and
> modern suspend-to-idle logic?


Yes, It can support suspend-to-ram and suspend-to-idle.

Thanks,
Yinbo

Re: [PATCH v6 2/2] soc: loongson2_pm: add power management support

[Ulf Hansson]

On Thu, 3 Aug 2023 at 09:03, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
> > The Loongson-2's power management controller was ACPI, supports ACPI
> > S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
> > Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
> > (USB, GMAC, PWRBTN, etc.). This driver was to add power management
> > controller support that base on dts for Loongson-2 series SoCs.
> >
> > Co-developed-by: Liu Yun <liuyun@loongson.cn>
> > Signed-off-by: Liu Yun <liuyun@loongson.cn>
> > Co-developed-by: Liu Peibao <liupeibao@loongson.cn>
> > Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
> > Cc: soc@kernel.org
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>
> I'm still waiting for Ulf to take a look here to see whether
> this should be in drivers/genpd instead, but he might still
> be on vacation.

I don't think this belongs in drivers/genpd/ as it's not a genpd
provider. Besides that, no further comments from me at this point.

Kind regards
Uffe

>
> A few minor comments from me in the meantime:
>
> > +#define loongson2_pm_readw(reg)              readw(loongson2_pm.base + reg)
> > +#define loongson2_pm_readl(reg)              readl(loongson2_pm.base + reg)
> > +#define loongson2_pm_writew(val, reg)        writew(val, loongson2_pm.base +
> > reg)
> > +#define loongson2_pm_writel(val, reg)        writel(val, loongson2_pm.base +
> > reg)
>
> I would prefer these to be 'static inline' functions rather than
> macros, or you can just open-code them, as each macro is only
> used once at the moment.
>
> > +static irqreturn_t loongson2_pm_irq_handler(int irq, void *dev_id)
> > +{
> > +     u16 status = loongson2_pm_readw(LOONGSON2_PM1_STS_REG);
> > +
> > +     if (!loongson2_pm.suspended && (status & LOONGSON2_PM1_PWRBTN_STS)) {
> > +             pr_info("Power Button pressed...\n");
>
> The message is probably more appropriate as a pr_debug() than
> pr_info().
>
> > +static int __maybe_unused loongson2_pm_suspend(struct device *dev)
> > +{
> > +     loongson2_pm.suspended = true;
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused loongson2_pm_resume(struct device *dev)
> > +{
> > +     loongson2_pm.suspended = false;
> > +
> > +     return 0;
> > +}
> > +static SIMPLE_DEV_PM_OPS(loongson2_pm_ops, loongson2_pm_suspend,
> > loongson2_pm_resume);
>
> Please change this to DEFINE_SIMPLE_DEV_PM_OPS() and remove the
> __maybe_unused, this is what all drivers should have these days.
>
> > +
> > +static int loongson2_pm_probe(struct platform_device *pdev)
> > +{
> > +     int irq, retval;
> > +     u64 suspend_addr;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     loongson2_pm.base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(loongson2_pm.base))
> > +             return PTR_ERR(loongson2_pm.base);
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq < 0)
> > +             return irq;
> > +
> > +     if (!device_property_read_u64(dev, "loongson,suspend-address",
> > &suspend_addr))
> > +             loongson_sysconf.suspend_addr = (u64)phys_to_virt(suspend_addr);
> > +     else
>
> Having a custom "loongson,suspend-address" property here feels wrong
> to me. Can't this be moved into the "regs" property that holds
> the other mmio registers?
>
>     Arnd

Re: [PATCH v6 2/2] soc: loongson2_pm: add power management support

[Arnd Bergmann]

On Tue, Aug 8, 2023, at 16:42, Ulf Hansson wrote:
> On Thu, 3 Aug 2023 at 09:03, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
>> > The Loongson-2's power management controller was ACPI, supports ACPI
>> > S2Idle (Suspend To Idle), ACPI S3 (Suspend To RAM), ACPI S4 (Suspend To
>> > Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
>> > (USB, GMAC, PWRBTN, etc.). This driver was to add power management
>> > controller support that base on dts for Loongson-2 series SoCs.
>> >
>> > Co-developed-by: Liu Yun <liuyun@loongson.cn>
>> > Signed-off-by: Liu Yun <liuyun@loongson.cn>
>> > Co-developed-by: Liu Peibao <liupeibao@loongson.cn>
>> > Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
>> > Cc: soc@kernel.org
>> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>
>> I'm still waiting for Ulf to take a look here to see whether
>> this should be in drivers/genpd instead, but he might still
>> be on vacation.
>
> I don't think this belongs in drivers/genpd/ as it's not a genpd
> provider. Besides that, no further comments from me at this point.

Ok, thanks!

Let's try to finish discussing the suspend-address question
and merge it through the soc tree once that is resolved then.

     Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Arnd Bergmann]

On Fri, Aug 4, 2023, at 04:54, Yinbo Zhu wrote:
> 在 2023/8/3 下午3:44, Arnd Bergmann 写道:
>> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
>> 
>>> +  loongson,suspend-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint64
>>> +    description:
>>> +      The "loongson,suspend-address" is a deep sleep state (Suspend To
>>> +      RAM) firmware entry address which was jumped from kernel and it's
>>> +      value was dependent on specific platform firmware code. In
>>> +      addition, the PM need according to it to indicate that current
>>> +      SoC whether support Suspend To RAM.
>>> +
>> 
>> I just commented on this in the driver patch, assuming this
>> was an MMIO address, but I'm even more confused now, since
>> we try hard to not rely on being able to just interface with
>> firmware like this.
>> 
>> If this is executable code, where does this actually reside?
>
>
> Pmon firmware code.
>
>> Is this some SRAM that needs to execute the suspend logic
>> in order to shut down memory and cache controllers? 
>
>
> Yes, The suspend-to-ram after into pmon firmware code and set
> self-refresh mode in memory controller and ensure that memory data is
> not lost then shut down memory controller.

I'm sorry I missed your reply earlier, getting back to the
thread now. So it's clear that this code needs to run in a
special memory from your description, but I'm still trying
to understand the details better.

I found https://github.com/loongson-community/pmon source
code, and a reference to its origin at LSI Logic at
https://www.linux-mips.org/wiki/PMON but otherwise have
no idea about what this actually is, or how it relates
to your UEFI firmware. Did you add UEFI support to PMON,
or do you use it as a first stage loader that loads
the actual UEFI implementation (EDK2 or u-boot, I guess)?

>> Or is
>> this a runtime firmware interface similar to how UEFI handles
>> its runtime services to keep the implementation out of
>> the kernel?
>
>
> No, The main cpu and other cpu will offline that after into firmware and
> finished Corresponding operations, the pmon firmware will not run.

I'm still trying to understand your explanations here.
You say that pmon no longer runs, but that seems to contradict
what you said earlier about branching into pmon firmware code
for suspend.

Is this executing directly from ROM then?

       Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Yinbo Zhu]

在 2023/8/12 下午8:25, Arnd Bergmann 写道:
> On Fri, Aug 4, 2023, at 04:54, Yinbo Zhu wrote:
>> 在 2023/8/3 下午3:44, Arnd Bergmann 写道:
>>> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
>>>
>>>> +  loongson,suspend-address:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint64
>>>> +    description:
>>>> +      The "loongson,suspend-address" is a deep sleep state (Suspend To
>>>> +      RAM) firmware entry address which was jumped from kernel and it's
>>>> +      value was dependent on specific platform firmware code. In
>>>> +      addition, the PM need according to it to indicate that current
>>>> +      SoC whether support Suspend To RAM.
>>>> +
>>>
>>> I just commented on this in the driver patch, assuming this
>>> was an MMIO address, but I'm even more confused now, since
>>> we try hard to not rely on being able to just interface with
>>> firmware like this.
>>>
>>> If this is executable code, where does this actually reside?
>>
>>
>> Pmon firmware code.
>>
>>> Is this some SRAM that needs to execute the suspend logic
>>> in order to shut down memory and cache controllers?
>>
>>
>> Yes, The suspend-to-ram after into pmon firmware code and set
>> self-refresh mode in memory controller and ensure that memory data is
>> not lost then shut down memory controller.
> 
> I'm sorry I missed your reply earlier, getting back to the
> thread now. So it's clear that this code needs to run in a
> special memory from your description, but I'm still trying
> to understand the details better.
> 
> I found https://github.com/loongson-community/pmon source
> code, and a reference to its origin at LSI Logic at
> https://www.linux-mips.org/wiki/PMON but otherwise have
> no idea about what this actually is, or how it relates
> to your UEFI firmware. Did you add UEFI support to PMON,
> or do you use it as a first stage loader that loads
> the actual UEFI implementation (EDK2 or u-boot, I guess)?


Pmon and uefi are two different firmware, and there is no connection
between them.

> 
>>> Or is
>>> this a runtime firmware interface similar to how UEFI handles
>>> its runtime services to keep the implementation out of
>>> the kernel?
>>
>>
>> No, The main cpu and other cpu will offline that after into firmware and
>> finished Corresponding operations, the pmon firmware will not run.
> 
> I'm still trying to understand your explanations here.
> You say that pmon no longer runs, but that seems to contradict
> what you said earlier about branching into pmon firmware code
> for suspend.


It's not contradictory.  The suspend-to-ram is that from kernel goto to
pmon firmware code, then pmon finished corresponding operations, which
was to set self-refresh mode in memory controller, then memory HW will
maintain its own data and no longer requires software processing, pmon
firmware will not run.

> 
> Is this executing directly from ROM then?


Yes.

Thanks,
Yinbo

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Arnd Bergmann]

On Mon, Aug 14, 2023, at 09:57, Yinbo Zhu wrote:
> 在 2023/8/12 下午8:25, Arnd Bergmann 写道:
>> On Fri, Aug 4, 2023, at 04:54, Yinbo Zhu wrote:
>>> 在 2023/8/3 下午3:44, Arnd Bergmann 写道:
>>>> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
>>>
>>>> Is this some SRAM that needs to execute the suspend logic
>>>> in order to shut down memory and cache controllers?
>>>
>>>
>>> Yes, The suspend-to-ram after into pmon firmware code and set
>>> self-refresh mode in memory controller and ensure that memory data is
>>> not lost then shut down memory controller.
>> 
>> I'm sorry I missed your reply earlier, getting back to the
>> thread now. So it's clear that this code needs to run in a
>> special memory from your description, but I'm still trying
>> to understand the details better.
>> 
>> I found https://github.com/loongson-community/pmon source
>> code, and a reference to its origin at LSI Logic at
>> https://www.linux-mips.org/wiki/PMON but otherwise have
>> no idea about what this actually is, or how it relates
>> to your UEFI firmware. Did you add UEFI support to PMON,
>> or do you use it as a first stage loader that loads
>> the actual UEFI implementation (EDK2 or u-boot, I guess)?
>
>
> Pmon and uefi are two different firmware, and there is no connection
> between them.

It sounds like we still have problems with terminology.

I don't think categorizing UEFI as a firmware is correct,
it's the interface used by various firmware implementations
to load the operating system. As far as I understand,
loongarch currently mandates the use of UEFI in whichever
firmware is used, so if you have Pmon installed in ROM,
and Pmon does not itself implement UEFI, it would have
to load some other firmware such as u-boot in order to
load a kernel through the UEFI protocol, right?

Has the assumption that loongarch requires UEFI changed?

>>>> Or is
>>>> this a runtime firmware interface similar to how UEFI handles
>>>> its runtime services to keep the implementation out of
>>>> the kernel?
>>>
>>>
>>> No, The main cpu and other cpu will offline that after into firmware and
>>> finished Corresponding operations, the pmon firmware will not run.
>> 
>> I'm still trying to understand your explanations here.
>> You say that pmon no longer runs, but that seems to contradict
>> what you said earlier about branching into pmon firmware code
>> for suspend.
>
>
> It's not contradictory.  The suspend-to-ram is that from kernel goto to
> pmon firmware code, then pmon finished corresponding operations, which
> was to set self-refresh mode in memory controller, then memory HW will
> maintain its own data and no longer requires software processing, pmon
> firmware will not run.

That is what I mean with a "runtime firmware interface", i.e. you
jump into firmware in order to request services from it. Clearly the
firmware itself does not run while the OS is executing code, but it is
still there and waiting to be called here, which is similar to
things like UEFI runtime services, PowerPC RTAS, Arm EL3/trustzone
based firmware or x86 SMM firmware, except that this is much less
formalized and only consists of an entry point with undocument
calling conventions.

>> Is this executing directly from ROM then?
>
> Yes.

Is this the only runtime call into the firmware, or are there
others that are either already called from mainline kernels
or in your downsteam implementation?

How do you ensure that the DTB matches the actual ROM code
after rebuilding Pmon? Does Pmon itself fill that field with
the correct address, or do you rely on it being a hardcoded
constant?

     Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Yinbo Zhu]

在 2023/8/14 下午4:19, Arnd Bergmann 写道:
> On Mon, Aug 14, 2023, at 09:57, Yinbo Zhu wrote:
>> 在 2023/8/12 下午8:25, Arnd Bergmann 写道:
>>> On Fri, Aug 4, 2023, at 04:54, Yinbo Zhu wrote:
>>>> 在 2023/8/3 下午3:44, Arnd Bergmann 写道:
>>>>> On Thu, Aug 3, 2023, at 08:37, Yinbo Zhu wrote:
>>>>
>>>>> Is this some SRAM that needs to execute the suspend logic
>>>>> in order to shut down memory and cache controllers?
>>>>
>>>>
>>>> Yes, The suspend-to-ram after into pmon firmware code and set
>>>> self-refresh mode in memory controller and ensure that memory data is
>>>> not lost then shut down memory controller.
>>>
>>> I'm sorry I missed your reply earlier, getting back to the
>>> thread now. So it's clear that this code needs to run in a
>>> special memory from your description, but I'm still trying
>>> to understand the details better.
>>>
>>> I found https://github.com/loongson-community/pmon source
>>> code, and a reference to its origin at LSI Logic at
>>> https://www.linux-mips.org/wiki/PMON but otherwise have
>>> no idea about what this actually is, or how it relates
>>> to your UEFI firmware. Did you add UEFI support to PMON,
>>> or do you use it as a first stage loader that loads
>>> the actual UEFI implementation (EDK2 or u-boot, I guess)?
>>
>>
>> Pmon and uefi are two different firmware, and there is no connection
>> between them.
> 
> It sounds like we still have problems with terminology. >
> I don't think categorizing UEFI as a firmware is correct,


Sorry to have confused you, uefi firmware is our internal name, which is
actually what you referred to as EDK2, the EDK2 need use UEFI.

> it's the interface used by various firmware implementations
> to load the operating system. As far as I understand,
> loongarch currently mandates the use of UEFI in whichever
> firmware is used, so if you have Pmon installed in ROM > and Pmon does not itself implement UEFI, it would have
> to load some other firmware such as u-boot in order to
> load a kernel through the UEFI protocol, right?


PMON is an independent firmware and loader that can directly load the
operating system and it does not require the use of UEFI.

> 
> Has the assumption that loongarch requires UEFI changed?


LoongArch embedded board was use Pmon firmware, The other one uses UEFI
firmware (EDK2) on LoongArch platform.

> 
>>>>> Or is
>>>>> this a runtime firmware interface similar to how UEFI handles
>>>>> its runtime services to keep the implementation out of
>>>>> the kernel?
>>>>
>>>>
>>>> No, The main cpu and other cpu will offline that after into firmware and
>>>> finished Corresponding operations, the pmon firmware will not run.
>>>
>>> I'm still trying to understand your explanations here.
>>> You say that pmon no longer runs, but that seems to contradict
>>> what you said earlier about branching into pmon firmware code
>>> for suspend.
>>
>>
>> It's not contradictory.  The suspend-to-ram is that from kernel goto to
>> pmon firmware code, then pmon finished corresponding operations, which
>> was to set self-refresh mode in memory controller, then memory HW will
>> maintain its own data and no longer requires software processing, pmon
>> firmware will not run.
> 
> That is what I mean with a "runtime firmware interface", i.e. you
> jump into firmware in order to request services from it. Clearly the
> firmware itself does not run while the OS is executing code, but it is
> still there and waiting to be called here, which is similar to
> things like UEFI runtime services, PowerPC RTAS, Arm EL3/trustzone
> based firmware or x86 SMM firmware, except that this is much less
> formalized and only consists of an entry point with undocument
> calling conventions.
> 
>>> Is this executing directly from ROM then?
>>
>> Yes.
> 
> Is this the only runtime call into the firmware, 


Only when suspend-to-ram occurs, the kernel will call into the firmware.
No other case.

> or are there
> others that are either already called from mainline kernels
> or in your downsteam implementation?
> 
> How do you ensure that the DTB matches the actual ROM code
> after rebuilding Pmon? Does Pmon itself fill that field with
> the correct address, or do you rely on it being a hardcoded
> constant?


Use Pmon, firmware team will always ensure that DTB matches the actual
ROM code.  The "suspend-address" of dtb and pmon entry address will
synchronized modification by firmware team.

Thanks,
Yinbo

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Arnd Bergmann]

On Mon, Aug 14, 2023, at 13:57, Yinbo Zhu wrote:
> 在 2023/8/14 下午4:19, Arnd Bergmann 写道:
>> On Mon, Aug 14, 2023, at 09:57, Yinbo Zhu wrote:
>>> 在 2023/8/12 下午8:25, Arnd Bergmann 写道:
>>>> I found https://github.com/loongson-community/pmon source
>>>> code, and a reference to its origin at LSI Logic at
>>>> https://www.linux-mips.org/wiki/PMON but otherwise have
>>>> no idea about what this actually is, or how it relates
>>>> to your UEFI firmware. Did you add UEFI support to PMON,
>>>> or do you use it as a first stage loader that loads
>>>> the actual UEFI implementation (EDK2 or u-boot, I guess)?
>>>
>>>
>>> Pmon and uefi are two different firmware, and there is no connection
>>> between them.
>> 
>> It sounds like we still have problems with terminology. >
>> I don't think categorizing UEFI as a firmware is correct,
>
>
> Sorry to have confused you, uefi firmware is our internal name, which is
> actually what you referred to as EDK2, the EDK2 need use UEFI.

Ok

>> it's the interface used by various firmware implementations
>> to load the operating system. As far as I understand,
>> loongarch currently mandates the use of UEFI in whichever
>> firmware is used, so if you have Pmon installed in ROM > and Pmon does not itself implement UEFI, it would have
>> to load some other firmware such as u-boot in order to
>> load a kernel through the UEFI protocol, right?
>
>
> PMON is an independent firmware and loader that can directly load the
> operating system and it does not require the use of UEFI.
>> 
>> Has the assumption that loongarch requires UEFI changed?
>
>
> LoongArch embedded board was use Pmon firmware, The other one uses UEFI
> firmware (EDK2) on LoongArch platform.

I'm pretty sure we discussed this when the loongarch port
was originally merged, with the decisionto just use UEFI for
booting any kernel, as the legacy entry point for the ACPI
based environment was not really well-defined and the UEFI
stub was an easy alternative to have more commonality
with other architectures.

I see this was already extended for embedded CPUs to use
the uefi stub with DT in commit 88d4d957edc70 ("LoongArch: Add
FDT booting support from efi system table"), which seems like
the right direction.

Can you explain why this board would want yet another method
for entering the kernel? Is there any documentation for the
boot protocol?

>>>> Is this executing directly from ROM then?
>>>
>>> Yes.
>> 
>> Is this the only runtime call into the firmware, 
>
>
> Only when suspend-to-ram occurs, the kernel will call into the firmware.
> No other case.

Ok

>> or are there
>> others that are either already called from mainline kernels
>> or in your downsteam implementation?
>> 
>> How do you ensure that the DTB matches the actual ROM code
>> after rebuilding Pmon? Does Pmon itself fill that field with
>> the correct address, or do you rely on it being a hardcoded
>> constant?
>
>
> Use Pmon, firmware team will always ensure that DTB matches the actual
> ROM code.  The "suspend-address" of dtb and pmon entry address will
> synchronized modification by firmware team.

Ok.  So it's linked against libfdt to fill the dtb information,
or do you have to provide a dtb blob that matches the firmware?

      Arnd

Re: [PATCH v6 1/2] soc: dt-bindings: add loongson-2 pm

[Yinbo Zhu]

在 2023/8/14 下午9:41, Arnd Bergmann 写道:
> On Mon, Aug 14, 2023, at 13:57, Yinbo Zhu wrote:
>> 在 2023/8/14 下午4:19, Arnd Bergmann 写道:
>>> On Mon, Aug 14, 2023, at 09:57, Yinbo Zhu wrote:
>>>> 在 2023/8/12 下午8:25, Arnd Bergmann 写道:
>>>>> I found https://github.com/loongson-community/pmon source
>>>>> code, and a reference to its origin at LSI Logic at
>>>>> https://www.linux-mips.org/wiki/PMON but otherwise have
>>>>> no idea about what this actually is, or how it relates
>>>>> to your UEFI firmware. Did you add UEFI support to PMON,
>>>>> or do you use it as a first stage loader that loads
>>>>> the actual UEFI implementation (EDK2 or u-boot, I guess)?
>>>>
>>>>
>>>> Pmon and uefi are two different firmware, and there is no connection
>>>> between them.
>>>
>>> It sounds like we still have problems with terminology. >
>>> I don't think categorizing UEFI as a firmware is correct,
>>
>>
>> Sorry to have confused you, uefi firmware is our internal name, which is
>> actually what you referred to as EDK2, the EDK2 need use UEFI.
> 
> Ok
> 
>>> it's the interface used by various firmware implementations
>>> to load the operating system. As far as I understand,
>>> loongarch currently mandates the use of UEFI in whichever
>>> firmware is used, so if you have Pmon installed in ROM > and Pmon does not itself implement UEFI, it would have
>>> to load some other firmware such as u-boot in order to
>>> load a kernel through the UEFI protocol, right?
>>
>>
>> PMON is an independent firmware and loader that can directly load the
>> operating system and it does not require the use of UEFI.
>>>
>>> Has the assumption that loongarch requires UEFI changed?
>>
>>
>> LoongArch embedded board was use Pmon firmware, The other one uses UEFI
>> firmware (EDK2) on LoongArch platform.
> 
> I'm pretty sure we discussed this when the loongarch port
> was originally merged, with the decisionto just use UEFI for
> booting any kernel, as the legacy entry point for the ACPI
> based environment was not really well-defined and the UEFI
> stub was an easy alternative to have more commonality
> with other architectures.
> 
> I see this was already extended for embedded CPUs to use
> the uefi stub with DT in commit 88d4d957edc70 ("LoongArch: Add
> FDT booting support from efi system table"), which seems like
> the right direction. >
> Can you explain why this board would want yet another method
> for entering the kernel? Is there any documentation for the
> boot protocol?

Yes, you're right, the latest PMON does indeed support UEFI, the PMON
used in the product code is still outdated and does not support UEFI.

Actually, whether using EDK2, Pmon firmware, or other firmware, the
LoongArch platform's s3 (suspend-to-ram) requires a suspend-address to
be defined, if dts is supported, it is defined in dts, and if acpi table
is supported, it is defined in acpi table.

> 
>>>>> Is this executing directly from ROM then?
>>>>
>>>> Yes.
>>>
>>> Is this the only runtime call into the firmware,
>>
>>
>> Only when suspend-to-ram occurs, the kernel will call into the firmware.
>> No other case.
> 
> Ok
> 
>>> or are there
>>> others that are either already called from mainline kernels
>>> or in your downsteam implementation?
>>>
>>> How do you ensure that the DTB matches the actual ROM code
>>> after rebuilding Pmon? Does Pmon itself fill that field with
>>> the correct address, or do you rely on it being a hardcoded
>>> constant?
>>
>>
>> Use Pmon, firmware team will always ensure that DTB matches the actual
>> ROM code.  The "suspend-address" of dtb and pmon entry address will
>> synchronized modification by firmware team.
> 
> Ok.  So it's linked against libfdt to fill the dtb information,
> or do you have to provide a dtb blob that matches the firmware?


For pmon firmware, the dtb was as part of firmware, the firmware has
defined the address of the DTB's suspend and the address of the firmware
entry, and is consistent.

Thanks,
Yinbo

Re: [PATCH v6 0/2] soc: loongson2_pm: add power management support

[Arnd Bergmann]

On Thu, Aug 3, 2023, at 02:37, Yinbo Zhu wrote:
> Loongson-2 platform support Power Management Controller (ACPI) and this
> series patch was to add PM driver that base on dts and PM binding support.
>
> Change in v6:
> 		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
> 		   pm interfaces" had been merged into mainline tree in v6.5-rc1
> 		   thus this v6 series patch need drop it and need depend on it
> 		   and it's patch link was:
> https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
> 		2. Adding Ulf Hansson to Cc.
> 		3. Adding soc@kernel.org to Cc.
> 		4. Keep indented with one tab +2 spaces in Kconfig help text.

I talked to WANG Xuerui on IRC, and he was able to clarify some of the
missing bits of information for me, after which I merged both patches,
even though my concerns are not fully addressed:

- I still think that branching into ROM code from the kernel is a mistake
  and we should have never allowed that as an ad-hoc interface in the ACPI
  variant to start with. It's hard to change that now though, and having
  a DT interface to access the same entry point does not really make it
  worse. This might need a redesign for future firmware though, to have
  a proper runtime interface

- The bigger problem I still see is the DT-enabled boot with PMon without
  the UEFI firmware. This does not impact the DT binding, but I would
  consider all non-UEFI booting firmware images broken and not supported
  by the kernel, as we originally discussed when merging the kernel.
  These should still be fixable by upgrading PMon to a UEFI-enabled
  version.

     Arnd

Re: [PATCH v6 0/2] soc: loongson2_pm: add power management support

[patchwork-bot+linux-soc]

Hello:

This series was applied to soc/soc.git (for-next)
by Arnd Bergmann <arnd@arndb.de>:

On Thu,  3 Aug 2023 14:37:01 +0800 you wrote:
> Loongson-2 platform support Power Management Controller (ACPI) and this
> series patch was to add PM driver that base on dts and PM binding support.
> 
> Change in v6:
> 		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
> 		   pm interfaces" had been merged into mainline tree in v6.5-rc1
> 		   thus this v6 series patch need drop it and need depend on it
> 		   and it's patch link was:
> https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
> 		2. Adding Ulf Hansson to Cc.
> 		3. Adding soc@kernel.org to Cc.
> 		4. Keep indented with one tab +2 spaces in Kconfig help text.
> Change in v5:
> 		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
> 		   pm interfaces" had been merged into linux-next tree thus this
> 		   v4 series patch need drop it and need depend on it and it's
> 		   patch link was:
> https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
> 		2. Swap the positions of compatible for 2k1000 and 2k0500.
> Change in v4:
> 		1. The patch "[PATCH v3 1/3] loongarch: export some arch-specific
> 		   pm interfaces" had been merged into linux-next tree thus this
> 		   v4 series patch need drop it and need depend on it and it's
> 		   patch link was:
> https://lore.kernel.org/all/20230615091757.24686-2-zhuyinbo@loongson.cn/
> 		2. Remove the pmc label in dt-binding patch.
> 		3. Add the Co-developed-by for driver patch.
> 		4. Simplify the loongson2_suspend_valid_state that "return
> 		   (state == PM_SUSPEND_MEM)".
> 		5. Use Using loongson2_pm_irq_enable() to replace.
> 		   loongson2_power_button_irq_enable().
> 		6. Remove the "oneOf" in dt-bindings patch.
> 		7. Replace "suspend-address" that use "loongson,suspend-address".
> 		8. Use u64 type that for "loongson,suspend-address".
> 		9. Rename "pm" to "power-mangement" in dt-bindings patch.
> 		10. Add the reivewed-by for dt-bindings patch.
> Change in v3:
> 		1. Reword the [1/3] patch commit log and title.
> 		2. Use the old naming for suspend interface for the [1/3] and
> 		   [3/3] patch.
> 		3. Combine some small function in the driver patch.
> 		4. Rename 'pwrbt' to 'button' in the driver patch.
> 		5. Use the specific compatible in yaml file.
> Change in v2:
> 		1. Fixup the "suspend-address" description.
> 		2. Remove the "return -EINVAL" in PM driver probe when firmware
> 		   no configure "suspend-address" property in dts in oder to
> 		   other PM state to work.
> 
> [...]

Here is the summary with links:
  - [v6,1/2] soc: dt-bindings: add loongson-2 pm
    https://git.kernel.org/soc/soc/c/6054a676e969
  - [v6,2/2] soc: loongson2_pm: add power management support
    https://git.kernel.org/soc/soc/c/67694c076bd7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html