Add reset controller to mt7988 infracfg

PATCH Superseded Frank Wunderlich 5 participants 13 messages Jan 17, 2024 – Feb 1, 2024

Subsystems (4)

Device tree bindings Arm MediaTek Common clk Reset controller framework

lore

Share & tools

lore

Frank Wunderlich

author

2024-01-17 18:41 UTC #0 lore raw

v3 cover Add reset controller to mt7988 infracfg

From: Frank Wunderlich <[email protected]>

Infracfg on mt7988 supports reset controller function which is needed to get lvts thermal working.

Patches are based on clk-next due to recently added mt7988 clock driver:
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git

changes:
v3:
- start resets on RST0 offset (LVTS is RST1)
- rename offset constants with MT7988 prefix (else collision with reset.h)
v2:
- change value of constant to 0 from 9
- add missing SoB and commit-message for binding-patch

Frank Wunderlich (2):
dt-bindings: reset: mediatek: add MT7988 reset IDs
clk: mediatek: add infracfg reset controller for mt7988

 drivers/clk/mediatek/clk-mt7988-infracfg.c    | 23 +++++++++++++++++++
 .../reset/mediatek,mt7988-resets.h            |  6 +++++
 2 files changed, 29 insertions(+)

Signature · 1 line

2.34.1

Frank Wunderlich

author

2024-01-17 18:41 UTC #1 lore raw

PATCH v3 1/2 dt-bindings: reset: mediatek: add MT7988 reset IDs

From: Frank Wunderlich <[email protected]>

Add reset constants for using as index in driver and dts.

Signed-off-by Frank Wunderlich <[email protected]>

---
v3:
- add pcie reset id as suggested by angelo

v2:
- add missing commit message and SoB
- change value of infrareset to 0

---
 include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/dt-bindings/reset/mediatek,mt7988-resets.h +6 -0

index 493301971367..0eb152889a89 100644
@@ -10,4 +10,10 @@
 /* ETHWARP resets */
 #define MT7988_ETHWARP_RST_SWITCH		0
 
+/* INFRA resets */
+#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
+#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
+
+
 #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT7988 */
+

Signature · 1 line

2.34.1

AngeloGioacchino Del Regno

maintainer Reviewed-by on v3 1/2

2024-01-18 08:44 UTC #2 lore raw

Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Il 17/01/24 19:41, Frank Wunderlich ha scritto:

Quoted 5 lines

From: Frank Wunderlich <[email protected]>
Add reset constants for using as index in driver and dts.
Signed-off-by Frank Wunderlich <[email protected]>

Reviewed-by AngeloGioacchino Del Regno <[email protected]>

Conor Dooley

2024-01-18 16:49 UTC #3 lore raw

Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

In reply to Frank Wunderlich

Quoted 21 lines · Frank Wunderlich

From: Frank Wunderlich <[email protected]>
Add reset constants for using as index in driver and dts.
Signed-off-by Frank Wunderlich <[email protected]>
---
v3:
- add pcie reset id as suggested by angelo
v2:
- add missing commit message and SoB
- change value of infrareset to 0
---
 include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/dt-bindings/reset/mediatek,mt7988-resets.h +0 -0
index 493301971367..0eb152889a89 100644

include/dt-bindings/reset/mediatek,mt7988-resets.h line 15

Quoted 6 lines

include/dt-bindings/reset/mediatek,mt7988-resets.h +3 -0

@@ -10,4 +10,10 @@
 /* ETHWARP resets */
 #define MT7988_ETHWARP_RST_SWITCH		0
 
+/* INFRA resets */
+#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
+#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1

These are just "random" numbers, why not continue the numbering from the ETHWARP?

Quoted 7 lines

include/dt-bindings/reset/mediatek,mt7988-resets.h +3 -0
+
+
 #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT7988 */
+
Signature · 1 line
2.34.1

Frank Wunderlich

2024-01-18 22:00 UTC #4 lore raw

Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Quoted 27 lines

Gesendet: Donnerstag, 18. Januar 2024 um 17:49 Uhr
Von: "Conor Dooley" <[email protected]>
Quoted 24 lines · Frank Wunderlich
From: Frank Wunderlich <[email protected]>
Add reset constants for using as index in driver and dts.
Signed-off-by Frank Wunderlich <[email protected]>
---
v3:
- add pcie reset id as suggested by angelo
v2:
- add missing commit message and SoB
- change value of infrareset to 0
---
 include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/dt-bindings/reset/mediatek,mt7988-resets.h +0 -0
