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