[v1,1/7] dt-bindings: power: Constrain properties for JH7110 PMU

  When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
interrupts properties.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../bindings/power/starfive,jh7110-pmu.yaml        | 14 ++++++++++++--
 include/dt-bindings/power/starfive,jh7110-pmu.h    |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)
  

On Mon, Apr 10, 2023 at 11:47:37PM -0700, Changhuang Liang wrote:
> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
> interrupts properties.

Please write a commit message explaining why this is needed.
The commit message as-is is insufficient, but also IMO wrong incorrect.
I think it would more accurately be "...: add jh7110 dphy pmu support" or
similar & the body should explain why this particular PMU has no
reg/interrupts.

Cheers,
Conor.

> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh7110-pmu.yaml        | 14 ++++++++++++--
>  include/dt-bindings/power/starfive,jh7110-pmu.h    |  3 +++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> index 98eb8b4110e7..ffb4406c2e56 100644
> --- a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> +++ b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> @@ -8,6 +8,7 @@ title: StarFive JH7110 Power Management Unit
>  
>  maintainers:
>    - Walker Chen <walker.chen@starfivetech.com>
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
>  
>  description: |
>    StarFive JH7110 SoC includes support for multiple power domains which can be
> @@ -17,6 +18,7 @@ properties:
>    compatible:
>      enum:
>        - starfive,jh7110-pmu
> +      - starfive,jh7110-pmu-dphy
>  
>    reg:
>      maxItems: 1
> @@ -29,10 +31,18 @@ properties:
>  
>  required:
>    - compatible
> -  - reg
> -  - interrupts
>    - "#power-domain-cells"
>  
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: starfive,jh7110-pmu
> +then:
> +  required:
> +    - reg
> +    - interrupts
> +
>  additionalProperties: false
>  
>  examples:
> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
> index 132bfe401fc8..0bfd6700c144 100644
> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> @@ -14,4 +14,7 @@
>  #define JH7110_PD_ISP		5
>  #define JH7110_PD_VENC		6
>  
> +#define JH7110_PD_DPHY_TX	0
> +#define JH7110_PD_DPHY_RX	1
> +
>  #endif
> -- 
> 2.25.1
>
  

On 2023/4/12 4:13, Conor Dooley wrote:
> On Mon, Apr 10, 2023 at 11:47:37PM -0700, Changhuang Liang wrote:
>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>> interrupts properties.
> 
> Please write a commit message explaining why this is needed.
> The commit message as-is is insufficient, but also IMO wrong incorrect.
> I think it would more accurately be "...: add jh7110 dphy pmu support" or
> similar & the body should explain why this particular PMU has no
> reg/interrupts.
> 
> Cheers,
> Conor.
> 

OK, Thanks for your comments, I will reorganize the commit message more clarity.

>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh7110-pmu.yaml        | 14 ++++++++++++--
>>  include/dt-bindings/power/starfive,jh7110-pmu.h    |  3 +++
[...]
>>
  

On 11/04/2023 08:47, Changhuang Liang wrote:
> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
> interrupts properties.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh7110-pmu.yaml        | 14 ++++++++++++--
>  include/dt-bindings/power/starfive,jh7110-pmu.h    |  3 +++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> index 98eb8b4110e7..ffb4406c2e56 100644
> --- a/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> +++ b/Documentation/devicetree/bindings/power/starfive,jh7110-pmu.yaml
> @@ -8,6 +8,7 @@ title: StarFive JH7110 Power Management Unit
>  
>  maintainers:
>    - Walker Chen <walker.chen@starfivetech.com>
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
>  
>  description: |
>    StarFive JH7110 SoC includes support for multiple power domains which can be
> @@ -17,6 +18,7 @@ properties:
>    compatible:
>      enum:
>        - starfive,jh7110-pmu
> +      - starfive,jh7110-pmu-dphy

You do here much more than commit msg says.

Isn'y DPHY a phy? Why is it in power?

>  
>    reg:
>      maxItems: 1
> @@ -29,10 +31,18 @@ properties:
>  
>  required:
>    - compatible
> -  - reg
> -  - interrupts
>    - "#power-domain-cells"
>  
> +if:

Put it under allOf (in this place). Will save you one re-indentation later.

> +  properties:
> +    compatible:
> +      contains:
> +        const: starfive,jh7110-pmu
> +then:
> +  required:
> +    - reg
> +    - interrupts
> +
>  additionalProperties: false
>  
>  examples:
> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
> index 132bfe401fc8..0bfd6700c144 100644
> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> @@ -14,4 +14,7 @@
>  #define JH7110_PD_ISP		5
>  #define JH7110_PD_VENC		6
>  
> +#define JH7110_PD_DPHY_TX	0
> +#define JH7110_PD_DPHY_RX	1
> +
>  #endif

