| Message ID | 20180110015848.11480-3-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-01-09 at 17:58 -0800, Stephen Boyd wrote: > 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. > +- ngpios-ranges: > + Usage: optional > + Value type: <prop-encoded-array> > + Definition: Tuples of GPIO ranges (base, size) indicating > + GPIOs available for use. Can be name more particular? We have on one hand gpio-range-list for mapping, on the other this one might become generic. So, there are few options (at least?) to consider: 1) re-use gpio-ranges 2) add a valid property to gpio-ranges 3) rename ngpios-ranges to something like gpio-valid-ranges (I don't like it so much either, but for me it looks more descriptive than ngpios-ranges)
On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > 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. > > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> I like the idea, let's check what we think about the details regarding naming and semantics, I need feedback from some DT people in particular. Paging in Grant on this as he might have some input. > I stuck this inside msm8996, but maybe it can go somewhere more generic? Yeah just put it in Documentation/devicetree/bindings/gpio/gpio.txt Everyone and its dog doing GPIO reservations "from another world" will need to use this. > +- ngpios-ranges: > + Usage: optional > + Value type: <prop-encoded-array> > + Definition: Tuples of GPIO ranges (base, size) indicating > + GPIOs available for use. > + > Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for > a general description of GPIO and interrupt bindings. I like the tuples syntax. That's fine. It's like gpio-ranges we have already to map between pin controllers and GPIO. I don't think we can reuse gpio-ranges because that is exclusively for pin control ATM, it would be fine if the ranges were for a specific device, like pin control does, like: gpio-ranges = <&secure_world_thing 0 20 10>; But you definately would need a node to tie it to, so that the driver for that node can specify that it's gonna take the GPIOs. But I think the semantics should be the inverse. That you point out "holes" with the lines we *can't* use. We already support a generic property "ngpios" that says how many of the GPIOs (counted from zero) that can be used, so if those should be able to use this as a generic property it is better with the inverse semantics and say that the "reserved-gpio-ranges", "secureworld-gpio-ranges" (or whatever we decide to call it) takes precedence over ngpios so we don't end up in ambigous places. Then, will it be possible to put the parsing, handling and disablement of these ranges into drivers/gpio/gpiolib-of.c where we handle the ranges today, or do we need to do it in the individual drivers? Yours, Linus Walleij
On 01/10, Linus Walleij wrote: > On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > +- ngpios-ranges: > > + Usage: optional > > + Value type: <prop-encoded-array> > > + Definition: Tuples of GPIO ranges (base, size) indicating > > + GPIOs available for use. > > + > > Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for > > a general description of GPIO and interrupt bindings. > > I like the tuples syntax. That's fine. It's like gpio-ranges we have > already to map between pin controllers and GPIO. > > I don't think we can reuse gpio-ranges because that is > exclusively for pin control ATM, it would be fine if the ranges > were for a specific device, like pin control does, like: > > gpio-ranges = <&secure_world_thing 0 20 10>; > > But you definately would need a node to tie it to, so that the > driver for that node can specify that it's gonna take the > GPIOs. > > But I think the semantics should be the inverse. That you > point out "holes" with the lines we *can't* use. Ok. I can invert the logic and push it into the core part of the code. I'll leave the ACPI part in the msm driver. > > We already support a generic property "ngpios" that says how > many of the GPIOs (counted from zero) that can be used, > so if those should be able to use this as a generic property it > is better with the inverse semantics and say that the > "reserved-gpio-ranges", "secureworld-gpio-ranges" > (or whatever we decide to call it) takes precedence over > ngpios so we don't end up in ambigous places. > > Then, will it be possible to put the parsing, handling and > disablement of these ranges into drivers/gpio/gpiolib-of.c > where we handle the ranges today, or do we need to > do it in the individual drivers? > I'll cook that up right now to do the inverse thing in the gpiolib core code with a 'reserved-gpio-ranges' property. I haven't looked in much detail, but I would hope that it would work pretty easily. Should it be decoupled from the GPIOLIB_IRQCHIP config? If the idea is generic, then it may not be related to irq lines, but for the qcom driver it was all fine because all three concepts: irq, gpios, and pins have a one to one relationship. The only place it breaks down is if we have more pins than gpios, in which case I punted and just considered non-gpio pins as always valid.
On Wed, 2018-01-10 at 08:37 -0800, Stephen Boyd wrote: > On 01/10, Linus Walleij wrote: > > On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> > > wrote: > for the qcom driver it was all fine > because all three concepts: irq, gpios, and pins have a one to > one relationship. Just a side note: While it might be the case for most of the controllers, don't assume it's a generic case.
On Wed, Jan 10, 2018 at 1:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jan 10, 2018 at 2:58 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> 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. What level of access control is implemented here? Is there access control for each GPIO individually, or is it done by banks of GPIOs? Just asking to make sure I understand the problem domain. >> >> Cc: <devicetree@vger.kernel.org> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > I like the idea, let's check what we think about the details regarding > naming and semantics, I need feedback from some DT people > in particular. > > Paging in Grant on this as he might have some input. > >> I stuck this inside msm8996, but maybe it can go somewhere more generic? > > Yeah just put it in Documentation/devicetree/bindings/gpio/gpio.txt > Everyone and its dog doing GPIO reservations "from another world" > will need to use this. > >> +- ngpios-ranges: >> + Usage: optional >> + Value type: <prop-encoded-array> >> + Definition: Tuples of GPIO ranges (base, size) indicating >> + GPIOs available for use. >> + >> Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for >> a general description of GPIO and interrupt bindings. > > I like the tuples syntax. That's fine. It's like gpio-ranges we have > already to map between pin controllers and GPIO. > > I don't think we can reuse gpio-ranges because that is > exclusively for pin control ATM, it would be fine if the ranges > were for a specific device, like pin control does, like: > > gpio-ranges = <&secure_world_thing 0 20 10>; > > But you definately would need a node to tie it to, so that the > driver for that node can specify that it's gonna take the > GPIOs. > > But I think the semantics should be the inverse. That you > point out "holes" with the lines we *can't* use. > > We already support a generic property "ngpios" that says how > many of the GPIOs (counted from zero) that can be used, > so if those should be able to use this as a generic property it > is better with the inverse semantics and say that the > "reserved-gpio-ranges", "secureworld-gpio-ranges" > (or whatever we decide to call it) takes precedence over > ngpios so we don't end up in ambigous places. Heh, I just went down the same thought process on the naming before I read the above. Yes I agree. The property name should have something like "reserved" in it. I vote for "gpio-reserved-ranges" because it puts the binding owner (gpio) at the front of the name, it indicates that the list is unavailable GPIOs, and that the format is a set of ranges. The fiddly bit is it assumes the GPIOs are described by a single number. It works fine as long as the GPIO controllers can use a single cell to describe a gpio number (instead of having #gpio-cells = 3 with the first cell being bank, the second being number in bank, and the third being flags). > > Then, will it be possible to put the parsing, handling and > disablement of these ranges into drivers/gpio/gpiolib-of.c > where we handle the ranges today, or do we need to > do it in the individual drivers? I certainly would prefer parsing this in common code, and not in individual drivers, but again it becomes hard for any driver using multiple cells to describe the local GPIO number. I think the guidance here needs to be that the property is relevant when the internal GPIO number representation fits within a uint32, which realistically should never be a problem. g.
On 01/11/2018 10:33 AM, Grant Likely wrote: > What level of access control is implemented here? Is there access > control for each GPIO individually, or is it done by banks of GPIOs? > Just asking to make sure I understand the problem domain. On our ACPI system, it's specific GPIOs. Each GPIO is in its own 64k page, which is what allows us to block specific ones.
On Thu, Jan 11, 2018 at 4:36 PM, Timur Tabi <timur@codeaurora.org> wrote: > On 01/11/2018 10:33 AM, Grant Likely wrote: >> >> What level of access control is implemented here? Is there access >> control for each GPIO individually, or is it done by banks of GPIOs? >> Just asking to make sure I understand the problem domain. > > > On our ACPI system, it's specific GPIOs. Each GPIO is in its own 64k page, > which is what allows us to block specific ones. Okay, thanks. g. > > -- > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm > Technologies, Inc. Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt index aaf01e929eea..8354ab270486 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt @@ -40,6 +40,12 @@ MSM8996 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/gpio/gpio.h> +- ngpios-ranges: + Usage: optional + Value type: <prop-encoded-array> + Definition: Tuples of GPIO ranges (base, size) indicating + GPIOs available for use. + Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for a general description of GPIO and interrupt bindings.
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. Cc: <devicetree@vger.kernel.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- I stuck this inside msm8996, but maybe it can go somewhere more generic? Documentation/devicetree/bindings/pinctrl/qcom,msm8996-pinctrl.txt | 6 ++++++ 1 file changed, 6 insertions(+)