Re: [PATCH 5/6] reset: rockchip: secure reset must be used by SCMI

[Corentin LABBE <clabbe@baylibre.com>]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip