[U-Boot] [Patch V5 1/4] spi: fsl_qspi: fix compile warning for 64-bit platform
From: Gong Qianyu Qianyu.Gong@freescale.com
This patch fixes the following compile warning: drivers/spi/fsl_qspi.c: In function 'fsl_qspi_probe': drivers/spi/fsl_qspi.c:937:15: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] priv->regs = (struct fsl_qspi_regs *)plat->reg_base; ^ Just make the cast explicit.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com --- V5: - Use uintptr_t instead of unsigned long. V4: - Revise the commit message. V2-V3: - No change.
drivers/spi/fsl_qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 542b6cf..38e5900 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -936,7 +936,7 @@ static int fsl_qspi_probe(struct udevice *bus)
dm_spi_bus->max_hz = plat->speed_hz;
- priv->regs = (struct fsl_qspi_regs *)plat->reg_base; + priv->regs = (struct fsl_qspi_regs *)(uintptr_t)plat->reg_base; priv->flags = plat->flags;
priv->speed_hz = plat->speed_hz;
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com --- V2-V5: - No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); - memcpy(rxbuf, &data, 4); + if (size < 4) + memcpy(rxbuf, &data, size); + else + memcpy(rxbuf, &data, 4); rxbuf++; size -= 4; i++;
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;
size -= 4 even if size was < 4?
-Scott
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32
*rxbuf, u32 len)
if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }
Regards, Qianyu
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32
*rxbuf, u32 len)
if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }
I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
-Scott
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32
*rxbuf, u32 len)
if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy, isn't it?
York
On 01/25/2016 04:16 AM, York Sun wrote:
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32
*rxbuf, u32 len)
if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy, isn't it?
York
How about like this?
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index feec3e8..13bba09 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 *rxbuf, u32 len) static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs; - u32 mcr_reg, rbsr_reg, data; - int i, size; + u32 mcr_reg, rbsr_reg, data, size; + int i;
mcr_reg = qspi_read32(priv->flags, ®s->mcr); qspi_write32(priv->flags, ®s->mcr, @@ -495,14 +495,14 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len)
i = 0; size = len; - while ((RX_BUFFER_SIZE >= size) && (size > 0)) { + while ((RX_BUFFER_SIZE >= size) && (size != 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); - memcpy(rxbuf, &data, 4); + memcpy(rxbuf, &data, min(size, 4)); + size = (size < 4) ? 0 : ( size - 4); rxbuf++; - size -= 4; i++; } }
On 01/24/2016 08:09 PM, Yao Yuan wrote:
On 01/25/2016 04:16 AM, York Sun wrote:
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver everytime we memcpy 4 bytes to the dest memory regardless of the remaining length. This patch adds checking the remaining length before memcpy. If the length is shorter than 4 bytes, memcpy the actual length of data to the dest memory.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V2-V5:
- No change.
drivers/spi/fsl_qspi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 38e5900..f178857 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32
*rxbuf, u32 len)
if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
if (size < 4)memcpy(rxbuf, &data, size);elsememcpy(rxbuf, &data, 4);memcpy(rxbuf, &data, min(size, 4));
rxbuf++; size -= 4; i++;size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy, isn't it?
York
How about like this?
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index feec3e8..13bba09 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 *rxbuf, u32 len) static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs;
u32 mcr_reg, rbsr_reg, data;int i, size;
u32 mcr_reg, rbsr_reg, data, size;int i; mcr_reg = qspi_read32(priv->flags, ®s->mcr); qspi_write32(priv->flags, ®s->mcr,@@ -495,14 +495,14 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len)
i = 0; size = len;
while ((RX_BUFFER_SIZE >= size) && (size > 0)) {
while ((RX_BUFFER_SIZE >= size) && (size != 0)) {
You can keep using "size > 0". It is still correct.
rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));size = (size < 4) ? 0 : ( size - 4); rxbuf++;
size -= 4; i++; } }
The rest looks OK to me.
York
On 01/25/2016 10:47 AM, york sun wrote:
On 01/24/2016 08:09 PM, Yao Yuan wrote:
On 01/25/2016 04:16 AM, York Sun wrote:
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:30 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/20/2016 09:43 PM, Gong Qianyu wrote: > From: Gong Qianyu Qianyu.Gong@freescale.com > > In current driver everytime we memcpy 4 bytes to the dest memory > regardless of the remaining length. > This patch adds checking the remaining length before memcpy. > If the length is shorter than 4 bytes, memcpy the actual length of > data to the dest memory. > > Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com > --- > V2-V5: > - No change. > > drivers/spi/fsl_qspi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index > 38e5900..f178857 100644 > --- a/drivers/spi/fsl_qspi.c > +++ b/drivers/spi/fsl_qspi.c > @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv > *priv, u32 *rxbuf, u32 len) > if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { > data = qspi_read32(priv->flags, ®s->rbdr[i]); > data = qspi_endian_xchg(data); > - memcpy(rxbuf, &data, 4); > + if (size < 4) > + memcpy(rxbuf, &data, size); > + else > + memcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));
> rxbuf++; > size -= 4; > i++;
size -= 4 even if size was < 4?
-Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy, isn't it?
York
How about like this?
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index feec3e8..13bba09 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 *rxbuf, u32 len) static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs;
u32 mcr_reg, rbsr_reg, data;int i, size;
u32 mcr_reg, rbsr_reg, data, size;int i; mcr_reg = qspi_read32(priv->flags, ®s->mcr); qspi_write32(priv->flags, ®s->mcr,@@ -495,14 +495,14 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len)
i = 0; size = len;
while ((RX_BUFFER_SIZE >= size) && (size > 0)) {
while ((RX_BUFFER_SIZE >= size) && (size != 0)) {You can keep using "size > 0". It is still correct.
And more robust.
rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));size = (size < 4) ? 0 : ( size - 4); rxbuf++;
size -= 4; i++; } }The rest looks OK to me.
It's still awkward compared to:
int chunk;
...
chunk = min(size, 4); memcpy(rxbuf, &data, chunk); size -= chunk;
-Scott
On 01/25/2016 09:01 AM, Scott Wood wrote:
On 01/25/2016 10:47 AM, york sun wrote:
On 01/24/2016 08:09 PM, Yao Yuan wrote:
On 01/25/2016 04:16 AM, York Sun wrote:
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote:
> -----Original Message----- > From: Scott Wood > Sent: Friday, January 22, 2016 3:30 AM > To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; > R58495@freescale.com > Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; > B48286@freescale.com; Shaohui.Xie@freescale.com; > Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong > Qianyu Qianyu.Gong@freescale.com > Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy > issue > > On 01/20/2016 09:43 PM, Gong Qianyu wrote: >> From: Gong Qianyu Qianyu.Gong@freescale.com >> >> In current driver everytime we memcpy 4 bytes to the dest memory >> regardless of the remaining length. >> This patch adds checking the remaining length before memcpy. >> If the length is shorter than 4 bytes, memcpy the actual length of >> data to the dest memory. >> >> Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com >> --- >> V2-V5: >> - No change. >> >> drivers/spi/fsl_qspi.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index >> 38e5900..f178857 100644 >> --- a/drivers/spi/fsl_qspi.c >> +++ b/drivers/spi/fsl_qspi.c >> @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct fsl_qspi_priv >> *priv, u32 > *rxbuf, u32 len) >> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { >> data = qspi_read32(priv->flags, ®s->rbdr[i]); >> data = qspi_endian_xchg(data); >> - memcpy(rxbuf, &data, 4); >> + if (size < 4) >> + memcpy(rxbuf, &data, size); >> + else >> + memcpy(rxbuf, &data, 4); > > memcpy(rxbuf, &data, min(size, 4)); > >> rxbuf++; >> size -= 4; >> i++; > > size -= 4 even if size was < 4? > > -Scott
Yes.. The following is complete code:
i = 0; size = len; while ((RX_BUFFER_SIZE >= size) && (size > 0)) { rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data); memcpy(rxbuf, &data, min(size, 4)); rxbuf++; size -= 4; i++; } }I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen is "unsigned int". So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy, isn't it?
York
How about like this?
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index feec3e8..13bba09 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 *rxbuf, u32 len) static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs;
u32 mcr_reg, rbsr_reg, data;int i, size;
u32 mcr_reg, rbsr_reg, data, size;int i; mcr_reg = qspi_read32(priv->flags, ®s->mcr); qspi_write32(priv->flags, ®s->mcr,@@ -495,14 +495,14 @@ static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len)
i = 0; size = len;
while ((RX_BUFFER_SIZE >= size) && (size > 0)) {
while ((RX_BUFFER_SIZE >= size) && (size != 0)) {You can keep using "size > 0". It is still correct.
And more robust.
rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));size = (size < 4) ? 0 : ( size - 4); rxbuf++;
size -= 4; i++; } }The rest looks OK to me.
It's still awkward compared to:
int chunk;
...
chunk = min(size, 4); memcpy(rxbuf, &data, chunk); size -= chunk;
Agree.
York
-----Original Message----- From: york sun Sent: Tuesday, January 26, 2016 1:02 AM To: Scott Wood scott.wood@nxp.com; Yao Yuan yao.yuan@nxp.com; Qianyu Gong qianyu.gong@nxp.com Cc: B48286@freescale.com; u-boot@lists.denx.de; Wenbin.Song@freescale.com; jteki@openedev.com Subject: Re: [U-Boot] [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid memcpy issue
On 01/25/2016 09:01 AM, Scott Wood wrote:
On 01/25/2016 10:47 AM, york sun wrote:
On 01/24/2016 08:09 PM, Yao Yuan wrote:
On 01/25/2016 04:16 AM, York Sun wrote:
On 01/22/2016 07:43 AM, Scott Wood wrote:
On 01/21/2016 09:35 PM, Qianyu Gong wrote: > >> -----Original Message----- >> From: Scott Wood >> Sent: Friday, January 22, 2016 3:30 AM >> To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; >> R58495@freescale.com >> Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; >> B48286@freescale.com; Shaohui.Xie@freescale.com; >> Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong >> Qianyu Qianyu.Gong@freescale.com >> Subject: Re: [Patch V5 2/4] spi: fsl_qspi: Fix qspi_op_rdid >> memcpy issue >> >> On 01/20/2016 09:43 PM, Gong Qianyu wrote: >>> From: Gong Qianyu Qianyu.Gong@freescale.com >>> >>> In current driver everytime we memcpy 4 bytes to the dest >>> memory regardless of the remaining length. >>> This patch adds checking the remaining length before memcpy. >>> If the length is shorter than 4 bytes, memcpy the actual length >>> of data to the dest memory. >>> >>> Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com >>> --- >>> V2-V5: >>> - No change. >>> >>> drivers/spi/fsl_qspi.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c >>> index >>> 38e5900..f178857 100644 >>> --- a/drivers/spi/fsl_qspi.c >>> +++ b/drivers/spi/fsl_qspi.c >>> @@ -500,7 +500,10 @@ static void qspi_op_rdid(struct >>> fsl_qspi_priv *priv, u32 >> *rxbuf, u32 len) >>> if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { >>> data = qspi_read32(priv->flags, ®s->rbdr[i]); >>> data = qspi_endian_xchg(data); >>> - memcpy(rxbuf, &data, 4); >>> + if (size < 4) >>> + memcpy(rxbuf, &data, size); >>> + else >>> + memcpy(rxbuf, &data, 4); >> >> memcpy(rxbuf, &data, min(size, 4)); >> >>> rxbuf++; >>> size -= 4; >>> i++; >> >> size -= 4 even if size was < 4? >> >> -Scott > > Yes.. The following is complete code: > > i = 0; > size = len; > while ((RX_BUFFER_SIZE >= size) && (size > 0)) { > rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); > if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { > data = qspi_read32(priv->flags, ®s->rbdr[i]); > data = qspi_endian_xchg(data); > memcpy(rxbuf, &data, min(size, 4)); > rxbuf++; > size -= 4; > i++; > } > }
I'm not saying it doesn't work (assuming i is signed, which the "complete code" above doesn't show). I'm saying it looks weird, and it would be better to have a variable that holds min(size, 4) and pass that to both memcpy and the subtraction.
Qianyu,
Previously I said it looked weird for doing this. Please fix.
"size" is declared as "int". "len" is declared as u32. That's not "int". If you trace back the functions, you may see it came from DIV_ROUND_UP(bitlen, 8) where bitlen
is "unsigned int".
So technically the code is safe. But it is _confusing_. We don't want to confuse ourselves when reading the code later. And the fix is easy,
isn't it?
York
How about like this?
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index feec3e8..13bba09 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -477,8 +477,8 @@ static void qspi_op_rdbank(struct fsl_qspi_priv *priv, u8 *rxbuf, u32 len) static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs;
u32 mcr_reg, rbsr_reg, data;int i, size;
u32 mcr_reg, rbsr_reg, data, size;int i; mcr_reg = qspi_read32(priv->flags, ®s->mcr); qspi_write32(priv->flags, ®s->mcr, @@ -495,14 +495,14 @@static void qspi_op_rdid(struct fsl_qspi_priv *priv, u32 *rxbuf, u32 len)
i = 0; size = len;
while ((RX_BUFFER_SIZE >= size) && (size > 0)) {
while ((RX_BUFFER_SIZE >= size) && (size != 0)) {You can keep using "size > 0". It is still correct.
And more robust.
rbsr_reg = qspi_read32(priv->flags, ®s->rbsr); if (rbsr_reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[i]); data = qspi_endian_xchg(data);
memcpy(rxbuf, &data, 4);
memcpy(rxbuf, &data, min(size, 4));size = (size < 4) ? 0 : ( size - 4); rxbuf++;
size -= 4; i++; } }The rest looks OK to me.
It's still awkward compared to:
int chunk;
...
chunk = min(size, 4); memcpy(rxbuf, &data, chunk); size -= chunk;
Agree.
York
Great. Thank you all. I send out a V7 patch set to fix it.
Regards, Qianyu
From: Gong Qianyu Qianyu.Gong@freescale.com
In current driver, we always copy 4 bytes to the dest memory. Actually the dest memory may be shorter than 4 bytes. Add an argument to indicate the dest memory length. Avoid writing memory outside of the bounds.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com --- V3-V5: - No change. V2: - New patch.
drivers/spi/fsl_qspi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index f178857..6c5f496 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -642,7 +642,7 @@ static void qspi_op_write(struct fsl_qspi_priv *priv, u8 *txbuf, u32 len) qspi_write32(priv->flags, ®s->mcr, mcr_reg); }
-static void qspi_op_rdsr(struct fsl_qspi_priv *priv, u32 *rxbuf) +static void qspi_op_rdsr(struct fsl_qspi_priv *priv, void *rxbuf, u32 len) { struct fsl_qspi_regs *regs = priv->regs; u32 mcr_reg, reg, data; @@ -665,7 +665,7 @@ static void qspi_op_rdsr(struct fsl_qspi_priv *priv, u32 *rxbuf) if (reg & QSPI_RBSR_RDBFL_MASK) { data = qspi_read32(priv->flags, ®s->rbdr[0]); data = qspi_endian_xchg(data); - memcpy(rxbuf, &data, 4); + memcpy(rxbuf, &data, len); qspi_write32(priv->flags, ®s->mcr, qspi_read32(priv->flags, ®s->mcr) | QSPI_MCR_CLR_RXF_MASK); @@ -754,7 +754,7 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen, } else if (priv->cur_seqid == QSPI_CMD_RDID) qspi_op_rdid(priv, din, bytes); else if (priv->cur_seqid == QSPI_CMD_RDSR) - qspi_op_rdsr(priv, din); + qspi_op_rdsr(priv, din, bytes); #ifdef CONFIG_SPI_FLASH_BAR else if ((priv->cur_seqid == QSPI_CMD_BRRD) || (priv->cur_seqid == QSPI_CMD_RDEAR)) {
From: Gong Qianyu Qianyu.Gong@freescale.com
It might be missed when converting spi_flash_probe() in cmd_sf.c.
This patch refers to commit fbb099183e3a ("dm: Convert spi_flash_probe() and 'sf probe' to use driver model").
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com --- V5: - Revise commit message. V4: - Use CONFIG_ENV_* instead of CONFIG_SF_*. - Remove the variables and call the macros directly. - Use set_default_env instead of print info. V3: - Remove redundant operations for saveenv() V2: - New patch.
common/env_sf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/common/env_sf.c b/common/env_sf.c index 9409831..892e6cb 100644 --- a/common/env_sf.c +++ b/common/env_sf.c @@ -16,6 +16,7 @@ #include <spi_flash.h> #include <search.h> #include <errno.h> +#include <dm/device-internal.h>
#ifndef CONFIG_ENV_SPI_BUS # define CONFIG_ENV_SPI_BUS 0 @@ -51,6 +52,19 @@ int saveenv(void) char *saved_buffer = NULL, flag = OBSOLETE_FLAG; u32 saved_size, saved_offset, sector = 1; int ret; +#ifdef CONFIG_DM_SPI_FLASH + struct udevice *new; + + ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, + CONFIG_ENV_SPI_MAX_HZ, + CONFIG_ENV_SPI_MODE, &new); + if (ret) { + set_default_env("!spi_flash_probe_bus_cs() failed"); + return 1; + } + + env_flash = dev_get_uclass_priv(new); +#else
if (!env_flash) { env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, @@ -61,6 +75,7 @@ int saveenv(void) return 1; } } +#endif
ret = env_export(&env_new); if (ret) @@ -227,6 +242,19 @@ int saveenv(void) char *saved_buffer = NULL; int ret = 1; env_t env_new; +#ifdef CONFIG_DM_SPI_FLASH + struct udevice *new; + + ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, + CONFIG_ENV_SPI_MAX_HZ, + CONFIG_ENV_SPI_MODE, &new); + if (ret) { + set_default_env("!spi_flash_probe_bus_cs() failed"); + return 1; + } + + env_flash = dev_get_uclass_priv(new); +#else
if (!env_flash) { env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, @@ -237,6 +265,7 @@ int saveenv(void) return 1; } } +#endif
/* Is the sector larger than the env (i.e. embedded) */ if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
On 01/20/2016 09:42 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
This patch fixes the following compile warning: drivers/spi/fsl_qspi.c: In function 'fsl_qspi_probe': drivers/spi/fsl_qspi.c:937:15: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] priv->regs = (struct fsl_qspi_regs *)plat->reg_base; ^ Just make the cast explicit.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V5:
- Use uintptr_t instead of unsigned long.
V4:
- Revise the commit message.
V2-V3:
- No change.
drivers/spi/fsl_qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 542b6cf..38e5900 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -936,7 +936,7 @@ static int fsl_qspi_probe(struct udevice *bus)
dm_spi_bus->max_hz = plat->speed_hz;
- priv->regs = (struct fsl_qspi_regs *)plat->reg_base;
priv->regs = (struct fsl_qspi_regs *)(uintptr_t)plat->reg_base; priv->flags = plat->flags;
priv->speed_hz = plat->speed_hz;
Use phys_to_virt().
-Scott
-----Original Message----- From: Scott Wood Sent: Friday, January 22, 2016 3:28 AM To: Qianyu Gong qianyu.gong@nxp.com; u-boot@lists.denx.de; R58495@freescale.com Cc: Mingkai.Hu@freescale.com; jteki@openedev.com; B48286@freescale.com; Shaohui.Xie@freescale.com; Wenbin.Song@freescale.com; Scott Wood oss@buserror.net; Gong Qianyu Qianyu.Gong@freescale.com Subject: Re: [Patch V5 1/4] spi: fsl_qspi: fix compile warning for 64-bit platform
On 01/20/2016 09:42 PM, Gong Qianyu wrote:
From: Gong Qianyu Qianyu.Gong@freescale.com
This patch fixes the following compile warning: drivers/spi/fsl_qspi.c: In function 'fsl_qspi_probe': drivers/spi/fsl_qspi.c:937:15: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] priv->regs = (struct fsl_qspi_regs *)plat->reg_base; ^ Just make the cast explicit.
Signed-off-by: Gong Qianyu Qianyu.Gong@freescale.com
V5:
- Use uintptr_t instead of unsigned long.
V4:
- Revise the commit message.
V2-V3:
- No change.
drivers/spi/fsl_qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 542b6cf..38e5900 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -936,7 +936,7 @@ static int fsl_qspi_probe(struct udevice *bus)
dm_spi_bus->max_hz = plat->speed_hz;
- priv->regs = (struct fsl_qspi_regs *)plat->reg_base;
priv->regs = (struct fsl_qspi_regs *)(uintptr_t)plat->reg_base; priv->flags = plat->flags;
priv->speed_hz = plat->speed_hz;
Use phys_to_virt().
-Scott
The function seems to be dropped in U-Boot? I just find it in arch/arm/include/asm/memory.h with ''#if 0''.
Regards, Qianyu
participants (5)
-
Gong Qianyu -
Qianyu Gong
-
Scott Wood -
Yao Yuan -
york sun