| Message ID | 20170524190652.13278-11-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 24, 2017 at 09:06:51PM +0200, Antoine Tenart wrote: > The run-time self-tests fail quite early, as soon as the input block > size is larger than 64 bytes: > > alg: hash: Test 4 failed for sha1-sun4i-ss > 00000000: b9 c9 1e 52 c0 26 d8 39 81 ff f2 3c 99 b1 27 b2 > 00000010: 30 d6 c9 85 > > One thing to notice is the value of the last word, which is the one > expected (it can sometime be the last two words). The datasheet isn't > very clear about when the digest is ready to retrieve and is seems the > bit SS_DATA_END is cleared when the digest was computed *but* that > doesn't mean the digest is ready to retrieve in the registers. > > A ndelay(1) is added before reading the computed digest to ensure it is > available in the SS_MD[] registers. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/crypto/sunxi-ss/sun4i-ss-hash.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c > index 685de5b6ab17..6da8d2bbd4da 100644 > --- a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c > +++ b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c > @@ -358,6 +358,15 @@ static int sun4i_hash(struct ahash_request *areq) > goto release_ss; > } > > + /* > + * The datasheet isn't very clear about when to retrieve the digest. The > + * bit SS_DATA_END is cleared when the engine has processed the data and > + * when the digest is computed *but* it doesn't mean the digest is > + * available in the diesgt registers. Hence the delay to be sure we can Hello Small typo here (diesgt/digest) > + * read it. > + */ > + ndelay(1); > + > for (i = 0; i < crypto_ahash_digestsize(tfm) / 4; i++) > op->hash[i] = readl(ss->base + SS_MD0 + i * 4); > > @@ -446,6 +455,15 @@ static int sun4i_hash(struct ahash_request *areq) > goto release_ss; > } > > + /* > + * The datasheet isn't very clear about when to retrieve the digest. The > + * bit SS_DATA_END is cleared when the engine has processed the data and > + * when the digest is computed *but* it doesn't mean the digest is > + * available in the diesgt registers. Hence the delay to be sure we can and here. This behaviour is very strange since I didnt get it on other platform. Could you give me the board name where you get it ? Does any wmb() after the writel(..SS_DATA_END..) do the trick ? Which speed are the SS clocks ? Anyway the whole serie seems good, I will test it just after this weekend. Thanks Regards Corentin Labbe
Hi Corentin, On Fri, May 26, 2017 at 04:55:01PM +0200, Corentin Labbe wrote: > On Wed, May 24, 2017 at 09:06:51PM +0200, Antoine Tenart wrote: > > > > + /* > > + * The datasheet isn't very clear about when to retrieve the digest. The > > + * bit SS_DATA_END is cleared when the engine has processed the data and > > + * when the digest is computed *but* it doesn't mean the digest is > > + * available in the diesgt registers. Hence the delay to be sure we can > > Small typo here (diesgt/digest) Oops. I'll fix it. > This behaviour is very strange since I didnt get it on other platform. > Could you give me the board name where you get it ? I used a CHIP (sun5i-r8-chip). > Does any wmb() after the writel(..SS_DATA_END..) do the trick ? Nope. I tried this and it didn't help. > Which speed are the SS clocks ? The AHB SS clk is running at 300 MHz and the SS clk at 150 MHz. SS clk is at the expected rate but the AHB SS clk has a higher rate that what's expected. In the probing function only the SS clk rate is explicitly set. I tried to set the AHB clk rate as well and removed the delays. This didn't fix the framework selftests at boot time. Is there any reason the AHB SS clk rate isn't explicitly set when probing the driver? (Should it?) > Anyway the whole serie seems good, I will test it just after this weekend. Let me know if your tests went well, I'll send a v2 fixing the typo below then (and setting the AHB SS clk rate explicitly if needed). Thanks, Antoine
On Mon, May 29, 2017 at 10:09:44AM +0200, Antoine Tenart wrote: > > Which speed are the SS clocks ? > > The AHB SS clk is running at 300 MHz and the SS clk at 150 MHz. SS clk > is at the expected rate but the AHB SS clk has a higher rate that what's > expected. > > In the probing function only the SS clk rate is explicitly set. I tried > to set the AHB clk rate as well and removed the delays. This didn't fix > the framework selftests at boot time. Is there any reason the AHB SS clk > rate isn't explicitly set when probing the driver? (Should it?) It probably shouldn't. The AHB clock is shared by most of the drivers, some of them actually using that clock to generate their signals. You would have to unbreak all those drivers first, which is probably not needed at all. I haven't seen a case where a block had a module clock and did care for its AHB clock rate. Maxime
Hi Maxime, On Mon, May 29, 2017 at 10:29:31AM +0200, Maxime Ripard wrote: > On Mon, May 29, 2017 at 10:09:44AM +0200, Antoine Tenart wrote: > > > Which speed are the SS clocks ? > > > > The AHB SS clk is running at 300 MHz and the SS clk at 150 MHz. SS clk > > is at the expected rate but the AHB SS clk has a higher rate that what's > > expected. > > > > In the probing function only the SS clk rate is explicitly set. I tried > > to set the AHB clk rate as well and removed the delays. This didn't fix > > the framework selftests at boot time. Is there any reason the AHB SS clk > > rate isn't explicitly set when probing the driver? (Should it?) > > It probably shouldn't. > > The AHB clock is shared by most of the drivers, some of them actually > using that clock to generate their signals. > > You would have to unbreak all those drivers first, which is probably > not needed at all. I haven't seen a case where a block had a module > clock and did care for its AHB clock rate. OK, makes sense. I'll wait for Corentin to test the series, and I'll send a v2 only fixing the typos then. Thanks! Antoine
diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c index 685de5b6ab17..6da8d2bbd4da 100644 --- a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c +++ b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c @@ -358,6 +358,15 @@ static int sun4i_hash(struct ahash_request *areq) goto release_ss; } + /* + * The datasheet isn't very clear about when to retrieve the digest. The + * bit SS_DATA_END is cleared when the engine has processed the data and + * when the digest is computed *but* it doesn't mean the digest is + * available in the diesgt registers. Hence the delay to be sure we can + * read it. + */ + ndelay(1); + for (i = 0; i < crypto_ahash_digestsize(tfm) / 4; i++) op->hash[i] = readl(ss->base + SS_MD0 + i * 4); @@ -446,6 +455,15 @@ static int sun4i_hash(struct ahash_request *areq) goto release_ss; } + /* + * The datasheet isn't very clear about when to retrieve the digest. The + * bit SS_DATA_END is cleared when the engine has processed the data and + * when the digest is computed *but* it doesn't mean the digest is + * available in the diesgt registers. Hence the delay to be sure we can + * read it. + */ + ndelay(1); + /* Get the hash from the device */ if (op->mode == SS_OP_SHA1) { for (i = 0; i < 5; i++) {
The run-time self-tests fail quite early, as soon as the input block size is larger than 64 bytes: alg: hash: Test 4 failed for sha1-sun4i-ss 00000000: b9 c9 1e 52 c0 26 d8 39 81 ff f2 3c 99 b1 27 b2 00000010: 30 d6 c9 85 One thing to notice is the value of the last word, which is the one expected (it can sometime be the last two words). The datasheet isn't very clear about when the digest is ready to retrieve and is seems the bit SS_DATA_END is cleared when the digest was computed *but* that doesn't mean the digest is ready to retrieve in the registers. A ndelay(1) is added before reading the computed digest to ensure it is available in the SS_MD[] registers. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/sunxi-ss/sun4i-ss-hash.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)