[PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Philipp Zabel]

This reverts commit b6b9585876da018bdde2d5f15d206a689c0d70f3.

This breaks MDIO on kswitch-d10, presumably because the global switch
reset is not released early enough anymore.

Reported-by: Michael Walle <michael@walle.cc>
Cc: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/Kconfig                  | 2 +-
 drivers/reset/reset-microchip-sparx5.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index d9a08ec343e2..f9a7cee01659 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -115,7 +115,7 @@ config RESET_LPC18XX
 	  This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
 
 config RESET_MCHP_SPARX5
-	tristate "Microchip Sparx5 reset driver"
+	bool "Microchip Sparx5 reset driver"
 	depends on ARCH_SPARX5 || SOC_LAN966 || COMPILE_TEST
 	default y if SPARX5_SWITCH
 	select MFD_SYSCON
diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 3d54dda3593e..00b612a0effa 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -149,7 +149,6 @@ static const struct of_device_id mchp_sparx5_reset_of_match[] = {
 	},
 	{ }
 };
-MODULE_DEVICE_TABLE(of, mchp_sparx5_reset_of_match);
 
 static struct platform_driver mchp_sparx5_reset_driver = {
 	.probe = mchp_sparx5_reset_probe,
@@ -159,7 +158,12 @@ static struct platform_driver mchp_sparx5_reset_driver = {
 	},
 };
 
-module_platform_driver(mchp_sparx5_reset_driver);
+static int __init mchp_sparx5_reset_init(void)
+{
+	return platform_driver_register(&mchp_sparx5_reset_driver);
+}
+
+postcore_initcall(mchp_sparx5_reset_init);
 
 MODULE_DESCRIPTION("Microchip Sparx5 switch reset driver");
 MODULE_AUTHOR("Steen Hegelund <steen.hegelund@microchip.com>");
-- 
2.30.2

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Michael Walle]

[+ Claudiu, Kavyasree ]
Am 2022-07-13 10:40, schrieb Philipp Zabel:
> This reverts commit b6b9585876da018bdde2d5f15d206a689c0d70f3.
> 
> This breaks MDIO on kswitch-d10, presumably because the global switch
> reset is not released early enough anymore.
> 
> Reported-by: Michael Walle <michael@walle.cc>
> Cc: Clément Léger <clement.leger@bootlin.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks!

Tested-by: Michael Walle <michael@walle.cc>

And maybe Microchip can chime in here and tell us what
subsystems in the SoC will actually be reset by this.
I fear this reset will affect almost every part of the
SoC. So it would have to be bound to every single
device? Or maybe adding that reset to the switch driver
was a mistake in the first place?

I mean, if it wouldn't be for the guard bit, it would
also reset the CPU core..

-michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Steen Hegelund]

Hi Michael,

I am afraid that the exact list of affected modules is not available, so using the
RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of resetting as much as possible, and
still continue execution.

This is what the Sparx5 datasheet has to say about the SYS_RST_PROT_VCORE protect bit:

The device can be soft-reset by writing SOFT_RST.SOFT_CHIP_RST. The VCore system and CPU can be
protected from a device soft-reset by writing RESET_PROT_STAT.SYS_RST_PROT_VCORE = 1 before
initiating soft-reset. 

In this case, a chip-level soft reset is applied to all other blocks except the VCore system and the
VCore CPU. When protecting the VCore system and CPU from a soft reset, the frame DMA must be
disabled prior to a chip-level soft reset. The SERDES and PLL blocks can be protected from reset by
writing to SOFT_RST.SOFT_SWC_RST instead of SOFT_CHIP_RST.

The VCore general purpose registers (CPU::GPR) and GPIO alternate modes (DEVCPU_GCB::GPIO_ALT) are
not affected by a soft-reset. These registers are only reset when an external reset is asserted.

BR
Steen

