[PATCH 0/2] Fix and update fingerprint flashing on herobrine

[PATCH 0/2] Fix and update fingerprint flashing on herobrine

[Stephen Boyd]

This series fixes fingerprint pins on herobrine so that the flashing
code is more reliable. Right now it fails depending on timings. The
second patch updates the node to be compliant with the new binding
being proposed.

This technically depends on the binding series[1] but only the second
patch. The first patch is a fix and should be merged at the earliest
convenience. Even the second patch could be merged and it would probably
be OK.

Stephen Boyd (2):
  arm64: dts: qcom: sc7280-herobrine: Drop outputs on fpmcu pins
  arm64: dts: qcom: Fully describe fingerprint node on Herobrine

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Alexandru M Stan <amstan@chromium.org>

[1] https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org

base-commit: 1e49defb863638cde53e48805747271f80f9abec
-- 
https://chromeos.dev

[PATCH 1/2] arm64: dts: qcom: sc7280-herobrine: Drop outputs on fpmcu pins

[Stephen Boyd]

Having these pins with outputs is good on a fresh boot because it puts
the boot and reset pins in a known "good" state. Unfortunately, that
conflicts with the fingerprint firmware flashing code. The firmware
flashing process binds and unbinds the cros-ec and spidev drivers and
that reapplies the pin output values after the flashing code has
overridden the gpio values. This causes a problem because we try to put
the device into bootloader mode, bind the spidev driver and that
inadvertently puts it right back into normal boot mode, breaking the
flashing process.

Fix this by removing the outputs. We'll introduce a binding for
fingerprint cros-ec specifically to set the gpios properly via gpio APIs
during cros-ec driver probe instead.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Alexandru M Stan <amstan@chromium.org>
Fixes: 116f7cc43d28 ("arm64: dts: qcom: sc7280: Add herobrine-r1")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index dc17f2079695..984a7faf0888 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -677,7 +677,6 @@ fp_rst_l: fp-rst-l {
 		function = "gpio";
 		bias-disable;
 		drive-strength = <2>;
-		output-high;
 	};
 
 	fp_to_ap_irq_l: fp-to-ap-irq-l {
@@ -691,7 +690,6 @@ fpmcu_boot0: fpmcu-boot0 {
 		pins = "gpio68";
 		function = "gpio";
 		bias-disable;
-		output-low;
 	};
 
 	gsc_ap_int_odl: gsc-ap-int-odl {
-- 
https://chromeos.dev

[PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Stephen Boyd]

Update the fingerprint node on Herobrine to match the fingerprint DT
binding. This will allow us to drive the reset and boot gpios from the
driver when it is re-attached after flashing. We'll also be able to boot
the fingerprint processor if the BIOS isn't doing it for us.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Cc: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 984a7faf0888..282dda78ba3f 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
 	cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
 
 	cros_ec_fp: ec@0 {
-		compatible = "google,cros-ec-spi";
+		compatible = "google,cros-ec-fp";
 		reg = <0>;
 		interrupt-parent = <&tlmm>;
 		interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
+		boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
 		spi-max-frequency = <3000000>;
+		vdd-supply = <&pp3300_fp_mcu>;
 	};
 };
 
-- 
https://chromeos.dev

Re: [PATCH 1/2] arm64: dts: qcom: sc7280-herobrine: Drop outputs on fpmcu pins

[Matthias Kaehlcke]

On Wed, Mar 16, 2022 at 06:06:39PM -0700, Stephen Boyd wrote:
> Having these pins with outputs is good on a fresh boot because it puts
> the boot and reset pins in a known "good" state. Unfortunately, that
> conflicts with the fingerprint firmware flashing code. The firmware
> flashing process binds and unbinds the cros-ec and spidev drivers and
> that reapplies the pin output values after the flashing code has
> overridden the gpio values. This causes a problem because we try to put
> the device into bootloader mode, bind the spidev driver and that
> inadvertently puts it right back into normal boot mode, breaking the
> flashing process.
> 
> Fix this by removing the outputs. We'll introduce a binding for
> fingerprint cros-ec specifically to set the gpios properly via gpio APIs
> during cros-ec driver probe instead.
> 
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Alexandru M Stan <amstan@chromium.org>
> Fixes: 116f7cc43d28 ("arm64: dts: qcom: sc7280: Add herobrine-r1")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Matthias Kaehlcke]

