| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c08ad574-d067-4557-a50c-802d6d7fe353@linaro.org>
Date: Thu, 2 Nov 2023 09:53:26 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jyan Chou <jyanchou@...ltek.com>, ulf.hansson@...aro.org,
adrian.hunter@...el.com, jh80.chung@...sung.com,
riteshh@...eaurora.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
asutoshd@...eaurora.org
Cc: p.zabel@...gutronix.de, linux-mmc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
arnd@...db.de, briannorris@...omium.org, doug@...morgal.com,
tonyhuang.sunplus@...il.com, abel.vesa@...aro.org,
william.qiu@...rfivetech.com
Subject: Re: [PATCH V5][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver
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
Powered by blists - more mailing lists