Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

[Archit Taneja]

On 07/29/2015 07:18 AM, Stephen Boyd wrote:
> On 07/27/2015 09:34 PM, Archit Taneja wrote:
> > Hi,
> >
> > On 07/25/2015 06:21 AM, Stephen Boyd wrote:
> > > On 07/21/2015 03:34 AM, Archit Taneja wrote:
> > >
> > > > +              int size)
> > > > +{Looks like a
> > > > +    struct desc_info *desc;
> > > > +    struct dma_async_tx_descriptor *dma_desc;
> > > > +    struct scatterlist *sgl;
> > > > +    int r;
> > > > +
> > > > +    desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> > > > +    if (!desc)
> > > > +        return -ENOMEM;
> > > > +
> > > > +    list_add_tail(&desc->list, &this->list);
> > > > +
> > > > +    sgl = &desc->sgl;
> > > > +
> > > > +    sg_init_one(sgl, vaddr, size);
> > > > +
> > > > +    desc->dir = DMA_MEM_TO_DEV;
> > > > +
> > > > +    r = dma_map_sg(this->dev, sgl, 1, desc->dir);
> > > > +    if (r == 0)
> > > > +        goto err;
> > >
> > > Should we return an error in this case? Looks like return 0.
> >
> > dma_map_sg returns the number of sg entries successfully mapped. In
> > this case, it should be 1.
>
> Right, but this function returns 0 (success?) if we failed to map anything.

Yes. The return value is number of entries successfully mapped. 
dma_map_sg is a macro that is replaced by dma_map_sg_attrs. Its comment
says:

"dma_maps_sg_attrs returns 0 on error and > 0 on success. It should 
never return a value < 0."

> > > > +
> > > > +    this->slave_conf.device_fc = 0;
> > > > +    this->slave_conf.dst_addr = this->res->start + reg_off;
> > > > +    this->slave_conf.dst_maxburst = 16;
> > >
> > > Is there any reason why slave_conf can't be on the stack? Otherwise it's
> > > odd that it's overwritten a few times before we submit the descriptors,
> > > so it must be copied by the dmaengine provider, but that isn't clear at
> > > all from the code. If it isn't copied, perhaps it should be part of the
> > > desc_info structure. If it is copied I wonder why it isn't const in the
> > > function signature.
> >
> > The dmaengine drivers either memcpy slave_config in their
> > device_config() dmaengine op, or populate their local members reading
> > params in the passed slave_config.
> >
> > I'll move slave_conf to stack. As you said, the config argument
> > in dmaengine_slave_config should ideally use const.
>
> Cool, someone should send a patch.
>
> > > > +
> > > > +    r = dmaengine_slave_config(this->chan, &this->slave_conf);
> > > > +    if (r) {
> > > > +        dev_err(this->dev, "failed to configure dma channel\n");
> > > > +        goto err;
> > > > +    }
> > > > +
> > > > +    dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1,
> > > > desc->dir, 0);
> > > > +    if (!dma_desc) {
> > > > +        dev_err(this->dev, "failed to prepare data write desc\n");
> > > > +        r = PTR_ERR(dma_desc);
> > > > +        goto err;
> > > > +    }
> > > > +
> > > > +    desc->dma_desc = dma_desc;
> > > > +
> > > > +    return 0;
> > > > +err:
> > > > +    kfree(desc);
> > > > +
> > > > +    return r;
> > > > +}
> > > > +
> > > > +/*
> > > > + * helper to prepare dma descriptors to configure registers needed
> > > > for reading a
> > > > + * codeword/step in a page
> > > > + */
> > > > +static void config_cw_read(struct qcom_nandc_data *this)
> > > > +{
> > > > +    struct nandc_regs *regs = this->regs;
> > > > +
> > > > +    write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
> > >
> > > Maybe it would be better to have a case statement inside
> > > {write,read}_reg_dma() that looked at the second argument and matched it
> > > up with an offset in regs. Then this could be
> > >
> > >      write_reg_dma(this, NAND_FLASH_CMD, 3, true);
> >
> > That's a good idea. However, we have at least one programming seqeunce
> > (in nandc_param) where we need to write two different values to the
> > same register. In such a case, we need two different locations to
> > store the two values.
> >
> > I can split up the programming sequence into two parts such that we
> > won't write to the same register twice. But doing this for the sake of
> > reducing an argument to write_reg_dma seems a bit unnecessary.
>
> Or you could have two #defines that indicate the different usage? Like
> NAND_CMD_FOO and NAND_CMD_BAR that both map to the same register but
> uses different locations as storage.

Yeah, that seems like a good option. I'll try this out.

> > > Are we guaranteed that this is called within the same context as where
> > > the buffer is passed to this function? Otherwise this stack check isn't
> > > going to work because object_is_on_stack() will silently fail.
> >
> > These are funcs are finally tied to mtd ops. I think in normal operation
> > it'll be the same context. I'll still cross check. The aim of the check
> > is to prevent a memcpy of the buffer to/from the layer above. A false
> > negative will result in a slower read/write operation.
>
> Right. It would be nice if the mtd layer was DMA aware and could
> indicate to drivers that DMA on the buffer is allowable or not. Trying
> to figure it out if the buffer is in lowmem after the buffer is mapped
> is error prone, which is why not a lot of usage of object_is_on_stack()
> exists. Honestly I'm confused, I thought the DMA APIs would "do the
> right thing" when highmem was passed into the mapping APIs, but maybe
> I'm wrong. I'll have to look.

It looks like NAND_USE_BOUNCE_BUFFER does just that. If we set this
flag, the nand_base layer provides a DMA-able buffer even if the
filesystem didn't.

No one was using this flag when I last checked. A new driver
brcmnand now uses it. I'll give this a try.

Archit

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/