[v9,2/4] arm64: dts: qcom: qcs8550: introduce qcs8550 dtsi

QCS8550 is derived from SM8550. The difference between SM8550 and
QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
in IoT products.
QCS8550 firmware has different memory map compared to SM8550.
The memory map will be runtime added through bootloader.
There are 3 types of reserved memory regions here:
1. Firmware related regions which aren't shared with kernel.
    The device tree source in kernel doesn't need to have node to indicate
the firmware related reserved information. Bootloader converys the
information by updating devicetree at runtime.
    This will be described as: UEFI saves the physical address of the
UEFI System Table to dts file's chosen node. Kernel read this table and
add reserved memory regions to efi config table. Current reserved memory
region may have reserved region which was not yet used, release note of
the firmware have such kind of information.
2. Firmware related memory regions which are shared with Kernel
    The device tree source in the kernel needs to include nodes that
indicate fimware-related shared information. A label name is suggested
because this type of shared information needs to be referenced by
specific drivers for handling purposes.
3. Remoteproc regions.
    Remoteproc regions will be reserved and then assigned to subsystem
firmware later.
Here is a reserved memory map for this platform:
0x100000000 +-------------------+
            |                   |
            | Firmware Related  |
            |                   |
 0xd4d00000 +-------------------+
            |                   |
            | Kernel Available  |
            |                   |
 0xa7000000 +-------------------+
            |                   |
            | Remoteproc Region |
            |                   |
 0x8a800000 +-------------------+
            |                   |
            | Firmware Related  |
            |                   |
 0x80000000 +-------------------+

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167 ++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi

On 6/3/2024 5:20 PM, Caleb Connolly wrote:
> Hi Tengfei,
> 
> On 29/05/2024 12:09, Tengfei Fan wrote:
>> QCS8550 is derived from SM8550. The difference between SM8550 and
>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
>> in IoT products.
>> QCS8550 firmware has different memory map compared to SM8550.
>> The memory map will be runtime added through bootloader.
>> There are 3 types of reserved memory regions here:
>> 1. Firmware related regions which aren't shared with kernel.
>>      The device tree source in kernel doesn't need to have node to
>> indicate
>> the firmware related reserved information. Bootloader converys the
>> information by updating devicetree at runtime.
>>      This will be described as: UEFI saves the physical address of the
>> UEFI System Table to dts file's chosen node. Kernel read this table and
>> add reserved memory regions to efi config table. Current reserved memory
>> region may have reserved region which was not yet used, release note of
>> the firmware have such kind of information.
> 
> Are you describing some particular quirk of the platform here, or just
> standard UEFI booting?

It's standard UEFI booting efi config table.
> 
> When booting with UEFI, the memory map is passed via the ESRT, so having
> memory that the kernel shouldn't use it pretty simple (and typical).

yes. It is very simple. And the bootloader firmware config the
"reserved" region in the efi config table from the uefi firmware.
>> 2. Firmware related memory regions which are shared with Kernel
>>      The device tree source in the kernel needs to include nodes that
>> indicate fimware-related shared information. A label name is suggested
>> because this type of shared information needs to be referenced by
>> specific drivers for handling purposes.
> 
> Again, is there something non-standard here? If not I would suggest
> dropping these detail comments as they might be misleading.

Detailed comments is used to describe current device tree reserved
memory regions.

Current patch is not creating a new mechanism to have memory map
described. But it is the first time qcom device trees use this design,
and have a simplified(also more compatible) device tree reserved memory
region(memory map). Previously, bootloader(apps bootloader) only pass
the whole physical memory base and size, and use reserved memory nodes
only in device tree(which is also a standard choose).

So that's why it is detailed comments for other qcom platform reference.