index 493301971367..0eb152889a89 100644
@@ -10,4 +10,10 @@
 /* ETHWARP resets */
 #define MT7988_ETHWARP_RST_SWITCH		0
 

Quoted 6 lines

include/dt-bindings/reset/mediatek,mt7988-resets.h line 15
Quoted 3 lines
include/dt-bindings/reset/mediatek,mt7988-resets.h +3 -0
+/* INFRA resets */
+#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
+#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
These are just "random" numbers, why not continue the numbering from the ETHWARP?

i can do...basicly these consts are used in DTS and driver only as index.

@angelo what do you think? I though also in leaving some space to allow grouping RST0 and RST1 when more consts are added, else the numbers are mixed up.

so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RST0 and RST1 can have up to 32 resets). That will allow adding more reset constants between my values and having raising numbers.

regards Frank

AngeloGioacchino Del Regno

maintainer

2024-01-19 09:28 UTC #5 lore raw

Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Il 18/01/24 23:00, Frank Wunderlich ha scritto:

Quoted 34 lines

Quoted 27 lines
Gesendet: Donnerstag, 18. Januar 2024 um 17:49 Uhr
Von: "Conor Dooley" <[email protected]>
Quoted 24 lines · Frank Wunderlich
From: Frank Wunderlich <[email protected]>
Add reset constants for using as index in driver and dts.
Signed-off-by Frank Wunderlich <[email protected]>
---
v3:
- add pcie reset id as suggested by angelo
v2:
- add missing commit message and SoB
- change value of infrareset to 0
---
  include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
  1 file changed, 6 insertions(+)
include/dt-bindings/reset/mediatek,mt7988-resets.h +0 -0
index 493301971367..0eb152889a89 100644
@@ -10,4 +10,10 @@
  /* ETHWARP resets */
  #define MT7988_ETHWARP_RST_SWITCH		0
 
Quoted 6 lines
include/dt-bindings/reset/mediatek,mt7988-resets.h line 15
Quoted 3 lines
include/dt-bindings/reset/mediatek,mt7988-resets.h +3 -0
+/* INFRA resets */
+#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
+#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
These are just "random" numbers, why not continue the numbering from the ETHWARP?

Quoted 7 lines

i can do...basicly these consts are used in DTS and driver only as index.
@angelo what do you think? I though also in leaving some space to allow grouping RST0 and RST1 when more consts are added, else the numbers are mixed up.
so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RST0 and RST1 can have up to 32 resets). That will allow adding more reset constants between my values and having raising numbers.

The resets are organized on a per-reset-controller basis, so, the ETHWARP reset controller's first reset is RST_SWITCH, the second one is RST_something_else, etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.

That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
I think that the numbering is good as it is, and having one driver start at index 5 while the other starts at index 12 would only overcomplicate registering the resets in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no good reason.

This is one header, but it should "in theory" be more than one... so we would have one for each hardware block - but that'd make the reset directory over-crowded, as other MediaTek SoCs have got even more resets in even more hardware blocks than the MT7988. That'd be something like ~4 reset headers per SoC (and will increase with newer ones)...
...and this is why we have one binding header for resets.

On the topic of leaving space to allow grouping RST0/RST1: -> No. <- The indices have to start from zero and have to be sequential, with no holes.

Cheers,
Angelo

Conor Dooley

2024-01-19 17:04 UTC #6 lore raw

Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Quoted 53 lines · AngeloGioacchino Del Regno

Il 18/01/24 23:00, Frank Wunderlich ha scritto:
Quoted 34 lines
Quoted 27 lines
Gesendet: Donnerstag, 18. Januar 2024 um 17:49 Uhr
Von: "Conor Dooley" <[email protected]>
Quoted 24 lines · Frank Wunderlich
From: Frank Wunderlich <[email protected]>
Add reset constants for using as index in driver and dts.
Signed-off-by Frank Wunderlich <[email protected]>
---
v3:
- add pcie reset id as suggested by angelo
v2:
- add missing commit message and SoB
- change value of infrareset to 0
---
  include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
  1 file changed, 6 insertions(+)
include/dt-bindings/reset/mediatek,mt7988-resets.h +0 -0
index 493301971367..0eb152889a89 100644
@@ -10,4 +10,10 @@
  /* ETHWARP resets */
  #define MT7988_ETHWARP_RST_SWITCH		0
 
