diff mbox series

[2/3] dt-bindings: qcom,pon: Add 'qcom,mode-in-imem' support

Message ID 20210704074045.21643-3-shawn.guo@linaro.org
State New
Headers show
Series Add 'qcom,mode-in-imem' support in PON driver | expand

Commit Message

Shawn Guo July 4, 2021, 7:40 a.m. UTC 
It's not always the case that reboot mode value gets stored in PON
register.  For example, Sony Xperia M4 Aqua phone (MSM8939) uses a
different set of mode value and stores them in IMEM.  Add property
'qcom,mode-in-imem' to distinguish this mechanism from the existing one.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

---
 .../bindings/power/reset/qcom,pon.yaml        | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.17.1

Comments

Stephan Gerhold July 4, 2021, 9:36 a.m. UTC | #1 
On Sun, Jul 04, 2021 at 03:40:44PM +0800, Shawn Guo wrote:
> It's not always the case that reboot mode value gets stored in PON

> register.  For example, Sony Xperia M4 Aqua phone (MSM8939) uses a

> different set of mode value and stores them in IMEM.  Add property

> 'qcom,mode-in-imem' to distinguish this mechanism from the existing one.

> 

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> ---

>  .../bindings/power/reset/qcom,pon.yaml        | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

> 

> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> index 7764c804af1d..a6270e39b7a2 100644

> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> @@ -23,6 +23,10 @@ properties:

>    reg:

>      maxItems: 1

>  

> +  qcom,mode-in-imem:

> +    description: Reboot mode is stored in IMEM rather than PON register

> +    type: boolean

> +

>  patternProperties:

>    "^mode-.+":

>      $ref: /schemas/types.yaml#/definitions/uint32

> @@ -35,6 +39,7 @@ required:

>  additionalProperties: false

>  

>  examples:

> +  # Example 1: Reboot mode is stored in PON register

>    - |

>      pmic {

>          #address-cells = <1>;

> @@ -47,3 +52,17 @@ examples:

>              mode-recovery = <0x1>;

>          };

>      };

> +  # Example 2: Reboot mode is stored in IMEM

> +  - |

> +    pmic {

> +        #address-cells = <1>;

> +        #size-cells = <0>;

> +

> +        pon@800 {

> +            compatible = "qcom,pm8916-pon";

> +            reg = <0x860065c>;


This is quite strange. pon@800 is a node of the PM8916 PMIC,
so the reg should refer to the address inside the PMIC, not some memory
address of the SoC. dtc will probably warn about this too since the unit
address (@800) should match the first reg. (At least on W=1.)

Actually we already have some devices using IMEM for the reboot mode,
like this (qcom-msm8974.dtsi plus qcom-msm8974-fairphone-fp2.dts):

	imem@fe805000 {
		compatible = "syscon", "simple-mfd";
		reg = <0xfe805000 0x1000>;

		reboot-mode {
			compatible = "syscon-reboot-mode";
			offset = <0x65c>;
			mode-normal	= <0x77665501>;
			mode-bootloader	= <0x77665500>;
			mode-recovery	= <0x77665502>;
		};
	};

Perhaps it would be cleaner to add a property to disable the reboot mode
functionality of pm8916-pon and then set it up like this?

Thanks!
Stephan
Shawn Guo July 5, 2021, 2:41 a.m. UTC | #2 
On Sun, Jul 04, 2021 at 11:36:41AM +0200, Stephan Gerhold wrote:
> On Sun, Jul 04, 2021 at 03:40:44PM +0800, Shawn Guo wrote:

> > It's not always the case that reboot mode value gets stored in PON

> > register.  For example, Sony Xperia M4 Aqua phone (MSM8939) uses a

> > different set of mode value and stores them in IMEM.  Add property

> > 'qcom,mode-in-imem' to distinguish this mechanism from the existing one.

> > 

> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> > ---

> >  .../bindings/power/reset/qcom,pon.yaml        | 19 +++++++++++++++++++

> >  1 file changed, 19 insertions(+)

> > 

> > diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> > index 7764c804af1d..a6270e39b7a2 100644

> > --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> > +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml

> > @@ -23,6 +23,10 @@ properties:

> >    reg:

> >      maxItems: 1

> >  

> > +  qcom,mode-in-imem:

> > +    description: Reboot mode is stored in IMEM rather than PON register

> > +    type: boolean

> > +

> >  patternProperties:

> >    "^mode-.+":

> >      $ref: /schemas/types.yaml#/definitions/uint32

> > @@ -35,6 +39,7 @@ required:

> >  additionalProperties: false

> >  

> >  examples:

> > +  # Example 1: Reboot mode is stored in PON register

> >    - |

> >      pmic {

> >          #address-cells = <1>;

> > @@ -47,3 +52,17 @@ examples:

> >              mode-recovery = <0x1>;

> >          };

> >      };

> > +  # Example 2: Reboot mode is stored in IMEM

> > +  - |

> > +    pmic {

> > +        #address-cells = <1>;

> > +        #size-cells = <0>;

> > +

> > +        pon@800 {

> > +            compatible = "qcom,pm8916-pon";

> > +            reg = <0x860065c>;

> 

> This is quite strange. pon@800 is a node of the PM8916 PMIC,

> so the reg should refer to the address inside the PMIC, not some memory

> address of the SoC. dtc will probably warn about this too since the unit

> address (@800) should match the first reg. (At least on W=1.)


Fair point.

> Actually we already have some devices using IMEM for the reboot mode,

> like this (qcom-msm8974.dtsi plus qcom-msm8974-fairphone-fp2.dts):

> 

> 	imem@fe805000 {

> 		compatible = "syscon", "simple-mfd";

> 		reg = <0xfe805000 0x1000>;

> 

> 		reboot-mode {

> 			compatible = "syscon-reboot-mode";

> 			offset = <0x65c>;

> 			mode-normal	= <0x77665501>;

> 			mode-bootloader	= <0x77665500>;

> 			mode-recovery	= <0x77665502>;

> 		};

> 	};

> 

> Perhaps it would be cleaner to add a property to disable the reboot mode

> functionality of pm8916-pon and then set it up like this?


Ah, yes, this is sensible.  Thanks for the msm8974 example!

Shawn
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
index 7764c804af1d..a6270e39b7a2 100644
--- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
+++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
@@ -23,6 +23,10 @@  properties:
   reg:
     maxItems: 1
 
+  qcom,mode-in-imem:
+    description: Reboot mode is stored in IMEM rather than PON register
+    type: boolean
+
 patternProperties:
   "^mode-.+":
     $ref: /schemas/types.yaml#/definitions/uint32
@@ -35,6 +39,7 @@  required:
 additionalProperties: false
 
 examples:
+  # Example 1: Reboot mode is stored in PON register
   - |
     pmic {
         #address-cells = <1>;
@@ -47,3 +52,17 @@  examples:
             mode-recovery = <0x1>;
         };
     };
+  # Example 2: Reboot mode is stored in IMEM
+  - |
+    pmic {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pon@800 {
+            compatible = "qcom,pm8916-pon";
+            reg = <0x860065c>;
+            qcom,mode-in-imem;
+            mode-bootloader = <0x77665500>;
+            mode-recovery = <0x77665502>;
+        };
+    };