> 
> Thanks and regards,
>> 3. Remoteproc regions.
>>      Remoteproc regions will be reserved and then assigned to subsystem
>> firmware later.
>> Here is a reserved memory map for this platform:
>> 0x100000000 +-------------------+
>>              |                   |
>>              | Firmware Related  |
>>              |                   |
>>   0xd4d00000 +-------------------+
>>              |                   |
>>              | Kernel Available  |
>>              |                   |
>>   0xa7000000 +-------------------+
>>              |                   |
>>              | Remoteproc Region |
>>              |                   |
>>   0x8a800000 +-------------------+
>>              |                   |
>>              | Firmware Related  |
>>              |                   |
>>   0x80000000 +-------------------+
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167 ++++++++++++++++++++++++++
>>   1 file changed, 167 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>> new file mode 100644
>> index 000000000000..685668c6ad14
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>> @@ -0,0 +1,167 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>> rights reserved.
>> + */
>> +
>> +#include "sm8550.dtsi"
>> +
>> +/delete-node/ &reserved_memory;
>> +
>> +/ {
>> +    reserved_memory: reserved-memory {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        ranges;
>> +
>> +
>> +        /* These are 3 types of reserved memory regions here:
>> +         * 1. Firmware related regions which aren't shared with kernel.
>> +         *     The device tree source in kernel doesn't need to have
>> node to
>> +         * indicate the firmware related reserved information.
>> Bootloader
>> +         * conveys the information by updating devicetree at runtime.
>> +         *     This will be described as: UEFI saves the physical
>> address of
>> +         * the UEFI System Table to dts file's chosen node. Kernel
>> read this
>> +         * table and add reserved memory regions to efi config table.
>> Current
>> +         * reserved memory region may have reserved region which was
>> not yet
>> +         * used, release note of the firmware have such kind of
>> information.
>> +         * 2. Firmware related memory regions which are shared with
>> Kernel
>> +         *     The device tree source in the kernel needs to include
>> nodes
>> +         * that indicate fimware-related shared information. A label
>> name
>> +         * is suggested because this type of shared information needs to
>> +         * be referenced by specific drivers for handling purposes.
>> +         * 3. Remoteproc regions.
>> +         *     Remoteproc regions will be reserved and then assigned to
>> +         * subsystem firmware later.
>> +         * Here is a reserved memory map for this platform:
>> +         * 0x100000000 +-------------------+
>> +         *             |                   |
>> +         *             | Firmware Related  |
>> +         *             |                   |
>> +         *  0xd4d00000 +-------------------+
>> +         *             |                   |
>> +         *             | Kernel Available  |
>> +         *             |                   |
>> +         *  0xa7000000 +-------------------+
>> +         *             |                   |
>> +         *             | Remoteproc Region |
>> +         *             |                   |
>> +         *  0x8a800000 +-------------------+
>> +         *             |                   |
>> +         *             | Firmware Related  |
>> +         *             |                   |
>> +         *  0x80000000 +-------------------+
>> +         */
>> +
>> +        /*
>> +         * Firmware related regions, bootloader will possible reserve
>> parts of
>> +         * region from 0x80000000..0x8a800000.
>> +         */
>> +        aop_image_mem: aop-image-region@81c00000 {
>> +            reg = <0x0 0x81c00000 0x0 0x60000>;
>> +            no-map;
>> +        };
>> +
>> +        aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>> +            compatible = "qcom,cmd-db";
>> +            reg = <0x0 0x81c60000 0x0 0x20000>;
>> +            no-map;
>> +        };
>> +
>> +        aop_config_mem: aop-config-region@81c80000 {
>> +            no-map;
>> +            reg = <0x0 0x81c80000 0x0 0x20000>;
>> +        };
>> +
>> +        smem_mem: smem-region@81d00000 {
>> +            compatible = "qcom,smem";
>> +            reg = <0x0 0x81d00000 0x0 0x200000>;
>> +            hwlocks = <&tcsr_mutex 3>;
>> +            no-map;
>> +        };
>> +
>> +        adsp_mhi_mem: adsp-mhi-region@81f00000 {
>> +            reg = <0x0 0x81f00000 0x0 0x20000>;
>> +            no-map;
>> +        };
>> +
>> +        /* PIL region */
>> +        mpss_mem: mpss-region@8a800000 {
>> +            reg = <0x0 0x8a800000 0x0 0x10800000>;
>> +            no-map;
>> +        };
>> +
>> +        q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
>> +            reg = <0x0 0x9b000000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        ipa_fw_mem: ipa-fw-region@9b080000 {
>> +            reg = <0x0 0x9b080000 0x0 0x10000>;
>> +            no-map;
>> +        };
>> +
>> +        ipa_gsi_mem: ipa-gsi-region@9b090000 {
>> +            reg = <0x0 0x9b090000 0x0 0xa000>;
>> +            no-map;
>> +        };
>> +
>> +        gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
>> +            reg = <0x0 0x9b09a000 0x0 0x2000>;
>> +            no-map;
>> +        };
>> +
>> +        spss_region_mem: spss-region@9b100000 {
>> +            reg = <0x0 0x9b100000 0x0 0x180000>;
>> +            no-map;
>> +        };
>> +
>> +        spu_secure_shared_memory_mem:
>> spu-secure-shared-memory-region@9b280000 {
>> +            reg = <0x0 0x9b280000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        camera_mem: camera-region@9b300000 {
>> +            reg = <0x0 0x9b300000 0x0 0x800000>;
>> +            no-map;
>> +        };
>> +
>> +        video_mem: video-region@9bb00000 {
>> +            reg = <0x0 0x9bb00000 0x0 0x700000>;
>> +            no-map;
>> +        };
>> +
>> +        cvp_mem: cvp-region@9c200000 {
>> +            reg = <0x0 0x9c200000 0x0 0x700000>;
>> +            no-map;
>> +        };
>> +
>> +        cdsp_mem: cdsp-region@9c900000 {
>> +            reg = <0x0 0x9c900000 0x0 0x2000000>;
>> +            no-map;
>> +        };
>> +
>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
>> +            reg = <0x0 0x9e900000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
>> +            reg = <0x0 0x9e980000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        adspslpi_mem: adspslpi-region@9ea00000 {
>> +            reg = <0x0 0x9ea00000 0x0 0x4080000>;
>> +            no-map;
>> +        };
>> +
>> +        /*
>> +         * Firmware related regions, bootloader will possible reserve
>> parts of
>> +         * region from 0xd8000000..0x100000000.
>> +         */
>> +        mpss_dsm_mem: mpss_dsm_region@d4d00000 {
>> +            reg = <0x0 0xd4d00000 0x0 0x3300000>;
>> +            no-map;
>> +        };
>> +    };
>> +};
>

On 04/06/2024 12:51, Aiqun Yu (Maria) wrote:
> 
> 
> On 6/3/2024 5:20 PM, Caleb Connolly wrote:
>> Hi Tengfei,
>>
>> On 29/05/2024 12:09, Tengfei Fan wrote:
>>> QCS8550 is derived from SM8550. The difference between SM8550 and
>>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
>>> in IoT products.
>>> QCS8550 firmware has different memory map compared to SM8550.
>>> The memory map will be runtime added through bootloader.
>>> There are 3 types of reserved memory regions here:
>>> 1. Firmware related regions which aren't shared with kernel.
>>>       The device tree source in kernel doesn't need to have node to
>>> indicate
>>> the firmware related reserved information. Bootloader converys the
>>> information by updating devicetree at runtime.
>>>       This will be described as: UEFI saves the physical address of the
>>> UEFI System Table to dts file's chosen node. Kernel read this table and
>>> add reserved memory regions to efi config table. Current reserved memory
>>> region may have reserved region which was not yet used, release note of
>>> the firmware have such kind of information.
>>
>> Are you describing some particular quirk of the platform here, or just
>> standard UEFI booting?
> 
> It's standard UEFI booting efi config table.

Ok, thanks for confirming.
>>
>> When booting with UEFI, the memory map is passed via the ESRT, so having
>> memory that the kernel shouldn't use it pretty simple (and typical).

woo! \o/
> 
> yes. It is very simple. And the bootloader firmware config the
> "reserved" region in the efi config table from the uefi firmware.
>>> 2. Firmware related memory regions which are shared with Kernel
>>>       The device tree source in the kernel needs to include nodes that
>>> indicate fimware-related shared information. A label name is suggested
>>> because this type of shared information needs to be referenced by
>>> specific drivers for handling purposes.
>>
>> Again, is there something non-standard here? If not I would suggest
>> dropping these detail comments as they might be misleading.
> 
> Detailed comments is used to describe current device tree reserved
> memory regions.
> 
> Current patch is not creating a new mechanism to have memory map
> described. But it is the first time qcom device trees use this design,
> and have a simplified(also more compatible) device tree reserved memory
> region(memory map). Previously, bootloader(apps bootloader) only pass
> the whole physical memory base and size, and use reserved memory nodes
> only in device tree(which is also a standard choose).
> 
> So that's why it is detailed comments for other qcom platform reference.

