Re: [PATCH] memory: tegra264: Add full set of MC clients

[Sumit Gupta <sumitg@nvidia.com>]

On 15/05/26 23:16, sashiko-bot@kernel.org wrote:
> External email: Use caution opening links or attachments
>
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Newly added BPMP-managed memory clients are missing their `.bpmp_id` and `.type` initializations.

Will add a separate patch in v2 with a check in tegra264_mc_icc_set()
to not forward ICC requests for clients without .bpmp_id/.type.
Some clients are added only for names in fault-log.

> - [Medium] The `tegra264_mc_clients` array is not strictly sorted by client ID, contradicting the commit message.

The array follows MC override/security register offsets and not
MC client_ID order. Will correct the commit message in v2.

Thank you,
Sumit Gupta


> -- commit fefb4bd46910389b4d7cd1d39ffbe180883f7665 Author: Sumit Gupta 
> <sumitg@nvidia.com> memory: tegra264: Add full set of MC clients This 
> commit extends the Tegra264 MC dt-bindings header and 
> tegra264_mc_clients table to cover the full set of memory clients 
> exposed by the SoC.
>> Extend the Tegra264 MC dt-bindings header and tegra264_mc_clients
>> table to cover the full set of memory clients exposed by the SoC.
>> Client name is used for MC fault reporting. Clients managed by the
>> bandwidth manager in BPMP additionally carry their bpmp_id and type.
> The commit message states that clients managed by the bandwidth manager in
> BPMP carry their bpmp_id and type. Should these fields be initialized for the
> newly added clients like NVENC, NVJPG, ISP, PVA, XUSB, and TSEC?
>
>> diff --git a/drivers/memory/tegra/tegra264.c b/drivers/memory/tegra/tegra264.c
>> --- a/drivers/memory/tegra/tegra264.c
>> +++ b/drivers/memory/tegra/tegra264.c
> [ ... ]
>> @@ -110,6 +208,12 @@ static const struct tegra_mc_client tegra264_mc_clients[] = {
>>                .name = "rcew",
>>                .bpmp_id = TEGRA264_BWMGR_RCE,
>>                .type = TEGRA_ICC_NISO,
>> +     }, {
>> +             .id = TEGRA264_MEMORY_CLIENT_NVENC1SRD2MC,
>> +             .name = "nvenc1srd2mc",
>> +     }, {
>> +             .id = TEGRA264_MEMORY_CLIENT_NVENC1SWR2MC,
>> +             .name = "nvenc1swr2mc",
>>        }, {
>>                .id = TEGRA264_MEMORY_CLIENT_PCIE0W,
>>                .name = "pcie0w",
> If the bpmp_id and type fields are omitted here, they default to 0. Would
> this cause drivers for these components to inadvertently pass 0 as the client
> ID to the BPMP firmware when attempting to configure interconnect bandwidth?
>
>> Entries in tegra264_mc_clients[] are sorted in increasing order of
>> their client IDs, which matches the order of the override and
>> security register offsets used in previous SoCs.
> This isn't a functional bug, but are these entries strictly sorted by their
> client IDs as the commit message indicates?
>
> [ ... ]
>> @@ -185,6 +289,402 @@ static const struct tegra_mc_client tegra264_mc_clients[] = {
>>                .name = "nvdecswr2mc",
>>                .bpmp_id = TEGRA264_BWMGR_NVDEC,
>>                .type = TEGRA_ICC_NISO,
>> +     }, {
>> +             .id = TEGRA264_MEMORY_CLIENT_MIU5R,
>> +             .name = "miu5r",
>> +     }, {
> Looking at the tegra264_mc_clients array, GPUR02MC (0xb6), GPUW02MC (0xb7),
> NVDECSRD2MC (0x78), and NVDECSWR2MC (0x79) appear immediately after
> PCIE5W (0xe3), and the newly added MIU5R (0xfc) is appended after them.
>
> Could this mismatch between the array's actual order and the commit
> message's claim cause confusion during future maintenance?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260515171911.1929868-1-sumitg@nvidia.com?part=1