When use "starfive,jh7110-pmu-dphy" compatible, do not need the reg and
interrupts properties.
Signed-off-by: Changhuang Liang <[email protected]>
---
.../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 <[email protected]>
+ - Changhuang Liang <[email protected]>
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 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 <[email protected]>
> ---
> .../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 <[email protected]>
> + - Changhuang Liang <[email protected]>
>
> 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
> ---
> .../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 <[email protected]>
> + - Changhuang Liang <[email protected]>
>
> 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.