Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/3] remoteproc: k3-r5: Adjust TCM sizes in Split-mode on
 J7200 SoCs
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
CC:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Lokesh Vutla <lokeshvutla@ti.com>,
        linux-remoteproc <linux-remoteproc@vger.kernel.org>,
        <devicetree@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20201119010531.21083-1-s-anna@ti.com>
 <20201119010531.21083-4-s-anna@ti.com> <20201123235129.GA529235@xps15>
 <CANLsYkwztm1hwXMGO-7025YdaEk=ttLMVmM2A8hRYyyrWpR7fQ@mail.gmail.com>
From:   Suman Anna <s-anna@ti.com>
Message-ID: <823f82d5-f100-23ed-8f29-73ec8b19ffd7@ti.com>
Date:   Mon, 30 Nov 2020 09:47:46 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CANLsYkwztm1hwXMGO-7025YdaEk=ttLMVmM2A8hRYyyrWpR7fQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/23/20 6:55 PM, Mathieu Poirier wrote:
> On Mon, 23 Nov 2020 at 16:51, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>>
>> Good afternoon Suman,
>>
>> On Wed, Nov 18, 2020 at 07:05:31PM -0600, Suman Anna wrote:
>>> The J7200 SoCs have a revised R5FSS IP that adds a unique feature w.r.t
>>> TCM sizing. Each R5F core in a cluster typically has 32 KB each of ATCM
>>> and BTCM, with only the Core0 TCMs usable in LockStep mode. This revised
>>> IP however doubles the total available TCM in LockStep mode by making the
>>> Core1 TCM visible immediately after the corresponding Core0 TCM.
>>>
>>> The R5F DT nodes on the J7200 SoCs define double (64 KB) the normal TCM
>>> size (32 KB) for R5F Core0 for each of ATCM and BTCM to represent the
>>> above. This increased TCM memory is only usable in LockStep-mode, and
>>> has to be adjusted to the normal 32 KB size in Split mode. Enhance the
>>> TI K3 R5F remoteproc for this logic through a new function. The adjustment
>>> is a no-op on prior SoCs and relies on the correct DTS node sizes in
>>> LockStep-mode on applicable SoCs.
>>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> ---
>>>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 43 ++++++++++++++++++++++++
>>>  1 file changed, 43 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> index 66a32dcdd7d0..62b5a4c29456 100644
>>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> @@ -71,9 +71,11 @@ enum cluster_mode {
>>>
>>>  /**
>>>   * struct k3_r5_soc_data - match data to handle SoC variations
>>> + * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
>>>   * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
>>>   */
>>>  struct k3_r5_soc_data {
>>> +     bool tcm_is_double;
>>>       bool tcm_ecc_autoinit;
>>>  };
>>>
>>> @@ -886,6 +888,43 @@ static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
>>>       of_reserved_mem_device_release(kproc->dev);
>>>  }
>>>
>>> +/*
>>> + * Each R5F core within a typical R5FSS instance has a total of 64 KB of TCMs,
>>> + * split equally into two 32 KB banks between ATCM and BTCM. The TCMs from both
>>> + * cores are usable in Split-mode, but only the Core0 TCMs can be used in
>>> + * LockStep-mode. The newer revisions of the R5FSS IP maximizes these TCMs by
>>> + * leveraging the Core1 TCMs as well in certain modes where they would have
>>> + * otherwise been unusable (Eg: LockStep-mode on J7200 SoCs). This is done by
>>> + * making a Core1 TCM visible immediately after the corresponding Core0 TCM.
>>> + * The SoC memory map uses the larger 64 KB sizes for the Core0 TCMs, and the
>>> + * dts representation reflects this increased size on supported SoCs. The Core0
>>> + * TCM sizes therefore have to be adjusted to only half the original size in
>>> + * Split mode.
>>> + */
>>> +static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
>>> +{
>>> +     struct k3_r5_cluster *cluster = kproc->cluster;
>>> +     struct k3_r5_core *core = kproc->core;
>>> +     struct device *cdev = core->dev;
>>> +     struct k3_r5_core *core0;
>>> +
>>> +     if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
>>> +         !cluster->soc_data->tcm_is_double)
>>> +             return;
>>
>> Shouldn't this be:
>>
>>         if (cluster->mode == CLUSTER_MODE_SPLIT ||
>>             !cluster->soc_data->tcm_is_double)
>>                 return;
>>
>> If am wrong then I'm pretty sure other people will be confused and a comment is
>> warranted.
>>
> 
> Forget the above, I misread the context.  The memories are already set
> to 64KB so there is nothing to do if in lockstep mode.

Yep, thanks for the review. The comments above the function has all the details.

regards
Suman

> 
>>> +
>>> +     core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
>>> +     if (core == core0) {
>>> +             WARN_ON(core->mem[0].size != SZ_64K);
>>> +             WARN_ON(core->mem[1].size != SZ_64K);
>>> +
>>> +             core->mem[0].size /= 2;
>>> +             core->mem[1].size /= 2;
>>> +
>>> +             dev_dbg(cdev, "adjusted TCM sizes, ATCM = 0x%zx BTCM = 0x%zx\n",
>>> +                     core->mem[0].size, core->mem[1].size);
>>> +     }
>>> +}
>>> +
>>>  static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>>>  {
>>>       struct k3_r5_cluster *cluster = platform_get_drvdata(pdev);
>>> @@ -933,6 +972,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>>>                       goto err_config;
>>>               }
>>>
>>> +             k3_r5_adjust_tcm_sizes(kproc);
>>> +
>>>               ret = k3_r5_reserved_mem_init(kproc);
>>>               if (ret) {
>>>                       dev_err(dev, "reserved memory init failed, ret = %d\n",
>>> @@ -1407,10 +1448,12 @@ static int k3_r5_probe(struct platform_device *pdev)
>>>  }
>>>
>>>  static const struct k3_r5_soc_data am65_j721e_soc_data = {
>>> +     .tcm_is_double = false,
>>>       .tcm_ecc_autoinit = false,
>>>  };
>>>
>>>  static const struct k3_r5_soc_data j7200_soc_data = {
>>> +     .tcm_is_double = true,
>>>       .tcm_ecc_autoinit = true,
>>
>> With the above and for the set:
>>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>>>  };
>>>
>>> --
>>> 2.28.0
>>>