mbox series

[0/9] arm64: dts: qcom: msm8916/39: Reserve firmware memory dynamically

Message ID 20230911-msm8916-rmem-v1-0-b7089ec3e3a1@gerhold.net
Headers show
Series arm64: dts: qcom: msm8916/39: Reserve firmware memory dynamically | expand

Message

Stephan Gerhold Sept. 11, 2023, 5:41 p.m. UTC
Refactor the MSM8916 and MSM8939 device trees to make use of dynamic 
memory reservations for the firmware regions, rather than hardcoding 
fixed addresses for each device. This allows to keep most of the 
definitions in the SoC.dtsi while defining the board-specific 
properties (such as firmware size) separately for each device.

The main motivation for this patch set is to simplify enabling the 
modem on the various MSM8916/39 devices. The modem firmware size 
differs on almost each device, which requires redefining *all* of
the firmware reservations with newly calculated addresses to make
room for the larger modem firmware. I've explained this in detail
in a previous RFC:
https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-4-3bf68873dbed@gerhold.net/

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Stephan Gerhold (9):
      arm64: dts: qcom: msm8916: Disable venus by default
      arm64: dts: qcom: msm8916/39: Disable GPU by default
      arm64: dts: qcom: msm8916-ufi: Drop gps_mem for now
      arm64: dts: qcom: msm8916: Reserve firmware memory dynamically
      arm64: dts: qcom: msm8916: Reserve MBA memory dynamically
      arm64: dts: qcom: msm8939: Reserve firmware memory dynamically
      arm64: dts: qcom: msm8916/39: Disable unneeded firmware reservations
      arm64: dts: qcom: msm8916/39: Move mpss_mem size to boards
      arm64: dts: qcom: msm8916/39: Fix venus memory size

 arch/arm64/boot/dts/qcom/apq8016-sbc.dts           | 25 ++++++++++++++
 arch/arm64/boot/dts/qcom/apq8039-t2.dts            |  8 +++++
 arch/arm64/boot/dts/qcom/msm8916-acer-a1-724.dts   | 12 +++++++
 .../boot/dts/qcom/msm8916-alcatel-idol347.dts      | 12 +++++++
 arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts     | 12 +++++++
 arch/arm64/boot/dts/qcom/msm8916-gplus-fl8005a.dts | 12 +++++++
 arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts     | 12 +++++++
 .../boot/dts/qcom/msm8916-longcheer-l8150.dts      | 21 ++++++++++--
 .../boot/dts/qcom/msm8916-longcheer-l8910.dts      | 12 +++++++
 .../dts/qcom/msm8916-samsung-a2015-common.dtsi     | 12 +++++++
 .../boot/dts/qcom/msm8916-samsung-a3u-eur.dts      |  4 +++
 .../boot/dts/qcom/msm8916-samsung-a5u-eur.dts      |  4 +++
 .../dts/qcom/msm8916-samsung-e2015-common.dtsi     |  4 +++
 .../boot/dts/qcom/msm8916-samsung-gt5-common.dtsi  | 12 +++++++
 .../boot/dts/qcom/msm8916-samsung-j5-common.dtsi   | 12 +++++++
 .../boot/dts/qcom/msm8916-samsung-serranove.dts    | 12 +++++++
 arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi          | 33 +++++++++++-------
 .../boot/dts/qcom/msm8916-wingtech-wt88047.dts     | 12 +++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi              | 39 +++++++++++++++++-----
 arch/arm64/boot/dts/qcom/msm8939-samsung-a7.dts    |  4 +++
 .../dts/qcom/msm8939-sony-xperia-kanuti-tulip.dts  |  8 +++++
 arch/arm64/boot/dts/qcom/msm8939.dtsi              | 37 +++++++++++++++-----
 22 files changed, 288 insertions(+), 31 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230720-msm8916-rmem-f88fff6bcded

Best regards,

Comments

