Re: [PATCH] memory: tegra: add multi-socket support to the memory interconnect

[Sumit Gupta]

On 29/05/26 20:53, Thierry Reding wrote:
> On Fri, May 29, 2026 at 02:55:35PM +0530, Sumit Gupta wrote:
> > On 28/05/26 18:35, Thierry Reding wrote:
> > > On Thu, May 28, 2026 at 02:20:07PM +0200, Krzysztof Kozlowski wrote:
> > > > On 28/05/2026 13:56, Thierry Reding wrote:
> > > > > > > > > -     mc->debugfs.root = debugfs_create_dir("mc", NULL);
> > > > > > > > > +     if (!mc_debugfs_root)
> > > > > > > > That's a probe path and you created a singletone. Looks like preventing
> > > > > > > > async probing for no real reason.
> > > > > > > >
> > > > > > > > I am very against singletons and debugfs does not look like justified
> > > > > > > > exception.
> > > > > > > The singleton was added so multi-socket MC/EMC instances could
> > > > > > > share a "mc"/"emc" parent. I'll drop it in v2.
> > > > > > >
> > > > > > > On single-socket SoCs, the "mc"/"emc" names will be unchanged.
> > > > > > > On multi-socket SoCs, each instance will create a top-level debugfs
> > > > > > > dir named with dev_name(). Same pattern in tegra186-emc.c.
> > > > > > >
> > > > > > >      if (dev_to_node(mc->dev) == NUMA_NO_NODE)
> > > > > > >          mc->debugfs.root = debugfs_create_dir("mc", NULL);
> > > > > > >      else
> > > > > > >          mc->debugfs.root = debugfs_create_dir(dev_name(mc->dev), NULL);
> > > > > > You assume this is fully synced, so you as well could do a look up and
> > > > > > then use what you found or create new dir. If you think that is racy, so
> > > > > > is this approach... How are other drivers handling per-device debugfs
> > > > > > directories? Do they also create such in the top-level? I think no.
> > > > > I think we want a top-level directory for a bit more structure in
> > > > > debugfs. But I also think we want to create that top-level directory in
> > > > > the module's init function rather than _probe.
> > > > I was thinking about this as well but that would mean your driver will
> > > > create it on every multi-arch kernel.
> > > >
> > > > This should be then moved to some core bus (and there are examples of
> > > > that, e.g. USB), except there is no core-MC bus code to do that.
> > > We have a utility function (soc_is_tegra()) that we've used in similar
> > > situations in the past. We haven't used them in a little while, but it
> > > could be useful here. It's not for free, but should be fairly quick to
> > > error out early on multi-arch kernels.
> > >
> > > Thierry
> > soc_is_tegra()'s match table currently has entries up to Tegra210
> > (seems only used by a legacy 32-bit ARM path), so it would skip
> > the SoCs this patch targets (Tegra186+).
> > Could we follow tegra_init_soc() in fuse-tegra.c. Only create the
> > "mc"/"emc" parent at module init when a matching DT node is present:
> >
> >      static int __init tegra_mc_init(void)
> >      {
> >          struct device_node *np;
> >
> >          np = of_find_matching_node(NULL, tegra_mc_of_match);
> >          if (np) {
> >              tegra_mc_debugfs_root = debugfs_create_dir("mc", NULL);
> >              of_node_put(np);
> >          }
> >
> >          return platform_driver_register(&tegra_mc_driver);
> >      }
> >      arch_initcall(tegra_mc_init);
> >
> > Each probe just creates its per-device child under that parent
> > without touching any shared state. Same in tegra186-emc.c.
> > Or would you prefer a different approach, e.g. extending the
> > soc_is_tegra() match table for arm64 SoCs, before I respin?
> of_find_matching_node() has the big downside that it scans the entire
> device tree to find matching nodes, which can be quite expensive. The
> soc_is_tegra() helper uses of_machine_device_match() which only checks
> the compatible string on the root node and therefore is much faster.

Good point. Will skip that approach.

> Jon is working on a different solution for a similar case for BPMP, so
> maybe we can also look at doing something similar for MC. It's a bit
> involved because it adds an extra mutex to make sure we don't end up
> racing the creation of the parent directory, but it has the advantage
> that it's only ever triggered when really needed and doesn't need any
> "tricks" like soc_is_tegra().

Sure, will do a similar change as Jon's v3 [1] for debugfs.
[1] 
https://lore.kernel.org/linux-tegra/20260529173337.155722-2-jonathanh@xxxxxxxxxx/

> I wonder if maybe a helper could be extracted from that to make this a
> bit easier to replicate elsewhere.
>
> Thierry

Will do it as a follow up if we have more users.

Thank you,
Sumit Gupta