[RESEND v2 0/6] Add JH7110 AON PMU support

[RESEND v2 0/6] Add JH7110 AON PMU support

[Changhuang Liang]

This patchset adds aon power domain driver for the StarFive JH7110 SoC.
It is used to turn on/off dphy rx/tx power switch. The series has been
tested on the VisionFive 2 board.

This patchset should be applied after the patchset [1]:
[1] https://lore.kernel.org/all/20230414024157.53203-1-xingyu.wu@starfivetech.com/

changes since v1:
- Updated commit message.
- Changed "starfive,jh7110-pmu-dphy" to "starfive,jh7110-aon-pmu".
- Put if condition under allOf in .yaml file.
- Updated spelling error.
- Dropped patch 4: Add pmu type operation.
- Changed "jh71xx_pmu_general_set_state" to "jh7110_pmu_set_state" and moved it in call back.
- Changed "jh7110_pmu_general_parse_dt" to "jh7110_pmu_parse_dt" and moved it in call back.
- Used pmu_status save the pmu status offset.
- Changed "JH71XX_PMU_DPHY_SWITCH" to "JH71XX_AON_PMU_SWITCH"
- Changed copyright to "2022-2023"

v1: https://lore.kernel.org/all/20230411064743.273388-1-changhuang.liang@starfivetech.com/

Changhuang Liang (6):
  dt-bindings: power: Add JH7110 AON PMU support
  soc: starfive: Replace SOC_STARFIVE with ARCH_STARFIVE
  soc: starfive: Modify ioremap to regmap
  soc: starfive: Extract JH7110 pmu private operations
  soc: starfive: Add JH7110 AON PMU support
  riscv: dts: starfive: jh7110: Add AON PMU node

 .../bindings/power/starfive,jh7110-pmu.yaml   |  15 +-
 MAINTAINERS                                   |   1 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   5 +
 drivers/soc/starfive/Kconfig                  |   4 +-
 drivers/soc/starfive/jh71xx_pmu.c             | 200 +++++++++++++-----
 .../dt-bindings/power/starfive,jh7110-pmu.h   |   3 +
 6 files changed, 174 insertions(+), 54 deletions(-)


base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
prerequisite-patch-id: 388b8adbb0fe2daf4d07a21eafd4f1bd50ce2403
prerequisite-patch-id: 1117ecaa40a353c667b71802ab34ecf9568d8bb2
prerequisite-patch-id: b00c6b21fbd0353d88b7c9b09093ba30b765f45b
prerequisite-patch-id: 08ec9027e8a5c6fdf201726833168c7464a9b94d
prerequisite-patch-id: fb5120248e48fe1faf053ae0b490c92507ec2b44
prerequisite-patch-id: 4b93d8d590b0a2abe7b4be5287232c494c35be4a
prerequisite-patch-id: 89f049f951e5acf75aab92541992f816fd0acc0d
prerequisite-patch-id: c09c4c68af017b8e5c97b515cb50b70c18a2e705
prerequisite-patch-id: 0df8ccb0e848c2df4c2da95026494bebecede92d
prerequisite-patch-id: 315303931e4b6499de7127a88113763f86e97e16
prerequisite-patch-id: 40cb8212ddb024c20593f73d8b87d9894877e172
prerequisite-patch-id: a1673a9e9f19d6fab5a51abb721e54e36636f067
prerequisite-patch-id: d57cc467fb036241b9276320ff076c4a30d376d6
prerequisite-patch-id: 6e563d68bc5dbf951d4ced17897f9cc4d56169fe
prerequisite-patch-id: 61ec2caa21fd0fc60e57977f7d16d3f72b135745
prerequisite-patch-id: 1387a7e87b446329dfc21f3e575ceae7ebcf954c
prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
prerequisite-patch-id: dbb0c0151b8bdf093e6ce79fd2fe3f60791a6e0b
prerequisite-patch-id: 9007c8610fdcd387592475949864edde874c20a2
prerequisite-patch-id: d57e95d31686772abc4c4d5aa1cadc344dc293cd
prerequisite-patch-id: 0a0ac5a8a90655b415f6b62e324f3db083cdaaee
prerequisite-patch-id: c9bb2bf387954769731a627a195a2e957506d7c0
prerequisite-patch-id: 3b13e6bd0cb1fede74841bada2a50d30c8049198
prerequisite-patch-id: 3dc2fe994cb83e5bd1faac17b9e40e4d9adf9863
prerequisite-patch-id: 4c4d536c34d6cb607d4d00d4c982175d217832be
prerequisite-patch-id: dd12678b9cdcd6dccdb900c45fe1f6ce11d67fd5
prerequisite-patch-id: 284b5d1b95c6d68bca08db1e82ed14930c98b777
prerequisite-patch-id: 2c2daec388f036ebc8decfaeaa216d979d792c58
--
2.25.1

[RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

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

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
 include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++
 2 files changed, 16 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..c50507c38e14 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-aon-pmu
 
   reg:
     maxItems: 1
@@ -29,10 +31,19 @@ properties:
 
 required:
   - compatible
-  - reg
-  - interrupts
   - "#power-domain-cells"
 
+allOf:
+  - 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

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3498 bytes --]

Hey Changhuang, DT/PHY folks,

On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
> rx/tx power switch, and it don't need the properties of reg and
> interrupts.