On Wed, 2022-07-13 at 11:03 +0200, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> [+ Claudiu, Kavyasree ]
> Am 2022-07-13 10:40, schrieb Philipp Zabel:
> > This reverts commit b6b9585876da018bdde2d5f15d206a689c0d70f3.
> > 
> > This breaks MDIO on kswitch-d10, presumably because the global switch
> > reset is not released early enough anymore.
> > 
> > Reported-by: Michael Walle <michael@walle.cc>
> > Cc: Clément Léger <clement.leger@bootlin.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Thanks!
> 
> Tested-by: Michael Walle <michael@walle.cc>
> 
> And maybe Microchip can chime in here and tell us what
> subsystems in the SoC will actually be reset by this.
> I fear this reset will affect almost every part of the
> SoC. So it would have to be bound to every single
> device? Or maybe adding that reset to the switch driver
> was a mistake in the first place?
> 
> I mean, if it wouldn't be for the guard bit, it would
> also reset the CPU core..
> 
> -michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Michael Walle]

[+ Horatiu, I missed you earlier, sorry]

Hi Steen,

Am 2022-07-13 11:40, schrieb Steen Hegelund:
> I am afraid that the exact list of affected modules is not available,
> so using the
> RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of
> resetting as much as possible, and
> still continue execution.

Mh, you are designing that chip (at least the LAN966x) no? Shouldn't
that information be available anywhere at Microchip? ;)

Anyway, it looks like almost the whole chip is reset
except some minor things. So the driver has actually a
wrong name. Until recently only the switch driver was the
sole user of it (at least on the lan966x). So, my question
remains, is this correct? I mean the switch driver says,
"reset the switch core", but what actually happens is that
the the entire SoC except the CPU and maybe the io mux is reset.
What about the watchdog for example? Will that be reset, too?

-michael

> This is what the Sparx5 datasheet has to say about the
> SYS_RST_PROT_VCORE protect bit:
> 
> The device can be soft-reset by writing SOFT_RST.SOFT_CHIP_RST. The
> VCore system and CPU can be
> protected from a device soft-reset by writing
> RESET_PROT_STAT.SYS_RST_PROT_VCORE = 1 before
> initiating soft-reset. 
> 
> In this case, a chip-level soft reset is applied to all other blocks
> except the VCore system and the
> VCore CPU. When protecting the VCore system and CPU from a soft reset,
> the frame DMA must be
> disabled prior to a chip-level soft reset. The SERDES and PLL blocks
> can be protected from reset by
> writing to SOFT_RST.SOFT_SWC_RST instead of SOFT_CHIP_RST.
> 
> The VCore general purpose registers (CPU::GPR) and GPIO alternate
> modes (DEVCPU_GCB::GPIO_ALT) are
> not affected by a soft-reset. These registers are only reset when an
> external reset is asserted.
> 
> BR
> Steen
> 
> On Wed, 2022-07-13 at 11:03 +0200, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> [+ Claudiu, Kavyasree ]
>> Am 2022-07-13 10:40, schrieb Philipp Zabel:
>> > This reverts commit b6b9585876da018bdde2d5f15d206a689c0d70f3.
>> >
>> > This breaks MDIO on kswitch-d10, presumably because the global switch
>> > reset is not released early enough anymore.
>> >
>> > Reported-by: Michael Walle <michael@walle.cc>
>> > Cc: Clément Léger <clement.leger@bootlin.com>
>> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> 
>> Thanks!
>> 
>> Tested-by: Michael Walle <michael@walle.cc>
>> 
>> And maybe Microchip can chime in here and tell us what
>> subsystems in the SoC will actually be reset by this.
>> I fear this reset will affect almost every part of the
>> SoC. So it would have to be bound to every single
>> device? Or maybe adding that reset to the switch driver
>> was a mistake in the first place?
>> 
>> I mean, if it wouldn't be for the guard bit, it would
>> also reset the CPU core..
>> 
>> -michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Philipp Zabel]

Hi,

On Mi, 2022-07-13 at 11:52 +0200, Michael Walle wrote:
> [+ Horatiu, I missed you earlier, sorry]
> 
> Hi Steen,
> 
> Am 2022-07-13 11:40, schrieb Steen Hegelund:
> > I am afraid that the exact list of affected modules is not available,
> > so using the
> > RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of
> > resetting as much as possible, and
> > still continue execution.
> 
> Mh, you are designing that chip (at least the LAN966x) no? Shouldn't
> that information be available anywhere at Microchip? ;)
> 
> Anyway, it looks like almost the whole chip is reset
> except some minor things. So the driver has actually a
> wrong name. Until recently only the switch driver was the
> sole user of it (at least on the lan966x). So, my question
> remains, is this correct? I mean the switch driver says,
> "reset the switch core", but what actually happens is that
> the the entire SoC except the CPU and maybe the io mux is reset.
> What about the watchdog for example? Will that be reset, too?

