| Message ID | 20211204204121.2367127-1-gsomlo@gmail.com |
|---|---|
| Headers | show |
| Series | mmc: Add LiteSDCard mmc driver | expand |
Hi Gabriel-- On 12/4/21 12:41, Gabriel Somlo wrote: > +MODULE_DESCRIPTION("LiteX SDCard driver"); > +MODULE_AUTHOR("Antmicro <www.antmicro.com>"); Admittedly it's not documented, but we would prefer to have some contact info in MODULE_AUTHOR(), like an email address or a person's name. <linux/module.h> says: /* * Author(s), use "Name <email>" or just "Name", for multiple * authors use multiple MODULE_AUTHOR() statements/lines. */ #define MODULE_AUTHOR(_author) MODULE_INFO(author, _author) > +MODULE_LICENSE("GPL v2"); Also, it's up to the MMC maintainer (Ulf), but the function signature style that is used in this driver is not the preferred style. E.g.: +static int +litex_set_bus_width(struct litex_mmc_host *host) +{ should be +static int litex_set_bus_width(struct litex_mmc_host *host) +{ thanks.
Hi Randy, Thanks for the feedback! On Sat, Dec 04, 2021 at 12:57:32PM -0800, Randy Dunlap wrote: > Hi Gabriel-- > > On 12/4/21 12:41, Gabriel Somlo wrote: > > +MODULE_DESCRIPTION("LiteX SDCard driver"); > > +MODULE_AUTHOR("Antmicro <www.antmicro.com>"); > > Admittedly it's not documented, but we would prefer to have some contact info > in MODULE_AUTHOR(), like an email address or a person's name. > > <linux/module.h> says: > > /* > * Author(s), use "Name <email>" or just "Name", for multiple > * authors use multiple MODULE_AUTHOR() statements/lines. > */ > #define MODULE_AUTHOR(_author) MODULE_INFO(author, _author) OK, so my plan is to add the following MODULE_AUTHOR() statements: 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>"); Antmicro folks: Kamil, Maciej, Mateusz, Karol: I'm not exactly clear on how you all prefer to be credited but I could either: 1. leave just the first line for Antmicro-the-company, with the "contact@..." email instead of the website URL, or 2. leave just the developer lines (Kamil and Maciej), or 3. leave all three lines as currently shown above. I'll queue that up for v3, once I hear back from you. > > +MODULE_LICENSE("GPL v2"); > > > Also, it's up to the MMC maintainer (Ulf), but the function signature > style that is used in this driver is not the preferred style. > > E.g.: > > +static int > +litex_set_bus_width(struct litex_mmc_host *host) > +{ > > should be > > +static int litex_set_bus_width(struct litex_mmc_host *host) > +{ Done, and also queued up for v3. Thanks again, --Gabriel
Hi Gabriel, On Sat, Dec 4, 2021 at 9:41 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! > > 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> Thanks for your patch! > --- /dev/null > +++ b/drivers/mmc/host/litex_mmc.c > +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; > + > + u32 resp[4]; > + u16 rca; > + > + void *buffer; > + size_t buf_size; > + dma_addr_t dma; > + > + unsigned int freq; > + unsigned int clock; > + bool is_bus_width_set; > + bool app_cmd; > + > + int irq; > + struct completion cmd_done; You may want to reorder the members to avoid implicit gaps (i.e. structs first, followed by integral types in decreasing size). > +}; > + > +static int > +sdcard_wait_done(void __iomem *reg) > +{ > + u8 evt; > + int ret; > + > + ret = read_poll_timeout(litex_read8, evt, (evt & 0x1), Lots of magic numbers. Please use defines, like #define EVT_FOO BIT(0) > + SD_SLEEP_US, SD_TIMEOUT_US, false, reg); > + if (ret || (evt & 0x4)) > + return SD_TIMEOUT; > + if (evt == 0x1) > + return SD_OK; > + if (evt & 0x2) > + return SD_WRITEERROR; > + if (evt & 0x8) > + return SD_CRCERROR; > + pr_err("%s: unknown error evt=%x\n", __func__, evt); > + return SD_ERR_OTHER; > +} > + > +static int > +send_cmd(struct litex_mmc_host *host, > + u8 cmd, u32 arg, u8 response_len, u8 transfer) > +{ > + void __iomem *reg; > + u8 i; unsigned int > + 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); > + 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; > + > + status = read_poll_timeout(litex_read8, i, (i & 0x1), > + SD_SLEEP_US, SD_TIMEOUT_US, false, reg); > + if (status) > + pr_err("DMA timeout (cmd %d)\n", cmd); > + > + return status; > +} > + > +static inline int No need for inline, the compiler can decide. > +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 Likewise. > +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_get_cd(struct mmc_host *mmc) litex_mmc_get_cd()? (i.e. "litex_mmc_"-prefix everywhere) > +{ > + struct litex_mmc_host *host = mmc_priv(mmc); > + int ret; > + > + if (!mmc_card_is_removable(mmc)) > + return 1; > + > + ret = mmc_gpio_get_cd(mmc); > + 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); Please use curly braces to delimit blocks larger than a single line. > + > + /* ensure bus width will be set (again) upon card (re)insertion */ > + if (ret == 0) > + host->is_bus_width_set = false; > + > + return ret; > +} > +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) { No need for else after return. > + if (cmd->flags & MMC_RSP_BUSY) > + return SDCARD_CTRL_RESPONSE_SHORT_BUSY; > + else > + return SDCARD_CTRL_RESPONSE_SHORT; > + } > + return SDCARD_CTRL_RESPONSE_NONE; Perhaps it's worthwhile to invert the logic of the last check, to reduce indentation? if (cmd->flags & MMC_RSP_136) return SDCARD_CTRL_RESPONSE_LONG; if (!(cmd->flags & MMC_RSP_PRESENT)) return SDCARD_CTRL_RESPONSE_NONE; if (cmd->flags & MMC_RSP_BUSY) return SDCARD_CTRL_RESPONSE_SHORT_BUSY; return SDCARD_CTRL_RESPONSE_SHORT; > +} > +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; The above might look nicer when using "reverse Xmas tree" order of declarations. > + > + 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) { > + /* 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); > + 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; Please add curly braces. > + > + 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); No need for the _t-variants if you make the constants unsigned (e.g. 2U). > + dev_info(&host->dev->dev, "sdclk_freq=%d: set to %d via div=%d\n", > + clk_freq, host->freq / div, div); > + litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div); > +} > +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); platform_get_irq_optional() So interrupts should not be required in the DT bindings. > + if (host->irq < 0) > + dev_err(&pdev->dev, "Failed to get IRQ, using polling\n"); This is not an error condition: dev_info() or dev_warn(). > + > + /* 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)); > + 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); > + if (host->buffer == NULL) { > + ret = -ENOMEM; > + goto err_free_host; > + } > + > + host->sdphy = devm_platform_ioremap_resource_byname(pdev, "phy"); So reg-names is required ;-) > + if (IS_ERR(host->sdphy)) { > + ret = PTR_ERR(host->sdphy); > + goto err_free_dma; > + } > + > + host->sdcore = devm_platform_ioremap_resource_byname(pdev, "core"); > + if (IS_ERR(host->sdcore)) { > + ret = PTR_ERR(host->sdcore); > + goto err_free_dma; > + } > + > + host->sdreader = devm_platform_ioremap_resource_byname(pdev, "reader"); > + if (IS_ERR(host->sdreader)) { > + ret = PTR_ERR(host->sdreader); > + goto err_free_dma; > + } > + > + host->sdwriter = devm_platform_ioremap_resource_byname(pdev, "writer"); > + if (IS_ERR(host->sdwriter)) { > + ret = PTR_ERR(host->sdwriter); > + goto err_free_dma; > + } > + > + if (host->irq > 0) { > + host->sdirq = devm_platform_ioremap_resource_byname(pdev, "irq"); So you need a "minItems: 4" for reg{,-names} in the DT bindings. > + if (IS_ERR(host->sdirq)) { > + ret = PTR_ERR(host->sdirq); > + goto err_free_dma; > + } > + } > + > + 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; > + > + 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; > + > + /* ensure DMA bus masters are disabled */ > + litex_write8(host->sdreader + LITEX_BLK2MEM_ENA, 0); > + litex_write8(host->sdwriter + LITEX_MEM2BLK_ENA, 0); > + > + /* set up interrupt handler */ > + if (host->irq > 0) { > + ret = request_irq(host->irq, litex_mmc_interrupt, 0, > + "litex-mmc", mmc); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "irq setup error %d, using polling\n", ret); > + host->irq = 0; > + } > + } > + > + /* enable card-change interrupts, or else ask for polling */ > + if (host->irq > 0) { > + litex_write32(host->sdirq + LITEX_IRQ_PENDING, > + SDIRQ_CARD_DETECT); /* clears it */ > + litex_write32(host->sdirq + LITEX_IRQ_ENABLE, > + SDIRQ_CARD_DETECT); > + } else { > + mmc->caps |= MMC_CAP_NEEDS_POLL; > + } > + > + return 0; > + > +err_free_dma: > + dma_free_coherent(&pdev->dev, host->buf_size, host->buffer, host->dma); > +err_free_host: > + mmc_free_host(mmc); > + return ret; > +} Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Sat, 4 Dec 2021 at 20:41, Gabriel Somlo <gsomlo@gmail.com> wrote: > > Add the litex_mmc (LiteSDCard) driver to the list of files maintained > under LiteX, and add myself as co-maintainer. I've helped develop some > of the existing drivers, and am currently curating the out-of-tree > drivers as they are tested and prepared for upstream submission. > > Cc: Karol Gugala <kgugala@antmicro.com> > Cc: Mateusz Holenko <mholenko@antmicro.com> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> Acked-by: Joel Stanley <joel@jms.id.au> If this is going to be a catch all for the drivers as well as the platform, we're probably missing a few entries: $ git grep -l litex Documentation/admin-guide/kernel-parameters.txt Documentation/devicetree/bindings/net/litex,liteeth.yaml Documentation/devicetree/bindings/serial/litex,liteuart.yaml Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml Documentation/devicetree/bindings/vendor-prefixes.yaml Documentation/sphinx/kfigure.py MAINTAINERS arch/openrisc/boot/dts/or1klitex.dts arch/openrisc/configs/or1klitex_defconfig arch/powerpc/boot/dts/microwatt.dts drivers/net/ethernet/Kconfig drivers/net/ethernet/Makefile drivers/net/ethernet/litex/Makefile drivers/net/ethernet/litex/litex_liteeth.c drivers/soc/Kconfig drivers/soc/Makefile drivers/soc/litex/Kconfig drivers/soc/litex/Makefile drivers/soc/litex/litex_soc_ctrl.c drivers/tty/serial/liteuart.c include/linux/litex.h scripts/clang-tools/gen_compile_commands.py I think we could add these entries to maintainers: +F: drivers/soc/litex/* +F: drivers/net/ethernet/litex/* +N: litex I would also add my name there. > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index faa9c34d837d..5fc65d4c4969 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11012,12 +11012,14 @@ F: lib/list-test.c > LITEX PLATFORM > M: Karol Gugala <kgugala@antmicro.com> > M: Mateusz Holenko <mholenko@antmicro.com> > +M: Gabriel Somlo <gsomlo@gmail.com> > S: Maintained > F: Documentation/devicetree/bindings/*/litex,*.yaml > F: arch/openrisc/boot/dts/or1klitex.dts > F: drivers/soc/litex/litex_soc_ctrl.c > F: drivers/tty/serial/liteuart.c > F: include/linux/litex.h > +F: drivers/mmc/host/litex_mmc.c > > LIVE PATCHING > M: Josh Poimboeuf <jpoimboe@redhat.com> > -- > 2.31.1 >
On Mon, Dec 06, 2021 at 11:59:47PM +0000, Joel Stanley wrote: > On Sat, 4 Dec 2021 at 20:41, Gabriel Somlo <gsomlo@gmail.com> wrote: > > > > Add the litex_mmc (LiteSDCard) driver to the list of files maintained > > under LiteX, and add myself as co-maintainer. I've helped develop some > > of the existing drivers, and am currently curating the out-of-tree > > drivers as they are tested and prepared for upstream submission. > > > > Cc: Karol Gugala <kgugala@antmicro.com> > > Cc: Mateusz Holenko <mholenko@antmicro.com> > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > Acked-by: Joel Stanley <joel@jms.id.au> > > If this is going to be a catch all for the drivers as well as the > platform, we're probably missing a few entries: > > $ git grep -l litex > Documentation/admin-guide/kernel-parameters.txt > Documentation/devicetree/bindings/net/litex,liteeth.yaml > Documentation/devicetree/bindings/serial/litex,liteuart.yaml > Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml > Documentation/devicetree/bindings/vendor-prefixes.yaml > Documentation/sphinx/kfigure.py > MAINTAINERS > arch/openrisc/boot/dts/or1klitex.dts > arch/openrisc/configs/or1klitex_defconfig > arch/powerpc/boot/dts/microwatt.dts > drivers/net/ethernet/Kconfig > drivers/net/ethernet/Makefile > drivers/net/ethernet/litex/Makefile > drivers/net/ethernet/litex/litex_liteeth.c > drivers/soc/Kconfig > drivers/soc/Makefile > drivers/soc/litex/Kconfig > drivers/soc/litex/Makefile > drivers/soc/litex/litex_soc_ctrl.c > drivers/tty/serial/liteuart.c > include/linux/litex.h > scripts/clang-tools/gen_compile_commands.py > > I think we could add these entries to maintainers: > > +F: drivers/soc/litex/* > +F: drivers/net/ethernet/litex/* > +N: litex Done. > I would also add my name there. Added you as proposed co-maintainer, will publish soon as part of [PATCH v3] series. Thanks much, --Gabriel > > --- > > MAINTAINERS | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index faa9c34d837d..5fc65d4c4969 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -11012,12 +11012,14 @@ F: lib/list-test.c > > LITEX PLATFORM > > M: Karol Gugala <kgugala@antmicro.com> > > M: Mateusz Holenko <mholenko@antmicro.com> > > +M: Gabriel Somlo <gsomlo@gmail.com> > > S: Maintained > > F: Documentation/devicetree/bindings/*/litex,*.yaml > > F: arch/openrisc/boot/dts/or1klitex.dts > > F: drivers/soc/litex/litex_soc_ctrl.c > > F: drivers/tty/serial/liteuart.c > > F: include/linux/litex.h > > +F: drivers/mmc/host/litex_mmc.c > > > > LIVE PATCHING > > M: Josh Poimboeuf <jpoimboe@redhat.com> > > -- > > 2.31.1 > >
On Thu, Dec 09, 2021 at 09:31:49AM +0100, Geert Uytterhoeven wrote: > Hi Gabriel, > > On Wed, Dec 8, 2021 at 9:14 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > I did *some* of this for v3, but since figured out how to use `pahole` :) > > Right, pahole. > > > On Mon, Dec 06, 2021 at 11:07:56AM +0100, Geert Uytterhoeven wrote: > > > > +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; > > > > + > > > > + u32 resp[4]; > > > > + u16 rca; > > > > + > > > > + void *buffer; > > > > + size_t buf_size; > > > > + dma_addr_t dma; > > > > + > > > > + unsigned int freq; > > > > + unsigned int clock; > > > > + bool is_bus_width_set; > > > > + bool app_cmd; > > > > + > > > > + int irq; > > > > + struct completion cmd_done; > > > > > > You may want to reorder the members to avoid implicit gaps > > > (i.e. structs first, followed by integral types in decreasing size). > > > > So, for v4, I'll have it looking like this, which `pahole` says is > > optimally packed: > > > > struct litex_mmc_host { > > struct mmc_host * mmc; /* 0 8 */ > > struct platform_device * dev; /* 8 8 */ > > void * sdphy; /* 16 8 */ > > void * sdcore; /* 24 8 */ > > void * sdreader; /* 32 8 */ > > void * sdwriter; /* 40 8 */ > > void * sdirq; /* 48 8 */ > > void * buffer; /* 56 8 */ > > /* --- cacheline 1 boundary (64 bytes) --- */ > > size_t buf_size; /* 64 8 */ > > size_t is 32-bit on RV32, so you may want to move it below cmd_done. > > > dma_addr_t dma; /* 72 8 */ > > struct completion cmd_done; /* 80 32 */ > > int irq; /* 112 4 */ > > unsigned int ref_clk; /* 116 4 */ > > unsigned int sd_clk; /* 120 4 */ > > u32 resp[4]; /* 124 16 */ > > /* --- cacheline 2 boundary (128 bytes) was 12 bytes ago --- */ > > u16 rca; /* 140 2 */ > > bool is_bus_width_set; /* 142 1 */ > > bool app_cmd; /* 143 1 */ > > > > /* size: 144, cachelines: 3, members: 18 */ > > /* last cacheline: 16 bytes */ > > }; After a bit of a fight, I managed to wrestle `pahole` to display useful information for 32-bit (rv32imac) builds: struct litex_mmc_host { struct mmc_host * mmc; /* 0 4 */ struct platform_device * dev; /* 4 4 */ void * sdphy; /* 8 4 */ void * sdcore; /* 12 4 */ void * sdreader; /* 16 4 */ void * sdwriter; /* 20 4 */ void * sdirq; /* 24 4 */ void * buffer; /* 28 4 */ size_t buf_size; /* 32 4 */ dma_addr_t dma; /* 36 4 */ struct completion cmd_done; /* 40 16 */ int irq; /* 56 4 */ unsigned int ref_clk; /* 60 4 */ /* --- cacheline 1 boundary (64 bytes) --- */ unsigned int sd_clk; /* 64 4 */ u32 resp[4]; /* 68 16 */ u16 rca; /* 84 2 */ bool is_bus_width_set; /* 86 1 */ bool app_cmd; /* 87 1 */ /* size: 88, cachelines: 2, members: 18 */ /* last cacheline: 24 bytes */ }; Looks like even with `size_t buf_size` where it is right now, there still are no holes. I like it where it is, as it's related to the field immediately preceding it (`buffer`). I'd rather not move it, particularly since we're not actually eliminating any additional holes. What do you think (i.e., is there a configuration where there's still a chance we may run into trouble)? Thanks, --Gabriel
Hi Gabriel, On Thu, Dec 9, 2021 at 9:58 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > On Thu, Dec 09, 2021 at 09:31:49AM +0100, Geert Uytterhoeven wrote: > > On Wed, Dec 8, 2021 at 9:14 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > I did *some* of this for v3, but since figured out how to use `pahole` :) > > > > Right, pahole. > > > > > On Mon, Dec 06, 2021 at 11:07:56AM +0100, Geert Uytterhoeven wrote: > > > > > +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; > > > > > + > > > > > + u32 resp[4]; > > > > > + u16 rca; > > > > > + > > > > > + void *buffer; > > > > > + size_t buf_size; > > > > > + dma_addr_t dma; > > > > > + > > > > > + unsigned int freq; > > > > > + unsigned int clock; > > > > > + bool is_bus_width_set; > > > > > + bool app_cmd; > > > > > + > > > > > + int irq; > > > > > + struct completion cmd_done; > > > > > > > > You may want to reorder the members to avoid implicit gaps > > > > (i.e. structs first, followed by integral types in decreasing size). > > > > > > So, for v4, I'll have it looking like this, which `pahole` says is > > > optimally packed: > > > > > > struct litex_mmc_host { > > > struct mmc_host * mmc; /* 0 8 */ > > > struct platform_device * dev; /* 8 8 */ > > > void * sdphy; /* 16 8 */ > > > void * sdcore; /* 24 8 */ > > > void * sdreader; /* 32 8 */ > > > void * sdwriter; /* 40 8 */ > > > void * sdirq; /* 48 8 */ > > > void * buffer; /* 56 8 */ > > > /* --- cacheline 1 boundary (64 bytes) --- */ > > > size_t buf_size; /* 64 8 */ > > > > size_t is 32-bit on RV32, so you may want to move it below cmd_done. > > > > > dma_addr_t dma; /* 72 8 */ > > > struct completion cmd_done; /* 80 32 */ > > > int irq; /* 112 4 */ > > > unsigned int ref_clk; /* 116 4 */ > > > unsigned int sd_clk; /* 120 4 */ > > > u32 resp[4]; /* 124 16 */ > > > /* --- cacheline 2 boundary (128 bytes) was 12 bytes ago --- */ > > > u16 rca; /* 140 2 */ > > > bool is_bus_width_set; /* 142 1 */ > > > bool app_cmd; /* 143 1 */ > > > > > > /* size: 144, cachelines: 3, members: 18 */ > > > /* last cacheline: 16 bytes */ > > > }; > > After a bit of a fight, I managed to wrestle `pahole` to display useful > information for 32-bit (rv32imac) builds: > > struct litex_mmc_host { > struct mmc_host * mmc; /* 0 4 */ > struct platform_device * dev; /* 4 4 */ > void * sdphy; /* 8 4 */ > void * sdcore; /* 12 4 */ > void * sdreader; /* 16 4 */ > void * sdwriter; /* 20 4 */ > void * sdirq; /* 24 4 */ > void * buffer; /* 28 4 */ > size_t buf_size; /* 32 4 */ > dma_addr_t dma; /* 36 4 */ > struct completion cmd_done; /* 40 16 */ > int irq; /* 56 4 */ > unsigned int ref_clk; /* 60 4 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > unsigned int sd_clk; /* 64 4 */ > u32 resp[4]; /* 68 16 */ > u16 rca; /* 84 2 */ > bool is_bus_width_set; /* 86 1 */ > bool app_cmd; /* 87 1 */ > > /* size: 88, cachelines: 2, members: 18 */ > /* last cacheline: 24 bytes */ > }; > > Looks like even with `size_t buf_size` where it is right now, there > still are no holes. I like it where it is, as it's related to the Right, dma_addr_t is 32-bit, too. I'm just too used to LPAE on ARM ;-) > field immediately preceding it (`buffer`). I'd rather not move it, > particularly since we're not actually eliminating any additional > holes. Thanks, LGTM. > What do you think (i.e., is there a configuration where there's still > a chance we may run into trouble)? ICONFIG_PHYS_ADDR_T_64BIT=y/CONFIG_ARCH_DMA_ADDR_T_64BIT=y on 32-bit, but that doesn't seem to be supported under arch/risc/ yet. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Add support for the LiteX SD-Card device, LiteSDCard. LiteSDCard is a simple SD-Card interface available as part of the LiteX environment, used with various RISC-V and other FPGA based SoCs. New in v2: - reword info message in litex_set_clk() - streamline code in litex_map_status() - fix typos in Kconfig (thanks Randy Dunlap <rdunlap@infradead.org>) - improvements suggested by Stafford Horne <shorne@gmail.com> - allow COMPILE_TEST in Kconfig - use read_poll_timeout() when waiting for cmd/data/DMA xfer completion - include interrupt.h (thanks kernel test robot <lkp@intel.com>) Gabriel Somlo (3): MAINTAINERS: co-maintain LiteX platform dt-bindings: mmc: Add bindings for LiteSDCard mmc: Add driver for LiteX's LiteSDCard interface .../devicetree/bindings/mmc/litex,mmc.yaml | 63 ++ MAINTAINERS | 2 + drivers/mmc/host/Kconfig | 7 + drivers/mmc/host/Makefile | 1 + drivers/mmc/host/litex_mmc.c | 670 ++++++++++++++++++ 5 files changed, 743 insertions(+) create mode 100644 Documentation/devicetree/bindings/mmc/litex,mmc.yaml create mode 100644 drivers/mmc/host/litex_mmc.c