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

[Shawn Guo <shawn.guo@linaro.org>]

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