examples: legacy_apps: xlnx: Remove xlnx_hw_to_bsp_irq()

[bentheredonethat]

The Lopper-generated amd_platform_info.h already accounts for BSP. So remove the unneeded function xlnx_hw_to_bsp_irq() which tries to account for IRQ system number difference from baremetal (generic) and FreeRTOS BSPs.

[tnmysh]

@bentheredonethat lopper is not widely used yet. So we should handle both cases i.e. with and without lopper.

Can we implement a way to know if generated header file is used or hard-code definition from the header file ?
I remember we had AMD_GENERATED macro passed previously. Can we use it?

[bentheredonethat]

Hi @tnmysh

yes its the AMD_GENERATED header file symbol, no?

In other words:
https://github.com/OpenAMP/openamp-system-reference/blob/main/examples/legacy_apps/machine/xlnx/CMakeLists.txt#L40

If we want some extra awareness without Lopper, why not push that to BSP logic? e.g. metal_xlnx_extension

[tnmysh]

Hi @tnmysh

yes its the AMD_GENERATED header file symbol, no?

In other words: https://github.com/OpenAMP/openamp-system-reference/blob/main/examples/legacy_apps/machine/xlnx/CMakeLists.txt#L40

If we want some extra awareness without Lopper, why not push that to BSP logic? e.g. metal_xlnx_extension

@bentheredonethat good point. Yes I had tried it. But that doesn't work because the zynqmp driver ops is also using the irq_info.
Here:

openamp-system-reference/examples/legacy_apps/machine/xlnx/zynqmp/zynqmp_linux_r5_proc.c

Line 138 in 4bdabbb

metal_irq_register(irq_vect, zynqmp_linux_r5_proc_irq_handler, rproc);

If we move irq conversion logic to BSP, then irq_info above is not passing correct irq information, and interrupt handler is not called.

[bentheredonethat]

Hi @tnmysh thanks for the comment -- I might be missing something but I dont think 'zynqmp' area applies.

• The routine xlnx_hw_to_bsp_irq () is not called in the 'zynqmp' area.
• The 'zynqmp' section is for Linux.
• The Lopper generated header is only used for baremetal/generic and FreeRTOS.

So it seems to me like this commit does not conflict at all but again I might be missing something here.

[tnmysh]

Hi @tnmysh thanks for the comment -- I might be missing something but I dont think 'zynqmp' area applies.

* The routine xlnx_hw_to_bsp_irq () is not called in the 'zynqmp' area.

* The 'zynqmp' section is for Linux.

This file is for r5: https://github.com/OpenAMP/openamp-system-reference/blob/4bdabbb5b6aaaa902f56d408c091dbefc11e62ca/examples/legacy_apps/machine/xlnx/zynqmp/zynqmp_linux_r5_proc.c

It maps irq_info interrupt number to isr routine in the *_init function.

* The Lopper generated header is only used for baremetal/generic and FreeRTOS.

So it seems to me like this commit does not conflict at all but again I might be missing something here.

[bentheredonethat]

@tnmysh Im still confused. Historically the 'zynqmp' subdir is for linux. you can see that a few ways:

1. it has a reference DTSI
2. i nthe file you reference it says 'Linux r5 remoteproc' where Linux is first https://github.com/OpenAMP/openamp-system-reference/blob/main/examples/legacy_apps/machine/xlnx/zynqmp/zynqmp_linux_r5_proc.c#L142
3. it is using printf (not xil_printf)
4. also we have used 'zynqmp' as convention for linux side in libmetal, openamp demos historically - i can provide pointers there.

also as further reasoning (said this in earlier comment) - there is no mention of the routine in the init function for this subdir https://github.com/OpenAMP/openamp-system-reference/blob/main/examples/legacy_apps/machine/xlnx/zynqmp/zynqmp_linux_r5_proc.c#L66

can you please clarify how the routine xlnx_hw_to_bsp_irq() is being called in this subdir or why this commit causes an issue?

I understand that xlnx_hw_to_bsp_irq() was setting up the IRQ INFO based on BSP. But for the zynqmp/linux subdir - this is not called.

The routine xlnx_hw_to_bsp_irq() is presently called only from xlnx_machine_init(). The routine xlnx_machine_init() is only called in https://github.com/OpenAMP/openamp-system-reference/blob/main/examples/legacy_apps/machine/xlnx/zynqmp_r5/platform_info.c. So from my looking through this - there is no link to the 'zynqmp' subdir that affects the removal of the xlnx_hw_to_bsp_irq() call.

Also the Lopper-flow is fully upstream and the README at https://github.com/OpenAMP/openamp-system-reference/tree/main/examples/legacy_apps/machine/xlnx shows how to generate the Lopper header file.

[tnmysh]

Hello @bentheredonethat ,

Thanks for pointing above mistake. I had posted link to a wrong file. Here is the actual file that is used for R5:

openamp-system-reference/examples/legacy_apps/machine/xlnx/zynqmp_r5/zynqmp_r5_a53_rproc.c

Line 88 in 4bdabbb

irq_vect = (uintptr_t)kick_dev->irq_info;

My concern was we can't move irq conversion from HW number to BSP number to metal extension because above place is using irq_info. And the conversion must happen before it.

We discussed offline, that we will be move to system-device-tree flow by default. And hence, old way of hardcoding of interrupt numbers are obsolete, and in that case this PR is valid.

I don't mind merging this PR once we have following:

1. If Vivado output XSA is accessible to everyone via public domain.

2. People have proper documentation on how to generate required header file from that XSA.
   2.a) XSA -> System-device-tree generation using sdt-gen.
   2.b) system-device-tree to amd_platform_info.h header file generation via lopper.

Once we have above two, I will merge this PR in the upstream repo.

Thank You.

[bentheredonethat]

Hi @tnmysh thanks for your comment

The README for the xlnx vendor shows this already -- https://github.com/OpenAMP/openamp-system-reference/tree/main/examples/legacy_apps/machine/xlnx

Is that sufficient?