Skip to content

Conversation

@nhutnguyenkc
Copy link

@nhutnguyenkc nhutnguyenkc commented Jun 26, 2025

Add support for Renesas RZ/A series
The original work is from https://github.com/renesas/rza-initial-program-loader

@MichaelAllportWM
Copy link

MichaelAllportWM commented Sep 22, 2025

Hi @binhnguyen2434, @nhutnguyenkc
I see that this pull request is in draft mode and has merge conflicts.
I'm assuming that I should only be reviewing the files added by the last commit.
I will continue with that whilst I wait for confirmation.

Add support for Renesas RZ/A series

Signed-off-by: Nhut Nguyen <[email protected]>
@nhutnguyenkc
Copy link
Author

Hi @MichaelAllportWM ,
Thanks for your review. I have rebased on master branch to resolve conflicts. Now you can see and review changes.
This PR is in draft mode; the official contribution will follow TF-A contribution guide via gerrit.

@MichaelAllportWM
Copy link

Param swizzle
There are a few param swizzle DDR configs that could be removed.
As far as I can tell the only ones used are:

  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T3bcud2.c

The ones below mention the V2L in the comments and are not used in your code:

  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1vbc.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1vc.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T2vc.c

I assume that they have remained in error.

There are also other param swizzle configurations which are not used but I understand if you want to keep them for possible use by customers with different board setups for example param_swizzle_T1b.c is not used by any of our boards but could possibly be used by other setups.

@MichaelAllportWM
Copy link

Checkpatch
There are quite a few checkpatch errors and warnings that could possibly be resolved.
I ran:
git diff master | ~/scripts/checkpatch/checkpatch.pl
And got:
total: 2 errors, 317 warnings, 22173 lines checked

@MichaelAllportWM
Copy link

Param MC
Again there are files included that are not used, however I also understand if you want to keep them in for customers with other setups. e.g. ./plat/renesas/rza/soc/a3ul/drivers/ddr/param_mc_C-011_D3-01-2.c is not used by any makefile. No change necessary if you are happy.

@MichaelAllportWM
Copy link

Strict aliasing rule
Some sections of the code that do not adhere to this rule, however there also appears to be plenty of places in TF-A where this rule is ignored. Perhaps then it is not a requirement for this code considering the architecture. I thought it was best to mention it though. No change necessary if you are happy.
e.g. plat/renesas/rza/common/drivers/auth/sblib/sblib_parser.c:67

@MichaelAllportWM
Copy link

#include <string.h>
A lot of the xspi, qspi, octa header files have this include. I'm not sure that it is necessary.

  • plat/renesas/rza/common/drivers/xspidevice/octaflash_mx66uw/octaflash_mx66uw_api.h
  • plat/renesas/rza/common/drivers/xspidevice/octaram_apsxx/octaram_apsxx_api.h
  • plat/renesas/rza/common/drivers/xspidevice/qspiflash_at25/qspiflash_at25_api.h
  • plat/renesas/rza/common/drivers/xspidevice/qspiflash_mx25l25645g/qspiflash_mx25l25645g_api.h
  • plat/renesas/rza/common/include/xspidevice_api.h

@MichaelAllportWM
Copy link

Dates in copywrite header
I'm not sure what the preferred conditions are for updating these, I've not yet checked any of these.

AUTH_METHOD_HASH, /* Authenticate by hash matching */
AUTH_METHOD_SIG, /* Authenticate by PK operation */
AUTH_METHOD_NV_CTR, /* Authenticate by Non-Volatile Counter */
AUTH_METHOD_SBLIB, /* Authenticate by Renesas RZ/G2L SBLib */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The G2L is not included in this repository so I think that this comment should probably change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the comment to RZ/A SBLib

*/
#define SB_RET_SAME_IMAGE_VERSION ((sb_ret_t)0x55005501UL)

/** A internal failure */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor spelling correction: An internal failure

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to An

/** Unsupported algorithm */
#define SB_RET_ERR_CRYPTO_UNSUPPORTED_ALGORITHM ((sb_ret_t)0xAAAA0202UL)

/** Other resorece is using CryptoIP. */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor spelling correction: Other resource

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to resource

/** CRC mismatch */
#define SB_RET_ERR_CRC_MISMATCH ((sb_ret_t)0xAAAA0300UL)

/** Unsupported polynominal */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor spelling correction: Unsupported polynomial

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to polynomial


static sb_secure_boot_api_t secure_boot_api;

