> -----Original Message----- > From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx> > Sent: Wednesday, July 31, 2019 2:26 PM > To: Pascal van Leeuwen <pascalvanl@xxxxxxxxx> > Cc: linux-crypto@xxxxxxxxxxxxxxx; antoine.tenart@xxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; > davem@xxxxxxxxxxxxx; Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > 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