| Message ID | 20240315060707.471248-4-sumit.garg@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS | expand |
On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > Support for Schneider Electric HMIBSC. Features: > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > - 1GiB RAM > - 8GiB eMMC, SD slot > - WiFi and Bluetooth > - 2x Host, 1x Device USB port > - HDMI > - Discrete TPM2 chip over SPI > - USB ethernet adaptors (soldered) > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > arch/arm64/boot/dts/qcom/Makefile | 1 + > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > 2 files changed, 511 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > index 39889d5f8e12..ad55e52e950b 100644 > --- a/arch/arm64/boot/dts/qcom/Makefile > +++ b/arch/arm64/boot/dts/qcom/Makefile > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > new file mode 100644 > index 000000000000..9c79a31a04db > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > @@ -0,0 +1,510 @@ > [...] > +&blsp_uart1 { > + label = "UART0"; > + status = "okay"; > +}; > + > +&blsp_uart2 { > + label = "UART1"; > + 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"; }; Nitpick: The &cti0 is in the correct alphabetically ordered place, but &replicator, &stm and &tpiu are not. I know you changed this based on the review comments but I personally think it was clearer having this separated as condensed block towards the end of the file (where it was before). The other option would be to put each element individually at the correctly ordered position in the file. However, having a single "Enable CoreSight" comment for the entire block would then not work anymore since all the lines would be interspersed throughout the file. > [...] > +&pm8916_codec { > + qcom,mbhc-vthreshold-low = <75 150 237 450 500>; > + qcom,mbhc-vthreshold-high = <75 150 237 450 500>; > + status = "okay"; > +}; > + > +&pm8916_gpios { > + gpio-line-names = > + "USB_HUB_RESET_N_PM", > + "USB_SW_SEL_PM", > + "NC", > + "NC"; > + > + usb_hub_reset_pm: usb-hub-reset-pm-state { > + pins = "gpio1"; > + function = PMIC_GPIO_FUNC_NORMAL; > + > + input-disable; > + output-high; > + }; > + > + usb_hub_reset_pm_device: usb-hub-reset-pm-device-state { > + pins = "gpio1"; > + function = PMIC_GPIO_FUNC_NORMAL; > + > + output-low; > + }; > + > + usb_sw_sel_pm: usb-sw-sel-pm-state { > + pins = "gpio2"; > + function = PMIC_GPIO_FUNC_NORMAL; > + > + power-source = <PM8916_GPIO_VPH>; > + input-disable; > + output-high; > + }; > + > + usb_sw_sel_pm_device: usb-sw-sel-pm-device-state { > + pins = "gpio2"; > + function = PMIC_GPIO_FUNC_NORMAL; > + > + power-source = <PM8916_GPIO_VPH>; > + input-disable; > + output-low; > + }; > +}; > + > +&pm8916_mpps { > + gpio-line-names = > + "NC", > + "WLAN_LED_CTRL", > + "BT_LED_CTRL", > + "NC"; > + > + pm8916_mpps_leds: pm8916-mpps-state { > + pins = "mpp2", "mpp3"; > + function = "digital"; > + > + output-low; > + }; > +}; > + > +&pm8916_resin { > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; What is the goal of overriding the interrupt here? It looks like you are changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This sounds a bit like you want the driver to receive just button release events (or just press events, not sure about the polarity). I'm not sure if the driver will handle this correctly. > + linux,code = <KEY_POWER>; > + status = "okay"; > +}; > + > +&pm8916_rpm_regulators { > + pm8916_l17: l17 { > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + }; > +}; > + > +&sdhc_1 { > + status = "okay"; > +}; > + > +&sdhc_2 { > + pinctrl-0 = <&sdc2_default &sdc2_cd_default>; > + pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>; > + pinctrl-names = "default", "sleep"; > + > + cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>; > + status = "okay"; > +}; > + > +&sound { > + pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>; > + pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>; > + pinctrl-names = "default", "sleep"; > + model = "HMIBSC"; > + audio-routing = > + "AMIC2", "MIC BIAS Internal2", > + "AMIC3", "MIC BIAS External1"; > + status = "okay"; > + > + 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>; > + }; > + }; > +}; > + > +&tlmm { > + pinctrl-0 = <&uart1_mux0_rs232_high &uart1_mux1_rs232_low>; > + pinctrl-names = "default"; > + > + adv7533_int_active: adv533-int-active-state { > + pins = "gpio31"; > + function = "gpio"; > + > + drive-strength = <16>; > + bias-disable; > + }; > + > + adv7533_int_suspend: adv7533-int-suspend-state { > + pins = "gpio31"; > + function = "gpio"; > + > + drive-strength = <2>; > + bias-disable; > + }; > + > + adv7533_switch_active: adv7533-switch-active-state { > + pins = "gpio32"; > + function = "gpio"; > + > + drive-strength = <16>; > + bias-disable; > + }; > + > + adv7533_switch_suspend: adv7533-switch-suspend-state { > + pins = "gpio32"; > + function = "gpio"; > + > + drive-strength = <2>; > + bias-disable; > + }; > + > + msm_key_volp_n_default: msm-key-volp-n-default-state { > + pins = "gpio107"; > + function = "gpio"; > + > + drive-strength = <8>; > + bias-pull-up; > + }; > + > + sdc2_cd_default: sdc2-cd-default-state { > + pins = "gpio38"; > + function = "gpio"; > + drive-strength = <2>; > + bias-disable; > + }; Nitpick: It would look a bit cleaner to have the empty lines consistent in all pinctrl nodes, i.e. either always having an empty line between function and drive-strength or never. I think Konrad prefers the more compact version without empty line (sadly I'm not sure if this is clearly documented anywhere). Same for &pm8916_gpios and mpps. > + > + /* > + * UART1 being the debug console supports various modes of > + * operation (RS-232/485/422) controlled via GPIOs configured > + * mux as follows: > + * > + * gpio100 gpio99 UART mode > + * 0 0 loopback > + * 0 1 RS-232 > + * 1 0 RS-485 > + * 1 1 RS-422 > + * > + * The default mode configured here is RS-232 mode. > + */ :D Thanks a lot for making this clear using the table. And also for all the other cleanup changes! Stephan
On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > > > Support for Schneider Electric HMIBSC. Features: > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > - 1GiB RAM > > - 8GiB eMMC, SD slot > > - WiFi and Bluetooth > > - 2x Host, 1x Device USB port > > - HDMI > > - Discrete TPM2 chip over SPI > > - USB ethernet adaptors (soldered) > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/Makefile | 1 + > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > > 2 files changed, 511 insertions(+) > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > > index 39889d5f8e12..ad55e52e950b 100644 > > --- a/arch/arm64/boot/dts/qcom/Makefile > > +++ b/arch/arm64/boot/dts/qcom/Makefile > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > new file mode 100644 > > index 000000000000..9c79a31a04db > > --- /dev/null > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > @@ -0,0 +1,510 @@ > > [...] > > +&blsp_uart1 { > > + label = "UART0"; > > + status = "okay"; > > +}; > > + > > +&blsp_uart2 { > > + label = "UART1"; > > + 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"; }; > > Nitpick: The &cti0 is in the correct alphabetically ordered place, but > &replicator, &stm and &tpiu are not. > > I know you changed this based on the review comments but I personally > think it was clearer having this separated as condensed block towards > the end of the file (where it was before). > > The other option would be to put each element individually at the > correctly ordered position in the file. However, having a single "Enable > CoreSight" comment for the entire block would then not work anymore > since all the lines would be interspersed throughout the file. IMO, having it as a condensed block is a bit more clear such that people are able to locate overrides easily given their function. However, there aren't any guidelines for such block orders. So let me wait to hear back from platform maintainers if they would like anything to be changed here. > > > [...] > > +&pm8916_codec { > > + qcom,mbhc-vthreshold-low = <75 150 237 450 500>; > > + qcom,mbhc-vthreshold-high = <75 150 237 450 500>; > > + status = "okay"; > > +}; > > + > > +&pm8916_gpios { > > + gpio-line-names = > > + "USB_HUB_RESET_N_PM", > > + "USB_SW_SEL_PM", > > + "NC", > > + "NC"; > > + > > + usb_hub_reset_pm: usb-hub-reset-pm-state { > > + pins = "gpio1"; > > + function = PMIC_GPIO_FUNC_NORMAL; > > + > > + input-disable; > > + output-high; > > + }; > > + > > + usb_hub_reset_pm_device: usb-hub-reset-pm-device-state { > > + pins = "gpio1"; > > + function = PMIC_GPIO_FUNC_NORMAL; > > + > > + output-low; > > + }; > > + > > + usb_sw_sel_pm: usb-sw-sel-pm-state { > > + pins = "gpio2"; > > + function = PMIC_GPIO_FUNC_NORMAL; > > + > > + power-source = <PM8916_GPIO_VPH>; > > + input-disable; > > + output-high; > > + }; > > + > > + usb_sw_sel_pm_device: usb-sw-sel-pm-device-state { > > + pins = "gpio2"; > > + function = PMIC_GPIO_FUNC_NORMAL; > > + > > + power-source = <PM8916_GPIO_VPH>; > > + input-disable; > > + output-low; > > + }; > > +}; > > + > > +&pm8916_mpps { > > + gpio-line-names = > > + "NC", > > + "WLAN_LED_CTRL", > > + "BT_LED_CTRL", > > + "NC"; > > + > > + pm8916_mpps_leds: pm8916-mpps-state { > > + pins = "mpp2", "mpp3"; > > + function = "digital"; > > + > > + output-low; > > + }; > > +}; > > + > > +&pm8916_resin { > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; > > What is the goal of overriding the interrupt here? It looks like you are > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This > sounds a bit like you want the driver to receive just button release > events (or just press events, not sure about the polarity). I'm not sure > if the driver will handle this correctly. The use-case here is to just act upon button release events and the driver handles this appropriately. Final use-case of the reset button: - Short press and release leads to normal Linux reboot. - Long press for more than 10 sec or so leads to a hard reset. With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple press leads to Linux reboot. > > > + linux,code = <KEY_POWER>; > > + status = "okay"; > > +}; > > + > > +&pm8916_rpm_regulators { > > + pm8916_l17: l17 { > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + }; > > +}; > > + > > +&sdhc_1 { > > + status = "okay"; > > +}; > > + > > +&sdhc_2 { > > + pinctrl-0 = <&sdc2_default &sdc2_cd_default>; > > + pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>; > > + pinctrl-names = "default", "sleep"; > > + > > + cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>; > > + status = "okay"; > > +}; > > + > > +&sound { > > + pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>; > > + pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>; > > + pinctrl-names = "default", "sleep"; > > + model = "HMIBSC"; > > + audio-routing = > > + "AMIC2", "MIC BIAS Internal2", > > + "AMIC3", "MIC BIAS External1"; > > + status = "okay"; > > + > > + 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>; > > + }; > > + }; > > +}; > > + > > +&tlmm { > > + pinctrl-0 = <&uart1_mux0_rs232_high &uart1_mux1_rs232_low>; > > + pinctrl-names = "default"; > > + > > + adv7533_int_active: adv533-int-active-state { > > + pins = "gpio31"; > > + function = "gpio"; > > + > > + drive-strength = <16>; > > + bias-disable; > > + }; > > + > > + adv7533_int_suspend: adv7533-int-suspend-state { > > + pins = "gpio31"; > > + function = "gpio"; > > + > > + drive-strength = <2>; > > + bias-disable; > > + }; > > + > > + adv7533_switch_active: adv7533-switch-active-state { > > + pins = "gpio32"; > > + function = "gpio"; > > + > > + drive-strength = <16>; > > + bias-disable; > > + }; > > + > > + adv7533_switch_suspend: adv7533-switch-suspend-state { > > + pins = "gpio32"; > > + function = "gpio"; > > + > > + drive-strength = <2>; > > + bias-disable; > > + }; > > + > > + msm_key_volp_n_default: msm-key-volp-n-default-state { > > + pins = "gpio107"; > > + function = "gpio"; > > + > > + drive-strength = <8>; > > + bias-pull-up; > > + }; > > + > > + sdc2_cd_default: sdc2-cd-default-state { > > + pins = "gpio38"; > > + function = "gpio"; > > + drive-strength = <2>; > > + bias-disable; > > + }; > > Nitpick: It would look a bit cleaner to have the empty lines consistent > in all pinctrl nodes, i.e. either always having an empty line between > function and drive-strength or never. I think Konrad prefers the more > compact version without empty line (sadly I'm not sure if this is > clearly documented anywhere). Same for &pm8916_gpios and mpps. > Sure I can remove the empty lines to be consistent with msm8916.dtsi. > > + > > + /* > > + * UART1 being the debug console supports various modes of > > + * operation (RS-232/485/422) controlled via GPIOs configured > > + * mux as follows: > > + * > > + * gpio100 gpio99 UART mode > > + * 0 0 loopback > > + * 0 1 RS-232 > > + * 1 0 RS-485 > > + * 1 1 RS-422 > > + * > > + * The default mode configured here is RS-232 mode. > > + */ > > :D > > Thanks a lot for making this clear using the table. And also for all the > other cleanup changes! Thanks for your review. -Sumit > > Stephan
On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote: > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > > > > > Support for Schneider Electric HMIBSC. Features: > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > > - 1GiB RAM > > > - 8GiB eMMC, SD slot > > > - WiFi and Bluetooth > > > - 2x Host, 1x Device USB port > > > - HDMI > > > - Discrete TPM2 chip over SPI > > > - USB ethernet adaptors (soldered) > > > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > --- > > > arch/arm64/boot/dts/qcom/Makefile | 1 + > > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > > > 2 files changed, 511 insertions(+) > > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > > > index 39889d5f8e12..ad55e52e950b 100644 > > > --- a/arch/arm64/boot/dts/qcom/Makefile > > > +++ b/arch/arm64/boot/dts/qcom/Makefile > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > new file mode 100644 > > > index 000000000000..9c79a31a04db > > > --- /dev/null > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > @@ -0,0 +1,510 @@ > > > [...] > > > + > > > +&pm8916_resin { > > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; > > > + linux,code = <KEY_POWER>; > > > + status = "okay"; > > > +}; > > > > What is the goal of overriding the interrupt here? It looks like you are > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This > > sounds a bit like you want the driver to receive just button release > > events (or just press events, not sure about the polarity). I'm not sure > > if the driver will handle this correctly. > > The use-case here is to just act upon button release events and the > driver handles this appropriately. Final use-case of the reset button: > > - Short press and release leads to normal Linux reboot. > - Long press for more than 10 sec or so leads to a hard reset. > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple > press leads to Linux reboot. > Thanks for explaining your use case. Is the DT really the right place to describe this? In the hardware, this is just a button that provides both press and release events. Linux typically forwards these events to user space, without interpreting them in any way. This means you likely have some user space component that listens to the events (e.g. systemd logind). Ideally that component should be reconfigured to trigger the reboot on release instead of press. If you hardcode this behavior in the DT you are essentially describing that the hardware is incapable of detecting the press event before the release event. This is not the case, right? There may be use cases where someone would still want to detect the key press (rather than just release). Thanks, Stephan
On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote: > > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > > > > > > > Support for Schneider Electric HMIBSC. Features: > > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > > > - 1GiB RAM > > > > - 8GiB eMMC, SD slot > > > > - WiFi and Bluetooth > > > > - 2x Host, 1x Device USB port > > > > - HDMI > > > > - Discrete TPM2 chip over SPI > > > > - USB ethernet adaptors (soldered) > > > > > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > > --- > > > > arch/arm64/boot/dts/qcom/Makefile | 1 + > > > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > > > > 2 files changed, 511 insertions(+) > > > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > > > > index 39889d5f8e12..ad55e52e950b 100644 > > > > --- a/arch/arm64/boot/dts/qcom/Makefile > > > > +++ b/arch/arm64/boot/dts/qcom/Makefile > > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > > > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > new file mode 100644 > > > > index 000000000000..9c79a31a04db > > > > --- /dev/null > > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > @@ -0,0 +1,510 @@ > > > > [...] > > > > + > > > > +&pm8916_resin { > > > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; > > > > + linux,code = <KEY_POWER>; > > > > + status = "okay"; > > > > +}; > > > > > > What is the goal of overriding the interrupt here? It looks like you are > > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This > > > sounds a bit like you want the driver to receive just button release > > > events (or just press events, not sure about the polarity). I'm not sure > > > if the driver will handle this correctly. > > > > The use-case here is to just act upon button release events and the > > driver handles this appropriately. Final use-case of the reset button: > > > > - Short press and release leads to normal Linux reboot. > > - Long press for more than 10 sec or so leads to a hard reset. > > > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple > > press leads to Linux reboot. > > > > Thanks for explaining your use case. Is the DT really the right place to > describe this? In the hardware, this is just a button that provides both > press and release events. Linux typically forwards these events to user > space, without interpreting them in any way. This means you likely have > some user space component that listens to the events (e.g. systemd > logind). Ideally that component should be reconfigured to trigger the > reboot on release instead of press. I am not sure if that's really the case. I only see power key value to be reported as follows: input_report_key(pwrkey->input, pwrkey->code, 1); or input_report_key(pwrkey->input, pwrkey->code, 0); It's not like a press event being a rising edge (0->1) or a release event being a falling edge (1->0) reported. AFAICS, a reboot is issued whenever the value of power key is reported as "1". > > If you hardcode this behavior in the DT you are essentially describing > that the hardware is incapable of detecting the press event before the > release event. This is not the case, right? There may be use cases where > someone would still want to detect the key press (rather than just > release). As of now there isn't such a use-case for the HMIBSC board. -Sumit > > Thanks, > Stephan
On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote: > On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote: > > > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > > > > > > > > > Support for Schneider Electric HMIBSC. Features: > > > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > > > > - 1GiB RAM > > > > > - 8GiB eMMC, SD slot > > > > > - WiFi and Bluetooth > > > > > - 2x Host, 1x Device USB port > > > > > - HDMI > > > > > - Discrete TPM2 chip over SPI > > > > > - USB ethernet adaptors (soldered) > > > > > > > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > > > --- > > > > > arch/arm64/boot/dts/qcom/Makefile | 1 + > > > > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > > > > > 2 files changed, 511 insertions(+) > > > > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > > > > > index 39889d5f8e12..ad55e52e950b 100644 > > > > > --- a/arch/arm64/boot/dts/qcom/Makefile > > > > > +++ b/arch/arm64/boot/dts/qcom/Makefile > > > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > > > > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > > > > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > new file mode 100644 > > > > > index 000000000000..9c79a31a04db > > > > > --- /dev/null > > > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > @@ -0,0 +1,510 @@ > > > > > [...] > > > > > + > > > > > +&pm8916_resin { > > > > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; > > > > > + linux,code = <KEY_POWER>; > > > > > + status = "okay"; > > > > > +}; > > > > > > > > What is the goal of overriding the interrupt here? It looks like you are > > > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This > > > > sounds a bit like you want the driver to receive just button release > > > > events (or just press events, not sure about the polarity). I'm not sure > > > > if the driver will handle this correctly. > > > > > > The use-case here is to just act upon button release events and the > > > driver handles this appropriately. Final use-case of the reset button: > > > > > > - Short press and release leads to normal Linux reboot. > > > - Long press for more than 10 sec or so leads to a hard reset. > > > > > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple > > > press leads to Linux reboot. > > > > > > > Thanks for explaining your use case. Is the DT really the right place to > > describe this? In the hardware, this is just a button that provides both > > press and release events. Linux typically forwards these events to user > > space, without interpreting them in any way. This means you likely have > > some user space component that listens to the events (e.g. systemd > > logind). Ideally that component should be reconfigured to trigger the > > reboot on release instead of press. > > I am not sure if that's really the case. I only see power key value to > be reported as follows: > > input_report_key(pwrkey->input, pwrkey->code, 1); > or > input_report_key(pwrkey->input, pwrkey->code, 0); > > It's not like a press event being a rising edge (0->1) or a release > event being a falling edge (1->0) reported. AFAICS, a reboot is issued > whenever the value of power key is reported as "1". > If you look inside the input_report_key() function you can see that the input subsystem internally tracks the key state. input_get_disposition() returns INPUT_IGNORE_EVENT if the key bit already has the same value. Only when the key changes its state, an event is sent to user space. This means that all events reported to user space are effectively rising/falling edges (an event with value "1" is a rising edge 0->1, an event with value "0" is a falling edge 1->0). The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey - simulate missed key press events") [1]. No event is reported when you start pressing the key. When you release the key, you get a key press event (rising edge) immediately followed by a key release (falling edge). But the workaround was added to handle potentially missed interrupts, not to inhibit reporting key press events. In my opinion, if you want to perform an action on key release rather than key press then you should adjust the user space program to do so. From the point of view of the hardware DT (and even the kernel), the key press happens when you actually start pressing the key, and not later when you release it. Thanks, Stephan [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85
On Thu, 21 Mar 2024 at 16:03, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote: > > On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > > On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote: > > > > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. > > > > > > > > > > > > Support for Schneider Electric HMIBSC. Features: > > > > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > > > > > - 1GiB RAM > > > > > > - 8GiB eMMC, SD slot > > > > > > - WiFi and Bluetooth > > > > > > - 2x Host, 1x Device USB port > > > > > > - HDMI > > > > > > - Discrete TPM2 chip over SPI > > > > > > - USB ethernet adaptors (soldered) > > > > > > > > > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> > > > > > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > > > > --- > > > > > > arch/arm64/boot/dts/qcom/Makefile | 1 + > > > > > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ > > > > > > 2 files changed, 511 insertions(+) > > > > > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > > > > > > index 39889d5f8e12..ad55e52e950b 100644 > > > > > > --- a/arch/arm64/boot/dts/qcom/Makefile > > > > > > +++ b/arch/arm64/boot/dts/qcom/Makefile > > > > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo > > > > > > > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb > > > > > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb > > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb > > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > new file mode 100644 > > > > > > index 000000000000..9c79a31a04db > > > > > > --- /dev/null > > > > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts > > > > > > @@ -0,0 +1,510 @@ > > > > > > [...] > > > > > > + > > > > > > +&pm8916_resin { > > > > > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; > > > > > > + linux,code = <KEY_POWER>; > > > > > > + status = "okay"; > > > > > > +}; > > > > > > > > > > What is the goal of overriding the interrupt here? It looks like you are > > > > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This > > > > > sounds a bit like you want the driver to receive just button release > > > > > events (or just press events, not sure about the polarity). I'm not sure > > > > > if the driver will handle this correctly. > > > > > > > > The use-case here is to just act upon button release events and the > > > > driver handles this appropriately. Final use-case of the reset button: > > > > > > > > - Short press and release leads to normal Linux reboot. > > > > - Long press for more than 10 sec or so leads to a hard reset. > > > > > > > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple > > > > press leads to Linux reboot. > > > > > > > > > > Thanks for explaining your use case. Is the DT really the right place to > > > describe this? In the hardware, this is just a button that provides both > > > press and release events. Linux typically forwards these events to user > > > space, without interpreting them in any way. This means you likely have > > > some user space component that listens to the events (e.g. systemd > > > logind). Ideally that component should be reconfigured to trigger the > > > reboot on release instead of press. > > > > I am not sure if that's really the case. I only see power key value to > > be reported as follows: > > > > input_report_key(pwrkey->input, pwrkey->code, 1); > > or > > input_report_key(pwrkey->input, pwrkey->code, 0); > > > > It's not like a press event being a rising edge (0->1) or a release > > event being a falling edge (1->0) reported. AFAICS, a reboot is issued > > whenever the value of power key is reported as "1". > > > > If you look inside the input_report_key() function you can see that the > input subsystem internally tracks the key state. input_get_disposition() > returns INPUT_IGNORE_EVENT if the key bit already has the same value. > Only when the key changes its state, an event is sent to user space. > This means that all events reported to user space are effectively > rising/falling edges (an event with value "1" is a rising edge 0->1, an > event with value "0" is a falling edge 1->0). > > The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because > of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey - > simulate missed key press events") [1]. No event is reported when you > start pressing the key. When you release the key, you get a key press > event (rising edge) immediately followed by a key release (falling > edge). But the workaround was added to handle potentially missed > interrupts, not to inhibit reporting key press events. I rather see it differently being actually allowing the current use-case to support only the IRQ_TYPE_EDGE_FALLING. As per your description above, a falling edge can only be an event for user-space if we have: input_report_key(pwrkey->input, pwrkey->code, 1); input_report_key(pwrkey->input, pwrkey->code, 0); rather than only input_report_key(pwrkey->input, pwrkey->code, 0); which won't trigger any event for user-space, right? > > In my opinion, if you want to perform an action on key release rather > than key press then you should adjust the user space program to do so. > From the point of view of the hardware DT (and even the kernel), the key > press happens when you actually start pressing the key, and not later > when you release it. The reason why we are discussing it back and forth looks like due to lack of clarity as to how HMIBSC board supports this button. The common implementation as per db410c DTS is to have two buttons: - One button is representing pwrkey(KEY_POWER) which is solely consumed by the operating system to cleanly power off/restart the db410c. - Another button is representing pm8916_resin(KEY_VOLUMEDOWN) which apart from normal volume down is also consumed by PMIC hardware to perform a hard reset on a long press (if more than 10 sec.). However, on HMIBSC board we only have a single power/reset button which has to support the dual role of above two buttons on db410c: Only one button as pm8916_resin(KEY_POWER), behaviour required: - The rising edge (or button press) has to be only consumed by PMIC to perform a hard reset of the HMIBSC board if pressed for more than 10 secs. - The falling edge has to be consumed by the operating system to cleanly reboot the HMIBSC board. So what you are asking here is that the operating system has to be notified of the rising edge of the button even if it has to just ignore that without any action. Do you really think that would be acceptable as a generic solution for systemd logind? Also, why DT isn't the right place for correctly describing the intended hardware behaviour? Or am I missing any DT policy which says hardware has to be described the exact way it is rather than the expected way for hardware to behave? BTW, If you want the hardware behaviour to be properly commented then I can do that too. -Sumit > > Thanks, > Stephan > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85
Hi both, On 22/03/2024 06:29, Sumit Garg wrote: > On Thu, 21 Mar 2024 at 16:03, Stephan Gerhold <stephan@gerhold.net> wrote: >> >> On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote: >>> On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <stephan@gerhold.net> wrote: >>>> On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote: >>>>> On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@gerhold.net> wrote: >>>>>> On Fri, Mar 15, 2024 at 11:37:07AM +0530, 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. >>>>>>> >>>>>>> Support for Schneider Electric HMIBSC. Features: >>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) >>>>>>> - 1GiB RAM >>>>>>> - 8GiB eMMC, SD slot >>>>>>> - WiFi and Bluetooth >>>>>>> - 2x Host, 1x Device USB port >>>>>>> - HDMI >>>>>>> - Discrete TPM2 chip over SPI >>>>>>> - USB ethernet adaptors (soldered) >>>>>>> >>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org> >>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org> >>>>>>> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> >>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> >>>>>>> --- >>>>>>> arch/arm64/boot/dts/qcom/Makefile | 1 + >>>>>>> .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++ >>>>>>> 2 files changed, 511 insertions(+) >>>>>>> create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts >>>>>>> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile >>>>>>> index 39889d5f8e12..ad55e52e950b 100644 >>>>>>> --- a/arch/arm64/boot/dts/qcom/Makefile >>>>>>> +++ b/arch/arm64/boot/dts/qcom/Makefile >>>>>>> @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo >>>>>>> >>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb >>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb >>>>>>> +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb >>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb >>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb >>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts >>>>>>> new file mode 100644 >>>>>>> index 000000000000..9c79a31a04db >>>>>>> --- /dev/null >>>>>>> +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts >>>>>>> @@ -0,0 +1,510 @@ >>>>>>> [...] >>>>>>> + >>>>>>> +&pm8916_resin { >>>>>>> + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; >>>>>>> + linux,code = <KEY_POWER>; >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>> >>>>>> What is the goal of overriding the interrupt here? It looks like you are >>>>>> changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This >>>>>> sounds a bit like you want the driver to receive just button release >>>>>> events (or just press events, not sure about the polarity). I'm not sure >>>>>> if the driver will handle this correctly. >>>>> >>>>> The use-case here is to just act upon button release events and the >>>>> driver handles this appropriately. Final use-case of the reset button: >>>>> >>>>> - Short press and release leads to normal Linux reboot. >>>>> - Long press for more than 10 sec or so leads to a hard reset. >>>>> >>>>> With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple >>>>> press leads to Linux reboot. >>>>> >>>> >>>> Thanks for explaining your use case. Is the DT really the right place to >>>> describe this? In the hardware, this is just a button that provides both >>>> press and release events. Linux typically forwards these events to user >>>> space, without interpreting them in any way. This means you likely have >>>> some user space component that listens to the events (e.g. systemd >>>> logind). Ideally that component should be reconfigured to trigger the >>>> reboot on release instead of press. >>> >>> I am not sure if that's really the case. I only see power key value to >>> be reported as follows: >>> >>> input_report_key(pwrkey->input, pwrkey->code, 1); >>> or >>> input_report_key(pwrkey->input, pwrkey->code, 0); >>> >>> It's not like a press event being a rising edge (0->1) or a release >>> event being a falling edge (1->0) reported. AFAICS, a reboot is issued >>> whenever the value of power key is reported as "1". >>> >> >> If you look inside the input_report_key() function you can see that the >> input subsystem internally tracks the key state. input_get_disposition() >> returns INPUT_IGNORE_EVENT if the key bit already has the same value. >> Only when the key changes its state, an event is sent to user space. >> This means that all events reported to user space are effectively >> rising/falling edges (an event with value "1" is a rising edge 0->1, an >> event with value "0" is a falling edge 1->0). >> >> The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because >> of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey - >> simulate missed key press events") [1]. No event is reported when you >> start pressing the key. When you release the key, you get a key press >> event (rising edge) immediately followed by a key release (falling >> edge). But the workaround was added to handle potentially missed >> interrupts, not to inhibit reporting key press events. > > I rather see it differently being actually allowing the current > use-case to support only the IRQ_TYPE_EDGE_FALLING. As per your > description above, a falling edge can only be an event for user-space > if we have: > > input_report_key(pwrkey->input, pwrkey->code, 1); > input_report_key(pwrkey->input, pwrkey->code, 0); > > rather than only > > input_report_key(pwrkey->input, pwrkey->code, 0); > > which won't trigger any event for user-space, right? > >> >> In my opinion, if you want to perform an action on key release rather >> than key press then you should adjust the user space program to do so. >> From the point of view of the hardware DT (and even the kernel), the key >> press happens when you actually start pressing the key, and not later >> when you release it. > > The reason why we are discussing it back and forth looks like due to > lack of clarity as to how HMIBSC board supports this button. The > common implementation as per db410c DTS is to have two buttons: > > - One button is representing pwrkey(KEY_POWER) which is solely > consumed by the operating system to cleanly power off/restart the > db410c. > - Another button is representing pm8916_resin(KEY_VOLUMEDOWN) which > apart from normal volume down is also consumed by PMIC hardware to > perform a hard reset on a long press (if more than 10 sec.). > > However, on HMIBSC board we only have a single power/reset button > which has to support the dual role of above two buttons on db410c: > > Only one button as pm8916_resin(KEY_POWER), behaviour required: > - The rising edge (or button press) has to be only consumed by PMIC to > perform a hard reset of the HMIBSC board if pressed for more than 10 > secs. > - The falling edge has to be consumed by the operating system to > cleanly reboot the HMIBSC board. > > So what you are asking here is that the operating system has to be > notified of the rising edge of the button even if it has to just > ignore that without any action. Do you really think that would be > acceptable as a generic solution for systemd logind? Yes, this does seem to be a bug in systemd-logind. It appears to have subtly different behaviour depending on if you have HandlePowerKeyLongPress configured or not. If it is set to any action then systemd will wait until you release the key to decide whether to perform HandlePowerKey or HandlePowerKeyLongPress. However if there is no LongPress action configured then it will immediately perform the HandlePowerKey action on key down. [2] As a workaround, try adding HandlePowerKeyLongPress="lock" to logind.conf and see if that gives you the desired behaviour on short press. > > Also, why DT isn't the right place for correctly describing the > intended hardware behaviour? Or am I missing any DT policy which says > hardware has to be described the exact way it is rather than the > expected way for hardware to behave? BTW, If you want the hardware > behaviour to be properly commented then I can do that too. DT is for describing the hardware, not for dictating its behaviour. As Stephen pointed out, your workaround here only works due to a workaround in the kernel for a bug... DT isn't just consumed by Linux, and some other OS might not behave correctly as a result of this hack. [1]: https://github.com/systemd/systemd/blob/main/src/login/logind-button.c#L215 > > -Sumit > >> >> Thanks, >> Stephan >> >> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 39889d5f8e12..ad55e52e950b 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts new file mode 100644 index 000000000000..9c79a31a04db --- /dev/null +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts @@ -0,0 +1,510 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2015, The Linux Foundation. All rights reserved. + * Copyright (c) 2024, Linaro Ltd. + */ + +/dts-v1/; + +#include "msm8916-pm8916.dtsi" +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> +#include <dt-bindings/leds/common.h> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> +#include <dt-bindings/pinctrl/qcom,pmic-mpp.h> +#include <dt-bindings/sound/apq8016-lpass.h> + +/ { + model = "Schneider Electric HMIBSC Board"; + compatible = "schneider,apq8016-hmibsc", "qcom,apq8016"; + + aliases { + i2c1 = &blsp_i2c6; + i2c3 = &blsp_i2c4; + i2c4 = &blsp_i2c3; + mmc0 = &sdhc_1; /* eMMC */ + mmc1 = &sdhc_2; /* SD card */ + serial0 = &blsp_uart1; + serial1 = &blsp_uart2; + spi0 = &blsp_spi5; + usid0 = &pm8916_0; + }; + + chosen { + stdout-path = "serial0"; + }; + + hdmi-out { + compatible = "hdmi-connector"; + type = "a"; + + port { + hdmi_con: endpoint { + remote-endpoint = <&adv7533_out>; + }; + }; + }; + + gpio-keys { + compatible = "gpio-keys"; + autorepeat; + + pinctrl-0 = <&msm_key_volp_n_default>; + pinctrl-names = "default"; + + button { + label = "Volume Up"; + linux,code = <KEY_VOLUMEUP>; + gpios = <&tlmm 107 GPIO_ACTIVE_LOW>; + }; + }; + + leds { + compatible = "gpio-leds"; + pinctrl-0 = <&pm8916_mpps_leds>; + pinctrl-names = "default"; + + led-1 { + function = LED_FUNCTION_WLAN; + color = <LED_COLOR_ID_YELLOW>; + gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>; + linux,default-trigger = "phy0tx"; + default-state = "off"; + }; + + led-2 { + function = LED_FUNCTION_BLUETOOTH; + color = <LED_COLOR_ID_BLUE>; + gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>; + linux,default-trigger = "bluetooth-power"; + default-state = "off"; + }; + }; + + 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>; + ecc-size = <16>; + }; + }; + + 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-0 = <&usb_id_default>; + pinctrl-names = "default"; + }; +}; + +&blsp_i2c3 { + status = "okay"; + + eeprom@50 { + compatible = "atmel,24c32"; + reg = <0x50>; + }; +}; + +&blsp_i2c4 { + status = "okay"; + + adv_bridge: bridge@39 { + compatible = "adi,adv7533"; + reg = <0x39>; + + interrupts-extended = <&tlmm 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-0 = <&adv7533_int_active &adv7533_switch_active>; + pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>; + pinctrl-names = "default","sleep"; + #sound-dai-cells = <0>; + + 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 { + cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>; + status = "okay"; + + tpm@0 { + compatible = "infineon,slb9670", "tcg,tpm_tis-spi"; + reg = <0>; + spi-max-frequency = <500000>; + }; +}; + +&blsp_uart1 { + label = "UART0"; + status = "okay"; +}; + +&blsp_uart2 { + label = "UART1"; + 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"; }; + +&lpass { + status = "okay"; +}; + +&mdss { + status = "okay"; +}; + +&mdss_dsi0_out { + data-lanes = <0 1 2 3>; + remote-endpoint = <&adv7533_in>; +}; + +&pm8916_codec { + qcom,mbhc-vthreshold-low = <75 150 237 450 500>; + qcom,mbhc-vthreshold-high = <75 150 237 450 500>; + status = "okay"; +}; + +&pm8916_gpios { + gpio-line-names = + "USB_HUB_RESET_N_PM", + "USB_SW_SEL_PM", + "NC", + "NC"; + + usb_hub_reset_pm: usb-hub-reset-pm-state { + pins = "gpio1"; + function = PMIC_GPIO_FUNC_NORMAL; + + input-disable; + output-high; + }; + + usb_hub_reset_pm_device: usb-hub-reset-pm-device-state { + pins = "gpio1"; + function = PMIC_GPIO_FUNC_NORMAL; + + output-low; + }; + + usb_sw_sel_pm: usb-sw-sel-pm-state { + pins = "gpio2"; + function = PMIC_GPIO_FUNC_NORMAL; + + power-source = <PM8916_GPIO_VPH>; + input-disable; + output-high; + }; + + usb_sw_sel_pm_device: usb-sw-sel-pm-device-state { + pins = "gpio2"; + function = PMIC_GPIO_FUNC_NORMAL; + + power-source = <PM8916_GPIO_VPH>; + input-disable; + output-low; + }; +}; + +&pm8916_mpps { + gpio-line-names = + "NC", + "WLAN_LED_CTRL", + "BT_LED_CTRL", + "NC"; + + pm8916_mpps_leds: pm8916-mpps-state { + pins = "mpp2", "mpp3"; + function = "digital"; + + output-low; + }; +}; + +&pm8916_resin { + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>; + linux,code = <KEY_POWER>; + status = "okay"; +}; + +&pm8916_rpm_regulators { + pm8916_l17: l17 { + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + }; +}; + +&sdhc_1 { + status = "okay"; +}; + +&sdhc_2 { + pinctrl-0 = <&sdc2_default &sdc2_cd_default>; + pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>; + pinctrl-names = "default", "sleep"; + + cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>; + status = "okay"; +}; + +&sound { + pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>; + pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>; + pinctrl-names = "default", "sleep"; + model = "HMIBSC"; + audio-routing = + "AMIC2", "MIC BIAS Internal2", + "AMIC3", "MIC BIAS External1"; + status = "okay"; + + 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>; + }; + }; +}; + +&tlmm { + pinctrl-0 = <&uart1_mux0_rs232_high &uart1_mux1_rs232_low>; + pinctrl-names = "default"; + + adv7533_int_active: adv533-int-active-state { + pins = "gpio31"; + function = "gpio"; + + drive-strength = <16>; + bias-disable; + }; + + adv7533_int_suspend: adv7533-int-suspend-state { + pins = "gpio31"; + function = "gpio"; + + drive-strength = <2>; + bias-disable; + }; + + adv7533_switch_active: adv7533-switch-active-state { + pins = "gpio32"; + function = "gpio"; + + drive-strength = <16>; + bias-disable; + }; + + adv7533_switch_suspend: adv7533-switch-suspend-state { + pins = "gpio32"; + function = "gpio"; + + drive-strength = <2>; + bias-disable; + }; + + msm_key_volp_n_default: msm-key-volp-n-default-state { + pins = "gpio107"; + function = "gpio"; + + drive-strength = <8>; + bias-pull-up; + }; + + sdc2_cd_default: sdc2-cd-default-state { + pins = "gpio38"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + /* + * UART1 being the debug console supports various modes of + * operation (RS-232/485/422) controlled via GPIOs configured + * mux as follows: + * + * gpio100 gpio99 UART mode + * 0 0 loopback + * 0 1 RS-232 + * 1 0 RS-485 + * 1 1 RS-422 + * + * The default mode configured here is RS-232 mode. + */ + uart1_mux0_rs232_high: uart1-mux0-rs232-state { + bootph-all; + pins = "gpio99"; + function = "gpio"; + + drive-strength = <16>; + bias-disable; + output-high; + }; + + uart1_mux1_rs232_low: uart1-mux1-rs232-state { + bootph-all; + pins = "gpio100"; + function = "gpio"; + + drive-strength = <16>; + bias-disable; + output-low; + }; + + usb_id_default: usb-id-default-state { + pins = "gpio110"; + function = "gpio"; + + drive-strength = <8>; + bias-pull-up; + }; +}; + +&usb { + extcon = <&usb_id>, <&usb_id>; + + pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>; + pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>; + pinctrl-names = "default", "device"; + status = "okay"; +}; + +&usb_hs_phy { + extcon = <&usb_id>; +}; + +&wcnss { + firmware-name = "qcom/apq8016/wcnss.mbn"; + status = "okay"; +}; + +&wcnss_ctrl { + firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin"; +}; + +&wcnss_iris { + compatible = "qcom,wcn3620"; +}; + +&wcnss_mem { + status = "okay"; +}; + +/* PINCTRL - additions to nodes defined in msm8916.dtsi */ + +/* + * 2mA drive strength is not enough when connecting multiple + * I2C devices with different pull up resistors. + */ +&blsp_i2c4_default { + drive-strength = <16>; +}; + +&blsp_i2c6_default { + drive-strength = <16>; +}; + +&blsp_uart1_default { + bootph-all; +};