Message-ID: <YdOUbYpGFNyxz3iD@errol.ini.cmu.edu>
Date:   Mon, 3 Jan 2022 19:27:25 -0500
From:   "Gabriel L. Somlo" <gsomlo@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
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

Thanks for the feedback! Some replies and follow-up comments below.

On Tue, Dec 28, 2021 at 05:15:25PM +0100, Ulf Hansson wrote:
> 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?

This would only be a concern for SDcard media that do not support
4-bit wide data transfer (if any such models even/still exist!). This
driver simply forces 4-bit wide data mode as soon as the very first
data transfer is "snooped", which is AFAICT legal w.r.t. the spec.

If the card does not support 4-bit wide data transfers, then
litex_mmc_set_bus_width() will fail, and the card will fail to
initialize, but I don't foresee much of anything else going wrong.
 
> > +
> > +       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?

During litex_mmc_probe(), I have:

	...
        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;
	...

This ensures no bus-width switches to anything other than 4-bit data
should ever occur. As far as I understand the SDcard spec, it's legal
to both send multiple redundant bus-width-set commands, and to start
doing so before the very first data transfer request is processed
(regardless of the fact that linux typically does a few 1-bit-wide
data transfers during card initialization before switching to a wider
mode, if available).

This driver simply ensures that any time we ever have a data transfer,
the bus width is set to 4 *before* said transfer is acted upon.

As I mentioned earlier, if we get a "weird" SDcard that can't support
4-bit data transfers, its initialization should fail shortly after
detection, and that's all there is to it, as far as I can tell.

> > +                */
> > +               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.

The LiteSDCard "hardware" (i.e., *gateware*) does not allow modification
or selection of voltage from the software side. When a CMD8 is issued,
the "voltage supplied" bit pattern is expected to be '0001b', which per
the spec means "2.7-3.6V". 

I tried adding this to the overall DTS:

	vreg_mmc: vreg_mmc_3v {
		compatible = "regulator-fixed";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
	};

and then added a reference to it to the LiteSDCard "mmc0" node in DTS,
like so:

	mmc0: mmc@...05000 {
		compatible = "litex,mmc";
		reg = <0x12005000 0x100>,
			<0x12003800 0x100>,
			<0x12003000 0x100>,
			<0x12004800 0x100>,
			<0x12004000 0x100>;
		reg-names = "phy", "core", "reader", "writer", "irq";
		clocks = <&sys_clk>;
		vmmc-supply = <&vreg_mmc>; /* <-------- HERE !!! */
		interrupt-parent = <&L1>;
		interrupts = <4>;
	};

Finally, I replaced the hardcoded setting of `mmc->ocr_avail` with a
call to `mmc_regulator_get_supply(mmc)`. Now, I get a bunch of timeouts
during attempts to send e.g., CMD8 and CMD55.
(going for 3200000 and 3400000 for min- and max-microvolt, respectively,
 -- or anything else in the allowed 2.7-3.6 range -- doesn't help either).

I might be doing something subtly wrong in the way I set things up
above, but it feels a bit overengineered, and IMHO fragile.

OTOH, going all out and setting:

        /* allow for generic 2.7-3.6V range, no software tuning available */
        mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
                         MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
                         MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;

seems to work just fine... :) Please do let me know what you think!
 
> > +       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?

Nope, I cannot -- so I removed MMC_CAP2_FULL_PWR_CYCLE from the list,
should be there in v6 of the series. Thanks for pointing that out!

> > +                     MMC_CAP2_NO_SDIO;
> 
> We should add MMC_CAP2_NO_MMC here as well, as it looks like it can't
> be supported. Right?

Right, and done -- lined up for v6.

> > +
> > +       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!

Thanks again for the review, and please LMK about the voltage regulator
question.

Happy New Year,
--Gabriel

Powered by blists - more mailing lists