Putting this here since the DT guys are more likely to see it this way..
Given how the implementation of the code driving this new
power-controller and the code driving the existing one are rather
different (you've basically re-written the entire driver in this series),
should the dphy driver implement its own power-controller?

I know originally Changuang had tried something along those lines:
https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/

I see that that was shut down pretty much, partly due to the
non-standard property, hence this series adding the dphy power domain to
the existing driver.

If it was done by looking up the pmu with a
of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
type thing, would that make sense? Although, maybe that is not a
question for you, and this series may actually have been better entirely
bundled with the dphy series so the whole thing can be reviewed as a
unit. I've added 

IOW, don't change this patch, or the dts patch, but move all of the
code back into the phy driver..

Sorry for not asking this sooner Changhuang,
Conor.

(hopefully this didn't get sent twice, mutt complained of a bad email
addr during sending the first time)

> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
>  include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++
>  2 files changed, 16 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..c50507c38e14 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-aon-pmu
>  
>    reg:
>      maxItems: 1
> @@ -29,10 +31,19 @@ properties:
>  
>  required:
>    - compatible
> -  - reg
> -  - interrupts
>    - "#power-domain-cells"
>  
> +allOf:
> +  - 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/20 2:29, Conor Dooley wrote:
> Hey Changhuang, DT/PHY folks,
> 
> On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
>> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
>> rx/tx power switch, and it don't need the properties of reg and
>> interrupts.
> 
> Putting this here since the DT guys are more likely to see it this way..
> Given how the implementation of the code driving this new
> power-controller and the code driving the existing one are rather
> different (you've basically re-written the entire driver in this series),
> should the dphy driver implement its own power-controller?
> 
> I know originally Changuang had tried something along those lines:
> https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
> 
> I see that that was shut down pretty much, partly due to the
> non-standard property, hence this series adding the dphy power domain to
> the existing driver.
> 
> If it was done by looking up the pmu with a
> of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
> type thing, would that make sense? Although, maybe that is not a
> question for you, and this series may actually have been better entirely
> bundled with the dphy series so the whole thing can be reviewed as a
> unit. I've added 
> 
> IOW, don't change this patch, or the dts patch, but move all of the
> code back into the phy driver..
> 

Maybe this way can not do that? power domain is binding before driver probe,
if I use "of_find_compatible_node" it phy(DPHY rx) probe. Maybe I can only operate 
this power switch in my phy(DPHY rx) driver, so the all patch of this series isn't 
make sense.

In my opinion, We will also submit DPHY TX module later which use this series.
Maybe this series should independent?

> Sorry for not asking this sooner Changhuang,
> Conor.
> 
> (hopefully this didn't get sent twice, mutt complained of a bad email
> addr during sending the first time)
> 

I'm sorry for that, I will notice later.

>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
>>  include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++
>>  2 files changed, 16 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..c50507c38e14 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-aon-pmu
>>  
>>    reg:
>>      maxItems: 1
>> @@ -29,10 +31,19 @@ properties:
>>  
>>  required:
>>    - compatible
>> -  - reg
>> -  - interrupts
>>    - "#power-domain-cells"
>>  
>> +allOf:
>> +  - 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
>>

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5616 bytes --]

Hey Changhuang,

On Thu, Apr 20, 2023 at 03:00:10PM +0800, Changhuang Liang wrote:
> On 2023/4/20 2:29, Conor Dooley wrote:
> > On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
> >> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
> >> rx/tx power switch, and it don't need the properties of reg and
> >> interrupts.
> > 
> > Putting this here since the DT guys are more likely to see it this way..
> > Given how the implementation of the code driving this new
> > power-controller and the code driving the existing one are rather
> > different (you've basically re-written the entire driver in this series),
> > should the dphy driver implement its own power-controller?
> > 
> > I know originally Changuang had tried something along those lines:
> > https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
> > 
> > I see that that was shut down pretty much, partly due to the
> > non-standard property, hence this series adding the dphy power domain to
> > the existing driver.
> > 
> > If it was done by looking up the pmu with a
> > of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
> > type thing, would that make sense? Although, maybe that is not a
> > question for you, and this series may actually have been better entirely
> > bundled with the dphy series so the whole thing can be reviewed as a
> > unit. I've added 
> > 
> > IOW, don't change this patch, or the dts patch, but move all of the
> > code back into the phy driver..
> > 
> 
> Maybe this way can not do that? power domain is binding before driver probe,
> if I use "of_find_compatible_node" it phy(DPHY rx) probe. Maybe I can only operate 
> this power switch in my phy(DPHY rx) driver, so the all patch of this series isn't 
> make sense.

I'm a wee bit lost here, as I unfortunately know little about how Linux
handles this power-domain stuff. If the DPHY tries to probe and some
pre-requisite does not yet exist, you can return -EPROBE_DEFER right?

But I don't think that's what you are asking, as using
of_find_compatible_node() doesn't depend on there being a driver AFAIU.

> In my opinion, We will also submit DPHY TX module later which use this series.
> Maybe this series should independent?

Is the DPHY tx module a different driver to the rx one?
If yes, does it have a different bit you must set in the syscon?

+CC Walker, do you have a register map for the jh7110? My TRM only says
what the registers are, but not the bits in them. Would make life easier
if I had that info.

I'm fine with taking this code, I just want to make sure that the soc
driver doing this is the right thing to do.
I was kinda hoping that combining with the DPHY-rx series might allow
the PHY folk to spot if you are doing something here with the power
domains that doesn't make sense.

> > Sorry for not asking this sooner Changhuang,
> > Conor.
> > 
> > (hopefully this didn't get sent twice, mutt complained of a bad email
> > addr during sending the first time)
> > 
> 
> I'm sorry for that, I will notice later.

No, this was my mail client doing things that I was unsure of. You
didn't do anything wrong.

> >> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> >> ---
> >>  .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
> >>  include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++
> >>  2 files changed, 16 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..c50507c38e14 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-aon-pmu

I was speaking to Rob about this over the weekend, he asked:
'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
itself?'
Do we actually need to add a new binding for this at all?

Cheers,
Conor.

> >>  
> >>    reg:
> >>      maxItems: 1
> >> @@ -29,10 +31,19 @@ properties:
> >>  
> >>  required:
> >>    - compatible
> >> -  - reg
> >> -  - interrupts
> >>    - "#power-domain-cells"
> >>  
> >> +allOf:
> >> +  - 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
> >>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/25 0:52, Conor Dooley wrote:
> Hey Changhuang,
> 
> On Thu, Apr 20, 2023 at 03:00:10PM +0800, Changhuang Liang wrote:
>> On 2023/4/20 2:29, Conor Dooley wrote:
>>> On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
>>>> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
>>>> rx/tx power switch, and it don't need the properties of reg and
>>>> interrupts.
>>>
>>> Putting this here since the DT guys are more likely to see it this way..
>>> Given how the implementation of the code driving this new
>>> power-controller and the code driving the existing one are rather
>>> different (you've basically re-written the entire driver in this series),
>>> should the dphy driver implement its own power-controller?
>>>
>>> I know originally Changuang had tried something along those lines:
>>> https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
>>>
>>> I see that that was shut down pretty much, partly due to the
>>> non-standard property, hence this series adding the dphy power domain to
>>> the existing driver.
>>>
>>> If it was done by looking up the pmu with a
>>> of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
>>> type thing, would that make sense? Although, maybe that is not a
>>> question for you, and this series may actually have been better entirely
>>> bundled with the dphy series so the whole thing can be reviewed as a
>>> unit. I've added 
>>>
>>> IOW, don't change this patch, or the dts patch, but move all of the
>>> code back into the phy driver..
>>>
>>
>> Maybe this way can not do that? power domain is binding before driver probe,
>> if I use "of_find_compatible_node" it phy(DPHY rx) probe. Maybe I can only operate 
>> this power switch in my phy(DPHY rx) driver, so the all patch of this series isn't 
>> make sense.
> 
> I'm a wee bit lost here, as I unfortunately know little about how Linux
> handles this power-domain stuff. If the DPHY tries to probe and some
> pre-requisite does not yet exist, you can return -EPROBE_DEFER right?
> 
> But I don't think that's what you are asking, as using
> of_find_compatible_node() doesn't depend on there being a driver AFAIU.
> 
>> In my opinion, We will also submit DPHY TX module later which use this series.
>> Maybe this series should independent?
> 
> Is the DPHY tx module a different driver to the rx one?> If yes, does it have a different bit you must set in the syscon?
> 

Yes, DPHY tx module is a different driver to the DPHY rx. And I have do a
different bit in PATCH 1:

#define JH7110_PD_DPHY_TX	0
#define JH7110_PD_DPHY_RX	1

also in PATCH 5:

static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
	[JH7110_PD_DPHY_TX] = {
		.name = "DPHY-TX",
		.bit = 30,
	},
	[JH7110_PD_DPHY_RX] = {
		.name = "DPHY-RX",
		.bit = 31,
	},
};

> +CC Walker, do you have a register map for the jh7110? My TRM only says
> what the registers are, but not the bits in them. Would make life easier
> if I had that info.
> 
> I'm fine with taking this code, I just want to make sure that the soc
> driver doing this is the right thing to do.
> I was kinda hoping that combining with the DPHY-rx series might allow
> the PHY folk to spot if you are doing something here with the power
> domains that doesn't make sense.
> 

I asked about our soc colleagues. This syscon register,
offset 0x00:
bit[31] ---> dphy rx power switch
bit[30] ---> dphy tx power switch 
other bits ---> Reserved

>>> Sorry for not asking this sooner Changhuang,
>>> Conor.
>>>
>>> (hopefully this didn't get sent twice, mutt complained of a bad email
>>> addr during sending the first time)
>>>
>>
>> I'm sorry for that, I will notice later.
> 
> No, this was my mail client doing things that I was unsure of. You
> didn't do anything wrong.
> 
[...]
>>>>    - 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-aon-pmu
> 
> I was speaking to Rob about this over the weekend, he asked:
> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
> itself?'

Maybe not, this syscon only offset "0x00" configure power switch.
other offset configure other functions, maybe not power, so this
"starfive,jh7110-aon-syscon" not the power-domain itself.

> Do we actually need to add a new binding for this at all?
> 
> Cheers,
> Conor.
> 

Maybe this patch do that.
https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/

>>>>  
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -29,10 +31,19 @@ properties:
>>>>  

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]

On Tue, Apr 25, 2023 at 11:41:38AM +0800, Changhuang Liang wrote:
> On 2023/4/25 0:52, Conor Dooley wrote:
> > On Thu, Apr 20, 2023 at 03:00:10PM +0800, Changhuang Liang wrote:
> >> On 2023/4/20 2:29, Conor Dooley wrote:
> >>> On Tue, Apr 18, 2023 at 08:56:41PM -0700, Changhuang Liang wrote:
> >>>> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
> >>>> rx/tx power switch, and it don't need the properties of reg and
> >>>> interrupts.
> >>>
> >>> Putting this here since the DT guys are more likely to see it this way..
> >>> Given how the implementation of the code driving this new
> >>> power-controller and the code driving the existing one are rather
> >>> different (you've basically re-written the entire driver in this series),
> >>> should the dphy driver implement its own power-controller?
> >>>
> >>> I know originally Changuang had tried something along those lines:
> >>> https://lore.kernel.org/linux-riscv/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
> >>>
> >>> I see that that was shut down pretty much, partly due to the
> >>> non-standard property, hence this series adding the dphy power domain to
> >>> the existing driver.
> >>>
> >>> If it was done by looking up the pmu with a
> >>> of_find_compatible_node(NULL, "power-controller", "starfive,jh7110-aon-pmu")
> >>> type thing, would that make sense? Although, maybe that is not a
> >>> question for you, and this series may actually have been better entirely
> >>> bundled with the dphy series so the whole thing can be reviewed as a
> >>> unit. I've added 
> >>>
> >>> IOW, don't change this patch, or the dts patch, but move all of the
> >>> code back into the phy driver..
> >>>
> >>
> >> Maybe this way can not do that? power domain is binding before driver probe,
> >> if I use "of_find_compatible_node" it phy(DPHY rx) probe. Maybe I can only operate 
> >> this power switch in my phy(DPHY rx) driver, so the all patch of this series isn't 
> >> make sense.
> > 
> > I'm a wee bit lost here, as I unfortunately know little about how Linux
> > handles this power-domain stuff. If the DPHY tries to probe and some
> > pre-requisite does not yet exist, you can return -EPROBE_DEFER right?
> > 
> > But I don't think that's what you are asking, as using
> > of_find_compatible_node() doesn't depend on there being a driver AFAIU.
> > 
> >> In my opinion, We will also submit DPHY TX module later which use this series.
> >> Maybe this series should independent?
> > 
> > Is the DPHY tx module a different driver to the rx one?> If yes, does it have a different bit you must set in the syscon?
> > 
> 
> Yes, DPHY tx module is a different driver to the DPHY rx. And I have do a
> different bit in PATCH 1:
> 
> #define JH7110_PD_DPHY_TX	0
> #define JH7110_PD_DPHY_RX	1
> 
> also in PATCH 5:
> 
> static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
> 	[JH7110_PD_DPHY_TX] = {
> 		.name = "DPHY-TX",
> 		.bit = 30,
> 	},
> 	[JH7110_PD_DPHY_RX] = {
> 		.name = "DPHY-RX",
> 		.bit = 31,
> 	},
> };
> 
> > +CC Walker, do you have a register map for the jh7110? My TRM only says
> > what the registers are, but not the bits in them. Would make life easier
> > if I had that info.
> > 
> > I'm fine with taking this code, I just want to make sure that the soc
> > driver doing this is the right thing to do.
> > I was kinda hoping that combining with the DPHY-rx series might allow
> > the PHY folk to spot if you are doing something here with the power
> > domains that doesn't make sense.
> > 
> 
> I asked about our soc colleagues. This syscon register,
> offset 0x00:
> bit[31] ---> dphy rx power switch
> bit[30] ---> dphy tx power switch 
> other bits ---> Reserved

Okay. Unless someone explicitly disagrees, I'm fine with doing this
stand-alone from the DPHY drivers.

> >>> Sorry for not asking this sooner Changhuang,
> >>> Conor.
> >>>
> >>> (hopefully this didn't get sent twice, mutt complained of a bad email
> >>> addr during sending the first time)
> >>>
> >>
> >> I'm sorry for that, I will notice later.
> > 
> > No, this was my mail client doing things that I was unsure of. You
> > didn't do anything wrong.
> > 
> [...]
> >>>>    - 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-aon-pmu
> > 
> > I was speaking to Rob about this over the weekend, he asked:
> > 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
> > itself?'
> 
> Maybe not, this syscon only offset "0x00" configure power switch.
> other offset configure other functions, maybe not power, so this
> "starfive,jh7110-aon-syscon" not the power-domain itself.
> 
> > Do we actually need to add a new binding for this at all?
> > 
> > Cheers,
> > Conor.
> > 
> 
> Maybe this patch do that.
> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/

This makes it a child-node right? I think Rob already said no to that in
and earlier revision of this series. What he meant the other day was
making the syscon itself a power domain controller, since the child node
has no meaningful properties (reg, interrupts etc).

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

>>>>>>  
>>>>>>  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-aon-pmu
>>>
>>> I was speaking to Rob about this over the weekend, he asked:
>>> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
>>> itself?'
>>
>> Maybe not, this syscon only offset "0x00" configure power switch.
>> other offset configure other functions, maybe not power, so this
>> "starfive,jh7110-aon-syscon" not the power-domain itself.
>>
>>> Do we actually need to add a new binding for this at all?
>>>
>>> Cheers,
>>> Conor.
>>>
>>
>> Maybe this patch do that.
>> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
> 
> This makes it a child-node right? I think Rob already said no to that in
> and earlier revision of this series. What he meant the other day was
> making the syscon itself a power domain controller, since the child node
> has no meaningful properties (reg, interrupts etc).
> 
> Cheers,
> Conor.

Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
to a power domain controller. I think using the child-node description is closer to
JH7110 SoC. 

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>>>  
>>>>>>>  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-aon-pmu
>>>>
>>>> I was speaking to Rob about this over the weekend, he asked:
>>>> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
>>>> itself?'
>>>
>>> Maybe not, this syscon only offset "0x00" configure power switch.
>>> other offset configure other functions, maybe not power, so this
>>> "starfive,jh7110-aon-syscon" not the power-domain itself.
>>>
>>>> Do we actually need to add a new binding for this at all?
>>>>
>>>> Cheers,
>>>> Conor.
>>>>
>>>
>>> Maybe this patch do that.
>>> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
>>
>> This makes it a child-node right? I think Rob already said no to that in
>> and earlier revision of this series. What he meant the other day was
>> making the syscon itself a power domain controller, since the child node
>> has no meaningful properties (reg, interrupts etc).
>>
>> Cheers,
>> Conor.
> 
> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
> to a power domain controller. I think using the child-node description is closer to
> JH7110 SoC. 

Unfortunately, I do not see the correlation between these, any
connection. Why being a child of syscon block would mean that this
should no be power domain controller? Really, why? These are two
unrelated things.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>>>>  
>>>>>>>>  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-aon-pmu
>>>>>
>>>>> I was speaking to Rob about this over the weekend, he asked:
>>>>> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
>>>>> itself?'
>>>>
>>>> Maybe not, this syscon only offset "0x00" configure power switch.
>>>> other offset configure other functions, maybe not power, so this
>>>> "starfive,jh7110-aon-syscon" not the power-domain itself.
>>>>
>>>>> Do we actually need to add a new binding for this at all?
>>>>>
>>>>> Cheers,
>>>>> Conor.
>>>>>
>>>>
>>>> Maybe this patch do that.
>>>> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
>>>
>>> This makes it a child-node right? I think Rob already said no to that in
>>> and earlier revision of this series. What he meant the other day was
>>> making the syscon itself a power domain controller, since the child node
>>> has no meaningful properties (reg, interrupts etc).
>>>
>>> Cheers,
>>> Conor.
>>
>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>> to a power domain controller. I think using the child-node description is closer to
>> JH7110 SoC. 
> 
> Unfortunately, I do not see the correlation between these, any
> connection. Why being a child of syscon block would mean that this
> should no be power domain controller? Really, why? These are two
> unrelated things.
> 
> Best regards,
> Krzysztof
> 

Let me summarize what has been discussed above. 

There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
1. (0x17010000) is power-controller node:

	aon_pwrc: power-controller@17010000 {
		compatible = "starfive,jh7110-aon-pmu", "syscon";
		reg = <0x0 0x17010000 0x0 0x1000>;
		#power-domain-cells = <1>;
	};


2. (0x17010000) is syscon node, power-controller is child-node of syscon:

	aon_syscon: syscon@17010000 {
		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
		reg = <0x0 0x17010000 0x0 0x1000>;

		aon_pwrc: power-controller {
			compatible = "starfive,jh7110-aon-pmu";
			#power-domain-cells = <1>;
		};
	};

I prefer the way of 2.
This is more in line with the hardware description of JH7110.

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
> 
> 
> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
> > On 25/04/2023 09:57, Changhuang Liang wrote:
> >>>>>>>>  
> >>>>>>>>  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-aon-pmu
> >>>>>
> >>>>> I was speaking to Rob about this over the weekend, he asked:
> >>>>> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
> >>>>> itself?'
> >>>>
> >>>> Maybe not, this syscon only offset "0x00" configure power switch.
> >>>> other offset configure other functions, maybe not power, so this
> >>>> "starfive,jh7110-aon-syscon" not the power-domain itself.
> >>>>
> >>>>> Do we actually need to add a new binding for this at all?
> >>>>>
> >>>>> Cheers,
> >>>>> Conor.
> >>>>>
> >>>>
> >>>> Maybe this patch do that.
> >>>> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
> >>>
> >>> This makes it a child-node right? I think Rob already said no to that in
> >>> and earlier revision of this series. What he meant the other day was
> >>> making the syscon itself a power domain controller, since the child node
> >>> has no meaningful properties (reg, interrupts etc).
> >>>
> >>> Cheers,
> >>> Conor.
> >>
> >> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
> >> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
> >> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
> >> to a power domain controller. I think using the child-node description is closer to
> >> JH7110 SoC. 
> > 
> > Unfortunately, I do not see the correlation between these, any
> > connection. Why being a child of syscon block would mean that this
> > should no be power domain controller? Really, why? These are two
> > unrelated things.
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> Let me summarize what has been discussed above. 
> 
> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
> 1. (0x17010000) is power-controller node:
> 
> 	aon_pwrc: power-controller@17010000 {
> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
> 		reg = <0x0 0x17010000 0x0 0x1000>;
> 		#power-domain-cells = <1>;
> 	};
> 
> 
> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
> 
> 	aon_syscon: syscon@17010000 {
> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
> 		reg = <0x0 0x17010000 0x0 0x1000>;
> 
> 		aon_pwrc: power-controller {
> 			compatible = "starfive,jh7110-aon-pmu";
> 			#power-domain-cells = <1>;
> 		};
> 	};

I thought that Rob was suggesting something like this:
	aon_syscon: syscon@17010000 {
		compatible = "starfive,jh7110-aon-syscon", ...
		reg = <0x0 0x17010000 0x0 0x1000>;
		#power-domain-cells = <1>;
	};

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/25 17:35, Conor Dooley wrote:
> On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
>>
>>
>> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
>>> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>>>>>>  
>>>>>>>>>>  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-aon-pmu
>>>>>>>
>>>>>>> I was speaking to Rob about this over the weekend, he asked:
>>>>>>> 'Why isn't "starfive,jh7110-aon-syscon" just the power-domain provider
>>>>>>> itself?'
>>>>>>
>>>>>> Maybe not, this syscon only offset "0x00" configure power switch.
>>>>>> other offset configure other functions, maybe not power, so this
>>>>>> "starfive,jh7110-aon-syscon" not the power-domain itself.
>>>>>>
>>>>>>> Do we actually need to add a new binding for this at all?
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Conor.
>>>>>>>
>>>>>>
>>>>>> Maybe this patch do that.
>>>>>> https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
>>>>>
>>>>> This makes it a child-node right? I think Rob already said no to that in
>>>>> and earlier revision of this series. What he meant the other day was
>>>>> making the syscon itself a power domain controller, since the child node
>>>>> has no meaningful properties (reg, interrupts etc).
>>>>>
>>>>> Cheers,
>>>>> Conor.
>>>>
>>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>>>> to a power domain controller. I think using the child-node description is closer to
>>>> JH7110 SoC. 
>>>
>>> Unfortunately, I do not see the correlation between these, any
>>> connection. Why being a child of syscon block would mean that this
>>> should no be power domain controller? Really, why? These are two
>>> unrelated things.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Let me summarize what has been discussed above. 
>>
>> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
>> 1. (0x17010000) is power-controller node:
>>
>> 	aon_pwrc: power-controller@17010000 {
>> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>> 		#power-domain-cells = <1>;
>> 	};
>>
>>
>> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
>>
>> 	aon_syscon: syscon@17010000 {
>> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>
>> 		aon_pwrc: power-controller {
>> 			compatible = "starfive,jh7110-aon-pmu";
>> 			#power-domain-cells = <1>;
>> 		};
>> 	};
> 
> I thought that Rob was suggesting something like this:
> 	aon_syscon: syscon@17010000 {
> 		compatible = "starfive,jh7110-aon-syscon", ...
> 		reg = <0x0 0x17010000 0x0 0x1000>;
> 		#power-domain-cells = <1>;
> 	};
> 
> Cheers,
> Conor.
> 

I see the kernel:
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
this file line 42:
it's power-controller also has no meaningful properties.
What do you think?

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2498 bytes --]

