Re: [PATCH 3/3] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 10.1 (SM-T530)

[Stephan Gerhold <stephan@gerhold.net>]

On Mon, Jul 18, 2022 at 03:51:54PM +0200, Krzysztof Kozlowski wrote:
> On 17/07/2022 23:34, Matti Lehtimäki wrote:
> > Add a device tree for the Samsung Galaxy Tab 4 10.1 (SM-T530) wifi tablet
> > based on the apq8026 platform.
> > 
> > Currently supported are accelerometer sensor, hall sensor, internal storage, physical
> > buttons (power & volume), screen (based on simple-framebuffer set up by
> > the bootloader) sdcard, touchscreen and USB.
> > 
> > Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > ---
> >  arch/arm/boot/dts/Makefile                    |   1 +
> >  .../dts/qcom-apq8026-samsung-matissewifi.dts  | 475 ++++++++++++++++++
> >  2 files changed, 476 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> > 
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 5112f493f494..4d02a1740079 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1010,6 +1010,7 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> >  	qcom-apq8016-sbc.dtb \
> >  	qcom-apq8026-asus-sparrow.dtb \
> >  	qcom-apq8026-lg-lenok.dtb \
> > +	qcom-apq8026-samsung-matissewifi.dtb \
> >  	qcom-apq8060-dragonboard.dtb \
> >  	qcom-apq8064-cm-qs600.dtb \
> >  	qcom-apq8064-ifc6410.dtb \
> > diff --git a/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts b/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> > new file mode 100644
> > index 000000000000..f4c5eb9db11c
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> > @@ -0,0 +1,475 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2022, Matti Lehtimäki <matti.lehtimaki@gmail.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "qcom-msm8226.dtsi"
> > +#include "qcom-pm8226.dtsi"
> > +#include <dt-bindings/input/input.h>
> > +
> > +/delete-node/ &smem_region;
> > +
> > +/ {
> > +	model = "Samsung Galaxy Tab 4 10.1";
> > +	compatible = "samsung,matissewifi", "qcom,apq8026";
> > +	chassis-type = "tablet";
> > +
> > +	qcom,msm-id = <0xC708FF01 0 0x20000>,
> > +		      <0xC708FF01 1 0x20000>,
> > +		      <0xC708FF01 2 0x20000>,
> > +		      <0xC708FF01 3 0x20000>;
> 
> Lower case hex and does not match bindings.
> https://lore.kernel.org/all/20220705130300.100882-2-krzysztof.kozlowski@linaro.org/
> 
> This would need detailed explanation because it really does not look
> correct.
> 

Just to give the explanation for reference: In general, qcom,msm-id with
three elements is something Qualcomm used for some old platforms before
introducing qcom,board-id.

qcom,msm-id = <X Y Z> should be equivalent to:
  qcom,msm-id = <X Z>;
  qcom,board-id = <Y 0>;

e.g. for apq8026-v2-mtp.dts Qualcomm used:
   qcom,msm-id = <199 8 0x20000>;
 = qcom,msm-id = <QCOM_ID_MSM8026 QCOM_BOARD_ID_MTP 0x20000>;
 = qcom,msm-id = <QCOM_ID_MSM8026 0x20000>;
   qcom,board-id = <QCOM_BOARD_ID_MTP 0>;

I guess old bootloaders may or may not accept the new form, depending on
the age of their code base.

Then Samsung took this and made it a lot worse, by replacing the SoC ID
with some random magic number (the 0xC708FF01). And what's even worse is
that all devices with the same SoC from Samsung use the same magic number
there. It is completely useless for dynamically matching the device.

In this case, I suggest just dropping the property because the device is
supported by lk2nd [1] which can be loaded as intermediary bootloader to
have a more standard boot process for mainline Linux. When booting
through lk2nd no qcom,msm-id/qcom,board-id is required, and it also adds
MAC addresses for WiFi/Bluetooth etc etc. :-)

[1]: https://github.com/msm8916-mainline/lk2nd

> [...]
> > +	reserved-memory {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges;
> > +
> > +		framebuffer@3200000 {
> 
> Generic node names, so memory@
> 

Rob specifically mentioned at some point that memory@ should not be used
in reserved-memory [1]. The device tree specification actually recommends
doing it like it is done here (at least for "framebuffer"):

> 3.5.2 /reserved-memory/ child nodes
> Following the generic-names recommended practice, node names should
> reflect the purpose of the node (ie. “framebuffer” or “dma-pool”).

[1]: https://lore.kernel.org/linux-arm-msm/CAL_Jsq+66j8Y5y+PQ+mezkaxN1pfHFKz524YUF4Lz_OU5E-mZQ@mail.gmail.com/

Thanks,
Stephan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel