[PATCH 0/5] imx8mp: add support for the IMX AIPSTZ bridge

[PATCH 0/5] imx8mp: add support for the IMX AIPSTZ bridge

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The AIPSTZ bridge offers some security-related configurations which can
be used to restrict master access to certain peripherals on the bridge.

Normally, this could be done from a secure environment such as ATF before
Linux boots but the configuration of AIPSTZ5 is lost each time the power
domain is powered off and then powered on. Because of this, it has to be
configured each time the power domain is turned on and before any master
tries to access the peripherals (e.g: AP, CM7, DSP, on i.MX8MP).

The child-parent relationship between the bridge and its peripherals
should guarantee that the bridge is configured before the AP attempts
to access the IPs.

Other masters should use the 'access-controllers' property to enforce
a dependency between their device and the bridge device (see the DSP,
for example).

At the moment, we only want to apply a default, more relaxed
configuration, which is why the number of access controller cells
is 0.

The initial version of the series can be found at [1]. The new version
should provide better management of the device dependencies.

[1]: https://lore.kernel.org/linux-arm-kernel/20241119130726.2761726-1-daniel.baluta@nxp.com/


Laurentiu Mihalcea (5):
  dt-bindings: bus: add documentation for the IMX AIPSTZ bridge
  dt-bindings: dsp: fsl,dsp: document 'access-controllers' property
  bus: add driver for IMX AIPSTZ bridge
  arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'
  arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'

 .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 62 +++++++++++++
 .../devicetree/bindings/dsp/fsl,dsp.yaml      |  3 +
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  9 +-
 drivers/bus/Kconfig                           |  6 ++
 drivers/bus/Makefile                          |  1 +
 drivers/bus/imx-aipstz.c                      | 92 +++++++++++++++++++
 6 files changed, 170 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
 create mode 100644 drivers/bus/imx-aipstz.c

-- 
2.34.1