On Tue, Apr 25, 2023 at 08:26:35PM +0800, Changhuang Liang wrote:
> On 2023/4/25 17:35, Conor Dooley wrote:
> > On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
> >> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
> >>> On 25/04/2023 09:57, Changhuang Liang wrote:
> >>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
> >>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
> >>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
> >>>> to a power domain controller. I think using the child-node description is closer to
> >>>> JH7110 SoC. 
> >>>
> >>> Unfortunately, I do not see the correlation between these, any
> >>> connection. Why being a child of syscon block would mean that this
> >>> should no be power domain controller? Really, why? These are two
> >>> unrelated things.
> >>
> >> Let me summarize what has been discussed above. 
> >>
> >> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
> >> 1. (0x17010000) is power-controller node:
> >>
> >> 	aon_pwrc: power-controller@17010000 {
> >> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
> >> 		reg = <0x0 0x17010000 0x0 0x1000>;
> >> 		#power-domain-cells = <1>;
> >> 	};
> >>
> >>
> >> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
> >>
> >> 	aon_syscon: syscon@17010000 {
> >> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
> >> 		reg = <0x0 0x17010000 0x0 0x1000>;
> >>
> >> 		aon_pwrc: power-controller {
> >> 			compatible = "starfive,jh7110-aon-pmu";
> >> 			#power-domain-cells = <1>;
> >> 		};
> >> 	};
> > 
> > I thought that Rob was suggesting something like this:
> > 	aon_syscon: syscon@17010000 {
> > 		compatible = "starfive,jh7110-aon-syscon", ...
> > 		reg = <0x0 0x17010000 0x0 0x1000>;
> > 		#power-domain-cells = <1>;
> > 	};

> I see the kernel:
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
> this file line 42:
> it's power-controller also has no meaningful properties.
> What do you think?

I'm not sure that I follow. It has a bunch of child-nodes does it not,
each of which is a domain?

I didn't see such domains in your dts patch, they're defined directly in
the driver instead AFAIU. Assuming I have understood that correctly,
your situation is different to that mediatek one?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/26 0:56, Conor Dooley wrote:
> On Tue, Apr 25, 2023 at 08:26:35PM +0800, Changhuang Liang wrote:
>> On 2023/4/25 17:35, Conor Dooley wrote:
>>> On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
>>>> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
>>>>> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>>>>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>>>>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>>>>>> to a power domain controller. I think using the child-node description is closer to
>>>>>> JH7110 SoC. 
>>>>>
>>>>> Unfortunately, I do not see the correlation between these, any
>>>>> connection. Why being a child of syscon block would mean that this
>>>>> should no be power domain controller? Really, why? These are two
>>>>> unrelated things.
>>>>
>>>> Let me summarize what has been discussed above. 
>>>>
>>>> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
>>>> 1. (0x17010000) is power-controller node:
>>>>
>>>> 	aon_pwrc: power-controller@17010000 {
>>>> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>> 		#power-domain-cells = <1>;
>>>> 	};
>>>>
>>>>
>>>> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
>>>>
>>>> 	aon_syscon: syscon@17010000 {
>>>> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>
>>>> 		aon_pwrc: power-controller {
>>>> 			compatible = "starfive,jh7110-aon-pmu";
>>>> 			#power-domain-cells = <1>;
>>>> 		};
>>>> 	};
>>>
>>> I thought that Rob was suggesting something like this:
>>> 	aon_syscon: syscon@17010000 {
>>> 		compatible = "starfive,jh7110-aon-syscon", ...
>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>> 		#power-domain-cells = <1>;
>>> 	};
> 
>> I see the kernel:
>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
>> this file line 42:
>> it's power-controller also has no meaningful properties.
>> What do you think?
> 
> I'm not sure that I follow. It has a bunch of child-nodes does it not,
> each of which is a domain?
> 
> I didn't see such domains in your dts patch, they're defined directly in
> the driver instead AFAIU. Assuming I have understood that correctly,
> your situation is different to that mediatek one?
> 
> Cheers,
> Conor.

I think there child-nodes just need to operate some clock signals. Maybe
we don't need to discuss other platforms.

If Rob's method is confirmed. I will try it next version.

Maybe like this:
aon_syscon: syscon@17010000 {
	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
	reg = <0x0 0x17010000 0x0 0x1000>;
	#power-domain-cells = <1>;
};

Rob and krzystof:

And I think patch[1][2] need to change. Right? 

[1] https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
[2] https://lore.kernel.org/all/20230414024157.53203-7-xingyu.wu@starfivetech.com/

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/26 0:56, Conor Dooley wrote:
> On Tue, Apr 25, 2023 at 08:26:35PM +0800, Changhuang Liang wrote:
>> On 2023/4/25 17:35, Conor Dooley wrote:
>>> On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
>>>> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
>>>>> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>>>>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>>>>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>>>>>> to a power domain controller. I think using the child-node description is closer to
>>>>>> JH7110 SoC. 
>>>>>
>>>>> Unfortunately, I do not see the correlation between these, any
>>>>> connection. Why being a child of syscon block would mean that this
>>>>> should no be power domain controller? Really, why? These are two
>>>>> unrelated things.
>>>>
>>>> Let me summarize what has been discussed above. 
>>>>
>>>> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
>>>> 1. (0x17010000) is power-controller node:
>>>>
>>>> 	aon_pwrc: power-controller@17010000 {
>>>> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>> 		#power-domain-cells = <1>;
>>>> 	};
>>>>
>>>>
>>>> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
>>>>
>>>> 	aon_syscon: syscon@17010000 {
>>>> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>
>>>> 		aon_pwrc: power-controller {
>>>> 			compatible = "starfive,jh7110-aon-pmu";
>>>> 			#power-domain-cells = <1>;
>>>> 		};
>>>> 	};
>>>
>>> I thought that Rob was suggesting something like this:
>>> 	aon_syscon: syscon@17010000 {
>>> 		compatible = "starfive,jh7110-aon-syscon", ...
>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>> 		#power-domain-cells = <1>;
>>> 	};
> 
>> I see the kernel:
>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
>> this file line 42:
>> it's power-controller also has no meaningful properties.
>> What do you think?
> 
> I'm not sure that I follow. It has a bunch of child-nodes does it not,
> each of which is a domain?
> 
> I didn't see such domains in your dts patch, they're defined directly in
> the driver instead AFAIU. Assuming I have understood that correctly,
> your situation is different to that mediatek one?
> 
> Cheers,
> Conor.

Conor and Rob, 

How about this way:

aon_syscon: syscon@17010000 {
	compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
	reg = <0x0 0x17010000 0x0 0x1000>;
	
	aon_pwrc: power-controller {
		compatible = "starfive,jh7110-aon-pmu";
		regmap = <&aon_syscon>;
		#power-domain-cells = <1>;
	};
};

Add a "regmap" property which is phandle. And it can keep the present child-node
structure. This is more consistent with our soc design.

Best regards,
Changhuang

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 04/05/2023 03:34, Changhuang Liang wrote:
> 
> 
> On 2023/4/26 0:56, Conor Dooley wrote:
>> On Tue, Apr 25, 2023 at 08:26:35PM +0800, Changhuang Liang wrote:
>>> On 2023/4/25 17:35, Conor Dooley wrote:
>>>> On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
>>>>> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
>>>>>> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>>>>>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>>>>>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>>>>>>> to a power domain controller. I think using the child-node description is closer to
>>>>>>> JH7110 SoC. 
>>>>>>
>>>>>> Unfortunately, I do not see the correlation between these, any
>>>>>> connection. Why being a child of syscon block would mean that this
>>>>>> should no be power domain controller? Really, why? These are two
>>>>>> unrelated things.
>>>>>
>>>>> Let me summarize what has been discussed above. 
>>>>>
>>>>> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
>>>>> 1. (0x17010000) is power-controller node:
>>>>>
>>>>> 	aon_pwrc: power-controller@17010000 {
>>>>> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
>>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>> 		#power-domain-cells = <1>;
>>>>> 	};
>>>>>
>>>>>
>>>>> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
>>>>>
>>>>> 	aon_syscon: syscon@17010000 {
>>>>> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>>
>>>>> 		aon_pwrc: power-controller {
>>>>> 			compatible = "starfive,jh7110-aon-pmu";
>>>>> 			#power-domain-cells = <1>;
>>>>> 		};
>>>>> 	};
>>>>
>>>> I thought that Rob was suggesting something like this:
>>>> 	aon_syscon: syscon@17010000 {
>>>> 		compatible = "starfive,jh7110-aon-syscon", ...
>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>> 		#power-domain-cells = <1>;
>>>> 	};
>>
>>> I see the kernel:
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
>>> this file line 42:
>>> it's power-controller also has no meaningful properties.
>>> What do you think?
>>
>> I'm not sure that I follow. It has a bunch of child-nodes does it not,
>> each of which is a domain?
>>
>> I didn't see such domains in your dts patch, they're defined directly in
>> the driver instead AFAIU. Assuming I have understood that correctly,
>> your situation is different to that mediatek one?
>>
>> Cheers,
>> Conor.
> 
> Conor and Rob, 
> 
> How about this way:
> 
> aon_syscon: syscon@17010000 {
> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
> 	reg = <0x0 0x17010000 0x0 0x1000>;
> 	
> 	aon_pwrc: power-controller {
> 		compatible = "starfive,jh7110-aon-pmu";
> 		regmap = <&aon_syscon>;
> 		#power-domain-cells = <1>;
> 	};
> };
> 
> Add a "regmap" property which is phandle. And it can keep the present child-node
> structure. This is more consistent with our soc design.

