Re: [PATCH RFC v6 4/9] interconnect: Add imx core driver

[Leonard Crestez <leonard.crestez@nxp.com>]

On 12.12.2019 09:29, Georgi Djakov wrote:
> Hi Leonard,
> 
> Thank you for your continuous work on the patches and sorry for not reviewing
> this earlier.
> On 11/14/19 22:09, Leonard Crestez wrote:
>> This adds support for i.MX SoC family to interconnect framework.
>>
>> Platform drivers can describe the interconnect graph and several
>> adjustment knobs where icc node bandwidth is converted to a
>> DEV_PM_QOS_MIN_FREQUENCY request.
>>
>> The interconnect provider is probed through the main NOC device and
>> other adjustable nodes on the same graph are found from a
>> fsl,scalable-nodes phandle array property.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

>> +static int imx_icc_node_init_qos(struct icc_provider *provider,
>> +				 struct icc_node *node)
>> +{
>> +	struct imx_icc_node *node_data = node->data;
>> +	struct device *dev = provider->dev;
>> +	struct device_node *dn = NULL;
>> +	struct platform_device *pdev;
>> +	int i, count;
>> +	u32 node_id;
>> +	int ret;
>> +
>> +	count = of_property_count_u32_elems(dev->of_node,
>> +					    "fsl,scalable-node-ids");
>> +	if (count < 0) {
>> +		dev_err(dev, "Failed to parse fsl,scalable-node-ids: %d\n",
>> +			count);
>> +		return count;
>> +	}
>> +
>> +	for (i = 0; i < count; i++) {
>> +		ret = of_property_read_u32_index(dev->of_node,
>> +						 "fsl,scalable-node-ids",
>> +						 i, &node_id);
>> +
>> +		if (ret < 0) {
>> +			dev_err(dev, "Failed to parse fsl,scalable-node-ids[%d]: %d\n",
>> +				i, ret);
>> +			return ret;
>> +		}
>> +		if (node_id != node->id)
>> +			continue;
>> +
>> +		dn = of_parse_phandle(dev->of_node, "fsl,scalable-nodes", i);
> 
> Why is this needed? I would expect that the interconnect provider driver already
> knows which nodes are scalable based on the platform compatible string.
> Then maybe this driver should create devfreq devices for each node that is scalable?

The scalable nodes are independent devfreq instances which are probed 
through their own DT compat strings. It's even possible to reload 
imx8m-ddrc (the driver scaling the dram controller) at runtime.

The most common solution to fetch other devices on DT systems is via 
phandles and fsl,scalable-nodes is a phandle array. Since the provider 
is platform-specific and knows the topology of the soc it could even use 
of_find_node_by_path but that seems very messy. It's also quite brittle, 
I've seen several bugs caused by DT node renaming.

This support for arbitrary "scalable nodes" might be excessively generic 
and strict DT compatibility might be difficult to maintain if too much 
is exposed. Changing per-soc driver data is otherwise easy.

In vendor tree we only ever scale the main NOC and DDRC anyway so 
equivalent functionality could be achieved with a single "fsl,ddrc" 
phandle property on the noc.

Support for scaling peripheral buses could be implemented by adding 
additional properties like "fsl,display-nic". Such a feature would need 
careful measurement on real hardware anyway.

--
Regards,
Leonard