* [U-Boot] [PATCH] syscon: reset node list syscon_list after relocation
@ 2018-10-12 15:26 Patrick Delaunay
2018-10-19 3:25 ` Simon Glass
0 siblings, 1 reply; 3+ messages in thread
From: Patrick Delaunay @ 2018-10-12 15:26 UTC (permalink / raw)
To: u-boot
Reset the list head after the reallocation because the list syscon_list
use allocated pointer and they are no more valid.
This patch avoid issue (crash) when syscon_node_to_regmap() is called
before and after reallocation.
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---
Hi
This patch correct a crash see on v2018.11-rc1 with my board stm32mp1
for the command "reset".
The crash is a side effect of 2 patches
1f6ca3f42f6edf143473159297f4c515b1cf36f6
sysreset: syscon: update regmap access to syscon
23471aed5c33e104d6fa64575932577618543bec
board_f: Add reset status printing
With the first patch the syscon_node_to_regmap() is called
each time that the class sysreset_syscon is probed.
=> in v2018.09 probe is done only when reset is requested
NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to
support reset in pre-reloc phases (allow panic).
With the second patch, U-Boot probes all the sysreset uclass in preloc
phase to allow to print the reset status, and the list is initialized
in board_f / pre-reloc:
-> print_resetinfo
--> uclass_first_device_err(UCLASS_SYSRESET, &dev);
---> syscon_reboot_probe()
----> syscon_node_to_regmap(node)
-----> of_syscon_register()
-------> list_add_tail(&syscon->list, &syscon_list);
During relocation, the content of syscon_list (static) is updated
but the list use pointers allocated by malloc in pre-reloc phasis
And when I execute the reset command
-> do_reset()
--> reset_cpu()
---> sysreset_walk_halt(SYSRESET_COLD);
----> loop on device UCLASS_SYSRESET
-----> syscon_reboot_probe()
------> syscon_node_to_regmap(node)
-------> list_for_each_entry(entry, &syscon_list, list)
I have a crash here because the pointer syscon_list.next is invalid.
I solve the issue by resetting the list after reloc and it is working
but I don't know if a more elegant solution exist
(keep the list in .bss section to be set to 0).
Regards
Patrick
drivers/core/syscon-uclass.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c
index 303e166..aacb43a 100644
--- a/drivers/core/syscon-uclass.c
+++ b/drivers/core/syscon-uclass.c
@@ -156,8 +156,15 @@ static struct syscon *of_syscon_register(ofnode node)
struct regmap *syscon_node_to_regmap(ofnode node)
{
+ static int reloc_done;
struct syscon *entry, *syscon = NULL;
+ /* content of list is not relocated (malloc): reinit when needed */
+ if (!reloc_done && (gd->flags & GD_FLG_RELOC)) {
+ INIT_LIST_HEAD(&syscon_list);
+ reloc_done++;
+ }
+
list_for_each_entry(entry, &syscon_list, list)
if (ofnode_equal(entry->node, node)) {
syscon = entry;
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH] syscon: reset node list syscon_list after relocation
2018-10-12 15:26 [U-Boot] [PATCH] syscon: reset node list syscon_list after relocation Patrick Delaunay
@ 2018-10-19 3:25 ` Simon Glass
2018-10-22 18:10 ` Patrick DELAUNAY
0 siblings, 1 reply; 3+ messages in thread
From: Simon Glass @ 2018-10-19 3:25 UTC (permalink / raw)
To: u-boot
Hi Patrick,
On 12 October 2018 at 09:26, Patrick Delaunay <patrick.delaunay@st.com> wrote:
> Reset the list head after the reallocation because the list syscon_list
> use allocated pointer and they are no more valid.
> This patch avoid issue (crash) when syscon_node_to_regmap() is called
> before and after reallocation.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> Hi
>
> This patch correct a crash see on v2018.11-rc1 with my board stm32mp1
> for the command "reset".
>
> The crash is a side effect of 2 patches
> 1f6ca3f42f6edf143473159297f4c515b1cf36f6
> sysreset: syscon: update regmap access to syscon
>
> 23471aed5c33e104d6fa64575932577618543bec
> board_f: Add reset status printing
>
> With the first patch the syscon_node_to_regmap() is called
> each time that the class sysreset_syscon is probed.
>
> => in v2018.09 probe is done only when reset is requested
>
> NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to
> support reset in pre-reloc phases (allow panic).
>
> With the second patch, U-Boot probes all the sysreset uclass in preloc
> phase to allow to print the reset status, and the list is initialized
> in board_f / pre-reloc:
>
> -> print_resetinfo
> --> uclass_first_device_err(UCLASS_SYSRESET, &dev);
> ---> syscon_reboot_probe()
> ----> syscon_node_to_regmap(node)
> -----> of_syscon_register()
> -------> list_add_tail(&syscon->list, &syscon_list);
>
> During relocation, the content of syscon_list (static) is updated
> but the list use pointers allocated by malloc in pre-reloc phasis
>
> And when I execute the reset command
> -> do_reset()
> --> reset_cpu()
> ---> sysreset_walk_halt(SYSRESET_COLD);
> ----> loop on device UCLASS_SYSRESET
> -----> syscon_reboot_probe()
> ------> syscon_node_to_regmap(node)
> -------> list_for_each_entry(entry, &syscon_list, list)
We have a similar issue with the timer - we set gd->timer to NULL in initr_dm().
I am not 100% what is going on here, but using pre-reloc devices (i.e.
which were set up before relocation) after relocation is bad. We need
to avoid that.
The syscon_list struct should be in the syscon uclass, i.e. declared
as uclass priv data in UCLASS_DRIVER(syscon). We should not have this
as free-standard static data. That's not how DM works...
So I guess Masahiro's patch needs to be adjusted.
Regards,
Simon
^ permalink raw reply [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH] syscon: reset node list syscon_list after relocation
2018-10-19 3:25 ` Simon Glass
@ 2018-10-22 18:10 ` Patrick DELAUNAY
0 siblings, 0 replies; 3+ messages in thread
From: Patrick DELAUNAY @ 2018-10-22 18:10 UTC (permalink / raw)
To: u-boot
Hi Simon,
> From: sjg at google.com <sjg@google.com> On Behalf Of Simon Glass
> Sent: vendredi 19 octobre 2018 05:25
>
> Hi Patrick,
>
> On 12 October 2018 at 09:26, Patrick Delaunay <patrick.delaunay@st.com>
> wrote:
> > Reset the list head after the reallocation because the list
> > syscon_list use allocated pointer and they are no more valid.
> > This patch avoid issue (crash) when syscon_node_to_regmap() is called
> > before and after reallocation.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> > Hi
> >
> > This patch correct a crash see on v2018.11-rc1 with my board stm32mp1
> > for the command "reset".
> >
> > The crash is a side effect of 2 patches
> > 1f6ca3f42f6edf143473159297f4c515b1cf36f6
> > sysreset: syscon: update regmap access to syscon
> >
> > 23471aed5c33e104d6fa64575932577618543bec
> > board_f: Add reset status printing
> >
> > With the first patch the syscon_node_to_regmap() is called each time
> > that the class sysreset_syscon is probed.
> >
> > => in v2018.09 probe is done only when reset is requested
> >
> > NB: for stm32mp1, the node rcc_reboot in tagged pre-relocated to
> > support reset in pre-reloc phases (allow panic).
> >
> > With the second patch, U-Boot probes all the sysreset uclass in preloc
> > phase to allow to print the reset status, and the list is initialized
> > in board_f / pre-reloc:
> >
> > -> print_resetinfo
> > --> uclass_first_device_err(UCLASS_SYSRESET, &dev);
> > ---> syscon_reboot_probe()
> > ----> syscon_node_to_regmap(node)
> > -----> of_syscon_register()
> > -------> list_add_tail(&syscon->list, &syscon_list);
> >
> > During relocation, the content of syscon_list (static) is updated but
> > the list use pointers allocated by malloc in pre-reloc phasis
> >
> > And when I execute the reset command
> > -> do_reset()
> > --> reset_cpu()
> > ---> sysreset_walk_halt(SYSRESET_COLD);
> > ----> loop on device UCLASS_SYSRESET
> > -----> syscon_reboot_probe()
> > ------> syscon_node_to_regmap(node)
> > -------> list_for_each_entry(entry, &syscon_list, list)
>
> We have a similar issue with the timer - we set gd->timer to NULL in initr_dm().
>
> I am not 100% what is going on here, but using pre-reloc devices (i.e.
> which were set up before relocation) after relocation is bad. We need to avoid
> that.
Ok, I agree with you.
> The syscon_list struct should be in the syscon uclass, i.e. declared as uclass priv
> data in UCLASS_DRIVER(syscon). We should not have this as free-standard static
> data. That's not how DM works...
> So I guess Masahiro's patch needs to be adjusted.
Today, I read your comment and I start to move the list in syscon uclass priv data (I just discovered the uclass priv data).
But after some time spent with this list, I prefer completely remove it and
have the same behavior with DM functions (as a list is already managed for the drivers).
It is more that only a adjustment, I completely update the function syscon_node_to_regmap():
force bound to syscon driver and probe it when the syscon driver don't yet exist.....
=> Do you think that is a good solution ?
The function change to:
struct regmap *syscon_node_to_regmap(ofnode node)
{
struct udevice *dev;
struct udevice *parent;
ofnode ofnode = node;
int ret;
if (!uclass_get_device_by_ofnode(UCLASS_SYSCON, node, &dev))
return syscon_get_regmap(dev);
if (!ofnode_device_is_compatible(node, "syscon"))
return ERR_PTR(-EINVAL);
if (device_find_global_by_ofnode(ofnode, &parent))
parent = dm_root();
/* force bound to syscon class */
ret = device_bind_driver_to_node(parent, "syscon",
ofnode_get_name(node),
node, &dev);
if (ret)
return ERR_PTR(ret);
ret = device_probe(dev);
if (ret)
return ERR_PTR(ret);
return syscon_get_regmap(dev);
}
Tested on my board, the initial issue is sole and I check the behavior with dm_dump_all();
It seems OK for my case (RCC driver) .
before relocation => syscon is correctly probed
Class index Probed Driver Name
-----------------------------------------
root 0 [ + ] root_drive root_driver
misc 0 [ ] stm32mp_bs |-- stm32mp_bsec
simple_bus 0 [ + ] generic_si |-- soc
serial 0 [ + ] serial_stm | |-- serial at 40010000
misc 1 [ + ] stm32-rcc | |-- rcc at 50000000
clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk
reset 0 [ ] stm32_rcc_ | | |-- stm32_rcc_reset
syscon 1 [ + ] syscon | | `-- rcc at 50000000 <<<< SYSCON PROBED
sysreset 0 [ + ] syscon_reb | |-- rcc-reboot at 50000000
mmc 0 [ ] stm32_sdmm | |-- sdmmc at 58005000
blk 0 [ ] mmc_blk | | `-- sdmmc@58005000.blk
And it is also the case for syscon reset (when I execute the command reset)
STM32MP> reset
resetting ...
STM32MP> reset
resetting ...
Class index Probed Driver Name
-----------------------------------------
root 0 [ + ] root_drive root_driver
misc 0 [ + ] stm32mp_bs |-- stm32mp_bsec
simple_bus 0 [ + ] generic_si |-- soc
serial 0 [ + ] serial_stm | |-- serial at 40010000
i2c 0 [ ] stm32f7-i2 | |-- i2c at 40013000
i2c 1 [ ] stm32f7-i2 | |-- i2c at 40015000
usb 0 [ ] dwc2_usb | |-- usb-otg at 49000000
misc 1 [ + ] stm32-rcc | |-- rcc at 50000000
clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk
reset 0 [ + ] stm32_rcc_ | | |-- stm32_rcc_reset
syscon 4 [ + ] syscon | | `-- rcc at 50000000 <<<<< SYSCON
sysreset 0 [ + ] syscon_reb | |-- rcc-reboot@50000000
And the driver is not binded/probed by default (checked with dm tree)
STM32MP> dm tree
Class index Probed Driver Name
-----------------------------------------
root 0 [ + ] root_drive root_driver
misc 0 [ + ] stm32mp_bs |-- stm32mp_bsec
simple_bus 0 [ + ] generic_si |-- soc
serial 0 [ + ] serial_stm | |-- serial at 40010000
i2c 0 [ ] stm32f7-i2 | |-- i2c at 40013000
i2c 1 [ ] stm32f7-i2 | |-- i2c at 40015000
usb 0 [ ] dwc2_usb | |-- usb-otg at 49000000
misc 1 [ + ] stm32-rcc | |-- rcc at 50000000 <<<<< NO SYSCON child
clk 0 [ + ] stm32mp1_c | | |-- stm32mp1_clk
reset 0 [ + ] stm32_rcc_ | | `-- stm32_rcc_reset
sysreset 0 [ ] syscon_reb | |-- rcc-reboot at 50000000
syscon 0 [ ] stmp32mp_s | |-- pwr@50001000
> Regards,
> Simon
Regards
Patrick
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-22 18:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 15:26 [U-Boot] [PATCH] syscon: reset node list syscon_list after relocation Patrick Delaunay
2018-10-19 3:25 ` Simon Glass
2018-10-22 18:10 ` Patrick DELAUNAY
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.