[PATCH v4 0/2] add support for PON GEN3 device

[PATCH v4 0/2] add support for PON GEN3 device

[Anjelique Melendez]

Changes from v3:
  - Addressed Krysztof's comments on patch 1/2
    - Added missing minItems/maxItems
    - Merged if statments with same constraints together
    - Removed description from "reg-names"
    
Changes from v2:
  - Added new "qcom,pmk8350-pon" compatible string as per Krysztof's
    advice
  - Updated dt logic to use comptaible strings to decide constraints
    as per Krysztof's comment
  - Added new patch (v3 2/2) to support new compatible string

Changes from v1:
  - Updated path which was missing Documentation/devicetree prefix
  - Updated CC list

New patch series to seperate this patch from applied patches.
Comments from original patch can be found
https://lore.kernel.org/linux-arm-msm/27515993-18f3-8891-4835-9b6a8d7f86b0@quicinc.com/

David Collins (1):
  dt-bindings: power: reset: qcom-pon: update "reg" property details

Anjelique Melendez (1):
  power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

 Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
 drivers/power/reset/qcom-pon.c | 1 +
 2 file changed, 47 insertions(+), 4 deletions(-)

-- 
2.35.1

[PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Anjelique Melendez]

From: David Collins <quic_collinsd@quicinc.com>

Update the description of "reg" property to add the PON_PBS base
address along with PON_HLOS base address.  Also add "reg-names"
property constraints.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
index 353f155d..d7b6b875 100644
--- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
+++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
@@ -15,18 +15,27 @@ description: |
 
   This DT node has pwrkey and resin as sub nodes.
 
-allOf:
-  - $ref: reboot-mode.yaml#
-
 properties:
   compatible:
     enum:
       - qcom,pm8916-pon
       - qcom,pms405-pon
       - qcom,pm8998-pon
+      - qcom,pmk8350-pon
 
   reg:
-    maxItems: 1
+    description: |
+      Specifies the SPMI base address for the PON (power-on) peripheral.  For
+      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
+      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
+      peripherals.  In that case, the PON_PBS address needs to be specified to
+      facilitate software debouncing on some PMIC.
+    minItems: 1
+    maxItems: 2
+
+  reg-names:
+    minItems: 1
+    maxItems: 2
 
   pwrkey:
     type: object
@@ -42,6 +51,39 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: reboot-mode.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pm8916-pon
+              - qcom,pms405-pon
+              - qcom,pm8998-pon
+    then:
+      properties:
+        reg:
+          maxItems: 1
+        reg-names:
+          items:
+            - const: pon
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,pmk8350-pon
+    then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 2
+        reg-names:
+          minItems: 1
+          items:
+            - const: hlos
+            - const: pbs
+
 examples:
   - |
    #include <dt-bindings/interrupt-controller/irq.h>
-- 
2.35.1

[PATCH v4 2/2] power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

[Anjelique Melendez]

Add support for the new "qcom,pmk8350-pon" comptaible string.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 drivers/power/reset/qcom-pon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
index 4a688741a88a..16bc01738be9 100644
--- a/drivers/power/reset/qcom-pon.c
+++ b/drivers/power/reset/qcom-pon.c
@@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
 	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
 	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
 	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
+	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
-- 
2.35.1

Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Krzysztof Kozlowski]

On 25/07/2022 21:13, Anjelique Melendez wrote:
> From: David Collins <quic_collinsd@quicinc.com>
> 
> Update the description of "reg" property to add the PON_PBS base
> address along with PON_HLOS base address.  Also add "reg-names"
> property constraints.
> 
> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
> index 353f155d..d7b6b875 100644
> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
> @@ -15,18 +15,27 @@ description: |
>  
>    This DT node has pwrkey and resin as sub nodes.
>  
> -allOf:
> -  - $ref: reboot-mode.yaml#
> -
>  properties:
>    compatible:
>      enum:
>        - qcom,pm8916-pon
>        - qcom,pms405-pon
>        - qcom,pm8998-pon
> +      - qcom,pmk8350-pon
>  
>    reg:
> -    maxItems: 1
> +    description: |
> +      Specifies the SPMI base address for the PON (power-on) peripheral.  For
> +      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
> +      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
> +      peripherals.  In that case, the PON_PBS address needs to be specified to
> +      facilitate software debouncing on some PMIC.
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 2
>  
>    pwrkey:
>      type: object
> @@ -42,6 +51,39 @@ required:
>  
>  unevaluatedProperties: false
>  
> +allOf:
> +  - $ref: reboot-mode.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pm8916-pon
> +              - qcom,pms405-pon
> +              - qcom,pm8998-pon
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +        reg-names:
> +          items:
> +            - const: pon

