On 04/04/2019 10:12 PM, Boris Brezillon wrote:
[...]
>>>>>>> +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.
>>>>
>>>
>>> Oops, it seems hard to get mtd->size info. from spi_mem_dirmap,
>>
>> It's in desc->info.length, no?
>
> It's the lengths of the mapping which not necessarily mtd->size, but in
> the SPI NOR case it is :-). Anyway, you should not assume
> dirmapdesc->info.length == memory_device->size.
>
>>
>>> I would like to keep 0x4000000.
>>
>> I'm seeing Boris in the CC's... Boris, is it legitimate to limit
>> a single dirmap read by the memory "window" size? Or should we try to
>> serve any valid transfer length?
>
> If by memory window you're talking about the memory region reserved in
Yes, we have 64 MiB window thru which we can "look into" the large MTD chips.
> the CPU address space, then no. It should not be limited to this size
> if possible.
Mhm, so we're expected to loop incrementing the window address register
in order to serve the full xfer request?
> Most HW have a way to configure an offset to apply to the spi-mem operation,
> and in that case, the driver should change this
> offset on the fly when one tries to access a region that's outside of
> the currently configured window.
Well, my question wasn't about that actually -- that seemed quite obvious.
>>>>>>> +
>>>>>>> + 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...
>>>
>>> okay,got it!
>>> what about just
>>> - regmap_write(rpc->mfd->regmap, RPC_DREAR, RPC_DREAR_EAC(1));
>>> + regmap_write(rpc->mfd->regmap, RPC_DREAR,
>>> + RPC_DREAR_EAV(offs >> 25) | RPC_DREAR_EAC(1));
>>>
>>> because only > 64MByte size make RPC_DREAR_EAV() work.
>>
>> Boris, what's your opinion on this?
>> Note that for the write dirmap we have just 256-byte buffer (reusing
>> the read cache). Is it legitimate to limit the served length to 256 bytes?
> I don't know what the HW is capable of,
As I said, there's 64 MiB read window, and for the writes we can re-use the
read cache to write (exactly) 256 bytes at a time...
> but drivers should use any try
> they have to dynamically move the memory map window (make it point at a
> different spi-mem offset on demand). Note that dirmap_read/write() are
> allowed to return less than the amount of data requested, in that case
> the caller should continue reading at the offset where things stopped.
> This avoids having to implement a loop that splits things in several
> accesses when the access cannot be done in one step.
Ah, this somewhat contradicts what you said earlier but seems clear now.
I'll go remove the loops. :-)
MBR, Sergei
