Re: [PATCH] pstore: Use root address-cells/size-cells as defaults for reserved-memory

[Andrey Skvortsov <andrej.skvortzov@gmail.com>]

Hi,

On 23-08-26 19:41, Andre Przywara wrote:
> On Sat, 26 Aug 2023 15:10:10 +0200
> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> Hi,
> 
> > On 8/26/23 14:16, Andrey Skvortsov wrote:
> > > u-boot adds reserve-memory node, if it's missing, with following
> > > properties:
> > >
> > > ```
> > >      reserved-memory {
> > >           #address-cells = <2>;
> > >           #size-cells = <2>;  
> > 
> > This defines the size of cells for the children of reserved-memory and
> > and for the ranges property. If you set the cell sizes to 1 you can no
> > longer encode 64 bit addresses.
> 
> Yes, this is expected in this case, the Allwinner A64 SoC has its memory
> map (including DRAM) completely below 4GB, hence the root node can go
> with 1/1.
> 
> > 
> > >           ranges;
> > >      }
> > > ```
> > >
> > > But with these default address-cells and size-cells values, pstore
> > > isn't working on A64. Root node for A64 defines 'address-cells' and
> > > 'size-cells' as 1.
> > >
> > > dtc complains if reserved-memory has different address-cells and
> > > size-cells.
> > >
> > > ```
> > >       Warning (ranges_format): /reserved-memory:ranges: empty "ranges"
> > >       property but its #address-cells (2) differs from / (1)  
> > 
> > I cannot find any such requirement in the Devicetree Specification 1.4.
> > Is this a dtc bug?
> 
> I think the culprit here is the *empty* ranges property:
> "If the property is defined with an <empty> value, it specifies that the
> parent and child address space is identical, and no address translation
> is required."
> As this is contradicted by the differing #a-c/#s-z properties, it looks
> like dtc has good reasons to warn.
> 
> > 
> > > ```
> > >
> > > This patch takes into account address-cells and size-cells of the root
> > > node and uses them as values for new reserved-memory node.  
> > 
> > Reservations may be above 4 GiB. How does your patch consider this?
> 
> If the root #a-c/#s-c don't allow for more than 4GB, then it's for a
> reason (in this case, the A64 being 32-bit only, even with AArch64
> capable cores), and keeping it isn't restricting it further. So I think
> copying the root properties is the right thing to do. I think almost
> every other 64-bit core uses 2/2 anyway, so it's just the odd outlier
> here.
> 
> Cheers,
> Andre

Are there any comments I should fix to get this change merged or maybe
another way of solving this problem?

I see, that patch requires review from Tom Rini currently on patchwork [1].

1. https://patchwork.ozlabs.org/project/uboot/patch/20230826121652.2487643-1-andrej.skvortzov@gmail.com/

-- 
Best regards,
Andrey Skvortsov