[RFC 0/3] Introduce memory controller mini-framework

[RFC 0/3] Introduce memory controller mini-framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this series proposes the introduction of a mini-framework for memory
controllers. The primary use-case for this right now is to allow for
drivers that depend on the memory controller to defer probe until the
memory controller has been successfully registered.

One example where this is needed is on Tegra186 and later SoCs where
the memory controller needs to program some registers to associate a
stream ID with memory clients. This requires that the IOMMU driver
defers probe until the memory controller has been registered.

This is achieved by providing a trivial memory controller registry that
can be queried.

I haven't written up a full device tree binding for this, but if people
think this is a reasonable proposal, I can flesh things out. Currently I
use something along these lines on Tegra186:

	mc: memory-controller@2c00000 {
		...
		#memory-controller-cells = <0>;
		...
	};

	iommu@12000000 {
		...
		memory-controllers = <&mc>;
		...
	};

Thierry

Thierry Reding (3):
  memory: Introduce memory controller mini-framework
  memory: tegra186: Register as memory controller
  iommu: arm-smmu: Get reference to memory controller

 drivers/iommu/arm-smmu.c          | 12 ++++
 drivers/iommu/arm-smmu.h          |  2 +
 drivers/memory/Makefile           |  1 +
 drivers/memory/core.c             | 99 +++++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra186.c   |  8 ++-
 include/linux/memory-controller.h | 25 ++++++++
 6 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 drivers/memory/core.c
 create mode 100644 include/linux/memory-controller.h

-- 
2.23.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC 1/3] memory: Introduce memory controller mini-framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This new framework is currently nothing more than a registry of memory
controllers, with the goal being to order device probing. One use-case
where this is useful, for example, is a memory controller device which
needs to program some registers before the system MMU can be enabled.
Associating the memory controller with the SMMU allows the SMMU driver
to defer the probe until the memory controller has been registered.

One such example is Tegra186 where the memory controller contains some
registers that are used to program stream IDs for the various memory
clients (display, USB, PCI, ...) in the system. Programming these SIDs
is required for the memory clients to emit the proper SIDs as part of
their memory requests. The memory controller driver therefore needs to
be programmed prior to the SMMU driver. To achieve that, the memory
controller will be referenced via phandle from the SMMU device tree
node, the SMMU driver can then use the memory controller framework to
find it and defer probe until it has been registered.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/memory/Makefile           |  1 +
 drivers/memory/core.c             | 99 +++++++++++++++++++++++++++++++
 include/linux/memory-controller.h | 25 ++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 drivers/memory/core.c
 create mode 100644 include/linux/memory-controller.h

diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 27b493435e61..d16e7dca8ef9 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -3,6 +3,7 @@
 # Makefile for memory devices
 #
 
+obj-y				+= core.o
 obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
 ifeq ($(CONFIG_DDR),y)
 obj-$(CONFIG_OF)		+= of_memory.o