Adding property from child to parent does not make any sense. Didn't you
already receive comment on this?

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/4 14:13, Krzysztof Kozlowski wrote:
> On 04/05/2023 03:34, Changhuang Liang wrote:
>>
>>
>> On 2023/4/26 0:56, Conor Dooley wrote:
>>> On Tue, Apr 25, 2023 at 08:26:35PM +0800, Changhuang Liang wrote:
>>>> On 2023/4/25 17:35, Conor Dooley wrote:
>>>>> On Tue, Apr 25, 2023 at 05:18:10PM +0800, Changhuang Liang wrote:
>>>>>> On 2023/4/25 16:19, Krzysztof Kozlowski wrote:
>>>>>>> On 25/04/2023 09:57, Changhuang Liang wrote:
>>>>>>>> Yes, "starfive,jh7110-aon-pmu" is a child-node of "starfive,jh7110-aon-syscon".
>>>>>>>> In my opinion, "0x17010000" is "aon-syscon" on JH7110 SoC, and this "aon-pmu" is just 
>>>>>>>> a part of "aon-syscon" function, so I think it is inappropriate to make "aon-syscon"
>>>>>>>> to a power domain controller. I think using the child-node description is closer to
>>>>>>>> JH7110 SoC. 
>>>>>>>
>>>>>>> Unfortunately, I do not see the correlation between these, any
>>>>>>> connection. Why being a child of syscon block would mean that this
>>>>>>> should no be power domain controller? Really, why? These are two
>>>>>>> unrelated things.
>>>>>>
>>>>>> Let me summarize what has been discussed above. 
>>>>>>
>>>>>> There has two ways to describe this "starfive,jh7110-aon-syscon"(0x17010000).
>>>>>> 1. (0x17010000) is power-controller node:
>>>>>>
>>>>>> 	aon_pwrc: power-controller@17010000 {
>>>>>> 		compatible = "starfive,jh7110-aon-pmu", "syscon";
>>>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>>> 		#power-domain-cells = <1>;
>>>>>> 	};
>>>>>>
>>>>>>
>>>>>> 2. (0x17010000) is syscon node, power-controller is child-node of syscon:
>>>>>>
>>>>>> 	aon_syscon: syscon@17010000 {
>>>>>> 		compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>>>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>>>
>>>>>> 		aon_pwrc: power-controller {
>>>>>> 			compatible = "starfive,jh7110-aon-pmu";
>>>>>> 			#power-domain-cells = <1>;
>>>>>> 		};
>>>>>> 	};
>>>>>
>>>>> I thought that Rob was suggesting something like this:
>>>>> 	aon_syscon: syscon@17010000 {
>>>>> 		compatible = "starfive,jh7110-aon-syscon", ...
>>>>> 		reg = <0x0 0x17010000 0x0 0x1000>;
>>>>> 		#power-domain-cells = <1>;
>>>>> 	};
>>>
>>>> I see the kernel:
>>>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8167.dtsi
>>>> this file line 42:
>>>> it's power-controller also has no meaningful properties.
>>>> What do you think?
>>>
>>> I'm not sure that I follow. It has a bunch of child-nodes does it not,
>>> each of which is a domain?
>>>
>>> I didn't see such domains in your dts patch, they're defined directly in
>>> the driver instead AFAIU. Assuming I have understood that correctly,
>>> your situation is different to that mediatek one?
>>>
>>> Cheers,
>>> Conor.
>>
>> Conor and Rob, 
>>
>> How about this way:
>>
>> aon_syscon: syscon@17010000 {
>> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>> 	
>> 	aon_pwrc: power-controller {
>> 		compatible = "starfive,jh7110-aon-pmu";
>> 		regmap = <&aon_syscon>;
>> 		#power-domain-cells = <1>;
>> 	};
>> };
>>
>> Add a "regmap" property which is phandle. And it can keep the present child-node
>> structure. This is more consistent with our soc design.
> 
> Adding property from child to parent does not make any sense. Didn't you
> already receive comment on this?
> 
> Best regards,
> Krzysztof
> 

Krzysztof,

I am confused about what to do next. How to add this power-controller's
node in device tree?

Best regards,
Changhuang

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 04/05/2023 08:53, Changhuang Liang wrote:
>>> 	};
>>> };
>>>
>>> Add a "regmap" property which is phandle. And it can keep the present child-node
>>> structure. This is more consistent with our soc design.
>>
>> Adding property from child to parent does not make any sense. Didn't you
>> already receive comment on this?
>>
>> Best regards,
>> Krzysztof
>>
> 
> Krzysztof,
> 
> I am confused about what to do next. How to add this power-controller's
> node in device tree?
> 

You just move power-domain-cells up.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/4 15:04, Krzysztof Kozlowski wrote:
> On 04/05/2023 08:53, Changhuang Liang wrote:
>>>> 	};
>>>> };
>>>>
>>>> Add a "regmap" property which is phandle. And it can keep the present child-node
>>>> structure. This is more consistent with our soc design.
>>>
>>> Adding property from child to parent does not make any sense. Didn't you
>>> already receive comment on this?
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Krzysztof,
>>
>> I am confused about what to do next. How to add this power-controller's
>> node in device tree?
>>
> 
> You just move power-domain-cells up.
> 
> Best regards,
> Krzysztof
> 

Like this? 

aon_syscon: syscon@17010000 {
	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
	reg = <0x0 0x17010000 0x0 0x1000>;
	#power-domain-cells = <1>;
};

If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 04/05/2023 09:20, Changhuang Liang wrote:
> 
> 
> On 2023/5/4 15:04, Krzysztof Kozlowski wrote:
>> On 04/05/2023 08:53, Changhuang Liang wrote:
>>>>> 	};
>>>>> };
>>>>>
>>>>> Add a "regmap" property which is phandle. And it can keep the present child-node
>>>>> structure. This is more consistent with our soc design.
>>>>
>>>> Adding property from child to parent does not make any sense. Didn't you
>>>> already receive comment on this?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Krzysztof,
>>>
>>> I am confused about what to do next. How to add this power-controller's
>>> node in device tree?
>>>
>>
>> You just move power-domain-cells up.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Like this? 
> 
> aon_syscon: syscon@17010000 {
> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
> 	reg = <0x0 0x17010000 0x0 0x1000>;
> 	#power-domain-cells = <1>;
> };
> 
> If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.

Yes, but your compatibles are now wrong. Just compatible =
"starfive,jh7110-aon-syscon", "syscon".

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/4 15:26, Krzysztof Kozlowski wrote:
> On 04/05/2023 09:20, Changhuang Liang wrote:
>>>>
>>>> Krzysztof,
>>>>
>>>> I am confused about what to do next. How to add this power-controller's
>>>> node in device tree?
>>>>
>>>
>>> You just move power-domain-cells up.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Like this? 
>>
>> aon_syscon: syscon@17010000 {
>> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>> 	#power-domain-cells = <1>;
>> };
>>
>> If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.
> 
> Yes, but your compatibles are now wrong. Just compatible =
> "starfive,jh7110-aon-syscon", "syscon".
> 

If compatible = "starfive,jh7110-aon-syscon", "syscon". My pmu drivers need use 
"starfive,jh7110-aon-syscon" to match. And my pmu series will add this 
aon_syscon in yaml and device tree, so the syscon patch's owner don't need 
to add the aon_syscon in its yaml and device tree?

Best regards,
Changhuang

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 04/05/2023 10:43, Changhuang Liang wrote:
> 
> 
> On 2023/5/4 15:26, Krzysztof Kozlowski wrote:
>> On 04/05/2023 09:20, Changhuang Liang wrote:
>>>>>
>>>>> Krzysztof,
>>>>>
>>>>> I am confused about what to do next. How to add this power-controller's
>>>>> node in device tree?
>>>>>
>>>>
>>>> You just move power-domain-cells up.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Like this? 
>>>
>>> aon_syscon: syscon@17010000 {
>>> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
>>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>>> 	#power-domain-cells = <1>;
>>> };
>>>
>>> If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.
>>
>> Yes, but your compatibles are now wrong. Just compatible =
>> "starfive,jh7110-aon-syscon", "syscon".
>>
> 
> If compatible = "starfive,jh7110-aon-syscon", "syscon". My pmu drivers need use 
> "starfive,jh7110-aon-syscon" to match.

And how it would even work with your proposal
"starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu"?

Try...

>  And my pmu series will add this 
> aon_syscon in yaml and device tree, so the syscon patch's owner don't need 
> to add the aon_syscon in its yaml and device tree?

I don't understand. But if you need to drop syscon, sure, drop it.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/4 17:36, Krzysztof Kozlowski wrote:
> On 04/05/2023 10:43, Changhuang Liang wrote:
>>
>>
>> On 2023/5/4 15:26, Krzysztof Kozlowski wrote:
>>> On 04/05/2023 09:20, Changhuang Liang wrote:
>>>>>>
>>>>>> Krzysztof,
>>>>>>
>>>>>> I am confused about what to do next. How to add this power-controller's
>>>>>> node in device tree?
>>>>>>
>>>>>
>>>>> You just move power-domain-cells up.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Like this? 
>>>>
>>>> aon_syscon: syscon@17010000 {
>>>> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
>>>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>>>> 	#power-domain-cells = <1>;
>>>> };
>>>>
>>>> If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.
>>>
>>> Yes, but your compatibles are now wrong. Just compatible =
>>> "starfive,jh7110-aon-syscon", "syscon".
>>>
>>
>> If compatible = "starfive,jh7110-aon-syscon", "syscon". My pmu drivers need use 
>> "starfive,jh7110-aon-syscon" to match.
> 
> And how it would even work with your proposal
> "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu"?
> 
> Try...
> 
>>  And my pmu series will add this 
>> aon_syscon in yaml and device tree, so the syscon patch's owner don't need 
>> to add the aon_syscon in its yaml and device tree?
> 
> I don't understand. But if you need to drop syscon, sure, drop it.
> 

Yes, I think it can drop aon_syscon node in syscon patch series. And maybe my
compatible = "starfive,jh7110-aon-pmu", "syscon"; is better.

aon_syscon: syscon@17010000 {
	compatible = "starfive,jh7110-aon-pmu", "syscon";
	reg = <0x0 0x17010000 0x0 0x1000>;
	#power-domain-cells = <1>;
};

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

On Thu, May 04, 2023 at 05:48:20PM +0800, Changhuang Liang wrote:
> On 2023/5/4 17:36, Krzysztof Kozlowski wrote:
> > On 04/05/2023 10:43, Changhuang Liang wrote:

> >> On 2023/5/4 15:26, Krzysztof Kozlowski wrote:

> >>>> aon_syscon: syscon@17010000 {
> >>>> 	compatible = "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu";
> >>>> 	reg = <0x0 0x17010000 0x0 0x1000>;
> >>>> 	#power-domain-cells = <1>;
> >>>> };
> >>>>
> >>>> If right? I will tell the syscon patch's owner delete the "simple-mfd" in aon_syscon node.
> >>>
> >>> Yes, but your compatibles are now wrong. Just compatible =
> >>> "starfive,jh7110-aon-syscon", "syscon".
> >>>
> >>
> >> If compatible = "starfive,jh7110-aon-syscon", "syscon". My pmu drivers need use 
> >> "starfive,jh7110-aon-syscon" to match.
> > 
> > And how it would even work with your proposal
> > "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu"?
> > 
> > Try...
> > 
> >>  And my pmu series will add this 
> >> aon_syscon in yaml and device tree, so the syscon patch's owner don't need 
> >> to add the aon_syscon in its yaml and device tree?
> > 
> > I don't understand. But if you need to drop syscon, sure, drop it.
> > 
> 
> Yes, I think it can drop aon_syscon node in syscon patch series. And maybe my
> compatible = "starfive,jh7110-aon-pmu", "syscon"; is better.
> 
> aon_syscon: syscon@17010000 {
> 	compatible = "starfive,jh7110-aon-pmu", "syscon";

I don't really understand why you actually need to have this compatible.
Why not keep "starfive,jh7110-aon-syscon" & register the PMU using a
software mechanism?

> 	reg = <0x0 0x17010000 0x0 0x1000>;
> 	#power-domain-cells = <1>;
> };
> 
> Best regards,
> Krzysztof

