Re: [PATCH V5][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver

[Krzysztof Kozlowski]

On 02/11/2023 09:15, Jyan Chou wrote:
> We implemented cmdq feature on Synopsys DesignWare mmc driver.
> The difference between dw_mmc.c and dw_mmc_cqe.c were distinct
> register definitions, mmc user flow and the addition of cmdq.
> 
> New version of User Guide had modify mmc driver's usage flow,
> we may need to renew code to precisely follow user guide.
> 
> More over, We added a wait status function to satisfy synopsys
> user guide's description, since this flow might be specific in
> synopsys host driver only.

...

> +	for (i = 0; i < host->dma_nents; i++, sg++) {
> +		dma_len = sg_dma_len(sg);
> +
> +		/* blk_cnt must be the multiple of 512(0x200) */
> +		if (dma_len < SZ_512)
> +			blk_cnt = 1;
> +		else
> +			blk_cnt  = dma_len >> 9;
> +
> +		remain_blk_cnt  = blk_cnt;
> +		dma_addr = sg_dma_address(sg);
> +
> +		while (remain_blk_cnt) {
> +			/* DW_MCI_MAX_SCRIPT_BLK is the max
> +			 * for each descriptor record
> +			 */
> +			if (remain_blk_cnt > DW_MCI_MAX_SCRIPT_BLK)
> +				cur_blk_cnt = DW_MCI_MAX_SCRIPT_BLK;
> +			else
> +				cur_blk_cnt = remain_blk_cnt;
> +
> +			/* In Synopsys DesignWare Databook Page 84,

/*
 *

I mentioned it last time. Use Linux coding style. For entire patchset.

> +			 * They mentioned the DMA 128MB restriction
> +			 */
> +			begin = dma_addr / SZ_128M;
> +			end = (dma_addr + cur_blk_cnt * SZ_512) / SZ_128M;
> +
> +			/* If begin and end in the different 128MB memory zone */
> +			if (begin != end)
> +				cur_blk_cnt = (end * SZ_128M - dma_addr) / SZ_512;
> +
> +			if (dma_len < SZ_512)
> +				tmp_val = ((dma_len) << 16) | VALID(0x1) | ACT(0x4);
> +			else
> +				tmp_val = ((cur_blk_cnt & 0x7f) << 25) | VALID(0x1) | ACT(0x4);
> +
> +			/* Last descriptor */
> +			if (i == host->dma_nents - 1 && remain_blk_cnt == cur_blk_cnt)
> +				tmp_val |= END(0x1);
> +
> +			desc_base[0] =  tmp_val;
> +			desc_base[1] =  dma_addr;
> +
> +			dma_addr = dma_addr + (cur_blk_cnt << 9);
> +			remain_blk_cnt -= cur_blk_cnt;
> +			desc_base += 2;
> +		}
> +	}
> +}
> +

...

> +
> +#ifdef CONFIG_OF
> +static struct dw_mci_board *dw_mci_cqe_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_board *pdata;
> +	struct device *dev = host->dev;
> +	const struct dw_mci_drv_data *drv_data = host->drv_data;
> +	int ret;
> +	u32 clock_frequency;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* find reset controller when exist */
> +	pdata->rstc = devm_reset_control_get_optional(dev, "reset");

Where is it described in the bindings?