diff --git a/drivers/memory/core.c b/drivers/memory/core.c
new file mode 100644
index 000000000000..1772e839305a
--- /dev/null
+++ b/drivers/memory/core.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 NVIDIA Corporation.
+ */
+
+#include <linux/memory-controller.h>
+#include <linux/of.h>
+
+static DEFINE_MUTEX(controllers_lock);
+static LIST_HEAD(controllers);
+
+static void memory_controller_release(struct kref *ref)
+{
+	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
+
+	WARN_ON(!list_empty(&mc->list));
+}
+
+int memory_controller_register(struct memory_controller *mc)
+{
+	kref_init(&mc->ref);
+
+	mutex_lock(&controllers_lock);
+	list_add_tail(&mc->list, &controllers);
+	mutex_unlock(&controllers_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(memory_controller_register);
+
+void memory_controller_unregister(struct memory_controller *mc)
+{
+	mutex_lock(&controllers_lock);
+	list_del_init(&mc->list);
+	mutex_unlock(&controllers_lock);
+
+	kref_put(&mc->ref, memory_controller_release);
+}
+EXPORT_SYMBOL_GPL(memory_controller_unregister);
+
+static struct memory_controller *
+of_memory_controller_get(struct device *dev, struct device_node *np,
+			 const char *con_id)
+{
+	const char *cells = "#memory-controller-cells";
+	const char *names = "memory-controller-names";
+	const char *prop = "memory-controllers";
+	struct memory_controller *mc;
+	struct of_phandle_args args;
+	int index = 0, err;
+
+	if (con_id) {
+		index = of_property_match_string(np, names, con_id);
+		if (index < 0)
+			return ERR_PTR(index);
+	}
+
+	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
+	if (err) {
+		if (err == -ENOENT)
+			err = -ENODEV;
+
+		return ERR_PTR(err);
+	}
+
+	mutex_lock(&controllers_lock);
+
+	list_for_each_entry(mc, &controllers, list) {
+		if (mc->dev && mc->dev->of_node == args.np) {
+			kref_get(&mc->ref);
+			mutex_unlock(&controllers_lock);
+			goto unlock;
+		}
+	}
+
+	mc = ERR_PTR(-EPROBE_DEFER);
+
+unlock:
+	mutex_unlock(&controllers_lock);
+	of_node_put(args.np);
+	return mc;
+}
+
+struct memory_controller *
+memory_controller_get(struct device *dev, const char *con_id)
+{
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		return of_memory_controller_get(dev, dev->of_node, con_id);
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL_GPL(memory_controller_get);
+
+void memory_controller_put(struct memory_controller *mc)
+{
+	if (mc)
+		kref_put(&mc->ref, memory_controller_release);
+}
+EXPORT_SYMBOL_GPL(memory_controller_put);
diff --git a/include/linux/memory-controller.h b/include/linux/memory-controller.h
new file mode 100644
index 000000000000..4b06b2ea1d14
--- /dev/null
+++ b/include/linux/memory-controller.h
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 NVIDIA Corporation.
+ */
+
+#ifndef _LINUX_MEMORY_CONTROLLER_H
+#define _LINUX_MEMORY_CONTROLLER_H
+
+#include <linux/device.h>
+#include <linux/list.h>
+
+struct memory_controller {
+	struct device *dev;
+	struct kref ref;
+	struct list_head list;
+};
+
+int memory_controller_register(struct memory_controller *mc);
+void memory_controller_unregister(struct memory_controller *mc);
+
+struct memory_controller *memory_controller_get(struct device *dev,
+						const char *con_id);
+void memory_controller_put(struct memory_controller *mc);
+
+#endif
-- 
2.23.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC 2/3] memory: tegra186: Register as memory controller

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Registering as memory controller allows other drivers to obtain a
reference to it. This is mostly useful as a way of ordering probe
between devices depending on one another.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/memory/tegra/tegra186.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 441213a35930..e94e960a79f4 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/memory-controller.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
@@ -11,6 +12,7 @@
 #include <dt-bindings/memory/tegra186-mc.h>
 
 struct tegra_mc {
+	struct memory_controller base;
 	struct device *dev;
 	void __iomem *regs;
 };
@@ -548,7 +550,7 @@ static int tegra186_mc_probe(struct platform_device *pdev)
 	if (IS_ERR(mc->regs))
 		return PTR_ERR(mc->regs);
 
-	mc->dev = &pdev->dev;
+	mc->base.dev = &pdev->dev;
 
 	for (i = 0; i < ARRAY_SIZE(tegra186_mc_clients); i++) {
 		const struct tegra_mc_client *client = &tegra186_mc_clients[i];
@@ -571,6 +573,10 @@ static int tegra186_mc_probe(struct platform_device *pdev)
 			client->name, override, security);
 	}
 
+	err = memory_controller_register(&mc->base);
+	if (err < 0)
+		return err;
+
 	platform_set_drvdata(pdev, mc);
 
 	return err;
-- 
2.23.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC 3/3] iommu: arm-smmu: Get reference to memory controller

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use the memory controller framework to obtain a reference to the memory
controller to which the SMMU will make memory requests. This allows the
two drivers to properly order their probes so that the memory controller
can be programmed first.

An example where this is required is Tegra186 where the stream IDs need
to be associated with memory clients before memory requests are emitted
with the correct stream ID.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/iommu/arm-smmu.c | 12 ++++++++++++
 drivers/iommu/arm-smmu.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b18aac4c105e..8dd214244926 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2015,6 +2015,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	smmu->mc = memory_controller_get(dev, NULL);
