| 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
| ||
|
Date: Tue, 28 Dec 2021 17:15:25 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Gabriel Somlo <gsomlo@...il.com>
Cc: linux-kernel@...r.kernel.org, robh+dt@...nel.org,
devicetree@...r.kernel.org, linux-mmc@...r.kernel.org,
kgugala@...micro.com, mholenko@...micro.com, krakoczy@...micro.com,
mdudek@...ernships.antmicro.com, paulus@...abs.org, joel@....id.au,
shorne@...il.com, geert@...ux-m68k.org,
david.abdurachmanov@...ive.com, florent@...oy-digital.fr,
rdunlap@...radead.org
Subject: Re: [PATCH v5 3/3] mmc: Add driver for LiteX's LiteSDCard interface
On Wed, 15 Dec 2021 at 14:09, Gabriel Somlo <gsomlo@...il.com> 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@...micro.com>
> Signed-off-by: Kamil Rakoczy <krakoczy@...micro.com>
> Co-developed-by: Maciej Dudek <mdudek@...ernships.antmicro.com>
> Signed-off-by: Maciej Dudek <mdudek@...ernships.antmicro.com>
> Co-developed-by: Paul Mackerras <paulus@...abs.org>
> Signed-off-by: Paul Mackerras <paulus@...abs.org>
> Signed-off-by: Gabriel Somlo <gsomlo@...il.com>
> Cc: Mateusz Holenko <mholenko@...micro.com>
> Cc: Karol Gugala <kgugala@...micro.com>
> Cc: Joel Stanley <joel@....id.au>
> Cc: Stafford Horne <shorne@...il.com>
> Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> Cc: David Abdurachmanov <david.abdurachmanov@...ive.com>
> Cc: Florent Kermarrec <florent@...oy-digital.fr>
> Reviewed-by: Joel Stanley <joel@....id.au>
[...]
> +
> +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@...micro.com>");
> +MODULE_AUTHOR("Kamil Rakoczy <krakoczy@...micro.com>");
> +MODULE_AUTHOR("Maciej Dudek <mdudek@...ernships.antmicro.com>");
> +MODULE_AUTHOR("Paul Mackerras <paulus@...abs.org>");
> +MODULE_AUTHOR("Gabriel Somlo <gsomlo@...il.com>");
> +MODULE_LICENSE("GPL v2");
> --
Other than the comments above, the code looks nice and was easy to
review, thanks!
Kind regards
Uffe
Powered by blists - more mailing lists