Hi Krzysztof,
On Wed, 13 Mar 2024 at 18:30, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 13/03/2024 13:30, Sumit Garg wrote:
> > Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > Box Core board based on the Qualcomm APQ8016E SoC.
> >
>
> ...
>
> > +
> > +/ {
> > + model = "Schneider Electric HMIBSC Board";
> > + compatible = "schneider,apq8016-hmibsc", "qcom,apq8016";
> > +
> > + aliases {
> > + mmc0 = &sdhc_1; /* eMMC */
> > + mmc1 = &sdhc_2; /* SD card */
> > + serial0 = &blsp_uart1;
> > + serial1 = &blsp_uart2;
> > + usid0 = &pm8916_0;
> > + i2c1 = &blsp_i2c6;
> > + i2c3 = &blsp_i2c4;
> > + i2c4 = &blsp_i2c3;
>
> The aliases should match schematics of the board, so I assume missing
> i2c2 is intentional, right?
Yeah that is intentional as per board schematics.
>
> > + spi0 = &blsp_spi5;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0";
> > + };
> > +
> > + memory@80000000 {
> > + reg = <0 0x80000000 0 0x40000000>;
> > + };
> > +
> > + reserved-memory {
> > + ramoops@bff00000 {
> > + compatible = "ramoops";
> > + reg = <0x0 0xbff00000 0x0 0x100000>;
> > +
> > + record-size = <0x20000>;
> > + console-size = <0x20000>;
> > + ftrace-size = <0x20000>;
> > + };
> > + };
> > +
> > + usb2513 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> e.g. usb-hub
>
Okay, I will rename it to usb-hub.
>
>
> > + compatible = "smsc,usb3503";
> > + reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> > + initial-mode = <1>;
> > + };
> > +
> > + usb_id: usb-id {
> > + compatible = "linux,extcon-usb-gpio";
> > + id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&usb_id_default>;
> > + };
> > +
> > + hdmi-out {
> > + compatible = "hdmi-connector";
> > + type = "a";
> > +
> > + port {
> > + hdmi_con: endpoint {
> > + remote-endpoint = <&adv7533_out>;
> > + };
> > + };
> > + };
> > +
> > + gpio-keys {
> > + compatible = "gpio-keys";
> > + autorepeat;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&msm_key_volp_n_default>;
> > +
> > + button {
> > + label = "Volume Up";
> > + linux,code = <KEY_VOLUMEUP>;
> > + gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
> > + };
> > + };
> > +
> > + leds {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pm8916_mpps_leds>;
>
> First property is always compatible. Please apply DTS coding style rules.
Ack.
>
> > +
> > + compatible = "gpio-leds";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> That's not a bus.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
I assumed earlier that W=1 is sufficient for DT schema checks but it
looks like those are two different entities. However, I added these
address and size cells properties only to get rid of warnings reported
by W=1, see below:
$ make qcom/apq8016-schneider-hmibsc.dtb W=1
DTC arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
no reg or ranges property
arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
no reg or ranges property
<snip>
So it looks like W=1 is reporting false warnings and we should rather
rely on dtbs_check only.
>
> > +
> > + led@5 {
> > + reg = <5>;
> > + label = "apq8016-hmibsc:green:wlan";
> > + function = LED_FUNCTION_WLAN;
> > + color = <LED_COLOR_ID_YELLOW>;
> > + gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
> > + linux,default-trigger = "phy0tx";
> > + default-state = "off";
> > + };
> > +
> > + led@6 {
> > + reg = <6>;
> > + label = "apq8016-hmibsc:yellow:bt";
> > + function = LED_FUNCTION_BLUETOOTH;
> > + color = <LED_COLOR_ID_BLUE>;
> > + gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>;
> > + linux,default-trigger = "bluetooth-power";
> > + default-state = "off";
> > + };
> > + };
> > +};
> > +
> > +&blsp_i2c3 {
> > + status = "okay";
> > +
> > + eeprom@50 {
> > + compatible = "atmel,24c32";
> > + reg = <0x50>;
> > + };
> > +};
> > +
> > +&blsp_i2c4 {
> > + status = "okay";
> > +
> > + adv_bridge: bridge@39 {
> > + status = "okay";
>
> Why do you need it? Was it disabled?
>
> And why this is before compatible? If this stays, please use DTS coding
> style rules for placement.
Okay I will remove it.
>
> > +
> > + compatible = "adi,adv7533";
> > + reg = <0x39>;
> > +
> > + interrupt-parent = <&tlmm>;
> > + interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> > +
> > + adi,dsi-lanes = <4>;
> > + clocks = <&rpmcc RPM_SMD_BB_CLK2>;
> > + clock-names = "cec";
> > +
> > + pd-gpios = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> > +
> > + avdd-supply = <&pm8916_l6>;
> > + a2vdd-supply = <&pm8916_l6>;
> > + dvdd-supply = <&pm8916_l6>;
> > + pvdd-supply = <&pm8916_l6>;
> > + v1p2-supply = <&pm8916_l6>;
> > + v3p3-supply = <&pm8916_l17>;
> > +
> > + pinctrl-names = "default","sleep";
> > + pinctrl-0 = <&adv7533_int_active &adv7533_switch_active>;
> > + pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>;
> > + #sound-dai-cells = <1>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + adv7533_in: endpoint {
> > + remote-endpoint = <&mdss_dsi0_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + adv7533_out: endpoint {
> > + remote-endpoint = <&hdmi_con>;
> > + };
> > + };
> > + };
> > + };
> > +};
> > +
> > +&blsp_i2c6 {
> > + status = "okay";
> > +
> > + rtc@30 {
> > + compatible = "sii,s35390a";
> > + reg = <0x30>;
> > + };
> > +
> > + eeprom@50 {
> > + compatible = "atmel,24c256";
> > + reg = <0x50>;
> > + };
> > +};
> > +
> > +&blsp_spi5 {
> > + status = "okay";
> > + cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>;
> > +
> > + tpm@0 {
> > + compatible = "tcg,tpm_tis-spi";
> > + reg = <0>;
> > + spi-max-frequency = <500000>;
> > + };
> > +};
> > +
> > +&blsp_uart1 {
> > + status = "okay";
> > + label = "UART0";
> > +};
> > +
> > +&blsp_uart2 {
> > + status = "okay";
> > + label = "UART1";
> > +};
> > +
> > +&lpass {
> > + status = "okay";
> > +};
> > +
> > +&mdss {
> > + status = "okay";
> > +};
> > +
> > +&mdss_dsi0_out {
> > + data-lanes = <0 1 2 3>;
> > + remote-endpoint = <&adv7533_in>;
> > +};
> > +
> > +&pm8916_codec {
> > + status = "okay";
> > + qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
> > + qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
> > +};
> > +
> > +&pm8916_resin {
> > + status = "okay";
> > + linux,code = <KEY_POWER>;
> > +};
> > +
> > +&pm8916_rpm_regulators {
> > + pm8916_l17: l17 {
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + };
> > +};
> > +
> > +&sdhc_1 {
> > + status = "okay";
> > +};
> > +
> > +&sdhc_2 {
> > + status = "okay";
> > +
> > + pinctrl-names = "default", "sleep";
> > + pinctrl-0 = <&sdc2_default &sdc2_cd_default>;
> > + pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>;
> > +
> > + cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
> > +};
> > +
> > +&sound {
> > + status = "okay";
>
> Is thi sneeded?
Yeah it is disabled by default.
>
> > +
> > + pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>;
> > + pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>;
> > + pinctrl-names = "default", "sleep";
> > + model = "DB410c";
> > + audio-routing =
> > + "AMIC2", "MIC BIAS Internal2",
> > + "AMIC3", "MIC BIAS External1";
> > +
> > + quaternary-dai-link {
> > + link-name = "ADV7533";
> > + cpu {
> > + sound-dai = <&lpass MI2S_QUATERNARY>;
> > + };
> > + codec {
> > + sound-dai = <&adv_bridge 0>;
> > + };
> > + };
> > +
> > + primary-dai-link {
> > + link-name = "WCD";
> > + cpu {
> > + sound-dai = <&lpass MI2S_PRIMARY>;
> > + };
> > + codec {
> > + sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>;
> > + };
> > + };
> > +
> > + tertiary-dai-link {
> > + link-name = "WCD-Capture";
> > + cpu {
> > + sound-dai = <&lpass MI2S_TERTIARY>;
> > + };
> > + codec {
> > + sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>;
> > + };
> > + };
> > +};
> > +
> > +&usb {
> > + status = "okay";
> > + extcon = <&usb_id>, <&usb_id>;
> > +
> > + pinctrl-names = "default", "device";
> > + pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
> > + pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>;
> > +};
> > +
> > +&usb_hs_phy {
> > + extcon = <&usb_id>;
> > +};
> > +
> > +&wcnss {
> > + status = "okay";
> > + firmware-name = "qcom/apq8016/wcnss.mbn";
> > +};
> > +
> > +&wcnss_ctrl {
> > + firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin";
> > +};
> > +
> > +&wcnss_iris {
> > + compatible = "qcom,wcn3620";
> > +};
> > +
> > +&wcnss_mem {
> > + status = "okay";
> > +};
> > +
> > +/* Enable CoreSight */
> > +&cti0 { status = "okay"; };
> > +&cti1 { status = "okay"; };
> > +&cti12 { status = "okay"; };
> > +&cti13 { status = "okay"; };
> > +&cti14 { status = "okay"; };
> > +&cti15 { status = "okay"; };
> > +&debug0 { status = "okay"; };
> > +&debug1 { status = "okay"; };
> > +&debug2 { status = "okay"; };
> > +&debug3 { status = "okay"; };
> > +&etf { status = "okay"; };
> > +&etm0 { status = "okay"; };
> > +&etm1 { status = "okay"; };
> > +&etm2 { status = "okay"; };
> > +&etm3 { status = "okay"; };
> > +&etr { status = "okay"; };
> > +&funnel0 { status = "okay"; };
> > +&funnel1 { status = "okay"; };
> > +&replicator { status = "okay"; };
> > +&stm { status = "okay"; };
> > +&tpiu { status = "okay"; };
> > +
> > +/*
> > + * 2mA drive strength is not enough when connecting multiple
> > + * I2C devices with different pull up resistors.
> > + */
> > +
> > +&blsp_i2c4_default {
>
> None of your overrides look like have proper alphabetical order. Please
> use alphabetical order.
>
Although these are already following the same order as
apq8016-sbc.dts, would you like the two DTS files based on the same
SoC to follow different orders?
-Sumit
>
>
> Best regards,
> Krzysztof
>