All your previous patches were actually missing (in commit msg, in the
code) that piece of information which you add here. You now add
reg-names with "pon" for older devices. I assumed previous that it is
somehow needed, so I gave you the hints how it should be coded. But I
don't understand - why are you doing it?

This should be explained in commit msg. To me it is not needed at all...
unless you want to mark that first address space is entirely different
for other devices?

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,pmk8350-pon
> +    then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 2
> +        reg-names:
> +          minItems: 1
> +          items:
> +            - const: hlos
> +            - const: pbs
> +
>  examples:
>    - |
>     #include <dt-bindings/interrupt-controller/irq.h>


Best regards,
Krzysztof

Re: [PATCH v4 2/2] power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

[Krzysztof Kozlowski]

On 25/07/2022 21:13, Anjelique Melendez wrote:
> Add support for the new "qcom,pmk8350-pon" comptaible string.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  drivers/power/reset/qcom-pon.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
> index 4a688741a88a..16bc01738be9 100644
> --- a/drivers/power/reset/qcom-pon.c
> +++ b/drivers/power/reset/qcom-pon.c
> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>  	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>  	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>  	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
> +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);

This is now confusing. The new device has entirely different first and
second IO address spaces, but you do not code here any differences.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Anjelique Melendez]

Hi Krzysztof,
First I would like to apologize for my lack of response to this patch series
over these past few weeks. I have been out of office.

On 7/26/2022 3:25 AM, Krzysztof Kozlowski wrote:
> On 25/07/2022 21:13, Anjelique Melendez wrote:
>> From: David Collins <quic_collinsd@quicinc.com>
>>
>> Update the description of "reg" property to add the PON_PBS base
>> address along with PON_HLOS base address.  Also add "reg-names"
>> property constraints.
>>
>> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
>>  1 file changed, 46 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>> index 353f155d..d7b6b875 100644
>> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>> @@ -15,18 +15,27 @@ description: |
>>  
>>    This DT node has pwrkey and resin as sub nodes.
>>  
>> -allOf:
>> -  - $ref: reboot-mode.yaml#
>> -
>>  properties:
>>    compatible:
>>      enum:
>>        - qcom,pm8916-pon
>>        - qcom,pms405-pon
>>        - qcom,pm8998-pon
>> +      - qcom,pmk8350-pon
>>  
>>    reg:
>> -    maxItems: 1
>> +    description: |
>> +      Specifies the SPMI base address for the PON (power-on) peripheral.  For
>> +      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
>> +      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
>> +      peripherals.  In that case, the PON_PBS address needs to be specified to
>> +      facilitate software debouncing on some PMIC.
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>    pwrkey:
>>      type: object
>> @@ -42,6 +51,39 @@ required:
>>  
>>  unevaluatedProperties: false
>>  
>> +allOf:
>> +  - $ref: reboot-mode.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pm8916-pon
>> +              - qcom,pms405-pon
>> +              - qcom,pm8998-pon
>> +    then:
>> +      properties:
>> +        reg:
>> +          maxItems: 1
>> +        reg-names:
>> +          items:
>> +            - const: pon
> 
> All your previous patches were actually missing (in commit msg, in the
> code) that piece of information which you add here. You now add
> reg-names with "pon" for older devices. I assumed previous that it is
> somehow needed, so I gave you the hints how it should be coded. But I
> don't understand - why are you doing it
> 
> This should be explained in commit msg. To me it is not needed at all...
> unless you want to mark that first address space is entirely different
> for other devices?
Adding reg-names "pon" for older devices is simply to provide clarification
about what the register relates to. Similar to reg-names "hlos" and "pbs"
for gen3 children devices, reg-names is completely optional and is not
consumed by any driver.

Before adding the "qcom,pmk8350-pon" compatible string, the best way to
differentiate between a "qcom,pm8998-pon" parent with a gen1/gen2 child vs a
"qcom,pm8998-pon" parent with a gen3 child with only an hlos register was to
include reg-names. i.e.

pon_hlos@1300 {
        reg = <0x1300>;
        compatible = "qcom,pm8998-pon";
	reg-names = "pon";
};

