[PATCH 0/5] of: reserved_mem: Provide more control about allocation behavior

[PATCH 0/5] of: reserved_mem: Provide more control about allocation behavior

[Stephan Gerhold]

Provide more control about the allocation behavior for dynamically 
allocated reserved memory by adding a "alloc-bottom-up" and 
"alloc-top-down" option and by making the allocation order 
deterministic.

The motivation for this patch series are the many different reserved 
firmware regions on Qualcomm platforms. Currently it is often necessary 
to duplicate them into each board DT, because minor differences for 
some of the firmware regions (e.g. the firmware size) requires shifting
the base address of all following firmware regions as well.

I propose describing the actual requirements (size, alignment, 
alloc-ranges) instead and allocating the reserved regions at runtime. 
This allows defining only the actual device-specific part in the board 
DT and having everything else shared in the SoC.dtsi.

The series starts with two minor additions to the of_reserved_mem code. 
The last two patches are examples that are meant to show the motivation
more clearly for the MSM8916 SoC. PATCH 4/5 shows the current (static) 
approach, then PATCH 5/5 switches to the dynamic allocation based on
the first 3 patches.

If the first 3 patches are accepted I would send the full MSM8916 DT
changes in a separate series.

Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
---
Stephan Gerhold (5):
      dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}
      of: reserved_mem: Implement alloc-{bottom-up,top-down}
      of: reserved_mem: Use stable allocation order
      [RFC] arm64: dts: qcom: msm8916: Enable modem on two phones
      [RFC] arm64: dts: qcom: msm8916: Reserve firmware memory dynamically

 .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++
 arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 13 +++++++
 .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 31 ++++++++++++++--
 .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 21 +++++++++++
 arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 29 ++++++++-------
 arch/arm64/boot/dts/qcom/msm8916.dtsi              | 42 ++++++++++++++++------
 drivers/of/of_reserved_mem.c                       | 14 +++++++-
 7 files changed, 163 insertions(+), 26 deletions(-)
---
base-commit: 715abedee4cd660ad390659aefa7482f05275bbd
change-id: 20230510-dt-resv-bottom-up-68d71ff6628f

Best regards,
-- 
Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>

[PATCH RFC 4/5] arm64: dts: qcom: msm8916: Enable modem on two phones

[Stephan Gerhold]

DO NOT APPLY - this is just an example to show the motivation for the
patch series. For clarity only some of the device trees are updated.

On MSM8916 there are different independent firmware regions, e.g. for
the modem (mpss), WiFi/Bluetooth (wcnss) or video acceleration (venus).
They need a specific size but the address is relocatable within a
certain memory region as long as the necessary alignment is respected.

The modem firmware size varies from device to device depending on the
available functionality. Right now msm8916.dtsi specifies the firmware
sizes for the DragonBoard 410c SBC, which has smaller firmware because
the modem firmware only implements GPS but not LTE etc.

The modem firmware is much larger on actual smartphones. Unfortunately,
changing the modem/mpss reserved memory size requires shifting all
following reserved memory regions to new addresses as well, even though
the size for the wcnss/venus firmware is the same for all devices.

This introduces quite a lot of churn with many magic addresses and room
for errors. Only two devices are shown in the example below, but similar
diffs would need to be applied to 30+ more similar MSM8916 device trees.

Note that msm8916-longcheer-l8150 is a single exception where the wcnss
firmware is not relocatable and must be loaded at a specific address.
It is included here to show that the dynamic reserved memory allocation
in the next patch handles this automatically.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 29 +++++++++++++++++++++-
 .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 28 +++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
index f1dd625e1822..4a5eab06c18b 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
@@ -21,14 +21,37 @@ chosen {
 		stdout-path = "serial0";
 	};
 
