On 07/05/23 14:41, Krzysztof Kozlowski wrote:
External email: Use caution opening links or attachments
On 24/04/2023 17:21, Sumit Gupta wrote:
On 24/04/23 20:30, Sumit Gupta wrote:
On 24/04/23 19:18, Krzysztof Kozlowski wrote:
External email: Use caution opening links or attachments
On 24/04/2023 15:13, Sumit Gupta wrote:
Add 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 is:
MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM
Thank you for your patch. There is something to discuss/improve.
+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;
+
+ 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;
+
+ icc_provider_init(&emc->provider);
+
+ /* create External Memory Controller node */
+ node = icc_node_create(TEGRA_ICC_EMC);
+ if (IS_ERR(node)) {
+ err = PTR_ERR(node);
+ goto err_msg;
+ }
+
+ 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);
+
+ err = icc_provider_register(&emc->provider);
+ if (err)
+ goto remove_nodes;
+
+ return 0;
+
+remove_nodes:
+ icc_nodes_remove(&emc->provider);
+err_msg:
+ dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
+
+ return err;
+}
+
static int tegra186_emc_probe(struct platform_device *pdev)
{
+ struct tegra_mc *mc = dev_get_drvdata(pdev->dev.parent);
struct mrq_emc_dvfs_latency_response response;
struct tegra_bpmp_message msg;
struct tegra186_emc *emc;
@@ -236,6 +339,29 @@ 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);
+ if (mc && mc->soc->icc_ops) {
+ /*
+ * Initialize the ICC even if BPMP-FW doesn't support
'MRQ_BWMGR_INT'.
+ * Use the flag 'mc->bwmgr_mrq_supported' within MC
driver and return
+ * EINVAL instead of passing the request to BPMP-FW
later when the BW
+ * request is made by client with 'icc_set_bw()' call.
+ */
+ err = tegra_emc_interconnect_init(emc);
+ if (err)
+ goto put_bpmp;
+
+ if (tegra_bpmp_mrq_is_supported(emc->bpmp,
MRQ_BWMGR_INT)) {
+ mc->bwmgr_mrq_supported = true;
+
+ /*
+ * MC driver probe can't get BPMP reference as
it gets probed
+ * earlier than BPMP. So, save the BPMP ref got
from the EMC
+ * DT node in the mc->bpmp and use it in MC's
icc_set hook.
+ */
+ mc->bpmp = emc->bpmp;
This (and ()) are called without any locking. You register first the
interconnect, so set() callback can be used, right? Then set() could be
called anytime between tegra_emc_interconnect_init() and assignment
above. How do you synchronize these?
Best regards,
Krzysztof
Currently, the tegra234_mc_icc_set() has NULL check. So, it will give
this error.
if (!mc->bpmp) {
How does it solve concurrent accesses and re-ordering of instructions by
compiler or CPU?
Now, the "mc->bpmp" is set before tegra_emc_interconnect_init().
So, until the EMC interconnect initializes, set() won't be
called as the devm_of_icc_get() call will fail.
Added the change in v8.
Thank you,
Sumit Gupta
dev_err(mc->dev, "BPMP reference NULL\n");
return -ENOENT;
}
To fix, we can do the below change and swap the order. Please let me
know if this sounds fine ?
if (mc && mc->soc->icc_ops) {
if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
mc->bwmgr_mrq_supported = true;
mc->bpmp = emc->bpmp;
}
err = tegra_emc_interconnect_init(emc);
if (err)
goto put_bpmp;
}
Sorry, replied too soon. Missed setting "mc->bpmp" to NULL. Sharing new
proposed change. Please let me know if this looks OK.
if (mc && mc->soc->icc_ops) {
if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
mc->bwmgr_mrq_supported = true;
mc->bpmp = emc->bpmp;
}
err = tegra_emc_interconnect_init(emc);
if (err) {
mc->bpmp = NULL;
goto put_bpmp;
Sorry, I didn't check the code. I assume you should do it...
Best regards,
Krzysztof
