[GIT,PULL,2/5] dt-bindings: power: Add power-domain header for JH7110

Commit Message

Conor Dooley Sept. 13, 2023, 1:54 p.m. UTC

From: Changhuang Liang <[e-mail]>

Add power-domain header for JH7110 SoC, it can use to operate dphy
power.

Signed-off-by: Changhuang Liang <[e-mail]>
Signed-off-by: Conor Dooley <[e-mail]>
---
 include/dt-bindings/power/starfive,jh7110-pmu.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven Sept. 26, 2023, 10:55 a.m. UTC | #1

Hi Conor, Changhuang,

On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <[e-mail]> wrote:
> From: Changhuang Liang <[e-mail]>
>
> Add power-domain header for JH7110 SoC, it can use to operate dphy
> power.
>
> Signed-off-by: Changhuang Liang <[e-mail]>
> Signed-off-by: Conor Dooley <[e-mail]>

Thanks for your patch, which is now commit 41b66b54a72bd796
("dt-bindings: power: Add power-domain header for JH7110")
in pmdomain/next.

Conor: looks like you forgot to update the patch description, as promised in
https://lore.kernel.org/all/20230519-irk-dwelled-6a499c482e62@spud?

> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>  /*
> - * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
>   * Author: Walker Chen <[e-mail]>
>   */
>  #ifndef __DT_BINDINGS_POWER_JH7110_POWER_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

These values are conflicting with similarly-named values above:

    #define JH7110_PD_SYSTOP        0
    #define JH7110_PD_CPU           1

Upon closer look, and diving into the driver, this header file contains
definitions for two separate number spaces: the first set is meant
to be used in PM Domain specifiers referring to a node compatible
with "starfive,jh7110-pmu", while the second set is meant to be
used in PM Domain specifiers referring to a node compatible with
"starfive,jh7110-aon-syscon".
I think it would be nice to have this reflected in the names of
the defines (e.g. JH7110_PD_AON_DPHY_TX), and/or to add comments
clearly delimiting the two separate number spaces, and explaining
where they apply to.

> +
>  #endif

Gr{oetje,eeting}s,

                        Geert

Conor Dooley Sept. 26, 2023, 11:20 a.m. UTC | #2

On Tue, Sep 26, 2023 at 12:55:01PM +0200, Geert Uytterhoeven wrote:
> Hi Conor, Changhuang,
> 
> On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <[e-mail]> wrote:
> > From: Changhuang Liang <[e-mail]>
> >
> > Add power-domain header for JH7110 SoC, it can use to operate dphy
> > power.
> >
> > Signed-off-by: Changhuang Liang <[e-mail]>
> > Signed-off-by: Conor Dooley <[e-mail]>
> 
> Thanks for your patch, which is now commit 41b66b54a72bd796
> ("dt-bindings: power: Add power-domain header for JH7110")
> in pmdomain/next.
> 
> Conor: looks like you forgot to update the patch description, as promised in
> https://lore.kernel.org/all/20230519-irk-dwelled-6a499c482e62@spud?

*sigh*, that's what happens when you apply the patches several months
after they were sent I guess. I'll try to make sure that that doesn't
happen again.

> > --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
> > +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
> > @@ -1,6 +1,6 @@
> >  /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >  /*
> > - * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
> >   * Author: Walker Chen <[e-mail]>
> >   */
> >  #ifndef __DT_BINDINGS_POWER_JH7110_POWER_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
> 
> These values are conflicting with similarly-named values above:
> 
>     #define JH7110_PD_SYSTOP        0
>     #define JH7110_PD_CPU           1
> 
> Upon closer look, and diving into the driver, this header file contains
> definitions for two separate number spaces: the first set is meant
> to be used in PM Domain specifiers referring to a node compatible
> with "starfive,jh7110-pmu", while the second set is meant to be
> used in PM Domain specifiers referring to a node compatible with
> "starfive,jh7110-aon-syscon".

Yeah, apologies for the confusion there. I maybe should have
requested an additional file, since the relevant compatibles
are different.

> I think it would be nice to have this reflected in the names of
> the defines (e.g. JH7110_PD_AON_DPHY_TX), and/or to add comments
> clearly delimiting the two separate number spaces, and explaining
> where they apply to.

Since this is 6.7 material, there's still plenty of time remaining to
make either of these changes. Changhuang Liang, do you think you would
be able to send a patch implementing either of the suggestions made by
Geert?

Thanks,
Conor.

Changhuang Liang Sept. 27, 2023, 1:24 a.m. UTC | #3

On 2023/9/26 18:55, Geert Uytterhoeven wrote:
> Hi Conor, Changhuang,
> 
> On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <[e-mail]> wrote:
>> From: Changhuang Liang <[e-mail]>
>>
>> Add power-domain header for JH7110 SoC, it can use to operate dphy
>> power.
>>
>> Signed-off-by: Changhuang Liang <[e-mail]>
>> Signed-off-by: Conor Dooley <[e-mail]>
> 
> Thanks for your patch, which is now commit 41b66b54a72bd796
> ("dt-bindings: power: Add power-domain header for JH7110")
> in pmdomain/next.
> 

Hi Geert,