Doesn't the rb3gen2 also use this design?
> 
>>
>> Thanks and regards,
>>> 3. Remoteproc regions.
>>>       Remoteproc regions will be reserved and then assigned to subsystem
>>> firmware later.
>>> Here is a reserved memory map for this platform:
>>> 0x100000000 +-------------------+
>>>               |                   |
>>>               | Firmware Related  |
>>>               |                   |
>>>    0xd4d00000 +-------------------+
>>>               |                   |
>>>               | Kernel Available  |
>>>               |                   |
>>>    0xa7000000 +-------------------+
>>>               |                   |
>>>               | Remoteproc Region |
>>>               |                   |
>>>    0x8a800000 +-------------------+
>>>               |                   |
>>>               | Firmware Related  |
>>>               |                   |
>>>    0x80000000 +-------------------+
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167 ++++++++++++++++++++++++++
>>>    1 file changed, 167 insertions(+)
>>>    create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>> new file mode 100644
>>> index 000000000000..685668c6ad14
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>> @@ -0,0 +1,167 @@
>>> +// SPDX-License-Identifier: BSD-3-Clause
>>> +/*
>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>>> rights reserved.
>>> + */
>>> +
>>> +#include "sm8550.dtsi"
>>> +
>>> +/delete-node/ &reserved_memory;
>>> +
>>> +/ {
>>> +    reserved_memory: reserved-memory {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges;
>>> +
>>> +
>>> +        /* These are 3 types of reserved memory regions here:
>>> +         * 1. Firmware related regions which aren't shared with kernel.
>>> +         *     The device tree source in kernel doesn't need to have
>>> node to
>>> +         * indicate the firmware related reserved information.
>>> Bootloader
>>> +         * conveys the information by updating devicetree at runtime.
>>> +         *     This will be described as: UEFI saves the physical
>>> address of
>>> +         * the UEFI System Table to dts file's chosen node. Kernel
>>> read this
>>> +         * table and add reserved memory regions to efi config table.
>>> Current
>>> +         * reserved memory region may have reserved region which was
>>> not yet
>>> +         * used, release note of the firmware have such kind of
>>> information.
>>> +         * 2. Firmware related memory regions which are shared with
>>> Kernel
>>> +         *     The device tree source in the kernel needs to include
>>> nodes
>>> +         * that indicate fimware-related shared information. A label
>>> name
>>> +         * is suggested because this type of shared information needs to
>>> +         * be referenced by specific drivers for handling purposes.
>>> +         * 3. Remoteproc regions.
>>> +         *     Remoteproc regions will be reserved and then assigned to
>>> +         * subsystem firmware later.
>>> +         * Here is a reserved memory map for this platform:
>>> +         * 0x100000000 +-------------------+
>>> +         *             |                   |
>>> +         *             | Firmware Related  |
>>> +         *             |                   |
>>> +         *  0xd4d00000 +-------------------+
>>> +         *             |                   |
>>> +         *             | Kernel Available  |
>>> +         *             |                   |
>>> +         *  0xa7000000 +-------------------+
>>> +         *             |                   |
>>> +         *             | Remoteproc Region |
>>> +         *             |                   |
>>> +         *  0x8a800000 +-------------------+
>>> +         *             |                   |
>>> +         *             | Firmware Related  |
>>> +         *             |                   |
>>> +         *  0x80000000 +-------------------+
>>> +         */
>>> +
>>> +        /*
>>> +         * Firmware related regions, bootloader will possible reserve
>>> parts of
>>> +         * region from 0x80000000..0x8a800000.
>>> +         */
>>> +        aop_image_mem: aop-image-region@81c00000 {
>>> +            reg = <0x0 0x81c00000 0x0 0x60000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>>> +            compatible = "qcom,cmd-db";
>>> +            reg = <0x0 0x81c60000 0x0 0x20000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        aop_config_mem: aop-config-region@81c80000 {
>>> +            no-map;
>>> +            reg = <0x0 0x81c80000 0x0 0x20000>;
>>> +        };
>>> +
>>> +        smem_mem: smem-region@81d00000 {
>>> +            compatible = "qcom,smem";
>>> +            reg = <0x0 0x81d00000 0x0 0x200000>;
>>> +            hwlocks = <&tcsr_mutex 3>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        adsp_mhi_mem: adsp-mhi-region@81f00000 {
>>> +            reg = <0x0 0x81f00000 0x0 0x20000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        /* PIL region */
>>> +        mpss_mem: mpss-region@8a800000 {
>>> +            reg = <0x0 0x8a800000 0x0 0x10800000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
>>> +            reg = <0x0 0x9b000000 0x0 0x80000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        ipa_fw_mem: ipa-fw-region@9b080000 {
>>> +            reg = <0x0 0x9b080000 0x0 0x10000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        ipa_gsi_mem: ipa-gsi-region@9b090000 {
>>> +            reg = <0x0 0x9b090000 0x0 0xa000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
>>> +            reg = <0x0 0x9b09a000 0x0 0x2000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        spss_region_mem: spss-region@9b100000 {
>>> +            reg = <0x0 0x9b100000 0x0 0x180000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        spu_secure_shared_memory_mem:
>>> spu-secure-shared-memory-region@9b280000 {
>>> +            reg = <0x0 0x9b280000 0x0 0x80000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        camera_mem: camera-region@9b300000 {
>>> +            reg = <0x0 0x9b300000 0x0 0x800000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        video_mem: video-region@9bb00000 {
>>> +            reg = <0x0 0x9bb00000 0x0 0x700000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        cvp_mem: cvp-region@9c200000 {
>>> +            reg = <0x0 0x9c200000 0x0 0x700000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        cdsp_mem: cdsp-region@9c900000 {
>>> +            reg = <0x0 0x9c900000 0x0 0x2000000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
>>> +            reg = <0x0 0x9e900000 0x0 0x80000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
>>> +            reg = <0x0 0x9e980000 0x0 0x80000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        adspslpi_mem: adspslpi-region@9ea00000 {
>>> +            reg = <0x0 0x9ea00000 0x0 0x4080000>;
>>> +            no-map;
>>> +        };
>>> +
>>> +        /*
>>> +         * Firmware related regions, bootloader will possible reserve
>>> parts of
>>> +         * region from 0xd8000000..0x100000000.
>>> +         */
>>> +        mpss_dsm_mem: mpss_dsm_region@d4d00000 {
>>> +            reg = <0x0 0xd4d00000 0x0 0x3300000>;
>>> +            no-map;
>>> +        };
>>> +    };
>>> +};
>>
>

On 6/4/2024 7:20 PM, Caleb Connolly wrote:
> 
> 
> On 04/06/2024 12:51, Aiqun Yu (Maria) wrote:
>>
>>
>> On 6/3/2024 5:20 PM, Caleb Connolly wrote:
>>> Hi Tengfei,
>>>
>>> On 29/05/2024 12:09, Tengfei Fan wrote:
>>>> QCS8550 is derived from SM8550. The difference between SM8550 and
>>>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
>>>> in IoT products.
>>>> QCS8550 firmware has different memory map compared to SM8550.
>>>> The memory map will be runtime added through bootloader.
>>>> There are 3 types of reserved memory regions here:
>>>> 1. Firmware related regions which aren't shared with kernel.
>>>>       The device tree source in kernel doesn't need to have node to
>>>> indicate
>>>> the firmware related reserved information. Bootloader converys the
>>>> information by updating devicetree at runtime.
>>>>       This will be described as: UEFI saves the physical address of the
>>>> UEFI System Table to dts file's chosen node. Kernel read this table and
>>>> add reserved memory regions to efi config table. Current reserved
>>>> memory
>>>> region may have reserved region which was not yet used, release note of
>>>> the firmware have such kind of information.
>>>
>>> Are you describing some particular quirk of the platform here, or just
>>> standard UEFI booting?
>>
>> It's standard UEFI booting efi config table.
> 
> Ok, thanks for confirming.
>>>
>>> When booting with UEFI, the memory map is passed via the ESRT, so having
>>> memory that the kernel shouldn't use it pretty simple (and typical).
> 
> woo! \o/
>>
>> yes. It is very simple. And the bootloader firmware config the
>> "reserved" region in the efi config table from the uefi firmware.
>>>> 2. Firmware related memory regions which are shared with Kernel
>>>>       The device tree source in the kernel needs to include nodes that
>>>> indicate fimware-related shared information. A label name is suggested
>>>> because this type of shared information needs to be referenced by
>>>> specific drivers for handling purposes.
>>>
>>> Again, is there something non-standard here? If not I would suggest
>>> dropping these detail comments as they might be misleading.
>>
>> Detailed comments is used to describe current device tree reserved
>> memory regions.
>>
>> Current patch is not creating a new mechanism to have memory map
>> described. But it is the first time qcom device trees use this design,
>> and have a simplified(also more compatible) device tree reserved memory
>> region(memory map). Previously, bootloader(apps bootloader) only pass
>> the whole physical memory base and size, and use reserved memory nodes
>> only in device tree(which is also a standard choose).
>>
>> So that's why it is detailed comments for other qcom platform reference.
> 
> Doesn't the rb3gen2 also use this design?

Checked current qcs6490-rb3gen2.dts still use the device tree to have
all the reserved regions, even have detailed regions like "Firmware
related regions which aren't shared with kernel."

Not sure current qcs6490 firmware efi config table looks like, if it
have all the reserved region marked carefully on efi config table, then
device tree don't need to mention the reserved regions which is not
shared to kernel.

The qcom memory map in device tree discussion was happened after qcs6490
rb3gen2 time frame. efi config table is standard. But we still need to
check what's the final config placed in the table for different
platforms. I will suggest to have current qcs8550 as an example to
config the current memory non-kernel needed to know region inside the
efi config table in bootloader, and have kernel shared reserved region
marked in the device tree.

>>
>>>
>>> Thanks and regards,
>>>> 3. Remoteproc regions.
>>>>       Remoteproc regions will be reserved and then assigned to
>>>> subsystem
>>>> firmware later.
>>>> Here is a reserved memory map for this platform:
>>>> 0x100000000 +-------------------+
>>>>               |                   |
>>>>               | Firmware Related  |
>>>>               |                   |
>>>>    0xd4d00000 +-------------------+
>>>>               |                   |
>>>>               | Kernel Available  |
>>>>               |                   |
>>>>    0xa7000000 +-------------------+
>>>>               |                   |
>>>>               | Remoteproc Region |
>>>>               |                   |
>>>>    0x8a800000 +-------------------+
>>>>               |                   |
>>>>               | Firmware Related  |
>>>>               |                   |
>>>>    0x80000000 +-------------------+
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167
>>>> ++++++++++++++++++++++++++
>>>>    1 file changed, 167 insertions(+)
>>>>    create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>> new file mode 100644
>>>> index 000000000000..685668c6ad14
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>> @@ -0,0 +1,167 @@
>>>> +// SPDX-License-Identifier: BSD-3-Clause
>>>> +/*
>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>>>> rights reserved.
>>>> + */
>>>> +
>>>> +#include "sm8550.dtsi"
>>>> +
>>>> +/delete-node/ &reserved_memory;
>>>> +
>>>> +/ {
>>>> +    reserved_memory: reserved-memory {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +        ranges;
>>>> +
>>>> +
>>>> +        /* These are 3 types of reserved memory regions here:
>>>> +         * 1. Firmware related regions which aren't shared with
>>>> kernel.
>>>> +         *     The device tree source in kernel doesn't need to have
>>>> node to
>>>> +         * indicate the firmware related reserved information.
>>>> Bootloader
>>>> +         * conveys the information by updating devicetree at runtime.
>>>> +         *     This will be described as: UEFI saves the physical
>>>> address of
>>>> +         * the UEFI System Table to dts file's chosen node. Kernel
>>>> read this
>>>> +         * table and add reserved memory regions to efi config table.
>>>> Current
>>>> +         * reserved memory region may have reserved region which was
>>>> not yet
>>>> +         * used, release note of the firmware have such kind of
>>>> information.
>>>> +         * 2. Firmware related memory regions which are shared with
>>>> Kernel
>>>> +         *     The device tree source in the kernel needs to include
>>>> nodes
>>>> +         * that indicate fimware-related shared information. A label
>>>> name
>>>> +         * is suggested because this type of shared information
>>>> needs to
>>>> +         * be referenced by specific drivers for handling purposes.
>>>> +         * 3. Remoteproc regions.
>>>> +         *     Remoteproc regions will be reserved and then
>>>> assigned to
>>>> +         * subsystem firmware later.
>>>> +         * Here is a reserved memory map for this platform:
>>>> +         * 0x100000000 +-------------------+
>>>> +         *             |                   |
>>>> +         *             | Firmware Related  |
>>>> +         *             |                   |
>>>> +         *  0xd4d00000 +-------------------+
>>>> +         *             |                   |
>>>> +         *             | Kernel Available  |
>>>> +         *             |                   |
>>>> +         *  0xa7000000 +-------------------+
>>>> +         *             |                   |
>>>> +         *             | Remoteproc Region |
>>>> +         *             |                   |
>>>> +         *  0x8a800000 +-------------------+
>>>> +         *             |                   |
>>>> +         *             | Firmware Related  |
>>>> +         *             |                   |
>>>> +         *  0x80000000 +-------------------+
>>>> +         */
>>>> +
>>>> +        /*
>>>> +         * Firmware related regions, bootloader will possible reserve
>>>> parts of
>>>> +         * region from 0x80000000..0x8a800000.
>>>> +         */
>>>> +        aop_image_mem: aop-image-region@81c00000 {
>>>> +            reg = <0x0 0x81c00000 0x0 0x60000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>>>> +            compatible = "qcom,cmd-db";
>>>> +            reg = <0x0 0x81c60000 0x0 0x20000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        aop_config_mem: aop-config-region@81c80000 {
>>>> +            no-map;
>>>> +            reg = <0x0 0x81c80000 0x0 0x20000>;
>>>> +        };
>>>> +
>>>> +        smem_mem: smem-region@81d00000 {
>>>> +            compatible = "qcom,smem";
>>>> +            reg = <0x0 0x81d00000 0x0 0x200000>;
>>>> +            hwlocks = <&tcsr_mutex 3>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        adsp_mhi_mem: adsp-mhi-region@81f00000 {
>>>> +            reg = <0x0 0x81f00000 0x0 0x20000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        /* PIL region */
>>>> +        mpss_mem: mpss-region@8a800000 {
>>>> +            reg = <0x0 0x8a800000 0x0 0x10800000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
>>>> +            reg = <0x0 0x9b000000 0x0 0x80000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        ipa_fw_mem: ipa-fw-region@9b080000 {
>>>> +            reg = <0x0 0x9b080000 0x0 0x10000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        ipa_gsi_mem: ipa-gsi-region@9b090000 {
>>>> +            reg = <0x0 0x9b090000 0x0 0xa000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
>>>> +            reg = <0x0 0x9b09a000 0x0 0x2000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        spss_region_mem: spss-region@9b100000 {
>>>> +            reg = <0x0 0x9b100000 0x0 0x180000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        spu_secure_shared_memory_mem:
>>>> spu-secure-shared-memory-region@9b280000 {
>>>> +            reg = <0x0 0x9b280000 0x0 0x80000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        camera_mem: camera-region@9b300000 {
>>>> +            reg = <0x0 0x9b300000 0x0 0x800000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        video_mem: video-region@9bb00000 {
>>>> +            reg = <0x0 0x9bb00000 0x0 0x700000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        cvp_mem: cvp-region@9c200000 {
>>>> +            reg = <0x0 0x9c200000 0x0 0x700000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        cdsp_mem: cdsp-region@9c900000 {
>>>> +            reg = <0x0 0x9c900000 0x0 0x2000000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
>>>> +            reg = <0x0 0x9e900000 0x0 0x80000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
>>>> +            reg = <0x0 0x9e980000 0x0 0x80000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        adspslpi_mem: adspslpi-region@9ea00000 {
>>>> +            reg = <0x0 0x9ea00000 0x0 0x4080000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        /*
>>>> +         * Firmware related regions, bootloader will possible reserve
>>>> parts of
>>>> +         * region from 0xd8000000..0x100000000.
>>>> +         */
>>>> +        mpss_dsm_mem: mpss_dsm_region@d4d00000 {
>>>> +            reg = <0x0 0xd4d00000 0x0 0x3300000>;
>>>> +            no-map;
>>>> +        };
>>>> +    };
>>>> +};
>>>
>>
>

Hi,

On 05/06/2024 06:51, Aiqun Yu (Maria) wrote:
> 
> 
> On 6/4/2024 7:20 PM, Caleb Connolly wrote:
>>
>>
>> On 04/06/2024 12:51, Aiqun Yu (Maria) wrote:
>>>
>>>
>>> On 6/3/2024 5:20 PM, Caleb Connolly wrote:
>>>> Hi Tengfei,
>>>>
>>>> On 29/05/2024 12:09, Tengfei Fan wrote:
>>>>> QCS8550 is derived from SM8550. The difference between SM8550 and
>>>>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly used
>>>>> in IoT products.
>>>>> QCS8550 firmware has different memory map compared to SM8550.
>>>>> The memory map will be runtime added through bootloader.
>>>>> There are 3 types of reserved memory regions here:
>>>>> 1. Firmware related regions which aren't shared with kernel.
>>>>>        The device tree source in kernel doesn't need to have node to
>>>>> indicate
>>>>> the firmware related reserved information. Bootloader converys the
>>>>> information by updating devicetree at runtime.
>>>>>        This will be described as: UEFI saves the physical address of the
>>>>> UEFI System Table to dts file's chosen node. Kernel read this table and
>>>>> add reserved memory regions to efi config table. Current reserved
>>>>> memory
>>>>> region may have reserved region which was not yet used, release note of
>>>>> the firmware have such kind of information.
>>>>
>>>> Are you describing some particular quirk of the platform here, or just
>>>> standard UEFI booting?
>>>
>>> It's standard UEFI booting efi config table.
>>
>> Ok, thanks for confirming.
>>>>
>>>> When booting with UEFI, the memory map is passed via the ESRT, so having
>>>> memory that the kernel shouldn't use it pretty simple (and typical).
>>
>> woo! \o/
>>>
>>> yes. It is very simple. And the bootloader firmware config the
>>> "reserved" region in the efi config table from the uefi firmware.
>>>>> 2. Firmware related memory regions which are shared with Kernel
>>>>>        The device tree source in the kernel needs to include nodes that
>>>>> indicate fimware-related shared information. A label name is suggested
>>>>> because this type of shared information needs to be referenced by
>>>>> specific drivers for handling purposes.
>>>>
>>>> Again, is there something non-standard here? If not I would suggest
>>>> dropping these detail comments as they might be misleading.
>>>
>>> Detailed comments is used to describe current device tree reserved
>>> memory regions.
>>>
>>> Current patch is not creating a new mechanism to have memory map
>>> described. But it is the first time qcom device trees use this design,
>>> and have a simplified(also more compatible) device tree reserved memory
>>> region(memory map). Previously, bootloader(apps bootloader) only pass
>>> the whole physical memory base and size, and use reserved memory nodes
>>> only in device tree(which is also a standard choose).
>>>
>>> So that's why it is detailed comments for other qcom platform reference.
>>
>> Doesn't the rb3gen2 also use this design?
> 
> Checked current qcs6490-rb3gen2.dts still use the device tree to have
> all the reserved regions, even have detailed regions like "Firmware
> related regions which aren't shared with kernel."

Right,
> 
> Not sure current qcs6490 firmware efi config table looks like, if it
> have all the reserved region marked carefully on efi config table, then
> device tree don't need to mention the reserved regions which is not
> shared to kernel.

That makes sense.
> 
> The qcom memory map in device tree discussion was happened after qcs6490
> rb3gen2 time frame. efi config table is standard. But we still need to
> check what's the final config placed in the table for different
> platforms. I will suggest to have current qcs8550 as an example to
> config the current memory non-kernel needed to know region inside the
> efi config table in bootloader, and have kernel shared reserved region
> marked in the device tree.

Ok, thanks for explaining the context here. Using the ESRT for this 
certainly makes more sense to me.

So regarding the comment in the reserved-memory node below, I think this 
could be simplified to just a sentence or two explaining how this 
platform is different. Maybe something like:

/* Unlike previous platforms, QCS8550 boots using EFI and describes most 
reserved regions in the ESRT memory map. As a result, reserved memory 
regions which aren't relevant to the kernel (like the hypervisor region) 
don't need to be described in DT. */

A few more comments in-line.
> 
>>>
>>>>
>>>> Thanks and regards,
>>>>> 3. Remoteproc regions.
>>>>>        Remoteproc regions will be reserved and then assigned to
>>>>> subsystem
>>>>> firmware later.
>>>>> Here is a reserved memory map for this platform:
>>>>> 0x100000000 +-------------------+
>>>>>                |                   |
>>>>>                | Firmware Related  |
>>>>>                |                   |
>>>>>     0xd4d00000 +-------------------+
>>>>>                |                   |
>>>>>                | Kernel Available  |
>>>>>                |                   |
>>>>>     0xa7000000 +-------------------+
>>>>>                |                   |
>>>>>                | Remoteproc Region |
>>>>>                |                   |
>>>>>     0x8a800000 +-------------------+
>>>>>                |                   |
>>>>>                | Firmware Related  |
>>>>>                |                   |
>>>>>     0x80000000 +-------------------+
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>>>> ---
>>>>>     arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167
>>>>> ++++++++++++++++++++++++++
>>>>>     1 file changed, 167 insertions(+)
>>>>>     create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..685668c6ad14
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>> @@ -0,0 +1,167 @@
>>>>> +// SPDX-License-Identifier: BSD-3-Clause
>>>>> +/*
>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>>>>> rights reserved.
>>>>> + */
>>>>> +
>>>>> +#include "sm8550.dtsi"
>>>>> +
>>>>> +/delete-node/ &reserved_memory;
>>>>> +
>>>>> +/ {
>>>>> +    reserved_memory: reserved-memory {
>>>>> +        #address-cells = <2>;
>>>>> +        #size-cells = <2>;
>>>>> +        ranges;
>>>>> +
>>>>> +
>>>>> +        /* These are 3 types of reserved memory regions here:
>>>>> +         * 1. Firmware related regions which aren't shared with
>>>>> kernel.
>>>>> +         *     The device tree source in kernel doesn't need to have
>>>>> node to
>>>>> +         * indicate the firmware related reserved information.
>>>>> Bootloader
>>>>> +         * conveys the information by updating devicetree at runtime.
>>>>> +         *     This will be described as: UEFI saves the physical
>>>>> address of
>>>>> +         * the UEFI System Table to dts file's chosen node. Kernel
>>>>> read this
>>>>> +         * table and add reserved memory regions to efi config table.
>>>>> Current
>>>>> +         * reserved memory region may have reserved region which was
>>>>> not yet
>>>>> +         * used, release note of the firmware have such kind of
>>>>> information.
>>>>> +         * 2. Firmware related memory regions which are shared with
>>>>> Kernel
>>>>> +         *     The device tree source in the kernel needs to include
>>>>> nodes
>>>>> +         * that indicate fimware-related shared information. A label
>>>>> name
>>>>> +         * is suggested because this type of shared information
>>>>> needs to
>>>>> +         * be referenced by specific drivers for handling purposes.
>>>>> +         * 3. Remoteproc regions.
>>>>> +         *     Remoteproc regions will be reserved and then
>>>>> assigned to
>>>>> +         * subsystem firmware later.
>>>>> +         * Here is a reserved memory map for this platform:
>>>>> +         * 0x100000000 +-------------------+
>>>>> +         *             |                   |
>>>>> +         *             | Firmware Related  |
>>>>> +         *             |                   |
>>>>> +         *  0xd4d00000 +-------------------+
>>>>> +         *             |                   |
>>>>> +         *             | Kernel Available  |
>>>>> +         *             |                   |
>>>>> +         *  0xa7000000 +-------------------+
>>>>> +         *             |                   |
>>>>> +         *             | Remoteproc Region |
>>>>> +         *             |                   |
>>>>> +         *  0x8a800000 +-------------------+
>>>>> +         *             |                   |
>>>>> +         *             | Firmware Related  |
>>>>> +         *             |                   |
>>>>> +         *  0x80000000 +-------------------+

I guess this is quite subjective, but this diagram looks "upside down" 
to me. I think it's generally more popular to have the lower addresses 
at the top.

>>>>> +         */
>>>>> +
>>>>> +        /*
>>>>> +         * Firmware related regions, bootloader will possible reserve
>>>>> parts of
>>>>> +         * region from 0x80000000..0x8a800000.

This is just duplicating info from the table, please drop this comment 
(it should be obvious from the above explanation).
>>>>> +         */
>>>>> +        aop_image_mem: aop-image-region@81c00000 {
>>>>> +            reg = <0x0 0x81c00000 0x0 0x60000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>>>>> +            compatible = "qcom,cmd-db";
>>>>> +            reg = <0x0 0x81c60000 0x0 0x20000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        aop_config_mem: aop-config-region@81c80000 {
>>>>> +            no-map;
>>>>> +            reg = <0x0 0x81c80000 0x0 0x20000>;
>>>>> +        };
>>>>> +
>>>>> +        smem_mem: smem-region@81d00000 {
>>>>> +            compatible = "qcom,smem";
>>>>> +            reg = <0x0 0x81d00000 0x0 0x200000>;
>>>>> +            hwlocks = <&tcsr_mutex 3>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        adsp_mhi_mem: adsp-mhi-region@81f00000 {
>>>>> +            reg = <0x0 0x81f00000 0x0 0x20000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        /* PIL region */

Drop this comment
>>>>> +        mpss_mem: mpss-region@8a800000 {
>>>>> +            reg = <0x0 0x8a800000 0x0 0x10800000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
>>>>> +            reg = <0x0 0x9b000000 0x0 0x80000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        ipa_fw_mem: ipa-fw-region@9b080000 {
>>>>> +            reg = <0x0 0x9b080000 0x0 0x10000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        ipa_gsi_mem: ipa-gsi-region@9b090000 {
>>>>> +            reg = <0x0 0x9b090000 0x0 0xa000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
>>>>> +            reg = <0x0 0x9b09a000 0x0 0x2000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        spss_region_mem: spss-region@9b100000 {
>>>>> +            reg = <0x0 0x9b100000 0x0 0x180000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        spu_secure_shared_memory_mem:
>>>>> spu-secure-shared-memory-region@9b280000 {
>>>>> +            reg = <0x0 0x9b280000 0x0 0x80000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        camera_mem: camera-region@9b300000 {
>>>>> +            reg = <0x0 0x9b300000 0x0 0x800000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        video_mem: video-region@9bb00000 {
>>>>> +            reg = <0x0 0x9bb00000 0x0 0x700000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        cvp_mem: cvp-region@9c200000 {
>>>>> +            reg = <0x0 0x9c200000 0x0 0x700000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        cdsp_mem: cdsp-region@9c900000 {
>>>>> +            reg = <0x0 0x9c900000 0x0 0x2000000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
>>>>> +            reg = <0x0 0x9e900000 0x0 0x80000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
>>>>> +            reg = <0x0 0x9e980000 0x0 0x80000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        adspslpi_mem: adspslpi-region@9ea00000 {
>>>>> +            reg = <0x0 0x9ea00000 0x0 0x4080000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +
>>>>> +        /*
>>>>> +         * Firmware related regions, bootloader will possible reserve
>>>>> parts of
>>>>> +         * region from 0xd8000000..0x100000000.
>>>>> +         */

The address specified in this comment (0xd8000000) doesn't match the 
mpss_dsm_mem region OR the diagram above. I would suggest dropping this 
comment too.
>>>>> +        mpss_dsm_mem: mpss_dsm_region@d4d00000 {
>>>>> +            reg = <0x0 0xd4d00000 0x0 0x3300000>;
>>>>> +            no-map;
>>>>> +        };
>>>>> +    };
>>>>> +};
>>>>
>>>
>>
> 

Kind regards,

On 6/5/2024 8:00 PM, Caleb Connolly wrote:
> Hi,
> 
> On 05/06/2024 06:51, Aiqun Yu (Maria) wrote:
>>
>>
>> On 6/4/2024 7:20 PM, Caleb Connolly wrote:
>>>
>>>
>>> On 04/06/2024 12:51, Aiqun Yu (Maria) wrote:
>>>>
>>>>
>>>> On 6/3/2024 5:20 PM, Caleb Connolly wrote:
>>>>> Hi Tengfei,
>>>>>
>>>>> On 29/05/2024 12:09, Tengfei Fan wrote:
>>>>>> QCS8550 is derived from SM8550. The difference between SM8550 and
>>>>>> QCS8550 is QCS8550 doesn't have modem RF system. QCS8550 is mainly
>>>>>> used
>>>>>> in IoT products.
>>>>>> QCS8550 firmware has different memory map compared to SM8550.
>>>>>> The memory map will be runtime added through bootloader.
>>>>>> There are 3 types of reserved memory regions here:
>>>>>> 1. Firmware related regions which aren't shared with kernel.
>>>>>>        The device tree source in kernel doesn't need to have node to
>>>>>> indicate
>>>>>> the firmware related reserved information. Bootloader converys the
>>>>>> information by updating devicetree at runtime.
>>>>>>        This will be described as: UEFI saves the physical address
>>>>>> of the
>>>>>> UEFI System Table to dts file's chosen node. Kernel read this
>>>>>> table and
>>>>>> add reserved memory regions to efi config table. Current reserved
>>>>>> memory
>>>>>> region may have reserved region which was not yet used, release
>>>>>> note of
>>>>>> the firmware have such kind of information.
>>>>>
>>>>> Are you describing some particular quirk of the platform here, or just
>>>>> standard UEFI booting?
>>>>
>>>> It's standard UEFI booting efi config table.
>>>
>>> Ok, thanks for confirming.
>>>>>
>>>>> When booting with UEFI, the memory map is passed via the ESRT, so
>>>>> having
>>>>> memory that the kernel shouldn't use it pretty simple (and typical).
>>>
>>> woo! \o/
>>>>
>>>> yes. It is very simple. And the bootloader firmware config the
>>>> "reserved" region in the efi config table from the uefi firmware.
>>>>>> 2. Firmware related memory regions which are shared with Kernel
>>>>>>        The device tree source in the kernel needs to include nodes
>>>>>> that
>>>>>> indicate fimware-related shared information. A label name is
>>>>>> suggested
>>>>>> because this type of shared information needs to be referenced by
>>>>>> specific drivers for handling purposes.
>>>>>
>>>>> Again, is there something non-standard here? If not I would suggest
>>>>> dropping these detail comments as they might be misleading.
>>>>
>>>> Detailed comments is used to describe current device tree reserved
>>>> memory regions.
>>>>
>>>> Current patch is not creating a new mechanism to have memory map
>>>> described. But it is the first time qcom device trees use this design,
>>>> and have a simplified(also more compatible) device tree reserved memory
>>>> region(memory map). Previously, bootloader(apps bootloader) only pass
>>>> the whole physical memory base and size, and use reserved memory nodes
>>>> only in device tree(which is also a standard choose).
>>>>
>>>> So that's why it is detailed comments for other qcom platform
>>>> reference.
>>>
>>> Doesn't the rb3gen2 also use this design?
>>
>> Checked current qcs6490-rb3gen2.dts still use the device tree to have
>> all the reserved regions, even have detailed regions like "Firmware
>> related regions which aren't shared with kernel."
> 
> Right,
>>
>> Not sure current qcs6490 firmware efi config table looks like, if it
>> have all the reserved region marked carefully on efi config table, then
>> device tree don't need to mention the reserved regions which is not
>> shared to kernel.
> 
> That makes sense.
>>
>> The qcom memory map in device tree discussion was happened after qcs6490
>> rb3gen2 time frame. efi config table is standard. But we still need to
>> check what's the final config placed in the table for different
>> platforms. I will suggest to have current qcs8550 as an example to
>> config the current memory non-kernel needed to know region inside the
>> efi config table in bootloader, and have kernel shared reserved region
>> marked in the device tree.
> 
> Ok, thanks for explaining the context here. Using the ESRT for this
> certainly makes more sense to me.
> 
> So regarding the comment in the reserved-memory node below, I think this
> could be simplified to just a sentence or two explaining how this
> platform is different. Maybe something like:
> 
> /* Unlike previous platforms, QCS8550 boots using EFI and describes most
> reserved regions in the ESRT memory map. As a result, reserved memory
> regions which aren't relevant to the kernel (like the hypervisor region)
> don't need to be described in DT. */

The previous message still accounts per my understanding since it can be
referenced to others who are not familiar with the memory map change or
ESRT memory map solution.

I think we can add your above message into the commit message to have
more information. Appreciate the comments if others have similar doubts
as you have.
> 
> A few more comments in-line.
>>
>>>>
>>>>>
>>>>> Thanks and regards,
>>>>>> 3. Remoteproc regions.
>>>>>>        Remoteproc regions will be reserved and then assigned to
>>>>>> subsystem
>>>>>> firmware later.
>>>>>> Here is a reserved memory map for this platform:
>>>>>> 0x100000000 +-------------------+
>>>>>>                |                   |
>>>>>>                | Firmware Related  |
>>>>>>                |                   |
>>>>>>     0xd4d00000 +-------------------+
>>>>>>                |                   |
>>>>>>                | Kernel Available  |
>>>>>>                |                   |
>>>>>>     0xa7000000 +-------------------+
>>>>>>                |                   |
>>>>>>                | Remoteproc Region |
>>>>>>                |                   |
>>>>>>     0x8a800000 +-------------------+
>>>>>>                |                   |
>>>>>>                | Firmware Related  |
>>>>>>                |                   |
>>>>>>     0x80000000 +-------------------+
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> Signed-off-by: Tengfei Fan <quic_tengfan@quicinc.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/qcom/qcs8550.dtsi | 167
>>>>>> ++++++++++++++++++++++++++
>>>>>>     1 file changed, 167 insertions(+)
>>>>>>     create mode 100644 arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>> b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>> new file mode 100644
>>>>>> index 000000000000..685668c6ad14
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
>>>>>> @@ -0,0 +1,167 @@
>>>>>> +// SPDX-License-Identifier: BSD-3-Clause
>>>>>> +/*
>>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All
>>>>>> rights reserved.
>>>>>> + */
>>>>>> +
>>>>>> +#include "sm8550.dtsi"
>>>>>> +
>>>>>> +/delete-node/ &reserved_memory;
>>>>>> +
>>>>>> +/ {
>>>>>> +    reserved_memory: reserved-memory {
>>>>>> +        #address-cells = <2>;
>>>>>> +        #size-cells = <2>;
>>>>>> +        ranges;
>>>>>> +
>>>>>> +
>>>>>> +        /* These are 3 types of reserved memory regions here:
>>>>>> +         * 1. Firmware related regions which aren't shared with
>>>>>> kernel.
>>>>>> +         *     The device tree source in kernel doesn't need to have
>>>>>> node to
>>>>>> +         * indicate the firmware related reserved information.
>>>>>> Bootloader
>>>>>> +         * conveys the information by updating devicetree at
>>>>>> runtime.
>>>>>> +         *     This will be described as: UEFI saves the physical
>>>>>> address of
>>>>>> +         * the UEFI System Table to dts file's chosen node. Kernel
>>>>>> read this
>>>>>> +         * table and add reserved memory regions to efi config
>>>>>> table.
>>>>>> Current
>>>>>> +         * reserved memory region may have reserved region which was
>>>>>> not yet
>>>>>> +         * used, release note of the firmware have such kind of
>>>>>> information.
>>>>>> +         * 2. Firmware related memory regions which are shared with
>>>>>> Kernel
>>>>>> +         *     The device tree source in the kernel needs to include
>>>>>> nodes
>>>>>> +         * that indicate fimware-related shared information. A label
>>>>>> name
>>>>>> +         * is suggested because this type of shared information
>>>>>> needs to
>>>>>> +         * be referenced by specific drivers for handling purposes.
>>>>>> +         * 3. Remoteproc regions.
>>>>>> +         *     Remoteproc regions will be reserved and then
>>>>>> assigned to
>>>>>> +         * subsystem firmware later.
>>>>>> +         * Here is a reserved memory map for this platform:
>>>>>> +         * 0x100000000 +-------------------+
>>>>>> +         *             |                   |
>>>>>> +         *             | Firmware Related  |
>>>>>> +         *             |                   |
>>>>>> +         *  0xd4d00000 +-------------------+
>>>>>> +         *             |                   |
>>>>>> +         *             | Kernel Available  |
>>>>>> +         *             |                   |
>>>>>> +         *  0xa7000000 +-------------------+
>>>>>> +         *             |                   |
>>>>>> +         *             | Remoteproc Region |
>>>>>> +         *             |                   |
>>>>>> +         *  0x8a800000 +-------------------+
>>>>>> +         *             |                   |
>>>>>> +         *             | Firmware Related  |
>>>>>> +         *             |                   |
>>>>>> +         *  0x80000000 +-------------------+
> 
> I guess this is quite subjective, but this diagram looks "upside down"
> to me. I think it's generally more popular to have the lower addresses
> at the top.

ack.
> 
>>>>>> +         */
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Firmware related regions, bootloader will possible
>>>>>> reserve
>>>>>> parts of
>>>>>> +         * region from 0x80000000..0x8a800000.
> 
> This is just duplicating info from the table, please drop this comment
> (it should be obvious from the above explanation).

ack.
>>>>>> +         */
>>>>>> +        aop_image_mem: aop-image-region@81c00000 {
>>>>>> +            reg = <0x0 0x81c00000 0x0 0x60000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>>>>>> +            compatible = "qcom,cmd-db";
>>>>>> +            reg = <0x0 0x81c60000 0x0 0x20000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        aop_config_mem: aop-config-region@81c80000 {
>>>>>> +            no-map;
>>>>>> +            reg = <0x0 0x81c80000 0x0 0x20000>;
>>>>>> +        };
>>>>>> +
>>>>>> +        smem_mem: smem-region@81d00000 {
>>>>>> +            compatible = "qcom,smem";
>>>>>> +            reg = <0x0 0x81d00000 0x0 0x200000>;
>>>>>> +            hwlocks = <&tcsr_mutex 3>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        adsp_mhi_mem: adsp-mhi-region@81f00000 {
>>>>>> +            reg = <0x0 0x81f00000 0x0 0x20000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        /* PIL region */
> 
> Drop this comment

ack.
>>>>>> +        mpss_mem: mpss-region@8a800000 {
>>>>>> +            reg = <0x0 0x8a800000 0x0 0x10800000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        q6_mpss_dtb_mem: q6-mpss-dtb-region@9b000000 {
>>>>>> +            reg = <0x0 0x9b000000 0x0 0x80000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        ipa_fw_mem: ipa-fw-region@9b080000 {
>>>>>> +            reg = <0x0 0x9b080000 0x0 0x10000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        ipa_gsi_mem: ipa-gsi-region@9b090000 {
>>>>>> +            reg = <0x0 0x9b090000 0x0 0xa000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        gpu_micro_code_mem: gpu-micro-code-region@9b09a000 {
>>>>>> +            reg = <0x0 0x9b09a000 0x0 0x2000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        spss_region_mem: spss-region@9b100000 {
>>>>>> +            reg = <0x0 0x9b100000 0x0 0x180000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        spu_secure_shared_memory_mem:
>>>>>> spu-secure-shared-memory-region@9b280000 {
>>>>>> +            reg = <0x0 0x9b280000 0x0 0x80000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        camera_mem: camera-region@9b300000 {
>>>>>> +            reg = <0x0 0x9b300000 0x0 0x800000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        video_mem: video-region@9bb00000 {
>>>>>> +            reg = <0x0 0x9bb00000 0x0 0x700000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        cvp_mem: cvp-region@9c200000 {
>>>>>> +            reg = <0x0 0x9c200000 0x0 0x700000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        cdsp_mem: cdsp-region@9c900000 {
>>>>>> +            reg = <0x0 0x9c900000 0x0 0x2000000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9e900000 {
>>>>>> +            reg = <0x0 0x9e900000 0x0 0x80000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9e980000 {
>>>>>> +            reg = <0x0 0x9e980000 0x0 0x80000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        adspslpi_mem: adspslpi-region@9ea00000 {
>>>>>> +            reg = <0x0 0x9ea00000 0x0 0x4080000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Firmware related regions, bootloader will possible
>>>>>> reserve
>>>>>> parts of
>>>>>> +         * region from 0xd8000000..0x100000000.
>>>>>> +         */
> 
> The address specified in this comment (0xd8000000) doesn't match the
> mpss_dsm_mem region OR the diagram above. I would suggest dropping this
> comment too.

ack.
>>>>>> +        mpss_dsm_mem: mpss_dsm_region@d4d00000 {
>>>>>> +            reg = <0x0 0xd4d00000 0x0 0x3300000>;
>>>>>> +            no-map;
>>>>>> +        };
>>>>>> +    };
>>>>>> +};
>>>>>
>>>>
>>>
>>
> 
> Kind regards,