Re: [PATCH 4/5] firmware: tegra: bpmp: Add per-SoC EMC clock ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] firmware: tegra: bpmp: missing emc_clk_id for Tegra186 and Tegra194
--

commit 8799d4cd04239b801d1fa35e2045aaeae3d68e3e
Author: Johnny Liu <johnliu@nvidia.com>

firmware: tegra: bpmp: Add per-SoC EMC clock ID

This commit adds an emc_clk_id field to struct tegra_bpmp_soc and sets it
for Tegra234, Tegra238, and Tegra264 while leaving it unset for Tegra186
and Tegra194.

>     firmware: tegra: bpmp: Add per-SoC EMC clock ID
>     
>     The EMC clock ID differs per chip (TEGRA234_CLK_EMC=31,
>     TEGRA238_CLK_EMC=43, TEGRA264_CLK_EMC=290).
>     
>     Add an emc_clk_id field to struct tegra_bpmp_soc and set it for
>     Tegra234, Tegra238 and Tegra264. Tegra186 and Tegra194 have no EMC clock
>     ID and leave it unset.
>     
>     Signed-off-by: Johnny Liu <johnliu@nvidia.com>
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index beeb385870f59..06da888535dc0 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c

[ ... ]

> @@ -1158,14 +1162,17 @@ static const struct tegra_bpmp_soc tegra186_soc = {
>  
>  static const struct tegra_bpmp_soc tegra234_soc = {
>  	TEGRA186_BPMP_SOC_COMMON,
> +	.emc_clk_id = TEGRA234_CLK_EMC,
>  };

[Severity: Medium]
Is it accurate that Tegra186 and Tegra194 have no EMC clock ID, and should
tegra186_soc (or a dedicated Tegra194 entry) also populate emc_clk_id?

The in-tree device tree bindings and device trees define and consume BPMP
EMC clocks on both SoCs:

include/dt-bindings/clock/tegra186-clock.h:
    #define TEGRA186_CLK_EMC 58

arch/arm64/boot/dts/nvidia/tegra186.dtsi:
    emc: external-memory-controller@2c60000 {
        ...
        clocks = <&bpmp TEGRA186_CLK_EMC>;

include/dt-bindings/clock/tegra194-clock.h:
    #define TEGRA194_CLK_EMC			31

arch/arm64/boot/dts/nvidia/tegra194.dtsi:
    emc: external-memory-controller@2c60000 {
        ...
        clocks = <&bpmp TEGRA194_CLK_EMC>;

In addition, drivers/memory/tegra/tegra194.c sets .icc_ops = &tegra_mc_icc_ops,
which scales the EMC clock via BPMP MRQ_BWMGR_INT without clk_set_rate()
involvement, and drivers/memory/tegra/tegra186-emc.c uses
clk_set_rate_range(), clk_set_min_rate(), and clk_set_max_rate() on emc->clk
across Tegra186, Tegra194, Tegra234, and Tegra264.

With emc_clk_id left at 0 in tegra186_soc (which matches TEGRA186_CLK_FUSE
on Tegra186), consumers such as tegra_bpmp_clk_register() in commit
1346a33d1522 ("clk: tegra: bpmp: Do not cache the EMC clock rate") will not
identify the EMC clock on Tegra186 and Tegra194, continuing to return stale
cached EMC clock rates when BPMP scales the EMC frequency. Also, because
arch/arm64/boot/dts/nvidia/tegra194.dtsi matches compatible = "nvidia,tegra186-bpmp"
and tegra_bpmp_match has no separate entry for Tegra194, both SoCs currently
share tegra186_soc despite having different EMC clock IDs (58 vs. 31).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028084900.1047287-1-johnliu@nvidia.com?part=4