| Message ID | 20231107155532.3747113-6-clabbe@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: rockchip: add support for rk3588/rk3568 | expand |
On 07/11/2023 16:55, Corentin Labbe wrote: > While working on the rk3588 crypto driver, I loose lot of time > understanding why resetting the IP failed. > This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, > so impossible to operate on it from the kernel. > All resets in this block must be handled via SCMI call. > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > drivers/clk/rockchip/rst-rk3588.c | 42 ------------ > .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > 2 files changed, 34 insertions(+), 76 deletions(-) > > diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c > index e855bb8d5413..6556d9d3c7ab 100644 > --- a/drivers/clk/rockchip/rst-rk3588.c > +++ b/drivers/clk/rockchip/rst-rk3588.c > @@ -16,9 +16,6 @@ > /* 0xFD7C8000 + 0x0A00 */ > #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) > > -/* 0xFD7D0000 + 0x0A00 */ > -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) > - > /* 0xFD7F0000 + 0x0A00 */ > #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) > > @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { > RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), > RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), > RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), > - > - /* SECURECRU_SOFTRST_CON00 */ > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), > - > - /* SECURECRU_SOFTRST_CON01 */ > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), > - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), > - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), > - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), > - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), > - > - /* SECURECRU_SOFTRST_CON02 */ > - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), > - > - /* SECURECRU_SOFTRST_CON03 */ > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), > - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), > - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), > - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), > }; > > void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) > diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h > index d4264db2a07f..c0d08ae78cd5 100644 > --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h > +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h > @@ -716,39 +716,39 @@ > #define SRST_P_GPIO0 627 > #define SRST_GPIO0 628 > > -#define SRST_A_SECURE_NS_BIU 629 > -#define SRST_H_SECURE_NS_BIU 630 > -#define SRST_A_SECURE_S_BIU 631 > -#define SRST_H_SECURE_S_BIU 632 > -#define SRST_P_SECURE_S_BIU 633 > -#define SRST_CRYPTO_CORE 634 > - > -#define SRST_CRYPTO_PKA 635 > -#define SRST_CRYPTO_RNG 636 > -#define SRST_A_CRYPTO 637 > -#define SRST_H_CRYPTO 638 > -#define SRST_KEYLADDER_CORE 639 > -#define SRST_KEYLADDER_RNG 640 > -#define SRST_A_KEYLADDER 641 > -#define SRST_H_KEYLADDER 642 > -#define SRST_P_OTPC_S 643 > -#define SRST_OTPC_S 644 > -#define SRST_WDT_S 645 > - > -#define SRST_T_WDT_S 646 > -#define SRST_H_BOOTROM 647 > -#define SRST_A_DCF 648 > -#define SRST_P_DCF 649 > -#define SRST_H_BOOTROM_NS 650 > -#define SRST_P_KEYLADDER 651 > -#define SRST_H_TRNG_S 652 > - > -#define SRST_H_TRNG_NS 653 > -#define SRST_D_SDMMC_BUFFER 654 > -#define SRST_H_SDMMC 655 > -#define SRST_H_SDMMC_BUFFER 656 > -#define SRST_SDMMC 657 > -#define SRST_P_TRNG_CHK 658 > -#define SRST_TRNG_S 659 > +#define SRST_A_SECURE_NS_BIU 10 NAK. You just broke all users. Best regards, Krzysztof
Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: > On 07/11/2023 16:55, Corentin Labbe wrote: > > While working on the rk3588 crypto driver, I loose lot of time > > understanding why resetting the IP failed. > > This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, > > so impossible to operate on it from the kernel. > > All resets in this block must be handled via SCMI call. > > > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > > --- > > drivers/clk/rockchip/rst-rk3588.c | 42 ------------ > > .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- > > Please run scripts/checkpatch.pl and fix reported warnings. Some > warnings can be ignored, but the code here looks like it needs a fix. > Feel free to get in touch if the warning is not clear. > > > 2 files changed, 34 insertions(+), 76 deletions(-) > > > > diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c > > index e855bb8d5413..6556d9d3c7ab 100644 > > --- a/drivers/clk/rockchip/rst-rk3588.c > > +++ b/drivers/clk/rockchip/rst-rk3588.c > > @@ -16,9 +16,6 @@ > > /* 0xFD7C8000 + 0x0A00 */ > > #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) > > > > -/* 0xFD7D0000 + 0x0A00 */ > > -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) > > - > > /* 0xFD7F0000 + 0x0A00 */ > > #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) > > > > @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { > > RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), > > RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), > > RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), > > - > > - /* SECURECRU_SOFTRST_CON00 */ > > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), > > - > > - /* SECURECRU_SOFTRST_CON01 */ > > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), > > - > > - /* SECURECRU_SOFTRST_CON02 */ > > - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), > > - > > - /* SECURECRU_SOFTRST_CON03 */ > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), > > - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), > > }; > > > > void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) > > diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h > > index d4264db2a07f..c0d08ae78cd5 100644 > > --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h > > +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h > > @@ -716,39 +716,39 @@ > > #define SRST_P_GPIO0 627 > > #define SRST_GPIO0 628 > > > > -#define SRST_A_SECURE_NS_BIU 629 > > -#define SRST_H_SECURE_NS_BIU 630 > > -#define SRST_A_SECURE_S_BIU 631 > > -#define SRST_H_SECURE_S_BIU 632 > > -#define SRST_P_SECURE_S_BIU 633 > > -#define SRST_CRYPTO_CORE 634 > > - > > -#define SRST_CRYPTO_PKA 635 > > -#define SRST_CRYPTO_RNG 636 > > -#define SRST_A_CRYPTO 637 > > -#define SRST_H_CRYPTO 638 > > -#define SRST_KEYLADDER_CORE 639 > > -#define SRST_KEYLADDER_RNG 640 > > -#define SRST_A_KEYLADDER 641 > > -#define SRST_H_KEYLADDER 642 > > -#define SRST_P_OTPC_S 643 > > -#define SRST_OTPC_S 644 > > -#define SRST_WDT_S 645 > > - > > -#define SRST_T_WDT_S 646 > > -#define SRST_H_BOOTROM 647 > > -#define SRST_A_DCF 648 > > -#define SRST_P_DCF 649 > > -#define SRST_H_BOOTROM_NS 650 > > -#define SRST_P_KEYLADDER 651 > > -#define SRST_H_TRNG_S 652 > > - > > -#define SRST_H_TRNG_NS 653 > > -#define SRST_D_SDMMC_BUFFER 654 > > -#define SRST_H_SDMMC 655 > > -#define SRST_H_SDMMC_BUFFER 656 > > -#define SRST_SDMMC 657 > > -#define SRST_P_TRNG_CHK 658 > > -#define SRST_TRNG_S 659 > > +#define SRST_A_SECURE_NS_BIU 10 > > NAK. You just broke all users. If I'm reading the commit message correctly, all resets in that area couldn't have any users to begin with, as the registers controlling them are in the secure space, and need a higher exception level So if anything is trying to handle these resets, would end up with some security exception right now. Though I guess we might want to use different names and not reuse the existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ?
On 07/11/2023 18:35, Heiko Stübner wrote: > Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: >> On 07/11/2023 16:55, Corentin Labbe wrote: >>> While working on the rk3588 crypto driver, I loose lot of time >>> understanding why resetting the IP failed. >>> This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, >>> so impossible to operate on it from the kernel. >>> All resets in this block must be handled via SCMI call. >>> >>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com> >>> --- >>> drivers/clk/rockchip/rst-rk3588.c | 42 ------------ >>> .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- >> >> Please run scripts/checkpatch.pl and fix reported warnings. Some >> warnings can be ignored, but the code here looks like it needs a fix. >> Feel free to get in touch if the warning is not clear. >> >>> 2 files changed, 34 insertions(+), 76 deletions(-) >>> >>> diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c >>> index e855bb8d5413..6556d9d3c7ab 100644 >>> --- a/drivers/clk/rockchip/rst-rk3588.c >>> +++ b/drivers/clk/rockchip/rst-rk3588.c >>> @@ -16,9 +16,6 @@ >>> /* 0xFD7C8000 + 0x0A00 */ >>> #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) >>> >>> -/* 0xFD7D0000 + 0x0A00 */ >>> -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) >>> - >>> /* 0xFD7F0000 + 0x0A00 */ >>> #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) >>> >>> @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), >>> - >>> - /* SECURECRU_SOFTRST_CON00 */ >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), >>> - >>> - /* SECURECRU_SOFTRST_CON01 */ >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), >>> - >>> - /* SECURECRU_SOFTRST_CON02 */ >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), >>> - >>> - /* SECURECRU_SOFTRST_CON03 */ >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), >>> }; >>> >>> void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) >>> diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h >>> index d4264db2a07f..c0d08ae78cd5 100644 >>> --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h >>> +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h >>> @@ -716,39 +716,39 @@ >>> #define SRST_P_GPIO0 627 >>> #define SRST_GPIO0 628 >>> >>> -#define SRST_A_SECURE_NS_BIU 629 >>> -#define SRST_H_SECURE_NS_BIU 630 >>> -#define SRST_A_SECURE_S_BIU 631 >>> -#define SRST_H_SECURE_S_BIU 632 >>> -#define SRST_P_SECURE_S_BIU 633 >>> -#define SRST_CRYPTO_CORE 634 >>> - >>> -#define SRST_CRYPTO_PKA 635 >>> -#define SRST_CRYPTO_RNG 636 >>> -#define SRST_A_CRYPTO 637 >>> -#define SRST_H_CRYPTO 638 >>> -#define SRST_KEYLADDER_CORE 639 >>> -#define SRST_KEYLADDER_RNG 640 >>> -#define SRST_A_KEYLADDER 641 >>> -#define SRST_H_KEYLADDER 642 >>> -#define SRST_P_OTPC_S 643 >>> -#define SRST_OTPC_S 644 >>> -#define SRST_WDT_S 645 >>> - >>> -#define SRST_T_WDT_S 646 >>> -#define SRST_H_BOOTROM 647 >>> -#define SRST_A_DCF 648 >>> -#define SRST_P_DCF 649 >>> -#define SRST_H_BOOTROM_NS 650 >>> -#define SRST_P_KEYLADDER 651 >>> -#define SRST_H_TRNG_S 652 >>> - >>> -#define SRST_H_TRNG_NS 653 >>> -#define SRST_D_SDMMC_BUFFER 654 >>> -#define SRST_H_SDMMC 655 >>> -#define SRST_H_SDMMC_BUFFER 656 >>> -#define SRST_SDMMC 657 >>> -#define SRST_P_TRNG_CHK 658 >>> -#define SRST_TRNG_S 659 >>> +#define SRST_A_SECURE_NS_BIU 10 >> >> NAK. You just broke all users. > > If I'm reading the commit message correctly, all resets in that area > couldn't have any users to begin with, as the registers controlling them > are in the secure space, and need a higher exception level > > So if anything is trying to handle these resets, would end up with some > security exception right now. > > Though I guess we might want to use different names and not reuse the > existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ? I don't quite get what the patch wants to achieve. Why dropping driver support for given reset ID is connected with changing the value of binding for given reset? What is the point of this define SRST_A_SECURE_NS_BIU 10? Best regards, Krzysztof
Hi, On Tue, Nov 07, 2023 at 06:45:03PM +0100, Krzysztof Kozlowski wrote: > On 07/11/2023 18:35, Heiko Stübner wrote: > > Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: > >> On 07/11/2023 16:55, Corentin Labbe wrote: > >>> While working on the rk3588 crypto driver, I loose lot of time > >>> understanding why resetting the IP failed. > >>> This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, > >>> so impossible to operate on it from the kernel. > >>> All resets in this block must be handled via SCMI call. > >>> > >>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > >>> --- > >>> drivers/clk/rockchip/rst-rk3588.c | 42 ------------ > >>> .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- > >> > >> Please run scripts/checkpatch.pl and fix reported warnings. Some > >> warnings can be ignored, but the code here looks like it needs a fix. > >> Feel free to get in touch if the warning is not clear. > >> > >>> 2 files changed, 34 insertions(+), 76 deletions(-) > >>> > >>> diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c > >>> index e855bb8d5413..6556d9d3c7ab 100644 > >>> --- a/drivers/clk/rockchip/rst-rk3588.c > >>> +++ b/drivers/clk/rockchip/rst-rk3588.c > >>> @@ -16,9 +16,6 @@ > >>> /* 0xFD7C8000 + 0x0A00 */ > >>> #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) > >>> > >>> -/* 0xFD7D0000 + 0x0A00 */ > >>> -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) > >>> - > >>> /* 0xFD7F0000 + 0x0A00 */ > >>> #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) > >>> > >>> @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { > >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), > >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), > >>> RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), > >>> - > >>> - /* SECURECRU_SOFTRST_CON00 */ > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), > >>> - > >>> - /* SECURECRU_SOFTRST_CON01 */ > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), > >>> - > >>> - /* SECURECRU_SOFTRST_CON02 */ > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), > >>> - > >>> - /* SECURECRU_SOFTRST_CON03 */ > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), > >>> - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), > >>> }; > >>> > >>> void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) > >>> diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>> index d4264db2a07f..c0d08ae78cd5 100644 > >>> --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>> +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>> @@ -716,39 +716,39 @@ > >>> #define SRST_P_GPIO0 627 > >>> #define SRST_GPIO0 628 > >>> > >>> -#define SRST_A_SECURE_NS_BIU 629 > >>> -#define SRST_H_SECURE_NS_BIU 630 > >>> -#define SRST_A_SECURE_S_BIU 631 > >>> -#define SRST_H_SECURE_S_BIU 632 > >>> -#define SRST_P_SECURE_S_BIU 633 > >>> -#define SRST_CRYPTO_CORE 634 > >>> - > >>> -#define SRST_CRYPTO_PKA 635 > >>> -#define SRST_CRYPTO_RNG 636 > >>> -#define SRST_A_CRYPTO 637 > >>> -#define SRST_H_CRYPTO 638 > >>> -#define SRST_KEYLADDER_CORE 639 > >>> -#define SRST_KEYLADDER_RNG 640 > >>> -#define SRST_A_KEYLADDER 641 > >>> -#define SRST_H_KEYLADDER 642 > >>> -#define SRST_P_OTPC_S 643 > >>> -#define SRST_OTPC_S 644 > >>> -#define SRST_WDT_S 645 > >>> - > >>> -#define SRST_T_WDT_S 646 > >>> -#define SRST_H_BOOTROM 647 > >>> -#define SRST_A_DCF 648 > >>> -#define SRST_P_DCF 649 > >>> -#define SRST_H_BOOTROM_NS 650 > >>> -#define SRST_P_KEYLADDER 651 > >>> -#define SRST_H_TRNG_S 652 > >>> - > >>> -#define SRST_H_TRNG_NS 653 > >>> -#define SRST_D_SDMMC_BUFFER 654 > >>> -#define SRST_H_SDMMC 655 > >>> -#define SRST_H_SDMMC_BUFFER 656 > >>> -#define SRST_SDMMC 657 > >>> -#define SRST_P_TRNG_CHK 658 > >>> -#define SRST_TRNG_S 659 > >>> +#define SRST_A_SECURE_NS_BIU 10 > >> > >> NAK. You just broke all users. > > > > If I'm reading the commit message correctly, all resets in that area > > couldn't have any users to begin with, as the registers controlling them > > are in the secure space, and need a higher exception level > > > > So if anything is trying to handle these resets, would end up with some > > security exception right now. > > > > Though I guess we might want to use different names and not reuse the > > existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ? > > I don't quite get what the patch wants to achieve. Why dropping driver > support for given reset ID is connected with changing the value of > binding for given reset? > > What is the point of this define SRST_A_SECURE_NS_BIU 10? This is about two different reset controllers. The IDs defined here are used by the operating system to access the correct registers. The kernel has a LUT from the ID to a register addresses, which is something you asked for during upstreaming. The ID defined by Corentin is for reset control via SCMI firmware, which has different number scheme than Linux. To me the suggestion from Heiko looks sensible (i.e. create a new ID scheme and keep the current one unchanged). Greetings, -- Sebastian
On 11/11/2023 21:51, Sebastian Reichel wrote: > Hi, > > On Tue, Nov 07, 2023 at 06:45:03PM +0100, Krzysztof Kozlowski wrote: >> On 07/11/2023 18:35, Heiko Stübner wrote: >>> Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: >>>> On 07/11/2023 16:55, Corentin Labbe wrote: >>>>> While working on the rk3588 crypto driver, I loose lot of time >>>>> understanding why resetting the IP failed. >>>>> This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, >>>>> so impossible to operate on it from the kernel. >>>>> All resets in this block must be handled via SCMI call. >>>>> >>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com> >>>>> --- >>>>> drivers/clk/rockchip/rst-rk3588.c | 42 ------------ >>>>> .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- >>>> >>>> Please run scripts/checkpatch.pl and fix reported warnings. Some >>>> warnings can be ignored, but the code here looks like it needs a fix. >>>> Feel free to get in touch if the warning is not clear. >>>> >>>>> 2 files changed, 34 insertions(+), 76 deletions(-) >>>>> >>>>> diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c >>>>> index e855bb8d5413..6556d9d3c7ab 100644 >>>>> --- a/drivers/clk/rockchip/rst-rk3588.c >>>>> +++ b/drivers/clk/rockchip/rst-rk3588.c >>>>> @@ -16,9 +16,6 @@ >>>>> /* 0xFD7C8000 + 0x0A00 */ >>>>> #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) >>>>> >>>>> -/* 0xFD7D0000 + 0x0A00 */ >>>>> -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) >>>>> - >>>>> /* 0xFD7F0000 + 0x0A00 */ >>>>> #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) >>>>> >>>>> @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), >>>>> - >>>>> - /* SECURECRU_SOFTRST_CON00 */ >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), >>>>> - >>>>> - /* SECURECRU_SOFTRST_CON01 */ >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), >>>>> - >>>>> - /* SECURECRU_SOFTRST_CON02 */ >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), >>>>> - >>>>> - /* SECURECRU_SOFTRST_CON03 */ >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), >>>>> }; >>>>> >>>>> void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) >>>>> diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h >>>>> index d4264db2a07f..c0d08ae78cd5 100644 >>>>> --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h >>>>> +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h >>>>> @@ -716,39 +716,39 @@ >>>>> #define SRST_P_GPIO0 627 >>>>> #define SRST_GPIO0 628 >>>>> >>>>> -#define SRST_A_SECURE_NS_BIU 629 >>>>> -#define SRST_H_SECURE_NS_BIU 630 >>>>> -#define SRST_A_SECURE_S_BIU 631 >>>>> -#define SRST_H_SECURE_S_BIU 632 >>>>> -#define SRST_P_SECURE_S_BIU 633 >>>>> -#define SRST_CRYPTO_CORE 634 >>>>> - >>>>> -#define SRST_CRYPTO_PKA 635 >>>>> -#define SRST_CRYPTO_RNG 636 >>>>> -#define SRST_A_CRYPTO 637 >>>>> -#define SRST_H_CRYPTO 638 >>>>> -#define SRST_KEYLADDER_CORE 639 >>>>> -#define SRST_KEYLADDER_RNG 640 >>>>> -#define SRST_A_KEYLADDER 641 >>>>> -#define SRST_H_KEYLADDER 642 >>>>> -#define SRST_P_OTPC_S 643 >>>>> -#define SRST_OTPC_S 644 >>>>> -#define SRST_WDT_S 645 >>>>> - >>>>> -#define SRST_T_WDT_S 646 >>>>> -#define SRST_H_BOOTROM 647 >>>>> -#define SRST_A_DCF 648 >>>>> -#define SRST_P_DCF 649 >>>>> -#define SRST_H_BOOTROM_NS 650 >>>>> -#define SRST_P_KEYLADDER 651 >>>>> -#define SRST_H_TRNG_S 652 >>>>> - >>>>> -#define SRST_H_TRNG_NS 653 >>>>> -#define SRST_D_SDMMC_BUFFER 654 >>>>> -#define SRST_H_SDMMC 655 >>>>> -#define SRST_H_SDMMC_BUFFER 656 >>>>> -#define SRST_SDMMC 657 >>>>> -#define SRST_P_TRNG_CHK 658 >>>>> -#define SRST_TRNG_S 659 >>>>> +#define SRST_A_SECURE_NS_BIU 10 >>>> >>>> NAK. You just broke all users. >>> >>> If I'm reading the commit message correctly, all resets in that area >>> couldn't have any users to begin with, as the registers controlling them >>> are in the secure space, and need a higher exception level >>> >>> So if anything is trying to handle these resets, would end up with some >>> security exception right now. >>> >>> Though I guess we might want to use different names and not reuse the >>> existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ? >> >> I don't quite get what the patch wants to achieve. Why dropping driver >> support for given reset ID is connected with changing the value of >> binding for given reset? >> >> What is the point of this define SRST_A_SECURE_NS_BIU 10? > > This is about two different reset controllers. The IDs defined here > are used by the operating system to access the correct registers. > The kernel has a LUT from the ID to a register addresses, which is > something you asked for during upstreaming. > > The ID defined by Corentin is for reset control via SCMI firmware, > which has different number scheme than Linux. To me the suggestion > from Heiko looks sensible (i.e. create a new ID scheme and keep the > current one unchanged). So the binding is not for Linux but for FW? This should be explained in the commit msg. Best regards, Krzysztof
Hi, On Sat, Nov 11, 2023 at 10:28:59PM +0100, Krzysztof Kozlowski wrote: > On 11/11/2023 21:51, Sebastian Reichel wrote: > > Hi, > > > > On Tue, Nov 07, 2023 at 06:45:03PM +0100, Krzysztof Kozlowski wrote: > >> On 07/11/2023 18:35, Heiko Stübner wrote: > >>> Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: > >>>> On 07/11/2023 16:55, Corentin Labbe wrote: > >>>>> While working on the rk3588 crypto driver, I loose lot of time > >>>>> understanding why resetting the IP failed. > >>>>> This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, > >>>>> so impossible to operate on it from the kernel. > >>>>> All resets in this block must be handled via SCMI call. > >>>>> > >>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > >>>>> --- > >>>>> drivers/clk/rockchip/rst-rk3588.c | 42 ------------ > >>>>> .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- > >>>> > >>>> Please run scripts/checkpatch.pl and fix reported warnings. Some > >>>> warnings can be ignored, but the code here looks like it needs a fix. > >>>> Feel free to get in touch if the warning is not clear. > >>>> > >>>>> 2 files changed, 34 insertions(+), 76 deletions(-) > >>>>> > >>>>> diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c > >>>>> index e855bb8d5413..6556d9d3c7ab 100644 > >>>>> --- a/drivers/clk/rockchip/rst-rk3588.c > >>>>> +++ b/drivers/clk/rockchip/rst-rk3588.c > >>>>> @@ -16,9 +16,6 @@ > >>>>> /* 0xFD7C8000 + 0x0A00 */ > >>>>> #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) > >>>>> > >>>>> -/* 0xFD7D0000 + 0x0A00 */ > >>>>> -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) > >>>>> - > >>>>> /* 0xFD7F0000 + 0x0A00 */ > >>>>> #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) > >>>>> > >>>>> @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), > >>>>> - > >>>>> - /* SECURECRU_SOFTRST_CON00 */ > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), > >>>>> - > >>>>> - /* SECURECRU_SOFTRST_CON01 */ > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), > >>>>> - > >>>>> - /* SECURECRU_SOFTRST_CON02 */ > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), > >>>>> - > >>>>> - /* SECURECRU_SOFTRST_CON03 */ > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), > >>>>> }; > >>>>> > >>>>> void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) > >>>>> diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>>>> index d4264db2a07f..c0d08ae78cd5 100644 > >>>>> --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>>>> +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h > >>>>> @@ -716,39 +716,39 @@ > >>>>> #define SRST_P_GPIO0 627 > >>>>> #define SRST_GPIO0 628 > >>>>> > >>>>> -#define SRST_A_SECURE_NS_BIU 629 > >>>>> -#define SRST_H_SECURE_NS_BIU 630 > >>>>> -#define SRST_A_SECURE_S_BIU 631 > >>>>> -#define SRST_H_SECURE_S_BIU 632 > >>>>> -#define SRST_P_SECURE_S_BIU 633 > >>>>> -#define SRST_CRYPTO_CORE 634 > >>>>> - > >>>>> -#define SRST_CRYPTO_PKA 635 > >>>>> -#define SRST_CRYPTO_RNG 636 > >>>>> -#define SRST_A_CRYPTO 637 > >>>>> -#define SRST_H_CRYPTO 638 > >>>>> -#define SRST_KEYLADDER_CORE 639 > >>>>> -#define SRST_KEYLADDER_RNG 640 > >>>>> -#define SRST_A_KEYLADDER 641 > >>>>> -#define SRST_H_KEYLADDER 642 > >>>>> -#define SRST_P_OTPC_S 643 > >>>>> -#define SRST_OTPC_S 644 > >>>>> -#define SRST_WDT_S 645 > >>>>> - > >>>>> -#define SRST_T_WDT_S 646 > >>>>> -#define SRST_H_BOOTROM 647 > >>>>> -#define SRST_A_DCF 648 > >>>>> -#define SRST_P_DCF 649 > >>>>> -#define SRST_H_BOOTROM_NS 650 > >>>>> -#define SRST_P_KEYLADDER 651 > >>>>> -#define SRST_H_TRNG_S 652 > >>>>> - > >>>>> -#define SRST_H_TRNG_NS 653 > >>>>> -#define SRST_D_SDMMC_BUFFER 654 > >>>>> -#define SRST_H_SDMMC 655 > >>>>> -#define SRST_H_SDMMC_BUFFER 656 > >>>>> -#define SRST_SDMMC 657 > >>>>> -#define SRST_P_TRNG_CHK 658 > >>>>> -#define SRST_TRNG_S 659 > >>>>> +#define SRST_A_SECURE_NS_BIU 10 > >>>> > >>>> NAK. You just broke all users. > >>> > >>> If I'm reading the commit message correctly, all resets in that area > >>> couldn't have any users to begin with, as the registers controlling them > >>> are in the secure space, and need a higher exception level > >>> > >>> So if anything is trying to handle these resets, would end up with some > >>> security exception right now. > >>> > >>> Though I guess we might want to use different names and not reuse the > >>> existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ? > >> > >> I don't quite get what the patch wants to achieve. Why dropping driver > >> support for given reset ID is connected with changing the value of > >> binding for given reset? > >> > >> What is the point of this define SRST_A_SECURE_NS_BIU 10? > > > > This is about two different reset controllers. The IDs defined here > > are used by the operating system to access the correct registers. > > The kernel has a LUT from the ID to a register addresses, which is > > something you asked for during upstreaming. > > > > The ID defined by Corentin is for reset control via SCMI firmware, > > which has different number scheme than Linux. To me the suggestion > > from Heiko looks sensible (i.e. create a new ID scheme and keep the > > current one unchanged). > > So the binding is not for Linux but for FW? This should be explained in > the commit msg. No. The current binding describes reset IDs, which are mapped by the Linux driver to register offsets in the CRU (clock-reset-unit). But accessing the crypto reset line directly from Linux (which usually does not run in secure state) will fail. Accessing it from correct security context with the current binding is fine though. Considering we are sharing the bindings with e.g. U-Boot, I suggest to keep the currently defined IDs. But Corentin tries to get this running on Linux. For that he needs to ask the (SCMI) firmware running in secure state to please take care of the reset. The firmware is using different reset IDs (apparently the ones used by downstream Linux, which are derived from register offset). In DT the difference looks like this (check the different phandles): #define SRST_A_SECURE_NS_BIU 629 crypto-old { // existing binding from Linux perspective // reset via direct CRU access // NOTE: permission denied resets = <&cru SRST_A_SECURE_NS_BIU>; }; #define SCMI_RST_A_SECURE_NS_BIU 10 crypto-new { // new binding from Linux perspective // reset via SCMI firmware request resets = <&scmi SCMI_RST_A_SECURE_NS_BIU>; }; Instead of introducing SCMI_RST_A_SECURE_NS_BIU, Corentin currently just redefines SRST_A_SECURE_NS_BIU. This is quite misleading. If somebody does '<&cru SRST_A_SECURE_NS_BIU>' with the '10' value for SCMI, it instead resets SRST_A_TOP_M300_BIU. So my suggestion is to go with the suggestion from Heiko and introduce SCMI_RST_A_SECURE_NS_BIU (or something similar). That also matches how the SCMI clks on RK3588 and some other platforms. See e.g.: of include/dt-bindings/clock/rockchip,rk3588-cru.h. Greetings, -- Sebastian
Le Sun, Nov 12, 2023 at 09:26:39PM +0100, Sebastian Reichel a écrit : > Hi, > > On Sat, Nov 11, 2023 at 10:28:59PM +0100, Krzysztof Kozlowski wrote: > > On 11/11/2023 21:51, Sebastian Reichel wrote: > > > Hi, > > > > > > On Tue, Nov 07, 2023 at 06:45:03PM +0100, Krzysztof Kozlowski wrote: > > >> On 07/11/2023 18:35, Heiko Stübner wrote: > > >>> Am Dienstag, 7. November 2023, 17:21:41 CET schrieb Krzysztof Kozlowski: > > >>>> On 07/11/2023 16:55, Corentin Labbe wrote: > > >>>>> While working on the rk3588 crypto driver, I loose lot of time > > >>>>> understanding why resetting the IP failed. > > >>>>> This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, > > >>>>> so impossible to operate on it from the kernel. > > >>>>> All resets in this block must be handled via SCMI call. > > >>>>> > > >>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > > >>>>> --- > > >>>>> drivers/clk/rockchip/rst-rk3588.c | 42 ------------ > > >>>>> .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- > > >>>> > > >>>> Please run scripts/checkpatch.pl and fix reported warnings. Some > > >>>> warnings can be ignored, but the code here looks like it needs a fix. > > >>>> Feel free to get in touch if the warning is not clear. > > >>>> > > >>>>> 2 files changed, 34 insertions(+), 76 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c > > >>>>> index e855bb8d5413..6556d9d3c7ab 100644 > > >>>>> --- a/drivers/clk/rockchip/rst-rk3588.c > > >>>>> +++ b/drivers/clk/rockchip/rst-rk3588.c > > >>>>> @@ -16,9 +16,6 @@ > > >>>>> /* 0xFD7C8000 + 0x0A00 */ > > >>>>> #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) > > >>>>> > > >>>>> -/* 0xFD7D0000 + 0x0A00 */ > > >>>>> -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) > > >>>>> - > > >>>>> /* 0xFD7F0000 + 0x0A00 */ > > >>>>> #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) > > >>>>> > > >>>>> @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { > > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), > > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), > > >>>>> RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), > > >>>>> - > > >>>>> - /* SECURECRU_SOFTRST_CON00 */ > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), > > >>>>> - > > >>>>> - /* SECURECRU_SOFTRST_CON01 */ > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), > > >>>>> - > > >>>>> - /* SECURECRU_SOFTRST_CON02 */ > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), > > >>>>> - > > >>>>> - /* SECURECRU_SOFTRST_CON03 */ > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), > > >>>>> - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), > > >>>>> }; > > >>>>> > > >>>>> void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) > > >>>>> diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h > > >>>>> index d4264db2a07f..c0d08ae78cd5 100644 > > >>>>> --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h > > >>>>> +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h > > >>>>> @@ -716,39 +716,39 @@ > > >>>>> #define SRST_P_GPIO0 627 > > >>>>> #define SRST_GPIO0 628 > > >>>>> > > >>>>> -#define SRST_A_SECURE_NS_BIU 629 > > >>>>> -#define SRST_H_SECURE_NS_BIU 630 > > >>>>> -#define SRST_A_SECURE_S_BIU 631 > > >>>>> -#define SRST_H_SECURE_S_BIU 632 > > >>>>> -#define SRST_P_SECURE_S_BIU 633 > > >>>>> -#define SRST_CRYPTO_CORE 634 > > >>>>> - > > >>>>> -#define SRST_CRYPTO_PKA 635 > > >>>>> -#define SRST_CRYPTO_RNG 636 > > >>>>> -#define SRST_A_CRYPTO 637 > > >>>>> -#define SRST_H_CRYPTO 638 > > >>>>> -#define SRST_KEYLADDER_CORE 639 > > >>>>> -#define SRST_KEYLADDER_RNG 640 > > >>>>> -#define SRST_A_KEYLADDER 641 > > >>>>> -#define SRST_H_KEYLADDER 642 > > >>>>> -#define SRST_P_OTPC_S 643 > > >>>>> -#define SRST_OTPC_S 644 > > >>>>> -#define SRST_WDT_S 645 > > >>>>> - > > >>>>> -#define SRST_T_WDT_S 646 > > >>>>> -#define SRST_H_BOOTROM 647 > > >>>>> -#define SRST_A_DCF 648 > > >>>>> -#define SRST_P_DCF 649 > > >>>>> -#define SRST_H_BOOTROM_NS 650 > > >>>>> -#define SRST_P_KEYLADDER 651 > > >>>>> -#define SRST_H_TRNG_S 652 > > >>>>> - > > >>>>> -#define SRST_H_TRNG_NS 653 > > >>>>> -#define SRST_D_SDMMC_BUFFER 654 > > >>>>> -#define SRST_H_SDMMC 655 > > >>>>> -#define SRST_H_SDMMC_BUFFER 656 > > >>>>> -#define SRST_SDMMC 657 > > >>>>> -#define SRST_P_TRNG_CHK 658 > > >>>>> -#define SRST_TRNG_S 659 > > >>>>> +#define SRST_A_SECURE_NS_BIU 10 > > >>>> > > >>>> NAK. You just broke all users. > > >>> > > >>> If I'm reading the commit message correctly, all resets in that area > > >>> couldn't have any users to begin with, as the registers controlling them > > >>> are in the secure space, and need a higher exception level > > >>> > > >>> So if anything is trying to handle these resets, would end up with some > > >>> security exception right now. > > >>> > > >>> Though I guess we might want to use different names and not reuse the > > >>> existing ones. scmi clocks use a SCMI_CLK_* id scheme, so maybe SCMI_SRST_* ? > > >> > > >> I don't quite get what the patch wants to achieve. Why dropping driver > > >> support for given reset ID is connected with changing the value of > > >> binding for given reset? > > >> > > >> What is the point of this define SRST_A_SECURE_NS_BIU 10? > > > > > > This is about two different reset controllers. The IDs defined here > > > are used by the operating system to access the correct registers. > > > The kernel has a LUT from the ID to a register addresses, which is > > > something you asked for during upstreaming. > > > > > > The ID defined by Corentin is for reset control via SCMI firmware, > > > which has different number scheme than Linux. To me the suggestion > > > from Heiko looks sensible (i.e. create a new ID scheme and keep the > > > current one unchanged). > > > > So the binding is not for Linux but for FW? This should be explained in > > the commit msg. > > No. > > The current binding describes reset IDs, which are mapped by the > Linux driver to register offsets in the CRU (clock-reset-unit). > But accessing the crypto reset line directly from Linux (which > usually does not run in secure state) will fail. Accessing it > from correct security context with the current binding is fine > though. Considering we are sharing the bindings with e.g. > U-Boot, I suggest to keep the currently defined IDs. > > But Corentin tries to get this running on Linux. For that he > needs to ask the (SCMI) firmware running in secure state to > please take care of the reset. The firmware is using different > reset IDs (apparently the ones used by downstream Linux, which > are derived from register offset). > > In DT the difference looks like this (check the different phandles): > > #define SRST_A_SECURE_NS_BIU 629 > crypto-old { > // existing binding from Linux perspective > // reset via direct CRU access > // NOTE: permission denied > resets = <&cru SRST_A_SECURE_NS_BIU>; > }; > > #define SCMI_RST_A_SECURE_NS_BIU 10 > crypto-new { > // new binding from Linux perspective > // reset via SCMI firmware request > resets = <&scmi SCMI_RST_A_SECURE_NS_BIU>; > }; > > Instead of introducing SCMI_RST_A_SECURE_NS_BIU, Corentin > currently just redefines SRST_A_SECURE_NS_BIU. This is quite > misleading. If somebody does '<&cru SRST_A_SECURE_NS_BIU>' > with the '10' value for SCMI, it instead resets > SRST_A_TOP_M300_BIU. > > So my suggestion is to go with the suggestion from Heiko and > introduce SCMI_RST_A_SECURE_NS_BIU (or something similar). > That also matches how the SCMI clks on RK3588 and some other > platforms. See e.g.: > > of include/dt-bindings/clock/rockchip,rk3588-cru.h. > > Greetings, > > -- Sebastian Thanks for yours suggestions, I will do it that way. Regards
diff --git a/drivers/clk/rockchip/rst-rk3588.c b/drivers/clk/rockchip/rst-rk3588.c index e855bb8d5413..6556d9d3c7ab 100644 --- a/drivers/clk/rockchip/rst-rk3588.c +++ b/drivers/clk/rockchip/rst-rk3588.c @@ -16,9 +16,6 @@ /* 0xFD7C8000 + 0x0A00 */ #define RK3588_PHPTOPCRU_RESET_OFFSET(id, reg, bit) [id] = (0x8000*4 + reg * 16 + bit) -/* 0xFD7D0000 + 0x0A00 */ -#define RK3588_SECURECRU_RESET_OFFSET(id, reg, bit) [id] = (0x10000*4 + reg * 16 + bit) - /* 0xFD7F0000 + 0x0A00 */ #define RK3588_PMU1CRU_RESET_OFFSET(id, reg, bit) [id] = (0x30000*4 + reg * 16 + bit) @@ -806,45 +803,6 @@ static const int rk3588_register_offset[] = { RK3588_PMU1CRU_RESET_OFFSET(SRST_P_PMU0IOC, 5, 4), RK3588_PMU1CRU_RESET_OFFSET(SRST_P_GPIO0, 5, 5), RK3588_PMU1CRU_RESET_OFFSET(SRST_GPIO0, 5, 6), - - /* SECURECRU_SOFTRST_CON00 */ - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_NS_BIU, 0, 10), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_NS_BIU, 0, 11), - RK3588_SECURECRU_RESET_OFFSET(SRST_A_SECURE_S_BIU, 0, 12), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SECURE_S_BIU, 0, 13), - RK3588_SECURECRU_RESET_OFFSET(SRST_P_SECURE_S_BIU, 0, 14), - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_CORE, 0, 15), - - /* SECURECRU_SOFTRST_CON01 */ - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_PKA, 1, 0), - RK3588_SECURECRU_RESET_OFFSET(SRST_CRYPTO_RNG, 1, 1), - RK3588_SECURECRU_RESET_OFFSET(SRST_A_CRYPTO, 1, 2), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_CRYPTO, 1, 3), - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_CORE, 1, 9), - RK3588_SECURECRU_RESET_OFFSET(SRST_KEYLADDER_RNG, 1, 10), - RK3588_SECURECRU_RESET_OFFSET(SRST_A_KEYLADDER, 1, 11), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_KEYLADDER, 1, 12), - RK3588_SECURECRU_RESET_OFFSET(SRST_P_OTPC_S, 1, 13), - RK3588_SECURECRU_RESET_OFFSET(SRST_OTPC_S, 1, 14), - RK3588_SECURECRU_RESET_OFFSET(SRST_WDT_S, 1, 15), - - /* SECURECRU_SOFTRST_CON02 */ - RK3588_SECURECRU_RESET_OFFSET(SRST_T_WDT_S, 2, 0), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM, 2, 1), - RK3588_SECURECRU_RESET_OFFSET(SRST_A_DCF, 2, 2), - RK3588_SECURECRU_RESET_OFFSET(SRST_P_DCF, 2, 3), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_BOOTROM_NS, 2, 5), - RK3588_SECURECRU_RESET_OFFSET(SRST_P_KEYLADDER, 2, 14), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_S, 2, 15), - - /* SECURECRU_SOFTRST_CON03 */ - RK3588_SECURECRU_RESET_OFFSET(SRST_H_TRNG_NS, 3, 0), - RK3588_SECURECRU_RESET_OFFSET(SRST_D_SDMMC_BUFFER, 3, 1), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC, 3, 2), - RK3588_SECURECRU_RESET_OFFSET(SRST_H_SDMMC_BUFFER, 3, 3), - RK3588_SECURECRU_RESET_OFFSET(SRST_SDMMC, 3, 4), - RK3588_SECURECRU_RESET_OFFSET(SRST_P_TRNG_CHK, 3, 5), - RK3588_SECURECRU_RESET_OFFSET(SRST_TRNG_S, 3, 6), }; void rk3588_rst_init(struct device_node *np, void __iomem *reg_base) diff --git a/include/dt-bindings/reset/rockchip,rk3588-cru.h b/include/dt-bindings/reset/rockchip,rk3588-cru.h index d4264db2a07f..c0d08ae78cd5 100644 --- a/include/dt-bindings/reset/rockchip,rk3588-cru.h +++ b/include/dt-bindings/reset/rockchip,rk3588-cru.h @@ -716,39 +716,39 @@ #define SRST_P_GPIO0 627 #define SRST_GPIO0 628 -#define SRST_A_SECURE_NS_BIU 629 -#define SRST_H_SECURE_NS_BIU 630 -#define SRST_A_SECURE_S_BIU 631 -#define SRST_H_SECURE_S_BIU 632 -#define SRST_P_SECURE_S_BIU 633 -#define SRST_CRYPTO_CORE 634 - -#define SRST_CRYPTO_PKA 635 -#define SRST_CRYPTO_RNG 636 -#define SRST_A_CRYPTO 637 -#define SRST_H_CRYPTO 638 -#define SRST_KEYLADDER_CORE 639 -#define SRST_KEYLADDER_RNG 640 -#define SRST_A_KEYLADDER 641 -#define SRST_H_KEYLADDER 642 -#define SRST_P_OTPC_S 643 -#define SRST_OTPC_S 644 -#define SRST_WDT_S 645 - -#define SRST_T_WDT_S 646 -#define SRST_H_BOOTROM 647 -#define SRST_A_DCF 648 -#define SRST_P_DCF 649 -#define SRST_H_BOOTROM_NS 650 -#define SRST_P_KEYLADDER 651 -#define SRST_H_TRNG_S 652 - -#define SRST_H_TRNG_NS 653 -#define SRST_D_SDMMC_BUFFER 654 -#define SRST_H_SDMMC 655 -#define SRST_H_SDMMC_BUFFER 656 -#define SRST_SDMMC 657 -#define SRST_P_TRNG_CHK 658 -#define SRST_TRNG_S 659 +#define SRST_A_SECURE_NS_BIU 10 +#define SRST_H_SECURE_NS_BIU 11 +#define SRST_A_SECURE_S_BIU 12 +#define SRST_H_SECURE_S_BIU 13 +#define SRST_P_SECURE_S_BIU 14 +#define SRST_CRYPTO_CORE 15 + +#define SRST_CRYPTO_PKA 16 +#define SRST_CRYPTO_RNG 17 +#define SRST_A_CRYPTO 18 +#define SRST_H_CRYPTO 19 +#define SRST_KEYLADDER_CORE 25 +#define SRST_KEYLADDER_RNG 26 +#define SRST_A_KEYLADDER 27 +#define SRST_H_KEYLADDER 28 +#define SRST_P_OTPC_S 29 +#define SRST_OTPC_S 30 +#define SRST_WDT_S 31 + +#define SRST_T_WDT_S 32 +#define SRST_H_BOOTROM 33 +#define SRST_A_DCF 34 +#define SRST_P_DCF 35 +#define SRST_H_BOOTROM_NS 37 +#define SRST_P_KEYLADDER 46 +#define SRST_H_TRNG_S 47 + +#define SRST_H_TRNG_NS 48 +#define SRST_D_SDMMC_BUFFER 49 +#define SRST_H_SDMMC 50 +#define SRST_H_SDMMC_BUFFER 51 +#define SRST_SDMMC 52 +#define SRST_P_TRNG_CHK 53 +#define SRST_TRNG_S 54 #endif
While working on the rk3588 crypto driver, I loose lot of time understanding why resetting the IP failed. This is due to RK3588_SECURECRU_RESET_OFFSET being in the secure world, so impossible to operate on it from the kernel. All resets in this block must be handled via SCMI call. Signed-off-by: Corentin Labbe <clabbe@baylibre.com> --- drivers/clk/rockchip/rst-rk3588.c | 42 ------------ .../dt-bindings/reset/rockchip,rk3588-cru.h | 68 +++++++++---------- 2 files changed, 34 insertions(+), 76 deletions(-)