Re: [PATCH 3/3] mtd: rawnand: brcmnand: Add support for flash-edu for dma transfers

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Kamal,

Kamal Dasu <kdasu.kdev@gmail.com> wrote on Wed, 20 Nov 2019 13:20:59
-0500:

> Legacy mips soc platforms that have controller v5.0 and 6.0 use
> flash-edu block for dma transfers. This change adds support for
> nand dma transfers using the EDU block.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>

I don't have the patch 2/3 in my mailbox :-/ Can you please resend with
the right numbering or Cc myself on the 2nd?

Otherwise, minor comments below :)

> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 269 ++++++++++++++++++++++-
>  1 file changed, 263 insertions(+), 6 deletions(-)

[...]

> +/* edu irq */
> +static irqreturn_t brcmnand_edu_irq(int irq, void *data)
> +{
> +	struct brcmnand_controller *ctrl = data;
> +
> +	if (ctrl->edu_count) {
> +		ctrl->edu_count--;
> +		while (!edu_readl(ctrl, EDU_DONE))
> +			udelay(1);
> +		edu_writel(ctrl, EDU_DONE, 0);
> +		(void)edu_readl(ctrl, EDU_DONE);

Why this cast? (and all the others)

> +	}
> +
> +	if (ctrl->edu_count) {
> +		ctrl->edu_dram_addr += FC_BYTES;
> +		ctrl->edu_ext_addr += FC_BYTES;
> +
> +		edu_writel(ctrl, EDU_DRAM_ADDR, (u32)ctrl->edu_dram_addr);
> +		(void)edu_readl(ctrl, EDU_DRAM_ADDR);
> +		edu_writel(ctrl, EDU_EXT_ADDR, ctrl->edu_ext_addr);
> +		(void)edu_readl(ctrl, EDU_EXT_ADDR);

[...]

>  
> @@ -2561,6 +2767,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  
>  	init_completion(&ctrl->done);
>  	init_completion(&ctrl->dma_done);
> +	init_completion(&ctrl->edu_done);
>  	nand_controller_init(&ctrl->controller);
>  	ctrl->controller.ops = &brcmnand_controller_ops;
>  	INIT_LIST_HEAD(&ctrl->host_list);
> @@ -2650,6 +2857,56 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  		dev_info(dev, "enabling FLASH_DMA\n");
>  	}
>  
> +	/* use EDU DMA only no FLASH_DMA present */
> +	if (has_flash_dma(ctrl))
> +		res = 0;
> +	else
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   "flash-edu");

Can we simplify this block?

> +
> +	if (res) {

What about a dedicated helper to do the EDU configuration only?

> +		ctrl->edu_base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(ctrl->edu_base))
> +			return PTR_ERR(ctrl->edu_base);
> +
> +		ctrl->edu_offsets = edu_regs;
> +
> +		edu_writel(ctrl, EDU_CONFIG, EDU_CONFIG_MODE_NAND |
> +			   EDU_CONFIG_SWAP_CFG);
> +		(void)edu_readl(ctrl, EDU_CONFIG);
> +
> +		/* initialize edu */
> +		edu_writel(ctrl, EDU_ERR_STATUS, 0);
> +		edu_writel(ctrl, EDU_DONE, 0);
> +		(void)edu_readl(ctrl, EDU_DONE);
> +
> +		ctrl->edu_irq = platform_get_irq(pdev, 1);
> +		if ((int)ctrl->edu_irq < 0) {
> +			dev_warn(dev,
> +				 "FLASH EDU enabled, using ctlrdy irq\n");
> +		} else {
> +			ret = devm_request_irq(dev, ctrl->edu_irq,
> +					       brcmnand_edu_irq, 0,
> +					       "brcmnand-edu", ctrl);
> +			if (ret < 0) {
> +				dev_err(dev, "can't allocate IRQ %d: error %d\n",
> +					ctrl->edu_irq, ret);
> +				return ret;
> +			}
> +
> +			dev_info(dev, "FLASH EDU enabled using irq %u\n",
> +				 ctrl->edu_irq);
> +		}
> +	}
> +
> +	/* set the appropriate dma transfer function to call */
> +	if (has_flash_dma(ctrl))
> +		ctrl->dma_trans = brcmnand_dma_trans;
> +	else if (has_edu(ctrl))
> +		ctrl->dma_trans = brcmnand_edu_trans;
> +	else
> +		ctrl->dma_trans = NULL;
> +
>  	/* Disable automatic device ID config, direct addressing */
>  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT,
>  			 CS_SELECT_AUTO_DEVICE_ID_CFG | 0xff, 0, 0);

Thanks,
Miquèl