^^^^^^^^^^^^^^
btw, your mailer is doing something odd with quotation.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/4 17:57, Conor Dooley wrote:
> On Thu, May 04, 2023 at 05:48:20PM +0800, Changhuang Liang wrote:
>> On 2023/5/4 17:36, Krzysztof Kozlowski wrote:
>>> On 04/05/2023 10:43, Changhuang Liang wrote:
> 
>>>> On 2023/5/4 15:26, Krzysztof Kozlowski wrote:
>>>>
>>>> If compatible = "starfive,jh7110-aon-syscon", "syscon". My pmu drivers need use 
>>>> "starfive,jh7110-aon-syscon" to match.
>>>
>>> And how it would even work with your proposal
>>> "starfive,jh7110-aon-syscon", "syscon", "starfive,jh7110-aon-pmu"?
>>>
>>> Try...
>>>
>>>>  And my pmu series will add this 
>>>> aon_syscon in yaml and device tree, so the syscon patch's owner don't need 
>>>> to add the aon_syscon in its yaml and device tree?
>>>
>>> I don't understand. But if you need to drop syscon, sure, drop it.
>>>
>>
>> Yes, I think it can drop aon_syscon node in syscon patch series. And maybe my
>> compatible = "starfive,jh7110-aon-pmu", "syscon"; is better.
>>
>> aon_syscon: syscon@17010000 {
>> 	compatible = "starfive,jh7110-aon-pmu", "syscon";
> 
> I don't really understand why you actually need to have this compatible.
> Why not keep "starfive,jh7110-aon-syscon" & register the PMU using a
> software mechanism?
> 

But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
it? Use this series dt-bindings or syscon series dt-bindings.

>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>> 	#power-domain-cells = <1>;
>> };
>>
>> Best regards,
>> Krzysztof
> 
> ^^^^^^^^^^^^^^
> btw, your mailer is doing something odd with quotation.
> 

OK, will pay attention to it.

> Cheers,
> Conor.

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Fri, May 05, 2023 at 09:29:15AM +0800, Changhuang Liang wrote:

> But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
> it? Use this series dt-bindings or syscon series dt-bindings.

There is no syscon series anymore, it's part of the PLL series now:
https://lore.kernel.org/linux-clk/20230414024157.53203-1-xingyu.wu@starfivetech.com/

I don't really care what you, Walker & Xingyu decide to do, but add the
binding in one series in a complete form. It's far less confusing to
have only have one version of the binding on the go at once.

Thanks,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/5 20:38, Conor Dooley wrote:
> On Fri, May 05, 2023 at 09:29:15AM +0800, Changhuang Liang wrote:
> 
>> But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
>> it? Use this series dt-bindings or syscon series dt-bindings.
> 
> There is no syscon series anymore, it's part of the PLL series now:
> https://lore.kernel.org/linux-clk/20230414024157.53203-1-xingyu.wu@starfivetech.com/
> 
> I don't really care what you, Walker & Xingyu decide to do, but add the
> binding in one series in a complete form. It's far less confusing to
> have only have one version of the binding on the go at once.
> 

Hi, Krzysztof and Conor

Due to the current aon pmu needs to be adjusted, it affects the syscon in PLL series.
So It's inevitable to change syscon in PLL series.

My current idea is PLL series don't add the aon_syscon node. I will add it in my
aon pmu series in next version like this:

aon_syscon: syscon@17010000 {
	compatible = "starfive,jh7110-aon-pmu", "syscon";
	reg = <0x0 0x17010000 0x0 0x1000>;
	#power-domain-cells = <1>;
};

In my opinion, the first we add "starfive,jh7110-aon-syscon" because "syscon" can 
not appear alone in the compatible. If we have "starfive,jh7110-aon-pmu", this
"starfive,jh7110-aon-syscon" is not a must-be need.

Do you agree with doing so.

Thanks,
Changhuang

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 06/05/2023 03:45, Changhuang Liang wrote:
> 
> 
> On 2023/5/5 20:38, Conor Dooley wrote:
>> On Fri, May 05, 2023 at 09:29:15AM +0800, Changhuang Liang wrote:
>>
>>> But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
>>> it? Use this series dt-bindings or syscon series dt-bindings.
>>
>> There is no syscon series anymore, it's part of the PLL series now:
>> https://lore.kernel.org/linux-clk/20230414024157.53203-1-xingyu.wu@starfivetech.com/
>>
>> I don't really care what you, Walker & Xingyu decide to do, but add the
>> binding in one series in a complete form. It's far less confusing to
>> have only have one version of the binding on the go at once.
>>
> 
> Hi, Krzysztof and Conor
> 
> Due to the current aon pmu needs to be adjusted, it affects the syscon in PLL series.
> So It's inevitable to change syscon in PLL series.
> 
> My current idea is PLL series don't add the aon_syscon node. I will add it in my
> aon pmu series in next version like this:
> 
> aon_syscon: syscon@17010000 {
> 	compatible = "starfive,jh7110-aon-pmu", "syscon";
> 	reg = <0x0 0x17010000 0x0 0x1000>;
> 	#power-domain-cells = <1>;
> };
> 
> In my opinion, the first we add "starfive,jh7110-aon-syscon" because "syscon" can 
> not appear alone in the compatible. If we have "starfive,jh7110-aon-pmu", this
> "starfive,jh7110-aon-syscon" is not a must-be need.
> 
> Do you agree with doing so.

Sorry guys, I don't know what you talk about. I have no clue what are
PLL and aon series. More over I don't understand what is complicated
here... all SoCs follow the same rules and similar way of development.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/6 14:31, Krzysztof Kozlowski wrote:
> On 06/05/2023 03:45, Changhuang Liang wrote:
>>
>> Hi, Krzysztof and Conor
>>
>> Due to the current aon pmu needs to be adjusted, it affects the syscon in PLL series.
>> So It's inevitable to change syscon in PLL series.
>>
>> My current idea is PLL series don't add the aon_syscon node. I will add it in my
>> aon pmu series in next version like this:
>>
>> aon_syscon: syscon@17010000 {
>> 	compatible = "starfive,jh7110-aon-pmu", "syscon";
>> 	reg = <0x0 0x17010000 0x0 0x1000>;
>> 	#power-domain-cells = <1>;
>> };
>>
>> In my opinion, the first we add "starfive,jh7110-aon-syscon" because "syscon" can 
>> not appear alone in the compatible. If we have "starfive,jh7110-aon-pmu", this
>> "starfive,jh7110-aon-syscon" is not a must-be need.
>>
>> Do you agree with doing so.
> 
> Sorry guys, I don't know what you talk about. I have no clue what are
> PLL and aon series. More over I don't understand what is complicated
> here... all SoCs follow the same rules and similar way of development.
> 

In other words, if I use the above approach, 
[1] https://lore.kernel.org/all/20230414024157.53203-6-xingyu.wu@starfivetech.com/
[2] https://lore.kernel.org/all/20230414024157.53203-7-xingyu.wu@starfivetech.com/
Links [1][2] need to be dropped "aon_syscon" node.

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Sat, May 06, 2023 at 09:45:07AM +0800, Changhuang Liang wrote:
> 
> 
> On 2023/5/5 20:38, Conor Dooley wrote:
> > On Fri, May 05, 2023 at 09:29:15AM +0800, Changhuang Liang wrote:
> > 
> >> But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
> >> it? Use this series dt-bindings or syscon series dt-bindings.
> > 
> > There is no syscon series anymore, it's part of the PLL series now:
> > https://lore.kernel.org/linux-clk/20230414024157.53203-1-xingyu.wu@starfivetech.com/
> > 
> > I don't really care what you, Walker & Xingyu decide to do, but add the
> > binding in one series in a complete form. It's far less confusing to
> > have only have one version of the binding on the go at once.
> > 
> 
> Due to the current aon pmu needs to be adjusted, it affects the syscon in PLL series.
> So It's inevitable to change syscon in PLL series.
> 
> My current idea is PLL series don't add the aon_syscon node. I will add it in my
> aon pmu series in next version

That's fine. Rob was happy with the clock related parts, which was the
original source of confusion there.

> like this:
> 
> aon_syscon: syscon@17010000 {
> 	compatible = "starfive,jh7110-aon-pmu", "syscon";

The syscon does a bunch of things of which one is a pmu. I don't see a
reason to name this other than "starfive,jh100-aon-syscon".

> 	reg = <0x0 0x17010000 0x0 0x1000>;
> 	#power-domain-cells = <1>;
> };
> 
> In my opinion, the first we add "starfive,jh7110-aon-syscon" because "syscon" can 
> not appear alone in the compatible. If we have "starfive,jh7110-aon-pmu", this
> "starfive,jh7110-aon-syscon" is not a must-be need.
> 
> Do you agree with doing so.
> 
> Thanks,
> Changhuang

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/6 18:17, Conor Dooley wrote:
> On Sat, May 06, 2023 at 09:45:07AM +0800, Changhuang Liang wrote:
>>
>>
>> On 2023/5/5 20:38, Conor Dooley wrote:
>>> On Fri, May 05, 2023 at 09:29:15AM +0800, Changhuang Liang wrote:
>>>
>>>> But if keep this "starfive,jh7110-aon-syscon" compatible. Which .yaml match to
>>>> it? Use this series dt-bindings or syscon series dt-bindings.
>>>
>>> There is no syscon series anymore, it's part of the PLL series now:
>>> https://lore.kernel.org/linux-clk/20230414024157.53203-1-xingyu.wu@starfivetech.com/
>>>
>>> I don't really care what you, Walker & Xingyu decide to do, but add the
>>> binding in one series in a complete form. It's far less confusing to
>>> have only have one version of the binding on the go at once.
>>>
>>
>> Due to the current aon pmu needs to be adjusted, it affects the syscon in PLL series.
>> So It's inevitable to change syscon in PLL series.
>>
>> My current idea is PLL series don't add the aon_syscon node. I will add it in my
>> aon pmu series in next version
> 
> That's fine. Rob was happy with the clock related parts, which was the
> original source of confusion there.
> 
>> like this:
>>
>> aon_syscon: syscon@17010000 {
>> 	compatible = "starfive,jh7110-aon-pmu", "syscon";
> 
> The syscon does a bunch of things of which one is a pmu. I don't see a
> reason to name this other than "starfive,jh100-aon-syscon".
> 

OK, will replace "starfive,jh7110-aon-pmu" with "starfive,jh100-aon-syscon" in this series.

Thanks,
Changhuang

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

On Sat, May 06, 2023 at 08:26:01PM +0800, Changhuang Liang wrote:

> OK, will replace "starfive,jh7110-aon-pmu" with "starfive,jh100-aon-syscon" in this series.
                                                            ^^^^^
Just make sure you don't propagate my typo here in the process.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/5/6 20:29, Conor Dooley wrote:
> On Sat, May 06, 2023 at 08:26:01PM +0800, Changhuang Liang wrote:
> 
>> OK, will replace "starfive,jh7110-aon-pmu" with "starfive,jh100-aon-syscon" in this series.
>                                                             ^^^^^
> Just make sure you don't propagate my typo here in the process.
> 

Yes, "starfive,jh7110-aon-syscon"

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Krzysztof Kozlowski]

On 19/04/2023 05:56, Changhuang Liang wrote:
> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
> rx/tx power switch, and it don't need the properties of reg and
> interrupts.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
>  include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++

I have impression I just reviewed your v2... Apply the comments,
reviews/acks etc from that email.

Best regards,
Krzysztof

Re: [RESEND v2 1/6] dt-bindings: power: Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/20 3:46, Krzysztof Kozlowski wrote:
> On 19/04/2023 05:56, Changhuang Liang wrote:
>> Add AON PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY
>> rx/tx power switch, and it don't need the properties of reg and
>> interrupts.
>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> ---
>>  .../bindings/power/starfive,jh7110-pmu.yaml       | 15 +++++++++++++--
>>  include/dt-bindings/power/starfive,jh7110-pmu.h   |  3 +++
> 
> I have impression I just reviewed your v2... Apply the comments,
> reviews/acks etc from that email.
> 
> Best regards,
> Krzysztof
> 

