HyperFlash devices fail to probe:
rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
In HyperFLASH or Octal-SPI Flash mode, the Transfer Data Enable bits
(SPIDE) in the Manual Mode Enable Setting Register (SMENR) are derived
from half of the transfer size, cfr. the rpcif_bits_set() helper
function. However, rpcif_reg_{read,write}() does not take the bus size
into account, and does not double all Manual Mode Data Register access
sizes when communicating with a HyperFLASH or Octal-SPI Flash device.
Fix this, and avoid the back-and-forth conversion between transfer size
and Transfer Data Enable bits, by explicitly storing the transfer size
in struct rpcif, and using that value to determine access size in
rpcif_reg_{read,write}().
Enforce that the "high" Manual Mode Read/Write Data Registers
(SM[RW]DR1) are only used for 8-byte data accesses.
While at it, forbid writing to the Manual Mode Read Data Registers,
as they are read-only.
Fixes: fff53a551db50f5e ("memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Lightly (read-only) tested on:
- Salvator-XS with R-Car M3-W ES1.0 with HyperFLASH,
- Falcon with R-Car V3U ES1.0 with QSPI.
v2:
- Use rpc->xfer_size instead of SPIDE register reads and
rpc->bus_size.
Note: Alternatively, rpcif_manual_xfer() could bypass regmap and
use {read,write}[bwl]() directly, cfr. commit 0d37f69cacb33435
("memory: renesas-rpc-if: Correct QSPI data transfer in Manual
mode") in the BSP.
- HF dirmap reads are confirmed to work on R-Car M3-W,
- Drop RFC.
On Salvator-XS with unlocked HyperFlash, the HyperFlash is now detected
again, cfr. (with DEBUG_CFI enabled):
Number of erase regions: 1
Primary Vendor Command Set: 0002 (AMD/Fujitsu Standard)
Primary Algorithm Table at 0040
Alternative Vendor Command Set: 0000 (None)
No Alternate Algorithm Table
Vcc Minimum: 1.7 V
Vcc Maximum: 1.9 V
No Vpp line
Typical byte/word write timeout: 512 \xc2\xb5s
Maximum byte/word write timeout: 2048 \xc2\xb5s
Typical full buffer write timeout: 512 \xc2\xb5s
Maximum full buffer write timeout: 2048 \xc2\xb5s
Typical block erase timeout: 1024 ms
Maximum block erase timeout: 4096 ms
Typical chip erase timeout: 262144 ms
Maximum chip erase timeout: 1048576 ms
Device size: 0x4000000 bytes (64 MiB)
Flash Device Interface description: 0x0000
- x8-only asynchronous interface
Max. bytes in buffer write: 0x200
Number of Erase Block Regions: 1
Erase Region #0: BlockSize 0x40000 bytes, 256 blocks
rpc-if-hyperflash: Found 1 x16 devices at 0x0 in 16-bit bank. Manufacturer ID 0x000001 Chip ID 0x007000
Amd/Fujitsu Extended Query Table at 0x0040
Amd/Fujitsu Extended Query version 1.5.
rpc-if-hyperflash: CFI contains unrecognised boot bank location (0). Assuming bottom.
number of CFI chips: 1
v1: https://lore.kernel.org/r/27107f2d578b198078df841ee2e4d7b71b183898.1647017136.git.geert+renesas@glider.be/
---
drivers/memory/renesas-rpc-if.c | 56 ++++++++++++++++++++++++++-------
include/memory/renesas-rpc-if.h | 1 +
2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index e4cc64f560196d55..a8c7abe1cf00be65 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -171,18 +171,32 @@ static int rpcif_reg_read(void *context, unsigned int reg, unsigned int *val)
{
struct rpcif *rpc = context;
- if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
- u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
-
- if (spide == 0x8) {
+ switch (reg) {
+ case RPCIF_SMRDR0:
+ case RPCIF_SMWDR0:
+ switch (rpc->xfer_size) {
+ case 1:
*val = readb(rpc->base + reg);
return 0;
- } else if (spide == 0xC) {
+
+ case 2:
*val = readw(rpc->base + reg);
return 0;
- } else if (spide != 0xF) {
+
+ case 4:
+ case 8:
+ break;
+
+ default:
return -EILSEQ;
}
+ break;
+
+ case RPCIF_SMRDR1:
+ case RPCIF_SMWDR1:
+ if (rpc->xfer_size != 8)
+ return -EILSEQ;
+ break;
}
*val = readl(rpc->base + reg);
@@ -193,18 +207,34 @@ static int rpcif_reg_write(void *context, unsigned int reg, unsigned int val)
{
struct rpcif *rpc = context;
- if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
- u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
-
- if (spide == 0x8) {
+ switch (reg) {
+ case RPCIF_SMWDR0:
+ switch (rpc->xfer_size) {
+ case 1:
writeb(val, rpc->base + reg);
return 0;
- } else if (spide == 0xC) {
+
+ case 2:
writew(val, rpc->base + reg);
return 0;
- } else if (spide != 0xF) {
+
+ case 4:
+ case 8:
+ break;
+
+ default:
return -EILSEQ;
}
+ break;
+
+ case RPCIF_SMWDR1:
+ if (rpc->xfer_size != 8)
+ return -EILSEQ;
+ break;
+
+ case RPCIF_SMRDR0:
+ case RPCIF_SMRDR1:
+ return -EPERM;
}
writel(val, rpc->base + reg);
@@ -469,6 +499,7 @@ int rpcif_manual_xfer(struct rpcif *rpc)
smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
+ rpc->xfer_size = nbytes;
memcpy(data, rpc->buffer + pos, nbytes);
if (nbytes == 8) {
@@ -533,6 +564,7 @@ int rpcif_manual_xfer(struct rpcif *rpc)
regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
regmap_write(rpc->regmap, RPCIF_SMCR,
rpc->smcr | RPCIF_SMCR_SPIE);
+ rpc->xfer_size = nbytes;
ret = wait_msg_xfer_end(rpc);
if (ret)
goto err_out;
diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
index 7c93f5177532f187..9c0ad64b8d292d49 100644
--- a/include/memory/renesas-rpc-if.h
+++ b/include/memory/renesas-rpc-if.h
@@ -72,6 +72,7 @@ struct rpcif {
enum rpcif_type type;
enum rpcif_data_dir dir;
u8 bus_size;
+ u8 xfer_size;
void *buffer;
u32 xferlen;
u32 smcr;
--
2.25.1
Hi Geert,
thank you for the patch!
> Lightly (read-only) tested on:
> - Salvator-XS with R-Car M3-W ES1.0 with HyperFLASH,
> - Falcon with R-Car V3U ES1.0 with QSPI.
I (also lightly) tested writing on the V3U and it seems to still work,
even with the problematic cases we needed to fix earlier.
> Note: Alternatively, rpcif_manual_xfer() could bypass regmap and
> use {read,write}[bwl]() directly, cfr. commit 0d37f69cacb33435
> ("memory: renesas-rpc-if: Correct QSPI data transfer in Manual
> mode") in the BSP.
As discuseed via IRC, I don't like bypassing regmap. I prefer your
solution here.
> @@ -171,18 +171,32 @@ static int rpcif_reg_read(void *context, unsigned int reg, unsigned int *val)
Before this function is a comment which needs to be updated (away from
SMENR_SPIDE to xfer_size).
> + switch (rpc->xfer_size) {
> + case 1:
> *val = readb(rpc->base + reg);
> return 0;
> - } else if (spide == 0xC) {
> +
> + case 2:
> *val = readw(rpc->base + reg);
> return 0;
> - } else if (spide != 0xF) {
> +
> + case 4:
> + case 8:
I think it is more readable to add here:
*val = readl(rpc->base + reg);
return 0;
> + break;
... and remove this break ...
> +
> + default:
> return -EILSEQ;
> }
> + break;
... and this one. Same for the write function. I am not strict on this
one, but I think this make following the logic easier.
Rest looks good. I like the additional checks!
All the best,
Wolfram
Hi Geert,
Thank you for the patch.
On Thu, Mar 17, 2022 at 12:54 PM Geert Uytterhoeven
<[email protected]> wrote:
>
> HyperFlash devices fail to probe:
>
> rpc-if-hyperflash rpc-if-hyperflash: probing of hyperbus device failed
>
> In HyperFLASH or Octal-SPI Flash mode, the Transfer Data Enable bits
> (SPIDE) in the Manual Mode Enable Setting Register (SMENR) are derived
> from half of the transfer size, cfr. the rpcif_bits_set() helper
> function. However, rpcif_reg_{read,write}() does not take the bus size
> into account, and does not double all Manual Mode Data Register access
> sizes when communicating with a HyperFLASH or Octal-SPI Flash device.
>
> Fix this, and avoid the back-and-forth conversion between transfer size
> and Transfer Data Enable bits, by explicitly storing the transfer size
> in struct rpcif, and using that value to determine access size in
> rpcif_reg_{read,write}().
>
> Enforce that the "high" Manual Mode Read/Write Data Registers
> (SM[RW]DR1) are only used for 8-byte data accesses.
> While at it, forbid writing to the Manual Mode Read Data Registers,
> as they are read-only.
>
> Fixes: fff53a551db50f5e ("memory: renesas-rpc-if: Correct QSPI data transfer in Manual mode")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Lightly (read-only) tested on:
> - Salvator-XS with R-Car M3-W ES1.0 with HyperFLASH,
> - Falcon with R-Car V3U ES1.0 with QSPI.
>
> v2:
> - Use rpc->xfer_size instead of SPIDE register reads and
> rpc->bus_size.
> Note: Alternatively, rpcif_manual_xfer() could bypass regmap and
> use {read,write}[bwl]() directly, cfr. commit 0d37f69cacb33435
> ("memory: renesas-rpc-if: Correct QSPI data transfer in Manual
> mode") in the BSP.
> - HF dirmap reads are confirmed to work on R-Car M3-W,
> - Drop RFC.
>
> On Salvator-XS with unlocked HyperFlash, the HyperFlash is now detected
> again, cfr. (with DEBUG_CFI enabled):
>
> Number of erase regions: 1
> Primary Vendor Command Set: 0002 (AMD/Fujitsu Standard)
> Primary Algorithm Table at 0040
> Alternative Vendor Command Set: 0000 (None)
> No Alternate Algorithm Table
> Vcc Minimum: 1.7 V
> Vcc Maximum: 1.9 V
> No Vpp line
> Typical byte/word write timeout: 512 \xc2\xb5s
> Maximum byte/word write timeout: 2048 \xc2\xb5s
> Typical full buffer write timeout: 512 \xc2\xb5s
> Maximum full buffer write timeout: 2048 \xc2\xb5s
> Typical block erase timeout: 1024 ms
> Maximum block erase timeout: 4096 ms
> Typical chip erase timeout: 262144 ms
> Maximum chip erase timeout: 1048576 ms
> Device size: 0x4000000 bytes (64 MiB)
> Flash Device Interface description: 0x0000
> - x8-only asynchronous interface
> Max. bytes in buffer write: 0x200
> Number of Erase Block Regions: 1
> Erase Region #0: BlockSize 0x40000 bytes, 256 blocks
> rpc-if-hyperflash: Found 1 x16 devices at 0x0 in 16-bit bank. Manufacturer ID 0x000001 Chip ID 0x007000
> Amd/Fujitsu Extended Query Table at 0x0040
> Amd/Fujitsu Extended Query version 1.5.
> rpc-if-hyperflash: CFI contains unrecognised boot bank location (0). Assuming bottom.
> number of CFI chips: 1
>
> v1: https://lore.kernel.org/r/27107f2d578b198078df841ee2e4d7b71b183898.1647017136.git.geert+renesas@glider.be/
> ---
> drivers/memory/renesas-rpc-if.c | 56 ++++++++++++++++++++++++++-------
> include/memory/renesas-rpc-if.h | 1 +
> 2 files changed, 45 insertions(+), 12 deletions(-)
>
I did some testing on RZ/G2L SMARC EVK. My read/write test case is
passing [0] for QSPI flash.
Tested-by: Lad Prabhakar <[email protected]>
[0] https://paste.debian.net/1235083/
Cheers,
Prabhakar
> diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
> index e4cc64f560196d55..a8c7abe1cf00be65 100644
> --- a/drivers/memory/renesas-rpc-if.c
> +++ b/drivers/memory/renesas-rpc-if.c
> @@ -171,18 +171,32 @@ static int rpcif_reg_read(void *context, unsigned int reg, unsigned int *val)
> {
> struct rpcif *rpc = context;
>
> - if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
> - u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
> -
> - if (spide == 0x8) {
> + switch (reg) {
> + case RPCIF_SMRDR0:
> + case RPCIF_SMWDR0:
> + switch (rpc->xfer_size) {
> + case 1:
> *val = readb(rpc->base + reg);
> return 0;
> - } else if (spide == 0xC) {
> +
> + case 2:
> *val = readw(rpc->base + reg);
> return 0;
> - } else if (spide != 0xF) {
> +
> + case 4:
> + case 8:
> + break;
> +
> + default:
> return -EILSEQ;
> }
> + break;
> +
> + case RPCIF_SMRDR1:
> + case RPCIF_SMWDR1:
> + if (rpc->xfer_size != 8)
> + return -EILSEQ;
> + break;
> }
>
> *val = readl(rpc->base + reg);
> @@ -193,18 +207,34 @@ static int rpcif_reg_write(void *context, unsigned int reg, unsigned int val)
> {
> struct rpcif *rpc = context;
>
> - if (reg == RPCIF_SMRDR0 || reg == RPCIF_SMWDR0) {
> - u32 spide = readl(rpc->base + RPCIF_SMENR) & RPCIF_SMENR_SPIDE(0xF);
> -
> - if (spide == 0x8) {
> + switch (reg) {
> + case RPCIF_SMWDR0:
> + switch (rpc->xfer_size) {
> + case 1:
> writeb(val, rpc->base + reg);
> return 0;
> - } else if (spide == 0xC) {
> +
> + case 2:
> writew(val, rpc->base + reg);
> return 0;
> - } else if (spide != 0xF) {
> +
> + case 4:
> + case 8:
> + break;
> +
> + default:
> return -EILSEQ;
> }
> + break;
> +
> + case RPCIF_SMWDR1:
> + if (rpc->xfer_size != 8)
> + return -EILSEQ;
> + break;
> +
> + case RPCIF_SMRDR0:
> + case RPCIF_SMRDR1:
> + return -EPERM;
> }
>
> writel(val, rpc->base + reg);
> @@ -469,6 +499,7 @@ int rpcif_manual_xfer(struct rpcif *rpc)
>
> smenr |= RPCIF_SMENR_SPIDE(rpcif_bits_set(rpc, nbytes));
> regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> + rpc->xfer_size = nbytes;
>
> memcpy(data, rpc->buffer + pos, nbytes);
> if (nbytes == 8) {
> @@ -533,6 +564,7 @@ int rpcif_manual_xfer(struct rpcif *rpc)
> regmap_write(rpc->regmap, RPCIF_SMENR, smenr);
> regmap_write(rpc->regmap, RPCIF_SMCR,
> rpc->smcr | RPCIF_SMCR_SPIE);
> + rpc->xfer_size = nbytes;
> ret = wait_msg_xfer_end(rpc);
> if (ret)
> goto err_out;
> diff --git a/include/memory/renesas-rpc-if.h b/include/memory/renesas-rpc-if.h
> index 7c93f5177532f187..9c0ad64b8d292d49 100644
> --- a/include/memory/renesas-rpc-if.h
> +++ b/include/memory/renesas-rpc-if.h
> @@ -72,6 +72,7 @@ struct rpcif {
> enum rpcif_type type;
> enum rpcif_data_dir dir;
> u8 bus_size;
> + u8 xfer_size;
> void *buffer;
> u32 xferlen;
> u32 smcr;
> --
> 2.25.1
>