On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <webgeek1...@gmail.com> wrote:
>
> On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <k...@kernel.org> wrote:
> >
> > On 08/04/2025 09:35, Aaron Kling wrote:
> > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski <k...@kernel.org>
> > > wrote:
> > >>
> > >> On 07/04/2025 18:00, Aaron Kling wrote:
> > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski <k...@kernel.org>
> > >>> wrote:
> > >>>>
> > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote:
> > >>>>> From: Aaron Kling <webgeek1...@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 <webgeek1...@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