> +	if (IS_ERR(pdata->rstc)) {
> +		if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> +			return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	device_property_read_u32(dev, "card-detect-delay",

Where is it described in the bindings? It's not. This is v5 but, sorry,
but I do not see much improvements. You still send code which clearly is
wrong.


> +				 &pdata->detect_delay_ms);
> +
> +	if (!device_property_read_u32(dev, "clock-frequency", &clock_frequency))
> +		pdata->bus_hz = clock_frequency;

Drop property. I don't think it is needed. MMC and CCF has other ways to
do it.

> +
> +	if (drv_data && drv_data->parse_dt) {
> +		ret = drv_data->parse_dt(host);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	return pdata;
> +}
> +
> +#else /* CONFIG_OF */
> +static struct dw_mci_board *dw_mci_cqe_parse_dt(struct dw_mci *host)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +#endif /* CONFIG_OF */
> +
> +static void dw_mci_cqe_cto_timer(struct timer_list *t)
> +{
> +	struct dw_mci *host = from_timer(host, t, timer);
> +
> +	if (host->int_waiting) {
> +		dev_err(host->dev, "fired, opcode=%d, arg=0x%x, irq status=0x%x, err irq=0x%x, auto err irq=0x%x\n",
> +			host->opcode, host->arg,
> +			host->normal_interrupt, host->error_interrupt,
> +			host->auto_error_interrupt);
> +
> +		dw_mci_clr_signal_int(host);
> +		dw_mci_get_int(host);
> +
> +		complete(host->int_waiting);
> +	}
> +}
> +
> +static void dw_mci_cqhci_init(struct dw_mci *host)
> +{
> +	if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> +		host->cqe = cqhci_pltfm_init(host->pdev);
> +		if (PTR_ERR(host->cqe) == -EINVAL ||
> +		    PTR_ERR(host->cqe) == -ENOMEM ||
> +		    PTR_ERR(host->cqe) == -EBUSY) {
> +			dev_err(host->dev, "Unable to get the cmdq related attribute,err = %ld\n",
> +				PTR_ERR(host->cqe));
> +			host->cqe = 0;
> +			host->pdata->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
> +		} else {
> +			host->cqe->ops = &dw_mci_cqhci_host_ops;
> +			cqhci_init(host->cqe, host->slot->mmc, 0);
> +		}
> +	}
> +}
> +
> +int dw_mci_cqe_probe(struct dw_mci *host)
> +{
> +	const struct dw_mci_drv_data *drv_data = host->drv_data;
> +	int ret = 0;
> +
> +	if (!host->pdata) {
> +		host->pdata = dw_mci_cqe_parse_dt(host);
> +		if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
> +			return -EPROBE_DEFER;
> +		} else if (IS_ERR(host->pdata)) {
> +			dev_err(host->dev, "platform data not available\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	host->biu_clk = devm_clk_get(host->dev, "biu");
> +	if (IS_ERR(host->biu_clk)) {
> +		dev_dbg(host->dev, "biu clock not available\n");
> +	} else {
> +		ret = clk_prepare_enable(host->biu_clk);
> +		if (ret) {
> +			dev_err(host->dev, "failed to enable biu clock\n");
> +			return ret;
> +		}
> +	}

All this should be probably devm_clk_get_enabled().

> +
> +	host->ciu_clk = devm_clk_get(host->dev, "ciu");
> +	if (IS_ERR(host->ciu_clk)) {
> +		dev_dbg(host->dev, "ciu clock not available\n");
> +		host->bus_hz = host->pdata->bus_hz;
> +	} else {
> +		ret = clk_prepare_enable(host->ciu_clk);
> +		if (ret) {
> +			dev_err(host->dev, "failed to enable ciu clock\n");
> +			goto err_clk_biu;
> +		}
> +
> +		if (host->pdata->bus_hz) {
> +			ret = clk_set_rate(host->ciu_clk, host->pdata->bus_hz);
> +			if (ret)
> +				dev_warn(host->dev,
> +					 "Unable to set bus rate to %uHz\n",
> +					 host->pdata->bus_hz);
> +		}

All this should be probably devm_clk_get_enabled().


> +		host->bus_hz = clk_get_rate(host->ciu_clk);

So this proves that your property clock-frequency is useless.

> +	}
> +
> +	if (!host->bus_hz) {
> +		dev_err(host->dev,
> +			"Platform data must supply bus speed\n");
> +		ret = -ENODEV;
> +		goto err_clk_ciu;
> +	}
> +
> +	if (!IS_ERR(host->pdata->rstc)) {
> +		reset_control_assert(host->pdata->rstc);
> +		usleep_range(10, 50);
> +		reset_control_deassert(host->pdata->rstc);
> +	}
> +
> +	timer_setup(&host->timer, dw_mci_cqe_cto_timer, 0);
> +
> +	spin_lock_init(&host->lock);
> +	spin_lock_init(&host->irq_lock);
> +	init_rwsem(&host->cr_rw_sem);
> +	tasklet_init(&host->tasklet, dw_mci_cqe_tasklet_func, (unsigned long)host);
> +
> +	dw_mci_cqe_setup(host);
> +
> +	dw_mci_cqe_init_dma(host);
> +
> +	host->tuning = 0;
> +	host->current_speed = 0;
> +
> +	if (drv_data && drv_data->init) {
> +		ret = drv_data->init(host);
> +		if (ret) {
> +			dev_err(host->dev,
> +				"implementation specific init failed\n");
> +			goto err_dmaunmap;
> +		}
> +	}
> +
> +	ret = dw_mci_cqe_init_slot(host);
> +	if (ret) {
> +		dev_err(host->dev, "slot 0 init failed\n");
> +		goto err_dmaunmap;
> +	}
> +
> +	ret = devm_request_irq(host->dev, host->irq, dw_mci_cqe_interrupt,
> +			       host->irq_flags, "dw-mci-cqe", host);
> +	if (ret)
> +		goto err_dmaunmap;
> +
> +	dw_mci_cqhci_init(host);
> +
> +	return 0;
> +
> +err_dmaunmap:
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);
> +err_clk_ciu:
> +	clk_disable_unprepare(host->ciu_clk);
> +
> +err_clk_biu:
> +	clk_disable_unprepare(host->biu_clk);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(dw_mci_cqe_probe);

EXPORT_SYMBOL_GPL

I should have been more explicit:
EXPORT_SYMBOL_GPL everywhere. In this patchset and all your future
patchsets. For all your current and future code.

> +
> +void dw_mci_cqe_remove(struct dw_mci *host)
> +{
> +	if (host->slot)
> +		dw_mci_cqe_cleanup_slot(host->slot);
> +
> +	if (!IS_ERR(host->pdata->rstc))
> +		reset_control_assert(host->pdata->rstc);
> +
> +	clk_disable_unprepare(host->ciu_clk);
> +	clk_disable_unprepare(host->biu_clk);
> +}
> +EXPORT_SYMBOL(dw_mci_cqe_remove);
> +




Best regards,
Krzysztof