Yes, thanks for you support!

[RESEND v2 2/6] soc: starfive: Replace SOC_STARFIVE with ARCH_STARFIVE

[Changhuang Liang]

Using ARCH_FOO symbol is preferred than SOC_FOO.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Walker Chen <walker.chen@starfivetech.com>
Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/soc/starfive/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
index bdb96dc4c989..1e9b0c414fec 100644
--- a/drivers/soc/starfive/Kconfig
+++ b/drivers/soc/starfive/Kconfig
@@ -3,8 +3,8 @@
 config JH71XX_PMU
 	bool "Support PMU for StarFive JH71XX Soc"
 	depends on PM
-	depends on SOC_STARFIVE || COMPILE_TEST
-	default SOC_STARFIVE
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	default ARCH_STARFIVE
 	select PM_GENERIC_DOMAINS
 	help
 	  Say 'y' here to enable support power domain support.
-- 
2.25.1

[RESEND v2 3/6] soc: starfive: Modify ioremap to regmap

[Changhuang Liang]

Modify ioremap to regmap which can be compatible with the syscon
interface, such as:
struct regmap *syscon_node_to_regmap(struct device_node *np)
Convenient introduction of syscon operation.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/soc/starfive/jh71xx_pmu.c | 43 +++++++++++++++++--------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
index 7d5f50d71c0d..306218c83691 100644
--- a/drivers/soc/starfive/jh71xx_pmu.c
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -6,13 +6,13 @@
  */
 
 #include <linux/interrupt.h>
-#include <linux/io.h>
-#include <linux/iopoll.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
+#include <linux/regmap.h>
 #include <dt-bindings/power/starfive,jh7110-pmu.h>
 
 /* register offset */
@@ -59,7 +59,7 @@ struct jh71xx_pmu_match_data {
 struct jh71xx_pmu {
 	struct device *dev;
 	const struct jh71xx_pmu_match_data *match_data;
-	void __iomem *base;
+	struct regmap *base;
 	struct generic_pm_domain **genpd;
 	struct genpd_onecell_data genpd_data;
 	int irq;
@@ -75,11 +75,14 @@ struct jh71xx_pmu_dev {
 static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
 {
 	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned int val;
 
 	if (!mask)
 		return -EINVAL;
 
-	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
+	regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val);
+
+	*is_on = val & mask;
 
 	return 0;
 }
@@ -130,7 +133,7 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
 	}
 
-	writel(mask, pmu->base + mode);
+	regmap_write(pmu->base, mode, mask);
 
 	/*
 	 * 2.Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
@@ -140,21 +143,21 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	 *   Then write the lower bits of the command sequence, followed by the upper
 	 *   bits. The sequence differs between powering on & off a domain.
 	 */
-	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
-	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
-	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
+	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, JH71XX_PMU_SW_ENCOURAGE_ON);
+	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_lo);
+	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_hi);
 
 	spin_unlock_irqrestore(&pmu->lock, flags);
 
 	/* Wait for the power domain bit to be enabled / disabled */
 	if (on) {
-		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
-						val, val & mask,
-						1, JH71XX_PMU_TIMEOUT_US);
+		ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE,
+						      val, val & mask,
+						      1, JH71XX_PMU_TIMEOUT_US);
 	} else {
-		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
-						val, !(val & mask),
-						1, JH71XX_PMU_TIMEOUT_US);
+		ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE,
+						      val, !(val & mask),
+						      1, JH71XX_PMU_TIMEOUT_US);
 	}
 
 	if (ret) {
@@ -190,14 +193,14 @@ static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable)
 	unsigned long flags;
 
 	spin_lock_irqsave(&pmu->lock, flags);
-	val = readl(pmu->base + JH71XX_PMU_TIMER_INT_MASK);
+	regmap_read(pmu->base, JH71XX_PMU_TIMER_INT_MASK, &val);
 
 	if (enable)
 		val &= ~mask;
 	else
 		val |= mask;
 
-	writel(val, pmu->base + JH71XX_PMU_TIMER_INT_MASK);
+	regmap_write(pmu->base, JH71XX_PMU_TIMER_INT_MASK, val);
 	spin_unlock_irqrestore(&pmu->lock, flags);
 }
 
@@ -206,7 +209,7 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
 	struct jh71xx_pmu *pmu = data;
 	u32 val;
 
-	val = readl(pmu->base + JH71XX_PMU_INT_STATUS);
+	regmap_read(pmu->base, JH71XX_PMU_INT_STATUS, &val);
 
 	if (val & JH71XX_PMU_INT_SEQ_DONE)
 		dev_dbg(pmu->dev, "sequence done.\n");
@@ -220,8 +223,8 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
 		dev_err(pmu->dev, "p-channel fail event.\n");
 
 	/* clear interrupts */
-	writel(val, pmu->base + JH71XX_PMU_INT_STATUS);
-	writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS);
+	regmap_write(pmu->base, JH71XX_PMU_INT_STATUS, val);
+	regmap_write(pmu->base, JH71XX_PMU_EVENT_STATUS, val);
 
 	return IRQ_HANDLED;
 }
@@ -271,7 +274,7 @@ static int jh71xx_pmu_probe(struct platform_device *pdev)
 	if (!pmu)
 		return -ENOMEM;
 
-	pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	pmu->base = device_node_to_regmap(np);
 	if (IS_ERR(pmu->base))
 		return PTR_ERR(pmu->base);
 
-- 
2.25.1

Re: [RESEND v2 3/6] soc: starfive: Modify ioremap to regmap

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5550 bytes --]

On Tue, Apr 18, 2023 at 08:56:43PM -0700, Changhuang Liang wrote:
> Modify ioremap to regmap which can be compatible with the syscon
> interface, such as:
> struct regmap *syscon_node_to_regmap(struct device_node *np)
> Convenient introduction of syscon operation.

What's missing here is an explanation of *why* you are making this
compatible with the syscon interface. Provided everything else is fine,
I can fix this when applying the series.

Thanks,
Conor.

> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  drivers/soc/starfive/jh71xx_pmu.c | 43 +++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> index 7d5f50d71c0d..306218c83691 100644
> --- a/drivers/soc/starfive/jh71xx_pmu.c
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -6,13 +6,13 @@
>   */
>  
>  #include <linux/interrupt.h>
> -#include <linux/io.h>
> -#include <linux/iopoll.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> +#include <linux/regmap.h>
>  #include <dt-bindings/power/starfive,jh7110-pmu.h>
>  
>  /* register offset */
> @@ -59,7 +59,7 @@ struct jh71xx_pmu_match_data {
>  struct jh71xx_pmu {
>  	struct device *dev;
>  	const struct jh71xx_pmu_match_data *match_data;
> -	void __iomem *base;
> +	struct regmap *base;
>  	struct generic_pm_domain **genpd;
>  	struct genpd_onecell_data genpd_data;
>  	int irq;
> @@ -75,11 +75,14 @@ struct jh71xx_pmu_dev {
>  static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>  {
>  	struct jh71xx_pmu *pmu = pmd->pmu;
> +	unsigned int val;
>  
>  	if (!mask)
>  		return -EINVAL;
>  
> -	*is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
> +	regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val);
> +
> +	*is_on = val & mask;
>  
>  	return 0;
>  }
> @@ -130,7 +133,7 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>  		encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>  	}
>  
> -	writel(mask, pmu->base + mode);
> +	regmap_write(pmu->base, mode, mask);
>  
>  	/*
>  	 * 2.Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
> @@ -140,21 +143,21 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>  	 *   Then write the lower bits of the command sequence, followed by the upper
>  	 *   bits. The sequence differs between powering on & off a domain.
>  	 */
> -	writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> -	writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> -	writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
> +	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, JH71XX_PMU_SW_ENCOURAGE_ON);
> +	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_lo);
> +	regmap_write(pmu->base, JH71XX_PMU_SW_ENCOURAGE, encourage_hi);
>  
>  	spin_unlock_irqrestore(&pmu->lock, flags);
>  
>  	/* Wait for the power domain bit to be enabled / disabled */
>  	if (on) {
> -		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> -						val, val & mask,
> -						1, JH71XX_PMU_TIMEOUT_US);
> +		ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE,
> +						      val, val & mask,
> +						      1, JH71XX_PMU_TIMEOUT_US);
>  	} else {
> -		ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
> -						val, !(val & mask),
> -						1, JH71XX_PMU_TIMEOUT_US);
> +		ret = regmap_read_poll_timeout_atomic(pmu->base, JH71XX_PMU_CURR_POWER_MODE,
> +						      val, !(val & mask),
> +						      1, JH71XX_PMU_TIMEOUT_US);
>  	}
>  
>  	if (ret) {
> @@ -190,14 +193,14 @@ static void jh71xx_pmu_int_enable(struct jh71xx_pmu *pmu, u32 mask, bool enable)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&pmu->lock, flags);
> -	val = readl(pmu->base + JH71XX_PMU_TIMER_INT_MASK);
> +	regmap_read(pmu->base, JH71XX_PMU_TIMER_INT_MASK, &val);
>  
>  	if (enable)
>  		val &= ~mask;
>  	else
>  		val |= mask;
>  
> -	writel(val, pmu->base + JH71XX_PMU_TIMER_INT_MASK);
> +	regmap_write(pmu->base, JH71XX_PMU_TIMER_INT_MASK, val);
>  	spin_unlock_irqrestore(&pmu->lock, flags);
>  }
>  
> @@ -206,7 +209,7 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
>  	struct jh71xx_pmu *pmu = data;
>  	u32 val;
>  
> -	val = readl(pmu->base + JH71XX_PMU_INT_STATUS);
> +	regmap_read(pmu->base, JH71XX_PMU_INT_STATUS, &val);
>  
>  	if (val & JH71XX_PMU_INT_SEQ_DONE)
>  		dev_dbg(pmu->dev, "sequence done.\n");
> @@ -220,8 +223,8 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
>  		dev_err(pmu->dev, "p-channel fail event.\n");
>  
>  	/* clear interrupts */
> -	writel(val, pmu->base + JH71XX_PMU_INT_STATUS);
> -	writel(val, pmu->base + JH71XX_PMU_EVENT_STATUS);
> +	regmap_write(pmu->base, JH71XX_PMU_INT_STATUS, val);
> +	regmap_write(pmu->base, JH71XX_PMU_EVENT_STATUS, val);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -271,7 +274,7 @@ static int jh71xx_pmu_probe(struct platform_device *pdev)
>  	if (!pmu)
>  		return -ENOMEM;
>  
> -	pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	pmu->base = device_node_to_regmap(np);
>  	if (IS_ERR(pmu->base))
>  		return PTR_ERR(pmu->base);
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 3/6] soc: starfive: Modify ioremap to regmap

[Changhuang Liang]

On 2023/4/20 1:29, Conor Dooley wrote:
> On Tue, Apr 18, 2023 at 08:56:43PM -0700, Changhuang Liang wrote:
>> Modify ioremap to regmap which can be compatible with the syscon
>> interface, such as:
>> struct regmap *syscon_node_to_regmap(struct device_node *np)
>> Convenient introduction of syscon operation.
> 
> What's missing here is an explanation of *why* you are making this
> compatible with the syscon interface. Provided everything else is fine,
> I can fix this when applying the series.
> 
> Thanks,
> Conor.
> 

Add this patch just for using the same member(struct regmap *base) in patch 5.

Maybe I should replace syscon_node_to_regmap() with of_iomap() in patch 5.
So that patch 5 will use (void __iomem *base) member, and I also can drop
this patch 3 in this series. 

struct regmap *syscon_node_to_regmap(struct device_node *np);
void __iomem *of_iomap(struct device_node *node, int index);

[RESEND v2 4/6] soc: starfive: Extract JH7110 pmu private operations

[Changhuang Liang]

Move JH7110 private operation into private data of compatible.
Convenient to expand different compatible.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/soc/starfive/jh71xx_pmu.c | 98 +++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 31 deletions(-)

diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
index 306218c83691..bb44cc93e822 100644
--- a/drivers/soc/starfive/jh71xx_pmu.c
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -51,9 +51,17 @@ struct jh71xx_domain_info {
 	u8 bit;
 };
 
