Received-SPF: pass (google.com: domain of linux-kernel+bounces-103021-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240313123017.362570-1-sumit.garg@linaro.org>
 <20240313123017.362570-4-sumit.garg@linaro.org> <c0e10cbf-c6f3-4b0c-8616-983da2a40236@linaro.org>
In-Reply-To: <c0e10cbf-c6f3-4b0c-8616-983da2a40236@linaro.org>
From: Sumit Garg <sumit.garg@linaro.org>
Date: Thu, 14 Mar 2024 14:34:53 +0530
Message-ID: <CAFA6WYNMjCaa0FKjNv6a8VFkco3=GBfgWNDuckGZdiZ9dGmHgg@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC
 board DTS
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: 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, stephan@gerhold.net, 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>
Content-Type: text/plain; charset="UTF-8"

Hi Konrad,

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.

>
> > +
> > +     reserved-memory {
> > +             ramoops@bff00000 {
> > +                     compatible = "ramoops";
> > +                     reg = <0x0 0xbff00000 0x0 0x100000>;
> > +
> > +                     record-size = <0x20000>;
> > +                     console-size = <0x20000>;
> > +                     ftrace-size = <0x20000>;
>
> No ecc?

Okay I can add that.

>
> > +             };
> > +     };
> > +
> > +     usb2513 {
>
> Please use a generic node name, like usb-hub

Ack.

> and keep the nodes
> sorted.

See other email thread...

>
> > +             compatible = "smsc,usb3503";
> > +             reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> > +             initial-mode = <1>;
> > +     };
> > +
> > +     usb_id: usb-id {
> > +             compatible = "linux,extcon-usb-gpio";
> > +             id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&usb_id_default>;
>
> property-n
> property-names

Ack.

>
> [...]
>
> > +             led@5 {
> > +                     reg = <5>;
> > +                     label = "apq8016-hmibsc:green:wlan";
>
> These names look overly complicated.. s/apq8016-hmibsc://g?
>

I will drop them as suggested by Krzysztof.

>
> > +
> > +&blsp_i2c3 {
> > +     status = "okay";
> > +
> > +     eeprom@50 {
> > +             compatible = "atmel,24c32";
> > +             reg = <0x50>;
> > +     };
> > +};
> > +
> > +&blsp_i2c4 {
> > +     status = "okay";
> > +
> > +     adv_bridge: bridge@39 {
> > +             status = "okay";
>
> ???

Will drop it.

>
> > +
> > +             compatible = "adi,adv7533";
> > +             reg = <0x39>;
> > +
> > +             interrupt-parent = <&tlmm>;
> > +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>
> interrupts-extended
>

Please see Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml.

-Sumit

> Konrad