| Message ID | 20250406-tegra-pstore-v1-1-bf5b57f12293@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | arm64: tegra: Enable ramoops on Tegra210 and newer | expand |
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#
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
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
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
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
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
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
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)>,