Re: CCREE performance on R-Car H3

2018-07-21 Thread Gilad Ben-Yossef 
Hi Geert,

On Thu, Jul 19, 2018 at 3:43 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> I've noticed CCREE is used with a LUKS-formatted disk, so I did some small
> performance benchmarks.  The disk is an old 160 GiB SATA hard drive,
> connected to either Salvator-XS (R-Car H3 ES2.0) or Koelsch (R-Car M2-W).
>
> hdparm -t /dev/sda (unencrypted data): 62 MB/s (both systems)
>
> hdparm -t /dev/dm-0 (encrypted data, LUKS AES):
>
> salvator-xs (CCREE): 15 MB/s
> salvator-xs (SW):62 MB/s
> koelsch (SW):47 MB/s
>
> I'm a bit disappointment by the results when using crypto acceleration.
> Apparently the in-kernel optimized arm64 implementation can decrypt at
> raw read speed, while CCREE can't keep up.

Interesting. What is the encryption sector size used?
If it is he default of 512 bytes, you might want to try setting the
new block_size DM-Crypt option. I believe it will have a large effect
on the result.


> Is this expected, and in line with your experiences?

Well, if you were indeed using the default 512 bytes and taking into
account that sadly the CryptoCell inside the R-Car is not wired to
utilize a coherent bus (the cache invalidation costs a fortune in
performance) it is not unexpected.

When reasoning about these things it is worth taking into account that
the design target of using CryptoCell is not to get the absolute best
raw performance as such but rather to get good performance / per cycle
/ per watt.

When the Arm AES Crypto Extension is used (I assume you are using that
and not, say, the NEON implementation) I expect the raw performance to
always be better than CryptoCell, certainly when it is not attached to
a coherent bus. However, if you check CPU utilization during the
operation (basically how much encryption is "stealing" computation
power from the CPU) and subsequent power consumption you are supposed
to see that there is a large area of work loads where it makes more
sense to utilize CryptoCell for disk encryption and use the CPU for
something else.

... in theory, at least. :-)

We've just in the middle of an office move here, but I hope things
will settle down during the ween and I'll be able to replicate your
test and see if I can advise better, but certainly using 4k encryption
section size is key, otherwise the overhead just kills everything.

Thanks!
Gilad
-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

[PATCH v2 1/5] crypto: ccree: correct host regs offset

2018-05-24 Thread Gilad Ben-Yossef 
The product signature and HW revision register have different offset on the
older HW revisions.
This fixes the problem of the driver failing sanity check on silicon
despite working on the FPGA emulation systems.

Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
Cc: sta...@vger.kernel.org
Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 drivers/crypto/ccree/cc_debugfs.c   | 7 +--
 drivers/crypto/ccree/cc_driver.c| 8 ++--
 drivers/crypto/ccree/cc_driver.h| 2 ++
 drivers/crypto/ccree/cc_host_regs.h | 6 --
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/ccree/cc_debugfs.c 
b/drivers/crypto/ccree/cc_debugfs.c
index 08f8db4..5ca184e 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
 static struct dentry *cc_debugfs_dir;
 
 static struct debugfs_reg32 debug_regs[] = {
-   CC_DEBUG_REG(HOST_SIGNATURE),
+   { .name = "SIGNATURE" }, /* Must be 0th */
+   { .name = "VERSION" }, /* Must be 1st */
CC_DEBUG_REG(HOST_IRR),
CC_DEBUG_REG(HOST_POWER_DOWN_EN),
CC_DEBUG_REG(AXIM_MON_ERR),
@@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
CC_DEBUG_REG(HOST_IMR),
CC_DEBUG_REG(AXIM_CFG),
CC_DEBUG_REG(AXIM_CACHE_PARAMS),
-   CC_DEBUG_REG(HOST_VERSION),
CC_DEBUG_REG(GPR_HOST),
CC_DEBUG_REG(AXIM_MON_COMP),
 };
@@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
struct debugfs_regset32 *regset;
struct dentry *file;
 
+   debug_regs[0].offset = drvdata->sig_offset;
+   debug_regs[1].offset = drvdata->ver_offset;
+
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..6f93ce7 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
if (hw_rev->rev >= CC_HW_REV_712) {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
+   new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
+   new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
} else {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
+   new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
+   new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
}
 