Best regards,
Krzysztof
  

On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
> On 11/04/2023 08:47, Changhuang Liang wrote:
>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>> interrupts properties.
[...]
>>  
>>  description: |
>>    StarFive JH7110 SoC includes support for multiple power domains which can be
>> @@ -17,6 +18,7 @@ properties:
>>    compatible:
>>      enum:
>>        - starfive,jh7110-pmu
>> +      - starfive,jh7110-pmu-dphy
> 
> You do here much more than commit msg says.
> 
> Isn'y DPHY a phy? Why is it in power?
> 

OK, I will add more description. This is a power framework used to turn on/off 
DPHY. So it in power, not a phy.

>>  
>>    reg:
>>      maxItems: 1
>> @@ -29,10 +31,18 @@ properties:
>>  
>>  required:
>>    - compatible
>> -  - reg
>> -  - interrupts
>>    - "#power-domain-cells"
>>  
>> +if:
> 
> Put it under allOf (in this place). Will save you one re-indentation later.
> 

OK, will fix it.

>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: starfive,jh7110-pmu
>> +then:
>> +  required:
>> +    - reg
>> +    - interrupts
>> +
>>  additionalProperties: false
>>  
>>  examples:
>> diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> index 132bfe401fc8..0bfd6700c144 100644
>> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
>> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> @@ -14,4 +14,7 @@
>>  #define JH7110_PD_ISP		5
>>  #define JH7110_PD_VENC		6
>>  
>> +#define JH7110_PD_DPHY_TX	0
>> +#define JH7110_PD_DPHY_RX	1
>> +
>>  #endif
> 
> Best regards,
> Krzysztof
>
  

On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
> 
> 
> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
> > On 11/04/2023 08:47, Changhuang Liang wrote:
> >> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
> >> interrupts properties.
> [...]
> >>  
> >>  description: |
> >>    StarFive JH7110 SoC includes support for multiple power domains which can be
> >> @@ -17,6 +18,7 @@ properties:
> >>    compatible:
> >>      enum:
> >>        - starfive,jh7110-pmu
> >> +      - starfive,jh7110-pmu-dphy
> > 
> > You do here much more than commit msg says.
> > 
> > Isn'y DPHY a phy? Why is it in power?
> > 
> 
> OK, I will add more description. This is a power framework used to turn on/off 
> DPHY. So it in power, not a phy.

Perhaps tie it less to its role w/ the phy, and more to do with its
location, say "jh7110-aon-pmu"?
There's already "aon"/"sys"/"stg" stuff used in clock-controller and
syscon compatibles etc.

Krzysztof, what do you think of that? (if you remember the whole
discussion we previously had about using those identifiers a few weeks
ago).
  

On 12/04/2023 11:42, Conor Dooley wrote:
> On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
>>
>>
>> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
>>> On 11/04/2023 08:47, Changhuang Liang wrote:
>>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>>>> interrupts properties.
>> [...]
>>>>  
>>>>  description: |
>>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
>>>> @@ -17,6 +18,7 @@ properties:
>>>>    compatible:
>>>>      enum:
>>>>        - starfive,jh7110-pmu
>>>> +      - starfive,jh7110-pmu-dphy
>>>
>>> You do here much more than commit msg says.
>>>
>>> Isn'y DPHY a phy? Why is it in power?
>>>
>>
>> OK, I will add more description. This is a power framework used to turn on/off 
>> DPHY. So it in power, not a phy.
> 
> Perhaps tie it less to its role w/ the phy, and more to do with its
> location, say "jh7110-aon-pmu"?
> There's already "aon"/"sys"/"stg" stuff used in clock-controller and
> syscon compatibles etc.
> 
> Krzysztof, what do you think of that? (if you remember the whole
> discussion we previously had about using those identifiers a few weeks
> ago).

Depends whether this is the same case or not. AFAIR, for AON/SYS/STG
these were blocks with few features, not only clock controller.

This sounds like just phy. Powering on/off phy is still a job of phy
controller... unless it is a power domain controller.
Best regards,
Krzysztof
  