+	if (IS_ERR(smmu->mc)) {
+		err = PTR_ERR(smmu->mc);
+
+		if (err != -ENODEV) {
+			dev_err(dev, "failed to get memory controller: %d\n", err);
+			return err;
+		}
+
+		smmu->mc = NULL;
+	}
+
 	if (dev->of_node)
 		err = arm_smmu_device_dt_probe(pdev, smmu);
 	else
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index b19b6cae9b5e..40b6d42eb3ab 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -17,6 +17,7 @@
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/io-pgtable.h>
 #include <linux/iommu.h>
+#include <linux/memory-controller.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
@@ -224,6 +225,7 @@ enum arm_smmu_implementation {
 
 struct arm_smmu_device {
 	struct device			*dev;
+	struct memory_controller	*mc;
 
 	void __iomem			*base;
 	unsigned int			numpage;
-- 
2.23.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC 1/3] memory: Introduce memory controller mini-framework

[Dmitry Osipenko]

15.10.2019 19:29, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> This new framework is currently nothing more than a registry of memory
> controllers, with the goal being to order device probing. One use-case
> where this is useful, for example, is a memory controller device which
> needs to program some registers before the system MMU can be enabled.
> Associating the memory controller with the SMMU allows the SMMU driver
> to defer the probe until the memory controller has been registered.
> 
> One such example is Tegra186 where the memory controller contains some
> registers that are used to program stream IDs for the various memory
> clients (display, USB, PCI, ...) in the system. Programming these SIDs
> is required for the memory clients to emit the proper SIDs as part of
> their memory requests. The memory controller driver therefore needs to
> be programmed prior to the SMMU driver. To achieve that, the memory
> controller will be referenced via phandle from the SMMU device tree
> node, the SMMU driver can then use the memory controller framework to
> find it and defer probe until it has been registered.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/memory/Makefile           |  1 +
>  drivers/memory/core.c             | 99 +++++++++++++++++++++++++++++++
>  include/linux/memory-controller.h | 25 ++++++++
>  3 files changed, 125 insertions(+)
>  create mode 100644 drivers/memory/core.c
>  create mode 100644 include/linux/memory-controller.h

Hello Thierry,

This looks like a very good endeavour! I have couple comments, please
see them below.

> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 27b493435e61..d16e7dca8ef9 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for memory devices
>  #
>  
> +obj-y				+= core.o
>  obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
>  ifeq ($(CONFIG_DDR),y)
>  obj-$(CONFIG_OF)		+= of_memory.o
> diff --git a/drivers/memory/core.c b/drivers/memory/core.c
> new file mode 100644
> index 000000000000..1772e839305a
> --- /dev/null
> +++ b/drivers/memory/core.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 NVIDIA Corporation.
> + */
> +
> +#include <linux/memory-controller.h>
> +#include <linux/of.h>
> +
> +static DEFINE_MUTEX(controllers_lock);
> +static LIST_HEAD(controllers);
> +
> +static void memory_controller_release(struct kref *ref)
> +{
> +	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
> +
> +	WARN_ON(!list_empty(&mc->list));
> +}
> +
> +int memory_controller_register(struct memory_controller *mc)
> +{
> +	kref_init(&mc->ref);
> +
> +	mutex_lock(&controllers_lock);
> +	list_add_tail(&mc->list, &controllers);
> +	mutex_unlock(&controllers_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_register);
> +
> +void memory_controller_unregister(struct memory_controller *mc)
> +{
> +	mutex_lock(&controllers_lock);
> +	list_del_init(&mc->list);
> +	mutex_unlock(&controllers_lock);
> +
> +	kref_put(&mc->ref, memory_controller_release);
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_unregister);
> +
> +static struct memory_controller *
> +of_memory_controller_get(struct device *dev, struct device_node *np,
> +			 const char *con_id)
> +{
> +	const char *cells = "#memory-controller-cells";
> +	const char *names = "memory-controller-names";
> +	const char *prop = "memory-controllers";
> +	struct memory_controller *mc;
> +	struct of_phandle_args args;
> +	int index = 0, err;
> +
> +	if (con_id) {
> +		index = of_property_match_string(np, names, con_id);
> +		if (index < 0)
> +			return ERR_PTR(index);
> +	}
> +
> +	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
> +	if (err) {
> +		if (err == -ENOENT)
> +			err = -ENODEV;
> +
> +		return ERR_PTR(err);
> +	}
> +
> +	mutex_lock(&controllers_lock);
> +
> +	list_for_each_entry(mc, &controllers, list) {
> +		if (mc->dev && mc->dev->of_node == args.np) {
> +			kref_get(&mc->ref);

This is not enough because memory controller driver could be a loadable
module, thus something like this is needed here:

	__module_get(mc->dev->driver->owner);

This won't allow MC driver to be unloaded while it has active users.

> +			mutex_unlock(&controllers_lock);
> +			goto unlock;
> +		}
> +	}
> +
> +	mc = ERR_PTR(-EPROBE_DEFER);
> +
> +unlock:
> +	mutex_unlock(&controllers_lock);
> +	of_node_put(args.np);
> +	return mc;
> +}
> +
> +struct memory_controller *
> +memory_controller_get(struct device *dev, const char *con_id)
> +{
> +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +		return of_memory_controller_get(dev, dev->of_node, con_id);
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_get);

In most cases memory controllers are unique in a system, so it looks to
me that it will be more universal to have ability to get MC by its
device-tree compatible name. Like this:

	of_memory_controller_get_by_compatible(const char *compatible);

This will allow current drivers (like Tegra20 devfreq driver for
example) to utilize this new API without having trouble of maintaining
backwards compatibility with older device-trees that do not have a
phandle to MC.

https://elixir.bootlin.com/linux/v5.4-rc5/source/drivers/devfreq/tegra20-devfreq.c#L100

Of course there could be cases where there are multiple controllers with
the same compatible, but that case could be supported later on by those
who really need it. I don't think that any of NVIDIA Tegra SoCs fall
into that category.

> +void memory_controller_put(struct memory_controller *mc)
> +{
> +	if (mc)
> +		kref_put(&mc->ref, memory_controller_release);
		module_put(mc->dev->driver->owner);

> +}
> +EXPORT_SYMBOL_GPL(memory_controller_put);


[snip]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC 1/3] memory: Introduce memory controller mini-framework

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7815 bytes --]

