Re: [PATCH 3/4] arm64: dts: qcom: sc7280: Factor out Chrome common fragment

From: Stephen Boyd <swboyd@chromium.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Douglas Anderson <dianders@chromium.org>
Cc: quic_rjendra@quicinc.com, sibis@codeaurora.org,
	kgodara1@codeaurora.org, mka@chromium.org,
	pmaliset@codeaurora.org, Andy Gross <agross@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] arm64: dts: qcom: sc7280: Factor out Chrome common fragment
Date: Thu, 13 Jan 2022 22:15:09 -0800	[thread overview]
Message-ID: <CAE-0n51e=rYtC3KyhpiO7KSJArxw2pmvh5v8TZ8Yrf-ToTv3Bw@mail.gmail.com> (raw)
In-Reply-To: <20220113164233.3.Iac012fa8d727be46448d47027a1813ea716423ce@changeid>

Quoting Douglas Anderson (2022-01-13 16:43:02)
> This factors out a device tree fragment from some sc7280 device
> trees. It represents the device tree bits that should be included for
> "Chrome" based sc7280 boards. On these boards the bootloader (Coreboot
> + Depthcharge) configures things slightly different than the
> bootloader that Qualcomm provides. The modem firmware on these boards
> also works differently than on other Qulacomm products and thus the
> reserved memory map needs to be adjusted.
>
> NOTES:
> - This is _not_ quite a no-op change. The "herobrine" and "idp"
>   fragments here were different and it looks like someone simply
>   forgot to update the herobrine version. This updates a few numbers
>   to match IDP. This will also cause the `pmk8350_pon` to be disabled
>   on idp/crd, which I belive is a correct change.
> - At the moment this assumes LTE skus. Once it's clearer how WiFi SKUs
>   will work (how much of the memory map they can reclaim) we may add
>   an extra fragment that will rejigger one way or the other.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

One comment below.

> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> new file mode 100644
> index 000000000000..9d4f25f77152
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> @@ -0,0 +1,97 @@
[...]
> +
> +/* Modem setup is different on Chrome setups than typical Qualcomm setup */
> +&remoteproc_mpss {
> +       status = "okay";
> +       compatible = "qcom,sc7280-mss-pil";
> +       iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
> +       memory-region = <&mba_mem &mpss_mem>;

I'd prefer this be

	memory-region = <&mba_mem>, <&mpss_mem>;

so that we know mpss_mem isn't an "argument" or cell for mba_mem. I see
that this is just moving around in this patch though so probably can be
done in a followup.

> +};
> +
> +/* Increase the size from 2.5MB to 8MB */
> +&rmtfs_mem {
> +       reg = <0x0 0x9c900000 0x0 0x800000>;
> +};