On Thu, May 8, 2025 at 4:27 PM Thierry Reding <[email protected]> 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 <[email protected]> wrote: > > > > > > On Tue, Apr 8, 2025 at 3:49 AM Aaron Kling <[email protected]> wrote: > > > > > > > > On Tue, Apr 8, 2025 at 3:17 AM Krzysztof Kozlowski <[email protected]> > > > > wrote: > > > > > > > > > > On 08/04/2025 09:35, Aaron Kling wrote: > > > > > > On Tue, Apr 8, 2025 at 1:08 AM Krzysztof Kozlowski > > > > > > <[email protected]> wrote: > > > > > >> > > > > > >> On 07/04/2025 18:00, Aaron Kling wrote: > > > > > >>> On Mon, Apr 7, 2025 at 7:59 AM Krzysztof Kozlowski > > > > > >>> <[email protected]> wrote: > > > > > >>>> > > > > > >>>> On 06/04/2025 23:12, Aaron Kling via B4 Relay wrote: > > > > > >>>>> From: Aaron Kling <[email protected]> > > > > > >>>>> > > > > > >>>>> 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 <[email protected]> > > > > > >>>>> --- > > > > > >>>>> 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