+struct jh71xx_pmu;
+struct jh71xx_pmu_dev;
+
 struct jh71xx_pmu_match_data {
 	const struct jh71xx_domain_info *domain_info;
 	int num_domains;
+	unsigned int pmu_status;
+	int (*pmu_parse_dt)(struct platform_device *pdev,
+			    struct jh71xx_pmu *pmu);
+	int (*pmu_set_state)(struct jh71xx_pmu_dev *pmd,
+			     u32 mask, bool on);
 };
 
 struct jh71xx_pmu {
@@ -80,14 +88,14 @@ static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_o
 	if (!mask)
 		return -EINVAL;
 
-	regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val);
+	regmap_read(pmu->base, pmu->match_data->pmu_status, &val);
 
 	*is_on = val & mask;
 
 	return 0;
 }
 
-static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+static int jh7110_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 {
 	struct jh71xx_pmu *pmu = pmd->pmu;
 	unsigned long flags;
@@ -95,22 +103,8 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	u32 mode;
 	u32 encourage_lo;
 	u32 encourage_hi;
-	bool is_on;
 	int ret;
 
-	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
-	if (ret) {
-		dev_dbg(pmu->dev, "unable to get current state for %s\n",
-			pmd->genpd.name);
-		return ret;
-	}
-
-	if (is_on == on) {
-		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
-			pmd->genpd.name, on ? "en" : "dis");
-		return 0;
-	}
-
 	spin_lock_irqsave(&pmu->lock, flags);
 
 	/*
@@ -169,6 +163,29 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	return 0;
 }
 
+static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	const struct jh71xx_pmu_match_data *match_data = pmu->match_data;
+	bool is_on;
+	int ret;
+
+	ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
+	if (ret) {
+		dev_dbg(pmu->dev, "unable to get current state for %s\n",
+			pmd->genpd.name);
+		return ret;
+	}
+
+	if (is_on == on) {
+		dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
+			pmd->genpd.name, on ? "en" : "dis");
+		return 0;
+	}
+
+	return match_data->pmu_set_state(pmd, mask, on);
+}
+
 static int jh71xx_pmu_on(struct generic_pm_domain *genpd)
 {
 	struct jh71xx_pmu_dev *pmd = container_of(genpd,
@@ -229,6 +246,30 @@ static irqreturn_t jh71xx_pmu_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int jh7110_pmu_parse_dt(struct platform_device *pdev, struct jh71xx_pmu *pmu)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	pmu->base = device_node_to_regmap(np);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	pmu->irq = platform_get_irq(pdev, 0);
+	if (pmu->irq < 0)
+		return pmu->irq;
+
+	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
+			       0, pdev->name, pmu);
+	if (ret)
+		dev_err(dev, "failed to request irq\n");
+
+	jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
+
+	return 0;
+}
+
 static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
 {
 	struct jh71xx_pmu_dev *pmd;
@@ -274,23 +315,18 @@ static int jh71xx_pmu_probe(struct platform_device *pdev)
 	if (!pmu)
 		return -ENOMEM;
 
-	pmu->base = device_node_to_regmap(np);
-	if (IS_ERR(pmu->base))
-		return PTR_ERR(pmu->base);
-
-	pmu->irq = platform_get_irq(pdev, 0);
-	if (pmu->irq < 0)
-		return pmu->irq;
-
-	ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
-			       0, pdev->name, pmu);
-	if (ret)
-		dev_err(dev, "failed to request irq\n");
+	spin_lock_init(&pmu->lock);
 
 	match_data = of_device_get_match_data(dev);
 	if (!match_data)
 		return -EINVAL;
 
+	ret = match_data->pmu_parse_dt(pdev, pmu);
+	if (ret) {
+		dev_err(dev, "failed to parse dt\n");
+		return ret;
+	}
+
 	pmu->genpd = devm_kcalloc(dev, match_data->num_domains,
 				  sizeof(struct generic_pm_domain *),
 				  GFP_KERNEL);
@@ -310,9 +346,6 @@ static int jh71xx_pmu_probe(struct platform_device *pdev)
 		}
 	}
 
-	spin_lock_init(&pmu->lock);
-	jh71xx_pmu_int_enable(pmu, JH71XX_PMU_INT_ALL_MASK & ~JH71XX_PMU_INT_PCH_FAIL, true);
-
 	ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
 	if (ret) {
 		dev_err(dev, "failed to register genpd driver: %d\n", ret);
@@ -360,6 +393,9 @@ static const struct jh71xx_domain_info jh7110_power_domains[] = {
 static const struct jh71xx_pmu_match_data jh7110_pmu = {
 	.num_domains = ARRAY_SIZE(jh7110_power_domains),
 	.domain_info = jh7110_power_domains,
+	.pmu_status = JH71XX_PMU_CURR_POWER_MODE,
+	.pmu_parse_dt = jh7110_pmu_parse_dt,
+	.pmu_set_state = jh7110_pmu_set_state,
 };
 
 static const struct of_device_id jh71xx_pmu_of_match[] = {
-- 
2.25.1

Re: [RESEND v2 4/6] soc: starfive: Extract JH7110 pmu private operations

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
> Move JH7110 private operation into private data of compatible.
> Convenient to expand different compatible.

I prefer how the code looks in v2, thanks.
However, just as in the prior patch, "Convenient to expand different
compatible" isn't really a justification - specifically, supporting the
power domain controller serving the dphy is your motivation here. The
important difference being that it uses a regmap from a syscon and has
no interrupts nor the encourage features.

Although, given the only real similarity the code driving each of the
PMUs is the variable names, I guess you could argue that this driver
should be left alone and the "aon dphy" should be a different driver
altogether.

I don't have a strong opinion though & if it's fine with Walker and
noone else objects, it's fine with me...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 4/6] soc: starfive: Extract JH7110 pmu private operations

[Changhuang Liang]

On 2023/4/20 1:47, Conor Dooley wrote:
> On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
>> Move JH7110 private operation into private data of compatible.
>> Convenient to expand different compatible.
> 
> I prefer how the code looks in v2, thanks.
> However, just as in the prior patch, "Convenient to expand different
> compatible" isn't really a justification - specifically, supporting the
> power domain controller serving the dphy is your motivation here. The
> important difference being that it uses a regmap from a syscon and has
> no interrupts nor the encourage features.
> 

So should I expand the commit message which called "in order to add the 
aon power domain" although the patch is applied behind current patch.

> Although, given the only real similarity the code driving each of the
> PMUs is the variable names, I guess you could argue that this driver
> should be left alone and the "aon dphy" should be a different driver
> altogether.
> 

I have tried independent this aon pmu, but it code is very similar to the
original pmu, so I think they can put together, reduce linux kernel bloat.

> I don't have a strong opinion though & if it's fine with Walker and
> noone else objects, it's fine with me...

Re: [RESEND v2 4/6] soc: starfive: Extract JH7110 pmu private operations

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Fri, Apr 21, 2023 at 11:27:52AM +0800, Changhuang Liang wrote:
> On 2023/4/20 1:47, Conor Dooley wrote:
> > On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
> >> Move JH7110 private operation into private data of compatible.
> >> Convenient to expand different compatible.
> > 
> > I prefer how the code looks in v2, thanks.
> > However, just as in the prior patch, "Convenient to expand different
> > compatible" isn't really a justification - specifically, supporting the
> > power domain controller serving the dphy is your motivation here. The
> > important difference being that it uses a regmap from a syscon and has
> > no interrupts nor the encourage features.
> > 
> 
> So should I expand the commit message which called "in order to add the 
> aon power domain" although the patch is applied behind current patch.

Only if you have to resend for some other reason. If there is no other
reason to resend then I will do this when I apply the patch.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 4/6] soc: starfive: Extract JH7110 pmu private operations

[Changhuang Liang]

On 2023/4/21 14:57, Conor Dooley wrote:
> On Fri, Apr 21, 2023 at 11:27:52AM +0800, Changhuang Liang wrote:
>> On 2023/4/20 1:47, Conor Dooley wrote:
>>> On Tue, Apr 18, 2023 at 08:56:44PM -0700, Changhuang Liang wrote:
>>>> Move JH7110 private operation into private data of compatible.
>>>> Convenient to expand different compatible.
>>>
>>> I prefer how the code looks in v2, thanks.
>>> However, just as in the prior patch, "Convenient to expand different
>>> compatible" isn't really a justification - specifically, supporting the
>>> power domain controller serving the dphy is your motivation here. The
>>> important difference being that it uses a regmap from a syscon and has
>>> no interrupts nor the encourage features.
>>>
>>
>> So should I expand the commit message which called "in order to add the 
>> aon power domain" although the patch is applied behind current patch.
> 
> Only if you have to resend for some other reason. If there is no other
> reason to resend then I will do this when I apply the patch.
> 
> Thanks,
> Conor.

OK, thanks for your support.

[RESEND v2 5/6] soc: starfive: Add JH7110 AON PMU support

[Changhuang Liang]

Add AON PMU for StarFive JH7110 SoC. It can be used to turn on/off the
dphy rx/tx power switch.

Reviewed-by: Walker Chen <walker.chen@starfivetech.com>
Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 MAINTAINERS                       |  1 +
 drivers/soc/starfive/jh71xx_pmu.c | 63 ++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fafeea8ebdb..8f32d43a9b67 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19950,6 +19950,7 @@ F:	include/dt-bindings/reset/starfive?jh71*.h
 
 STARFIVE JH71XX PMU CONTROLLER DRIVER
 M:	Walker Chen <walker.chen@starfivetech.com>
+M:	Changhuang Liang <changhuang.liang@starfivetech.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/power/starfive*
 F:	drivers/soc/starfive/jh71xx_pmu.c
diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
index bb44cc93e822..1303826aa7b5 100644
--- a/drivers/soc/starfive/jh71xx_pmu.c
+++ b/drivers/soc/starfive/jh71xx_pmu.c
@@ -2,7 +2,7 @@
 /*
  * StarFive JH71XX PMU (Power Management Unit) Controller Driver
  *
- * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
  */
 
 #include <linux/interrupt.h>
@@ -24,6 +24,9 @@
 #define JH71XX_PMU_EVENT_STATUS		0x88
 #define JH71XX_PMU_INT_STATUS		0x8C
 
+/* aon pmu register offset */
+#define JH71XX_AON_PMU_SWITCH		0x00
+
 /* sw encourage cfg */
 #define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
 #define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
@@ -163,6 +166,23 @@ static int jh7110_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 	return 0;
 }
 
