[PATCH v2 01/10] memory: tegra: Implement SID override programming

[Krzysztof Kozlowski]

On 26/04/2021 14:13, Thierry Reding wrote:
> On Mon, Apr 26, 2021 at 10:28:43AM +0200, Krzysztof Kozlowski wrote:

(...)

>>> +
>>> +	value = readl(mc->regs + client->regs.sid.override);
>>> +	old = value & MC_SID_STREAMID_OVERRIDE_MASK;
>>> +
>>> +	if (old != sid) {
>>> +		dev_dbg(mc->dev, "overriding SID %x for %s with %x\n", old,
>>> +			client->name, sid);
>>> +		writel(sid, mc->regs + client->regs.sid.override);
>>> +	}
>>> +}
>>> +
>>> +static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
>>> +{
>>> +#if IS_ENABLED(CONFIG_IOMMU_API)
>>
>> Is this part really build-time dependent? I don't see here any uses of
>> IOMMU specific fields, so maybe this should be runtime choice based on
>> enabled interconnect devices?
> 
> Unfortunately it is. struct iommu_fwspec is an empty structure for
> !CONFIG_IOMMU_API, so the code below that tries to access fwspec->ids
> fails for !CONFIG_IOMMU_API configurations if we don't protect this with
> the preprocessor guard.

OK, thanks.

> 
>>
>>> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> +	struct of_phandle_args args;
>>> +	unsigned int i, index = 0;
>>> +
>>> +	while (!of_parse_phandle_with_args(dev->of_node, "interconnects", "#interconnect-cells",
>>> +					   index, &args)) {
>>> +		if (args.np == mc->dev->of_node && args.args_count != 0) {
>>> +			for (i = 0; i < mc->soc->num_clients; i++) {
>>> +				const struct tegra_mc_client *client = &mc->soc->clients[i];
>>> +
>>> +				if (client->id == args.args[0]) {
>>> +					u32 sid = fwspec->ids[0] & MC_SID_STREAMID_OVERRIDE_MASK;
>>> +
>>> +					tegra186_mc_client_sid_override(mc, client, sid);
>>> +				}
>>> +			}
>>> +		}
>>> +
>>> +		index++;
>>> +	}
>>> +#endif
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  const struct tegra_mc_ops tegra186_mc_ops = {
>>>  	.probe = tegra186_mc_probe,
>>>  	.remove = tegra186_mc_remove,
>>>  	.resume = tegra186_mc_resume,
>>> +	.probe_device = tegra186_mc_probe_device,
>>>  };
>>>  
>>>  #if defined(CONFIG_ARCH_TEGRA_186_SOC)
>>> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
>>> index 1387747d574b..bbad6330008b 100644
>>> --- a/include/soc/tegra/mc.h
>>> +++ b/include/soc/tegra/mc.h
>>> @@ -176,6 +176,7 @@ struct tegra_mc_ops {
>>>  	int (*suspend)(struct tegra_mc *mc);
>>>  	int (*resume)(struct tegra_mc *mc);
>>>  	irqreturn_t (*handle_irq)(int irq, void *data);
>>> +	int (*probe_device)(struct tegra_mc *mc, struct device *dev);
>>>  };
>>>  
>>>  struct tegra_mc_soc {
>>> @@ -240,4 +241,6 @@ devm_tegra_memory_controller_get(struct device *dev)
>>>  }
>>>  #endif
>>>  
>>> +int tegra_mc_probe_device(struct tegra_mc *mc, struct device *dev);
>>> +
>>
>> What about !CONFIG_TEGRA_MC? I think arm-smmmu will fail.
> 
> I think it doesn't fail because for !CONFIG_TEGRA_MC it basically throws
> away most of nvidia_smmu_impl_init() already because ERR_PTR(-ENODEV) is
> returned by devm_tegra_memory_controller_get() and so it unconditionally
> fails early on already.
> 
> I say I /think/ that happens because I can't reproduce a build failure
> even if I manually tweak the .config such that ARM_SMMU is enabled and
> TEGRA_MC is disabled. But I can't say I fully understand why it's
> working, because, yes, the symbol definitely doesn't exist. But again,
> if the compiler is clever enough to figure out that that function can't
> be called anyway and doesn't even want it, why bother making it more
> complicated than it has to be?

Since you tested that case, it's fine.


Best regards,
Krzysztof