[PATCH 3/3] arm64: dts: stratix10: Add SMMU Node

Hi Robin,

On 07/13/2018 01:09 PM, Robin Murphy wrote:

On 13/07/18 17:27, thor.thayer at linux.intel.com wrote:

quoted

From: Thor Thayer <redacted>

Add the SMMU node and IOMMU parameters to the
Stratix10 Device Tree.

Signed-off-by: Thor Thayer <redacted>
---
? arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 44 
+++++++++++++++++++++++
? 1 file changed, 44 insertions(+)

diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi 

b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index ca67ecb5866e..9b6ead87ae70 100644

--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi

@@ -162,6 +162,8 @@

????????????? reset-names = "stmmaceth", "stmmaceth-ocp";
????????????? clocks = <&clkmgr STRATIX10_EMAC0_CLK>;
????????????? clock-names = "stmmaceth";
+??????????? #stream-id-cells = <1>;

The #stream-id-cells property is part of the deprecated mmu-masters 
binding, so you don't need to add these.

OK. Thank you.

quoted

+??????????? iommus = <&smmu 1>;
????????????? status = "disabled";
????????? };

<SNIP>

quoted

????????????? status = "disabled";
????????? };

@@ -323,6 +331,8 @@

????????????? #dma-requests = <32>;
????????????? clocks = <&clkmgr STRATIX10_L4_MAIN_CLK>;
????????????? clock-names = "apb_pclk";
+??????????? #stream-id-cells = <1>;
+??????????? iommus = <&smmu 8>;

Just to double-check, all the channel threads and the manager thread 
share the one stream ID? (I'm accustomed to seeing DMA-330 integrated 
with an SMMU by tapping the AxID outputs off to the stream ID input.)

Yes, we have only one stream ID for the DMA. I'll forward the 
differences you noted to our architecture team as something to consider 
for future chips.

quoted

????????? };
????????? rst: rstmgr at ffd11000 {

@@ -332,6 +342,36 @@

????????????? altr,modrst-offset = <0x20>;
????????? };
+??????? smmu: iommu at fa000000 {
+??????????? compatible = "arm,mmu-500", "arm,smmu-v2";
+??????????? reg = <0xfa000000 0x40000>;
+??????????? #global-interrupts = <9>;
+??????????? #iommu-cells = <1>;
+??????????? clocks = <&clkmgr STRATIX10_L4_MAIN_CLK>;
+??????????? clock-names = "smmu_clk";
+??????????? interrupt-parent = <&intc>;
+??????????? interrupts = <0 128 4>,??? /* Global Secure Fault */
+??????????????? <0 129 4>, /* Global Non-secure Fault */
+??????????????? <0 130 4>, /* FPGA Performance Counter */
+??????????????? <0 131 4>, /* DMA Performance Counter */
+??????????????? <0 132 4>, /* EMAC Performance Counter */
+??????????????? <0 133 4>, /* IO Performance Counter */
+??????????????? <0 134 4>, /* SDM Performance Counter */

Note that there isn't much benefit to adding the secure or PMU 
interrupts here other than to document the hardware - FWIW I have 
actually been working on a PMU driver, and needless to say it turns out 
not to be sufficient just having those munged into the SMMU global fault 
handler.

Thanks for pointing this out. I was following other SMMU-500 device 
trees. Just to clarify, how should I simplify this? Should I replace all 
the above with the following?

	<0 129 4>, /* Global Non-secure Fault */

Or will your upcoming PMU driver need the PMU units? It sounded like 
using the just Global fault was not sufficient.

quoted

+??????????????? <0 136 4>, /* Non-secure Combined Interrupt */
+??????????????? <0 137 4>, /* Secure Combined Interrupt */

Similarly the combined interrupt; that's literally just all these other 
interrupt lines ORed together at the SMMU end, and would generally only 
be useful if you *didn't* have the individual lines wired up. As it 
stands with everything listed, any event will also generate a spurious 
global fault IRQ, which isn't ideal (not that you should get many 
interrupts during normal operation, but still...)

And I'd remove both of these then, right?

Thanks for the review and helpful comments (and also pointing out the 
existing clock patch in my patch series summary)!

Thor

Robin.

quoted

+??????????????? /* Non-secure Context Interrupts (32) */
+??????????????? <0 138 4>, <0 139 4>, <0 140 4>, <0 141 4>,
+??????????????? <0 142 4>, <0 143 4>, <0 144 4>, <0 145 4>,
+??????????????? <0 146 4>, <0 147 4>, <0 148 4>, <0 149 4>,
+??????????????? <0 150 4>, <0 151 4>, <0 152 4>, <0 153 4>,
+??????????????? <0 154 4>, <0 155 4>, <0 156 4>, <0 157 4>,
+??????????????? <0 158 4>, <0 159 4>, <0 160 4>, <0 161 4>,
+??????????????? <0 162 4>, <0 163 4>, <0 164 4>, <0 165 4>,
+??????????????? <0 166 4>, <0 167 4>, <0 168 4>, <0 169 4>;
+??????????? stream-match-mask = <0x7ff0>;
+??????????? status = "disabled";
+??????? };
+

<snip>