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

[thor.thayer@linux.intel.com (Thor Thayer)]

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:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Add the SMMU node and IOMMU parameters to the
>> Stratix10 Device Tree.
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>> ? 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.

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

<SNIP>

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

>> ????????? };
>> ????????? 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.

>> +??????????????? <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.
> 
>> +??????????????? /* 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>