Hi,

Thank you once again for more of your comments.

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122 to

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6167

All the patches from the patch-set, are reviewed and all the comments are disposed-off till date.

Please share any further action, I need to work on.

Looking forward for your continued support.

Regards

Pankaj

From: Varun Wadekar <vwadekar@nvidia.com>
Sent: Tuesday, February 2, 2021 12:11 AM
To: Pankaj Gupta <pankaj.gupta@nxp.com>; Manish Pandey2 <Manish.Pandey2@arm.com>; Olivier Deprez <Olivier.Deprez@arm.com>; javier.almansasobrino@arm.com; jimmy.brisson@arm.com
Cc: Joanna Farley <Joanna.Farley@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Madhukar Pappireddy <Madhukar.Pappireddy@arm.com>
Subject: [EXT] RE: NXP Patch-Set for platform lx2160ardb/lx2160aqds/lx2162aqds

Caution: EXT Email

Thanks Pankaj. I don’t have further comments.

From: Pankaj Gupta <pankaj.gupta@nxp.com>
Sent: Sunday, January 31, 2021 11:14 PM
To: Varun Wadekar <vwadekar@nvidia.com>; Manish Pandey2 <Manish.Pandey2@arm.com>; Olivier Deprez <Olivier.Deprez@arm.com>; javier.almansasobrino@arm.com; jimmy.brisson@arm.com
Cc: Joanna Farley <Joanna.Farley@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Madhukar Pappireddy <Madhukar.Pappireddy@arm.com>
Subject: NXP Patch-Set for platform lx2160ardb/lx2160aqds/lx2162aqds

External email: Use caution opening links or attachments

Hi all,

Thanks to all of you, for your efforts in reviewing the patches.

All the patches from the patch-set, are reviewed and all the comments are disposed-off till date.

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122 to https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6167

Please share any further action, I need to work on.

Looking forward for your continued support.

Thanks & regards

Pankaj

From: Varun Wadekar <vwadekar@nvidia.com>
Sent: Friday, December 4, 2020 10:37 PM
To: Pankaj Gupta <pankaj.gupta@nxp.com>; Manish Pandey2 <Manish.Pandey2@arm.com>
Cc: Joanna Farley <Joanna.Farley@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Madhukar Pappireddy <Madhukar.Pappireddy@arm.com>
Subject: RE: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

Caution: EXT Email

Thanks Pankaj and Manish. Glad to see us agreeing to a path forward.

From: Pankaj Gupta <pankaj.gupta@nxp.com>
Sent: Thursday, December 3, 2020 11:39 PM
To: Manish Pandey2 <Manish.Pandey2@arm.com>; Varun Wadekar <vwadekar@nvidia.com>
Cc: Joanna Farley <Joanna.Farley@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Madhukar Pappireddy <Madhukar.Pappireddy@arm.com>
Subject: RE: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

External email: Use caution opening links or attachments

Hi,

Thanks for the email.

Your suggestion is good too. But with your suggestion, two lines are need for one line using the macro.

My concern is that, there are 6 SoC and 15+ platforms based on these 6 SoC, still pending to be added to this code base.

Keeping things simple in soc.mk and platform.mk is of key importance.

To achieve it:

Complexity of source file addition is moved away from platform makefile to:

o drivers/nxp/driver.mk, and

o drivers/nxp/<ip>/<ip>.mk

As a result, the soc.mk & platform.mk is less cluttered.

o With the single line addition based on this macro, it is easier to understand, which IP is part of BL2, BL31 or both.

o All the complexity about IP files to be included or not is moved to drivers/nxp/<ip>/<ip>.mk.

I got your point here.

Since it is very much specific to NXP platforms, it is to be moved to “plat/nxp”.

I have tried this way. It is working and ready for review.

Regards

Pankaj

From: Manish Pandey2 <Manish.Pandey2@arm.com>
Sent: Friday, December 4, 2020 6:24 AM
To: Varun Wadekar <vwadekar@nvidia.com>; Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Joanna Farley <Joanna.Farley@arm.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Madhukar Pappireddy <Madhukar.Pappireddy@arm.com>
Subject: Re: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

Caution: EXT Email

Hi Pankaj,

I am still trying to understand the complete patchset, also trying to understand different level of makefiles e.g drivers/nxp/drivers.mk(is it really needed?)

I think the main concern is complexity added by intermediate mk files, this complexity can be reduced by declaring most of things in specific platform mk file as we know at compile time which all images need a particular driver.

For e.g XSPI driver, it has same source files in all the 3 scenarios (BL2/BL31 & COMMON) and there is no usage of IMAGE_BL31, IMAGE_BL2 in xspi driver suggesting that Image type does not alters functionality of driver.

So, what we can do is keeping driver mk file simpler and other details in platform mk file.

flexspi_xor.mk will be like:

XSPI_BOOT_SOURCES += $(FLEXSPI_DRIVERS_PATH)/flexspi_nor.c \

${FLEXSPI_DRIVERS_PATH}/fspi.c

ifeq ($(DEBUG),1)

XSPI_BOOT_SOURCES += ${FLEXSPI_DRIVERS_PATH}/test_fspi.c

endif

Platform mk file will be like: (say only bl2 needs xspi)

XSPI_NEEDED := yes

bl2_sources += ${XSPI_BOOT_SOURCES}

Regarding NEED_BL31/NEED_BL2, these flags tell if binary for given BL stage needs to be generated or not.

Finally, I am not saying that your approach is wrong but what i suggested is currently done by most of the platforms.

Also, see my comments on your SET_FLAG patch, if we indeed decide to go ahead with your current approach.

Thanks

Manish

From: Varun Wadekar <vwadekar@nvidia.com>
Sent: 03 December 2020 17:35
To: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Manish Pandey2 <Manish.Pandey2@arm.com>
Subject: RE: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