platform_set_drvdata(plat_dev, new_drvdata);
@@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
}
 
/* Verify correct mapping */
-   signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
+   signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
if (signature_val != hw_rev->sig) {
dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != 
expected=0x%08X\n",
signature_val, hw_rev->sig);
@@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 
/* Display HW versions */
dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver 
version %s\n",
-hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
+hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
 DRV_MODULE_VERSION);
 
rc = init_cc_regs(new_drvdata, true);
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index 2048fde..95f82b2 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -129,6 +129,8 @@ struct cc_drvdata {
enum cc_hw_rev hw_rev;
u32 hash_len_sz;
u32 axim_mon_offset;
+   u32 sig_offset;
+   u32 ver_offset;
 };
 
 struct cc_crypto_alg {
diff --git a/drivers/crypto/ccree/cc_host_regs.h 
b/drivers/crypto/ccree/cc_host_regs.h
index f510018..616b2e1 100644
--- a/drivers/crypto/ccree/cc_host_regs.h
+++ b/drivers/crypto/ccree/cc_host_regs.h
@@ -45,7 +45,8 @@
 #define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE0x1UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT  0x17UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE   0x1UL
-#define CC_HOST_SIGNATURE_REG_OFFSET   0xA24UL
+#define CC_HOST_SIGNATURE_712_REG_OFFSET   0xA24UL
+#define CC_HOST_SIGNATURE_630_REG_OFFSET   0xAC8UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT  0x0UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SIZE   0x20UL
 #define CC_HOST_BOOT_REG_OFFSET0xA28UL
@@ -105,7 +106,8 @@
 #define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE   0x1UL
 #defi

[PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

2018-05-24 Thread Gilad Ben-Yossef 
The patch set enables the use of CryptoCell found in some Renesas R-Car
Salvator-X boards and fixes some driver issues uncovered that prevented
to work properly.

Changes from v1:
- Properly fix the bug that caused us to read a bad signature register
  rather than dropping the check
- Proper DT fields as indicated by Geert Uytterhoeven.
- Better clock enabling as suggested by Geert Uytterhoeven.

Note! the last two patches in the set depend on the
"clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.

Gilad Ben-Yossef (5):
  crypto: ccree: correct host regs offset
  crypto: ccree: better clock handling
  crypto: ccree: silence debug prints
  clk: renesas: r8a7795: add ccree clock bindings
  arm64: dts: renesas: r8a7795: add ccree binding

 arch/arm64/boot/dts/renesas/r8a7795.dtsi |  9 +
 drivers/clk/renesas/r8a7795-cpg-mssr.c   |  1 +
 drivers/crypto/ccree/cc_debugfs.c|  7 +--
 drivers/crypto/ccree/cc_driver.c | 34 ++--
 drivers/crypto/ccree/cc_driver.h |  2 ++
 drivers/crypto/ccree/cc_host_regs.h  |  6 --
 6 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 2/5] crypto: ccree: better clock handling

2018-05-24 Thread Gilad Ben-Yossef 
Use managed clock handling, differentiate between no clock (possibly OK)
and clock init failure (never OK) and correctly handle clock detection
being deferred.

Suggested-by: Geert Uytterhoeven <ge...@linux-m68k.org>
Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 drivers/crypto/ccree/cc_driver.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f93ce7..b266657 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
u64 dma_mask;
const struct cc_hw_data *hw_rev;
const struct of_device_id *dev_id;
+   struct clk *clk;
int rc = 0;
 
new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
@@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
platform_set_drvdata(plat_dev, new_drvdata);
new_drvdata->plat_dev = plat_dev;
 
-   new_drvdata->clk = of_clk_get(np, 0);
+   clk = devm_clk_get(dev, NULL);
+   if (IS_ERR(clk))
+   switch (PTR_ERR(clk)) {
+   /* Clock is optional so this might be fine */
+   case -ENOENT:
+   break;
+
+   /* Clock not available, let's try again soon */
+   case -EPROBE_DEFER:
+   return -EPROBE_DEFER;
+
+   default:
+   dev_err(dev, "Error getting clock: %ld\n",
+   PTR_ERR(clk));
+   return PTR_ERR(clk);
+   }
+   new_drvdata->clk = clk;
+
new_drvdata->coherent = of_dma_is_coherent(np);
 
/* Get device resources */
-- 
2.7.4

[PATCH v2 3/5] crypto: ccree: silence debug prints

2018-05-24 Thread Gilad Ben-Yossef 
The cache parameter register configuration was being too verbose.
Use dev_dbg() to only provide the information if needed.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 drivers/crypto/ccree/cc_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index b266657..1e40e76 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -168,14 +168,14 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool 
is_probe)
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
if (is_probe)
-   dev_info(dev, "Cache params previous: 0x%08X\n", val);
+   dev_dbg(dev, "Cache params previous: 0x%08X\n", val);
 
cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params);
val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
if (is_probe)
-   dev_info(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
-val, cache_params);
+   dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
+   val, cache_params);
 
return 0;
 }
-- 
2.7.4

[PATCH v2 4/5] clk: renesas: r8a7795: add ccree clock bindings

2018-05-24 Thread Gilad Ben-Yossef 
This patch adds the clock used by the CryptoCell 630p instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---

This patch depends upon the "clk: renesas: r8a7795: Add CR clock" patch
from Geert Uytterhoeven.

 drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c 
b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index e5b1865..a85dd50 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -133,6 +133,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
+   DEF_MOD("sceg-pub",  229,   R8A7795_CLK_CR),
DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
-- 
2.7.4

[PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-24 Thread Gilad Ben-Yossef 
Add bindings for CryptoCell instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index d842940..3ac75db 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -528,6 +528,15 @@
status = "disabled";
};
 
+   arm_cc630p: crypto@e6601000 {
+   compatible = "arm,cryptocell-630p-ree";
+   interrupts = ;
+   reg = <0x0 0xe6601000 0 0x1000>;
+   clocks = < CPG_MOD 229>;
+   resets = < 229>;
+   power-domains = < R8A7795_PD_ALWAYS_ON>;
+   };
+
i2c3: i2c@e66d {
#address-cells = <1>;
#size-cells = <0>;
-- 
2.7.4

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-24 Thread Gilad Ben-Yossef 
On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>> <ge...@linux-m68k.org> wrote:
>>> 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).
>>
>> I was trying to do as you suggested but I didn't quite get what is the
>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>
> It's the (optional) name of the clock, helpful in case there is more than one.
> In your case, NULL is fine.
>

I have assumed as much and tried it, it did not work and so I assumed
I am missing something and asked you.
It turns out I was missing the fact I was using the wrong device tree
file... :-(

So thanks, it works now :-)

Having said that, while using devm)clk_get() is a better approach, it
does not seems to distinguish
between no "clocks" and a failure to clock information - it returns
ENOENT in both cases as well.

Thanks,
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

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-21 Thread Gilad Ben-Yossef 
On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:

>
> 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).
>

I was trying to do as you suggested but I didn't quite get what is the
dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.

I see what of_clk_get() is doing, so can replicate that but it seems
an over kill.

Any ideas?

Thanks,
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

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-17 Thread Gilad Ben-Yossef 
On Thu, May 17, 2018 at 4:35 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 3:09 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>> <ge...@linux-m68k.org> wrote:
>>> 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.
>
> Sorry, I had completely forgotten about the first patch from the series.
> With that applied, it continues:
>
> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
> 0x, Driver version 4.0
> ccree e6601000.crypto: Cache params previous: 0x0777
> ccree e6601000.crypto: Cache params current: 0x
> (expect: 0x)
> alg: No test for cts1(cbc(aes)) (cts1-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),cbc(aes))
> (authenc-xcbc-aes-cbc-aes-ccree)
> alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes)))
> (authenc-xcbc-aes-rfc3686-ctr-aes-ccree)
> ccree e6601000.crypto: ARM ccree device initialized
>
> Is HW version 0x expected?

It's related to the problem with reading the wrong register I've
mentioned before.

>
>> If you take all the 3 patches, it will work.
>
> is there an easy way to test proper operation?

The lines of the form "  alg: No test for cts1(cbc(aes))
(cts1-cbc-aes-ccree)" indicates
you have the Crypto API testmgr enable (or rather not disabled would
be more accurate) so every
cryptographic algorithm except those specified in these messages was
tested with test
vectors from crypto/testmgr.c upon registration. If you don't seen
failure warnings, it works.