On 2023/4/12 19:29, Krzysztof Kozlowski wrote:
> On 12/04/2023 11:42, Conor Dooley wrote:
>> On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
>>>
>>>
>>> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
>>>> On 11/04/2023 08:47, Changhuang Liang wrote:
>>>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>>>>> interrupts properties.
>>> [...]
>>>>>  
>>>>>  description: |
>>>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
>>>>> @@ -17,6 +18,7 @@ properties:
>>>>>    compatible:
>>>>>      enum:
>>>>>        - starfive,jh7110-pmu
>>>>> +      - starfive,jh7110-pmu-dphy
>>>>
>>>> You do here much more than commit msg says.
>>>>
>>>> Isn'y DPHY a phy? Why is it in power?
>>>>
>>>
>>> OK, I will add more description. This is a power framework used to turn on/off 
>>> DPHY. So it in power, not a phy.
>>
>> Perhaps tie it less to its role w/ the phy, and more to do with its
>> location, say "jh7110-aon-pmu"?
>> There's already "aon"/"sys"/"stg" stuff used in clock-controller and
>> syscon compatibles etc.
>>
>> Krzysztof, what do you think of that? (if you remember the whole
>> discussion we previously had about using those identifiers a few weeks
>> ago).
> 
> Depends whether this is the same case or not. AFAIR, for AON/SYS/STG
> these were blocks with few features, not only clock controller.
> 
> This sounds like just phy. Powering on/off phy is still a job of phy
> controller... unless it is a power domain controller.
> Best regards,
> Krzysztof
> 

Hi, Coner and Krzysztof,

Next version I will change commit message:

dt-bindings: power: Add JH7110 DPHY PMU support.

Add DPHY PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY rx/tx
power switch, and it don't need the reg and interrupt properties.

I think this commit message will helpful for you to understand it.

Best regards,
Changhuang
  

On 2023/4/12 19:29, Krzysztof Kozlowski wrote:
> On 12/04/2023 11:42, Conor Dooley wrote:
>> On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
>>>
>>>
>>> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
>>>> On 11/04/2023 08:47, Changhuang Liang wrote:
>>>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>>>>> interrupts properties.
>>> [...]
>>>>>  
>>>>>  description: |
>>>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
>>>>> @@ -17,6 +18,7 @@ properties:
>>>>>    compatible:
>>>>>      enum:
>>>>>        - starfive,jh7110-pmu
>>>>> +      - starfive,jh7110-pmu-dphy
>>>>
>>>> You do here much more than commit msg says.
>>>>
>>>> Isn'y DPHY a phy? Why is it in power?
>>>>
>>>
>>> OK, I will add more description. This is a power framework used to turn on/off 
>>> DPHY. So it in power, not a phy.

I found something wrong with my description here, not turn on/off DPHY,
is turn on/off DPHY power switch. 

>>
>> Perhaps tie it less to its role w/ the phy, and more to do with its
>> location, say "jh7110-aon-pmu"?
>> There's already "aon"/"sys"/"stg" stuff used in clock-controller and
>> syscon compatibles etc.
>>
>> Krzysztof, what do you think of that? (if you remember the whole
>> discussion we previously had about using those identifiers a few weeks
>> ago).
> 
> Depends whether this is the same case or not. AFAIR, for AON/SYS/STG
> these were blocks with few features, not only clock controller.
> 
> This sounds like just phy. Powering on/off phy is still a job of phy
> controller... unless it is a power domain controller.
> Best regards,
> Krzysztof
> 

So, next version the compatible can be changed to "jh7110-aon-pmu"?
  

On Wed, Apr 12, 2023 at 01:29:57PM +0200, Krzysztof Kozlowski wrote:
> On 12/04/2023 11:42, Conor Dooley wrote:
> > On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
> >>
> >>
> >> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
> >>> On 11/04/2023 08:47, Changhuang Liang wrote:
> >>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
> >>>> interrupts properties.
> >> [...]
> >>>>  
> >>>>  description: |
> >>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
> >>>> @@ -17,6 +18,7 @@ properties:
> >>>>    compatible:
> >>>>      enum:
> >>>>        - starfive,jh7110-pmu
> >>>> +      - starfive,jh7110-pmu-dphy
> >>>
> >>> You do here much more than commit msg says.
> >>>
> >>> Isn'y DPHY a phy? Why is it in power?
> >>>
> >>
> >> OK, I will add more description. This is a power framework used to turn on/off 
> >> DPHY. So it in power, not a phy.
> > 
> > Perhaps tie it less to its role w/ the phy, and more to do with its
> > location, say "jh7110-aon-pmu"?
> > There's already "aon"/"sys"/"stg" stuff used in clock-controller and
> > syscon compatibles etc.
> > 
> > Krzysztof, what do you think of that? (if you remember the whole
> > discussion we previously had about using those identifiers a few weeks
> > ago).
> 
> Depends whether this is the same case or not.

> AFAIR, for AON/SYS/STG
> these were blocks with few features, not only clock controller.

Correct, yes. In the dts, this "pmu-dphy" node is a child node of the
aon syscon, so this pmu stuff would be one of the several features.
  

