Messages in this thread | | | From | Jyan Chou [周芷安] <> | Subject | RE: [PATCH V5][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver | Date | Thu, 9 Nov 2023 07:10:35 +0000 |
| |
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
| |