Re: [PATCH v9 2/3] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

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