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
