| Message ID | 20241015122427.15995-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: x1e80100-crd: describe HID supplies | expand |
On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote: > Add the missing HID supplies to avoid relying on other consumers to keep > them on. > > This also avoids the following warnings on boot: > > i2c_hid_of 0-0010: supply vdd not found, using dummy regulator > i2c_hid_of 0-0010: supply vddl not found, using dummy regulator > i2c_hid_of 1-0015: supply vdd not found, using dummy regulator > i2c_hid_of 1-0015: supply vddl not found, using dummy regulator > i2c_hid_of 1-003a: supply vdd not found, using dummy regulator > i2c_hid_of 1-003a: supply vddl not found, using dummy regulator > > Note that VREG_MISC_3P3 is also used for things like the fingerprint > reader which are not yet fully described so mark the regulator as always > on for now. > > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") > Cc: Abel Vesa <abel.vesa@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 24-10-15 14:24:27, Johan Hovold wrote: > Add the missing HID supplies to avoid relying on other consumers to keep > them on. > > This also avoids the following warnings on boot: > > i2c_hid_of 0-0010: supply vdd not found, using dummy regulator > i2c_hid_of 0-0010: supply vddl not found, using dummy regulator > i2c_hid_of 1-0015: supply vdd not found, using dummy regulator > i2c_hid_of 1-0015: supply vddl not found, using dummy regulator > i2c_hid_of 1-003a: supply vdd not found, using dummy regulator > i2c_hid_of 1-003a: supply vddl not found, using dummy regulator > > Note that VREG_MISC_3P3 is also used for things like the fingerprint > reader which are not yet fully described so mark the regulator as always > on for now. > > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") > Cc: Abel Vesa <abel.vesa@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > index 10b28d870f08..4ab7078f76e0 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 { > regulator-boot-on; > }; > [...] > > +&pm8550ve_8_gpios { > + misc_3p3_reg_en: misc-3p3-reg-en-state { > + pins = "gpio6"; > + function = "normal"; > + bias-disable; Maybe output-enable and input-disable are needed. Can you please check? Otherwise, LGTM: Reviewed-by: Abel Vesa <abel.vesa@linaro.org> > + }; > +}; > + > &pmc8380_3_gpios { > edp_bl_en: edp-bl-en-state { > pins = "gpio4"; > -- > 2.45.2 >
On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote: > Add the missing HID supplies to avoid relying on other consumers to keep > them on. > > This also avoids the following warnings on boot: > > i2c_hid_of 0-0010: supply vdd not found, using dummy regulator > i2c_hid_of 0-0010: supply vddl not found, using dummy regulator > i2c_hid_of 1-0015: supply vdd not found, using dummy regulator > i2c_hid_of 1-0015: supply vddl not found, using dummy regulator > i2c_hid_of 1-003a: supply vdd not found, using dummy regulator > i2c_hid_of 1-003a: supply vddl not found, using dummy regulator > > Note that VREG_MISC_3P3 is also used for things like the fingerprint > reader which are not yet fully described so mark the regulator as always > on for now. > > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") > Cc: Abel Vesa <abel.vesa@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > index 10b28d870f08..4ab7078f76e0 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 { > regulator-boot-on; > }; > > + vreg_misc_3p3: regulator-misc-3p3 { > + compatible = "regulator-fixed"; > + > + regulator-name = "VREG_MISC_3P3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + gpio = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&misc_3p3_reg_en>; > + > + regulator-boot-on; > + regulator-always-on; > + }; > + > vreg_nvme: regulator-nvme { > compatible = "regulator-fixed"; > > @@ -689,6 +706,9 @@ touchpad@15 { > hid-descr-addr = <0x1>; > interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>; > > + vdd-supply = <&vreg_misc_3p3>; > + vddl-supply = <&vreg_l12b_1p2>; > + > pinctrl-0 = <&tpad_default>; > pinctrl-names = "default"; > > @@ -702,6 +722,9 @@ keyboard@3a { > hid-descr-addr = <0x1>; > interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>; > > + vdd-supply = <&vreg_misc_3p3>; > + vddl-supply = <&vreg_l12b_1p2>; > + > pinctrl-0 = <&kybd_default>; > pinctrl-names = "default"; > > @@ -721,6 +744,9 @@ touchscreen@10 { > hid-descr-addr = <0x1>; > interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>; > > + vdd-supply = <&vreg_misc_3p3>; > + vddl-supply = <&vreg_l15b_1p8>; > + > pinctrl-0 = <&ts0_default>; > pinctrl-names = "default"; > }; > @@ -854,6 +880,14 @@ &pcie6a_phy { > status = "okay"; > }; > > +&pm8550ve_8_gpios { > + misc_3p3_reg_en: misc-3p3-reg-en-state { > + pins = "gpio6"; > + function = "normal"; > + bias-disable; Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V (VPH_PWR) or 1.8V, depending on which power-source is selected. Without that, we rely on the firmware to set the voltage level for the GPIO during boot. I'm not sure which one would be suitable here. I guess we can just replicate what the firmware configures during boot. Thanks, Stephan
On Tue, Oct 15, 2024 at 04:41:13PM +0300, Abel Vesa wrote: > On 24-10-15 14:24:27, Johan Hovold wrote: > > Add the missing HID supplies to avoid relying on other consumers to keep > > them on. > > > > This also avoids the following warnings on boot: > > > > i2c_hid_of 0-0010: supply vdd not found, using dummy regulator > > i2c_hid_of 0-0010: supply vddl not found, using dummy regulator > > i2c_hid_of 1-0015: supply vdd not found, using dummy regulator > > i2c_hid_of 1-0015: supply vddl not found, using dummy regulator > > i2c_hid_of 1-003a: supply vdd not found, using dummy regulator > > i2c_hid_of 1-003a: supply vddl not found, using dummy regulator > > > > Note that VREG_MISC_3P3 is also used for things like the fingerprint > > reader which are not yet fully described so mark the regulator as always > > on for now. > > > > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") > > Cc: Abel Vesa <abel.vesa@linaro.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > index 10b28d870f08..4ab7078f76e0 100644 > > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 { > > regulator-boot-on; > > }; > > > > [...] > > > > > +&pm8550ve_8_gpios { > > + misc_3p3_reg_en: misc-3p3-reg-en-state { > > + pins = "gpio6"; > > + function = "normal"; > > + bias-disable; > > Maybe output-enable and input-disable are needed. Can you please check? > FWIW, there is a reason behind explicitly describing the intended direction of the pin for PMIC GPIOs with properties like "output-enable" or "input-disable": On QC platforms, PMIC GPIOs can be either in "input" mode, "output" mode, or "input+output" mode. If you don't specify exactly what you want, then the pinctrl-spmi-gpio driver will only add to the existing configuration. For the configuration above this means: 1. If GPIO6 is disabled or in "output" mode during boot, the resulting mode will be "output". 2. If GPIO6 is in "input" mode during boot, the resulting mode will be "input+output". I don't know if "input+output" mode has any negative impact compared to pure "output" mode. We usually want to have the pins in a consistent state though (i.e. independent of the boot up state). Thanks, Stephan
On 24-10-15 16:57:18, Stephan Gerhold wrote: > On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote: > > Add the missing HID supplies to avoid relying on other consumers to keep > > them on. > > > > This also avoids the following warnings on boot: > > > > i2c_hid_of 0-0010: supply vdd not found, using dummy regulator > > i2c_hid_of 0-0010: supply vddl not found, using dummy regulator > > i2c_hid_of 1-0015: supply vdd not found, using dummy regulator > > i2c_hid_of 1-0015: supply vddl not found, using dummy regulator > > i2c_hid_of 1-003a: supply vdd not found, using dummy regulator > > i2c_hid_of 1-003a: supply vddl not found, using dummy regulator > > > > Note that VREG_MISC_3P3 is also used for things like the fingerprint > > reader which are not yet fully described so mark the regulator as always > > on for now. > > > > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") > > Cc: Abel Vesa <abel.vesa@linaro.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > index 10b28d870f08..4ab7078f76e0 100644 > > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts > > @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 { > > regulator-boot-on; > > }; > > > > + vreg_misc_3p3: regulator-misc-3p3 { > > + compatible = "regulator-fixed"; > > + > > + regulator-name = "VREG_MISC_3P3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + > > + gpio = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&misc_3p3_reg_en>; > > + > > + regulator-boot-on; > > + regulator-always-on; > > + }; > > + > > vreg_nvme: regulator-nvme { > > compatible = "regulator-fixed"; > > > > @@ -689,6 +706,9 @@ touchpad@15 { > > hid-descr-addr = <0x1>; > > interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>; > > > > + vdd-supply = <&vreg_misc_3p3>; > > + vddl-supply = <&vreg_l12b_1p2>; > > + > > pinctrl-0 = <&tpad_default>; > > pinctrl-names = "default"; > > > > @@ -702,6 +722,9 @@ keyboard@3a { > > hid-descr-addr = <0x1>; > > interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>; > > > > + vdd-supply = <&vreg_misc_3p3>; > > + vddl-supply = <&vreg_l12b_1p2>; > > + > > pinctrl-0 = <&kybd_default>; > > pinctrl-names = "default"; > > > > @@ -721,6 +744,9 @@ touchscreen@10 { > > hid-descr-addr = <0x1>; > > interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>; > > > > + vdd-supply = <&vreg_misc_3p3>; > > + vddl-supply = <&vreg_l15b_1p8>; > > + > > pinctrl-0 = <&ts0_default>; > > pinctrl-names = "default"; > > }; > > @@ -854,6 +880,14 @@ &pcie6a_phy { > > status = "okay"; > > }; > > > > +&pm8550ve_8_gpios { > > + misc_3p3_reg_en: misc-3p3-reg-en-state { > > + pins = "gpio6"; > > + function = "normal"; > > + bias-disable; > > Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V > (VPH_PWR) or 1.8V, depending on which power-source is selected. Without > that, we rely on the firmware to set the voltage level for the GPIO > during boot. AFAIU, the power-source here should be 0, which selects VPH_PWR which is 3.3V. In that case I think we can avoid explicitly setting it. > > I'm not sure which one would be suitable here. I guess we can just > replicate what the firmware configures during boot. > > Thanks, > Stephan
On Wed, Oct 16, 2024 at 06:43:26PM +0200, Stephan Gerhold wrote: > On Tue, Oct 15, 2024 at 04:41:13PM +0300, Abel Vesa wrote: > > On 24-10-15 14:24:27, Johan Hovold wrote: > > > +&pm8550ve_8_gpios { > > > + misc_3p3_reg_en: misc-3p3-reg-en-state { > > > + pins = "gpio6"; > > > + function = "normal"; > > > + bias-disable; > > > > Maybe output-enable and input-disable are needed. Can you please check? > > FWIW, there is a reason behind explicitly describing the intended > direction of the pin for PMIC GPIOs with properties like "output-enable" > or "input-disable": On QC platforms, PMIC GPIOs can be either in "input" > mode, "output" mode, or "input+output" mode. If you don't specify > exactly what you want, then the pinctrl-spmi-gpio driver will only add > to the existing configuration. > > For the configuration above this means: > > 1. If GPIO6 is disabled or in "output" mode during boot, the resulting > mode will be "output". > > 2. If GPIO6 is in "input" mode during boot, the resulting mode will be > "input+output". > > I don't know if "input+output" mode has any negative impact compared to > pure "output" mode. We usually want to have the pins in a consistent > state though (i.e. independent of the boot up state). Fair enough. I was worried that configuring the pin as an output without setting the output value could cause trouble (e.g. always default to low), but it seems at least the Linux driver handles that. Johan
On Fri, Oct 18, 2024 at 08:19:35PM +0300, Abel Vesa wrote: > On 24-10-15 16:57:18, Stephan Gerhold wrote: > > On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote: > > > +&pm8550ve_8_gpios { > > > + misc_3p3_reg_en: misc-3p3-reg-en-state { > > > + pins = "gpio6"; > > > + function = "normal"; > > > + bias-disable; > > > > Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V > > (VPH_PWR) or 1.8V, depending on which power-source is selected. Without > > that, we rely on the firmware to set the voltage level for the GPIO > > during boot. > > AFAIU, the power-source here should be 0, which selects VPH_PWR which is > 3.3V. In that case I think we can avoid explicitly setting it. The firmware uses 1.8 V here in fact, but it seems like 3.3 V would work as well. > > I'm not sure which one would be suitable here. I guess we can just > > replicate what the firmware configures during boot. Let's start with that at least. Johan
diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts index 10b28d870f08..4ab7078f76e0 100644 --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 { regulator-boot-on; }; + vreg_misc_3p3: regulator-misc-3p3 { + compatible = "regulator-fixed"; + + regulator-name = "VREG_MISC_3P3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>; + enable-active-high; + + pinctrl-names = "default"; + pinctrl-0 = <&misc_3p3_reg_en>; + + regulator-boot-on; + regulator-always-on; + }; + vreg_nvme: regulator-nvme { compatible = "regulator-fixed"; @@ -689,6 +706,9 @@ touchpad@15 { hid-descr-addr = <0x1>; interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <&vreg_misc_3p3>; + vddl-supply = <&vreg_l12b_1p2>; + pinctrl-0 = <&tpad_default>; pinctrl-names = "default"; @@ -702,6 +722,9 @@ keyboard@3a { hid-descr-addr = <0x1>; interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <&vreg_misc_3p3>; + vddl-supply = <&vreg_l12b_1p2>; + pinctrl-0 = <&kybd_default>; pinctrl-names = "default"; @@ -721,6 +744,9 @@ touchscreen@10 { hid-descr-addr = <0x1>; interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <&vreg_misc_3p3>; + vddl-supply = <&vreg_l15b_1p8>; + pinctrl-0 = <&ts0_default>; pinctrl-names = "default"; }; @@ -854,6 +880,14 @@ &pcie6a_phy { status = "okay"; }; +&pm8550ve_8_gpios { + misc_3p3_reg_en: misc-3p3-reg-en-state { + pins = "gpio6"; + function = "normal"; + bias-disable; + }; +}; + &pmc8380_3_gpios { edp_bl_en: edp-bl-en-state { pins = "gpio4";
Add the missing HID supplies to avoid relying on other consumers to keep them on. This also avoids the following warnings on boot: i2c_hid_of 0-0010: supply vdd not found, using dummy regulator i2c_hid_of 0-0010: supply vddl not found, using dummy regulator i2c_hid_of 1-0015: supply vdd not found, using dummy regulator i2c_hid_of 1-0015: supply vddl not found, using dummy regulator i2c_hid_of 1-003a: supply vdd not found, using dummy regulator i2c_hid_of 1-003a: supply vddl not found, using dummy regulator Note that VREG_MISC_3P3 is also used for things like the fingerprint reader which are not yet fully described so mark the regulator as always on for now. Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support") Cc: Abel Vesa <abel.vesa@linaro.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++ 1 file changed, 34 insertions(+)