Quoted 6 lines
include/dt-bindings/reset/mediatek,mt7988-resets.h line 15
Quoted 3 lines
include/dt-bindings/reset/mediatek,mt7988-resets.h +3 -0
+/* INFRA resets */
+#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
+#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
These are just "random" numbers, why not continue the numbering from the ETHWARP?
Quoted 7 lines
i can do...basicly these consts are used in DTS and driver only as index.
@angelo what do you think? I though also in leaving some space to allow grouping RST0 and RST1 when more consts are added, else the numbers are mixed up.
so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RST0 and RST1 can have up to 32 resets). That will allow adding more reset constants between my values and having raising numbers.
The resets are organized on a per-reset-controller basis, so, the ETHWARP reset controller's first reset is RST_SWITCH, the second one is RST_something_else, etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
I think that the numbering is good as it is, and having one driver start at index 5 while the other starts at index 12 would only overcomplicate registering the resets in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no good reason.

Quoted 6 lines

This is one header, but it should "in theory" be more than one... so we would have one for each hardware block - but that'd make the reset directory over-crowded, as other MediaTek SoCs have got even more resets in even more hardware blocks than the MT7988. That'd be something like ~4 reset headers per SoC (and will increase with newer ones)...
...and this is why we have one binding header for resets.

That's okay. The commit message leaves me, who clearly isn't a mediatek guy, with no information as to why these are not one contiguous set. IMO being for different reset controllers entirely is fine.

Quoted 2 lines

On the topic of leaving space to allow grouping RST0/RST1: -> No. <- The indices have to start from zero and have to be sequential, with no holes.

Agreed.

Frank Wunderlich

2024-02-01 11:40 UTC #7 lore raw

Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Am 19. Januar 2024 18:04:36 MEZ schrieb Conor Dooley <[email protected]>:

Quoted 19 lines · AngeloGioacchino Del Regno

Quoted 11 lines
The resets are organized on a per-reset-controller basis, so, the ETHWARP reset controller's first reset is RST_SWITCH, the second one is RST_something_else, etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
I think that the numbering is good as it is, and having one driver start at index 5 while the other starts at index 12 would only overcomplicate registering the resets in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no good reason.
Quoted 6 lines
This is one header, but it should "in theory" be more than one... so we would have one for each hardware block - but that'd make the reset directory over-crowded, as other MediaTek SoCs have got even more resets in even more hardware blocks than the MT7988. That'd be something like ~4 reset headers per SoC (and will increase with newer ones)...
...and this is why we have one binding header for resets.

Quoted 8 lines

That's okay. The commit message leaves me, who clearly isn't a mediatek guy, with no information as to why these are not one contiguous set. IMO being for different reset controllers entirely is fine.
Quoted 2 lines
On the topic of leaving space to allow grouping RST0/RST1: -> No. <- The indices have to start from zero and have to be sequential, with no holes.
Agreed.

Hi,

Just a friendly reminder.

As far as i understood, Patches are fine so far and do not need any rework,right?

But i have not seen them picked up yet in linux-next. regards Frank

Conor Dooley

Acked-by

2024-02-01 13:12 UTC #8 lore raw

Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Quoted 27 lines · Frank Wunderlich

Am 19. Januar 2024 18:04:36 MEZ schrieb Conor Dooley <[email protected]>:
Quoted 26 lines · AngeloGioacchino Del Regno
Quoted 11 lines
The resets are organized on a per-reset-controller basis, so, the ETHWARP reset controller's first reset is RST_SWITCH, the second one is RST_something_else, etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
I think that the numbering is good as it is, and having one driver start at index 5 while the other starts at index 12 would only overcomplicate registering the resets in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no good reason.
Quoted 6 lines
This is one header, but it should "in theory" be more than one... so we would have one for each hardware block - but that'd make the reset directory over-crowded, as other MediaTek SoCs have got even more resets in even more hardware blocks than the MT7988. That'd be something like ~4 reset headers per SoC (and will increase with newer ones)...
...and this is why we have one binding header for resets.
That's okay. The commit message leaves me, who clearly isn't a mediatek guy, with no information as to why these are not one contiguous set. IMO being for different reset controllers entirely is fine.
Quoted 2 lines
On the topic of leaving space to allow grouping RST0/RST1: -> No. <- The indices have to start from zero and have to be sequential, with no holes.

Quoted 7 lines

Quoted 1 line
Agreed.
Hi,
Just a friendly reminder.
As far as i understood, Patches are fine so far and do not need any rework,right?

I suspect I was asking for a commit message that explains why these numbers don't continue in sequence. With that,

Acked-by Conor Dooley <[email protected]>

Cheers,
Conor.

Quoted 3 lines

But i have not seen them picked up yet in linux-next. regards Frank

Frank Wunderlich

2024-02-01 14:23 UTC #9 lore raw