vs 

pon_hlos@1300 {
        reg = <0x1300>;
        compatible = "qcom,pm8998-pon";
	reg-names = "hlos";
};

Adding the new "qcom,pmk8350-pon" compatible string would now be used to
differentiate between gen1/gen2 vs gen3 children, so we could get rid of the
addition of reg-names for older devices.


Similarly we could get rid of reg-names and the "qcom,pmk8350-pon" compatible
string as a whole like mentioned in
https://lore.kernel.org/all/99a5d9ac-9c20-b441-44af-26772a0e989d@linaro.org/,
if reg-names and the new compatible string is causing too much confusion.


Thanks,
Anjelique 
> 

>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: qcom,pmk8350-pon
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 1
>> +          maxItems: 2
>> +        reg-names:
>> +          minItems: 1
>> +          items:
>> +            - const: hlos
>> +            - const: pbs
>> +
>>  examples:
>>    - |
>>     #include <dt-bindings/interrupt-controller/irq.h>
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v4 2/2] power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

[Anjelique Melendez]

On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
> On 25/07/2022 21:13, Anjelique Melendez wrote:
>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  drivers/power/reset/qcom-pon.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>> index 4a688741a88a..16bc01738be9 100644
>> --- a/drivers/power/reset/qcom-pon.c
>> +++ b/drivers/power/reset/qcom-pon.c
>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>>  	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>>  	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>>  	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>> +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
> 
> This is now confusing. The new device has entirely different first and
> second IO address spaces, but you do not code here any differences.
> 

Based on previous responses to this patch series, it was decided that a new
"qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
vs gen3 children pon devices. Currently the child handles obtaining the register
address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
The patch that handled that change can be found at
https://lore.kernel.org/linux-arm-msm/20220422191239.6271-4-quic_amelende@quicinc.com/.

This driver, as the parent, does not actually need any changes besides adding the
new parent compatible string. Specifically this driver handles reboot mode support.
Everything needed to handle reboot mode is in the first register and reboot mode
is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
in pmk8350 does not affect reboot mode. This is why before we added a new compatible
string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
registers could be defined in the device tree, as a 2nd register is optional for gen3
children devices,the fact that this driver uses of_property_read_u32() will ensure that
the base address used for reboot mode comes from the first register.  

I do understand that this can be confusing but since "qcom,pmk8350-pon" still needs the
shift reason = 1 we could do

        #define GEN1_REASON_SHIFT		2
        #define GEN2_REASON_SHIFT		1	
   +    #define GEN3_REASON_SHIFT		1
 
  	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
  	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
  	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
   +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN3_REASON_SHIFT },
 	{ }


It would still have the exact same functionality but it may visually make more sense.


Thanks,
Anjelique

Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Anjelique Melendez]

Hi,

Wanted to send a reminder for this patch conversation.

