Hi Albert,

On Wed, 28 May 2025 at 10:55, Albert Yang [off-list ref] 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 <redacted>

Thanks for your patch!

depends on ARCH_BST || COMPILE_TEST

+       depends on MMC_SDHCI_PLTFM
+       depends on OF
+       depends on COMMON_CLK
+       help
+         This selects Synopsys DesignWare Cores Mobile Storage Controller
+         support.
+         If you have a controller with this interface, say Y or M here.
+         If unsure, say N.

+#define SDEMMC_CRM_BCLK_DIV_CTRL       0x08
+#define SDEMMC_CRM_RX_CLK_CTRL         0x14
+#define SDEMMC_CRM_TIMER_DIV_CTRL      0x0C
+#define SDEMMC_CRM_VOL_CTRL            0x1C
+#define DRIVER_NAME                    "sdhci_bst"
+#define BST_VOL_STABLE_ON              (0x1 << 7)
+#define SDHCI_DUMP_BST(f, x...)                \
+       pr_info("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ##x)
+#define SD_3_3V                                0
+#define SD_1_8V                                1
+#define default_max_freq               200000ul
+
+void sdhci_bst_print_vendor(struct sdhci_host *host)
+{
+       SDHCI_DUMP_BST("============ SDHCI VENDOR REGISTER DUMP ===========\n");
+
+       SDHCI_DUMP_BST("VER_ID:  0x%08x | VER_TPYE:  0x%08x\n",
+                      sdhci_readl(host, SDHC_MHSC_VER_ID_R),
+                      sdhci_readl(host, SDHC_MHSC_VER_TPYE_R));
+       SDHCI_DUMP_BST("MHSC_CTRL:  0x%08x | MBIU_CTRL:  0x%08x\n",
+                      sdhci_readw(host, SDHC_MHSC_CTRL_R),
+                      sdhci_readw(host, SDHC_MBIU_CTRL_R));
+       SDHCI_DUMP_BST("EMMC_CTRL:  0x%08x | BOOT_CTRL: 0x%08x\n",
+                      sdhci_readl(host, SDHC_EMMC_CTRL_R),
+                      sdhci_readw(host, SDHC_BOOT_CTRL_R));
+       SDHCI_DUMP_BST("GP_IN:   0x%08x | GP_OUT: 0x%08x\n",
+                      sdhci_readl(host, SDHC_GP_IN_R),
+                      sdhci_readb(host, SDHC_GP_OUT_R));
+       SDHCI_DUMP_BST("AT_CTRL:     0x%08x | AT_STAT:  0x%08x\n",
+                      sdhci_readb(host, SDHC_AT_CTRL_R),
+                      sdhci_readb(host, SDHC_AT_STAT_R));
+}
+EXPORT_SYMBOL_GPL(sdhci_bst_print_vendor);

Why do you need this?

+
+static u32 bst_read_phys_bst(u32 phys_addr)
+{
+       u32 phys_addr_page = phys_addr & 0xFFFFE000;
+       u32 phys_offset = phys_addr & 0x00001FFF;
+       u32 map_size = phys_offset + sizeof(u32);
+       u32 ret = 0xDEADBEEF;
+       void *mem_mapped = ioremap(phys_addr_page, map_size);
+
+       if (mem_mapped) {
+               ret = (u32)ioread32(((u8 *)mem_mapped) + phys_offset);
+               iounmap(mem_mapped);
+       }

Please do not ioremap()/iounmap() for each register read...

+
+       return ret;
+}
+
+static void bst_write_phys_bst(u32 phys_addr, u32 value)
+{
+       u32 phys_addr_page = phys_addr & 0xFFFFE000;
+       u32 phys_offset = phys_addr & 0x00001FFF;
+       u32 map_size = phys_offset + sizeof(u32);
+       void *mem_mapped = ioremap(phys_addr_page, map_size);
+
+       if (mem_mapped) {
+               iowrite32(value, ((u8 *)mem_mapped) + phys_offset);
+               iounmap(mem_mapped);
+       }

or write...

+}

Aborting review.

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