Hi Krzysztof,
>> 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.
Thanks for your remind. Sorry for didn't correct some of Linux coding style error.
>> + * 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?
I will add it in new version and test it.
>> + 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.
It doesn't need to be in the bindings, I will remove this useless code.
>> + &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.
Alright, I will drop 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().
Thanks. We will correct it.
>> +
>> + 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().
Thanks. We will correct it.
>> + host->bus_hz = clk_get_rate(host->ciu_clk);
> So this proves that your property clock-frequency is useless.
Yes... we will remove it in bindings.
>> + }
>> +
>> + 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.
Thanks. We will use EXPORT_SYMBOL_GPL in all our patchset.
>> +
>> +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);
>> +
-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
Sent: Thursday, November 2, 2023 4:53 PM
To: Jyan Chou [周芷安] <jyanchou@xxxxxxxxxxx>; ulf.hansson@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; jh80.chung@xxxxxxxxxxx; riteshh@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx
Cc: p.zabel@xxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; briannorris@xxxxxxxxxxxx; doug@xxxxxxxxxxxxx; tonyhuang.sunplus@xxxxxxxxx; abel.vesa@xxxxxxxxxx; william.qiu@xxxxxxxxxxxxxxxx
Subject: Re: [PATCH V5][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver
External mail.
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