+	/*
+	 * For some reason, the wcnss firmware is not relocatable, so it must be
+	 * loaded at 0x8b600000. Unfortunately this overlaps with the memory
+	 * region usually used for the modem firmware (0x86800000-0x8b800000).
+	 * However, the modem firmware is relocatable, so wcnss is placed at the
+	 * fixed address and all other firmware regions are moved to a fitting place.
+	 */
 	reserved-memory {
-		/* wcnss.mdt is not relocatable, so it must be loaded at 0x8b600000 */
+		/delete-node/ mpss@86800000;
 		/delete-node/ wcnss@89300000;
+		/delete-node/ venus@89900000;
 
 		wcnss_mem: wcnss@8b600000 {
 			reg = <0x0 0x8b600000 0x0 0x600000>;
 			no-map;
 		};
+
+		venus_mem: venus@8bc00000 {
+			reg = <0x0 0x8bc00000 0x0 0x500000>;
+			no-map;
+		};
+
+		mpss_mem: mpss@8ec00000 {
+			reg = <0x0 0x8ec00000 0x0 0x5000000>;
+			no-map;
+		};
+
+		gps_mem: gps@93c00000 {
+			reg = <0x0 0x93c00000 0x0 0x200000>;
+			no-map;
+		};
 	};
 
 	gpio-keys {
@@ -218,6 +241,10 @@ &blsp1_uart2 {
 	status = "okay";
 };
 
+&mpss {
+	status = "okay";
+};
+
 &pm8916_resin {
 	status = "okay";
 	linux,code = <KEY_VOLUMEDOWN>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
index 1a41a4db874d..25ad098b1503 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
@@ -36,11 +36,35 @@ chosen {
 	};
 
 	reserved-memory {
+		/delete-node/ mpss@86800000;
+		/delete-node/ wcnss@89300000;
+		/delete-node/ venus@89900000;
+
 		/* Additional memory used by Samsung firmware modifications */
 		tz-apps@85500000 {
 			reg = <0x0 0x85500000 0x0 0xb00000>;
 			no-map;
 		};
+
+		mpss_mem: mpss@86800000 {
+			reg = <0x0 0x86800000 0x0 0x5a00000>;
+			no-map;
+		};
+
+		gps_mem: gps@8c200000 {
+			reg = <0x0 0x8c200000 0x0 0x200000>;
+			no-map;
+		};
+
+		wcnss_mem: wcnss@8c400000 {
+			reg = <0x0 0x8c400000 0x0 0x600000>;
+			no-map;
+		};
+
+		venus_mem: venus@8ca00000 {
+			reg = <0x0 0x8ca00000 0x0 0x500000>;
+			no-map;
+		};
 	};
 
 	gpio-keys {
@@ -263,6 +287,10 @@ &blsp1_uart2 {
 	status = "okay";
 };
 
+&mpss {
+	status = "okay";
+};
+
 &pm8916_resin {
 	status = "okay";
 	linux,code = <KEY_VOLUMEDOWN>;

-- 
2.40.1

[PATCH 1/5] dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}

[Stephan Gerhold]

Right now the allocation behavior for dynamic reserved memory is
implementation-defined. On Linux it is dependent on the architecture.
This is usually fine if the address is completely arbitrary.

However, when using "alloc-ranges" it is helpful to allow controlling
this. That way you can make sure that the reservations are placed next
to other (static) allocations to keep the free memory contiguous if
possible.

Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
---
 .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
index c680e397cfd2..56f4bc6137e7 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
@@ -52,6 +52,18 @@ properties:
       Address and Length pairs. Specifies regions of memory that are
       acceptable to allocate from.
 
+  alloc-bottom-up:
+    type: boolean
+    description: >
+      Specifies that the memory region should be preferably allocated
+      at the lowest available address within the "alloc-ranges" region.
+
+  alloc-top-down:
+    type: boolean
+    description: >
+      Specifies that the memory region should be preferably allocated
+      at the highest available address within the "alloc-ranges" region.
+
   iommu-addresses:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description: >
@@ -93,6 +105,10 @@ properties:
       system can use that region to store volatile or cached data that
       can be otherwise regenerated or migrated elsewhere.
 
+dependencies:
+  alloc-bottom-up: [alloc-ranges]
+  alloc-top-down: [alloc-ranges]
+
 allOf:
   - if:
       required:
@@ -178,4 +194,27 @@ examples:
         };
       };
     };
+
+  - |
+    / {
+      compatible = "foo";
+      model = "foo";
+
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        adsp_mem: adsp {
+          size = <0x0 0x600000>;
+          alignment = <0x0 0x100000>;
+          alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+          alloc-bottom-up;
+          no-map;
+        };
+      };
+    };
 ...

-- 
2.40.1

Re: [PATCH 1/5] dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}

[Rob Herring]

