[PATCH 1/2] meson_sm: Extend meson_sm driver to be compatible with gxl chip

[PATCH 1/2] meson_sm: Extend meson_sm driver to be compatible with gxl chip

[Krzysztof Michonski]

From: Krzysztof Michonski <michonskikrzysztof@gmail.com>

Extend the meson_sm driver by adding gxl structure and SECURITY_KEY
commands.

Signed-off-by: Krzysztof Michonski <michonskikrzysztof@gmail.com>
---
 drivers/firmware/meson/meson_sm.c       | 14 ++++++++++++++
 include/linux/firmware/meson/meson_sm.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 29fbc818a573..b19489f759fc 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -54,6 +54,19 @@ struct meson_sm_chip gxbb_chip = {
 	},
 };
 
+struct meson_sm_chip gxl_chip = {
+	.shmem_size		= SZ_4K,
+	.cmd_shmem_in_base	= 0x82000023,
+	.cmd_shmem_out_base	= 0x82000024,
+	.cmd = {
+		CMD(SM_SECURITY_KEY_READ,	0x82000061),
+		CMD(SM_SECURITY_KEY_WRITE,	0x82000062),
+		CMD(SM_SECURITY_KEY_TELL,	0x82000063),
+		CMD(SM_GET_CHIP_ID,	0x82000044),
+		{ /* sentinel */ },
+	},
+};
+
 struct meson_sm_firmware {
 	const struct meson_sm_chip *chip;
 	void __iomem *sm_shmem_in_base;
@@ -269,6 +282,7 @@ static const struct attribute_group meson_sm_sysfs_attr_group = {
 
 static const struct of_device_id meson_sm_ids[] = {
 	{ .compatible = "amlogic,meson-gxbb-sm", .data = &gxbb_chip },
+	{ .compatible = "amlogic,meson-gxl-sm", .data = &gxl_chip },
 	{ /* sentinel */ },
 };
 
diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
index f98c20dd266e..9352d35c6e96 100644
--- a/include/linux/firmware/meson/meson_sm.h
+++ b/include/linux/firmware/meson/meson_sm.h
@@ -18,6 +18,9 @@ enum {
 	SM_EFUSE_WRITE,
 	SM_EFUSE_USER_MAX,
 	SM_GET_CHIP_ID,
+	SM_SECURITY_KEY_READ,
+	SM_SECURITY_KEY_WRITE,
+	SM_SECURITY_KEY_TELL,
 };
 
 struct meson_sm_firmware;
-- 
2.21.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 2/2] Enable secure monitor on gxl

[Krzysztof Michonski]

From: Krzysztof Michonski <michonskikrzysztof@gmail.com>

Add secure monitor node in the gxl dtsi file.

Signed-off-by: Krzysztof Michonski <michonskikrzysztof@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 3093ae421b17..fd75538ee9f9 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -37,6 +37,10 @@
 			};
 		};
 	};
+
+	sm: secure-monitor {
+		compatible = "amlogic,meson-gxl-sm";
+	};
 };
 
 &apb {
-- 
2.21.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/2] Enable secure monitor on gxl

[Martin Blumenstingl]

Hi Krzysztof,

the subject should start with: "arm64: dts: meson-gxl:", see the git
history (note that there was a rule change to use "arm64" instead of
"ARM64")

On Thu, Jun 6, 2019 at 3:24 PM Krzysztof Michonski
<michonskikrzysztof@gmail.com> wrote:
>
> From: Krzysztof Michonski <michonskikrzysztof@gmail.com>
>
> Add secure monitor node in the gxl dtsi file.
I assume that this is to enable the GXL and GXM specific commands?

> Signed-off-by: Krzysztof Michonski <michonskikrzysztof@gmail.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> index 3093ae421b17..fd75538ee9f9 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> @@ -37,6 +37,10 @@
>                         };
>                 };
>         };
> +
> +       sm: secure-monitor {
> +               compatible = "amlogic,meson-gxl-sm";
> +       };
>  };
if you keep the secure-monitor node in meson-gx.dtsi then you can use
something like:
&sm {
    compatible = "amlogic,meson-gxl-sm";
};