[PATCH 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Add documentation for IMX AIPSTZ bridge.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml

diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
new file mode 100644
index 000000000000..b0d6eaf70a84
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/fsl,imx8mp-aipstz.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Secure AHB to IP Slave bus (AIPSTZ) bridge
+
+description:
+  The secure AIPS bridge (AIPSTZ) acts as a bridge for AHB masters
+  issuing transactions to IP Slave peripherals. Additionally, this module
+  offers access control configurations meant to restrict which peripherals
+  a master can access.
+
+maintainers:
+  - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
+
+select:
+  properties:
+    compatible:
+      contains:
+        const: fsl,imx8mp-aipstz
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: fsl,imx8mp-aipstz
+      - const: simple-bus
+
+  reg:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  "#access-controller-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - power-domains
+  - "#access-controller-cells"
+
+allOf:
+  - $ref: /schemas/simple-bus.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    bus@30df0000 {
+      compatible = "fsl,imx8mp-aipstz", "simple-bus";
+      reg = <0x30df0000 0x10000>;
+      power-domains = <&pgc_audio>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      #access-controller-cells = <0>;
+      ranges;
+    };
-- 
2.34.1

Re: [PATCH 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge

[Frank Li]

On Fri, Feb 21, 2025 at 02:19:05PM -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>
> Add documentation for IMX AIPSTZ bridge.
>
> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
>
> diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
> new file mode 100644
> index 000000000000..b0d6eaf70a84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bus/fsl,imx8mp-aipstz.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Secure AHB to IP Slave bus (AIPSTZ) bridge
> +
> +description:
> +  The secure AIPS bridge (AIPSTZ) acts as a bridge for AHB masters
> +  issuing transactions to IP Slave peripherals. Additionally, this module
> +  offers access control configurations meant to restrict which peripherals
> +  a master can access.
> +
> +maintainers:
> +  - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: fsl,imx8mp-aipstz
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: fsl,imx8mp-aipstz
> +      - const: simple-bus
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  "#access-controller-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - power-domains
> +  - "#access-controller-cells"
> +
> +allOf:
> +  - $ref: /schemas/simple-bus.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    bus@30df0000 {
> +      compatible = "fsl,imx8mp-aipstz", "simple-bus";
> +      reg = <0x30df0000 0x10000>;
> +      power-domains = <&pgc_audio>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      #access-controller-cells = <0>;
> +      ranges;
> +    };
> --
> 2.34.1
>

Re: [PATCH 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge

[Krzysztof Kozlowski]

On Fri, Feb 21, 2025 at 02:19:05PM -0500, Laurentiu Mihalcea wrote:
> +properties:
> +  compatible:
> +    items:
> +      - const: fsl,imx8mp-aipstz
> +      - const: simple-bus
> +
> +  reg:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1

That's not a simple bus anymore. simple-bus does not have reg. Neither
power domain really, because this means children depends on this device,
so again: not simple.

Best regards,
Krzysztof

[PATCH 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Some DSP instances may have their access to certain peripherals
conditioned by a bus access controller such as the one from the
AIPSTZ bridge.

Add the optional 'access-controllers' property, which may be used
in such cases.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
index ab93ffd3d2e5..869df7fcece0 100644
--- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -82,6 +82,9 @@ properties:
     description:
       Phandle to syscon block which provide access for processor enablement
 
+  access-controllers:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.34.1

Re: [PATCH 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property

[Frank Li]

On Fri, Feb 21, 2025 at 02:19:06PM -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>
> Some DSP instances may have their access to certain peripherals
> conditioned by a bus access controller such as the one from the
> AIPSTZ bridge.
>
> Add the optional 'access-controllers' property, which may be used
> in such cases.
>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  Documentation/devicetree/bindings/dsp/fsl,dsp.yaml | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index ab93ffd3d2e5..869df7fcece0 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -82,6 +82,9 @@ properties:
>      description:
>        Phandle to syscon block which provide access for processor enablement
>
> +  access-controllers:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> --
> 2.34.1
>

Re: [PATCH 2/5] dt-bindings: dsp: fsl,dsp: document 'access-controllers' property

[Rob Herring (Arm)]

On Fri, 21 Feb 2025 14:19:06 -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> 
> Some DSP instances may have their access to certain peripherals
> conditioned by a bus access controller such as the one from the
> AIPSTZ bridge.
> 
> Add the optional 'access-controllers' property, which may be used
> in such cases.
> 
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  Documentation/devicetree/bindings/dsp/fsl,dsp.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 3/5] bus: add driver for IMX AIPSTZ bridge

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The secure AHB to IP Slave (AIPSTZ) bus bridge provides access control
configurations meant to restrict access to certain peripherals.
Some of the configurations include:

	1) Marking masters as trusted for R/W. Based on this
	(and the configuration of the accessed peripheral), the bridge
	may choose to abort the R/W transactions issued by certain
	masters.

	2) Allowing/disallowing write accesses to peripherals.

Add driver for this IP. Since there's currently no framework for
access controllers (and since there's currently no need for having
flexibility w.r.t the configurations) all this driver does is it
applies a relaxed, "default" configuration, in which all masters
are trusted for R/W.

Note that some instances of this IP may be tied to a power domain and may
lose their configuration when the domain is powered off. This is why the
configuration has to be restored when the domain is powered on.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 drivers/bus/Kconfig      |  6 +++
 drivers/bus/Makefile     |  1 +
 drivers/bus/imx-aipstz.c | 92 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 drivers/bus/imx-aipstz.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index ff669a8ccad9..fe7600283e70 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -87,6 +87,12 @@ config HISILICON_LPC
 	  Driver to enable I/O access to devices attached to the Low Pin
 	  Count bus on the HiSilicon Hip06/7 SoC.
 
+config IMX_AIPSTZ
+	tristate "Support for IMX Secure AHB to IP Slave bus (AIPSTZ) bridge"
+	depends on ARCH_MXC
+	help
+	  Enable support for IMX AIPSTZ bridge.
+
 config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index cddd4984d6af..8e693fe8a03a 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
 
 obj-$(CONFIG_BT1_APB)		+= bt1-apb.o
 obj-$(CONFIG_BT1_AXI)		+= bt1-axi.o
+obj-$(CONFIG_IMX_AIPSTZ)	+= imx-aipstz.o
 obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
 obj-$(CONFIG_INTEL_IXP4XX_EB)	+= intel-ixp4xx-eb.o
 obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
diff --git a/drivers/bus/imx-aipstz.c b/drivers/bus/imx-aipstz.c
new file mode 100644
index 000000000000..75696eac8ba2
--- /dev/null
+++ b/drivers/bus/imx-aipstz.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#define IMX_AIPSTZ_MPR0 0x0
+
+struct imx_aipstz_config {
+	u32 mpr0;
+};
+
+static void imx_aipstz_apply_default(void __iomem *base,
+				     const struct imx_aipstz_config *default_cfg)
+{
+	writel(default_cfg->mpr0, base + IMX_AIPSTZ_MPR0);
+}
+
+static int imx_aipstz_probe(struct platform_device *pdev)
+{
+	void __iomem *base;
+	const struct imx_aipstz_config *default_cfg;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(base))
+		return dev_err_probe(&pdev->dev, -ENOMEM,
+				     "failed to get/ioremap memory\n");
+
+	default_cfg = of_device_get_match_data(&pdev->dev);
+
+	imx_aipstz_apply_default(base, default_cfg);
+
+	dev_set_drvdata(&pdev->dev, base);
+
+	pm_runtime_set_active(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
+
+	return devm_of_platform_populate(&pdev->dev);
+}
+
+static int imx_aipstz_runtime_resume(struct device *dev)
+{
+	void __iomem *base;
+	const struct imx_aipstz_config *default_cfg;
+
+	base = dev_get_drvdata(dev);
+	default_cfg = of_device_get_match_data(dev);
+
+	/* restore potentially lost configuration during domain power-off */
+	imx_aipstz_apply_default(base, default_cfg);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_aipstz_pm_ops = {
+	RUNTIME_PM_OPS(NULL, imx_aipstz_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
+/*
+ * following configuration is equivalent to:
+ *	masters 0-7 => trusted for R/W + use AHB's HPROT[1] to det. privilege
+ */
+static const struct imx_aipstz_config imx8mp_aipstz_default_cfg = {
+	.mpr0 = 0x77777777,
+};
+
+static const struct of_device_id imx_aipstz_of_ids[] = {
+	{ .compatible = "fsl,imx8mp-aipstz", .data = &imx8mp_aipstz_default_cfg },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, imx_aipstz_of_ids);
+
+static struct platform_driver imx_aipstz_of_driver = {
+	.probe = imx_aipstz_probe,
+	.driver = {
+		.name = "imx-aipstz",
+		.of_match_table = imx_aipstz_of_ids,
+		.pm = pm_ptr(&imx_aipstz_pm_ops),
+	},
+};
+module_platform_driver(imx_aipstz_of_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("IMX secure AHB to IP Slave bus (AIPSTZ) bridge driver");
+MODULE_AUTHOR("Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>");
-- 
2.34.1

Re: [PATCH 3/5] bus: add driver for IMX AIPSTZ bridge

[Frank Li]

On Fri, Feb 21, 2025 at 02:19:07PM -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>
> The secure AHB to IP Slave (AIPSTZ) bus bridge provides access control
> configurations meant to restrict access to certain peripherals.
> Some of the configurations include:
>
> 	1) Marking masters as trusted for R/W. Based on this
> 	(and the configuration of the accessed peripheral), the bridge
> 	may choose to abort the R/W transactions issued by certain
> 	masters.
>
> 	2) Allowing/disallowing write accesses to peripherals.
>
> Add driver for this IP. Since there's currently no framework for
> access controllers (and since there's currently no need for having
> flexibility w.r.t the configurations) all this driver does is it
> applies a relaxed, "default" configuration, in which all masters
> are trusted for R/W.
>
> Note that some instances of this IP may be tied to a power domain and may

Can you talk about detail which instance will lost power?

> lose their configuration when the domain is powered off. This is why the
> configuration has to be restored when the domain is powered on.
>
> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  drivers/bus/Kconfig      |  6 +++
>  drivers/bus/Makefile     |  1 +
>  drivers/bus/imx-aipstz.c | 92 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 drivers/bus/imx-aipstz.c
>
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index ff669a8ccad9..fe7600283e70 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -87,6 +87,12 @@ config HISILICON_LPC
>  	  Driver to enable I/O access to devices attached to the Low Pin
>  	  Count bus on the HiSilicon Hip06/7 SoC.
>
> +config IMX_AIPSTZ
> +	tristate "Support for IMX Secure AHB to IP Slave bus (AIPSTZ) bridge"
> +	depends on ARCH_MXC
> +	help
> +	  Enable support for IMX AIPSTZ bridge.
> +
>  config IMX_WEIM
>  	bool "Freescale EIM DRIVER"
>  	depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index cddd4984d6af..8e693fe8a03a 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
>
>  obj-$(CONFIG_BT1_APB)		+= bt1-apb.o
>  obj-$(CONFIG_BT1_AXI)		+= bt1-axi.o
> +obj-$(CONFIG_IMX_AIPSTZ)	+= imx-aipstz.o
>  obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
>  obj-$(CONFIG_INTEL_IXP4XX_EB)	+= intel-ixp4xx-eb.o
>  obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
> diff --git a/drivers/bus/imx-aipstz.c b/drivers/bus/imx-aipstz.c
> new file mode 100644
> index 000000000000..75696eac8ba2
> --- /dev/null
> +++ b/drivers/bus/imx-aipstz.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define IMX_AIPSTZ_MPR0 0x0
> +
> +struct imx_aipstz_config {
> +	u32 mpr0;
> +};
> +
> +static void imx_aipstz_apply_default(void __iomem *base,
> +				     const struct imx_aipstz_config *default_cfg)
> +{
> +	writel(default_cfg->mpr0, base + IMX_AIPSTZ_MPR0);
> +}
> +
> +static int imx_aipstz_probe(struct platform_device *pdev)
> +{
> +	void __iomem *base;
> +	const struct imx_aipstz_config *default_cfg;

try keep reverse christmas order.

> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +				     "failed to get/ioremap memory\n");
> +
> +	default_cfg = of_device_get_match_data(&pdev->dev);
> +
> +	imx_aipstz_apply_default(base, default_cfg);
> +
> +	dev_set_drvdata(&pdev->dev, base);
> +
> +	pm_runtime_set_active(&pdev->dev);
> +	devm_pm_runtime_enable(&pdev->dev);
> +
> +	return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static int imx_aipstz_runtime_resume(struct device *dev)
> +{
> +	void __iomem *base;
> +	const struct imx_aipstz_config *default_cfg;
> +
> +	base = dev_get_drvdata(dev);
> +	default_cfg = of_device_get_match_data(dev);
> +
> +	/* restore potentially lost configuration during domain power-off */
> +	imx_aipstz_apply_default(base, default_cfg);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops imx_aipstz_pm_ops = {
> +	RUNTIME_PM_OPS(NULL, imx_aipstz_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +
> +/*
> + * following configuration is equivalent to:
> + *	masters 0-7 => trusted for R/W + use AHB's HPROT[1] to det. privilege
> + */
> +static const struct imx_aipstz_config imx8mp_aipstz_default_cfg = {
> +	.mpr0 = 0x77777777,
> +};
> +
> +static const struct of_device_id imx_aipstz_of_ids[] = {
> +	{ .compatible = "fsl,imx8mp-aipstz", .data = &imx8mp_aipstz_default_cfg },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_aipstz_of_ids);
> +
> +static struct platform_driver imx_aipstz_of_driver = {
> +	.probe = imx_aipstz_probe,
> +	.driver = {
> +		.name = "imx-aipstz",
> +		.of_match_table = imx_aipstz_of_ids,
> +		.pm = pm_ptr(&imx_aipstz_pm_ops),
> +	},
> +};
> +module_platform_driver(imx_aipstz_of_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("IMX secure AHB to IP Slave bus (AIPSTZ) bridge driver");
> +MODULE_AUTHOR("Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>");
> --
> 2.34.1
>

Re: [PATCH 3/5] bus: add driver for IMX AIPSTZ bridge

[Marco Felsch]

Hi Laurentiu,

thanks for your patch.

On 25-02-21, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> 
> The secure AHB to IP Slave (AIPSTZ) bus bridge provides access control
> configurations meant to restrict access to certain peripherals.
> Some of the configurations include:
> 
> 	1) Marking masters as trusted for R/W. Based on this
> 	(and the configuration of the accessed peripheral), the bridge
> 	may choose to abort the R/W transactions issued by certain
> 	masters.

Setting these bits requires very often that the core is running at EL3
(e.g. secure-monitor) which is not the case for Linux. Can you please
provide more information how Linux can set these bits?

Regards,
  Marco


> 
> 	2) Allowing/disallowing write accesses to peripherals.
> 
> Add driver for this IP. Since there's currently no framework for
> access controllers (and since there's currently no need for having
> flexibility w.r.t the configurations) all this driver does is it
> applies a relaxed, "default" configuration, in which all masters
> are trusted for R/W.
> 
> Note that some instances of this IP may be tied to a power domain and may
> lose their configuration when the domain is powered off. This is why the
> configuration has to be restored when the domain is powered on.
> 
> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  drivers/bus/Kconfig      |  6 +++
>  drivers/bus/Makefile     |  1 +
>  drivers/bus/imx-aipstz.c | 92 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 drivers/bus/imx-aipstz.c
> 
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index ff669a8ccad9..fe7600283e70 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -87,6 +87,12 @@ config HISILICON_LPC
>  	  Driver to enable I/O access to devices attached to the Low Pin
>  	  Count bus on the HiSilicon Hip06/7 SoC.
>  
> +config IMX_AIPSTZ
> +	tristate "Support for IMX Secure AHB to IP Slave bus (AIPSTZ) bridge"
> +	depends on ARCH_MXC
> +	help
> +	  Enable support for IMX AIPSTZ bridge.
> +
>  config IMX_WEIM
>  	bool "Freescale EIM DRIVER"
>  	depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index cddd4984d6af..8e693fe8a03a 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_FSL_MC_BUS)	+= fsl-mc/
>  
>  obj-$(CONFIG_BT1_APB)		+= bt1-apb.o
>  obj-$(CONFIG_BT1_AXI)		+= bt1-axi.o
> +obj-$(CONFIG_IMX_AIPSTZ)	+= imx-aipstz.o
>  obj-$(CONFIG_IMX_WEIM)		+= imx-weim.o
>  obj-$(CONFIG_INTEL_IXP4XX_EB)	+= intel-ixp4xx-eb.o
>  obj-$(CONFIG_MIPS_CDMM)		+= mips_cdmm.o
> diff --git a/drivers/bus/imx-aipstz.c b/drivers/bus/imx-aipstz.c
> new file mode 100644
> index 000000000000..75696eac8ba2
> --- /dev/null
> +++ b/drivers/bus/imx-aipstz.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define IMX_AIPSTZ_MPR0 0x0
> +
> +struct imx_aipstz_config {
> +	u32 mpr0;
> +};
> +
> +static void imx_aipstz_apply_default(void __iomem *base,
> +				     const struct imx_aipstz_config *default_cfg)
> +{
> +	writel(default_cfg->mpr0, base + IMX_AIPSTZ_MPR0);
> +}
> +
> +static int imx_aipstz_probe(struct platform_device *pdev)
> +{
> +	void __iomem *base;
> +	const struct imx_aipstz_config *default_cfg;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> +				     "failed to get/ioremap memory\n");
> +
> +	default_cfg = of_device_get_match_data(&pdev->dev);
> +
> +	imx_aipstz_apply_default(base, default_cfg);
> +
> +	dev_set_drvdata(&pdev->dev, base);
> +
> +	pm_runtime_set_active(&pdev->dev);
> +	devm_pm_runtime_enable(&pdev->dev);
> +
> +	return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static int imx_aipstz_runtime_resume(struct device *dev)
> +{
> +	void __iomem *base;
> +	const struct imx_aipstz_config *default_cfg;
> +
> +	base = dev_get_drvdata(dev);
> +	default_cfg = of_device_get_match_data(dev);
> +
> +	/* restore potentially lost configuration during domain power-off */
> +	imx_aipstz_apply_default(base, default_cfg);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops imx_aipstz_pm_ops = {
> +	RUNTIME_PM_OPS(NULL, imx_aipstz_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +
> +/*
> + * following configuration is equivalent to:
> + *	masters 0-7 => trusted for R/W + use AHB's HPROT[1] to det. privilege
> + */
> +static const struct imx_aipstz_config imx8mp_aipstz_default_cfg = {
> +	.mpr0 = 0x77777777,
> +};
> +
> +static const struct of_device_id imx_aipstz_of_ids[] = {
> +	{ .compatible = "fsl,imx8mp-aipstz", .data = &imx8mp_aipstz_default_cfg },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_aipstz_of_ids);
> +
> +static struct platform_driver imx_aipstz_of_driver = {
> +	.probe = imx_aipstz_probe,
> +	.driver = {
> +		.name = "imx-aipstz",
> +		.of_match_table = imx_aipstz_of_ids,
> +		.pm = pm_ptr(&imx_aipstz_pm_ops),
> +	},
> +};
> +module_platform_driver(imx_aipstz_of_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("IMX secure AHB to IP Slave bus (AIPSTZ) bridge driver");
> +MODULE_AUTHOR("Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>");
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH 3/5] bus: add driver for IMX AIPSTZ bridge

[Mihalcea Laurentiu]

On 24.02.2025 09:55, Marco Felsch wrote:
> Hi Laurentiu,
>
> thanks for your patch.
>
> On 25-02-21, Laurentiu Mihalcea wrote:
>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>
>> The secure AHB to IP Slave (AIPSTZ) bus bridge provides access control
>> configurations meant to restrict access to certain peripherals.
>> Some of the configurations include:
>>
>> 	1) Marking masters as trusted for R/W. Based on this
>> 	(and the configuration of the accessed peripheral), the bridge
>> 	may choose to abort the R/W transactions issued by certain
>> 	masters.
> Setting these bits requires very often that the core is running at EL3
> (e.g. secure-monitor) which is not the case for Linux. Can you please
> provide more information how Linux can set these bits?
>
> Regards,
>   Marco


In this particular case, as far as I was able to understand, NS EL1 has enough

privilege to program this IP. This is why Linux can do it.

[PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

AIPS5 is actually AIPSTZ5 as it offers some security-related
configurations. Since these configurations need to be applied before
accessing any of the peripherals on the bus, it's better to make AIPSTZ5
be their parent instead of keeping AIPS5 and adding a child node for
AIPSTZ5. Also, because of the security configurations, the address space
of the bus has to be changed to that of the configuration registers.

Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the
missing 'power-domains' property. The domain needs to be powered on before
attempting to configure the security-related registers.

The DT node name is not changed to avoid potential issues with DTs in
which this node is referenced.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index e0d3b8cba221..a1d9b834d2da 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
 			};
 		};
 
-		aips5: bus@30c00000 {
-			compatible = "fsl,aips-bus", "simple-bus";
-			reg = <0x30c00000 0x400000>;
+		aips5: bus@30df0000 {
+			compatible = "fsl,imx8mp-aipstz", "simple-bus";
+			reg = <0x30df0000 0x10000>;
+			power-domains = <&pgc_audio>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#access-controller-cells = <0>;
 			ranges;
 
 			spba-bus@30c00000 {
-- 
2.34.1

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Frank Li]

On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>
> AIPS5 is actually AIPSTZ5 as it offers some security-related
> configurations. Since these configurations need to be applied before
> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> be their parent instead of keeping AIPS5 and adding a child node for
> AIPSTZ5. Also, because of the security configurations, the address space
> of the bus has to be changed to that of the configuration registers.

The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
config address part in your drivers.

Frank

>
> Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the
> missing 'power-domains' property. The domain needs to be powered on before
> attempting to configure the security-related registers.
>
> The DT node name is not changed to avoid potential issues with DTs in
> which this node is referenced.
>
> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index e0d3b8cba221..a1d9b834d2da 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
>  			};
>  		};
>
> -		aips5: bus@30c00000 {
> -			compatible = "fsl,aips-bus", "simple-bus";
> -			reg = <0x30c00000 0x400000>;
> +		aips5: bus@30df0000 {
> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> +			reg = <0x30df0000 0x10000>;
> +			power-domains = <&pgc_audio>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +			#access-controller-cells = <0>;
>  			ranges;
>
>  			spba-bus@30c00000 {
> --
> 2.34.1
>

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Mihalcea Laurentiu]

On 21.02.2025 21:56, Frank Li wrote:
> On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>
>> AIPS5 is actually AIPSTZ5 as it offers some security-related
>> configurations. Since these configurations need to be applied before
>> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
>> be their parent instead of keeping AIPS5 and adding a child node for
>> AIPSTZ5. Also, because of the security configurations, the address space
>> of the bus has to be changed to that of the configuration registers.
> The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> config address part in your drivers.
>
> Frank


Any concerns/anything wrong with current approach?


I find it a bit awkward to have the whole bus address space

in the DT given that we're only interested in using the access

controller register space.


I'm fine with the approach you suggested but I don't see a

reason for using it?


>
>> Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the
>> missing 'power-domains' property. The domain needs to be powered on before
>> attempting to configure the security-related registers.
>>
>> The DT node name is not changed to avoid potential issues with DTs in
>> which this node is referenced.
>>
>> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>> ---
>>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>> index e0d3b8cba221..a1d9b834d2da 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
>>  			};
>>  		};
>>
>> -		aips5: bus@30c00000 {
>> -			compatible = "fsl,aips-bus", "simple-bus";
>> -			reg = <0x30c00000 0x400000>;
>> +		aips5: bus@30df0000 {
>> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
>> +			reg = <0x30df0000 0x10000>;
>> +			power-domains = <&pgc_audio>;
>>  			#address-cells = <1>;
>>  			#size-cells = <1>;
>> +			#access-controller-cells = <0>;
>>  			ranges;
>>
>>  			spba-bus@30c00000 {
>> --
>> 2.34.1
>>

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Frank Li]

On Tue, Feb 25, 2025 at 04:14:34PM +0200, Mihalcea Laurentiu wrote:
>
> On 21.02.2025 21:56, Frank Li wrote:
> > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>
> >> AIPS5 is actually AIPSTZ5 as it offers some security-related
> >> configurations. Since these configurations need to be applied before
> >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> >> be their parent instead of keeping AIPS5 and adding a child node for
> >> AIPSTZ5. Also, because of the security configurations, the address space
> >> of the bus has to be changed to that of the configuration registers.
> > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> > config address part in your drivers.
> >
> > Frank
>
>
> Any concerns/anything wrong with current approach?
>
>
> I find it a bit awkward to have the whole bus address space
>
> in the DT given that we're only interested in using the access
>
> controller register space.
>
>
> I'm fine with the approach you suggested but I don't see a
>
> reason for using it?

After second through, reg should indicate only used space. The current
method is good enough. Just need figure out KK's comment about 'simple-bus'

Frank

>
>
> >
> >> Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the
> >> missing 'power-domains' property. The domain needs to be powered on before
> >> attempting to configure the security-related registers.
> >>
> >> The DT node name is not changed to avoid potential issues with DTs in
> >> which this node is referenced.
> >>
> >> Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
> >> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >> ---
> >>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> index e0d3b8cba221..a1d9b834d2da 100644
> >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> >>  			};
> >>  		};
> >>
> >> -		aips5: bus@30c00000 {
> >> -			compatible = "fsl,aips-bus", "simple-bus";
> >> -			reg = <0x30c00000 0x400000>;
> >> +		aips5: bus@30df0000 {
> >> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> >> +			reg = <0x30df0000 0x10000>;
> >> +			power-domains = <&pgc_audio>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <1>;
> >> +			#access-controller-cells = <0>;
> >>  			ranges;
> >>
> >>  			spba-bus@30c00000 {
> >> --
> >> 2.34.1
> >>

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]

On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> 
> On 21.02.2025 21:56, Frank Li wrote:
> > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>
> >> AIPS5 is actually AIPSTZ5 as it offers some security-related
> >> configurations. Since these configurations need to be applied before
> >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> >> be their parent instead of keeping AIPS5 and adding a child node for
> >> AIPSTZ5. Also, because of the security configurations, the address space
> >> of the bus has to be changed to that of the configuration registers.
> > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> > config address part in your drivers.
> >
> > Frank
> 
> 
> Any concerns/anything wrong with current approach?
> 
> 
> I find it a bit awkward to have the whole bus address space
> in the DT given that we're only interested in using the access
> controller register space.
> 
> 
> I'm fine with the approach you suggested but I don't see a
> reason for using it?

Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
Applications Processor Reference Manual, Rev. 3, 08/2024), the
AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
different than the bus configuration. Why not model it as part of the
bus?

> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> index e0d3b8cba221..a1d9b834d2da 100644
> >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> >>  			};
> >>  		};
> >>
> >> -		aips5: bus@30c00000 {
> >> -			compatible = "fsl,aips-bus", "simple-bus";
> >> -			reg = <0x30c00000 0x400000>;
> >> +		aips5: bus@30df0000 {
                       ^^^^^^^^^^^^
> >> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> >> +			reg = <0x30df0000 0x10000>;
> >> +			power-domains = <&pgc_audio>;
> >>  			#address-cells = <1>;
> >>  			#size-cells = <1>;
> >> +			#access-controller-cells = <0>;
> >>  			ranges;
> >>
> >>  			spba-bus@30c00000 {
                        ^^^^^^^^^^^^^^^^^

This looks very strange: The aips5 bus starts at 0x30df0000 and has a
child bus starting at 0x30c00000?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Frank Li]

On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
> On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> >
> > On 21.02.2025 21:56, Frank Li wrote:
> > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> > >>
> > >> AIPS5 is actually AIPSTZ5 as it offers some security-related
> > >> configurations. Since these configurations need to be applied before
> > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> > >> be their parent instead of keeping AIPS5 and adding a child node for
> > >> AIPSTZ5. Also, because of the security configurations, the address space
> > >> of the bus has to be changed to that of the configuration registers.
> > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> > > config address part in your drivers.
> > >
> > > Frank
> >
> >
> > Any concerns/anything wrong with current approach?
> >
> >
> > I find it a bit awkward to have the whole bus address space
> > in the DT given that we're only interested in using the access
> > controller register space.
> >
> >
> > I'm fine with the approach you suggested but I don't see a
> > reason for using it?
>
> Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
> Applications Processor Reference Manual, Rev. 3, 08/2024), the
> AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
> different than the bus configuration. Why not model it as part of the
> bus?
>
> > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > >> index e0d3b8cba221..a1d9b834d2da 100644
> > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> > >>  			};
> > >>  		};
> > >>
> > >> -		aips5: bus@30c00000 {
> > >> -			compatible = "fsl,aips-bus", "simple-bus";
> > >> -			reg = <0x30c00000 0x400000>;
> > >> +		aips5: bus@30df0000 {
>                        ^^^^^^^^^^^^
> > >> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> > >> +			reg = <0x30df0000 0x10000>;
> > >> +			power-domains = <&pgc_audio>;
> > >>  			#address-cells = <1>;
> > >>  			#size-cells = <1>;
> > >> +			#access-controller-cells = <0>;
> > >>  			ranges;
> > >>
> > >>  			spba-bus@30c00000 {
>                         ^^^^^^^^^^^^^^^^^
>
> This looks very strange: The aips5 bus starts at 0x30df0000 and has a
> child bus starting at 0x30c00000?

@30df0000 should match controller reg's address.

subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.

So it should be reasonable. another example:
i2c@1000 {

	device@1c <- which use difference address space.
}

The similar case also happen at pcie.

Frank

>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3239 bytes --]

On 27.02.2025 11:45:36, Frank Li wrote:
> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
> > On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> > >
> > > On 21.02.2025 21:56, Frank Li wrote:
> > > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> > > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> > > >>
> > > >> AIPS5 is actually AIPSTZ5 as it offers some security-related
> > > >> configurations. Since these configurations need to be applied before
> > > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> > > >> be their parent instead of keeping AIPS5 and adding a child node for
> > > >> AIPSTZ5. Also, because of the security configurations, the address space
> > > >> of the bus has to be changed to that of the configuration registers.
> > > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> > > > config address part in your drivers.
> > > >
> > > > Frank
> > >
> > >
> > > Any concerns/anything wrong with current approach?
> > >
> > >
> > > I find it a bit awkward to have the whole bus address space
> > > in the DT given that we're only interested in using the access
> > > controller register space.
> > >
> > >
> > > I'm fine with the approach you suggested but I don't see a
> > > reason for using it?
> >
> > Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
> > Applications Processor Reference Manual, Rev. 3, 08/2024), the
> > AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
> > different than the bus configuration. Why not model it as part of the
> > bus?
> >
> > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> index e0d3b8cba221..a1d9b834d2da 100644
> > > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> > > >>  			};
> > > >>  		};
> > > >>
> > > >> -		aips5: bus@30c00000 {
> > > >> -			compatible = "fsl,aips-bus", "simple-bus";
> > > >> -			reg = <0x30c00000 0x400000>;
> > > >> +		aips5: bus@30df0000 {
> >                        ^^^^^^^^^^^^
> > > >> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> > > >> +			reg = <0x30df0000 0x10000>;
> > > >> +			power-domains = <&pgc_audio>;
> > > >>  			#address-cells = <1>;
> > > >>  			#size-cells = <1>;
> > > >> +			#access-controller-cells = <0>;
> > > >>  			ranges;
> > > >>
> > > >>  			spba-bus@30c00000 {
> >                         ^^^^^^^^^^^^^^^^^
> >
> > This looks very strange: The aips5 bus starts at 0x30df0000 and has a
> > child bus starting at 0x30c00000?
> 
> @30df0000 should match controller reg's address.
> 
> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.

Ok. What about aips1...4? Should the be changed as well in this patch?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Marco Felsch]

Hi,

On 25-02-27, Frank Li wrote:
> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
> > On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> > >
> > > On 21.02.2025 21:56, Frank Li wrote:
> > > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> > > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> > > >>
> > > >> AIPS5 is actually AIPSTZ5 as it offers some security-related
> > > >> configurations. Since these configurations need to be applied before
> > > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> > > >> be their parent instead of keeping AIPS5 and adding a child node for
> > > >> AIPSTZ5. Also, because of the security configurations, the address space
> > > >> of the bus has to be changed to that of the configuration registers.
> > > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> > > > config address part in your drivers.
> > > >
> > > > Frank
> > >
> > > Any concerns/anything wrong with current approach?
> > >
> > >
> > > I find it a bit awkward to have the whole bus address space
> > > in the DT given that we're only interested in using the access
> > > controller register space.
> > >
> > >
> > > I'm fine with the approach you suggested but I don't see a
> > > reason for using it?
> >
> > Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
> > Applications Processor Reference Manual, Rev. 3, 08/2024), the
> > AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
> > different than the bus configuration. Why not model it as part of the
> > bus?
> >
> > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> index e0d3b8cba221..a1d9b834d2da 100644
> > > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> > > >>  			};
> > > >>  		};
> > > >>
> > > >> -		aips5: bus@30c00000 {
> > > >> -			compatible = "fsl,aips-bus", "simple-bus";
> > > >> -			reg = <0x30c00000 0x400000>;
> > > >> +		aips5: bus@30df0000 {
> >                        ^^^^^^^^^^^^
> > > >> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> > > >> +			reg = <0x30df0000 0x10000>;
> > > >> +			power-domains = <&pgc_audio>;
> > > >>  			#address-cells = <1>;
> > > >>  			#size-cells = <1>;
> > > >> +			#access-controller-cells = <0>;
> > > >>  			ranges;
> > > >>
> > > >>  			spba-bus@30c00000 {
> >                         ^^^^^^^^^^^^^^^^^
> >
> > This looks very strange: The aips5 bus starts at 0x30df0000 and has a
> > child bus starting at 0x30c00000?
> 
> @30df0000 should match controller reg's address.
> 
> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.
> 
> So it should be reasonable. another example:
> i2c@1000 {
> 
> 	device@1c <- which use difference address space.
> }
> 
> The similar case also happen at pcie.

I'm not really convinced that pcie and i2c are good examples here. I2C
does have an other addressing scheme by nature and the hotplug-able pcie
is dependeds on the pcie device memory map of course.

Here we're talking about an access control IP core on a bus which is
static.

But.. it looks like from DT abstraction it's fine because STM did
something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc
albeit it does look strange and I don't know why we have to limit the
address space since it was already mapped but used by the fsl,aips-bus
driver.

Regards,
  Marco


> 
> Frank
> 
> >
> > regards,
> > Marc
> >
> > --
> > Pengutronix e.K.                 | Marc Kleine-Budde          |
> > Embedded Linux                   | https://www.pengutronix.de |
> > Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> > Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
> 
> 
> 
> 

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Laurentiu Mihalcea]

On 2/28/2025 12:19 PM, Marco Felsch wrote:
> Hi,
>
> On 25-02-27, Frank Li wrote:
>> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
>>> On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
>>>> On 21.02.2025 21:56, Frank Li wrote:
>>>>> On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
>>>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>>>>>
>>>>>> AIPS5 is actually AIPSTZ5 as it offers some security-related
>>>>>> configurations. Since these configurations need to be applied before
>>>>>> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
>>>>>> be their parent instead of keeping AIPS5 and adding a child node for
>>>>>> AIPSTZ5. Also, because of the security configurations, the address space
>>>>>> of the bus has to be changed to that of the configuration registers.
>>>>> The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
>>>>> config address part in your drivers.
>>>>>
>>>>> Frank
>>>> Any concerns/anything wrong with current approach?
>>>>
>>>>
>>>> I find it a bit awkward to have the whole bus address space
>>>> in the DT given that we're only interested in using the access
>>>> controller register space.
>>>>
>>>>
>>>> I'm fine with the approach you suggested but I don't see a
>>>> reason for using it?
>>> Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
>>> Applications Processor Reference Manual, Rev. 3, 08/2024), the
>>> AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
>>> different than the bus configuration. Why not model it as part of the
>>> bus?
>>>
>>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>> index e0d3b8cba221..a1d9b834d2da 100644
>>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
>>>>>>  			};
>>>>>>  		};
>>>>>>
>>>>>> -		aips5: bus@30c00000 {
>>>>>> -			compatible = "fsl,aips-bus", "simple-bus";
>>>>>> -			reg = <0x30c00000 0x400000>;
>>>>>> +		aips5: bus@30df0000 {
>>>                        ^^^^^^^^^^^^
>>>>>> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
>>>>>> +			reg = <0x30df0000 0x10000>;
>>>>>> +			power-domains = <&pgc_audio>;
>>>>>>  			#address-cells = <1>;
>>>>>>  			#size-cells = <1>;
>>>>>> +			#access-controller-cells = <0>;
>>>>>>  			ranges;
>>>>>>
>>>>>>  			spba-bus@30c00000 {
>>>                         ^^^^^^^^^^^^^^^^^
>>>
>>> This looks very strange: The aips5 bus starts at 0x30df0000 and has a
>>> child bus starting at 0x30c00000?
>> @30df0000 should match controller reg's address.
>>
>> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.
>>
>> So it should be reasonable. another example:
>> i2c@1000 {
>>
>> 	device@1c <- which use difference address space.
>> }
>>
>> The similar case also happen at pcie.
> I'm not really convinced that pcie and i2c are good examples here. I2C
> does have an other addressing scheme by nature and the hotplug-able pcie
> is dependeds on the pcie device memory map of course.
>
> Here we're talking about an access control IP core on a bus which is
> static.
>
> But.. it looks like from DT abstraction it's fine because STM did
> something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc
> albeit it does look strange and I don't know why we have to limit the
> address space since it was already mapped but used by the fsl,aips-bus
> driver.
>
> Regards,
>   Marco

The address space of the bridge was changed to that of the bridge's configuration
space because I think it's very awkward from the software's point of view to have
to hardcode the offset and size of the configuration space inside the driver. I also
looked at what STM did with "st,stm32-etzpc" so I thought this would be acceptable
from the DT's POV.

Regarding why I chose not to model the access controller part as a subnode of the
bus:

    1) The access controller is part of the bridge itself (not a separate module accessible
    via the bridge like it's the case for its children) so I think the current approach
    should also make sense if we take the hardware into consideration.

    2) The access controller configuration also impacts the AP. As such, we needed a way to
    enforce a dependency between the children of the aips5 bus and the access controller
    (we could have used the 'access-controllers' property like we did with the DSP but having
    to do that for all children of the bus is not ideal I'd say.

Of course, argument no. 2 is somewhat brittle in the context of i.MX8MP as the reset
values of the AC's registers already allow the AP to access said peripherals. Despite this,
I think the current approach would be more scalable given that the IP offers some more
configuration bits which we might want to toggle. For that to work, we need to make sure
the bits are toggled before the AP accesses the peripherals on the bridge.


Note that we don't do this for aips1-aips4 because it's really not needed. If I'm not mistaken,
they're not really attached to a PD so we don't need to re-configure them each time the domain
is power cycled (which is why we can just do it once from ATF during the boot process)
  

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Marco Felsch]

On 25-03-04, Laurentiu Mihalcea wrote:
> 
> On 2/28/2025 12:19 PM, Marco Felsch wrote:
> > Hi,
> >
> > On 25-02-27, Frank Li wrote:
> >> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
> >>> On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> >>>> On 21.02.2025 21:56, Frank Li wrote:
> >>>>> On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> >>>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>>>>>
> >>>>>> AIPS5 is actually AIPSTZ5 as it offers some security-related
> >>>>>> configurations. Since these configurations need to be applied before
> >>>>>> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> >>>>>> be their parent instead of keeping AIPS5 and adding a child node for
> >>>>>> AIPSTZ5. Also, because of the security configurations, the address space
> >>>>>> of the bus has to be changed to that of the configuration registers.
> >>>>> The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> >>>>> config address part in your drivers.
> >>>>>
> >>>>> Frank
> >>>> Any concerns/anything wrong with current approach?
> >>>>
> >>>>
> >>>> I find it a bit awkward to have the whole bus address space
> >>>> in the DT given that we're only interested in using the access
> >>>> controller register space.
> >>>>
> >>>>
> >>>> I'm fine with the approach you suggested but I don't see a
> >>>> reason for using it?
> >>> Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
> >>> Applications Processor Reference Manual, Rev. 3, 08/2024), the
> >>> AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
> >>> different than the bus configuration. Why not model it as part of the
> >>> bus?
> >>>
> >>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>> index e0d3b8cba221..a1d9b834d2da 100644
> >>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> >>>>>>  			};
> >>>>>>  		};
> >>>>>>
> >>>>>> -		aips5: bus@30c00000 {
> >>>>>> -			compatible = "fsl,aips-bus", "simple-bus";
> >>>>>> -			reg = <0x30c00000 0x400000>;
> >>>>>> +		aips5: bus@30df0000 {
> >>>                        ^^^^^^^^^^^^
> >>>>>> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> >>>>>> +			reg = <0x30df0000 0x10000>;
> >>>>>> +			power-domains = <&pgc_audio>;
> >>>>>>  			#address-cells = <1>;
> >>>>>>  			#size-cells = <1>;
> >>>>>> +			#access-controller-cells = <0>;
> >>>>>>  			ranges;
> >>>>>>
> >>>>>>  			spba-bus@30c00000 {
> >>>                         ^^^^^^^^^^^^^^^^^
> >>>
> >>> This looks very strange: The aips5 bus starts at 0x30df0000 and has a
> >>> child bus starting at 0x30c00000?
> >> @30df0000 should match controller reg's address.
> >>
> >> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.
> >>
> >> So it should be reasonable. another example:
> >> i2c@1000 {
> >>
> >> 	device@1c <- which use difference address space.
> >> }
> >>
> >> The similar case also happen at pcie.
> > I'm not really convinced that pcie and i2c are good examples here. I2C
> > does have an other addressing scheme by nature and the hotplug-able pcie
> > is dependeds on the pcie device memory map of course.
> >
> > Here we're talking about an access control IP core on a bus which is
> > static.
> >
> > But.. it looks like from DT abstraction it's fine because STM did
> > something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc
> > albeit it does look strange and I don't know why we have to limit the
> > address space since it was already mapped but used by the fsl,aips-bus
> > driver.
> >
> > Regards,
> >   Marco
> 
> The address space of the bridge was changed to that of the bridge's
> configuration space because I think it's very awkward from the
> software's point of view to have to hardcode the offset and size of
> the configuration space inside the driver. 

You mean the access-controller IP core. I could also arguee that it's
akward to put the bridge access-controller IP core into the middle of
the bridge address-space instead of placing it at the very beginning of
the bridge. But this doesn't help here :)

I see what you mean but from DT abstraction POV it seems more reasonable
to keep it as it is and just adapt the compatible. The current driver
maps the whole address space too, so I don't see why we need to change
it if we change it to the aipstz driver. If you see the
access-controller IP core as part of the bus I don't see any problem and
would argue that the offset detail needs to be handled within the
driver.

> I also looked at what STM did with "st,stm32-etzpc" so I thought this
> would be acceptable from the DT's POV.
>
> Regarding why I chose not to model the access controller part as a subnode of the
> bus:
> 
>     1) The access controller is part of the bridge itself (not a separate module accessible
>     via the bridge like it's the case for its children) so I think the current approach
>     should also make sense if we take the hardware into consideration.

I don't like this approach if you see the controller as part of the
bridge because the offset could be handled within the bridge driver.
I also that the register offset needs to be supplied else we can't reuse
the driver and we don't want to adapt the driver for each SoC.

What came into my mind is the following:

	spba-bus@30c00000 {
		compatible = "nxp,imx8mp-aiptz-bus", "nxp,aiptz-bus";
		reg = <0x30c00000 0x400000>, <0x30df0000 0x10000>;
		reg-names = "bus", "aipstz";

		child-nodes {};
		child-nodes {};
		child-nodes {};
	}

This way we can abstract the access-controller register space and the
whole bus register space and a generic driver could be written just by
making use two reg fields.

>     2) The access controller configuration also impacts the AP. As such, we needed a way to
>     enforce a dependency between the children of the aips5 bus and the access controller
>     (we could have used the 'access-controllers' property like we did with the DSP but having
>     to do that for all children of the bus is not ideal I'd say.
> 
> Of course, argument no. 2 is somewhat brittle in the context of i.MX8MP as the reset
> values of the AC's registers already allow the AP to access said peripherals. Despite this,
> I think the current approach would be more scalable given that the IP offers some more
> configuration bits which we might want to toggle. For that to work, we need to make sure
> the bits are toggled before the AP accesses the peripherals on the bridge.

Please have a look on my suggestion above.

Regards,
  Marco

> Note that we don't do this for aips1-aips4 because it's really not needed. If I'm not mistaken,
> they're not really attached to a PD so we don't need to re-configure them each time the domain
> is power cycled (which is why we can just do it once from ATF during the boot process)

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Laurentiu Mihalcea]

On 3/7/2025 5:22 PM, Marco Felsch wrote:
> On 25-03-04, Laurentiu Mihalcea wrote:
>> On 2/28/2025 12:19 PM, Marco Felsch wrote:
>>> Hi,
>>>
>>> On 25-02-27, Frank Li wrote:
>>>> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
>>>>> On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
>>>>>> On 21.02.2025 21:56, Frank Li wrote:
>>>>>>> On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
>>>>>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>>>>>>>
>>>>>>>> AIPS5 is actually AIPSTZ5 as it offers some security-related
>>>>>>>> configurations. Since these configurations need to be applied before
>>>>>>>> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
>>>>>>>> be their parent instead of keeping AIPS5 and adding a child node for
>>>>>>>> AIPSTZ5. Also, because of the security configurations, the address space
>>>>>>>> of the bus has to be changed to that of the configuration registers.
>>>>>>> The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
>>>>>>> config address part in your drivers.
>>>>>>>
>>>>>>> Frank
>>>>>> Any concerns/anything wrong with current approach?
>>>>>>
>>>>>>
>>>>>> I find it a bit awkward to have the whole bus address space
>>>>>> in the DT given that we're only interested in using the access
>>>>>> controller register space.
>>>>>>
>>>>>>
>>>>>> I'm fine with the approach you suggested but I don't see a
>>>>>> reason for using it?
>>>>> Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
>>>>> Applications Processor Reference Manual, Rev. 3, 08/2024), the
>>>>> AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
>>>>> different than the bus configuration. Why not model it as part of the
>>>>> bus?
>>>>>
>>>>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>>>> index e0d3b8cba221..a1d9b834d2da 100644
>>>>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
>>>>>>>> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
>>>>>>>>  			};
>>>>>>>>  		};
>>>>>>>>
>>>>>>>> -		aips5: bus@30c00000 {
>>>>>>>> -			compatible = "fsl,aips-bus", "simple-bus";
>>>>>>>> -			reg = <0x30c00000 0x400000>;
>>>>>>>> +		aips5: bus@30df0000 {
>>>>>                        ^^^^^^^^^^^^
>>>>>>>> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
>>>>>>>> +			reg = <0x30df0000 0x10000>;
>>>>>>>> +			power-domains = <&pgc_audio>;
>>>>>>>>  			#address-cells = <1>;
>>>>>>>>  			#size-cells = <1>;
>>>>>>>> +			#access-controller-cells = <0>;
>>>>>>>>  			ranges;
>>>>>>>>
>>>>>>>>  			spba-bus@30c00000 {
>>>>>                         ^^^^^^^^^^^^^^^^^
>>>>>
>>>>> This looks very strange: The aips5 bus starts at 0x30df0000 and has a
>>>>> child bus starting at 0x30c00000?
>>>> @30df0000 should match controller reg's address.
>>>>
>>>> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.
>>>>
>>>> So it should be reasonable. another example:
>>>> i2c@1000 {
>>>>
>>>> 	device@1c <- which use difference address space.
>>>> }
>>>>
>>>> The similar case also happen at pcie.
>>> I'm not really convinced that pcie and i2c are good examples here. I2C
>>> does have an other addressing scheme by nature and the hotplug-able pcie
>>> is dependeds on the pcie device memory map of course.
>>>
>>> Here we're talking about an access control IP core on a bus which is
>>> static.
>>>
>>> But.. it looks like from DT abstraction it's fine because STM did
>>> something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc
>>> albeit it does look strange and I don't know why we have to limit the
>>> address space since it was already mapped but used by the fsl,aips-bus
>>> driver.
>>>
>>> Regards,
>>>   Marco
>> The address space of the bridge was changed to that of the bridge's
>> configuration space because I think it's very awkward from the
>> software's point of view to have to hardcode the offset and size of
>> the configuration space inside the driver. 
> You mean the access-controller IP core. I could also arguee that it's
> akward to put the bridge access-controller IP core into the middle of
> the bridge address-space instead of placing it at the very beginning of
> the bridge. But this doesn't help here :)
>
> I see what you mean but from DT abstraction POV it seems more reasonable
> to keep it as it is and just adapt the compatible. The current driver
> maps the whole address space too, so I don't see why we need to change
> it if we change it to the aipstz driver. If you see the
> access-controller IP core as part of the bus I don't see any problem and
> would argue that the offset detail needs to be handled within the
> driver.
>
>> I also looked at what STM did with "st,stm32-etzpc" so I thought this
>> would be acceptable from the DT's POV.
>>
>> Regarding why I chose not to model the access controller part as a subnode of the
>> bus:
>>
>>     1) The access controller is part of the bridge itself (not a separate module accessible
>>     via the bridge like it's the case for its children) so I think the current approach
>>     should also make sense if we take the hardware into consideration.
> I don't like this approach if you see the controller as part of the
> bridge because the offset could be handled within the bridge driver.
> I also that the register offset needs to be supplied else we can't reuse
> the driver and we don't want to adapt the driver for each SoC.
>
> What came into my mind is the following:
>
> 	spba-bus@30c00000 {
> 		compatible = "nxp,imx8mp-aiptz-bus", "nxp,aiptz-bus";
> 		reg = <0x30c00000 0x400000>, <0x30df0000 0x10000>;
> 		reg-names = "bus", "aipstz";
>
> 		child-nodes {};
> 		child-nodes {};
> 		child-nodes {};
> 	}
>
> This way we can abstract the access-controller register space and the
> whole bus register space and a generic driver could be written just by
> making use two reg fields.


by changing the compatible, we've also effectively changed the programming model.
I don't really see why we need to stick to the old way of configuring the bus node (i.e:
specify the whole address space of the bus as well) when all we really care about is the AC
configuration region?
 
anyhow, I'm not going to insist on this. I think the proposed approach will work just
fine. If there's no other comments on this then I'll just switch to it in V3.


>
>>     2) The access controller configuration also impacts the AP. As such, we needed a way to
>>     enforce a dependency between the children of the aips5 bus and the access controller
>>     (we could have used the 'access-controllers' property like we did with the DSP but having
>>     to do that for all children of the bus is not ideal I'd say.
>>
>> Of course, argument no. 2 is somewhat brittle in the context of i.MX8MP as the reset
>> values of the AC's registers already allow the AP to access said peripherals. Despite this,
>> I think the current approach would be more scalable given that the IP offers some more
>> configuration bits which we might want to toggle. For that to work, we need to make sure
>> the bits are toggled before the AP accesses the peripherals on the bridge.
> Please have a look on my suggestion above.
>
> Regards,
>   Marco
>
>> Note that we don't do this for aips1-aips4 because it's really not needed. If I'm not mistaken,
>> they're not really attached to a PD so we don't need to re-configure them each time the domain
>> is power cycled (which is why we can just do it once from ATF during the boot process)

Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'

[Marco Felsch]

On 25-03-10, Laurentiu Mihalcea wrote:
> On 3/7/2025 5:22 PM, Marco Felsch wrote:
> > On 25-03-04, Laurentiu Mihalcea wrote:
> >> On 2/28/2025 12:19 PM, Marco Felsch wrote:
> >>> Hi,
> >>>
> >>> On 25-02-27, Frank Li wrote:
> >>>> On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:
> >>>>> On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:
> >>>>>> On 21.02.2025 21:56, Frank Li wrote:
> >>>>>>> On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:
> >>>>>>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> >>>>>>>>
> >>>>>>>> AIPS5 is actually AIPSTZ5 as it offers some security-related
> >>>>>>>> configurations. Since these configurations need to be applied before
> >>>>>>>> accessing any of the peripherals on the bus, it's better to make AIPSTZ5
> >>>>>>>> be their parent instead of keeping AIPS5 and adding a child node for
> >>>>>>>> AIPSTZ5. Also, because of the security configurations, the address space
> >>>>>>>> of the bus has to be changed to that of the configuration registers.
> >>>>>>> The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only
> >>>>>>> config address part in your drivers.
> >>>>>>>
> >>>>>>> Frank
> >>>>>> Any concerns/anything wrong with current approach?
> >>>>>>
> >>>>>>
> >>>>>> I find it a bit awkward to have the whole bus address space
> >>>>>> in the DT given that we're only interested in using the access
> >>>>>> controller register space.
> >>>>>>
> >>>>>>
> >>>>>> I'm fine with the approach you suggested but I don't see a
> >>>>>> reason for using it?
> >>>>> Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus
> >>>>> Applications Processor Reference Manual, Rev. 3, 08/2024), the
> >>>>> AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something
> >>>>> different than the bus configuration. Why not model it as part of the
> >>>>> bus?
> >>>>>
> >>>>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>>>> index e0d3b8cba221..a1d9b834d2da 100644
> >>>>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >>>>>>>> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 {
> >>>>>>>>  			};
> >>>>>>>>  		};
> >>>>>>>>
> >>>>>>>> -		aips5: bus@30c00000 {
> >>>>>>>> -			compatible = "fsl,aips-bus", "simple-bus";
> >>>>>>>> -			reg = <0x30c00000 0x400000>;
> >>>>>>>> +		aips5: bus@30df0000 {
> >>>>>                        ^^^^^^^^^^^^
> >>>>>>>> +			compatible = "fsl,imx8mp-aipstz", "simple-bus";
> >>>>>>>> +			reg = <0x30df0000 0x10000>;
> >>>>>>>> +			power-domains = <&pgc_audio>;
> >>>>>>>>  			#address-cells = <1>;
> >>>>>>>>  			#size-cells = <1>;
> >>>>>>>> +			#access-controller-cells = <0>;
> >>>>>>>>  			ranges;
> >>>>>>>>
> >>>>>>>>  			spba-bus@30c00000 {
> >>>>>                         ^^^^^^^^^^^^^^^^^
> >>>>>
> >>>>> This looks very strange: The aips5 bus starts at 0x30df0000 and has a
> >>>>> child bus starting at 0x30c00000?
> >>>> @30df0000 should match controller reg's address.
> >>>>
> >>>> subnode address 0x30c00000,  should be descript in "ranges", which 1:1 map.
> >>>>
> >>>> So it should be reasonable. another example:
> >>>> i2c@1000 {
> >>>>
> >>>> 	device@1c <- which use difference address space.
> >>>> }
> >>>>
> >>>> The similar case also happen at pcie.
> >>> I'm not really convinced that pcie and i2c are good examples here. I2C
> >>> does have an other addressing scheme by nature and the hotplug-able pcie
> >>> is dependeds on the pcie device memory map of course.
> >>>
> >>> Here we're talking about an access control IP core on a bus which is
> >>> static.
> >>>
> >>> But.. it looks like from DT abstraction it's fine because STM did
> >>> something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc
> >>> albeit it does look strange and I don't know why we have to limit the
> >>> address space since it was already mapped but used by the fsl,aips-bus
> >>> driver.
> >>>
> >>> Regards,
> >>>   Marco
> >> The address space of the bridge was changed to that of the bridge's
> >> configuration space because I think it's very awkward from the
> >> software's point of view to have to hardcode the offset and size of
> >> the configuration space inside the driver. 
> > You mean the access-controller IP core. I could also arguee that it's
> > akward to put the bridge access-controller IP core into the middle of
> > the bridge address-space instead of placing it at the very beginning of
> > the bridge. But this doesn't help here :)
> >
> > I see what you mean but from DT abstraction POV it seems more reasonable
> > to keep it as it is and just adapt the compatible. The current driver
> > maps the whole address space too, so I don't see why we need to change
> > it if we change it to the aipstz driver. If you see the
> > access-controller IP core as part of the bus I don't see any problem and
> > would argue that the offset detail needs to be handled within the
> > driver.
> >
> >> I also looked at what STM did with "st,stm32-etzpc" so I thought this
> >> would be acceptable from the DT's POV.
> >>
> >> Regarding why I chose not to model the access controller part as a subnode of the
> >> bus:
> >>
> >>     1) The access controller is part of the bridge itself (not a separate module accessible
> >>     via the bridge like it's the case for its children) so I think the current approach
> >>     should also make sense if we take the hardware into consideration.
> > I don't like this approach if you see the controller as part of the
> > bridge because the offset could be handled within the bridge driver.
> > I also that the register offset needs to be supplied else we can't reuse
> > the driver and we don't want to adapt the driver for each SoC.
> >
> > What came into my mind is the following:
> >
> > 	spba-bus@30c00000 {
> > 		compatible = "nxp,imx8mp-aiptz-bus", "nxp,aiptz-bus";
> > 		reg = <0x30c00000 0x400000>, <0x30df0000 0x10000>;
> > 		reg-names = "bus", "aipstz";
> >
> > 		child-nodes {};
> > 		child-nodes {};
> > 		child-nodes {};
> > 	}
> >
> > This way we can abstract the access-controller register space and the
> > whole bus register space and a generic driver could be written just by
> > making use two reg fields.
> 
> by changing the compatible, we've also effectively changed the
> programming model.

You need to keep the backward compatible which means, a new kernel have
to boot an old dts (firmware), which is stil the case since the
"simple-bus" driver sill matches on the old dts files.

But this doesn't mean that a new dts (firmware) have to be compatible
with an old kernel.

> I don't really see why we need to stick to the old way of configuring
> the bus node (i.e: specify the whole address space of the bus as well)
> when all we really care about is the AC configuration region?

I see and as I said it's just my oppinion that we should abstract it
this way since upstream already accepted the approach you implemented
for the STM32 case.

If NXP would have placed the AC at the beginning of each AIPS bus you
wouldn't need to deal with this problem too since you would have the
base register already.

> anyhow, I'm not going to insist on this. I think the proposed approach
> will work just fine. If there's no other comments on this then I'll
> just switch to it in V3.

That would be nice but please wait for maintainers feedback e.g Shawn.

Regards,
  Marco

[PATCH 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'

[Laurentiu Mihalcea]

From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The DSP may want to access peripherals on AIPSTZ5. To do so, the
security-related registers of the bridge have to be configured before
the DSP is started. Enforce a dependency on AIPSTZ5 by adding the
'access-controllers' property to the 'dsp' node.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index a1d9b834d2da..9ec51e7e6678 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -2423,6 +2423,7 @@ dsp: dsp@3b6e8000 {
 			mboxes = <&mu2 2 0>, <&mu2 2 1>,
 				<&mu2 3 0>, <&mu2 3 1>;
 			memory-region = <&dsp_reserved>;
+			access-controllers = <&aips5>;
 			status = "disabled";
 		};
 	};
-- 
2.34.1

Re: [PATCH 5/5] arm64: dts: imx8mp: make 'dsp' node depend on 'aips5'

[Frank Li]

On Fri, Feb 21, 2025 at 02:19:09PM -0500, Laurentiu Mihalcea wrote:
> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>
> The DSP may want to access peripherals on AIPSTZ5. To do so, the

It'd better descript as

The DSP need access ... at ... case.

Frank

> security-related registers of the bridge have to be configured before
> the DSP is started. Enforce a dependency on AIPSTZ5 by adding the
> 'access-controllers' property to the 'dsp' node.
>
> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index a1d9b834d2da..9ec51e7e6678 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -2423,6 +2423,7 @@ dsp: dsp@3b6e8000 {
>  			mboxes = <&mu2 2 0>, <&mu2 2 1>,
>  				<&mu2 3 0>, <&mu2 3 1>;
>  			memory-region = <&dsp_reserved>;
> +			access-controllers = <&aips5>;
>  			status = "disabled";
>  		};
>  	};
> --
> 2.34.1
>