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

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 ++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)
 create mode 100644 drivers/misc/c3.c
 create mode 100644 include/c3.h

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
generic, e.g. "spear-c3" or "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.

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.

Thanks,
Stefan

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
>
> .
>

On 12/07/2012 10:11 AM, Vipin Kumar wrote:
>> 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

Okay.

>> 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/ ?

Or perhaps arch/arm/lib?

On powerpc we have a special SoC specific memcpy version as well. You
might want to take a look at it:

arch/powerpc/lib/memcpy_mpc5200.c

It doesn't use a special device for transfer though, only has some
alignment restrictions.

> Is the drivers/misc a special place. Why not here ?

From my understanding the C3 "driver" you implemented with this memset()
is not a "real" *driver*. Thats my main reasoning why its a bit
misplaced in "drivers/*".

But I have no strong feelings here. Perhaps others have
thought/ideas/comments as well. Let's wait for further input a bit...

Thanks,
Stefan