Re: [PATCH v1 5/9] mmc: sdhci: add Black Sesame Technologies BST C1200 controller driver

[Adrian Hunter <adrian.hunter@intel.com>]

On 27/06/2025 13:22, yangzh0906@thundersoft.com wrote:
> Dear Mr. Hunter,
> 
> Our platform supports 64-bit physical addressing, but the eMMC controller's SRAM-based DMA engine is constrained to a 32-bit address space. 
> When using the standard SDHCI interface, which allocates DDR-based DMA buffers with 64-bit addresses, thedma_map_single() operation fails 
> because the DMA engine cannot handle addresses beyond 32 bits.

SDHCI controllers can use 32-bit DMA or 64-bit DMA, however even with
64-bit DMA it is possible to restrict the DMA addresses to 32-bits
by setting a 32-bit DMA mask.

If the host controller capabilities indicate support for 64-bit DMA
but you want the driver to use 32-bit DMA, set SDHCI_QUIRK2_BROKEN_64_BIT_DMA.

However, if you want to use 64-bit DMA with only 32-bit DMA addresses 
you can instead implement sdhci host op ->set_dma_mask() and in that
function dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32))

> 
> To resolve this hardware limitation, we implement a bounce buffer allocated via dma_alloc_coherent() to satisfy DMA addressing constraints.

The bounce buffer should not be needed to satisfy DMA addressing
constraints.  It is used when SDHCI ADMA (scatter/gather) is broken.

