Received-SPF: pass (google.com: domain of linux-kernel+bounces-77975-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <b09c737d-9ee4-4b6e-a895-684d82559691@quicinc.com>
Date: Fri, 23 Feb 2024 17:10:05 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 4/6] arm64: dts: qcom: sm8650p: introduce sm8650p dtsi
Content-Language: en-US
To: <neil.armstrong@linaro.org>, Jingyi Wang <quic_jingyw@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>
CC: <kernel@quicinc.com>
References: <20240205115721.1195336-1-quic_jingyw@quicinc.com>
 <20240205115721.1195336-5-quic_jingyw@quicinc.com>
 <665f37e9-eda4-4d82-8d94-a1238514dbff@linaro.org>
From: "Aiqun Yu (Maria)" <quic_aiquny@quicinc.com>
In-Reply-To: <665f37e9-eda4-4d82-8d94-a1238514dbff@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 2/22/2024 9:21 PM, neil.armstrong@linaro.org wrote:
> On 05/02/2024 12:57, Jingyi Wang wrote:
>> Introduce sm8650p dtsi, sm8650p has same base functions
>> as sm8650 with different memory regions.
>>
>> There are 3 types of reserved memory regions here:
>> 1. Firmware related regions.
>>      This will be described as: reserved-region@address. Current
>> reserved-region may have reserved area which was not yet used, release
>> note of the firmware can have such kind of information.
>> 2. Firmware related which shared with kernel access.
>>      Each region will have a specific node with specific label name for
>> later phandle reference from other driver dt node. May overlapping with
>> above type regions.
>> 3. PIL regions.
>>      PIL regions are allocated by kernel and assigned to subsystem
>> firmware later.
>> Here is a map for this platform:
>> 0x100000000 +------------------+
>>              |                  |
>>              | Firmware Related |
>>              |                  |
>>   0xd8000000 +------------------+
>>              |                  |
>>              | Kernel Available |
>>              |                  |
>>   0xA7000000 +------------------+
>>              |                  |
>>              |    PIL Region    |
>>              |                  |
>>   0x8BC00000 +------------------+
>>              |                  |
>>              | Firmware Related |
>>              |                  |
>>   0x80000000 +------------------+
>> Note that:
>> 1. 0xA7000000 to 0xA8000000 was used by bootloader as well, not suggest
>> for other usage.
>> 2. Kernel start address was start at 0xA8000000.
>>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8650p.dtsi | 180 ++++++++++++++++++++++++++
>>   1 file changed, 180 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/sm8650p.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650p.dtsi 
>> b/arch/arm64/boot/dts/qcom/sm8650p.dtsi
>> new file mode 100644
>> index 000000000000..26dfe315b49d
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sm8650p.dtsi
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include "sm8650.dtsi"
>> +
>> +/delete-node/ &reserved_memory;
>> +
>> +/ {
>> +    reserved_memory: reserved-memory {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        ranges;
>> +
>> +        /*
>> +         * There are 3 types of reserved memory regions here:
>> +         * 1. Firmware related regions.
>> +         *     This will be described as: reserved-region@address. 
>> Current
>> +         * reserved-region may have reserved area which was not yet 
>> used,
>> +         * release note of the firmware can have such kind of 
>> information.
>> +         * 2. Firmware related which shared with kernel access.
>> +         *     Each region will have a specific node with specific label
>> +         * name for later phandle reference from other driver dt 
>> node. May
>> +         * overlapping with above type regions.
>> +         * 3. PIL regions.
>> +         *     PIL regions are allocated by kernel and assigned to 
>> subsystem
>> +         * firmware later.
>> +         * Here is a map for this platform:
>> +         * 0x100000000 +------------------+
>> +         *             |                  |
>> +         *             | Firmware Related |
>> +         *             |                  |
>> +         *  0xd8000000 +------------------+
>> +         *             |                  |
>> +         *             | Kernel Available |
>> +         *             |                  |
>> +         *  0xA7000000 +------------------+
>> +         *             |                  |
>> +         *             |    PIL Region    |
>> +         *             |                  |
>> +         *  0x8BC00000 +------------------+
>> +         *             |                  |
>> +         *             | Firmware Related |
>> +         *             |                  |
>> +         *  0x80000000 +------------------+
>> +         * Note that:
>> +         * 1. 0xA7000000 to 0xA8000000 was used by bootloader as 
>> well, not
>> +         * suggest for other usage.
>> +         * 2. Kernel start address was start at 0xA8000000.
>> +         */
>> +
>> +        /* Firmware related regions */
>> +        reserved-region@80000000 {
>> +            reg = <0x0 0x80000000 0x0 0xbc00000>;
>> +            no-map;
>> +        };
> 
> Ok this region goes up to 0x8BC00000 and so overlaps with the next regions:
The idea here is to reserve more needed ddr regions for different 
version of firmware compatibility. While inside this region which shared 
device memory from firmware to kernel, it is still needed to have node 
information in the device tree.

More clear reference here for the firmware needed no-map reserved region 
diagram, take the smem_mem here to be exposed shared read to kernel:
*  0x8BC00000 +------------------+
*             |                  |
*             | reserved_region2 |
*  0x81c60000 +------------------+
*             |    smem_mem      |
*  0x81c00000 +------------------+
*             |  reserved_region1|
*  0x80000000 +------------------+

what's the suggestion here for this requirement?:
option 1: have a big region_reserved, and then have smem_mem overlap 
reserved region node information inside the dt.
option 2: Have each separate "reserved_region1 node + smem_mem node + 
reserved_region2 node".
other options?

> 
>> +
>> +        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;
>> +        };
>> +
>> +        global_sync_mem: global-sync@82600000 {
>> +            reg = <0 0x82600000 0 0x100000>;
>> +            no-map;
>> +        };
>> +
>> +        mpss_dsm_mem: mpss-dsm@86b00000 {
>> +            reg = <0 0x86b00000 0 0x4900000>;
>> +            no-map;
>> +        };
>> +
>> +        mpss_dsm_mem_2: mpss-dsm-2@8b400000 {
>> +            reg = <0 0x8b400000 0 0x800000>;
>> +            no-map;
>> +        };
> 
> up to here
> 
> Please fix this,
> 
> I just checked against plain sm8650.dtsi and actually the memory 
> adresses are the same.
> 
> So what's the _real_ difference here ? Just drop the superfluous memory 
> zones and redefine them if needed.
With big reserved regions agreed, I think the memory map can be modified 
directly in sm8650.dtsi. It will be a memory map support different 
derived soc firmware release as well.
> 
> Thanks,
> Neil
> 
>> +
>> +        /* PIL region */
>> +        mpss_mem: mpss-region@8bc00000 {
>> +            reg = <0x0 0x8bc00000 0x0 0xf400000>;
>> +            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@9b0a0000 {
>> +            reg = <0x0 0x9b0a0000 0x0 0x1e0000>;
>> +            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 0x800000>;
>> +            no-map;
>> +        };
>> +
>> +        cvp_mem: cvp-region@9c300000 {
>> +            reg = <0x0 0x9c300000 0x0 0x700000>;
>> +            no-map;
>> +        };
>> +
>> +        cdsp_mem: cdsp-region@9ca00000 {
>> +            reg = <0x0 0x9ca00000 0x0 0x1400000>;
>> +            no-map;
>> +        };
>> +
>> +        q6_cdsp_dtb_mem: q6-cdsp-dtb-region@9de00000 {
>> +            reg = <0x0 0x9de00000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        q6_adsp_dtb_mem: q6-adsp-dtb-region@9de80000 {
>> +            reg = <0x0 0x9de80000 0x0 0x80000>;
>> +            no-map;
>> +        };
>> +
>> +        adspslpi_mem: adspslpi-region@9df00000 {
>> +            reg = <0x0 0x9df00000 0x0 0x4080000>;
>> +            no-map;
>> +        };
>> +
>> +        /* Firmware related regions */
>> +        reserved-region@d8000000 {
>> +            reg = <0x0 0xd8000000 0x0 0x28000000>;
>> +            no-map;
>> +        };
>> +
>> +    };
>> +};
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu