[U-Boot] [PATCH resend] misc/crypto: Add support for C3

[Vipin Kumar <vipin.kumar@st.com>]

On 12/6/2012 5:26 PM, Stefan Roese wrote:
> On 12/06/2012 10:15 AM, Vipin Kumar wrote:
>> C3 is a cryptographic controller which is used by the SPL when DDR ECC support
>> is enabled.
>>
>> Basically, the DDR ECC feature requires the initialization of ECC values before
>> the DDR can actually be used. To accomplish this, the complete on board DDR is
>> initialized with zeroes. This initialization can be done using
>>    * CPU
>>    * CPU (with Dcache enabled)
>>    * C3
>>
>> The current SPL code uses C3 because the initialization using the CPU is slow
>> and we do not have enough memory in SPL to initialize page tables required to
>> enable MMU and Dcache
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> Reviewed-by: Shiraz Hashim<shiraz.hashim@st.com>
>> ---
>>   drivers/misc/Makefile |   1 +
>>   drivers/misc/c3.c     | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/c3.h          |  63 ++++++++++++++++++++++++++
>
> I'm not so sure about the name of this "driver" and its location in
> drivers/misc. Is "C3" a generic crypto IP name? On which devices/SoC's
> is it currently implemented? Perhaps the name should be a little less

It is an ST peripheral and is used in spear SoCs and could be used in 
other ST SoCs

> generic, e.g. "spear-c3" or "st-crypto-c3"...?
>

hmm, ok. I can rename it to st-crypto-c3

> And if this "driver" only supports this memory fill operation for some
> ST SoC (SPEAr?), then its perhaps better located in arch/arm/ right now.
> Not sure.
>

You mean arch/arm/cpu/armv7/spear13xx/ ?
Is the drivers/misc a special place. Why not here ?

> Still some review comments below.
>
>>   3 files changed, 186 insertions(+)
>>   create mode 100644 drivers/misc/c3.c
>>   create mode 100644 include/c3.h
>>
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 9fac190..3ef8177 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
>>   LIB	:= $(obj)libmisc.o
>>
>>   COBJS-$(CONFIG_ALI152X) += ali512x.o
>> +COBJS-$(CONFIG_C3) += c3.o
>>   COBJS-$(CONFIG_DS4510)  += ds4510.o
>>   COBJS-$(CONFIG_FSL_LAW) += fsl_law.o
>>   COBJS-$(CONFIG_GPIO_LED) += gpio_led.o
>> diff --git a/drivers/misc/c3.c b/drivers/misc/c3.c
>> new file mode 100644
>> index 0000000..5f1eaee
>> --- /dev/null
>> +++ b/drivers/misc/c3.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * (C) Copyright 2012
>> + * ST Micoelectronics Pvt. Ltd.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#include<common.h>
>> +#include<c3.h>
>> +#include<errno.h>
>> +#include<asm/io.h>
>> +#include<asm/arch/hardware.h>
>> +
>> +static unsigned long c3_mem_xlate(void *addr)
>> +{
>> +
>
> Remove empty line.
>
>> +	if (((ulong)addr<  C3_INT_MEM_BASE_ADDR) || \
>
> The "\" is not necessary, or?
>
>> +		((ulong)addr>= (C3_INT_MEM_BASE_ADDR + C3_INT_MEM_SIZE)))
>> +		return (ulong)addr;
>> +
>> +	return (unsigned long)addr - C3_INT_MEM_BASE_ADDR +
>> +		C3_LOCAL_MEM_ADDR;
>> +}
>> +
>> +int c3_init(void)
>> +{
>> +	if (readl(C3_ID0_SCR) != C3_ID0_DEF_RDY_VAL)
>> +		writel(C3_ID0_SCR_RST, C3_ID0_SCR);
>
> Please use structs to access the SoC registers, see below (.h).
>
>> +
>> +	if (readl(C3_MOVE_CHANNEL_ID) == C3_MOVE_CHANNEL_ID_VAL)
>> +		return -EINVAL;
>> +
>> +	writel(C3_HIF_MCR_ENB_INT_MEM, C3_HIF_MCR);
>> +	writel(C3_LOCAL_MEM_ADDR, C3_HIF_MBAR);
>> +
>> +	return 0;
>> +}
>> +
>> +static int c3_run(void *prog_start)
>> +{
>> +	writel(c3_mem_xlate(prog_start), C3_ID0_IP);
>> +
>> +	while ((readl(C3_ID0_SCR)&  C3_ID0_STATE_MASK) == C3_ID0_STATE_RUN)
>> +		;
>> +
>> +	if ((readl(C3_ID0_SCR)&  C3_ID0_STATE_MASK) != C3_ID0_STATE_IDLE) {
>> +		/* If not back to idle an error occured */
>> +		writel(C3_ID0_SCR_RST, C3_ID0_SCR);
>> +
>> +		/* Set internal access to run c3 programs */
>> +		writel(C3_HIF_MCR_ENB_INT_MEM, C3_HIF_MCR);
>> +
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int c3_move(void *dest, void *src, int cnt, int optype, int opdata)
>> +{
>> +	unsigned long *c3_prog;
>> +	int ret = 0;
>> +
>> +	/* 3.b Prepare program */
>> +	c3_prog = (unsigned long *)C3_INT_MEM_BASE_ADDR;
>> +
>> +	/* 3.b.i. Mov init */
>> +	c3_prog[0] = C3_CMD_MOVE_INIT;
>> +	c3_prog[1] = opdata;
>> +
>> +	/* 3.b.ii. Mov data */
>> +	c3_prog[2] = C3_CMD_MOVE_DATA + cnt + optype;
>> +	c3_prog[3] = c3_mem_xlate(src);
>> +	c3_prog[4] = c3_mem_xlate(dest);
>> +
>> +	/* 3.b.iii. Stop */
>> +	c3_prog[5] = C3_CMD_STOP;
>> +
>> +	/* 4. Execute and wait */
>> +	ret = c3_run(c3_prog);
>> +
>> +	return ret;
>> +}
>> +
>> +void *c3_memset(void *s, int c, size_t count)
>> +{
>> +#define DATA_SIZE (1024*4)
>
> Move this define up or into the header please. And space before and
> after "*".
>
>> +	u32 data = C3_INT_MEM_BASE_ADDR + 0x100;
>> +	u32 size;
>> +	size_t cur = 0;
>> +
>> +	writel(0x100, C3_HIF_MAAR);
>> +	writel(c, C3_HIF_MADR);
>> +
>> +	for (size = 4; size<  DATA_SIZE; size<<= 1)
>> +		c3_move((void *)(data + size), (void *)data, size,
>> +				C3_MOVE_AND, 0xffffffff);
>> +
>> +	while (cur<  count) {
>> +		c3_move(s + cur, (void *)data, DATA_SIZE,
>> +				C3_MOVE_AND, 0xffffffff);
>> +		cur += DATA_SIZE;
>> +	}
>> +
>> +	return s;
>> +}
>> diff --git a/include/c3.h b/include/c3.h
>> new file mode 100644
>> index 0000000..541d702
>> --- /dev/null
>> +++ b/include/c3.h
>> @@ -0,0 +1,63 @@
>> +/*
>> + * (C) Copyright 2012
>> + * ST Micoelectronics Pvt. Ltd.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#ifndef _MISC_C3_
>> +#define _MISC_C3_
>> +
>> +#include<asm/arch/hardware.h>
>> +
>> +#define C3_HIF_OFF		0x400	/* master interface registers */
>> +
>> +#define C3_INT_MEM_BASE_ADDR		(CONFIG_SYS_C3_BASE + 0x400)
>> +#define C3_HIF_MBAR			(C3_INT_MEM_BASE_ADDR + 0x304)
>> +	#define C3_LOCAL_MEM_ADDR		0xF0000000
>> +#define C3_HIF_MCR			(C3_INT_MEM_BASE_ADDR + 0x308)
>> +	#define C3_HIF_MCR_ENB_INT_MEM		0x01
>> +#define C3_HIF_MAAR			(C3_INT_MEM_BASE_ADDR + 0x310)
>> +#define C3_HIF_MADR			(C3_INT_MEM_BASE_ADDR + 0x314)
>
> These seem to be registers. Better to define a struct for them and
> access via struct.
>

All other review comments accepted. I can float a v2 once we finalize 
the location where we are going to place it

Thanks
Vipin

> Thanks,
> Stefan
>
> .
>