Re: [PATCH v3 18/26] arm64: dts: rockchip: rk3399: add crypto node

Robin Murphy
2022-03-22T12:00:15Z

On 2022-03-21 20:07, Corentin Labbe wrote:
> The rk3399 has a crypto IP handled by the rk3288 crypto driver so adds a
> node for it.
>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 88f26d89eea1..ca2c658371a5 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -573,6 +573,18 @@ saradc: saradc@ff100000 {
> status = "disabled";
> };
>
> + crypto0: crypto@ff8b0000 {
> + compatible = "rockchip,rk3399-crypto";
> + reg = <0x0 0xff8b0000 0x0 0x4000>,
> + <0x0 0xff8b8000 0x0 0x4000>;
> + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH 0>,
> + <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru SCLK_CRYPTO0>, <&cru HCLK_M_CRYPTO0>, <&cru HCLK_S_CRYPTO0>,
> + <&cru SCLK_CRYPTO1>, <&cru HCLK_M_CRYPTO1>, <&cru HCLK_S_CRYPTO1>;
> + resets = <&cru SRST_CRYPTO0>, <&cru SRST_CRYPTO0_S>, <&cru SRST_CRYPTO0_M>,
> + <&cru SRST_CRYPTO1>, <&cru SRST_CRYPTO1_S>, <&cru SRST_CRYPTO1_M>;
> + };

What's going on here? If these are simply two instances of the same IP
block as the evidence suggests, why are they crammed into a single DT
node rather than simply being described as two separate instances? I was
rather wondering what all the confusing mess in patch #16 was about,
until I got here.

If there's something in the crypto API that means the driver can't
simply naively register itself multiple times, there should be any
number of ways for the probe routine to keep track of whether it's
already registered something and associate any subsequent devices with
the first one internally if need be. Linux implementation details should
not leak out as non-standard DT weirdness.

I know the Rockchip IOMMU driver does this, but in that case the two
IOMMU instances are closely coupled and sharing work such that they
effectively need to be programmed identically at all times, so it was a
bit more justifiable. I don't know the full story here, but it certainly
looks like rk_get_engine_number() is just a means to schedule work on
any available unit independently, so looks like it wouldn't take much to
select between distinct devices at that point, and actually end up a lot
simpler and cleaner overall.

Robin.

> +
> i2c1: i2c@ff110000 {
> compatible = "rockchip,rk3399-i2c";
> reg = <0x0 0xff110000 0x0 0x1000>;