Aw: Re: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Quoted 8 lines

Gesendet: Donnerstag, 01. Februar 2024 um 14:11 Uhr
Von: "Conor Dooley" <[email protected]>
I suspect I was asking for a commit message that explains why these numbers don't continue in sequence. With that,
Acked-by Conor Dooley <[email protected]>
Cheers,
Conor.

OK, i would add this to commit message:

"Value is starting again from 0 because resets are used in another driver than existing constants."

and adding infracfg in commit title, if this is enough?

regards Frank

Conor Dooley

2024-02-01 18:20 UTC #10 lore raw

Re: Aw: Re: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

Quoted 6 lines · Frank Wunderlich

Quoted 6 lines
Gesendet: Donnerstag, 01. Februar 2024 um 14:11 Uhr
Von: "Conor Dooley" <[email protected]>
I suspect I was asking for a commit message that explains why these numbers don't continue in sequence. With that,
Acked-by Conor Dooley <[email protected]>

Quoted 7 lines

Quoted 2 lines
Cheers,
Conor.
OK, i would add this to commit message:
"Value is starting again from 0 because resets are used in another driver than existing constants."

s/driver/device/ and sure.

Frank Wunderlich

author

2024-01-17 18:41 UTC #11 lore raw

PATCH v3 2/2 clk: mediatek: add infracfg reset controller for mt7988

In reply to Frank Wunderlich

From: Frank Wunderlich <[email protected]>

Infracfg can also operate as reset controller, add support for it.

Signed-off-by Frank Wunderlich <[email protected]>

---
v3:
- start with RST0 (LVTS is in RST1)
- rename reset offset to contain SOC to not collide with constants defined in reset.h

---
 drivers/clk/mediatek/clk-mt7988-infracfg.c | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

drivers/clk/mediatek/clk-mt7988-infracfg.c +23 -0

index 8011ef278bea..449041f8abbc 100644
@@ -14,6 +14,10 @@
 #include "clk-gate.h"
 #include "clk-mux.h"
 #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+#include <dt-bindings/reset/mediatek,mt7988-resets.h>
+
+#define	MT7988_INFRA_RST0_SET_OFFSET	0x70
+#define	MT7988_INFRA_RST1_SET_OFFSET	0x80
 
 static DEFINE_SPINLOCK(mt7988_clk_lock);

@@ -249,12 +253,31 @@ static const struct mtk_gate infra_clks[] = {
 	GATE_INFRA3(CLK_INFRA_133M_PCIE_CK_P3, "infra_133m_pcie_ck_p3", "sysaxi_sel", 31),
 };
 
+static u16 infra_rst_ofs[] = {
+	MT7988_INFRA_RST0_SET_OFFSET,
+	MT7988_INFRA_RST1_SET_OFFSET,
+};
+
+static u16 infra_idx_map[] = {
+	[MT7988_INFRA_RST0_PEXTP_MAC_SWRST] = 0 * RST_NR_PER_BANK + 6,
+	[MT7988_INFRA_RST1_THERM_CTRL_SWRST] = 1 * RST_NR_PER_BANK + 9,
+};
+
+static struct mtk_clk_rst_desc infra_rst_desc = {
+	.version = MTK_RST_SET_CLR,
+	.rst_bank_ofs = infra_rst_ofs,
+	.rst_bank_nr = ARRAY_SIZE(infra_rst_ofs),
+	.rst_idx_map = infra_idx_map,
+	.rst_idx_map_nr = ARRAY_SIZE(infra_idx_map),
+};
+
 static const struct mtk_clk_desc infra_desc = {
 	.clks = infra_clks,
 	.num_clks = ARRAY_SIZE(infra_clks),
 	.mux_clks = infra_muxes,
 	.num_mux_clks = ARRAY_SIZE(infra_muxes),
 	.clk_lock = &mt7988_clk_lock,
+	.rst_desc = &infra_rst_desc,
 };
 
 static const struct of_device_id of_match_clk_mt7988_infracfg[] = {

Signature · 1 line

2.34.1

AngeloGioacchino Del Regno

maintainer Reviewed-by on v3 2/2

2024-01-18 08:44 UTC #12 lore raw

Re: [PATCH v3 2/2] clk: mediatek: add infracfg reset controller for mt7988

Il 17/01/24 19:41, Frank Wunderlich ha scritto:

Quoted 5 lines

From: Frank Wunderlich <[email protected]>
Infracfg can also operate as reset controller, add support for it.
Signed-off-by Frank Wunderlich <[email protected]>

Reviewed-by AngeloGioacchino Del Regno <[email protected]>