On 15/03/2024 09:31, 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. >> >> "standardized" I'm not so sure... But yes, smem does offer this. The >> definition in DT here is for U-Boot, > > We should move away from that thinking that U-Boot has its own DT and > Linux kernel has its own. IMO, that's just the opposite of the true DT > definition. I was clarifying here that the memory node being defined with real values was because U-Boot uses them. Just in case some folks thought that Linux was using whatever values were here. > >> ABL will always clobber it, and so >> does U-Boot before handing over to the kernel. Linux should never see >> this without a bootloader having looked at it. > > Where does U-Boot clobber SMEM? I would be interested to see if ABL > clobbers it too? Not SMEM, the /memory node. > >> >> 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. Agreed > >>> >>> 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? > > -Sumit > >>> >>> As for linux, I am working on making Linux aware of the DDR capabilities >>> on Snapdragons, for other reasons, but it's on the back burner, as it >>> still needs some broad thinking about integrating it with the interested >>> consumers.. Bottom line is, Linux should be fed a devicetree with DRAM size >>> filled. >>> >>> Konrad >> >> -- >> // Caleb (they/them) -- // Caleb (they/them)