+static int jh7110_aon_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
+{
+	struct jh71xx_pmu *pmu = pmd->pmu;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pmu->lock, flags);
+
+	if (on)
+		regmap_update_bits(pmu->base, JH71XX_AON_PMU_SWITCH, mask, mask);
+	else
+		regmap_update_bits(pmu->base, JH71XX_AON_PMU_SWITCH, mask, 0);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
+
+	return 0;
+}
+
 static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
 {
 	struct jh71xx_pmu *pmu = pmd->pmu;
@@ -270,6 +290,24 @@ static int jh7110_pmu_parse_dt(struct platform_device *pdev, struct jh71xx_pmu *
 	return 0;
 }
 
+static int jh7110_aon_pmu_parse_dt(struct platform_device *pdev, struct jh71xx_pmu *pmu)
+{
+	struct device *parent;
+	struct device *dev = &pdev->dev;
+
+	parent = pdev->dev.parent;
+	if (!parent) {
+		dev_err(dev, "No parent for syscon pmu\n");
+		return -ENODEV;
+	}
+
+	pmu->base = syscon_node_to_regmap(parent->of_node);
+	if (IS_ERR(pmu->base))
+		return PTR_ERR(pmu->base);
+
+	return 0;
+}
+
 static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
 {
 	struct jh71xx_pmu_dev *pmd;
@@ -398,10 +436,32 @@ static const struct jh71xx_pmu_match_data jh7110_pmu = {
 	.pmu_set_state = jh7110_pmu_set_state,
 };
 
+static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
+	[JH7110_PD_DPHY_TX] = {
+		.name = "DPHY-TX",
+		.bit = 30,
+	},
+	[JH7110_PD_DPHY_RX] = {
+		.name = "DPHY-RX",
+		.bit = 31,
+	},
+};
+
+static const struct jh71xx_pmu_match_data jh7110_aon_pmu = {
+	.num_domains = ARRAY_SIZE(jh7110_aon_power_domains),
+	.domain_info = jh7110_aon_power_domains,
+	.pmu_status = JH71XX_AON_PMU_SWITCH,
+	.pmu_parse_dt = jh7110_aon_pmu_parse_dt,
+	.pmu_set_state = jh7110_aon_pmu_set_state,
+};
+
 static const struct of_device_id jh71xx_pmu_of_match[] = {
 	{
 		.compatible = "starfive,jh7110-pmu",
 		.data = (void *)&jh7110_pmu,
+	}, {
+		.compatible = "starfive,jh7110-aon-pmu",
+		.data = (void *)&jh7110_aon_pmu,
 	}, {
 		/* sentinel */
 	}
@@ -418,5 +478,6 @@ static struct platform_driver jh71xx_pmu_driver = {
 builtin_platform_driver(jh71xx_pmu_driver);
 
 MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
+MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
 MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
 MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [RESEND v2 5/6] soc: starfive: Add JH7110 AON PMU support

[Shengyu Qu]

[-- Attachment #1.1.1: Type: text/plain, Size: 4571 bytes --]

Hi,

> Add AON PMU for StarFive JH7110 SoC. It can be used to turn on/off the
> dphy rx/tx power switch.
>
> Reviewed-by: Walker Chen <walker.chen@starfivetech.com>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>   MAINTAINERS                       |  1 +
>   drivers/soc/starfive/jh71xx_pmu.c | 63 ++++++++++++++++++++++++++++++-
>   2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fafeea8ebdb..8f32d43a9b67 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19950,6 +19950,7 @@ F:	include/dt-bindings/reset/starfive?jh71*.h
>   
>   STARFIVE JH71XX PMU CONTROLLER DRIVER
>   M:	Walker Chen <walker.chen@starfivetech.com>
> +M:	Changhuang Liang <changhuang.liang@starfivetech.com>
>   S:	Supported
>   F:	Documentation/devicetree/bindings/power/starfive*
>   F:	drivers/soc/starfive/jh71xx_pmu.c
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> index bb44cc93e822..1303826aa7b5 100644
> --- a/drivers/soc/starfive/jh71xx_pmu.c
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -2,7 +2,7 @@
>   /*
>    * StarFive JH71XX PMU (Power Management Unit) Controller Driver
>    *
> - * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
>    */
>   
>   #include <linux/interrupt.h>
> @@ -24,6 +24,9 @@
>   #define JH71XX_PMU_EVENT_STATUS		0x88
>   #define JH71XX_PMU_INT_STATUS		0x8C
>   
> +/* aon pmu register offset */
> +#define JH71XX_AON_PMU_SWITCH		0x00
> +
>   /* sw encourage cfg */
>   #define JH71XX_PMU_SW_ENCOURAGE_EN_LO	0x05
>   #define JH71XX_PMU_SW_ENCOURAGE_EN_HI	0x50
> @@ -163,6 +166,23 @@ static int jh7110_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>   	return 0;
>   }
>   
> +static int jh7110_aon_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
> +{
> +	struct jh71xx_pmu *pmu = pmd->pmu;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);
> +
> +	if (on)
> +		regmap_update_bits(pmu->base, JH71XX_AON_PMU_SWITCH, mask, mask);
> +	else
> +		regmap_update_bits(pmu->base, JH71XX_AON_PMU_SWITCH, mask, 0);
> +
> +	spin_unlock_irqrestore(&pmu->lock, flags);
> +
> +	return 0;
> +}
> +
>   static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>   {
>   	struct jh71xx_pmu *pmu = pmd->pmu;
> @@ -270,6 +290,24 @@ static int jh7110_pmu_parse_dt(struct platform_device *pdev, struct jh71xx_pmu *
>   	return 0;
>   }
>   
> +static int jh7110_aon_pmu_parse_dt(struct platform_device *pdev, struct jh71xx_pmu *pmu)
> +{
> +	struct device *parent;
> +	struct device *dev = &pdev->dev;
> +
> +	parent = pdev->dev.parent;
> +	if (!parent) {
> +		dev_err(dev, "No parent for syscon pmu\n");
> +		return -ENODEV;
> +	}
> +
> +	pmu->base = syscon_node_to_regmap(parent->of_node);
> +	if (IS_ERR(pmu->base))
> +		return PTR_ERR(pmu->base);
> +
> +	return 0;
> +}
> +
>   static int jh71xx_pmu_init_domain(struct jh71xx_pmu *pmu, int index)
>   {
>   	struct jh71xx_pmu_dev *pmd;
> @@ -398,10 +436,32 @@ static const struct jh71xx_pmu_match_data jh7110_pmu = {
>   	.pmu_set_state = jh7110_pmu_set_state,
>   };
>   
> +static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
> +	[JH7110_PD_DPHY_TX] = {
> +		.name = "DPHY-TX",
> +		.bit = 30,
> +	},
> +	[JH7110_PD_DPHY_RX] = {
> +		.name = "DPHY-RX",
> +		.bit = 31,

Where are JH7110_PD_DPHY_RX and JH7110_PD_DPHY_TX defined?

Best regards,

Shengyu

> +	},
> +};
> +
> +static const struct jh71xx_pmu_match_data jh7110_aon_pmu = {
> +	.num_domains = ARRAY_SIZE(jh7110_aon_power_domains),
> +	.domain_info = jh7110_aon_power_domains,
> +	.pmu_status = JH71XX_AON_PMU_SWITCH,
> +	.pmu_parse_dt = jh7110_aon_pmu_parse_dt,
> +	.pmu_set_state = jh7110_aon_pmu_set_state,
> +};
> +
>   static const struct of_device_id jh71xx_pmu_of_match[] = {
>   	{
>   		.compatible = "starfive,jh7110-pmu",
>   		.data = (void *)&jh7110_pmu,
> +	}, {
> +		.compatible = "starfive,jh7110-aon-pmu",
> +		.data = (void *)&jh7110_aon_pmu,
>   	}, {
>   		/* sentinel */
>   	}
> @@ -418,5 +478,6 @@ static struct platform_driver jh71xx_pmu_driver = {
>   builtin_platform_driver(jh71xx_pmu_driver);
>   
>   MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>");
> +MODULE_AUTHOR("Changhuang Liang <changhuang.liang@starfivetech.com>");
>   MODULE_DESCRIPTION("StarFive JH71XX PMU Driver");
>   MODULE_LICENSE("GPL");

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6977 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RESEND v2 5/6] soc: starfive: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Sat, May 06, 2023 at 09:58:39PM +0800, Shengyu Qu wrote:
> > +static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
> > +	[JH7110_PD_DPHY_TX] = {
> > +		.name = "DPHY-TX",
> > +		.bit = 30,
> > +	},
> > +	[JH7110_PD_DPHY_RX] = {
> > +		.name = "DPHY-RX",
> > +		.bit = 31,
> 
> Where are JH7110_PD_DPHY_RX and JH7110_PD_DPHY_TX defined?

In the dt-binding header added in 1/6.

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 5/6] soc: starfive: Add JH7110 AON PMU support

[Shengyu Qu]

[-- Attachment #1.1.1: Type: text/plain, Size: 569 bytes --]

Hi Conor,

Seems a bug or something, patchwork dropped patch 1, and I didn't 
noticed that, thanks.

Best regards,

Shengyu

> On Sat, May 06, 2023 at 09:58:39PM +0800, Shengyu Qu wrote:
>>> +static const struct jh71xx_domain_info jh7110_aon_power_domains[] = {
>>> +	[JH7110_PD_DPHY_TX] = {
>>> +		.name = "DPHY-TX",
>>> +		.bit = 30,
>>> +	},
>>> +	[JH7110_PD_DPHY_RX] = {
>>> +		.name = "DPHY-RX",
>>> +		.bit = 31,
>> Where are JH7110_PD_DPHY_RX and JH7110_PD_DPHY_TX defined?
> In the dt-binding header added in 1/6.
>
> Cheers,
> Conor.
>

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6977 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RESEND v2 5/6] soc: starfive: Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Sat, May 06, 2023 at 10:07:39PM +0800, Shengyu Qu wrote:
> Hi Conor,
> 
> Seems a bug or something, patchwork dropped patch 1, and I didn't noticed
> that, thanks.

It's there:
https://patchwork.kernel.org/project/linux-riscv/list/?submitter=208605

But for whatever reason it arrived before the cover, and thus was named
differently. We've been having some list troubles lately & that may be
the cause.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[RESEND v2 6/6] riscv: dts: starfive: jh7110: Add AON PMU node

[Changhuang Liang]

Add AON PMU node to configure power. It can be used to turn on/off dphy
rx/tx power switch.

Reviewed-by: Walker Chen <walker.chen@starfivetech.com>
Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 27f8ef37d029..3414edc877d5 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -509,6 +509,11 @@ aoncrg: clock-controller@17000000 {
 		aon_syscon: syscon@17010000 {
 			compatible = "starfive,jh7110-aon-syscon", "syscon", "simple-mfd";
 			reg = <0x0 0x17010000 0x0 0x1000>;
+
+			aon_pwrc: power-controller {
+				compatible = "starfive,jh7110-aon-pmu";
+				#power-domain-cells = <1>;
+			};
 		};
 
 		aongpio: pinctrl@17020000 {
-- 
2.25.1

Re: [RESEND v2 0/6] Add JH7110 AON PMU support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Tue, Apr 18, 2023 at 08:56:40PM -0700, Changhuang Liang wrote:
> This patchset adds aon power domain driver for the StarFive JH7110 SoC.
> It is used to turn on/off dphy rx/tx power switch. The series has been
> tested on the VisionFive 2 board.
> 
> This patchset should be applied after the patchset [1]:
> [1] https://lore.kernel.org/all/20230414024157.53203-1-xingyu.wu@starfivetech.com/
> 
> changes since v1:
> - Updated commit message.
> - Changed "starfive,jh7110-pmu-dphy" to "starfive,jh7110-aon-pmu".
> - Put if condition under allOf in .yaml file.
> - Updated spelling error.
> - Dropped patch 4: Add pmu type operation.
> - Changed "jh71xx_pmu_general_set_state" to "jh7110_pmu_set_state" and moved it in call back.
> - Changed "jh7110_pmu_general_parse_dt" to "jh7110_pmu_parse_dt" and moved it in call back.
> - Used pmu_status save the pmu status offset.
> - Changed "JH71XX_PMU_DPHY_SWITCH" to "JH71XX_AON_PMU_SWITCH"
> - Changed copyright to "2022-2023"

For the future, when you resend, please say why you did.

> 
> v1: https://lore.kernel.org/all/20230411064743.273388-1-changhuang.liang@starfivetech.com/
> 
> Changhuang Liang (6):
>   dt-bindings: power: Add JH7110 AON PMU support
>   soc: starfive: Replace SOC_STARFIVE with ARCH_STARFIVE
>   soc: starfive: Modify ioremap to regmap
>   soc: starfive: Extract JH7110 pmu private operations
>   soc: starfive: Add JH7110 AON PMU support
>   riscv: dts: starfive: jh7110: Add AON PMU node
> 
>  .../bindings/power/starfive,jh7110-pmu.yaml   |  15 +-
>  MAINTAINERS                                   |   1 +
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |   5 +
>  drivers/soc/starfive/Kconfig                  |   4 +-
>  drivers/soc/starfive/jh71xx_pmu.c             | 200 +++++++++++++-----
>  .../dt-bindings/power/starfive,jh7110-pmu.h   |   3 +
>  6 files changed, 174 insertions(+), 54 deletions(-)
> 
> 
> base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
> prerequisite-patch-id: 388b8adbb0fe2daf4d07a21eafd4f1bd50ce2403

Also, all of this pre-req patch id stuff is pretty useless. You've
already pointed out the patches that this is based on, so IMO this
information isn't helpful & is probably more likely confuse tooling
than anything else.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RESEND v2 0/6] Add JH7110 AON PMU support

[Changhuang Liang]

On 2023/4/19 14:23, Conor Dooley wrote:
> On Tue, Apr 18, 2023 at 08:56:40PM -0700, Changhuang Liang wrote:
[...]
>> - Used pmu_status save the pmu status offset.
>> - Changed "JH71XX_PMU_DPHY_SWITCH" to "JH71XX_AON_PMU_SWITCH"
>> - Changed copyright to "2022-2023"
> 
> For the future, when you resend, please say why you did.
> 

OK, thanks for your correction. I'll pay attention.

>>
>> v1: https://lore.kernel.org/all/20230411064743.273388-1-changhuang.liang@starfivetech.com/
>>
[...]
>>
>> base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
>> prerequisite-patch-id: 388b8adbb0fe2daf4d07a21eafd4f1bd50ce2403
> 
> Also, all of this pre-req patch id stuff is pretty useless. You've
> already pointed out the patches that this is based on, so IMO this
> information isn't helpful & is probably more likely confuse tooling
> than anything else.
> 
> Cheers,
> Conor.

OK, will not use "--base" for next.