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

[Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>]

Hi Geert,

On 12/16/2018 12:03 PM, Geert Uytterhoeven wrote:
> 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.

FWIW I've managed to find only R01UH0573EJ0100 Rev.1.00 R-Car D1 User’s
Manual (see Merlot evaluation board), and it describes OUTDTSEL/OUTDTH/OUTDTL
and BOTHEDGE registers, thus a GPIO controller on this R-Car Gen1 SoC looks
similar to GPIO controllers on R-Car Gen2/Gen3 SoCs.

Thank you for clarification about R-Car M1A and H1 GPIO controllers, my v2
is in progress.

>> * 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?

Hmm, I was reflecting on necessity to save/restore OUTDTSEL value as a whole
independently of per line gpiochip_line_is_valid() value, but let's omit it.

I'm still influenced by a use-case of competing access to a GPIO controller
from two OSes, there might be an overlapping with Linux PM routines in
the driver.

As a side note I'm not convinced that gpiochip_line_is_valid() and
gpiochip->valid_mask usage in the driver is justified, unless it is agreed
that 'gpio-reserved-ranges' property is really supposed to describe "holes"
in GPIO controllers. The property found in r8a77470.dtsi (RZ/G1C) looks like
a kludge instead of making a proper assignment of 'gpio-ranges' property:

-                       gpio-ranges = <&pfc 0 96 30>;
-                       gpio-reserved-ranges = <17 10>;
+                       gpio-ranges = <&pfc 0 96 17>, <&pfc 27 123 3>;

The change above is untested and I have no access to RZ/G1C manual, it is
shared just to demonstrate an alternative idea of describing holes.

>> * 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.

Would it be seen as beneficial to add support of a likely better interface
for modern SoCs? The associated complexity in the driver won't be drastic.

--
Best wishes,
Vladimir