| Message ID | 1692120000-46900-5-git-send-email-lizhi.hou@amd.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Generate device tree node for pci devices | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
Hi Lizhi, On Tue, 15 Aug 2023, Lizhi Hou wrote: > Currently, in an overlay fdt fragment, it needs to specify the exact > location in base DT. In another word, when the fdt fragment is generated, > the base DT location for the fragment is already known. > > There is new use case that the base DT location is unknown when fdt > fragment is generated. For example, the add-on device provide a fdt > overlay with its firmware to describe its downstream devices. Because it > is add-on device which can be plugged to different systems, its firmware > will not be able to know the overlay location in base DT. Instead, the > device driver will load the overlay fdt and apply it to base DT at runtime. > In this case, of_overlay_fdt_apply() needs to be extended to specify > the target node for device driver to apply overlay fdt. > int overlay_fdt_apply(..., struct device_node *base); > > Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> Thanks for your patch, which is now commit 47284862bfc7fd56 ("of: overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next. > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -715,6 +730,7 @@ static struct device_node *find_target(struct device_node *info_node) > /** > * init_overlay_changeset() - initialize overlay changeset from overlay tree > * @ovcs: Overlay changeset to build > + * @target_base: Point to the target node to apply overlay > * > * Initialize @ovcs. Populate @ovcs->fragments with node information from > * the top level of @overlay_root. The relevant top level nodes are the As an overlay can contain one or more fragments, this means the base (when specified) will be applied to all fragments, and will thus override the target-path properties in all fragments. However, for the use case of an overlay that you can plug into a random location (and of which there can be multiple instances), there can really be only a single fragment. Even nodes that typically live at the root level (e.g. gpio-leds or gpio-keys) must be inserted below the specified location, to avoid conflicts. Hence: 1. Should init_overlay_changeset() return -EINVAL if target_base is specified, and there is more than one fragment? 2. Should there be a convention about the target-path property's contents in the original overlay? drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5] of: unittest: Add pci_dt_testdrv pci driver" uses target-path=""; which cannot be represented when using sugar syntax. "/" should work fine, though. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for reviewing the patch. I add my comment in-line. On 8/24/23 01:31, Geert Uytterhoeven wrote: > Hi Lizhi, > > On Tue, 15 Aug 2023, Lizhi Hou wrote: >> Currently, in an overlay fdt fragment, it needs to specify the exact >> location in base DT. In another word, when the fdt fragment is >> generated, >> the base DT location for the fragment is already known. >> >> There is new use case that the base DT location is unknown when fdt >> fragment is generated. For example, the add-on device provide a fdt >> overlay with its firmware to describe its downstream devices. Because it >> is add-on device which can be plugged to different systems, its firmware >> will not be able to know the overlay location in base DT. Instead, the >> device driver will load the overlay fdt and apply it to base DT at >> runtime. >> In this case, of_overlay_fdt_apply() needs to be extended to specify >> the target node for device driver to apply overlay fdt. >> int overlay_fdt_apply(..., struct device_node *base); >> >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> > > Thanks for your patch, which is now commit 47284862bfc7fd56 ("of: > overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next. > >> --- a/drivers/of/overlay.c >> +++ b/drivers/of/overlay.c >> @@ -715,6 +730,7 @@ static struct device_node *find_target(struct >> device_node *info_node) >> /** >> * init_overlay_changeset() - initialize overlay changeset from >> overlay tree >> * @ovcs: Overlay changeset to build >> + * @target_base: Point to the target node to apply overlay >> * >> * Initialize @ovcs. Populate @ovcs->fragments with node information >> from >> * the top level of @overlay_root. The relevant top level nodes are the > > As an overlay can contain one or more fragments, this means the > base (when specified) will be applied to all fragments, and will thus > override the target-path properties in all fragments. > > However, for the use case of an overlay that you can plug into > a random location (and of which there can be multiple instances), > there can really be only a single fragment. Even nodes that typically > live at the root level (e.g. gpio-leds or gpio-keys) must be inserted > below the specified location, to avoid conflicts. > > Hence: > 1. Should init_overlay_changeset() return -EINVAL if target_base is > specified, and there is more than one fragment? Maybe allowing more than one fragment make the interface more generic? For example, it could support the use case that multiple fragments share the same base node. Currently, the fragment overlay path is "base node path" + "fragment target path". Thus, for the structure: /a/b/c/fragment0 /a/b/d/fagment1 It can be two fragments in one fdt by using base node path = /a/b fragment0 target path = /c fragment1 target path = /d I am not sure if there will be this kind of use case or not. And I think it would not be hurt to allow that. > > 2. Should there be a convention about the target-path property's > contents in the original overlay? > drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5] > of: unittest: Add pci_dt_testdrv pci driver" uses > > target-path=""; > > which cannot be represented when using sugar syntax. > "/" should work fine, though. Because the fragment overlay path is "base node path" + "fragment target path", I may add code to check if "fragment target patch is '/' and ignore it. I think that would support sugar syntax with only '/' specified. Thanks, Lizhi > > Thanks! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a > hacker. But > when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
On Thu, Aug 24, 2023 at 1:40 PM Lizhi Hou <lizhi.hou@amd.com> wrote: > > Hi Geert, > > Thanks for reviewing the patch. I add my comment in-line. > > On 8/24/23 01:31, Geert Uytterhoeven wrote: > > Hi Lizhi, > > > > On Tue, 15 Aug 2023, Lizhi Hou wrote: > >> Currently, in an overlay fdt fragment, it needs to specify the exact > >> location in base DT. In another word, when the fdt fragment is > >> generated, > >> the base DT location for the fragment is already known. > >> > >> There is new use case that the base DT location is unknown when fdt > >> fragment is generated. For example, the add-on device provide a fdt > >> overlay with its firmware to describe its downstream devices. Because it > >> is add-on device which can be plugged to different systems, its firmware > >> will not be able to know the overlay location in base DT. Instead, the > >> device driver will load the overlay fdt and apply it to base DT at > >> runtime. > >> In this case, of_overlay_fdt_apply() needs to be extended to specify > >> the target node for device driver to apply overlay fdt. > >> int overlay_fdt_apply(..., struct device_node *base); > >> > >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> > > > > Thanks for your patch, which is now commit 47284862bfc7fd56 ("of: > > overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next. > > > >> --- a/drivers/of/overlay.c > >> +++ b/drivers/of/overlay.c > >> @@ -715,6 +730,7 @@ static struct device_node *find_target(struct > >> device_node *info_node) > >> /** > >> * init_overlay_changeset() - initialize overlay changeset from > >> overlay tree > >> * @ovcs: Overlay changeset to build > >> + * @target_base: Point to the target node to apply overlay > >> * > >> * Initialize @ovcs. Populate @ovcs->fragments with node information > >> from > >> * the top level of @overlay_root. The relevant top level nodes are the > > > > As an overlay can contain one or more fragments, this means the > > base (when specified) will be applied to all fragments, and will thus > > override the target-path properties in all fragments. > > > > However, for the use case of an overlay that you can plug into > > a random location (and of which there can be multiple instances), > > there can really be only a single fragment. Even nodes that typically > > live at the root level (e.g. gpio-leds or gpio-keys) must be inserted > > below the specified location, to avoid conflicts. It's not a random location, but a location where the full path and/or unit-address are not known. What we should know is the node's base name and compatible. I think we can assume for this kind of usecase, that adding nodes only under a defined base node is allowed. This is also just the restriction I've asked for every time more general support of applying overlays by the kernel is requested. The add-on card, hat, cape, etc. usecases should all be applied downstream of some node. > > > > Hence: > > 1. Should init_overlay_changeset() return -EINVAL if target_base is > > specified, and there is more than one fragment? > > Maybe allowing more than one fragment make the interface more generic? > For example, it could support the use case that multiple fragments share > the same base node. > > Currently, the fragment overlay path is "base node path" + "fragment > target path". Thus, for the structure: > > /a/b/c/fragment0 > > /a/b/d/fagment1 > > It can be two fragments in one fdt by using > > base node path = /a/b > > fragment0 target path = /c > > fragment1 target path = /d > > I am not sure if there will be this kind of use case or not. And I think > it would not be hurt to allow that. > > > > > 2. Should there be a convention about the target-path property's > > contents in the original overlay? > > drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5] > > of: unittest: Add pci_dt_testdrv pci driver" uses > > > > target-path=""; > > > > which cannot be represented when using sugar syntax. > > "/" should work fine, though. > > Because the fragment overlay path is "base node path" + "fragment target > path", I may add code to check if "fragment target patch is '/' and > ignore it. I think that would support sugar syntax with only '/' specified. Note that "/" is also a valid target path. I think it would be better to have a form that's obviously not a fixed path. I think what's needed is to be able to specify just the nodename with or without the unit-address. I don't know if dtc will accept that. As labels are part of the ABI with overlays, a target label could also work. Though the kernel would have to learn to add new labels or get a label path from another source as a label doesn't exist on a generated node. Rob
Hi Lizhi, On Thu, Aug 24, 2023 at 8:40 PM Lizhi Hou <lizhi.hou@amd.com> wrote: > On 8/24/23 01:31, Geert Uytterhoeven wrote: > > On Tue, 15 Aug 2023, Lizhi Hou wrote: > >> Currently, in an overlay fdt fragment, it needs to specify the exact > >> location in base DT. In another word, when the fdt fragment is > >> generated, > >> the base DT location for the fragment is already known. > >> > >> There is new use case that the base DT location is unknown when fdt > >> fragment is generated. For example, the add-on device provide a fdt > >> overlay with its firmware to describe its downstream devices. Because it > >> is add-on device which can be plugged to different systems, its firmware > >> will not be able to know the overlay location in base DT. Instead, the > >> device driver will load the overlay fdt and apply it to base DT at > >> runtime. > >> In this case, of_overlay_fdt_apply() needs to be extended to specify > >> the target node for device driver to apply overlay fdt. > >> int overlay_fdt_apply(..., struct device_node *base); > >> > >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> > > > > Thanks for your patch, which is now commit 47284862bfc7fd56 ("of: > > overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next. > > > >> --- a/drivers/of/overlay.c > >> +++ b/drivers/of/overlay.c > >> @@ -715,6 +730,7 @@ static struct device_node *find_target(struct > >> device_node *info_node) > >> /** > >> * init_overlay_changeset() - initialize overlay changeset from > >> overlay tree > >> * @ovcs: Overlay changeset to build > >> + * @target_base: Point to the target node to apply overlay > >> * > >> * Initialize @ovcs. Populate @ovcs->fragments with node information > >> from > >> * the top level of @overlay_root. The relevant top level nodes are the > > > > As an overlay can contain one or more fragments, this means the > > base (when specified) will be applied to all fragments, and will thus > > override the target-path properties in all fragments. > > > > However, for the use case of an overlay that you can plug into > > a random location (and of which there can be multiple instances), > > there can really be only a single fragment. Even nodes that typically > > live at the root level (e.g. gpio-leds or gpio-keys) must be inserted > > below the specified location, to avoid conflicts. > > > > Hence: > > 1. Should init_overlay_changeset() return -EINVAL if target_base is > > specified, and there is more than one fragment? > > Maybe allowing more than one fragment make the interface more generic? > For example, it could support the use case that multiple fragments share > the same base node. > > Currently, the fragment overlay path is "base node path" + "fragment > target path". Thus, for the structure: Oh, I had missed that the "fragment target path" is appended, and thought it was just overridden. > /a/b/c/fragment0 > > /a/b/d/fagment1 > > It can be two fragments in one fdt by using > > base node path = /a/b > > fragment0 target path = /c > > fragment1 target path = /d > > I am not sure if there will be this kind of use case or not. And I think > it would not be hurt to allow that. Is there a need for that? Both c and d can be handled as subnodes in a single fragment if the target path is empty (and see below). > > 2. Should there be a convention about the target-path property's > > contents in the original overlay? > > drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5] > > of: unittest: Add pci_dt_testdrv pci driver" uses > > > > target-path=""; > > > > which cannot be represented when using sugar syntax. > > "/" should work fine, though. > > Because the fragment overlay path is "base node path" + "fragment target > path", I may add code to check if "fragment target patch is '/' and > ignore it. I think that would support sugar syntax with only '/' specified. That makes sense. Thanks! Gr{oetje,eeting}s, Geert
On 8/25/23 00:25, Geert Uytterhoeven wrote: > Hi Lizhi, > > On Thu, Aug 24, 2023 at 8:40 PM Lizhi Hou <lizhi.hou@amd.com> wrote: >> On 8/24/23 01:31, Geert Uytterhoeven wrote: >>> On Tue, 15 Aug 2023, Lizhi Hou wrote: >>>> Currently, in an overlay fdt fragment, it needs to specify the exact >>>> location in base DT. In another word, when the fdt fragment is >>>> generated, >>>> the base DT location for the fragment is already known. >>>> >>>> There is new use case that the base DT location is unknown when fdt >>>> fragment is generated. For example, the add-on device provide a fdt >>>> overlay with its firmware to describe its downstream devices. Because it >>>> is add-on device which can be plugged to different systems, its firmware >>>> will not be able to know the overlay location in base DT. Instead, the >>>> device driver will load the overlay fdt and apply it to base DT at >>>> runtime. >>>> In this case, of_overlay_fdt_apply() needs to be extended to specify >>>> the target node for device driver to apply overlay fdt. >>>> int overlay_fdt_apply(..., struct device_node *base); >>>> >>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> >>> Thanks for your patch, which is now commit 47284862bfc7fd56 ("of: >>> overlay: Extend of_overlay_fdt_apply() in dt-rh/for-next. >>> >>>> --- a/drivers/of/overlay.c >>>> +++ b/drivers/of/overlay.c >>>> @@ -715,6 +730,7 @@ static struct device_node *find_target(struct >>>> device_node *info_node) >>>> /** >>>> * init_overlay_changeset() - initialize overlay changeset from >>>> overlay tree >>>> * @ovcs: Overlay changeset to build >>>> + * @target_base: Point to the target node to apply overlay >>>> * >>>> * Initialize @ovcs. Populate @ovcs->fragments with node information >>>> from >>>> * the top level of @overlay_root. The relevant top level nodes are the >>> As an overlay can contain one or more fragments, this means the >>> base (when specified) will be applied to all fragments, and will thus >>> override the target-path properties in all fragments. >>> >>> However, for the use case of an overlay that you can plug into >>> a random location (and of which there can be multiple instances), >>> there can really be only a single fragment. Even nodes that typically >>> live at the root level (e.g. gpio-leds or gpio-keys) must be inserted >>> below the specified location, to avoid conflicts. >>> >>> Hence: >>> 1. Should init_overlay_changeset() return -EINVAL if target_base is >>> specified, and there is more than one fragment? >> Maybe allowing more than one fragment make the interface more generic? >> For example, it could support the use case that multiple fragments share >> the same base node. >> >> Currently, the fragment overlay path is "base node path" + "fragment >> target path". Thus, for the structure: > Oh, I had missed that the "fragment target path" is appended, > and thought it was just overridden. > >> /a/b/c/fragment0 >> >> /a/b/d/fagment1 >> >> It can be two fragments in one fdt by using >> >> base node path = /a/b >> >> fragment0 target path = /c >> >> fragment1 target path = /d >> >> I am not sure if there will be this kind of use case or not. And I think >> it would not be hurt to allow that. > Is there a need for that? Both c and d can be handled as subnodes > in a single fragment if the target path is empty (and see below). In our use case, we do not need that. I am just not sure if it should be adding the restriction to limit one fragment here. Because the fragment target path is appended to the base node path, each fragment is still applied to a specific location as before. The only difference is the fragment target path does not need to always start with "/". Thanks, Lizhi > >>> 2. Should there be a convention about the target-path property's >>> contents in the original overlay? >>> drivers/of/unittest-data/overlay_pci_node.dtso in "[PATCH V13 5/5] >>> of: unittest: Add pci_dt_testdrv pci driver" uses >>> >>> target-path=""; >>> >>> which cannot be represented when using sugar syntax. >>> "/" should work fine, though. >> Because the fragment overlay path is "base node path" + "fragment target >> path", I may add code to check if "fragment target patch is '/' and >> ignore it. I think that would support sugar syntax with only '/' specified. > That makes sense. > Thanks! > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index 7feb643f1370..6f3ae30c878d 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -682,9 +682,11 @@ static int build_changeset(struct overlay_changeset *ovcs) * 1) "target" property containing the phandle of the target * 2) "target-path" property containing the path of the target */ -static struct device_node *find_target(struct device_node *info_node) +static struct device_node *find_target(struct device_node *info_node, + struct device_node *target_base) { struct device_node *node; + char *target_path; const char *path; u32 val; int ret; @@ -700,10 +702,23 @@ static struct device_node *find_target(struct device_node *info_node) ret = of_property_read_string(info_node, "target-path", &path); if (!ret) { - node = of_find_node_by_path(path); - if (!node) - pr_err("find target, node: %pOF, path '%s' not found\n", - info_node, path); + if (target_base) { + target_path = kasprintf(GFP_KERNEL, "%pOF%s", target_base, path); + if (!target_path) + return NULL; + node = of_find_node_by_path(target_path); + if (!node) { + pr_err("find target, node: %pOF, path '%s' not found\n", + info_node, target_path); + } + kfree(target_path); + } else { + node = of_find_node_by_path(path); + if (!node) { + pr_err("find target, node: %pOF, path '%s' not found\n", + info_node, path); + } + } return node; } @@ -715,6 +730,7 @@ static struct device_node *find_target(struct device_node *info_node) /** * init_overlay_changeset() - initialize overlay changeset from overlay tree * @ovcs: Overlay changeset to build + * @target_base: Point to the target node to apply overlay * * Initialize @ovcs. Populate @ovcs->fragments with node information from * the top level of @overlay_root. The relevant top level nodes are the @@ -725,7 +741,8 @@ static struct device_node *find_target(struct device_node *info_node) * detected in @overlay_root. On error return, the caller of * init_overlay_changeset() must call free_overlay_changeset(). */ -static int init_overlay_changeset(struct overlay_changeset *ovcs) +static int init_overlay_changeset(struct overlay_changeset *ovcs, + struct device_node *target_base) { struct device_node *node, *overlay_node; struct fragment *fragment; @@ -786,7 +803,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs) fragment = &fragments[cnt]; fragment->overlay = overlay_node; - fragment->target = find_target(node); + fragment->target = find_target(node, target_base); if (!fragment->target) { of_node_put(fragment->overlay); ret = -EINVAL; @@ -877,6 +894,7 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * * of_overlay_apply() - Create and apply an overlay changeset * @ovcs: overlay changeset + * @base: point to the target node to apply overlay * * Creates and applies an overlay changeset. * @@ -900,7 +918,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * the caller of of_overlay_apply() must call free_overlay_changeset(). */ -static int of_overlay_apply(struct overlay_changeset *ovcs) +static int of_overlay_apply(struct overlay_changeset *ovcs, + struct device_node *base) { int ret = 0, ret_revert, ret_tmp; @@ -908,7 +927,7 @@ static int of_overlay_apply(struct overlay_changeset *ovcs) if (ret) goto out; - ret = init_overlay_changeset(ovcs); + ret = init_overlay_changeset(ovcs, base); if (ret) goto out; @@ -952,6 +971,7 @@ static int of_overlay_apply(struct overlay_changeset *ovcs) * @overlay_fdt: pointer to overlay FDT * @overlay_fdt_size: number of bytes in @overlay_fdt * @ret_ovcs_id: pointer for returning created changeset id + * @base: pointer for the target node to apply overlay * * Creates and applies an overlay changeset. * @@ -967,7 +987,7 @@ static int of_overlay_apply(struct overlay_changeset *ovcs) */ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, - int *ret_ovcs_id) + int *ret_ovcs_id, struct device_node *base) { void *new_fdt; void *new_fdt_align; @@ -1037,7 +1057,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, } ovcs->overlay_mem = overlay_mem; - ret = of_overlay_apply(ovcs); + ret = of_overlay_apply(ovcs, base); /* * If of_overlay_apply() error, calling free_overlay_changeset() may * result in a memory leak if the apply partly succeeded, so do NOT diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c index d2b286d32db0..7bff6c4cb653 100644 --- a/drivers/of/unittest.c +++ b/drivers/of/unittest.c @@ -3478,7 +3478,8 @@ static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id) if (!size) pr_err("no overlay data for %s\n", overlay_name); - ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id); + ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id, + NULL); if (ovcs_id) *ovcs_id = info->ovcs_id; if (ret < 0) diff --git a/include/linux/of.h b/include/linux/of.h index 82d0a476ec75..5fe5257a7ab7 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -1668,7 +1668,7 @@ struct of_overlay_notify_data { #ifdef CONFIG_OF_OVERLAY int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, - int *ovcs_id); + int *ovcs_id, struct device_node *target_base); int of_overlay_remove(int *ovcs_id); int of_overlay_remove_all(void);
Currently, in an overlay fdt fragment, it needs to specify the exact location in base DT. In another word, when the fdt fragment is generated, the base DT location for the fragment is already known. There is new use case that the base DT location is unknown when fdt fragment is generated. For example, the add-on device provide a fdt overlay with its firmware to describe its downstream devices. Because it is add-on device which can be plugged to different systems, its firmware will not be able to know the overlay location in base DT. Instead, the device driver will load the overlay fdt and apply it to base DT at runtime. In this case, of_overlay_fdt_apply() needs to be extended to specify the target node for device driver to apply overlay fdt. int overlay_fdt_apply(..., struct device_node *base); Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> --- drivers/of/overlay.c | 42 +++++++++++++++++++++++++++++++----------- drivers/of/unittest.c | 3 ++- include/linux/of.h | 2 +- 3 files changed, 34 insertions(+), 13 deletions(-)