Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Stephan Gerhold]

On Fri, Mar 15, 2024 at 03:01:27PM +0530, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 21:07, Caleb Connolly <caleb.connolly@xxxxxxxxxx> wrote:
> > On 14/03/2024 15:20, Konrad Dybcio wrote:
> > > On 3/14/24 14:50, Sumit Garg wrote:
> > >> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@xxxxxxxxxxx>
> > >> wrote:
> > >>>
> > >>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> > >>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@xxxxxxxxxxx>
> > >>>> wrote:
> > >>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > >>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
> > >>>>>> <konrad.dybcio@xxxxxxxxxx> wrote:
> > >>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
> > >>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
> > >>>>>>>> <konrad.dybcio@xxxxxxxxxx> wrote:
> > >>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
> > >>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
> > >>>>>>>>>> an IIoT Edge
> > >>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
> > >>>>>>>>>>
> > >>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
> > >>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
> > >>>>>>>>>> 306)
> > >>>>>>>>>> - 1GiB RAM
> > >>>>>>>>>> - 8GiB eMMC, SD slot
> > >>>>>>>>>> - WiFi and Bluetooth
> > >>>>>>>>>> - 2x Host, 1x Device USB port
> > >>>>>>>>>> - HDMI
> > >>>>>>>>>> - Discrete TPM2 chip over SPI
> > >>>>>>>>>> - USB ethernet adaptors (soldered)
> > >>>>>>>>>>
> > >>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@xxxxxxxxxx>
> > >>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@xxxxxxxxxx>
> > >>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > >>>>>>>>>> ---
> > >>>>>>>>>
> > >>>>>>>>> [...]
> > >>>>>>>>>
> > >>>>>>>>>> +     memory@80000000 {
> > >>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
> > >>>>>>>>>> +     };
> > >>>>>>>>>
> > >>>>>>>>> I'm not sure the entirety of DRAM is accessible..
> > >>>>>>>>>
> > >>>>>>>>> This override should be unnecessary, as bootloaders generally
> > >>>>>>>>> update
> > >>>>>>>>> the size field anyway.
> > >>>>>>>>
> > >>>>>>>> On this board, U-Boot is used as the first stage bootloader
> > >>>>>>>> (replacing
> > >>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > >>>>>>>> memory range from DT as Linux does but doesn't require any
> > >>>>>>>> memory to
> > >>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
> > >>>>>>>> explicitly described in DT all the other DRAM regions are
> > >>>>>>>> accessible.
> > >>>>>>>
> > >>>>>>> Still, u-boot has code to fetch the size dynamically, no?
> > >>>>>>>
> > >>>>>>
> > >>>>>> No U-Boot being the first stage bootloader fetches size from DT which
> > >>>>>> is bundled into U-Boot binary.
> > >>>>>>
> > >>>>>
> > >>>>> Back when I added support for using U-Boot as first stage
> > >>>>> bootloader on
> > >>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> > >>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
> > >>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> > >>>>> When booting Linux, the Linux DT was dynamically patched with the
> > >>>>> right
> > >>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
> > >>>>> DB4
> > >>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> > >>>>> later got the full 2 GiB patched into its DTB.
> > >>>>>
> > >>>>> I didn't have much time for testing U-Boot myself lately but a quick
> > >>>>> look at the recent changes suggest that Caleb accidentally removed
> > >>>>> that
> > >>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> > >>>>> size detection was removed in commit 14868845db54 ("board:
> > >>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
> > >>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
> > >>>>
> > >>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
> > >>>> based approach the standardized way used by early stage boot-loaders
> > >>>> on other Qcom SoCs too?
> > >>>>
> > >>>
> > >>> It is definitely used on all the SoCs that were deployed with LK. I am
> > >>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
> > >>> the ABL source code suggests it is abstracted through an EFI protocol
> > >>> there (so we cannot see where the information comes from with just the
> > >>> open-source code). However, in my experience SMEM data structures are
> > >>> usually kept quite stable (or properly versioned), so it is quite likely
> > >>> that we could use this approach for all Qualcomm SoCs.
> > >>>
> > >>
> > >> If the SoCs which support this standardized way to dynamic discover
> > >> DRAM size via SMEM then why do we need to rely on DT at all for those
> > >> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
> > >> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
> > >> DT when that information can be discovered dynamically.
> >
> > [...]
> >
> > The reason I decided to hardcode this in DT for U-Boot is because SMEM
> > currently relies on the driver model and isn't available early enough.
> >
> > Also admittedly I just wasn't that familiar with the U-Boot codebase. I
> > just wanted to avoid hardcoding this in C code, and given that this was
> > already supported for the "chainloading from ABL" usecase, just
> > hardcoding the values was the obvious solution.
> >
> > I would definitely be open to revisiting this in U-Boot, having an SMEM
> > framework that we could use without the driver model which would just
> > take a base address and then let us interact with SMEM and populate the
> > dram bank data would be a good improvement, and would let us avoid
> > hardcoding the memory layout in DT. We'd just need to manually find the
> > SMEM base address in the FDT as part of "dram_init_banksize()" and
> > retrieve the data there.
> 
> These are the similar problems Linux has to deal with too but on Qcom
> platforms it is rather offloaded to bootloaders to rather implement
> this. It leads to custom DT modification or board code in U-Boot which
> is hard to maintain. If we want to implement it properly then
> corresponding bindings should be upstreamed too regarding how DRAM
> range is to be discovered via SMEM.
> 
> >
> > That all being said, I don't see any reason not to define the memory
> > layout in DT, it's a hardware feature, DT describes the hardware. The
> > whole "bootloader will fill this in" implies that the bootloader isn't
> > also using DT as the source of truth, but now with U-Boot it actually
> > is, so it's all the more important that DT be accurate ;P
> 
> I agree DT should be accurate and not a fan of DT fixups. However,
> when it comes to some hardware configuration being discoverable then
> IMHO DT isn't the appropriate place for that. For the time being I am
> fine with the DRAM range to be specified in DT.
> 
> > >
> > > You're mixing two things. Linux expects a devicetree where
> > > /memory/reg[size]
> > > is valid. Such driver should indeed be (re)implemented in u-boot to provide
> > > this information.
> 
> No, I don't think so. We should rather start thinking about the
> overall stack rather than just being Linux kernel centric. Do you have
> a generic solution for U-Boot regarding how this should be
> implemented?
> 

Detecting available memory via /memory in the DT or other firmware
services (such as UEFI GetMemoryMap()) is *the* generic solution used
everywhere, independent of the hardware (e.g. Qualcomm) and the
operating system (e.g. Linux or Xen).

It allows us to implement the board/Qualcomm-specific memory detection
in a single project, rather than having extra porting overhead for each
operating system for something as essential as available memory.

If we implement the SMEM-based memory detection in U-Boot (probably in
dram_init_banksize() as Caleb suggested) everything else will just work
without any Qualcomm-specific DT patching in U-Boot, and without any
special code in the operating system:

 - U-Boot automatically updates the /memory node in generic code (see
   fdt_fixup_memory_banks() call in arch/arm/lib/bootm-fdt.c). If you
   are using UEFI for booting, U-Boot will provide the same information
   via GetMemoryMap(). (The DT spec says /memory should be ignored on
   UEFI systems.)

 - Linux, Xen, and any other operating system can obtain the memory size
   via the standard method, and do not need any Qualcomm-specific at all
   (at least for memory detection).

I don't think there is anything Linux kernel centric for the memory
detection here. Quite the opposite really. :)

Thanks,
Stephan