On Mon, May 15, 2023 at 12:12:16PM +0200, Stephan Gerhold wrote:
> Right now the allocation behavior for dynamic reserved memory is
> implementation-defined. On Linux it is dependent on the architecture.
> This is usually fine if the address is completely arbitrary.
> 
> However, when using "alloc-ranges" it is helpful to allow controlling
> this. That way you can make sure that the reservations are placed next
> to other (static) allocations to keep the free memory contiguous if
> possible.

That should already be possible with all the information you 
already have. IOW, you are looking at all the region and "alloc-ranges" 
addresses to decide top-down or bottom-up. Why can't the kernel do that.

Alternatively, if you really care about the allocation locations, don't 
use dynamic regions.

> 
> Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> ---
>  .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> index c680e397cfd2..56f4bc6137e7 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> @@ -52,6 +52,18 @@ properties:
>        Address and Length pairs. Specifies regions of memory that are
>        acceptable to allocate from.
>  
> +  alloc-bottom-up:
> +    type: boolean
> +    description: >
> +      Specifies that the memory region should be preferably allocated
> +      at the lowest available address within the "alloc-ranges" region.
> +
> +  alloc-top-down:
> +    type: boolean
> +    description: >
> +      Specifies that the memory region should be preferably allocated
> +      at the highest available address within the "alloc-ranges" region.

What happens when both are set?

> +
>    iommu-addresses:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      description: >
> @@ -93,6 +105,10 @@ properties:
>        system can use that region to store volatile or cached data that
>        can be otherwise regenerated or migrated elsewhere.
>  
> +dependencies:
> +  alloc-bottom-up: [alloc-ranges]
> +  alloc-top-down: [alloc-ranges]
> +
>  allOf:
>    - if:
>        required:
> @@ -178,4 +194,27 @@ examples:
>          };
>        };
>      };
> +
> +  - |
> +    / {
> +      compatible = "foo";
> +      model = "foo";
> +
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ranges;
> +
> +        adsp_mem: adsp {
> +          size = <0x0 0x600000>;
> +          alignment = <0x0 0x100000>;
> +          alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
> +          alloc-bottom-up;
> +          no-map;
> +        };
> +      };
> +    };
>  ...
> 
> -- 
> 2.40.1
> 

Re: [PATCH 1/5] dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}

[Stephan Gerhold]

Hi Rob,

Thanks for your suggestions!

On Thu, Jun 08, 2023 at 08:02:56AM -0600, Rob Herring wrote:
> On Mon, May 15, 2023 at 12:12:16PM +0200, Stephan Gerhold wrote:
> > Right now the allocation behavior for dynamic reserved memory is
> > implementation-defined. On Linux it is dependent on the architecture.
> > This is usually fine if the address is completely arbitrary.
> > 
> > However, when using "alloc-ranges" it is helpful to allow controlling
> > this. That way you can make sure that the reservations are placed next
> > to other (static) allocations to keep the free memory contiguous if
> > possible.
> 
> That should already be possible with all the information you 
> already have. IOW, you are looking at all the region and "alloc-ranges" 
> addresses to decide top-down or bottom-up. Why can't the kernel do that.
> 

Would you accept a patch implementing such a behavior?

There are obviously infinitely complicated algorithms possible for the
allocation. A fairly simple one would be to check if the "alloc-ranges"
overlap or are adjacent to an already existing reservation, i.e.

  1. If the "alloc-range" starts at the end or inside an existing
     reservation, use bottom-up.
  2. If the "alloc-range" ends at the start or inside an existing
     reservation, use top-down.
  3. If both or none is the case, keep current (implementation-defined)
     behavior.

For reference, here are some examples how it behaves. |...| is the
unallocated memory, RRR existing allocations, and each RRR--- line
below a requested alloc-range (and where it was allocated):

Bottom-up (rule 1):
  |.....RRRR................RRRRRRRRR...........|
            RRR----
         ---RRR-------

Top-down (rule 2):
  |.....RRRR................RRRRRRRRR...........|
                     ----RRR
                ---------RRR------

Otherwise rule 3 just behaves as currently where either bottom-up
or top-down is used depending on the implementation/architecture:
  |.....RRRR................RRRRRRRRR...........|
               -----RRR
     or        RRR-----
          ---------------RRR----
     or   --RRR-----------------

There are plenty of edge cases where it doesn't produce the optimal
result, but it just results in exactly the same behavior as currently
so it's not any worse (with rule 3):

  |.....RRRR................RRRRRRRRR...........|
                          -----------RRR-----
                 or       ----------------RRR
                     ---------------------RRR  (no way to handle this
                 or  RRR---------------------   with top-down/bottom-up)

