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

[Jyan Chou [周芷安]]

>> 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.
>>
>> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
>>
>> —--
>> v3 -> v4:
>> - Modify dma mode selection and dma addressing bit to statisfy
>>   linux coding style.
>>

> I asked to fix several coding style issues so it will look a bit as matching Linux coding style. I don't see improvements.

> Please read carefully, more than once, the Linux coding style. Then document in changelog what you fixed. If you document nothing, means you ignored the feedback.

> Fix every warning from checkpatch --strict. Then document in changelog what you fixed. If you document nothing, means you ignored the feedback.

Thanks for your remind, we had fixed more coding style issues and checked with checkpatch --strict.

>> +
>> +     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);
>> +
>> +     /*pio mode's parameters should be initialized here*/

> Nothing improved.

We had fixed it.

>> +
>> +     /*Initialize the eMMC IP related attribute*/
>> +     dw_mci_cqe_setup(host);
>> +
>> +     dw_mci_cqe_init_dma(host);
>> +
>> +     /* This flag will be set 1 when doing tuning,

> Nothing improved.

We had fixed it.

>> +      * we add this flag because
>> +      * some vendors might use other cmd instead of 21
>> +      * to tune phase under high speed interface.
>> +      * we use this flag to recognize if the system is under tuning stage.
>> +      */
>> +     host->tuning = 0;
>> +
>> +     /*Timing_setting is to avoid sending command

> Nothing improved.

We had fixed it.

>> +      *before setting phase in hs200, hs400
>> +      */
>> +     host->current_speed = 0;
>> +
>> +     /*Do the rest of init for specific*/
>> +     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;
>> +
>> +     /*After the slot initialization,

> Nothing improved.

We had fixed it.

>> +      *now we have mmc data and can initialize cmdq if user enabled
>> +      */
>> +     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);
>> +
>> +void dw_mci_cqe_remove(struct dw_mci *host) {
>> +     dev_dbg(host->dev, "remove slot\n");

> Nothing improved.

We had fixed it.

>> +     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);
>> +
>> +#ifdef CONFIG_PM
>> +int dw_mci_cqe_runtime_suspend(struct device *dev) {
>> +     struct dw_mci *host = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
>> +             if (host->slot) {
>> +                     dev_info(host->dev, "cqe suspend\n");

> Nothing improved.

We had dropped it.

>> +                     ret = cqhci_suspend(host->slot->mmc);
>> +                     if (ret) {
>> +                             dev_err(host->dev, "cqe suspend 
>> + failed\n");

> Nothing improved.

We had dropped it.

>> +                             return ret;
>> +                     }
>> +             }
>> +     }
>> +
>> +     clk_disable_unprepare(host->ciu_clk);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(dw_mci_cqe_runtime_suspend);
>> +
>> +int dw_mci_cqe_runtime_resume(struct device *dev) {
>> +     struct dw_mci *host = dev_get_drvdata(dev);
>> +     const struct dw_mci_drv_data *drv_data = host->drv_data;
>> +     int ret = 0;
>> +
>> +     clk_prepare_enable(host->ciu_clk);
>> +
>> +     dw_mci_cqe_setup(host);
>> +     if (drv_data && drv_data->init) {
>> +             ret = drv_data->init(host);
>> +             if (ret)
>> +                     dev_err(host->dev, "implementation specific init failed\n");
>> +     }
>> +
>> +     init_completion(host->int_waiting);
>> +
>> +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
>> +             if (host->slot) {
>> +                     dev_info(host->dev, "cqe resume\n");
>> +                     ret = cqhci_resume(host->slot->mmc);
>> +                     if (ret)
>> +                             dev_err(host->dev, "cqe resume 
>> + failed\n");


> Nothing improved.

We had fixed it.

>> +             }
>> +     }
>> +
>> +     dw_mci_cqe_setup_bus(host->slot, true);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(dw_mci_cqe_runtime_resume);
>> +#endif /* CONFIG_PM */
>> +
>> +static int __init dw_mci_cqe_init(void) {
>> +     pr_info("Synopsys Designware Multimedia Card Interface 
>> +Driver\n");


> Nothing improved.

We had dropped it.

>> +     return 0;
>> +}
>> +
>> +static void __exit dw_mci_cqe_exit(void) { }
>> +
>> +module_init(dw_mci_cqe_init);
>> +module_exit(dw_mci_cqe_exit);

> This part of code is just useless.

We had removed useless code.

Best regards,
Jyan

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 
Sent: Monday, October 30, 2023 3:39 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; p.zabel@xxxxxxxxxxxxxx
Cc: 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 V4][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver


External mail.



On 30/10/2023 07:27, 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.
>
> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
>
> —--
> v3 -> v4:
> - Modify dma mode selection and dma addressing bit to statisfy
>   linux coding style.
>

I asked to fix several coding style issues so it will look a bit as matching Linux coding style. I don't see improvements.

Please read carefully, more than once, the Linux coding style. Then document in changelog what you fixed. If you document nothing, means you ignored the feedback.

Fix every warning from checkpatch --strict. Then document in changelog what you fixed. If you document nothing, means you ignored the feedback.

> +
> +     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);
> +
> +     /*pio mode's parameters should be initialized here*/

Nothing improved.

> +
> +     /*Initialize the eMMC IP related attribute*/
> +     dw_mci_cqe_setup(host);
> +
> +     dw_mci_cqe_init_dma(host);
> +
> +     /* This flag will be set 1 when doing tuning,

Nothing improved.

> +      * we add this flag because
> +      * some vendors might use other cmd instead of 21
> +      * to tune phase under high speed interface.
> +      * we use this flag to recognize if the system is under tuning stage.
> +      */
> +     host->tuning = 0;
> +
> +     /*Timing_setting is to avoid sending command

Nothing improved.

> +      *before setting phase in hs200, hs400
> +      */
> +     host->current_speed = 0;
> +
> +     /*Do the rest of init for specific*/
> +     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;
> +
> +     /*After the slot initialization,

Nothing improved.

> +      *now we have mmc data and can initialize cmdq if user enabled
> +      */
> +     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);
> +
> +void dw_mci_cqe_remove(struct dw_mci *host) {
> +     dev_dbg(host->dev, "remove slot\n");

Nothing improved.

> +     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);
> +
> +#ifdef CONFIG_PM
> +int dw_mci_cqe_runtime_suspend(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     int ret = 0;
> +
> +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> +             if (host->slot) {
> +                     dev_info(host->dev, "cqe suspend\n");

Nothing improved.

> +                     ret = cqhci_suspend(host->slot->mmc);
> +                     if (ret) {
> +                             dev_err(host->dev, "cqe suspend 
> + failed\n");

Nothing improved.

> +                             return ret;
> +                     }
> +             }
> +     }
> +
> +     clk_disable_unprepare(host->ciu_clk);
> +
> +     return ret;
> +}
> +EXPORT_SYMBOL(dw_mci_cqe_runtime_suspend);
> +
> +int dw_mci_cqe_runtime_resume(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     const struct dw_mci_drv_data *drv_data = host->drv_data;
> +     int ret = 0;
> +
> +     clk_prepare_enable(host->ciu_clk);
> +
> +     dw_mci_cqe_setup(host);
> +     if (drv_data && drv_data->init) {
> +             ret = drv_data->init(host);
> +             if (ret)
> +                     dev_err(host->dev, "implementation specific init failed\n");
> +     }
> +
> +     init_completion(host->int_waiting);
> +
> +     if (host->pdata && (host->pdata->caps2 & MMC_CAP2_CQE)) {
> +             if (host->slot) {
> +                     dev_info(host->dev, "cqe resume\n");
> +                     ret = cqhci_resume(host->slot->mmc);
> +                     if (ret)
> +                             dev_err(host->dev, "cqe resume 
> + failed\n");


Nothing improved.

> +             }
> +     }
> +
> +     dw_mci_cqe_setup_bus(host->slot, true);
> +
> +     return ret;
> +}
> +EXPORT_SYMBOL(dw_mci_cqe_runtime_resume);
> +#endif /* CONFIG_PM */
> +
> +static int __init dw_mci_cqe_init(void) {
> +     pr_info("Synopsys Designware Multimedia Card Interface 
> +Driver\n");


Nothing improved.

> +     return 0;
> +}
> +
> +static void __exit dw_mci_cqe_exit(void) { }
> +
> +module_init(dw_mci_cqe_init);
> +module_exit(dw_mci_cqe_exit);

This part of code is just useless.

Best regards,
Krzysztof