Hi Geert,
On 4/27/20 10:41 PM, Geert Uytterhoeven wrote:
> Hi Vladimir,
>
> Sorry for taking so long to get back to you, after our chat at Embedded
> Recipes.
>
> On Tue, Dec 18, 2018 at 1:58 PM Vladimir Zapolskiy
> <vladimir_zapolskiy@xxxxxxxxxx> wrote:
>> 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.
>
> Actually this is what Biju's v1 did[1].
Nice to know, I'm still inclined to think that this version is correct.
> But making that works means adding support for multiple "gpio-ranges" to
> the gpio-rcar driver.
The thing is that the complete support of single or multiple "gpio-ranges"
property is done in gpiolib, so I would expect that a proper fix is about
to remove the code and reuse gpiolib rather than to add.
> Of course that can be done, but it will complicate things, as all "offset"
> parameters in GPIO controller callbacks would need to take into account
> the holes when converting from contiguous offsets to discontiguous
> register bits.
Well, it may happen that the gpio-rcar driver is one of the few, which do
the machinery by itself.
Do you see an option to replace in-house gpio_rcar_request()/gpio_rcar_free()
by gpiochip_generic_* functions?
>
> Note that the second tuple is <&pfc 27 123 3> not <&pfc 27 113 3>, i.e.
> the hole is not only present in the GPIO bank, but also in the pin
> controller's pin numbering (the PFC GPSR3 register has the same hole).
>
> Documentation/devicetree/bindings/gpio/gpio.txt says:
> | Some system-on-chips (SoCs) use the concept of GPIO banks. A GPIO bank is an
> | instance of a hardware IP core on a silicon die, usually exposed to the
> | programmer as a coherent range of I/O addresses. Usually each such bank is
> | exposed in the device tree as an individual gpio-controller node, reflecting
> | the fact that the hardware was synthesized by reusing the same IP block a
> | few times over.
>
> ... which applies to the R-Car GPIO block.
>
What do you call "the R-Car GPIO block" here?
I agree to the statement if "GPIO block" is replaced by "GPIO controller".
Here, and from my experience, the documentation correctly represents
the reality by claiming that
> Usually each such bank is exposed in the device tree as an individual
> gpio-controller node
and another "usually" in the paragraph
> usually exposed to the programmer as a coherent range of I/O addresses.
is less usual :)
Roughly speaking about MMIO GPIO controllers there should be a correspondence
between a GPIO bank and GPIO contoller's MMIO range.
> | Optionally, a GPIO controller may have a "ngpios" property. This property
> | indicates the number of in-use slots of available slots for GPIOs. The
> | typical example is something like this: the hardware register is 32 bits
> | wide, but only 18 of the bits have a physical counterpart. The driver is
> | generally written so that all 32 bits can be used, but the IP block is reused
> | in a lot of designs, some using all 32 bits, some using 18 and some using
> | 12. In this case, setting "ngpios = <18>;" informs the driver that only the
> | first 18 GPIOs, at local offset 0 .. 17, are in use.
>
> So far the R-Car GPIO DT bindings never had a need for the "ngpios"
> property. The same information can easily be extracted from the last
> cell of the (single) mandatory "gpio-ranges" value.
>
That's fine, please note that device tree bindings of GPIO controllers which
drivers rely on gpiochip_generic_request() do not introduce "ngpios" property
also by obvious reasons, the machinery is hidden inside gpiolib.
> | If these GPIOs do not happen to be the first N GPIOs at offset 0...N-1, an
> | additional set of tuples is needed to specify which GPIOs are unusable, with
> | the gpio-reserved-ranges binding. This property indicates the start and size
> | of the GPIOs that can't be used.
>
> This also matches the case on RZ/G1C.
> So IMHO using "gpio-reserved-ranges" is appropriate for RZ/G1C.
I disagree, there is no any indicator that "gpio-reserved-ranges" here is
justified.
> I do think the commit description of commit b9c725ed73b7cecc
> ("dt-bindings: gpio: Add a gpio-reserved-ranges property"):
>
> | Some qcom platforms make some GPIOs or pins unavailable for use
> | by non-secure operating systems, and thus reading or writing the
> | registers for those pins will cause access control issues.
> | Introduce a DT property to describe the set of GPIOs that are
> | available for use so that higher level OSes are able to know what
> | pins to avoid reading/writing.
>
> is confusing, as it only talks about pins that can be used by a secure
> OS only, but the actual changes to gpio.txt are in-line with GPIO lines
> that are simply not wired to physical pins, which is what the original
> paragraph was talking about, too.
> What do other people think?
For what it's worth in my opinion "gpio-reserved-ranges" has a quite clear
and nonambiquous meaning, but special care, and of course a clarification
from maintainers, is needed to prevent a possible misusage of the property.
I'm an admirer of Occam's razor, and if "holes" in banks can be described
by a standard and wide spread "gpio-ranges" mechanism, then I see no
good enough reason to mix in a more exotic "gpio-reserved-ranges" property.
--
Best wishes,
Vladimir