> Alternatively, if you really care about the allocation locations, don't 
> use dynamic regions.
> 

Yes, this is the option used at the moment. As outlined in detail in the
examples of RFC PATCH 4/5 and 5/5 I would like a solution inbetween. The
exact address doesn't matter but the way (direction) the region is
filled should preferably stay the same.

> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > index c680e397cfd2..56f4bc6137e7 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > @@ -52,6 +52,18 @@ properties:
> >        Address and Length pairs. Specifies regions of memory that are
> >        acceptable to allocate from.
> >  
> > +  alloc-bottom-up:
> > +    type: boolean
> > +    description: >
> > +      Specifies that the memory region should be preferably allocated
> > +      at the lowest available address within the "alloc-ranges" region.
> > +
> > +  alloc-top-down:
> > +    type: boolean
> > +    description: >
> > +      Specifies that the memory region should be preferably allocated
> > +      at the highest available address within the "alloc-ranges" region.
> 
> What happens when both are set?
> 

They are not meant to be both set. I should have added an if statement
for this, sorry about that.

Thanks,
Stephan

Re: [PATCH 1/5] dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}

[Rob Herring]

On Fri, Jun 09, 2023 at 11:16:01AM +0200, Stephan Gerhold wrote:
> Hi Rob,
> 
> Thanks for your suggestions!
> 
> On Thu, Jun 08, 2023 at 08:02:56AM -0600, Rob Herring wrote:
> > On Mon, May 15, 2023 at 12:12:16PM +0200, Stephan Gerhold wrote:
> > > Right now the allocation behavior for dynamic reserved memory is
> > > implementation-defined. On Linux it is dependent on the architecture.
> > > This is usually fine if the address is completely arbitrary.
> > > 
> > > However, when using "alloc-ranges" it is helpful to allow controlling
> > > this. That way you can make sure that the reservations are placed next
> > > to other (static) allocations to keep the free memory contiguous if
> > > possible.
> > 
> > That should already be possible with all the information you 
> > already have. IOW, you are looking at all the region and "alloc-ranges" 
> > addresses to decide top-down or bottom-up. Why can't the kernel do that.
> > 
> 
> Would you accept a patch implementing such a behavior?

Yes.
 
> There are obviously infinitely complicated algorithms possible for the
> allocation. A fairly simple one would be to check if the "alloc-ranges"
> overlap or are adjacent to an already existing reservation, i.e.
> 
>   1. If the "alloc-range" starts at the end or inside an existing
>      reservation, use bottom-up.
>   2. If the "alloc-range" ends at the start or inside an existing
>      reservation, use top-down.
>   3. If both or none is the case, keep current (implementation-defined)
>      behavior.
> 
> For reference, here are some examples how it behaves. |...| is the
> unallocated memory, RRR existing allocations, and each RRR--- line
> below a requested alloc-range (and where it was allocated):
> 
> Bottom-up (rule 1):
>   |.....RRRR................RRRRRRRRR...........|
>             RRR----
>          ---RRR-------
> 
> Top-down (rule 2):
>   |.....RRRR................RRRRRRRRR...........|
>                      ----RRR
>                 ---------RRR------
> 
> Otherwise rule 3 just behaves as currently where either bottom-up
> or top-down is used depending on the implementation/architecture:
>   |.....RRRR................RRRRRRRRR...........|
>                -----RRR
>      or        RRR-----
>           ---------------RRR----
>      or   --RRR-----------------
> 
> There are plenty of edge cases where it doesn't produce the optimal
> result, but it just results in exactly the same behavior as currently
> so it's not any worse (with rule 3):
> 
>   |.....RRRR................RRRRRRRRR...........|
>                           -----------RRR-----
>                  or       ----------------RRR
>                      ---------------------RRR  (no way to handle this
>                  or  RRR---------------------   with top-down/bottom-up)
> 
> > Alternatively, if you really care about the allocation locations, don't 
> > use dynamic regions.
> > 
> 
> Yes, this is the option used at the moment. As outlined in detail in the
> examples of RFC PATCH 4/5 and 5/5 I would like a solution inbetween. The
> exact address doesn't matter but the way (direction) the region is
> filled should preferably stay the same.
> 
> > > 
> > > Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> > > ---
> > >  .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > > index c680e397cfd2..56f4bc6137e7 100644
> > > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > > @@ -52,6 +52,18 @@ properties:
> > >        Address and Length pairs. Specifies regions of memory that are
> > >        acceptable to allocate from.
> > >  
> > > +  alloc-bottom-up:
> > > +    type: boolean
> > > +    description: >
> > > +      Specifies that the memory region should be preferably allocated
> > > +      at the lowest available address within the "alloc-ranges" region.
> > > +
> > > +  alloc-top-down:
> > > +    type: boolean
> > > +    description: >
> > > +      Specifies that the memory region should be preferably allocated
> > > +      at the highest available address within the "alloc-ranges" region.
> > 
> > What happens when both are set?
> > 
> 
> They are not meant to be both set. I should have added an if statement
> for this, sorry about that.

