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
