[PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling via B4 Relay]

From: Aaron Kling <webgeek1234@gmail.com>

This allows using pstore on all such platforms. There are some
differences per arch:

* Tegra132: Flounder does not appear to enumerate pstore and I do not
  have access to norrin, thus Tegra132 is left out of this commit.
* Tegra210: Does not support ramoops carveouts in the bootloader, instead
  relying on a dowstream driver to allocate the carveout, hence this
  hardcodes a location matching what the downstream driver picks.
* Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
  size in a node specifically named /reserved-memory/ramoops_carveout,
  thus these cannot be renamed.
* Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
  compatible, however the dt still does not know the address, so keeping
  the node name consistent on Tegra186 and newer.

Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
 4 files changed, 61 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -2051,6 +2051,22 @@ pmu-denver {
 		interrupt-affinity = <&denver_0 &denver_1>;
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ramoops_carveout {
+			compatible = "ramoops";
+			size = <0x0 0x200000>;
+			record-size = <0x00010000>;
+			console-size = <0x00080000>;
+			alignment = <0x0 0x10000>;
+			alloc-ranges = <0x0 0x0 0x1 0x0>;
+			no-map;
+		};
+	};
+
 	sound {
 		status = "disabled";
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 33f92b77cd9d9e530eae87a4bb8ba61993ceffeb..90ffea161a57a8986c2493573c73e3cf9e2c43c0 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -3105,6 +3105,22 @@ psci {
 		method = "smc";
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ramoops_carveout {
+			compatible = "ramoops";
+			size = <0x0 0x200000>;
+			record-size = <0x00010000>;
+			console-size = <0x00080000>;
+			alignment = <0x0 0x10000>;
+			alloc-ranges = <0x0 0x0 0x1 0x0>;
+			no-map;
+		};
+	};
+
 	tcu: serial {
 		compatible = "nvidia,tegra194-tcu";
 		mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM TEGRA_HSP_SM_RX(0)>,
diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index b6c84d195c0ef9ae90721fada09ffd46a9c11fa3..00ae127e8b8af3fe3b95d8ce5986d937a4fc6325 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -2025,6 +2025,19 @@ pmu {
 				      &{/cpus/cpu@2} &{/cpus/cpu@3}>;
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ramoops@b0000000 {
+			compatible = "ramoops";
+			reg = <0x0 0xb0000000 0x0 0x200000>;
+			record-size = <0x00010000>;
+			console-size = <0x00080000>;
+		};
+	};
+
 	sound {
 		status = "disabled";
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 2601b43b2d8cadeb0d1f428018a82b144aa79392..36f35c6dc774d42aca8871dbfa0e0a16414cb860 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -5723,6 +5723,22 @@ psci {
 		method = "smc";
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ramoops_carveout {
+			compatible = "ramoops";
+			size = <0x0 0x200000>;
+			record-size = <0x00010000>;
+			console-size = <0x00080000>;
+			alignment = <0x0 0x10000>;
+			alloc-ranges = <0x0 0x0 0x1 0x0>;
+			no-map;
+		};
+	};
+
 	tcu: serial {
 		compatible = "nvidia,tegra234-tcu", "nvidia,tegra194-tcu";
 		mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM TEGRA_HSP_SM_RX(0)>,

---
base-commit: 91e5bfe317d8f8471fbaa3e70cf66cae1314a516
change-id: 20250404-tegra-pstore-5bed3f721390

Best regards,
-- 
Aaron Kling <webgeek1234@gmail.com>

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Rob Herring (Arm)]

On Sun, 06 Apr 2025 16:12:43 -0500, Aaron Kling wrote:
> This allows using pstore on all such platforms. There are some
> differences per arch:
> 
> * Tegra132: Flounder does not appear to enumerate pstore and I do not
>   have access to norrin, thus Tegra132 is left out of this commit.
> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
>   relying on a dowstream driver to allocate the carveout, hence this
>   hardcodes a location matching what the downstream driver picks.
> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
>   size in a node specifically named /reserved-memory/ramoops_carveout,
>   thus these cannot be renamed.
> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
>   compatible, however the dt still does not know the address, so keeping
>   the node name consistent on Tegra186 and newer.
> 
> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
>  4 files changed, 61 insertions(+)
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


This patch series was applied (using b4) to base:
 Base: using specified base-commit 91e5bfe317d8f8471fbaa3e70cf66cae1314a516

If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)

New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/nvidia/' for 20250406-tegra-pstore-v1-1-bf5b57f12293@gmail.com:

arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0000.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0000.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3740-0002+p3701-0008.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0008.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra186-p3509-0000+p3636-0001.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra234-p3768-0000+p3767-0005.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#
arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dtb: ramoops_carveout (ramoops): 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/reserved-memory/ramoops.yaml#

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Krzysztof Kozlowski]

On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.com>
> 
> This allows using pstore on all such platforms. There are some
> differences per arch:
> 
> * Tegra132: Flounder does not appear to enumerate pstore and I do not
>   have access to norrin, thus Tegra132 is left out of this commit.
> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
>   relying on a dowstream driver to allocate the carveout, hence this
>   hardcodes a location matching what the downstream driver picks.
> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
>   size in a node specifically named /reserved-memory/ramoops_carveout,
>   thus these cannot be renamed.
> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
>   compatible, however the dt still does not know the address, so keeping
>   the node name consistent on Tegra186 and newer.
> 
> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> @@ -2051,6 +2051,22 @@ pmu-denver {
>  		interrupt-affinity = <&denver_0 &denver_1>;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		ramoops_carveout {

Please follow DTS coding style for name, so this is probably only ramoops.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > From: Aaron Kling <webgeek1234@gmail.com>
> >
> > This allows using pstore on all such platforms. There are some
> > differences per arch:
> >
> > * Tegra132: Flounder does not appear to enumerate pstore and I do not
> >   have access to norrin, thus Tegra132 is left out of this commit.
> > * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> >   relying on a dowstream driver to allocate the carveout, hence this
> >   hardcodes a location matching what the downstream driver picks.
> > * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> >   size in a node specifically named /reserved-memory/ramoops_carveout,
> >   thus these cannot be renamed.
> > * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> >   compatible, however the dt still does not know the address, so keeping
> >   the node name consistent on Tegra186 and newer.
> >
> > Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> >  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> >  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> >  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> >  4 files changed, 61 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > @@ -2051,6 +2051,22 @@ pmu-denver {
> >               interrupt-affinity = <&denver_0 &denver_1>;
> >       };
> >
> > +     reserved-memory {
> > +             #address-cells = <2>;
> > +             #size-cells = <2>;
> > +             ranges;
> > +
> > +             ramoops_carveout {
>
> Please follow DTS coding style for name, so this is probably only ramoops.

As per the commit message regarding tegra186: bootloader fills in the
address and size in a node specifically named
/reserved-memory/ramoops_carveout, thus these cannot be renamed.

>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.

The bot is reporting that the reg field is missing from the added
ramoops nodes on t186, t194, and t234. However, as also mentioned in
the commit message, this is intentional because it is expected for the
bootloader to fill that in. It is not known at dt compile time. Is
there a way to mark this as intentional, so dtschema doesn't flag it?

>
> Best regards,
> Krzysztof

Sincerely,
Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Krzysztof Kozlowski]

On 07/04/2025 18:00, Aaron Kling wrote:
> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
>>> From: Aaron Kling <webgeek1234@gmail.com>
>>>
>>> This allows using pstore on all such platforms. There are some
>>> differences per arch:
>>>
>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
>>>   have access to norrin, thus Tegra132 is left out of this commit.
>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
>>>   relying on a dowstream driver to allocate the carveout, hence this
>>>   hardcodes a location matching what the downstream driver picks.
>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
>>>   thus these cannot be renamed.
>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
>>>   compatible, however the dt still does not know the address, so keeping
>>>   the node name consistent on Tegra186 and newer.
>>>
>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
>>> ---
>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
>>>  4 files changed, 61 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>> @@ -2051,6 +2051,22 @@ pmu-denver {
>>>               interrupt-affinity = <&denver_0 &denver_1>;
>>>       };
>>>
>>> +     reserved-memory {
>>> +             #address-cells = <2>;
>>> +             #size-cells = <2>;
>>> +             ranges;
>>> +
>>> +             ramoops_carveout {
>>
>> Please follow DTS coding style for name, so this is probably only ramoops.
> 
> As per the commit message regarding tegra186: bootloader fills in the
> address and size in a node specifically named
> /reserved-memory/ramoops_carveout, thus these cannot be renamed.

That's not a reason to introduce issues. Bootloader is supposed to
follow same conventions or use aliases or labels (depending on the node).

If bootloader adds junk, does it mean we have to accept that junk?

> 
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
>> Maybe you need to update your dtschema and yamllint. Don't rely on
>> distro packages for dtschema and be sure you are using the latest
>> released dtschema.
> 
> The bot is reporting that the reg field is missing from the added
> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> the commit message, this is intentional because it is expected for the
> bootloader to fill that in. It is not known at dt compile time. Is
> there a way to mark this as intentional, so dtschema doesn't flag it?

Fix your bootloader or chain load some normal one, like U-Boot.

Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 07/04/2025 18:00, Aaron Kling wrote:
> > On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> >>> From: Aaron Kling <webgeek1234@gmail.com>
> >>>
> >>> This allows using pstore on all such platforms. There are some
> >>> differences per arch:
> >>>
> >>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> >>>   have access to norrin, thus Tegra132 is left out of this commit.
> >>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> >>>   relying on a dowstream driver to allocate the carveout, hence this
> >>>   hardcodes a location matching what the downstream driver picks.
> >>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> >>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> >>>   thus these cannot be renamed.
> >>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> >>>   compatible, however the dt still does not know the address, so keeping
> >>>   the node name consistent on Tegra186 and newer.
> >>>
> >>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> >>> ---
> >>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> >>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> >>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> >>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> >>>  4 files changed, 61 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> >>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>> @@ -2051,6 +2051,22 @@ pmu-denver {
> >>>               interrupt-affinity = <&denver_0 &denver_1>;
> >>>       };
> >>>
> >>> +     reserved-memory {
> >>> +             #address-cells = <2>;
> >>> +             #size-cells = <2>;
> >>> +             ranges;
> >>> +
> >>> +             ramoops_carveout {
> >>
> >> Please follow DTS coding style for name, so this is probably only ramoops.
> >
> > As per the commit message regarding tegra186: bootloader fills in the
> > address and size in a node specifically named
> > /reserved-memory/ramoops_carveout, thus these cannot be renamed.
>
> That's not a reason to introduce issues. Bootloader is supposed to
> follow same conventions or use aliases or labels (depending on the node).
>
> If bootloader adds junk, does it mean we have to accept that junk?
>
> >
> >>
> >> It does not look like you tested the DTS against bindings. Please run
> >> `make dtbs_check W=1` (see
> >> Documentation/devicetree/bindings/writing-schema.rst or
> >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> >> for instructions).
> >> Maybe you need to update your dtschema and yamllint. Don't rely on
> >> distro packages for dtschema and be sure you are using the latest
> >> released dtschema.
> >
> > The bot is reporting that the reg field is missing from the added
> > ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > the commit message, this is intentional because it is expected for the
> > bootloader to fill that in. It is not known at dt compile time. Is
> > there a way to mark this as intentional, so dtschema doesn't flag it?
>
> Fix your bootloader or chain load some normal one, like U-Boot.
How would chainloading a second bootloader 'fix' previous stage
bootloaders trampling on an out-of-sync hardcoded reserved-memory
address? It's possible for carveout addresses and sizes to change. Not
from boot to boot on the same version of the Nvidia bootloader, but
potentially from one version to another. Depending on if the
bootloader was configured with different carveout sizes.

There is precedence for this. When blind cleanup was done on arm
device trees, a chromebook broke because the memory node has to be
named exactly '/memory' [0]. How is this any different from that case?
These nodes are an ABI to an existing bootloader. Carveouts on these
archs are set up in bl1 or bl2, which are not source available. I
could potentially hardcode things for myself in bl33, which is source
available, but the earlier stages could still overwrite any chosen
block depending on how carveouts are configured. But even then, that
will not change the behaviour of the vast majority of units that use a
fully prebuilt boot stack direct from Nvidia. My intent here is for
pstore to work on such units without users needing to use a custom
bootloader.
>
> Best regards,
> Krzysztof

Sincerely,
Aaron

[0] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20190211110919.10388-1-thierry.reding@gmail.com/#22474263

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Krzysztof Kozlowski]

On 08/04/2025 09:35, Aaron Kling wrote:
> On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 07/04/2025 18:00, Aaron Kling wrote:
>>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
>>>>> From: Aaron Kling <webgeek1234@gmail.com>
>>>>>
>>>>> This allows using pstore on all such platforms. There are some
>>>>> differences per arch:
>>>>>
>>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
>>>>>   have access to norrin, thus Tegra132 is left out of this commit.
>>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
>>>>>   relying on a dowstream driver to allocate the carveout, hence this
>>>>>   hardcodes a location matching what the downstream driver picks.
>>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
>>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
>>>>>   thus these cannot be renamed.
>>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
>>>>>   compatible, however the dt still does not know the address, so keeping
>>>>>   the node name consistent on Tegra186 and newer.
>>>>>
>>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
>>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
>>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
>>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
>>>>>  4 files changed, 61 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
>>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
>>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
>>>>>               interrupt-affinity = <&denver_0 &denver_1>;
>>>>>       };
>>>>>
>>>>> +     reserved-memory {
>>>>> +             #address-cells = <2>;
>>>>> +             #size-cells = <2>;
>>>>> +             ranges;
>>>>> +
>>>>> +             ramoops_carveout {
>>>>
>>>> Please follow DTS coding style for name, so this is probably only ramoops.
>>>
>>> As per the commit message regarding tegra186: bootloader fills in the
>>> address and size in a node specifically named
>>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
>>
>> That's not a reason to introduce issues. Bootloader is supposed to
>> follow same conventions or use aliases or labels (depending on the node).
>>
>> If bootloader adds junk, does it mean we have to accept that junk?
>>
>>>
>>>>
>>>> It does not look like you tested the DTS against bindings. Please run
>>>> `make dtbs_check W=1` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst or
>>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>>>> for instructions).
>>>> Maybe you need to update your dtschema and yamllint. Don't rely on
>>>> distro packages for dtschema and be sure you are using the latest
>>>> released dtschema.
>>>
>>> The bot is reporting that the reg field is missing from the added
>>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
>>> the commit message, this is intentional because it is expected for the
>>> bootloader to fill that in. It is not known at dt compile time. Is
>>> there a way to mark this as intentional, so dtschema doesn't flag it?
>>
>> Fix your bootloader or chain load some normal one, like U-Boot.
> How would chainloading a second bootloader 'fix' previous stage
> bootloaders trampling on an out-of-sync hardcoded reserved-memory
> address? It's possible for carveout addresses and sizes to change. Not
> from boot to boot on the same version of the Nvidia bootloader, but
> potentially from one version to another. Depending on if the
> bootloader was configured with different carveout sizes.
> 
> There is precedence for this. When blind cleanup was done on arm
> device trees, a chromebook broke because the memory node has to be
> named exactly '/memory' [0]. How is this any different from that case?

That was an existing node, so ABI.

> These nodes are an ABI to an existing bootloader. Carveouts on these

You add new ABI, which I object to.

> archs are set up in bl1 or bl2, which are not source available. I
> could potentially hardcode things for myself in bl33, which is source
> available, but the earlier stages could still overwrite any chosen
> block depending on how carveouts are configured. But even then, that
> will not change the behaviour of the vast majority of units that use a
> fully prebuilt boot stack direct from Nvidia. My intent here is for
> pstore to work on such units without users needing to use a custom
> bootloader.
I understand your goal. What I still do not understand, why bootloader
even bothers with ramoops carveout. It shouldn't and you should just
ignore whatever bootloader provides, no?

It's not the same case as memory, where bootloader needs to fill out the
actual size, or some other boot-specific properties.

Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 08/04/2025 09:35, Aaron Kling wrote:
> > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 07/04/2025 18:00, Aaron Kling wrote:
> >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>
> >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> >>>>> From: Aaron Kling <webgeek1234@gmail.com>
> >>>>>
> >>>>> This allows using pstore on all such platforms. There are some
> >>>>> differences per arch:
> >>>>>
> >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> >>>>>   hardcodes a location matching what the downstream driver picks.
> >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> >>>>>   thus these cannot be renamed.
> >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> >>>>>   compatible, however the dt still does not know the address, so keeping
> >>>>>   the node name consistent on Tegra186 and newer.
> >>>>>
> >>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> >>>>>  4 files changed, 61 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> >>>>>       };
> >>>>>
> >>>>> +     reserved-memory {
> >>>>> +             #address-cells = <2>;
> >>>>> +             #size-cells = <2>;
> >>>>> +             ranges;
> >>>>> +
> >>>>> +             ramoops_carveout {
> >>>>
> >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> >>>
> >>> As per the commit message regarding tegra186: bootloader fills in the
> >>> address and size in a node specifically named
> >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> >>
> >> That's not a reason to introduce issues. Bootloader is supposed to
> >> follow same conventions or use aliases or labels (depending on the node).
> >>
> >> If bootloader adds junk, does it mean we have to accept that junk?
> >>
> >>>
> >>>>
> >>>> It does not look like you tested the DTS against bindings. Please run
> >>>> `make dtbs_check W=1` (see
> >>>> Documentation/devicetree/bindings/writing-schema.rst or
> >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> >>>> for instructions).
> >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> >>>> distro packages for dtschema and be sure you are using the latest
> >>>> released dtschema.
> >>>
> >>> The bot is reporting that the reg field is missing from the added
> >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> >>> the commit message, this is intentional because it is expected for the
> >>> bootloader to fill that in. It is not known at dt compile time. Is
> >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> >>
> >> Fix your bootloader or chain load some normal one, like U-Boot.
> > How would chainloading a second bootloader 'fix' previous stage
> > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > address? It's possible for carveout addresses and sizes to change. Not
> > from boot to boot on the same version of the Nvidia bootloader, but
> > potentially from one version to another. Depending on if the
> > bootloader was configured with different carveout sizes.
> >
> > There is precedence for this. When blind cleanup was done on arm
> > device trees, a chromebook broke because the memory node has to be
> > named exactly '/memory' [0]. How is this any different from that case?
>
> That was an existing node, so ABI.
>
> > These nodes are an ABI to an existing bootloader. Carveouts on these
>
> You add new ABI, which I object to.
>
> > archs are set up in bl1 or bl2, which are not source available. I
> > could potentially hardcode things for myself in bl33, which is source
> > available, but the earlier stages could still overwrite any chosen
> > block depending on how carveouts are configured. But even then, that
> > will not change the behaviour of the vast majority of units that use a
> > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > pstore to work on such units without users needing to use a custom
> > bootloader.
> I understand your goal. What I still do not understand, why bootloader
> even bothers with ramoops carveout. It shouldn't and you should just
> ignore whatever bootloader provides, no?

Mmm, I actually don't have the answer to this. Ramoops carveout
handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
late in the life cycle. But it has always been in edk2 for t194 and
t234 afaik. I could hazard some guesses, but don't have any
documentation on why the decision was made. Maybe Thierry or Jonathan
could chime in on why this was done.

>
> It's not the same case as memory, where bootloader needs to fill out the
> actual size, or some other boot-specific properties.
>
> Best regards,
> Krzysztof
Sincerely,
Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1234@gmail.com> wrote:
>
> On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 08/04/2025 09:35, Aaron Kling wrote:
> > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >>
> > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >>>>
> > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > >>>>> From: Aaron Kling <webgeek1234@gmail.com>
> > >>>>>
> > >>>>> This allows using pstore on all such platforms. There are some
> > >>>>> differences per arch:
> > >>>>>
> > >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> > >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> > >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> > >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> > >>>>>   hardcodes a location matching what the downstream driver picks.
> > >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> > >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> > >>>>>   thus these cannot be renamed.
> > >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> > >>>>>   compatible, however the dt still does not know the address, so keeping
> > >>>>>   the node name consistent on Tegra186 and newer.
> > >>>>>
> > >>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > >>>>> ---
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> > >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> > >>>>>  4 files changed, 61 insertions(+)
> > >>>>>
> > >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> > >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> > >>>>>       };
> > >>>>>
> > >>>>> +     reserved-memory {
> > >>>>> +             #address-cells = <2>;
> > >>>>> +             #size-cells = <2>;
> > >>>>> +             ranges;
> > >>>>> +
> > >>>>> +             ramoops_carveout {
> > >>>>
> > >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> > >>>
> > >>> As per the commit message regarding tegra186: bootloader fills in the
> > >>> address and size in a node specifically named
> > >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> > >>
> > >> That's not a reason to introduce issues. Bootloader is supposed to
> > >> follow same conventions or use aliases or labels (depending on the node).
> > >>
> > >> If bootloader adds junk, does it mean we have to accept that junk?
> > >>
> > >>>
> > >>>>
> > >>>> It does not look like you tested the DTS against bindings. Please run
> > >>>> `make dtbs_check W=1` (see
> > >>>> Documentation/devicetree/bindings/writing-schema.rst or
> > >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > >>>> for instructions).
> > >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> > >>>> distro packages for dtschema and be sure you are using the latest
> > >>>> released dtschema.
> > >>>
> > >>> The bot is reporting that the reg field is missing from the added
> > >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > >>> the commit message, this is intentional because it is expected for the
> > >>> bootloader to fill that in. It is not known at dt compile time. Is
> > >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> > >>
> > >> Fix your bootloader or chain load some normal one, like U-Boot.
> > > How would chainloading a second bootloader 'fix' previous stage
> > > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > > address? It's possible for carveout addresses and sizes to change. Not
> > > from boot to boot on the same version of the Nvidia bootloader, but
> > > potentially from one version to another. Depending on if the
> > > bootloader was configured with different carveout sizes.
> > >
> > > There is precedence for this. When blind cleanup was done on arm
> > > device trees, a chromebook broke because the memory node has to be
> > > named exactly '/memory' [0]. How is this any different from that case?
> >
> > That was an existing node, so ABI.
> >
> > > These nodes are an ABI to an existing bootloader. Carveouts on these
> >
> > You add new ABI, which I object to.
> >
> > > archs are set up in bl1 or bl2, which are not source available. I
> > > could potentially hardcode things for myself in bl33, which is source
> > > available, but the earlier stages could still overwrite any chosen
> > > block depending on how carveouts are configured. But even then, that
> > > will not change the behaviour of the vast majority of units that use a
> > > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > > pstore to work on such units without users needing to use a custom
> > > bootloader.
> > I understand your goal. What I still do not understand, why bootloader
> > even bothers with ramoops carveout. It shouldn't and you should just
> > ignore whatever bootloader provides, no?
>
> Mmm, I actually don't have the answer to this. Ramoops carveout
> handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
> late in the life cycle. But it has always been in edk2 for t194 and
> t234 afaik. I could hazard some guesses, but don't have any
> documentation on why the decision was made. Maybe Thierry or Jonathan
> could chime in on why this was done.
>

Friendly reminder to the Tegra maintainers about this question.

Sincerely,
Aaron Kling

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Sun, Apr 20, 2025 at 8:45 PM Aaron Kling <webgeek1234@gmail.com> wrote:
>
> On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1234@gmail.com> wrote:
> >
> > On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On 08/04/2025 09:35, Aaron Kling wrote:
> > > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >>
> > > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >>>>
> > > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > > >>>>> From: Aaron Kling <webgeek1234@gmail.com>
> > > >>>>>
> > > >>>>> This allows using pstore on all such platforms. There are some
> > > >>>>> differences per arch:
> > > >>>>>
> > > >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> > > >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> > > >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> > > >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> > > >>>>>   hardcodes a location matching what the downstream driver picks.
> > > >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> > > >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> > > >>>>>   thus these cannot be renamed.
> > > >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> > > >>>>>   compatible, however the dt still does not know the address, so keeping
> > > >>>>>   the node name consistent on Tegra186 and newer.
> > > >>>>>
> > > >>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > >>>>> ---
> > > >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> > > >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> > > >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> > > >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> > > >>>>>  4 files changed, 61 insertions(+)
> > > >>>>>
> > > >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > > >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> > > >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> > > >>>>>       };
> > > >>>>>
> > > >>>>> +     reserved-memory {
> > > >>>>> +             #address-cells = <2>;
> > > >>>>> +             #size-cells = <2>;
> > > >>>>> +             ranges;
> > > >>>>> +
> > > >>>>> +             ramoops_carveout {
> > > >>>>
> > > >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> > > >>>
> > > >>> As per the commit message regarding tegra186: bootloader fills in the
> > > >>> address and size in a node specifically named
> > > >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> > > >>
> > > >> That's not a reason to introduce issues. Bootloader is supposed to
> > > >> follow same conventions or use aliases or labels (depending on the node).
> > > >>
> > > >> If bootloader adds junk, does it mean we have to accept that junk?
> > > >>
> > > >>>
> > > >>>>
> > > >>>> It does not look like you tested the DTS against bindings. Please run
> > > >>>> `make dtbs_check W=1` (see
> > > >>>> Documentation/devicetree/bindings/writing-schema.rst or
> > > >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > > >>>> for instructions).
> > > >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> > > >>>> distro packages for dtschema and be sure you are using the latest
> > > >>>> released dtschema.
> > > >>>
> > > >>> The bot is reporting that the reg field is missing from the added
> > > >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > > >>> the commit message, this is intentional because it is expected for the
> > > >>> bootloader to fill that in. It is not known at dt compile time. Is
> > > >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> > > >>
> > > >> Fix your bootloader or chain load some normal one, like U-Boot.
> > > > How would chainloading a second bootloader 'fix' previous stage
> > > > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > > > address? It's possible for carveout addresses and sizes to change. Not
> > > > from boot to boot on the same version of the Nvidia bootloader, but
> > > > potentially from one version to another. Depending on if the
> > > > bootloader was configured with different carveout sizes.
> > > >
> > > > There is precedence for this. When blind cleanup was done on arm
> > > > device trees, a chromebook broke because the memory node has to be
> > > > named exactly '/memory' [0]. How is this any different from that case?
> > >
> > > That was an existing node, so ABI.
> > >
> > > > These nodes are an ABI to an existing bootloader. Carveouts on these
> > >
> > > You add new ABI, which I object to.
> > >
> > > > archs are set up in bl1 or bl2, which are not source available. I
> > > > could potentially hardcode things for myself in bl33, which is source
> > > > available, but the earlier stages could still overwrite any chosen
> > > > block depending on how carveouts are configured. But even then, that
> > > > will not change the behaviour of the vast majority of units that use a
> > > > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > > > pstore to work on such units without users needing to use a custom
> > > > bootloader.
> > > I understand your goal. What I still do not understand, why bootloader
> > > even bothers with ramoops carveout. It shouldn't and you should just
> > > ignore whatever bootloader provides, no?
> >
> > Mmm, I actually don't have the answer to this. Ramoops carveout
> > handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
> > late in the life cycle. But it has always been in edk2 for t194 and
> > t234 afaik. I could hazard some guesses, but don't have any
> > documentation on why the decision was made. Maybe Thierry or Jonathan
> > could chime in on why this was done.
> >
>
> Friendly reminder to the Tegra maintainers about this question.
>
In lieu of a response from the Tegra subsystem maintainers, I can only
hazard an assumption, Krzysztof. I presume the pstore carveout is
bootloader controlled because various stages of the boot stack can
dynamically allocate memory, and this became bootloader controlled to
prevent any of those from overwriting pstore. I worry about hardcoding
an address in the kernel dt, then finding out later that there's an
in-use configuration that overwrites or corrupts that section of ram
during boot. What are your thoughts on this? And is there any way for
this patch to proceed?

Sincerely,
Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Thierry Reding]

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

On Mon, Apr 28, 2025 at 08:21:55PM -0500, Aaron Kling wrote:
> On Sun, Apr 20, 2025 at 8:45 PM Aaron Kling <webgeek1234@gmail.com> wrote:
> >
> > On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1234@gmail.com> wrote:
> > >
> > > On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > On 08/04/2025 09:35, Aaron Kling wrote:
> > > > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >>
> > > > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > > > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >>>>
> > > > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > > > >>>>> From: Aaron Kling <webgeek1234@gmail.com>
> > > > >>>>>
> > > > >>>>> This allows using pstore on all such platforms. There are some
> > > > >>>>> differences per arch:
> > > > >>>>>
> > > > >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> > > > >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> > > > >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> > > > >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> > > > >>>>>   hardcodes a location matching what the downstream driver picks.
> > > > >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> > > > >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> > > > >>>>>   thus these cannot be renamed.
> > > > >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> > > > >>>>>   compatible, however the dt still does not know the address, so keeping
> > > > >>>>>   the node name consistent on Tegra186 and newer.
> > > > >>>>>
> > > > >>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > > >>>>> ---
> > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> > > > >>>>>  4 files changed, 61 insertions(+)
> > > > >>>>>
> > > > >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > > > >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> > > > >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> > > > >>>>>       };
> > > > >>>>>
> > > > >>>>> +     reserved-memory {
> > > > >>>>> +             #address-cells = <2>;
> > > > >>>>> +             #size-cells = <2>;
> > > > >>>>> +             ranges;
> > > > >>>>> +
> > > > >>>>> +             ramoops_carveout {
> > > > >>>>
> > > > >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> > > > >>>
> > > > >>> As per the commit message regarding tegra186: bootloader fills in the
> > > > >>> address and size in a node specifically named
> > > > >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> > > > >>
> > > > >> That's not a reason to introduce issues. Bootloader is supposed to
> > > > >> follow same conventions or use aliases or labels (depending on the node).
> > > > >>
> > > > >> If bootloader adds junk, does it mean we have to accept that junk?
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> It does not look like you tested the DTS against bindings. Please run
> > > > >>>> `make dtbs_check W=1` (see
> > > > >>>> Documentation/devicetree/bindings/writing-schema.rst or
> > > > >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > > > >>>> for instructions).
> > > > >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> > > > >>>> distro packages for dtschema and be sure you are using the latest
> > > > >>>> released dtschema.
> > > > >>>
> > > > >>> The bot is reporting that the reg field is missing from the added
> > > > >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > > > >>> the commit message, this is intentional because it is expected for the
> > > > >>> bootloader to fill that in. It is not known at dt compile time. Is
> > > > >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> > > > >>
> > > > >> Fix your bootloader or chain load some normal one, like U-Boot.
> > > > > How would chainloading a second bootloader 'fix' previous stage
> > > > > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > > > > address? It's possible for carveout addresses and sizes to change. Not
> > > > > from boot to boot on the same version of the Nvidia bootloader, but
> > > > > potentially from one version to another. Depending on if the
> > > > > bootloader was configured with different carveout sizes.
> > > > >
> > > > > There is precedence for this. When blind cleanup was done on arm
> > > > > device trees, a chromebook broke because the memory node has to be
> > > > > named exactly '/memory' [0]. How is this any different from that case?
> > > >
> > > > That was an existing node, so ABI.
> > > >
> > > > > These nodes are an ABI to an existing bootloader. Carveouts on these
> > > >
> > > > You add new ABI, which I object to.
> > > >
> > > > > archs are set up in bl1 or bl2, which are not source available. I
> > > > > could potentially hardcode things for myself in bl33, which is source
> > > > > available, but the earlier stages could still overwrite any chosen
> > > > > block depending on how carveouts are configured. But even then, that
> > > > > will not change the behaviour of the vast majority of units that use a
> > > > > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > > > > pstore to work on such units without users needing to use a custom
> > > > > bootloader.
> > > > I understand your goal. What I still do not understand, why bootloader
> > > > even bothers with ramoops carveout. It shouldn't and you should just
> > > > ignore whatever bootloader provides, no?
> > >
> > > Mmm, I actually don't have the answer to this. Ramoops carveout
> > > handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
> > > late in the life cycle. But it has always been in edk2 for t194 and
> > > t234 afaik. I could hazard some guesses, but don't have any
> > > documentation on why the decision was made. Maybe Thierry or Jonathan
> > > could chime in on why this was done.
> > >
> >
> > Friendly reminder to the Tegra maintainers about this question.
> >
> In lieu of a response from the Tegra subsystem maintainers, I can only
> hazard an assumption, Krzysztof. I presume the pstore carveout is
> bootloader controlled because various stages of the boot stack can
> dynamically allocate memory, and this became bootloader controlled to
> prevent any of those from overwriting pstore. I worry about hardcoding
> an address in the kernel dt, then finding out later that there's an
> in-use configuration that overwrites or corrupts that section of ram
> during boot. What are your thoughts on this? And is there any way for
> this patch to proceed?

I haven't been able to find anything out about this yet. Generally it's
difficult to get the bootloaders updated for these devices. Tegra194 and
Tegra234 may be new enough to make an update eventually go into a
release, but for Tegra186 and older, I honestly wouldn't hold my
breath.

Thierry

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

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Thierry Reding]

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

On Sun, Apr 06, 2025 at 04:12:43PM -0500, Aaron Kling via B4 Relay wrote:
> From: Aaron Kling <webgeek1234@gmail.com>
> 
> This allows using pstore on all such platforms. There are some
> differences per arch:
> 
> * Tegra132: Flounder does not appear to enumerate pstore and I do not
>   have access to norrin, thus Tegra132 is left out of this commit.
> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
>   relying on a dowstream driver to allocate the carveout, hence this
>   hardcodes a location matching what the downstream driver picks.
> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
>   size in a node specifically named /reserved-memory/ramoops_carveout,
>   thus these cannot be renamed.
> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
>   compatible, however the dt still does not know the address, so keeping
>   the node name consistent on Tegra186 and newer.

In order for this to be compatible with the standard bindings, all that
edk2 would need to do is add the unit-address, correct? I think that's
something we could probably get done since we do this for framebuffer
carveouts already.

Thierry

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

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Thu, May 8, 2025 at 4:27 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Apr 28, 2025 at 08:21:55PM -0500, Aaron Kling wrote:
> > On Sun, Apr 20, 2025 at 8:45 PM Aaron Kling <webgeek1234@gmail.com> wrote:
> > >
> > > On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1234@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >
> > > > > On 08/04/2025 09:35, Aaron Kling wrote:
> > > > > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > > >>
> > > > > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > > > > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > > >>>>
> > > > > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > > > > >>>>> From: Aaron Kling <webgeek1234@gmail.com>
> > > > > >>>>>
> > > > > >>>>> This allows using pstore on all such platforms. There are some
> > > > > >>>>> differences per arch:
> > > > > >>>>>
> > > > > >>>>> * Tegra132: Flounder does not appear to enumerate pstore and I do not
> > > > > >>>>>   have access to norrin, thus Tegra132 is left out of this commit.
> > > > > >>>>> * Tegra210: Does not support ramoops carveouts in the bootloader, instead
> > > > > >>>>>   relying on a dowstream driver to allocate the carveout, hence this
> > > > > >>>>>   hardcodes a location matching what the downstream driver picks.
> > > > > >>>>> * Tegra186 and Tegra194 on cboot: Bootloader fills in the address and
> > > > > >>>>>   size in a node specifically named /reserved-memory/ramoops_carveout,
> > > > > >>>>>   thus these cannot be renamed.
> > > > > >>>>> * Tegra194 and Tegra234 on edk2: Bootloader looks up the node based on
> > > > > >>>>>   compatible, however the dt still does not know the address, so keeping
> > > > > >>>>>   the node name consistent on Tegra186 and newer.
> > > > > >>>>>
> > > > > >>>>> Signed-off-by: Aaron Kling <webgeek1234@gmail.com>
> > > > > >>>>> ---
> > > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 16 ++++++++++++++++
> > > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 16 ++++++++++++++++
> > > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra210.dtsi | 13 +++++++++++++
> > > > > >>>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 16 ++++++++++++++++
> > > > > >>>>>  4 files changed, 61 insertions(+)
> > > > > >>>>>
> > > > > >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > > >>>>> index 2b3bb5d0af17bd521f87db0484fcbe943dd1a797..2e2b27deb957dfd754e42dd03f5a1da5079971dc 100644
> > > > > >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > > >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> > > > > >>>>> @@ -2051,6 +2051,22 @@ pmu-denver {
> > > > > >>>>>               interrupt-affinity = <&denver_0 &denver_1>;
> > > > > >>>>>       };
> > > > > >>>>>
> > > > > >>>>> +     reserved-memory {
> > > > > >>>>> +             #address-cells = <2>;
> > > > > >>>>> +             #size-cells = <2>;
> > > > > >>>>> +             ranges;
> > > > > >>>>> +
> > > > > >>>>> +             ramoops_carveout {
> > > > > >>>>
> > > > > >>>> Please follow DTS coding style for name, so this is probably only ramoops.
> > > > > >>>
> > > > > >>> As per the commit message regarding tegra186: bootloader fills in the
> > > > > >>> address and size in a node specifically named
> > > > > >>> /reserved-memory/ramoops_carveout, thus these cannot be renamed.
> > > > > >>
> > > > > >> That's not a reason to introduce issues. Bootloader is supposed to
> > > > > >> follow same conventions or use aliases or labels (depending on the node).
> > > > > >>
> > > > > >> If bootloader adds junk, does it mean we have to accept that junk?
> > > > > >>
> > > > > >>>
> > > > > >>>>
> > > > > >>>> It does not look like you tested the DTS against bindings. Please run
> > > > > >>>> `make dtbs_check W=1` (see
> > > > > >>>> Documentation/devicetree/bindings/writing-schema.rst or
> > > > > >>>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > > > > >>>> for instructions).
> > > > > >>>> Maybe you need to update your dtschema and yamllint. Don't rely on
> > > > > >>>> distro packages for dtschema and be sure you are using the latest
> > > > > >>>> released dtschema.
> > > > > >>>
> > > > > >>> The bot is reporting that the reg field is missing from the added
> > > > > >>> ramoops nodes on t186, t194, and t234. However, as also mentioned in
> > > > > >>> the commit message, this is intentional because it is expected for the
> > > > > >>> bootloader to fill that in. It is not known at dt compile time. Is
> > > > > >>> there a way to mark this as intentional, so dtschema doesn't flag it?
> > > > > >>
> > > > > >> Fix your bootloader or chain load some normal one, like U-Boot.
> > > > > > How would chainloading a second bootloader 'fix' previous stage
> > > > > > bootloaders trampling on an out-of-sync hardcoded reserved-memory
> > > > > > address? It's possible for carveout addresses and sizes to change. Not
> > > > > > from boot to boot on the same version of the Nvidia bootloader, but
> > > > > > potentially from one version to another. Depending on if the
> > > > > > bootloader was configured with different carveout sizes.
> > > > > >
> > > > > > There is precedence for this. When blind cleanup was done on arm
> > > > > > device trees, a chromebook broke because the memory node has to be
> > > > > > named exactly '/memory' [0]. How is this any different from that case?
> > > > >
> > > > > That was an existing node, so ABI.
> > > > >
> > > > > > These nodes are an ABI to an existing bootloader. Carveouts on these
> > > > >
> > > > > You add new ABI, which I object to.
> > > > >
> > > > > > archs are set up in bl1 or bl2, which are not source available. I
> > > > > > could potentially hardcode things for myself in bl33, which is source
> > > > > > available, but the earlier stages could still overwrite any chosen
> > > > > > block depending on how carveouts are configured. But even then, that
> > > > > > will not change the behaviour of the vast majority of units that use a
> > > > > > fully prebuilt boot stack direct from Nvidia. My intent here is for
> > > > > > pstore to work on such units without users needing to use a custom
> > > > > > bootloader.
> > > > > I understand your goal. What I still do not understand, why bootloader
> > > > > even bothers with ramoops carveout. It shouldn't and you should just
> > > > > ignore whatever bootloader provides, no?
> > > >
> > > > Mmm, I actually don't have the answer to this. Ramoops carveout
> > > > handling was added to t186 and t194 in cboot for L4T r32.7.3, fairly
> > > > late in the life cycle. But it has always been in edk2 for t194 and
> > > > t234 afaik. I could hazard some guesses, but don't have any
> > > > documentation on why the decision was made. Maybe Thierry or Jonathan
> > > > could chime in on why this was done.
> > > >
> > >
> > > Friendly reminder to the Tegra maintainers about this question.
> > >
> > In lieu of a response from the Tegra subsystem maintainers, I can only
> > hazard an assumption, Krzysztof. I presume the pstore carveout is
> > bootloader controlled because various stages of the boot stack can
> > dynamically allocate memory, and this became bootloader controlled to
> > prevent any of those from overwriting pstore. I worry about hardcoding
> > an address in the kernel dt, then finding out later that there's an
> > in-use configuration that overwrites or corrupts that section of ram
> > during boot. What are your thoughts on this? And is there any way for
> > this patch to proceed?
>
> I haven't been able to find anything out about this yet. Generally it's
> difficult to get the bootloaders updated for these devices. Tegra194 and
> Tegra234 may be new enough to make an update eventually go into a
> release, but for Tegra186 and older, I honestly wouldn't hold my
> breath.
>
> Thierry

Krzysztof, based on this response, is there any way or form that the
Tegra186 part of this could be submitted? I can drop the newer
platforms from this patch if Thierry can get a response to his other
reply about how the bootloader could conform.

Sincerely,
Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Krzysztof Kozlowski]

On 28/05/2025 19:35, Aaron Kling wrote:
>>>>
>>>> Friendly reminder to the Tegra maintainers about this question.
>>>>
>>> In lieu of a response from the Tegra subsystem maintainers, I can only
>>> hazard an assumption, Krzysztof. I presume the pstore carveout is
>>> bootloader controlled because various stages of the boot stack can
>>> dynamically allocate memory, and this became bootloader controlled to
>>> prevent any of those from overwriting pstore. I worry about hardcoding
>>> an address in the kernel dt, then finding out later that there's an
>>> in-use configuration that overwrites or corrupts that section of ram
>>> during boot. What are your thoughts on this? And is there any way for
>>> this patch to proceed?
>>
>> I haven't been able to find anything out about this yet. Generally it's
>> difficult to get the bootloaders updated for these devices. Tegra194 and
>> Tegra234 may be new enough to make an update eventually go into a
>> release, but for Tegra186 and older, I honestly wouldn't hold my
>> breath.
>>
>> Thierry
> 
> Krzysztof, based on this response, is there any way or form that the
> Tegra186 part of this could be submitted? I can drop the newer
> platforms from this patch if Thierry can get a response to his other
> reply about how the bootloader could conform.
> 
I don't NAK it. Eventually it is up to platform maintainer if they
accept known DTC warnings.

Best regards,
Krzysztof

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Thu, May 29, 2025 at 3:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 28/05/2025 19:35, Aaron Kling wrote:
> >>>>
> >>>> Friendly reminder to the Tegra maintainers about this question.
> >>>>
> >>> In lieu of a response from the Tegra subsystem maintainers, I can only
> >>> hazard an assumption, Krzysztof. I presume the pstore carveout is
> >>> bootloader controlled because various stages of the boot stack can
> >>> dynamically allocate memory, and this became bootloader controlled to
> >>> prevent any of those from overwriting pstore. I worry about hardcoding
> >>> an address in the kernel dt, then finding out later that there's an
> >>> in-use configuration that overwrites or corrupts that section of ram
> >>> during boot. What are your thoughts on this? And is there any way for
> >>> this patch to proceed?
> >>
> >> I haven't been able to find anything out about this yet. Generally it's
> >> difficult to get the bootloaders updated for these devices. Tegra194 and
> >> Tegra234 may be new enough to make an update eventually go into a
> >> release, but for Tegra186 and older, I honestly wouldn't hold my
> >> breath.
> >>
> >> Thierry
> >
> > Krzysztof, based on this response, is there any way or form that the
> > Tegra186 part of this could be submitted? I can drop the newer
> > platforms from this patch if Thierry can get a response to his other
> > reply about how the bootloader could conform.
> >
> I don't NAK it. Eventually it is up to platform maintainer if they
> accept known DTC warnings.
>
> Best regards,
> Krzysztof

If the decision is up the the tegra maintainers, then Thierry, what's
your thoughts now? What is in this patch should be compatible with
existing l4t and android bootloaders. But it does add a few new dtb
check lines.

Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Thierry Reding]

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

On Mon, Jun 30, 2025 at 01:48:28PM -0500, Aaron Kling wrote:
> On Thu, May 29, 2025 at 3:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 28/05/2025 19:35, Aaron Kling wrote:
> > >>>>
> > >>>> Friendly reminder to the Tegra maintainers about this question.
> > >>>>
> > >>> In lieu of a response from the Tegra subsystem maintainers, I can only
> > >>> hazard an assumption, Krzysztof. I presume the pstore carveout is
> > >>> bootloader controlled because various stages of the boot stack can
> > >>> dynamically allocate memory, and this became bootloader controlled to
> > >>> prevent any of those from overwriting pstore. I worry about hardcoding
> > >>> an address in the kernel dt, then finding out later that there's an
> > >>> in-use configuration that overwrites or corrupts that section of ram
> > >>> during boot. What are your thoughts on this? And is there any way for
> > >>> this patch to proceed?
> > >>
> > >> I haven't been able to find anything out about this yet. Generally it's
> > >> difficult to get the bootloaders updated for these devices. Tegra194 and
> > >> Tegra234 may be new enough to make an update eventually go into a
> > >> release, but for Tegra186 and older, I honestly wouldn't hold my
> > >> breath.
> > >>
> > >> Thierry
> > >
> > > Krzysztof, based on this response, is there any way or form that the
> > > Tegra186 part of this could be submitted? I can drop the newer
> > > platforms from this patch if Thierry can get a response to his other
> > > reply about how the bootloader could conform.
> > >
> > I don't NAK it. Eventually it is up to platform maintainer if they
> > accept known DTC warnings.
> >
> > Best regards,
> > Krzysztof
> 
> If the decision is up the the tegra maintainers, then Thierry, what's
> your thoughts now? What is in this patch should be compatible with
> existing l4t and android bootloaders. But it does add a few new dtb
> check lines.

I don't adding new DTC warnings, especially ones that we know up front
we can never get rid of. The memory one is a notable exception because
the system becomes unusable without it.

ramoops is not in that same category. While it's certainly nice to have,
I don't think it's critical enough to warrant that permanent exception.
Where possible I think we need to work to address issues souch as this
at the root and fix bootloaders to do the right thing.

For any cases where we can't fix the bootloaders, I think that's
something we have to live with. Having the support for this live in a
fork is a fair compromise, I think.

I know this is frustrating, and it's very painful for me personally
because I initially set out to redress a lot of these things and failed
to do so.

However I can't justify accepting endless amounts of quirks upstream,
all of which would set a bad precedent, just for the sake of things
being upstream.

Thierry

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

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Thu, Jul 3, 2025 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Jun 30, 2025 at 01:48:28PM -0500, Aaron Kling wrote:
> > On Thu, May 29, 2025 at 3:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On 28/05/2025 19:35, Aaron Kling wrote:
> > > >>>>
> > > >>>> Friendly reminder to the Tegra maintainers about this question.
> > > >>>>
> > > >>> In lieu of a response from the Tegra subsystem maintainers, I can only
> > > >>> hazard an assumption, Krzysztof. I presume the pstore carveout is
> > > >>> bootloader controlled because various stages of the boot stack can
> > > >>> dynamically allocate memory, and this became bootloader controlled to
> > > >>> prevent any of those from overwriting pstore. I worry about hardcoding
> > > >>> an address in the kernel dt, then finding out later that there's an
> > > >>> in-use configuration that overwrites or corrupts that section of ram
> > > >>> during boot. What are your thoughts on this? And is there any way for
> > > >>> this patch to proceed?
> > > >>
> > > >> I haven't been able to find anything out about this yet. Generally it's
> > > >> difficult to get the bootloaders updated for these devices. Tegra194 and
> > > >> Tegra234 may be new enough to make an update eventually go into a
> > > >> release, but for Tegra186 and older, I honestly wouldn't hold my
> > > >> breath.
> > > >>
> > > >> Thierry
> > > >
> > > > Krzysztof, based on this response, is there any way or form that the
> > > > Tegra186 part of this could be submitted? I can drop the newer
> > > > platforms from this patch if Thierry can get a response to his other
> > > > reply about how the bootloader could conform.
> > > >
> > > I don't NAK it. Eventually it is up to platform maintainer if they
> > > accept known DTC warnings.
> > >
> > > Best regards,
> > > Krzysztof
> >
> > If the decision is up the the tegra maintainers, then Thierry, what's
> > your thoughts now? What is in this patch should be compatible with
> > existing l4t and android bootloaders. But it does add a few new dtb
> > check lines.
>
> I don't adding new DTC warnings, especially ones that we know up front
> we can never get rid of. The memory one is a notable exception because
> the system becomes unusable without it.
>
> ramoops is not in that same category. While it's certainly nice to have,
> I don't think it's critical enough to warrant that permanent exception.
> Where possible I think we need to work to address issues souch as this
> at the root and fix bootloaders to do the right thing.
>
> For any cases where we can't fix the bootloaders, I think that's
> something we have to live with. Having the support for this live in a
> fork is a fair compromise, I think.
>
> I know this is frustrating, and it's very painful for me personally
> because I initially set out to redress a lot of these things and failed
> to do so.
>
> However I can't justify accepting endless amounts of quirks upstream,
> all of which would set a bad precedent, just for the sake of things
> being upstream.
>
> Thierry

Alright, so to make sure everything is on the same page, let me walk
through the archs.

T210: This fits within dt check requirements afaik. If I send a v2
with only t210, would that patch be acceptable? Though, I would like
to double check that my assumption about the arch is correct. The
downstream 4.9 kernel does allocations for ramoop I can't quite track
in the vendor code. I'm assuming that by matching what the downstream
kernel picks, that it's within a large carveout that the bootloader
will never touch. I've not seen any corruption in my use of it so far.
Is this a safe assumption?

T186: Software support for this arch is eol, so what the bootloader
does cannot be changed. Presumably no other choice but to relegate to
a commit in a fork or out of tree patches.

T194: Some software support still exists for this arch in L4T r35. Is
there any positive feedback on making bootloader changes to meet dt
check requirements, or is it too late in the cycle?

T234: Still has active software support in L4T r36. But essentially
the same question as t194.

T264: I assume whatever happens for t234 will be mirrored here.

Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Aaron Kling]

On Mon, Jul 14, 2025 at 1:01 AM Aaron Kling <webgeek1234@gmail.com> wrote:
>
> On Thu, Jul 3, 2025 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Jun 30, 2025 at 01:48:28PM -0500, Aaron Kling wrote:
> > > On Thu, May 29, 2025 at 3:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > On 28/05/2025 19:35, Aaron Kling wrote:
> > > > >>>>
> > > > >>>> Friendly reminder to the Tegra maintainers about this question.
> > > > >>>>
> > > > >>> In lieu of a response from the Tegra subsystem maintainers, I can only
> > > > >>> hazard an assumption, Krzysztof. I presume the pstore carveout is
> > > > >>> bootloader controlled because various stages of the boot stack can
> > > > >>> dynamically allocate memory, and this became bootloader controlled to
> > > > >>> prevent any of those from overwriting pstore. I worry about hardcoding
> > > > >>> an address in the kernel dt, then finding out later that there's an
> > > > >>> in-use configuration that overwrites or corrupts that section of ram
> > > > >>> during boot. What are your thoughts on this? And is there any way for
> > > > >>> this patch to proceed?
> > > > >>
> > > > >> I haven't been able to find anything out about this yet. Generally it's
> > > > >> difficult to get the bootloaders updated for these devices. Tegra194 and
> > > > >> Tegra234 may be new enough to make an update eventually go into a
> > > > >> release, but for Tegra186 and older, I honestly wouldn't hold my
> > > > >> breath.
> > > > >>
> > > > >> Thierry
> > > > >
> > > > > Krzysztof, based on this response, is there any way or form that the
> > > > > Tegra186 part of this could be submitted? I can drop the newer
> > > > > platforms from this patch if Thierry can get a response to his other
> > > > > reply about how the bootloader could conform.
> > > > >
> > > > I don't NAK it. Eventually it is up to platform maintainer if they
> > > > accept known DTC warnings.
> > > >
> > > > Best regards,
> > > > Krzysztof
> > >
> > > If the decision is up the the tegra maintainers, then Thierry, what's
> > > your thoughts now? What is in this patch should be compatible with
> > > existing l4t and android bootloaders. But it does add a few new dtb
> > > check lines.
> >
> > I don't adding new DTC warnings, especially ones that we know up front
> > we can never get rid of. The memory one is a notable exception because
> > the system becomes unusable without it.
> >
> > ramoops is not in that same category. While it's certainly nice to have,
> > I don't think it's critical enough to warrant that permanent exception.
> > Where possible I think we need to work to address issues souch as this
> > at the root and fix bootloaders to do the right thing.
> >
> > For any cases where we can't fix the bootloaders, I think that's
> > something we have to live with. Having the support for this live in a
> > fork is a fair compromise, I think.
> >
> > I know this is frustrating, and it's very painful for me personally
> > because I initially set out to redress a lot of these things and failed
> > to do so.
> >
> > However I can't justify accepting endless amounts of quirks upstream,
> > all of which would set a bad precedent, just for the sake of things
> > being upstream.
> >
> > Thierry
>
> Alright, so to make sure everything is on the same page, let me walk
> through the archs.
>
> T210: This fits within dt check requirements afaik. If I send a v2
> with only t210, would that patch be acceptable? Though, I would like
> to double check that my assumption about the arch is correct. The
> downstream 4.9 kernel does allocations for ramoop I can't quite track
> in the vendor code. I'm assuming that by matching what the downstream
> kernel picks, that it's within a large carveout that the bootloader
> will never touch. I've not seen any corruption in my use of it so far.
> Is this a safe assumption?
>
> T186: Software support for this arch is eol, so what the bootloader
> does cannot be changed. Presumably no other choice but to relegate to
> a commit in a fork or out of tree patches.
>
> T194: Some software support still exists for this arch in L4T r35. Is
> there any positive feedback on making bootloader changes to meet dt
> check requirements, or is it too late in the cycle?
>
> T234: Still has active software support in L4T r36. But essentially
> the same question as t194.
>
> T264: I assume whatever happens for t234 will be mirrored here.

Reminder about this set of questions.

Aaron

Re: [PATCH] arm64: tegra: Enable ramoops on Tegra210 and newer

[Thierry Reding]

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

On Mon, Jul 14, 2025 at 01:01:19AM -0500, Aaron Kling wrote:
> On Thu, Jul 3, 2025 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Jun 30, 2025 at 01:48:28PM -0500, Aaron Kling wrote:
> > > On Thu, May 29, 2025 at 3:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > On 28/05/2025 19:35, Aaron Kling wrote:
> > > > >>>>
> > > > >>>> Friendly reminder to the Tegra maintainers about this question.
> > > > >>>>
> > > > >>> In lieu of a response from the Tegra subsystem maintainers, I can only
> > > > >>> hazard an assumption, Krzysztof. I presume the pstore carveout is
> > > > >>> bootloader controlled because various stages of the boot stack can
> > > > >>> dynamically allocate memory, and this became bootloader controlled to
> > > > >>> prevent any of those from overwriting pstore. I worry about hardcoding
> > > > >>> an address in the kernel dt, then finding out later that there's an
> > > > >>> in-use configuration that overwrites or corrupts that section of ram
> > > > >>> during boot. What are your thoughts on this? And is there any way for
> > > > >>> this patch to proceed?
> > > > >>
> > > > >> I haven't been able to find anything out about this yet. Generally it's
> > > > >> difficult to get the bootloaders updated for these devices. Tegra194 and
> > > > >> Tegra234 may be new enough to make an update eventually go into a
> > > > >> release, but for Tegra186 and older, I honestly wouldn't hold my
> > > > >> breath.
> > > > >>
> > > > >> Thierry
> > > > >
> > > > > Krzysztof, based on this response, is there any way or form that the
> > > > > Tegra186 part of this could be submitted? I can drop the newer
> > > > > platforms from this patch if Thierry can get a response to his other
> > > > > reply about how the bootloader could conform.
> > > > >
> > > > I don't NAK it. Eventually it is up to platform maintainer if they
> > > > accept known DTC warnings.
> > > >
> > > > Best regards,
> > > > Krzysztof
> > >
> > > If the decision is up the the tegra maintainers, then Thierry, what's
> > > your thoughts now? What is in this patch should be compatible with
> > > existing l4t and android bootloaders. But it does add a few new dtb
> > > check lines.
> >
> > I don't adding new DTC warnings, especially ones that we know up front
> > we can never get rid of. The memory one is a notable exception because
> > the system becomes unusable without it.
> >
> > ramoops is not in that same category. While it's certainly nice to have,
> > I don't think it's critical enough to warrant that permanent exception.
> > Where possible I think we need to work to address issues souch as this
> > at the root and fix bootloaders to do the right thing.
> >
> > For any cases where we can't fix the bootloaders, I think that's
> > something we have to live with. Having the support for this live in a
> > fork is a fair compromise, I think.
> >
> > I know this is frustrating, and it's very painful for me personally
> > because I initially set out to redress a lot of these things and failed
> > to do so.
> >
> > However I can't justify accepting endless amounts of quirks upstream,
> > all of which would set a bad precedent, just for the sake of things
> > being upstream.
> >
> > Thierry
> 
> Alright, so to make sure everything is on the same page, let me walk
> through the archs.
> 
> T210: This fits within dt check requirements afaik. If I send a v2
> with only t210, would that patch be acceptable? Though, I would like
> to double check that my assumption about the arch is correct. The
> downstream 4.9 kernel does allocations for ramoop I can't quite track
> in the vendor code. I'm assuming that by matching what the downstream
> kernel picks, that it's within a large carveout that the bootloader
> will never touch. I've not seen any corruption in my use of it so far.
> Is this a safe assumption?

I haven't looked into ramoops support at all yet, so I don't know how
this is handled. If we can find out where exactly this memory is and
that it's guaranteed to be static, then maybe we can hardcode this. If
not, I don't think there's much of a chance to get anything in.

> T186: Software support for this arch is eol, so what the bootloader
> does cannot be changed. Presumably no other choice but to relegate to
> a commit in a fork or out of tree patches.

Sounds about right.

> T194: Some software support still exists for this arch in L4T r35. Is
> there any positive feedback on making bootloader changes to meet dt
> check requirements, or is it too late in the cycle?
> 
> T234: Still has active software support in L4T r36. But essentially
> the same question as t194.

We can probably fix things for these two generations.

> T264: I assume whatever happens for t234 will be mirrored here.

Yes. We do try to incrementally improve things on newer generations, so
anything we fix for Tegra194/Tegra234 I expect will be done for Tegra264
and later chips, too.

Thierry

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