Latest Xilinx-AMD System Device Tree flow update

[bentheredonethat]

This patch enables library and demo builds using latest Xilinx-AMD System Device (SDT) Flow update.

[arnopo]

Please tell me if I'm wrong but seems here that your issue is that XPAR_SCUGIC_0_DEVICE_ID and XPAR_SCUGIC_0_DIST_BASEADDR are not defined.
in such case you perhaps the code should be:

- #ifdef XPAR_SCUGIC_SINGLE_DEVICE_ID
+ #ifdef XPAR_SCUGIC_0_DEVICE_ID
#define XPAR_SCUGIC_0_DEVICE_ID XPAR_SCUGIC_SINGLE_DEVICE_ID
#endif

- #ifdef XPAR_SCUGIC_DIST_BASEADDR
+ #ifndef XPAR_SCUGIC_0_DIST_BASEADDR
#define XPAR_SCUGIC_0_DIST_BASEADDR XPAR_SCUGIC_DIST_BASEADDR
#endif

Then I assume that the 0 in the definition is the remoteproc instance. What's happen if you need more instances?

[bentheredonethat]

I am not sure what this comment means, can you explain?

I can re-work to instead be:
`
#ifndef XPAR_SCUGIC_0_DEVICE_ID

#define XPAR_SCUGIC_0_DEVICE_ID XPAR_SCUGIC_SINGLE_DEVICE_ID

#endif

#ifndef XPAR_SCUGIC_0_DIST_BASEADDR

#define XPAR_SCUGIC_0_DIST_BASEADDR XPAR_SCUGIC_DIST_BASEADDR

#endif
`

is that ok?

[arnopo]

my comment concatenates 2 points:

1. use the ifndef XPAR_SCUGIC_0_DEVICE_ID and ifndef XPAR_SCUGIC_0_DIST_BASEADDR
   you answer to it.
2. The second point is probably more a concern/question. What does the 0 mean in XPAR_SCUGIC_0_DEVICE_ID? It looks to me that it represents the remoteproc instance.
   In such case my question is are you able to manage 2 remote processors in your system DT?
   If yes you would have to define also XPAR_SCUGIC_1_DEVICE_ID and XPAR_SCUGIC_1_DIST_BASEADDR, right?

[bentheredonethat]

Thank you for clarification!
1 - will do.
2 - This is due to our internal-tooling's dynamic allocation of symbols. For example in our tooling a user could in theory add X GIC instances to their design. In this case the default, first GIC instance 0-based is IP_0, IP_1 ... and so on for further instances of any IP added.

Does that make sense? To clarify these are other symbols that the BSP provides that we don't control.

[bentheredonethat]

my comment concatenates 2 points:

1. use the ifndef XPAR_SCUGIC_0_DEVICE_ID and ifndef XPAR_SCUGIC_0_DIST_BASEADDR
   you answer to it.
2. The second point is probably more a concern/question. What does the 0 mean in XPAR_SCUGIC_0_DEVICE_ID? It looks to me that it represents the remoteproc instance.
   In such case my question is are you able to manage 2 remote processors in your system DT?
   If yes you would have to define also XPAR_SCUGIC_1_DEVICE_ID and XPAR_SCUGIC_1_DIST_BASEADDR, right?

updated with point 1's #ifndef's as suggested. confirmed compile step too with this.

[arnopo]

Thank you for clarification! 1 - will do. 2 - This is due to our internal-tooling's dynamic allocation of symbols. For example in our tooling a user could in theory add X GIC instances to their design. In this case the default, first GIC instance 0-based is IP_0, IP_1 ... and so on for further instances of any IP added.

Does that make sense? To clarify these are other symbols that the BSP provides that we don't control.

My concern behind my comment is that this file may not be reliable, as it depends on the system DT generation that, as you said, you do not control.
I would like to be sure that we do not received PR from users of your tool to add new definitions for each instance of IP added.

[bentheredonethat]

That is a valid concern. A Few things for context:

• Today AMD-Xilinx has to support both the classic flow (original xlnx/sys.h) and new SDT flow (xlnx/sys_devicetree.h). Eventually the old way will be deprecated and at that point we will fold in the xlnx/sys_devicetree.h into the xlnx/sys.h.

• The above means the API's sys_irq_enable and sys_irq_disable in xlnx/sys.h may have to account for multiple GIC instances in the future regardless - this at least provides a path forward if we did have to account for such a problem. (I see this as unlikely due to below explanation)

• Adding GIc instances is not to my knowledge a common value-add proposition so it might be overkill to consider that corner-case since a user can use 1 GIC instance to map interrupts on a large number of managed cores.

[bentheredonethat]

@arnopo does this address your concerns?

[arnopo]

Thanks for the additional information.
Let's keep it like this for the moment and see how the system DT will impact libmetal in the future