HI Patrick,
On 30 October 2018 at 07:44, Patrick Delaunay <patrick.delau...@st.com> wrote:
>
> + Update the function syscon_node_to_regmap() to force bound on
> syscon uclass and directly use the list of device from DM.
> + Remove the static list syscon_list.
>
> This patch avoid issue (crash) when syscon_node_to_regmap() is called
> before and after reallocation (list content is invalid).
>
> Signed-off-by: Patrick Delaunay <patrick.delau...@st.com>
> ---
> Hi
>
> This patch is a modified (after Simon Glass review)
> and rebased version on v2018.11-rc3 for
> [U-Boot] syscon: reset node list syscon_list after relocation
> http://patchwork.ozlabs.org/patch/983139/
>
> This patch correct a crash see on v2018.11-rc1 with my board stm32mp1
> for the command "reset", reproduced on v2018.11-rc3.
>
> 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.
>
> This patch solves the issue by using DM list for syscon uclass.
>
> Tested on my board, the initial issue is solved and
> I check the behavior with dm_dump_all():
> the RCC driver is not binded/probed as syscon uclass by default
> (checked with dm tree) and correctly bounded and probed when
> it is necessary (before relocation: print_resetinfo() or after
> relocation for command reset).
>
> Regards
>
> Patrick
>
>
> drivers/core/syscon-uclass.c | 55
> ++++++++++++++------------------------------
> 1 file changed, 17 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c
> index 303e166..aeb3583 100644
> --- a/drivers/core/syscon-uclass.c
> +++ b/drivers/core/syscon-uclass.c
> @@ -123,52 +123,31 @@ U_BOOT_DRIVER(generic_syscon) = {
> * The syscon node can be bound to another driver, but still works
> * as a syscon provider.
> */
> -static LIST_HEAD(syscon_list);
> -
> -struct syscon {
> - ofnode node;
> - struct regmap *regmap;
> - struct list_head list;
> -};
> -
> -static struct syscon *of_syscon_register(ofnode node)
> +struct regmap *syscon_node_to_regmap(ofnode node)
> {
> - struct syscon *syscon;
> + struct udevice *dev, *parent;
> + ofnode ofnode = node;
What is the purpose of this variable if it is always set to '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);
>
> - syscon = malloc(sizeof(*syscon));
> - if (!syscon)
> - return ERR_PTR(-ENOMEM);
> + if (device_find_global_by_ofnode(ofnode, &parent))
> + parent = dm_root();
So if you fail to find the node, you use the root? Is this so that you
have somewhere to 'hang' the device? I think this code could all use a
few comments.
>
> - ret = regmap_init_mem(node, &syscon->regmap);
> - if (ret) {
> - free(syscon);
> + /* 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);
> - }
> -
> - list_add_tail(&syscon->list, &syscon_list);
> -
> - return syscon;
> -}
>
> -struct regmap *syscon_node_to_regmap(ofnode node)
> -{
> - struct syscon *entry, *syscon = NULL;
> -
> - list_for_each_entry(entry, &syscon_list, list)
> - if (ofnode_equal(entry->node, node)) {
> - syscon = entry;
> - break;
> - }
> -
> - if (!syscon)
> - syscon = of_syscon_register(node);
> -
> - if (IS_ERR(syscon))
> - return ERR_CAST(syscon);
> + ret = device_probe(dev);
> + if (ret)
> + return ERR_PTR(ret);
>
> - return syscon->regmap;
> + return syscon_get_regmap(dev);
> }
> --
> 2.7.4
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