Ideally, you define the properties in a way to avoid that situation 
rather than relying on schema checks. For example, a single property 
with values defined for top-down and bottom-up.

Rob

[PATCH 2/5] of: reserved_mem: Implement alloc-{bottom-up,top-down}

[Stephan Gerhold]

Use memblock_set_bottom_up() to specify an explicit allocation order for
dynamic reservations with "alloc-ranges". Since the default value varies
between architectures the previous value is saved and restored after
trying the allocations.

If neither alloc-bottom-up or alloc-top-down are specified the previous
implementation-defined allocation order is preserved.

Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
---
 drivers/of/of_reserved_mem.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 948efa9f99e3..6443140deacf 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -89,7 +89,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
 	phys_addr_t base = 0, align = 0, size;
 	int len;
 	const __be32 *prop;
-	bool nomap;
+	bool nomap, prev_bottom_up;
 	int ret;
 
 	prop = of_get_flat_dt_prop(node, "size", &len);
@@ -130,6 +130,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
 			return -EINVAL;
 		}
 
+		prev_bottom_up = memblock_bottom_up();
+		if (of_get_flat_dt_prop(node, "alloc-bottom-up", NULL))
+			memblock_set_bottom_up(true);
+		if (of_get_flat_dt_prop(node, "alloc-top-down", NULL))
+			memblock_set_bottom_up(false);
+
 		base = 0;
 
 		while (len > 0) {
@@ -148,6 +154,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
 			len -= t_len;
 		}
 
+		memblock_set_bottom_up(prev_bottom_up);
 	} else {
 		ret = early_init_dt_alloc_reserved_memory_arch(size, align,
 							0, 0, nomap, &base);

-- 
2.40.1

Re: [PATCH 2/5] of: reserved_mem: Implement alloc-{bottom-up,top-down}

[Konrad Dybcio]

On 15.05.2023 12:12, Stephan Gerhold wrote:
> Use memblock_set_bottom_up() to specify an explicit allocation order for
> dynamic reservations with "alloc-ranges". Since the default value varies
> between architectures the previous value is saved and restored after
> trying the allocations.
> 
> If neither alloc-bottom-up or alloc-top-down are specified the previous
> implementation-defined allocation order is preserved.
> 
> Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Konrad
>  drivers/of/of_reserved_mem.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 948efa9f99e3..6443140deacf 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -89,7 +89,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>  	phys_addr_t base = 0, align = 0, size;
>  	int len;
>  	const __be32 *prop;
> -	bool nomap;
> +	bool nomap, prev_bottom_up;
>  	int ret;
>  
>  	prop = of_get_flat_dt_prop(node, "size", &len);
> @@ -130,6 +130,12 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>  			return -EINVAL;
>  		}
>  
> +		prev_bottom_up = memblock_bottom_up();
> +		if (of_get_flat_dt_prop(node, "alloc-bottom-up", NULL))
> +			memblock_set_bottom_up(true);
> +		if (of_get_flat_dt_prop(node, "alloc-top-down", NULL))
> +			memblock_set_bottom_up(false);
> +
>  		base = 0;
>  
>  		while (len > 0) {
> @@ -148,6 +154,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>  			len -= t_len;
>  		}
>  
> +		memblock_set_bottom_up(prev_bottom_up);
>  	} else {
>  		ret = early_init_dt_alloc_reserved_memory_arch(size, align,
>  							0, 0, nomap, &base);
> 

[PATCH 3/5] of: reserved_mem: Use stable allocation order

[Stephan Gerhold]

sort() in Linux is based on heapsort which is not a stable sort
algorithm - equal elements are being reordered. For reserved memory in
the device tree this happens mainly for dynamic allocations: They do not
have an address to sort with, so they are reordered somewhat randomly
when adding/removing other unrelated reserved memory nodes.

Functionally this is not a big problem, but it's confusing during
development when all the addresses change after adding unrelated
reserved memory nodes.

Make the order stable by sorting dynamic allocations according to
the node order in the device tree. Static allocations are not affected
by this because they are still sorted by their (fixed) address.

Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
---
 drivers/of/of_reserved_mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 6443140deacf..f6d14354a534 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -224,6 +224,11 @@ 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;
 }
 

