diff mbox

[U-Boot] imximage: fix commands other than write_data

Message ID 1442279191-6615-1-git-send-email-troy.kisky@boundarydevices.com
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Troy Kisky Sept. 15, 2015, 1:06 a.m. UTC 
When CHECK_BITS_SET was added, they forgot to add
a new command table, and instead overwrote the
previous table.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---

Note: this needs tested to make sure imx7dsabresd still boots
as its dcd header has changed

Comments

Fabio Estevam Sept. 15, 2015, 1:16 a.m. UTC | #1 
On Mon, Sep 14, 2015 at 10:06 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:
> When CHECK_BITS_SET was added, they forgot to add
> a new command table, and instead overwrote the
> previous table.
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>
> ---
>
> Note: this needs tested to make sure imx7dsabresd still boots
> as its dcd header has changed

Boots fine on mx7dsabres, thanks:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
Stefano Babic Sept. 20, 2015, 8:23 a.m. UTC | #2 
On 15/09/2015 03:06, Troy Kisky wrote:
> When CHECK_BITS_SET was added, they forgot to add
> a new command table, and instead overwrote the
> previous table.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> ---


Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic
Stefano Babic Sept. 20, 2015, 8:59 a.m. UTC | #3 
Hi Troy,

On 15/09/2015 03:06, Troy Kisky wrote:
> When CHECK_BITS_SET was added, they forgot to add
> a new command table, and instead overwrote the
> previous table.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> ---
> 

This patch breaks building boards with SPL:

Building current source for 85 boards (6 threads, 1 job per thread)
       arm:  +   colibri_vf_dtb
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [u-boot-dtb.imx] Error 1
+make[1]: *** [u-boot-dtb.imx] Error 2
+make: *** [sub-make] Error 2
       arm:  +   mx6sabresd_spl
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [SPL] Error 1
+make[1]: *** [SPL] Error 2
+make: *** [sub-make] Error 2
       arm:  +   colibri_vf
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [u-boot.imx] Error 1
+make[1]: *** [u-boot.imx] Error 2
+make: *** [sub-make] Error 2
       arm:  +   vf610twr
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [u-boot.imx] Error 1
+make[1]: *** [u-boot.imx] Error 2
+make: *** [sub-make] Error 2
       arm:  +   cm_fx6
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [SPL] Error 1
+make[1]: *** [SPL] Error 2
+make: *** [sub-make] Error 2
       arm:  +   vf610twr_nand
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [u-boot.imx] Error 1
+make[1]: *** [u-boot.imx] Error 2
+make: *** [sub-make] Error 2
       arm:  +   mx6cuboxi
+Error: Image corrupt DCD size 536870911 exceed maximum 220
+make[2]: *** [SPL] Error 1
+make[1]: *** [SPL] Error 2
+make: *** [sub-make] Error 2

Can you take a look ? Thanks !

Best regards,
Stefano Babic

