Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
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.red...@gmail.com 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.red...@gmail.com 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 ..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. 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. Thierry pgp4nmOj7RvZR.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On Mon, Nov 3, 2014 at 5:22 PM, Thierry Reding thierry.red...@gmail.com 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.red...@gmail.com 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.red...@gmail.com 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 ..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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On Fri, Oct 31, 2014 at 10:27 PM, Thierry Reding thierry.red...@gmail.com 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.red...@gmail.com 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 ..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. 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
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.red...@gmail.com 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 ..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. Thierry pgpOp1ho8erQa.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On 30.10.2014 12:03, Alexandre Courbot wrote: I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU to work with GK20A. The reason is still not completely clear to me, but if you look at the TRM you see that 0xaa8 is basically constant, with the SMMU translation bit hardcoded to DISABLE (and the ASID field being meaningless in that case). However right after that register you have a functional one named GPUB instead of GPU, and this one is fully writeable (and has the expected effect). GPU has two SW group IDs, because it accesses memory both with and without translation. The bit 34 in addresses in f.ex. PTE chooses between the two. GPU is hard-wired to disable translation. For GPUB translation can be enabled. Terje ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On 30.10.2014 12:22, Alexandre Courbot wrote: So should I understand that the GPU group is for addresses without bit 34 set (hence forcibly disabled) while GPUB is used when that bit is set? Or is it something else? That's exactly correct. And only GPUB can be programmed to be SMMU translated. Terje ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
diff --git a/drivers/memory/tegra/tegra124-mc.c
b/drivers/memory/tegra/tegra124-mc.c
...
+static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
+ { .swgroup = TEGRA_SWGROUP_DC,.reg = 0x240 },
+ { .swgroup = TEGRA_SWGROUP_DCB, .reg = 0x244 },
+ { .swgroup = TEGRA_SWGROUP_AFI, .reg = 0x238 },
+ { .swgroup = TEGRA_SWGROUP_AVPC, .reg = 0x23c },
+ { .swgroup = TEGRA_SWGROUP_HDA, .reg = 0x254 },
+ { .swgroup = TEGRA_SWGROUP_HC,.reg = 0x250 },
+ { .swgroup = TEGRA_SWGROUP_MSENC, .reg = 0x264 },
+ { .swgroup = TEGRA_SWGROUP_PPCS, .reg = 0x270 },
+ { .swgroup = TEGRA_SWGROUP_SATA, .reg = 0x274 },
+ { .swgroup = TEGRA_SWGROUP_VDE, .reg = 0x27c },
+ { .swgroup = TEGRA_SWGROUP_ISP2, .reg = 0x258 },
+ { .swgroup = TEGRA_SWGROUP_XUSB_HOST, .reg = 0x288 },
+ { .swgroup = TEGRA_SWGROUP_XUSB_DEV, .reg = 0x28c },
+ { .swgroup = TEGRA_SWGROUP_ISP2B, .reg = 0xaa4 },
+ { .swgroup = TEGRA_SWGROUP_TSEC, .reg = 0x294 },
+ { .swgroup = TEGRA_SWGROUP_A9AVP, .reg = 0x290 },
+ { .swgroup = TEGRA_SWGROUP_GPU, .reg = 0xaa8 },
I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU
to work with GK20A. The reason is still not completely clear to me, but
if you look at the TRM you see that 0xaa8 is basically constant, with
the SMMU translation bit hardcoded to DISABLE (and the ASID field being
meaningless in that case). However right after that register you have a
functional one named GPUB instead of GPU, and this one is fully
writeable (and has the expected effect).
I will try to get more information about the why of this, but for now
this setting is what works for me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On 10/30/2014 08:04 PM, Terje Bergström wrote: On 30.10.2014 12:22, Alexandre Courbot wrote: So should I understand that the GPU group is for addresses without bit 34 set (hence forcibly disabled) while GPUB is used when that bit is set? Or is it something else? That's exactly correct. And only GPUB can be programmed to be SMMU translated. Great, thanks for confirming! Thierry, how do you want to address this? We could change the register for the GPU group, or (maybe preferable if we want to reflect the actual hardware state) add the GPUB group. I don't know if that would be easy though since we would have the problem of the gpusrd and gpuswr clients ownership (seems like they would belong to both groups?) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On 30.10.2014 15:35, Alexandre Courbot wrote: Great, thanks for confirming! Thierry, how do you want to address this? We could change the register for the GPU group, or (maybe preferable if we want to reflect the actual hardware state) add the GPUB group. I don't know if that would be easy though since we would have the problem of the gpusrd and gpuswr clients ownership (seems like they would belong to both groups?) gpusrd and gpuswr are client IDs for GPU reads and writes on MC. GPU and GPUB are SW group IDs for SMMU. There's no 1:1 or hierarchical mapping. Terje ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On Thu, Oct 30, 2014 at 7:18 PM, Terje Bergström tbergst...@nvidia.com wrote: On 30.10.2014 12:03, Alexandre Courbot wrote: I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU to work with GK20A. The reason is still not completely clear to me, but if you look at the TRM you see that 0xaa8 is basically constant, with the SMMU translation bit hardcoded to DISABLE (and the ASID field being meaningless in that case). However right after that register you have a functional one named GPUB instead of GPU, and this one is fully writeable (and has the expected effect). GPU has two SW group IDs, because it accesses memory both with and without translation. The bit 34 in addresses in f.ex. PTE chooses between the two. Indeed, to enable SMMU translation I have to program 0xaac correctly *and* set the bit 34 of every address that needs to go through the SMMU. GPU is hard-wired to disable translation. For GPUB translation can be enabled. So should I understand that the GPU group is for addresses without bit 34 set (hence forcibly disabled) while GPUB is used when that bit is set? Or is it something else? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
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.red...@gmail.com 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 ..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. Stephen, does that accurately reflect what we had discussed? Thierry pgp0Eet5vQNyP.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
On Wed, Oct 15, 2014 at 03:05:36PM -0700, Olof Johansson wrote:
Hi,
On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
thierry.red...@gmail.com wrote:
[...]
diff --git a/drivers/memory/tegra/tegra-mc.c
b/drivers/memory/tegra/tegra-mc.c
new file mode 100644
index ..0f0c8be096d0
--- /dev/null
+++ b/drivers/memory/tegra/tegra-mc.c
@@ -0,0 +1,1061 @@
+/*
+ * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/clk.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/iommu.h
+#include linux/kernel.h
+#include linux/module.h
You need a linux/mm.h in here (on 64-bit).
Can you show what build error this fixes? I don't see any build failures
(after fixing up the obvious ones you pointed out below).
diff --git a/drivers/memory/tegra/tegra124-mc.c
b/drivers/memory/tegra/tegra124-mc.c
new file mode 100644
index ..db31c96fc288
--- /dev/null
+++ b/drivers/memory/tegra/tegra124-mc.c
[...]
@@ -0,0 +1,1028 @@
+/*
+ * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/of.h
+#include linux/mm.h
+
+#include asm/cacheflush.h
+
+#include dt-bindings/memory/tegra124-mc.h
+
+#include tegra-mc.h
+
+static const struct tegra_mc_client tegra124_mc_clients[] = {
+ {
+ .id = 0x00,
+ .name = ptcr,
+ .swgroup = TEGRA_SWGROUP_PTC,
+ }, {
+ .id = 0x01,
+ .name = display0a,
+ .swgroup = TEGRA_SWGROUP_DC,
+ .smmu = {
+ .reg = 0x228,
+ .bit = 1,
+ },
+ .latency = {
+ .reg = 0x2e8,
+ .shift = 0,
+ .mask = 0xff,
+ .def = 0xc2,
+ },
+ }, {
These are very verbose tables. Having a macro for the initializers
could help density a lot.
I've tried to use macros here, but I find that it hurts readability:
...
}, {
TEGRA_MC_CLIENT(0x01, display0a, TEGRA_SWGROUP_DC),
TEGRA_MC_SMMU_ENABLE(0x228, 1),
TEGRA_MC_LATENCY(0x2e8, 0, 0xff, 0xc2),
}, {
...
The original is more readable because it immediately gives you the
context, whereas with the macros you need to look up what the parameters
refer to.
+#ifdef CONFIG_ARCH_TEGRA_132_SOC
+static void tegra132_flush_dcache(struct page *page, unsigned long offset,
+ size_t size)
+{
+ void *virt = page_address(page) + offset;
+
+ __flush_dcache_area(virt, size);
+}
+
+static const struct tegra_smmu_ops tegra132_smmu_ops = {
+ .flush_dcache = tegra132_flush_dcache,
+};
+
+static const struct tegra_smmu_soc tegra132_smmu_soc = {
+ .groups = tegra124_smmu_groups,
+ .num_groups = ARRAY_SIZE(tegra124_smmu_groups),
+ .clients = tegra124_mc_clients,
+ .num_clients = ARRAY_SIZE(tegra124_mc_clients),
+ .swgroups = tegra124_swgroups,
+ .num_swgroups = ARRAY_SIZE(tegra124_swgroups),
+ .supports_round_robin_arbitration = true,
+ .supports_request_limit = true,
+ .num_address_bits = 34,
+ .num_asids = 128,
+ .ops = tegra132_smmu_ops,
+};
+
+const struct tegra_mc_soc tegra132_mc_soc = {
+ .clients = tegra124_mc_clients,
+ .num_clients = ARRAY_SIZE(tegra124_mc_clients),
+ .atom_size = 32,
+ .smmu = tegra132_smmu_soc,
+};
+#endif /* CONFIG_ARCH_TEGRA_132_SOC */
This won't compile -- several of the tegra132_smmu_soc members are no
longer valid. In particular:
groups
num_groups
Fixed.
supports_round_robin_arbitration
supports_request_limit
In the version that I have these are still part of the tegra_smmu_soc
structure.
I've been thinking of extracting the Tegra132 changes into a separate
patch that can be applied once we have basic Tegra132 SoC support. It
feels wrong to merge Tegra132 SMMU support if there's no support in
arch/arm64 for the SoC yet. Though if nobody else thinks that's a
problem that's fine with me too.
Thierry
pgpqyUorNQDLw.pgp
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
Hi,
On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
thierry.red...@gmail.com wrote:
[...]
diff --git a/drivers/memory/tegra/tegra-mc.c b/drivers/memory/tegra/tegra-mc.c
new file mode 100644
index ..0f0c8be096d0
--- /dev/null
+++ b/drivers/memory/tegra/tegra-mc.c
@@ -0,0 +1,1061 @@
+/*
+ * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/clk.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/iommu.h
+#include linux/kernel.h
+#include linux/module.h
You need a linux/mm.h in here (on 64-bit).
+#include linux/of.h
+#include linux/of_platform.h
+#include linux/platform_device.h
+#include linux/slab.h
[...]
diff --git a/drivers/memory/tegra/tegra124-mc.c
b/drivers/memory/tegra/tegra124-mc.c
new file mode 100644
index ..db31c96fc288
--- /dev/null
+++ b/drivers/memory/tegra/tegra124-mc.c
[...]
@@ -0,0 +1,1028 @@
+/*
+ * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/of.h
+#include linux/mm.h
+
+#include asm/cacheflush.h
+
+#include dt-bindings/memory/tegra124-mc.h
+
+#include tegra-mc.h
+
+static const struct tegra_mc_client tegra124_mc_clients[] = {
+ {
+ .id = 0x00,
+ .name = ptcr,
+ .swgroup = TEGRA_SWGROUP_PTC,
+ }, {
+ .id = 0x01,
+ .name = display0a,
+ .swgroup = TEGRA_SWGROUP_DC,
+ .smmu = {
+ .reg = 0x228,
+ .bit = 1,
+ },
+ .latency = {
+ .reg = 0x2e8,
+ .shift = 0,
+ .mask = 0xff,
+ .def = 0xc2,
+ },
+ }, {
These are very verbose tables. Having a macro for the initializers
could help density a lot.
+#ifdef CONFIG_ARCH_TEGRA_132_SOC
+static void tegra132_flush_dcache(struct page *page, unsigned long offset,
+ size_t size)
+{
+ void *virt = page_address(page) + offset;
+
+ __flush_dcache_area(virt, size);
+}
+
+static const struct tegra_smmu_ops tegra132_smmu_ops = {
+ .flush_dcache = tegra132_flush_dcache,
+};
+
+static const struct tegra_smmu_soc tegra132_smmu_soc = {
+ .groups = tegra124_smmu_groups,
+ .num_groups = ARRAY_SIZE(tegra124_smmu_groups),
+ .clients = tegra124_mc_clients,
+ .num_clients = ARRAY_SIZE(tegra124_mc_clients),
+ .swgroups = tegra124_swgroups,
+ .num_swgroups = ARRAY_SIZE(tegra124_swgroups),
+ .supports_round_robin_arbitration = true,
+ .supports_request_limit = true,
+ .num_address_bits = 34,
+ .num_asids = 128,
+ .ops = tegra132_smmu_ops,
+};
+
+const struct tegra_mc_soc tegra132_mc_soc = {
+ .clients = tegra124_mc_clients,
+ .num_clients = ARRAY_SIZE(tegra124_mc_clients),
+ .atom_size = 32,
+ .smmu = tegra132_smmu_soc,
+};
+#endif /* CONFIG_ARCH_TEGRA_132_SOC */
This won't compile -- several of the tegra132_smmu_soc members are no
longer valid. In particular:
groups
num_groups
supports_round_robin_arbitration
supports_request_limit
-Olof
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
Hi, Oh, a few more comments: On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding thierry.red...@gmail.com 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 ..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). 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. -Olof ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 05/12] memory: Add NVIDIA Tegra memory controller support
From: Thierry Reding tred...@nvidia.com
The memory controller on NVIDIA Tegra exposes various knobs that can be
used to tune the behaviour of the clients attached to it.
Currently this driver sets up the latency allowance registers to the HW
defaults. Eventually an API should be exported by this driver (via a
custom API or a generic subsystem) to allow clients to register latency
requirements.
This driver also registers an IOMMU (SMMU) that's implemented by the
memory controller. It is supported on Tegra30, Tegra114 and Tegra124
currently. Tegra20 has a GART instead.
Signed-off-by: Thierry Reding tred...@nvidia.com
---
Changes in v4:
- remove DMA/IOMMU integration glue
- don't initialize linear SMMU mapping
- fail attachment for non-masters
- free empty page tables
Changes in v3:
- drop changes related to probe ordering that were rejected
- remove old SMMU driver
drivers/iommu/Makefile |1 -
drivers/iommu/tegra-smmu.c | 1295 --
drivers/memory/Makefile |3 +-
drivers/memory/tegra/Makefile|5 +
drivers/memory/tegra/tegra-mc.c | 1061
drivers/memory/tegra/tegra-mc.h | 90 +++
drivers/memory/tegra/tegra114-mc.c | 948 ++
drivers/memory/tegra/tegra124-mc.c | 1028
drivers/memory/tegra/tegra30-mc.c| 970 ++
drivers/memory/tegra30-mc.c | 378 -
include/dt-bindings/memory/tegra114-mc.h | 25 +
include/dt-bindings/memory/tegra124-mc.h | 31 +
include/dt-bindings/memory/tegra30-mc.h | 24 +
13 files changed, 4184 insertions(+), 1675 deletions(-)
delete mode 100644 drivers/iommu/tegra-smmu.c
create mode 100644 drivers/memory/tegra/Makefile
create mode 100644 drivers/memory/tegra/tegra-mc.c
create mode 100644 drivers/memory/tegra/tegra-mc.h
create mode 100644 drivers/memory/tegra/tegra114-mc.c
create mode 100644 drivers/memory/tegra/tegra124-mc.c
create mode 100644 drivers/memory/tegra/tegra30-mc.c
delete mode 100644 drivers/memory/tegra30-mc.c
create mode 100644 include/dt-bindings/memory/tegra114-mc.h
create mode 100644 include/dt-bindings/memory/tegra124-mc.h
create mode 100644 include/dt-bindings/memory/tegra30-mc.h
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 16edef74b8ee..fe926ac4290d 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -14,7 +14,6 @@ obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
obj-$(CONFIG_OMAP_IOMMU) += omap-iommu2.o
obj-$(CONFIG_OMAP_IOMMU_DEBUG) += omap-iommu-debug.o
obj-$(CONFIG_TEGRA_IOMMU_GART) += tegra-gart.o
-obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o
obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
obj-$(CONFIG_SHMOBILE_IOMMU) += shmobile-iommu.o
obj-$(CONFIG_SHMOBILE_IPMMU) += shmobile-ipmmu.o
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
deleted file mode 100644
index 3afdf43f732a..
--- a/drivers/iommu/tegra-smmu.c
+++ /dev/null
@@ -1,1295 +0,0 @@
-/*
- * IOMMU API for SMMU in Tegra30
- *
- * Copyright (c) 2011-2013, NVIDIA CORPORATION. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-#define pr_fmt(fmt)%s(): fmt, __func__
-
-#include linux/err.h
-#include linux/module.h
-#include linux/platform_device.h
-#include linux/spinlock.h
-#include linux/slab.h
-#include linux/vmalloc.h
-#include linux/mm.h
-#include linux/pagemap.h
-#include linux/device.h
-#include linux/sched.h
-#include linux/iommu.h
-#include linux/io.h
-#include linux/of.h
-#include linux/of_iommu.h
-#include linux/debugfs.h
-#include linux/seq_file.h
-
-#include soc/tegra/ahb.h
-
-#include asm/page.h
-#include asm/cacheflush.h
-
-enum smmu_hwgrp {
- HWGRP_AFI,
- HWGRP_AVPC,
- HWGRP_DC,
- HWGRP_DCB,
- HWGRP_EPP,
- HWGRP_G2,
- HWGRP_HC,
- HWGRP_HDA,
- HWGRP_ISP,
- HWGRP_MPE,
- HWGRP_NV,
- HWGRP_NV2,
- HWGRP_PPCS,
- HWGRP_SATA,
- HWGRP_VDE,
- HWGRP_VI,
-
- HWGRP_COUNT,
-
- HWGRP_END = ~0,
-};
-
-#define HWG_AFI(1 HWGRP_AFI)
-#define HWG_AVPC (1 HWGRP_AVPC)
-#define HWG_DC (1 HWGRP_DC)
-#define HWG_DCB(1 HWGRP_DCB)
-#define HWG_EPP(1 HWGRP_EPP)
