From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com [IPv6:2a00:1450:400c:c09::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E7D048210C for ; Mon, 13 Feb 2017 04:15:56 -0800 (PST) Received: by mail-wm0-x236.google.com with SMTP id v186so156433907wmd.0 for ; Mon, 13 Feb 2017 04:15:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=f4BW+RtnPdRYnjgMrjaK1UHUJsqSRuI5i/yjWA6jFv4=; b=GQitJInqdShLExUXxuHERv7WUMn5xcPGmMEHZesostAABJbmeQSScQsIql58OUfSEU eARszYFcwczp3PFuiMeQdgCxiaq+O2laarKpAhehWS1XBSyc6OIskAoROxJGgzsgjInd g9p5nqvF6Lvh7JFXFr9xDBSakGT79aMU7fVcw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=f4BW+RtnPdRYnjgMrjaK1UHUJsqSRuI5i/yjWA6jFv4=; b=FpPGQxvJRTO0GJQAL8F3iyeFZdpdBqa/93didX2Yh+9b2ETrv5NqZvmtBln763S66x Ha8jM68zjmLl5uutL4mfvBmB2wRVMxOjuqxoFwyW67fUWsLF0JIThUc0bpdRhDr8rHYP kRhsAAza/WS8s4aWbieoYg2tBWjxBP4W5RJx5bpkfq2GN+dtAjubKHl/8c96IYk1jl7S 8dmY72Y0DIf1d3XvrCUsrtu6ExhuhT0CGE4ZTavGcXyyzlFusffa8q7XGUyajewB/4/t BUERULUcDlyuKjcsoYr37pOezs7Fh74szS5PKLPpvvA53PuzqeVnp4RuSoEq6t825v7b PN4Q== X-Gm-Message-State: AMke39nLim/y8luEpk0Xq7K2iTIpRrdjsrAjGsn08X6giyTup8O3mgECFuI53NjR3eJPrFYW X-Received: by 10.28.148.76 with SMTP id w73mr38337795wmd.74.1486988155152; Mon, 13 Feb 2017 04:15:55 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id e6sm13672402wrc.30.2017.02.13.04.15.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Feb 2017 04:15:54 -0800 (PST) Date: Mon, 13 Feb 2017 12:15:52 +0000 From: Leif Lindholm To: evan.lloyd@arm.com Cc: edk2-devel@ml01.01.org, Ard Biesheuvel , Ryan Harkin Message-ID: <20170213121552.GU16034@bivouac.eciton.net> References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> MIME-Version: 1.0 In-Reply-To: <20170209192623.262044-5-evan.lloyd@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Feb 2017 12:15:57 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 09, 2017 at 07:26:23PM +0000, evan.lloyd@arm.com wrote: > From: Girish Pathak > > This change implements GetTriggerType and SetTriggerType functions > in ArmGicV2Dxe (GicV2GetTriggerType/GicV2SetTriggerType) > and ArmGicV3Dxe (GicV3GetTriggerType/GicV3SetTriggerType) > > SetTriggerType configures the interrupt mode of an interrupt > as edge sensitive or level sensitive. > > GetTriggerType function returns the mode of an interrupt. > > The requirement for this change derives from a problem detected on ARM > Juno boards, but the change is of generic relevance. > > NOTE: At this point the GICv3 code is not tested. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Girish Pathak > Signed-off-by: Evan Lloyd > Tested-by: Girish Pathak (Tested-by: is usually considered to be implicit from the code author - its value comes when added by someone else.) > --- > ArmPkg/Include/Library/ArmGicLib.h | 4 + > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 165 ++++++++++++++++++-- > ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 159 +++++++++++++++++-- > 3 files changed, 308 insertions(+), 20 deletions(-) > > diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h > index 4364f3ffef464596f64cf59881d703cf54cf0ddd..6610f356c20e73d84ff3ba519956b426d97ef1eb 100644 > --- a/ArmPkg/Include/Library/ArmGicLib.h > +++ b/ArmPkg/Include/Library/ArmGicLib.h > @@ -51,6 +51,10 @@ > #define ARM_GIC_ICDDCR_ARE (1 << 4) // Affinity Routing Enable (ARE) > #define ARM_GIC_ICDDCR_DS (1 << 6) // Disable Security (DS) > > +// GICD_ICDICFR bits > +#define ARM_GIC_ICDICFR_LEVEL_TRIGGERED 0x0 // Level triggered interrupt > +#define ARM_GIC_ICDICFR_EDGE_TRIGGERED 0x1 // Edge triggered interrupt > + > // > // GIC Redistributor > // > diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > index 8c4d66125e2e8c7af9898f336ee742ed0aebf058..1f47403c6cdc7e8c0f6ac65d3b95a562da6a2d32 100644 > --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > @@ -29,6 +29,7 @@ Abstract: > #define ARM_GIC_DEFAULT_PRIORITY 0x80 > > extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol; > +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol; > > STATIC UINT32 mGicInterruptInterfaceBase; > STATIC UINT32 mGicDistributorBase; > @@ -193,19 +194,95 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol = { > GicV2EndOfInterrupt > }; > > +/** > + Calculate GICD_ICFGRn base address and corresponding bit > + field Int_config[1] of the GIC distributor register. > + > + @param Source Hardware source of the interrupt. > + @param RegAddress Corresponding GICD_ICFGRn base address. > + @param BitNumber Bit number in the register to set/reset. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > STATIC > EFI_STATUS > +GicGetDistributorIntrCfgBaseAndBitField ( I don't see Interrupt generally truncated to Intr in this driver. Since what is being looked for is the the ICFG, why not just call it GicGetDistributorICfgBaseAndBitField? > + IN HARDWARE_INTERRUPT_SOURCE Source, > + OUT UINTN *RegAddress, > + OUT UINTN *BitNumber > + ) > +{ > + UINTN RegOffset; > + UINTN Field; > + > + if (Source >= mGicNumInterrupts) { > + ASSERT(Source < mGicNumInterrupts); > + return EFI_UNSUPPORTED; > + } > + > + RegOffset = Source / 16; > + Field = Source % 16; > + *RegAddress = PcdGet64 (PcdGicDistributorBase) > + + ARM_GIC_ICDICFR > + + (4 * RegOffset); > + *BitNumber = (Field * 2) + 1; A lot of magic values above - can this be improved with some added #defines in ArmGicLib.h? > + > + return EFI_SUCCESS; > +} > + > +/** > + Get interrupt trigger type of an interrupt > + > + @param This Instance pointer for this protocol > + @param Source Hardware source of the interrupt. > + @param TriggerType Returns interrupt trigger type. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > +EFI_STATUS > EFIAPI > GicV2GetTriggerType ( > IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This, > - IN HARDWARE_INTERRUPT_SOURCE Source, > + IN HARDWARE_INTERRUPT_SOURCE Source, Cosmetic whitespace change only. > OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType > ) > { > + UINTN RegAddress; > + UINTN BitNumber; > + EFI_STATUS Status; > + > + RegAddress = 0; > + BitNumber = 0; > + > + Status = GicGetDistributorIntrCfgBaseAndBitField ( > + Source, > + &RegAddress, > + &BitNumber > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) > + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH > + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; > + Ternaries are excellent when they increase code readability. I am not convinced that is the case here. Consider: if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) { *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH; } else { *TriggerType = EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; } ? The versions generate identical code with gcc at -O1 and above. > return EFI_SUCCESS; > } > > -STATIC > +/** > + Set interrupt trigger type of an interrupt > + > + @param This Instance pointer for this protocol > + @param Source Hardware source of the interrupt. > + @param TriggerType Interrupt trigger type. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > EFI_STATUS > EFIAPI > GicV2SetTriggerType ( > @@ -214,20 +291,83 @@ GicV2SetTriggerType ( > IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType > ) > { > + UINTN RegAddress = 0; > + UINTN BitNumber = 0; > + UINT32 Value; > + EFI_STATUS Status; > + BOOLEAN IntrSourceEnabled; "Interrupt" isn't truncated in variable/function names elsewhere in this function. If you want to abbreviate the name - how about just calling it SourceEnabled? > + > + if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING > + && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) { Should that && not line up with the 'T' rather than the '('? > + DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \ > + TriggerType)); > + ASSERT (FALSE); > + return EFI_UNSUPPORTED; > + } > + > + Status = GicGetDistributorIntrCfgBaseAndBitField ( > + Source, > + &RegAddress, > + &BitNumber > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = GicV2GetInterruptSourceState ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source, > + &IntrSourceEnabled > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING) > + ? ARM_GIC_ICDICFR_EDGE_TRIGGERED > + : ARM_GIC_ICDICFR_LEVEL_TRIGGERED; Again, consider if/else. > + > + // > + // Before changing the value, we must disable the interrupt, > + // otherwise GIC behavior is UNPREDICTABLE. > + // > + if (IntrSourceEnabled) { > + GicV2DisableInterruptSource ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source > + ); > + } > + > + MmioAndThenOr32 ( > + RegAddress, > + ~(0x1 << BitNumber), > + Value << BitNumber > + ); > + // > + // Restore interrupt state > + // > + if (IntrSourceEnabled) { > + GicV2EnableInterruptSource ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source > + ); > + } > + > return EFI_SUCCESS; > } > > -STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = { > - (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource, > - (HARDWARE_INTERRUPT2_ENABLE)GicV2EnableInterruptSource, > - (HARDWARE_INTERRUPT2_DISABLE)GicV2DisableInterruptSource, > - (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV2GetInterruptSourceState, > - (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV2EndOfInterrupt, > +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V2Protocol = { > + (HARDWARE_INTERRUPT2_REGISTER) RegisterInterruptSource, > + (HARDWARE_INTERRUPT2_ENABLE) GicV2EnableInterruptSource, > + (HARDWARE_INTERRUPT2_DISABLE) GicV2DisableInterruptSource, > + (HARDWARE_INTERRUPT2_INTERRUPT_STATE) GicV2GetInterruptSourceState, > + (HARDWARE_INTERRUPT2_END_OF_INTERRUPT) GicV2EndOfInterrupt, Apart from the dropped STATIC, This is a cosmetic whitespace-only change that also seems incorrect to me. > GicV2GetTriggerType, > GicV2SetTriggerType > }; > > - > /** > Shutdown our hardware > > @@ -346,8 +486,11 @@ GicV2DxeInitialize ( > ArmGicEnableDistributor (mGicDistributorBase); > > Status = InstallAndRegisterInterruptService ( > - &gHardwareInterruptV2Protocol, &gHardwareInterrupt2V2Protocol, > - GicV2IrqInterruptHandler, GicV2ExitBootServicesEvent); > + &gHardwareInterruptV2Protocol, > + &gHardwareInterrupt2V2Protocol, > + GicV2IrqInterruptHandler, > + GicV2ExitBootServicesEvent This is a whitespace-only change. > + ); > > return Status; > } > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > index 02deeef78b6d7737172a5992c6decac43cfdd64a..a0383ecd7738750f73a2253811403d6ed0d2fd51 100644 > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > @@ -19,6 +19,7 @@ > #define ARM_GIC_DEFAULT_PRIORITY 0x80 > > extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol; > +extern EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol; > > STATIC UINTN mGicDistributorBase; > STATIC UINTN mGicRedistributorsBase; > @@ -184,8 +185,54 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV3Protocol = { > GicV3EndOfInterrupt > }; > > +/** > + Calculate GICD_ICFGRn base address and corresponding bit > + field Int_config[1] in the GIC distributor register. > + > + @param Source Hardware source of the interrupt. > + @param RegAddress Corresponding GICD_ICFGRn base address. > + @param BitNumber Bit number in the register to set/reset. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > STATIC > EFI_STATUS > +GicGetDistributorIntrCfgBaseAndBitField ( ICfg? > + IN HARDWARE_INTERRUPT_SOURCE Source, > + OUT UINTN *RegAddress, > + OUT UINTN *BitNumber > + ) > +{ > + UINTN RegOffset; > + UINTN Field; > + > + if (Source >= mGicNumInterrupts) { > + ASSERT(FALSE); > + return EFI_UNSUPPORTED; > + } > + > + RegOffset = Source / 16; > + Field = Source % 16; > + *RegAddress = PcdGet64 (PcdGicDistributorBase) > + + ARM_GIC_ICDICFR > + + (4 * RegOffset); > + *BitNumber = (Field * 2) + 1; A lot of magic numbers above. Can they be improved with some added #defines in ArmGicLib.h? > + > + return EFI_SUCCESS; > +} > + > +/** > + Get interrupt trigger type of an interrupt > + > + @param This Instance pointer for this protocol > + @param Source Hardware source of the interrupt. > + @param TriggerType Returns interrupt trigger type. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > +EFI_STATUS > EFIAPI > GicV3GetTriggerType ( > IN EFI_HARDWARE_INTERRUPT2_PROTOCOL *This, > @@ -193,10 +240,37 @@ GicV3GetTriggerType ( > OUT EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE *TriggerType > ) > { > + UINTN RegAddress = 0; > + UINTN BitNumber = 0; > + EFI_STATUS Status; > + > + Status = GicGetDistributorIntrCfgBaseAndBitField ( > + Source, > + &RegAddress, > + &BitNumber > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + *TriggerType = (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) == 0) > + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH > + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; > + Consider if/else? > return EFI_SUCCESS; > } > > -STATIC > +/** > + Set interrupt trigger type of an interrupt > + > + @param This Instance pointer for this protocol > + @param Source Hardware source of the interrupt. > + @param TriggerType Interrupt trigger type. > + > + @retval EFI_SUCCESS Source interrupt supported. > + @retval EFI_UNSUPPORTED Source interrupt is not supported. > +**/ > EFI_STATUS > EFIAPI > GicV3SetTriggerType ( > @@ -205,15 +279,79 @@ GicV3SetTriggerType ( > IN EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE TriggerType > ) > { > + UINTN RegAddress; > + UINTN BitNumber; > + UINT32 Value; > + EFI_STATUS Status; > + BOOLEAN IntrSourceEnabled; Consider SourceEnabled? > + > + if (TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING > + && TriggerType != EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH) { Line up with 'T' instead of '('? > + DEBUG ((EFI_D_ERROR, "Invalid interrupt trigger type: %d\n", \ > + TriggerType)); This lines up differently from in the v2 driver (which I would argue gets it right). > + ASSERT (FALSE); > + return EFI_UNSUPPORTED; > + } > + > + Status = GicGetDistributorIntrCfgBaseAndBitField ( > + Source, > + &RegAddress, > + &BitNumber > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = GicV3GetInterruptSourceState ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source, > + &IntrSourceEnabled > + ); > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Value = (TriggerType == EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING) > + ? ARM_GIC_ICDICFR_EDGE_TRIGGERED > + : ARM_GIC_ICDICFR_LEVEL_TRIGGERED; Consider if/else? > + > + // > + // Before changing the value, we must disable the interrupt, > + // otherwise GIC behavior is UNPREDICTABLE. > + // > + if (IntrSourceEnabled) { > + GicV3DisableInterruptSource ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source > + ); > + } > + > + MmioAndThenOr32 ( > + RegAddress, > + ~(0x1 << BitNumber), > + Value << BitNumber > + ); > + // > + // Restore interrupt state > + // > + if (IntrSourceEnabled) { > + GicV3EnableInterruptSource ( > + (EFI_HARDWARE_INTERRUPT_PROTOCOL*)This, > + Source > + ); > + } > + > return EFI_SUCCESS; > } > > -STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = { > - (HARDWARE_INTERRUPT2_REGISTER)RegisterInterruptSource, > - (HARDWARE_INTERRUPT2_ENABLE)GicV3EnableInterruptSource, > - (HARDWARE_INTERRUPT2_DISABLE)GicV3DisableInterruptSource, > - (HARDWARE_INTERRUPT2_INTERRUPT_STATE)GicV3GetInterruptSourceState, > - (HARDWARE_INTERRUPT2_END_OF_INTERRUPT)GicV3EndOfInterrupt, > +EFI_HARDWARE_INTERRUPT2_PROTOCOL gHardwareInterrupt2V3Protocol = { > + (HARDWARE_INTERRUPT2_REGISTER) RegisterInterruptSource, > + (HARDWARE_INTERRUPT2_ENABLE) GicV3EnableInterruptSource, > + (HARDWARE_INTERRUPT2_DISABLE) GicV3DisableInterruptSource, > + (HARDWARE_INTERRUPT2_INTERRUPT_STATE) GicV3GetInterruptSourceState, > + (HARDWARE_INTERRUPT2_END_OF_INTERRUPT) GicV3EndOfInterrupt, Apart from dropped STATIC, a whitespace-only change. > GicV3GetTriggerType, > GicV3SetTriggerType > }; > @@ -365,8 +503,11 @@ GicV3DxeInitialize ( > ArmGicEnableDistributor (mGicDistributorBase); > > Status = InstallAndRegisterInterruptService ( > - &gHardwareInterruptV3Protocol, &gHardwareInterrupt2V3Protocol, > - GicV3IrqInterruptHandler, GicV3ExitBootServicesEvent); > + &gHardwareInterruptV3Protocol, > + &gHardwareInterrupt2V3Protocol, > + GicV3IrqInterruptHandler, > + GicV3ExitBootServicesEvent > + ); Whitespace-only change. > > return Status; > } > -- > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >