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

[Laurentiu Mihalcea <laurentiumihalcea111@gmail.com>]

On 4/3/2025 11:30 AM, Alexander Stein wrote:
> Hi,
>
> Am Dienstag, 1. April 2025, 17:44:01 CEST schrieb 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 (e.g: AIPSTZ5 on i.MX8MP) 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..44db40dae71b
>> --- /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)
>> +{
>> +	const struct imx_aipstz_config *default_cfg;
>> +	void __iomem *base;
>> +
>> +	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 AC memory\n");
>> +
>> +	default_cfg = of_device_get_match_data(&pdev->dev);
> Shouldn't you use the configuration setup by trusted firmware (TF-A)?


not sure I see the value in doing that? the TF-A configuration will be overriden
anyways if an AC API is ever introduced in Linux. Also, for AIPSTZ5, you'd need to:

1) Make sure the AUDIOMIX domain is not power cycled before latching on to the
TF-A configuration otherwise you'll lose it.

2) Add an extra step in which you actually configure the bridge's AC from TF-A since
it's not ATM.

I'm not sure why you'd want to do that when you can just set the configuration directly
from Linux?


>
>> +
>> +	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)
>> +{
>> +	const struct imx_aipstz_config *default_cfg;
>> +	void __iomem *base;
>> +
>> +	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);
> Shouldn't you store the configuration at suspend and restore that one
> instead of this fixed one?

you're only using the fixed configuration here and you're not modifying it anywhere
so no need to save it during suspend.

>
> What's going to happen if trusted firmware decides that Cortex-A53 domain
> is not allowed to access AIPSTZ?

then you'll get a bus fault and will have to model AIPSTZ as just AIPS via the devicetree
(like we do for AIPSTZ1-AIPSTZ4 right now)