You can also check /proc/crypto for all the algorithm with ccree
listed as their driver and check
that their test passed.


> I enabled CONFIG_CRYPT_TEST, and did insmod tcrypt.ko, but I mostly see
> "Failed to load transform" messages.
>

tcrypt.ko is a rather crude developer tool. It has hard coded lists of
test for different algorithms that does
not take into account if some crypto algs are enagled in the build or
not. It's more of a stress test.


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

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-17 Thread Gilad Ben-Yossef 
On Thu, May 17, 2018 at 12:04 PM, Simon Horman <ho...@verge.net.au> wrote:
> On Thu, May 17, 2018 at 11:01:57AM +0300, Gilad Ben-Yossef wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman <ho...@verge.net.au> wrote:
>> > On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> >> Hi Gilad,
>> >>
>> >> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef <gi...@benyossef.com> 
>> >> wrote:
>> >> > Add bindings for CryptoCell instance in the SoC.
>> >> >
>> >> > Signed-off-by: Gilad Ben-Yossef <gi...@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 = ;
>> >> > +   #interrupt-cells = <2>;
>> >>
>> >> I believe the #interrupt-cells property is not needed.
>> >>
>> >> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> >> > +   clocks = < CPG_MOD 229>;
>> >> > +   };
>> >>
>> >> The rest looks good, but I cannot verify the register block.
>> >>
>> >> > +
>> >> > i2c3: i2c@e66d {
>> >> > #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.
>
> I think its fine if it does not yet work.
> But not if its causes things that previously worked to stop working.

Based on the further discussion with Geert my recommendation is to
drop my patch for now,
take Geert CR clock  patch and I will follow up next week with a v2
that fixes the clock
handing as discussed with Geert.

Many thanks,
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

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-17 Thread Gilad Ben-Yossef 
On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Thu, May 17, 2018 at 10:01 AM, Gilad Ben-Yossef <gi...@benyossef.com> 
> wrote:
>> On Wed, May 16, 2018 at 10:43 AM, Simon Horman <ho...@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 <gi...@benyossef.com> 
>>>> wrote:
>>>> > Add bindings for CryptoCell instance in the SoC.
>>>> >
>>>> > Signed-off-by: Gilad Ben-Yossef <gi...@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 = ;
>>>> > +   #interrupt-cells = <2>;
>>>>
>>>> I believe the #interrupt-cells property is not needed.
>>>>
>>>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>>>> > +   clocks = < CPG_MOD 229>;
>
> Missing "power-domains = < 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@e66d {
>>>> > #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=0x
> != 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

Re: [PATCH 1/3] crypto: ccree: drop signature register check

2018-05-17 Thread Gilad Ben-Yossef 
Herbert,

On Tue, May 15, 2018 at 3:29 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote:
> We were using the content of the signature register as a sanity
> check for the hardware functioning but it turns out not all
> implementers use the same values so the check is giving false
> negative on certain SoCs and so we drop it.
>

Please drop this patch. I have found a better fix and will send a v2 soon.

Thanks,
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

Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-17 Thread Gilad Ben-Yossef 
On Wed, May 16, 2018 at 10:43 AM, Simon Horman <ho...@verge.net.au> wrote:
> On Tue, May 15, 2018 at 04:50:44PM +0200, Geert Uytterhoeven wrote:
>> Hi Gilad,
>>
>> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef <gi...@benyossef.com> 
>> wrote:
>> > Add bindings for CryptoCell instance in the SoC.
>> >
>> > Signed-off-by: Gilad Ben-Yossef <gi...@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 = ;
>> > +   #interrupt-cells = <2>;
>>
>> I believe the #interrupt-cells property is not needed.
>>
>> > +   reg = <0x0 0xe6601000 0 0x1000>;
>> > +   clocks = < CPG_MOD 229>;
>> > +   };
>>
>> The rest looks good, but I cannot verify the register block.
>>
>> > +
>> > i2c3: i2c@e66d {
>> >     #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.

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

Re: [PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

2018-05-17 Thread Gilad Ben-Yossef 
On Tue, May 15, 2018 at 5:47 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Tue, May 15, 2018 at 2:29 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote:
>> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>>
>> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
>
> Thanks for your patch!
>
>> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
>> @@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata 
>> = {
>> DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
>> DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
>> +   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
>
> I don't know if "ccree" is the proper name for this clock, as there
> may be multiple
> instances.

I'd be happy to rename it to anything else. Suggestions?

> I also can't verify the parent clock.

I'm afraid I can't really help. This is based on code snippet from
Renesas. I verified it works but
I am not an expert on the clock settings :-(

>
>> DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
>> DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
>> DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds



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

Re: Booting Salvator-X ES1 board with upstream kernel

2018-05-15 Thread Gilad Ben-Yossef 
On Mon, May 14, 2018 at 5:29 PM, Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Thu, May 10, 2018 at 10:12 AM, Gilad Ben-Yossef <gi...@benyossef.com> 
> wrote:
>> I am trying to add support for the CryptoCell security IP in the
>> R-Rcar boards to mainline but I've run into some trouble.
>
> Ah, we have found a user ;-)

Yes. I would be very happy if all our customer would be as cooperative
as Renesas.
That would make my life and the life of the users easier :-)

I've also have the HiKey960 on my todo list.

>
>> I have an R-Car 3rd gen Salvator-X ES1 board. I've been able to boot
>> Linux 4.9 on it from the rareness-bsp tree, using the defconfig and
>> the r8a7795-es1-salvator-x DTB with no problems, so the HW is fine.
>>
>> However, I can't seem to boot an upstream kernel (Linus master or
>> soc-for-v4.17 branch) on it. I just get total silence on the UART
>> after u-boot.
>>
>> Any tips or ideas for me to try?
>
> Please try appending the following to the kernel command line:
>
> earlycon keep_bootcon
>
> FWIW, I'm using:
>
> tftpboot 0x4808 h3-salvator-x/Image
> tftpboot 0x49f0 h3-salvator-x/r8a7795-es1-salvator-x.dtb
> booti 0x4808 - 0x49f0
>
> Which firmware revision do you have? It may be too old to boot moden
> (large) kernel images. Check the first line of BL2 output.
>
> Which config are you using? Please try renesas_defconfig from Simon's
> repository.

That was it. I was building with defoncfig and getting a kernel that won't be.
Simon's config did the trick.

Many thanks!

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

[PATCH 1/3] crypto: ccree: drop signature register check

2018-05-15 Thread Gilad Ben-Yossef 
We were using the content of the signature register as a sanity
check for the hardware functioning but it turns out not all
implementers use the same values so the check is giving false
negative on certain SoCs and so we drop it.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 drivers/crypto/ccree/cc_driver.c | 18 +++---
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..f8ff358 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -38,21 +38,20 @@ MODULE_PARM_DESC(cc_dump_bytes, "Dump buffers to kernel log 
as debugging aid");
 struct cc_hw_data {
char *name;
enum cc_hw_rev rev;
-   u32 sig;
 };
 
 /* Hardware revisions defs. */
 
 static const struct cc_hw_data cc712_hw = {
-   .name = "712", .rev = CC_HW_REV_712, .sig =  0xDCC71200U
+   .name = "712", .rev = CC_HW_REV_712
 };
 
 static const struct cc_hw_data cc710_hw = {
-   .name = "710", .rev = CC_HW_REV_710, .sig =  0xDCC63200U
+   .name = "710", .rev = CC_HW_REV_710
 };
 
 static const struct cc_hw_data cc630p_hw = {
-   .name = "630P", .rev = CC_HW_REV_630, .sig = 0xDCC63000U
+   .name = "630P", .rev = CC_HW_REV_630
 };
 
 static const struct of_device_id arm_ccree_dev_of_match[] = {
@@ -186,7 +185,6 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
struct cc_drvdata *new_drvdata;
struct device *dev = _dev->dev;
struct device_node *np = dev->of_node;
-   u32 signature_val;
u64 dma_mask;
const struct cc_hw_data *hw_rev;
const struct of_device_id *dev_id;
@@ -275,16 +273,6 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
return rc;
}
 
-   /* Verify correct mapping */
-   signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
-   if (signature_val != hw_rev->sig) {
-   dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != 
expected=0x%08X\n",
-   signature_val, hw_rev->sig);
-   rc = -EINVAL;
-   goto post_clk_err;
-   }
-   dev_dbg(dev, "CC SIGNATURE=0x%08X\n", signature_val);
-
/* Display HW versions */
dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver 
version %s\n",
 hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
-- 
2.7.4

[PATCH 2/3] clk: renesas: r8a7795: Add ccree clock

2018-05-15 Thread Gilad Ben-Yossef 
This patch adds the clock used by the CryptoCell 630p instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c 
b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index 775b0ce..642706a 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -132,6 +132,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
DEF_MOD("sys-dmac2", 217,   R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac1", 218,   R8A7795_CLK_S0D3),
DEF_MOD("sys-dmac0", 219,   R8A7795_CLK_S0D3),
+   DEF_MOD("ccree", 229,   R8A7795_CLK_S3D2),
DEF_MOD("cmt3",  300,   R8A7795_CLK_R),
DEF_MOD("cmt2",  301,   R8A7795_CLK_R),
DEF_MOD("cmt1",  302,   R8A7795_CLK_R),
-- 
2.7.4

[PATCH 0/3] enable ccree on Renesas R-Car platform

2018-05-15 Thread Gilad Ben-Yossef 
The following patch set enables CryptoCell present in the Renesas
R-Car SoC.

Gilad Ben-Yossef (3):
  crypto: ccree: drop signature register check
  clk: renesas: r8a7795: Add ccree clock
  arm64: dts: renesas: r8a7795: add ccree binding

 arch/arm64/boot/dts/renesas/r8a7795.dtsi |  8 
 drivers/clk/renesas/r8a7795-cpg-mssr.c   |  1 +
 drivers/crypto/ccree/cc_driver.c | 18 +++---
 3 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding

2018-05-15 Thread Gilad Ben-Yossef 
Add bindings for CryptoCell instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 91486b4..6c76841 100644
--- 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 = ;
+   #interrupt-cells = <2>;
+   reg = <0x0 0xe6601000 0 0x1000>;
+   clocks = < CPG_MOD 229>;
+   };
+
i2c3: i2c@e66d {
#address-cells = <1>;
#size-cells = <0>;
-- 
2.7.4

Re: Booting Salvator-X ES1 board with upstream kernel

2018-05-10 Thread Gilad Ben-Yossef 
On Thu, May 10, 2018 at 11:19 AM, Magnus Damm <magnus.d...@gmail.com> wrote:
> Hi Gilad,
>
> Thanks for your email.
>
> On Thu, May 10, 2018 at 5:12 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote:
>> Hi there,
>>
>> I am trying to add support for the CryptoCell security IP in the
>> R-Rcar boards to mainline but I've run into some trouble.
>>
>> I have an R-Car 3rd gen Salvator-X ES1 board. I've been able to boot
>> Linux 4.9 on it from the rareness-bsp tree, using the defconfig and
>> the r8a7795-es1-salvator-x DTB with no problems, so the HW is fine.
>>
>> However, I can't seem to boot an upstream kernel (Linus master or
>> soc-for-v4.17 branch) on it. I just get total silence on the UART
>> after u-boot.
>>
>> Any tips or ideas for me to try?
>
> Just to clarify, are you using r8a7795 R-Car H3 SoC ES1.0, or some other SoC?
>
> Salvator-X comes in different configurations.

To the best of my knowledge I am indeed using the r8a7795 R-Car H3 SoC ES1.0

Thanks,
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

Booting Salvator-X ES1 board with upstream kernel

2018-05-10 Thread Gilad Ben-Yossef 
Hi there,

I am trying to add support for the CryptoCell security IP in the
R-Rcar boards to mainline but I've run into some trouble.

I have an R-Car 3rd gen Salvator-X ES1 board. I've been able to boot
Linux 4.9 on it from the rareness-bsp tree, using the defconfig and
the r8a7795-es1-salvator-x DTB with no problems, so the HW is fine.

However, I can't seem to boot an upstream kernel (Linus master or
soc-for-v4.17 branch) on it. I just get total silence on the UART
after u-boot.

Any tips or ideas for me to try?

Many thanks,
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