Re: [PATCH v1 1/1] ARM: dts: rockchip: set crypto default disabled on rk3288

[Heiko Stuebner]

Hi Elon,

Am Donnerstag, 29. August 2019, 13:31:00 CEST schrieb Elon Zhang:
> On 8/27/2019 22:28, Heiko Stuebner wrote:
> > Am Dienstag, 27. August 2019, 09:14:39 CEST schrieb Elon Zhang:
> >> Not every board needs to enable crypto node, so the node should
> >> be set default disabled in rk3288.dtsi and enabled in specific
> >> board dts file.
> > Can you give a bit more rationale here? There would need to be a very
> > specific reason because of the following:
> >
> > The crypto module is not wired to some board-specific components,
> > so its usability does not depend on the specific board at all.
> > Instead every board can just use it out of the box and the devicetree
> > is supposed to describe the hardware and is _not_ meant as a space
> > for user configuration.
> 
> Right for almost all normal hardware modules but the crypto module was 
> designed
> 
> for secure world. As a result,  the crypto module will become 
> inaccessible for linux kernel if secure world enable it.
> 
> We plan to enable the crypto module in secure world so we should set 
> crypto module default disabled in linux kernel.

ok ... I'm halfway convinced ;-) .

The big thing I want to see is that secure setting in the actual firmware.
Aka right now you probably have that in your Rockchip-specific ATF fork
and I really want to see the relevant change for public uboot or ATF.

I don't necessarily require it to be fully merged before taking this, but
I really want to see the change either on a mailing list or atf gerrit
instance [that makes the crypto engine secure only].

Rationale behind this is that we don't care very much about private stuff
that the general ecosystem doesn't benefit from.


Thanks
Heiko


> > So in fact the status property should probably go away completely from
> > the crypto node, as it's usable out of the box in all cases.
> >
> >
> > Heiko
> >
> >
> >
> >> Signed-off-by: Elon Zhang <zhangzj@xxxxxxxxxxxxxx>
> >> ---
> >>   arch/arm/boot/dts/rk3288.dtsi | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> >> index cc893e154fe5..d509aa24177c 100644
> >> --- a/arch/arm/boot/dts/rk3288.dtsi
> >> +++ b/arch/arm/boot/dts/rk3288.dtsi
> >> @@ -984,7 +984,7 @@
> >>   		clock-names = "aclk", "hclk", "sclk", "apb_pclk";
> >>   		resets = <&cru SRST_CRYPTO>;
> >>   		reset-names = "crypto-rst";
> >> -		status = "okay";
> >> +		status = "disabled";
> >>   	};
> >>   
> >>   	iep_mmu: iommu@ff900800 {
> >>
> >
> >
> >
> >
> >
> >
> 
> 
>