> -----Original Message-----
> From: Antoine Tenart <antoine.ten...@bootlin.com>
> Sent: Wednesday, July 31, 2019 2:26 PM
> To: Pascal van Leeuwen <pascalv...@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.ten...@bootlin.com;
> herb...@gondor.apana.org.au;
> da...@davemloft.net; Pascal Van Leeuwen <pvanleeu...@verimatrix.com>
> Subject: Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the
> EIP197
> without vendor firmware
>
> Hi Pascal,
>
> Thanks for reworking this not to include the firmware blob, the patch
> looks good and I only have minor comments.
>
> On Fri, Jul 26, 2019 at 02:43:25PM +0200, Pascal van Leeuwen wrote:
> > +
> > +static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> > + const struct firmware *fw)
> > +{
> > + const u32 *data = (const u32 *)fw->data;
> > + int i;
> >
> > /* Write the firmware */
> > for (i = 0; i < fw->size / sizeof(u32); i++)
> > writel(be32_to_cpu(data[i]),
> > priv->base + EIP197_CLASSIFICATION_RAMS + i *
> > sizeof(u32));
> >
> > - /* Disable access to the program memory */
> > - writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> > + return i - 2;
>
> Could you add a comment (or if applicable, a define) for this '- 2'?
>
Of course
> What happens if i < 2 ?
>
Ok, I did not consider that as it can't happen for any kind of legal FW. But it
wouldn't be pretty (neither would 2 itself, BTW). I could throw an error for it
but it wouldn't make that much sense as we don't do any checks on the firm-
ware *contents* either ... So either way, if your firmware file is no good, you
have a problem ...
> > + for (pe = 0; pe < priv->config.pes; pe++) {
> > + base = EIP197_PE_ICE_SCRATCH_RAM(pe);
> > + pollcnt = EIP197_FW_START_POLLCNT;
> > + while (pollcnt &&
> > + (readl(EIP197_PE(priv) + base +
> > + pollofs) != 1)) {
> > + pollcnt--;
> > + cpu_relax();
>
> You can probably use readl_relaxed() here.
>
Ok, will do
> > + }
> > + if (!pollcnt) {
> > + dev_err(priv->dev, "FW(%d) for PE %d failed to start",
> > + fpp, pe);
>
> A \n is missing at the end of the string.
>
Well spotted, will fix
> > +static bool eip197_start_firmware(struct safexcel_crypto_priv *priv,
> > + int ipuesz, int ifppsz, int minifw)
> > +{
> > + int pe;
> > + u32 val;
> > +
> > + for (pe = 0; pe < priv->config.pes; pe++) {
> > + /* Disable access to all program memory */
> > + writel(0, EIP197_PE(priv) + EIP197_PE_ICE_RAM_CTRL(pe));
> > +
> > + /* Start IFPP microengines */
> > + if (minifw)
> > + val = 0;
> > + else
> > + val = (((ifppsz - 1) & 0x7ff0) << 16) | BIT(3);
>
> Could you define the mask and the 'BIT(3)'?
>
i.e. add a define. Sure.
> > + writel(val, EIP197_PE(priv) + EIP197_PE_ICE_FPP_CTRL(pe));
> > +
> > + /* Start IPUE microengines */
> > + if (minifw)
> > + val = 0;
> > + else
> > + val = ((ipuesz - 1) & 0x7ff0) << 16 | BIT(3);
>
> Ditto.
>
> >
> > + if (!minifw) {
> > + /* Retry with minifw path */
> > + dev_dbg(priv->dev, "Firmware set not (fully) present or init
> > failed, falling
> back to BCLA mode");
>
> A \n is missing here.
>
Yes, thanks
> > + dir = "eip197_minifw";
> > + minifw = 1;
> > + goto retry_fw;
> > + }
> > +
> > + dev_dbg(priv->dev, "Firmware load failed.");
>
> Ditto.
>
Ack
> Thanks,
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