-- 
2.40.1

[PATCH RFC 5/5] arm64: dts: qcom: msm8916: Reserve firmware memory dynamically

[Stephan Gerhold]

DO NOT APPLY - this is just an example to show the motivation for the
patch series. For clarity only some of the device trees are updated.

Most of the reserved firmware memory on MSM8916 can be relocated when
respecting the required alignment. To avoid having to precompute the
reserved memory regions in every board DT, describe the actual
requirements (size, alignment, alloc-ranges) using the dynamic reserved
memory allocation. Use the new "alloc-bottom-up" option to allocate the
memory contiguously to the other reserved memory regions.

This approach has several advantages:

 1. We can define "templates" for the reserved memory regions in
    msm8916.dtsi and keep only device-specific details in the board DT.
    This is useful for the "mpss" region size for example, which varies
    from device to device. It is no longer necessary to redefine all
    firmware regions to shift their addresses.

 2. When some of the functionality (e.g. WiFi, Bluetooth, modem) is not
    enabled or needed for a device, the reserved memory can stay
    disabled, freeing up the unused reservation for Linux.

 3. Devices with special requirements for one of the firmware regions
    are handled automatically. For example, msm8916-longcheer-l8150
    has non-relocatable "wcnss" firmware that must be loaded exactly
    at address 0x8b600000. When this is defined as a static region,
    the other dynamic allocations automatically adjust to a different
    place with suitable alignment.

All in all this approach significantly reduces the boilerplate necessary
to define the different firmware regions, and makes it easier to enable
functionality on the different devices.

Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
---
 arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 13 +++++++
 .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 36 +++++++++----------
 .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 41 +++++++++------------
 arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 29 ++++++++-------
 arch/arm64/boot/dts/qcom/msm8916.dtsi              | 42 ++++++++++++++++------
 5 files changed, 96 insertions(+), 65 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
index 59860a2223b8..534fc9b2f816 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts
@@ -310,6 +310,10 @@ &lpass {
 	status = "okay";
 };
 
+&mba_mem {
+	status = "okay";
+};
+
 &mdss {
 	status = "okay";
 };
@@ -320,6 +324,11 @@ &mpss {
 	firmware-name = "qcom/apq8016/mba.mbn", "qcom/apq8016/modem.mbn";
 };
 
