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:3.768 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site