LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Gilad Ben-Yossef <gilad@benyossef.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Simon Horman <horms@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Ofir Drang <ofir.drang@arm.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Subject: Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding
Date: Thu, 17 May 2018 16:09:51 +0300	[thread overview]
Message-ID: <CAOtvUMdVNX_vBRofpc5rNiVL-a1NvWMDyWSOaXKYPkm57wkR_A@mail.gmail.com> (raw)
In-Reply-To: <CAMuHMdVM55S7+PdnKusX-qkTxioc=0kQ6EGSbwv+kbLk5RCUYw@mail.gmail.com>

On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman <horms@verge.net.au> wrote:
>>> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>>>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>>> > Add bindings for CryptoCell instance in the SoC.
>>>> >
>>>> > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>>>>
>>>> Thanks for your patch!
>>>>
>>>> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
>>>> > @@ -528,6 +528,14 @@
>>>> >                         status = "disabled";
>>>> >                 };
>>>> >
>>>> > +               arm_cc630p: crypto@e6601000 {
>>>> > +                       compatible = "arm,cryptocell-630p-ree";
>>>> > +                       interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
>>>> > +                       #interrupt-cells = <2>;
>>>>
>>>> I believe the #interrupt-cells property is not needed.
>>>>
>>>> > +                       reg = <0x0 0xe6601000 0 0x1000>;
>>>> > +                       clocks = <&cpg CPG_MOD 229>;
>
> Missing "power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;", as
> the Secure Engine is part of the CPG/MSSR clock domain (see below [*]).

Thank you. I didn't get this information from Renesas :-)

>
>>>> > +               };
>>>>
>>>> The rest looks good, but I cannot verify the register block.
>>>>
>>>> > +
>>>> >                 i2c3: i2c@e66d0000 {
>>>> >                         #address-cells = <1>;
>>>> >                         #size-cells = <0>;
>>>
>>> Thanks, I have applied this after dropping the #interrupt-cells property.
>>
>> Thanks you!
>>
>> Alas, it will not work without the clk patch (the previous one in the
>> series) so they need to be
>> taken or dropped together.
>
> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
> does not distinguish between the absence of the clock property, and an
> actual error in getting the clock, and never considers any error a failure
> (incl. -PROBE_DEFER).
>
> As of_clk_get() returns -ENOENT for both a missing clock property and a
> missing clock, you should use (devm_)clk_get() instead, and distinguish
> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>

Thank you, this is very valuable. I will do as you suggested.


> Hence in the absence of the clock patch, the driver accesses the crypto
> engine while its module clock is turned off, leading to:
>
>     ccree e6601000.crypto: Invalid CC signature: SIGNATURE=0x00000000
> != expected=0xDCC63000
>
> You must be lucky, though, usually you get an imprecise external abort
> later, crashing the whole system ;-)
>
> So I think this patch should be dropped for now.
>
> However, even with your clock patch, the signature checking fails for me,
> on both R-Car H3 ES1.0 and ES2.0.
> Does this need changes to the ARM Trusted Firmware, to allow Linux to
> access the public SCEG module?

Well, this is actually something different. If you look you will
notice that my patch was part of a 3 part patch series,
the first of which disabled this test.

If you take all the 3 patches, it will work.

To make things more interesting, I have since sending the patch
learned WHY the test does not work, so disabling
it is not needed - to make a long story short, I was reading the wrong
register that just happens to have the right value
in our FPGA based tests systems but does not in the real silicon
implementations.

But you are right - if the clock is not enabled and you are try to
read from the register the system does freeze.

I will send a fixed v2. based on your patch enabling the CR clock.

>
> [*] More on the subject of clock control:
> At least for Renesas SoCs, where the module is part of a clock domain, and
> can be controlled automatically by Runtime PM, you could drop the explicit
> clock control, and use Runtime PM instead
> (pm_runtime_{enable,get_sync,put,disable}()).  That would allow the driver
> to work on systems with any kind of PM Domains, too.
> Depending on the other platforms that include a CryptoCell and their
> (non)reliance on PM Domains, you may have to keep the explicit clock
> handling, in addition to Runtime PM.
>



> To decrease power consumption, I suggest to move the clock and/or Runtime
> PM handling to the routines that actually use the hardware, instead of
> powering the module in the probe routine.
>

This is very interesting and I will give it a try.

Thanks again!
Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

  reply	other threads:[~2018-05-17 13:09 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 12:29 [PATCH 0/3] enable ccree on Renesas R-Car platform Gilad Ben-Yossef
2018-05-15 12:29 ` [PATCH 1/3] crypto: ccree: drop signature register check Gilad Ben-Yossef
2018-05-17 12:54   ` Gilad Ben-Yossef
2018-05-15 12:29 ` [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock Gilad Ben-Yossef
2018-05-15 14:47   ` Geert Uytterhoeven
2018-05-17  8:00     ` Gilad Ben-Yossef
2018-05-17  8:41       ` Geert Uytterhoeven
2018-05-15 12:29 ` [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding Gilad Ben-Yossef
2018-05-15 14:50   ` Geert Uytterhoeven
2018-05-16  7:43     ` Simon Horman
2018-05-17  8:01       ` Gilad Ben-Yossef
2018-05-17  9:04         ` Simon Horman
2018-05-17 13:12           ` Gilad Ben-Yossef
2018-05-18  9:50             ` Simon Horman
2018-05-17 10:16         ` Geert Uytterhoeven
2018-05-17 13:09           ` Gilad Ben-Yossef [this message]
2018-05-17 13:35             ` Geert Uytterhoeven
2018-05-17 13:41               ` Gilad Ben-Yossef
2018-05-17 13:49                 ` Geert Uytterhoeven
2018-05-17 13:17           ` Geert Uytterhoeven
2018-05-21 13:43           ` Gilad Ben-Yossef
2018-05-22  7:48             ` Geert Uytterhoeven
2018-05-24 13:20               ` Gilad Ben-Yossef
2018-05-24 13:44                 ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOtvUMdVNX_vBRofpc5rNiVL-a1NvWMDyWSOaXKYPkm57wkR_A@mail.gmail.com \
    --to=gilad@benyossef.com \
    --cc=catalin.marinas@arm.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=geert@linux-m68k.org \
    --cc=herbert@gondor.apana.org.au \
    --cc=horms@verge.net.au \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=magnus.damm@gmail.com \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=ofir.drang@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).