| Message ID | 20220713084010.168720-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "reset: microchip-sparx5: allow building as a module" | expand |
[+ 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
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
[+ 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
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
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
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
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
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
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>");
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(-)