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

[Dmitry Osipenko <digetx@gmail.com>]

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