Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
From: Sergei Shtylyov
Date: Tue Apr 02 2019 - 16:10:31 EST
Hello!
On 04/02/2019 08:48 AM, masonccyang@xxxxxxxxxxx wrote:
>> > +static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
>> > + const struct spi_mem_op *op,
>> > + u64 *offs, size_t *len)
>> > +{
>> > + struct rpc_spi *rpc = spi_controller_get_devdata(spi->controller);
>> > +
>> > + rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
>> > + rpc->smenr = RPC_SMENR_CDE |
>> > + RPC_SMENR_CDB(ilog2(op->cmd.buswidth));
>> > + rpc->totalxferlen = 1;
>> > + rpc->xfer_dir = SPI_MEM_NO_DATA;
>> > + rpc->xferlen = 0;
>> > + rpc->addr = 0;
>> > +
>> > + if (op->addr.nbytes) {
>> > + rpc->smenr |= RPC_SMENR_ADB(ilog2(op->addr.buswidth));
>> > + if (op->addr.nbytes == 4)
>> > + rpc->smenr |= RPC_SMENR_ADE(0xf);
>> > + else
>> > + rpc->smenr |= RPC_SMENR_ADE(0x7);
>> > +
>> > + if (offs && len)
>> > + rpc->addr = *offs;
>> > + else
>> > + rpc->addr = op->addr.val;
>> > + rpc->totalxferlen += op->addr.nbytes;
>> > + }
>> > +
>> > + if (op->dummy.nbytes) {
>> > + rpc->smenr |= RPC_SMENR_DME;
>> > + rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
>>
>> So you haven't fixed this bug? I repeat, the driver doesn't work right
>> w/o this fixed!
>
> Do you mean
>
> rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes) * 8; ?
Yes. Or some other code that amounts to it.
> What is your flash part number?
Spansion/Cypress S25FS512S. The datasheet says there must be 8 dummy cycles
for the RSFDP command...
> The problem you found is in 1 bit or 4 bits width ?
1-bit, I think.
>>
>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>> > + u64 offs, size_t len, void *buf)
>> > +{
>> > + struct rpc_spi *rpc =
>> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> > + int ret;
>> > +
>> > + if (offs + desc->info.offset + len > U32_MAX)
>> > + return -EINVAL;
>> > +
>> > + if (len > 0x4000000)
>> > + len = 0x4000000;
>>
>> Ugh...
>
> by mtd->size ?
That'd be better, yes.
>> > +
>> > + ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
>> > + &desc->info.op_tmpl, &offs, &len);
>> > +
>> > + regmap_update_bits(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD, 0);
>> > + regmap_write(rpc->regmap, RPC_DRCR, RPC_DRCR_RBURST(32) |
>> > + RPC_DRCR_RBE);
>> > +
>> > + regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>
>> So you're not even trying to support flashes larger than the read dirmap?
>> Now I don't think it's acceptable (and I have rewritten this code internally).
>
> what about the size comes form mtd->size ?
I'm not talking about size here; we should use the full address. I'm attaching
my patch...
>> [...]
>> > +static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
>> > + u64 offs, size_t len, const void *buf)
>> > +{
>> > + struct rpc_spi *rpc =
>> > + spi_controller_get_devdata(desc->mem->spi->controller);
>> > + int ret;
>> > +
>> > + if (offs + desc->info.offset + len > U32_MAX)
>> > + return -EINVAL;
>> > +
>> > + if (len > RPC_WBUF_SIZE)
>> > + len = RPC_WBUF_SIZE;
>>
>> Ugh! Again, I no longer think this is acceptable... Maybe we just need
>> to drop the support of the write buffer...
>>
>
> why ?
> Using write buffer got the much better write performance.
OK, let's keep it then.
[...]
MBR, Sergei
From: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
Subject: spi: renesas-rpc: allow reading from large flashes
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
---
drivers/spi/spi-renesas-rpc.c | 26 ++++++++++++++++++++------
include/linux/mfd/renesas-rpc.h | 2 ++
2 files changed, 22 insertions(+), 6 deletions(-)
Index: renesas/drivers/spi/spi-renesas-rpc.c
===================================================================
--- renesas.orig/drivers/spi/spi-renesas-rpc.c
+++ renesas/drivers/spi/spi-renesas-rpc.c
@@ -264,14 +264,12 @@ static ssize_t rpc_spi_mem_dirmap_read(s
{
struct rpc_spi *rpc =
spi_controller_get_devdata(desc->mem->spi->controller);
+ size_t _len = len;
int ret;
if (offs + desc->info.offset + len > U32_MAX)
return -EINVAL;
- if (len > 0x4000000)
- len = 0x4000000;
-
ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
if (ret)
return ret;
@@ -284,15 +282,31 @@ static ssize_t rpc_spi_mem_dirmap_read(s
RPC_DRCR_RBE);
regmap_write(rpc->regmap, RPC_DRCMR, rpc->cmd);
- regmap_write(rpc->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
regmap_write(rpc->regmap, RPC_DROPR, 0);
regmap_write(rpc->regmap, RPC_DRENR, rpc->smenr);
regmap_write(rpc->regmap, RPC_DRDMCR, rpc->dummy);
regmap_write(rpc->regmap, RPC_DRDRENR, 0);
- memcpy_fromio(buf, rpc->dirmap + desc->info.offset + offs, len);
+ while (len) {
+ loff_t from = offs & (RPC_DIRMAP_SIZE - 1);
+ size_t size = RPC_DIRMAP_SIZE - from;
+ u32 val = RPC_DREAR_EAC(1);
+
+ if (rpc->smenr & RPC_DRENR_ADE(8))
+ val |= RPC_DREAR_EAV(offs >> 25);
+ regmap_write(rpc->regmap, RPC_DREAR, val);
+
+ if (size > len)
+ size = len;
+
+ memcpy_fromio(buf, rpc->dirmap + desc->info.offset + from,
+ size);
+ buf += size;
+ offs += size;
+ len -= size;
+ }
- return len;
+ return _len;
}
static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
Index: renesas/include/linux/mfd/renesas-rpc.h
===================================================================
--- renesas.orig/include/linux/mfd/renesas-rpc.h
+++ renesas/include/linux/mfd/renesas-rpc.h
@@ -57,6 +57,7 @@
#define RPC_DRCMR_OCMD(c) (((c) & 0xFF) << 0)
#define RPC_DREAR 0x0014 // R/W
+#define RPC_DREAR_EAV(v) (((v) & 0xff) << 16)
#define RPC_DREAR_EAC(c) (((c) & 0x7) << 0)
#define RPC_DROPR 0x0018 // R/W
@@ -140,6 +141,7 @@
#define RPC_PHYOFFSET2 0x0084 // R/W
#define RPC_PHYOFFSET2_OCTTMG(v) (((v) & 0x7) << 8)
+#define RPC_DIRMAP_SIZE 0x4000000
#define RPC_WBUF_SIZE 256 // Write Buffer size
struct rpc_mfd {