| Message ID | 20240327162355.24584-12-kabel@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | Turris Omnia - New board revision support | expand |
On 3/27/24 17:23, Marek Behún wrote: > Add driver model support for sysreset via mvebu system controller. This is > currently only available for U-Boot proper. > > Signed-off-by: Marek Behún <kabel@kernel.org> Only a minor comment below. Other than this: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > arch/arm/mach-mvebu/Kconfig | 18 +++++- > arch/arm/mach-mvebu/Makefile | 2 +- > arch/arm/mach-mvebu/cpu.c | 2 + > arch/arm/mach-mvebu/system-controller.c | 74 +++++++++++++++++++++++-- > 4 files changed, 89 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig > index 623432a60e..f15d3cc5ed 100644 > --- a/arch/arm/mach-mvebu/Kconfig > +++ b/arch/arm/mach-mvebu/Kconfig > @@ -19,6 +19,7 @@ config ARMADA_32BIT > select SPL_SYS_NO_VECTOR_TABLE if SPL > select ARCH_VERY_EARLY_INIT > select ARMADA_32BIT_SYSCON_RESET if DM_RESET && PCI_MVEBU > + select ARMADA_32BIT_SYSCON_SYSRESET if SYSRESET > > # ARMv7 SoCs... > config ARMADA_375 > @@ -457,16 +458,29 @@ config SF_DEFAULT_MODE > default 0x0 > depends on MVEBU_SPL_BOOT_DEVICE_SPI > > +config ARMADA_32BIT_SYSCON > + bool > + depends on ARMADA_32BIT > + select REGMAP > + select SYSCON > + > config ARMADA_32BIT_SYSCON_RESET > bool "Support Armada XP/375/38x/39x reset controller" > depends on ARMADA_32BIT > depends on DM_RESET > - select REGMAP > - select SYSCON > + select ARMADA_32BIT_SYSCON > help > Build support for Armada XP/375/38x/39x reset controller. This is > needed for PCIe support. > > +config ARMADA_32BIT_SYSCON_SYSRESET > + bool "Support Armada XP/375/38x/39x sysreset via driver model" > + depends on ARMADA_32BIT > + depends on SYSRESET > + select ARMADA_32BIT_SYSCON > + help > + Build support for Armada XP/375/38x/39x system reset via driver model. > + > source "board/solidrun/clearfog/Kconfig" > source "board/kobol/helios4/Kconfig" > > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile > index d44ca3a0df..329c2e4915 100644 > --- a/arch/arm/mach-mvebu/Makefile > +++ b/arch/arm/mach-mvebu/Makefile > @@ -28,7 +28,7 @@ obj-$(CONFIG_ARMADA_38X) += ../../../drivers/ddr/marvell/a38x/xor.o > obj-$(CONFIG_ARMADA_XP) += ../../../drivers/ddr/marvell/axp/xor.o > obj-$(CONFIG_ARMADA_MSYS) += ../../../drivers/ddr/marvell/axp/xor.o > > -obj-$(CONFIG_ARMADA_32BIT_SYSCON_RESET) += system-controller.o > +obj-$(CONFIG_ARMADA_32BIT_SYSCON) += system-controller.o > > ifdef CONFIG_ARMADA_38X > obj-$(CONFIG_MVEBU_EFUSE) += efuse.o > diff --git a/arch/arm/mach-mvebu/cpu.c b/arch/arm/mach-mvebu/cpu.c > index 8e0de93538..7c62a5dbb6 100644 > --- a/arch/arm/mach-mvebu/cpu.c > +++ b/arch/arm/mach-mvebu/cpu.c > @@ -52,6 +52,7 @@ void lowlevel_init(void) > */ > } > > +#if defined(CONFIG_SPL_BUILD) || !defined(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) > void reset_cpu(void) > { > struct mvebu_system_registers *reg = > @@ -62,6 +63,7 @@ void reset_cpu(void) > while (1) > ; > } > +#endif > > u32 get_boot_device(void) > { > diff --git a/arch/arm/mach-mvebu/system-controller.c b/arch/arm/mach-mvebu/system-controller.c > index c5c05922f2..b5f8afb96d 100644 > --- a/arch/arm/mach-mvebu/system-controller.c > +++ b/arch/arm/mach-mvebu/system-controller.c > @@ -10,11 +10,24 @@ > #include <regmap.h> > #include <reset-uclass.h> > #include <syscon.h> > +#include <sysreset.h> > #include <asm/io.h> > > -#define MVEBU_SOC_CONTROL_1_REG 0x4 > +#define MVEBU_SOC_CONTROL_1_REG 0x4 > > -#define MVEBU_PCIE_ID 0 > +#if defined(CONFIG_ARMADA_375) > +# define MVEBU_RSTOUTN_MASK_REG 0x54 > +# define MVEBU_SYS_SOFT_RST_REG 0x58 > +#else > +# define MVEBU_RSTOUTN_MASK_REG 0x60 > +# define MVEBU_SYS_SOFT_RST_REG 0x64 > +#endif > + > +#define MVEBU_GLOBAL_SOFT_RST_BIT BIT(0) > + > +#define MVEBU_PCIE_ID 0 > + > +#if IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET) > > static int mvebu_reset_of_xlate(struct reset_ctl *rst, > struct ofnode_phandle_args *args) > @@ -90,11 +103,64 @@ U_BOOT_DRIVER(mvebu_reset) = { > .ops = &mvebu_reset_ops, > }; > > +#endif /* IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET) */ > + > +#if IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) > + > +static int mvebu_sysreset_request(struct udevice *dev, enum sysreset_t type) > +{ > + struct regmap *regmap = syscon_get_regmap(dev->parent); > + uint bit; > + > + if (type != SYSRESET_COLD) > + return -EPROTONOSUPPORT; > + > + bit = MVEBU_GLOBAL_SOFT_RST_BIT; > + > + regmap_update_bits(regmap, MVEBU_RSTOUTN_MASK_REG, bit, bit); > + regmap_update_bits(regmap, MVEBU_SYS_SOFT_RST_REG, bit, bit); > + > + while (1) > + ; A comment before this endless loop might be helpful here. > + > + return 0; > +} > + > +static struct sysreset_ops mvebu_sysreset_ops = { > + .request = mvebu_sysreset_request, > +}; > + > +U_BOOT_DRIVER(mvebu_sysreset) = { > + .name = "mvebu-sysreset", > + .id = UCLASS_SYSRESET, > + .ops = &mvebu_sysreset_ops, > +}; > + > +#endif /* IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) */ > + > static int mvebu_syscon_bind(struct udevice *dev) > { > + int ret = 0; > + > /* bind also mvebu-reset, with the same ofnode */ > - return device_bind_driver_to_node(dev, "mvebu-reset", "mvebu-reset", > - dev_ofnode(dev), NULL); > + if (IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET)) { > + ret = device_bind_driver_to_node(dev, "mvebu-reset", > + "mvebu-reset", dev_ofnode(dev), > + NULL); > + if (ret < 0) > + return ret; > + } > + > + /* bind also mvebu-sysreset, with the same ofnode */ > + if (IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET)) { > + ret = device_bind_driver_to_node(dev, "mvebu-sysreset", > + "mvebu-sysreset", > + dev_ofnode(dev), NULL); > + if (ret < 0) > + return ret; > + } > + > + return ret; > } > > static const struct udevice_id mvebu_syscon_of_match[] = { Viele Grüße, Stefan Roese
On Thu, 28 Mar 2024 11:04:45 +0100 Stefan Roese <sr@denx.de> wrote: > > +static int mvebu_sysreset_request(struct udevice *dev, enum sysreset_t type) > > +{ > > + struct regmap *regmap = syscon_get_regmap(dev->parent); > > + uint bit; > > + > > + if (type != SYSRESET_COLD) > > + return -EPROTONOSUPPORT; > > + > > + bit = MVEBU_GLOBAL_SOFT_RST_BIT; > > + > > + regmap_update_bits(regmap, MVEBU_RSTOUTN_MASK_REG, bit, bit); > > + regmap_update_bits(regmap, MVEBU_SYS_SOFT_RST_REG, bit, bit); > > + > > + while (1) > > + ; > > A comment before this endless loop might be helpful here. The code does the same as reset_cpu() in cpu.c, and the while() cycle is not commented there. But we can add something like /* something has gone wrong if we reach here, so we may as well stay * here */ What do you think? Could you amend the patch? Marek
On 3/28/24 12:21, Marek Behún wrote: > On Thu, 28 Mar 2024 11:04:45 +0100 > Stefan Roese <sr@denx.de> wrote: > >>> +static int mvebu_sysreset_request(struct udevice *dev, enum sysreset_t type) >>> +{ >>> + struct regmap *regmap = syscon_get_regmap(dev->parent); >>> + uint bit; >>> + >>> + if (type != SYSRESET_COLD) >>> + return -EPROTONOSUPPORT; >>> + >>> + bit = MVEBU_GLOBAL_SOFT_RST_BIT; >>> + >>> + regmap_update_bits(regmap, MVEBU_RSTOUTN_MASK_REG, bit, bit); >>> + regmap_update_bits(regmap, MVEBU_SYS_SOFT_RST_REG, bit, bit); >>> + >>> + while (1) >>> + ; >> >> A comment before this endless loop might be helpful here. > > The code does the same as reset_cpu() in cpu.c, and the while() cycle > is not commented there. Sure, other code might suffer this undocumented endless loop as well. And again, this is more a nitpicking comment than a real requirement. > But we can add something like > /* something has gone wrong if we reach here, so we may as well stay > * here */ > > What do you think? Could you amend the patch? More something like this: /* Loop while waiting for the reset */ while (1) ; And yes, I can fold this into your patchset, if I don't forget about this. Still, if there is need for a v4, then please add it yourself. Thanks, Stefan
On Thu, 28 Mar 2024 14:01:22 +0100 Stefan Roese <sr@denx.de> wrote: > On 3/28/24 12:21, Marek Behún wrote: > > On Thu, 28 Mar 2024 11:04:45 +0100 > > Stefan Roese <sr@denx.de> wrote: > > > >>> +static int mvebu_sysreset_request(struct udevice *dev, enum sysreset_t type) > >>> +{ > >>> + struct regmap *regmap = syscon_get_regmap(dev->parent); > >>> + uint bit; > >>> + > >>> + if (type != SYSRESET_COLD) > >>> + return -EPROTONOSUPPORT; > >>> + > >>> + bit = MVEBU_GLOBAL_SOFT_RST_BIT; > >>> + > >>> + regmap_update_bits(regmap, MVEBU_RSTOUTN_MASK_REG, bit, bit); > >>> + regmap_update_bits(regmap, MVEBU_SYS_SOFT_RST_REG, bit, bit); > >>> + > >>> + while (1) > >>> + ; > >> > >> A comment before this endless loop might be helpful here. > > > > The code does the same as reset_cpu() in cpu.c, and the while() cycle > > is not commented there. > > Sure, other code might suffer this undocumented endless loop as well. > And again, this is more a nitpicking comment than a real requirement. > > > But we can add something like > > /* something has gone wrong if we reach here, so we may as well stay > > * here */ > > > > What do you think? Could you amend the patch? > > More something like this: > > /* Loop while waiting for the reset */ > while (1) > ; As of now I don't see a need for v4. I may sent another patches regarding DDR training, but it will be made on top of this series. Marek
diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig index 623432a60e..f15d3cc5ed 100644 --- a/arch/arm/mach-mvebu/Kconfig +++ b/arch/arm/mach-mvebu/Kconfig @@ -19,6 +19,7 @@ config ARMADA_32BIT select SPL_SYS_NO_VECTOR_TABLE if SPL select ARCH_VERY_EARLY_INIT select ARMADA_32BIT_SYSCON_RESET if DM_RESET && PCI_MVEBU + select ARMADA_32BIT_SYSCON_SYSRESET if SYSRESET # ARMv7 SoCs... config ARMADA_375 @@ -457,16 +458,29 @@ config SF_DEFAULT_MODE default 0x0 depends on MVEBU_SPL_BOOT_DEVICE_SPI +config ARMADA_32BIT_SYSCON + bool + depends on ARMADA_32BIT + select REGMAP + select SYSCON + config ARMADA_32BIT_SYSCON_RESET bool "Support Armada XP/375/38x/39x reset controller" depends on ARMADA_32BIT depends on DM_RESET - select REGMAP - select SYSCON + select ARMADA_32BIT_SYSCON help Build support for Armada XP/375/38x/39x reset controller. This is needed for PCIe support. +config ARMADA_32BIT_SYSCON_SYSRESET + bool "Support Armada XP/375/38x/39x sysreset via driver model" + depends on ARMADA_32BIT + depends on SYSRESET + select ARMADA_32BIT_SYSCON + help + Build support for Armada XP/375/38x/39x system reset via driver model. + source "board/solidrun/clearfog/Kconfig" source "board/kobol/helios4/Kconfig" diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile index d44ca3a0df..329c2e4915 100644 --- a/arch/arm/mach-mvebu/Makefile +++ b/arch/arm/mach-mvebu/Makefile @@ -28,7 +28,7 @@ obj-$(CONFIG_ARMADA_38X) += ../../../drivers/ddr/marvell/a38x/xor.o obj-$(CONFIG_ARMADA_XP) += ../../../drivers/ddr/marvell/axp/xor.o obj-$(CONFIG_ARMADA_MSYS) += ../../../drivers/ddr/marvell/axp/xor.o -obj-$(CONFIG_ARMADA_32BIT_SYSCON_RESET) += system-controller.o +obj-$(CONFIG_ARMADA_32BIT_SYSCON) += system-controller.o ifdef CONFIG_ARMADA_38X obj-$(CONFIG_MVEBU_EFUSE) += efuse.o diff --git a/arch/arm/mach-mvebu/cpu.c b/arch/arm/mach-mvebu/cpu.c index 8e0de93538..7c62a5dbb6 100644 --- a/arch/arm/mach-mvebu/cpu.c +++ b/arch/arm/mach-mvebu/cpu.c @@ -52,6 +52,7 @@ void lowlevel_init(void) */ } +#if defined(CONFIG_SPL_BUILD) || !defined(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) void reset_cpu(void) { struct mvebu_system_registers *reg = @@ -62,6 +63,7 @@ void reset_cpu(void) while (1) ; } +#endif u32 get_boot_device(void) { diff --git a/arch/arm/mach-mvebu/system-controller.c b/arch/arm/mach-mvebu/system-controller.c index c5c05922f2..b5f8afb96d 100644 --- a/arch/arm/mach-mvebu/system-controller.c +++ b/arch/arm/mach-mvebu/system-controller.c @@ -10,11 +10,24 @@ #include <regmap.h> #include <reset-uclass.h> #include <syscon.h> +#include <sysreset.h> #include <asm/io.h> -#define MVEBU_SOC_CONTROL_1_REG 0x4 +#define MVEBU_SOC_CONTROL_1_REG 0x4 -#define MVEBU_PCIE_ID 0 +#if defined(CONFIG_ARMADA_375) +# define MVEBU_RSTOUTN_MASK_REG 0x54 +# define MVEBU_SYS_SOFT_RST_REG 0x58 +#else +# define MVEBU_RSTOUTN_MASK_REG 0x60 +# define MVEBU_SYS_SOFT_RST_REG 0x64 +#endif + +#define MVEBU_GLOBAL_SOFT_RST_BIT BIT(0) + +#define MVEBU_PCIE_ID 0 + +#if IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET) static int mvebu_reset_of_xlate(struct reset_ctl *rst, struct ofnode_phandle_args *args) @@ -90,11 +103,64 @@ U_BOOT_DRIVER(mvebu_reset) = { .ops = &mvebu_reset_ops, }; +#endif /* IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET) */ + +#if IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) + +static int mvebu_sysreset_request(struct udevice *dev, enum sysreset_t type) +{ + struct regmap *regmap = syscon_get_regmap(dev->parent); + uint bit; + + if (type != SYSRESET_COLD) + return -EPROTONOSUPPORT; + + bit = MVEBU_GLOBAL_SOFT_RST_BIT; + + regmap_update_bits(regmap, MVEBU_RSTOUTN_MASK_REG, bit, bit); + regmap_update_bits(regmap, MVEBU_SYS_SOFT_RST_REG, bit, bit); + + while (1) + ; + + return 0; +} + +static struct sysreset_ops mvebu_sysreset_ops = { + .request = mvebu_sysreset_request, +}; + +U_BOOT_DRIVER(mvebu_sysreset) = { + .name = "mvebu-sysreset", + .id = UCLASS_SYSRESET, + .ops = &mvebu_sysreset_ops, +}; + +#endif /* IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET) */ + static int mvebu_syscon_bind(struct udevice *dev) { + int ret = 0; + /* bind also mvebu-reset, with the same ofnode */ - return device_bind_driver_to_node(dev, "mvebu-reset", "mvebu-reset", - dev_ofnode(dev), NULL); + if (IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_RESET)) { + ret = device_bind_driver_to_node(dev, "mvebu-reset", + "mvebu-reset", dev_ofnode(dev), + NULL); + if (ret < 0) + return ret; + } + + /* bind also mvebu-sysreset, with the same ofnode */ + if (IS_ENABLED(CONFIG_ARMADA_32BIT_SYSCON_SYSRESET)) { + ret = device_bind_driver_to_node(dev, "mvebu-sysreset", + "mvebu-sysreset", + dev_ofnode(dev), NULL); + if (ret < 0) + return ret; + } + + return ret; } static const struct udevice_id mvebu_syscon_of_match[] = {
Add driver model support for sysreset via mvebu system controller. This is currently only available for U-Boot proper. Signed-off-by: Marek Behún <kabel@kernel.org> --- arch/arm/mach-mvebu/Kconfig | 18 +++++- arch/arm/mach-mvebu/Makefile | 2 +- arch/arm/mach-mvebu/cpu.c | 2 + arch/arm/mach-mvebu/system-controller.c | 74 +++++++++++++++++++++++-- 4 files changed, 89 insertions(+), 7 deletions(-)