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

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Jul 2, 2025, at 11:44, Albert Yang wrote:

> +
> +config MMC_SDHCI_BST
> +	tristate "SDHCI OF support for the BST DWC MSHC"
> +	depends on ARCH_BST || COMPILE_TEST
> +	depends on MMC_SDHCI_PLTFM
> +	depends on OF
> +	help
> +	  This selects Synopsys DesignWare Cores Mobile Storage Controller
> +	  support.

The description does not mention the actual device it's for
but only DesignWare.

Try to keep this sorted alphabetically between the other
CONFIG_MMC_SDHCI_* backends

> +
> +struct dwcmshc_priv {
> +	void __iomem *crm_reg_base;
> +	u32 phy_crm_reg_base;
> +	u32 phy_crm_reg_size;
> +};

You are only using the first member here, the phy_crm_reg_base
and phy_crm_reg_size are assigned during probe but not referenced
later.  devm_platform_ioremap_resource() should help simplify
that code further.

> +
> +static void bst_write_phys_bst(void __iomem *addr, u32 value)
> +{
> +	iowrite32(value, addr);
> +}

You always pass priv->crm_reg_base into this helper, so
it would be simpler to make it take the sdhci_pltfm_host
pointer and the offset instead of the address.

> +static int bst_sdhci_reallocate_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;

The comment says 64K, but the size you use is 32K.


> +	/* Get CRM registers from the second reg entry */
> +	crm_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

devm_platform_ioremap_resource()

> +	/*
> +	 * Hardware limitation workaround:
> +	 *
> +	 * 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, the
> +	 * dma_map_single() operation fails because the DMA engine cannot
> +	 * handle addresses beyond 32 bits.
> +	 *
> +	 * To resolve this hardware limitation, we implement a bounce buffer
> +	 * allocated via dma_alloc_coherent() to satisfy DMA addressing
> +	 * constraints.
> +	 */
> +	err = bst_sdhci_reallocate_bounce_buffer(host);

Having an explanation here makes sense, but I don't think this
captures what is actually going on, in particular:

- dma_alloc_coherent() being backed by an SRAM that is under
  the 4GB boundary
- the problem that the SoC is configured that all of DRAM
  is outside of ZONE_DMA32
- The type of hardware bug that leads to 64-bit DMA being
  broken in this SoC.

I still have some hope that the hardware is not actually
that broken and you can get it working normally, in one
of these ways:
- enabling 64-bit addressing in the parent bus
- enabling SMMU translation for the parent bus
- configuring the parent bus or the sdhci itself to
  access the first 4GB of RAM, and describing the
  offset in dma-ranges
- moving the start of RAM in a global SoC config

It is rather unlikely that the SoC designer chose to
integrate a 32-bit-only device without adding some
way to configure it to access RAM.

      Arnd