On Wed, Mar 16, 2022 at 06:06:40PM -0700, Stephen Boyd wrote:
> Update the fingerprint node on Herobrine to match the fingerprint DT
> binding. This will allow us to drive the reset and boot gpios from the
> driver when it is re-attached after flashing. We'll also be able to boot
> the fingerprint processor if the BIOS isn't doing it for us.
> 
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Alexandru M Stan <amstan@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org
> 
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index 984a7faf0888..282dda78ba3f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
>  	cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
>  
>  	cros_ec_fp: ec@0 {
> -		compatible = "google,cros-ec-spi";
> +		compatible = "google,cros-ec-fp";
>  		reg = <0>;
>  		interrupt-parent = <&tlmm>;
>  		interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
> +		boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
>  		spi-max-frequency = <3000000>;
> +		vdd-supply = <&pp3300_fp_mcu>;

IIUC userspace controls this regulator. Do you add it just for completeness
even though the kernel doesn't use it?

>  	};
>  };

Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Stephen Boyd]

Quoting Matthias Kaehlcke (2022-03-17 15:06:38)
> On Wed, Mar 16, 2022 at 06:06:40PM -0700, Stephen Boyd wrote:
> > Update the fingerprint node on Herobrine to match the fingerprint DT
> > binding. This will allow us to drive the reset and boot gpios from the
> > driver when it is re-attached after flashing. We'll also be able to boot
> > the fingerprint processor if the BIOS isn't doing it for us.
> >
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Matthias Kaehlcke <mka@chromium.org>
> > Cc: Alexandru M Stan <amstan@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org
> >
> >  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > index 984a7faf0888..282dda78ba3f 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > @@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
> >       cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
> >
> >       cros_ec_fp: ec@0 {
> > -             compatible = "google,cros-ec-spi";
> > +             compatible = "google,cros-ec-fp";
> >               reg = <0>;
> >               interrupt-parent = <&tlmm>;
> >               interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
> > +             boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> > +             reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> >               spi-max-frequency = <3000000>;
> > +             vdd-supply = <&pp3300_fp_mcu>;
>
> IIUC userspace controls this regulator. Do you add it just for completeness
> even though the kernel doesn't use it?
>

Yes. Maybe in the future the kernel can use it by keeping the driver
bound and manually controlling the power supply during flashing. In
theory we could then power it down during suspend or when it isn't in
use too as long as we can hide the multi-second power up time.

Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Matthias Kaehlcke]

On Wed, Mar 16, 2022 at 06:06:40PM -0700, Stephen Boyd wrote:
> Update the fingerprint node on Herobrine to match the fingerprint DT
> binding. This will allow us to drive the reset and boot gpios from the
> driver when it is re-attached after flashing. We'll also be able to boot
> the fingerprint processor if the BIOS isn't doing it for us.
> 
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Alexandru M Stan <amstan@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 1/2] arm64: dts: qcom: sc7280-herobrine: Drop outputs on fpmcu pins

[Doug Anderson]

Hi,

On Wed, Mar 16, 2022 at 6:06 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Having these pins with outputs is good on a fresh boot because it puts
> the boot and reset pins in a known "good" state. Unfortunately, that
> conflicts with the fingerprint firmware flashing code. The firmware
> flashing process binds and unbinds the cros-ec and spidev drivers and
> that reapplies the pin output values after the flashing code has
> overridden the gpio values. This causes a problem because we try to put
> the device into bootloader mode, bind the spidev driver and that
> inadvertently puts it right back into normal boot mode, breaking the
> flashing process.
>
> Fix this by removing the outputs. We'll introduce a binding for
> fingerprint cros-ec specifically to set the gpios properly via gpio APIs
> during cros-ec driver probe instead.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Alexandru M Stan <amstan@chromium.org>
> Fixes: 116f7cc43d28 ("arm64: dts: qcom: sc7280: Add herobrine-r1")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Doug Anderson]

