From: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
To: Rob Herring <robh+dt@kernel.org>
Cc: <catalin.marinas@arm.com>, <will@kernel.org>,
<frowand.list@gmail.com>, <vgupta@kernel.org>, <arnd@arndb.de>,
<olof@lixom.net>, <soc@kernel.org>, <guoren@kernel.org>,
<monstr@monstr.eu>, <palmer@dabbelt.com>, <aou@eecs.berkeley.edu>,
<dinguyen@kernel.org>, <chenhuacai@kernel.org>,
<tsbogend@alpha.franken.de>, <jonas@southpole.se>,
<stefan.kristiansson@saunalahti.fi>, <shorne@gmail.com>,
<mpe@ellerman.id.au>, <ysato@users.sourceforge.jp>,
<dalias@libc.org>, <glaubitz@physik.fu-berlin.de>,
<richard@nod.at>, <anton.ivanov@cambridgegreys.com>,
<johannes@sipsolutions.net>, <chris@zankel.net>,
<jcmvbkbc@gmail.com>, <linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-arm-msm@vger.kernel.org>, <kernel@quicinc.com>
Subject: Re: [PATCH v5 3/4] of: reserved_mem: Use the unflatten_devicetree APIs to scan reserved mem. nodes
Date: Fri, 12 Apr 2024 14:38:54 -0700 [thread overview]
Message-ID: <7befeeb2-a481-46ac-97d0-a9d6d023427d@quicinc.com> (raw)
In-Reply-To: <CAL_Jsq+RQaKTqB6hnsCJ_d0zM6FkrMXQ0NF0r1P22q95_ZDM4A@mail.gmail.com>
On 4/11/2024 11:59 AM, Rob Herring wrote:
> On Thu, Mar 28, 2024 at 4:16 PM Oreoluwa Babatunde
> <quic_obabatun@quicinc.com> wrote:
>> The unflatten_devicetree APIs have been setup and are available to be
>> used by the time the fdt_init_reserved_mem() function is called.
>> Since the unflatten_devicetree APIs are a more efficient way of scanning
>> through the DT nodes, switch to using these APIs to facilitate the rest
>> of the reserved memory processing.
> Please use get_maintainers.pl. Specifically, you missed maintainers
> for kernel/dma/.
Sorry about that, Will include them in the next version.
>
>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
>> ---
>> drivers/of/of_reserved_mem.c | 77 +++++++++++++++++++++------------
>> include/linux/of_reserved_mem.h | 2 +-
>> kernel/dma/coherent.c | 8 ++--
>> kernel/dma/contiguous.c | 8 ++--
>> kernel/dma/swiotlb.c | 10 ++---
>> 5 files changed, 64 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 0aba366eba59..68d1f4cca4bb 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -99,7 +99,7 @@ static void __init alloc_reserved_mem_array(void)
>> /*
>> * fdt_reserved_mem_save_node() - save fdt node for second pass initialization
>> */
>> -static void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>> +static void __init fdt_reserved_mem_save_node(struct device_node *node, const char *uname,
>> phys_addr_t base, phys_addr_t size)
>> {
>> struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
>> @@ -109,7 +109,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>> return;
>> }
>>
>> - rmem->fdt_node = node;
>> + rmem->dev_node = node;
>> rmem->name = uname;
>> rmem->base = base;
>> rmem->size = size;
>> @@ -178,11 +178,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>> }
>>
>> /*
>> - * __reserved_mem_check_root() - check if #size-cells, #address-cells provided
>> + * __fdt_reserved_mem_check_root() - check if #size-cells, #address-cells provided
>> * in /reserved-memory matches the values supported by the current implementation,
>> * also check if ranges property has been provided
>> */
>> -static int __init __reserved_mem_check_root(unsigned long node)
>> +static int __init __fdt_reserved_mem_check_root(unsigned long node)
>> {
>> const __be32 *prop;
>>
>> @@ -200,6 +200,29 @@ static int __init __reserved_mem_check_root(unsigned long node)
>> return 0;
>> }
>>
>> +/*
>> + * __dt_reserved_mem_check_root() - check if #size-cells, #address-cells provided
>> + * in /reserved-memory matches the values supported by the current implementation,
>> + * also check if ranges property has been provided
>> + */
>> +static int __init __dt_reserved_mem_check_root(struct device_node *node)
> The normal prefix is 'of', not 'dt'. Weird, right?, but it dates back
> to OpenFirmware.
Got it! I will follow the same for the functions that I renamed in Patch 04 as well.
>
>> +{
>> + const __be32 *prop;
>> +
>> + prop = of_get_property(node, "#size-cells", NULL);
> Throughout, use the appropriate typed function. Here for example,
> of_property_read_u32().
ack.
>
>> + if (!prop || be32_to_cpup(prop) != dt_root_size_cells)
>> + return -EINVAL;
>> +
>> + prop = of_get_property(node, "#address-cells", NULL);
>> + if (!prop || be32_to_cpup(prop) != dt_root_addr_cells)
>> + return -EINVAL;
>> +
>> + prop = of_get_property(node, "ranges", NULL);
> Or for presence, just of_property_present().
ack.
>
>> + if (!prop)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> /**
>> * fdt_scan_reserved_mem_reg_nodes() - Store info for the "reg" defined
>> * reserved memory regions.
>> @@ -213,33 +236,38 @@ static int __init __reserved_mem_check_root(unsigned long node)
>> static void __init fdt_scan_reserved_mem_reg_nodes(void)
>> {
>> int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
>> - const void *fdt = initial_boot_params;
>> + struct device_node *node, *child;
>> phys_addr_t base, size;
>> const __be32 *prop;
>> - int node, child;
>> int len;
>>
>> - node = fdt_path_offset(fdt, "/reserved-memory");
>> - if (node < 0) {
>> + node = of_find_node_by_path("/reserved-memory");
>> + if (!node) {
>> pr_info("Reserved memory: No reserved-memory node in the DT\n");
>> return;
>> }
>>
>> - if (__reserved_mem_check_root(node)) {
>> + if (__dt_reserved_mem_check_root(node)) {
>> pr_err("Reserved memory: unsupported node format, ignoring\n");
>> return;
>> }
>>
>> - fdt_for_each_subnode(child, fdt, node) {
>> + for_each_child_of_node(node, child) {
>> const char *uname;
>> + struct reserved_mem *rmem;
>>
>> - prop = of_get_flat_dt_prop(child, "reg", &len);
>> - if (!prop)
>> + if (!of_device_is_available(child))
>> continue;
>> - if (!of_fdt_device_is_available(fdt, child))
>> +
>> + prop = of_get_property(child, "reg", &len);
> We have specific unflattened functions for reading 'reg'. Note that
> you should use the 'translated' ones even though we have a check to
> disallow any real translation. That restriction should be fixed at
> some point.
"of_address_to_resource()" seems to be a function that reads the 'reg' property
the way you are describing here. I'll switch to that function and see how it works!
Please let me know if you had another function in mind for me to use here.
>
>> + if (!prop) {
>> + rmem = of_reserved_mem_lookup(child);
>> + if (rmem)
>> + rmem->dev_node = child;
>> continue;
>> + }
>>
>> - uname = fdt_get_name(fdt, child, NULL);
>> + uname = of_node_full_name(child);
>> if (len && len % t_len != 0) {
>> pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
>> uname);
>> @@ -269,7 +297,7 @@ int __init fdt_scan_reserved_mem(void)
>> if (node < 0)
>> return -ENODEV;
>>
>> - if (__reserved_mem_check_root(node) != 0) {
>> + if (__fdt_reserved_mem_check_root(node) != 0) {
>> pr_err("Reserved memory: unsupported node format, ignoring\n");
>> return -EINVAL;
>> }
>> @@ -447,7 +475,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>> uname, (unsigned long)(size / SZ_1M));
>> return -ENOMEM;
>> }
>> - fdt_reserved_mem_save_node(node, uname, base, size);
>> + fdt_reserved_mem_save_node(NULL, uname, base, size);
>> return 0;
>> }
>>
>> @@ -467,7 +495,7 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>> reservedmem_of_init_fn initfn = i->data;
>> const char *compat = i->compatible;
>>
>> - if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
>> + if (!of_device_is_compatible(rmem->dev_node, compat))
>> continue;
>>
>> ret = initfn(rmem);
>> @@ -500,11 +528,6 @@ static int __init __rmem_cmp(const void *a, const void *b)
>> if (ra->size > rb->size)
>> return 1;
>>
>> - if (ra->fdt_node < rb->fdt_node)
>> - return -1;
>> - if (ra->fdt_node > rb->fdt_node)
>> - return 1;
>> -
>> return 0;
>> }
>>
>> @@ -551,16 +574,16 @@ void __init fdt_init_reserved_mem(void)
>>
>> for (i = 0; i < reserved_mem_count; i++) {
>> struct reserved_mem *rmem = &reserved_mem[i];
>> - unsigned long node = rmem->fdt_node;
>> + struct device_node *node = rmem->dev_node;
>> int len;
>> const __be32 *prop;
>> int err = 0;
>> bool nomap;
>>
>> - nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>> - prop = of_get_flat_dt_prop(node, "phandle", &len);
>> + nomap = of_get_property(node, "no-map", NULL) != NULL;
>> + prop = of_get_property(node, "phandle", &len);
> We store the phandle in struct device_node, so reading and storing it
> here shouldn't be needed I think.
ack.
>
>> if (!prop)
>> - prop = of_get_flat_dt_prop(node, "linux,phandle", &len);
>> + prop = of_get_property(node, "linux,phandle", &len);
>> if (prop)
>> rmem->phandle = of_read_number(prop, len/4);
>>
>> @@ -574,7 +597,7 @@ void __init fdt_init_reserved_mem(void)
>> } else {
>> phys_addr_t end = rmem->base + rmem->size - 1;
>> bool reusable =
>> - (of_get_flat_dt_prop(node, "reusable", NULL)) != NULL;
>> + (of_get_property(node, "reusable", NULL)) != NULL;
>>
>> pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
>> &rmem->base, &end, (unsigned long)(rmem->size / SZ_1K),
>> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
>> index 4de2a24cadc9..b6107a18d170 100644
>> --- a/include/linux/of_reserved_mem.h
>> +++ b/include/linux/of_reserved_mem.h
>> @@ -10,7 +10,7 @@ struct reserved_mem_ops;
>>
>> struct reserved_mem {
>> const char *name;
>> - unsigned long fdt_node;
>> + struct device_node *dev_node;
>> unsigned long phandle;
>> const struct reserved_mem_ops *ops;
>> phys_addr_t base;
>> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
>> index ff5683a57f77..0db0aae83102 100644
>> --- a/kernel/dma/coherent.c
>> +++ b/kernel/dma/coherent.c
>> @@ -362,20 +362,20 @@ static const struct reserved_mem_ops rmem_dma_ops = {
>>
>> static int __init rmem_dma_setup(struct reserved_mem *rmem)
>> {
>> - unsigned long node = rmem->fdt_node;
>> + struct device_node *node = rmem->dev_node;
>>
>> - if (of_get_flat_dt_prop(node, "reusable", NULL))
>> + if (of_get_property(node, "reusable", NULL))
>> return -EINVAL;
>>
>> #ifdef CONFIG_ARM
>> - if (!of_get_flat_dt_prop(node, "no-map", NULL)) {
>> + if (!of_get_property(node, "no-map", NULL)) {
> While you are here, convert this to IS_ENABLED():
>
> if (IS_ENABLED(CONFIG_ARM) && !of_property_read_bool(node)) {
> ...
> }
ack.
Thank you for your feedback!
Oreoluwa
next prev parent reply other threads:[~2024-04-12 21:41 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 21:15 [PATCH v5 0/4] Dynamic Allocation of the reserved_mem array Oreoluwa Babatunde
2024-03-28 21:15 ` [PATCH v5 1/4] of: reserved_mem: Restruture how the reserved memory regions are processed Oreoluwa Babatunde
2024-03-28 21:15 ` [PATCH v5 2/4] of: reserved_mem: Add code to dynamically allocate reserved_mem array Oreoluwa Babatunde
2024-03-28 21:15 ` [PATCH v5 3/4] of: reserved_mem: Use the unflatten_devicetree APIs to scan reserved mem. nodes Oreoluwa Babatunde
2024-04-11 18:59 ` Rob Herring
2024-04-12 21:38 ` Oreoluwa Babatunde [this message]
2024-03-28 21:15 ` [PATCH v5 4/4] of: reserved_mem: Rename fdt_* functions to refelct use of unflatten_devicetree APIs Oreoluwa Babatunde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7befeeb2-a481-46ac-97d0-a9d6d023427d@quicinc.com \
--to=quic_obabatun@quicinc.com \
--cc=anton.ivanov@cambridgegreys.com \
--cc=aou@eecs.berkeley.edu \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=chenhuacai@kernel.org \
--cc=chris@zankel.net \
--cc=dalias@libc.org \
--cc=devicetree@vger.kernel.org \
--cc=dinguyen@kernel.org \
--cc=frowand.list@gmail.com \
--cc=glaubitz@physik.fu-berlin.de \
--cc=guoren@kernel.org \
--cc=jcmvbkbc@gmail.com \
--cc=johannes@sipsolutions.net \
--cc=jonas@southpole.se \
--cc=kernel@quicinc.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=monstr@monstr.eu \
--cc=mpe@ellerman.id.au \
--cc=olof@lixom.net \
--cc=palmer@dabbelt.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=shorne@gmail.com \
--cc=soc@kernel.org \
--cc=stefan.kristiansson@saunalahti.fi \
--cc=tsbogend@alpha.franken.de \
--cc=vgupta@kernel.org \
--cc=will@kernel.org \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).