From: m.szyprowski@samsung•com (Marek Szyprowski)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v5 2/3] drivers: of: add initialization code for dma reserved memory
Date: Mon, 19 Aug 2013 16:47:17 +0200 [thread overview]
Message-ID: <52122FF5.7000802@samsung.com> (raw)
In-Reply-To: <CAL_JsqJR_mdCiA8knd3COuZ=DcpuAtdzsCy8tAPMSMq67m=SAw@mail.gmail.com>
Hello,
On 8/13/2013 3:00 PM, Rob Herring wrote:
> On Mon, Aug 12, 2013 at 3:34 AM, Marek Szyprowski
> <m.szyprowski@samsung•com> wrote:
> > Hello,
> >
> >
> > On 8/10/2013 7:33 PM, Rob Herring wrote:
> >>
> >> On Fri, Aug 9, 2013 at 6:51 AM, Marek Szyprowski
> >> <m.szyprowski@samsung•com> wrote:
> >> > Add device tree support for contiguous and reserved memory regions
> >> > defined in device tree. Initialization is done in 2 steps. First, the
> >> > memory is reserved, what happens very early when only flattened device
> >>
> >> s/what/which/
> >>
> >> > tree is available. Then on device initialization the corresponding cma
> >> > and reserved regions are assigned to each device structure.
> >>
> >> What this commit message does not tell me is why does the reservation
> >> have to happen before the fdt is unflattened? It would greatly
> >> simplify the code if it didn't.
> >
> >
> > Large memory blocks can be RELIABLY reserved only during early boot. This
> > must happen before the whole memory management subsystem is initialized,
> > because we need to ensure that the given contiguous blocks are not yet
> > allocated by kernel. Also it must happen before kernel mappings for the
> > whole low memory are created, to ensure that there will be no mappings
> > (for reserved blocks) or mapping with special properties can be created
> > (for CMA blocks). This all happens before device tree structures are
> > unflattened, so we need to get reserved memory layout directly from fdt.
> >
>
> Okay. Just making sure.
>
>
> >> > + } else if (depth == 2 && my_depth == 1 &&
> >> > + strcmp(uname, "reserved-memory") == 0) {
> >> > + prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
> >> > + if (prop)
> >> > + size_cells = be32_to_cpup(prop);
> >> > +
> >> > + prop = of_get_flat_dt_prop(node, "#address-cells",
> >> > NULL);
> >> > + if (prop)
> >> > + addr_cells = be32_to_cpup(prop);
> >>
> >> I think we should just require these be the same size as the memory
> >> node which would be dt_root_*_cells.
> >>
> >> I'm fine with moving this into drivers/of/fdt.c if that simplifies things.
> >
> >
> > dt_root_*_cells are global variables, so its not a problem to get access to
> > them. However I wonder how can we ensure that user/device tree creator will
> > set #size-cells/#address-cells to the same values as for root memory node?
> > Would it be enough to state that in binding documentation? If so then the
> > reserved memory code can skip parsing them and use dt_root_*_cells directly,
> > what will simplify the code.
>
> Yes, just add a note to the binding that the cell sizes are the same
> as the memory node.
>
> >> > +
> >> > + my_depth = depth;
> >> > + /* scan next node */
> >> > + return 0;
> >> > + } else if (depth != 3 && my_depth != 2) {
> >> > + /* scan next node */
> >> > + return 0;
> >> > + } else if (depth < my_depth) {
> >> > + /* break scan now */
> >> > + return 1;
> >> > + }
> >>
> >> This code bothers me and is hard to follow. I don't think trying to
> >> use of_scan_flat_dt is the right approach here. What you really want
> >> here is check for reserved-memory node under the memory node and then
> >> scan each child node. This could all be done from
> >> early_init_dt_scan_memory.
> >
> >
> > early_init_dt_scan_memory() is also called from of_scan_flat_dt() and
> > it also implements similar state machine to parse fdt. The only
> > difference is the fact that "memory" is a direct child of root node,
> > so the state machine is much simpler (there is no need to parse
> > /memory/reserved-memory path).
> >
>
> If you have already found the memory node, then why find it again? I
> don't think the existing scan functions handle anything but top-level
> nodes very well. So doing something like this from
> early_init_dt_scan_memory() is what I was thinking. It is a very rough
> sketch:
>
> // find the reserved-memory node
> for (ndepth = 0, off = fdt_next_node(blob, parent_offset, &ndepth);
> (off >= 0) && (ndepth > 0);
> off = fdt_next_node(blob, off, &ndepth)) {
> if (ndepth == 1) {
> name = fdt_get_name(blob, off, 0), off);
> if (strcmp(name, "reserved-memory") == 0) {
> parent_offset = off;
> break; // found
> }
> }
>
> // find the child nodes
> for (ndepth = 0, off = fdt_next_node(blob, parent_offset, &ndepth);
> (off >= 0) && (ndepth == 1);
> off = fdt_next_node(blob, off, &ndepth)) {
> // now handle each child
> }
>
> These could probably be further refined with some loop iterator macros.
The above code looks pretty nice, but there are some problems with it:
1. Although it would look nice to call it from early_init_dt_scan_memory()
it won't be possible, because that time it is too early. memblock structures
are initialized after a call to early_init_dt_scan_memory() and until that
no changes to memory layout are easily possible.
2. Currently there are no fdt parsing functions in the kernel. I've tried
to split of_scan_flat_dt() into fdt_next_node() + fdt_get_name() and use
them both in of_scan_flat_dt() and above reserved memory parsing functions.
In the end I got quite a lot of code, which is still quite hard to follow.
Because of the above I decided to resurrect of_scan_flat_dt_by_path()
function from the previous version and use #size-cells/#address-cells
attributes from root node what in the end simplified reserved memory
parsing function. I hope that it can be accepted after such changes
without introducing a burden caused by the whole infrastructure for
manual parsing fdt.
> >> > + name = kbasename(node->full_name);
> >> > + for (i = 0; i < reserved_mem_count; i++)
> >> > + if (strcmp(name, reserved_mem[i].name) == 0)
> >> > + return &reserved_mem[i];
> >> > + return NULL;
> >>
> >> Matching against a struct device_node pointer would be more common way
> >> to match. So it would be good to update reserved_mem with a
> >> device_node ptr when we unflatten the DT.
> >
> >
> > I wonder if it really makes sense. To get device_node ptr I will need to
> > scan /memody/reserved-memory node and match all its children BY NAME
> > with the structures parsed from FDT (stored in reserved_mem array). Then
> > I will need to iterate again for each device node with memory-region
> > property to find the needed entry. Names are unique, IMHO they can serve
> > as a key for matching structures between FDT and regular, unflattened DT.
>
> You are iterating multiple times using a string match versus iterating
> once more and then doing a pointer match. However, it is not really
> performance critical and is fine for now.
Thanks!
Best regards
--
Marek Szyprowski
Samsung R&D Institute Poland
next prev parent reply other threads:[~2013-08-19 14:47 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-09 11:51 [PATCH v5 0/3] Device Tree support for CMA (Contiguous Memory Allocator) Marek Szyprowski
2013-08-09 11:51 ` [PATCH v5 1/3] drivers: dma-contiguous: clean source code and prepare for device tree Marek Szyprowski
2013-08-09 11:51 ` [PATCH v5 2/3] drivers: of: add initialization code for dma reserved memory Marek Szyprowski
2013-08-10 17:33 ` Rob Herring
2013-08-12 8:34 ` Marek Szyprowski
2013-08-13 13:00 ` Rob Herring
2013-08-19 14:47 ` Marek Szyprowski [this message]
2013-08-19 19:36 ` Rob Herring
2013-08-13 20:08 ` Stephen Warren
2013-08-16 5:25 ` Olof Johansson
2013-08-16 16:06 ` Stephen Warren
2013-08-16 5:32 ` Olof Johansson
2013-08-09 11:51 ` [PATCH v5 3/3] ARM: init: add support for reserved memory defined by device tree Marek Szyprowski
2013-08-09 13:19 ` [PATCH v5 0/3] Device Tree support for CMA (Contiguous Memory Allocator) Tomasz Figa
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=52122FF5.7000802@samsung.com \
--to=m.szyprowski@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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