Hi,

On Wed, Mar 16, 2022 at 6:06 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Update the fingerprint node on Herobrine to match the fingerprint DT
> binding. This will allow us to drive the reset and boot gpios from the
> driver when it is re-attached after flashing. We'll also be able to boot
> the fingerprint processor if the BIOS isn't doing it for us.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Alexandru M Stan <amstan@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>
> Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org
>
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index 984a7faf0888..282dda78ba3f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
>         cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
>
>         cros_ec_fp: ec@0 {
> -               compatible = "google,cros-ec-spi";
> +               compatible = "google,cros-ec-fp";
>                 reg = <0>;
>                 interrupt-parent = <&tlmm>;
>                 interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
> +               boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> +               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
>                 spi-max-frequency = <3000000>;
> +               vdd-supply = <&pp3300_fp_mcu>;

IMO we shouldn't specify vdd-supply here when it's a bogus regulator
(doesn't actually control the relevant GPIO). Having device trees like
this will make it hard to transition to the kernel controlling this
GPIO in the future because the cros-ec-fp driver won't know whether
it's controlling the GPIO or not. So my vote would be either:

1. Go whole hog and have the kernel in charge of the regulator,
exposing regulator control to the userspace updater through some sort
of GPIO

- or -

2. Make the "vdd-supply" optional and don't specify it until we're
ready to go whole hog.


Also note: looking back at the note about the fingerprint regulator,
there's something I wonder if we tried. Did we try to have the
userspace "updater" try unbinding the fingerprint regulator so it
could get control of the GPIO? Then the regulator could normally have
control of it but if userspace wanted control it would unbind the
regulator driver.

-Doug

Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

[Stephen Boyd]

Quoting Doug Anderson (2022-03-18 13:55:56)
> Hi,
>
> On Wed, Mar 16, 2022 at 6:06 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Update the fingerprint node on Herobrine to match the fingerprint DT
> > binding. This will allow us to drive the reset and boot gpios from the
> > driver when it is re-attached after flashing. We'll also be able to boot
> > the fingerprint processor if the BIOS isn't doing it for us.
> >
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Matthias Kaehlcke <mka@chromium.org>
> > Cc: Alexandru M Stan <amstan@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org
> >
> >  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > index 984a7faf0888..282dda78ba3f 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > @@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
> >         cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
> >
> >         cros_ec_fp: ec@0 {
> > -               compatible = "google,cros-ec-spi";
> > +               compatible = "google,cros-ec-fp";
> >                 reg = <0>;
> >                 interrupt-parent = <&tlmm>;
> >                 interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
> > +               boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> > +               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> >                 spi-max-frequency = <3000000>;
> > +               vdd-supply = <&pp3300_fp_mcu>;
>
> IMO we shouldn't specify vdd-supply here when it's a bogus regulator
> (doesn't actually control the relevant GPIO). Having device trees like
> this will make it hard to transition to the kernel controlling this
> GPIO in the future because the cros-ec-fp driver won't know whether
> it's controlling the GPIO or not. So my vote would be either:
>
> 1. Go whole hog and have the kernel in charge of the regulator,
> exposing regulator control to the userspace updater through some sort
> of GPIO
>
> - or -
>
> 2. Make the "vdd-supply" optional and don't specify it until we're
> ready to go whole hog.

It isn't an optional supply because the device always has this voltage
pin connected to power it. I think we decided in the driver side patches
that this isn't an issue because the driver will ignore the regulator
for now. Eventually it will take control and firmware flashing will be
done differently. If you don't agree please let me know.

>
> Also note: looking back at the note about the fingerprint regulator,
> there's something I wonder if we tried. Did we try to have the
> userspace "updater" try unbinding the fingerprint regulator so it
> could get control of the GPIO? Then the regulator could normally have
> control of it but if userspace wanted control it would unbind the
> regulator driver.
>

Nope we didn't try that. I find it pretty disappointing that userspace
needs to control the regulator at all though. We should work towards
moving the control into the kernel for all these gpios and regulators so
that userspace simply flashes the firmware in a platform agnostic way.