> Note: this needs tested to make sure imx7dsabresd still boots
> as its dcd header has changed
> 
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 0da48a7..97a6880 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -160,54 +160,80 @@ static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
>  	}
>  }
>  
> +static struct dcd_v2_cmd *gd_last_cmd;
> +
>  static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  		int32_t cmd)
>  {
>  	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> +	struct dcd_v2_cmd *d = gd_last_cmd;
> +	struct dcd_v2_cmd *d2;
> +	int len;
> +
> +	if (!d)
> +		d = &dcd_v2->dcd_cmd;
> +	d2 = d;
> +	len = be16_to_cpu(d->write_dcd_command.length);
> +	if (len > 4)
> +		d2 = (struct dcd_v2_cmd *)(((char *)d) + len);
>  
>  	switch (cmd) {
>  	case CMD_WRITE_DATA:
> -		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> -		dcd_v2->write_dcd_command.length = cpu_to_be16(
> -				dcd_len * sizeof(dcd_addr_data_t) + 4);
> -		dcd_v2->write_dcd_command.param = DCD_WRITE_DATA_PARAM;
> +		if ((d->write_dcd_command.tag == DCD_WRITE_DATA_COMMAND_TAG) &&
> +		    (d->write_dcd_command.param == DCD_WRITE_DATA_PARAM))
> +			break;
> +		d = d2;
> +		d->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> +		d->write_dcd_command.length = cpu_to_be16(4);
> +		d->write_dcd_command.param = DCD_WRITE_DATA_PARAM;
>  		break;
>  	case CMD_WRITE_CLR_BIT:
> -		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> -		dcd_v2->write_dcd_command.length = cpu_to_be16(
> -				dcd_len * sizeof(dcd_addr_data_t) + 4);
> -		dcd_v2->write_dcd_command.param = DCD_WRITE_CLR_BIT_PARAM;
> +		if ((d->write_dcd_command.tag == DCD_WRITE_DATA_COMMAND_TAG) &&
> +		    (d->write_dcd_command.param == DCD_WRITE_CLR_BIT_PARAM))
> +			break;
> +		d = d2;
> +		d->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
> +		d->write_dcd_command.length = cpu_to_be16(4);
> +		d->write_dcd_command.param = DCD_WRITE_CLR_BIT_PARAM;
>  		break;
>  	/*
>  	 * Check data command only supports one entry,
> -	 * so use 0xC = size(address + value + command).
>  	 */
>  	case CMD_CHECK_BITS_SET:
> -		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> -		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
> -		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
> +		d = d2;
> +		d->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> +		d->write_dcd_command.length = cpu_to_be16(4);
> +		d->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
>  		break;
>  	case CMD_CHECK_BITS_CLR:
> -		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> -		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
> -		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
> +		d = d2;
> +		d->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
> +		d->write_dcd_command.length = cpu_to_be16(4);
> +		d->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
>  		break;
>  	default:
>  		break;
>  	}
> +	gd_last_cmd = d;
>  }
>  
>  static void set_dcd_val_v2(struct imx_header *imxhdr, char *name, int lineno,
>  					int fld, uint32_t value, uint32_t off)
>  {
> -	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> +	struct dcd_v2_cmd *d = gd_last_cmd;
> +	int len;
> +
> +	len = be16_to_cpu(d->write_dcd_command.length);
> +	off = (len - 4) >> 3;
>  
>  	switch (fld) {
>  	case CFG_REG_ADDRESS:
> -		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
> +		d->addr_data[off].addr = cpu_to_be32(value);
>  		break;
>  	case CFG_REG_VALUE:
> -		dcd_v2->addr_data[off].value = cpu_to_be32(value);
> +		d->addr_data[off].value = cpu_to_be32(value);
> +		off++;
> +		d->write_dcd_command.length = cpu_to_be16((off << 3) + 4);
>  		break;
>  	default:
>  		break;
> @@ -236,12 +262,20 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  						char *name, int lineno)
>  {
>  	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
> +	struct dcd_v2_cmd *d = gd_last_cmd;
> +	int len;
> +
> +	if (!d)
> +		d = &dcd_v2->dcd_cmd;
> +	len = be16_to_cpu(d->write_dcd_command.length);
> +	if (len > 4)
> +		d = (struct dcd_v2_cmd *)(((char *)d) + len);
> +
> +	len = (char *)d - (char *)&dcd_v2->header;
>  
>  	dcd_v2->header.tag = DCD_HEADER_TAG;
> -	dcd_v2->header.length = cpu_to_be16(
> -			dcd_len * sizeof(dcd_addr_data_t) + 8);
> +	dcd_v2->header.length = cpu_to_be16(len);
>  	dcd_v2->header.version = DCD_VERSION;
> -	set_dcd_param_v2(imxhdr, dcd_len, CMD_WRITE_DATA);
>  }
>  
>  static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
> @@ -314,6 +348,7 @@ static void set_hdr_func(void)
>  		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
>  		break;
>  	case IMXIMAGE_V2:
> +		gd_last_cmd = NULL;
>  		set_dcd_val = set_dcd_val_v2;
>  		set_dcd_param = set_dcd_param_v2;
>  		set_dcd_rst = set_dcd_rst_v2;
> diff --git a/tools/imximage.h b/tools/imximage.h
> index d41c74f..c7b9b5c 100644
> --- a/tools/imximage.h
> +++ b/tools/imximage.h
> @@ -133,10 +133,14 @@ typedef struct {
>  	uint8_t param;
>  } __attribute__((packed)) write_dcd_command_t;
>  
> -typedef struct {
> -	ivt_header_t header;
> +struct dcd_v2_cmd {
>  	write_dcd_command_t write_dcd_command;
>  	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
> +};
> +
> +typedef struct {
> +	ivt_header_t header;
> +	struct dcd_v2_cmd dcd_cmd;
>  	uint32_t padding[1]; /* end up on an 8-byte boundary */
>  } dcd_v2_t;
>  
>
Troy Kisky Sept. 21, 2015, 9:01 p.m. UTC | #4 
On 9/20/2015 1:59 AM, Stefano Babic wrote:
> Hi Troy,
> 
> On 15/09/2015 03:06, Troy Kisky wrote:
>> When CHECK_BITS_SET was added, they forgot to add
>> a new command table, and instead overwrote the
>> previous table.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>
>> ---
>>
> 
> This patch breaks building boards with SPL:
>        arm:  +   mx6sabresd_spl
> +Error: Image corrupt DCD size 536870911 exceed maximum 220
> +make[2]: *** [SPL] Error 1
> +make[1]: *** [SPL] Error 2
> +make: *** [sub-make] Error 2

Looks like the error check is wrong. So previously, it was probably
writing 0 to address 0. I'll send a patch.

Troy
Stefano Babic Oct. 7, 2015, 12:05 p.m. UTC | #5 
Hi Troy,

On 15/09/2015 03:06, Troy Kisky wrote:
> When CHECK_BITS_SET was added, they forgot to add
> a new command table, and instead overwrote the
> previous table.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> ---

Applied to u-boot-imx (fix), thanks !

Best regards,
Stefano Babic
Troy Kisky Oct. 7, 2015, 5:54 p.m. UTC | #6 
On 10/7/2015 5:05 AM, Stefano Babic wrote:
> Hi Troy,
> 
> On 15/09/2015 03:06, Troy Kisky wrote:
>> When CHECK_BITS_SET was added, they forgot to add
>> a new command table, and instead overwrote the
>> previous table.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>
>> ---
> 
> Applied to u-boot-imx (fix), thanks !
> 
> Best regards,
> Stefano Babic
> 

Could someone verify that one of the following boards still boot
with this patch?



colibri_vf_dtb
mx6sabresd_spl
colibri_vf
vf610twr
cm_fx6
vf610twr_nand
mx6cuboxi


Just in case the ROM has a need for a non-empty DCD table.

Thanks
Troy
Fabio Estevam Oct. 7, 2015, 6:25 p.m. UTC | #7 
On Wed, Oct 7, 2015 at 2:54 PM, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:

> Could someone verify that one of the following boards still boot
> with this patch?
>
>
>
> colibri_vf_dtb
> mx6sabresd_spl

Tested with mx6sabresd_spl and it still boots.
Troy Kisky Oct. 7, 2015, 9:07 p.m. UTC | #8 
On 10/7/2015 11:25 AM, Fabio Estevam wrote:
> On Wed, Oct 7, 2015 at 2:54 PM, Troy Kisky
> <troy.kisky@boundarydevices.com> wrote:
> 
>> Could someone verify that one of the following boards still boot
>> with this patch?
>>
>>
>>
>> colibri_vf_dtb
>> mx6sabresd_spl
> 
> Tested with mx6sabresd_spl and it still boots.
> 
Thanks Fabio
Stefano Babic Oct. 8, 2015, 8:46 a.m. UTC | #9 
Hi Troy,

On 07/10/2015 19:54, Troy Kisky wrote:
> On 10/7/2015 5:05 AM, Stefano Babic wrote:
>> Hi Troy,
>>
>> On 15/09/2015 03:06, Troy Kisky wrote:
>>> When CHECK_BITS_SET was added, they forgot to add
>>> a new command table, and instead overwrote the
>>> previous table.
>>>
>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>
>>> ---
>>
>> Applied to u-boot-imx (fix), thanks !
>>
>> Best regards,
>> Stefano Babic
>>
> 
> Could someone verify that one of the following boards still boot
> with this patch?
> 
> 
> 
> colibri_vf_dtb
> mx6sabresd_spl
> colibri_vf
> vf610twr
> cm_fx6
> vf610twr_nand
> mx6cuboxi

Tested on mx6cuboxi, it works fine.

Best regards,
Stefano
diff mbox

Patch

diff --git a/tools/imximage.c b/tools/imximage.c
index 0da48a7..97a6880 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -160,54 +160,80 @@  static void set_dcd_val_v1(struct imx_header *imxhdr, char *name, int lineno,
 	}
 }
 
+static struct dcd_v2_cmd *gd_last_cmd;
+
 static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 		int32_t cmd)
 {
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+	struct dcd_v2_cmd *d = gd_last_cmd;
+	struct dcd_v2_cmd *d2;
+	int len;
+
+	if (!d)
+		d = &dcd_v2->dcd_cmd;
+	d2 = d;
+	len = be16_to_cpu(d->write_dcd_command.length);
+	if (len > 4)
+		d2 = (struct dcd_v2_cmd *)(((char *)d) + len);
 
 	switch (cmd) {
 	case CMD_WRITE_DATA:
-		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
-		dcd_v2->write_dcd_command.length = cpu_to_be16(
-				dcd_len * sizeof(dcd_addr_data_t) + 4);
-		dcd_v2->write_dcd_command.param = DCD_WRITE_DATA_PARAM;
+		if ((d->write_dcd_command.tag == DCD_WRITE_DATA_COMMAND_TAG) &&
+		    (d->write_dcd_command.param == DCD_WRITE_DATA_PARAM))
+			break;
+		d = d2;
+		d->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
+		d->write_dcd_command.length = cpu_to_be16(4);
+		d->write_dcd_command.param = DCD_WRITE_DATA_PARAM;
 		break;
 	case CMD_WRITE_CLR_BIT:
-		dcd_v2->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
-		dcd_v2->write_dcd_command.length = cpu_to_be16(
-				dcd_len * sizeof(dcd_addr_data_t) + 4);
-		dcd_v2->write_dcd_command.param = DCD_WRITE_CLR_BIT_PARAM;
+		if ((d->write_dcd_command.tag == DCD_WRITE_DATA_COMMAND_TAG) &&
+		    (d->write_dcd_command.param == DCD_WRITE_CLR_BIT_PARAM))
+			break;
+		d = d2;
+		d->write_dcd_command.tag = DCD_WRITE_DATA_COMMAND_TAG;
+		d->write_dcd_command.length = cpu_to_be16(4);
+		d->write_dcd_command.param = DCD_WRITE_CLR_BIT_PARAM;
 		break;
 	/*
 	 * Check data command only supports one entry,
-	 * so use 0xC = size(address + value + command).
 	 */
 	case CMD_CHECK_BITS_SET:
-		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
-		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
-		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
+		d = d2;
+		d->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
+		d->write_dcd_command.length = cpu_to_be16(4);
+		d->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
 		break;
 	case CMD_CHECK_BITS_CLR:
-		dcd_v2->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
-		dcd_v2->write_dcd_command.length = cpu_to_be16(0xC);
-		dcd_v2->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
+		d = d2;
+		d->write_dcd_command.tag = DCD_CHECK_DATA_COMMAND_TAG;
+		d->write_dcd_command.length = cpu_to_be16(4);
+		d->write_dcd_command.param = DCD_CHECK_BITS_SET_PARAM;
 		break;
 	default:
 		break;
 	}
+	gd_last_cmd = d;
 }
 
 static void set_dcd_val_v2(struct imx_header *imxhdr, char *name, int lineno,
 					int fld, uint32_t value, uint32_t off)
 {
-	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+	struct dcd_v2_cmd *d = gd_last_cmd;
+	int len;
+
+	len = be16_to_cpu(d->write_dcd_command.length);
+	off = (len - 4) >> 3;
 
 	switch (fld) {
 	case CFG_REG_ADDRESS:
-		dcd_v2->addr_data[off].addr = cpu_to_be32(value);
+		d->addr_data[off].addr = cpu_to_be32(value);
 		break;
 	case CFG_REG_VALUE:
-		dcd_v2->addr_data[off].value = cpu_to_be32(value);
+		d->addr_data[off].value = cpu_to_be32(value);
+		off++;
+		d->write_dcd_command.length = cpu_to_be16((off << 3) + 4);
 		break;
 	default:
 		break;
@@ -236,12 +262,20 @@  static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 						char *name, int lineno)
 {
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
+	struct dcd_v2_cmd *d = gd_last_cmd;
+	int len;
+
+	if (!d)
+		d = &dcd_v2->dcd_cmd;
+	len = be16_to_cpu(d->write_dcd_command.length);
+	if (len > 4)
+		d = (struct dcd_v2_cmd *)(((char *)d) + len);
+
+	len = (char *)d - (char *)&dcd_v2->header;
 
 	dcd_v2->header.tag = DCD_HEADER_TAG;
-	dcd_v2->header.length = cpu_to_be16(
-			dcd_len * sizeof(dcd_addr_data_t) + 8);
+	dcd_v2->header.length = cpu_to_be16(len);
 	dcd_v2->header.version = DCD_VERSION;
-	set_dcd_param_v2(imxhdr, dcd_len, CMD_WRITE_DATA);
 }
 
 static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
@@ -314,6 +348,7 @@  static void set_hdr_func(void)
 		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
 		break;
 	case IMXIMAGE_V2:
+		gd_last_cmd = NULL;
 		set_dcd_val = set_dcd_val_v2;
 		set_dcd_param = set_dcd_param_v2;
 		set_dcd_rst = set_dcd_rst_v2;
diff --git a/tools/imximage.h b/tools/imximage.h
index d41c74f..c7b9b5c 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -133,10 +133,14 @@  typedef struct {
 	uint8_t param;
 } __attribute__((packed)) write_dcd_command_t;
 
-typedef struct {
-	ivt_header_t header;
+struct dcd_v2_cmd {
 	write_dcd_command_t write_dcd_command;
 	dcd_addr_data_t addr_data[MAX_HW_CFG_SIZE_V2];
+};
+
+typedef struct {
+	ivt_header_t header;
+	struct dcd_v2_cmd dcd_cmd;
 	uint32_t padding[1]; /* end up on an 8-byte boundary */
 } dcd_v2_t;