Where is the website link for the pmdomain/next?

Thanks,
Changhuang

Changhuang Liang Sept. 27, 2023, 1:31 a.m. UTC | #4

>>>  #ifndef __DT_BINDINGS_POWER_JH7110_POWER_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
>>
>> These values are conflicting with similarly-named values above:
>>
>>     #define JH7110_PD_SYSTOP        0
>>     #define JH7110_PD_CPU           1
>>
>> Upon closer look, and diving into the driver, this header file contains
>> definitions for two separate number spaces: the first set is meant
>> to be used in PM Domain specifiers referring to a node compatible
>> with "starfive,jh7110-pmu", while the second set is meant to be
>> used in PM Domain specifiers referring to a node compatible with
>> "starfive,jh7110-aon-syscon".
> 
> Yeah, apologies for the confusion there. I maybe should have
> requested an additional file, since the relevant compatibles
> are different.
> 
>> I think it would be nice to have this reflected in the names of
>> the defines (e.g. JH7110_PD_AON_DPHY_TX), and/or to add comments
>> clearly delimiting the two separate number spaces, and explaining
>> where they apply to.
> 
> Since this is 6.7 material, there's still plenty of time remaining to
> make either of these changes. Changhuang Liang, do you think you would
> be able to send a patch implementing either of the suggestions made by
> Geert?
> 

Hi Conor,

I will send a patch to fix this confusion.

Thanks,
Changhuang

Geert Uytterhoeven Sept. 27, 2023, 7:12 a.m. UTC | #5

Hi Changhuang,

On Wed, Sep 27, 2023 at 3:25 AM Changhuang Liang
<[e-mail]> wrote:
> On 2023/9/26 18:55, Geert Uytterhoeven wrote:
> > On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <[e-mail]> wrote:
> >> From: Changhuang Liang <[e-mail]>
> >>
> >> Add power-domain header for JH7110 SoC, it can use to operate dphy
> >> power.
> >>
> >> Signed-off-by: Changhuang Liang <[e-mail]>
> >> Signed-off-by: Conor Dooley <[e-mail]>
> >
> > Thanks for your patch, which is now commit 41b66b54a72bd796
> > ("dt-bindings: power: Add power-domain header for JH7110")
> > in pmdomain/next.
>
> Where is the website link for the pmdomain/next?

As per https://elixir.bootlin.com/linux/v6.6-rc3/source/MAINTAINERS#L8729
it can be found at
https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git/log/?h=next

Gr{oetje,eeting}s,

                        Geert

Changhuang Liang Sept. 27, 2023, 1:26 p.m. UTC | #6

On 2023/9/26 18:55, Geert Uytterhoeven wrote:
> Hi Conor, Changhuang,
> 
> On Wed, Sep 13, 2023 at 3:58 PM Conor Dooley <[e-mail]> wrote:
>> From: Changhuang Liang <[e-mail]>
>>
>> Add power-domain header for JH7110 SoC, it can use to operate dphy
>> power.
>>
>> Signed-off-by: Changhuang Liang <[e-mail]>
>> Signed-off-by: Conor Dooley <[e-mail]>
> 
> Thanks for your patch, which is now commit 41b66b54a72bd796
> ("dt-bindings: power: Add power-domain header for JH7110")
> in pmdomain/next.
> 
> Conor: looks like you forgot to update the patch description, as promised in
> https://lore.kernel.org/all/20230519-irk-dwelled-6a499c482e62@spud?
> 
>> --- a/include/dt-bindings/power/starfive,jh7110-pmu.h
>> +++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>  /*
>> - * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
>>   * Author: Walker Chen <[e-mail]>
>>   */
>>  #ifndef __DT_BINDINGS_POWER_JH7110_POWER_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
> 
> These values are conflicting with similarly-named values above:
> 
>     #define JH7110_PD_SYSTOP        0
>     #define JH7110_PD_CPU           1
> 
> Upon closer look, and diving into the driver, this header file contains
> definitions for two separate number spaces: the first set is meant
> to be used in PM Domain specifiers referring to a node compatible
> with "starfive,jh7110-pmu", while the second set is meant to be
> used in PM Domain specifiers referring to a node compatible with
> "starfive,jh7110-aon-syscon".
> I think it would be nice to have this reflected in the names of
> the defines (e.g. JH7110_PD_AON_DPHY_TX), and/or to add comments
> clearly delimiting the two separate number spaces, and explaining
> where they apply to.
> 

Hi Geert,

I have sent a series to fix this, but I forget to send it to you.
https://patchwork.kernel.org/project/linux-pm/patch/[e-mail]/

Thanks,
Changhuang

Patch

diff --git a/include/dt-bindings/power/starfive,jh7110-pmu.h b/include/dt-bindings/power/starfive,jh7110-pmu.h
index 132bfe401fc8..341e2a0676ba 100644
--- a/include/dt-bindings/power/starfive,jh7110-pmu.h
+++ b/include/dt-bindings/power/starfive,jh7110-pmu.h
@@ -1,6 +1,6 @@ 
 /* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
 /*
- * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
  * Author: Walker Chen <[e-mail]>
  */
 #ifndef __DT_BINDINGS_POWER_JH7110_POWER_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