Thanks,
Anjelique
On 8/19/2022 1:26 PM, Anjelique Melendez wrote:
> 
> Hi Krzysztof,
> First I would like to apologize for my lack of response to this patch series
> over these past few weeks. I have been out of office.
> 
> On 7/26/2022 3:25 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> From: David Collins <quic_collinsd@quicinc.com>
>>>
>>> Update the description of "reg" property to add the PON_PBS base
>>> address along with PON_HLOS base address.  Also add "reg-names"
>>> property constraints.
>>>
>>> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
>>>  1 file changed, 46 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> index 353f155d..d7b6b875 100644
>>> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> @@ -15,18 +15,27 @@ description: |
>>>  
>>>    This DT node has pwrkey and resin as sub nodes.
>>>  
>>> -allOf:
>>> -  - $ref: reboot-mode.yaml#
>>> -
>>>  properties:
>>>    compatible:
>>>      enum:
>>>        - qcom,pm8916-pon
>>>        - qcom,pms405-pon
>>>        - qcom,pm8998-pon
>>> +      - qcom,pmk8350-pon
>>>  
>>>    reg:
>>> -    maxItems: 1
>>> +    description: |
>>> +      Specifies the SPMI base address for the PON (power-on) peripheral.  For
>>> +      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
>>> +      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
>>> +      peripherals.  In that case, the PON_PBS address needs to be specified to
>>> +      facilitate software debouncing on some PMIC.
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +
>>> +  reg-names:
>>> +    minItems: 1
>>> +    maxItems: 2
>>>  
>>>    pwrkey:
>>>      type: object
>>> @@ -42,6 +51,39 @@ required:
>>>  
>>>  unevaluatedProperties: false
>>>  
>>> +allOf:
>>> +  - $ref: reboot-mode.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,pm8916-pon
>>> +              - qcom,pms405-pon
>>> +              - qcom,pm8998-pon
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          maxItems: 1
>>> +        reg-names:
>>> +          items:
>>> +            - const: pon
>>
>> All your previous patches were actually missing (in commit msg, in the
>> code) that piece of information which you add here. You now add
>> reg-names with "pon" for older devices. I assumed previous that it is
>> somehow needed, so I gave you the hints how it should be coded. But I
>> don't understand - why are you doing it
>>
>> This should be explained in commit msg. To me it is not needed at all...
>> unless you want to mark that first address space is entirely different
>> for other devices?
> Adding reg-names "pon" for older devices is simply to provide clarification
> about what the register relates to. Similar to reg-names "hlos" and "pbs"
> for gen3 children devices, reg-names is completely optional and is not
> consumed by any driver.
> 
> Before adding the "qcom,pmk8350-pon" compatible string, the best way to
> differentiate between a "qcom,pm8998-pon" parent with a gen1/gen2 child vs a
> "qcom,pm8998-pon" parent with a gen3 child with only an hlos register was to
> include reg-names. i.e.
> 
> pon_hlos@1300 {
>         reg = <0x1300>;
>         compatible = "qcom,pm8998-pon";
> 	reg-names = "pon";
> };
> 
> vs 
> 
> pon_hlos@1300 {
>         reg = <0x1300>;
>         compatible = "qcom,pm8998-pon";
> 	reg-names = "hlos";
> };
> 
> Adding the new "qcom,pmk8350-pon" compatible string would now be used to
> differentiate between gen1/gen2 vs gen3 children, so we could get rid of the
> addition of reg-names for older devices.
> 
> 
> Similarly we could get rid of reg-names and the "qcom,pmk8350-pon" compatible
> string as a whole like mentioned in
> https://lore.kernel.org/all/99a5d9ac-9c20-b441-44af-26772a0e989d@linaro.org/,
> if reg-names and the new compatible string is causing too much confusion.
> 
> 
> Thanks,
> Anjelique 
>>
> 
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: qcom,pmk8350-pon
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>> +          maxItems: 2
>>> +        reg-names:
>>> +          minItems: 1
>>> +          items:
>>> +            - const: hlos
>>> +            - const: pbs
>>> +
>>>  examples:
>>>    - |
>>>     #include <dt-bindings/interrupt-controller/irq.h>
>>
>>
>> Best regards,
>> Krzysztof

Re: [PATCH v4 2/2] power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

[Anjelique Melendez]

Hi,

Wanted to send a reminder for this patch conversation.

Thanks,
Anjelique

On 8/19/2022 1:26 PM, Anjelique Melendez wrote:
> 
> 
> On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>  drivers/power/reset/qcom-pon.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>>> index 4a688741a88a..16bc01738be9 100644
>>> --- a/drivers/power/reset/qcom-pon.c
>>> +++ b/drivers/power/reset/qcom-pon.c
>>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>>>  	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>>>  	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>>>  	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>>>  	{ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
>>
>> This is now confusing. The new device has entirely different first and
>> second IO address spaces, but you do not code here any differences.
>>
> 
> Based on previous responses to this patch series, it was decided that a new
> "qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
> vs gen3 children pon devices. Currently the child handles obtaining the register
> address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
> The patch that handled that change can be found at
> https://lore.kernel.org/linux-arm-msm/20220422191239.6271-4-quic_amelende@quicinc.com/.
> 
> This driver, as the parent, does not actually need any changes besides adding the
> new parent compatible string. Specifically this driver handles reboot mode support.
> Everything needed to handle reboot mode is in the first register and reboot mode
> is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
> in pmk8350 does not affect reboot mode. This is why before we added a new compatible
> string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
> registers could be defined in the device tree, as a 2nd register is optional for gen3
> children devices,the fact that this driver uses of_property_read_u32() will ensure that
> the base address used for reboot mode comes from the first register.  
> 
> I do understand that this can be confusing but since "qcom,pmk8350-pon" still needs the
> shift reason = 1 we could do
> 
>         #define GEN1_REASON_SHIFT		2
>         #define GEN2_REASON_SHIFT		1	
>    +    #define GEN3_REASON_SHIFT		1
>  
>   	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>   	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>   	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>    +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN3_REASON_SHIFT },
>  	{ }
> 
> 
> It would still have the exact same functionality but it may visually make more sense.
> 
> 
> Thanks,
> Anjelique
> 

Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Krzysztof Kozlowski]

