Re: [PATCH v2 2/2] arm64: dts: mediatek: Introduce MT8188 Geralt platform based Ciri

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 07/11/24 07:58, Fei Shao ha scritto:
> On Wed, Nov 6, 2024 at 9:19 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 05/11/24 10:30, Fei Shao ha scritto:
>>> Introduce MT8188-based Chromebook Ciri, also known commercially as
>>> Lenovo Chromebook Duet (11", 9).
>>>
>>> Ciri is a detachable device based on the Geralt design, where Geralt is
>>> the codename for the MT8188 platform. Ciri offers 8 SKUs to accommodate
>>> different combinations of second-source components, including:
>>> - audio codecs (RT5682S and ES8326)
>>> - speaker amps (TAS2563 and MAX98390)
>>> - MIPI-DSI panels (BOE nv110wum-l60 and IVO t109nw41)
>>>
>>> Signed-off-by: Fei Shao <fshao@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - remove invalid or undocumented properties
>>>       e.g. mediatek,dai-link, maxim,dsm_param_name etc.
>>> - remove touchscreen as the driver is not yet accepted in upstream
>>> - update sound DAI link node name to match the binding
>>> - add missing pinctrls in audio codec nodes
>>>
>>>    arch/arm64/boot/dts/mediatek/Makefile         |    8 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku0.dts  |   11 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku1.dts  |   60 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku2.dts  |   56 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku3.dts  |   15 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku4.dts  |   43 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku5.dts  |   73 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku6.dts  |   69 +
>>>    .../dts/mediatek/mt8188-geralt-ciri-sku7.dts  |   47 +
>>>    .../boot/dts/mediatek/mt8188-geralt-ciri.dtsi |  397 +++++
>>>    .../boot/dts/mediatek/mt8188-geralt.dtsi      | 1492 +++++++++++++++++
>>>    11 files changed, 2271 insertions(+)
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku0.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku1.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku2.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku3.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku4.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku5.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku6.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri-sku7.dts
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri.dtsi
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>>
> [...]
> 

[...]

>>> +&pmic {
>>> +     interrupts-extended = <&pio 222 IRQ_TYPE_LEVEL_HIGH>;
>>> +};
>>> +
>>> +&scp {
>>
>> Is this SCP-dual or SCP?
>> I see SCP, but I also see a SCP-Dual memory region... what's going on here?
>>
>> Of course, the SCP-Dual won't work if you don't override the compatible string...
> 
> To clarify, the second SCP core is used for MIPI camera in downstream,
> and I deliberately only describe the first SCP core here since the MTK
> camera ISP driver isn't in upstream at the moment.
> I had a fixup patch for removing the scp-dual reserved memory region,
> but likely it was missing during the rebase... let me check again if
> it can be removed, just in case there's firmware protecting the region
> and the kernel shouldn't access it.
> 

Hmm... but the second SCP core can still be brought up, even if the MIPI Camera
driver is not upstreamed yet, right?

That shouldn't cause lockups and/or other kinds of bad behavior, and should
bring up a core and just never use it, without any particular issues.

If we can enable the secondary core, let's just go for it.. as that will help
specifying the exact memory layout of this board (and failing to do that may
create some other issues, that's why I'm proposing to enable that even if it
is not really used in this case).

What do you think? :-)

>>
>>> +     firmware-name = "mediatek/mt8188/scp.img";
>>> +     memory-region = <&scp_mem>;
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&scp_pins>;
>>> +     status = "okay";
>>> +
>>> +     cros-ec-rpmsg {
>>> +             compatible = "google,cros-ec-rpmsg";
>>> +             mediatek,rpmsg-name = "cros-ec-rpmsg";
>>> +     };
>>> +};
>>> +
>>> +&sound {
>>> +     compatible = "mediatek,mt8188-nau8825";
>>> +     model = "mt8188_m98390_8825";
>>> +     pinctrl-names = "aud_etdm_hp_on",
>>> +                     "aud_etdm_hp_off",
>>> +                     "aud_etdm_spk_on",
>>> +                     "aud_etdm_spk_off",
>>> +                     "aud_mtkaif_on",
>>> +                     "aud_mtkaif_off";
>>
>>          pinctrl-names = "aud_etdm_hp_on", "aud_etdm_hp_off",
>>                          "aud_etdm_spk_on", "aud_etdm_spk_off",
>>                          "aud_mtkaif_on", "aud_mtkaif_off";
> 
> Acked.
> 
>>
>>> +     pinctrl-0 = <&aud_etdm_hp_on>;
>>> +     pinctrl-1 = <&aud_etdm_hp_off>;
>>> +     pinctrl-2 = <&aud_etdm_spk_on>;
>>> +     pinctrl-3 = <&aud_etdm_spk_off>;
>>> +     pinctrl-4 = <&aud_mtkaif_on>;
>>> +     pinctrl-5 = <&aud_mtkaif_off>;
>>
>> Add a comment:
>>
>>          /* The audio-routing is defined in each board dts */
>>
>>> +     audio-routing = "ETDM1_OUT", "ETDM_SPK_PIN",
>>> +                     "ETDM2_OUT", "ETDM_HP_PIN",
>>> +                     "ETDM1_IN", "ETDM_SPK_PIN",
>>> +                     "ETDM2_IN", "ETDM_HP_PIN",
>>> +                     "ADDA Capture", "MTKAIF_PIN",
>>> +                     "Headphone Jack", "HPOL",
>>> +                     "Headphone Jack", "HPOR",
>>> +                     "MIC", "Headset Mic",
>>> +                     "Left Spk", "Front Left BE_OUT",
>>> +                     "Right Spk", "Front Right BE_OUT",
>>> +                     "Rear Left Spk", "Rear Left BE_OUT",
>>> +                     "Rear Right Spk", "Rear Right BE_OUT";
>>> +
>>
>> ...and remove the audio-routing from this dtsi; it's anyway being
>> overridden by the -ciri.dtsi devicetree...
> 
> Acknowledged, and thanks for all the feedback here.
> 

You're welcome :-)

Cheers,
Angelo