Also please be aware that "top-posting" is discouraged, refer:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> Best regards,
>> Albert
>>
>      
>     *From:* Adrian Hunter <mailto:adrian.hunter@intel.com>
>     *Date:* 2025-06-16 19:19
>     *To:* Albert Yang <mailto:yangzh0906@thundersoft.com>; Ulf Hansson <mailto:ulf.hansson@linaro.org>; Ge Gordon <mailto:gordon.ge@bst.ai>
>     *CC:* BST Linux Kernel Upstream Group <mailto:bst-upstream@bstai.top>; linux-mmc@vger.kernel.org <mailto:linux-mmc@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <mailto:linux-arm-kernel@lists.infradead.org>; linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>; Geert Uytterhoeven <mailto:geert+renesas@glider.be>; Victor Shih <mailto:victor.shih@genesyslogic.com.tw>; Shan-Chun Hung <mailto:shanchun1218@gmail.com>; Arnd Bergmann <mailto:arnd@arndb.de>; AngeloGioacchino Del Regno <mailto:angelogioacchino.delregno@collabora.com>; Peter Robinson <mailto:pbrobinson@gmail.com>; Ben Chuang <mailto:ben.chuang@genesyslogic.com.tw>
>     *Subject:* Re: [PATCH v1 5/9] mmc: sdhci: add Black Sesame Technologies BST C1200 controller driver
>     On 28/05/2025 11:54, Albert Yang wrote:
>     > Add a driver for the DesignWare Mobile Storage Host Controller (DWCMSHC)
>     > SDHCI controller found in Black Sesame Technologies C1200 SoCs.
>     >
>     > The driver provides specialized clock configuration, tuning, voltage
>     > switching, and power management for the BST DWCMSHC controller. It also
>     > includes support for eMMC boot and memory-mapped I/O for CRM registers.
>     >
>     > Signed-off-by: Ge Gordon <gordon.ge@bst.ai>
>     > Signed-off-by: Albert Yang <yangzh0906@thundersoft.com>
>     > ---
>     >  drivers/mmc/host/Kconfig              |  11 +
>     >  drivers/mmc/host/Makefile             |   1 +
>     >  drivers/mmc/host/sdhci-of-bst-c1200.c | 920 ++++++++++++++++++++++++++
>     >  3 files changed, 932 insertions(+)
>     >  create mode 100644 drivers/mmc/host/sdhci-of-bst-c1200.c
>      
>     <SNIP>
>      
>     > +static void bst_sdhci_allocate_bounce_buffer(struct sdhci_host *host)
>     > +{
>     > + struct mmc_host *mmc = host->mmc;
>     > + unsigned int max_blocks;
>     > + unsigned int bounce_size;
>     > + int ret;
>     > +
>     > + /*
>     > + * Cap the bounce buffer at 64KB. Using a bigger bounce buffer
>     > + * has diminishing returns, this is probably because SD/MMC
>     > + * cards are usually optimized to handle this size of requests.
>     > + */
>     > + bounce_size = SZ_32K;
>     > + /*
>     > + * Adjust downwards to maximum request size if this is less
>     > + * than our segment size, else hammer down the maximum
>     > + * request size to the maximum buffer size.
>     > + */
>     > + if (mmc->max_req_size < bounce_size)
>     > + bounce_size = mmc->max_req_size;
>     > + max_blocks = bounce_size / 512;
>     > +
>     > + ret = of_reserved_mem_device_init_by_idx(mmc_dev(mmc), mmc_dev(mmc)->of_node, 0);
>     > + if (ret) {
>     > + dev_err(mmc_dev(mmc), "of_reserved_mem_device_init error\n");
>     > + return;
>     > + }
>     > + host->bounce_buffer = dma_alloc_coherent(mmc_dev(mmc), bounce_size,
>     > + &host->bounce_addr, GFP_KERNEL);
>      
>     sdhci uses dma_sync_single_for_device() and dma_sync_single_for_cpu()
>     with this buffer.  Does that really work?
>      
>     > +
>     > + ret = dma_mapping_error(mmc_dev(mmc), host->bounce_addr);
>     > + if (ret) {
>     > + devm_kfree(mmc_dev(mmc), host->bounce_buffer);
>     > + host->bounce_buffer = NULL;
>     > + /* Again fall back to max_segs == 1 */
>     > + return;
>     > + }
>     > +
>     > + host->bounce_buffer_size = bounce_size;
>     > +
>     > + /* Lie about this since we're bouncing */
>     > + mmc->max_segs = max_blocks;
>     > + mmc->max_seg_size = bounce_size;
>     > + mmc->max_req_size = bounce_size;
>     > +
>     > + pr_info("BST reallocate %s bounce up to %u segments into one, max segment size %u bytes\n",
>     > + mmc_hostname(mmc), max_blocks, bounce_size);
>     > +}
>     > +
>     > +static int bst_sdhci_set_dma_mask(struct sdhci_host *host)
>      
>     This is identical to sdhci_set_dma_mask() just just drop it.
>      
>     > +{
>     > + struct mmc_host *mmc = host->mmc;
>     > + struct device *dev = mmc_dev(mmc);
>     > + int ret = -EINVAL;
>     > +
>     > + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
>     > + host->flags &= ~SDHCI_USE_64_BIT_DMA;
>     > +
>     > + /* Try 64-bit mask if hardware is capable  of it */
>     > + if (host->flags & SDHCI_USE_64_BIT_DMA) {
>     > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>     > + if (ret) {
>     > + pr_warn("%s: Failed to set 64-bit DMA mask.\n",
>     > + mmc_hostname(mmc));
>     > + host->flags &= ~SDHCI_USE_64_BIT_DMA;
>     > + }
>     > + }
>     > +
>     > + /* 32-bit mask as default & fallback */
>     > + if (ret) {
>     > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>     > + if (ret)
>     > + pr_warn("%s: Failed to set 32-bit DMA mask.\n",
>     > + mmc_hostname(mmc));
>     > + }
>     > +
>     > + return ret;
>     > +}
>     > +
>     > +int bst_sdhci_setup_host(struct sdhci_host *host)
>      
>     It is not acceptable for the driver to have its own copy of
>     sdhci_setup().
>      
>     Please describe what you need to customize and why.
>      
>      
>      
>