| Message ID | 20221220160240.27494-2-sumitg@nvidia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Tegra234 Memory interconnect support | expand |
20.12.2022 19:02, Sumit Gupta пишет: > @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = { > */ > static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > { > + struct tegra_icc_node *tnode; > struct icc_node *node; > unsigned int i; > int err; > @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > mc->provider.data = &mc->provider; > mc->provider.set = mc->soc->icc_ops->set; > mc->provider.aggregate = mc->soc->icc_ops->aggregate; > - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > + mc->provider.get_bw = mc->soc->icc_ops->get_bw; > + if (mc->soc->icc_ops->xlate) > + mc->provider.xlate = mc->soc->icc_ops->xlate; > + if (mc->soc->icc_ops->xlate_extended) > + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; These IFs look pointless
20.12.2022 19:02, Sumit Gupta пишет: > + if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) { > + err = tegra_emc_interconnect_init(emc); > + if (!err) > + return err; "return err" doesn't sound good > + dev_err(&pdev->dev, "tegra_emc_interconnect_init failed:%d\n", err); > + goto put_bpmp; Move error message to tegra_emc_interconnect_init()
20.12.2022 19:02, Sumit Gupta пишет: > +#ifndef MEMORY_TEGRA_ICC_H > +#define MEMORY_TEGRA_ICC_H > + > +enum tegra_icc_client_type { > + TEGRA_ICC_NONE, > + TEGRA_ICC_NISO, > + TEGRA_ICC_ISO_DISPLAY, > + TEGRA_ICC_ISO_VI, > + TEGRA_ICC_ISO_AUDIO, > + TEGRA_ICC_ISO_VIFAL, > +}; You using only TEGRA_ICC_NISO and !TEGRA_ICC_NISO in the code. include/soc/tegra/mc.h defines TAG_DEFAULT/ISO, please drop all these duplicated and unused "types" unless there is a good reason to keep them
20.12.2022 19:02, Sumit Gupta пишет: > +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak) > +{ > + *avg = 0; > + *peak = 0; > + > + return 0; > +} Looks wrong, you should add ICC support to all the drivers first and only then enable ICC. I think you added this init_bw() to work around the fact that ICC isn't supported by T234 drivers.
20.12.2022 19:02, Sumit Gupta пишет: > +static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + struct tegra_mc *mc = icc_provider_to_tegra_mc(dst->provider); > + struct tegra_icc_node *tnode = src->data; > + > + /* > + * Same Src and Dst node will happen during boot from icc_node_add(). > + * This can be used to pre-initialize and set bandwidth for all clients > + * before their drivers are loaded. We are skipping this case as for us, > + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. > + */ > + if (src->id == dst->id) > + return 0; > + > + if (tnode->node) > + mc->curr_tnode = tnode; > + else > + pr_err("%s, tegra_icc_node is null\n", __func__); The tnode->node can't be NULL.
On 20/12/22 23:35, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = { >> */ >> static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> { >> + struct tegra_icc_node *tnode; >> struct icc_node *node; >> unsigned int i; >> int err; >> @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> mc->provider.data = &mc->provider; >> mc->provider.set = mc->soc->icc_ops->set; >> mc->provider.aggregate = mc->soc->icc_ops->aggregate; >> - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; >> + mc->provider.get_bw = mc->soc->icc_ops->get_bw; >> + if (mc->soc->icc_ops->xlate) >> + mc->provider.xlate = mc->soc->icc_ops->xlate; >> + if (mc->soc->icc_ops->xlate_extended) >> + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > > These IFs look pointless Ok. Will remove.
On 20/12/22 23:36, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> + if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) { >> + err = tegra_emc_interconnect_init(emc); >> + if (!err) >> + return err; > > "return err" doesn't sound good > >> + dev_err(&pdev->dev, "tegra_emc_interconnect_init failed:%d\n", err); >> + goto put_bpmp; > > Move error message to tegra_emc_interconnect_init() Ok, will change.
On 20/12/22 23:37, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> +#ifndef MEMORY_TEGRA_ICC_H >> +#define MEMORY_TEGRA_ICC_H >> + >> +enum tegra_icc_client_type { >> + TEGRA_ICC_NONE, >> + TEGRA_ICC_NISO, >> + TEGRA_ICC_ISO_DISPLAY, >> + TEGRA_ICC_ISO_VI, >> + TEGRA_ICC_ISO_AUDIO, >> + TEGRA_ICC_ISO_VIFAL, >> +}; > > You using only TEGRA_ICC_NISO and !TEGRA_ICC_NISO in the code. > > include/soc/tegra/mc.h defines TAG_DEFAULT/ISO, please drop all these > duplicated and unused "types" unless there is a good reason to keep them > These type are used while defining clients in "tegra234_mc_clients[]" and its passed to BPMP-FW which has handling for each client type. kernel$ grep -B2 ".type = TEGRA_ICC_ISO" drivers/memory/tegra/tegra234.c .name = "hdar", .bpmp_id = TEGRA_ICC_BPMP_HDA, .type = TEGRA_ICC_ISO_AUDIO, -- .name = "hdaw", .bpmp_id = TEGRA_ICC_BPMP_HDA, .type = TEGRA_ICC_ISO_AUDIO, -- .name = "vi2w", .bpmp_id = TEGRA_ICC_BPMP_VI2, .type = TEGRA_ICC_ISO_VI, -- .name = "vi2falr", .bpmp_id = TEGRA_ICC_BPMP_VI2FAL, .type = TEGRA_ICC_ISO_VIFAL, -- .name = "vi2falw", .bpmp_id = TEGRA_ICC_BPMP_VI2FAL, .type = TEGRA_ICC_ISO_VIFAL, -- .name = "aper", .bpmp_id = TEGRA_ICC_BPMP_APE, .type = TEGRA_ICC_ISO_AUDIO, -- .name = "apew", .bpmp_id = TEGRA_ICC_BPMP_APE, .type = TEGRA_ICC_ISO_AUDIO, -- .name = "nvdisplayr", .bpmp_id = TEGRA_ICC_BPMP_DISPLAY, .type = TEGRA_ICC_ISO_DISPLAY, -- .name = "nvdisplayr1", .bpmp_id = TEGRA_ICC_BPMP_DISPLAY, .type = TEGRA_ICC_ISO_DISPLAY, -- .name = "apedmar", .bpmp_id = TEGRA_ICC_BPMP_APEDMA, .type = TEGRA_ICC_ISO_AUDIO, -- .name = "apedmaw", .bpmp_id = TEGRA_ICC_BPMP_APEDMA, .type = TEGRA_ICC_ISO_AUDIO,
On 21/12/22 06:25, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> +static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst) >> +{ >> + struct tegra_mc *mc = icc_provider_to_tegra_mc(dst->provider); >> + struct tegra_icc_node *tnode = src->data; >> + >> + /* >> + * Same Src and Dst node will happen during boot from icc_node_add(). >> + * This can be used to pre-initialize and set bandwidth for all clients >> + * before their drivers are loaded. We are skipping this case as for us, >> + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. >> + */ >> + if (src->id == dst->id) >> + return 0; >> + >> + if (tnode->node) >> + mc->curr_tnode = tnode; >> + else >> + pr_err("%s, tegra_icc_node is null\n", __func__); > > The tnode->node can't be NULL. Ok, will remove the check.
On 20/12/22 23:40, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak) >> +{ >> + *avg = 0; >> + *peak = 0; >> + >> + return 0; >> +} > > Looks wrong, you should add ICC support to all the drivers first and > only then enable ICC. I think you added this init_bw() to work around > the fact that ICC isn't supported by T234 drivers. If get_bw hook is not added then max freq is set due to 'INT_MAX' below. void icc_node_add(struct icc_node *node, struct icc_provider *provider) { .... /* get the initial bandwidth values and sync them with hardware */ if (provider->get_bw) { provider->get_bw(node, &node->init_avg, &node->init_peak); } else { node->init_avg = INT_MAX; node->init_peak = INT_MAX; } So, will have to add the empty functions at least. static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak) { - *avg = 0; - *peak = 0; - return 0; } Support to all the client drivers can't be added at once as there are many drivers all with different requirements and handling. This patch series is the beginning to add the basic interconnect support in new Tegra SoC's. Support for more clients will be added later one by one or in batch.
21.12.2022 12:35, Sumit Gupta пишет: > > > On 20/12/22 23:40, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 20.12.2022 19:02, Sumit Gupta пишет: >>> +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 >>> *avg, u32 *peak) >>> +{ >>> + *avg = 0; >>> + *peak = 0; >>> + >>> + return 0; >>> +} >> >> Looks wrong, you should add ICC support to all the drivers first and >> only then enable ICC. I think you added this init_bw() to work around >> the fact that ICC isn't supported by T234 drivers. > > If get_bw hook is not added then max freq is set due to 'INT_MAX' below. > > void icc_node_add(struct icc_node *node, struct icc_provider *provider) > { > .... > /* get the initial bandwidth values and sync them with hardware */ > if (provider->get_bw) { > provider->get_bw(node, &node->init_avg, &node->init_peak); > } else { > node->init_avg = INT_MAX; > node->init_peak = INT_MAX; > } > > So, will have to add the empty functions at least. > > static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, > u32 *peak) > { > - *avg = 0; > - *peak = 0; > - > return 0; > } > > Support to all the client drivers can't be added at once as there are > many drivers all with different requirements and handling. This patch > series is the beginning to add the basic interconnect support in new > Tegra SoC's. Support for more clients will be added later one by one or > in batch. This means that bandwidth management isn't working properly. You should leave the freq to INT_MAX and fix the missing integer overflows in the code if any, or read out the BW from FW. Once you'll enable ICC for all drivers, it will start working.
21.12.2022 11:05, Sumit Gupta пишет: > On 20/12/22 23:37, Dmitry Osipenko wrote: >> External email: Use caution opening links or attachments >> >> >> 20.12.2022 19:02, Sumit Gupta пишет: >>> +#ifndef MEMORY_TEGRA_ICC_H >>> +#define MEMORY_TEGRA_ICC_H >>> + >>> +enum tegra_icc_client_type { >>> + TEGRA_ICC_NONE, >>> + TEGRA_ICC_NISO, >>> + TEGRA_ICC_ISO_DISPLAY, >>> + TEGRA_ICC_ISO_VI, >>> + TEGRA_ICC_ISO_AUDIO, >>> + TEGRA_ICC_ISO_VIFAL, >>> +}; >> >> You using only TEGRA_ICC_NISO and !TEGRA_ICC_NISO in the code. >> >> include/soc/tegra/mc.h defines TAG_DEFAULT/ISO, please drop all these >> duplicated and unused "types" unless there is a good reason to keep them >> > > These type are used while defining clients in "tegra234_mc_clients[]" > and its passed to BPMP-FW which has handling for each client type. The type should be based on the ICC tag, IMO. AFAICS, type isn't fixed in FW and you can set both ISO and NISO BW, hence it's up to a device driver to select the appropriate tag.
20.12.2022 19:02, Sumit Gupta пишет: > static int tegra186_emc_probe(struct platform_device *pdev) > { > struct mrq_emc_dvfs_latency_response response; > struct tegra_bpmp_message msg; > struct tegra186_emc *emc; > + struct tegra_mc *mc; > unsigned int i; > int err; > > @@ -158,6 +307,9 @@ static int tegra186_emc_probe(struct platform_device *pdev) > if (!emc) > return -ENOMEM; > > + platform_set_drvdata(pdev, emc); > + emc->dev = &pdev->dev; > + > emc->bpmp = tegra_bpmp_get(&pdev->dev); > if (IS_ERR(emc->bpmp)) > return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n"); > @@ -236,6 +388,19 @@ static int tegra186_emc_probe(struct platform_device *pdev) > debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root, > emc, &tegra186_emc_debug_max_rate_fops); > > + mc = dev_get_drvdata(emc->dev->parent); > + if (mc && mc->soc->icc_ops) { > + if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) { > + err = tegra_emc_interconnect_init(emc); > + if (!err) > + return err; > + dev_err(&pdev->dev, "tegra_emc_interconnect_init failed:%d\n", err); > + goto put_bpmp; > + } else { > + dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n"); > + } If there is no MRQ_BWMGR_INT, then device drivers using ICC won't probe. This is either a error condition, or ICC should inited and then ICC changes should be skipped.
20.12.2022 19:02, Sumit Gupta пишет: > +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) > +{ > + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); > + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); > + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; > + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; > + struct tegra_icc_node *tnode = mc->curr_tnode; > + struct tegra_bpmp_message msg; > + int ret = 0; Nit: unnecessarily initialized var, the same for the rest of the code
20.12.2022 19:02, Sumit Gupta пишет: > +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) > +{ > + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); > + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); > + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; > + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; > + struct tegra_icc_node *tnode = mc->curr_tnode; > + struct tegra_bpmp_message msg; > + int ret = 0; > + > + /* > + * Same Src and Dst node will happen during boot from icc_node_add(). > + * This can be used to pre-initialize and set bandwidth for all clients > + * before their drivers are loaded. We are skipping this case as for us, > + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. > + */ > + if (src->id == dst->id) > + return 0; > + > + if (mc->curr_tnode->type == TEGRA_ICC_NISO) The mc->curr_tnode usage looks suspicious, why you can't use src node?
20.12.2022 19:02, Sumit Gupta пишет: > static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > { > + struct tegra_icc_node *tnode; > struct icc_node *node; > unsigned int i; > int err; > @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > mc->provider.data = &mc->provider; > mc->provider.set = mc->soc->icc_ops->set; > mc->provider.aggregate = mc->soc->icc_ops->aggregate; > - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > + mc->provider.get_bw = mc->soc->icc_ops->get_bw; > + if (mc->soc->icc_ops->xlate) > + mc->provider.xlate = mc->soc->icc_ops->xlate; > + if (mc->soc->icc_ops->xlate_extended) > + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > > err = icc_provider_add(&mc->provider); > if (err) > @@ -814,6 +820,10 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > goto remove_nodes; > > for (i = 0; i < mc->soc->num_clients; i++) { > + tnode = kzalloc(sizeof(*tnode), GFP_KERNEL); devm_kzalloc On the other hand, the tnode is unnecessary at all. Use struct tegra_mc_client for sw clients.
On 20/12/2022 17:02, Sumit Gupta wrote: > Adding Interconnect framework support to dynamically set the DRAM > bandwidth from different clients. Both the MC and EMC drivers are > added as ICC providers. The path for any request will be: > MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM > > MC clients will request for bandwidth to the MC driver which will > pass the tegra icc node having current request info to the EMC driver. > The EMC driver will send the BPMP Client ID, Client type and bandwidth > request info to the BPMP-FW where the final DRAM freq for achieving the > requested bandwidth is set based on the passed parameters. > > Signed-off-by: Sumit Gupta <sumitg@nvidia.com> > --- > drivers/memory/tegra/mc.c | 18 ++- > drivers/memory/tegra/tegra186-emc.c | 166 ++++++++++++++++++++++++++++ > drivers/memory/tegra/tegra234.c | 101 ++++++++++++++++- > include/soc/tegra/mc.h | 7 ++ > include/soc/tegra/tegra-icc.h | 72 ++++++++++++ > 5 files changed, 362 insertions(+), 2 deletions(-) > create mode 100644 include/soc/tegra/tegra-icc.h > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > index 592907546ee6..ff887fb03bce 100644 > --- a/drivers/memory/tegra/mc.c > +++ b/drivers/memory/tegra/mc.c > @@ -17,6 +17,7 @@ > #include <linux/sort.h> > > #include <soc/tegra/fuse.h> > +#include <soc/tegra/tegra-icc.h> > > #include "mc.h" > > @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = { > */ > static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > { > + struct tegra_icc_node *tnode; > struct icc_node *node; > unsigned int i; > int err; > @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > mc->provider.data = &mc->provider; > mc->provider.set = mc->soc->icc_ops->set; > mc->provider.aggregate = mc->soc->icc_ops->aggregate; > - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > + mc->provider.get_bw = mc->soc->icc_ops->get_bw; > + if (mc->soc->icc_ops->xlate) > + mc->provider.xlate = mc->soc->icc_ops->xlate; > + if (mc->soc->icc_ops->xlate_extended) > + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; > > err = icc_provider_add(&mc->provider); > if (err) > @@ -814,6 +820,10 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > goto remove_nodes; > > for (i = 0; i < mc->soc->num_clients; i++) { > + tnode = kzalloc(sizeof(*tnode), GFP_KERNEL); > + if (!tnode) > + return -ENOMEM; > + > /* create MC client node */ > node = icc_node_create(mc->soc->clients[i].id); > if (IS_ERR(node)) { > @@ -828,6 +838,12 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > err = icc_link_create(node, TEGRA_ICC_MC); > if (err) > goto remove_nodes; > + > + node->data = tnode; Where is it freed? (...) > > struct tegra_mc_ops { > @@ -238,6 +243,8 @@ struct tegra_mc { > struct { > struct dentry *root; > } debugfs; > + > + struct tegra_icc_node *curr_tnode; > }; > > int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate); > diff --git a/include/soc/tegra/tegra-icc.h b/include/soc/tegra/tegra-icc.h > new file mode 100644 > index 000000000000..3855d8571281 > --- /dev/null > +++ b/include/soc/tegra/tegra-icc.h Why not in linux? > @@ -0,0 +1,72 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2022-2023 NVIDIA CORPORATION. All rights reserved. > + */ > + > +#ifndef MEMORY_TEGRA_ICC_H This does not match the path/name. > +#define MEMORY_TEGRA_ICC_H > + > +enum tegra_icc_client_type { > + TEGRA_ICC_NONE, > + TEGRA_ICC_NISO, > + TEGRA_ICC_ISO_DISPLAY, > + TEGRA_ICC_ISO_VI, > + TEGRA_ICC_ISO_AUDIO, > + TEGRA_ICC_ISO_VIFAL, > +}; > + > +struct tegra_icc_node { > + struct icc_node *node; > + struct tegra_mc *mc; > + u32 bpmp_id; > + u32 type; > +}; > + > +/* ICC ID's for MC client's used in BPMP */ > +#define TEGRA_ICC_BPMP_DEBUG 1 > +#define TEGRA_ICC_BPMP_CPU_CLUSTER0 2 > +#define TEGRA_ICC_BPMP_CPU_CLUSTER1 3 > +#define TEGRA_ICC_BPMP_CPU_CLUSTER2 4 > +#define TEGRA_ICC_BPMP_GPU 5 > +#define TEGRA_ICC_BPMP_CACTMON 6 > +#define TEGRA_ICC_BPMP_DISPLAY 7 > +#define TEGRA_ICC_BPMP_VI 8 > +#define TEGRA_ICC_BPMP_EQOS 9 > +#define TEGRA_ICC_BPMP_PCIE_0 10 > +#define TEGRA_ICC_BPMP_PCIE_1 11 > +#define TEGRA_ICC_BPMP_PCIE_2 12 > +#define TEGRA_ICC_BPMP_PCIE_3 13 > +#define TEGRA_ICC_BPMP_PCIE_4 14 > +#define TEGRA_ICC_BPMP_PCIE_5 15 > +#define TEGRA_ICC_BPMP_PCIE_6 16 > +#define TEGRA_ICC_BPMP_PCIE_7 17 > +#define TEGRA_ICC_BPMP_PCIE_8 18 > +#define TEGRA_ICC_BPMP_PCIE_9 19 > +#define TEGRA_ICC_BPMP_PCIE_10 20 > +#define TEGRA_ICC_BPMP_DLA_0 21 > +#define TEGRA_ICC_BPMP_DLA_1 22 > +#define TEGRA_ICC_BPMP_SDMMC_1 23 > +#define TEGRA_ICC_BPMP_SDMMC_2 24 > +#define TEGRA_ICC_BPMP_SDMMC_3 25 > +#define TEGRA_ICC_BPMP_SDMMC_4 26 > +#define TEGRA_ICC_BPMP_NVDEC 27 > +#define TEGRA_ICC_BPMP_NVENC 28 > +#define TEGRA_ICC_BPMP_NVJPG_0 29 > +#define TEGRA_ICC_BPMP_NVJPG_1 30 > +#define TEGRA_ICC_BPMP_OFAA 31 > +#define TEGRA_ICC_BPMP_XUSB_HOST 32 > +#define TEGRA_ICC_BPMP_XUSB_DEV 33 > +#define TEGRA_ICC_BPMP_TSEC 34 > +#define TEGRA_ICC_BPMP_VIC 35 > +#define TEGRA_ICC_BPMP_APE 36 > +#define TEGRA_ICC_BPMP_APEDMA 37 > +#define TEGRA_ICC_BPMP_SE 38 > +#define TEGRA_ICC_BPMP_ISP 39 > +#define TEGRA_ICC_BPMP_HDA 40 > +#define TEGRA_ICC_BPMP_VIFAL 41 > +#define TEGRA_ICC_BPMP_VI2FAL 42 > +#define TEGRA_ICC_BPMP_VI2 43 > +#define TEGRA_ICC_BPMP_RCE 44 > +#define TEGRA_ICC_BPMP_PVA 45 > + > +#endif /* MEMORY_TEGRA_ICC_H */ Best regards, Krzysztof
21.12.2022 22:20, Dmitry Osipenko пишет: > 20.12.2022 19:02, Sumit Gupta пишет: >> +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) >> +{ >> + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); >> + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); >> + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; >> + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; >> + struct tegra_icc_node *tnode = mc->curr_tnode; >> + struct tegra_bpmp_message msg; >> + int ret = 0; >> + >> + /* >> + * Same Src and Dst node will happen during boot from icc_node_add(). >> + * This can be used to pre-initialize and set bandwidth for all clients >> + * before their drivers are loaded. We are skipping this case as for us, >> + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. >> + */ >> + if (src->id == dst->id) >> + return 0; >> + >> + if (mc->curr_tnode->type == TEGRA_ICC_NISO) > > The mc->curr_tnode usage looks suspicious, why you can't use src node? > This function sets memory BW for a memory client and not for EMC. Apparently, you should move the BW setting to tegra234_mc_icc_set() and then tegra_emc_icc_set_bw() will be a noop.
On 21/12/22 22:13, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 21.12.2022 12:35, Sumit Gupta пишет: >> >> >> On 20/12/22 23:40, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 20.12.2022 19:02, Sumit Gupta пишет: >>>> +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 >>>> *avg, u32 *peak) >>>> +{ >>>> + *avg = 0; >>>> + *peak = 0; >>>> + >>>> + return 0; >>>> +} >>> >>> Looks wrong, you should add ICC support to all the drivers first and >>> only then enable ICC. I think you added this init_bw() to work around >>> the fact that ICC isn't supported by T234 drivers. >> >> If get_bw hook is not added then max freq is set due to 'INT_MAX' below. >> >> void icc_node_add(struct icc_node *node, struct icc_provider *provider) >> { >> .... >> /* get the initial bandwidth values and sync them with hardware */ >> if (provider->get_bw) { >> provider->get_bw(node, &node->init_avg, &node->init_peak); >> } else { >> node->init_avg = INT_MAX; >> node->init_peak = INT_MAX; >> } >> >> So, will have to add the empty functions at least. >> >> static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, >> u32 *peak) >> { >> - *avg = 0; >> - *peak = 0; >> - >> return 0; >> } >> >> Support to all the client drivers can't be added at once as there are >> many drivers all with different requirements and handling. This patch >> series is the beginning to add the basic interconnect support in new >> Tegra SoC's. Support for more clients will be added later one by one or >> in batch. > > This means that bandwidth management isn't working properly. You should > leave the freq to INT_MAX and fix the missing integer overflows in the > code if any, or read out the BW from FW. > > Once you'll enable ICC for all drivers, it will start working. > Referred the below patches and now understand what you mean. https://patchwork.kernel.org/project/linux-arm-msm/cover/20200825170152.6434-1-georgi.djakov@linaro.org/ https://lore.kernel.org/all/20210912183009.6400-1-digetx@gmail.com/ There is no MRQ currently in BPMP-FW to get the bandwidth for a client. So, can't add that in get_bw(). As explained earlier, we are in process adding of adding support for all clients but that may take some time. We can remove the get_bw() calls initializing "*bw = 0" once we have the support added in all clients. Noticed that below drivers also do same. $ grep -rB3 "*peak = 0" drivers/interconnect/* drivers/interconnect/imx/imx.c-static int imx_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak) drivers/interconnect/imx/imx.c-{ drivers/interconnect/imx/imx.c- *avg = 0; drivers/interconnect/imx/imx.c: *peak = 0; -- drivers/interconnect/qcom/msm8974.c-static int msm8974_get_bw(struct icc_node *node, u32 *avg, u32 *peak) drivers/interconnect/qcom/msm8974.c-{ drivers/interconnect/qcom/msm8974.c- *avg = 0; drivers/interconnect/qcom/msm8974.c: *peak = 0;
On 21/12/22 22:24, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> static int tegra186_emc_probe(struct platform_device *pdev) >> { >> struct mrq_emc_dvfs_latency_response response; >> struct tegra_bpmp_message msg; >> struct tegra186_emc *emc; >> + struct tegra_mc *mc; >> unsigned int i; >> int err; >> >> @@ -158,6 +307,9 @@ static int tegra186_emc_probe(struct platform_device *pdev) >> if (!emc) >> return -ENOMEM; >> >> + platform_set_drvdata(pdev, emc); >> + emc->dev = &pdev->dev; >> + >> emc->bpmp = tegra_bpmp_get(&pdev->dev); >> if (IS_ERR(emc->bpmp)) >> return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n"); >> @@ -236,6 +388,19 @@ static int tegra186_emc_probe(struct platform_device *pdev) >> debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root, >> emc, &tegra186_emc_debug_max_rate_fops); >> >> + mc = dev_get_drvdata(emc->dev->parent); >> + if (mc && mc->soc->icc_ops) { >> + if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) { >> + err = tegra_emc_interconnect_init(emc); >> + if (!err) >> + return err; >> + dev_err(&pdev->dev, "tegra_emc_interconnect_init failed:%d\n", err); >> + goto put_bpmp; >> + } else { >> + dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n"); >> + } > > If there is no MRQ_BWMGR_INT, then device drivers using ICC won't probe. > This is either a error condition, or ICC should inited and then ICC > changes should be skipped. > If the MRQ_BW_MGR is not supported by a BPMP-FW binary, then the MC & EMC drivers will still probe successfully and scaling will be disabled.
On 22/12/22 21:26, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 21.12.2022 22:20, Dmitry Osipenko пишет: >> 20.12.2022 19:02, Sumit Gupta пишет: >>> +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) >>> +{ >>> + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); >>> + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); >>> + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; >>> + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; >>> + struct tegra_icc_node *tnode = mc->curr_tnode; >>> + struct tegra_bpmp_message msg; >>> + int ret = 0; >>> + >>> + /* >>> + * Same Src and Dst node will happen during boot from icc_node_add(). >>> + * This can be used to pre-initialize and set bandwidth for all clients >>> + * before their drivers are loaded. We are skipping this case as for us, >>> + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. >>> + */ >>> + if (src->id == dst->id) >>> + return 0; >>> + >>> + if (mc->curr_tnode->type == TEGRA_ICC_NISO) >> >> The mc->curr_tnode usage looks suspicious, why you can't use src node? >> > > This function sets memory BW for a memory client and not for EMC. > Apparently, you should move the BW setting to tegra234_mc_icc_set() and > then tegra_emc_icc_set_bw() will be a noop. > Yes, will move this code to set_bw() in MC driver. The set_bw call in EMC will be a dummy function after change. This change will also help to remove "struct tegra_icc_node *curr_tnode;" from "struct tegra_mc" which is used for passing data between MC and EMC drivers to transfer to BPMP. static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) { return 0; }
On 22/12/22 00:50, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.12.2022 19:02, Sumit Gupta пишет: >> +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) >> +{ >> + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); >> + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); >> + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; >> + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; >> + struct tegra_icc_node *tnode = mc->curr_tnode; >> + struct tegra_bpmp_message msg; >> + int ret = 0; >> + >> + /* >> + * Same Src and Dst node will happen during boot from icc_node_add(). >> + * This can be used to pre-initialize and set bandwidth for all clients >> + * before their drivers are loaded. We are skipping this case as for us, >> + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. >> + */ >> + if (src->id == dst->id) >> + return 0; >> + >> + if (mc->curr_tnode->type == TEGRA_ICC_NISO) > > The mc->curr_tnode usage looks suspicious, why you can't use src node? > Now, we can get rid of "curr_tnode" after moving the code from the EMC to MC driver for transferring MC client's request info to the BPMP.
On 21/12/22 22:14, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 21.12.2022 11:05, Sumit Gupta пишет: >> On 20/12/22 23:37, Dmitry Osipenko wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 20.12.2022 19:02, Sumit Gupta пишет: >>>> +#ifndef MEMORY_TEGRA_ICC_H >>>> +#define MEMORY_TEGRA_ICC_H >>>> + >>>> +enum tegra_icc_client_type { >>>> + TEGRA_ICC_NONE, >>>> + TEGRA_ICC_NISO, >>>> + TEGRA_ICC_ISO_DISPLAY, >>>> + TEGRA_ICC_ISO_VI, >>>> + TEGRA_ICC_ISO_AUDIO, >>>> + TEGRA_ICC_ISO_VIFAL, >>>> +}; >>> >>> You using only TEGRA_ICC_NISO and !TEGRA_ICC_NISO in the code. >>> >>> include/soc/tegra/mc.h defines TAG_DEFAULT/ISO, please drop all these >>> duplicated and unused "types" unless there is a good reason to keep them >>> >> >> These type are used while defining clients in "tegra234_mc_clients[]" >> and its passed to BPMP-FW which has handling for each client type. > > The type should be based on the ICC tag, IMO. AFAICS, type isn't fixed > in FW and you can set both ISO and NISO BW, hence it's up to a device > driver to select the appropriate tag. > Type for a MC client is fixed. So, adding the tag and giving option to client driver won't have impact. Also, we need to pass the type to BPMP from bw set api. But the tag info is available to aggregate api and not set.
On 22/12/22 17:02, Krzysztof Kozlowski wrote: > External email: Use caution opening links or attachments > > > On 20/12/2022 17:02, Sumit Gupta wrote: >> Adding Interconnect framework support to dynamically set the DRAM >> bandwidth from different clients. Both the MC and EMC drivers are >> added as ICC providers. The path for any request will be: >> MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM >> >> MC clients will request for bandwidth to the MC driver which will >> pass the tegra icc node having current request info to the EMC driver. >> The EMC driver will send the BPMP Client ID, Client type and bandwidth >> request info to the BPMP-FW where the final DRAM freq for achieving the >> requested bandwidth is set based on the passed parameters. >> >> Signed-off-by: Sumit Gupta <sumitg@nvidia.com> >> --- >> drivers/memory/tegra/mc.c | 18 ++- >> drivers/memory/tegra/tegra186-emc.c | 166 ++++++++++++++++++++++++++++ >> drivers/memory/tegra/tegra234.c | 101 ++++++++++++++++- >> include/soc/tegra/mc.h | 7 ++ >> include/soc/tegra/tegra-icc.h | 72 ++++++++++++ >> 5 files changed, 362 insertions(+), 2 deletions(-) >> create mode 100644 include/soc/tegra/tegra-icc.h >> >> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c >> index 592907546ee6..ff887fb03bce 100644 >> --- a/drivers/memory/tegra/mc.c >> +++ b/drivers/memory/tegra/mc.c >> @@ -17,6 +17,7 @@ >> #include <linux/sort.h> >> >> #include <soc/tegra/fuse.h> >> +#include <soc/tegra/tegra-icc.h> >> >> #include "mc.h" >> >> @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = { >> */ >> static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> { >> + struct tegra_icc_node *tnode; >> struct icc_node *node; >> unsigned int i; >> int err; >> @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> mc->provider.data = &mc->provider; >> mc->provider.set = mc->soc->icc_ops->set; >> mc->provider.aggregate = mc->soc->icc_ops->aggregate; >> - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; >> + mc->provider.get_bw = mc->soc->icc_ops->get_bw; >> + if (mc->soc->icc_ops->xlate) >> + mc->provider.xlate = mc->soc->icc_ops->xlate; >> + if (mc->soc->icc_ops->xlate_extended) >> + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; >> >> err = icc_provider_add(&mc->provider); >> if (err) >> @@ -814,6 +820,10 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> goto remove_nodes; >> >> for (i = 0; i < mc->soc->num_clients; i++) { >> + tnode = kzalloc(sizeof(*tnode), GFP_KERNEL); >> + if (!tnode) >> + return -ENOMEM; >> + >> /* create MC client node */ >> node = icc_node_create(mc->soc->clients[i].id); >> if (IS_ERR(node)) { >> @@ -828,6 +838,12 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> err = icc_link_create(node, TEGRA_ICC_MC); >> if (err) >> goto remove_nodes; >> + >> + node->data = tnode; > > Where is it freed? > > > (...) > Have removed 'struct tegra_icc_node' in v2. Instead 'node->data' now points to the entry of MC client in the static table 'tegra234_mc_clients' as below. So, the old alloc of 'struct tegra_icc_node' is not required now. + node->data = (char *)&(mc->soc->clients[i]); >> >> struct tegra_mc_ops { >> @@ -238,6 +243,8 @@ struct tegra_mc { >> struct { >> struct dentry *root; >> } debugfs; >> + >> + struct tegra_icc_node *curr_tnode; >> }; >> >> int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate); >> diff --git a/include/soc/tegra/tegra-icc.h b/include/soc/tegra/tegra-icc.h >> new file mode 100644 >> index 000000000000..3855d8571281 >> --- /dev/null >> +++ b/include/soc/tegra/tegra-icc.h > > Why not in linux? > Moved the file to 'include/linux/tegra-icc.h' in v2. >> @@ -0,0 +1,72 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2022-2023 NVIDIA CORPORATION. All rights reserved. >> + */ >> + >> +#ifndef MEMORY_TEGRA_ICC_H > > This does not match the path/name. > Have changed path name to below in v2. +#ifndef LINUX_TEGRA_ICC_H +#define LINUX_TEGRA_ICC_H >> +#define MEMORY_TEGRA_ICC_H >> + >> +enum tegra_icc_client_type { >> + TEGRA_ICC_NONE, >> + TEGRA_ICC_NISO, >> + TEGRA_ICC_ISO_DISPLAY, >> + TEGRA_ICC_ISO_VI, >> + TEGRA_ICC_ISO_AUDIO, >> + TEGRA_ICC_ISO_VIFAL, >> +}; >> + >> +struct tegra_icc_node { >> + struct icc_node *node; >> + struct tegra_mc *mc; >> + u32 bpmp_id; >> + u32 type; >> +}; >> + >> +/* ICC ID's for MC client's used in BPMP */ >> +#define TEGRA_ICC_BPMP_DEBUG 1 >> +#define TEGRA_ICC_BPMP_CPU_CLUSTER0 2 >> +#define TEGRA_ICC_BPMP_CPU_CLUSTER1 3 >> +#define TEGRA_ICC_BPMP_CPU_CLUSTER2 4 >> +#define TEGRA_ICC_BPMP_GPU 5 >> +#define TEGRA_ICC_BPMP_CACTMON 6 >> +#define TEGRA_ICC_BPMP_DISPLAY 7 >> +#define TEGRA_ICC_BPMP_VI 8 >> +#define TEGRA_ICC_BPMP_EQOS 9 >> +#define TEGRA_ICC_BPMP_PCIE_0 10 >> +#define TEGRA_ICC_BPMP_PCIE_1 11 >> +#define TEGRA_ICC_BPMP_PCIE_2 12 >> +#define TEGRA_ICC_BPMP_PCIE_3 13 >> +#define TEGRA_ICC_BPMP_PCIE_4 14 >> +#define TEGRA_ICC_BPMP_PCIE_5 15 >> +#define TEGRA_ICC_BPMP_PCIE_6 16 >> +#define TEGRA_ICC_BPMP_PCIE_7 17 >> +#define TEGRA_ICC_BPMP_PCIE_8 18 >> +#define TEGRA_ICC_BPMP_PCIE_9 19 >> +#define TEGRA_ICC_BPMP_PCIE_10 20 >> +#define TEGRA_ICC_BPMP_DLA_0 21 >> +#define TEGRA_ICC_BPMP_DLA_1 22 >> +#define TEGRA_ICC_BPMP_SDMMC_1 23 >> +#define TEGRA_ICC_BPMP_SDMMC_2 24 >> +#define TEGRA_ICC_BPMP_SDMMC_3 25 >> +#define TEGRA_ICC_BPMP_SDMMC_4 26 >> +#define TEGRA_ICC_BPMP_NVDEC 27 >> +#define TEGRA_ICC_BPMP_NVENC 28 >> +#define TEGRA_ICC_BPMP_NVJPG_0 29 >> +#define TEGRA_ICC_BPMP_NVJPG_1 30 >> +#define TEGRA_ICC_BPMP_OFAA 31 >> +#define TEGRA_ICC_BPMP_XUSB_HOST 32 >> +#define TEGRA_ICC_BPMP_XUSB_DEV 33 >> +#define TEGRA_ICC_BPMP_TSEC 34 >> +#define TEGRA_ICC_BPMP_VIC 35 >> +#define TEGRA_ICC_BPMP_APE 36 >> +#define TEGRA_ICC_BPMP_APEDMA 37 >> +#define TEGRA_ICC_BPMP_SE 38 >> +#define TEGRA_ICC_BPMP_ISP 39 >> +#define TEGRA_ICC_BPMP_HDA 40 >> +#define TEGRA_ICC_BPMP_VIFAL 41 >> +#define TEGRA_ICC_BPMP_VI2FAL 42 >> +#define TEGRA_ICC_BPMP_VI2 43 >> +#define TEGRA_ICC_BPMP_RCE 44 >> +#define TEGRA_ICC_BPMP_PVA 45 >> + >> +#endif /* MEMORY_TEGRA_ICC_H */ > > Best regards, > Krzysztof >
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c index 592907546ee6..ff887fb03bce 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -17,6 +17,7 @@ #include <linux/sort.h> #include <soc/tegra/fuse.h> +#include <soc/tegra/tegra-icc.h> #include "mc.h" @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = { */ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) { + struct tegra_icc_node *tnode; struct icc_node *node; unsigned int i; int err; @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) mc->provider.data = &mc->provider; mc->provider.set = mc->soc->icc_ops->set; mc->provider.aggregate = mc->soc->icc_ops->aggregate; - mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; + mc->provider.get_bw = mc->soc->icc_ops->get_bw; + if (mc->soc->icc_ops->xlate) + mc->provider.xlate = mc->soc->icc_ops->xlate; + if (mc->soc->icc_ops->xlate_extended) + mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended; err = icc_provider_add(&mc->provider); if (err) @@ -814,6 +820,10 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) goto remove_nodes; for (i = 0; i < mc->soc->num_clients; i++) { + tnode = kzalloc(sizeof(*tnode), GFP_KERNEL); + if (!tnode) + return -ENOMEM; + /* create MC client node */ node = icc_node_create(mc->soc->clients[i].id); if (IS_ERR(node)) { @@ -828,6 +838,12 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc) err = icc_link_create(node, TEGRA_ICC_MC); if (err) goto remove_nodes; + + node->data = tnode; + tnode->node = node; + tnode->type = mc->soc->clients[i].type; + tnode->bpmp_id = mc->soc->clients[i].bpmp_id; + tnode->mc = mc; } return 0; diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c index 26e763bde92a..3500ed6ccd8f 100644 --- a/drivers/memory/tegra/tegra186-emc.c +++ b/drivers/memory/tegra/tegra186-emc.c @@ -8,8 +8,11 @@ #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/platform_device.h> +#include <linux/of_platform.h> #include <soc/tegra/bpmp.h> +#include <soc/tegra/tegra-icc.h> +#include "mc.h" struct tegra186_emc_dvfs { unsigned long latency; @@ -29,8 +32,15 @@ struct tegra186_emc { unsigned long min_rate; unsigned long max_rate; } debugfs; + + struct icc_provider provider; }; +static inline struct tegra186_emc *to_tegra186_emc(struct icc_provider *provider) +{ + return container_of(provider, struct tegra186_emc, provider); +} + /* * debugfs interface * @@ -146,11 +156,150 @@ DEFINE_DEBUGFS_ATTRIBUTE(tegra186_emc_debug_max_rate_fops, tegra186_emc_debug_max_rate_get, tegra186_emc_debug_max_rate_set, "%llu\n"); +/* + * tegra_emc_icc_set_bw() - Pass MC client info and BW request to BPMP + * @src: ICC node for External Memory Controller (EMC) + * @dst: ICC node for External Memory (DRAM) + * + * Passing the MC client info and Banwidth request to BPMP-FW where + * LA and PTSA registers are accessed and the final EMC freq is set based + * on client, type, latency and bandwidth. + */ +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst) +{ + struct tegra186_emc *emc = to_tegra186_emc(dst->provider); + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); + struct mrq_bwmgr_int_request bwmgr_req = { 0 }; + struct mrq_bwmgr_int_response bwmgr_resp = { 0 }; + struct tegra_icc_node *tnode = mc->curr_tnode; + struct tegra_bpmp_message msg; + int ret = 0; + + /* + * Same Src and Dst node will happen during boot from icc_node_add(). + * This can be used to pre-initialize and set bandwidth for all clients + * before their drivers are loaded. We are skipping this case as for us, + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. + */ + if (src->id == dst->id) + return 0; + + if (mc->curr_tnode->type == TEGRA_ICC_NISO) + bwmgr_req.bwmgr_calc_set_req.niso_bw = tnode->node->avg_bw; + else + bwmgr_req.bwmgr_calc_set_req.iso_bw = tnode->node->avg_bw; + + bwmgr_req.bwmgr_calc_set_req.client_id = tnode->bpmp_id; + + bwmgr_req.cmd = CMD_BWMGR_INT_CALC_AND_SET; + bwmgr_req.bwmgr_calc_set_req.mc_floor = tnode->node->peak_bw; + bwmgr_req.bwmgr_calc_set_req.floor_unit = BWMGR_INT_UNIT_KBPS; + + memset(&msg, 0, sizeof(msg)); + msg.mrq = MRQ_BWMGR_INT; + msg.tx.data = &bwmgr_req; + msg.tx.size = sizeof(bwmgr_req); + msg.rx.data = &bwmgr_resp; + msg.rx.size = sizeof(bwmgr_resp); + + ret = tegra_bpmp_transfer(emc->bpmp, &msg); + if (ret < 0) { + dev_err(emc->dev, "BPMP transfer failed: %d\n", ret); + goto error; + } + if (msg.rx.ret < 0) { + pr_err("failed to set bandwidth for %u: %d\n", + bwmgr_req.bwmgr_calc_set_req.client_id, msg.rx.ret); + ret = -EINVAL; + } + +error: + return ret; +} + +static struct icc_node * +tegra_emc_of_icc_xlate(struct of_phandle_args *spec, void *data) +{ + struct icc_provider *provider = data; + struct icc_node *node; + + /* External Memory is the only possible ICC route */ + list_for_each_entry(node, &provider->nodes, node_list) { + if (node->id != TEGRA_ICC_EMEM) + continue; + + return node; + } + + return ERR_PTR(-EPROBE_DEFER); +} + +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak) +{ + *avg = 0; + *peak = 0; + + return 0; +} + +static int tegra_emc_interconnect_init(struct tegra186_emc *emc) +{ + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent); + const struct tegra_mc_soc *soc = mc->soc; + struct icc_node *node; + int err = 0; + + emc->provider.dev = emc->dev; + emc->provider.set = tegra_emc_icc_set_bw; + emc->provider.data = &emc->provider; + emc->provider.aggregate = soc->icc_ops->aggregate; + emc->provider.xlate = tegra_emc_of_icc_xlate; + emc->provider.get_bw = tegra_emc_icc_get_init_bw; + + err = icc_provider_add(&emc->provider); + if (err) + return err; + + /* create External Memory Controller node */ + node = icc_node_create(TEGRA_ICC_EMC); + if (IS_ERR(node)) { + err = PTR_ERR(node); + goto del_provider; + } + + node->name = "External Memory Controller"; + icc_node_add(node, &emc->provider); + + /* link External Memory Controller to External Memory (DRAM) */ + err = icc_link_create(node, TEGRA_ICC_EMEM); + if (err) + goto remove_nodes; + + /* create External Memory node */ + node = icc_node_create(TEGRA_ICC_EMEM); + if (IS_ERR(node)) { + err = PTR_ERR(node); + goto remove_nodes; + } + + node->name = "External Memory (DRAM)"; + icc_node_add(node, &emc->provider); + + return 0; +remove_nodes: + icc_nodes_remove(&emc->provider); +del_provider: + icc_provider_del(&emc->provider); + + return err; +} + static int tegra186_emc_probe(struct platform_device *pdev) { struct mrq_emc_dvfs_latency_response response; struct tegra_bpmp_message msg; struct tegra186_emc *emc; + struct tegra_mc *mc; unsigned int i; int err; @@ -158,6 +307,9 @@ static int tegra186_emc_probe(struct platform_device *pdev) if (!emc) return -ENOMEM; + platform_set_drvdata(pdev, emc); + emc->dev = &pdev->dev; + emc->bpmp = tegra_bpmp_get(&pdev->dev); if (IS_ERR(emc->bpmp)) return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n"); @@ -236,6 +388,19 @@ static int tegra186_emc_probe(struct platform_device *pdev) debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root, emc, &tegra186_emc_debug_max_rate_fops); + mc = dev_get_drvdata(emc->dev->parent); + if (mc && mc->soc->icc_ops) { + if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) { + err = tegra_emc_interconnect_init(emc); + if (!err) + return err; + dev_err(&pdev->dev, "tegra_emc_interconnect_init failed:%d\n", err); + goto put_bpmp; + } else { + dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n"); + } + } + return 0; put_bpmp: @@ -272,6 +437,7 @@ static struct platform_driver tegra186_emc_driver = { .name = "tegra186-emc", .of_match_table = tegra186_emc_of_match, .suppress_bind_attrs = true, + .sync_state = icc_sync_state, }, .probe = tegra186_emc_probe, .remove = tegra186_emc_remove, diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c index 02dcc5748bba..65d4e32ee118 100644 --- a/drivers/memory/tegra/tegra234.c +++ b/drivers/memory/tegra/tegra234.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (C) 2021-2022, NVIDIA CORPORATION. All rights reserved. + * Copyright (C) 2021-2023, NVIDIA CORPORATION. All rights reserved. */ #include <soc/tegra/mc.h> @@ -8,11 +8,16 @@ #include <dt-bindings/memory/tegra234-mc.h> #include "mc.h" +#include <soc/tegra/tegra-icc.h> +#include <linux/interconnect.h> +#include <linux/of_device.h> static const struct tegra_mc_client tegra234_mc_clients[] = { { .id = TEGRA234_MEMORY_CLIENT_MGBEARD, .name = "mgbeard", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE, .regs = { .sid = { @@ -23,6 +28,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBEBRD, .name = "mgbebrd", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF1, .regs = { .sid = { @@ -33,6 +40,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBECRD, .name = "mgbecrd", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF2, .regs = { .sid = { @@ -43,6 +52,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBEDRD, .name = "mgbedrd", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF3, .regs = { .sid = { @@ -52,6 +63,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, }, { .id = TEGRA234_MEMORY_CLIENT_MGBEAWR, + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .name = "mgbeawr", .sid = TEGRA234_SID_MGBE, .regs = { @@ -63,6 +76,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBEBWR, .name = "mgbebwr", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF1, .regs = { .sid = { @@ -73,6 +88,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBECWR, .name = "mgbecwr", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF2, .regs = { .sid = { @@ -83,6 +100,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_SDMMCRAB, .name = "sdmmcrab", + .bpmp_id = TEGRA_ICC_BPMP_SDMMC_4, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_SDMMC4, .regs = { .sid = { @@ -93,6 +112,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_MGBEDWR, .name = "mgbedwr", + .bpmp_id = TEGRA_ICC_BPMP_EQOS, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_MGBE_VF3, .regs = { .sid = { @@ -103,6 +124,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_SDMMCWAB, .name = "sdmmcwab", + .bpmp_id = TEGRA_ICC_BPMP_SDMMC_4, + .type = TEGRA_ICC_NISO, .sid = TEGRA234_SID_SDMMC4, .regs = { .sid = { @@ -153,6 +176,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_APEDMAR, .name = "apedmar", + .bpmp_id = TEGRA_ICC_BPMP_APEDMA, + .type = TEGRA_ICC_ISO_AUDIO, .sid = TEGRA234_SID_APE, .regs = { .sid = { @@ -163,6 +188,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, { .id = TEGRA234_MEMORY_CLIENT_APEDMAW, .name = "apedmaw", + .bpmp_id = TEGRA_ICC_BPMP_APEDMA, + .type = TEGRA_ICC_ISO_AUDIO, .sid = TEGRA234_SID_APE, .regs = { .sid = { @@ -333,6 +360,77 @@ static const struct tegra_mc_client tegra234_mc_clients[] = { }, }; +/* + * tegra234_mc_icc_set() - Pass MC client info to External Memory Controller (EMC) + * @src: ICC node for Memory Controller's (MC) Client + * @dst: ICC node for Memory Controller (MC) + * + * Passing the current request info from the MC to the EMC driver using + * 'struct tegra_mc'. EMC driver will further send the MC client's request info + * to the BPMP-FW where LA and PTSA registers are accessed and the final EMC freq + * is set based on client, type, latency and bandwidth. + * icc_set_bw() makes set_bw calls for both MC and EMC providers in sequence. + * Both the calls are protected by 'mutex_lock(&icc_lock)'. So, the data passed + * from MC and EMC won't be updated by concurrent set calls from other clients. + */ +static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst) +{ + struct tegra_mc *mc = icc_provider_to_tegra_mc(dst->provider); + struct tegra_icc_node *tnode = src->data; + + /* + * Same Src and Dst node will happen during boot from icc_node_add(). + * This can be used to pre-initialize and set bandwidth for all clients + * before their drivers are loaded. We are skipping this case as for us, + * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW. + */ + if (src->id == dst->id) + return 0; + + if (tnode->node) + mc->curr_tnode = tnode; + else + pr_err("%s, tegra_icc_node is null\n", __func__); + + return 0; +} + +static struct icc_node* +tegra234_mc_of_icc_xlate(struct of_phandle_args *spec, void *data) +{ + struct tegra_mc *mc = icc_provider_to_tegra_mc(data); + unsigned int cl_id = spec->args[0]; + struct icc_node *node; + + list_for_each_entry(node, &mc->provider.nodes, node_list) { + if (node->id != cl_id) + continue; + + return node; + } + + /* + * If a client driver calls devm_of_icc_get() before the MC driver + * is probed, then return EPROBE_DEFER to the client driver. + */ + return ERR_PTR(-EPROBE_DEFER); +} + +static int tegra234_mc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak) +{ + *avg = 0; + *peak = 0; + + return 0; +} + +static const struct tegra_mc_icc_ops tegra234_mc_icc_ops = { + .xlate = tegra234_mc_of_icc_xlate, + .get_bw = tegra234_mc_icc_get_init_bw, + .aggregate = icc_std_aggregate, + .set = tegra234_mc_icc_set, +}; + const struct tegra_mc_soc tegra234_mc_soc = { .num_clients = ARRAY_SIZE(tegra234_mc_clients), .clients = tegra234_mc_clients, @@ -345,6 +443,7 @@ const struct tegra_mc_soc tegra234_mc_soc = { MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM, .has_addr_hi_reg = true, .ops = &tegra186_mc_ops, + .icc_ops = &tegra234_mc_icc_ops, .ch_intmask = 0x0000ff00, .global_intstatus_channel_shift = 8, /* diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 51a2263e1bc5..0a32a9eb12a4 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -13,6 +13,7 @@ #include <linux/irq.h> #include <linux/reset-controller.h> #include <linux/types.h> +#include <soc/tegra/tegra-icc.h> struct clk; struct device; @@ -26,6 +27,8 @@ struct tegra_mc_timing { struct tegra_mc_client { unsigned int id; + unsigned int bpmp_id; + enum tegra_icc_client_type type; const char *name; /* * For Tegra210 and earlier, this is the SWGROUP ID used for IOVA translations in the @@ -166,8 +169,10 @@ struct tegra_mc_icc_ops { int (*set)(struct icc_node *src, struct icc_node *dst); int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw, u32 peak_bw, u32 *agg_avg, u32 *agg_peak); + struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data); struct icc_node_data *(*xlate_extended)(struct of_phandle_args *spec, void *data); + int (*get_bw)(struct icc_node *node, u32 *avg, u32 *peak); }; struct tegra_mc_ops { @@ -238,6 +243,8 @@ struct tegra_mc { struct { struct dentry *root; } debugfs; + + struct tegra_icc_node *curr_tnode; }; int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate); diff --git a/include/soc/tegra/tegra-icc.h b/include/soc/tegra/tegra-icc.h new file mode 100644 index 000000000000..3855d8571281 --- /dev/null +++ b/include/soc/tegra/tegra-icc.h @@ -0,0 +1,72 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2022-2023 NVIDIA CORPORATION. All rights reserved. + */ + +#ifndef MEMORY_TEGRA_ICC_H +#define MEMORY_TEGRA_ICC_H + +enum tegra_icc_client_type { + TEGRA_ICC_NONE, + TEGRA_ICC_NISO, + TEGRA_ICC_ISO_DISPLAY, + TEGRA_ICC_ISO_VI, + TEGRA_ICC_ISO_AUDIO, + TEGRA_ICC_ISO_VIFAL, +}; + +struct tegra_icc_node { + struct icc_node *node; + struct tegra_mc *mc; + u32 bpmp_id; + u32 type; +}; + +/* ICC ID's for MC client's used in BPMP */ +#define TEGRA_ICC_BPMP_DEBUG 1 +#define TEGRA_ICC_BPMP_CPU_CLUSTER0 2 +#define TEGRA_ICC_BPMP_CPU_CLUSTER1 3 +#define TEGRA_ICC_BPMP_CPU_CLUSTER2 4 +#define TEGRA_ICC_BPMP_GPU 5 +#define TEGRA_ICC_BPMP_CACTMON 6 +#define TEGRA_ICC_BPMP_DISPLAY 7 +#define TEGRA_ICC_BPMP_VI 8 +#define TEGRA_ICC_BPMP_EQOS 9 +#define TEGRA_ICC_BPMP_PCIE_0 10 +#define TEGRA_ICC_BPMP_PCIE_1 11 +#define TEGRA_ICC_BPMP_PCIE_2 12 +#define TEGRA_ICC_BPMP_PCIE_3 13 +#define TEGRA_ICC_BPMP_PCIE_4 14 +#define TEGRA_ICC_BPMP_PCIE_5 15 +#define TEGRA_ICC_BPMP_PCIE_6 16 +#define TEGRA_ICC_BPMP_PCIE_7 17 +#define TEGRA_ICC_BPMP_PCIE_8 18 +#define TEGRA_ICC_BPMP_PCIE_9 19 +#define TEGRA_ICC_BPMP_PCIE_10 20 +#define TEGRA_ICC_BPMP_DLA_0 21 +#define TEGRA_ICC_BPMP_DLA_1 22 +#define TEGRA_ICC_BPMP_SDMMC_1 23 +#define TEGRA_ICC_BPMP_SDMMC_2 24 +#define TEGRA_ICC_BPMP_SDMMC_3 25 +#define TEGRA_ICC_BPMP_SDMMC_4 26 +#define TEGRA_ICC_BPMP_NVDEC 27 +#define TEGRA_ICC_BPMP_NVENC 28 +#define TEGRA_ICC_BPMP_NVJPG_0 29 +#define TEGRA_ICC_BPMP_NVJPG_1 30 +#define TEGRA_ICC_BPMP_OFAA 31 +#define TEGRA_ICC_BPMP_XUSB_HOST 32 +#define TEGRA_ICC_BPMP_XUSB_DEV 33 +#define TEGRA_ICC_BPMP_TSEC 34 +#define TEGRA_ICC_BPMP_VIC 35 +#define TEGRA_ICC_BPMP_APE 36 +#define TEGRA_ICC_BPMP_APEDMA 37 +#define TEGRA_ICC_BPMP_SE 38 +#define TEGRA_ICC_BPMP_ISP 39 +#define TEGRA_ICC_BPMP_HDA 40 +#define TEGRA_ICC_BPMP_VIFAL 41 +#define TEGRA_ICC_BPMP_VI2FAL 42 +#define TEGRA_ICC_BPMP_VI2 43 +#define TEGRA_ICC_BPMP_RCE 44 +#define TEGRA_ICC_BPMP_PVA 45 + +#endif /* MEMORY_TEGRA_ICC_H */
Adding Interconnect framework support to dynamically set the DRAM bandwidth from different clients. Both the MC and EMC drivers are added as ICC providers. The path for any request will be: MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM MC clients will request for bandwidth to the MC driver which will pass the tegra icc node having current request info to the EMC driver. The EMC driver will send the BPMP Client ID, Client type and bandwidth request info to the BPMP-FW where the final DRAM freq for achieving the requested bandwidth is set based on the passed parameters. Signed-off-by: Sumit Gupta <sumitg@nvidia.com> --- drivers/memory/tegra/mc.c | 18 ++- drivers/memory/tegra/tegra186-emc.c | 166 ++++++++++++++++++++++++++++ drivers/memory/tegra/tegra234.c | 101 ++++++++++++++++- include/soc/tegra/mc.h | 7 ++ include/soc/tegra/tegra-icc.h | 72 ++++++++++++ 5 files changed, 362 insertions(+), 2 deletions(-) create mode 100644 include/soc/tegra/tegra-icc.h