[PATCH v2 2/2] spi: npcm-fiu: add NPCM FIU controller driver

[Tomer Maimon]

O.K.

Thanks a lot for the clarifications

I will add it in the next patch.

On Fri, 9 Aug 2019 at 18:51, Boris Brezillon <boris.brezillon at collabora.com>
wrote:

> On Fri, 9 Aug 2019 18:47:08 +0300
> Tomer Maimon <tmaimon77 at gmail.com> wrote:
>
> > On Fri, 9 Aug 2019 at 18:26, Boris Brezillon <
> boris.brezillon at collabora.com>
> > wrote:
> >
> > > On Fri, 9 Aug 2019 18:26:23 +0300
> > > Tomer Maimon <tmaimon77 at gmail.com> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > Thanks a lot for your comment.
> > > >
> > > > On Thu, 8 Aug 2019 at 18:32, Boris Brezillon <
> > > boris.brezillon at collabora.com>
> > > > wrote:
> > > >
> > > > > On Thu,  8 Aug 2019 16:14:48 +0300
> > > > > Tomer Maimon <tmaimon77 at gmail.com> wrote:
> > > > >
> > > > >
> > > > > > +
> > > > > > +static const struct spi_controller_mem_ops npcm_fiu_mem_ops = {
> > > > > > +     .exec_op = npcm_fiu_exec_op,
> > > > >
> > > > > No npcm_supports_op()? That's suspicious, especially after looking
> at
> > > > > the npcm_fiu_exec_op() (and the functions called from there) where
> the
> > > > > requested ->buswidth seems to be completely ignored...
> > > > >
> > > > > Sorry but I do not fully understand it, do you mean a support for
> the
> > > > buswidth?
> > > > If yes it been done in the UMA functions as follow:
> > > >
> > > >                 uma_cfg |= ilog2(op->cmd.buswidth);
> > > >                 uma_cfg |= ilog2(op->addr.buswidth) <<
> > > >                         NPCM_FIU_UMA_CFG_ADBPCK_SHIFT;
> > > >                 uma_cfg |= ilog2(op->data.buswidth) <<
> > > >                         NPCM_FIU_UMA_CFG_WDBPCK_SHIFT;
> > > >                 uma_cfg |= op->addr.nbytes <<
> > > NPCM_FIU_UMA_CFG_ADDSIZ_SHIFT;
> > > >                 regmap_write(fiu->regmap, NPCM_FIU_UMA_ADDR,
> > > op->addr.val);
> > > >
> > >
> > > Hm, the default supports_op() implementation might be just fine for
> > > your use case. But there's one thing you still need to check: the
> > > number of addr cycles (or address size as you call it in this driver).
> > > Looks like your IP is limited to 4 address cycles, if I'm right, you
> > > should reject any operation that have op->addr.nbytes > 4. I also
> > >
> > Indeed our IP limited to 4 address cycle (bytes) do we have NOR Flash
> with
> > more than 32bit address?
>
> spi-mem is not only about spi-nor, it can be used for any kind of
> memory (NOR, NAND, SRAM, ...) or even to communicate with an FGPA, so
> yes, you have to take care of that.
>
> > I will add this limitation thanks!
> >
> > > wonder if there's a limitation on the data size you can have on a
> > > single transfer. If there's one you should implement ->adjust_op() too.
> > >
> > there is a limitation in a single transfer but I handle it in the
> > npcm_fiu_manualwrite
> > function.
> > Do you suggest to use ->adjust_op() instead?
>
> Yes, should be exposed through ->adjust_op() => the caller needs to
> know when a new operation (one containing an opcode+address) is issued,
> because sometimes such splits are not supported by the memory.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20190809/9ac7a31a/attachment.htm>