This is a RFC patch in order to allow AHB to IP Bridge (AIPSTZ)
configuration to be restored after a suspend/resume operation.

This is particularly useful for aips5 bus where the aipstz
configuration is lost at suspend.

In order to configure aipstz bridge we register a platform driver
that will set default configuration permission at probe.

Because AIPS configuration is lost at suspend/resume time we register
a power notifier callback that will take care of re-initializing the
configuration at resume.

More details about AHB to IP Bridge:

This peripheral is designated as the bridge between AHB bus and
peripherals with the lower bandwidth IP Slave (IPS) buses.

The bridge is responsible  for indicating to IPS peripherals if
an access is in supervisor or user mode. It may block user mode
accesses to certain IPS peripherals or it may allow the individual
IPS peripherals to determine if user mode accesses are allowed.

The bridge supports the notion of "trusted" masters for security
purposes. Masters may be individually designated as trusted for
reads, writes or both.

Even more details (including register description) can be found
in Chapter 4.7 AHB to IP Bridge (AIPSTZ)
of i.MX8M Plus Application Processor Reference Manual.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---

This RFC patch is to get your opinion on the direction on how should
we proceed with handling configuration registers for this kind of
device.

It registers itself as a platform driver at boot time and then is probed
once the dts node is discovered.

TBD:
- device tree binding file
- enhance error checking
- is this the correct way to handle this kind of "peripheral".

 arch/arm64/boot/dts/freescale/imx8mp.dtsi |   7 ++
 drivers/soc/imx/aipstz.c                  | 114 ++++++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 drivers/soc/imx/aipstz.c

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index e0d3b8cba221..7775f5f58036 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1406,6 +1406,13 @@ aips5: bus@30c00000 {
 			#size-cells = <1>;
 			ranges;
 
+			aipstz5: bridge@30df0000 {
+				compatible = "fsl,imx8mp-aipstz";
+				reg = <0x30df0000 0x10000>;
+				power-domains = <&pgc_audio>;
+				status = "okay";
+			};
+
 			spba-bus@30c00000 {
 				compatible = "fsl,spba-bus", "simple-bus";
 				reg = <0x30c00000 0x100000>;
diff --git a/drivers/soc/imx/aipstz.c b/drivers/soc/imx/aipstz.c
new file mode 100644
index 000000000000..49a619f17dda
--- /dev/null
+++ b/drivers/soc/imx/aipstz.c
@@ -0,0 +1,114 @@
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/pm_domain.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define DRV_NAME "aips-bridge"
+
+#define AIPSTZ_MPR0		0x0
+#define AIPSTZ_MPR1		0x4
+
+#define AIPSTZ_OPACR_NUM	(0x5)
+#define OPACR_OFFSET(i)		((i) * 4 + 0x40)
+
+struct aipstz_drv {
+	void __iomem *base;
+	struct notifier_block power_nb;
+	struct aipstz_cfg *cfg;
+};
+
+struct aipstz_cfg {
+	uint32_t mpr0;
+	uint32_t mpr1;
+	uint32_t opacr[AIPSTZ_OPACR_NUM];
+};
+
+static struct aipstz_cfg aipstz5 = {
+	0x77777777,
+	0x77777777,
+	.opacr = {0x0, 0x0, 0x0, 0x0, 0x0}
+};
+
+static void imx_aipstz_config_init(const struct aipstz_drv *drv)
+
+{
+	const struct aipstz_cfg *aipstz = drv->cfg;
+
+	writel(aipstz->mpr0, drv->base + AIPSTZ_MPR0);
+	writel(aipstz->mpr1, drv->base + AIPSTZ_MPR1);
+
+	for (int i = 0; i < AIPSTZ_OPACR_NUM; i++)
+		writel(aipstz->opacr[i], drv->base + OPACR_OFFSET(i));
+}
+
+static int aipstz_power_notifier(struct notifier_block *nb,
+				 unsigned long action, void *data)
+{
+	struct aipstz_drv *drv = container_of(nb, struct aipstz_drv, power_nb);
+
+	if (action != GENPD_NOTIFY_ON)
+		return NOTIFY_OK;
+
+	imx_aipstz_config_init(drv);
+
+	return NOTIFY_OK;
+}
+
+static int aipstz_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aipstz_drv *drv;
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->base = of_iomap(pdev->dev.of_node, 0);
+	drv->power_nb.notifier_call = aipstz_power_notifier;
+	drv->cfg = &aipstz5;
+
+	imx_aipstz_config_init(drv);
+
+	if (dev->pm_domain)
+		dev_pm_genpd_add_notifier(dev, &drv->power_nb);
+
+	dev_set_drvdata(dev, drv);
+
+	return 0;
+}
+
+static const struct of_device_id aipstz_of_match[] = {
+	{.compatible = "fsl,imx8mp-aipstz", },
+	{}
+};
+
+static struct platform_driver aipstz_driver = {
+	.probe =  aipstz_probe,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = of_match_ptr(aipstz_of_match),
+	},
+};
+
+static int __init aipstz_driver_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&aipstz_driver);
+	if (ret) {
+		pr_err("Failed to register aipstz platform driver\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+device_initcall(aipstz_driver_init);
+
+MODULE_DESCRIPTION("i.MX8 AIPS bus configuration driver");
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.43.0

On Tue, Nov 19, 2024 at 03:07:26PM +0200, Daniel Baluta wrote:
> This is a RFC patch in order to allow AHB to IP Bridge (AIPSTZ)
> configuration to be restored after a suspend/resume operation.
>
> This is particularly useful for aips5 bus where the aipstz
> configuration is lost at suspend.
>
> In order to configure aipstz bridge we register a platform driver
> that will set default configuration permission at probe.
>
> Because AIPS configuration is lost at suspend/resume time we register
> a power notifier callback that will take care of re-initializing the
> configuration at resume.

why not use suspend/resume callback? what's happen if devices under AIPS
suspend/resume before AIPS resume function.


>
> More details about AHB to IP Bridge:
>
> This peripheral is designated as the bridge between AHB bus and
> peripherals with the lower bandwidth IP Slave (IPS) buses.
>
> The bridge is responsible  for indicating to IPS peripherals if
> an access is in supervisor or user mode. It may block user mode
> accesses to certain IPS peripherals or it may allow the individual
> IPS peripherals to determine if user mode accesses are allowed.
>
> The bridge supports the notion of "trusted" masters for security
> purposes. Masters may be individually designated as trusted for
> reads, writes or both.
>
> Even more details (including register description) can be found
> in Chapter 4.7 AHB to IP Bridge (AIPSTZ)
> of i.MX8M Plus Application Processor Reference Manual.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>
> This RFC patch is to get your opinion on the direction on how should
> we proceed with handling configuration registers for this kind of
> device.
>
> It registers itself as a platform driver at boot time and then is probed
> once the dts node is discovered.
>
> TBD:
> - device tree binding file
> - enhance error checking
> - is this the correct way to handle this kind of "peripheral".
>
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi |   7 ++
>  drivers/soc/imx/aipstz.c                  | 114 ++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 drivers/soc/imx/aipstz.c
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index e0d3b8cba221..7775f5f58036 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1406,6 +1406,13 @@ aips5: bus@30c00000 {
>  			#size-cells = <1>;
>  			ranges;
>
> +			aipstz5: bridge@30df0000 {
> +				compatible = "fsl,imx8mp-aipstz";
> +				reg = <0x30df0000 0x10000>;
> +				power-domains = <&pgc_audio>;
> +				status = "okay";
> +			};

supposed
	bridge@30df0000 {
		...
		some child node {
		}

		so child node's devies suspend/resume will after bidged's
suspend resume.

	}

> +
>  			spba-bus@30c00000 {
>  				compatible = "fsl,spba-bus", "simple-bus";
>  				reg = <0x30c00000 0x100000>;
> diff --git a/drivers/soc/imx/aipstz.c b/drivers/soc/imx/aipstz.c
> new file mode 100644
> index 000000000000..49a619f17dda
> --- /dev/null
> +++ b/drivers/soc/imx/aipstz.c
> @@ -0,0 +1,114 @@
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/pm_domain.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#define DRV_NAME "aips-bridge"
> +
> +#define AIPSTZ_MPR0		0x0
> +#define AIPSTZ_MPR1		0x4
> +
> +#define AIPSTZ_OPACR_NUM	(0x5)
> +#define OPACR_OFFSET(i)		((i) * 4 + 0x40)
> +
> +struct aipstz_drv {
> +	void __iomem *base;
> +	struct notifier_block power_nb;
> +	struct aipstz_cfg *cfg;
> +};
> +
> +struct aipstz_cfg {
> +	uint32_t mpr0;
> +	uint32_t mpr1;
> +	uint32_t opacr[AIPSTZ_OPACR_NUM];
> +};
> +
> +static struct aipstz_cfg aipstz5 = {
> +	0x77777777,
> +	0x77777777,
> +	.opacr = {0x0, 0x0, 0x0, 0x0, 0x0}
> +};
> +
> +static void imx_aipstz_config_init(const struct aipstz_drv *drv)
> +
> +{
> +	const struct aipstz_cfg *aipstz = drv->cfg;
> +
> +	writel(aipstz->mpr0, drv->base + AIPSTZ_MPR0);
> +	writel(aipstz->mpr1, drv->base + AIPSTZ_MPR1);
> +
> +	for (int i = 0; i < AIPSTZ_OPACR_NUM; i++)
> +		writel(aipstz->opacr[i], drv->base + OPACR_OFFSET(i));
> +}
> +
> +static int aipstz_power_notifier(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct aipstz_drv *drv = container_of(nb, struct aipstz_drv, power_nb);
> +
> +	if (action != GENPD_NOTIFY_ON)
> +		return NOTIFY_OK;
> +
> +	imx_aipstz_config_init(drv);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int aipstz_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct aipstz_drv *drv;
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->base = of_iomap(pdev->dev.of_node, 0);
> +	drv->power_nb.notifier_call = aipstz_power_notifier;
> +	drv->cfg = &aipstz5;
> +
> +	imx_aipstz_config_init(drv);
> +
> +	if (dev->pm_domain)
> +		dev_pm_genpd_add_notifier(dev, &drv->power_nb);
> +
> +	dev_set_drvdata(dev, drv);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aipstz_of_match[] = {
> +	{.compatible = "fsl,imx8mp-aipstz", },
> +	{}
> +};
> +
> +static struct platform_driver aipstz_driver = {
> +	.probe =  aipstz_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(aipstz_of_match),
> +	},
> +};
> +
> +static int __init aipstz_driver_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&aipstz_driver);
> +	if (ret) {
> +		pr_err("Failed to register aipstz platform driver\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +device_initcall(aipstz_driver_init);
> +
> +MODULE_DESCRIPTION("i.MX8 AIPS bus configuration driver");
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> --
> 2.43.0
>

On Tue, Nov 19, 2024 at 5:00 PM Frank Li <Frank.li@nxp.com> wrote:
>
> On Tue, Nov 19, 2024 at 03:07:26PM +0200, Daniel Baluta wrote:
> > This is a RFC patch in order to allow AHB to IP Bridge (AIPSTZ)
> > configuration to be restored after a suspend/resume operation.
> >
> > This is particularly useful for aips5 bus where the aipstz
> > configuration is lost at suspend.
> >
> > In order to configure aipstz bridge we register a platform driver
> > that will set default configuration permission at probe.
> >
> > Because AIPS configuration is lost at suspend/resume time we register
> > a power notifier callback that will take care of re-initializing the
> > configuration at resume.
>
> why not use suspend/resume callback? what's happen if devices under AIPS
> suspend/resume before AIPS resume function.

We are attaching to AIPS power domain. So any device that will resume
will have first to power up the domain which in turn will configure the bridge.

So with this it doesn't really matter the suspend/resume order
becasuse the notifier
that will configure the bridge is already register.

But I wonder if there could be a problem if a device goes
suspend/resume before the bridge
probe function gets a chance to be called.

> supposed
>         bridge@30df0000 {
>                 ...
>                 some child node {
>                 }
>
>                 so child node's devies suspend/resume will after bidged's
> suspend resume.
>
>         }