Messages in this thread | | | From | Thor Thayer <> | Subject | Re: [PATCH 1/2] arm64: dts: stratix10: Add Stratix10 SMMU support | Date | Thu, 16 Aug 2018 22:33:31 -0500 |
| |
Hi Robin,
On 08/08/2018 12:38 PM, Robin Murphy wrote: > On 25/07/18 19:24, thor.thayer@linux.intel.com wrote: >> From: Thor Thayer <thor.thayer@linux.intel.com> >> >> Add SMMU support to the Stratix10 Device Tree which >> includes adding the SMMU node and adding IOMMU stream >> ids to the SMMU peripherals. Update bindings. >> >> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> >> --- >> This patch is dependent on the patch series >> "iommu/arm-smmu: Add runtime pm/sleep support" >> (https://patchwork.ozlabs.org/cover/946160/) >> --- >> .../devicetree/bindings/iommu/arm,smmu.txt | 25 >> ++++++++++++++++++ >> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 30 >> ++++++++++++++++++++++ >> 2 files changed, 55 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> index 7c71a6ed465a..8e3fe0594e3e 100644 >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >> @@ -18,6 +18,7 @@ conditions. >> "arm,mmu-500" >> "cavium,smmu-v2" >> "qcom,<soc>-smmu-v2", "qcom,smmu-v2" >> + "altr,smmu-v2" > > Can we guarantee that no Altera SoC will ever exist with a different > SMMU implementation, configuration, or clock tree? If we must have > compatibles for SoC-specific integrations, I'd be a lot happier if they > were actually SoC-specific, i.e. at least "altr,stratix10-smmu", or even > something like "altr,gx5500-smmu" if there's a chance of new > incompatible designs being added to the Stratix 10 family in future. >
Good point. I'll get a better compatible string if I pursue this.
> I'm still dubious that we actually need this for MMU-500, though, since > we will always need the TCU clock enabled to do anything, and given the > difficulty in associating particular TBU clocks with whichever domains > might cause allocations into which TBU's TLBs, it seems highly unlikely > that it'll ever be feasible to work at a granularity finer than "all of > the clocks". And at that point the names don't really matter, and we > merely need something like the proposed of_clk_bulk_get()[1], which > works fine regardless of how many TBUs and distinct clocks exist for a > particular MMU-500 configuration and integration. >
Yes, I would prefer to use the standard arm,mmu-500 but with the changes proposed by [2] that this patch was dependent on, it seemed I would need to make a new structure and type.
I like the patch series for devm_clk_bulk_get_all() that includes of_clk_bulk_get(). That would enable my patch to work with minor changes to add bulk_clk to arm-smmu.c. However, the patchset doesn't seem to have been accepted yet.
>> depending on the particular implementation and/or the >> version of the architecture implemented. >> @@ -179,3 +180,27 @@ conditions. >> <&mmcc SMMU_MDP_AHB_CLK>; >> clock-names = "bus", "iface"; >> }; >> + >> + /* Stratix10 arm,smmu-v2 implementation */ >> + smmu5: iommu@fa000000 { >> + compatible = "altr,smmu-v2", "arm,mmu-500", >> + "arm,smmu-v2"; >> + reg = <0xfa000000 0x40000>; >> + #global-interrupts = <2>; >> + #iommu-cells = <1>; >> + clocks = <&clkmgr STRATIX10_L4_MAIN_CLK>; >> + clock-names = "masters"; > > This isn't documented as an actual property, and if it also clocks the > TCU then I'm not sure it's really the most accurate name. > > Robin. > > [1] https://patchwork.kernel.org/patch/10427095/
In the patch I'll remove the clock-names.
I'll keep track of the status of this patch (and [3] from the same patchset). I tested a simple patch that uses devm_clk_bulk_get_all() from these bulk_clk patches and it works well with the standard "arm,mmu-500" compatible.
This patch has dependencies on [2]. It seems like [2] could use the bulk_clk patches in [1] above (in particular [2]'s patch 1/4). The function arm_smmu_fill_clk_data() wouldn't be needed because everything happens in devm_clk_bulk_get_all(). However, those bulk_clk patches have been hanging out there since May.
I'm unclear on how to proceed. Do I continue with dependency on [2] or create a new patch adding the bulk clocks changes in [1] (and [3])?
Thanks for reviewing!
Thor
[2] https://patchwork.kernel.org/patch/10546677/ [3] https://patchwork.kernel.org/patch/10427079/ > >> + interrupt-parent = <&intc>; >> + interrupts = <0 128 4>, /* Global Secure Fault */ >> + <0 129 4>, /* Global Non-secure Fault */ >> + /* 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>; >> + }; >> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi >> b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi >> index d033da401c26..e38ca86d48f6 100644 >> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi >> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi >> @@ -137,6 +137,7 @@ >> reset-names = "stmmaceth", "stmmaceth-ocp"; >> clocks = <&clkmgr STRATIX10_EMAC0_CLK>; >> clock-names = "stmmaceth"; >> + iommus = <&smmu 1>; >> status = "disabled"; >> }; >> @@ -150,6 +151,7 @@ >> reset-names = "stmmaceth", "stmmaceth-ocp"; >> clocks = <&clkmgr STRATIX10_EMAC1_CLK>; >> clock-names = "stmmaceth"; >> + iommus = <&smmu 2>; >> status = "disabled"; >> }; >> @@ -163,6 +165,7 @@ >> reset-names = "stmmaceth", "stmmaceth-ocp"; >> clocks = <&clkmgr STRATIX10_EMAC2_CLK>; >> clock-names = "stmmaceth"; >> + iommus = <&smmu 3>; >> status = "disabled"; >> }; >> @@ -273,6 +276,7 @@ >> clocks = <&clkmgr STRATIX10_L4_MP_CLK>, >> <&clkmgr STRATIX10_SDMMC_CLK>; >> clock-names = "biu", "ciu"; >> + iommus = <&smmu 5>; >> status = "disabled"; >> }; >> @@ -307,6 +311,30 @@ >> altr,modrst-offset = <0x20>; >> }; >> + smmu: iommu@fa000000 { >> + compatible = "altr,smmu-v2", "arm,mmu-500", >> + "arm,smmu-v2"; >> + reg = <0xfa000000 0x40000>; >> + #global-interrupts = <2>; >> + #iommu-cells = <1>; >> + clocks = <&clkmgr STRATIX10_L4_MAIN_CLK>; >> + clock-names = "masters"; >> + interrupt-parent = <&intc>; >> + interrupts = <0 128 4>, /* Global Secure Fault */ >> + <0 129 4>, /* Global Non-secure Fault */ >> + /* 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@ffda4000 { >> compatible = "snps,dw-apb-ssi"; >> #address-cells = <1>; >> @@ -416,6 +444,7 @@ >> resets = <&rst USB0_RESET>, <&rst USB0_OCP_RESET>; >> reset-names = "dwc2", "dwc2-ecc"; >> clocks = <&clkmgr STRATIX10_USB_CLK>; >> + iommus = <&smmu 6>; >> status = "disabled"; >> }; >> @@ -428,6 +457,7 @@ >> resets = <&rst USB1_RESET>, <&rst USB1_OCP_RESET>; >> reset-names = "dwc2", "dwc2-ecc"; >> clocks = <&clkmgr STRATIX10_USB_CLK>; >> + iommus = <&smmu 7>; >> status = "disabled"; >> }; >> >
| |