On Wed, Nov 6, 2024 at 9:19 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > > > 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 > > [...] > > diff --git a/arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri.dtsi b/arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri.dtsi > > new file mode 100644 > > index 000000000000..62c8a37a4c3d > > --- /dev/null > > +++ b/arch/arm64/boot/dts/mediatek/mt8188-geralt-ciri.dtsi > > @@ -0,0 +1,397 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > > +/* > > + * Copyright 2023 Google LLC > > + */ > > +/dts-v1/; > > +#include "mt8188-geralt.dtsi" > > + > > +/delete-node/ &pp3300_edp_disp; > > + > > +&aud_etdm_hp_on { > > + pins-mclk { > > + pinmux = <PINMUX_GPIO114__FUNC_O_I2SO2_MCK>; > > + }; > > +}; > > + > > +&aud_etdm_hp_off { > > + pins-mclk { > > + pinmux = <PINMUX_GPIO114__FUNC_B_GPIO114>; > > + bias-pull-down; > > + input-enable; > > + }; > > +}; > > + > > +&aud_etdm_spk_on { > > + pins-bus { > > + drive-strength = <8>; > > + }; > > +}; > > + > > +/* Ciri's TDP design target is 90 degrees */ > > ...and there's only Ciri in this submission, so move that to -geralt.dtsi In response to the "there's only Ciri, so move to / don't declare unused stuff in -geralt.dtsi" and similar comments - I'll adjust each of the mentioned lines. My intention was to clearly describe the differences between Ciri and the proto hardware, but I understand and also agree with your point that these deltas should be introduced only when a real product utilizes them. I will follow this approach in the next revision. > > > +&cpu_little0_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&cpu_little1_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&cpu_little2_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&cpu_little3_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&cpu_big0_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&cpu_big1_alert0 { > > + temperature = <90000>; > > + hysteresis = <2000>; > > + type = "passive"; > > +}; > > + > > +&dp_intf0 { > > + /delete-node/ port; > > Just don't add dp_intf0 if there's none, instead of removing the port here. > > > +}; > > + > > +&dsi_panel { > > + compatible = "boe,nv110wum-l60", "himax,hx83102"; > > Move this to each SKU dts file. Will do. > > > +}; > > + > > +&edp_tx { > > + /delete-node/ ports; > > + /delete-node/ aux-bus; > > +}; > > + > > +&i2c0 { > > + /delete-node/ audio-codec@1a; > > + /delete-node/ amplifier@3a; > > + /delete-node/ amplifier@3b; > > No board ever uses those three nodes, because it's all Ciri and nothing else. > Just never declare these in -geralt.dtsi and never delete them here. > > > + > > + rt5682s: audio-codec@1a { > > + compatible = "realtek,rt5682s"; > > + reg = <0x1a>; > > + interrupts-extended = <&pio 108 IRQ_TYPE_EDGE_BOTH>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&audio_codec_pins>; > > + #sound-dai-cells = <1>; > > + > > + AVDD-supply = <&mt6359_vio18_ldo_reg>; > > + DBVDD-supply = <&mt6359_vio18_ldo_reg>; > > + LDO1-IN-supply = <&mt6359_vio18_ldo_reg>; > > + MICVDD-supply = <&pp3300_s3>; > > + realtek,jd-src = <1>; > > + }; > > +}; > > + > > +&i2c2 { > > + status = "disabled"; > > ...and if there's no i2c2, just don't add it to -geralt.dtsi in the first place... > but I believe that you just wanted to avoid probing the device that you declared > in -geralt.dtsi on this bus, so you can either remove the nodes for all of the > unused i2c busses in your board designs, or you can keep them but remove all of > the devices that aren't actually there. > > It's your choice in the end, but disabling this here doesn't make much sense imo. I think I'll follow the latter to describe the pinctrl and clock frequency of the i2c buses, so future board DT (if any) can just focus on declaring the devices on the bus. > > > +}; > > + > > +&i2c_tunnel { > > + /delete-node/ sbs-battery@b; > > Since nothing ever uses sbs-battery@b, just remove sbs-battery@b entirely > from -geralt.dtsi instead of deleting it here. > > Non-Ciri boards, if any, will define the @b one if necessary in their own > dts/dtsi file(s). > > > + > > + battery: sbs-battery@f { > > + compatible = "sbs,sbs-battery"; > > + reg = <0xf>; > > + sbs,i2c-retry-count = <2>; > > + sbs,poll-retry-count = <1>; > > + }; > > +}; > > + > > +&max98390_38 { > > + sound-name-prefix = "Front Right"; > > Move to -geralt.dtsi > > > +}; > > + > > +&max98390_39 { > > + sound-name-prefix = "Front Left"; > > ditto > > > +}; > > + > > +&mipi_tx_config0 { > > + drive-strength-microamp = <5200>; > > +}; > > + > > +&mmc1 { > > + status = "disabled"; > > Why are you configuring mmc1 in -geralt.dtsi if no board uses it at all? This is for the SD cards, though Ciri didn't adopt that in the end... I'll remove it. > > > +}; > > + > > +&mt6359_vm18_ldo_reg { > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1900000>; > > + regulator-microvolt-offset = <100000>; > > +}; > > + > > +&sound { > > + compatible = "mediatek,mt8188-rt5682s"; > > + model = "mt8188_m98390_5682"; > > + > > + 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", > > + "IN1P", "Headset Mic", > > + "Left Spk", "Front Left BE_OUT", > > + "Right Spk", "Front Right BE_OUT"; > > Please move compatible, model and audio-routing to SKU0 (and also copy that to > SKU3): as you're continuously overriding it in all other SKUs, having it here > can only confuse people... I agree. I'll update it. > > > + status = "okay"; > > + > > + dai-link-0 { > > + dai-format = "i2s"; > > ...and move dai-format to -geralt.dtsi. > > > + }; > > + > > + dai-link-1 { > > + dai-format = "i2s"; > > + codec { > > + sound-dai = <&max98390_38>, > > + <&max98390_39>; > > + }; > > + }; > > + > > + dai-link-2 { > > + codec { > > + sound-dai = <&rt5682s 0>; > > + }; > > + }; > > + > > + dai-link-3 { > > + codec { > > + sound-dai = <&rt5682s 0>; > > + }; > > + }; > > +}; > > + > > +&spi1 { > > + pinctrl-names = "default", "sleep"; > > + pinctrl-0 = <&spi1_pins_default>; > > + pinctrl-1 = <&spi1_pins_sleep>; > > Also move this to -geralt.dtsi, it's even the same pins...! > P.S.: Of course, move the spi1_pins_sleep to -geralt.dtsi as well. > > > + num-cs = <1>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > ...address and size cells, but no children nodes? Oh because the Himax touchscreen was removed in v2... I'll drop them. > > > +}; > > + > > +&pio { > > ..snip.. > > > + > > + touchscreen_rst: touchscreen-rst-pins { > > + pins-tchscr-rst { > > + pinmux = <PINMUX_GPIO5__FUNC_B_GPIO5>; > > + output-high; > > + }; > > + }; > > touchscreen_rst is unused - either use it or remove it. Will do. > > > + > > + spi1_pins_default: spi1-default-pins { > > + pins-bus { > > + pinmux = <PINMUX_GPIO75__FUNC_O_SPIM1_CSB>, > > + <PINMUX_GPIO76__FUNC_O_SPIM1_CLK>, > > + <PINMUX_GPIO77__FUNC_B0_SPIM1_MOSI>, > > + <PINMUX_GPIO78__FUNC_B0_SPIM1_MISO>; > > + bias-disable; > > + drive-strength = <10>; > > + }; > > + }; > > + > > + spi1_pins_sleep: spi1-sleep-pins { > > + pins-bus { > > + pinmux = <PINMUX_GPIO75__FUNC_B_GPIO75>, > > + <PINMUX_GPIO76__FUNC_B_GPIO76>, > > + <PINMUX_GPIO77__FUNC_B_GPIO77>, > > + <PINMUX_GPIO78__FUNC_B_GPIO78>; > > + bias-pull-down; > > + input-enable; > > + }; > > + }; > > +}; > > diff --git a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi > > new file mode 100644 > > index 000000000000..0d33ae82f0eb > > --- /dev/null > > +++ b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi > > @@ -0,0 +1,1492 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > > +/* > > + * Copyright (C) 2022 MediaTek Inc. > > + */ > > +/dts-v1/; > > +#include <dt-bindings/gpio/gpio.h> > > +#include "mt8188.dtsi" > > +#include "mt6359.dtsi" > > + > > +/ { > > + aliases { > > + i2c0 = &i2c0; > > + i2c1 = &i2c1; > > + i2c2 = &i2c2; > > + i2c3 = &i2c3; > > + i2c4 = &i2c4; > > + i2c5 = &i2c5; > > + i2c6 = &i2c6; > > + mmc0 = &mmc0; > > + mmc1 = &mmc1; > > + serial0 = &uart0; > > + }; > > + > > + backlight_lcd0: backlight-lcd0 { > > + compatible = "pwm-backlight"; > > + brightness-levels = <0 1023>; > > + default-brightness-level = <576>; > > + enable-gpios = <&pio 1 GPIO_ACTIVE_HIGH>; > > + num-interpolated-steps = <1023>; > > + power-supply = <&ppvar_sys>; > > + pwms = <&disp_pwm0 0 500000>; > > + }; > > + > > + chosen { > > + stdout-path = "serial0:115200n8"; > > + }; > > + > > + dmic-codec { > > + compatible = "dmic-codec"; > > + num-channels = <2>; > > + wakeup-delay-ms = <100>; > > + }; > > + > > + memory@40000000 { > > + device_type = "memory"; > > /* The size will be filled in by the bootloader */ > reg = <0 0x40000000 0 0>; Acknowledged. > > > > + reg = <0 0x40000000 0 0x80000000>; > > + }; > > + > > ..snip.. > > > + > > +&disp_dsi0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "okay"; > > + > > + dsi_panel: panel@0 { > > + compatible = "boe,tv110c9m-ll3"; > > Remove the compatible string, then add a comment (93 cols, it's ok in one line): > > /* Compatible string for different panels can be found in each device dts */ Acknowledged. > > > + reg = <0>; > > + enable-gpios = <&pio 25 GPIO_ACTIVE_HIGH>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&mipi_dsi_pins>; > > + > > + backlight = <&backlight_lcd0>; > > + avdd-supply = <&ppvar_mipi_disp_avdd>; > > + avee-supply = <&ppvar_mipi_disp_avee>; > > + pp1800-supply = <&mt6359_vm18_ldo_reg>; > > + rotation = <270>; > > + > > + status = "okay"; > > + > > + port { > > + dsi_panel_in: endpoint { > > + remote-endpoint = <&dsi_out>; > > + }; > > + }; > > + }; > > + > > + port { > > + dsi_out: endpoint { > > + remote-endpoint = <&dsi_panel_in>; > > + }; > > + }; > > +}; > > + > > +&disp_pwm0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&disp_pwm0_pins>; > > + status = "okay"; > > +}; > > + > > +&disp_pwm1 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&disp_pwm1_pins>; > > +}; > > + > > +&dp_intf0 { > > + port { > > + dp_intf0_out: endpoint { > > + remote-endpoint = <&edp_in>; > > + }; > > + }; > > +}; > > + > > +&dp_intf1 { > > + status = "okay"; > > + > > + port { > > + dp_intf1_out: endpoint { > > + remote-endpoint = <&dptx_in>; > > + }; > > + }; > > +}; > > + > > +&dp_tx { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&dp_tx_hpd>; > > + #sound-dai-cells = <0>; > > + status = "okay"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + dptx_in: endpoint { > > + remote-endpoint = <&dp_intf1_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + dptx_out: endpoint { > > + data-lanes = <0 1 2 3>; > > + }; > > + }; > > + }; > > +}; > > + > > +&edp_tx { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&edp_tx_hpd>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + edp_in: endpoint { > > + remote-endpoint = <&dp_intf0_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + edp_out: endpoint { > > + data-lanes = <0 1 2 3>; > > + remote-endpoint = <&edp_panel_in>; > > + }; > > + }; > > + }; > > + > > + /* > > + * Geralt also supports eDP OLED panels, which control panel > > + * brightness via the AUX channel and don't require PWM pin > > + * control. > > + * This is an auxiliary panel path for hardware layout > > + * validation and demonstration, so it's disabled by default. > > + * Boards adopting MIPI-DSI panels may not have this path. > > + **/ > > There's no board using this: please remove edp_tx entirely, as this is > only adding lines to this device tree for no useful reason. > > If a board with that appears, you can reintroduce this here, or if it is > a single board, you can add that in the board dts. Acknowledged. > > > + aux-bus { > > + edp_panel: panel { > > + compatible = "lg,lp120up1"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&edp_bl_en>; > > + power-supply = <&pp3300_edp_disp>; > > + status = "disabled"; > > + > > + port { > > + edp_panel_in: endpoint { > > + remote-endpoint = <&edp_out>; > > + }; > > + }; > > + }; > > + }; > > +}; > > + > > ..snip.. > > + edp_bl_en: edp-bl-en-pins { > > + pins-ap-disp-bklten { > > + pinmux = <PINMUX_GPIO1__FUNC_B_GPIO1>; > > + output-low; > > + }; > > + }; > > + > > + edp_disp_en: edp-disp-en-pins { > > + pins-en-pp3300-edp-disp { > > + pinmux = <PINMUX_GPIO27__FUNC_B_GPIO27>; > > + output-low; > > + }; > > + }; > > + > > + edp_tx_hpd: edp-tx-hpd-pins { > > + pins-dp-tx-hpd { > > + pinmux = <PINMUX_GPIO17__FUNC_I0_EDP_TX_HPD>; > > + }; > > + }; > > After removing the always disabled edp nodes, you can also remove these pins > as they are then unused. Will do. > > > + > > + gsc_int: gsc-int-pins { > > + pins-gsc-ap-int-odl { > > + pinmux = <PINMUX_GPIO0__FUNC_B_GPIO0>; > > + input-enable; > > + }; > > + }; > > + > > ..snip.. > > > +&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. > > > + 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. Regards, Fei > > > + mediatek,adsp = <&adsp>; > > + > > + status = "okay"; > > + > > + dai-link-0 { > > + link-name = "ETDM1_IN_BE"; > > + dai-format = "dsp_b"; > > + mediatek,clk-provider = "cpu"; > > + }; > > + > > + dai-link-1 { > > + link-name = "ETDM1_OUT_BE"; > > + dai-format = "dsp_b"; > > + mediatek,clk-provider = "cpu"; > > + > > + codec { > > + sound-dai = <&max98390_38>, > > + <&max98390_39>, > > + <&max98390_3a>, > > + <&max98390_3b>; > > + }; > > + }; > > + > > + dai-link-2 { > > + link-name = "ETDM2_IN_BE"; > > + mediatek,clk-provider = "cpu"; > > + > > + codec { > > + sound-dai = <&nau8825>; > > + }; > > + }; > > + > > + dai-link-3 { > > + link-name = "ETDM2_OUT_BE"; > > + mediatek,clk-provider = "cpu"; > > + > > + codec { > > + sound-dai = <&nau8825>; > > + }; > > + }; > > + > > + dai-link-4 { > > + link-name = "DPTX_BE"; > > + > > + codec { > > + sound-dai = <&dp_tx>; > > + }; > > + }; > > +}; > > + > > Cheers, > Angelo >