Re: [Freedreno] [PATCH v8 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant

[Tomasz Figa]

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Fri, Feb 9, 2018 at 7:57 PM, Vivek Gautam
 wrote:
> qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
> clock and power requirements. This smmu core is used with
> multiple masters on msm8996, viz. mdss, video, etc.
> Add bindings for the same.
>
> Signed-off-by: Vivek Gautam 
> Reviewed-by: Rob Herring 
> ---
>
> Changes in v8:
>  - Added the missing IOMMU_OF_DECLARE declaration for "qcom,smmu-v2"
>
>  .../devicetree/bindings/iommu/arm,smmu.txt | 43 
> ++
>  drivers/iommu/arm-smmu.c   | 14 +++
>  2 files changed, 57 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 8a6ffce12af5..169222ae2706 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -17,10 +17,19 @@ conditions.
>  "arm,mmu-401"
>  "arm,mmu-500"
>  "cavium,smmu-v2"
> +"qcom,-smmu-v2", "qcom,smmu-v2"
>
>depending on the particular implementation and/or the
>version of the architecture implemented.
>
> +  A number of Qcom SoCs use qcom,smmu-v2 version of the IP.
> +  "qcom,-smmu-v2" represents a soc specific compatible
> +  string that should be present along with the "qcom,smmu-v2"
> +  to facilitate SoC specific clocks/power connections and to
> +  address specific bug fixes.
> +  An example string would be -
> +  "qcom,msm8996-smmu-v2", "qcom,smmu-v2".

Hmm, I remember that for  and similar wildcards we required
explicitly listing allowed values. Rob, has it changed since I
stumbled upon such thing last time (or I just got it wrong before)?

> +
>  - reg   : Base address and size of the SMMU.
>
>  - #global-interrupts : The number of global interrupts exposed by the
> @@ -71,6 +80,23 @@ conditions.
>or using stream matching with #iommu-cells = <2>, and
>may be ignored if present in such cases.
>
> +- clock-names:Should be "bus", and "iface" for "qcom,smmu-v2"
> +  implementation.
> +
> +  "bus" clock for "qcom,smmu-v2" is required for downstream
> +  bus access and for the smmu ptw.
> +
> +  "iface" clock is required to access smmu's registers 
> through
> +  the TCU's programming interface.

nit: Could we have it in a bit more structured way? E.g.

- clock-names: List of names of clocks input to the device. The
required list depends on particular implementation and is as follows:
 - for "qcom,smmu-v2":
   - "bus": clock required for downstream bus access and for the smmu ptw,
   - "iface":  clock required to access smmu's registers through the
TCU's programming interface.
 - unspecified for other implementations.

(+/- wrapping)

> +
> +- clocks: Phandles for respective clocks described by clock-names.

Phandle is just a pointer to another node. Clocks are however
specified using a phandle and a number of arguments, depending on the
clock controller. I'd change it to:

- clocks: Specifiers for all clocks listed in the clock-names
property, as per generic clock bindings.

> +
> +- power-domains:  Phandles to SMMU's power domain specifier. This is
> +  required even if SMMU belongs to the master's power
> +  domain, as the SMMU will have to be enabled and
> +  accessed before master gets enabled and linked to its
> +  SMMU.

From DT point of view, the relationship of SMMU belonging to a master
device doesn't exist. The power-domains property needs to be properly
specified for all devices within power domains represented in DT, with
an exception of the case when the parent-child relationship is
explicitly stated in DT by child devices being represented by child
nodes of the parent device node.

- power-domains: Specifiers for power domains required to be powered
on for the SMMU to operate, as per generic power domain bindings.

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[Freedreno] [PATCH v8 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant

[Vivek Gautam]

qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
clock and power requirements. This smmu core is used with
multiple masters on msm8996, viz. mdss, video, etc.
Add bindings for the same.

Signed-off-by: Vivek Gautam 
Reviewed-by: Rob Herring 
---

Changes in v8:
 - Added the missing IOMMU_OF_DECLARE declaration for "qcom,smmu-v2"

 .../devicetree/bindings/iommu/arm,smmu.txt | 43 ++
 drivers/iommu/arm-smmu.c   | 14 +++
 2 files changed, 57 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 8a6ffce12af5..169222ae2706 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -17,10 +17,19 @@ conditions.
 "arm,mmu-401"
 "arm,mmu-500"
 "cavium,smmu-v2"
+"qcom,-smmu-v2", "qcom,smmu-v2"
 
   depending on the particular implementation and/or the
   version of the architecture implemented.
 
+  A number of Qcom SoCs use qcom,smmu-v2 version of the IP.
+  "qcom,-smmu-v2" represents a soc specific compatible
+  string that should be present along with the "qcom,smmu-v2"
+  to facilitate SoC specific clocks/power connections and to
+  address specific bug fixes.
+  An example string would be -
+  "qcom,msm8996-smmu-v2", "qcom,smmu-v2".
+
 - reg   : Base address and size of the SMMU.
 
 - #global-interrupts : The number of global interrupts exposed by the
@@ -71,6 +80,23 @@ conditions.
   or using stream matching with #iommu-cells = <2>, and
   may be ignored if present in such cases.
 
+- clock-names:Should be "bus", and "iface" for "qcom,smmu-v2"
+  implementation.
+
+  "bus" clock for "qcom,smmu-v2" is required for downstream
+  bus access and for the smmu ptw.
+
+  "iface" clock is required to access smmu's registers through
+  the TCU's programming interface.
+
+- clocks: Phandles for respective clocks described by clock-names.
+
+- power-domains:  Phandles to SMMU's power domain specifier. This is
+  required even if SMMU belongs to the master's power
+  domain, as the SMMU will have to be enabled and
+  accessed before master gets enabled and linked to its
+  SMMU.
+
 ** Deprecated properties:
 
 - mmu-masters (deprecated in favour of the generic "iommus" binding) :
@@ -137,3 +163,20 @@ conditions.
 iommu-map = <0  0 0x400>;
 ...
 };
+
+   /* Qcom's arm,smmu-v2 implementation */
+   smmu4: iommu {
+   compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+   reg = <0xd0 0x1>;
+
+   #global-interrupts = <1>;
+   interrupts = ,
+,
+;
+   #iommu-cells = <1>;
+   power-domains = < MDSS_GDSC>;
+
+   clocks = < SMMU_MDP_AXI_CLK>,
+< SMMU_MDP_AHB_CLK>;
+   clock-names = "bus", "iface";
+   };
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c7e924d553bd..40da3f251acf 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -119,6 +119,7 @@ enum arm_smmu_implementation {
GENERIC_SMMU,
ARM_MMU500,
CAVIUM_SMMUV2,
+   QCOM_SMMUV2,
 };
 
 struct arm_smmu_s2cr {
@@ -1950,6 +1951,17 @@ struct arm_smmu_match_data {
 ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
 ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
 
+static const char * const qcom_smmuv2_clks[] = {
+   "bus", "iface",
+};
+
+static const struct arm_smmu_match_data qcom_smmuv2 = {
+   .version = ARM_SMMU_V2,
+   .model = QCOM_SMMUV2,
+   .clks = qcom_smmuv2_clks,
+   .num_clks = ARRAY_SIZE(qcom_smmuv2_clks),
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v1", .data = _generic_v1 },
{ .compatible = "arm,smmu-v2", .data = _generic_v2 },
@@ -1957,6 +1969,7 @@ struct arm_smmu_match_data {
{ .compatible = "arm,mmu-401", .data = _mmu401 },
{ .compatible = "arm,mmu-500", .data = _mmu500 },
{ .compatible = "cavium,smmu-v2", .data = _smmuv2 },
+   { .compatible = "qcom,smmu-v2", .data = _smmuv2 },
{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
@@ -2319,6 +2332,7 @@ static int __maybe_unused arm_smmu_runtime_suspend(struct 
device *dev)
 IOMMU_OF_DECLARE(arm_mmu401, "arm,mmu-401");
 IOMMU_OF_DECLARE(arm_mmu500, "arm,mmu-500");