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, ®s->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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
