On Mon, Nov 3, 2014 at 5:22 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Sat, Nov 01, 2014 at 02:38:26PM +0900, Alexandre Courbot wrote: >> On Fri, Oct 31, 2014 at 10:27 PM, Thierry Reding >> <thierry.reding@xxxxxxxxx> wrote: >> > On Thu, Oct 30, 2014 at 04:08:41PM +0100, Thierry Reding wrote: >> >> On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote: >> >> > Hi, >> >> > >> >> > Oh, a few more comments: >> >> > >> >> > On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding >> >> > <thierry.reding@xxxxxxxxx> wrote: >> >> > >> >> > > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >> >> > > index c32d31981be3..1c932e7e7b8d 100644 >> >> > > --- a/drivers/memory/Makefile >> >> > > +++ b/drivers/memory/Makefile >> >> > > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF) += fsl-corenet-cf.o >> >> > > obj-$(CONFIG_FSL_IFC) += fsl_ifc.o >> >> > > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >> >> > > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >> >> > > -obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >> >> > > + >> >> > > +obj-$(CONFIG_ARCH_TEGRA) += tegra/ >> >> > > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile >> >> > > new file mode 100644 >> >> > > index 000000000000..51b9e8fcde1b >> >> > > --- /dev/null >> >> > > +++ b/drivers/memory/tegra/Makefile >> >> > > @@ -0,0 +1,5 @@ >> >> > > +obj-y = tegra-mc.o >> >> > > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += tegra30-mc.o >> >> > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC) += tegra114-mc.o >> >> > > +obj-$(CONFIG_ARCH_TEGRA_124_SOC) += tegra124-mc.o >> >> > > +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += tegra124-mc.o >> >> > >> >> > You'll need a Kconfig and not just a makefile -- there are definitely >> >> > dependencies on this driver (IOMMU in particular). >> >> >> >> This is handled within the tegra-mc driver by only setting up the IOMMU >> >> when TEGRA_IOMMU_SMMU is enabled. That config option remains in place. >> >> >> >> > Also, the problem of having a global enable bit that is only under >> >> > control of TrustZone FW is a big problem -- if the bit is not set, the >> >> > driver will not work (and the machine will crash). >> >> > >> >> > I think you'll need to come up with a way to detect that in the >> >> > driver. I don't have a good idea of how it can be done though. >> >> >> >> I don't think I ever got back to you on this. We discussed this >> >> internally and it seems like there's no way to detect this properly, so >> >> the best suggestion so far was to make it a requirement on the secure >> >> firmware to enable IOMMU or not. Since there's no way for the kernel to >> >> detect whether IOMMU was enabled or not, I think the firmware would >> >> equally have to adjust the SMMU's device tree node's status property >> >> appropriately. >> > >> > The other option would be for the firmware not to touch the SMMU device >> > tree node and the kernel simply assuming that if it's running in non- >> > secure mode then there must be secure firmware and it has enabled the >> > SMMU. Enabling the SMMU would become part of the contract between >> > firmware and kernel, much like locking the VPR is required to get the >> > GPU to work. >> > >> > Those are really the only two choices we have. >> >> We got the exact same problem with GPU and VPR registers, and it seems >> like the approach we will be taking here is to have the >> firmware/bootloader do whatever is needed to get the GPU working and >> enable the DT node once it did. IOW, the kernel will never touch >> protected registers, and will not freeze if the hardware is not >> properly set up. > > I think the situation is slightly different. For the SMMU we still have > the option to enable translations when running in secure mode with code > that's pretty trivial to have in the IOMMU driver (it's just a single > bit that gets written to a register). And when the IOMMU driver does > that everything will work just fine. > > So I'm thinking that a workable alternative to what we've done for VPR > would be to just always enable translations in the SMMU driver (that > operation will simply be discarded in non-secure mode) and assume that > it'll work. So the contract would be that running in secure mode the > kernel sets everything up and when running in non-secure mode the kernel > will assume that firmware set everything up already. > > Requiring firmware to change the device node's status to "okay" seems > rather restrictive since it would have to do that even if it boots the > kernel in secure mode. Sounds good to me, as long as the kernel can always run using the same code in both modes. > >> It would be nice for consistency if the same approach can be taken >> with the IOMMU. OTOH we will need to make sure that all these >> initialization contracts are clearly documented somewhere. Maybe a >> comment in the DTS to explain what is expected from the firmware to >> enable such nodes would be a good idea, too. > > The device tree binding seems like a good place to document this. Indeed. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
