Hi Dmitry,
On 7/2/20 02:36, Dmitry Osipenko wrote:
> 01.07.2020 20:12, Georgi Djakov пишет:
>> Hi Dmitry,
>>
>> Thank you for updating the patches!
>
> Hello, Georgi!
>
> Thank you for the review!
>
>> On 6/9/20 16:13, Dmitry Osipenko wrote:
>>> Now memory controller is a memory interconnection provider. This allows us
>>> to use interconnect API in order to change memory configuration.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>> ---
>>> drivers/memory/tegra/Kconfig | 1 +
>>> drivers/memory/tegra/mc.c | 114 +++++++++++++++++++++++++++++++++++
>>> drivers/memory/tegra/mc.h | 8 +++
>>> include/soc/tegra/mc.h | 3 +
>>> 4 files changed, 126 insertions(+)
>>>
>>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>>> index 5bf75b316a2f..7055fdef2c32 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 772aa021b5f6..7ef7ac9e103e 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -594,6 +594,118 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +static int tegra_mc_icc_set(struct icc_node *src, struct icc_node *dst)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_mc_icc_aggregate(struct icc_node *node,
>>> + u32 tag, u32 avg_bw, u32 peak_bw,
>>> + u32 *agg_avg, u32 *agg_peak)
>>> +{
>>> + *agg_avg = min((u64)avg_bw + (*agg_avg), (u64)U32_MAX);
>>> + *agg_peak = max(*agg_peak, peak_bw);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * 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_onecell_data *data;
>>> + struct icc_node *node;
>>> + unsigned int num_nodes;
>>> + unsigned int i;
>>> + int err;
>>> +
>>> + /* older device-trees don't have interconnect properties */
>>> + if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL))
>>> + return 0;
>>> +
>>> + num_nodes = mc->soc->num_clients;
>>> +
>>> + data = devm_kzalloc(mc->dev, struct_size(data, nodes, num_nodes),
>>> + GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + mc->provider.dev = mc->dev;
>>> + mc->provider.set = tegra_mc_icc_set;
>>
>> Hmm, maybe the core should not require a set() implementation and we can
>> just make it optional instead. Then the dummy function would not be needed.
>
> Eventually this dummy function might become populated with a memory
> latency allowness programming. I could add a comment into that function
> in the next version, saying that it's to-be-done for now.
Ah ok! Sounds good, thanks for clarifying!
>>> + mc->provider.data = data;
>>> + mc->provider.xlate = of_icc_xlate_onecell;
>>> + mc->provider.aggregate = tegra_mc_icc_aggregate;
>>> +
>>> + err = icc_provider_add(&mc->provider);
>>> + if (err)
>>> + goto err_msg;
>>
>> Nit: I am planning to re-organize some of the existing drivers to call
>> icc_provider_add() after the topology is populated. Could you please move
>> this after the nodes are created and linked.
>
> Are you planning to remove the provider's list-head initialization from
> the icc_provider_add() [1] and move it to the individual provider
> drivers, correct?
Yes, that would be the first step, but i need to post some patches first,
so let's keep it as-is for now. Sorry for the confusion.
Thanks,
Georgi
