[U-Boot] [PATCH] syscon: reset node list syscon_list after relocation

[Patrick DELAUNAY]

Hi Simon,

> From: sjg at google.com <sjg at 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 at 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 at 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 at 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 at 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 at 50001000


> Regards,
> Simon

Regards
Patrick