2019-10-25 14:01:13

by Ayan Halder

[permalink] [raw]
Subject: Question regarding "reserved-memory"


Hi Folks,

I have a question regarding "reserved-memory". I am using an Arm Juno
platform which has a chunk of ram in its fpga. I intend to make this
memory as reserved so that it can be shared between various devices
for passing framebuffer.

My dts looks like the following:-

/ {
.... // some nodes

tlx@60000000 {
compatible = "simple-bus";
...

juno_wrapper {

... /* here we have all the nodes */
/* corresponding to the devices in the fpga */

memory@d000000 {
device_type = "memory";
reg = <0x00 0x60000000 0x00 0x8000000>;
};

reserved-memory {
#address-cells = <0x01>;
#size-cells = <0x01>;
ranges;

framebuffer@d000000 {
compatible = "shared-dma-pool";
linux,cma-default;
reusable;
reg = <0x00 0x60000000 0x00 0x8000000>;
phandle = <0x44>;
};
};
...
}
}
...
}

Note that the depth of the "reserved-memory" node is 3.

Refer __fdt_scan_reserved_mem() :-

if (!found && depth == 1 && strcmp(uname, "reserved-memory") == 0) {

if (__reserved_mem_check_root(node) != 0) {
pr_err("Reserved memory: unsupported node
format, ignoring\n");
/* break scan */
return 1;
}
found = 1;

/* scan next node */
return 0;
}

It expects the "reserved-memory" node to be at depth == 1 and so it
does not probe it in our case.

Niether from the
Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
nor from commit - e8d9d1f5485b52ec3c4d7af839e6914438f6c285,
I could understand the reason for such restriction.

So, I seek the community's advice as to whether I should fix up
__fdt_scan_reserved_mem() so as to do away with the restriction or
put the "reserved-memory" node outside of 'tlx@60000000' (which looks
logically incorrect as the memory is on the fpga platform).


Thanks,
Ayan


2019-10-25 14:04:24

by Rob Herring

[permalink] [raw]
Subject: Re: Question regarding "reserved-memory"

On Thu, Oct 24, 2019 at 9:22 AM Ayan Halder <[email protected]> wrote:
>
>
> Hi Folks,
>
> I have a question regarding "reserved-memory". I am using an Arm Juno
> platform which has a chunk of ram in its fpga. I intend to make this
> memory as reserved so that it can be shared between various devices
> for passing framebuffer.
>
> My dts looks like the following:-
>
> / {
> .... // some nodes
>
> tlx@60000000 {
> compatible = "simple-bus";
> ...
>
> juno_wrapper {
>
> ... /* here we have all the nodes */
> /* corresponding to the devices in the fpga */
>
> memory@d000000 {
> device_type = "memory";
> reg = <0x00 0x60000000 0x00 0x8000000>;
> };
>
> reserved-memory {
> #address-cells = <0x01>;
> #size-cells = <0x01>;
> ranges;
>
> framebuffer@d000000 {
> compatible = "shared-dma-pool";
> linux,cma-default;
> reusable;
> reg = <0x00 0x60000000 0x00 0x8000000>;
> phandle = <0x44>;
> };
> };
> ...
> }
> }
> ...
> }
>
> Note that the depth of the "reserved-memory" node is 3.
>
> Refer __fdt_scan_reserved_mem() :-
>
> if (!found && depth == 1 && strcmp(uname, "reserved-memory") == 0) {
>
> if (__reserved_mem_check_root(node) != 0) {
> pr_err("Reserved memory: unsupported node
> format, ignoring\n");
> /* break scan */
> return 1;
> }
> found = 1;
>
> /* scan next node */
> return 0;
> }
>
> It expects the "reserved-memory" node to be at depth == 1 and so it
> does not probe it in our case.
>
> Niether from the
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> nor from commit - e8d9d1f5485b52ec3c4d7af839e6914438f6c285,
> I could understand the reason for such restriction.
>
> So, I seek the community's advice as to whether I should fix up
> __fdt_scan_reserved_mem() so as to do away with the restriction or
> put the "reserved-memory" node outside of 'tlx@60000000' (which looks
> logically incorrect as the memory is on the fpga platform).

For now, I'm going to say it must be at the root level. I'd guess the
memory@d000000 node is also just ignored (wrong unit-address BTW).

I think you're also forgetting the later unflattened parsing of
/reserved-memory. The other complication IIRC is booting with UEFI
does it's own reserved memory table which often uses the DT table as
its source.

Rob

2019-10-25 17:59:50

by Ayan Halder

[permalink] [raw]
Subject: Re: Question regarding "reserved-memory"

On Thu, Oct 24, 2019 at 09:51:04AM -0500, Rob Herring wrote:
> On Thu, Oct 24, 2019 at 9:22 AM Ayan Halder <[email protected]> wrote:
Hi Bob,

Thanks for your quick response.
> >
> >
> > Hi Folks,
> >
> > I have a question regarding "reserved-memory". I am using an Arm Juno
> > platform which has a chunk of ram in its fpga. I intend to make this
> > memory as reserved so that it can be shared between various devices
> > for passing framebuffer.
> >
> > My dts looks like the following:-
> >
> > / {
> > .... // some nodes
> >
> > tlx@60000000 {
> > compatible = "simple-bus";
> > ...
> >
> > juno_wrapper {
> >
> > ... /* here we have all the nodes */
> > /* corresponding to the devices in the fpga */
> >
> > memory@d000000 {
> > device_type = "memory";
> > reg = <0x00 0x60000000 0x00 0x8000000>;
> > };
> >
> > reserved-memory {
> > #address-cells = <0x01>;
> > #size-cells = <0x01>;
> > ranges;
> >
> > framebuffer@d000000 {
> > compatible = "shared-dma-pool";
> > linux,cma-default;
> > reusable;
> > reg = <0x00 0x60000000 0x00 0x8000000>;
> > phandle = <0x44>;
> > };
> > };
> > ...
> > }
> > }
> > ...
> > }
> >
> > Note that the depth of the "reserved-memory" node is 3.
> >
> > Refer __fdt_scan_reserved_mem() :-
> >
> > if (!found && depth == 1 && strcmp(uname, "reserved-memory") == 0) {
> >
> > if (__reserved_mem_check_root(node) != 0) {
> > pr_err("Reserved memory: unsupported node
> > format, ignoring\n");
> > /* break scan */
> > return 1;
> > }
> > found = 1;
> >
> > /* scan next node */
> > return 0;
> > }
> >
> > It expects the "reserved-memory" node to be at depth == 1 and so it
> > does not probe it in our case.
> >
> > Niether from the
> > Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > nor from commit - e8d9d1f5485b52ec3c4d7af839e6914438f6c285,
> > I could understand the reason for such restriction.
> >
> > So, I seek the community's advice as to whether I should fix up
> > __fdt_scan_reserved_mem() so as to do away with the restriction or
> > put the "reserved-memory" node outside of 'tlx@60000000' (which looks
> > logically incorrect as the memory is on the fpga platform).
>
> For now, I'm going to say it must be at the root level.
Can you mention it in the Documentation/.../reserved-memory.txt,
please?

> I'd guess the
> memory@d000000 node is also just ignored (wrong unit-address BTW).
I would request you to ignore the address for the time being. In
juno_wrapper{}, we have a complex remapping of addresses of all the
sub-devices. I deliberately did not put that in the snippet, so as to
prevent any distraction from the core issue.

>
> I think you're also forgetting the later unflattened parsing of
> /reserved-memory.
Are you talking about the remaining part of the
__fdt_scan_reserved_mem() ie

....
} else if (found && depth < 2) {
/* scanning of /reserved-memory has been finished */
return 1;
}

if (!of_fdt_device_is_available(initial_boot_params, node))
return 0;

err = __reserved_mem_reserve_reg(node, uname);
if (err == -ENOENT && of_get_flat_dt_prop(node, "size", NULL))
fdt_reserved_mem_save_node(node, uname, 0, 0);

/* scan next node */
return 0;

If so, I agree with you that it needs to be changed as well (if we
were to do away with the restriction).

> The other complication IIRC is booting with UEFI
> does it's own reserved memory table which often uses the DT table as
> its source.
I have no knowledge of UEFI booting. So if UEFI expects
"reserved-memory" nodes to be at root level, then we must adhere to
the restriction. :)

Ayan
>
> Rob