On 19/08/2022 22:26, Anjelique Melendez wrote:
> 
> Hi Krzysztof,
> First I would like to apologize for my lack of response to this patch series
> over these past few weeks. I have been out of office.
> 
> On 7/26/2022 3:25 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> From: David Collins <quic_collinsd@quicinc.com>
>>>
>>> Update the description of "reg" property to add the PON_PBS base
>>> address along with PON_HLOS base address.  Also add "reg-names"
>>> property constraints.
>>>
>>> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
>>>  1 file changed, 46 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> index 353f155d..d7b6b875 100644
>>> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>> @@ -15,18 +15,27 @@ description: |
>>>  
>>>    This DT node has pwrkey and resin as sub nodes.
>>>  
>>> -allOf:
>>> -  - $ref: reboot-mode.yaml#
>>> -
>>>  properties:
>>>    compatible:
>>>      enum:
>>>        - qcom,pm8916-pon
>>>        - qcom,pms405-pon
>>>        - qcom,pm8998-pon
>>> +      - qcom,pmk8350-pon
>>>  
>>>    reg:
>>> -    maxItems: 1
>>> +    description: |
>>> +      Specifies the SPMI base address for the PON (power-on) peripheral.  For
>>> +      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
>>> +      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
>>> +      peripherals.  In that case, the PON_PBS address needs to be specified to
>>> +      facilitate software debouncing on some PMIC.
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +
>>> +  reg-names:
>>> +    minItems: 1
>>> +    maxItems: 2
>>>  
>>>    pwrkey:
>>>      type: object
>>> @@ -42,6 +51,39 @@ required:
>>>  
>>>  unevaluatedProperties: false
>>>  
>>> +allOf:
>>> +  - $ref: reboot-mode.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - qcom,pm8916-pon
>>> +              - qcom,pms405-pon
>>> +              - qcom,pm8998-pon
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          maxItems: 1
>>> +        reg-names:
>>> +          items:
>>> +            - const: pon
>>
>> All your previous patches were actually missing (in commit msg, in the
>> code) that piece of information which you add here. You now add
>> reg-names with "pon" for older devices. I assumed previous that it is
>> somehow needed, so I gave you the hints how it should be coded. But I
>> don't understand - why are you doing it
>>
>> This should be explained in commit msg. To me it is not needed at all...
>> unless you want to mark that first address space is entirely different
>> for other devices?
> Adding reg-names "pon" for older devices is simply to provide clarification
> about what the register relates to. Similar to reg-names "hlos" and "pbs"
> for gen3 children devices, reg-names is completely optional and is not
> consumed by any driver.

OK, can be. Include it in the commit msg, please.


Best regards,
Krzysztof

Re: [PATCH v4 2/2] power: reset: qcom-pon: add support for qcom,pmk8350-pon compatible string

[Krzysztof Kozlowski]

