| Message ID | b409ea3fae4a6778fd6b44815ad17c9e8e10df87.1647512831.git.geert+renesas@glider.be |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] memory: renesas-rpc-if: Fix HF/OSPI data transfer in Manual mode | expand |
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 <geert+renesas@glider.be> 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 <geert+renesas@glider.be> > --- > 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 <prabhakar.mahadev-lad.rj@bp.renesas.com> [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 >
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;
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 <geert+renesas@glider.be> --- 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(-)