SOCFPGA Stratix10 SMMU Support
Subsystems (4)
cover SOCFPGA Stratix10 SMMU Support
From: Thor Thayer <[email protected]>
This patch series adds SMMU support to the SOCFPGA Stratix10 family of parts. The addition of a clock parameter was required to ungate the SMMU clock.
Thor Thayer (3):
Docs: dt: arm-smmu: Add optional clock parameter iommu/arm-smmu: Add optional SMMU clock
arm64: dts: stratix10: Add SMMU Node
.../devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++ arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 44 ++++++++++++++++++++++ drivers/iommu/arm-smmu.c | 17 +++++++++ 3 files changed, 77 insertions(+)
Signature · 1 line
2.7.4
PATCH 1/3 Docs: dt: arm-smmu: Add optional clock parameter
From: Thor Thayer <[email protected]>
Add a clock to the SMMU node bindings.
--- Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu.txt +16 -0
@@ -71,6 +71,8 @@ conditions. or using stream matching with #iommu-cells = <2>, and may be ignored if present in such cases. +- clock: clock provider specifier + ** Deprecated properties: - mmu-masters (deprecated in favour of the generic "iommus" binding) :
@@ -137,3 +139,17 @@ conditions. iommu-map = <0 &smmu3 0 0x400>; ... }; + + /* ARM MMU-500 with clock node */ + smmu4: iommu { + compatible = "arm,mmu-500", "arm,smmu-v2"; + clocks = <&clkmgr STRATIX10_L4_MAIN_CLK>; + ... + #iommu-cells = <1>; + }; + + /* device with stream ID 1 */ + master4 { + iommus = <&smmu4 1>; + }; +
Signature · 1 line
2.7.4
Rob Herring
maintainerPATCH 1/3 Docs: dt: arm-smmu: Add optional clock parameter
Quoted 14 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add a clock to the SMMU node bindings.
Signed-off-by Thor Thayer <[email protected]>--- Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)Documentation/devicetree/bindings/iommu/arm,smmu.txt +0
-0
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 75
Quoted 5 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +2
-0@@ -71,6 +71,8 @@ conditions. or using stream matching with #iommu-cells = <2>, and may be ignored if present in such cases. +- clock: clock provider specifier +
The TRM says there is a TCU clock and clock per TBU.
Rob
PATCH 1/3 Docs: dt: arm-smmu: Add optional clock parameter
Hi Rob,
Quoted 18 lines · Rob Herring
Quoted 17 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add a clock to the SMMU node bindings.
Signed-off-by Thor Thayer <[email protected]>--- Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)Documentation/devicetree/bindings/iommu/arm,smmu.txt +0
-0@@ -71,6 +71,8 @@ conditions. or using stream matching with #iommu-cells = <2>, and may be ignored if present in such cases.
Quoted 7 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 75
Quoted 2 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +2
-0+- clock: clock provider specifier +The TRM says there is a TCU clock and clock per TBU.
Rob
Yes, good point. I'm abandoning this review and will use the bulk clock suggested in [1].
In our case, the TCU clock is always on and we have 1 clock for the TBU masters.
Thanks for reviewing!
Thor
PATCH 2/3 iommu/arm-smmu: Add optional SMMU clock
In reply to thor.thayer at linux.intel.comFrom: Thor Thayer <[email protected]>
Add a clock to the SMMU structure. In the device tree case, check for a clock handle and enable the clock if found.
--- drivers/iommu/arm-smmu.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/iommu/arm-smmu.c +17 -0
@@ -212,6 +212,7 @@ struct arm_smmu_device {/* IOMMU core code handle */ struct iommu_device iommu; +struct clk *clk; };enum arm_smmu_context_fmt {
@@ -1992,6 +1993,15 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,struct device *dev = &pdev->dev;bool legacy_binding; +/* If a clock is declared, enable it */ + smmu->clk =devm_clk_get (smmu->dev,NULL ); +if (IS_ERR (smmu->clk)) { + smmu->clk =NULL ; +dev_dbg (dev,"cannot get smmu clock\n" ); + }else { +clk_prepare_enable (smmu->clk); + } +if (of_property_read_u32 (dev->of_node,"#global-interrupts" , &smmu->num_global_irqs)) {dev_err (dev,"missing #global-interrupts property\n" );
@@ -2181,6 +2191,10 @@ static int arm_smmu_device_remove(struct platform_device *pdev)/* Turn the thing off */ writel (sCR0_CLIENTPD,ARM_SMMU_GR0_NS (smmu) + ARM_SMMU_GR0_sCR0); + +if (smmu->clk) +clk_disable_unprepare (smmu->clk); +return 0 ; }
@@ -2193,6 +2207,9 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) {struct arm_smmu_device *smmu =dev_get_drvdata (dev); +if (smmu->clk) +clk_prepare_enable (smmu->clk); +arm_smmu_device_reset (smmu);return 0 ; }
Signature · 1 line
2.7.4
PATCH 3/3 arm64: dts: stratix10: Add SMMU Node
In reply to thor.thayer at linux.intel.comFrom: Thor Thayer <[email protected]>
Add the SMMU node and IOMMU parameters to the
Stratix10 Device Tree.
--- arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+)
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +44 -0
@@ -162,6 +162,8 @@ reset-names = "stmmaceth", "stmmaceth-ocp"; clocks = <&clkmgr STRATIX10_EMAC0_CLK>; clock-names = "stmmaceth"; + #stream-id-cells = <1>; + iommus = <&smmu 1>; status = "disabled"; };
@@ -175,6 +177,8 @@ reset-names = "stmmaceth", "stmmaceth-ocp"; clocks = <&clkmgr STRATIX10_EMAC1_CLK>; clock-names = "stmmaceth"; + #stream-id-cells = <1>; + iommus = <&smmu 2>; status = "disabled"; };
@@ -188,6 +192,8 @@ reset-names = "stmmaceth", "stmmaceth-ocp"; clocks = <&clkmgr STRATIX10_EMAC2_CLK>; clock-names = "stmmaceth"; + #stream-id-cells = <1>; + iommus = <&smmu 3>; status = "disabled"; };
@@ -298,6 +304,8 @@ clocks = <&clkmgr STRATIX10_L4_MP_CLK>, <&clkmgr STRATIX10_SDMMC_CLK>; clock-names = "biu", "ciu"; + #stream-id-cells = <1>; + iommus = <&smmu 5>; 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>; }; 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 */ + <0 136 4>, /* Non-secure Combined Interrupt */ + <0 137 4>, /* Secure Combined Interrupt */ + /* 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"; + }; + spi0: spi at ffda4000 { compatible = "snps,dw-apb-ssi"; #address-cells = <1>;
@@ -439,6 +479,8 @@ resets = <&rst USB0_RESET>, <&rst USB0_OCP_RESET>; reset-names = "dwc2", "dwc2-ecc"; clocks = <&clkmgr STRATIX10_USB_CLK>; + #stream-id-cells = <1>; + iommus = <&smmu 6>; status = "disabled"; };
@@ -451,6 +493,8 @@ resets = <&rst USB1_RESET>, <&rst USB1_OCP_RESET>; reset-names = "dwc2", "dwc2-ecc"; clocks = <&clkmgr STRATIX10_USB_CLK>; + #stream-id-cells = <1>; + iommus = <&smmu 7>; status = "disabled"; };
Signature · 1 line
2.7.4
Robin Murphy
reviewerPATCH 3/3 arm64: dts: stratix10: Add SMMU Node
Quoted 15 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the SMMU node and IOMMU parameters to the
Stratix10 Device Tree.Signed-off-by Thor Thayer <[email protected]>--- arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+)arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +0
-0
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 165
Quoted 4 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +1
-0@@ -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.
Quoted 32 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +7
-0+ iommus = <&smmu 1>; status = "disabled"; };@@ -175,6 +177,8 @@ reset-names = "stmmaceth", "stmmaceth-ocp"; clocks = <&clkmgr STRATIX10_EMAC1_CLK>; clock-names = "stmmaceth"; + #stream-id-cells = <1>; + iommus = <&smmu 2>; status = "disabled"; };@@ -188,6 +192,8 @@ reset-names = "stmmaceth", "stmmaceth-ocp"; clocks = <&clkmgr STRATIX10_EMAC2_CLK>; clock-names = "stmmaceth"; + #stream-id-cells = <1>; + iommus = <&smmu 3>; status = "disabled"; };@@ -298,6 +304,8 @@ clocks = <&clkmgr STRATIX10_L4_MP_CLK>, <&clkmgr STRATIX10_SDMMC_CLK>; clock-names = "biu", "ciu"; + #stream-id-cells = <1>; + iommus = <&smmu 5>; status = "disabled"; };
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 335
Quoted 5 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +2
-0@@ -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.)
Quoted 12 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +5
-0}; 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>;
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 359
Quoted 10 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +10
-0+ 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.
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 361
Quoted 2 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +2
-0+ <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...)
Robin.
Quoted 35 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +17
-0+ /* 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"; + }; + spi0: spi at ffda4000 { compatible = "snps,dw-apb-ssi"; #address-cells = <1>;@@ -439,6 +479,8 @@ resets = <&rst USB0_RESET>, <&rst USB0_OCP_RESET>; reset-names = "dwc2", "dwc2-ecc"; clocks = <&clkmgr STRATIX10_USB_CLK>; + #stream-id-cells = <1>; + iommus = <&smmu 6>; status = "disabled"; };@@ -451,6 +493,8 @@ resets = <&rst USB1_RESET>, <&rst USB1_OCP_RESET>; reset-names = "dwc2", "dwc2-ecc"; clocks = <&clkmgr STRATIX10_USB_CLK>; + #stream-id-cells = <1>; + iommus = <&smmu 7>; status = "disabled"; };
PATCH 3/3 arm64: dts: stratix10: Add SMMU Node
Hi Robin,
Quoted 12 lines · Robin Murphy
Quoted 11 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
Add the SMMU node and IOMMU parameters to the
Stratix10 Device Tree.Signed-off-by Thor Thayer <[email protected]>---
?arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi| 44
+++++++++++++++++++++++
? 1 file changed, 44 insertions(+)
Quoted 14 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 165
Quoted 10 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +1
-0@@ -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 3 lines
Quoted 3 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +1
-0+??????????? iommus = <&smmu 1>; ????????????? status = "disabled"; ????????? };
<SNIP>
Quoted 4 lines
Quoted 4 lines
????????????? status = "disabled";
????????? };arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +0
-0@@ -323,6 +331,8 @@ ????????????? #dma-requests = <32>;
Quoted 9 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 335
Quoted 4 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +2
-0????????????? 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 18 lines
Quoted 18 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +13
-0????????? }; ????????? 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 */
Quoted 9 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 358
Quoted 2 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +2
-0+??????????????? <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 10 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi line 360
Quoted 2 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +2
-0+??????????????? <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
Quoted 15 lines
Robin.
Quoted 13 lines
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi +13
-0+??????????????? /* 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>
Robin Murphy
reviewerPATCH 3/3 arm64: dts: stratix10: Add SMMU Node
On 16/07/18 19:56, Thor Thayer wrote:
[...]
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
(quoted diff) +13
-0@@ -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 */Quoted 9 lines
Review line 358
Quoted 2 lines
(quoted diff) +2
-0+??????????????? <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.
Quoted 8 lines
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.
No, you had it right - what I meant was it's only worth including the actual global fault signals themselves here (there's no harm in leaving the secure one in if you like, it just won't do anything if the GIC is configured correctly). What I found was that the current SMMU binding leaves no reasonable way to encode the PMU interrupts in this interrupt list that doesn't break at least one of the possible old/new driver/DT combinations, so whatever happens they will need to be specified separately once a PMU binding is defined.
Quoted 11 lines
Quoted 10 lines
Review line 360
Quoted 2 lines
(quoted diff) +2
-0+??????????????? <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?
Indeed.
Quoted 2 lines
Thanks for the review and helpful comments (and also pointing out the existing clock patch in my patch series summary)!
Cheers,
Robin.
Robin Murphy
reviewercover SOCFPGA Stratix10 SMMU Support
In reply to thor.thayer at linux.intel.comHi Thor,
Quoted 5 lines · thor.thayer at linux.intel.com
From: Thor Thayer <[email protected]>
This patch series adds SMMU support to the SOCFPGA Stratix10 family of parts. The addition of a clock parameter was required to ungate the SMMU clock.
Please see the latest version of Vivek's runtime PM series[1] - there's already been an awful lot of discussion over how to handle clocks, since they're not part of the SMMU architecture and different implementations can have very different requirements (and even different configurations of the same implementation may vary, e.g. MMU-500 could technically have up to 33 separate clocks).
Thanks,
Robin.
[1]
https://www.mail-archive.com/freedreno at lists.freedesktop.org/msg02422.html
Quoted 10 lines
Thor Thayer (3):
Docs: dt: arm-smmu: Add optional clock parameter iommu/arm-smmu: Add optional SMMU clock
arm64: dts: stratix10: Add SMMU Node.../devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++ arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 44 ++++++++++++++++++++++ drivers/iommu/arm-smmu.c | 17 +++++++++ 3 files changed, 77 insertions(+)