On Thu, Oct 31, 2019 at 06:11:33PM +0300, Dmitry Osipenko wrote:
> 15.10.2019 19:29, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This new framework is currently nothing more than a registry of memory
> > controllers, with the goal being to order device probing. One use-case
> > where this is useful, for example, is a memory controller device which
> > needs to program some registers before the system MMU can be enabled.
> > Associating the memory controller with the SMMU allows the SMMU driver
> > to defer the probe until the memory controller has been registered.
> > 
> > One such example is Tegra186 where the memory controller contains some
> > registers that are used to program stream IDs for the various memory
> > clients (display, USB, PCI, ...) in the system. Programming these SIDs
> > is required for the memory clients to emit the proper SIDs as part of
> > their memory requests. The memory controller driver therefore needs to
> > be programmed prior to the SMMU driver. To achieve that, the memory
> > controller will be referenced via phandle from the SMMU device tree
> > node, the SMMU driver can then use the memory controller framework to
> > find it and defer probe until it has been registered.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/memory/Makefile           |  1 +
> >  drivers/memory/core.c             | 99 +++++++++++++++++++++++++++++++
> >  include/linux/memory-controller.h | 25 ++++++++
> >  3 files changed, 125 insertions(+)
> >  create mode 100644 drivers/memory/core.c
> >  create mode 100644 include/linux/memory-controller.h
> 
> Hello Thierry,
> 
> This looks like a very good endeavour! I have couple comments, please
> see them below.
> 
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index 27b493435e61..d16e7dca8ef9 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -3,6 +3,7 @@
> >  # Makefile for memory devices
> >  #
> >  
> > +obj-y				+= core.o
> >  obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
> >  ifeq ($(CONFIG_DDR),y)
> >  obj-$(CONFIG_OF)		+= of_memory.o
> > diff --git a/drivers/memory/core.c b/drivers/memory/core.c
> > new file mode 100644
> > index 000000000000..1772e839305a
> > --- /dev/null
> > +++ b/drivers/memory/core.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 NVIDIA Corporation.
> > + */
> > +
> > +#include <linux/memory-controller.h>
> > +#include <linux/of.h>
> > +
> > +static DEFINE_MUTEX(controllers_lock);
> > +static LIST_HEAD(controllers);
> > +
> > +static void memory_controller_release(struct kref *ref)
> > +{
> > +	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
> > +
> > +	WARN_ON(!list_empty(&mc->list));
> > +}
> > +
> > +int memory_controller_register(struct memory_controller *mc)
> > +{
> > +	kref_init(&mc->ref);
> > +
> > +	mutex_lock(&controllers_lock);
> > +	list_add_tail(&mc->list, &controllers);
> > +	mutex_unlock(&controllers_lock);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_register);
> > +
> > +void memory_controller_unregister(struct memory_controller *mc)
> > +{
> > +	mutex_lock(&controllers_lock);
> > +	list_del_init(&mc->list);
> > +	mutex_unlock(&controllers_lock);
> > +
> > +	kref_put(&mc->ref, memory_controller_release);
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_unregister);
> > +
> > +static struct memory_controller *
> > +of_memory_controller_get(struct device *dev, struct device_node *np,
> > +			 const char *con_id)
> > +{
> > +	const char *cells = "#memory-controller-cells";
> > +	const char *names = "memory-controller-names";
> > +	const char *prop = "memory-controllers";
> > +	struct memory_controller *mc;
> > +	struct of_phandle_args args;
> > +	int index = 0, err;
> > +
> > +	if (con_id) {
> > +		index = of_property_match_string(np, names, con_id);
> > +		if (index < 0)
> > +			return ERR_PTR(index);
> > +	}
> > +
> > +	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
> > +	if (err) {
> > +		if (err == -ENOENT)
> > +			err = -ENODEV;
> > +
> > +		return ERR_PTR(err);
> > +	}
> > +
> > +	mutex_lock(&controllers_lock);
> > +
> > +	list_for_each_entry(mc, &controllers, list) {
> > +		if (mc->dev && mc->dev->of_node == args.np) {
> > +			kref_get(&mc->ref);
> 
> This is not enough because memory controller driver could be a loadable
> module, thus something like this is needed here:
> 
> 	__module_get(mc->dev->driver->owner);
> 
> This won't allow MC driver to be unloaded while it has active users.

Good catch. I've added that (and the module_put() from below) to the
patch.

> > +			mutex_unlock(&controllers_lock);
> > +			goto unlock;
> > +		}
> > +	}
> > +
> > +	mc = ERR_PTR(-EPROBE_DEFER);
> > +
> > +unlock:
> > +	mutex_unlock(&controllers_lock);
> > +	of_node_put(args.np);
> > +	return mc;
> > +}
> > +
> > +struct memory_controller *
> > +memory_controller_get(struct device *dev, const char *con_id)
> > +{
> > +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > +		return of_memory_controller_get(dev, dev->of_node, con_id);
> > +
> > +	return ERR_PTR(-ENODEV);
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_get);
> 
> In most cases memory controllers are unique in a system, so it looks to
> me that it will be more universal to have ability to get MC by its
> device-tree compatible name. Like this:
> 
> 	of_memory_controller_get_by_compatible(const char *compatible);
> 
> This will allow current drivers (like Tegra20 devfreq driver for
> example) to utilize this new API without having trouble of maintaining
> backwards compatibility with older device-trees that do not have a
> phandle to MC.
> 
> https://elixir.bootlin.com/linux/v5.4-rc5/source/drivers/devfreq/tegra20-devfreq.c#L100
> 
> Of course there could be cases where there are multiple controllers with
> the same compatible, but that case could be supported later on by those
> who really need it. I don't think that any of NVIDIA Tegra SoCs fall
> into that category.

