| Message ID | 20201025221735.3062-30-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce memory interconnect for NVIDIA Tegra SoCs | expand |
On Mon, Oct 26, 2020 at 01:17:12AM +0300, Dmitry Osipenko wrote: > Now Memory Controller is a memory interconnection provider. This allows > us to use interconnect API for tuning of memory configuration. This patch > adds common ICC core and adds hooks which should be implemented by the SoC > drivers. > > Tested-by: Peter Geis <pgwipeout@gmail.com> > Tested-by: Nicolas Chauvet <kwizart@gmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/memory/tegra/Kconfig | 1 + > drivers/memory/tegra/mc.c | 129 +++++++++++++++++++++++++++++++++++ > drivers/memory/tegra/mc.h | 8 +++ > include/soc/tegra/mc.h | 16 +++++ > 4 files changed, 154 insertions(+) > > diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig > index 9f0a96bf9ccc..b38e5255effe 100644 > --- a/drivers/memory/tegra/Kconfig > +++ b/drivers/memory/tegra/Kconfig > @@ -3,6 +3,7 @@ config TEGRA_MC > bool "NVIDIA Tegra Memory Controller support" > default y > depends on ARCH_TEGRA > + select INTERCONNECT > help > This driver supports the Memory Controller (MC) hardware found on > NVIDIA Tegra SoCs. > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c > index 12ea2c79205a..53d61b05ebf8 100644 > --- a/drivers/memory/tegra/mc.c > +++ b/drivers/memory/tegra/mc.c > @@ -639,6 +639,133 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data) > return IRQ_HANDLED; > } > > +static struct icc_node_data * > +tegra_mc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data) > +{ > + struct icc_provider *provider = data; > + unsigned int idx = spec->args[0]; > + struct icc_node_data *ndata; > + struct icc_node *node; > + > + list_for_each_entry(node, &provider->nodes, node_list) { > + if (node->id != idx) > + continue; > + > + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL); > + if (!ndata) > + return ERR_PTR(-ENOMEM); > + > + ndata->node = node; > + > + /* these clients are isochronous by default on all SoCs */ > + if (strstarts(node->name, "display") || > + strstarts(node->name, "ptc") || > + strstarts(node->name, "vi")) > + ndata->tag = TEGRA_MC_ICC_TAG_ISO; This looks like something that might be better left to the drivers to decide. Doing this here seems okay for now, but I suspect that this will get fairly complicated to keep accurate as we add more clients later on. > + > + return ndata; > + } > + > + pr_err("%s: invalid client index %u\n", __func__, idx); Perhaps use "dev_err(provider->dev, ...);"? > + > + return ERR_PTR(-EINVAL); > +} > + > +/* > + * Memory Controller (MC) has few Memory Clients that are issuing memory > + * bandwidth allocation requests to the MC interconnect provider. The MC > + * provider aggregates the requests and then sends the aggregated request > + * up to the External Memory Controller (EMC) interconnect provider which > + * re-configures hardware interface to External Memory (EMEM) in accordance > + * to the required bandwidth. Each MC interconnect node represents an > + * individual Memory Client. > + * > + * Memory interconnect topology: > + * > + * +----+ > + * +--------+ | | > + * | TEXSRD +--->+ | > + * +--------+ | | > + * | | +-----+ +------+ > + * ... | MC +--->+ EMC +--->+ EMEM | > + * | | +-----+ +------+ > + * +--------+ | | > + * | DISP.. +--->+ | > + * +--------+ | | > + * +----+ > + */ > +static int tegra_mc_interconnect_setup(struct tegra_mc *mc) > +{ > + struct icc_node *node; > + unsigned int i; > + int err; > + > + /* older device-trees don't have interconnect properties */ > + if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL) || > + !mc->soc->icc_ops) > + return 0; This indicates that this property is indeed optional, so the bindings should reflect that. > + > + mc->provider.dev = mc->dev; > + 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 = tegra_mc_of_icc_xlate_extended; > + > + err = icc_provider_add(&mc->provider); > + if (err) > + goto err_msg; > + > + /* create Memory Controller node */ > + node = icc_node_create(TEGRA_ICC_MC); > + err = PTR_ERR_OR_ZERO(node); > + if (err) > + goto del_provider; > + > + node->name = "Memory Controller"; > + icc_node_add(node, &mc->provider); > + > + /* link Memory Controller to External Memory Controller */ > + err = icc_link_create(node, TEGRA_ICC_EMC); > + if (err) > + goto remove_nodes; > + > + for (i = 0; i < mc->soc->num_clients; i++) { > + /* create MC client node */ > + node = icc_node_create(mc->soc->clients[i].id); > + err = PTR_ERR_OR_ZERO(node); > + if (err) > + goto remove_nodes; > + > + node->name = mc->soc->clients[i].name; > + icc_node_add(node, &mc->provider); I'm not fully familiar with how these nodes are set up, but would it be possible to set the isochronous tag here already? I'd still prefer this to be up to the drivers because I think that nicely localizes the device-specific information in the driver, but if that's not an option, then doing it here, based on lookup data from the MC clients table sounds like the next best thing. > + /* link Memory Client to Memory Controller */ > + err = icc_link_create(node, TEGRA_ICC_MC); > + if (err) > + goto remove_nodes; > + } > + > + /* > + * MC driver is registered too early, so early that generic driver > + * syncing doesn't work for the MC. But it doesn't really matter > + * since syncing works for the EMC drivers, hence the we can sync > + * the MC driver by ourselves and then EMC will complete syncing of > + * the whole ICC state. > + */ > + icc_sync_state(mc->dev); > + > + return 0; > + > +remove_nodes: > + icc_nodes_remove(&mc->provider); > +del_provider: > + icc_provider_del(&mc->provider); > +err_msg: > + dev_err(mc->dev, "failed to initialize ICC: %d\n", err); > + > + return err; > +} > + > static int tegra_mc_probe(struct platform_device *pdev) > { > struct resource *res; > @@ -747,6 +874,8 @@ static int tegra_mc_probe(struct platform_device *pdev) > } > } > > + tegra_mc_interconnect_setup(mc); Do you want to check the return value here for errors? If not, might as well make the function return void. > + > return 0; > } > > diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h > index afa3ba45c9e6..abeb6a2cc36a 100644 > --- a/drivers/memory/tegra/mc.h > +++ b/drivers/memory/tegra/mc.h > @@ -115,4 +115,12 @@ extern const struct tegra_mc_soc tegra132_mc_soc; > extern const struct tegra_mc_soc tegra210_mc_soc; > #endif > > +/* > + * These IDs are for internal use of Tegra's ICC, the values are chosen > + * such that they don't conflict with the device-tree ICC node IDs. > + */ > +#define TEGRA_ICC_EMC 1000 > +#define TEGRA_ICC_EMEM 2000 > +#define TEGRA_ICC_MC 3000 Sounds to me like these could equally well be 1000, 1001 and 1002. Why leave these large holes in the number space? Thierry
27.10.2020 16:48, Thierry Reding пишет: ... >> +static struct icc_node_data * >> +tegra_mc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data) >> +{ >> + struct icc_provider *provider = data; >> + unsigned int idx = spec->args[0]; >> + struct icc_node_data *ndata; >> + struct icc_node *node; >> + >> + list_for_each_entry(node, &provider->nodes, node_list) { >> + if (node->id != idx) >> + continue; >> + >> + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL); >> + if (!ndata) >> + return ERR_PTR(-ENOMEM); >> + >> + ndata->node = node; >> + >> + /* these clients are isochronous by default on all SoCs */ >> + if (strstarts(node->name, "display") || >> + strstarts(node->name, "ptc") || >> + strstarts(node->name, "vi")) >> + ndata->tag = TEGRA_MC_ICC_TAG_ISO; > > This looks like something that might be better left to the drivers to > decide. Doing this here seems okay for now, but I suspect that this will > get fairly complicated to keep accurate as we add more clients later on. It's not a problem to add a driver-specific hook for the xlate_extended(), like it's done for the aggregate() and set() hooks below. ... >> +static int tegra_mc_interconnect_setup(struct tegra_mc *mc) >> +{ >> + struct icc_node *node; >> + unsigned int i; >> + int err; >> + >> + /* older device-trees don't have interconnect properties */ >> + if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL) || >> + !mc->soc->icc_ops) >> + return 0; > > This indicates that this property is indeed optional, so the bindings > should reflect that. Yes, but the property isn't optional for the newer binding. Does it really need to be documented as optional? >> + mc->provider.dev = mc->dev; >> + 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 = tegra_mc_of_icc_xlate_extended; >> + >> + err = icc_provider_add(&mc->provider); >> + if (err) >> + goto err_msg; >> + >> + /* create Memory Controller node */ >> + node = icc_node_create(TEGRA_ICC_MC); >> + err = PTR_ERR_OR_ZERO(node); >> + if (err) >> + goto del_provider; >> + >> + node->name = "Memory Controller"; >> + icc_node_add(node, &mc->provider); >> + >> + /* link Memory Controller to External Memory Controller */ >> + err = icc_link_create(node, TEGRA_ICC_EMC); >> + if (err) >> + goto remove_nodes; >> + >> + for (i = 0; i < mc->soc->num_clients; i++) { >> + /* create MC client node */ >> + node = icc_node_create(mc->soc->clients[i].id); >> + err = PTR_ERR_OR_ZERO(node); >> + if (err) >> + goto remove_nodes; >> + >> + node->name = mc->soc->clients[i].name; >> + icc_node_add(node, &mc->provider); > > I'm not fully familiar with how these nodes are set up, but would it be > possible to set the isochronous tag here already? I'd still prefer this > to be up to the drivers because I think that nicely localizes the > device-specific information in the driver, but if that's not an option, > then doing it here, based on lookup data from the MC clients table > sounds like the next best thing. The tag needs to be set by xlate_extended(), otherwise it won't be applied by default. https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/interconnect/core.c#L501 ... >> static int tegra_mc_probe(struct platform_device *pdev) >> { >> struct resource *res; >> @@ -747,6 +874,8 @@ static int tegra_mc_probe(struct platform_device *pdev) >> } >> } >> >> + tegra_mc_interconnect_setup(mc); > > Do you want to check the return value here for errors? If not, might as > well make the function return void. The error won't be fatal and shouldn't block the rest functionality of the MC driver. It's possible to return void, but it's not necessary because compiler will take care of optimizing the code and to me it's more consistent to have error code returned by the function. Perhaps should be better to just add a comment telling that error skipping is intentional? ... >> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h >> index afa3ba45c9e6..abeb6a2cc36a 100644 >> --- a/drivers/memory/tegra/mc.h >> +++ b/drivers/memory/tegra/mc.h >> @@ -115,4 +115,12 @@ extern const struct tegra_mc_soc tegra132_mc_soc; >> extern const struct tegra_mc_soc tegra210_mc_soc; >> #endif >> >> +/* >> + * These IDs are for internal use of Tegra's ICC, the values are chosen >> + * such that they don't conflict with the device-tree ICC node IDs. >> + */ >> +#define TEGRA_ICC_EMC 1000 >> +#define TEGRA_ICC_EMEM 2000 >> +#define TEGRA_ICC_MC 3000 > > Sounds to me like these could equally well be 1000, 1001 and 1002. Why > leave these large holes in the number space? There is no specific reason, I can change the numbers if you want.
diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig index 9f0a96bf9ccc..b38e5255effe 100644 --- a/drivers/memory/tegra/Kconfig +++ b/drivers/memory/tegra/Kconfig @@ -3,6 +3,7 @@ config TEGRA_MC bool "NVIDIA Tegra Memory Controller support" default y depends on ARCH_TEGRA + select INTERCONNECT help This driver supports the Memory Controller (MC) hardware found on NVIDIA Tegra SoCs. diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c index 12ea2c79205a..53d61b05ebf8 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -639,6 +639,133 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data) return IRQ_HANDLED; } +static struct icc_node_data * +tegra_mc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data) +{ + struct icc_provider *provider = data; + unsigned int idx = spec->args[0]; + struct icc_node_data *ndata; + struct icc_node *node; + + list_for_each_entry(node, &provider->nodes, node_list) { + if (node->id != idx) + continue; + + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL); + if (!ndata) + return ERR_PTR(-ENOMEM); + + ndata->node = node; + + /* these clients are isochronous by default on all SoCs */ + if (strstarts(node->name, "display") || + strstarts(node->name, "ptc") || + strstarts(node->name, "vi")) + ndata->tag = TEGRA_MC_ICC_TAG_ISO; + + return ndata; + } + + pr_err("%s: invalid client index %u\n", __func__, idx); + + return ERR_PTR(-EINVAL); +} + +/* + * Memory Controller (MC) has few Memory Clients that are issuing memory + * bandwidth allocation requests to the MC interconnect provider. The MC + * provider aggregates the requests and then sends the aggregated request + * up to the External Memory Controller (EMC) interconnect provider which + * re-configures hardware interface to External Memory (EMEM) in accordance + * to the required bandwidth. Each MC interconnect node represents an + * individual Memory Client. + * + * Memory interconnect topology: + * + * +----+ + * +--------+ | | + * | TEXSRD +--->+ | + * +--------+ | | + * | | +-----+ +------+ + * ... | MC +--->+ EMC +--->+ EMEM | + * | | +-----+ +------+ + * +--------+ | | + * | DISP.. +--->+ | + * +--------+ | | + * +----+ + */ +static int tegra_mc_interconnect_setup(struct tegra_mc *mc) +{ + struct icc_node *node; + unsigned int i; + int err; + + /* older device-trees don't have interconnect properties */ + if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL) || + !mc->soc->icc_ops) + return 0; + + mc->provider.dev = mc->dev; + 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 = tegra_mc_of_icc_xlate_extended; + + err = icc_provider_add(&mc->provider); + if (err) + goto err_msg; + + /* create Memory Controller node */ + node = icc_node_create(TEGRA_ICC_MC); + err = PTR_ERR_OR_ZERO(node); + if (err) + goto del_provider; + + node->name = "Memory Controller"; + icc_node_add(node, &mc->provider); + + /* link Memory Controller to External Memory Controller */ + err = icc_link_create(node, TEGRA_ICC_EMC); + if (err) + goto remove_nodes; + + for (i = 0; i < mc->soc->num_clients; i++) { + /* create MC client node */ + node = icc_node_create(mc->soc->clients[i].id); + err = PTR_ERR_OR_ZERO(node); + if (err) + goto remove_nodes; + + node->name = mc->soc->clients[i].name; + icc_node_add(node, &mc->provider); + + /* link Memory Client to Memory Controller */ + err = icc_link_create(node, TEGRA_ICC_MC); + if (err) + goto remove_nodes; + } + + /* + * MC driver is registered too early, so early that generic driver + * syncing doesn't work for the MC. But it doesn't really matter + * since syncing works for the EMC drivers, hence the we can sync + * the MC driver by ourselves and then EMC will complete syncing of + * the whole ICC state. + */ + icc_sync_state(mc->dev); + + return 0; + +remove_nodes: + icc_nodes_remove(&mc->provider); +del_provider: + icc_provider_del(&mc->provider); +err_msg: + dev_err(mc->dev, "failed to initialize ICC: %d\n", err); + + return err; +} + static int tegra_mc_probe(struct platform_device *pdev) { struct resource *res; @@ -747,6 +874,8 @@ static int tegra_mc_probe(struct platform_device *pdev) } } + tegra_mc_interconnect_setup(mc); + return 0; } diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h index afa3ba45c9e6..abeb6a2cc36a 100644 --- a/drivers/memory/tegra/mc.h +++ b/drivers/memory/tegra/mc.h @@ -115,4 +115,12 @@ extern const struct tegra_mc_soc tegra132_mc_soc; extern const struct tegra_mc_soc tegra210_mc_soc; #endif +/* + * These IDs are for internal use of Tegra's ICC, the values are chosen + * such that they don't conflict with the device-tree ICC node IDs. + */ +#define TEGRA_ICC_EMC 1000 +#define TEGRA_ICC_EMEM 2000 +#define TEGRA_ICC_MC 3000 + #endif /* MEMORY_TEGRA_MC_H */ diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 1b7dfed6afb8..09b3fe30c8e7 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -7,6 +7,7 @@ #define __SOC_TEGRA_MC_H__ #include <linux/err.h> +#include <linux/interconnect-provider.h> #include <linux/reset-controller.h> #include <linux/types.h> @@ -141,6 +142,17 @@ struct tegra_mc_reset_ops { const struct tegra_mc_reset *rst); }; +enum terga_mc_icc_tag { + TEGRA_MC_ICC_TAG_DEFAULT, + TEGRA_MC_ICC_TAG_ISO, +}; + +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 tegra_mc_soc { const struct tegra_mc_client *clients; unsigned int num_clients; @@ -160,6 +172,8 @@ struct tegra_mc_soc { const struct tegra_mc_reset_ops *reset_ops; const struct tegra_mc_reset *resets; unsigned int num_resets; + + const struct tegra_mc_icc_ops *icc_ops; }; struct tegra_mc { @@ -178,6 +192,8 @@ struct tegra_mc { struct reset_controller_dev reset; + struct icc_provider provider; + spinlock_t lock; };