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@xxxxxxxxxxxx>
>>>>> ---
>>>>> 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
