mbox series

[0/3] Firmware loading option

Message ID 20201029170313.25529-1-andrej.valek@siemens.com
Headers show
Series Firmware loading option | expand

Message

Valek, Andrej Oct. 29, 2020, 5:03 p.m. UTC
Add option to prevent firmware/configuration loading during each boot.

Andrej Valek (3):
  Input: goodix - add option to disable firmware loading
  dt-bindings: goodix
  Input: atmel_mxt_ts - add option to disable firmware loading

 .../bindings/input/touchscreen/goodix.yaml      |  1 +
 drivers/input/touchscreen/atmel_mxt_ts.c        | 17 ++++++++++++++---
 drivers/input/touchscreen/goodix.c              |  4 +++-
 3 files changed, 18 insertions(+), 4 deletions(-)

Comments

Dmitry Torokhov Oct. 29, 2020, 8:36 p.m. UTC | #1
Hi Andrej,

On Thu, Oct 29, 2020 at 06:03:11PM +0100, Andrej Valek wrote:
> Firmware file loadind for GT911 controller takes too much time (~60s).

> There is no check that configuration is the same which is already present.

> This happens always during boot, which makes touchscreen unusable.

> 

> Add there an option to prevent firmware file loading, but keep it enabled

> by default.


I thought that Goodix was losing firmware loading at poweroff. Is this
not the case with this model?

Adding Hans as he was working with this driver/code.

> 

> Signed-off-by: Andrej Valek <andrej.valek@siemens.com>

> ---

>  drivers/input/touchscreen/goodix.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c

> index 02c75ea385e08..44336ecd2acdf 100644

> --- a/drivers/input/touchscreen/goodix.c

> +++ b/drivers/input/touchscreen/goodix.c

> @@ -941,7 +941,9 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)

>  	default:

>  		if (ts->gpiod_int && ts->gpiod_rst) {

>  			ts->reset_controller_at_probe = true;

> -			ts->load_cfg_from_disk = true;

> +			/* Prevent cfg loading for each start */

> +			ts->load_cfg_from_disk = !device_property_read_bool(dev,

> +						 "touchscreen-do-not-load-fw");

>  			ts->irq_pin_access_method = IRQ_PIN_ACCESS_GPIO;

>  		}

>  	}

> -- 

> 2.20.1

> 


Thanks.

-- 
Dmitry
Valek, Andrej Oct. 30, 2020, 11:02 a.m. UTC | #2
Hi Hans,

I am not saying, that just configuration loading took such a long time. Thu full process including configuration and FW loading takes it. 

Means that I would like to prevent this situation, but keep the old scenario as a default behavior.

Regards,
Andrej

> Hi,

>

> On 10/29/20 9:36 PM, Dmitry Torokhov wrote:

>> Hi Andrej,

>> 

> On Thu, Oct 29, 2020 at 06:03:11PM +0100, Andrej Valek wrote:

>>> Firmware file loadind for GT911 controller takes too much time (~60s).

>>> There is no check that configuration is the same which is already present.

>>> This happens always during boot, which makes touchscreen unusable.

>>>

>>> Add there an option to prevent firmware file loading, but keep it 

>>> enabled by default.

>> 

>> I thought that Goodix was losing firmware loading at poweroff. Is this 

>> not the case with this model?

>

> So first of all there are 2 sorts of firmware involved with the Goodix touchscreen controllers, the actual firmware and a > block of config data for that firmware which I presume adjusts it for the specific (model of) the digitizer which is attached.

>

> ATM the mainline Linux driver does not support models where the actual firmware itself needs to be loaded (because they only have RAM, so they come up without firmware).

>

> I do have one model tablet with a ROM-less goodix touchpad controller, so if I ever find the time I might add support for loading the actual firmware.

>

> So what we are talking about here is just loading the config data and I'm a bit surprised that this take so long.

>

>> Adding Hans as he was working with this driver/code.

>

> With all that said I have no objection to this change.

>

> Regards,

>

> Hans

>

>

>

>> 

>>>

>>> Signed-off-by: Andrej Valek <andrej.valek@siemens.com>

>>> ---

>>>  drivers/input/touchscreen/goodix.c | 4 +++-

>>>  1 file changed, 3 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/input/touchscreen/goodix.c 

>>> b/drivers/input/touchscreen/goodix.c

>>> index 02c75ea385e08..44336ecd2acdf 100644

>>> --- a/drivers/input/touchscreen/goodix.c

>>> +++ b/drivers/input/touchscreen/goodix.c

>>> @@ -941,7 +941,9 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)

>>>  	default:

>>>  		if (ts->gpiod_int && ts->gpiod_rst) {

>>>  			ts->reset_controller_at_probe = true;

>>> -			ts->load_cfg_from_disk = true;

>>> +			/* Prevent cfg loading for each start */

>>> +			ts->load_cfg_from_disk = !device_property_read_bool(dev,

>>> +						 "touchscreen-do-not-load-fw");

>>>  			ts->irq_pin_access_method = IRQ_PIN_ACCESS_GPIO;

>>>  		}

>>>  	}

>>> --

>>> 2.20.1

>>>

>> 

>> Thanks.

>>
Rob Herring Nov. 9, 2020, 9:37 p.m. UTC | #3
On Fri, Nov 06, 2020 at 11:05:38AM +0100, Andrej Valek wrote:
> Add information about option how to disable FW loading for each boot.


Again, fix your subject.

> 

> Signed-off-by: Andrej Valek <andrej.valek@siemens.com>

> ---

>  Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml

> index e7d9404c86ab..8b0fa25ae96e 100644

> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml

> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml

> @@ -53,7 +53,7 @@ properties:

>    touchscreen-size-x: true

>    touchscreen-size-y: true

>    touchscreen-swapped-x-y: true

