Re: [PATCH v1 3/3] mmc: Add driver for LiteX's LiteSDCard interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Joel,

Thanks for the review. Couple of thoughts and follow-up questions
inline:

On Mon, Dec 06, 2021 at 10:53:17AM +0000, Joel Stanley wrote:
>  On Fri, 3 Dec 2021 at 23:42, Gabriel Somlo <gsomlo@xxxxxxxxx> wrote:
> >
> > LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
> > that targets FPGAs. LiteSDCard is a small footprint, configurable
> > SDCard core commonly used in LiteX designs.
> >
> > The driver was first written in May 2020 and has been maintained
> > cooperatively by the LiteX community. Thanks to all contributors!
> >
> > Co-developed-by: Kamil Rakoczy <krakoczy@xxxxxxxxxxxx>
> > Signed-off-by: Kamil Rakoczy <krakoczy@xxxxxxxxxxxx>
> > Co-developed-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx>
> > Co-developed-by: Paul Mackerras <paulus@xxxxxxxxxx>
> > Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx>
> > Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx>
> > Cc: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > Cc: Karol Gugala <kgugala@xxxxxxxxxxxx>
> > Cc: Joel Stanley <joel@xxxxxxxxx>
> > Cc: Stafford Horne <shorne@xxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Cc: David Abdurachmanov <david.abdurachmanov@xxxxxxxxxx>
> > Cc: Florent Kermarrec <florent@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/mmc/host/Kconfig     |   6 +
> >  drivers/mmc/host/Makefile    |   1 +
> >  drivers/mmc/host/litex_mmc.c | 677 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 684 insertions(+)
> >  create mode 100644 drivers/mmc/host/litex_mmc.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 5af8494c31b5..84c64e72195d 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -1093,3 +1093,9 @@ config MMC_OWL
> >
> >  config MMC_SDHCI_EXTERNAL_DMA
> >         bool
> > +
> > +config MMC_LITEX
> > +       tristate "Support for the MMC Controller in LiteX SOCs"
> 
> Did you test using this as a module?
> 
> > +       depends on OF && LITEX
> 
> I don't like having litex drivers depend on the LITEX kconfig. The
> symbol is not user visible, and to enable it we need to build in the
> litex controller driver, which platforms may or may not have.
> 
> The microwatt platform is an example of a SoC that embeds some LITEX
> IP, but may or may not be a litex SoC.

I can remove dependency on "LITEX" and, with that, succesfully build
the driver as a module -- same principle as the LiteETH driver. 
I'm queueing that up for the long promised v3, soon as I clear up a
few remaining questions... :)

Right now I have:

	depends on OF || COMPILE_TEST

> > +       help
> > +         Generic MCC driver for LiteX
> > 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..3877379757cd
> > --- /dev/null
> > +++ b/drivers/mmc/host/litex_mmc.c
> > @@ -0,0 +1,677 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX LiteSDCard driver
> > + *
> > + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
> 
> You should list your co-authors here I think.

Done.

