Messages in this thread | | | Subject | Re: [RFT PATCH v3 06/27] dt-bindings: timer: arm,arch_timer: Add interrupt-names support | From | Hector Martin <> | Date | Wed, 10 Mar 2021 05:28:14 +0900 |
| |
On 10/03/2021 01.11, Rob Herring wrote: > On Mon, Mar 8, 2021 at 3:42 PM Marc Zyngier <maz@kernel.org> wrote: >> >> On Mon, 08 Mar 2021 20:38:41 +0000, >> Rob Herring <robh@kernel.org> wrote: >>> >>> On Fri, Mar 05, 2021 at 06:38:41AM +0900, Hector Martin wrote: >>>> Not all platforms provide the same set of timers/interrupts, and Linux >>>> only needs one (plus kvm/guest ones); some platforms are working around >>>> this by using dummy fake interrupts. Implementing interrupt-names allows >>>> the devicetree to specify an arbitrary set of available interrupts, so >>>> the timer code can pick the right one. >>>> >>>> This also adds the hyp-virt timer/interrupt, which was previously not >>>> expressed in the fixed 4-interrupt form. >>>> >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> --- >>>> .../devicetree/bindings/timer/arm,arch_timer.yaml | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml >>>> index 2c75105c1398..ebe9b0bebe41 100644 >>>> --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml >>>> +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml >>>> @@ -34,11 +34,25 @@ properties: >>>> - arm,armv8-timer >>>> >>>> interrupts: >>>> + minItems: 1 >>>> + maxItems: 5 >>>> items: >>>> - description: secure timer irq >>>> - description: non-secure timer irq >>>> - description: virtual timer irq >>>> - description: hypervisor timer irq >>>> + - description: hypervisor virtual timer irq >>>> + >>>> + interrupt-names: >>>> + minItems: 1 >>>> + maxItems: 5 >>>> + items: >>>> + enum: >>>> + - phys-secure >>>> + - phys >>>> + - virt >>>> + - hyp-phys >>>> + - hyp-virt >>> >>> phys-secure and hyp-phys is not very consistent. secure-phys or sec-phys >>> instead? >>> >>> This allows any order which is not ideal (unfortunately json-schema >>> doesn't have a way to define order with optional entries in the middle). >>> How many possible combinations are there which make sense? If that's a >>> reasonable number, I'd rather see them listed out. >> >> The available of interrupts are a function of the number of security >> states, privileged exception levels and architecture revisions, as >> described in D11.1.1: >> >> <quote> >> - An EL1 physical timer. >> - A Non-secure EL2 physical timer. >> - An EL3 physical timer. >> - An EL1 virtual timer. >> - A Non-secure EL2 virtual timer. >> - A Secure EL2 virtual timer. >> - A Secure EL2 physical timer. >> </quote> >> >> * Single security state, EL1 only, ARMv7 & ARMv8.0+ (assumed NS): >> - physical, virtual >> >> * Single security state, EL1 + EL2, ARMv7 & ARMv8.0 (assumed NS) >> - physical, virtual, hyp physical >> >> * Single security state, EL1 + EL2, ARMv8.1+ (assumed NS) >> - physical, virtual, hyp physical, hyp virtual >> >> * Two security states, EL1 + EL3, ARMv7 & ARMv8.0+: >> - secure physical, physical, virtual >> >> * Two security states, EL1 + EL2 + EL3, ARMv7 & ARMv8.0 >> - secure physical, physical, virtual, hyp physical >> >> * Two security states, EL1 + EL2 + EL3, ARMv8.1+ >> - secure physical, physical, virtual, hyp physical, hyp virtual >> >> * Two security states, EL1 + EL2 + S-EL2 + EL3, ARMv8.4+ >> - secure physical, physical, virtual, hyp physical, hyp virtual, >> secure hyp physical, secure hyp virtual >> >> Nobody has seen the last combination in the wild (that is, outside of >> a SW model). >> >> I'm really not convinced we want to express this kind of complexity in >> the binding (each of the 7 cases), specially given that we don't >> encode the underlying HW architecture level or number of exception >> levels anywhere, and have ho way to validate such information. > > Actually, we can simplify this down to 2 cases: > > oneOf: > - minItems: 2 > items: > - const: phys > - const: virt > - const: hyp-phys > - const: hyp-virt > - minItems: 3 > items: > - const: sec-phys > - const: phys > - const: virt > - const: hyp-phys > - const: hyp-virt > - const: sec-hyp-phy > - const: sec-hyp-virt > > And that's below my threshold for not worth the complexity.
This makes sense. Since we aren't using the sec-hyp stuff here, and those go at the end of the list, we can omit them from this patch for now and add them whenever they're needed for a platform. Does that sound OK?
-- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub
| |