Hi,

Can you point me to a change that uses the newly introduced makefile macro? IIUC, you just need a way to differentiate between a BL2 v BL31 build.

Manish can you confirm if NEED_BL31 and NEED_BL2 can be helpful in this case?

>> I will replace the SET_FLAG macro with these two flags from entire source tree.

I suppose you are alluding to the NXP platform port only. Correct?

-Varun

From: Pankaj Gupta <pankaj.gupta@nxp.com>
Sent: Thursday, December 3, 2020 12:57 AM
To: Varun Wadekar <vwadekar@nvidia.com>
Cc: Manish Pandey2 <Manish.Pandey2@arm.com>
Subject: RE: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

External email: Use caution opening links or attachments

Hi Varun,

Thanks for your email.

NXP make files are implemented as :

Each IP driver has its own .mk file.
Each platform has its own “platform.mk”.

Every “platform.mk” includes “soc.mk” for the SoC on which this platform is based.

Based on the SoC, mandatory IP(s) are included(soc.mk) to BL2 or BL31 or both.

This requires to pass the flag “<BL2 or BL31 or BL_COMM>_<IP>_NEEDED = yes” from soc.mk to mandatory IP(s) .mk.

But for certain IP(s), IP file sources inclusions to either BL2 or BL31, is based on optional features.

This also requires to pass the flag to IP(s) .mk.

For instance:

· If flexspi_nor as a boot-source, This macro sets 2 flags.

i. XSPI_NEEDED = yes

XSPI_NEEDED help identify if the xspi.mk needs to be included or not.

ii. BL2_ XSPI_NEEDED = yes

XSPI_NEEDED needs to be included in BL2

In optional feature WARM_RESET, I need to save the last reset_cause in flexspi_nor in BL31.

i. Again need two flags: XSPI_NEEDED = yes & BL31_ XSPI_NEEDED = yes

ii. XSPI_NEEDED = yes, is still required as it might happen the boot source is SD.

In this case BL2_XSPI_NEEDED, is not set.

What I am gaining with this macros is:

Without this macro, I need to add two lines:

<IP>_NEEDED = yes.

To include this driver.

One of the flag to be added depending on the IP source file inclusion to BL2 or BL31 or BL_COMM:

Flag as BL2_<IP>_NEEDED = yes
Flag as BL31_<IP>_NEEDED = yes.
Flag as BLCOMM_<IP>_NEEDED = yes.

This macros helps me add one line $(eval $(call SET_FLAG, <IP>_NEEDED,BL2)), instead of above two lines.

If you suggest to remove the SET_FLAG macro, I will replace the SET_FLAG macro with these two flags from entire source tree.

Please share your view.

I reviewed the usage of NEED_BL31 and NEED_BL2.

They are set to ‘yes’ in ./Makefile, only when BL2_SOURCES & BL31_SOURCES is non-null;

They can be over-ridden, but cannot be used for each IP(s) source file.

Regards

Pankaj

From: Varun Wadekar <vwadekar@nvidia.com>
Sent: Thursday, December 3, 2020 4:17 AM
To: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Manish Pandey2 <Manish.Pandey2@arm.com>
Subject: [EXT] RE: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

Caution: EXT Email

Hi,

From your description, I think you are including different makefiles from NXP tree for BL31 versus BL2. Can you please help me understand why this decision needs to be taken at the top level makefile? Alternatively, why can’t NXP platform decide which files to include?

Did you get a chance to review the NEED_BL31, NEED_BL2 makefile variables?

From the gerrit change it is very difficult to understand how the newly introduced macro is used, so trying to suggest already available options to see if they work for you.

-Varun

From: Pankaj Gupta <pankaj.gupta@nxp.com>
Sent: Wednesday, December 2, 2020 5:21 AM
To: Varun Wadekar <vwadekar@nvidia.com>
Cc: Manish Pandey2 <Manish.Pandey2@arm.com>
Subject: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/6122

External email: Use caution opening links or attachments

Hi Varun,

As suggested by you, IMAGE_BL31, IMAGE_BL2 etc macros are useful for segregating the code within the same source file.

But this will not serve the purpose in our case.

Let me try to explain you with an example:

-- For one platform; and for a particulate IP driver’s source file, if it is true to have in both, then :

#if defined(IMAGE_BL2) || #if defined(IMAGE_BL31)

-- But for another platform and for the same IP driver’s source file, if it is true to have in BL2 only, then:

#if defined(IMAGE_BL2)

-- And for another SoC and for the same IP driver’s source file, it if is true to have in BL31 only, then:

#if defined(IMAGE_BL31)

It will not be possible to write all the three varying inclusion of code for one IP used across multiple SoC and their platforms.

Now, I will explain how this macro helps me with above case:

Taking an example of flexspi_nor as a boot-source:

$(eval $(call SET_FLAG, XSPI_NEEDED,BL2))
This macro sets 2 flags.

XSPI_NEEDED = yes

XSPI_NEEDED help identify if the xspi.mk needs to be included or not.

BL2_ XSPI_NEEDED = yes

XSPI_NEEDED needs to be included in BL2.

For a conditional feature to enable in BL31, XSPI is to be included in BL31.

BL31_XSPI_NEEDED = yes needs to be set.
The correct solution should be if the feature is enabled, then:

$(eval $(call SET_FLAG, XSPI_NEEDED,BL31))

In this case, I cannot set BL_COMM_XSPI_NEED = yes for all the platforms.

I hope, I am able to convey my thoughts to you.

This macro is very important for my code orientation. If you think it will not be required by other contributors, then lets rename this macro by prepending it with NXP or any name you suggests.

Thanks.

Regards

Pankaj

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.