This has the slight disadvantage that we would have to iterate over a
number of compatible strings in case we want to transparently support
more than a single version of the memory controller.

An alternative, which is used by a number of other resource registry
APIs, would be to work with lookup tables. Basically those would make
a mapping between a provider and a device/consumer pair. The result
would look something like this:

	struct memory_controller_lookup {
		const char *provider;
		const char *dev_id;
		const char *con_id;
	};

	static const struct memory_controller_lookup *tegra124_mc_lookup[] = {
		{ "70019000.memory-controller", "6000c800.actmon", NULL },
	};

memory_controller_get() could then use that as a last-resort to find a
reference to a memory controller if a device tree phandle isn't
available.

On the other hand it should be fairly easy to conditionalize all the
code based purely on the availability of a phandle:

	mc = memory_controller_get(dev, NULL);
	if (IS_ERR(mc)) {
		if (mc != ERR_PTR(-ENODEV))
			return PTR_ERR(mc);

		mc = NULL;
	}

	...

	if (mc) {
		...
	}

The above could be simplified by wrapping the logic in a helper that can
be used if consumers can work without: memory_controller_get_optional().

Thierry

> > +void memory_controller_put(struct memory_controller *mc)
> > +{
> > +	if (mc)
> > +		kref_put(&mc->ref, memory_controller_release);
> 		module_put(mc->dev->driver->owner);
> 
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_put);
> 
> 
> [snip]

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC 1/3] memory: Introduce memory controller mini-framework

