lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <59f4bcc1-c752-4f2f-8e55-349cc2432b8a@collabora.com>
Date: Thu, 7 Nov 2024 11:37:34 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Fei Shao <fshao@...omium.org>
Cc: Matthias Brugger <matthias.bgg@...il.com>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] arm64: dts: mediatek: Introduce MT8188 Geralt
 platform based Ciri

Il 07/11/24 07:58, Fei Shao ha scritto:
> On Wed, Nov 6, 2024 at 9:19 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.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@...omium.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