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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 18/07/2022 20:10, Stephan Gerhold wrote:
> 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, makes sense a lot!

Best regards,
Krzysztof