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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e6bf142-ca56-2414-86c4-1a18b74b3ba6@quicinc.com>
Date:   Fri, 19 Aug 2022 13:26:25 -0700
From:   Anjelique Melendez <quic_amelende@...cinc.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <corbet@....net>, <sre@...nel.org>, <robh+dt@...nel.org>,
        <agross@...nel.org>, <bjorn.andersson@...aro.org>
CC:     <krzysztof.kozlowski+dt@...aro.org>, <vkoul@...nel.org>,
        <linux-doc@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        David Collins <quic_collinsd@...cinc.com>
Subject: Re: [PATCH v4 1/2] dt-bindings: power: reset: qcom-pon: update "reg"
 property details


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@...cinc.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@...cinc.com>
>> Signed-off-by: Anjelique Melendez <quic_amelende@...cinc.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@...0 {
        reg = <0x1300>;
        compatible = "qcom,pm8998-pon";
	reg-names = "pon";
};

vs 

pon_hlos@...0 {
        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