Xilinx Zynq GIC Usage V8.2.2

razed11 wrote on Thursday, October 01, 2015:

In portZynq7000.c there is a routine to configure the RTOS timer and install its handler. It declares a static instance of XScuGIC so it is only available to this function.

In the 7.0.2 port (used by our current product) the instance of XScuGIC was declared static at file scope and routines were provided as an interface so that the application could install other interrupts using this same instance.

Why the change? Is it OK for the application create another instance and call XScuGic_CfgInitialize() and add interrupts separately?

Thanks.

From 8.2.2 port:

void FreeRTOS_SetupTickInterrupt( void )
{
static XScuGic xInterruptController; 	/* Interrupt controller instance */
BaseType_t xStatus;
extern void FreeRTOS_Tick_Handler( void );
XScuTimer_Config *pxTimerConfig;
XScuGic_Config *pxGICConfig;
const uint8_t ucRisingEdge = 3;

	/* This function is called with the IRQ interrupt disabled, and the IRQ
	interrupt should be left disabled.  It is enabled automatically when the
	scheduler is started. */

	/* Ensure XScuGic_CfgInitialize() has been called.  In this demo it has
	already been called from prvSetupHardware() in main(). */
	pxGICConfig = XScuGic_LookupConfig( XPAR_SCUGIC_SINGLE_DEVICE_ID );
	xStatus = XScuGic_CfgInitialize( &xInterruptController, pxGICConfig, pxGICConfig->CpuBaseAddress );

From 7.0.2 port:

void setupIRQhandler(int int_no, void *fce, void *param) {
	int Status;

	clearIRQhandler(int_no);

	Status = XScuGic_Connect(&InterruptController, int_no,
			(Xil_ExceptionHandler) fce, param);
	if (Status != XST_SUCCESS) {
		return;
	}
	XScuGic_ClearPending(InterruptController.Config->DistBaseAddress, int_no);
	XScuGic_Enable(&InterruptController, int_no);
}

rtel wrote on Friday, October 02, 2015:

The code you highlight is in a callback function, so you can change its implementation if you like, however looking at the demo from the FreeRTOS download I see that there is still a global variable called xInterruptController which you can use - and indeed it is already being used to install interrupt handlers for the UART and the timers that generate the interrupts used by the interrupt nesting tests. So, in this case, the static declaration used in vConfigureTickInterrupt() is actually a duplicate, and perhaps obsolete.

If you look at prvSetupHardware() in main.c you will find the following code:

	/* Install a default handler for each GIC interrupt. */
	xStatus = XScuGic_CfgInitialize( &xInterruptController, pxGICConfig, pxGICConfig->CpuBaseAddress );

…where xInterruptController is declared as a global in main.c.

Regards.

razed11 wrote on Friday, October 02, 2015:

I think that duplicate is being used here and the global used in the other code. I’m pretty sure C would resolve that local name before the global (though you’d think a warning would be thrown).

I took a cursory look at CfgInitialize() for the V3.0 driver. My concern is that the handler table will reinitialized if this is called twice. If we have two instances of the XScuGIC then IsReady will be false and the it will go on to initialize the table. Since it checks if the handlers are NULL before assigning the stub that looks OK. The problem is that clobbers the callback reference which it seems should be in the if block.

I can fix this on my end by moving that callback to my code as you suggest. Thanks for the suggestion.

s32  XScuGic_CfgInitialize(XScuGic *InstancePtr,
				XScuGic_Config *ConfigPtr,
				u32 EffectiveAddr)
{
	u32 Int_Id;
	u32 Cpu_Id = (u32)XPAR_CPU_ID + (u32)1;
	(void) EffectiveAddr;

	Xil_AssertNonvoid(InstancePtr != NULL);
	Xil_AssertNonvoid(ConfigPtr != NULL);

	if(InstancePtr->IsReady != XIL_COMPONENT_IS_READY) {

		InstancePtr->IsReady = 0;
		InstancePtr->Config = ConfigPtr;


		for (Int_Id = 0U; Int_Id<XSCUGIC_MAX_NUM_INTR_INPUTS;Int_Id++) {
			/*
			* Initalize the handler to point to a stub to handle an
			* interrupt which has not been connected to a handler. Only
			* initialize it if the handler is 0 which means it was not
			* initialized statically by the tools/user. Set the callback
			* reference to this instance so that unhandled interrupts
			* can be tracked.
			*/
			if 	((InstancePtr->Config->HandlerTable[Int_Id].Handler == NULL)) {
				InstancePtr->Config->HandlerTable[Int_Id].Handler =
									StubHandler;
			}
			InstancePtr->Config->HandlerTable[Int_Id].CallBackRef =
								InstancePtr;
		}

		DistributorInit(InstancePtr, Cpu_Id);
		CPUInitialize(InstancePtr);

		InstancePtr->IsReady = XIL_COMPONENT_IS_READY;
	}

	return XST_SUCCESS;
}

razed11 wrote on Saturday, October 03, 2015:

It probably helps to understand that I’m trying to use FreeRTOS as a Xilinx SDK repository but since I cannot override that routine (there is no weakly defined vConfigureTickInterrupt in V8.2.2) then I have to copy or implement portZynq.c to fix this issue. Then I have to exclude that source from the project.

Per my other post, to fix the calling convention of the tick routine I need to alter port.c. So the repository route is a fail for me.

The fixes I think are to make vConfigureTickInterrupt and something like vClearTickInterrupt as weak. Another macro can be used to define the signature of the tick handler. Borrowing the global GIC structure doesn’t work for me as I’m using a C++ class to manage it.

rtel wrote on Saturday, October 03, 2015:

It probably helps to understand that I’m trying to use FreeRTOS as a Xilinx SDK repository

Right - that will by why the variable is declared within the function then, rather than as a global.

I cannot override that routine (there is no weakly defined vConfigureTickInterrupt in V8.2.2)

I have just made FreeRTOS_SetupTickInterrupt() and FreeRTOS_ClearTickInterrupt() weak symbols in portZynq7000.c, so that will get incorporated into the next release. In the mean time, you can make the change in the file within the repo directory, then regenerate the repository source files to have the same change:

void FreeRTOS_SetupTickInterrupt( void )  __attribute__ ( ( weak ) );
void FreeRTOS_ClearTickInterrupt( void )  __attribute__ ( ( weak ) );

Per my other post, to fix the calling convention of the tick routine I need to alter port.c

I will reply to that in your other post.

I think that duplicate is being used here and the global used in the other code. I’m pretty sure C would resolve that local name before the global (though you’d think a warning would be thrown).

I don’t think any variable name symbols are being duplicated, as inside portZynq7000.c the xInterruptController variable only has function scope - not even file scope.

The problem is that clobbers the callback reference which it seems should be in the if block

I’m not sure about this one either:

  1. InstancePtr->Config->HandlerTable[Int_Id].Handler is checked, and only overwritten if it is NULL, so a handler that is already installed will not be overwritten, so that is ok.
  2. InstancePtr->Config->HandlerTable[Int_Id].CallBackRef is set even if it has been set previously.

Therefore, if you application code has its own XScuGic variable, and installs interrupts using that variable, then it starts FreeRTOS - the XScuGic structure passed into the interrupt service routine will be the one statically declared inside FreeRTOS_SetupTickInterrupt(), rather than the one declared in the application. However the question is - does this matter? First, are you using the parameter inside the ISR (do you have more than one core processing these interrupts?), second are both XScuGic pointing to the same instance of the GIC anyway. If the XScuGic_LookupConfig( XPAR_SCUGIC_SINGLE_DEVICE_ID ); structure obtained using XScuGic_LookupConfig( XPAR_SCUGIC_SINGLE_DEVICE_ID ); (with the “single”) then I think both are pointing to the same place anyway.

I think the case where this won’t work is when there is more than one SCUGIC, but that is not a use case I have tried myself.

Thoughts? What would be difficult to do is have the FreeRTOS repository dependent on the application providing a global symbol. If it were the other way around, and the application was dependent on the repoitory providing a global symbol then the symbol would not be valid until after the RTOS had been started. [I can also talk to Xilinx about this if necessary]

richard_damon wrote on Saturday, October 03, 2015:

Having two InterruptController variables cause problems, as it gets a fresh config, so any interrupts added by code prior to creating the tick handler (which is anything created during main) will get erased and removed.

razed11 wrote on Monday, October 05, 2015:

Thanks for taking the time to discuss this.

The callbackref is unconditionally set to a pointer to the GIC structure which is what StubHandler expects but if I defined say a UART handler that makes use of the callback reference then it will be clobbered when the function-scoped version is initialized.

Here’s the SCUGIC code for driver 3.0. I think this is a bug. The handler is only overwritten if NULL but the member CallbackRef is overwritten for all interrupts. It should probably update both the callback reference and the handler if the handler is NULL.

		for (Int_Id = 0U; Int_Id<XSCUGIC_MAX_NUM_INTR_INPUTS;Int_Id++) {
			/*
			* Initalize the handler to point to a stub to handle an
			* interrupt which has not been connected to a handler. Only
			* initialize it if the handler is 0 which means it was not
			* initialized statically by the tools/user. Set the callback
			* reference to this instance so that unhandled interrupts
			* can be tracked.
			*/
			if 	((InstancePtr->Config->HandlerTable[Int_Id].Handler == NULL)) {
				InstancePtr->Config->HandlerTable[Int_Id].Handler =
									StubHandler;
			}
			InstancePtr->Config->HandlerTable[Int_Id].CallBackRef =
								InstancePtr;
		}

You are right that they are pointing to the same configuration structure but the issue above is the sticking point. Perhaps your changes to add the weak symbols and the fix to the driver are all that are required.

I’ll comment on the tick handler signature separately.

wonger wrote on Monday, November 30, 2015:

I ran into exactly this a few months ago:
https://sourceforge.net/p/freertos/discussion/382005/thread/17257557

What I did then was remove the static instance of xInterruptController and the call to XScuGic_CfgInitialize(). This was on v8.2.1, so the function was still vConfigureTickInterrupt() not FreeRTOS_SetupTickInterrupt(). And I used the global instance of xInterruptController from main.c instead.

This seems to fix things, but wondering if this is still the best way to resolve this now that this issue has been percolating a bit and has had more eyes on it.

edwards3 wrote on Monday, November 30, 2015:

Check out the version change description for version 8.2.3, there are some notes on this http://www.freertos.org/History.txt

wonger wrote on Monday, November 30, 2015:

I actually wrote a new forum post asking about the changes in v8.2.3, and submitted it twice, but it’s not appearing on the forum. Please allow me to ask it again here, since it’s become related:

FreeRTOS V8.2.3 has this change for which I’m interested in seeing the code modifications:

Zynq7000 port layer now declares the functions that setup and clear the
tick interrupt as weak symbols so they can be overridden by the
application, and uses a global XScuGic object so the same object can be
used by the application code

I found the commit for the first part (weak symbols) here:
https://sourceforge.net/p/freertos/code/2383/

I can’t find the code change for the second part (using a global XScuGic object). Can anyone help me track this one down? Thanks for your help.

rtel wrote on Monday, November 30, 2015:

These changes relate to the Zynq BSP, not the generic Cortex-A port. See the attached screenshot.

wonger wrote on Monday, November 30, 2015:

OK, thanks. So for example, in Zynq FreeRTOS Demo, the existing global instance of XScuGic (https://sourceforge.net/p/freertos/code/2383/tree/trunk/FreeRTOS/Demo/CORTEX_A9_Zynq_ZC702/RTOSDemo/src/main.c#l179) should go away and start using this BSP one?

rtel wrote on Tuesday, December 01, 2015:

The project in /FreeRTOS/Demo/CORTEX_A9_Zynq_ZC702 uses the generic ARM
Cortex-A9 port. The generic port does not contain anything Zynq
specific - instead the Zynq specific code is provided via callback
functions into the application code. Therefore the way the interrupt
controller is declared and used is part of the application. See the
source file FreeRTOS_tick_config.c in the project. As it is part of the
application you can either use the files exactly as supplied, or make
any edits you wish (like making a global interrupt controller object)
without effecting the RTOS source files - and without impacting your
ability to update the version of FreeRTOS being used in the future.

The files in /FreeRTOS/Demo/Xilinx_FreeRTOS_BSP is not a standalone
project, but a repository that allows the Xilinx SDK to create a
FreeRTOS application for you. Therefore it has to include all the
necessary code - both the generic FreeRTOS port layer, and the Zynq
specific code - and edits you make there will get lost if you later
recreate the BSP (by performing a clean build, etc.). So the files in
the Xilinx_FreeRTOS_BSP directory were updated to allow them to be
overridden by application code (hence the addition of the weak
attributes), and to use the interrupt controller object in a more user
friendly way.

Would it be helpful if the
/FreeRTOS/Demo/CORTEX_A9_Zynq_ZC702 project was edited too - so by
default (without any user edits) the application provided callbacks also
used a global interrupt controller?

Regards.

wonger wrote on Tuesday, December 01, 2015:

Yes, I think modifying the /FreeRTOS/Demo/CORTEX_A9_Zynq_ZC702 project to remove two instances of the interrupt controller would help, so that others don’t run into the situation I did where, out of the box, XScuGic_CfgInitialize() is called twice and overwrites some of my intended interrupt controller configuration.