lkml.org 
[lkml]   [2019]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
From
Date
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

\
 
 \ /
  Last update: 2019-04-06 22:00    [W:0.101 / U:1.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site