On Wed, 15 Dec 2021 at 14:09, Gabriel Somlo <gsomlo@xxxxxxxxx> wrote: > > LiteX (https://github.com/enjoy-digital/litex) is a SoC framework > that targets FPGAs. LiteSDCard is a small footprint, configurable > SDCard core commonly used in LiteX designs. > > The driver was first written in May 2020 and has been maintained > cooperatively by the LiteX community. Thanks to all contributors! > > Co-developed-by: Kamil Rakoczy <krakoczy@xxxxxxxxxxxx> > Signed-off-by: Kamil Rakoczy <krakoczy@xxxxxxxxxxxx> > Co-developed-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx> > Co-developed-by: Paul Mackerras <paulus@xxxxxxxxxx> > Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx> > Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx> > Cc: Mateusz Holenko <mholenko@xxxxxxxxxxxx> > Cc: Karol Gugala <kgugala@xxxxxxxxxxxx> > Cc: Joel Stanley <joel@xxxxxxxxx> > Cc: Stafford Horne <shorne@xxxxxxxxx> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > Cc: David Abdurachmanov <david.abdurachmanov@xxxxxxxxxx> > Cc: Florent Kermarrec <florent@xxxxxxxxxxxxxxxx> > Reviewed-by: Joel Stanley <joel@xxxxxxxxx> [...] > + > +static int litex_mmc_set_bus_width(struct litex_mmc_host *host) > +{ > + bool app_cmd_sent; > + int ret; > + > + if (host->is_bus_width_set) > + return 0; > + > + /* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */ > + app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */ > + if (!app_cmd_sent) { > + ret = litex_mmc_send_app_cmd(host); > + if (ret) > + return ret; > + } > + > + /* litesdcard only supports 4-bit bus width */ > + ret = litex_mmc_send_set_bus_w_cmd(host, MMC_BUS_WIDTH_4); > + if (ret) > + return ret; > + > + /* re-send 'app_cmd' if necessary */ > + if (app_cmd_sent) { > + ret = litex_mmc_send_app_cmd(host); > + if (ret) > + return ret; > + } I understand that you are trying to address the limitation that the controller supports 4-bit width only. In principle it looks like we may need to violate the SD spec to be able to initialise an SD card, right? Isn't that a concern for you? > + > + host->is_bus_width_set = true; > + > + return 0; > +} [...] > + > +static void litex_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) > +{ > + struct litex_mmc_host *host = mmc_priv(mmc); > + struct device *dev = mmc_dev(mmc); > + struct mmc_command *cmd = mrq->cmd; > + struct mmc_command *sbc = mrq->sbc; > + struct mmc_data *data = mrq->data; > + struct mmc_command *stop = mrq->stop; > + unsigned int retries = cmd->retries; > + unsigned int len = 0; > + bool direct = false; > + u32 response_len = litex_mmc_response_len(cmd); > + u8 transfer = SD_CTL_DATA_XFER_NONE; > + > + /* First check that the card is still there */ > + if (!litex_mmc_get_cd(mmc)) { > + cmd->error = -ENOMEDIUM; > + mmc_request_done(mmc, mrq); > + return; > + } > + > + /* Send set-block-count command if needed */ > + if (sbc) { > + sbc->error = litex_mmc_send_cmd(host, sbc->opcode, sbc->arg, > + litex_mmc_response_len(sbc), > + SD_CTL_DATA_XFER_NONE); > + if (sbc->error) { > + host->is_bus_width_set = false; > + mmc_request_done(mmc, mrq); > + return; > + } > + } > + > + if (data) { > + /* LiteSDCard only supports 4-bit bus width; therefore, we MUST > + * inject a SET_BUS_WIDTH (acmd6) before the very first data > + * transfer, earlier than when the mmc subsystem would normally > + * get around to it! This means that you may end up trying to switch bus-width, to a width that isn't supported by the card, for example. As also stated above, I wonder how this conforms to the SD spec from the initialization sequence point of view. Have you verified that this isn't a problem? > + */ > + cmd->error = litex_mmc_set_bus_width(host); > + if (cmd->error) { > + dev_err(dev, "Can't set bus width!\n"); > + mmc_request_done(mmc, mrq); > + return; > + } > + > + litex_mmc_do_dma(host, data, &len, &direct, &transfer); > + } > + > + do { > + cmd->error = litex_mmc_send_cmd(host, cmd->opcode, cmd->arg, > + response_len, transfer); > + } while (cmd->error && retries-- > 0); > + > + if (cmd->error) { > + /* card may be gone; don't assume bus width is still set */ > + host->is_bus_width_set = false; > + } > + > + if (response_len == SD_CTL_RESP_SHORT) { > + /* pull short response fields from appropriate host registers */ > + cmd->resp[0] = host->resp[3]; > + cmd->resp[1] = host->resp[2] & 0xFF; > + } else if (response_len == SD_CTL_RESP_LONG) { > + cmd->resp[0] = host->resp[0]; > + cmd->resp[1] = host->resp[1]; > + cmd->resp[2] = host->resp[2]; > + cmd->resp[3] = host->resp[3]; > + } > + > + /* Send stop-transmission command if required */ > + if (stop && (cmd->error || !sbc)) { > + stop->error = litex_mmc_send_cmd(host, stop->opcode, stop->arg, > + litex_mmc_response_len(stop), > + SD_CTL_DATA_XFER_NONE); > + if (stop->error) > + host->is_bus_width_set = false; > + } > + > + if (data) { > + dma_unmap_sg(dev, data->sg, data->sg_len, > + mmc_get_dma_dir(data)); > + } > + > + if (!cmd->error && transfer != SD_CTL_DATA_XFER_NONE) { > + data->bytes_xfered = min(len, mmc->max_req_size); > + if (transfer == SD_CTL_DATA_XFER_READ && !direct) { > + sg_copy_from_buffer(data->sg, sg_nents(data->sg), > + host->buffer, data->bytes_xfered); > + } > + } > + > + mmc_request_done(mmc, mrq); > +} > + [...] > + > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; I noticed that you use these hard coded values and don't really care to manage voltage changes via ->set_ios(). Rather than doing it like this, I would prefer if you can hook up a fixed vmmc regulator in the DTS. Then call mmc_regulator_get_supply() to fetch it from here, which will let the mmc core create the mmc->ocr_avail mask, based upon the voltage level the regulator supports. This becomes more generic and allows more flexibility for the platform configuration. > + mmc->ops = &litex_mmc_ops; > + > + /* set default sd_clk frequency range based on empirical observations > + * of LiteSDCard gateware behavior on typical SDCard media > + */ > + mmc->f_min = 12.5e6; > + mmc->f_max = 50e6; > + > + ret = mmc_of_parse(mmc); > + if (ret) > + goto err; > + > + /* force 4-bit bus_width (only width supported by hardware) */ > + mmc->caps &= ~MMC_CAP_8_BIT_DATA; > + mmc->caps |= MMC_CAP_4_BIT_DATA; > + > + /* set default capabilities */ > + mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | > + MMC_CAP_DRIVER_TYPE_D | > + MMC_CAP_CMD23; > + mmc->caps2 |= MMC_CAP2_NO_WRITE_PROTECT | > + MMC_CAP2_FULL_PWR_CYCLE | A full power cycle requires you to be able to power on/off the vmmc regulator (unless this is internally managed by the controller). Can you really do that? > + MMC_CAP2_NO_SDIO; We should add MMC_CAP2_NO_MMC here as well, as it looks like it can't be supported. Right? > + > + platform_set_drvdata(pdev, host); > + > + ret = mmc_add_host(mmc); > + if (ret < 0) > + goto err; > + > + return 0; > + > +err: > + mmc_free_host(mmc); > + return ret; > +} > + > +static int litex_mmc_remove(struct platform_device *pdev) > +{ > + struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev); > + > + if (host->irq > 0) > + free_irq(host->irq, host->mmc); > + mmc_remove_host(host->mmc); > + mmc_free_host(host->mmc); > + > + return 0; > +} > + > +static const struct of_device_id litex_match[] = { > + { .compatible = "litex,mmc" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, litex_match); > + > +static struct platform_driver litex_mmc_driver = { > + .probe = litex_mmc_probe, > + .remove = litex_mmc_remove, > + .driver = { > + .name = "litex-mmc", > + .of_match_table = of_match_ptr(litex_match), > + }, > +}; > +module_platform_driver(litex_mmc_driver); > + > +MODULE_DESCRIPTION("LiteX SDCard driver"); > +MODULE_AUTHOR("Antmicro <contact@xxxxxxxxxxxx>"); > +MODULE_AUTHOR("Kamil Rakoczy <krakoczy@xxxxxxxxxxxx>"); > +MODULE_AUTHOR("Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx>"); > +MODULE_AUTHOR("Paul Mackerras <paulus@xxxxxxxxxx>"); > +MODULE_AUTHOR("Gabriel Somlo <gsomlo@xxxxxxxxx>"); > +MODULE_LICENSE("GPL v2"); > -- Other than the comments above, the code looks nice and was easy to review, thanks! Kind regards Uffe