On Fri, Apr 14, 2023 at 10:20:31AM +0800, Changhuang Liang wrote:
> 
> 
> On 2023/4/12 19:29, Krzysztof Kozlowski wrote:
> > On 12/04/2023 11:42, Conor Dooley wrote:
> >> On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
> >>>
> >>>
> >>> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
> >>>> On 11/04/2023 08:47, Changhuang Liang wrote:
> >>>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
> >>>>> interrupts properties.
> >>> [...]
> >>>>>  
> >>>>>  description: |
> >>>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
> >>>>> @@ -17,6 +18,7 @@ properties:
> >>>>>    compatible:
> >>>>>      enum:
> >>>>>        - starfive,jh7110-pmu
> >>>>> +      - starfive,jh7110-pmu-dphy
> >>>>
> >>>> You do here much more than commit msg says.
> >>>>
> >>>> Isn'y DPHY a phy? Why is it in power?
> >>>>
> >>>
> >>> OK, I will add more description. This is a power framework used to turn on/off 
> >>> DPHY. So it in power, not a phy.
> 
> I found something wrong with my description here, not turn on/off DPHY,
> is turn on/off DPHY power switch. 
> 
> >>
> >> Perhaps tie it less to its role w/ the phy, and more to do with its
> >> location, say "jh7110-aon-pmu"?
> >> There's already "aon"/"sys"/"stg" stuff used in clock-controller and
> >> syscon compatibles etc.
> >>
> >> Krzysztof, what do you think of that? (if you remember the whole
> >> discussion we previously had about using those identifiers a few weeks
> >> ago).
> > 
> > Depends whether this is the same case or not. AFAIR, for AON/SYS/STG
> > these were blocks with few features, not only clock controller.
> > 
> > This sounds like just phy. Powering on/off phy is still a job of phy
> > controller... unless it is a power domain controller.
> > Best regards,
> > Krzysztof
> > 
> 
> So, next version the compatible can be changed to "jh7110-aon-pmu"?

Hmm, is the dphy the only thing that's power is controlled by registers
in the aon syscon? I tried looking in the "preliminary" TRM that I have,
but it's not really got a proper register map so I could not tell.

If there are, it'd help your case I think Changhuang Liang.
  

On 2023/4/18 2:55, Conor Dooley wrote:
> On Fri, Apr 14, 2023 at 10:20:31AM +0800, Changhuang Liang wrote:
>>
>>
>> On 2023/4/12 19:29, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 11:42, Conor Dooley wrote:
>>>> On Wed, Apr 12, 2023 at 04:51:16PM +0800, Changhuang Liang wrote:
>>>>>
>>>>>
>>>>> On 2023/4/12 16:35, Krzysztof Kozlowski wrote:
>>>>>> On 11/04/2023 08:47, Changhuang Liang wrote:
>>>>>>> When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
>>>>>>> interrupts properties.
>>>>> [...]
>>>>>>>  
>>>>>>>  description: |
>>>>>>>    StarFive JH7110 SoC includes support for multiple power domains which can be
>>>>>>> @@ -17,6 +18,7 @@ properties:
>>>>>>>    compatible:
>>>>>>>      enum:
>>>>>>>        - starfive,jh7110-pmu
>>>>>>> +      - starfive,jh7110-pmu-dphy
>>>>>>
>>>>>> You do here much more than commit msg says.
>>>>>>
>>>>>> Isn'y DPHY a phy? Why is it in power?
>>>>>>
>>>>>
>>>>> OK, I will add more description. This is a power framework used to turn on/off 
>>>>> DPHY. So it in power, not a phy.
>>
>> I found something wrong with my description here, not turn on/off DPHY,
>> is turn on/off DPHY power switch. 
>>
>>>>
>>>> Perhaps tie it less to its role w/ the phy, and more to do with its
>>>> location, say "jh7110-aon-pmu"?
>>>> There's already "aon"/"sys"/"stg" stuff used in clock-controller and
>>>> syscon compatibles etc.
>>>>
>>>> Krzysztof, what do you think of that? (if you remember the whole
>>>> discussion we previously had about using those identifiers a few weeks
>>>> ago).
>>>
>>> Depends whether this is the same case or not. AFAIR, for AON/SYS/STG
>>> these were blocks with few features, not only clock controller.
>>>
>>> This sounds like just phy. Powering on/off phy is still a job of phy
>>> controller... unless it is a power domain controller.
>>> Best regards,
>>> Krzysztof
>>>
>>
>> So, next version the compatible can be changed to "jh7110-aon-pmu"?
> 
> Hmm, is the dphy the only thing that's power is controlled by registers
> in the aon syscon? I tried looking in the "preliminary" TRM that I have,
> but it's not really got a proper register map so I could not tell.
> 
> If there are, it'd help your case I think Changhuang Liang.

I made a discussion with Walker, We don't use other bit on the visionfive2 
board. And I first naming by function. So I will change to "jh7110-aon-pmu"
next version.