Re: [PATCH] gpio: rcar: select General Output Register to set output states

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Vladimir,

(increasing review priority due to Linus applying the patch)

On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy
<vladimir_zapolskiy@xxxxxxxxxx> wrote:
> R-Car GPIO controller provides two interfaces to set GPIO line output
> signal state, and for a particular GPIO line the selected interface is
> determined by OUTDTSEL bit value.
>
> At the moment the driver supports only one of two interfaces, namely
> OUTDT General Output Register is used to control the output signal.
>
> While this selection is the default one on reset, it is not explicitly
> configured on probe, thus it might be possible that kernel and userspace
> consumers of a GPIO won't be able to set the wanted GPIO output signal.
>
> Below is a simple test case to reproduce the described problem and
> verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL
> configuration from a bootloader:
>
>   u-boot    > mw.l 0xe6055440 0x3000 1
>   ...
>   userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger
>   userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger
>
> Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3")
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>

Thanks for your patch!

> The proposed change could be seen as an invitation for a more interesting
> discussion about a necessity to add a pretty trivial support of the second
> interface, for instance by selecting between OUTDT and OUTDTH/OUTDTL on
> basis of read-only value of OUTDTSEL register, or, simply by switching
> the driver to use the second interface only, because it does not require
> an additional gpio_rcar_read() call, theoretically it should give noticeably
> faster rate of bitbanging.
>
> For reference the problem with the original interface comes from an inability
> to set GPIO output signals from an RTOS, which runs in parallel to Linux and
> wants to control some GPIOs on its own, usage of OUTDTH/OUTDTL excludes
> a race in concurrent read/write register operations.
>
> As a note in my opinion the selection of OUTDT vs. OUTDTH/OUTDTL should
> NOT be done in DTS, extension to 3-cell values for GPIO consumers seems
> unreasonable.

Indeed, this is pure software configuration, not hardware description, so it
does not belong in DT.

> Below is the list of helpful tips for change reviewers, comments are welcome:
> * I didn't manage to find H1 or M1A User's Manuals to confirm that
>   OUTDTSEL register and the second OUTDTH/OUTDTL interface is present
>   on the GPIO controllers found on R-Car Gen1 SoCs,

Unfortunately R-Car M1A and H1 do not have the OUTDTSEL nor OUTDTH/OUTDL
registers. So your patch may break them.

> * Fixes tag here is pretty weak, nevertheless I suppose it is a fix in fact,

IMHO the SHA1 is not appropriate, as commit 119f5e448d32c ("gpio: Renesas
R-Car GPIO driver V3") added support for R-Car Gen1 only, while the OUTDT*
registers appeared in R-Car Gen2.

> * gpio_rcar_suspend()/gpio_rcar_resume() don't respect OUTDTSEL/OUTDTH/OUTDTL
>   values, if there is a reason to dump/restore registers, it might be good
>   to include them to the list also,

Given resume calls gpio_rcar_direction_{in,out}put(), at least OUTDTSEL
will be restored for output. Is that sufficient, or should it be restored for
input, too?

> * alternatively it might be possible to replace the original interface with
>   OUTDTH/OUTDTL one, it will be a nice valid fix also.

Unfortunately that is not supported by all SoCs supported by the driver.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds