Re: [PATCH v6 29/52] memory: tegra-mc: Add interconnect framework

[Dmitry Osipenko]

27.10.2020 16:48, Thierry Reding пишет:
...
>> +static struct icc_node_data *
>> +tegra_mc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data)
>> +{
>> +	struct icc_provider *provider = data;
>> +	unsigned int idx = spec->args[0];
>> +	struct icc_node_data *ndata;
>> +	struct icc_node *node;
>> +
>> +	list_for_each_entry(node, &provider->nodes, node_list) {
>> +		if (node->id != idx)
>> +			continue;
>> +
>> +		ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
>> +		if (!ndata)
>> +			return ERR_PTR(-ENOMEM);
>> +
>> +		ndata->node = node;
>> +
>> +		/* these clients are isochronous by default on all SoCs */
>> +		if (strstarts(node->name, "display") ||
>> +		    strstarts(node->name, "ptc") ||
>> +		    strstarts(node->name, "vi"))
>> +			ndata->tag = TEGRA_MC_ICC_TAG_ISO;
> 
> This looks like something that might be better left to the drivers to
> decide. Doing this here seems okay for now, but I suspect that this will
> get fairly complicated to keep accurate as we add more clients later on.

It's not a problem to add a driver-specific hook for the
xlate_extended(), like it's done for the aggregate() and set() hooks below.

...
>> +static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
>> +{
>> +	struct icc_node *node;
>> +	unsigned int i;
>> +	int err;
>> +
>> +	/* older device-trees don't have interconnect properties */
>> +	if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL) ||
>> +	    !mc->soc->icc_ops)
>> +		return 0;
> 
> This indicates that this property is indeed optional, so the bindings
> should reflect that.

Yes, but the property isn't optional for the newer binding. Does it
really need to be documented as optional?

>> +	mc->provider.dev = mc->dev;
>> +	mc->provider.data = &mc->provider;
>> +	mc->provider.set = mc->soc->icc_ops->set;
>> +	mc->provider.aggregate = mc->soc->icc_ops->aggregate;
>> +	mc->provider.xlate_extended = tegra_mc_of_icc_xlate_extended;
>> +
>> +	err = icc_provider_add(&mc->provider);
>> +	if (err)
>> +		goto err_msg;
>> +
>> +	/* create Memory Controller node */
>> +	node = icc_node_create(TEGRA_ICC_MC);
>> +	err = PTR_ERR_OR_ZERO(node);
>> +	if (err)
>> +		goto del_provider;
>> +
>> +	node->name = "Memory Controller";
>> +	icc_node_add(node, &mc->provider);
>> +
>> +	/* link Memory Controller to External Memory Controller */
>> +	err = icc_link_create(node, TEGRA_ICC_EMC);
>> +	if (err)
>> +		goto remove_nodes;
>> +
>> +	for (i = 0; i < mc->soc->num_clients; i++) {
>> +		/* create MC client node */
>> +		node = icc_node_create(mc->soc->clients[i].id);
>> +		err = PTR_ERR_OR_ZERO(node);
>> +		if (err)
>> +			goto remove_nodes;
>> +
>> +		node->name = mc->soc->clients[i].name;
>> +		icc_node_add(node, &mc->provider);
> 
> I'm not fully familiar with how these nodes are set up, but would it be
> possible to set the isochronous tag here already? I'd still prefer this
> to be up to the drivers because I think that nicely localizes the
> device-specific information in the driver, but if that's not an option,
> then doing it here, based on lookup data from the MC clients table
> sounds like the next best thing.

The tag needs to be set by xlate_extended(), otherwise it won't be
applied by default.

https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/interconnect/core.c#L501

...
>>  static int tegra_mc_probe(struct platform_device *pdev)
>>  {
>>  	struct resource *res;
>> @@ -747,6 +874,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> +	tegra_mc_interconnect_setup(mc);
> 
> Do you want to check the return value here for errors? If not, might as
> well make the function return void.

The error won't be fatal and shouldn't block the rest functionality of
the MC driver.

It's possible to return void, but it's not necessary because compiler
will take care of optimizing the code and to me it's more consistent to
have error code returned by the function.

Perhaps should be better to just add a comment telling that error
skipping is intentional?

...
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>> index afa3ba45c9e6..abeb6a2cc36a 100644
>> --- a/drivers/memory/tegra/mc.h
>> +++ b/drivers/memory/tegra/mc.h
>> @@ -115,4 +115,12 @@ extern const struct tegra_mc_soc tegra132_mc_soc;
>>  extern const struct tegra_mc_soc tegra210_mc_soc;
>>  #endif
>>  
>> +/*
>> + * These IDs are for internal use of Tegra's ICC, the values are chosen
>> + * such that they don't conflict with the device-tree ICC node IDs.
>> + */
>> +#define TEGRA_ICC_EMC		1000
>> +#define TEGRA_ICC_EMEM		2000
>> +#define TEGRA_ICC_MC		3000
> 
> Sounds to me like these could equally well be 1000, 1001 and 1002. Why
> leave these large holes in the number space?

There is no specific reason, I can change the numbers if you want.