Hello!

On 04/02/2019 08:48 AM, masonccyang@mxic.com.tw 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