> -  touchscreen-do-not-load-fw: false

> +  goodix-do-not-load-fw: false


Why is this incremental on v1?

And that's not how vendor prefixes on properties work.
Valek, Andrej Nov. 10, 2020, 9:07 a.m. UTC | #4
Add option to prevent firmware/configuration loading during each boot.

Andrej Valek (4):
  Input: goodix - add option to disable firmware loading
  dt-bindings: touchscreen: goodix: add info about disabling FW loading
  Input: atmel_mxt_ts - add option to disable firmware loading
  Input: st1232 - add support resolution reading

 .../bindings/input/touchscreen/goodix.yaml    |  1 +
 drivers/input/touchscreen/atmel_mxt_ts.c      | 17 ++++--
 drivers/input/touchscreen/goodix.c            |  4 +-
 drivers/input/touchscreen/st1232.c            | 52 +++++++++++++------
 4 files changed, 54 insertions(+), 20 deletions(-)
Rob Herring Nov. 10, 2020, 1:43 p.m. UTC | #5
On Tue, 10 Nov 2020 10:07:18 +0100, Andrej Valek wrote:
> Add information about option how to disable FW loading for each boot.

> 

> Signed-off-by: Andrej Valek <andrej.valek@siemens.com>

> ---

>  Documentation/devicetree/bindings/input/touchscreen/goodix.yaml | 1 +

>  1 file changed, 1 insertion(+)

> 



My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml: properties:goodix,do-not-load-fw: False is not of type 'object'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml: ignoring, error in schema: properties: goodix,do-not-load-fw
warning: no schema found in file: ./Documentation/devicetree/bindings/input/touchscreen/goodix.yaml


See https://patchwork.ozlabs.org/patch/1397438

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Dmitry Torokhov Nov. 23, 2020, 6:53 a.m. UTC | #6
On Fri, Oct 30, 2020 at 10:56:20AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 10/29/20 9:36 PM, Dmitry Torokhov wrote:
> > Hi Andrej,
> > 
> > On Thu, Oct 29, 2020 at 06:03:11PM +0100, Andrej Valek wrote:
> >> Firmware file loadind for GT911 controller takes too much time (~60s).
> >> There is no check that configuration is the same which is already present.
> >> This happens always during boot, which makes touchscreen unusable.
> >>
> >> Add there an option to prevent firmware file loading, but keep it enabled
> >> by default.
> > 
> > I thought that Goodix was losing firmware loading at poweroff. Is this
> > not the case with this model?
> 
> So first of all there are 2 sorts of firmware involved with the
> Goodix touchscreen controllers, the actual firmware and a block
> of config data for that firmware which I presume adjusts it for
> the specific (model of) the digitizer which is attached.
> 
> ATM the mainline Linux driver does not support models where
> the actual firmware itself needs to be loaded (because they
> only have RAM, so they come up without firmware).
> 
> I do have one model tablet with a ROM-less goodix touchpad
> controller, so if I ever find the time I might add support
> for loading the actual firmware.
> 
> So what we are talking about here is just loading the config
> data and I'm a bit surprised that this take so long.

So I am still confused about this: is the config stored in RAM or NVRAM?
I.e. do we actually need to re-load it every time on boot, or it
supposed to be flashed only when it is changed (or lost)?

Thanks.
Hans de Goede Nov. 23, 2020, 8:46 a.m. UTC | #7
Hi,

On 11/23/20 7:53 AM, Dmitry Torokhov wrote:
> On Fri, Oct 30, 2020 at 10:56:20AM +0100, Hans de Goede wrote:

>> Hi,

>>

>> On 10/29/20 9:36 PM, Dmitry Torokhov wrote:

>>> Hi Andrej,

>>>

>>> On Thu, Oct 29, 2020 at 06:03:11PM +0100, Andrej Valek wrote:

>>>> Firmware file loadind for GT911 controller takes too much time (~60s).

>>>> There is no check that configuration is the same which is already present.

>>>> This happens always during boot, which makes touchscreen unusable.

>>>>

>>>> Add there an option to prevent firmware file loading, but keep it enabled

>>>> by default.

>>>

>>> I thought that Goodix was losing firmware loading at poweroff. Is this

>>> not the case with this model?

>>

>> So first of all there are 2 sorts of firmware involved with the

>> Goodix touchscreen controllers, the actual firmware and a block

>> of config data for that firmware which I presume adjusts it for

>> the specific (model of) the digitizer which is attached.

>>

>> ATM the mainline Linux driver does not support models where

>> the actual firmware itself needs to be loaded (because they

>> only have RAM, so they come up without firmware).

>>

>> I do have one model tablet with a ROM-less goodix touchpad

>> controller, so if I ever find the time I might add support

>> for loading the actual firmware.

>>

>> So what we are talking about here is just loading the config

>> data and I'm a bit surprised that this take so long.

> 

> So I am still confused about this: is the config stored in RAM or NVRAM?

> I.e. do we actually need to re-load it every time on boot, or it

> supposed to be flashed only when it is changed (or lost)?


I only know about these touchscreens on x86, where we have a BIOS
muddling the waters.

We have seen devices which loose the config over suspend/resume,
which suggests it is in RAM. I recently added a fix for these which
saves the config at boot, which suggests that at least on the device
model with the suspend/resume issue the config is loaded into the chip
by the BIOS.

But I'm not sure that this is the case everywhere. Most other models
likely have the config in NVRAM.

I guess this is the same as with the firmware, and it differs per
model. I know for sure that their are RAM only models which need
the firmware loaded at boot, these are mostly found on ARM devs,
but I have one X86 devices (which currently does not work) which
also has RAM only and thus needs Linux to load the firmware
(which is not supported atm). These RAM only models, presumably
also have only RAM for the config.

Regards,

Hans