int crypto_sblib_auth(void *data_ptr, size_t len, const void *key_cert,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*data_ptr and len are unused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed *data_ptr and len

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again consider changing large arrays to static const.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed them to static const in pfc.c

Comment on lines 218 to 229
func console_rza_flush
ldr x0, [x0, #CONSOLE_T_BASE]
1:
/* Check TEND flag */
ldrh w1, [x0, #FSR]
and w1, w1, #FSR_TEND
cmp w1, #FSR_TEND
bne 1b

mov w0, #0
ret
endfunc console_rza_flush

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue if built with LOG_LEVEL=0
The TEND flag will never be set, thus the device may loop infinitely.

This is catered for in this driver:
https://github.com/renesas-rz/rzg_trusted-firmware-a/blob/2.10.5/rzv2h_1.1.0/plat/renesas/rz/common/drivers/scifa.S

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I applied your suggestion

Comment on lines 45 to 54
while (1) {
read_status =
spi_multi_cmd_read(SMCMR_CMD_READ_STATUS_REGISTER_1);
if ((read_status & STATUS_1_BUSY_BIT) == STATUS_1_BUSY) {
udelay(STATUS_BUSY_READ_DELAY_TIME);
continue;
} else {
break;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point- It might be possible to write this section with fewer lines

do {
    read_status = spi_multi_cmd_read(SMCMR_CMD_READ_STATUS_REGISTER_1);
    if (!(read_status & STATUS_1_BUSY_BIT)) {
        break;
    }
    udelay(STATUS_BUSY_READ_DELAY_TIME);
} while (1);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is unused, so I removed it

Comment on lines 287 to 289
if (false)
flash_read_status_register_spi(
myctrl); /* Avoiding build error */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream reviewers may ask for this to be changed.
Consider using a typedef'ed version of __attribute__((unused)) or removing the unused function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I add __unused to the definition of flash_read_status_register_spi and remove the line 287-289

Comment on lines 69 to 72
// Temporarily disable DMA.
// if ((buffer + length - 1U) <= (uintptr_t)UINT32_MAX) {
// emmc_dma = LOADIMAGE_FLAGS_DMA_ENABLE;
// }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code. Could this possibly be removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emmc is not supported, so I removed this file

Improve RZ/A platform

Signed-off-by: Nhut Nguyen <[email protected]>
@nhutnguyenkc
Copy link
Author

nhutnguyenkc commented Oct 15, 2025

Hi @MichaelAllportWM ,
Thank you very much for your review.

Param swizzle There are a few param swizzle DDR configs that could be removed. As far as I can tell the only ones used are:

  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T3bcud2.c

The ones below mention the V2L in the comments and are not used in your code:

  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1vbc.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T1vc.c
  • ./plat/renesas/rza/common/drivers/ddr/param_swizzle_T2vc.c

I assume that they have remained in error.

There are also other param swizzle configurations which are not used but I understand if you want to keep them for possible use by customers with different board setups for example param_swizzle_T1b.c is not used by any of our boards but could possibly be used by other setups.

Param MC
Again there are files included that are not used, however I also understand if you want to keep them in for customers with other setups. e.g. ./plat/renesas/rza/soc/a3ul/drivers/ddr/param_mc_C-011_D3-01-2.c is not used by any makefile. No change necessary if you are happy.

I have rechecked all and removed irrelevant ones, just keeping files built

@nhutnguyenkc
Copy link
Author

#include <string.h> A lot of the xspi, qspi, octa header files have this include. I'm not sure that it is necessary.

  • plat/renesas/rza/common/drivers/xspidevice/octaflash_mx66uw/octaflash_mx66uw_api.h
  • plat/renesas/rza/common/drivers/xspidevice/octaram_apsxx/octaram_apsxx_api.h
  • plat/renesas/rza/common/drivers/xspidevice/qspiflash_at25/qspiflash_at25_api.h
  • plat/renesas/rza/common/drivers/xspidevice/qspiflash_mx25l25645g/qspiflash_mx25l25645g_api.h
  • plat/renesas/rza/common/include/xspidevice_api.h

Agree, this include is unnecessary. I removed it

@nhutnguyenkc
Copy link
Author

Dates in copywrite header I'm not sure what the preferred conditions are for updating these, I've not yet checked any of these.

I would like to keep the original date and extent to this year 2025

@nhutnguyenkc
Copy link
Author

Checkpatch There are quite a few checkpatch errors and warnings that could possibly be resolved. I ran: git diff master | ~/scripts/checkpatch/checkpatch.pl And got: total: 2 errors, 317 warnings, 22173 lines checked

I fixed all error and most of warnings except for this one WARNING: Prefer strscpy over strlcpy since I see strscpy is also used in other TF-A source

@nhutnguyenkc
Copy link
Author

Along with your findings, following improvements have been made.

Your feedback is very helpful for us. Please let us know if you have any further suggestions or guidance.

@MichaelAllportWM
Copy link

MichaelAllportWM commented Oct 17, 2025

Checkpatch There are quite a few checkpatch errors and warnings that could possibly be resolved. I ran: git diff master | ~/scripts/checkpatch/checkpatch.pl And got: total: 2 errors, 317 warnings, 22173 lines checked

I fixed all error and most of warnings except for this one WARNING: Prefer strscpy over strlcpy since I see strscpy is also used in other TF-A source

If I rungit diff 0a29cac8fe0f7bdb835b469d9ea11b8e17377a92 | ~/scripts/checkpatch/checkpatch.pl

I get 2 errors and 6 warnings review_checkpatch_issues.txt

The ones I think are worth considering fixing are:

  • the 2 warnings with the misspelling of configuration.
  • the 2 errors. These might result in a fair amount of rework. Obviously its not possible to put do while loops around case labels, however I'm unsure if a checkpatch error will be accepted by the upstream reviewers.

The other warnings are probably ok I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants