Re: [PATCH v4 3/5] memory: tegra186-emc: Support non-bpmp icc scaling

[Jon Hunter <jonathanh@nvidia.com>]

On 11/11/2025 01:39, Aaron Kling wrote:
> On Mon, Nov 10, 2025 at 3:55 PM Aaron Kling <webgeek1234@gmail.com> wrote:
>>
>> On Mon, Nov 10, 2025 at 3:25 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>>
>>> On 27/10/2025 18:55, Aaron Kling via B4 Relay wrote:
>>>> From: Aaron Kling <webgeek1234@gmail.com>
>>>>
>>>> This adds support for dynamic frequency scaling of external memory on
>>>> devices with bpmp firmware that does not support bwmgr.
>>>>
>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
>>>> ---
>>>>    drivers/memory/tegra/tegra186-emc.c | 132 +++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 130 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
>>>> index 9959ad5804b444b269456d1fbae87b4bc111661b..74be09968baa7a0fbdce4359f470ce56b18acb10 100644
>>>> --- a/drivers/memory/tegra/tegra186-emc.c
>>>> +++ b/drivers/memory/tegra/tegra186-emc.c
>>>> @@ -18,6 +18,17 @@ struct tegra186_emc_dvfs {
>>>>        unsigned long rate;
>>>>    };
>>>>
>>>> +enum emc_rate_request_type {
>>>> +     EMC_RATE_DEBUG,
>>>> +     EMC_RATE_ICC,
>>>> +     EMC_RATE_TYPE_MAX,
>>>> +};
>>>> +
>>>> +struct emc_rate_request {
>>>> +     unsigned long min_rate;
>>>> +     unsigned long max_rate;
>>>> +};
>>>> +
>>>>    struct tegra186_emc {
>>>>        struct tegra_bpmp *bpmp;
>>>>        struct device *dev;
>>>> @@ -33,8 +44,90 @@ struct tegra186_emc {
>>>>        } debugfs;
>>>>
>>>>        struct icc_provider provider;
>>>> +
>>>> +     /*
>>>> +      * There are multiple sources in the EMC driver which could request
>>>> +      * a min/max clock rate, these rates are contained in this array.
>>>> +      */
>>>> +     struct emc_rate_request requested_rate[EMC_RATE_TYPE_MAX];
>>>> +
>>>> +     /* protect shared rate-change code path */
>>>> +     struct mutex rate_lock;
>>>>    };
>>>>
>>>> +static void tegra186_emc_rate_requests_init(struct tegra186_emc *emc)
>>>> +{
>>>> +     unsigned int i;
>>>> +
>>>> +     for (i = 0; i < EMC_RATE_TYPE_MAX; i++) {
>>>> +             emc->requested_rate[i].min_rate = 0;
>>>> +             emc->requested_rate[i].max_rate = ULONG_MAX;
>>>> +     }
>>>> +}
>>>> +
>>>> +static int emc_request_rate(struct tegra186_emc *emc,
>>>> +                         unsigned long new_min_rate,
>>>> +                         unsigned long new_max_rate,
>>>> +                         enum emc_rate_request_type type)
>>>> +{
>>>> +     struct emc_rate_request *req = emc->requested_rate;
>>>> +     unsigned long min_rate = 0, max_rate = ULONG_MAX;
>>>> +     unsigned int i;
>>>> +     int err;
>>>> +
>>>> +     /* select minimum and maximum rates among the requested rates */
>>>> +     for (i = 0; i < EMC_RATE_TYPE_MAX; i++, req++) {
>>>> +             if (i == type) {
>>>> +                     min_rate = max(new_min_rate, min_rate);
>>>> +                     max_rate = min(new_max_rate, max_rate);
>>>> +             } else {
>>>> +                     min_rate = max(req->min_rate, min_rate);
>>>> +                     max_rate = min(req->max_rate, max_rate);
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (min_rate > max_rate) {
>>>> +             dev_err_ratelimited(emc->dev, "%s: type %u: out of range: %lu %lu\n",
>>>> +                                 __func__, type, min_rate, max_rate);
>>>> +             return -ERANGE;
>>>> +     }
>>>> +
>>>> +     err = clk_set_rate(emc->clk, min_rate);
>>>> +     if (err)
>>>> +             return err;
>>>> +
>>>> +     emc->requested_rate[type].min_rate = new_min_rate;
>>>> +     emc->requested_rate[type].max_rate = new_max_rate;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int emc_set_min_rate(struct tegra186_emc *emc, unsigned long rate,
>>>> +                         enum emc_rate_request_type type)
>>>> +{
>>>> +     struct emc_rate_request *req = &emc->requested_rate[type];
>>>> +     int ret;
>>>> +
>>>> +     mutex_lock(&emc->rate_lock);
>>>> +     ret = emc_request_rate(emc, rate, req->max_rate, type);
>>>> +     mutex_unlock(&emc->rate_lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int emc_set_max_rate(struct tegra186_emc *emc, unsigned long rate,
>>>> +                         enum emc_rate_request_type type)
>>>> +{
>>>> +     struct emc_rate_request *req = &emc->requested_rate[type];
>>>> +     int ret;
>>>> +
>>>> +     mutex_lock(&emc->rate_lock);
>>>> +     ret = emc_request_rate(emc, req->min_rate, rate, type);
>>>> +     mutex_unlock(&emc->rate_lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>>    /*
>>>>     * debugfs interface
>>>>     *
>>>> @@ -107,7 +200,7 @@ static int tegra186_emc_debug_min_rate_set(void *data, u64 rate)
>>>>        if (!tegra186_emc_validate_rate(emc, rate))
>>>>                return -EINVAL;
>>>>
>>>> -     err = clk_set_min_rate(emc->clk, rate);
>>>> +     err = emc_set_min_rate(emc, rate, EMC_RATE_DEBUG);
>>>>        if (err < 0)
>>>>                return err;
>>>>
>>>> @@ -137,7 +230,7 @@ static int tegra186_emc_debug_max_rate_set(void *data, u64 rate)
>>>>        if (!tegra186_emc_validate_rate(emc, rate))
>>>>                return -EINVAL;
>>>>
>>>> -     err = clk_set_max_rate(emc->clk, rate);
>>>> +     err = emc_set_max_rate(emc, rate, EMC_RATE_DEBUG);
>>>>        if (err < 0)
>>>>                return err;
>>>>
>>>> @@ -217,6 +310,12 @@ static int tegra186_emc_get_emc_dvfs_latency(struct tegra186_emc *emc)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static inline struct tegra186_emc *
>>>> +to_tegra186_emc_provider(struct icc_provider *provider)
>>>> +{
>>>> +     return container_of(provider, struct tegra186_emc, provider);
>>>> +}
>>>> +
>>>>    /*
>>>>     * tegra186_emc_icc_set_bw() - Set BW api for EMC provider
>>>>     * @src: ICC node for External Memory Controller (EMC)
>>>> @@ -227,6 +326,33 @@ static int tegra186_emc_get_emc_dvfs_latency(struct tegra186_emc *emc)
>>>>     */
>>>>    static int tegra186_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst)
>>>>    {
>>>> +     struct tegra186_emc *emc = to_tegra186_emc_provider(dst->provider);
>>>> +     struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);
>>>> +     unsigned long long peak_bw = icc_units_to_bps(dst->peak_bw);
>>>> +     unsigned long long avg_bw = icc_units_to_bps(dst->avg_bw);
>>>> +     unsigned long long rate = max(avg_bw, peak_bw);
>>>> +     const unsigned int ddr = 2;
>>>> +     int err;
>>>> +
>>>> +     /*
>>>> +      * Do nothing here if bwmgr is supported in BPMP-FW. BPMP-FW sets the final
>>>> +      * Freq based on the passed values.
>>>> +      */
>>>> +     if (mc->bwmgr_mrq_supported)
>>>> +             return 0;
>>>> +
>>>> +     /*
>>>> +      * Tegra186 EMC runs on a clock rate of SDRAM bus. This means that
>>>> +      * EMC clock rate is twice smaller than the peak data rate because
>>>> +      * data is sampled on both EMC clock edges.
>>>> +      */
>>>> +     do_div(rate, ddr);
>>>> +     rate = min_t(u64, rate, U32_MAX);
>>>> +
>>>> +     err = emc_set_min_rate(emc, rate, EMC_RATE_ICC);
>>>> +     if (err)
>>>> +             return err;
>>>> +
>>>>        return 0;
>>>>    }
>>>>
>>>> @@ -329,6 +455,8 @@ static int tegra186_emc_probe(struct platform_device *pdev)
>>>>        platform_set_drvdata(pdev, emc);
>>>>        emc->dev = &pdev->dev;
>>>>
>>>> +     tegra186_emc_rate_requests_init(emc);
>>>> +
>>>>        if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_EMC_DVFS_LATENCY)) {
>>>>                err = tegra186_emc_get_emc_dvfs_latency(emc);
>>>>                if (err)
>>>>
>>>
>>>
>>> FYI, this patch is causing a boot regression on Tegra194 devices. I
>>> noticed that tegra194-p2972-0000 and tegra194-p3509-0000+p3668-0000 are
>>> no longer booting and bisect is pointing to this. I will have a closer
>>> look and try to see why this is.
>>
>> Interesting. Both were booting for me during my verification, though
>> my use case involves the dt changes that I don't believe have been
>> picked up yet. Thought I had explicitly verified without the dt
>> changes too, though. Since I was asked to do so on this or one of the
>> other similar series. I will try to check linux-next as-is soon.
> 
> I just built next-20251110 using the standard arm64 defconfig and
> flashed the resulting Image and dtb's to p2972 and p3518 (p3509+p3668)
> and both booted to cli on a barebones busybox ramdisk. I do not see
> any errors from tegra-mc, and the only error I see from tegra186-emc
> is that it can't find the opp tables, which is expected without the dt
> changes, and is not fatal.

Thanks for testing. Something is not right because our boards are 
failing. So may be we are doing/testing something different. However, 
this should not break. So there is a problem here.

Jon

-- 
nvpublic