> > +
> > +static int
> > +sdcard_wait_done(void __iomem *reg)
> > +{
> > +       u8 evt;
> > +
> > +       for (;;) {
> > +               evt = litex_read8(reg);
> > +               if (evt & 0x1)
> > +                       break;
> > +               udelay(5);
> 
> This is an infinite loop. Take a look at the commits here and grab the
> fix to make it timeout:
> 
> https://github.com/shenki/linux/commits/microwatt-v5.16
> 
> Well behaving hardware should be ok, but a broken or missing IP block
> will cause the kernel to lock up for ever.

Replaced with readx_poll_timeout().

> > +       }
> > +       if (evt == 0x1)
> > +               return SD_OK;
> > +       if (evt & 0x2)
> > +               return SD_WRITEERROR;
> > +       if (evt & 0x4)
> > +               return SD_TIMEOUT;
> > +       if (evt & 0x8)
> > +               return SD_CRCERROR;
> > +       pr_err("%s: unknown error evt=%x\n", __func__, evt);
> > +       return SD_ERR_OTHER;
> 
> I would consider refactoring the driver to not have it's own error
> codes that map to real ones. You can get rid of map_status too.

Done.

> > +}
> > +
> > +static int
> > +send_cmd(struct litex_mmc_host *host,
> > +        u8 cmd, u32 arg, u8 response_len, u8 transfer)
> > +{
> > +       void __iomem *reg;
> > +       ulong n;
> > +       u8 i;
> > +       int status;
> > +
> > +       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 != SDCARD_CTRL_DATA_TRANSFER_NONE ||
> > +            response_len == SDCARD_CTRL_RESPONSE_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);
> > +       }
> > +
> > +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_CMDEVT);
> > +
> > +       if (status != SD_OK) {
> > +               pr_err("Command (cmd %d) failed, status %d\n", cmd, status);
> 
> Take a look at the patch in my tree that fixes up the error messages
> to have the driver prefix (dev_err/dev_info/etc).

Done, thanks.

> > +               return status;
> > +       }
> > +
> > +       if (response_len != SDCARD_CTRL_RESPONSE_NONE) {
> > +               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 == SDCARD_CTRL_DATA_TRANSFER_NONE)
> > +               return status; /* SD_OK from prior sdcard_wait_done(cmd_evt) */
> > +
> > +       status = sdcard_wait_done(host->sdcore + LITEX_CORE_DATAEVT);
> > +       if (status != SD_OK) {
> > +               pr_err("Data xfer (cmd %d) failed, status %d\n", cmd, status);
> > +               return status;
> > +       }
> > +
> > +       /* wait for completion of (read or write) DMA transfer */
> > +       reg = (transfer == SDCARD_CTRL_DATA_TRANSFER_READ) ?
> > +               host->sdreader + LITEX_BLK2MEM_DONE :
> > +               host->sdwriter + LITEX_MEM2BLK_DONE;
> > +       n = jiffies + (HZ << 1);
> > +       while ((litex_read8(reg) & 0x01) == 0)
> > +               if (time_after(jiffies, n)) {
> 
> Use readx_poll_timeout.

Done.
 
> > +                       pr_err("DMA timeout (cmd %d)\n", cmd);
> > +                       return SD_TIMEOUT;
> > +               }
> > +
> > +       return status;
> > +}
> > +
> > +static inline int
> > +send_app_cmd(struct litex_mmc_host *host)
> > +{
> > +       return send_cmd(host, MMC_APP_CMD, host->rca << 16,
> > +                       SDCARD_CTRL_RESPONSE_SHORT,
> > +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +}
> > +
> > +static inline int
> > +send_app_set_bus_width_cmd(struct litex_mmc_host *host, u32 width)
> > +{
> > +       return send_cmd(host, SD_APP_SET_BUS_WIDTH, width,
> > +                       SDCARD_CTRL_RESPONSE_SHORT,
> > +                       SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +}
> > +
> > +static int
> > +litex_set_bus_width(struct litex_mmc_host *host)
> > +{
> > +       bool app_cmd_sent = host->app_cmd; /* was preceding command app_cmd? */
> > +       int status;
> > +
> > +       /* ensure 'app_cmd' precedes 'app_set_bus_width_cmd' */
> > +       if (!app_cmd_sent)
> > +               send_app_cmd(host);
> > +
> > +       /* litesdcard only supports 4-bit bus width */
> > +       status = send_app_set_bus_width_cmd(host, MMC_BUS_WIDTH_4);
> > +
> > +       /* re-send 'app_cmd' if necessary */
> > +       if (app_cmd_sent)
> > +               send_app_cmd(host);
> > +
> > +       return status;
> > +}
> > +
> > +static int
> > +litex_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 = mmc_gpio_get_cd(mmc);
> 
> Bindings.

This was part of the original Antmicro version of the driver, but I
have never actually used gpio based card detection. I'm inclined to
remove it from this submission entirely (and keep it around as an
out-of-tree fixup patch) until someone with the appropriate hardware
setup can provide a "tested-by" (and preferably an example on how to
configure it in DT).

Thoughts?

> > +       if (ret >= 0)
> > +               /* GPIO based card-detect explicitly specified in DTS */
> > +               ret = !!ret;
> > +       else
> > +               /* use gateware card-detect bit by default */
> > +               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_response_len(struct mmc_command *cmd)
> > +{
> > +       if (cmd->flags & MMC_RSP_136) {
> > +               return SDCARD_CTRL_RESPONSE_LONG;
> > +       } else if (cmd->flags & MMC_RSP_PRESENT) {
> > +               if (cmd->flags & MMC_RSP_BUSY)
> > +                       return SDCARD_CTRL_RESPONSE_SHORT_BUSY;
> > +               else
> > +                       return SDCARD_CTRL_RESPONSE_SHORT;
> > +       }
> > +       return SDCARD_CTRL_RESPONSE_NONE;
> > +}
> > +
> > +static int
> > +litex_map_status(int status)
> > +{
> > +       int error;
> > +
> > +       switch (status) {
> > +       case SD_OK:
> > +               error = 0;
> > +               break;
> > +       case SD_WRITEERROR:
> > +               error = -EIO;
> > +               break;
> > +       case SD_TIMEOUT:
> > +               error = -ETIMEDOUT;
> > +               break;
> > +       case SD_CRCERROR:
> > +               error = -EILSEQ;
> > +               break;
> > +       default:
> > +               error = -EINVAL;
> > +               break;
> > +       }
> > +       return error;
> > +}
> > +
> > +static void
> > +litex_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > +{
> > +       struct litex_mmc_host *host = mmc_priv(mmc);
> > +       struct platform_device *pdev = to_platform_device(mmc->parent);
> > +       struct device *dev = &pdev->dev;
> > +       struct mmc_data *data = mrq->data;
> > +       struct mmc_command *sbc = mrq->sbc;
> > +       struct mmc_command *cmd = mrq->cmd;
> > +       struct mmc_command *stop = mrq->stop;
> > +       unsigned int retries = cmd->retries;
> > +       int status;
> > +       int sg_count;
> > +       enum dma_data_direction dir = DMA_TO_DEVICE;
> > +       bool direct = false;
> > +       dma_addr_t dma;
> > +       unsigned int len = 0;
> > +
> > +       u32 response_len = litex_response_len(cmd);
> > +       u32 transfer = SDCARD_CTRL_DATA_TRANSFER_NONE;
> > +
> > +       /* First check that the card is still there */
> > +       if (!litex_get_cd(mmc)) {
> > +               cmd->error = -ENOMEDIUM;
> > +               mmc_request_done(mmc, mrq);
> > +               return;
> > +       }
> > +
> > +       /* Send set-block-count command if needed */
> > +       if (sbc) {
> > +               status = send_cmd(host, sbc->opcode, sbc->arg,
> > +                                 litex_response_len(sbc),
> > +                                 SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +               sbc->error = litex_map_status(status);
> > +               if (status != SD_OK) {
> > +                       host->is_bus_width_set = false;
> > +                       mmc_request_done(mmc, mrq);
> > +                       return;
> > +               }
> > +       }
> > +
> > +       if (data) {
> 
> This is a big ol' if(). Consider splitting it (or some of it?) out
> into some other functions to make it easier to read.

I'll see what I can do before I submit v3 -- depending on how much
surgery is needed to extricate just the body of that if() from the
rest of the function :)

> > +               /* 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!
> > +                */
> > +               if (!host->is_bus_width_set) {
> > +                       ulong n = jiffies + 2 * HZ; // 500ms timeout
> > +
> > +                       while (litex_set_bus_width(host) != SD_OK) {
> > +                               if (time_after(jiffies, n)) {
> > +                                       dev_warn(dev, "Can't set bus width!\n");
> > +                                       cmd->error = -ETIMEDOUT;
> > +                                       mmc_request_done(mmc, mrq);
> > +                                       return;
> > +                               }
> > +                       }
> > +                       host->is_bus_width_set = true;
> > +               }
> > +
> > +               /* 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;
> > +               if (data->flags & MMC_DATA_READ)
> > +                       dir = DMA_FROM_DEVICE
> > +               sg_count = dma_map_sg(&host->dev->dev,
> > +                                     data->sg, data->sg_len, dir)
> 
> dma_map_sg(..., mmc_get_dma_dir(data))

Nice, and much cleaner -- thanks! And done.

> > +               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 = SDCARD_CTRL_DATA_TRANSFER_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 = SDCARD_CTRL_DATA_TRANSFER_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);
> > +       }
> > +
> > +       do {
> > +               status = send_cmd(host, cmd->opcode, cmd->arg,
> > +                                 response_len, transfer);
> > +       } while (status != SD_OK && retries-- > 0);
> > +
> > +       cmd->error = litex_map_status(status);
> > +       if (status != SD_OK)
> > +               /* card may be gone; don't assume bus width is still set */
> > +               host->is_bus_width_set = false;
> > +
> > +       if (response_len == SDCARD_CTRL_RESPONSE_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 == SDCARD_CTRL_RESPONSE_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)) {
> > +               int stop_stat;
> > +
> > +               stop_stat = send_cmd(host, stop->opcode, stop->arg,
> > +                                    litex_response_len(stop),
> > +                                    SDCARD_CTRL_DATA_TRANSFER_NONE);
> > +               stop->error = litex_map_status(stop_stat);
> > +               if (stop_stat != SD_OK)
> > +                       host->is_bus_width_set = false;
> > +       }
> > +
> > +       if (data)
> > +               dma_unmap_sg(&host->dev->dev, data->sg, data->sg_len, dir);
> > +
> > +       if (status == SD_OK && transfer != SDCARD_CTRL_DATA_TRANSFER_NONE) {
> > +               data->bytes_xfered = min(len, mmc->max_req_size);
> > +               if (transfer == SDCARD_CTRL_DATA_TRANSFER_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_set_clk(struct litex_mmc_host *host, unsigned int clk_freq)
> > +{
> > +       u32 div = clk_freq ? host->freq / clk_freq : 256;
> > +
> > +       div = roundup_pow_of_two(div);
> > +       div = min_t(u32, max_t(u32, div, 2), 256);
> > +       dev_info(&host->dev->dev, "clk_freq=%d: set to %d via div=%d\n",
> > +                clk_freq, host->freq / div, div);
> 
> dev_dbg?

Done.
 
> > +       litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
> > +}
> > +
> > +static void
> > +litex_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 sdcard clock */
> > +       if (ios->clock != host->clock) {
> > +               litex_set_clk(host, ios->clock);
> > +               host->clock = ios->clock;
> > +       }
> > +}
> > +
> > +static const struct mmc_host_ops litex_mmc_ops = {
> > +       .get_cd = litex_get_cd,
> > +       .request = litex_request,
> > +       .set_ios = litex_set_ios,
> > +};
> > +
> > +static int
> > +litex_mmc_probe(struct platform_device *pdev)
> > +{
> > +       struct litex_mmc_host *host;
> > +       struct mmc_host *mmc;
> > +       struct device_node *cpu;
> > +       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;
> > +
> > +       host->clock = 0;
> > +       cpu = of_get_next_cpu_node(NULL);
> > +       ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);
> 
> > +       of_node_put(cpu);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
> > +               goto err_free_host;
> > +       }
> > +
> > +       init_completion(&host->cmd_done);
> > +       host->irq = platform_get_irq(pdev, 0);
> > +       if (host->irq < 0)
> > +               dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
> 
> Can you put all of the irq handling together? Move the hardware sanity
> checking up higher and put it together too:
> 
> litex_write32(host->sdirq + LITEX_IRQ_PENDING, SDIRQ_CARD_DETECT);
>   /* clears it */
> /* ensure DMA bus masters are disabled */
> litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0);
> litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0);
> 
> if (host->irq < 0) {
>                 dev_err(&pdev->dev, "Failed to get IRQ, using polling\n");
>                 mmc->caps |= MMC_CAP_NEEDS_POLL;
> } else {
>               ret = request_irq(host->irq, litex_mmc_interrupt, 0,
> "litex-mmc", mmc);
>               if (ret < 0) {
>                  goto err_free_host;
>                /* enable card-change interrupts */
>                litex_write32(host->sdirq + LITEX_IRQ_ENABLE, SDIRQ_CARD_DETECT);
>  }

I moved it all as close together as possible, but it has to all go
*after* all of the `dev_platform_ioremap_resource[_byname]()` calls,
since those pointers are all used when calling `litex_write*()`.

I'll have it in v3, and you can let me know then if it's OK or still
needs yet more work.

> > +
> > +       /* 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;
> > +
> > +       ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> 
> Is this going to be true on all platforms? How do we handle those
> where it's not true?

I'll need to do a bit of digging here, unless anyone has ideas ready
to go...

> > +       if (ret)
> > +               goto err_free_host;
> > +
> > +       host->buf_size = mmc->max_req_size * 2;
> > +       host->buffer = dma_alloc_coherent(&pdev->dev, host->buf_size,
> > +                                         &host->dma, GFP_DMA);
> 
> dmam_alloc_coherent?

Does this mean I no longer have to `dma[m]_free_coherent()` at [1] and
[2] below, since it's automatically handled by the "managed" bits?

> > +       if (host->buffer == NULL) {
> > +               ret = -ENOMEM;
> > +               goto err_free_host;
> > +       }
> > +
> > +
> > +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +       mmc->ops = &litex_mmc_ops;
> > +
> > +       mmc->f_min = 12.5e6;
> > +       mmc->f_max = 50e6;
> 
> How did you pick these?
> 
> Are these always absolute? (wouldn't they depend on the system they
> are in? host clocks?)

They are the minimum and maximum frequency empirically observed to work
on typical sdcard media with LiteSDCard, in the wild. I can state that
in a comment (after I do another pass of double-checking, crossing Ts
and dotting Is), hope that's what you were looking for.
 
> > +
> > +       ret = mmc_of_parse(mmc);
> > +       if (ret)
> > +               goto err_free_dma;
> > +
> > +       /* 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_free_dma;
> > +
> 
> > +
> > +       return 0;
> > +
> > +err_free_dma:
> > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);

[1] is this made optional by having used `dmam_alloc_coherent()` above?

> > +err_free_host:
> > +       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);
> > +       dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma);

[2] ditto?

Thanks again for all the help and advice!

--Gabriel

> > +       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 <www.antmicro.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.31.1
> >



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux