[PATCH v2 0/5] soc: loongson: Fix some issues about loongson_pm2

[PATCH v2 0/5] soc: loongson: Fix some issues about loongson_pm2

[Binbin Zhou]

Hi all:

Since commit 67694c076bd7 ("soc: loongson2_pm: add power management support"),
the Loongson-2K PM driver was added, some issues have been found and
this patchset is planned to fix these issues.

Specific:
Patch 1: Compilation error found by Randy;
Patch 2/3: Rewriting the ls2k-pmc compatible attribute with fallback
compatible;
Patch 4/5: Add Loongson-2K SoC reboot/shutdown support as part of power
management.

Thanks.

-----
V2:
patch 1:
 - Change "depends on INPUT" to "depends on INPUT=y", because the
   compilation error still exists when "INPUT=m".
patch 2:
  - Use ls2k0500 as the fallback compatible;
  - Add ls2k2000 compatible.
patch 3:
 - Drop ls2k1000 compatible, for ls2k0500 is used as the fallback
   compatible.
patch 4:
  - Drop label;
  - Drop regmap property in syscon-reboot, for it's deprecated;
  - Add missing space.

Link to V1:
https://lore.kernel.org/all/cover.1693218539.git.zhoubinbin@loongson.cn/

Binbin Zhou (5):
  soc: loongson: loongson_pm2: Add dependency for INPUT
  dt-bindings: soc: loongson,ls2k-pmc: Use fallbacks for ls2k-pmc
    compatible
  soc: loongson: loongson_pm2: Drop useless of_device_id compatible
  dt-bindings: soc: loongson,ls2k-pmc: Allow
    syscon-reboot/syscon-poweroff as child
  soc: loongson: loongson_pm2: Populate children syscon nodes

 .../soc/loongson/loongson,ls2k-pmc.yaml       | 43 ++++++++++++++++---
 drivers/soc/loongson/Kconfig                  |  1 +
 drivers/soc/loongson/loongson2_pm.c           |  7 ++-
 3 files changed, 44 insertions(+), 7 deletions(-)

-- 
2.39.3

[PATCH v2 1/5] soc: loongson: loongson_pm2: Add dependency for INPUT

[Binbin Zhou]

