Received-SPF: pass (google.com: domain of linux-kernel+bounces-103297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 14 Mar 2024 14:24:22 +0100
From: Stephan Gerhold <stephan@gerhold.net>
To: Sumit Garg <sumit.garg@linaro.org>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>, linux-arm-msm@vger.kernel.org,
	devicetree@vger.kernel.org, andersson@kernel.org,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, caleb.connolly@linaro.org,
	neil.armstrong@linaro.org, laetitia.mariottini@se.com,
	pascal.eberhard@se.com, abdou.saker@se.com, jimmy.lalande@se.com,
	benjamin.missey@non.se.com, daniel.thompson@linaro.org,
	linux-kernel@vger.kernel.org,
	Jagdish Gediya <jagdish.gediya@linaro.org>
Subject: Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC
 board DTS
Message-ID: <ZfL6hi0kfp9MXQ0H@gerhold.net>
References: <20240313123017.362570-1-sumit.garg@linaro.org>
 <20240313123017.362570-4-sumit.garg@linaro.org>
 <c0e10cbf-c6f3-4b0c-8616-983da2a40236@linaro.org>
 <CAFA6WYNMjCaa0FKjNv6a8VFkco3=GBfgWNDuckGZdiZ9dGmHgg@mail.gmail.com>
 <d82ab1f8-e677-485f-9a6b-4115acfd7239@linaro.org>
 <CAFA6WYNSumyScax=GkN42GJOG56T3odF5Ed9A2i1nk_exCyGtA@mail.gmail.com>
 <ZfLUu6_Vq7MvG2G3@gerhold.net>
 <CAFA6WYPN2Bt7zvDyd+02jrsZJz0sFhkD_o4W+PvU=-VC4W5k=A@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFA6WYPN2Bt7zvDyd+02jrsZJz0sFhkD_o4W+PvU=-VC4W5k=A@mail.gmail.com>
Content-Transfer-Encoding: 7bit

On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net> 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@linaro.org> wrote:
> > > > On 3/14/24 10:04, Sumit Garg wrote:
> > > > > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> 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@linaro.org>
> > > > >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > > >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > >>> ---
> > > > >>
> > > > >> [...]
> > > > >>
> > > > >>> +     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.

> >
> > Also, the DRAM size is now always taken from the DT (which is probably
> > better than the previous hardcoded size in the U-Boot board code).
> >
> > I think we should bring the dynamic DRAM size detection back, because
> > there are quite some boards available with varying DRAM size. Restoring
> > msm_fixup_memory() would likely be easiest, I guess the ideal solution
> > would be to parse SMEM in U-Boot's dram_init() function so even U-Boot
> > has the correct amount of DRAM to work with.
> 
> In the context of the HMIBSC board, it has 1 GB RAM LPDDR3 internal
> not expandable. IMO, having it in DT as default should be fine.
> 

Right. I was more talking in terms of DB410c here, where the lack of
this feature is kind of a regression.

Personally, I'm fine if you put the memory node here like this. If there
are concerns from others you could also move it to the -u-boot.dtsi and
omit it for Linux.

Thanks,
Stephan