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

["Jyan Chou [周芷安]" <jyanchou@realtek.com>]

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@linaro.org> 
Sent: Thursday, November 2, 2023 4:53 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; adrian.hunter@intel.com; jh80.chung@samsung.com; riteshh@codeaurora.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; asutoshd@codeaurora.org
Cc: p.zabel@pengutronix.de; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; arnd@arndb.de; briannorris@chromium.org; doug@schmorgal.com; tonyhuang.sunplus@gmail.com; abel.vesa@linaro.org; william.qiu@starfivetech.com
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