| Message ID | 20220830133300.1.I7dd7a79c4cc5fe91c3feb004473feb3b34b7b2d8@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add a new board device tree named 'evoker' for herobrine variant. | expand |
On 30/08/2022 08:33, Sheng-Liang Pan wrote: > Add a basic device tree for the herobrine evoker board. > > Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@quanta.corp-partner.google.com> > --- > > arch/arm64/boot/dts/qcom/Makefile | 1 + > .../dts/qcom/sc7280-herobrine-evoker-r0.dts | 333 ++++++++++++++++++ > 2 files changed, 334 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > index 1d86a33de528c..59c22ba54a366 100644 > --- a/arch/arm64/boot/dts/qcom/Makefile > +++ b/arch/arm64/boot/dts/qcom/Makefile > @@ -103,6 +103,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1-lte.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-crd.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-herobrine-r1.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-villager-r0.dtb > +dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-evoker-r0.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp2.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-crd-r3.dtb > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts > new file mode 100644 > index 0000000000000..ccbe50b6249ab > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts > @@ -0,0 +1,333 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Google Evoker board device tree source > + * > + * Copyright 2022 Google LLC. > + */ > + > +/dts-v1/; > + > +#include "sc7280-herobrine.dtsi" > + > +/ { > + model = "Google Evoker"; > + compatible = "google,evoker", "qcom,sc7280"; Undocumented compatible. Please run scripts/checkpatch.pl and fix reported warnings. Does not look like you tested the bindings. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > +}; > + > +/* > + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES What does it mean and why it's SCREAMING? > + * > + * Sort order matches the order in the parent files (parents before children). Why? Sorting should be rather alphabetical. Best regards, Krzysztof
On 30/08/2022 08:33, Sheng-Liang Pan wrote: > Add a basic device tree for the herobrine evoker board. > > Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@quanta.corp-partner.google.com> > --- > > arch/arm64/boot/dts/qcom/Makefile | 1 + > .../dts/qcom/sc7280-herobrine-evoker-r0.dts | 333 ++++++++++++++++++ > 2 files changed, 334 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > index 1d86a33de528c..59c22ba54a366 100644 > --- a/arch/arm64/boot/dts/qcom/Makefile > +++ b/arch/arm64/boot/dts/qcom/Makefile > @@ -103,6 +103,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1-lte.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-crd.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-herobrine-r1.dtb > dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-villager-r0.dtb > +dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-evoker-r0.dtb Why breaking ordering? Best regards, Krzysztof
Hi, On Tue, Aug 30, 2022 at 2:33 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > +}; > > + > > +/* > > + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES > > What does it mean and why it's SCREAMING? > > + * > > + * Sort order matches the order in the parent files (parents before children). > > Why? Sorting should be rather alphabetical. We've had this discussion on the lists in the past. See, for instance: https://lore.kernel.org/r/CAD=FV=U2C1W+JHWyGRfyRB=WiPKLYvtjO90UDoJ9p+Xwe09+ow@mail.gmail.com/ The tl;dr summary is: * Going with exclusively alphabetical sort ordering tends to make things more confusing when there's a mix of things that are overridden vs. things that are fully defined from scratch. * If we accept that some ordering isn't alphabetical then it makes sense to have some sort of section headers to define when sort ordering changes. Otherwise people are constantly confused and putting things in the wrong place. Certainly there are lots of styles of section headers, some of which use all caps. * What's in this patch with regards to section headers and sort orders matches other herobrine-related files. If you want to propose a new way to organize files then perhaps you can submit a patch to do so and we can evaluate whether it's cleaner or not. If you want to propose a different organization, perhaps propose it on sc7180-trogdor files so we can see how it looks on a fully fleshed out family. That will help us evaluate whether it's easier or harder to understand. -Doug
On 30/08/2022 19:10, Doug Anderson wrote: > Hi, > > On Tue, Aug 30, 2022 at 2:33 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >>> +}; >>> + >>> +/* >>> + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES >> >> What does it mean and why it's SCREAMING? >>> + * >>> + * Sort order matches the order in the parent files (parents before children). >> >> Why? Sorting should be rather alphabetical. > > We've had this discussion on the lists in the past. See, for instance: > > https://lore.kernel.org/r/CAD=FV=U2C1W+JHWyGRfyRB=WiPKLYvtjO90UDoJ9p+Xwe09+ow@mail.gmail.com/ Good explanation, such sorting rule is quite nice. The part about regulators is still a bit confusing, I guess it is about some other pieces in some other place? But isn't this kind of obvious from including other DTSI? Best regards, Krzysztof
Hi, On Tue, Aug 30, 2022 at 9:50 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 30/08/2022 19:10, Doug Anderson wrote: > > Hi, > > > > On Tue, Aug 30, 2022 at 2:33 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >>> +}; > >>> + > >>> +/* > >>> + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES > >> > >> What does it mean and why it's SCREAMING? > >>> + * > >>> + * Sort order matches the order in the parent files (parents before children). > >> > >> Why? Sorting should be rather alphabetical. > > > > We've had this discussion on the lists in the past. See, for instance: > > > > https://lore.kernel.org/r/CAD=FV=U2C1W+JHWyGRfyRB=WiPKLYvtjO90UDoJ9p+Xwe09+ow@mail.gmail.com/ > > Good explanation, such sorting rule is quite nice. The part about > regulators is still a bit confusing, I guess it is about some other > pieces in some other place? Yeah, we originally started with the regulator sorting of "parents above children" long ago when it helped avoid some cases of -EPROBE_DEFER in Linux. The -EPROBE_DEFER isn't a reason these days, but when I looked back at it I decided that I quite liked "parents above children" and that it matched my mental model. Specifically, take a look at "/sys/kernel/debug/regulator/regulator_summary". Parent regulators are listed above child regulators because it makes the most sense to think of the regulator tree. Obviously we can only do this in the dts for regulators that are separate nodes and not ones provided by a big PMIC, but we often end up with quite a few of those in the end. In "child" device trees that are overriding a single regulator (like evoker) the comment is a bit nonsensical, of course. I'd be OK with removing the "Sort order matches the order in the parent files (parents before children)." in the evoker device tree since there's really only one regulator in this section. The only downside would be that when someone adds that second regulator then they might not know the sort ordering. ...so I would be fine keeping it too... > But isn't this kind of obvious from > including other DTSI? Isn't what kind of obvious from including the other DTSI? That the sort order should match the sort order of the parent for this section? It wasn't obvious to me. Since there are usually just a few regulators that referenced like this it seemed like it might be easiest to just alphabetize them in the child device trees. ...but I settled on thinking that matching the parent was marginally better. Since I debated it myself I decided it was probably better to comment so others understood the sort order... -Doug
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 1d86a33de528c..59c22ba54a366 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -103,6 +103,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1-lte.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-crd.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-herobrine-r1.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-villager-r0.dtb +dtb-$(CONFIG_ARCH_QCOM) += sc7280-herobrine-evoker-r0.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp2.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7280-crd-r3.dtb diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts new file mode 100644 index 0000000000000..ccbe50b6249ab --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts @@ -0,0 +1,333 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Google Evoker board device tree source + * + * Copyright 2022 Google LLC. + */ + +/dts-v1/; + +#include "sc7280-herobrine.dtsi" + +/ { + model = "Google Evoker"; + compatible = "google,evoker", "qcom,sc7280"; +}; + +/* + * ADDITIONS TO FIXED REGULATORS DEFINED IN PARENT DEVICE TREE FILES + * + * Sort order matches the order in the parent files (parents before children). + */ + +&pp3300_codec { + status = "okay"; +}; + +/* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */ + +ap_tp_i2c: &i2c0 { + status = "okay"; + clock-frequency = <400000>; + + trackpad: trackpad@2c { + compatible = "hid-over-i2c"; + reg = <0x2c>; + pinctrl-names = "default"; + pinctrl-0 = <&tp_int_odl>; + + interrupt-parent = <&tlmm>; + interrupts = <7 IRQ_TYPE_EDGE_FALLING>; + + hid-descr-addr = <0x20>; + vcc-supply = <&pp3300_z1>; + + wakeup-source; + }; +}; + +ts_i2c: &i2c13 { + status = "okay"; + clock-frequency = <400000>; + + ap_ts: touchscreen@10 { + compatible = "elan,ekth6915"; + reg = <0x10>; + pinctrl-names = "default"; + pinctrl-0 = <&ts_int_conn>, <&ts_rst_conn>; + + interrupt-parent = <&tlmm>; + interrupts = <55 IRQ_TYPE_LEVEL_LOW>; + + reset-gpios = <&tlmm 54 GPIO_ACTIVE_LOW>; + + vcc33-supply = <&ts_avdd>; + }; +}; + +&ap_sar_sensor_i2c { + status = "okay"; +}; + +&ap_sar_sensor0 { + status = "okay"; +}; + +&ap_sar_sensor1 { + status = "okay"; +}; + +&mdss_edp { + status = "okay"; +}; + +&mdss_edp_phy { + status = "okay"; +}; + +/* For nvme */ +&pcie1 { + status = "okay"; +}; + +/* For nvme */ +&pcie1_phy { + status = "okay"; +}; + +&pwmleds { + status = "okay"; +}; + +/* For eMMC */ +&sdhc_1 { + status = "okay"; +}; + +/* PINCTRL - ADDITIONS TO NODES IN PARENT DEVICE TREE FILES */ + +&ts_rst_conn { + bias-disable; +}; + +/* PINCTRL - BOARD-SPECIFIC */ + +/* + * Methodology for gpio-line-names: + * - If a pin goes to herobrine board and is named it gets that name. + * - If a pin goes to herobrine board and is not named, it gets no name. + * - If a pin is totally internal to Qcard then it gets Qcard name. + * - If a pin is not hooked up on Qcard, it gets no name. + */ + +&pm8350c_gpios { + gpio-line-names = "FLASH_STROBE_1", /* 1 */ + "AP_SUSPEND", + "PM8008_1_RST_N", + "", + "", + "", + "PMIC_EDP_BL_EN", + "PMIC_EDP_BL_PWM", + ""; +}; + +&tlmm { + gpio-line-names = "AP_TP_I2C_SDA", /* 0 */ + "AP_TP_I2C_SCL", + "SSD_RST_L", + "PE_WAKE_ODL", + "AP_SAR_SDA", + "AP_SAR_SCL", + "PRB_SC_GPIO_6", + "TP_INT_ODL", + "HP_I2C_SDA", + "HP_I2C_SCL", + + "GNSS_L1_EN", /* 10 */ + "GNSS_L5_EN", + "SPI_AP_MOSI", + "SPI_AP_MISO", + "SPI_AP_CLK", + "SPI_AP_CS0_L", + /* + * AP_FLASH_WP is crossystem ABI. Schematics + * call it BIOS_FLASH_WP_OD. + */ + "AP_FLASH_WP", + "", + "AP_EC_INT_L", + "", + + "UF_CAM_RST_L", /* 20 */ + "WF_CAM_RST_L", + "UART_AP_TX_DBG_RX", + "UART_DBG_TX_AP_RX", + "", + "PM8008_IRQ_1", + "HOST2WLAN_SOL", + "WLAN2HOST_SOL", + "MOS_BT_UART_CTS", + "MOS_BT_UART_RFR", + + "MOS_BT_UART_TX", /* 30 */ + "MOS_BT_UART_RX", + "PRB_SC_GPIO_32", + "HUB_RST_L", + "", + "", + "AP_SPI_FP_MISO", + "AP_SPI_FP_MOSI", + "AP_SPI_FP_CLK", + "AP_SPI_FP_CS_L", + + "AP_EC_SPI_MISO", /* 40 */ + "AP_EC_SPI_MOSI", + "AP_EC_SPI_CLK", + "AP_EC_SPI_CS_L", + "LCM_RST_L", + "EARLY_EUD_N", + "", + "DP_HOT_PLUG_DET", + "IO_BRD_MLB_ID0", + "IO_BRD_MLB_ID1", + + "IO_BRD_MLB_ID2", /* 50 */ + "SSD_EN", + "TS_I2C_SDA_CONN", + "TS_I2C_CLK_CONN", + "TS_RST_CONN", + "TS_INT_CONN", + "AP_I2C_TPM_SDA", + "AP_I2C_TPM_SCL", + "PRB_SC_GPIO_58", + "PRB_SC_GPIO_59", + + "EDP_HOT_PLUG_DET_N", /* 60 */ + "FP_TO_AP_IRQ_L", + "", + "AMP_EN", + "CAM0_MCLK_GPIO_64", + "CAM1_MCLK_GPIO_65", + "WF_CAM_MCLK", + "PRB_SC_GPIO_67", + "FPMCU_BOOT0", + "UF_CAM_SDA", + + "UF_CAM_SCL", /* 70 */ + "", + "", + "WF_CAM_SDA", + "WF_CAM_SCL", + "", + "", + "EN_FP_RAILS", + "FP_RST_L", + "PCIE1_CLKREQ_ODL", + + "EN_PP3300_DX_EDP", /* 80 */ + "SC_GPIO_81", + "FORCED_USB_BOOT", + "WCD_RESET_N", + "MOS_WLAN_EN", + "MOS_BT_EN", + "MOS_SW_CTRL", + "MOS_PCIE0_RST", + "MOS_PCIE0_CLKREQ_N", + "MOS_PCIE0_WAKE_N", + + "MOS_LAA_AS_EN", /* 90 */ + "SD_CD_ODL", + "", + "", + "MOS_BT_WLAN_SLIMBUS_CLK", + "MOS_BT_WLAN_SLIMBUS_DAT0", + "HP_MCLK", + "HP_BCLK", + "HP_DOUT", + "HP_DIN", + + "HP_LRCLK", /* 100 */ + "HP_IRQ", + "", + "", + "GSC_AP_INT_ODL", + "EN_PP3300_CODEC", + "AMP_BCLK", + "AMP_DIN", + "AMP_LRCLK", + "UIM1_DATA_GPIO_109", + + "UIM1_CLK_GPIO_110", /* 110 */ + "UIM1_RESET_GPIO_111", + "PRB_SC_GPIO_112", + "UIM0_DATA", + "UIM0_CLK", + "UIM0_RST", + "UIM0_PRESENT_ODL", + "SDM_RFFE0_CLK", + "SDM_RFFE0_DATA", + "WF_CAM_EN", + + "FASTBOOT_SEL_0", /* 120 */ + "SC_GPIO_121", + "FASTBOOT_SEL_1", + "SC_GPIO_123", + "FASTBOOT_SEL_2", + "SM_RFFE4_CLK_GRFC_8", + "SM_RFFE4_DATA_GRFC_9", + "WLAN_COEX_UART1_RX", + "WLAN_COEX_UART1_TX", + "PRB_SC_GPIO_129", + + "LCM_ID0", /* 130 */ + "LCM_ID1", + "", + "SDR_QLINK_REQ", + "SDR_QLINK_EN", + "QLINK0_WMSS_RESET_N", + "SMR526_QLINK1_REQ", + "SMR526_QLINK1_EN", + "SMR526_QLINK1_WMSS_RESET_N", + "PRB_SC_GPIO_139", + + "SAR1_IRQ_ODL", /* 140 */ + "SAR0_IRQ_ODL", + "PRB_SC_GPIO_142", + "", + "WCD_SWR_TX_CLK", + "WCD_SWR_TX_DATA0", + "WCD_SWR_TX_DATA1", + "WCD_SWR_RX_CLK", + "WCD_SWR_RX_DATA0", + "WCD_SWR_RX_DATA1", + + "DMIC01_CLK", /* 150 */ + "DMIC01_DATA", + "DMIC23_CLK", + "DMIC23_DATA", + "", + "", + "EC_IN_RW_ODL", + "HUB_EN", + "WCD_SWR_TX_DATA2", + "", + + "", /* 160 */ + "", + "", + "", + "", + "", + "", + "", + "", + "", + + "", /* 170 */ + "MOS_BLE_UART_TX", + "MOS_BLE_UART_RX", + "", + "", + ""; +};
Add a basic device tree for the herobrine evoker board. Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@quanta.corp-partner.google.com> --- arch/arm64/boot/dts/qcom/Makefile | 1 + .../dts/qcom/sc7280-herobrine-evoker-r0.dts | 333 ++++++++++++++++++ 2 files changed, 334 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker-r0.dts