Since commit 67694c076bd7 ("soc: loongson2_pm: add power management
support"), the Loongson-2K PM driver was added, but it didn't update the
Kconfig entry for the INPUT dependency, leading to build errors, so
update the Kconfig entry to depend on INPUT.

/opt/crosstool/gcc-13.2.0-nolibc/loongarch64-linux/bin/loongarch64-linux-ld:
drivers/soc/loongson/loongson2_pm.o: in function `loongson2_power_button_init':
/work/lnx/next/linux-next-20230825/LOONG64/../drivers/soc/loongson/loongson2_pm.c:101:(.text+0x350): undefined reference to `input_allocate_device'
/opt/crosstool/gcc-13.2.0-nolibc/loongarch64-linux/bin/loongarch64-linux-ld:
/work/lnx/next/linux-next-20230825/LOONG64/../drivers/soc/loongson/loongson2_pm.c:109:(.text+0x3dc): undefined reference to `input_set_capability'
/opt/crosstool/gcc-13.2.0-nolibc/loongarch64-linux/bin/loongarch64-linux-ld:
/work/lnx/next/linux-next-20230825/LOONG64/../drivers/soc/loongson/loongson2_pm.c:111:(.text+0x3e4): undefined reference to `input_register_device'
/opt/crosstool/gcc-13.2.0-nolibc/loongarch64-linux/bin/loongarch64-linux-ld:
/work/lnx/next/linux-next-20230825/LOONG64/../drivers/soc/loongson/loongson2_pm.c:125:(.text+0x3fc): undefined reference to `input_free_device'
/opt/crosstool/gcc-13.2.0-nolibc/loongarch64-linux/bin/loongarch64-linux-ld: drivers/soc/loongson/loongson2_pm.o: in function `input_report_key':
/work/lnx/next/linux-next-20230825/LOONG64/../include/linux/input.h:425:(.text+0x58c): undefined reference to `input_event'

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/soc/loongson/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/loongson/Kconfig b/drivers/soc/loongson/Kconfig
index 314e13bb3e01..368344943a93 100644
--- a/drivers/soc/loongson/Kconfig
+++ b/drivers/soc/loongson/Kconfig
@@ -20,6 +20,7 @@ config LOONGSON2_GUTS
 config LOONGSON2_PM
 	bool "Loongson-2 SoC Power Management Controller Driver"
 	depends on LOONGARCH && OF
+	depends on INPUT=y
 	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
-- 
2.39.3

[PATCH v2 2/5] dt-bindings: soc: loongson,ls2k-pmc: Use fallbacks for ls2k-pmc compatible

[Binbin Zhou]

The Loongson-2K series chips (ls2k0500/ls2k1000/ls2k2000) share the same
PM system controller, using ls2k0500 compatible as fallback for the
others.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../soc/loongson/loongson,ls2k-pmc.yaml         | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
index da2dcfeebf12..c45f5e7fc0e6 100644
--- a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
+++ b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
@@ -11,11 +11,16 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - loongson,ls2k0500-pmc
-          - loongson,ls2k1000-pmc
-      - const: syscon
+    oneOf:
+      - items:
+          - const: loongson,ls2k0500-pmc
+          - const: syscon
+      - items:
+          - enum:
+              - loongson,ls2k1000-pmc
+              - loongson,ls2k2000-pmc
+          - const: loongson,ls2k0500-pmc
+          - const: syscon
 
   reg:
     maxItems: 1
@@ -44,7 +49,7 @@ examples:
     #include <dt-bindings/interrupt-controller/irq.h>
 
     power-management@1fe27000 {
-        compatible = "loongson,ls2k1000-pmc", "syscon";
+        compatible = "loongson,ls2k1000-pmc", "loongson,ls2k0500-pmc", "syscon";
         reg = <0x1fe27000 0x58>;
         interrupt-parent = <&liointc1>;
         interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
-- 
2.39.3

[PATCH v2 3/5] soc: loongson: loongson_pm2: Drop useless of_device_id compatible

[Binbin Zhou]

Now, "loongson,ls2k0500-pmc" is used as fallback compatible, so the
ls2k1000 compatible in the driver can be dropped directly.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/soc/loongson/loongson2_pm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soc/loongson/loongson2_pm.c b/drivers/soc/loongson/loongson2_pm.c
index 796add6e8b63..5ffb77afd9eb 100644
--- a/drivers/soc/loongson/loongson2_pm.c
+++ b/drivers/soc/loongson/loongson2_pm.c
@@ -197,7 +197,6 @@ static int loongson2_pm_probe(struct platform_device *pdev)
 
 static const struct of_device_id loongson2_pm_match[] = {
 	{ .compatible = "loongson,ls2k0500-pmc", },
-	{ .compatible = "loongson,ls2k1000-pmc", },
 	{},
 };
 
-- 
2.39.3

Re: [PATCH v2 3/5] soc: loongson: loongson_pm2: Drop useless of_device_id compatible

[Binbin Zhou]

Hi Conor:

Thanks for your reply.

On Thu, Aug 31, 2023 at 11:15 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Aug 31, 2023 at 07:43:13PM +0800, Binbin Zhou wrote:
> > Now, "loongson,ls2k0500-pmc" is used as fallback compatible, so the
> > ls2k1000 compatible in the driver can be dropped directly.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  drivers/soc/loongson/loongson2_pm.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/soc/loongson/loongson2_pm.c b/drivers/soc/loongson/loongson2_pm.c
> > index 796add6e8b63..5ffb77afd9eb 100644
> > --- a/drivers/soc/loongson/loongson2_pm.c
> > +++ b/drivers/soc/loongson/loongson2_pm.c
> > @@ -197,7 +197,6 @@ static int loongson2_pm_probe(struct platform_device *pdev)
> >
> >  static const struct of_device_id loongson2_pm_match[] = {
> >       { .compatible = "loongson,ls2k0500-pmc", },
> > -     { .compatible = "loongson,ls2k1000-pmc", },
>
> What about older devicetrees that do not have the fallback? Aren't you
> going to remove support for those?

This driver was just merged in during the v6.6 window, and no release
will use it until now.

commit 67694c076bd7 soc: loongson2_pm: add power management support
commit 6054a676e969 soc: dt-bindings: add loongson-2 pm

Of course, it was our mistake not to make better use of fallback
compatible when submitting the driver, and we hope to fix this mistake
soon.
Also, this driver will only be used by the LoongArch architecture, no
need to worry about other architectures.

Thanks.
Binbin
>
> >       {},
> >  };
> >
> > --
> > 2.39.3
> >

Re: [PATCH v2 3/5] soc: loongson: loongson_pm2: Drop useless of_device_id compatible

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Fri, Sep 01, 2023 at 11:05:12AM +0800, Binbin Zhou wrote:
> Hi Conor:
> 
> Thanks for your reply.
> 
> On Thu, Aug 31, 2023 at 11:15 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Aug 31, 2023 at 07:43:13PM +0800, Binbin Zhou wrote:
> > > Now, "loongson,ls2k0500-pmc" is used as fallback compatible, so the
> > > ls2k1000 compatible in the driver can be dropped directly.
> > >
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---
> > >  drivers/soc/loongson/loongson2_pm.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/soc/loongson/loongson2_pm.c b/drivers/soc/loongson/loongson2_pm.c
> > > index 796add6e8b63..5ffb77afd9eb 100644
> > > --- a/drivers/soc/loongson/loongson2_pm.c
> > > +++ b/drivers/soc/loongson/loongson2_pm.c
> > > @@ -197,7 +197,6 @@ static int loongson2_pm_probe(struct platform_device *pdev)
> > >
> > >  static const struct of_device_id loongson2_pm_match[] = {
> > >       { .compatible = "loongson,ls2k0500-pmc", },
> > > -     { .compatible = "loongson,ls2k1000-pmc", },
> >
> > What about older devicetrees that do not have the fallback? Aren't you
> > going to remove support for those?
> 
> This driver was just merged in during the v6.6 window, and no release
> will use it until now.

Ahh, thanks for the info. Your commits 1-3 should then all have Fixes:
tags pointing to the commits tat introduced the problems, if you intend
getting these applied as fixes before v6.6 gets released.

Thanks,
Conor.

> commit 67694c076bd7 soc: loongson2_pm: add power management support
> commit 6054a676e969 soc: dt-bindings: add loongson-2 pm
> 
> Of course, it was our mistake not to make better use of fallback
> compatible when submitting the driver, and we hope to fix this mistake
> soon.
> Also, this driver will only be used by the LoongArch architecture, no
> need to worry about other architectures.
> 
> Thanks.
> Binbin
> >
> > >       {},
> > >  };
> > >
> > > --
> > > 2.39.3
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 4/5] dt-bindings: soc: loongson,ls2k-pmc: Allow syscon-reboot/syscon-poweroff as child

[Binbin Zhou]

The reboot and poweroff features are actually part of the Power
Management Unit system controller, thus allow them as its children,
instead of specifying as separate device nodes with syscon phandle.

Without it, the reboot/poweroff feature becomes unavailable.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../soc/loongson/loongson,ls2k-pmc.yaml       | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
index c45f5e7fc0e6..510f6cb0f084 100644
--- a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
+++ b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-pmc.yaml
@@ -37,6 +37,18 @@ properties:
       addition, the PM need according to it to indicate that current
       SoC whether support Suspend To RAM.
 
+  syscon-poweroff:
+    $ref: /schemas/power/reset/syscon-poweroff.yaml#
+    type: object
+    description:
+      Node for power off method
+
+  syscon-reboot:
+    $ref: /schemas/power/reset/syscon-reboot.yaml#
+    type: object
+    description:
+      Node for reboot method
+
 required:
   - compatible
   - reg
@@ -54,4 +66,18 @@ examples:
         interrupt-parent = <&liointc1>;
         interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
         loongson,suspend-address = <0x0 0x1c000500>;
+
+        syscon-reboot {
+            compatible = "syscon-reboot";
+            offset = <0x30>;
+            mask = <0x1>;
+        };
+
+        syscon-poweroff {
+            compatible = "syscon-poweroff";
+            regmap = <&pmc>;
+            offset = <0x14>;
+            mask = <0x3c00>;
+            value = <0x3c00>;
+        };
     };
-- 
2.39.3

Re: [PATCH v2 4/5] dt-bindings: soc: loongson,ls2k-pmc: Allow syscon-reboot/syscon-poweroff as child

[Krzysztof Kozlowski]

On 31/08/2023 13:43, Binbin Zhou wrote:
>  required:
>    - compatible
>    - reg
> @@ -54,4 +66,18 @@ examples:
>          interrupt-parent = <&liointc1>;
>          interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>          loongson,suspend-address = <0x0 0x1c000500>;
> +
> +        syscon-reboot {
> +            compatible = "syscon-reboot";
> +            offset = <0x30>;
> +            mask = <0x1>;
> +        };
> +
> +        syscon-poweroff {
> +            compatible = "syscon-poweroff";
> +            regmap = <&pmc>;

???

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Re: [PATCH v2 4/5] dt-bindings: soc: loongson,ls2k-pmc: Allow syscon-reboot/syscon-poweroff as child

[Binbin Zhou]

Hi Krzysztof:


On Fri, Sep 1, 2023 at 5:06 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 31/08/2023 13:43, Binbin Zhou wrote:
> >  required:
> >    - compatible
> >    - reg
> > @@ -54,4 +66,18 @@ examples:
> >          interrupt-parent = <&liointc1>;
> >          interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> >          loongson,suspend-address = <0x0 0x1c000500>;
> > +
> > +        syscon-reboot {
> > +            compatible = "syscon-reboot";
> > +            offset = <0x30>;
> > +            mask = <0x1>;
> > +        };
> > +
> > +        syscon-poweroff {
> > +            compatible = "syscon-poweroff";
> > +            regmap = <&pmc>;
>
> ???

I did notice that commit [1] changed "regmap" to "unrequired" for
"syscon-reboot", but "syscon-poweroff" did not do the same.
So, at least under the current "syscon-poweroff" rule, "regmap" is "required".

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml?h=v6.5#n41

I had my doubts before, but seeing that some dts do have
"syscon-poweroff" as a separate node, I assumed there was a
difference.

commit[1]: 2140d68d69d4 dt-bindings: power: reset: Unrequired regmap
property in syscon-reboot node

Thanks.
Binbin
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 4/5] dt-bindings: soc: loongson,ls2k-pmc: Allow syscon-reboot/syscon-poweroff as child

[Krzysztof Kozlowski]

On 01/09/2023 11:27, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> 
> On Fri, Sep 1, 2023 at 5:06 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 31/08/2023 13:43, Binbin Zhou wrote:
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -54,4 +66,18 @@ examples:
>>>          interrupt-parent = <&liointc1>;
>>>          interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>>>          loongson,suspend-address = <0x0 0x1c000500>;
>>> +
>>> +        syscon-reboot {
>>> +            compatible = "syscon-reboot";
>>> +            offset = <0x30>;
>>> +            mask = <0x1>;
>>> +        };
>>> +
>>> +        syscon-poweroff {
>>> +            compatible = "syscon-poweroff";
>>> +            regmap = <&pmc>;
>>
>> ???
> 
> I did notice that commit [1] changed "regmap" to "unrequired" for
> "syscon-reboot", but "syscon-poweroff" did not do the same.
> So, at least under the current "syscon-poweroff" rule, "regmap" is "required".
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/reset/syscon-poweroff.yaml?h=v6.5#n41
> 
> I had my doubts before, but seeing that some dts do have
> "syscon-poweroff" as a separate node, I assumed there was a
> difference.
> 
> commit[1]: 2140d68d69d4 dt-bindings: power: reset: Unrequired regmap
> property in syscon-reboot node
> 

You are right. I wonder why Serge did not change others.

Looks good, thanks for clarification.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 5/5] soc: loongson: loongson_pm2: Populate children syscon nodes

[Binbin Zhou]

The syscon poweroff and reboot nodes logically belong to the Power
Management Unit so populate possible children.

Without it, the reboot/poweroff feature becomes unavailable.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/soc/loongson/loongson2_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/loongson/loongson2_pm.c b/drivers/soc/loongson/loongson2_pm.c
index 5ffb77afd9eb..b8e5e1e3528a 100644
--- a/drivers/soc/loongson/loongson2_pm.c
+++ b/drivers/soc/loongson/loongson2_pm.c
@@ -11,6 +11,7 @@
 #include <linux/input.h>
 #include <linux/suspend.h>
 #include <linux/interrupt.h>
+#include <linux/of_platform.h>
 #include <linux/pm_wakeirq.h>
 #include <linux/platform_device.h>
 #include <asm/bootinfo.h>
@@ -192,6 +193,11 @@ static int loongson2_pm_probe(struct platform_device *pdev)
 	if (loongson_sysconf.suspend_addr)
 		suspend_set_ops(&loongson2_suspend_ops);
 
+	/* Populate children */
+	retval = devm_of_platform_populate(dev);
+	if (retval)
+		dev_err(dev, "Error populating children, reboot and poweroff might not work properly\n");
+
 	return 0;
 }
 
-- 
2.39.3