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

[Aaron Kling]

On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 08/04/2025 09:35, Aaron Kling wrote:
> > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> >>
> >> On 07/04/2025 18:00, Aaron Kling wrote:
> >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> >>>>> From: Aaron Kling <webgeek1234@xxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxx>
> >>>>> ---
> >>>>>  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