Bryan O'Donoghue Sept. 12, 2023, 6:36 a.m. UTC | #1
On 11/09/2023 18:41, Stephan Gerhold wrote:
> gps_mem is needed by the modem firmware for GPS to work. However, it is
> accessed via QMI memshare [1] which is not available upstream yet.
> Until it lands upstream reserving this does not provide any advantage.
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20210319172321.22248-1-nikitos.tr@gmail.com/
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>   arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> index c759c0544dd9..69f268db4df9 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> @@ -22,11 +22,6 @@ mpss_mem: mpss@86800000 {
>   			reg = <0x0 0x86800000 0x0 0x5500000>;
>   			no-map;
>   		};
> -
> -		gps_mem: gps@8bd00000 {
> -			reg = <0x0 0x8bd00000 0x0 0x200000>;
> -			no-map;
> -		};
>   	};
>   
>   	gpio-keys {
> 

Should this have a Fixes tag, should probably be applied to stable.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Stephan Gerhold Sept. 12, 2023, 7:42 a.m. UTC | #2
On Tue, Sep 12, 2023 at 07:36:43AM +0100, Bryan O'Donoghue wrote:
> On 11/09/2023 18:41, Stephan Gerhold wrote:
> > gps_mem is needed by the modem firmware for GPS to work. However, it is
> > accessed via QMI memshare [1] which is not available upstream yet.
> > Until it lands upstream reserving this does not provide any advantage.
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20210319172321.22248-1-nikitos.tr@gmail.com/
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >   arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi | 5 -----
> >   1 file changed, 5 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> > index c759c0544dd9..69f268db4df9 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-ufi.dtsi
> > @@ -22,11 +22,6 @@ mpss_mem: mpss@86800000 {
> >   			reg = <0x0 0x86800000 0x0 0x5500000>;
> >   			no-map;
> >   		};
> > -
> > -		gps_mem: gps@8bd00000 {
> > -			reg = <0x0 0x8bd00000 0x0 0x200000>;
> > -			no-map;
> > -		};
> >   	};
> >   	gpio-keys {
> > 
> 
> Should this have a Fixes tag, should probably be applied to stable.
> 

I'm probably a bit "overcautious" when it comes to stable backporting,
but I think this is not strictly a fix but more like cleanup. It doesn't
cause any trouble to reserve the extra memory, it just wastes some RAM.

Thanks,
Stephan
Konrad Dybcio Sept. 13, 2023, 8:59 a.m. UTC | #3
On 11.09.2023 19:41, Stephan Gerhold wrote:
> At a first glance the MBA memory region on MSM8916 looks intentionally
> placed at the fixed address 0x8ea00000. This is what the ELF headers of
> the firmware specify as base address, and the typical Qualcomm-specific
> bits suggest the binary is not relocatable.
> 
> However, on a closer look this is pointless: Unlike other firmware
> images the hardware expects to have the raw ELF image loaded to the MBA
> region, including the ELF header (without parsing it at all). This
> means that we actually just load the ELF header (not the code!) at
> 0x8ea00000. The real LOAD segments follow at arbitrary aligned
> addresses depending on the structure of the ELF binary.
> 
> In practice it looks like we can use an arbitrary 1 MiB-aligned region
> for MBA. The downstream/vendor kernel just allocates this dynamically
> at an arbitrary (aligned) address.
> 
> Drop the pointless fixed address and use the new dynamic reserved
> memory mechanism to allocate a region close to the others. This reduces
> gaps in the memory map and provides Linux with more contiguous memory.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Bryan O'Donoghue Sept. 13, 2023, 9:12 a.m. UTC | #4
On 13/09/2023 10:06, Konrad Dybcio wrote:
> On 11.09.2023 19:41, Stephan Gerhold wrote:
>> 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.
>>
>> 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. WCNSS, Modem, Venus) 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@gerhold.net>
>> ---
> [...]
> 
>>   
>>   		mpss_mem: mpss@86800000 {
>> +			/*
>> +			 * The memory region for the mpss firmware is generally
>> +			 * relocatable and could be allocated dynamically.
>> +			 * However, many firmware versions tend to fail when
>> +			 * loaded to some special addresses, so it is hard to
>> +			 * define reliable alloc-ranges.
>> +			 *
>> +			 * alignment = <0x0 0x400000>;
>> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
>> +			 */
> Do we know of any devices that this would actually work on?
> 
> Konrad

I have the same question here.

How was this tested ?

---
bod
Stephan Gerhold Sept. 13, 2023, 10:14 a.m. UTC | #5
On Wed, Sep 13, 2023 at 10:12:12AM +0100, Bryan O'Donoghue wrote:
> On 13/09/2023 10:06, Konrad Dybcio wrote:
> > On 11.09.2023 19:41, Stephan Gerhold wrote:
> > > 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.
> > > 
> > > 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. WCNSS, Modem, Venus) 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@gerhold.net>
> > > ---
> > [...]
> > 
> > >   		mpss_mem: mpss@86800000 {
> > > +			/*
> > > +			 * The memory region for the mpss firmware is generally
> > > +			 * relocatable and could be allocated dynamically.
> > > +			 * However, many firmware versions tend to fail when
> > > +			 * loaded to some special addresses, so it is hard to
> > > +			 * define reliable alloc-ranges.
> > > +			 *
> > > +			 * alignment = <0x0 0x400000>;
> > > +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
> > > +			 */
> > Do we know of any devices that this would actually work on?

Yes, the "modem" firmware on DB410c seems to be fine with literally all
correctly aligned addresses I've tested so far. But when I manually
experimented with other addresses on actual smartphones it exploded on
certain addresses, specific to the firmware version / device.

> I have the same question here.
> 
> How was this tested ?
> 

Bryan, your question sounds more general, about the dynamic allocation
of the firmware regions in general (also for wcnss etc).

I'll just explain my tests for both with more detail :D

For wcnss/venus a different memory region has been used in postmarketOS
for years because I shifted it early on to make room for the larger
modem firmware. I've never heard of any problems with this on the 50+
different MSM8916-related devices we support there, with many different
firmware versions. When preparing this patch set I experimented some
more and noticed there is a limit (I think around 0x90000000) where TZ
refuses to load/verify the firmware. The alloc-ranges is chosen
conservatively to avoid potential trouble. Also because I didn't see
any use case to have a huge range specified there...

For mpss it's more tricky. First, if you'd comment out the alignment and
alloc-ranges it would most likely work correctly because Linux just
allocates it at the start of the range, on 0x86800000 like it was
before. However, the DT spec doesn't strictly specify where inside the
range the region is allocated, so I manually experimented with some
other addresses within the range:

 - On DB410c it works just fine. All addresses I tried work without any
   problems.

 - On longcheer-l8150 the modem firmare works fine when the memory
   region starts somewhere between 0x86800000 and 0x8a800000. It also
   works again after 0x8e800000. But on anything between 0x8a800000 and
   0x8e800000 it's broken for who knows what reason.

 - On some Samsung devices only 0x86800000 and maybe one or two other
   addresses worked, again for who knows what reason. Most other
   addresses were broken.

The symptom is usually some timeout while starting, the actual signature
verification seems to work fine. I believe that in theory it's supposed
to be relocatable, but in practice they hardcoded some addresses or did
some other funny stuff in some of the firmware versions.

To be safe my conclusion was to keep mpss at a fixed address and only
allocate the others dynamically. This is how the patch implements it.

Thanks,
Stephan
Konrad Dybcio Sept. 13, 2023, 7:39 p.m. UTC | #6
On 13.09.2023 12:14, Stephan Gerhold wrote:
> On Wed, Sep 13, 2023 at 10:12:12AM +0100, Bryan O'Donoghue wrote:
>> On 13/09/2023 10:06, Konrad Dybcio wrote:
>>> On 11.09.2023 19:41, Stephan Gerhold wrote:
>>>> 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.
>>>>
>>>> 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. WCNSS, Modem, Venus) 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@gerhold.net>
>>>> ---
>>> [...]
>>>
>>>>   		mpss_mem: mpss@86800000 {
>>>> +			/*
>>>> +			 * The memory region for the mpss firmware is generally
>>>> +			 * relocatable and could be allocated dynamically.
>>>> +			 * However, many firmware versions tend to fail when
>>>> +			 * loaded to some special addresses, so it is hard to
>>>> +			 * define reliable alloc-ranges.
>>>> +			 *
>>>> +			 * alignment = <0x0 0x400000>;
>>>> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
>>>> +			 */
>>> Do we know of any devices that this would actually work on?
> 
> Yes, the "modem" firmware on DB410c seems to be fine with literally all
> correctly aligned addresses I've tested so far. But when I manually
> experimented with other addresses on actual smartphones it exploded on
> certain addresses, specific to the firmware version / device.
Moreover, the "modem" on DB410c would probably be fine with
*anything* you try to give it..

[...]

> 
>  - On DB410c it works just fine. All addresses I tried work without any
>    problems.
> 
>  - On longcheer-l8150 the modem firmare works fine when the memory
>    region starts somewhere between 0x86800000 and 0x8a800000. It also
>    works again after 0x8e800000. But on anything between 0x8a800000 and
>    0x8e800000 it's broken for who knows what reason.
> 
>  - On some Samsung devices only 0x86800000 and maybe one or two other
>    addresses worked, again for who knows what reason. Most other
>    addresses were broken.
Were you able to find a phone (likely a very reference-design-based
one) that this worked on, btw?

[...]

> To be safe my conclusion was to keep mpss at a fixed address and only
> allocate the others dynamically. This is how the patch implements it.
That sounds like the sane approach indeed.

Konrad
Stephan Gerhold Sept. 14, 2023, 2:09 p.m. UTC | #7
On Wed, Sep 13, 2023 at 09:39:50PM +0200, Konrad Dybcio wrote:
> On 13.09.2023 12:14, Stephan Gerhold wrote:
> > On Wed, Sep 13, 2023 at 10:12:12AM +0100, Bryan O'Donoghue wrote:
> >> On 13/09/2023 10:06, Konrad Dybcio wrote:
> >>> On 11.09.2023 19:41, Stephan Gerhold wrote:
> >>>> 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.
> >>>>
> >>>> 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. WCNSS, Modem, Venus) 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@gerhold.net>
> >>>> ---
> >>> [...]
> >>>
> >>>>   		mpss_mem: mpss@86800000 {
> >>>> +			/*
> >>>> +			 * The memory region for the mpss firmware is generally
> >>>> +			 * relocatable and could be allocated dynamically.
> >>>> +			 * However, many firmware versions tend to fail when
> >>>> +			 * loaded to some special addresses, so it is hard to
> >>>> +			 * define reliable alloc-ranges.
> >>>> +			 *
> >>>> +			 * alignment = <0x0 0x400000>;
> >>>> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
> >>>> +			 */
> >>> Do we know of any devices that this would actually work on?
> [...]
> >  - On DB410c it works just fine. All addresses I tried work without any
> >    problems.
> > 
> >  - On longcheer-l8150 the modem firmare works fine when the memory
> >    region starts somewhere between 0x86800000 and 0x8a800000. It also
> >    works again after 0x8e800000. But on anything between 0x8a800000 and
> >    0x8e800000 it's broken for who knows what reason.
> > [...]
> Were you able to find a phone (likely a very reference-design-based
> one) that this worked on, btw?

Actually I would count the Longcheer devices (l8150 = Wileyfox Swift and
l8910 = BQ Aquaris X5) to the category of close-to-QRD-based devices.
Based on quick tests both behave like described above (only
0x8a800000-0x8e800000 is broken). Same for wingtech-wt88047.

In other words, for those using the dynamic allocation would work fine,
because the alloc-ranges = <0x0 0x86800000 0x0 0x8000000>; only includes
working start addresses from 0x86800000 to ~0x89800000 (with a size of
0x5000000).

I guess I could use it for them and only make other devices use a fixed
address. But I also don't quite have the capacity to test every device
to see if relocating the region works or not.

I think it's still easiest to allocate mpss on a fixed address
everywhere. The only real disadvantage is that overriding "reg", e.g.

	&mpss_mem {
		reg = <0x0 0x86800000 0x0 0x5100000>;
	};

is a bit more ugly than overriding size:

	&mpss_mem {
		size = <0x0 0x5100000>;
	};

but well, this is a very minor disadvantage.

Thanks,
Stephan
Konrad Dybcio Sept. 15, 2023, 1:52 p.m. UTC | #8
On 14.09.2023 16:09, Stephan Gerhold wrote:
> On Wed, Sep 13, 2023 at 09:39:50PM +0200, Konrad Dybcio wrote:
>> On 13.09.2023 12:14, Stephan Gerhold wrote:
>>> On Wed, Sep 13, 2023 at 10:12:12AM +0100, Bryan O'Donoghue wrote:
>>>> On 13/09/2023 10:06, Konrad Dybcio wrote:
>>>>> On 11.09.2023 19:41, Stephan Gerhold wrote:
>>>>>> 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.
>>>>>>
>>>>>> 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. WCNSS, Modem, Venus) 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@gerhold.net>
>>>>>> ---
>>>>> [...]
>>>>>
>>>>>>   		mpss_mem: mpss@86800000 {
>>>>>> +			/*
>>>>>> +			 * The memory region for the mpss firmware is generally
>>>>>> +			 * relocatable and could be allocated dynamically.
>>>>>> +			 * However, many firmware versions tend to fail when
>>>>>> +			 * loaded to some special addresses, so it is hard to
>>>>>> +			 * define reliable alloc-ranges.
>>>>>> +			 *
>>>>>> +			 * alignment = <0x0 0x400000>;
>>>>>> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
>>>>>> +			 */
>>>>> Do we know of any devices that this would actually work on?
>> [...]
>>>  - On DB410c it works just fine. All addresses I tried work without any
>>>    problems.
>>>
>>>  - On longcheer-l8150 the modem firmare works fine when the memory
>>>    region starts somewhere between 0x86800000 and 0x8a800000. It also
>>>    works again after 0x8e800000. But on anything between 0x8a800000 and
>>>    0x8e800000 it's broken for who knows what reason.
>>> [...]
>> Were you able to find a phone (likely a very reference-design-based
>> one) that this worked on, btw?
> 
> Actually I would count the Longcheer devices (l8150 = Wileyfox Swift and
> l8910 = BQ Aquaris X5) to the category of close-to-QRD-based devices.
> Based on quick tests both behave like described above (only
> 0x8a800000-0x8e800000 is broken). Same for wingtech-wt88047.
> 
> In other words, for those using the dynamic allocation would work fine,
> because the alloc-ranges = <0x0 0x86800000 0x0 0x8000000>; only includes
> working start addresses from 0x86800000 to ~0x89800000 (with a size of
> 0x5000000).
> 
> I guess I could use it for them and only make other devices use a fixed
> address. But I also don't quite have the capacity to test every device
> to see if relocating the region works or not.
> 
> I think it's still easiest to allocate mpss on a fixed address
> everywhere. The only real disadvantage is that overriding "reg", e.g.
> 
> 	&mpss_mem {
> 		reg = <0x0 0x86800000 0x0 0x5100000>;
> 	};
> 
> is a bit more ugly than overriding size:
> 
> 	&mpss_mem {
> 		size = <0x0 0x5100000>;
> 	};
> 
> but well, this is a very minor disadvantage.
So in other words, this only *really* works on apq8016?

Konrad
Stephan Gerhold Sept. 15, 2023, 2 p.m. UTC | #9
On Fri, Sep 15, 2023 at 03:52:36PM +0200, Konrad Dybcio wrote:
> On 14.09.2023 16:09, Stephan Gerhold wrote:
> > On Wed, Sep 13, 2023 at 09:39:50PM +0200, Konrad Dybcio wrote:
> >> On 13.09.2023 12:14, Stephan Gerhold wrote:
> >>> On Wed, Sep 13, 2023 at 10:12:12AM +0100, Bryan O'Donoghue wrote:
> >>>> On 13/09/2023 10:06, Konrad Dybcio wrote:
> >>>>> On 11.09.2023 19:41, Stephan Gerhold wrote:
> >>>>>> 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.
> >>>>>>
> >>>>>> 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. WCNSS, Modem, Venus) 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@gerhold.net>
> >>>>>> ---
> >>>>> [...]
> >>>>>
> >>>>>>   		mpss_mem: mpss@86800000 {
> >>>>>> +			/*
> >>>>>> +			 * The memory region for the mpss firmware is generally
> >>>>>> +			 * relocatable and could be allocated dynamically.
> >>>>>> +			 * However, many firmware versions tend to fail when
> >>>>>> +			 * loaded to some special addresses, so it is hard to
> >>>>>> +			 * define reliable alloc-ranges.
> >>>>>> +			 *
> >>>>>> +			 * alignment = <0x0 0x400000>;
> >>>>>> +			 * alloc-ranges = <0x0 0x86800000 0x0 0x8000000>;
> >>>>>> +			 */
> >>>>> Do we know of any devices that this would actually work on?
> >> [...]
> >>>  - On DB410c it works just fine. All addresses I tried work without any
> >>>    problems.
> >>>
> >>>  - On longcheer-l8150 the modem firmare works fine when the memory
> >>>    region starts somewhere between 0x86800000 and 0x8a800000. It also
> >>>    works again after 0x8e800000. But on anything between 0x8a800000 and
> >>>    0x8e800000 it's broken for who knows what reason.
> >>> [...]
> >> Were you able to find a phone (likely a very reference-design-based
> >> one) that this worked on, btw?
> > 
> > Actually I would count the Longcheer devices (l8150 = Wileyfox Swift and
> > l8910 = BQ Aquaris X5) to the category of close-to-QRD-based devices.
> > Based on quick tests both behave like described above (only
> > 0x8a800000-0x8e800000 is broken). Same for wingtech-wt88047.
> > 
> > In other words, for those using the dynamic allocation would work fine,
> > because the alloc-ranges = <0x0 0x86800000 0x0 0x8000000>; only includes
> > working start addresses from 0x86800000 to ~0x89800000 (with a size of
> > 0x5000000).
> > 
> > I guess I could use it for them and only make other devices use a fixed
> > address. But I also don't quite have the capacity to test every device
> > to see if relocating the region works or not.
> > 
> > I think it's still easiest to allocate mpss on a fixed address
> > everywhere. The only real disadvantage is that overriding "reg", e.g.
> > 
> > 	&mpss_mem {
> > 		reg = <0x0 0x86800000 0x0 0x5100000>;
> > 	};
> > 
> > is a bit more ugly than overriding size:
> > 
> > 	&mpss_mem {
> > 		size = <0x0 0x5100000>;
> > 	};
> > 
> > but well, this is a very minor disadvantage.
> So in other words, this only *really* works on apq8016?
> 

Hm no? As I wrote, with alloc-ranges = <0x0 0x86800000 0x0 0x8000000> it
also works on the close-to-QRD MSM8916 devices. There are some addresses
that don't work but those might also exist on APQ8016. I haven't tested
placing the firmware all over RAM on all sorts of obscure addresses (and
there is probably little advantage doing so).

With the limited alloc-ranges it would likely work on all devices that
are close to the reference design/firmware from Qualcomm.

Thanks,
Stephan
Bjorn Andersson Sept. 20, 2023, 5:14 p.m. UTC | #10
On Mon, 11 Sep 2023 19:41:42 +0200, Stephan Gerhold wrote:
> Refactor the MSM8916 and MSM8939 device trees to make use of dynamic
> memory reservations for the firmware regions, rather than hardcoding
> fixed addresses for each device. This allows to keep most of the
> definitions in the SoC.dtsi while defining the board-specific
> properties (such as firmware size) separately for each device.
> 
> The main motivation for this patch set is to simplify enabling the
> modem on the various MSM8916/39 devices. The modem firmware size
> differs on almost each device, which requires redefining *all* of
> the firmware reservations with newly calculated addresses to make
> room for the larger modem firmware. I've explained this in detail
> in a previous RFC:
> https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-4-3bf68873dbed@gerhold.net/
> 
> [...]

Applied, thanks!

[1/9] arm64: dts: qcom: msm8916: Disable venus by default
      commit: 29589248420766cd492e6db0632d6f59ec216e92
[2/9] arm64: dts: qcom: msm8916/39: Disable GPU by default
      commit: 0ce5bb825d54c904b217cc7f1131e084e02ed037
[3/9] arm64: dts: qcom: msm8916-ufi: Drop gps_mem for now
      commit: 40eb256e5fd1fd49813a27a252b7f45ccca89fde
[4/9] arm64: dts: qcom: msm8916: Reserve firmware memory dynamically
      commit: 0ed3d82862e8451cc2b14ddb1b064e72ba3e5a60
[5/9] arm64: dts: qcom: msm8916: Reserve MBA memory dynamically
      commit: b4f3a410061bf5a8cc148c9435479da580abf85b
[6/9] arm64: dts: qcom: msm8939: Reserve firmware memory dynamically
      commit: b22bef3dbc3a67a796f82f49a6df9e413211cb40
[7/9] arm64: dts: qcom: msm8916/39: Disable unneeded firmware reservations
      commit: 0ece6438a8c0492a7df036d57ac299500a25cb70
[8/9] arm64: dts: qcom: msm8916/39: Move mpss_mem size to boards
      commit: 35efa1be51bd6db067d7380b34538b17b9f250a8
[9/9] arm64: dts: qcom: msm8916/39: Fix venus memory size
      commit: e3c6386c6a5d0187f103fc9bf39850ac15c01690

Best regards,