Message-ID: <9ced2822-a9d2-2e59-fe40-6c6f690be487@somainline.org>
Date:   Thu, 27 Oct 2022 11:40:25 +0200
From:   Konrad Dybcio <konrad.dybcio@...ainline.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, robh+dt@...nel.org
Cc:     krzysztof.kozlowski+dt@...aro.org, vkoul@...nel.org,
        chaotian.jing@...iatek.com, ulf.hansson@...aro.org,
        matthias.bgg@...il.com, hsinyi@...omium.org,
        nfraprado@...labora.com, allen-kh.cheng@...iatek.com,
        fparent@...libre.com, sam.shih@...iatek.com,
        sean.wang@...iatek.com, long.cheng@...iatek.com,
        wenbin.mei@...iatek.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 7/7] arm64: dts: mediatek: Add support for MT6795 Sony
 Xperia M5 smartphone


On 27/10/2022 11:28, AngeloGioacchino Del Regno wrote:
> Il 29/07/22 14:00, Konrad Dybcio ha scritto:
>>
>>
>> On 29.07.2022 12:44, AngeloGioacchino Del Regno wrote:
>>> Add a basic support for the Sony Xperia M5 (codename "Holly")
>>> smartphone, powered by a MediaTek Helio X10 SoC.
>>>
>>> This achieves a console boot.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@...labora.com>
>
> Hello Konrad,
> First of all, I'm sorry for the very late reply.
>
>>> ---
>>>   arch/arm64/boot/dts/mediatek/Makefile         |  1 +
>>>   .../dts/mediatek/mt6795-sony-xperia-m5.dts    | 90 
>>> +++++++++++++++++++
>>>   2 files changed, 91 insertions(+)
>>>   create mode 100644 
>>> arch/arm64/boot/dts/mediatek/mt6795-sony-xperia-m5.dts
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile 
>>> b/arch/arm64/boot/dts/mediatek/Makefile
>>> index af362a085a02..72fd683c9264 100644
>>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>>> @@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt2712-evb.dtb
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6755-evb.dtb
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6779-evb.dtb
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-evb.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-sony-xperia-m5.dtb
>> -holly.dtb?
>>
>
> I prefer using the commercial name to identify the device.
> "Holly" is the smartphone project codename and that is mentioned 
> almost nowhere:
> the aim here is to enhance readability as to make it immediately 
> understandable
> that this devicetree is for the Xperia M5 device.

Ok, sounds good.


>
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
>>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt6795-sony-xperia-m5.dts 
>>> b/arch/arm64/boot/dts/mediatek/mt6795-sony-xperia-m5.dts
>>> new file mode 100644
>>> index 000000000000..94d011c4126c
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt6795-sony-xperia-m5.dts
>>> @@ -0,0 +1,90 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2022, Collabora Ltd
>>> + * Author: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@...labora.com>
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "mt6795.dtsi"
>>> +
>>> +#include <dt-bindings/gpio/gpio.h>
>> Looks unused.
>>
>
> Right, I'll remove that in v2.
>
>>> +
>>> +/ {
>>> +    model = "Sony Xperia M5";
>>> +    compatible = "sony,xperia-m5", "mediatek,mt6795";
>> sony,holly?
>>
>
> I'm sorry, but I can't understand the sense of adding that compatible 
> string to
> the mix. To the kernel, it doesn't mean anything - and we already have 
> another
> string advertising the specific machine, which is "sony,xperia-m5".

I was suggesting replacing xperia-m5 with holly, but since we agreed on 
keeping

m5 in the dtb name, I suppose it's fine for this one to stay too.


>
> Of course, there is no Xperia M5 with a different SoC and, even if 
> there was a
> xperia-m5 with a different SoC, we anyway have both a machine 
> compatible and a
> SoC compatible in here, so that would still not pose any issue.
>
>>> +    chassis-type = "handset";
>>> +
>>> +    aliases {
>>> +        mmc0 = &mmc0;
>>> +        mmc1 = &mmc1;
>>> +        serial0 = &uart0;
>>> +        serial1 = &uart1;
>>> +    };
>>> +
>>> +    memory@...00000 {
>>> +        device_type = "memory";
>>> +        reg = <0 0x40000000 0 0x1E800000>;
>> Lowercase hex in size. Also, doesn't the bootloader fill it in?
>>
>
> Updating the device to the latest software version will give you a 
> bootloader
> that fills that in, but the first-ever software release contains one 
> that will
> not do that in particular conditions (fastboot boot).

Ugh. If only vendors tested their software before shipping it to users..

I think it's worth to adding a comment mentioning that, though.


>
>>> +    };
>>> +
>>> +    reserved_memory: reserved-memory {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges;
>>> +
>>> +        /* 128 KiB reserved for ARM Trusted Firmware (BL31) */
>> Is that true for all devices with this SoC, or..? If so, it may be worth
>> moving this into mt6795.dtsi.
>>
>>> +        bl31_secmon_reserved: secmon@...00000 {
>> memory@, everywhere. Use labels to name the nodes.
>>
>
> I'm afraid that's not possible, as the bootloader is reading the 
> devicetree
> and requires these nodes to follow this naming.

Wow that's bad.. probably deserves a comment to prevent 'cleanups' 
breaking this.


Konrad

>
>>> +            no-map;
>> reg goes first.
>
> Will fix in v2.
>
> Best regards,
> Angelo
>
>

Powered by blists - more mailing lists