the compatible string has to be added to the documentation in
Documentation/devicetree/bindings/firmware/meson/meson_sm.txt with a
separate patch
devicetree bindings should be backwards compatible (at least that's
the rule of thumb)
you can use a fallback compatible string so this new .dts/.dtb also
works with old kernels (which don't support the new, GXL specific,
compatible string yet):
  compatible = "amlogic,meson-gxl-sm", "amlogic,meson-gxbb-sm";


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/2] Enable secure monitor on gxl

[Martin Blumenstingl]

(adding back the linux-amlogic@lists.infradead.org mailing list)

On Fri, Jun 7, 2019 at 9:13 AM Krzysztof Michoński
<michonskikrzysztof@gmail.com> wrote:
>
> Hi Martin,
> > the subject should start with: "arm64: dts: meson-gxl:", see the git
> > history (note that there was a rule change to use "arm64" instead of
> > "ARM64")
> Thanks, so I will resend the patches with correct topic soon.
thank you!

> > you can use a fallback compatible string so this new .dts/.dtb also
> > works with old kernels (which don't support the new, GXL specific,
> > compatible string yet):
> The secure monitor then will not be compatible with gxl since
> the share_in/out storage addresses in the gxl struct differ from gxbb.
I missed that, thank you for clarifying this. I'll reply with a
specific question to your other patch

> Should I still add the fallback string then?
assuming GXBB and GXL aren't compatible then I would do it like this:
- move the existing secure monitor node from meson-gx.dtsi to
meson-gxbb.dtsi (because based on your description it doesn't work on
GXL/GXM anyways)
- like your patch already does: introduce a new secure monitor node in
meson-gxl.dtsi


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/2] meson_sm: Extend meson_sm driver to be compatible with gxl chip

[Martin Blumenstingl]

Hi Krzysztof,

On Thu, Jun 6, 2019 at 3:24 PM Krzysztof Michonski
<michonskikrzysztof@gmail.com> wrote:
[...]
> +struct meson_sm_chip gxl_chip = {
> +       .shmem_size             = SZ_4K,
> +       .cmd_shmem_in_base      = 0x82000023,
> +       .cmd_shmem_out_base     = 0x82000024,
where did you get these values from and/or what issues did you see
with the values from GXBB?

I checked Amlogic's buildroot kernel whether they are doing something similar:
$ grep in_base_func
buildroot-openlinux-A113-201901/kernel/aml-4.9/arch/arm64/boot/dts/amlogic/mesong*.dtsi
| cut -d':' -f2 | sort -u
                in_base_func = <0x82000020>;
that includes GXL, GXM, G12A and G12B. however, I admit that I didn't
have time to test your patch yet (so it may be just fine and the
vendor kernel is buggy)


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/2] meson_sm: Extend meson_sm driver to be compatible with gxl chip

[Neil Armstrong]

On 07/06/2019 22:27, Martin Blumenstingl wrote:
> Hi Krzysztof,
> 
> On Thu, Jun 6, 2019 at 3:24 PM Krzysztof Michonski
> <michonskikrzysztof@gmail.com> wrote:
> [...]
>> +struct meson_sm_chip gxl_chip = {
>> +       .shmem_size             = SZ_4K,
>> +       .cmd_shmem_in_base      = 0x82000023,
>> +       .cmd_shmem_out_base     = 0x82000024,
> where did you get these values from and/or what issues did you see
> with the values from GXBB?
> 
> I checked Amlogic's buildroot kernel whether they are doing something similar:
> $ grep in_base_func
> buildroot-openlinux-A113-201901/kernel/aml-4.9/arch/arm64/boot/dts/amlogic/mesong*.dtsi
> | cut -d':' -f2 | sort -u
>                 in_base_func = <0x82000020>;
> that includes GXL, GXM, G12A and G12B. however, I admit that I didn't
> have time to test your patch yet (so it may be just fine and the
> vendor kernel is buggy)

Same question from me, we've been using the same shmem address on GXL for years now
without any issues, can you elaborate ?

Neil

> 
> 
> Martin
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/2] meson_sm: Extend meson_sm driver to be compatible with gxl chip

[Krzysztof Michoński]

Hi,
a word of explanation from me. The reason why I thought the addresses
differ is I've had wrong understanding of the shared storage and
shared memory. Due to a problem we've faced we were made to read from
the secure storage and as you can see the secure monitor driver after
altering the shmem_in/out addresses can be used map shared storage.
This patches then are incorrect.

Krzysztof Michoński

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic