Re: [RFC PATCH 3/3] decompress/keepalive.h: add config option for toggling a set of bits

[Rasmus Villemoes]

On 24/10/2019 14.17, Linus Walleij wrote:
> On Thu, Oct 17, 2019 at 1:49 PM Rasmus Villemoes
> <linux@xxxxxxxxxxxxxxxxxx> wrote:
> 
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_REG
>> +       hex "Address of register to modify while decompressing"
>> +       help
>> +         Set this to a physical address of a 32-bit memory word to
>> +         modify while decompressing.
>> +
>> +config DECOMPRESS_KEEPALIVE_TOGGLE_MASK
>> +       hex "Bit mask to toggle while decompressing"
>> +       help
>> +         The register selected above will periodically be xor'ed with
>> +         this value during decompression.
> 
> I would not allow users to store these vital hex values in their
> defconfig and other unsafe places. Instead follow the pattern from
> arch/arm/Kconfig.debug for storing the DEBUG_UART_PHYS:
> 
> config DEBUG_UART_PHYS
>         hex "Physical base address of debug UART"
>         default 0x01c20000 if DEBUG_DAVINCI_DMx_UART0
>         default 0x01c28000 if DEBUG_SUNXI_UART0
>         default 0x01c28400 if DEBUG_SUNXI_UART1
> ....
> i.e. make sure to provide the right default values. We probably
> need at least one example for others to follow.
>
> Maybe this is your plan, I don't know, wanted to point it out
> anyways.

The thing is, there is no proper default value for the use cases I have
in mind: Custom hardware based on some SOC, where the designer has wired
on an external gpio-triggered watchdog. That could be gpio 25 of gpio
bank 0, or gpio 2 of gpio bank 3, or ... so I don't see how there could
possibly be any sane default value - the kernel certainly shouldn't grow
a config option for every single custom board out there.

That's why this is different from the previously existing
arch_decomp_wdog - that was (AFAICT) about feeding the SOC's builtin
watchdog.

I realize this is rather specific, and the current implementation for
example won't work if the gpio value cannot be toggled in such a simple
way (perhaps there are separate "set" and "clear" registers or whatnot)
- but as I said, it is sufficient for the many different cases I've seen
so far (and something like my patches have been used for years on those
boards).

An alternative is to simply provide a complete implementation of
decompress_keepalive() (or arch_decomp_wdog if we want to keep that
name) in an external .o/.c/.S file, and do something like

OBJS += $(CONFIG_DECOMP_WDOG:"%"=%)

in arch/foo/boot/compressed/Makefile. Then the physical address etc. do
not get written in Kconfig, and it should work for all cases, including
the ones that need to write 0x55, 0xaa, 0x12 in order to some
SOC-specific register.

Rasmus