drivers: rockchip: extract OTP driver from rk3588 platform #7545
Conversation
|
I extracted the patch to extract the Rockchip OTP from #7511, because the refactoring is useful even without the rksecure PTA. |
tretter
force-pushed
the
rockchip-otp
branch
from
b022f41 to
bb7485c
Compare
| #include <string_ext.h> | ||
| #include <utee_defines.h> | ||
|
|
||
| #include <drivers/rockchip_otp.h> |
There was a problem hiding this comment.
Could you move this directive above:
#include <common.h>
+#include <drivers/rockchip_otp.h>
#include <io.h>
#include <kernel/panic.h>
core/drivers/rockchip_otp.c
Outdated
| #include <mm/core_memprot.h> | ||
| #include <utee_defines.h> | ||
|
|
||
| #include <drivers/rockchip_otp.h> |
There was a problem hiding this comment.
#include <common.h>
+#include <drivers/rockchip_otp.h>
#include <io.h>
#include <kernel/panic.h>
| @@ -0,0 +1,189 @@ | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
| /* | |||
| * Copyright (C) 2025, Pengutronix, Michael Tretter <[email protected]> | |||
There was a problem hiding this comment.
I think you should also add the original copyrigth owners:
* Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
* Copyright (c) 2024, Rockchip, Inc. All rights reserved.
core/drivers/rockchip_otp.c
Outdated
| return TEE_SUCCESS; | ||
| } | ||
|
|
||
| register_phys_mem_pgdir(MEM_AREA_IO_SEC, OTP_S_BASE, OTP_S_SIZE); |
There was a problem hiding this comment.
Could you move this above function definitions?
|
All comments have been addressed |
etienne-lms
left a comment
etienne-lms
left a comment
There was a problem hiding this comment.
Feel free to squash existing fixup commits.
| #define OTP_POLL_PERIOD_US 0 | ||
| #define OTP_POLL_TIMEOUT_US 1000 | ||
|
|
||
| #define HW_UNIQUE_KEY_INDEX 0x104 |
There was a problem hiding this comment.
HW_UNIQUE_KEY_INDEX is only used by tee_otp_get_hw_unique_key() that remains in core/arch/arm/plat-rockchip/platform_rk3588.c. I think the macro definition can remain in this source file.
There was a problem hiding this comment.
HW_UNIQUE_KEY_INDEX describes the layout of the data in the OTP fuses. There will be other constants for other data, for example the secure boot status or the public key hash. The HW_UNIQUE_KEY_INDEX and other indexes are API for how the OTP driver shall be used and, thus, belongs into the header.
There was a problem hiding this comment.
Move the macro definition to platform_config.h if it's platform flavor specific?
But okay if you really think it deserves to be defined in that driver header file.
That said, being moved outside a specific source file, I think it should also be renamed with ROCKCHIP_OPT_ or like prefix.
There was a problem hiding this comment.
Moving to platform_config.h sounds good. I moved it and changed the name to include a ROCKCHIP_OTP_ prefix.
core/include/drivers/rockchip_otp.h
Outdated
| * @index index of the first word to read in 32 bit words | ||
| * @count number of 32 bit words to read | ||
| */ | ||
| TEE_Result tee_otp_read_secure(uint32_t *value, uint32_t index, |
There was a problem hiding this comment.
Could you rename these 2 exported functions?
|
All comments have been addressed |
etienne-lms
left a comment
etienne-lms
left a comment
There was a problem hiding this comment.
LGTM. I suggest an extra build-time assertion. Feel free to squash the fixup commits.
Reviewed-by: Etienne Carriere <[email protected]>
| { | ||
| TEE_Result res = TEE_SUCCESS; | ||
| uint32_t buffer[HW_UNIQUE_KEY_LENGTH / sizeof(uint32_t)] = { }; | ||
| uint32_t buffer[ROCKCHIP_OTP_HUK_SIZE] = { }; |
There was a problem hiding this comment.
I would suggest a build time assertion like static_assert(sizeof(buffer) == sizeof(hwkey->data)).
There was a problem hiding this comment.
I like the idea of a build time assertion, but I'd add it in a follow-up pull request, since #7515 touches this code as well.
The OTP handling is useful outside the rk3588 platform implementation. For example, the fuses for secure boot are accessible via the OTP. Extract the OTP write and read support to a separate driver to make it available for other modules. Signed-off-by: Michael Tretter <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>
|
Fixed up commits with tag and ready to be merged |
3 participants
The OTP handling is useful outside the rk3588 platform implementation. For example, the fuses for secure boot are accessible via the OTP.
Extract the OTP write and read support to a separate driver to make it available for other modules.