If [1-3] are to be trusted, RESET_PROT_STAT[VCORE_RST_PROT_WDT], which
protects the watchdog from soft reset, is not set by default. So yes?

There are also AMBA, PCIe, PDBG protection bits against Vcore soft
reset in this register, depending on the platform.

[1] https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=cpu,cpu_regs,reset_prot_stat
[2] https://microchip-ung.github.io/lan9662_reginfo/reginfo_LAN9662.html?select=cpu,cpu_regs,reset_prot_stat
[3] https://microchip-ung.github.io/lan9668_reginfo/reginfo_LAN9668.html?select=cpu,cpu_regs,reset_prot_stat

regards
Philipp

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Michael Walle]

Am 2022-07-13 14:08, schrieb Philipp Zabel:
> Hi,
> 
> On Mi, 2022-07-13 at 11:52 +0200, Michael Walle wrote:
>> [+ Horatiu, I missed you earlier, sorry]
>> 
>> Hi Steen,
>> 
>> Am 2022-07-13 11:40, schrieb Steen Hegelund:
>> > I am afraid that the exact list of affected modules is not available,
>> > so using the
>> > RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of
>> > resetting as much as possible, and
>> > still continue execution.
>> 
>> Mh, you are designing that chip (at least the LAN966x) no? Shouldn't
>> that information be available anywhere at Microchip? ;)
>> 
>> Anyway, it looks like almost the whole chip is reset
>> except some minor things. So the driver has actually a
>> wrong name. Until recently only the switch driver was the
>> sole user of it (at least on the lan966x). So, my question
>> remains, is this correct? I mean the switch driver says,
>> "reset the switch core", but what actually happens is that
>> the the entire SoC except the CPU and maybe the io mux is reset.
>> What about the watchdog for example? Will that be reset, too?
> 
> If [1-3] are to be trusted, RESET_PROT_STAT[VCORE_RST_PROT_WDT], which
> protects the watchdog from soft reset, is not set by default. So yes?
> 
> There are also AMBA, PCIe, PDBG protection bits against Vcore soft
> reset in this register, depending on the platform.
> 
> [1] 
> https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=cpu,cpu_regs,reset_prot_stat
> [2] 
> https://microchip-ung.github.io/lan9662_reginfo/reginfo_LAN9662.html?select=cpu,cpu_regs,reset_prot_stat
> [3] 
> https://microchip-ung.github.io/lan9668_reginfo/reginfo_LAN9668.html?select=cpu,cpu_regs,reset_prot_stat

Ping. any news here?

-michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Steen Hegelund]

Hi Michael,

Sorry, but most people have been OOO (including me), so this has delayed the response.

The protection bit protects the VCore Shared Bus (SBA) blocks shown on Figure 5-1. VCore System
Block Diagram in the Datasheet.  So in this case also the watchdog (which is the WDT block).

I hope this clarifies the usage.

Best Regards
Steen

