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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6949 bytes --]

On Mon, Apr 26, 2021 at 10:28:43AM +0200, Krzysztof Kozlowski wrote:
> On 20/04/2021 19:26, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Instead of programming all SID overrides during early boot, perform the
> > operation on-demand after the SMMU translations have been set up for a
> > device. This reuses data from device tree to match memory clients for a
> > device and programs the SID specified in device tree, which corresponds
> > to the SID used for the SMMU context banks for the device.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/memory/tegra/mc.c       |  9 +++++
> >  drivers/memory/tegra/tegra186.c | 72 +++++++++++++++++++++++++++++++++
> >  include/soc/tegra/mc.h          |  3 ++
> >  3 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > index c854639cf30c..bace5ecfe770 100644
> > --- a/drivers/memory/tegra/mc.c
> > +++ b/drivers/memory/tegra/mc.c
> > @@ -97,6 +97,15 @@ struct tegra_mc *devm_tegra_memory_controller_get(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(devm_tegra_memory_controller_get);
> >  
> > +int tegra_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> > +{
> > +	if (mc->soc->ops && mc->soc->ops->probe_device)
> > +		return mc->soc->ops->probe_device(mc, dev);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tegra_mc_probe_device);
> > +
> >  static int tegra_mc_block_dma_common(struct tegra_mc *mc,
> >  				     const struct tegra_mc_reset *rst)
> >  {
> > diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> > index 1f87915ccd62..e65eac5764d4 100644
> > --- a/drivers/memory/tegra/tegra186.c
> > +++ b/drivers/memory/tegra/tegra186.c
> > @@ -4,6 +4,7 @@
> >   */
> >  
> >  #include <linux/io.h>
> > +#include <linux/iommu.h>
> >  #include <linux/module.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/of_device.h>
> > @@ -15,6 +16,10 @@
> >  #include <dt-bindings/memory/tegra186-mc.h>
> >  #endif
> >  
> > +#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
> > +#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
> > +#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
> > +
> >  static void tegra186_mc_program_sid(struct tegra_mc *mc)
> >  {
> >  	unsigned int i;
> > @@ -66,10 +71,77 @@ static int tegra186_mc_resume(struct tegra_mc *mc)
> >  	return 0;
> >  }
> >  
> > +static void tegra186_mc_client_sid_override(struct tegra_mc *mc,
> > +					    const struct tegra_mc_client *client,
> > +					    unsigned int sid)
> > +{
> > +	u32 value, old;
> > +
> > +	value = readl(mc->regs + client->regs.sid.security);
> > +	if ((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0) {
> > +		/*
> > +		 * If the secure firmware has locked this down the override
> > +		 * for this memory client, there's nothing we can do here.
> > +		 */
> > +		if (value & MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED)
> > +			return;
> > +
> > +		/*
> > +		 * Otherwise, try to set the override itself. Typically the
> > +		 * secure firmware will never have set this configuration.
> > +		 * Instead, it will either have disabled write access to
> > +		 * this field, or it will already have set an explicit
> > +		 * override itself.
> > +		 */
> > +		WARN_ON((value & MC_SID_STREAMID_SECURITY_OVERRIDE) == 0);
> > +
> > +		value |= MC_SID_STREAMID_SECURITY_OVERRIDE;
> > +		writel(value, mc->regs + client->regs.sid.security);
> > +	}
> > +
> > +	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.

> 
> > +	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?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel