Re: [PATCHv2 3/3] crypto: inside-secure - add support for using the EIP197 without vendor firmware

[Antoine Tenart <antoine.tenart@bootlin.com>]

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