On Thu, 2022-08-04 at 09:53 +0200, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-13 14:08, schrieb Philipp Zabel:
> > Hi,
> > 
> > On Mi, 2022-07-13 at 11:52 +0200, Michael Walle wrote:
> > > [+ Horatiu, I missed you earlier, sorry]
> > > 
> > > Hi Steen,
> > > 
> > > Am 2022-07-13 11:40, schrieb Steen Hegelund:
> > > > I am afraid that the exact list of affected modules is not available,
> > > > so using the
> > > > RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of
> > > > resetting as much as possible, and
> > > > still continue execution.
> > > 
> > > Mh, you are designing that chip (at least the LAN966x) no? Shouldn't
> > > that information be available anywhere at Microchip? ;)
> > > 
> > > Anyway, it looks like almost the whole chip is reset
> > > except some minor things. So the driver has actually a
> > > wrong name. Until recently only the switch driver was the
> > > sole user of it (at least on the lan966x). So, my question
> > > remains, is this correct? I mean the switch driver says,
> > > "reset the switch core", but what actually happens is that
> > > the the entire SoC except the CPU and maybe the io mux is reset.
> > > What about the watchdog for example? Will that be reset, too?
> > 
> > If [1-3] are to be trusted, RESET_PROT_STAT[VCORE_RST_PROT_WDT], which
> > protects the watchdog from soft reset, is not set by default. So yes?
> > 
> > There are also AMBA, PCIe, PDBG protection bits against Vcore soft
> > reset in this register, depending on the platform.
> > 
> > [1]
> > https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=cpu,cpu_regs,reset_prot_stat
> > [2]
> > https://microchip-ung.github.io/lan9662_reginfo/reginfo_LAN9662.html?select=cpu,cpu_regs,reset_prot_stat
> > [3]
> > https://microchip-ung.github.io/lan9668_reginfo/reginfo_LAN9668.html?select=cpu,cpu_regs,reset_prot_stat
> 
> Ping. any news here?
> 
> -michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Michael Walle]

Hi Steen,

Am 2022-08-09 12:19, schrieb Steen Hegelund:
> Sorry, but most people have been OOO (including me), so this has
> delayed the response.
> 
> The protection bit protects the VCore Shared Bus (SBA) blocks shown on
> Figure 5-1. VCore System
> Block Diagram in the Datasheet.  So in this case also the watchdog
> (which is the WDT block).
> 
> I hope this clarifies the usage.

Actually, it does not. The watchdog was just an example. I'm
questioning the use of this reset for the switch block. Esp.
as it turns out that not even microchip knows what blocks
are actually reset.

This reset was first introduced for the DSA switch, since then
more and more spurious errors were discovered and it turns out
all of these errors was because the "switch" reset had some
severe side effects, like resetting the GPIO core and thus
turning all the GPIOs to input mode which then took some
peripherals into reset.

-michael

Re: [PATCH] Revert "reset: microchip-sparx5: allow building as a module"

[Michael Walle]

Hi,

Am 2022-07-13 14:08, schrieb Philipp Zabel:
> On Mi, 2022-07-13 at 11:52 +0200, Michael Walle wrote:
>> [+ Horatiu, I missed you earlier, sorry]
>> 
>> Hi Steen,
>> 
>> Am 2022-07-13 11:40, schrieb Steen Hegelund:
>> > I am afraid that the exact list of affected modules is not available,
>> > so using the
>> > RESET_PROT_STAT.SYS_RST_PROT_VCORE bit is the best known way of
>> > resetting as much as possible, and
>> > still continue execution.
>> 
>> Mh, you are designing that chip (at least the LAN966x) no? Shouldn't
>> that information be available anywhere at Microchip? ;)
>> 
>> Anyway, it looks like almost the whole chip is reset
>> except some minor things. So the driver has actually a
>> wrong name. Until recently only the switch driver was the
>> sole user of it (at least on the lan966x). So, my question
>> remains, is this correct? I mean the switch driver says,
>> "reset the switch core", but what actually happens is that
>> the the entire SoC except the CPU and maybe the io mux is reset.
>> What about the watchdog for example? Will that be reset, too?
> 
> If [1-3] are to be trusted, RESET_PROT_STAT[VCORE_RST_PROT_WDT], which
> protects the watchdog from soft reset, is not set by default. So yes?
> 
> There are also AMBA, PCIe, PDBG protection bits against Vcore soft
> reset in this register, depending on the platform.

*But* this also prevents it from reset by the watchdog. I don't know
if we want that?!

I.e. what happens if one sets RESET_PROT_STAT[VCORE_RST_PROT_WDT] and
the watchdog does a reset? OTHO, I guess it is also bad to reset the
watchdog during boot.. IMHO this reset logic doesn't look that well
designed.

> [1] 
> https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=cpu,cpu_regs,reset_prot_stat
> [2] 
> https://microchip-ung.github.io/lan9662_reginfo/reginfo_LAN9662.html?select=cpu,cpu_regs,reset_prot_stat
> [3] 
> https://microchip-ung.github.io/lan9668_reginfo/reginfo_LAN9668.html?select=cpu,cpu_regs,reset_prot_stat

-michael