[RFC PATCH 2/2] binman: j721e: Add firewall configurations for atf

[Andrew Davis]

On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
> The following commits adds the configuration of firewalls required to
> protect ATF and OP-TEE memory region from non-secure reads and
> writes using master and slave firewalls present in our K3 SOCs.
> 
> Signed-off-by: Manorit Chawdhry <m-chawdhry at ti.com>
> ---
>   arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 161 insertions(+)
> 
> diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi
> index 4f566c21a9af..0569a592597e 100644
> --- a/arch/arm/dts/k3-j721e-binman.dtsi
> +++ b/arch/arm/dts/k3-j721e-binman.dtsi
> @@ -330,6 +330,73 @@
>   					ti-secure {
>   						content = <&atf>;
>   						keyfile = "custMpk.pem";
> +						auth_in_place = <0xa02>;
> +
> +						// cpu_0_cpu_0_msmc Background Firewall - 0

I don't have a personal preference, but I see most comments in DTS
are C style /* */.

Also it might be easier to understand if you put the comments right before
the properties that they relate to, i.g.

firewall-0 {
	/* cpu_0_cpu_0_msmc */
	id = <257>;
	region = <0>;
	/* Background, Locked */
	control = <0x31a>;
	permissions = <0xc3ffff>;
	start_address = <0x0 0x0>;
	end_address = <0xff 0xffffffff>;
};

For permissions, I wonder if it would be easier to read if we add
some helper macros:

#define FWPRIVID_ALL 0xc3

#define FWPERM_SECURE_PRIV_WRITE      BIT(0)
#define FWPERM_SECURE_PRIV_READ       BIT(1)
#define FWPERM_SECURE_PRIV_CACHEABLE  BIT(2)
#define FWPERM_SECURE_PRIV_DEBUG      BIT(3)
#define FWPERM_SECURE_USER_WRITE      BIT(4)
...

Then you would have:

permissions = <FWPRIVID_ALL |
                FWPERM_SECURE_PRIV_WRITE |
                FWPERM_SECURE_PRIV_READ |
                ...
>;

> +						firewall-0 {
> +							id = <257>;
> +							region = <0>;
> +							control = <0x31a>;
> +							permissions = <0xc3ffff>;
> +							start_address = <0x0 0x0>;
> +							end_address = <0xff 0xffffffff>;
> +						};
> +
> +						// cpu_0_cpu_0_msmc Foreground Firewall
> +						firewall-1 {
> +							id = <257>;
> +							region = <1>;
> +							control = <0x1a>;
> +							permissions = <0x0100ff>;
> +							start_address = <0x0 0x70000000>;

This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR?
Not sure how you would get the end address as we don't really know ATF size..

Andrew