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'? What happens if i < 2 ? > + 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. > + } > + 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. > +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)'? > + 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. > + dir = "eip197_minifw"; > + minifw = 1; > + goto retry_fw; > + } > + > + dev_dbg(priv->dev, "Firmware load failed."); Ditto. Thanks, Antoine -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com