From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Liang Date: Wed, 16 Sep 2020 17:41:33 +0800 Subject: [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode In-Reply-To: References: <20200905132211.412711-1-seanga2@gmail.com> <705ddfd4-ebe3-bb6e-4a85-cbb32ab5832c@gmail.com> <5f2fa7b0-56d3-76d4-d8ce-cdfd12dbb753@gmail.com> Message-ID: <20200916094132.GA25540@andestech.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, Sep 14, 2020 at 07:57:04AM -0400, Sean Anderson wrote: > On 9/14/20 2:38 AM, Bin Meng wrote: > > On Sat, Sep 12, 2020 at 2:25 AM Sean Anderson wrote: > >> > >> On 9/11/20 10:43 AM, Bin Meng wrote: > >>> On Fri, Sep 11, 2020 at 6:20 PM Sean Anderson wrote: > >>>> > >>>> On 9/11/20 3:29 AM, Bin Meng wrote: > >>>>> Hi Sean, > >>>>> > >>>>> On Sat, Sep 5, 2020 at 9:22 PM Sean Anderson wrote: > >>>>>> > >>>>>> It is unsafe to enable OF_BOARD_FIXUP only based on OF_SEPARATE. > >>>>>> OF_SEPARATE may indicate that the user wishes U-Boot to use a different > >>>>>> device tree than one obtained via OF_PRIOR_STAGE. However, OF_SEPARATE may > >>>>>> also indicate that the device tree which would be obtained via > >>>>>> OF_PRIOR_STAGE is invalid, nonexistant, or otherwise unusable. In this > >>>>> > >>>>> typo: nonexistent > >>>>> > >>>>>> latter case, enabling OF_BOARD_FIXUP will result in corruption of the > >>>>>> device tree. To remedy this, only enable OF_BOARD_FIXUP if U-Boot is > >>>>>> configured for S-Mode. > >>>>>> > >>>>>> Fixes: 1c17e55594a394ced7de88d91be294eaf8c564c1 > >>>>> > >>>>> nits: the format should be: commit_id ("description")> > >>>>>> Signed-off-by: Sean Anderson > >>>>>> --- > >>>>>> > >>>>>> arch/riscv/Kconfig | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > >>>>>> index 009a545fcf..13fac51483 100644 > >>>>>> --- a/arch/riscv/Kconfig > >>>>>> +++ b/arch/riscv/Kconfig > >>>>>> @@ -288,6 +288,6 @@ config STACK_SIZE_SHIFT > >>>>>> default 14 > >>>>>> > >>>>>> config OF_BOARD_FIXUP > >>>>>> - default y if OF_SEPARATE > >>>>>> + default y if OF_SEPARATE && RISCV_SMODE > >>>>> > >>>>> Is that your board is running U-Boot M-mode with OF_SEPARATE that does not work? > >>>> > >>>> Yes, because the reason we use OF_SEPARATE is because no device tree is > >>>> passed to U-Boot. Trying to use the device tree passed to U-Boot even > >>> > >>> I don't get it. If no device tree is passed to U-Boot, why using > >>> OF_SEPARATE in the first place? > >> > >> Because it has to come from somewhere. Where else would U-Boot get the > >> device tree? > > > > Sounds like there was some misunderstanding on "passed to U-Boot" .. > > But I got it now. > > > >> > >>>> though OF_SEPARATE is enabled results in garbage being written to the > >>> > >>> What garbage data is written? > >> > >> It might not be garbage written. I didn't document the exact failure > >> mode at the time I discovered this bug, so I went back to try and > >> reproduce it for a more thorough analysis. However, I was unable to > >> reproduce this bug, even on the branch where I originally triggered it. > >> I documented my reasoning behind this patch at [1]. In my testing, I > >> could only trigger a "periodic-32" bug. > >> > >> In any case, this behavior could still cause problems in the future. > >> From my testing, on the k210, a1 usually holds some address on the ROM's > >> stack. However, if it (for instance) instead held an address which > > > > So U-Boot on K210 boots with M-mode from the K210 ROM, and the ROM > > code does not hold DTB address in a1 before jumping to U-Boot, right? > > > >> raised a load access fault, or was misaligned, then booting would fail. > >> In the general case, I was very surpised that U-Boot was using the value > >> of a1 on entry even with OF_SEPARATE specified. I would expect it only > >> to use that value if configured with OF_PRIOR_STAGE. > > > > Because U-Boot S-mode needs to fix up the DT when OF_SEPERATE is used. > > Right. It's just unexpected because OF_SEPARATE appears to imply to both > use a separate device tree and to not use the passed-in device tree. > This is because it is mutually exclusive with OF_PRIOR_STAGE. However, > with OF_BOARD_FIXUP, it's as if one has selected both OF_SEPARATE and > OF_PRIOR_STAGE at once. I think defaulting OF_BOARD_FIXUP to y only > S-Mode is more likely to result in unsurprising behavior on new boards. > > --Sean Reviewed-by: Leo Liang