[Dmitry Osipenko]

01.11.2019 13:18, Thierry Reding пишет:
> On Thu, Oct 31, 2019 at 06:11:33PM +0300, Dmitry Osipenko wrote:
>> 15.10.2019 19:29, Thierry Reding пишет:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> This new framework is currently nothing more than a registry of memory
>>> controllers, with the goal being to order device probing. One use-case
>>> where this is useful, for example, is a memory controller device which
>>> needs to program some registers before the system MMU can be enabled.
>>> Associating the memory controller with the SMMU allows the SMMU driver
>>> to defer the probe until the memory controller has been registered.
>>>
>>> One such example is Tegra186 where the memory controller contains some
>>> registers that are used to program stream IDs for the various memory
>>> clients (display, USB, PCI, ...) in the system. Programming these SIDs
>>> is required for the memory clients to emit the proper SIDs as part of
>>> their memory requests. The memory controller driver therefore needs to
>>> be programmed prior to the SMMU driver. To achieve that, the memory
>>> controller will be referenced via phandle from the SMMU device tree
>>> node, the SMMU driver can then use the memory controller framework to
>>> find it and defer probe until it has been registered.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/memory/Makefile           |  1 +
>>>  drivers/memory/core.c             | 99 +++++++++++++++++++++++++++++++
>>>  include/linux/memory-controller.h | 25 ++++++++
>>>  3 files changed, 125 insertions(+)
>>>  create mode 100644 drivers/memory/core.c
>>>  create mode 100644 include/linux/memory-controller.h
>>
>> Hello Thierry,
>>
>> This looks like a very good endeavour! I have couple comments, please
>> see them below.
>>
>>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>>> index 27b493435e61..d16e7dca8ef9 100644
>>> --- a/drivers/memory/Makefile
>>> +++ b/drivers/memory/Makefile
>>> @@ -3,6 +3,7 @@
>>>  # Makefile for memory devices
>>>  #
>>>  
>>> +obj-y				+= core.o
>>>  obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
>>>  ifeq ($(CONFIG_DDR),y)
>>>  obj-$(CONFIG_OF)		+= of_memory.o
>>> diff --git a/drivers/memory/core.c b/drivers/memory/core.c
>>> new file mode 100644
>>> index 000000000000..1772e839305a
>>> --- /dev/null
>>> +++ b/drivers/memory/core.c
>>> @@ -0,0 +1,99 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2019 NVIDIA Corporation.
>>> + */
>>> +
>>> +#include <linux/memory-controller.h>
>>> +#include <linux/of.h>
>>> +
>>> +static DEFINE_MUTEX(controllers_lock);
>>> +static LIST_HEAD(controllers);
>>> +
>>> +static void memory_controller_release(struct kref *ref)
>>> +{
>>> +	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
>>> +
>>> +	WARN_ON(!list_empty(&mc->list));
>>> +}
>>> +
>>> +int memory_controller_register(struct memory_controller *mc)
>>> +{
>>> +	kref_init(&mc->ref);
>>> +
>>> +	mutex_lock(&controllers_lock);
>>> +	list_add_tail(&mc->list, &controllers);
>>> +	mutex_unlock(&controllers_lock);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(memory_controller_register);
>>> +
>>> +void memory_controller_unregister(struct memory_controller *mc)
>>> +{
>>> +	mutex_lock(&controllers_lock);
>>> +	list_del_init(&mc->list);
>>> +	mutex_unlock(&controllers_lock);
>>> +
>>> +	kref_put(&mc->ref, memory_controller_release);
>>> +}
>>> +EXPORT_SYMBOL_GPL(memory_controller_unregister);
>>> +
>>> +static struct memory_controller *
>>> +of_memory_controller_get(struct device *dev, struct device_node *np,
>>> +			 const char *con_id)
>>> +{
>>> +	const char *cells = "#memory-controller-cells";
>>> +	const char *names = "memory-controller-names";
>>> +	const char *prop = "memory-controllers";
>>> +	struct memory_controller *mc;
>>> +	struct of_phandle_args args;
>>> +	int index = 0, err;
>>> +
>>> +	if (con_id) {
>>> +		index = of_property_match_string(np, names, con_id);
>>> +		if (index < 0)
>>> +			return ERR_PTR(index);
>>> +	}
>>> +
>>> +	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
>>> +	if (err) {
>>> +		if (err == -ENOENT)
>>> +			err = -ENODEV;
>>> +
>>> +		return ERR_PTR(err);
>>> +	}
>>> +
>>> +	mutex_lock(&controllers_lock);
>>> +
>>> +	list_for_each_entry(mc, &controllers, list) {
>>> +		if (mc->dev && mc->dev->of_node == args.np) {
>>> +			kref_get(&mc->ref);
>>
>> This is not enough because memory controller driver could be a loadable
>> module, thus something like this is needed here:
>>
>> 	__module_get(mc->dev->driver->owner);
>>
>> This won't allow MC driver to be unloaded while it has active users.
> 
> Good catch. I've added that (and the module_put() from below) to the
> patch.
> 
>>> +			mutex_unlock(&controllers_lock);
>>> +			goto unlock;
>>> +		}
>>> +	}
>>> +
>>> +	mc = ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +unlock:
>>> +	mutex_unlock(&controllers_lock);
>>> +	of_node_put(args.np);
>>> +	return mc;
>>> +}
>>> +
>>> +struct memory_controller *
>>> +memory_controller_get(struct device *dev, const char *con_id)
>>> +{
>>> +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>>> +		return of_memory_controller_get(dev, dev->of_node, con_id);
>>> +
>>> +	return ERR_PTR(-ENODEV);
>>> +}
>>> +EXPORT_SYMBOL_GPL(memory_controller_get);
>>
>> In most cases memory controllers are unique in a system, so it looks to
>> me that it will be more universal to have ability to get MC by its
>> device-tree compatible name. Like this:
>>
>> 	of_memory_controller_get_by_compatible(const char *compatible);
>>
>> This will allow current drivers (like Tegra20 devfreq driver for
>> example) to utilize this new API without having trouble of maintaining
>> backwards compatibility with older device-trees that do not have a
>> phandle to MC.
>>
>> https://elixir.bootlin.com/linux/v5.4-rc5/source/drivers/devfreq/tegra20-devfreq.c#L100
>>
>> Of course there could be cases where there are multiple controllers with
>> the same compatible, but that case could be supported later on by those
>> who really need it. I don't think that any of NVIDIA Tegra SoCs fall
>> into that category.
> 
> This has the slight disadvantage that we would have to iterate over a
> number of compatible strings in case we want to transparently support
> more than a single version of the memory controller.

Good point.

> An alternative, which is used by a number of other resource registry
> APIs, would be to work with lookup tables. Basically those would make
> a mapping between a provider and a device/consumer pair. The result
> would look something like this:
> 
> 	struct memory_controller_lookup {
> 		const char *provider;
> 		const char *dev_id;
> 		const char *con_id;
> 	};
> 
> 	static const struct memory_controller_lookup *tegra124_mc_lookup[] = {
> 		{ "70019000.memory-controller", "6000c800.actmon", NULL },
> 	};
> 
> memory_controller_get() could then use that as a last-resort to find a
> reference to a memory controller if a device tree phandle isn't
> available.

The explicit lookup table sounds like a good idea because it should be
usable in a case of a non-OF devices as well.

> On the other hand it should be fairly easy to conditionalize all the
> code based purely on the availability of a phandle:
> 
> 	mc = memory_controller_get(dev, NULL);
> 	if (IS_ERR(mc)) {
> 		if (mc != ERR_PTR(-ENODEV))
> 			return PTR_ERR(mc);
> 
> 		mc = NULL;
> 	}
> 
> 	...
> 
> 	if (mc) {
> 		...
> 	}
> 
> The above could be simplified by wrapping the logic in a helper that can
> be used if consumers can work without: memory_controller_get_optional().

Optional retrieval helpers are a common thing among subsystem APIs.
Although it probably shouldn't be necessary for the start of the MC API
and could be added later on, once there will be a real need in it.
AFAIK, none of the Tegra drivers have such a need right now.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu