| Message ID | 20211215130711.111186-4-gsomlo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: Add LiteSDCard mmc driver | expand |
On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.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! ... > + int ret; > + > + host->irq = platform_get_irq_optional(host->dev, 0); > + if (host->irq <= 0) { > + dev_warn(dev, "Failed to get IRQ, using polling\n"); > + goto use_polling; > + } Same comment as per v3. ... > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT Why under ifdeffery? > + /* increase from default 32 on 64-bit-DMA capable architectures */ > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + if (ret) > + goto err; > +#endif
Hi Andy, Thanks for the feedback! On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.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! > > ... > > > + int ret; > > + > > + host->irq = platform_get_irq_optional(host->dev, 0); > > + if (host->irq <= 0) { > > + dev_warn(dev, "Failed to get IRQ, using polling\n"); > > + goto use_polling; > > + } > > [Same comment as per v3.] > This is wrong. It missed the deferred probe, for example. > > The best approach is > > ret = platform_get_irq_optional(...); > if (ret < 0 && ret != -ENXIO) > return ret; > if (ret > 0) > ...we got it... > > It will allow the future API fix of platform_get_irq_optional() to be > really optional. Thanks for the example. I still need to work in a decision to use polling, though. How about something like this instead: ret = platform_get_irq_optional(...); if (ret == -ENXIO) goto use_polling; if (ret < 0) return ret; // deferred probe (-EAGAIN likely?) if (ret > 0) ...we got it, keep going... > > ... > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > Why under ifdeffery? Because I only want to do it on 64-bit capable architectures. The alternative would be to call dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); on *all* architectures, but ignore the returned error (-EIO, presumably on architetures that only support 32-bit DMA). Do you think that would be cleaner? Thanks, --Gabriel > > + /* increase from default 32 on 64-bit-DMA capable architectures */ > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > + if (ret) > > + goto err; > > +#endif > > With Best Regards, > Andy Shevchenko
On Sun, Dec 26, 2021 at 1:45 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > Hi Andy, > > Thanks for the feedback! > > On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.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! > > > > ... > > > > > + int ret; > > > + > > > + host->irq = platform_get_irq_optional(host->dev, 0); > > > + if (host->irq <= 0) { > > > + dev_warn(dev, "Failed to get IRQ, using polling\n"); > > > + goto use_polling; > > > + } > > > > [Same comment as per v3.] > > > This is wrong. It missed the deferred probe, for example. > > > > The best approach is > > > > ret = platform_get_irq_optional(...); > > if (ret < 0 && ret != -ENXIO) > > return ret; > > if (ret > 0) > > ...we got it... > > > > It will allow the future API fix of platform_get_irq_optional() to be > > really optional. > > Thanks for the example. I still need to work in a decision to use > polling, though. How about something like this instead: > > ret = platform_get_irq_optional(...); > if (ret == -ENXIO) > goto use_polling; > if (ret < 0) > return ret; // deferred probe (-EAGAIN likely?) > if (ret > 0) > ...we got it, keep going... > > > > > ... > > > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > > > Why under ifdeffery? > > Because I only want to do it on 64-bit capable architectures. > > The alternative would be to call > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > on *all* architectures, but ignore the returned error (-EIO, > presumably on architetures that only support 32-bit DMA). > > Do you think that would be cleaner? > > Thanks, > --Gabriel > > > > + /* increase from default 32 on 64-bit-DMA capable architectures */ > > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > > + if (ret) > > > + goto err; > > > +#endif > > > > With Best Regards, > > Andy Shevchenko
On Sun, Dec 26, 2021 at 1:45 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.com> wrote: ... > > This is wrong. It missed the deferred probe, for example. > > > > The best approach is > > > > ret = platform_get_irq_optional(...); > > if (ret < 0 && ret != -ENXIO) > > return ret; > > if (ret > 0) > > ...we got it... > > > > It will allow the future API fix of platform_get_irq_optional() to be > > really optional. > > Thanks for the example. I still need to work in a decision to use > polling, though. How about something like this instead: > > ret = platform_get_irq_optional(...); > if (ret == -ENXIO) > goto use_polling; > if (ret < 0) > return ret; // deferred probe (-EAGAIN likely?) > if (ret > 0) > ...we got it, keep going... This doesn't define what you should do when you get 0. I suggest to take my variant with below modification if (ret > 0) ...we have IRQ... else goto USE POLLING; It will take care of the case. ... > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > > > Why under ifdeffery? > > Because I only want to do it on 64-bit capable architectures. > > The alternative would be to call > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > on *all* architectures, but ignore the returned error (-EIO, > presumably on architetures that only support 32-bit DMA). I don't understand why you are supposed to ignore errors and why you expect to get such. > Do you think that would be cleaner?
On Sun, Dec 26, 2021 at 03:13:21PM +0200, Andy Shevchenko wrote: > On Sun, Dec 26, 2021 at 1:45 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > > > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.com> wrote: > > ... > > > > This is wrong. It missed the deferred probe, for example. > > > > > > The best approach is > > > > > > ret = platform_get_irq_optional(...); > > > if (ret < 0 && ret != -ENXIO) > > > return ret; > > > if (ret > 0) > > > ...we got it... > > > > > > It will allow the future API fix of platform_get_irq_optional() to be > > > really optional. > > > > Thanks for the example. I still need to work in a decision to use > > polling, though. How about something like this instead: > > > > ret = platform_get_irq_optional(...); > > if (ret == -ENXIO) > > goto use_polling; > > if (ret < 0) > > return ret; // deferred probe (-EAGAIN likely?) > > if (ret > 0) > > ...we got it, keep going... > > This doesn't define what you should do when you get 0. > I suggest to take my variant with below modification > > if (ret > 0) > ...we have IRQ... > else > goto USE POLLING; > > It will take care of the case. OK, will do. > ... > > > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > > > > > Why under ifdeffery? > > > > Because I only want to do it on 64-bit capable architectures. > > > > The alternative would be to call > > > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > > > on *all* architectures, but ignore the returned error (-EIO, > > presumably on architetures that only support 32-bit DMA). > > I don't understand why you are supposed to ignore errors and why you > expect to get such. If I call `dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));` on a machine where `CONFIG_ARCH_DMA_ADDR_T_64BIT` is *not* set, I expect an error. The implicit default (per Documentation/core-api/dma-api-howto.rst), is DMA_BIT_MASK(32). I'm working under the impression that on machines with CONFIG_ARCH_DMA_ADDR_T_64BIT I should increase that to DMA_BIT_MASK(64). So if I don't #ifdef it, that call will fail on machines supporting only 32-bits. What am I missing? Thanks, --Gabriel > > Do you think that would be cleaner? > > -- > With Best Regards, > Andy Shevchenko
On Sun, Dec 26, 2021 at 3:36 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > On Sun, Dec 26, 2021 at 03:13:21PM +0200, Andy Shevchenko wrote: > > On Sun, Dec 26, 2021 at 1:45 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > > > > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.com> wrote: ... > > > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > > > > > > > Why under ifdeffery? > > > > > > Because I only want to do it on 64-bit capable architectures. > > > > > > The alternative would be to call > > > > > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > > > > > on *all* architectures, but ignore the returned error (-EIO, > > > presumably on architetures that only support 32-bit DMA). > > > > I don't understand why you are supposed to ignore errors and why you > > expect to get such. > > If I call `dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));` > on a machine where `CONFIG_ARCH_DMA_ADDR_T_64BIT` is *not* set, I > expect an error. The implicit default > (per Documentation/core-api/dma-api-howto.rst), is DMA_BIT_MASK(32). > I'm working under the impression that on machines with > CONFIG_ARCH_DMA_ADDR_T_64BIT I should increase that to DMA_BIT_MASK(64). > > So if I don't #ifdef it, that call will fail on machines supporting > only 32-bits. > > What am I missing? This thread: https://lkml.org/lkml/2021/6/7/398 ?
On Sun, Dec 26, 2021 at 04:01:03PM +0200, Andy Shevchenko wrote: > On Sun, Dec 26, 2021 at 3:36 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > On Sun, Dec 26, 2021 at 03:13:21PM +0200, Andy Shevchenko wrote: > > > On Sun, Dec 26, 2021 at 1:45 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > > On Sat, Dec 25, 2021 at 06:43:22PM +0200, Andy Shevchenko wrote: > > > > > On Wed, Dec 15, 2021 at 10:00 PM Gabriel Somlo <gsomlo@gmail.com> wrote: > > ... > > > > > > > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT > > > > > > > > > > Why under ifdeffery? > > > > > > > > Because I only want to do it on 64-bit capable architectures. > > > > > > > > The alternative would be to call > > > > > > > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > > > > > > > on *all* architectures, but ignore the returned error (-EIO, > > > > presumably on architetures that only support 32-bit DMA). > > > > > > I don't understand why you are supposed to ignore errors and why you > > > expect to get such. > > > > If I call `dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));` > > on a machine where `CONFIG_ARCH_DMA_ADDR_T_64BIT` is *not* set, I > > expect an error. The implicit default > > (per Documentation/core-api/dma-api-howto.rst), is DMA_BIT_MASK(32). > > I'm working under the impression that on machines with > > CONFIG_ARCH_DMA_ADDR_T_64BIT I should increase that to DMA_BIT_MASK(64). > > > > So if I don't #ifdef it, that call will fail on machines supporting > > only 32-bits. > > > > What am I missing? > > This thread: https://lkml.org/lkml/2021/6/7/398 ? OK, so just call `dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));` regardless of 32- or 64-bit dma capability, *do* check the return value, and it should *not* fail on 32-bit systems. I'll do that in v6 (should go out in early January '22, since I'm traveling with only occasional email access at the moment). Thanks, --Gabriel
On Wed, 15 Dec 2021 at 14:09, Gabriel Somlo <gsomlo@gmail.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@antmicro.com> > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com> > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com> > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com> > Co-developed-by: Paul Mackerras <paulus@ozlabs.org> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > Cc: Mateusz Holenko <mholenko@antmicro.com> > Cc: Karol Gugala <kgugala@antmicro.com> > Cc: Joel Stanley <joel@jms.id.au> > Cc: Stafford Horne <shorne@gmail.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: David Abdurachmanov <david.abdurachmanov@sifive.com> > Cc: Florent Kermarrec <florent@enjoy-digital.fr> > Reviewed-by: Joel Stanley <joel@jms.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@antmicro.com>"); > +MODULE_AUTHOR("Kamil Rakoczy <krakoczy@antmicro.com>"); > +MODULE_AUTHOR("Maciej Dudek <mdudek@internships.antmicro.com>"); > +MODULE_AUTHOR("Paul Mackerras <paulus@ozlabs.org>"); > +MODULE_AUTHOR("Gabriel Somlo <gsomlo@gmail.com>"); > +MODULE_LICENSE("GPL v2"); > -- Other than the comments above, the code looks nice and was easy to review, thanks! Kind regards Uffe
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@gmail.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@antmicro.com> > > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com> > > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com> > > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com> > > Co-developed-by: Paul Mackerras <paulus@ozlabs.org> > > Signed-off-by: Paul Mackerras <paulus@ozlabs.org> > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > Cc: Mateusz Holenko <mholenko@antmicro.com> > > Cc: Karol Gugala <kgugala@antmicro.com> > > Cc: Joel Stanley <joel@jms.id.au> > > Cc: Stafford Horne <shorne@gmail.com> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: David Abdurachmanov <david.abdurachmanov@sifive.com> > > Cc: Florent Kermarrec <florent@enjoy-digital.fr> > > Reviewed-by: Joel Stanley <joel@jms.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@12005000 { 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@antmicro.com>"); > > +MODULE_AUTHOR("Kamil Rakoczy <krakoczy@antmicro.com>"); > > +MODULE_AUTHOR("Maciej Dudek <mdudek@internships.antmicro.com>"); > > +MODULE_AUTHOR("Paul Mackerras <paulus@ozlabs.org>"); > > +MODULE_AUTHOR("Gabriel Somlo <gsomlo@gmail.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
On Mon, Jan 03, 2022 at 07:27:28PM -0500, Gabriel L. Somlo wrote: > On Tue, Dec 28, 2021 at 05:15:25PM +0100, Ulf Hansson wrote: > > 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@12005000 { > 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! I dug around `drivers/mmc/core/regulator.c` a bit more, and it turns out `mmc_regulator_get_supply()` is allowed to return 0 even if not all regulators have been found, "because they all are optional", and I still need to write additional code to check if my regulator got populated -- I assume that means checking if `mmc->ocr_avail` was set to something useful, or whether it's still 0. In my case, with the above-mentioned modifications in DTS, I still end up with `mmc->ocr_avail == 0` after calling `mmc_regulator_get_supply()`, which explains why the card doesnoesn't work correctly after being probed. Not quite sure what to do in that situation -- any ideas? I still think it's a bit overkill to set up a dummy regulator in DTS and probe for it when the "hardware" doesn't actually support variable/configurable voltages or dynamic changes in voltage -- a hard-coded constant somehow feels more appropriate, wouldn't you agree? IMHO, it makes more sense to define the entire generic/standard range described in the SDCard specification (2.7-3.6V) as a constant, e.g.: #define LITEX_MMC_OCR (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) and then initialize `mmc->ocr_avail = LITEX_MMC_OCR` to that instead. This is how they do it in drivers/mmc/host/au1xmmc.c, for instance. I'm happy to learn more about why going the DTS-dummy-regulator configurable route is better, so let me know what you think. I'm going to send out v6 with the hard-coded constant version above soon, unless I hear back from you before then. But we can always go another round (i.e., v7) unless you agree with my argument -- please let me know either way! :) Thanks again, --Gabriel
[...] > > > + > > > +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. Alright, I get the point. I guess it should work. I will have another closer look at the corresponding code from your last submitted version. > > > > + */ > > > + 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". If you provide a range (2.7-3.6V), that means that your hardware supports the entire range, not just one single part of it. > > 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@12005000 { > 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. At a quick glance, the above looks correct to me. Maybe there is something wrong with the code in the driver instead? > > 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! No, this isn't the way we want it to work. That's because it means that we would lie to the card about what voltage range the HW actually supports. It's better to let the DTS file give that information about the HW. [...] Kind regards Uffe
Hi Uffe, On Tue, Jan 11, 2022 at 04:47:07PM +0100, Ulf Hansson wrote: > [...] > > > > > + > > > > +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. > > Alright, I get the point. I guess it should work. I will have another > closer look at the corresponding code from your last submitted > version. Thanks -- it's now on v12 :) > > > > > > + */ > > > > + 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". > > If you provide a range (2.7-3.6V), that means that your hardware > supports the entire range, not just one single part of it. The "gateware" (open source migen/verilog at https://github.com/enjoy-digital/litesdcard) supports any value provided by the underlying FPGA dev board (typically 3.3v) -- by not attempting to manage it in any way. SD media presumably doesn't care as long as voltage is somewhere within 2.7-3.6V (at least that's how I read the spec, there's only one register value representing anything within that range). > > > > 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@12005000 { > > 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. > > At a quick glance, the above looks correct to me. Maybe there is > something wrong with the code in the driver instead? After some more hacking, I learned that: - an additional `regulator-name` line (e.g. `regulator-name = "vreg_mmc";`) is required - setting `regulator-always-on;` seems to help reduce attempts by the kernel to "manage" the regulator, but does not appear to be required In other words: ... vreg_mmc: vreg_mmc { compatible = "regulator-fixed"; regulator-name = "vreg_mmc"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-always-on; }; ... Additionally, CONFIG_REGULATOR=y and CONFIG_REGULATOR_FIXED_VOLTAGE=y *MUST* be enabled in the kernel's .config file, to prevent either litex_mmc_probe() from being deferred, or mmc_regulator_get_supply() from simply returning 0 without having set mmc->ocr_avail to anything at all! Presumably this would also mean either `select REGULATOR_FIXED_VOLTAGE` or `depends on REGULATOR_FIXED_VOLTAGE` in the mmc driver's Kconfig entry. Predictably, the "regulator-[min|max]-microvolt = <3300000>" setting gets us ocr_avail == MMC_VDD_32_33 | MMC_VDD_33_34 > > > > 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! > > No, this isn't the way we want it to work. That's because it means > that we would lie to the card about what voltage range the HW actually > supports. > > It's better to let the DTS file give that information about the HW. I may be needlessly concerned, but it feels a bit weird to me to drag in CONFIG_REGULATOR_FIXED_VOLTAGE as an added dependency for what is ultimately a roundabout way of setting a constant... :) Thanks in advance for any additional clue! Best, --Gabriel
[...] > > > > [...] > > > > > > > > > + > > > > > + 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". > > > > If you provide a range (2.7-3.6V), that means that your hardware > > supports the entire range, not just one single part of it. > > The "gateware" (open source migen/verilog at > https://github.com/enjoy-digital/litesdcard) > supports any value provided by the underlying FPGA dev board > (typically 3.3v) -- by not attempting to manage it in any way. > > SD media presumably doesn't care as long as voltage is somewhere > within 2.7-3.6V (at least that's how I read the spec, there's only > one register value representing anything within that range). > > > > > > > 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@12005000 { > > > 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. > > > > At a quick glance, the above looks correct to me. Maybe there is > > something wrong with the code in the driver instead? > > After some more hacking, I learned that: > > - an additional `regulator-name` line > (e.g. `regulator-name = "vreg_mmc";`) is required > > - setting `regulator-always-on;` seems to help reduce attempts > by the kernel to "manage" the regulator, but does not appear > to be required > > In other words: > > ... > vreg_mmc: vreg_mmc { > compatible = "regulator-fixed"; > regulator-name = "vreg_mmc"; > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > regulator-always-on; > }; > ... > > Additionally, CONFIG_REGULATOR=y and CONFIG_REGULATOR_FIXED_VOLTAGE=y > *MUST* be enabled in the kernel's .config file, to prevent either > litex_mmc_probe() from being deferred, or mmc_regulator_get_supply() > from simply returning 0 without having set mmc->ocr_avail to anything > at all! > > Presumably this would also mean either `select REGULATOR_FIXED_VOLTAGE` > or `depends on REGULATOR_FIXED_VOLTAGE` in the mmc driver's Kconfig > entry. Yep, that's correct. If you don't like to manage that dependency in the Kconfig, an option is to check if mmc->ocr_avail is zero and if so, we could log a message *and* assign mmc->ocr_avail a default value. > > Predictably, the "regulator-[min|max]-microvolt = <3300000>" setting > gets us > > ocr_avail == MMC_VDD_32_33 | MMC_VDD_33_34 > > > > > > > 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! > > > > No, this isn't the way we want it to work. That's because it means > > that we would lie to the card about what voltage range the HW actually > > supports. > > > > It's better to let the DTS file give that information about the HW. > > I may be needlessly concerned, but it feels a bit weird to me to drag > in CONFIG_REGULATOR_FIXED_VOLTAGE as an added dependency for what is > ultimately a roundabout way of setting a constant... :) The point is, it shouldn't really be a constant set by the driver, because it would mean initialising a card under potentially wrong conditions. However, I am fine assigning it a default value as a fallback and best effort, if it turns out that DT didn't provide us information about what the HW is capable of. > > Thanks in advance for any additional clue! Looks like there are two options, just pick one of them, then I am happy. :-) Kind regards Uffe
On Wed, Jan 12, 2022 at 11:24:34AM +0100, Ulf Hansson wrote: > [...] > > > > > > [...] > > > > > > > > > > > + > > > > > > + 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". > > > > > > If you provide a range (2.7-3.6V), that means that your hardware > > > supports the entire range, not just one single part of it. > > > > The "gateware" (open source migen/verilog at > > https://github.com/enjoy-digital/litesdcard) > > supports any value provided by the underlying FPGA dev board > > (typically 3.3v) -- by not attempting to manage it in any way. > > > > SD media presumably doesn't care as long as voltage is somewhere > > within 2.7-3.6V (at least that's how I read the spec, there's only > > one register value representing anything within that range). > > > > > > > > > > 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@12005000 { > > > > 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. > > > > > > At a quick glance, the above looks correct to me. Maybe there is > > > something wrong with the code in the driver instead? > > > > After some more hacking, I learned that: > > > > - an additional `regulator-name` line > > (e.g. `regulator-name = "vreg_mmc";`) is required > > > > - setting `regulator-always-on;` seems to help reduce attempts > > by the kernel to "manage" the regulator, but does not appear > > to be required > > > > In other words: > > > > ... > > vreg_mmc: vreg_mmc { > > compatible = "regulator-fixed"; > > regulator-name = "vreg_mmc"; > > regulator-min-microvolt = <3300000>; > > regulator-max-microvolt = <3300000>; > > regulator-always-on; > > }; > > ... > > > > Additionally, CONFIG_REGULATOR=y and CONFIG_REGULATOR_FIXED_VOLTAGE=y > > *MUST* be enabled in the kernel's .config file, to prevent either > > litex_mmc_probe() from being deferred, or mmc_regulator_get_supply() > > from simply returning 0 without having set mmc->ocr_avail to anything > > at all! > > > > Presumably this would also mean either `select REGULATOR_FIXED_VOLTAGE` > > or `depends on REGULATOR_FIXED_VOLTAGE` in the mmc driver's Kconfig > > entry. > > Yep, that's correct. > > If you don't like to manage that dependency in the Kconfig, an option > is to check if mmc->ocr_avail is zero and if so, we could log a > message *and* assign mmc->ocr_avail a default value. > > > > > Predictably, the "regulator-[min|max]-microvolt = <3300000>" setting > > gets us > > > > ocr_avail == MMC_VDD_32_33 | MMC_VDD_33_34 > > > > > > > > > > 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! > > > > > > No, this isn't the way we want it to work. That's because it means > > > that we would lie to the card about what voltage range the HW actually > > > supports. > > > > > > It's better to let the DTS file give that information about the HW. > > > > I may be needlessly concerned, but it feels a bit weird to me to drag > > in CONFIG_REGULATOR_FIXED_VOLTAGE as an added dependency for what is > > ultimately a roundabout way of setting a constant... :) > > The point is, it shouldn't really be a constant set by the driver, > because it would mean initialising a card under potentially wrong > conditions. > > However, I am fine assigning it a default value as a fallback and best > effort, if it turns out that DT didn't provide us information about > what the HW is capable of. > > > > > Thanks in advance for any additional clue! > > Looks like there are two options, just pick one of them, then I am happy. :-) Sounds like a plan! I'll send out lucky v13 once I've had a chance to test it on my FPGA, later this evening. Thanks again, --Gabriel
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 5af8494c31b5..c1b66d06d1c9 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -1093,3 +1093,12 @@ config MMC_OWL config MMC_SDHCI_EXTERNAL_DMA bool + +config MMC_LITEX + tristate "LiteX MMC Host Controller support" + depends on OF + depends on PPC_MICROWATT || LITEX || COMPILE_TEST + help + This selects support for the MMC Host Controller found in LiteX SoCs. + + If unsure, say N. diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index ea36d379bd3c..4e4ceb32c4b4 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -101,6 +101,7 @@ obj-$(CONFIG_MMC_CQHCI) += cqhci.o cqhci-y += cqhci-core.o cqhci-$(CONFIG_MMC_CRYPTO) += cqhci-crypto.o obj-$(CONFIG_MMC_HSQ) += mmc_hsq.o +obj-$(CONFIG_MMC_LITEX) += litex_mmc.o ifeq ($(CONFIG_CB710_DEBUG),y) CFLAGS-cb710-mmc += -DDEBUG diff --git a/drivers/mmc/host/litex_mmc.c b/drivers/mmc/host/litex_mmc.c new file mode 100644 index 000000000000..fbc75367cd54 --- /dev/null +++ b/drivers/mmc/host/litex_mmc.c @@ -0,0 +1,652 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LiteX LiteSDCard driver + * + * Copyright (C) 2019-2020 Antmicro <contact@antmicro.com> + * Copyright (C) 2019-2020 Kamil Rakoczy <krakoczy@antmicro.com> + * Copyright (C) 2019-2020 Maciej Dudek <mdudek@internships.antmicro.com> + * Copyright (C) 2020 Paul Mackerras <paulus@ozlabs.org> + * Copyright (C) 2020-2021 Gabriel Somlo <gsomlo@gmail.com> + * + */ + +#include <linux/module.h> +#include <linux/litex.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/iopoll.h> +#include <linux/mmc/sd.h> +#include <linux/mmc/mmc.h> +#include <linux/mmc/host.h> +#include <linux/delay.h> +#include <linux/dma-mapping.h> + +#define LITEX_PHY_CARDDETECT 0x00 +#define LITEX_PHY_CLOCKERDIV 0x04 +#define LITEX_PHY_INITIALIZE 0x08 +#define LITEX_PHY_WRITESTATUS 0x0C +#define LITEX_CORE_CMDARG 0x00 +#define LITEX_CORE_CMDCMD 0x04 +#define LITEX_CORE_CMDSND 0x08 +#define LITEX_CORE_CMDRSP 0x0C +#define LITEX_CORE_CMDEVT 0x1C +#define LITEX_CORE_DATAEVT 0x20 +#define LITEX_CORE_BLKLEN 0x24 +#define LITEX_CORE_BLKCNT 0x28 +#define LITEX_BLK2MEM_BASE 0x00 +#define LITEX_BLK2MEM_LEN 0x08 +#define LITEX_BLK2MEM_ENA 0x0C +#define LITEX_BLK2MEM_DONE 0x10 +#define LITEX_BLK2MEM_LOOP 0x14 +#define LITEX_MEM2BLK_BASE 0x00 +#define LITEX_MEM2BLK_LEN 0x08 +#define LITEX_MEM2BLK_ENA 0x0C +#define LITEX_MEM2BLK_DONE 0x10 +#define LITEX_MEM2BLK_LOOP 0x14 +#define LITEX_MEM2BLK 0x18 +#define LITEX_IRQ_STATUS 0x00 +#define LITEX_IRQ_PENDING 0x04 +#define LITEX_IRQ_ENABLE 0x08 + +#define SD_CTL_DATA_XFER_NONE 0 +#define SD_CTL_DATA_XFER_READ 1 +#define SD_CTL_DATA_XFER_WRITE 2 + +#define SD_CTL_RESP_NONE 0 +#define SD_CTL_RESP_SHORT 1 +#define SD_CTL_RESP_LONG 2 +#define SD_CTL_RESP_SHORT_BUSY 3 + +#define SD_BIT_DONE BIT(0) +#define SD_BIT_WR_ERR BIT(1) +#define SD_BIT_TIMEOUT BIT(2) +#define SD_BIT_CRC_ERR BIT(3) + +#define SD_SLEEP_US 5 +#define SD_TIMEOUT_US 20000 + +#define SDIRQ_CARD_DETECT 1 +#define SDIRQ_SD_TO_MEM_DONE 2 +#define SDIRQ_MEM_TO_SD_DONE 4 +#define SDIRQ_CMD_DONE 8 + +struct litex_mmc_host { + struct mmc_host *mmc; + struct platform_device *dev; + + void __iomem *sdphy; + void __iomem *sdcore; + void __iomem *sdreader; + void __iomem *sdwriter; + void __iomem *sdirq; + + void *buffer; + size_t buf_size; + dma_addr_t dma; + + struct completion cmd_done; + int irq; + + unsigned int ref_clk; + unsigned int sd_clk; + + u32 resp[4]; + u16 rca; + + bool is_bus_width_set; + bool app_cmd; +}; + +static int litex_mmc_sdcard_wait_done(void __iomem *reg) +{ + u8 evt; + int ret; + + ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE), + SD_SLEEP_US, SD_TIMEOUT_US); + if (ret || (evt & SD_BIT_TIMEOUT)) + return -ETIMEDOUT; + if (evt == SD_BIT_DONE) + return 0; + if (evt & SD_BIT_WR_ERR) + return -EIO; + if (evt & SD_BIT_CRC_ERR) + return -EILSEQ; + pr_err("%s: unknown error evt=%x\n", __func__, evt); + return -EINVAL; +} + +static int litex_mmc_send_cmd(struct litex_mmc_host *host, + u8 cmd, u32 arg, u8 response_len, u8 transfer) +{ + struct device *dev = mmc_dev(host->mmc); + void __iomem *reg; + int ret; + u8 evt; + + litex_write32(host->sdcore + LITEX_CORE_CMDARG, arg); + litex_write32(host->sdcore + LITEX_CORE_CMDCMD, + cmd << 8 | transfer << 5 | response_len); + litex_write8(host->sdcore + LITEX_CORE_CMDSND, 1); + + /* Wait for an interrupt if we have an interrupt and either there is + * data to be transferred, or if the card can report busy via DAT0. + */ + if (host->irq > 0 && + (transfer != SD_CTL_DATA_XFER_NONE || + response_len == SD_CTL_RESP_SHORT_BUSY)) { + reinit_completion(&host->cmd_done); + litex_write32(host->sdirq + LITEX_IRQ_ENABLE, + SDIRQ_CMD_DONE | SDIRQ_CARD_DETECT); + wait_for_completion(&host->cmd_done); + } + + ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT); + if (ret) { + dev_err(dev, "Command (cmd %d) error, status %d\n", cmd, ret); + return ret; + } + + if (response_len != SD_CTL_RESP_NONE) { + int i; + + reg = host->sdcore + LITEX_CORE_CMDRSP; + for (i = 0; i < 4; i++) { + host->resp[i] = litex_read32(reg); + reg += sizeof(u32); + } + } + + if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR) + host->rca = (host->resp[3] >> 16) & 0xffff; + + host->app_cmd = (cmd == MMC_APP_CMD); + + if (transfer == SD_CTL_DATA_XFER_NONE) + return ret; /* OK from prior litex_mmc_sdcard_wait_done() */ + + ret = litex_mmc_sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT); + if (ret) { + dev_err(dev, "Data xfer (cmd %d) error, status %d\n", cmd, ret); + return ret; + } + + /* wait for completion of (read or write) DMA transfer */ + reg = (transfer == SD_CTL_DATA_XFER_READ) ? + host->sdreader + LITEX_BLK2MEM_DONE : + host->sdwriter + LITEX_MEM2BLK_DONE; + ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE), + SD_SLEEP_US, SD_TIMEOUT_US); + if (ret) + dev_err(dev, "DMA timeout (cmd %d)\n", cmd); + + return ret; +} + +static int litex_mmc_send_app_cmd(struct litex_mmc_host *host) +{ + return litex_mmc_send_cmd(host, MMC_APP_CMD, host->rca << 16, + SD_CTL_RESP_SHORT, SD_CTL_DATA_XFER_NONE); +} + +static int litex_mmc_send_set_bus_w_cmd(struct litex_mmc_host *host, u32 width) +{ + return litex_mmc_send_cmd(host, SD_APP_SET_BUS_WIDTH, width, + SD_CTL_RESP_SHORT, SD_CTL_DATA_XFER_NONE); +} + +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; + } + + host->is_bus_width_set = true; + + return 0; +} + +static int litex_mmc_get_cd(struct mmc_host *mmc) +{ + struct litex_mmc_host *host = mmc_priv(mmc); + int ret; + + if (!mmc_card_is_removable(mmc)) + return 1; + + ret = !litex_read8(host->sdphy + LITEX_PHY_CARDDETECT); + + /* ensure bus width will be set (again) upon card (re)insertion */ + if (ret == 0) + host->is_bus_width_set = false; + + return ret; +} + +static irqreturn_t litex_mmc_interrupt(int irq, void *arg) +{ + struct mmc_host *mmc = arg; + struct litex_mmc_host *host = mmc_priv(mmc); + u32 pending = litex_read32(host->sdirq + LITEX_IRQ_PENDING); + + /* Check for card change interrupt */ + if (pending & SDIRQ_CARD_DETECT) { + litex_write32(host->sdirq + LITEX_IRQ_PENDING, + SDIRQ_CARD_DETECT); + mmc_detect_change(mmc, msecs_to_jiffies(10)); + } + + /* Check for command completed */ + if (pending & SDIRQ_CMD_DONE) { + /* Disable it so it doesn't keep interrupting */ + litex_write32(host->sdirq + LITEX_IRQ_ENABLE, + SDIRQ_CARD_DETECT); + complete(&host->cmd_done); + } + + return IRQ_HANDLED; +} + +static u32 litex_mmc_response_len(struct mmc_command *cmd) +{ + if (cmd->flags & MMC_RSP_136) + return SD_CTL_RESP_LONG; + if (!(cmd->flags & MMC_RSP_PRESENT)) + return SD_CTL_RESP_NONE; + if (cmd->flags & MMC_RSP_BUSY) + return SD_CTL_RESP_SHORT_BUSY; + return SD_CTL_RESP_SHORT; +} + +static void litex_mmc_do_dma(struct litex_mmc_host *host, struct mmc_data *data, + unsigned int *len, bool *direct, u8 *transfer) +{ + struct device *dev = mmc_dev(host->mmc); + dma_addr_t dma; + int sg_count; + + /* Try to DMA directly to/from the data buffer. + * We can do that if the buffer can be mapped for DMA + * in one contiguous chunk. + */ + dma = host->dma; + *len = data->blksz * data->blocks; + sg_count = dma_map_sg(dev, data->sg, data->sg_len, + mmc_get_dma_dir(data)); + if (sg_count == 1) { + dma = sg_dma_address(data->sg); + *len = sg_dma_len(data->sg); + *direct = true; + } else if (*len > host->buf_size) + *len = host->buf_size; + + if (data->flags & MMC_DATA_READ) { + litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0); + litex_write64(host->sdreader + LITEX_BLK2MEM_BASE, dma); + litex_write32(host->sdreader + LITEX_BLK2MEM_LEN, *len); + litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 1); + *transfer = SD_CTL_DATA_XFER_READ; + } else if (data->flags & MMC_DATA_WRITE) { + if (!*direct) + sg_copy_to_buffer(data->sg, data->sg_len, + host->buffer, *len); + litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0); + litex_write64(host->sdwriter + LITEX_MEM2BLK_BASE, dma); + litex_write32(host->sdwriter + LITEX_MEM2BLK_LEN, *len); + litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 1); + *transfer = SD_CTL_DATA_XFER_WRITE; + } else { + dev_warn(dev, "Data present w/o read or write flag.\n"); + /* Continue: set cmd status, mark req done */ + } + + litex_write16(host->sdcore + LITEX_CORE_BLKLEN, data->blksz); + litex_write32(host->sdcore + LITEX_CORE_BLKCNT, data->blocks); +} + +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! + */ + 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); +} + +static void litex_mmc_setclk(struct litex_mmc_host *host, unsigned int freq) +{ + struct device *dev = mmc_dev(host->mmc); + u32 div; + + div = freq ? host->ref_clk / freq : 256U; + div = roundup_pow_of_two(div); + div = min(max(div, 2U), 256U); + dev_dbg(dev, "sd_clk_freq=%d: set to %d via div=%d\n", + freq, host->ref_clk / div, div); + litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div); + host->sd_clk = freq; +} + +static void litex_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) +{ + struct litex_mmc_host *host = mmc_priv(mmc); + + /* NOTE: Ignore any ios->bus_width updates; they occur right after + * the mmc core sends its own acmd6 bus-width change notification, + * which is redundant since we snoop on the command flow and inject + * an early acmd6 before the first data transfer command is sent! + */ + + /* update sd_clk */ + if (ios->clock != host->sd_clk) + litex_mmc_setclk(host, ios->clock); +} + +static const struct mmc_host_ops litex_mmc_ops = { + .get_cd = litex_mmc_get_cd, + .request = litex_mmc_request, + .set_ios = litex_mmc_set_ios, +}; + +static int litex_mmc_irq_init(struct litex_mmc_host *host) +{ + struct device *dev = mmc_dev(host->mmc); + int ret; + + host->irq = platform_get_irq_optional(host->dev, 0); + if (host->irq <= 0) { + dev_warn(dev, "Failed to get IRQ, using polling\n"); + goto use_polling; + } + + host->sdirq = devm_platform_ioremap_resource_byname(host->dev, "irq"); + if (IS_ERR(host->sdirq)) + return PTR_ERR(host->sdirq); + + ret = request_irq(host->irq, litex_mmc_interrupt, 0, + "litex-mmc", host->mmc); + if (ret < 0) { + dev_warn(dev, "IRQ request error %d, using polling\n", ret); + goto use_polling; + } + + /* clear & enable card-change interrupts */ + litex_write32(host->sdirq + LITEX_IRQ_PENDING, SDIRQ_CARD_DETECT); + litex_write32(host->sdirq + LITEX_IRQ_ENABLE, SDIRQ_CARD_DETECT); + + return 0; + +use_polling: + host->mmc->caps |= MMC_CAP_NEEDS_POLL; + host->irq = 0; + return 0; +} + +static int litex_mmc_probe(struct platform_device *pdev) +{ + struct litex_mmc_host *host; + struct mmc_host *mmc; + struct clk *clk; + int ret; + + mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev); + /* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512, + * and max_blk_count accordingly set to 8; + * If for some reason we need to modify max_blk_count, we must also + * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;` + */ + if (!mmc) + return -ENOMEM; + + host = mmc_priv(mmc); + host->mmc = mmc; + host->dev = pdev; + + /* initialize clock source */ + clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) { + ret = dev_err_probe(&pdev->dev, + PTR_ERR(clk), "can't get clock\n"); + goto err; + } + host->ref_clk = clk_get_rate(clk); + host->sd_clk = 0; + + /* 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! + */ + host->is_bus_width_set = false; + host->app_cmd = false; + +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT + /* increase from default 32 on 64-bit-DMA capable architectures */ + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + if (ret) + goto err; +#endif + + host->buf_size = mmc->max_req_size * 2; + host->buffer = dmam_alloc_coherent(&pdev->dev, host->buf_size, + &host->dma, GFP_DMA); + if (host->buffer == NULL) { + ret = -ENOMEM; + goto err; + } + + host->sdphy = devm_platform_ioremap_resource_byname(pdev, "phy"); + if (IS_ERR(host->sdphy)) { + ret = PTR_ERR(host->sdphy); + goto err; + } + + host->sdcore = devm_platform_ioremap_resource_byname(pdev, "core"); + if (IS_ERR(host->sdcore)) { + ret = PTR_ERR(host->sdcore); + goto err; + } + + host->sdreader = devm_platform_ioremap_resource_byname(pdev, "reader"); + if (IS_ERR(host->sdreader)) { + ret = PTR_ERR(host->sdreader); + goto err; + } + + host->sdwriter = devm_platform_ioremap_resource_byname(pdev, "writer"); + if (IS_ERR(host->sdwriter)) { + ret = PTR_ERR(host->sdwriter); + goto err; + } + + /* ensure DMA bus masters are disabled */ + litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0); + litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0); + + init_completion(&host->cmd_done); + ret = litex_mmc_irq_init(host); + if (ret) + goto err; + + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; + 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 | + MMC_CAP2_NO_SDIO; + + 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@antmicro.com>"); +MODULE_AUTHOR("Kamil Rakoczy <krakoczy@antmicro.com>"); +MODULE_AUTHOR("Maciej Dudek <mdudek@internships.antmicro.com>"); +MODULE_AUTHOR("Paul Mackerras <paulus@ozlabs.org>"); +MODULE_AUTHOR("Gabriel Somlo <gsomlo@gmail.com>"); +MODULE_LICENSE("GPL v2");