On 19/08/2022 22:26, Anjelique Melendez wrote:
> 
> 
> On 7/26/2022 3:27 AM, Krzysztof Kozlowski wrote:
>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>> Add support for the new "qcom,pmk8350-pon" comptaible string.
>>>
>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>> ---
>>>  drivers/power/reset/qcom-pon.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>>> index 4a688741a88a..16bc01738be9 100644
>>> --- a/drivers/power/reset/qcom-pon.c
>>> +++ b/drivers/power/reset/qcom-pon.c
>>> @@ -82,6 +82,7 @@ static const struct of_device_id pm8916_pon_id_table[] = {
>>>  	{ .compatible = "qcom,pm8916-pon", .data = (void *)GEN1_REASON_SHIFT },
>>>  	{ .compatible = "qcom,pms405-pon", .data = (void *)GEN1_REASON_SHIFT },
>>>  	{ .compatible = "qcom,pm8998-pon", .data = (void *)GEN2_REASON_SHIFT },
>>> +	{ .compatible = "qcom,pmk8350-pon", .data = (void *)GEN2_REASON_SHIFT },
>>>  	{ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, pm8916_pon_id_table);
>>
>> This is now confusing. The new device has entirely different first and
>> second IO address spaces, but you do not code here any differences.
>>
> 
> Based on previous responses to this patch series, it was decided that a new
> "qcom,pmk8350-pon" compatible string is needed to differentiate between gen1/gen2
> vs gen3 children pon devices. Currently the child handles obtaining the register
> address(es) from its parent's regmap in drivers/input/misc/pm8941-pwrkey.c
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/misc/pm8941-pwrkey.c?h=v6.0-rc1#n287).
> The patch that handled that change can be found at
> https://lore.kernel.org/linux-arm-msm/20220422191239.6271-4-quic_amelende@quicinc.com/.
> 
> This driver, as the parent, does not actually need any changes besides adding the
> new parent compatible string. Specifically this driver handles reboot mode support.
> Everything needed to handle reboot mode is in the first register and reboot mode
> is handled the same as a "qcom,pm8998-pon" parent. The split of the pon register
> in pmk8350 does not affect reboot mode. This is why before we added a new compatible
> string "qcom,pm8998-pon" still worked for gen3 children devices. Even though 2
> registers could be defined in the device tree, as a 2nd register is optional for gen3
> children devices,the fact that this driver uses of_property_read_u32() will ensure that
> the base address used for reboot mode comes from the first register.  
> 

Ah, makes sense.


Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg" property details

[Anjelique Melendez]

On 9/8/2022 7:55 AM, Krzysztof Kozlowski wrote:
> On 19/08/2022 22:26, Anjelique Melendez wrote:
>>
>> Hi Krzysztof,
>> First I would like to apologize for my lack of response to this patch series
>> over these past few weeks. I have been out of office.
>>
>> On 7/26/2022 3:25 AM, Krzysztof Kozlowski wrote:
>>> On 25/07/2022 21:13, Anjelique Melendez wrote:
>>>> From: David Collins <quic_collinsd@quicinc.com>
>>>>
>>>> Update the description of "reg" property to add the PON_PBS base
>>>> address along with PON_HLOS base address.  Also add "reg-names"
>>>> property constraints.
>>>>
>>>> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 50 +++++++++++++++++++++++++++---
>>>>  1 file changed, 46 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>>> index 353f155d..d7b6b875 100644
>>>> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>>> +++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>>>> @@ -15,18 +15,27 @@ description: |
>>>>  
>>>>    This DT node has pwrkey and resin as sub nodes.
>>>>  
>>>> -allOf:
>>>> -  - $ref: reboot-mode.yaml#
>>>> -
>>>>  properties:
>>>>    compatible:
>>>>      enum:
>>>>        - qcom,pm8916-pon
>>>>        - qcom,pms405-pon
>>>>        - qcom,pm8998-pon
>>>> +      - qcom,pmk8350-pon
>>>>  
>>>>    reg:
>>>> -    maxItems: 1
>>>> +    description: |
>>>> +      Specifies the SPMI base address for the PON (power-on) peripheral.  For
>>>> +      PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
>>>> +      (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
>>>> +      peripherals.  In that case, the PON_PBS address needs to be specified to
>>>> +      facilitate software debouncing on some PMIC.
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>> +
>>>> +  reg-names:
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>>  
>>>>    pwrkey:
>>>>      type: object
>>>> @@ -42,6 +51,39 @@ required:
>>>>  
>>>>  unevaluatedProperties: false
>>>>  
>>>> +allOf:
>>>> +  - $ref: reboot-mode.yaml#
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,pm8916-pon
>>>> +              - qcom,pms405-pon
>>>> +              - qcom,pm8998-pon
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          maxItems: 1
>>>> +        reg-names:
>>>> +          items:
>>>> +            - const: pon
>>>
>>> All your previous patches were actually missing (in commit msg, in the
>>> code) that piece of information which you add here. You now add
>>> reg-names with "pon" for older devices. I assumed previous that it is
>>> somehow needed, so I gave you the hints how it should be coded. But I
>>> don't understand - why are you doing it
>>>
>>> This should be explained in commit msg. To me it is not needed at all...
>>> unless you want to mark that first address space is entirely different
>>> for other devices?
>> Adding reg-names "pon" for older devices is simply to provide clarification
>> about what the register relates to. Similar to reg-names "hlos" and "pbs"
>> for gen3 children devices, reg-names is completely optional and is not
>> consumed by any driver.
> 
> OK, can be. Include it in the commit msg, please.

ACK

> 
> 
> Best regards,
> Krzysztof