+&mpss_mem {
+	status = "okay";
+	size = <0x0 0x2b00000>;
+};
+
 &pm8916_resin {
 	status = "okay";
 	linux,code = <KEY_VOLUMEDOWN>;
@@ -418,6 +427,10 @@ &wcnss_iris {
 	compatible = "qcom,wcn3620";
 };
 
+&wcnss_mem {
+	status = "okay";
+};
+
 /* Enable CoreSight */
 &cti0 { status = "okay"; };
 &cti1 { status = "okay"; };
diff --git a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
index 4a5eab06c18b..2d2bf255b315 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dts
@@ -29,29 +29,12 @@ chosen {
 	 * fixed address and all other firmware regions are moved to a fitting place.
 	 */
 	reserved-memory {
-		/delete-node/ mpss@86800000;
-		/delete-node/ wcnss@89300000;
-		/delete-node/ venus@89900000;
+		/delete-node/ wcnss;
 
 		wcnss_mem: wcnss@8b600000 {
 			reg = <0x0 0x8b600000 0x0 0x600000>;
 			no-map;
 		};
-
-		venus_mem: venus@8bc00000 {
-			reg = <0x0 0x8bc00000 0x0 0x500000>;
-			no-map;
-		};
-
-		mpss_mem: mpss@8ec00000 {
-			reg = <0x0 0x8ec00000 0x0 0x5000000>;
-			no-map;
-		};
-
-		gps_mem: gps@93c00000 {
-			reg = <0x0 0x93c00000 0x0 0x200000>;
-			no-map;
-		};
 	};
 
 	gpio-keys {
@@ -241,10 +224,23 @@ &blsp1_uart2 {
 	status = "okay";
 };
 
+&gps_mem {
+	status = "okay";
+};
+
+&mba_mem {
+	status = "okay";
+};
+
 &mpss {
 	status = "okay";
 };
 
+&mpss_mem {
+	status = "okay";
+	size = <0x0 0x5000000>;
+};
+
 &pm8916_resin {
 	status = "okay";
 	linux,code = <KEY_VOLUMEDOWN>;
@@ -294,6 +290,10 @@ &wcnss_iris {
 	compatible = "qcom,wcn3620";
 };
 
+&wcnss_mem {
+	status = "okay";
+};
+
 &smd_rpm_regulators {
 	vdd_l1_l2_l3-supply = <&pm8916_s3>;
 	vdd_l4_l5_l6-supply = <&pm8916_s4>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
index 25ad098b1503..82402689b414 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
@@ -36,35 +36,11 @@ chosen {
 	};
 
 	reserved-memory {
-		/delete-node/ mpss@86800000;
-		/delete-node/ wcnss@89300000;
-		/delete-node/ venus@89900000;
-
 		/* Additional memory used by Samsung firmware modifications */
 		tz-apps@85500000 {
 			reg = <0x0 0x85500000 0x0 0xb00000>;
 			no-map;
 		};
-
-		mpss_mem: mpss@86800000 {
-			reg = <0x0 0x86800000 0x0 0x5a00000>;
-			no-map;
-		};
-
-		gps_mem: gps@8c200000 {
-			reg = <0x0 0x8c200000 0x0 0x200000>;
-			no-map;
-		};
-
-		wcnss_mem: wcnss@8c400000 {
-			reg = <0x0 0x8c400000 0x0 0x600000>;
-			no-map;
-		};
-
-		venus_mem: venus@8ca00000 {
-			reg = <0x0 0x8ca00000 0x0 0x500000>;
-			no-map;
-		};
 	};
 
 	gpio-keys {
@@ -287,10 +263,23 @@ &blsp1_uart2 {
 	status = "okay";
 };
 
+&gps_mem {
+	status = "okay";
+};
+
+&mba_mem {
+	status = "okay";
+};
+
 &mpss {
 	status = "okay";
 };
 
+&mpss_mem {
+	status = "okay";
+	size = <0x0 0x5a00000>;
+};
+
 &pm8916_resin {
 	status = "okay";
 	linux,code = <KEY_VOLUMEDOWN>;
@@ -348,6 +337,10 @@ &wcnss_iris {
 	compatible = "qcom,wcn3660b";
 };
 
+&wcnss_mem {
+	status = "okay";
+};
+
 &smd_rpm_regulators {
 	vdd_l1_l2_l3-supply = <&pm8916_s3>;
 	vdd_l4_l5_l6-supply = <&pm8916_s4>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
index 50bae6f214f1..ec073cfbb435 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
@@ -16,18 +16,6 @@ chosen {
 		stdout-path = "serial0";
 	};
 
-	reserved-memory {
-		mpss_mem: mpss@86800000 {
-			reg = <0x0 0x86800000 0x0 0x5500000>;
-			no-map;
-		};
-
-		gps_mem: gps@8bd00000 {
-			reg = <0x0 0x8bd00000 0x0 0x200000>;
-			no-map;
-		};
-	};
-
 	gpio-keys {
 		compatible = "gpio-keys";
 
@@ -91,10 +79,23 @@ &gcc {
 	clocks = <&xo_board>, <&sleep_clk>, <0>, <0>, <0>, <0>, <0>;
 };
 
+&gps_mem {
+	status = "okay";
+};
+
+&mba_mem {
+	status = "okay";
+};
+
 &mpss {
 	status = "okay";
 };
 
+&mpss_mem {
+	status = "okay";
+	size = <0x0 0x5500000>;
+};
+
 &pm8916_usbin {
 	status = "okay";
 };
@@ -126,6 +127,10 @@ &wcnss_iris {
 	compatible = "qcom,wcn3620";
 };
 
+&wcnss_mem {
+	status = "okay";
+};
+
 &smd_rpm_regulators {
 	vdd_l1_l2_l3-supply = <&pm8916_s3>;
 	vdd_l4_l5_l6-supply = <&pm8916_s4>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 7e0fa37a3adf..bc072a0e4d4f 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -78,24 +78,44 @@ rfsa@867e0000 {
 			no-map;
 		};
 
-		mpss_mem: mpss@86800000 {
-			reg = <0x0 0x86800000 0x0 0x2b00000>;
+		mpss_mem: mpss {
+			/* size is device-specific */
+			alignment = <0x0 0x400000>;
+			alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+			alloc-bottom-up;
 			no-map;
+			status = "disabled";
 		};
-
-		wcnss_mem: wcnss@89300000 {
-			reg = <0x0 0x89300000 0x0 0x600000>;
+		gps_mem: gps {
+			size = <0x0 0x200000>;
+			alignment = <0x0 0x100000>;
+			alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+			alloc-bottom-up;
 			no-map;
+			status = "disabled";
 		};
-
-		venus_mem: venus@89900000 {
-			reg = <0x0 0x89900000 0x0 0x600000>;
+		mba_mem: mba {
+			size = <0x0 0x100000>;
+			alignment = <0x0 0x100000>;
+			alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+			alloc-bottom-up;
 			no-map;
+			status = "disabled";
 		};
-
-		mba_mem: mba@8ea00000 {
+		wcnss_mem: wcnss {
+			size = <0x0 0x600000>;
+			alignment = <0x0 0x100000>;
+			alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+			alloc-bottom-up;
+			no-map;
+			status = "disabled";
+		};
+		venus_mem: venus {
+			size = <0x0 0x500000>;
+			alignment = <0x0 0x100000>;
+			alloc-ranges = <0x0 0x86800000 0x0 0x10000000>;
+			alloc-bottom-up;
 			no-map;
-			reg = <0 0x8ea00000 0 0x100000>;
 		};
 	};
 

-- 
2.40.1

Re: [PATCH 0/5] of: reserved_mem: Provide more control about allocation behavior

[Konrad Dybcio]

On 15.05.2023 12:12, Stephan Gerhold wrote:
> Provide more control about the allocation behavior for dynamically 
> allocated reserved memory by adding a "alloc-bottom-up" and 
> "alloc-top-down" option and by making the allocation order 
> deterministic.
> 
> The motivation for this patch series are the many different reserved 
> firmware regions on Qualcomm platforms. Currently it is often necessary 
> to duplicate them into each board DT, because minor differences for 
> some of the firmware regions (e.g. the firmware size) requires shifting
> the base address of all following firmware regions as well.
> 
> I propose describing the actual requirements (size, alignment, 
> alloc-ranges) instead and allocating the reserved regions at runtime. 
> This allows defining only the actual device-specific part in the board 
> DT and having everything else shared in the SoC.dtsi.
> 
> The series starts with two minor additions to the of_reserved_mem code. 
> The last two patches are examples that are meant to show the motivation
> more clearly for the MSM8916 SoC. PATCH 4/5 shows the current (static) 
> approach, then PATCH 5/5 switches to the dynamic allocation based on
> the first 3 patches.
> 
> If the first 3 patches are accepted I would send the full MSM8916 DT
> changes in a separate series.
> 
> Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> ---
Sounds great!

Konrad
> Stephan Gerhold (5):
>       dt-bindings: reserved-memory: Add alloc-{bottom-up,top-down}
>       of: reserved_mem: Implement alloc-{bottom-up,top-down}
>       of: reserved_mem: Use stable allocation order
>       [RFC] arm64: dts: qcom: msm8916: Enable modem on two phones
>       [RFC] arm64: dts: qcom: msm8916: Reserve firmware memory dynamically
> 
>  .../bindings/reserved-memory/reserved-memory.yaml  | 39 ++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 13 +++++++
>  .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 31 ++++++++++++++--
>  .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 21 +++++++++++
>  arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 29 ++++++++-------
>  arch/arm64/boot/dts/qcom/msm8916.dtsi              | 42 ++++++++++++++++------
>  drivers/of/of_reserved_mem.c                       | 14 +++++++-
>  7 files changed, 163 insertions(+), 26 deletions(-)
> ---
> base-commit: 715abedee4cd660ad390659aefa7482f05275bbd
> change-id: 20230510-dt-resv-bottom-up-68d71ff6628f
> 
> Best regards,